Fix Issue #972 | Workflow Composition#1282
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughImplements workflow-level looping ( Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Parent as Parent Workflow
participant Loader as Loader/Validator
participant Executor as DAG Executor
participant Child as Child Workflow
participant Store as Workflow Store
Parent->>Loader: Parse & validate workflow (includes `workflow:` node)
Loader-->>Parent: Validated DAG
Parent->>Executor: executeDagWorkflow(parent)
Executor->>Child: spawn child WorkflowRun (executeWorkflowNode)
Child->>Store: register run + SSE subscription
Child->>Child: execute child DAG (may iterate via loop_until)
Child-->>Executor: final child run status/output
Executor->>Executor: map child output → $nodeId.output
Executor->>Parent: continue parent DAG with substituted outputs
sequenceDiagram
autonumber
participant Executor as DAG Executor
participant Nodes as DAG Nodes
participant Eval as Condition Evaluator
participant SSE as Event Emitter
Executor->>Nodes: iteration 1 — execute topological layers (fresh nodeOutputs)
Nodes-->>Executor: populate nodeOutputs
Executor->>SSE: emit iteration_completed
Executor->>Eval: evaluate loop_until ($nodeId.field == value)
Eval-->>Executor: result true|false
alt result == false AND iteration < max_iterations
Executor->>Executor: iteration++, clear nodeOutputs, repeat
else result == true OR max_iterations reached
Executor->>SSE: emit finalization (stop looping)
end
Executor->>Executor: aggregate terminal outputs & persist WorkflowRun
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes 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.
Actionable comments posted: 5
🧹 Nitpick comments (5)
packages/workflows/src/schemas/workflow.ts (1)
57-58: Field naming inconsistency — confirm snake_case is intentional here.Neighboring workflow-level fields (
modelReasoningEffort,webSearchMode,additionalDirectories,fallbackModel) are camelCase, but these new fields are snake_case. Node-level fields likedepends_on/on_rejectare snake_case, so there's precedent on the node side — but mixing both styles at the same schema level will be confusing for YAML authors. If this is deliberate to match the iteration primitive style, fine; otherwise considerloopUntil/maxIterationsfor consistency with surrounding workflow-level fields.Also worth considering:
max_iterationscurrently has no upper bound. The executor floors it at 1 (Math.max(1, workflow.max_iterations ?? 1)) but a misconfigured1e9would stall a run. A schema.max(...)(e.g. 100) would provide an early, authored-time guardrail. Optional.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/workflows/src/schemas/workflow.ts` around lines 57 - 58, Change the workflow-level fields to use consistent camelCase naming and add an upper bound on iterations: rename loop_until → loopUntil and max_iterations → maxIterations in the workflow schema (where fields like modelReasoningEffort, webSearchMode, additionalDirectories, fallbackModel are defined) and constrain maxIterations with a sensible .max(...) (e.g., 100) to prevent runaway values; update any code that reads workflow.max_iterations or workflow.loop_until to the new workflow.maxIterations and workflow.loopUntil accessors.packages/workflows/src/loader.ts (1)
96-100: Stale docstring —input:(andloop.prompt) are now also scanned.The function JSDoc still says refs are checked only "in when:/prompt: fields", but the body now also scans
loop.prompt(existing) andinput:on workflow nodes (new in this PR). The inline comment on Line 151 is already updated — please bring this block in line so readers don't rely on the stale description.📝 Proposed doc update
/** * Validate DAG structure: unique IDs, depends_on references exist, no cycles, - * and $nodeId.output refs in when:/prompt: fields point to known nodes. + * and $nodeId.output refs in when:, prompt:, loop.prompt, and workflow-node + * input: fields point to known nodes. * Returns error message or null if valid. */🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/workflows/src/loader.ts` around lines 96 - 100, Update the stale JSDoc for the DAG validation function (the block that begins "Validate DAG structure: unique IDs...") to reflect that the validator now also scans loop.prompt and input: fields in addition to when:/prompt: fields; modify the description to explicitly list "when:/prompt:, loop.prompt, and input:" as locations where $nodeId.output refs are checked and return that the function still returns an error message or null if valid.packages/workflows/src/dag-executor.test.ts (1)
5492-5564: Add coverage thatloop_untiliterations reset DAG scope.These tests verify retry counts and early stop, but not the PR objective that each workflow-level iteration uses a fresh scope. A regression that reuses prior
nodeOutputsacross iterations could still pass here.Suggested test shape
+ it('loop_until starts each pass with fresh node output scope', async () => { + let callCount = 0; + mockSendQueryDag.mockImplementation(function* (prompt: string) { + callCount++; + if (prompt.includes('$missing.output') || prompt.includes('previous-only')) { + yield { type: 'assistant', content: 'leaked previous scope' }; + } else { + yield { type: 'assistant', content: callCount >= 2 ? 'done' : 'not yet' }; + } + yield { type: 'result', sessionId: `loop-session-${String(callCount)}` }; + }); + + const store = createMockStore(); + const mockDeps = createMockDeps(store); + const platform = createMockPlatform(); + const workflowRun = makeWorkflowRun('loop-fresh-scope-run', { workflow_name: 'loop-test' }); + + await executeDagWorkflow( + mockDeps, + platform, + 'conv-loop', + testDir, + { + name: 'loop-test', + nodes: [{ id: 'step', prompt: 'do work' }], + loop_until: "$step.output == 'done'", + max_iterations: 3, + }, + workflowRun, + 'claude', + undefined, + join(testDir, 'artifacts'), + join(testDir, 'logs'), + 'main', + 'docs/', + minimalConfig + ); + + expect(mockSendQueryDag.mock.calls.length).toBe(2); + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/workflows/src/dag-executor.test.ts` around lines 5492 - 5564, Add a test (e.g., "loop_until resets DAG scope between iterations") that calls executeDagWorkflow with a loop_until and a mockSendQueryDag implementation which captures the per-iteration request context/inputs and verifies no prior-iteration nodeOutputs are carried forward; specifically, have mockSendQueryDag yield different assistant contents per iteration and assert that the payload/context it receives for iteration N does NOT contain the "step" output from iteration N-1 (i.e., ensure nodeOutputs/reset of DAG scope between iterations), using the existing helpers createMockStore, makeWorkflowRun and the same loop_until/max_iterations shape as the other loop tests.packages/workflows/src/schemas/dag-node.ts (1)
290-320: Schema additions look correct — small consistency nit on silently-ignored fields.
workflowNodeSchema,WorkflowNode, andisWorkflowNodefollow the existingcamelCase+z.infer<typeof schema>conventions and mutual-exclusivity treatment;WORKFLOW_NODE_AI_FIELDSmatches the bash/script pattern for loader warnings.One consistency gap:
retryis explicitly rejected on loop nodes at Line 523 ("retry is not supported on loop nodes"), but workflow nodes acceptretrywithout complaint even though the dispatcher atdag-executor.tsLine 2811 returns before the retry loop — soretry:on aworkflow:node is silently discarded. Either reject it insuperRefinealongside the loop check, or document it in the WORKFLOW_NODE_AI_FIELDS loader warning so users don't assume it works.As per coding guidelines: "Prefer throwing early with a clear error for unsupported or unsafe states — never silently swallow errors or broaden permissions/capabilities; document intentional fallback behavior with a comment".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/workflows/src/schemas/dag-node.ts` around lines 290 - 320, workflowNodeSchema currently allows a retry field but retries are ignored at runtime (dispatcher returns before retry logic), so update workflowNodeSchema to explicitly reject "retry" in its validation path: add a superRefine check on workflowNodeSchema that fails with a clear message like "'retry' is not supported on workflow nodes" when an incoming object contains retry, mirror the loop-node rejection behavior (see LoopNode superRefine pattern), and also add the same note to WORKFLOW_NODE_AI_FIELDS loader warning so users see it at load-time; update the WorkflowNode type/comments if needed to reflect the rejection.packages/workflows/src/dag-executor.ts (1)
2453-2454:max_iterationsdefaults to1whenloop_untilis set — effectively never loops.
Math.max(1, workflow.max_iterations ?? 1)gives a default of1, so a workflow that specifiesloop_until:but forgetsmax_iterations:will run the DAG exactly once and evaluate the condition once (since the evaluation is the last step of an iteration, the re-run branch at Line 3131 is unreachable on the default). This matches the PR objective of "default max_iterations = 1 if omitted", but it's a silently surprising no-op — users will assumeloop_untiliterates by default.Consider either (a) defaulting to something more useful like
5whenloop_untilis present, or (b) emitting a one-time warning viasafeSendMessage/ logger whenloop_untilis set butmax_iterationsis absent, so users know iteration is effectively disabled.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/workflows/src/dag-executor.ts` around lines 2453 - 2454, The current maxIterations calculation (const maxIterations = workflow.loop_until !== undefined ? Math.max(1, workflow.max_iterations ?? 1) : 1) silently makes loop_until a no-op when max_iterations is omitted; change it so that when workflow.loop_until is defined and workflow.max_iterations is undefined you both (1) emit a clear warning via safeSendMessage or the existing logger that loop_until is set but max_iterations was omitted, and (2) use a more useful default (e.g., 5) instead of 1; update the code that computes maxIterations (the const maxIterations line) and add the one-time warning before it so callers see the message when max_iterations is not provided.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/workflows/src/condition-evaluator.ts`:
- Around line 100-101: The atom regex (atomPattern) allows unquoted
numeric/boolean RHS but the evaluator still uses string equality (actual ===
expected) when operator is ==/!=; update the atom evaluation logic (the function
that matches atomPattern and performs the comparison) to detect when the RHS
matched the unquoted numeric/boolean capture and coerce/parse both actual (from
resolveOutputRef) and expected into the appropriate numeric or boolean types
before performing ==/!= (or use numeric comparison behavior parallel to
<,>,<=,>=), falling back to string comparison for quoted RHS; ensure you
reference atomPattern and replace the string equality branch (actual ===
expected) with type-aware comparison and handle parse failures safely.
In `@packages/workflows/src/dag-executor.ts`:
- Around line 2186-2201: Add a runtime guard to prevent infinite recursion for
workflow: nodes by threading a call-chain (e.g., an array of invoked workflow
names) or a depth counter through executeDagWorkflow and check it before calling
discoverWorkflowsWithConfig; in executeDagWorkflow (or the caller that handles
node execution) detect if node.workflow already appears in the current
invocation chain or if depth > 8, then log via getLog().error with node.id and
child workflow, call safeSendMessage with a clear error message, and return a
failed state (same shape as the existing catch block) rather than proceeding to
discoverWorkflowsWithConfig/execute the child, ensuring WorkflowRun rows are not
repeatedly created and recursion fails fast.
- Around line 3111-3152: The loop_until branches currently treat parse failures
and max-iteration exhaustion as successful completion; update the handling in
the evaluate loop (the branch that checks workflow.loop_until after
evaluateCondition) so that on an unparseable expression or when iteration
reaches maxIterations without result === true you mark the workflow run as
failed (instead of letting it fall through to the normal completion path) and
persist a failure reason in the run metadata (e.g., include keys like
loop_until_parse_error or loop_until_max_iterations_exceeded and the offending
expression/maxIterations). Concretely, modify the parse-failure branch that logs
getLog().warn(...) and sends the safeSendMessage to also call the workflow
completion/fail helper with state='failed' (or invoke the same
completeWorkflowRun/finish function but passing failure state and metadata), and
do the same for the maxIterations branch that currently logs getLog().warn(...);
ensure you reference workflow.loop_until, maxIterations, evaluateCondition, and
the path that calls completeWorkflowRun/finish so the failure state and metadata
are persisted and visible to callers/UI.
- Around line 2282-2380: The child workflow's cost isn't added to the parent's
total_cost_usd because executeWorkflowNode returns NodeOutput (no costUsd) and
we never read the child's persisted total_cost_usd; fix by after a successful
child run (where childFinalStatus === 'completed') call the store to fetch the
child's run record (e.g., deps.store.getWorkflowRun(childRun.id) or the
appropriate get-by-id API), read its total_cost_usd, and propagate it back into
the parent node result: set/return a costUsd field on the node output (or
convert the node result to NodeExecutionResult with costUsd) and ensure the
parent accumulator (finalTotalCostUsd) adds that value; update places that
construct the node result from executeDagWorkflow/executeWorkflowNode to include
this costUsd so composed workflows aggregate child costs.
- Around line 2253-2331: executeWorkflowNode currently awaits
executeDagWorkflow(childRun, …) without propagating parent cancellation; add
cancellation propagation by creating an AbortController (or a short-lived poll
interval) in executeWorkflowNode and pass the signal into executeDagWorkflow
(extend executeDagWorkflow signature to accept a parentAbort?: AbortSignal),
and/or start a periodic check using
deps.store.getWorkflowRunStatus(parentRun.id) that, when the parent status is no
longer 'running', calls deps.store.updateWorkflowRun(childRun.id, { status:
'cancelled' }), emitter.unregisterRun(childRun.id), and signals the
AbortController; ensure the interval is cleared and the AbortSignal handled by
the child path so executeDagWorkflow aborts promptly and cleanup
(unregister/cleanup) happens on both normal completion and parent-triggered
cancellation.
---
Nitpick comments:
In `@packages/workflows/src/dag-executor.test.ts`:
- Around line 5492-5564: Add a test (e.g., "loop_until resets DAG scope between
iterations") that calls executeDagWorkflow with a loop_until and a
mockSendQueryDag implementation which captures the per-iteration request
context/inputs and verifies no prior-iteration nodeOutputs are carried forward;
specifically, have mockSendQueryDag yield different assistant contents per
iteration and assert that the payload/context it receives for iteration N does
NOT contain the "step" output from iteration N-1 (i.e., ensure nodeOutputs/reset
of DAG scope between iterations), using the existing helpers createMockStore,
makeWorkflowRun and the same loop_until/max_iterations shape as the other loop
tests.
In `@packages/workflows/src/dag-executor.ts`:
- Around line 2453-2454: The current maxIterations calculation (const
maxIterations = workflow.loop_until !== undefined ? Math.max(1,
workflow.max_iterations ?? 1) : 1) silently makes loop_until a no-op when
max_iterations is omitted; change it so that when workflow.loop_until is defined
and workflow.max_iterations is undefined you both (1) emit a clear warning via
safeSendMessage or the existing logger that loop_until is set but max_iterations
was omitted, and (2) use a more useful default (e.g., 5) instead of 1; update
the code that computes maxIterations (the const maxIterations line) and add the
one-time warning before it so callers see the message when max_iterations is not
provided.
In `@packages/workflows/src/loader.ts`:
- Around line 96-100: Update the stale JSDoc for the DAG validation function
(the block that begins "Validate DAG structure: unique IDs...") to reflect that
the validator now also scans loop.prompt and input: fields in addition to
when:/prompt: fields; modify the description to explicitly list "when:/prompt:,
loop.prompt, and input:" as locations where $nodeId.output refs are checked and
return that the function still returns an error message or null if valid.
In `@packages/workflows/src/schemas/dag-node.ts`:
- Around line 290-320: workflowNodeSchema currently allows a retry field but
retries are ignored at runtime (dispatcher returns before retry logic), so
update workflowNodeSchema to explicitly reject "retry" in its validation path:
add a superRefine check on workflowNodeSchema that fails with a clear message
like "'retry' is not supported on workflow nodes" when an incoming object
contains retry, mirror the loop-node rejection behavior (see LoopNode
superRefine pattern), and also add the same note to WORKFLOW_NODE_AI_FIELDS
loader warning so users see it at load-time; update the WorkflowNode
type/comments if needed to reflect the rejection.
In `@packages/workflows/src/schemas/workflow.ts`:
- Around line 57-58: Change the workflow-level fields to use consistent
camelCase naming and add an upper bound on iterations: rename loop_until →
loopUntil and max_iterations → maxIterations in the workflow schema (where
fields like modelReasoningEffort, webSearchMode, additionalDirectories,
fallbackModel are defined) and constrain maxIterations with a sensible .max(...)
(e.g., 100) to prevent runaway values; update any code that reads
workflow.max_iterations or workflow.loop_until to the new workflow.maxIterations
and workflow.loopUntil accessors.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4647ba62-a249-4507-b043-898b02ff953c
📒 Files selected for processing (11)
.archon/workflows/defaults/archon-compose-plan-implement-qa.yaml.archon/workflows/defaults/archon-compose-planner.yamlpackages/workflows/src/condition-evaluator.test.tspackages/workflows/src/condition-evaluator.tspackages/workflows/src/dag-executor.test.tspackages/workflows/src/dag-executor.tspackages/workflows/src/defaults/bundled-defaults.generated.tspackages/workflows/src/loader.tspackages/workflows/src/schemas/dag-node.tspackages/workflows/src/schemas/index.tspackages/workflows/src/schemas/workflow.ts
| const atomPattern = | ||
| /^\$([a-zA-Z_][a-zA-Z0-9_-]*)\.output(?:\.([a-zA-Z_][a-zA-Z0-9_]*))?\s*(==|!=|<=|>=|<|>)\s*'([^']*)'$/; | ||
| /^\$([a-zA-Z_][a-zA-Z0-9_-]*)\.([a-zA-Z_][a-zA-Z0-9_]*)(?:\.([a-zA-Z_][a-zA-Z0-9_]*))?\s*(==|!=|<=|>=|<|>)\s*(?:'([^']*)'|(-?\d+(?:\.\d+)?|true|false))$/; |
There was a problem hiding this comment.
Heads up: ==/!= with unquoted numeric/boolean RHS is still a string comparison.
The regex accepts unquoted numbers/booleans as RHS, but expected is kept as the raw match text and actual === expected on Line 143 is a plain string compare. Consequently:
$n.exit_code == 0evaluatesfalsewhenactualresolves to"0.0"or"00"(or"0 "before trimming byresolveOutputRef).$n.score == 0.5evaluatesfalsewhenactualis".5"or"0.50".
This matches the prior quoted-form semantics, so it isn't a regression — but users adopting the new unquoted-number sugar will likely expect numeric equality the same way >/< do. Worth either:
- Using numeric equality when the unquoted RHS branch matched (parallel to the
< > <= >=path), or - Documenting the string-equality behavior on the doc comment block (Lines 10-11) so authors know to normalize producer output.
Not a blocker — flagging as a UX gotcha discovered while tracing atom evaluation.
Also applies to: 136-143
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/workflows/src/condition-evaluator.ts` around lines 100 - 101, The
atom regex (atomPattern) allows unquoted numeric/boolean RHS but the evaluator
still uses string equality (actual === expected) when operator is ==/!=; update
the atom evaluation logic (the function that matches atomPattern and performs
the comparison) to detect when the RHS matched the unquoted numeric/boolean
capture and coerce/parse both actual (from resolveOutputRef) and expected into
the appropriate numeric or boolean types before performing ==/!= (or use numeric
comparison behavior parallel to <,>,<=,>=), falling back to string comparison
for quoted RHS; ensure you reference atomPattern and replace the string equality
branch (actual === expected) with type-aware comparison and handle parse
failures safely.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
packages/workflows/src/dag-executor.test.ts (3)
5753-5759: Preserve parent conversation context in the stateful store fixture.The workflow-node tests exercise child-run behavior, but this fixture discards
parent_conversation_id. That can mask regressions in the context inheritance path.Suggested fixture fix
id: `run-${String(++nextRunSuffix)}-${data.workflow_name.replace(/\s/g, '-')}`, workflow_name: data.workflow_name, conversation_id: data.conversation_id, - parent_conversation_id: null, + parent_conversation_id: data.parent_conversation_id ?? null, codebase_id: data.codebase_id ?? null,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/workflows/src/dag-executor.test.ts` around lines 5753 - 5759, The stateful store fixture's createWorkflowRun mock is dropping parent_conversation_id, which hides child-run context behavior; update the mock implementation for createWorkflowRun (mock of IWorkflowStore['createWorkflowRun']) to preserve and return data.parent_conversation_id (use data.parent_conversation_id ?? null) in the resolved object (alongside id, workflow_name, conversation_id, codebase_id and nextRunSuffix) so parent conversation context flows through tests that rely on child-run inheritance.
5863-5864: Assert the parent run completed, not just two runs.
toHaveBeenCalledTimes(2)proves two completions happened, but not thatparent-run-1was one of them. Checking IDs makes the parent-completion guarantee explicit.Suggested assertion tightening
- // completeWorkflowRun is called once for child and once for parent (2 total) - expect(parentCompleteSpy).toHaveBeenCalledTimes(2); + // completeWorkflowRun is called once for child and once for parent. + const completedRunIds = parentCompleteSpy.mock.calls.map(([runId]) => runId); + expect(completedRunIds).toContain('parent-run-1'); + expect(completedRunIds.some(runId => runId.includes('child-workflow'))).toBe(true);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/workflows/src/dag-executor.test.ts` around lines 5863 - 5864, The test currently asserts parentCompleteSpy was called twice but doesn't verify one call was for the parent run; update the assertion to explicitly check that completeWorkflowRun was called for the parent run id (e.g., "parent-run-1") by inspecting parentCompleteSpy.mock.calls (or using toHaveBeenCalledWith) so one of the recorded calls includes the parent run id; keep the existing toHaveBeenCalledTimes(2) and add an additional assertion that at least one call contains "parent-run-1".
6056-6172: Add a workflow-node cancellation propagation case.These tests verify
parentAbortwhen passed directly intoexecuteDagWorkflow, but they do not cover the new workflow-node path where parent cancellation should poll, abort the child controller, and cancel the child run. Add a case that invokes aworkflow:node, flips the parent status tocancelledwhile the child is active, then asserts the child run is cancelled and later child layers do not start.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/workflows/src/dag-executor.test.ts` around lines 6056 - 6172, Add a new test in dag-executor.test.ts that exercises the workflow-node cancellation path: call executeDagWorkflow with a workflow node (use createMockStore, createMockDeps, createMockPlatform, makeWorkflowRun) that launches a child run, then while the child is active flip the parent run status to 'cancelled' (mock getWorkflowRunStatus or whichever parent-status poll is used) to simulate external cancellation; assert that the child controller is aborted and the child run is marked cancelled via the store mock (check the mock that updates run status, e.g. ensure a call setting status 'cancelled' or invoking the child-run cancellation path), and verify later child-layer nodes do not start by inspecting createWorkflowEvent calls for absent 'node_started' events for downstream steps. Ensure you reference executeDagWorkflow and the workflow: node id used in the test so the test covers the workflow-node cancellation propagation path.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/workflows/src/dag-executor.test.ts`:
- Around line 5667-5703: The test named "failed pass aborts loop..." doesn't
assert that the failing node stops being retried; add a call-count assertion for
the generator stub mockSendQueryDag to ensure it was invoked only once (or the
expected number of times) so the loop is aborted immediately; place the
expect(mockSendQueryDag).toHaveBeenCalledTimes(1) (or appropriate count) before
restoring failSpy/completeSpy to confirm the executor stopped iterating after
the error thrown by mockSendQueryDag.
---
Nitpick comments:
In `@packages/workflows/src/dag-executor.test.ts`:
- Around line 5753-5759: The stateful store fixture's createWorkflowRun mock is
dropping parent_conversation_id, which hides child-run context behavior; update
the mock implementation for createWorkflowRun (mock of
IWorkflowStore['createWorkflowRun']) to preserve and return
data.parent_conversation_id (use data.parent_conversation_id ?? null) in the
resolved object (alongside id, workflow_name, conversation_id, codebase_id and
nextRunSuffix) so parent conversation context flows through tests that rely on
child-run inheritance.
- Around line 5863-5864: The test currently asserts parentCompleteSpy was called
twice but doesn't verify one call was for the parent run; update the assertion
to explicitly check that completeWorkflowRun was called for the parent run id
(e.g., "parent-run-1") by inspecting parentCompleteSpy.mock.calls (or using
toHaveBeenCalledWith) so one of the recorded calls includes the parent run id;
keep the existing toHaveBeenCalledTimes(2) and add an additional assertion that
at least one call contains "parent-run-1".
- Around line 6056-6172: Add a new test in dag-executor.test.ts that exercises
the workflow-node cancellation path: call executeDagWorkflow with a workflow
node (use createMockStore, createMockDeps, createMockPlatform, makeWorkflowRun)
that launches a child run, then while the child is active flip the parent run
status to 'cancelled' (mock getWorkflowRunStatus or whichever parent-status poll
is used) to simulate external cancellation; assert that the child controller is
aborted and the child run is marked cancelled via the store mock (check the mock
that updates run status, e.g. ensure a call setting status 'cancelled' or
invoking the child-run cancellation path), and verify later child-layer nodes do
not start by inspecting createWorkflowEvent calls for absent 'node_started'
events for downstream steps. Ensure you reference executeDagWorkflow and the
workflow: node id used in the test so the test covers the workflow-node
cancellation propagation path.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ac918e23-2cca-4b1d-9b77-579c574fa0bf
📒 Files selected for processing (2)
packages/workflows/src/dag-executor.test.tspackages/workflows/src/dag-executor.ts
✅ Files skipped from review due to trivial changes (1)
- packages/workflows/src/dag-executor.ts
980dec2 to
244583a
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/workflows/src/dag-executor.test.ts`:
- Around line 5755-5764: The mock for createWorkflowRun currently drops
parent_conversation_id, so update the mock in createStatefulStore's
createWorkflowRun (the function named createWorkflowRun in the test mock) to
copy parent_conversation_id from the input (e.g., parent_conversation_id:
data.parent_conversation_id ?? null) so child-run context is preserved, and then
update/ensure the workflow-node tests assert that the stored run's
parent_conversation_id matches the input.
- Around line 5598-5631: The test currently asserts a completed workflow when an
unparseable loop_until is supplied; change it to assert a failed workflow and no
completion: in the test that calls executeDagWorkflow with loop_until: 'this is
not valid syntax' update assertions to expect store.failWorkflowRun (or the spy
created from createMockStore) toHaveBeenCalledTimes(1) and expect
store.completeWorkflowRun (completeSpy) toHaveBeenCalledTimes(0) (or removed),
keep the check that a warning message about 'loop_until' was sent via
platform.sendMessage, and restore/cleanup the spies; reference symbols:
executeDagWorkflow, loop_until, createMockStore / completeWorkflowRun,
failWorkflowRun, mockDeps, platform.sendMessage.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d38562f8-3263-40e3-b9d7-33fcd06a6bc7
📒 Files selected for processing (1)
packages/workflows/src/dag-executor.test.ts
| createWorkflowRun: mock((data: Parameters<IWorkflowStore['createWorkflowRun']>[0]) => | ||
| Promise.resolve({ | ||
| id: `run-${String(++nextRunSuffix)}-${data.workflow_name.replace(/\s/g, '-')}`, | ||
| workflow_name: data.workflow_name, | ||
| conversation_id: data.conversation_id, | ||
| parent_conversation_id: null, | ||
| codebase_id: data.codebase_id ?? null, | ||
| status: 'pending' as const, | ||
| user_message: data.user_message, | ||
| metadata: data.metadata ?? {}, |
There was a problem hiding this comment.
Preserve parent context in the child-run store mock.
createStatefulStore() currently discards parent_conversation_id, so workflow-node tests can pass even if child-run context inheritance regresses. Mirror the input data here and assert it in the workflow-node tests.
Proposed mock fix
createWorkflowRun: mock((data: Parameters<IWorkflowStore['createWorkflowRun']>[0]) =>
Promise.resolve({
id: `run-${String(++nextRunSuffix)}-${data.workflow_name.replace(/\s/g, '-')}`,
workflow_name: data.workflow_name,
conversation_id: data.conversation_id,
- parent_conversation_id: null,
+ parent_conversation_id: data.parent_conversation_id ?? null,
codebase_id: data.codebase_id ?? null,
status: 'pending' as const,
user_message: data.user_message,📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| createWorkflowRun: mock((data: Parameters<IWorkflowStore['createWorkflowRun']>[0]) => | |
| Promise.resolve({ | |
| id: `run-${String(++nextRunSuffix)}-${data.workflow_name.replace(/\s/g, '-')}`, | |
| workflow_name: data.workflow_name, | |
| conversation_id: data.conversation_id, | |
| parent_conversation_id: null, | |
| codebase_id: data.codebase_id ?? null, | |
| status: 'pending' as const, | |
| user_message: data.user_message, | |
| metadata: data.metadata ?? {}, | |
| createWorkflowRun: mock((data: Parameters<IWorkflowStore['createWorkflowRun']>[0]) => | |
| Promise.resolve({ | |
| id: `run-${String(++nextRunSuffix)}-${data.workflow_name.replace(/\s/g, '-')}`, | |
| workflow_name: data.workflow_name, | |
| conversation_id: data.conversation_id, | |
| parent_conversation_id: data.parent_conversation_id ?? null, | |
| codebase_id: data.codebase_id ?? null, | |
| status: 'pending' as const, | |
| user_message: data.user_message, | |
| metadata: data.metadata ?? {}, |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/workflows/src/dag-executor.test.ts` around lines 5755 - 5764, The
mock for createWorkflowRun currently drops parent_conversation_id, so update the
mock in createStatefulStore's createWorkflowRun (the function named
createWorkflowRun in the test mock) to copy parent_conversation_id from the
input (e.g., parent_conversation_id: data.parent_conversation_id ?? null) so
child-run context is preserved, and then update/ensure the workflow-node tests
assert that the stored run's parent_conversation_id matches the input.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/workflows/src/dag-executor.ts (1)
2335-2353: Redundant spread of workflow-level options onto child workflow.
childWorkflowLevelOptionsis constructed directly fromchildWorkflow(lines 2282–2288), so{ ...childWorkflow, ...childWorkflowLevelOptions }overwrites the same fields with their own values. You can passchildWorkflowdirectly —executeDagWorkflowalready pickseffort/thinking/fallbackModel/betas/sandboxoff the workflow argument viaWorkflowLevelOptions.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/workflows/src/dag-executor.ts` around lines 2335 - 2353, The call to executeDagWorkflow is redundantly merging childWorkflowLevelOptions onto childWorkflow causing a no-op overwrite; instead pass childWorkflow directly (remove the `{ ...childWorkflow, ...childWorkflowLevelOptions }` spread) since executeDagWorkflow itself extracts WorkflowLevelOptions (effort/thinking/fallbackModel/betas/sandbox) from the workflow argument; update the invocation to use childWorkflow and remove childWorkflowLevelOptions merging to avoid unnecessary duplication.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/workflows/src/dag-executor.ts`:
- Around line 2297-2330: The parent-status poll currently treats any
non-'running' status as cancellation; update the check inside the
parentCancelPoll (the async block using deps.store.getWorkflowRunStatus,
parentWorkflowRun.id, childRun.id, parentCancelController, updateWorkflowRun and
emitter.unregisterRun) so that it does not cancel the child when the parent is
'paused' — only treat explicit terminal/cancellation statuses (e.g., 'cancelled'
and other terminal statuses your system uses) as triggers to clearInterval, log
dag.workflow_node_parent_cancelled, updateWorkflowRun(childRun.id, { status:
'cancelled' }), unregister the run, and abort parentCancelController. Ensure the
condition replaces the broad parentStatus !== 'running' check with an explicit
check for cancellation/terminal statuses and ignores 'paused'.
---
Nitpick comments:
In `@packages/workflows/src/dag-executor.ts`:
- Around line 2335-2353: The call to executeDagWorkflow is redundantly merging
childWorkflowLevelOptions onto childWorkflow causing a no-op overwrite; instead
pass childWorkflow directly (remove the `{ ...childWorkflow,
...childWorkflowLevelOptions }` spread) since executeDagWorkflow itself extracts
WorkflowLevelOptions (effort/thinking/fallbackModel/betas/sandbox) from the
workflow argument; update the invocation to use childWorkflow and remove
childWorkflowLevelOptions merging to avoid unnecessary duplication.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7dba4de6-7785-4de7-9727-bf80fb27aa7a
📒 Files selected for processing (2)
packages/workflows/src/dag-executor.test.tspackages/workflows/src/dag-executor.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/workflows/src/dag-executor.test.ts
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
packages/workflows/src/dag-executor.ts (3)
2116-2449:⚠️ Potential issue | 🟡 MinorNo recursion depth guard or cycle detection for
workflow:nodes.
discoverWorkflowsWithConfigreturns every workflow in the repo, including the one currently running or an ancestor. A self-referencing workflow (workflow: AinsideA) or mutually referencing pair (A→B→A) will recurse until the Node stack blows, creating NWorkflowRunrows per level with no user-interpretable error.A minimal runtime guard — threading an invocation chain (array of invoked workflow names) or a depth counter through
executeDagWorkflowand failing fast whennode.workflowappears in the chain or depth exceeds a small cap (e.g. 8) — would make the failure mode predictable. Depth/chain could be passed as an additional optional parameter alongsideparentAbort.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/workflows/src/dag-executor.ts` around lines 2116 - 2449, The code lacks recursion/cycle detection for workflow: nodes leading to infinite recursion; update executeDagWorkflow and executeWorkflowNode to accept an optional invocationChain: string[] (or depth:number) parameter, propagate it when calling executeDagWorkflow from executeWorkflowNode, and before creating/starting a child run check if node.workflow already exists in invocationChain or if invocationChain.length >= 8 (cap) and fail fast with a clear error (send safeSendMessage, create node_failed event, return failed NodeOutput). Ensure executeWorkflowNode appends the child workflow name to the chain when recursing and that executeDagWorkflow uses the same parameter for further child calls so cycles and excessive depth are detected early.
2371-2449:⚠️ Potential issue | 🟡 MinorChild workflow cost still not aggregated into parent
total_cost_usd.
executeWorkflowNodereturnsNodeOutputwithoutcostUsd, so the parent'stotalCostUsd += output.costUsdaccumulator at Line 3009 skips child cost entirely. For composed workflows where the child does the bulk of the AI work the parent'stotal_cost_usdwill be materially understated.Simplest fix: after the child completes successfully, fetch the child's persisted
total_cost_usdviadeps.storeand return it ascostUsdon the parent node result (and widen the return type toNodeExecutionResultlike sibling executors).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/workflows/src/dag-executor.ts` around lines 2371 - 2449, executeWorkflowNode currently returns NodeOutput without costUsd so parent totalCostUsd misses child workflow costs; after detecting childFinalStatus === 'completed' call deps.store.getWorkflowRun(childRun.id) (or the store method that returns the persisted workflow run) and read its total_cost_usd, set output.costUsd = childTotalCost (or 0 if missing), widen the function's return type from NodeOutput to NodeExecutionResult (or include costUsd in the returned shape), and ensure the caller's accumulator (totalCostUsd += output.costUsd) will now pick up the child's cost; also handle and log any errors when fetching the child run and default cost to 0 on failure.
3162-3200:⚠️ Potential issue | 🟡 Minor
loop_untilparse failure and exhaustion still complete the run as success.Both the unparseable-expression branch (Line 3165) and the max-iteration exhaustion branch (Lines 3193-3199)
breakout and fall through tocompleteWorkflowRun(...)at Line 3207, so the run is markedcompleted. Node-level loops take the opposite stance — they returnstate: 'failed'on exhaustion (Line 1933).For a
loop_until-driven pattern (e.g. test→fix→retest) the caller almost always needs a distinguishable non-success state so CI/automation can gate on "never passed". Either fail the run in these branches, or at minimum persist a reason in run metadata (e.g.loop_until_parse_error,loop_until_max_iterations_exceeded) so UI/consumers can tell "gate passed" from "ran out of iterations".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/workflows/src/dag-executor.ts` around lines 3162 - 3200, The loop_until unparseable branch and max-iteration branch currently break and let the run be marked completed; update these branches to record a failing outcome instead: when evaluateCondition returns parsed=false or when maxIterations is reached, call the same failure-completion path used for node-level exhaustion (set the WorkflowRun status to failed and include a clear metadata key like loop_until_parse_error or loop_until_max_iterations_exceeded and a message), or invoke completeWorkflowRun/finishRun with a failure result and the metadata; locate the logic around workflow.loop_until, evaluateCondition, safeSendMessage and completeWorkflowRun in dag-executor.ts and replace the final break/warn+break with code that persists the failure state and reason so consumers can distinguish “never satisfied” from success.
🧹 Nitpick comments (2)
packages/workflows/src/dag-executor.ts (2)
2187-2201: Discovery runs from disk on everyworkflow:node invocation (including every loop iteration).
discoverWorkflowsWithConfig(cwd, deps.loadConfig)walks the filesystem each time this node fires. In aloop_untilworkflow that invokes a child viaworkflow:this cost multiplies by iteration count, and the result is effectively invariant for the duration of the parent run.Not a correctness bug — just worth caching the discovery result on the parent run (e.g. memoize on
parentWorkflowRun.idor pass a pre-computed workflow map throughexecuteDagWorkflow) so composed/iterative workflows don't pay the I/O tax repeatedly.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/workflows/src/dag-executor.ts` around lines 2187 - 2201, The discovery call discoverWorkflowsWithConfig is being invoked on every workflow node execution (including each loop iteration) causing repeated disk I/O; modify dag execution to cache the discovery result for the lifetime of the parent run (e.g., memoize by parentWorkflowRun.id or compute once and pass a pre-built workflow map into executeDagWorkflow), then replace direct calls to discoverWorkflowsWithConfig in the workflow node handling (where childWorkflow is resolved) to use the cached map instead; reference the discoverWorkflowsWithConfig call and the childWorkflow lookup in dag-executor.ts and add a cache parameter or module-level memo keyed by parentWorkflowRun.id so subsequent node executions reuse the discovered workflows.
2332-2353: Redundant spread ofchildWorkflowLevelOptionsoverchildWorkflow.
childWorkflowis aWorkflowDefinitionthat already carrieseffort/thinking/fallbackModel/betas/sandbox, andchildWorkflowLevelOptionsis derived from those same fields at Lines 2282-2288. The{ ...childWorkflow, ...childWorkflowLevelOptions }spread just re-applies identical values. You can passchildWorkflowdirectly.♻️ Proposed simplification
- childTerminalOutput = await executeDagWorkflow( - deps, - platform, - conversationId, - cwd, - { ...childWorkflow, ...childWorkflowLevelOptions }, + childTerminalOutput = await executeDagWorkflow( + deps, + platform, + conversationId, + cwd, + childWorkflow,(and delete the now-unused
childWorkflowLevelOptionsblock at Lines 2282-2288)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/workflows/src/dag-executor.ts` around lines 2332 - 2353, The call to executeDagWorkflow is redundantly spreading childWorkflowLevelOptions over childWorkflow; remove the spread and pass childWorkflow directly as the workflow argument (replace "{ ...childWorkflow, ...childWorkflowLevelOptions }" with "childWorkflow"), and delete the now-unused childWorkflowLevelOptions construction (the block that creates childWorkflowLevelOptions based on effort/thinking/fallbackModel/betas/sandbox) so there are no leftover unused variables; verify callers use executeDagWorkflow(workflow: childWorkflow) as before.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/workflows/src/dag-executor.ts`:
- Around line 2489-2493: The current maxIterations calculation uses 1 when
workflow.loop_until is present but workflow.max_iterations is omitted, causing
an unintended single-pass cap; change the logic in the DAG executor where
maxIterations is computed (the variable maxIterations that references
workflow.loop_until and workflow.max_iterations) so that if loop_until is
defined and max_iterations is undefined you set a safer default (e.g., 10)
instead of 1, e.g. compute maxIterations = workflow.loop_until !== undefined ?
Math.max(1, workflow.max_iterations ?? 10) : 1; ensure any user-facing warning
string that references the max-iterations cap still works with the new default
and update the inline comment to reflect the new behavior.
---
Duplicate comments:
In `@packages/workflows/src/dag-executor.ts`:
- Around line 2116-2449: The code lacks recursion/cycle detection for workflow:
nodes leading to infinite recursion; update executeDagWorkflow and
executeWorkflowNode to accept an optional invocationChain: string[] (or
depth:number) parameter, propagate it when calling executeDagWorkflow from
executeWorkflowNode, and before creating/starting a child run check if
node.workflow already exists in invocationChain or if invocationChain.length >=
8 (cap) and fail fast with a clear error (send safeSendMessage, create
node_failed event, return failed NodeOutput). Ensure executeWorkflowNode appends
the child workflow name to the chain when recursing and that executeDagWorkflow
uses the same parameter for further child calls so cycles and excessive depth
are detected early.
- Around line 2371-2449: executeWorkflowNode currently returns NodeOutput
without costUsd so parent totalCostUsd misses child workflow costs; after
detecting childFinalStatus === 'completed' call
deps.store.getWorkflowRun(childRun.id) (or the store method that returns the
persisted workflow run) and read its total_cost_usd, set output.costUsd =
childTotalCost (or 0 if missing), widen the function's return type from
NodeOutput to NodeExecutionResult (or include costUsd in the returned shape),
and ensure the caller's accumulator (totalCostUsd += output.costUsd) will now
pick up the child's cost; also handle and log any errors when fetching the child
run and default cost to 0 on failure.
- Around line 3162-3200: The loop_until unparseable branch and max-iteration
branch currently break and let the run be marked completed; update these
branches to record a failing outcome instead: when evaluateCondition returns
parsed=false or when maxIterations is reached, call the same failure-completion
path used for node-level exhaustion (set the WorkflowRun status to failed and
include a clear metadata key like loop_until_parse_error or
loop_until_max_iterations_exceeded and a message), or invoke
completeWorkflowRun/finishRun with a failure result and the metadata; locate the
logic around workflow.loop_until, evaluateCondition, safeSendMessage and
completeWorkflowRun in dag-executor.ts and replace the final break/warn+break
with code that persists the failure state and reason so consumers can
distinguish “never satisfied” from success.
---
Nitpick comments:
In `@packages/workflows/src/dag-executor.ts`:
- Around line 2187-2201: The discovery call discoverWorkflowsWithConfig is being
invoked on every workflow node execution (including each loop iteration) causing
repeated disk I/O; modify dag execution to cache the discovery result for the
lifetime of the parent run (e.g., memoize by parentWorkflowRun.id or compute
once and pass a pre-built workflow map into executeDagWorkflow), then replace
direct calls to discoverWorkflowsWithConfig in the workflow node handling (where
childWorkflow is resolved) to use the cached map instead; reference the
discoverWorkflowsWithConfig call and the childWorkflow lookup in dag-executor.ts
and add a cache parameter or module-level memo keyed by parentWorkflowRun.id so
subsequent node executions reuse the discovered workflows.
- Around line 2332-2353: The call to executeDagWorkflow is redundantly spreading
childWorkflowLevelOptions over childWorkflow; remove the spread and pass
childWorkflow directly as the workflow argument (replace "{ ...childWorkflow,
...childWorkflowLevelOptions }" with "childWorkflow"), and delete the now-unused
childWorkflowLevelOptions construction (the block that creates
childWorkflowLevelOptions based on effort/thinking/fallbackModel/betas/sandbox)
so there are no leftover unused variables; verify callers use
executeDagWorkflow(workflow: childWorkflow) as before.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f302228f-498c-42b6-9854-b55ac34c73bb
📒 Files selected for processing (1)
packages/workflows/src/dag-executor.ts
…te child cost, fail loop on exhaustion - Default max_iterations to 10 (was 1) when loop_until is set but max_iterations is omitted - Add invocationChain to executeDagWorkflow/executeWorkflowNode to detect cycles and cap nesting at depth 8 - Pre-discover child workflows once per DAG run to avoid repeated disk I/O in workflow nodes - Remove redundant childWorkflowLevelOptions spread — pass childWorkflow directly to executeDagWorkflow - Propagate child run total_cost_usd into parent cost aggregation via getWorkflowRun on completion - Replace silent break on loop_until parse error / max-iterations exhaustion with explicit run failure (updateWorkflowRun status=failed with metadata keys loop_until_parse_error / loop_until_max_iterations_exceeded)
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/workflows/src/dag-executor.ts (1)
2588-3264:⚠️ Potential issue | 🟡 MinorAdd workflow-level iteration lifecycle events for
loop_until.The workflow-level loop logs and sends chat messages but does not emit SSE or persisted events for iteration start/completion/failure. Event consumers cannot observe iteration progress.
Emit
loop_iteration_started,loop_iteration_completed, andloop_iteration_failedevents (matching the node-level loop pattern inexecuteLoopNode). Since this is workflow-level iteration (no node context), omitstep_namebut includeiteration,maxIterations, andloop_untilin the event data.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/workflows/src/dag-executor.ts` around lines 2588 - 3264, Add persisted and SSE events for workflow-level loop iterations: when handling workflow.loop_until in the loop around evaluateCondition (use the existing variables iteration and maxIterations), call deps.store.createWorkflowEvent with event_type values 'loop_iteration_started', 'loop_iteration_completed', and 'loop_iteration_failed' (include data { iteration, maxIterations, loop_until: workflow.loop_until, ... } and do not set step_name), and also emit matching SSE via getWorkflowEventEmitter().emit with type 'loop_iteration_started'/'loop_iteration_completed'/'loop_iteration_failed' and the same data (include workflowRun.id for run context); emit 'loop_iteration_started' at the start of each iteration before evaluating loop_until, emit 'loop_iteration_completed' when the condition is satisfied and you break, and emit 'loop_iteration_failed' both for an unparseable expression (where loopFailureMeta is set) and when maxIterations is exceeded (where loopFailureMeta indicates max_iterations); follow the existing error handling pattern (catch createWorkflowEvent errors and log with getLog().error) and mirror payload fields used by executeLoopNode events (iteration, maxIterations, loop_until) without step_name.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/workflows/src/dag-executor.ts`:
- Around line 2393-2485: The code treats childFinalStatus === 'paused' as a
failure which orphanes paused child workflows; update the branch after checking
childFinalStatus to explicitly handle the 'paused' case: when childFinalStatus
=== 'paused' call logNodeError (or add a logNodePaused helper) to record the
pause, create a workflow event via deps.store.createWorkflowEvent with
event_type 'node_paused' and data including child_run_id and child_workflow,
call getLog().info/warn with { nodeId: node.id, childRunId: childRun.id }, emit
an emitter event with type 'node_paused' (including runId: parentWorkflowRun.id,
nodeId, nodeName:`workflow:${node.workflow}`, childRunId), and return a paused
result (e.g., { state: 'paused', output: '', childRunId: childRun.id }) instead
of treating it as failed so the parent can be resumed when the child finishes.
- Around line 3272-3279: The updateWorkflowRun call that sets status to 'failed'
currently swallows DB errors in the catch and only logs them; change it to
ensure persistence failures are surfaced: call the same terminal failure helper
used elsewhere (or rethrow the error after logging) instead of silently
continuing, and validate the update result (check rowCount/affectedRows from
deps.store.updateWorkflowRun) to detect missing rows and throw if zero;
updateWorkflowRun, workflowRun.id, and getLog() are the key symbols to modify so
the caller can observe persistence errors rather than leaving the run stuck in
'running'.
- Around line 2322-2351: The parent-cancel polling can still let an in-flight
callback overwrite a finished child run; guard the async callback with
parentCancelController.signal (check signal.aborted at start and right before
any state change) and bail out if aborted, and change the update path that
currently calls deps.store.updateWorkflowRun(childRun.id, { status: 'cancelled'
}) so it only proceeds when the child's current status matches an expected
pre-cancel state (e.g., running/paused) and verifies the store response
(rowCount or returned row) to ensure the update actually occurred before calling
emitter.unregisterRun(childRun.id) and parentCancelController.abort(); in short,
use parentCancelController.signal checks to prevent late callbacks and make
deps.store.updateWorkflowRun conditional/verified so it cannot overwrite a
completed child run.
---
Outside diff comments:
In `@packages/workflows/src/dag-executor.ts`:
- Around line 2588-3264: Add persisted and SSE events for workflow-level loop
iterations: when handling workflow.loop_until in the loop around
evaluateCondition (use the existing variables iteration and maxIterations), call
deps.store.createWorkflowEvent with event_type values 'loop_iteration_started',
'loop_iteration_completed', and 'loop_iteration_failed' (include data {
iteration, maxIterations, loop_until: workflow.loop_until, ... } and do not set
step_name), and also emit matching SSE via getWorkflowEventEmitter().emit with
type 'loop_iteration_started'/'loop_iteration_completed'/'loop_iteration_failed'
and the same data (include workflowRun.id for run context); emit
'loop_iteration_started' at the start of each iteration before evaluating
loop_until, emit 'loop_iteration_completed' when the condition is satisfied and
you break, and emit 'loop_iteration_failed' both for an unparseable expression
(where loopFailureMeta is set) and when maxIterations is exceeded (where
loopFailureMeta indicates max_iterations); follow the existing error handling
pattern (catch createWorkflowEvent errors and log with getLog().error) and
mirror payload fields used by executeLoopNode events (iteration, maxIterations,
loop_until) without step_name.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: aa24c3c8-04ec-40fb-a8d0-418c27037d21
📒 Files selected for processing (2)
packages/workflows/src/dag-executor.test.tspackages/workflows/src/dag-executor.ts
| const parentCancelController = new AbortController(); | ||
| const parentCancelPoll = setInterval(() => { | ||
| void (async (): Promise<void> => { | ||
| try { | ||
| const parentStatus = await deps.store.getWorkflowRunStatus(parentWorkflowRun.id); | ||
| if (parentStatus !== 'running' && parentStatus !== 'paused') { | ||
| clearInterval(parentCancelPoll); | ||
| getLog().info( | ||
| { parentRunId: parentWorkflowRun.id, parentStatus, childRunId: childRun.id }, | ||
| 'dag.workflow_node_parent_cancelled' | ||
| ); | ||
| await deps.store | ||
| .updateWorkflowRun(childRun.id, { status: 'cancelled' }) | ||
| .catch((e: Error) => { | ||
| getLog().warn( | ||
| { err: e, childRunId: childRun.id }, | ||
| 'dag.workflow_node_cancel_child_failed' | ||
| ); | ||
| }); | ||
| emitter.unregisterRun(childRun.id); | ||
| parentCancelController.abort(); | ||
| } | ||
| } catch (e) { | ||
| getLog().warn( | ||
| { err: e as Error, parentRunId: parentWorkflowRun.id }, | ||
| 'dag.workflow_node_parent_status_poll_failed' | ||
| ); | ||
| } | ||
| })(); | ||
| }, 5000); |
There was a problem hiding this comment.
❓ Verification inconclusive
Script executed:
#!/bin/bash
# Description: Show the parent cancellation poll and cleanup block for manual verification.
rg -n -C 8 'parentCancelPoll|parentCancelController|childExecutionFinished|clearInterval\(parentCancelPoll\)' packages/workflows/src/dag-executor.tsRepository: coleam00/Archon
Repository: coleam00/Archon
Exit code: 0
stdout:
2314- `↳ Running child workflow \`${childWorkflow.name}\` (node \`${node.id}\`)`,
2315- nodeContext
2316- );
2317-
2318- // Propagate parent cancellation into the child run via a poll + AbortController.
2319- // When the parent run leaves 'running', we mark the child 'cancelled' in the DB
2320- // so that the child's own skipIfStatusChanged guard picks it up at the next layer
2321- // boundary, and we signal the controller so the pre-layer check exits immediately.
2322: const parentCancelController = new AbortController();
2323: const parentCancelPoll = setInterval(() => {
2324- void (async (): Promise<void> => {
2325- try {
2326- const parentStatus = await deps.store.getWorkflowRunStatus(parentWorkflowRun.id);
2327- if (parentStatus !== 'running' && parentStatus !== 'paused') {
2328: clearInterval(parentCancelPoll);
2329- getLog().info(
2330- { parentRunId: parentWorkflowRun.id, parentStatus, childRunId: childRun.id },
2331- 'dag.workflow_node_parent_cancelled'
2332- );
2333- await deps.store
2334- .updateWorkflowRun(childRun.id, { status: 'cancelled' })
2335- .catch((e: Error) => {
2336- getLog().warn(
2337- { err: e, childRunId: childRun.id },
2338- 'dag.workflow_node_cancel_child_failed'
2339- );
2340- });
2341- emitter.unregisterRun(childRun.id);
2342: parentCancelController.abort();
2343- }
2344- } catch (e) {
2345- getLog().warn(
2346- { err: e as Error, parentRunId: parentWorkflowRun.id },
2347- 'dag.workflow_node_parent_status_poll_failed'
2348- );
2349- }
2350- })();
--
2365- childArtifactsDir,
2366- logDir,
2367- baseBranch,
2368- docsDir,
2369- config,
2370- configuredCommandFolder,
2371- issueContext,
2372- undefined, // no priorCompletedNodes — child runs are always fresh
2373: parentCancelController.signal,
2374- [...invocationChain, node.workflow]
2375- );
2376- } catch (error) {
2377- const err = error as Error;
2378- const errMsg = `Node '${node.id}': child workflow '${node.workflow}' threw unexpectedly: ${err.message}`;
2379- getLog().error(
2380- { err, nodeId: node.id, childRunId: childRun.id },
2381- 'dag.workflow_node_child_threw'
2382- );
2383- await deps.store.failWorkflowRun(childRun.id, err.message).catch((_e: Error) => {
2384- getLog().warn({ childRunId: childRun.id }, 'dag.workflow_node_fail_cleanup_failed');
2385- });
2386- emitter.unregisterRun(childRun.id);
2387- await safeSendMessage(platform, conversationId, errMsg, nodeContext);
2388- return { state: 'failed', output: '', error: errMsg };
2389- } finally {
2390: clearInterval(parentCancelPoll);
2391- }
2392-
2393- // Check final status from DB to determine success vs failure
2394- let childFinalStatus: WorkflowRunStatus | null = null;
2395- try {
2396- childFinalStatus = await deps.store.getWorkflowRunStatus(childRun.id);
2397- } catch (error) {
2398- getLog().error(
Guard the parent-cancel poll against late callbacks overwriting completed child run.
clearInterval(parentCancelPoll) does not stop an already-running async callback. A callback that started before the finally block executes can still complete and update the child to cancelled after the child has already finished with a different status, violating autonomous lifecycle mutation during multi-process execution.
Additionally, the updateWorkflowRun call (line 2334) does not verify rowCount or validate the child's pre-update status before marking it cancelled.
Suggested fix
+ let childExecutionFinished = false;
const parentCancelController = new AbortController();
const parentCancelPoll = setInterval(() => {
void (async (): Promise<void> => {
try {
+ if (childExecutionFinished) return;
const parentStatus = await deps.store.getWorkflowRunStatus(parentWorkflowRun.id);
+ if (childExecutionFinished) return;
if (parentStatus !== 'running' && parentStatus !== 'paused') {
clearInterval(parentCancelPoll);
+ const childStatus = await deps.store.getWorkflowRunStatus(childRun.id);
+ if (childExecutionFinished || childStatus !== 'running') return;
getLog().info(
{ parentRunId: parentWorkflowRun.id, parentStatus, childRunId: childRun.id },
'dag.workflow_node_parent_cancelled'
);
await deps.store
.updateWorkflowRun(childRun.id, { status: 'cancelled' })
- .catch((e: Error) => {
- getLog().warn(
- { err: e, childRunId: childRun.id },
- 'dag.workflow_node_cancel_child_failed'
- );
- });
+ .then(result => {
+ if (result.rowCount === 0) {
+ getLog().warn(
+ { childRunId: childRun.id },
+ 'dag.workflow_node_cancel_child_already_terminal'
+ );
+ }
+ })
+ .catch((e: Error) => {
+ throw new Error(`Failed to cancel child run: ${e.message}`);
+ }); } finally {
+ childExecutionFinished = true;
clearInterval(parentCancelPoll);
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/workflows/src/dag-executor.ts` around lines 2322 - 2351, The
parent-cancel polling can still let an in-flight callback overwrite a finished
child run; guard the async callback with parentCancelController.signal (check
signal.aborted at start and right before any state change) and bail out if
aborted, and change the update path that currently calls
deps.store.updateWorkflowRun(childRun.id, { status: 'cancelled' }) so it only
proceeds when the child's current status matches an expected pre-cancel state
(e.g., running/paused) and verifies the store response (rowCount or returned
row) to ensure the update actually occurred before calling
emitter.unregisterRun(childRun.id) and parentCancelController.abort(); in short,
use parentCancelController.signal checks to prevent late callbacks and make
deps.store.updateWorkflowRun conditional/verified so it cannot overwrite a
completed child run.
| // Check final status from DB to determine success vs failure | ||
| let childFinalStatus: WorkflowRunStatus | null = null; | ||
| try { | ||
| childFinalStatus = await deps.store.getWorkflowRunStatus(childRun.id); | ||
| } catch (error) { | ||
| getLog().error( | ||
| { err: error as Error, childRunId: childRun.id }, | ||
| 'dag.workflow_node_status_check_failed' | ||
| ); | ||
| } | ||
|
|
||
| const duration = Date.now() - nodeStartTime; | ||
|
|
||
| if (childFinalStatus === 'completed') { | ||
| const output = childTerminalOutput ?? ''; | ||
|
|
||
| // Propagate child run's accumulated cost into the parent's cost aggregation. | ||
| let childCostUsd: number | undefined; | ||
| try { | ||
| const childRunRecord = await deps.store.getWorkflowRun(childRun.id); | ||
| const rawCost = childRunRecord?.metadata?.total_cost_usd; | ||
| childCostUsd = typeof rawCost === 'number' ? rawCost : undefined; | ||
| } catch (err) { | ||
| getLog().warn({ err, childRunId: childRun.id }, 'dag.workflow_node_cost_fetch_failed'); | ||
| } | ||
|
|
||
| getLog().info( | ||
| { nodeId: node.id, childRunId: childRun.id, durationMs: duration }, | ||
| 'dag.workflow_node_completed' | ||
| ); | ||
| await logNodeComplete(logDir, parentWorkflowRun.id, node.id, `workflow:${node.workflow}`, { | ||
| durationMs: duration, | ||
| }); | ||
| deps.store | ||
| .createWorkflowEvent({ | ||
| workflow_run_id: parentWorkflowRun.id, | ||
| event_type: 'node_completed', | ||
| step_name: node.id, | ||
| data: { | ||
| duration_ms: duration, | ||
| node_output: output, | ||
| child_run_id: childRun.id, | ||
| child_workflow: node.workflow, | ||
| }, | ||
| }) | ||
| .catch((err: Error) => { | ||
| getLog().error( | ||
| { err, workflowRunId: parentWorkflowRun.id, eventType: 'node_completed' }, | ||
| 'workflow_event_persist_failed' | ||
| ); | ||
| }); | ||
| emitter.emit({ | ||
| type: 'node_completed', | ||
| runId: parentWorkflowRun.id, | ||
| nodeId: node.id, | ||
| nodeName: `workflow:${node.workflow}`, | ||
| duration, | ||
| }); | ||
| return { | ||
| state: 'completed', | ||
| output, | ||
| ...(childCostUsd !== undefined ? { costUsd: childCostUsd } : {}), | ||
| }; | ||
| } | ||
|
|
||
| // Child failed, was cancelled, or status is unknown | ||
| const errMsg = `Node '${node.id}': child workflow '${node.workflow}' did not complete (status: ${childFinalStatus ?? 'unknown'})`; | ||
| getLog().error( | ||
| { nodeId: node.id, childRunId: childRun.id, childStatus: childFinalStatus }, | ||
| 'dag.workflow_node_child_failed' | ||
| ); | ||
| await logNodeError(logDir, parentWorkflowRun.id, node.id, errMsg); | ||
| deps.store | ||
| .createWorkflowEvent({ | ||
| workflow_run_id: parentWorkflowRun.id, | ||
| event_type: 'node_failed', | ||
| step_name: node.id, | ||
| data: { error: errMsg, child_run_id: childRun.id, child_workflow: node.workflow }, | ||
| }) | ||
| .catch((err: Error) => { | ||
| getLog().error( | ||
| { err, workflowRunId: parentWorkflowRun.id, eventType: 'node_failed' }, | ||
| 'workflow_event_persist_failed' | ||
| ); | ||
| }); | ||
| emitter.emit({ | ||
| type: 'node_failed', | ||
| runId: parentWorkflowRun.id, | ||
| nodeId: node.id, | ||
| nodeName: `workflow:${node.workflow}`, | ||
| error: errMsg, | ||
| }); | ||
| return { state: 'failed', output: '', error: errMsg }; |
There was a problem hiding this comment.
Handle child workflows that pause instead of failing the parent.
A child workflow containing an approval gate or interactive loop can leave childFinalStatus === 'paused'. This branch currently treats that as a failed workflow node, leaving the child paused/orphaned while the parent fails. Either propagate the pause to the parent and resume it when the child finishes, or reject pausing child workflows before creating the child run with a clear unsupported-state error.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/workflows/src/dag-executor.ts` around lines 2393 - 2485, The code
treats childFinalStatus === 'paused' as a failure which orphanes paused child
workflows; update the branch after checking childFinalStatus to explicitly
handle the 'paused' case: when childFinalStatus === 'paused' call logNodeError
(or add a logNodePaused helper) to record the pause, create a workflow event via
deps.store.createWorkflowEvent with event_type 'node_paused' and data including
child_run_id and child_workflow, call getLog().info/warn with { nodeId: node.id,
childRunId: childRun.id }, emit an emitter event with type 'node_paused'
(including runId: parentWorkflowRun.id, nodeId,
nodeName:`workflow:${node.workflow}`, childRunId), and return a paused result
(e.g., { state: 'paused', output: '', childRunId: childRun.id }) instead of
treating it as failed so the parent can be resumed when the child finishes.
|
@Dev-Force related to #1238 — overlapping area or partial fix. |
|
@Dev-Force related to #1286 — overlapping area or partial fix. |
…l loop iteration events - Handle childFinalStatus === 'paused' explicitly in executeWorkflowNode: emit node_paused store event + SSE instead of lumping it with failures - Guard parent-cancel polling callback with signal.aborted checks to prevent late-firing interval from overwriting a completed child run - Emit loop_iteration_started/completed/failed events for workflow-level loop_until loops (mirrors the existing executeLoopNode pattern) - Add NodePausedEvent to WorkflowEmitterEvent and node_paused to WORKFLOW_EVENT_TYPES; handle node_paused in workflow-bridge SSE switch
|
Hello @Dev-Force @Wirasm , may I know what's the remaining action items for this? I would like to help get this done because we kind of need it. |
I think im mostly done. Im not aware of the process the owners approve the changes. |
|
🔗 Related issue: This PR may also be related to issue #1238 ("Feature request: inline composite loops for multi-node iteration"). The loop composition groundwork added here is relevant to the multi-node iteration use case described in that issue. |
|
Btw, if anybody is looking for this functionality (Sub-workflow or nested loop). Try https://github.com/dagucloud/dagu, it worked for me. |
|
i apologize for the wait, we have a lot of pr's and im trying to work through them, checking this now |
Review SummaryVerdict: minor-fixes-needed This PR adds workflow composition — Blocking issues
Suggested fixes
Minor / nice-to-have
Compliments
Reviewed via maintainer-review-pr workflow (Pi/Minimax). Aspects run: code-review, error-handling, test-coverage, comment-quality, docs-impact. |
Maintainer note (follow-up to the Pi review above)Thanks @Dev-Force — workflow composition is a direction I want to land. A few clarifications and corrections on the Pi review you just got, plus rebase guidance: Severity calibration. The Pi-generated comment above is labelled On Pi's CRITICAL #1 (loop node provider resolution, On the suspected "workflow node dispatch missing" remark in Pi's error-handling reasoning. I verified locally — Rebase scope (heads up). This PR is 106 commits behind
Must-fix gates before re-review:
Mediums are negotiable in scope but the docs entries (CHANGELOG, two new bundled workflows, Ping me when the rebase is up — happy to re-run the maintainer review. |
Review SummaryVerdict: minor-fixes-needed This is a strong PR that adds workflow composition — child Blocking issues
Suggested fixes
Minor / nice-to-haveTests (run
Docs (new public surface needs coverage before users can discover it):
Small polish:
ComplimentsThe expression evaluator test additions (18 new cases covering numeric/boolean RHS, shorthand paths, nested sub-segments, and JSON parse failures) are comprehensive and well-structured — a good model for future tests. The abort signal propagation comment in |
Review SummaryVerdict: minor-fixes-needed This is a large, well-structured PR adding child workflow composition ( Blocking issues
Suggested fixes
Minor / nice-to-have
Compliments
Reviewed via maintainer-review-pr workflow (Pi/Minimax). Aspects run: code-review, error-handling, test-coverage, comment-quality, docs-impact. |
|
Closing to land the 3 features independently — your design for each is solid, the bundling is the blocker (1190/-499 in I've filed 3 focused issues that carry your design forward and credit you in the references:
@Dev-Force, please feel free to pick up any of these. One PR per issue — we want to prove one pattern at a time before stacking. #1763 is the lowest-risk starting point if you want a fast first land. Ping me on the issue you start with and I'll watch the PR closely. Closing this PR isn't a rejection of the work — it's just that a single 2421-LOC change touching the load-bearing DAG executor is too much to land atomically given the executor's blast radius. Three small wins read better than one big risk. |
|
That's a better approach, to be honest, and will bring the features in a more controlled way. |
Summary
loop:node) iterates a single prompt, not a multi-step graph.when:conditions used substring matching rather than real expression evaluation.when:matching; (2) workflow-levelloop_until+max_iterationsthat re-runs the full DAG until a condition is met; (3)workflow:node type that invokes a named child workflow as a single DAG node with its output flowing downstream; (4) two bundled default workflows demonstrating composition.loop:primitive (PR 🐛 [Bug]: can get list of projects #785) is untouched and orthogonal. DAG cycle detection, executor control flow,IWorkflowStore, platform adapters, and database schema are all unchanged.UX Journey
Before
After
Architecture Diagram
Before
After
Connection inventory:
dag-executor.tscondition-evaluator.tswhen:now callsevaluateCondition()instead of substring matchdag-executor.tsdag-executor.ts(recursive)executeWorkflowNodecallsexecuteDagWorkflowfor child workflowdag-executor.tsworkflow-discovery.tsdiscoverWorkflowsWithConfigdag-executor.tsIWorkflowStoreWorkflowRunrecords perworkflow:nodeloader.tsschemas/dag-node.tsisWorkflowNodenow used for node-type classification andinput:ref validationschemas/workflow.tsschemas/workflow.tsloop_until,max_iterationsfieldsschemas/dag-node.tsschemas/dag-node.tsworkflowNodeSchema,WorkflowNode,isWorkflowNodeschemas/index.tsschemas/dag-node.tsWorkflowNode,isWorkflowNodedefaults/bundled-defaults.generated.tsloader.ts→IWorkflowStoredag-executor.ts→IWorkflowPlatformLabel Snapshot
risk: mediumsize: Lworkflowsworkflows:executor,workflows:schemas,workflows:loader,workflows:condition-evaluatorChange Metadata
featureworkflowsLinked Issue
loop:— complementary, unchanged)Validation Evidence (required)
All five checks passed:
check:bundled— bundled defaults match filesystem ✓type-check— all 10 packages clean ✓lint— 0 warnings, 0 errors ✓format:check— clean ✓test— all packages green (per-package isolation, no cross-file mock pollution) ✓New test coverage added:
condition-evaluator.test.ts— 138 lines, covers all operators, compound AND/OR, path access, unquoted literals, fail-closed on bad expressionsdag-executor.test.ts— 322 lines for workflow composition (child invocation, output propagation, error paths,input:substitution); 343 lines forloop_until/max_iterations(condition met, max exhausted, fresh scope per iteration, unparseable expression)Evidence provided:
bun run validateexit 0, test counts verifiedNo commands skipped
Security Impact (required)
workflow:nodes can invoke any named workflow discoverable on the filesystem at execution time. This is equivalent to the existingcommand:node behavior (also filesystem-discovered). No escalation beyond what already exists.cwdand isolation environment as the parent; no new paths are opened.Compatibility / Migration
loop_until,max_iterations,workflow:node) are optional. Existing workflows parse and run unchanged.WorkflowRunrecords reuse the existingworkflow_runs+workflow_eventsschema with no new columns.Human Verification (required)
condition-evaluator.tsunit-tested for all operators and edge casesdag-executorunit tests forloop_until(condition met early, max exhausted, unparseable expr) andworkflow:node (success path, child not found, input substitution, failed child propagation)archon-compose-plan-implement-qa,archon-compose-planner) parsed successfully by the loader and bundled correctlybun run validatepasses end-to-endloop_untilwithmax_iterations: 1(single pass, condition never evaluated for re-run)loop_untilexpression that can't be parsed → fail-closed, user-visible warning, stops gracefullyworkflow:node referencing a non-existent child →failedstate with clear error messageinput:field with$nodeId.outputrefs that fail substitution →failedstate, not a crashworkflow:node cycle at runtime (not caught at load time — would recurse until run is cancelled externally)Side Effects / Blast Radius (required)
@archon/workflowsonly. No changes to@archon/core, adapters, server, web, CLI, or database.workflow:nodes create childWorkflowRunDB records — each invocation is visible in the runs list. Users may see more rows in workflow run history for composite workflows.loop_untilcan make a workflow run significantly longer (up tomax_iterationsfull DAG passes). Users can still cancel runs via/workflow cancelor the web UI.loop_untilexpression that always evaluates false will run the full DAGmax_iterationstimes, incurring full AI cost per pass. User is warned at exhaustion.max_iterationsdefaults to 1 ifloop_untilis set without an explicit cap; user receives an in-chat warning on both unparseable expressions and iteration exhaustion.Rollback Plan (required)
git revertthe 4 commits on this branch; no DB migration to reverse.loop_until/workflow:nodes — they are opt-in.dag.workflow_node_not_foundordag.workflow_node_child_threwlog events; childWorkflowRunrecords stuck infailedstate; user sees error message in chat for the failing node.Risks and Mitigations
Risk: Unbounded recursion via
workflow: selfor A→B→A cross-workflow cycles (not detected at load time).max_iterations(defaulting to 1 if not set) and stop with a user-visible warning. A future follow-up can add static cross-workflow cycle detection in the loader. No infinite loop is possible because each child is a separate run with its own iteration cap.Risk: Child workflow cost is not rolled up to the parent run's
total_cost_usdfield.WorkflowRunrecords its own cost. This is a known accounting gap; the parent's cost reflects only its own direct AI nodes.Summary by CodeRabbit
New Features
Tests
Validation