fix(workflows): concise failure messages for bash/script nodes (#1389)#1393
Merged
Conversation
📝 WalkthroughWalkthroughSubprocess failure formatting was added and integrated into DAG executor error paths for Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Executor as DAG Executor
participant Subproc as Subprocess (bash/script)
participant Formatter as formatSubprocessFailure
participant Logger as Logger / Node Store
Executor->>Subproc: spawn/run node subprocess
Subproc-->>Executor: exit non‑zero / error (stderr, message, code, killed)
Executor->>Formatter: formatSubprocessFailure(err, label)
Formatter-->>Executor: { userMessage, logFields }
Executor->>Logger: store node_failed.data.error = userMessage
Executor->>Logger: emit dag_node_failed with { ...logFields, nodeId, nodeType, isTimeout }
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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 docstrings
🧪 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. Review rate limit: 7/8 reviews remaining, refill in 7 minutes and 30 seconds.Comment |
Collaborator
Author
Automated self-reviewVerdict: PASS (0 critical, 0 important) Strengths
Suggestions (non-blocking)
No security concerns. Checklist
|
There was a problem hiding this comment.
🧹 Nitpick comments (2)
packages/workflows/src/dag-executor.ts (2)
1315-1336: Consider populatinglogFieldsin the timeout/ENOENT/EACCES branches for consistency.In the non-
formatSubprocessFailurebranches,logFieldsstays{}, so the resultingdag_node_failedlog entry loses potentially useful diagnostic fields (exitCode,killed) that are present onerr. The user-facing message is already concise for these branches, but the structured log entry is now less informative than before this change (where the rawerrwas logged). A minimal adjustment that keeps the script body out while preserving structured diagnostics:♻️ Proposed refactor
- const err = error as Error & { killed?: boolean; code?: number | string; stderr?: string }; + const err = error as Error & { killed?: boolean; code?: number | string; stderr?: string }; const isTimeout = err.killed === true || (err.message ?? '').includes('timed out'); const label = `Bash node '${node.id}'`; let errorMsg: string; - let logFields: Record<string, unknown> = {}; + let logFields: Record<string, unknown> = { + exitCode: err.code, + killed: err.killed === true, + }; if (isTimeout) { errorMsg = `${label} timed out after ${String(timeout)}ms`; } else if (err.message?.includes('ENOENT')) { errorMsg = `${label} failed: bash executable not found in PATH`; } else if (err.message?.includes('EACCES')) { errorMsg = `${label} failed: permission denied (check cwd permissions)`; } else { const formatted = formatSubprocessFailure(err, label); errorMsg = formatted.userMessage; logFields = formatted.logFields; }Non-blocking — feel free to skip if you prefer to keep those branches minimal.
🤖 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 1315 - 1336, The structured log (getLog().error) drops diagnostic fields in the timeout/ENOENT/EACCES branches because logFields remains empty; update those branches in the catch block around node execution to populate logFields with relevant properties from err (e.g., killed, code or exitCode, stderr) so they match the data returned by formatSubprocessFailure; keep the existing concise user-facing errorMsg for each branch but set logFields = { killed: err.killed, exitCode: err.code ?? (err as any).exitCode, stderr: err.stderr } (or similar keys used by formatSubprocessFailure) before calling getLog().error for consistent structured diagnostics for node.id and timeout cases.
1315-1362: Near-identical catch blocks inexecuteBashNodeandexecuteScriptNode— consider extracting.The post-
execFileAsynccatch logic (error classification →errorMsg/logFields→ structured log →logNodeError→ event persist → emitter.emit → return failedNodeOutput) is duplicated between the two subprocess node executors, differing only in thelabel, the missing-executable name, and the eventtype: 'bash' | 'script'. A small helper (e.g.,handleSubprocessFailure(…)) would keep both nodes in lockstep on future changes (idle-timeout handling, cancellation classification, etc.) and reduce the risk of the two paths drifting.Not required for this PR — the current change is already a clear improvement; noting as a follow-up.
Also applies to: 1578-1625
🤖 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 1315 - 1362, The catch block logic in executeBashNode and executeScriptNode is duplicated (error classification → errorMsg/logFields → structured logging → logNodeError → event persist → emitter.emit → return failed NodeOutput); extract this into a helper like handleSubprocessFailure(node, workflowRun, err, label, eventType, timeout, logDir, deps, emitter) that: classifies timeouts/ENOENT/EACCES using formatSubprocessFailure, builds errorMsg/logFields, calls getLog().error with nodeId/isTimeout/errType, awaits logNodeError(logDir, workflowRun.id, node.id, errorMsg), calls deps.store.createWorkflowEvent({... data: { error: errorMsg, type: eventType }}).catch(...), emits the same emitter.emit payload, and returns the standard { state: 'failed', output: '', error: errorMsg } so both executeBashNode and executeScriptNode just call handleSubprocessFailure(...) in their catch blocks.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/workflows/src/dag-executor.ts`:
- Around line 1315-1336: The structured log (getLog().error) drops diagnostic
fields in the timeout/ENOENT/EACCES branches because logFields remains empty;
update those branches in the catch block around node execution to populate
logFields with relevant properties from err (e.g., killed, code or exitCode,
stderr) so they match the data returned by formatSubprocessFailure; keep the
existing concise user-facing errorMsg for each branch but set logFields = {
killed: err.killed, exitCode: err.code ?? (err as any).exitCode, stderr:
err.stderr } (or similar keys used by formatSubprocessFailure) before calling
getLog().error for consistent structured diagnostics for node.id and timeout
cases.
- Around line 1315-1362: The catch block logic in executeBashNode and
executeScriptNode is duplicated (error classification → errorMsg/logFields →
structured logging → logNodeError → event persist → emitter.emit → return failed
NodeOutput); extract this into a helper like handleSubprocessFailure(node,
workflowRun, err, label, eventType, timeout, logDir, deps, emitter) that:
classifies timeouts/ENOENT/EACCES using formatSubprocessFailure, builds
errorMsg/logFields, calls getLog().error with nodeId/isTimeout/errType, awaits
logNodeError(logDir, workflowRun.id, node.id, errorMsg), calls
deps.store.createWorkflowEvent({... data: { error: errorMsg, type: eventType
}}).catch(...), emits the same emitter.emit payload, and returns the standard {
state: 'failed', output: '', error: errorMsg } so both executeBashNode and
executeScriptNode just call handleSubprocessFailure(...) in their catch blocks.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8da317c5-a401-4122-8dd8-a90c8eb05165
📒 Files selected for processing (4)
packages/workflows/src/dag-executor.test.tspackages/workflows/src/dag-executor.tspackages/workflows/src/executor-shared.test.tspackages/workflows/src/executor-shared.ts
Collaborator
Author
|
Hi @Wirasm — thanks for opening this PR. This repository uses a PR template at
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. |
Collaborator
Author
PR Review SummaryMulti-agent review of PR 1393 — Critical IssuesNone. Important Issues
Suggestions
Strengths
Documentation Issues
VerdictNEEDS FIXES — non-blocking but worth addressing before merge. The two Recommended Actions
Generated by |
When a `bash:` or `script:` node fails, the executor was embedding `err.message` verbatim into the user-visible error. For inline scripts run via `bash -c <body>` / `bun -e <body>`, Node's `promisify(execFile)` puts the entire substituted script body into `err.message`, `err.cmd`, and the first line of `err.stack` — and the Pino log serialized all three, repeating the body ~3× in one structured log line. The actionable diagnostic was buried under kilobytes of echoed source. Route both catch-block default branches through a new `formatSubprocessFailure(err, label)` helper in `executor-shared.ts` that: - strips the `Command failed: <cmd>` prefix line (which carries the body) - prefers `err.stderr` — Bun/bash emit the actionable diagnostic there - tail-truncates at 2 KB with a `…[truncated]` marker - returns a controlled `logFields` subset (`exitCode`, `killed`, `stderrTail`) so Pino never re-serializes `err.message` / `err.stack` / `err.cmd` Also drops the script-node `stderrHint` concatenation — stderr is already handled by the helper, so the previous code appended it twice. Timeout / ENOENT / EACCES branches are preserved verbatim. Fixes #1389
- Run formatSubprocessFailure unconditionally so timeout / ENOENT / EACCES
branches also get sanitized log fields (the timeout message also embeds
the `Command failed: bash -c <body>` line).
- Drop `errType: err.constructor.name` (always 'Error' in production) and
replace with `nodeType: 'bash' | 'script'` for actual discriminating value.
- Replace chained `||` + ternary in diagnostic selection with explicit
if/else for readability.
- Simplify exit suffix guard: `err.code != null` instead of double typeof.
- Make stderrTail emptiness check explicit.
- Drop `RawSubprocessError` export (no external consumer) and widen `code`
to `number | string | null` to mirror Node's ExecFileException.
- Tighten test length bound to <2100 (was <4000 / <2200) so a doubling of
SUBPROCESS_ERROR_MAX_CHARS would actually trip the assertion.
- Replace broad regex assertion with `.toContain('[eval]')` — the location
marker is the strongest signal that diagnostic content survived.
- Strip issue-number citations from describe block and test comments.
- Update script-nodes.md to distinguish stderr behavior on success vs
failure paths.
- Add CHANGELOG Unreleased / Fixed entry for the user-visible change.
ba561bc to
fe1deba
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/workflows/src/dag-executor.test.ts (1)
1141-1187: Add a logger assertion here too.These regressions cover persisted
node_failed.data.error, but they still leave the sanitizedgetLog().error(...)path untested. Since the leak fix also depends on passing onlyformatted.logFieldsinpackages/workflows/src/dag-executor.ts, a future reintroduction of rawerrlogging would still pass this suite. A smallmockLogger.errorassertion that rejectsCommand failed:/ body markers would close that gap.Also applies to: 6177-6229
🤖 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 1141 - 1187, Add an assertion against the logger used by executeDagWorkflow to ensure the sanitized error was logged (so regressions that only fix persisted data but still log raw errors will fail): inspect mockDeps.logger.error (or whatever mock logger is provided by createMockDeps) call(s) after executeDagWorkflow and assert the logged message for the 'node_failed' for step 'fail-bash-1389' contains "diagnostic from stderr" and "[exit 1]" and does NOT contain "Command failed:" or "UNIQUE_CMDLINE_MARKER_1389" (similar to the checks on failedEvent.data.error); place this immediately after you locate failedEvent so it validates the getLog().error path for the same failure.
🤖 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/docs-web/src/content/docs/guides/script-nodes.md`:
- Around line 76-80: The paragraph currently implies the full stderr is always
returned verbatim; update the wording so it matches formatSubprocessFailure():
state that the formatter prefers stderr but falls back to other output when
stderr is empty, and that diagnostics are tail-truncated to ~2 KB rather than
returned in full; keep the example failure pattern (Script node 'X' failed [exit
N]: <stderr>) but clarify that the shown <stderr> may be truncated or
substituted per formatSubprocessFailure() and that the script body is never
echoed back to users.
---
Nitpick comments:
In `@packages/workflows/src/dag-executor.test.ts`:
- Around line 1141-1187: Add an assertion against the logger used by
executeDagWorkflow to ensure the sanitized error was logged (so regressions that
only fix persisted data but still log raw errors will fail): inspect
mockDeps.logger.error (or whatever mock logger is provided by createMockDeps)
call(s) after executeDagWorkflow and assert the logged message for the
'node_failed' for step 'fail-bash-1389' contains "diagnostic from stderr" and
"[exit 1]" and does NOT contain "Command failed:" or
"UNIQUE_CMDLINE_MARKER_1389" (similar to the checks on failedEvent.data.error);
place this immediately after you locate failedEvent so it validates the
getLog().error path for the same failure.
🪄 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: 7d44f3cc-e5be-411c-b2cf-43cf420ba8fd
📒 Files selected for processing (6)
CHANGELOG.mdpackages/docs-web/src/content/docs/guides/script-nodes.mdpackages/workflows/src/dag-executor.test.tspackages/workflows/src/dag-executor.tspackages/workflows/src/executor-shared.test.tspackages/workflows/src/executor-shared.ts
✅ Files skipped from review due to trivial changes (2)
- CHANGELOG.md
- packages/workflows/src/executor-shared.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/workflows/src/executor-shared.test.ts
- packages/workflows/src/dag-executor.ts
Comment on lines
+76
to
+80
| `$nodeId.output`. On a successful run, `stderr` is logged as a warning and | ||
| posted to the conversation but does **not** fail the node. A non-zero exit | ||
| code fails the node; on failure, `stderr` is the diagnostic surfaced in the | ||
| error message (`Script node 'X' failed [exit N]: <stderr>`) — the script | ||
| body is never echoed back to users. |
There was a problem hiding this comment.
Soften the failure-format wording to match the formatter.
This reads like the full stderr is always returned verbatim, but formatSubprocessFailure() actually prefers stderr, falls back when it is empty, and tail-truncates diagnostics at 2 KB. A wording tweak here would avoid setting the wrong expectation for long or stderr-less failures.
📝 Suggested wording
- `$nodeId.output`. On a successful run, `stderr` is logged as a warning and
- posted to the conversation but does **not** fail the node. A non-zero exit
- code fails the node; on failure, `stderr` is the diagnostic surfaced in the
- error message (`Script node 'X' failed [exit N]: <stderr>`) — the script
- body is never echoed back to users.
+ `$nodeId.output`. On a successful run, `stderr` is logged as a warning and
+ posted to the conversation but does **not** fail the node. A non-zero exit
+ code fails the node; on failure, the error message prefers `stderr`, strips
+ the leading `Command failed:` wrapper, and tail-caps very large diagnostics
+ (for example: `Script node 'X' failed [exit N]: <diagnostic>`). The script
+ body is never echoed back to users.📝 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.
Suggested change
| `$nodeId.output`. On a successful run, `stderr` is logged as a warning and | |
| posted to the conversation but does **not** fail the node. A non-zero exit | |
| code fails the node; on failure, `stderr` is the diagnostic surfaced in the | |
| error message (`Script node 'X' failed [exit N]: <stderr>`) — the script | |
| body is never echoed back to users. | |
| `$nodeId.output`. On a successful run, `stderr` is logged as a warning and | |
| posted to the conversation but does **not** fail the node. A non-zero exit | |
| code fails the node; on failure, the error message prefers `stderr`, strips | |
| the leading `Command failed:` wrapper, and tail-caps very large diagnostics | |
| (for example: `Script node 'X' failed [exit N]: <diagnostic>`). The script | |
| body is never echoed back to users. |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/docs-web/src/content/docs/guides/script-nodes.md` around lines 76 -
80, The paragraph currently implies the full stderr is always returned verbatim;
update the wording so it matches formatSubprocessFailure(): state that the
formatter prefers stderr but falls back to other output when stderr is empty,
and that diagnostics are tail-truncated to ~2 KB rather than returned in full;
keep the example failure pattern (Script node 'X' failed [exit N]: <stderr>) but
clarify that the shown <stderr> may be truncated or substituted per
formatSubprocessFailure() and that the script body is never echoed back to
users.
Merged
Merged
6 tasks
prospapledge88
added a commit
to prospapledge88/Archon
that referenced
this pull request
May 5, 2026
…h 7 (#12) * fix(workflows): make archon-adversarial-dev sed replacement macOS-safe (coleam00#1155) * fix(workflows): make adversarial init sed portable on macOS * chore: regenerate bundled-defaults after adversarial-dev sed fix Sync generated bundle with the new temp-file sed pattern in archon-adversarial-dev.yaml so check:bundled passes and binary distributions ship the macOS-safe version. --------- Co-authored-by: laplace young <yangqk12@whu.edu.cn> Co-authored-by: Rasmus Widing <rasmus.widing@gmail.com> * fix(workflows): filter user-plugin MCP noise out of workflow warnings (coleam00#1327) * fix(workflows): filter user-plugin MCP noise out of workflow warnings Before this change, the dag-executor surfaced every entry in the Claude SDK's "MCP server connection failed: …" system message to the user. That message includes user-level plugin MCPs inherited from ~/.claude/ (e.g. `telegram`) that fail to connect in the headless workflow subprocess — they're non-actionable noise for the workflow author. Fix: - Pre-compute the set of workflow-configured MCP server names per node by parsing the `mcp:` config file once at the start of executeNodeInternal. No caller-facing API change; no duplication of the provider's env-var expansion logic (we only need the keys). - Split the system-message handler: the `MCP server connection failed:` path now surfaces only the subset of failing names that match the node's configured set; user-plugin failures are debug-logged as `dag.mcp_plugin_connection_suppressed`. The `⚠️ ` branch is unchanged. Supersedes coleam00#1134 (closed as stale — the Windows HOME fix in that PR was already shipped via coleam00#1302, and the claude.ts enabledPlugins change targeted a file that has since moved into @archon/providers). Credits @MrFadiAi for identifying and reporting the underlying issue. Co-authored-by: Fadi Ai <MrFadiAi@users.noreply.github.com> * fix(workflows): preserve MCP failure status in filtered message + observability Address review feedback on PR coleam00#1327: - parseMcpFailureServerNames now returns {name, segment} entries so the forwarded "MCP server connection failed: ..." message preserves the per-server status detail (e.g. "(timeout)", "(disconnected)") that the bare-name reconstruction was dropping. - loadConfiguredMcpServerNames now debug-logs read failures as dag.mcp_filter_config_read_failed instead of swallowing them silently. A transient EMFILE/EBUSY at filter time would otherwise silently reclassify a real workflow-MCP failure as plugin noise. - Add 4 integration tests through executeDagWorkflow covering the mixed workflow/plugin split, all-plugin suppression, no-mcp:-config nodes, and the unchanged⚠️ warning path. - Drop a WHAT comment above configuredMcpNames and a temporal phrase ("before this filter landed") that would rot on merge. - Document the filtering boundary in guides/mcp-servers.md and add a troubleshooting row for users debugging silently-suppressed plugin MCPs. --------- Co-authored-by: Fadi Ai <MrFadiAi@users.noreply.github.com> * test(workflows): add anyFailed status derivation coverage for DAG executor (coleam00#1403) PIV Task 1: Adds three new tests in a dedicated describe block 'executeDagWorkflow -- final status derivation' covering the anyFailed branch (dag-executor.ts ~line 2956) that previously had no direct test: - one success + one independent failure calls failWorkflowRun (not completeWorkflowRun) - multiple successes + one failure calls failWorkflowRun (not completeWorkflowRun) - trigger_rule: none_failed skips dependent node but anyFailed still marks run failed Fixes coleam00#1381. * fix(workflow): migrate piv-loop plan handoff to $ARTIFACTS_DIR (coleam00#1398) * fix(workflow): migrate piv-loop plan handoff to $ARTIFACTS_DIR (coleam00#1380) The create-plan node used a relative path (.claude/archon/plans/{slug}.plan.md) that the AI agent would sometimes write to a different location, breaking all downstream nodes that glob for the plan file. Migrated all plan/progress file references to $ARTIFACTS_DIR/plan.md and $ARTIFACTS_DIR/progress.txt, matching the pattern used by archon-fix-github-issue and other workflows. Changes: - Replace slug-based plan path with $ARTIFACTS_DIR/plan.md in create-plan node - Replace ls -t glob discovery with direct $ARTIFACTS_DIR/plan.md reads in refine-plan, code-review, and fix-feedback nodes - Replace empty-string guard with file-existence check in implement-setup bash - Migrate progress.txt references in implement loop to $ARTIFACTS_DIR/ - Add explicit plan/progress paths in finalize node - Regenerated bundled-defaults.generated.ts Fixes coleam00#1380 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix(workflow): address review findings in archon-piv-loop - Rename 'Step 2: Write the Plan' to 'Step 2: Plan File Location' to eliminate the duplicate heading that collided with Step 3's identical title in the create-plan node - Guard implement-setup against a 0-task plan file: exit 1 with a clear error when no '### Task N:' sections are found, preventing a silent no-op implement loop - Remove 2>/dev/null from code-review commit so pre-commit hook failures and other stderr are visible to the agent instead of silently swallowed - Replace '|| true' on git push in finalize with an explicit WARNING echo so push failures (auth, upstream conflict, no remote) surface to the agent rather than being silently ignored - Regenerate bundled-defaults.generated.ts Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * chore(workflows): regenerate bundled defaults to match opus[1m] alias The bundle was stale relative to the YAML sources after coleam00#1395 merged — check:bundled was failing CI. Regenerated; no YAML edits. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * chore(workflows): switch default Opus pin to opus[1m] alias (coleam00#1395) Anthropic's Opus 4.7 landed 2026-04-16; on the Anthropic API, opus / opus[1m] now resolve to 4.7 with a 1M context window at standard pricing. Using the alias instead of the hard-pinned claude-opus-4-6[1m] lets bundled default workflows auto-track the recommended Opus version. No explicit effort is set, so nodes inherit the per-model default (xhigh on 4.7, high on 4.6). * fix(workflows): approval gate bypass after reject-with-redraft on resume (coleam00#1435) * fix(workflows): approval gate bypass after reject-with-redraft on resume When an approval node was rejected with on_reject.prompt, the synthetic PromptNode built to run the on_reject prompt reused the approval gate's own node ID. executeNodeInternal then wrote a node_completed event with that ID, causing getCompletedDagNodeOutputs to treat the gate as already completed on the next resume — bypassing the human gate entirely. Fix: give the synthetic node the ID `${node.id}:on_reject` so its node_completed event has a distinct step_name that won't match the approval gate slot in priorCompletedNodes. Adds a regression test asserting no node_completed event with the approval gate's ID is written during on_reject execution. Fixes coleam00#1429 * test(workflows): add positive assertion and SSE side-effect comment for on_reject synthetic node Add complementary positive assertion to the regression test to verify that node_completed is written exactly once with step_name 'review:on_reject', ensuring future refactors that suppress the event entirely would be caught. Add inline comment in executeApprovalNode documenting the known SSE side-effect: node_started/node_completed events with nodeId='review:on_reject' flow through the SSE pipeline into the web UI, resulting in a transient phantom node in the execution view. This is cosmetic-only — the human gate contract is preserved. * simplify: reduce duplicate cast pattern in on_reject test assertions * fix(workflows): concise failure messages for bash/script nodes (coleam00#1389) (coleam00#1393) * fix(workflows): concise failure messages for bash/script nodes (coleam00#1389) When a `bash:` or `script:` node fails, the executor was embedding `err.message` verbatim into the user-visible error. For inline scripts run via `bash -c <body>` / `bun -e <body>`, Node's `promisify(execFile)` puts the entire substituted script body into `err.message`, `err.cmd`, and the first line of `err.stack` — and the Pino log serialized all three, repeating the body ~3× in one structured log line. The actionable diagnostic was buried under kilobytes of echoed source. Route both catch-block default branches through a new `formatSubprocessFailure(err, label)` helper in `executor-shared.ts` that: - strips the `Command failed: <cmd>` prefix line (which carries the body) - prefers `err.stderr` — Bun/bash emit the actionable diagnostic there - tail-truncates at 2 KB with a `…[truncated]` marker - returns a controlled `logFields` subset (`exitCode`, `killed`, `stderrTail`) so Pino never re-serializes `err.message` / `err.stack` / `err.cmd` Also drops the script-node `stderrHint` concatenation — stderr is already handled by the helper, so the previous code appended it twice. Timeout / ENOENT / EACCES branches are preserved verbatim. Fixes coleam00#1389 * fix(workflows): address PR review feedback for coleam00#1389 - Run formatSubprocessFailure unconditionally so timeout / ENOENT / EACCES branches also get sanitized log fields (the timeout message also embeds the `Command failed: bash -c <body>` line). - Drop `errType: err.constructor.name` (always 'Error' in production) and replace with `nodeType: 'bash' | 'script'` for actual discriminating value. - Replace chained `||` + ternary in diagnostic selection with explicit if/else for readability. - Simplify exit suffix guard: `err.code != null` instead of double typeof. - Make stderrTail emptiness check explicit. - Drop `RawSubprocessError` export (no external consumer) and widen `code` to `number | string | null` to mirror Node's ExecFileException. - Tighten test length bound to <2100 (was <4000 / <2200) so a doubling of SUBPROCESS_ERROR_MAX_CHARS would actually trip the assertion. - Replace broad regex assertion with `.toContain('[eval]')` — the location marker is the strongest signal that diagnostic content survived. - Strip issue-number citations from describe block and test comments. - Update script-nodes.md to distinguish stderr behavior on success vs failure paths. - Add CHANGELOG Unreleased / Fixed entry for the user-visible change. * chore(workflows): regenerate bundled defaults after Tier 6 picks * docs: add CHANGELOG entry for Tier 6 workflow polish batch --------- Co-authored-by: CauchYoung <2024302072042@whu.edu.cn> Co-authored-by: laplace young <yangqk12@whu.edu.cn> Co-authored-by: Rasmus Widing <rasmus.widing@gmail.com> Co-authored-by: Rasmus Widing <152263317+Wirasm@users.noreply.github.com> Co-authored-by: Fadi Ai <MrFadiAi@users.noreply.github.com> Co-authored-by: Cole Medin <cole@dynamous.ai> Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Co-authored-by: cjnprospa <sirhcle.j23@gmail.com>
2 tasks
bluedevilcollectibles
added a commit
to bluedevilcollectibles/bdc-harness
that referenced
this pull request
May 16, 2026
… [WO-160] (#86) ## Summary Triage of pre-existing failure surfaced during MB's WO-153 testing (unrelated to that PR; was already broken on dev). Closes bluedevilcollectibles/bdc-xo#160 ## What the test was testing `packages/workflows/src/dag-executor.test.ts` -- `failure message strips the "Command failed: bun -e <body>" prefix and stays small` (added in fccfe42 / PR coleam00#1393 / fix coleam00#1389). It guards subprocess-failure sanitization: when an inline `bun -e <multi-KB-script>` fails with a parse error, the user-visible `node_failed` event must (a) carry the `Script node '<id>' failed` prefix, (b) NOT leak the raw `Command failed: bun -e <full body>` message, (c) stay under 2.1 KB, and (d) still contain the `[eval]` source marker. Real value -- this is the SUBPROCESS_ERROR_MAX_CHARS guard. ## Why broken Platform-fragile on Windows dev boxes, not a real production bug. Root cause: on Windows, `npm`-installed bun resolves to `C:\...\bun.cmd` (a shim), not `bun.exe`. Node's `execFile('bun', ['--no-env-file', '-e', <3.2KB-script>])` then invokes the cmd shim. The shim mangles or truncates the multi-KB `-e` argument, so Bun receives nothing meaningful, exits 0 with empty stdout/stderr, and the dag-executor takes the success branch -- it emits `node_completed` instead of `node_failed`, and the test's `eventCalls.find(... 'node_failed' ...)` returns undefined. Verified directly: - `bun --no-env-file -e '<padding>; const x = "marker"; this is not valid javascript'` from PowerShell: exits 1, stderr ~422 bytes, contains `[eval]` marker. Works. - Same script via `execFile('bun', ...)` from Node on Windows: NO throw, empty stdout, empty stderr, exit 0. Broken. Production runtime is the Linux `archon-app` Docker container where `bun` is a single binary executed directly -- the cmd-shim path doesn't exist, the test passes there, and the production guard works. ## Action taken `it` -> `it.skipIf(isWindows)` with a 9-line comment explaining the cmd-shim mechanism + pointer to follow-up. Skipped only on Windows; still runs on Linux CI where the assertion is meaningful. Follow-up filed: **bluedevilcollectibles/bdc-xo#186** -- rewrite using a named-script fixture (writeFile to `.archon/scripts/`) instead of inline `-e`, which bypasses the shim and restores cross-platform coverage. ## Test plan - [x] `cd packages/workflows && bun test src/dag-executor.test.ts` -- before: 222 pass, 1 fail (this test). After: 222 pass, 1 skip, 0 fail. - [ ] Linux CI runs the test unskipped and it passes (existing behavior pre-WO-160).
This was referenced May 26, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes #1389.
When a
bash:orscript:node fails, the user-visible error message currently embeds the entire substituted script body. Node'spromisify(execFile)puts the body intoerr.message,err.cmd, and the first line oferr.stack; the old catch blocks interpolatederr.messageverbatim and logged the rawerr, so Pino's default serializer ended up writing the body three times per failure and the CLI / web UI / DB event all carried the same noisy string. Useful diagnostics (e.g.Expected ")" but found "x" at [eval]:4:241) were buried at the end.This change routes both catch blocks through a new
formatSubprocessFailure(err, label)helper inexecutor-shared.tsthat preferserr.stderr, strips theCommand failed: <cmd>prefix line, and tail-caps at 2 KB. The helper also returns a controlledlogFieldssubset so Pino never re-serializes message/stack/cmd.Root Cause
Two lines, one in each catch block:
dag-executor.ts:1325(bash) —errorMsg = \Bash node '${node.id}' failed: ${err.message}``dag-executor.ts:1582(script) —errorMsg = \Script node '${node.id}' failed: ${err.message}${stderrHint}``err.messagefrom Node'sExecFileExceptionis"Command failed: <cmd> <args>\n<stderr>". Forbash -c <body>/bun -e <body>, the first line contains the substituted script body. Compounded bygetLog().error({ err, … })which serializeserr.message+err.stack+err.cmd— three copies of the body in one log line.Changes
packages/workflows/src/executor-shared.tsformatSubprocessFailure(err, label)+RawSubprocessErrortypepackages/workflows/src/dag-executor.tslogFieldsinstead of rawerr; drop script-node redundantstderrHintpackages/workflows/src/executor-shared.test.tspackages/workflows/src/dag-executor.test.tsdata.erroron thenode_failedevent does not contain theCommand failed:prefix and caps at a small sizeTimeout / ENOENT / EACCES branches are preserved verbatim.
Before / After
Before (real output from the issue):
After:
Location markers (
at [eval]:L:C) are preserved so agents can still parse them for structured follow-up edits.Test plan
bun run type-check— 10/10 packagesbun test packages/workflows/src/executor-shared.test.ts— 58/58 pass (7 new)bun test packages/workflows/src/dag-executor.test.ts— 191/191 pass (2 new regression)bun run validate— clean exit 0 (bundled check, type-check, lint, prettier, full test suite)script:node with a body containing a syntax error, run the workflow, confirm the error is ~150 chars and preserves theat [eval]:L:ClocationScope
elsebranches in the bash/script node catch blocks and the new helperfailedAt: { line, col }field onNodeFailedEventgetLog().error({ err, … })callsites (AI-provider errors don't exhibit the same body-leak)@archon/gitexecFileAsyncprimitiveSummary by CodeRabbit