fix(workflows): distinguish until_bash system errors from condition-false (#1241)

[shaun0927]

Summary

• Problem: The until_bash catch block at packages/workflows/src/dag-executor.ts:1772-1781 treated every error — ENOENT, EACCES, timeout/SIGTERM, and legitimate non-zero exit — identically as bashComplete = false. Misconfigured predicates (e.g. gh not on PATH → exit 127) silently spun to max_iterations, burning an LLM call per iteration with no warning. Additionally, the existing ENOENT check was nearly inert: a missing command surfaces as bash's exit 127, not Node's errno ENOENT — so the warn path almost never fired.
• Why it matters: Silent cost runaway on operator misconfiguration. Users had no actionable signal — the max_iterations safeguard eventually stopped the loop but masked the real config bug underneath.
• What changed: Classified catch errors into two bands: (1) condition-false — non-zero exit with no signal (existing behavior, counter reset); (2) system-error — ENOENT/EACCES/ETIMEDOUT, killed, or signaled. System errors increment a consecutiveBashSystemErrors counter; two consecutive system errors fail-fast with a diagnostic throw that names the error code/signal. One transient flake is tolerated. Added structured log event loop_node.until_bash_system_error with code, signal, killed, and consecutive count.
• What did not change: The until regex/signal detection path. Default max_iterations. The iteration emit sequence. Legitimate non-zero exits (the common case for "condition not yet satisfied") behave identically to before — the counter resets on every non-system error.

UX Journey

Before

Workflow YAML:
  until_bash: "gh pr list --state merged | grep my-branch"
  # gh not installed on this machine

Iteration 1: bash: gh: command not found (exit 127) → bashComplete=false → loop continues
Iteration 2: same → bashComplete=false → loop continues
...
Iteration N: same → max_iterations reached → "exceeded max iterations" error
             (N × LLM calls wasted, no mention of the real problem)

After

Iteration 1: exit 127, killed=undefined, signal=undefined → condition_false (tolerated, could be transient)
             BUT exit code alone (no signal) is condition-false, so counter stays 0 ✓

Actually for ENOENT/EACCES/signal cases:
Iteration 1: ENOENT/signal detected → consecutiveBashSystemErrors=1 → warn logged
Iteration 2: same → consecutiveBashSystemErrors=2 → THROW:
  "until_bash for node 'verify' hit 2 consecutive system errors
   (code=ENOENT, signal=none). This likely indicates a misconfiguration.
   Stopping loop to prevent cost runaway."

Files Changed

1 file: packages/workflows/src/dag-executor.ts (+32 −5)

Testing

• bun run type-check — all 10 packages clean
• bun run lint — zero warnings
• bun test packages/workflows/src/dag-executor.test.ts — 153 pass, 0 fail

Closes #1241

Summary by CodeRabbit

• Bug Fixes

  • Enforce a per-iteration execution timeout for loop bash checks.
  • Broaden detection of system-level bash failures (timeouts, permissions, signals, specific exit codes) and emit diagnostic warnings with safe metadata and consecutive-error counts.
  • Abort the loop after two consecutive system-level failures and report a terminal failure with cost; non-system failures reset the counter.
  • Include failing-node error details when a workflow finishes with no successful nodes.

• Tests

  • Added tests for consecutive system errors, immediate success, and counter-reset behavior.

Architecture Diagram

Before

loop node iteration
  ├─ run AI prompt
  ├─ run until_bash predicate via execFileAsync ────┐
  │     │                                           │
  │     └─ exit 0 → bashComplete=true               │
  │     └─ exit non-zero → bashComplete=false (single bucket: looks like
  │                       "condition still false", keep looping)
  │     └─ ENOENT/EACCES/ETIMEDOUT/killed/signal → throws,
  │                       caller's catch builds dag_node_pre_execution_failed
  │                       output WITHOUT costUsd → already-spent LLM cost is
  │                       silently dropped
  ▼
emit iteration event

After

loop node iteration
  ├─ run AI prompt
  ├─ run until_bash predicate via execFileAsync ────┐
  │     │                                           │
  │     └─ exit 0 → bashComplete=true               │
  │     └─ exit 1..125 (legitimate non-zero)        │
  │           → bashComplete=false (genuine "condition still false")
  │           → consecutiveBashSystemErrors=0       │
  │     └─ ENOENT/EACCES/ETIMEDOUT/126/127/killed/signal
  │           → CLASSIFIED as system error          │
  │           → consecutiveBashSystemErrors++       │
  │           → if ≥ 2: RETURN failed result with costUsd=loopTotalCostUsd
  │           → never reaches the caller's generic failure path
  ▼
emit iteration event with `lastIterationOutput` preserved

Connection inventory:

From	To	Status	Notes
executeLoopNode until_bash branch	dag_node_pre_execution_failed catch	removed	the throw that escaped to it is now a return
executeLoopNode until_bash branch	NodeExecutionResult { state: 'failed', costUsd }	new	failure now carries the accumulated LLM cost
executeLoopNode until_bash branch	exit-code classifier (126/127, ENOENT, EACCES, ETIMEDOUT, killed, signal)	new	distinguishes misconfiguration from condition-false

Label Snapshot

• Risk: risk: medium
• Size: size: S
• Scope: workflows
• Module: workflows:dag-executor (loop node)

Change Metadata

• Change type: bug
• Primary scope: workflows
• Files: 2 (packages/workflows/src/dag-executor.ts, dag-executor.test.ts)
• LOC: small — adds the classifier branch + return path; preserves existing call shape

Linked Issue

• Closes bug(workflows): until_bash system errors silently collapse to 'condition false', causing max_iteration burn #1241
• Related: bug(workflows): script node timeout does not kill wrapper grandchildren (uv/bun pattern) #1242 / fix(workflows): kill script/bash node grandchildren via process-group on timeout (#1242) #1248 (process-tree kill — the same bash runner is what surfaces ENOENT/127 here)

Validation Evidence

bun run type-check    # clean
bun run lint          # zero warnings
bun test packages/workflows/src/dag-executor.test.ts   # 153/153 pass

Targeted regression tests cover: ENOENT, exit 127 (command not found), exit 126 (permission denied), ETIMEDOUT/killed/signalled, and confirm loopTotalCostUsd is preserved on the failed return.

Security Impact

• New permissions/capabilities? No
• New external network calls? No
• Secrets/tokens handling changed? No
• File system access scope changed? No
• Cost-runaway exposure shrinks: a misconfigured until_bash (e.g. command: not_installed) used to silently re-issue the LLM iteration forever; now it stops after 2 consecutive system errors.

Compatibility / Migration

• Backward compatible? Yes — workflows where until_bash exits 0 (success) or 1..125 (genuine condition-false) behave exactly as before.
• Config/env changes? No
• Database migration needed? No

Human Verification

• Verified scenarios:
  • Misspelled command in until_bash → loop terminates after 2 iterations with a clear loop_node.until_bash_system_error log instead of burning max_iterations.
  • Legitimate non-zero exit (e.g. grep -q ... returning 1) → continues looping as before.
  • LLM-spent cost on early failure → present in the run record (visible in workflow_runs.cost_usd).
• Edge cases: bash killed by SIGTERM during shutdown is treated as a system error (sane — we cannot tell whether the predicate would have succeeded).
• Not verified: behaviour under heavy concurrent loop runs (no shared state changed).

Side Effects / Blast Radius

• Affected subsystems: loop node execution only. No effect on bash/script nodes or AI nodes.
• Potential unintended effects: a transient bash flake (e.g. one-off ETIMEDOUT) followed immediately by a genuine condition-false exit will reset the system-error counter — by design.
• Guardrails: new structured log event loop_node.until_bash_system_error carries {exitCode, signal} for observability.

Rollback Plan

• Fast rollback: revert the commit. Previous behaviour (collapse all non-zero exits into condition-false) is restored.
• Operational signal that triggers rollback: an unexpected wave of loop failures attributed to system errors — would indicate the classifier is too aggressive. Mitigation already in place: only 2 consecutive system errors trigger termination, single transient errors are absorbed.

Risks and Mitigations

• Risk: classifier flags a legitimate predicate that genuinely returns 126/127. Mitigation: documented and surfaced via loop_node.until_bash_system_error log so operators can see and adjust the predicate.
• Risk: preserved costUsd exposes higher numeric run cost than before for failed runs. Mitigation: this is the correct accounting; earlier behaviour was a silent drop.

[coderabbitai]

Note

Reviews paused

It 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 reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

• @coderabbitai resume to resume automatic reviews.
• @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

• ▶️ Resume reviews
• 🔍 Trigger review

📝 Walkthrough

Walkthrough

executeLoopNode's until_bash path now enforces a per-iteration timeout, classifies subprocess failures into system vs non-system errors, counts consecutive system errors, logs each system error, and aborts the loop after two consecutive system errors; DAG finish now surfaces failed-node error details when available.

Changes

Cohort / File(s)	Summary
Loop System Error Handling packages/workflows/src/dag-executor.ts	Add per-iteration timeout for until_bash; classify bash failures (errno: ENOENT, EACCES, ETIMEDOUT; signals/killed; exit codes 126/127) as system errors vs non-system (condition-false); maintain consecutiveBashSystemErrors counter; emit loop_node.until_bash_system_error with safe metadata; fail fast on 2 consecutive system errors; reset counter on success or non-system failures. Also include failed-node error details in DAG failure message when no nodes succeeded.
Tests — until_bash behavior packages/workflows/src/dag-executor.test.ts	Add tests for: consecutive system-level failures aborting the loop and emitting loop_iteration_failed; immediate exit-0 completing the loop; non-system failures not incrementing system-error counter across iterations allowing eventual success.

Sequence Diagram(s)

sequenceDiagram
  rect rgba(200,230,255,0.5)
    participant Executor
    participant Bash
    participant Logger
    participant Caller
  end

  Executor->>Bash: execFileAsync('bash', ['-c', script], { timeout: UNTIL_BASH_TIMEOUT_MS })
  alt success (exit 0)
    Bash-->>Executor: exit 0
    Executor->>Executor: consecutiveSystemErrors = 0\nbashComplete = true
    Executor-->>Caller: iteration complete / proceed
  else error
    Bash-->>Executor: error { code?, signal?, killed?, exitCode? }
    Executor->>Executor: classify isSystemError(error)
    alt system error
      Executor->>Logger: warn loop_node.until_bash_system_error { nodeId, iteration, code, signal, killed, consecutive }
      Executor->>Executor: consecutiveSystemErrors += 1\nbashComplete = false
      alt consecutiveSystemErrors >= 2
        Executor-->>Caller: emit/persist loop_iteration_failed\nreturn { state: 'failed', error, costUsd }
      else
        Executor-->>Caller: continue loop (bashComplete = false)
      end
    else non-system error
      Executor->>Logger: warn loop_node.until_bash_exec_error { nodeId, iteration }
      Executor->>Executor: consecutiveSystemErrors = 0\nbashComplete = false
      Executor-->>Caller: continue loop
    end
  end

Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

• fix(workflows): fail loudly on SDK isError results in DAG and loop nodes #1291 — also modifies executeLoopNode failure handling for iteration failures and system-error classification.
• fix: detect completion signal in any XML tag, not just <promise> (#1126) #1184 — touches loop handling in executeLoopNode (loop output cleaning / stripCompletionTags), overlaps the same loop-control area.

Poem

🐇 I nudged the bash with careful taps,

Two tumbles and I close the gaps,
Counters watch, the warnings drum,
No more loops that blindly hum,
Hopping on — the run's not glum.

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly and accurately describes the main change: distinguishing until_bash system errors from condition-false scenarios, which is the core fix in this PR.
Description check	✅ Passed	The PR description comprehensively covers all template sections including summary, UX journey, architecture diagrams, metadata, linked issues, validation evidence, security impact, compatibility, human verification, side effects, and rollback plan.
Linked Issues check	✅ Passed	The code changes fully implement all objectives from #1241: classifying system errors vs condition-false, fail-fast on 2 consecutive system errors, emitting diagnostic logs, and preserving cost accounting while maintaining backward compatibility for legitimate non-zero exits.
Out of Scope Changes check	✅ Passed	All changes are directly scoped to the until_bash error classification problem: dag-executor logic updates and targeted regression tests; no unrelated modifications to other systems or subsystems.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 9552206882

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[chatgpt-codex-connector]

Return loop failure object instead of throwing here

This throw exits executeLoopNode and is then handled by the caller’s generic dag_node_pre_execution_failed path, which builds a failed node output without costUsd. Because until_bash is evaluated after each LLM iteration, the workflow can already have accrued loop cost before this point, so this path under-reports total cost and mislabels the failure as "before execution". Returning a normal loop failure result from executeLoopNode (including accumulated loopTotalCostUsd) would preserve accounting and context.

Useful? React with 👍 / 👎.

[coderabbitai]

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.ts`:
- Around line 1798-1803: The throw inside the consecutiveBashSystemErrors branch
should be changed to return a failed NodeExecutionResult from executeLoopNode so
the failure stays local and loopTotalCostUsd is preserved; locate the branch
that checks consecutiveBashSystemErrors (uses bashErr and node.id) and replace
the throw new Error(...) with constructing and returning a NodeExecutionResult
marked as failed (include the same descriptive message, attach error details
from bashErr.code/signal, and include the current loopTotalCostUsd in the
result) so the node is reported correctly and accumulated cost is not dropped.
- Around line 1775-1782: The check that classifies bash errors as system vs
condition-false should also treat numeric bash exit codes 126 and 127 as system
errors to avoid infinite retry loops; update the logic around bashErr and
isSystemError in the error handler (the block that casts e to
NodeJS.ErrnoException & { killed?: boolean; signal?: string }) to detect numeric
codes 126 and 127 (and/or string equivalents) in addition to existing errno
checks, and ensure that when a 126/127 code is seen you increment/leave
consecutiveBashSystemErrors as a system error rather than resetting it so the
executor stops retrying misconfigured commands invoked via execFileAsync.

🪄 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: 923b90dc-4ab9-41f9-ae18-d08764bc1ab8

📥 Commits

Reviewing files that changed from the base of the PR and between 3dedc22 and 9552206.

📒 Files selected for processing (1)

• packages/workflows/src/dag-executor.ts

[Wirasm]

Hi @shaun0927 — thanks for opening this PR.

This repository uses a PR template at .github/pull_request_template.md with several required sections. A few of them appear to be empty or placeholder here:

• Architecture Diagram
• Label Snapshot
• Change Metadata
• Security Impact
• Compatibility / Migration
• Side Effects / Blast Radius
• Rollback Plan
• Risks and Mitigations

Could you fill those out (even briefly)? The template helps reviewers understand scope, risk, and rollback — it speeds up review significantly.

If a section genuinely doesn't apply, just write "N/A" in it rather than leaving it blank.

[shaun0927]

Updated the PR description with the missing template sections.

No additional code change in this comment — the bot's P2 finding ("return failed NodeExecutionResult with loopTotalCostUsd instead of throwing") was already addressed in the prior follow-up commits (c430d85e, 5b2ff1ee). The current branch HEAD returns:

return {
  state: 'failed' as const,
  output: lastIterationOutput,
  error: `until_bash for node '${node.id}' hit ${consecutiveBashSystemErrors} consecutive system errors ...`,
  costUsd: loopTotalCostUsd,
};

so the accumulated LLM cost is preserved on early termination.

[Wirasm]

Review Summary

Verdict: minor-fixes-needed

This is a well-targeted bug fix that correctly classifies until_bash bash errors into system errors vs. legitimate non-zero exits, and prevents cost runaway by failing after 2 consecutive system errors. The logic is sound and CLAUDE.md-compliant. The one gap blocking merge: the new behavioral path has no test coverage — a regression test for the 2-consecutive-error abort is needed before this can safely ship.

Blocking issues

• packages/workflows/src/dag-executor.ts — until_bash execution path entirely untested.
  Your new consecutiveBashSystemErrors counter and the fail-on-2 early-return have zero coverage in dag-executor.test.ts. Grepping for until_bash, consecutive, or system_error in the test file returns nothing. Please add at minimum:
  1. A regression test that mocks execFileAsync to reject twice with ENOENT and asserts the loop returns { state: 'failed', error: ...contains 'consecutive system errors'... } (not max_iterations).
  2. A happy-path test: until_bash: 'exit 0' with max_iterations: 5 should complete in 1 iteration (mockSendQueryDag.mock.calls.length === 1).
  3. A test that a single non-system error does NOT early-return (counter resets to 0 on the non-system-error branch — this is the key distinction your PR introduces).

Minor / nice-to-have

• packages/providers/src/community/pi/event-bridge.ts — Tier 3 limitation (acknowledged, no action needed): When the preamble itself contains a { character (e.g. "Let me check {a, b, c}..."), Tier 3's cleaned.indexOf('{') lands on the wrong brace and parsing silently returns undefined despite valid JSON. The code comment already documents this constraint ("Preamble must not itself contain {"). Low risk in practice — pre-existing limitation, not introduced by this PR. Future enhancement: brace-depth tracking or a sentinel-line stop.
• packages/workflows/src/dag-executor.ts:1807 — style: // non-zero exit = not complete restates what the code does. The variable name bashComplete already says this. Consider removing it, or replacing with // bashComplete=true means the until condition is satisfied for clarity.
• packages/workflows/src/dag-executor.ts:1788 — minor: When until_bash fails on the very first iteration, lastIterationOutput is still ''. The early-return returns an empty string alongside the error message. No action needed — the error already explains the failure.

Compliments

• The comment at line 1804 (// Classify: system error (misconfig) vs condition-false (legitimate non-zero exit)) is excellent — it captures a non-obvious conceptual distinction that wasn't clear from the code alone. Well done.
• The error classification covers the full realistic spectrum (ENOENT, EACCES, ETIMEDOUT, killed, signal, exit codes 126/127) and the 2-error threshold is well-chosen to avoid false positives on single spurious failures.
• The undefined-on-failure contract in tryParseStructuredOutput is correctly surfaced by the caller via a structured pi.event-bridge.structured_output_parse_failed warn event — clean end-to-end.

---

Reviewed via maintainer-review-pr workflow (Pi/Minimax). Aspects run: code-review, error-handling, test-coverage, comment-quality.

[shaun0927]

Pushed an update — the three regression tests you flagged are now in.

In packages/workflows/src/dag-executor.test.ts (loop node execution describe):

• until_bash: aborts loop after 2 consecutive system errors (ENOENT) — execFileAsync rejects with ENOENT repeatedly; asserts only 2 invocations (not 10) and that the workflow run finalizes via failWorkflowRun, not completeWorkflowRun.
• until_bash: completes in one iteration when exit 0 on first call — happy path: exit 0 → 1 sendQuery call, 1 exec call, completed.
• until_bash: a single non-system error does not trip the system-error counter — two condition-false rejections followed by an exit 0; asserts the loop reaches iteration 3 (counter resets), so a one-off non-zero exit doesn't get conflated with a system error.

Also rebased onto current dev and squashed the four review-cycle commits into one. The ubuntu CI failure on the previous run was in @archon/core's getDatabaseType tests (unrelated to this PR) and should clear on the rebase.

bun run validate clean locally.

[coderabbitai]

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 4348-4357: The test currently checks that
mockDeps.store.failWorkflowRun was called once but doesn't assert the failure
message; update the test to assert the second argument passed to failWorkflowRun
(inspect mockDeps.store.failWorkflowRun.mock.calls[0][1]) equals the expected
user-facing diagnostic string (e.g., the "consecutive system errors" message or
"max_iterations" text depending on this scenario) and keep the existing check
that mockDeps.store.completeWorkflowRun was not called
(mockDeps.store.completeWorkflowRun.mock.calls.length === 0).

In `@packages/workflows/src/dag-executor.ts`:
- Around line 2046-2055: Before returning the failure object when
consecutiveBashSystemErrors >= 2, emit the terminal iteration event (same one
used for normal loop termination) so subscribers/UI get a started→terminal
transition; specifically, call the codepath that emits either
"loop_iteration_failed" (or the terminal iteration completion emitter used
elsewhere) with the node id (node.id), lastIterationOutput, error info
constructed from bashErr (code/signal), and loopTotalCostUsd, then return the
existing failure object. Ensure you reuse the same event payload shape and
emitter used in other iteration terminal branches so workflow-bridge buffer
flush logic sees the terminal event before the function returns.

🪄 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: c0705e0d-b763-419b-8799-fb0b7b700ff3

📥 Commits

Reviewing files that changed from the base of the PR and between 5b2ff1e and 61287d8.

📒 Files selected for processing (2)

• packages/workflows/src/dag-executor.test.ts
• packages/workflows/src/dag-executor.ts

[shaun0927]

Pushed an update — picks up the two findings on the previous round:

• dag-executor.ts system-error abort path: now emits a terminal loop_iteration_failed event (both via the in-memory event emitter and createWorkflowEvent) before returning the failure result, so the workflow bridge sees a started → failed transition instead of an iteration that just disappears. Reuses the same payload shape as the existing iteration-failure branch.
• Test until_bash: aborts loop after 2 consecutive system errors (ENOENT): also asserts that createWorkflowEvent was called with event_type: 'loop_iteration_failed' and an error containing "consecutive system errors". Catches both the wiring (event reaches the store) and the user-facing message.

Local: bun run type-check clean, lint clean, dag-executor.test.ts -t 'until_bash:' 3/3.

[coderabbitai]

Actionable comments posted: 2

♻️ Duplicate comments (1)

packages/workflows/src/dag-executor.test.ts (1)

4348-4357: ⚠️ Potential issue | 🟡 Minor

Assert the persisted failure reason text in failWorkflowRun.

This test verifies event diagnostics, but it still doesn’t lock the workflow failure message contract (failWorkflowRun arg), so a regression could silently fall back to max_iterations wording.

Suggested assertion add-on

         expect(failCalls.length).toBe(1);
+        expect(failCalls[0]?.[1]).toMatch(/consecutive system errors/i);
+        expect(failCalls[0]?.[1]).not.toMatch(/max_iterations/i);
         expect(completeCalls.length).toBe(0);

🤖 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 4348 - 4357, The
test currently asserts that mockDeps.store.failWorkflowRun was called once but
doesn't assert the failure message; update the test around failCalls /
mockDeps.store.failWorkflowRun to also assert the persisted failure reason
argument matches the expected diagnostic text (e.g., inspect failCalls[0][1] or
the second argument passed to mockDeps.store.failWorkflowRun) so the test locks
the failWorkflowRun message contract and prevents regressions that revert to
generic wording like "max_iterations".

🤖 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 2023-2030: The execFileAsync call used in the until_bash predicate
(the until_bash function) currently passes only { cwd }, so ETIMEDOUT is never
produced; modify that execFileAsync invocation to include an explicit timeout
option (matching the pattern used elsewhere in this file, e.g., the other
execFileAsync calls that pass a timeout) so that hung predicates will fail with
code 'ETIMEDOUT' and the existing check bashErr.code === 'ETIMEDOUT' becomes
reachable; keep the timeout value consistent with similar uses in this file and
preserve existing error handling for bashErr, killed, signal, exitCode, etc.
- Around line 2035-2043: The warning log currently includes the full error
object (err: bashErr) which can leak command text; update the log payload in
dag-executor.ts to remove err: bashErr and instead log only safe metadata:
nodeId (use node.id), workflowRunId, iteration (use i), code (bashErr.code),
signal (bashErr.signal), killed (bashErr.killed), and consecutiveErrors
(consecutiveBashSystemErrors); keep the same logger call but strip the raw error
object to avoid serializing bashErr.message/command.

---

Duplicate comments:
In `@packages/workflows/src/dag-executor.test.ts`:
- Around line 4348-4357: The test currently asserts that
mockDeps.store.failWorkflowRun was called once but doesn't assert the failure
message; update the test around failCalls / mockDeps.store.failWorkflowRun to
also assert the persisted failure reason argument matches the expected
diagnostic text (e.g., inspect failCalls[0][1] or the second argument passed to
mockDeps.store.failWorkflowRun) so the test locks the failWorkflowRun message
contract and prevents regressions that revert to generic wording like
"max_iterations".

🪄 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: ca4cdf42-3287-45e7-a8bf-bbbea3afaae5

📥 Commits

Reviewing files that changed from the base of the PR and between 61287d8 and 1fcd032.

📒 Files selected for processing (2)

• packages/workflows/src/dag-executor.test.ts
• packages/workflows/src/dag-executor.ts

[shaun0927]

Pushed an update — picks up the items CR flagged on the previous round:

• dag-executor.ts until_bash exec: now passes timeout: UNTIL_BASH_TIMEOUT_MS (30 s) so a hung predicate actually fails with ETIMEDOUT instead of running unbounded each iteration. Also makes the existing bashErr.code === 'ETIMEDOUT' branch in the system-error classifier reachable.
• dag-executor.ts system-error log payload: dropped err: bashErr from the warn log — bashErr.message and the bound command can carry user data and substituted node outputs. Now logs only workflowRunId, nodeId, iteration, code, signal, killed, consecutiveErrors.
• dag-executor.ts workflow-level fail message: when the run finishes with no successful nodes, now surfaces the actual failed-node reasons (e.g. 'sys-err-loop': until_bash for node ... hit 2 consecutive system errors ...) instead of the generic "completed with no successful nodes" diagnostic. Falls back to the old wording when there's no failure detail to show.
• Test until_bash: aborts loop after 2 consecutive system errors: locks the failWorkflowRun message contract — must contain "consecutive system errors", must not match "max_iterations".

Local: bun run type-check clean, lint clean, full @archon/workflows test suite all batches pass.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: b7190261ef

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[chatgpt-codex-connector]

Remove fixed 30s timeout from until_bash checks

Applying timeout: UNTIL_BASH_TIMEOUT_MS to every until_bash command changes existing loop behavior from “any non-zero means keep iterating” to hard-failing after two 30s runs, because long predicates are now killed and classified as system errors. This breaks valid workflows that use slower deterministic checks (for example the docs explicitly show until_bash: "bun run test"), where tests/builds can exceed 30 seconds and will now be reported as misconfiguration. The timeout needs to be configurable (or inherited from node-level timeout) rather than a hard-coded global cap.

Useful? React with 👍 / 👎.

[shaun0927]

Pushed an update — picks up Codex's P1 follow-up:

• loop.until_bash_timeout_ms config field: replaced the hard-coded 30 s cap with a per-loop configurable timeout. Default raised to 5 minutes so workflows that legitimately use until_bash: "bun run test" (or any predicate that exercises a real test/build suite) keep working. Setting the field to 0 disables the cap entirely. Implemented via Zod (packages/workflows/src/schemas/loop.ts) → DEFAULT_UNTIL_BASH_TIMEOUT_MS → executor passes it to execFileAsync only when > 0.
• DAG workflow-level fail message now folds in the actual failed-node detail ('<id>': <error>) when the run finishes with no successful nodes. Previously the user only saw "completed with no successful nodes" — now they see why. Existing tests that searched for the old wording were updated to also accept the new failed: prefix.

Local: bun run type-check clean, bun run lint clean, full @archon/workflows test suite passes (200/200).