fix(workflows): persist structuredOutput on NodeOutput so $node.output.field works for Pi#1654
Conversation
…t.field works for Pi When a provider parses fence-wrapped or preamble-prefixed JSON onto the result chunk (Pi/Minimax via tryParseStructuredOutput), the executor captured it locally but never persisted it onto NodeOutput. Downstream consumers (substituteNodeOutputRefs, condition-evaluator) then JSON.parse(output)'d the original prose-prefixed text, which threw, and $node.output.field resolved to empty. This persists structuredOutput on NodeOutput (single-shot and loop-terminal-iteration success paths) and teaches both consumers to prefer the parsed object over re-parsing prose. Falls back to JSON.parse(output) when structuredOutput is absent so Claude/Codex output_format-encoded NodeOutput rows (and older rows written before this field existed) keep working. Cross-resume rehydration of structuredOutput from event_data is out of scope here; resumed runs that re-execute downstream nodes will fall through to the JSON.parse path, which matches existing behavior. Closes coleam00#1571
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThis PR introduces a ChangesStructured Output Preference Throughout Resolution and Execution
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (3)
packages/workflows/src/dag-executor.ts (1)
302-321: 💤 Low valueClarify comment at line 320 to match the accurate version at line 335.
Similar to the issue in
condition-evaluator.ts, the comment at line 320 states// null, undefined, symbol, bigint → empty, but null field values are actually JSON-stringified at line 317 (becausetypeof null === 'object').The comment at line 335 (in the JSON.parse fallback path) correctly notes "null is caught above by typeof check". Line 320 should use the same accurate phrasing for consistency.
Suggested clarification
- return escapedForBash ? "''" : ''; + return escapedForBash ? "''" : ''; // undefined, symbol, bigint → empty (null is caught above by typeof check)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/workflows/src/dag-executor.ts` around lines 302 - 321, Update the misleading inline comment near the structuredOutput handling (around the block using the `structured` variable and the field extraction) so it matches the accurate wording used in the JSON.parse fallback: explicitly note that null is handled above by the typeof/object check rather than being treated as "empty"; e.g., change the remark that currently says "null, undefined, symbol, bigint → empty" to indicate "undefined, symbol, bigint → empty (null is caught above by typeof check)" so the intent is consistent with the fallback path.packages/workflows/src/condition-evaluator.ts (2)
48-63: 💤 Low valueClarify comment: null is JSON-stringified, not coerced to empty string.
The comment at line 62 states
// null, undefined, symbol, bigint → empty, but this is misleading fornull. Becausetypeof null === 'object'in JavaScript, a null field value matches the condition at line 61 and returnsJSON.stringify(null)(the string"null"), not an empty string. Onlyundefined,symbol, andbigintreach line 62.The code behavior is correct (confirmed by test at lines 416-420), but the comment could confuse future maintainers.
Suggested clarification
- return ''; // null, undefined, symbol, bigint → empty + return ''; // undefined, symbol, bigint → empty (null is JSON-stringified above)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/workflows/src/condition-evaluator.ts` around lines 48 - 63, The comment describing fallback behavior for values in the structuredOutput branch is inaccurate about null; update the comment near the structuredOutput handling in condition-evaluator.ts (the block that checks const structured = 'structuredOutput' in nodeOutput ? nodeOutput.structuredOutput : undefined and then inspects value) to clarify that null will be JSON.stringify'd (resulting in "null") while undefined, symbol, and bigint fall through to the empty-string return; keep existing logic unchanged and only adjust the explanatory comment to accurately reflect these cases.
65-82: 💤 Low valueSame comment issue as above: clarify null handling.
The comment at line 74 has the same misleading phrasing as line 62. A null field value is JSON-stringified (line 73), not coerced to empty string.
Suggested clarification
- return ''; // null, undefined, symbol, bigint → empty + return ''; // undefined, symbol, bigint → empty (null is JSON-stringified above)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/workflows/src/condition-evaluator.ts` around lines 65 - 82, The comment claiming "null, undefined, symbol, bigint → empty" is misleading because null currently matches the Array.isArray(value) || typeof value === 'object' branch and is JSON.stringify'd, producing "null"; update the comment near the fallback parsing logic to state that null will be JSON.stringify'd (resulting in "null") while undefined, symbol, and bigint return '' and ensure the wording around parsed/value and the Array.isArray(value) || typeof value === 'object' branch (and the related earlier comment) clearly reflect this behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@packages/workflows/src/condition-evaluator.ts`:
- Around line 48-63: The comment describing fallback behavior for values in the
structuredOutput branch is inaccurate about null; update the comment near the
structuredOutput handling in condition-evaluator.ts (the block that checks const
structured = 'structuredOutput' in nodeOutput ? nodeOutput.structuredOutput :
undefined and then inspects value) to clarify that null will be JSON.stringify'd
(resulting in "null") while undefined, symbol, and bigint fall through to the
empty-string return; keep existing logic unchanged and only adjust the
explanatory comment to accurately reflect these cases.
- Around line 65-82: The comment claiming "null, undefined, symbol, bigint →
empty" is misleading because null currently matches the Array.isArray(value) ||
typeof value === 'object' branch and is JSON.stringify'd, producing "null";
update the comment near the fallback parsing logic to state that null will be
JSON.stringify'd (resulting in "null") while undefined, symbol, and bigint
return '' and ensure the wording around parsed/value and the
Array.isArray(value) || typeof value === 'object' branch (and the related
earlier comment) clearly reflect this behavior.
In `@packages/workflows/src/dag-executor.ts`:
- Around line 302-321: Update the misleading inline comment near the
structuredOutput handling (around the block using the `structured` variable and
the field extraction) so it matches the accurate wording used in the JSON.parse
fallback: explicitly note that null is handled above by the typeof/object check
rather than being treated as "empty"; e.g., change the remark that currently
says "null, undefined, symbol, bigint → empty" to indicate "undefined, symbol,
bigint → empty (null is caught above by typeof check)" so the intent is
consistent with the fallback path.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 03392aec-066f-436b-b7c5-314cc215be2d
📒 Files selected for processing (5)
packages/workflows/src/condition-evaluator.test.tspackages/workflows/src/condition-evaluator.tspackages/workflows/src/dag-executor.test.tspackages/workflows/src/dag-executor.tspackages/workflows/src/schemas/workflow-run.ts
Review SummaryVerdict: ready-to-merge This is a solid bug fix. The PR adds Blocking issuesNone. Suggested fixesNone. Minor / nice-to-have
Compliments
Reviewed via maintainer-review-pr workflow (Pi/Minimax). Aspects run: code-review, test-coverage, comment-quality. |
…t.field works for Pi (coleam00#1654) * fix(workflows): persist structuredOutput on NodeOutput so $node.output.field works for Pi When a provider parses fence-wrapped or preamble-prefixed JSON onto the result chunk (Pi/Minimax via tryParseStructuredOutput), the executor captured it locally but never persisted it onto NodeOutput. Downstream consumers (substituteNodeOutputRefs, condition-evaluator) then JSON.parse(output)'d the original prose-prefixed text, which threw, and $node.output.field resolved to empty. This persists structuredOutput on NodeOutput (single-shot and loop-terminal-iteration success paths) and teaches both consumers to prefer the parsed object over re-parsing prose. Falls back to JSON.parse(output) when structuredOutput is absent so Claude/Codex output_format-encoded NodeOutput rows (and older rows written before this field existed) keep working. Cross-resume rehydration of structuredOutput from event_data is out of scope here; resumed runs that re-execute downstream nodes will fall through to the JSON.parse path, which matches existing behavior. Closes coleam00#1571 * test(workflows): docstring the structuredOutput makeOutput fixtures
Re-target of #1637 onto
devper @Wirasm. No code change; rebased onto upstream/dev. Branch tip:fcb33ed4(ED25519 signed). All Validation Evidence below re-run on the rebased branch.Summary
tryParseStructuredOutput), the DAG executor captures it into a local variable but never persists it ontoNodeOutput. Downstream$node.output.fieldsubstitution andwhen:conditions then tryJSON.parse(output)on the original prose-prefixed text, throw, and resolve to empty.$classifier.output.type(or any other field) silently routes to the wrong branch or drops the value.structuredOutput?: unknownadded to the completed/running and failed NodeOutput branches; producer call sites persist it; consumers (substituteNodeOutputRefs,condition-evaluator.resolveOutputRef) prefer it overJSON.parse(output)and fall back to the existing path when absent.$node.output(unfielded) still readsoutputtext. Cross-resume rehydration fromevent_datais out of scope (resumed runs that re-execute downstream nodes fall through to JSON.parse). Providers that already overwriteoutputwith a JSON.stringify of structuredOutput (Claude/Codex withoutput_format) are unaffected because theiroutputis already JSON.UX Journey
Before
After
Architecture Diagram
Before
After
Connection inventory:
dag-executor(single-shot success return)NodeOutput.structuredOutputdag-executor(loop terminal-iteration return)NodeOutput.structuredOutputlastIterationStructuredOutputlocalsubstituteNodeOutputRefsNodeOutput.structuredOutputcondition-evaluator.resolveOutputRefNodeOutput.structuredOutputnodeOutputSchema(completed/running, failed)z.unknown().optional()Label Snapshot
risk: lowsize: Sworkflowsworkflows:executor,workflows:condition-evaluator,workflows:schemasChange Metadata
bugworkflowsLinked Issue
Validation Evidence (required)
New tests added:
condition-evaluator.test.ts: 12 new tests covering structuredOutput preference, Claude/Codex JSON.parse fallback regression, numeric/boolean/object/array field coercion, null/top-level-array/primitive fall-through, missing-field semantics, and bare$node.output(no field) still uses output text.dag-executor.test.ts: 13 new tests forsubstituteNodeOutputRefscovering the same matrix plus shell-escaping safety on structuredOutput fields.Security Impact (required)
NoNoNoNoCompatibility / Migration
Yes.structuredOutputis optional on the schema; producers populate it only when the provider emits one; consumers fall through to the existingJSON.parse(output)path when it is absent. Workflows that depend on the JSON.parse path keep working unchanged.NoNo. NodeOutput is stored inline in workflow event data; older rows withoutstructuredOutputdeserialize asundefinedand hit the JSON.parse fallback.NodeOutputfromevent_data. If the original run was on this version, the field is preserved; if older, it is absent and the JSON.parse fallback runs. Cross-version resume of Pi workflows that previously routed wrong will route correctly once the upstream re-runs on the new version.Human Verification (required)
$node.output(no field) ignores structuredOutput and reads output text.escapedForBash=true) wraps structuredOutput strings/objects in single quotes; numbers/booleans stay unquoted.Side Effects / Blast Radius (required)
$node.output.fieldsubstitution or awhen:expression that uses dot notation.JSON.parse(output)returning a different shape than what the provider's parsed structuredOutput contains (e.g. provider parses one JSON object out of multiple in the prose) would see a behavior change. In practice the providers that emitstructuredOutputare emitting the canonical structured payload that the prompt asked for, so the change is the intended fix, not a surprise.condition_json_parse_failed,dag_node_output_ref_json_parse_failed) still fire on the fallback path; no new error paths introduced.Rollback Plan (required)
$node.output.fieldwould resolve to JSON-stringified content where it previously resolved to a plain string (would surface as condition mismatches or wrongly-formatted prompts on Claude/Codex output_format workflows).Risks and Mitigations
$node.output.fieldon a Pi/Minimax node (because the JSON.parse failed) might now see the resolved value and take a previously-unreachable branch. This is the intended behavior change for Parsed structured output not persisted to NodeOutput — condition evaluator and $node.output.field re-parse raw text, breaking Pi/Minimax #1571 but could surprise authors who built around the bug.''when the structuredOutput object doesn't contain the requested key). The fix only changes the case where the provider actually parsed the structured payload the prompt requested.Summary by CodeRabbit
New Features
Behavior Changes
Tests