Skip to content

Commit 3fd8b98

Browse files
Wirasmcoderabbitai[bot]CodeRabbit
authored
fix(workflows): stop a declared output_format from silencing an unparseable output (#2460)
* fix(workflows): stop a declared output_format from silencing an unparseable output resolveNodeOutputField treated "no parseable object at all" as a declared-optional field and returned empty, but only on the declared-schema path — the schemaless path threw. So declaring output_format made a broken producer QUIETER than declaring nothing, which is backwards. It bites hardest on workflow: sub-run nodes. Their output_format is never validated against the child; executeWorkflowNode uses it for one thing, deriving declaredFields. A child that returns prose instead of JSON therefore turned every declared field into '' with no error, no warning and no failing test, while the same child under a node with no output_format failed loudly. Now both paths throw 'unparseable'. The leniency that was actually intended survives untouched: a declared field missing from a payload that genuinely parsed still resolves to '' — this only changes the case where there is no object to read. AI nodes are unaffected: when structured output validates, nodeOutputText is overwritten with the serialized JSON, so a completed AI node persists JSON and the resume path (which rehydrates text only) still parses it. This fixes the inversion, not the absence of validation — a child emitting JSON that does not match the declared schema still passes. That half stays open. * fix: apply CodeRabbit auto-fixes Fixed 1 file(s) based on 1 unresolved review comment. Co-authored-by: CodeRabbit <noreply@coderabbit.ai> * docs(workflows): correct when an undeclared output field surfaces The variables reference said an undeclared $node.output.field was a 'load-visible mistake'. The loader never validates field names against output_format — loader.ts has no reference to output_format, declaredFields, or not-in-schema. The 'not-in-schema' error is raised by resolveNodeOutputField at execution and fails the consuming node, like every other OutputRefError reason. The mechanism described was right; only the timing was wrong. Authors reading this page need to know they find out mid-run, not at load. * fix(workflows): tell a clipped output apart from a producer that emitted no JSON The unparseable error told the author to 'Emit JSON containing <field>'. For output clipped at the persisted-event cap that advice is wrong — the node already did, and only a resumed run sees the clipped copy. output_format sits on dagNodeBaseSchema, so a bash node can declare one, and bash stdout is exactly what formatPersistedBashOutput clips at 32 KiB. In-run the full stdout is returned and parses; getDagResumeSnapshot rehydrates the clipped copy, which does not. Before #2456 that resolved to '' on the declared path, so nobody saw it. Now it fails, and it should fail with advice that points somewhere real. Adds an OutputRefError reason 'truncated', chosen at both throw sites so the declared and schemaless paths stay symmetric — the property this PR exists to establish. The marker moves to utils/output-truncation.ts so the writer (dag-executor) and the reader (output-ref) cannot drift; the string is byte-identical and nothing about what gets persisted changes. The detector is anchored to end-of-string, so output that merely quotes the phrase is still reported as a plain producer error. --------- Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Co-authored-by: CodeRabbit <noreply@coderabbit.ai>
1 parent 87e559c commit 3fd8b98

5 files changed

Lines changed: 176 additions & 5 deletions

File tree

packages/docs-web/src/content/docs/reference/variables.md

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -62,7 +62,9 @@ In DAG workflows, nodes can reference the output of any completed upstream node.
6262
| Pattern | Resolves to | Notes |
6363
|---------|-------------|-------|
6464
| `$nodeId.output` | Full output string of the referenced node | The node must be a declared dependency (in `depends_on`) |
65-
| `$nodeId.output.field` | A specific JSON field from the node's output | Requires the upstream node to use `output_format` for structured JSON |
65+
| `$nodeId.output.field` | A specific JSON field from the node's output | Works on any JSON-object output; `output_format` adds stricter validation — see notes below |
66+
67+
A `.field` reference **fails the consuming node** when the producer's output is not a JSON object — whether or not the producer declared an `output_format`. Declaring a schema buys you a stricter check on the field *name* (an undeclared field fails the consuming node with a named error rather than resolving to a silent empty), and lets a declared-but-absent field resolve to `''`; it never makes a broken producer quieter. This matters most for `workflow:` sub-run nodes, where `output_format` populates the accessible field names but is **not** validated against what the child actually returns.
6668

6769
During the current run, downstream interpolation and `when:` conditions see the full returned node output. Successful bash events retain only a 32 KiB UTF-8 audit preview, so after a process boundary a resumed run rehydrates that persisted preview rather than the full output. If a large gate verdict must survive a restart intact, store it through a deliberately managed artifact contract instead of relying on the event preview.
6870

packages/workflows/src/dag-executor.ts

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -78,6 +78,7 @@ import {
7878
OutputRefError,
7979
similarNodeIds,
8080
} from './output-ref';
81+
import { buildTruncationMarker } from './utils/output-truncation';
8182
import { writeNodeArtifact, readNodeArtifacts } from './artifacts-index';
8283
import {
8384
logNodeStart,
@@ -2522,7 +2523,7 @@ function formatPersistedBashOutput(output: string): {
25222523
return { nodeOutput: output, truncated: false };
25232524
}
25242525

2525-
const marker = `\n\n… [truncated; original output was ${String(outputBytes.byteLength)} bytes]`;
2526+
const marker = buildTruncationMarker(outputBytes.byteLength);
25262527
const markerBytes = Buffer.byteLength(marker, 'utf8');
25272528
let headEnd = PERSISTED_BASH_OUTPUT_MAX_BYTES - markerBytes;
25282529

packages/workflows/src/output-ref.test.ts

Lines changed: 101 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@ import {
66
resolveNodeOutputField,
77
similarNodeIds,
88
} from './output-ref';
9+
import { buildTruncationMarker, hasTruncationMarker } from './utils/output-truncation';
910
import type { NodeOutput } from './schemas';
1011

1112
function completed(
@@ -110,6 +111,106 @@ describe('resolveNodeOutputField — declared-schema producer', () => {
110111
const r = resolveNodeOutputField(completed('{"type":"BUG"}', undefined, ['type']), 'n', 'type');
111112
expect(r).toEqual({ kind: 'value', value: 'BUG' });
112113
});
114+
115+
// #2456 — a declared schema must never be QUIETER than no schema at all. Before this,
116+
// an unparseable output returned empty here while the schemaless path threw, so
117+
// declaring output_format on a `workflow:` node (whose child output is never
118+
// validated) silently turned every declared field into ''.
119+
it('unparseable output → throws, exactly like the schemaless path (#2456)', () => {
120+
const broken = completed('I could not produce JSON, sorry.', undefined, declared);
121+
expect(() => resolveNodeOutputField(broken, 'n', 'type')).toThrow(OutputRefError);
122+
try {
123+
resolveNodeOutputField(broken, 'n', 'type');
124+
} catch (e) {
125+
expect((e as OutputRefError).reason).toBe('unparseable');
126+
}
127+
});
128+
129+
it('declaring a schema is never quieter than declaring none (#2456)', () => {
130+
const text = 'not json at all';
131+
const withSchema = (): unknown =>
132+
resolveNodeOutputField(completed(text, undefined, ['f']), 'n', 'f');
133+
const withoutSchema = (): unknown => resolveNodeOutputField(completed(text), 'n', 'f');
134+
// Both throw, and for the same reason — that symmetry IS the contract.
135+
expect(withSchema).toThrow(OutputRefError);
136+
expect(withoutSchema).toThrow(OutputRefError);
137+
const reasonOf = (fn: () => unknown): string | undefined => {
138+
try {
139+
fn();
140+
} catch (e) {
141+
return (e as OutputRefError).reason;
142+
}
143+
return undefined;
144+
};
145+
expect(reasonOf(withSchema)).toBe(reasonOf(withoutSchema));
146+
});
147+
148+
// The leniency that SURVIVES: a declared-optional field missing from a payload that
149+
// genuinely parsed. Only "no parseable object at all" changed.
150+
it('still lenient for a missing key inside a parsed object (#2456 scope guard)', () => {
151+
const r = resolveNodeOutputField(completed('{"type":"BUG"}', undefined, declared), 'n', 'note');
152+
expect(r).toEqual({ kind: 'empty' });
153+
});
154+
});
155+
156+
/**
157+
* Clipped-on-persist output parses no better than prose, but the author needs the
158+
* opposite advice: the producer was right and a RESUMED run is reading the clipped
159+
* copy. `output_format` lives on `dagNodeBaseSchema`, so a bash node can declare one
160+
* — and bash stdout is the thing the event cap clips.
161+
*/
162+
describe('resolveNodeOutputField — output clipped before persistence', () => {
163+
/** What `getDagResumeSnapshot` hands back for a bash node that exceeded the cap. */
164+
function clipped(payload: string): string {
165+
return payload.slice(0, 40) + buildTruncationMarker(Buffer.byteLength(payload));
166+
}
167+
168+
const bigPayload = JSON.stringify({ verdict: 'pass', blob: 'x'.repeat(40_000) });
169+
170+
it('reports truncated, not unparseable, on the declared-schema path', () => {
171+
const node = completed(clipped(bigPayload), undefined, ['verdict', 'blob']);
172+
try {
173+
resolveNodeOutputField(node, 'gen', 'verdict');
174+
throw new Error('expected a throw');
175+
} catch (e) {
176+
expect(e).toBeInstanceOf(OutputRefError);
177+
expect((e as OutputRefError).reason).toBe('truncated');
178+
}
179+
});
180+
181+
it('reports truncated on the schemaless path too — both paths stay symmetric', () => {
182+
try {
183+
resolveNodeOutputField(completed(clipped(bigPayload)), 'gen', 'verdict');
184+
throw new Error('expected a throw');
185+
} catch (e) {
186+
expect((e as OutputRefError).reason).toBe('truncated');
187+
}
188+
});
189+
190+
it('does not blame truncation for output that merely mentions it', () => {
191+
// The marker is anchored, so prose quoting the phrase mid-string is still a
192+
// plain producer error — otherwise this branch would misdiagnose in reverse.
193+
const prose = 'the log said … [truncated; original output was 5 bytes] and then stopped';
194+
try {
195+
resolveNodeOutputField(completed(prose, undefined, ['verdict']), 'gen', 'verdict');
196+
throw new Error('expected a throw');
197+
} catch (e) {
198+
expect((e as OutputRefError).reason).toBe('unparseable');
199+
}
200+
});
201+
202+
it('says the producer was probably right, and points at the artifacts dir', () => {
203+
const err = new OutputRefError('gen', 'verdict', 'truncated');
204+
expect(err.message).toContain('clipped');
205+
expect(err.message).toContain('$ARTIFACTS_DIR');
206+
// The old advice was actively wrong here — the node DID emit the field.
207+
expect(err.message).not.toContain('Emit JSON containing');
208+
});
209+
210+
it('marker round-trips through build/detect', () => {
211+
expect(hasTruncationMarker(`head${buildTruncationMarker(1234)}`)).toBe(true);
212+
expect(hasTruncationMarker('no marker here')).toBe(false);
213+
});
113214
});
114215

115216
describe('resolveNodeOutputField — structuredOutput without a declared schema (lenient)', () => {

packages/workflows/src/output-ref.ts

Lines changed: 37 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,15 @@
1010
* field ∈ declaredFields, value present → value
1111
* field ∈ declaredFields, value absent/null → '' (declared-optional / explicit null)
1212
* field ∉ declaredFields → THROW (typo / not in the contract)
13+
* output is not a JSON object at all → THROW (#2456 — a declared schema is
14+
* never quieter than no schema; the
15+
* leniency above covers a missing KEY
16+
* in a parsed object, not a missing object)
17+
*
18+
* Either THROW-on-unparseable above reports reason 'truncated' instead of
19+
* 'unparseable' when the output carries the persistence truncation marker — same
20+
* parse failure, but the producer was right and a resumed run is reading a clipped
21+
* copy, so the author needs opposite advice. See utils/output-truncation.ts.
1322
* 2. Has a `structuredOutput` object but NO `declaredFields` (legacy rows, or a
1423
* non-object schema) — prefer it, but stay LENIENT: with no declared schema we
1524
* can't tell optional-absent from a typo, so:
@@ -31,6 +40,7 @@
3140
*/
3241
import type { NodeOutput } from './schemas';
3342
import { findSimilar } from './utils/fuzzy-match';
43+
import { hasTruncationMarker } from './utils/output-truncation';
3444

3545
/**
3646
* Thrown when a `$nodeId.output.field` reference cannot be honored under the
@@ -40,6 +50,7 @@ import { findSimilar } from './utils/fuzzy-match';
4050
export type OutputRefErrorReason =
4151
| 'not-in-schema'
4252
| 'unparseable'
53+
| 'truncated'
4354
| 'missing-key'
4455
| 'producer-not-run'
4556
| 'unknown-node';
@@ -68,6 +79,8 @@ export class OutputRefError extends Error {
6879
return `'${ref}' references field '${field}', which is not declared in node '${nodeId}'s output_format schema. Add '${field}' to the schema (and mark it optional if it can be absent), or fix the reference.`;
6980
case 'unparseable':
7081
return `'${ref}' references field '${field}', but node '${nodeId}'s output is not a JSON object, so the field cannot be read. Emit JSON containing '${field}', or reference '$${nodeId}.output' (whole text) instead.`;
82+
case 'truncated':
83+
return `'${ref}' references field '${field}', but node '${nodeId}'s persisted output was clipped at the event size cap and no longer parses as JSON. The node very likely emitted '${field}' correctly — this surfaces on a resumed run, which reads the clipped copy rather than the original. Write the payload to a file under $ARTIFACTS_DIR and read it downstream, or shrink the node's output.`;
7184
case 'missing-key':
7285
return `'${ref}' references field '${field}', but node '${nodeId}'s JSON output has no such key. Emit '${field}' in the output, or fix the reference.`;
7386
case 'producer-not-run':
@@ -111,6 +124,16 @@ export function declaredFieldsFromSchema(
111124
return Object.keys(props as Record<string, unknown>);
112125
}
113126

127+
/**
128+
* Distinguish "the producer emitted no JSON" from "the JSON it emitted was clipped
129+
* before persistence". Same failure to parse, opposite advice to the author: the
130+
* first means fix the producer, the second means the producer was already right and
131+
* a resumed run is reading a clipped copy.
132+
*/
133+
function unparseableReason(output: string): OutputRefErrorReason {
134+
return hasTruncationMarker(output) ? 'truncated' : 'unparseable';
135+
}
136+
114137
export type FieldResolution = { kind: 'value'; value: unknown } | { kind: 'empty' };
115138

116139
/** Strip a single markdown code fence (```json … ```) some models/scripts wrap JSON in. */
@@ -162,9 +185,18 @@ export function resolveNodeOutputField(
162185
throw new OutputRefError(nodeId, field, 'not-in-schema');
163186
}
164187
// Prefer the parsed payload; fall back to parsing the JSON-serialized output
165-
// (covers older NodeOutput rows that predate `structuredOutput`).
188+
// (covers older NodeOutput rows that predate `structuredOutput`, and the resume
189+
// path, which rehydrates text only).
166190
const obj = structuredObj ?? parseOutputObject(nodeOutput.output);
167-
if (obj === undefined) return { kind: 'empty' };
191+
// No parseable object AT ALL is not a declared-optional field — it is a producer
192+
// that did not honour its schema, and it must fail exactly as loudly as the
193+
// schemaless path below (#2456). Returning empty here made declaring
194+
// `output_format` QUIETER than declaring nothing, which is backwards: a
195+
// `workflow:` node's output_format is never validated against the child (it only
196+
// populates declaredFields), so every declared field silently became ''.
197+
if (obj === undefined) {
198+
throw new OutputRefError(nodeId, field, unparseableReason(nodeOutput.output));
199+
}
168200
const value = obj[field];
169201
// Required fields are guaranteed present (the producer validated post-parse),
170202
// so a missing/explicit-null value here is a declared-optional field → empty.
@@ -186,7 +218,9 @@ export function resolveNodeOutputField(
186218
// 3. Schemaless producer (bash/script/prose). The author wrote `.field`, so
187219
// JSON carrying that key is expected; anything else is a drop they must see.
188220
const obj = parseOutputObject(nodeOutput.output);
189-
if (obj === undefined) throw new OutputRefError(nodeId, field, 'unparseable');
221+
if (obj === undefined) {
222+
throw new OutputRefError(nodeId, field, unparseableReason(nodeOutput.output));
223+
}
190224
if (!(field in obj)) throw new OutputRefError(nodeId, field, 'missing-key');
191225
return { kind: 'value', value: obj[field] };
192226
}
Lines changed: 33 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,33 @@
1+
/**
2+
* The marker appended to node output that was clipped before persistence.
3+
*
4+
* Two sides must agree on this string, so it lives in one place rather than
5+
* being written by one and pattern-matched by the other:
6+
* - `formatPersistedBashOutput` (dag-executor) WRITES it when successful bash
7+
* stdout exceeds the persisted-event byte cap.
8+
* - `resolveNodeOutputField` (output-ref) RECOGNISES it so a resumed run can
9+
* explain why an output it cannot parse was nonetheless emitted correctly.
10+
*
11+
* That second case is not hypothetical. `output_format` is declared on
12+
* `dagNodeBaseSchema`, so a bash node may carry one; the fresh path holds the
13+
* full stdout in memory and parses fine, while a resumed run rehydrates the
14+
* clipped text and cannot. Without this marker the failure reads "output is not
15+
* a JSON object — emit JSON containing 'x'", which sends the author to fix a
16+
* producer that was already correct.
17+
*/
18+
19+
/** Build the marker for an output clipped from `originalBytes` UTF-8 bytes. */
20+
export function buildTruncationMarker(originalBytes: number): string {
21+
return `\n\n… [truncated; original output was ${String(originalBytes)} bytes]`;
22+
}
23+
24+
/**
25+
* Matches {@link buildTruncationMarker} at end-of-string. Anchored so arbitrary
26+
* node output that merely quotes the phrase is not mistaken for a clipped one.
27+
*/
28+
const TRUNCATION_MARKER_PATTERN = /\n\n \[truncated; original output was \d+ bytes\]$/;
29+
30+
/** Whether `output` ends with the persistence truncation marker. */
31+
export function hasTruncationMarker(output: string): boolean {
32+
return TRUNCATION_MARKER_PATTERN.test(output);
33+
}

0 commit comments

Comments
 (0)