fix(workflows): make archon-adversarial-dev sed replacement macOS-safe#1155
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughReplaced an in-place Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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.
🧹 Nitpick comments (1)
packages/workflows/src/defaults/bundled-defaults.test.ts (1)
122-129: Harden this regression test to cover allsed -iforms and the requiredmvstep.At Line 128, the negative assertion only blocks one exact literal. It can miss other
sed -ivariants, and the test currently doesn’t verify the atomic replace command itself.Proposed test hardening
expect(content).toContain('STATE_TMP="$ARTIFACTS/state.json.tmp"'); expect(content).toContain( 'sed "s/SPRINT_COUNT_PLACEHOLDER/$SPRINT_COUNT/" "$ARTIFACTS/state.json" > "$STATE_TMP"' ); - expect(content).not.toContain('sed -i "s/SPRINT_COUNT_PLACEHOLDER/$SPRINT_COUNT/"'); + expect(content).toContain('mv "$STATE_TMP" "$ARTIFACTS/state.json"'); + expect(content).not.toMatch(/\bsed\s+-i\b/);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/workflows/src/defaults/bundled-defaults.test.ts` around lines 122 - 129, The test for 'archon-adversarial-dev init-workspace should avoid non-portable sed -i' is too narrow; update the assertions that inspect BUNDLED_WORKFLOWS['archon-adversarial-dev'] (variable content) to assert there is no use of any sed -i form (use a negative match against a regex covering -i, -i''/"" and -i with an arg) and also assert the script uses the safe two-step atomic replace: the redirected sed invocation (existing check for 'sed "s/SPRINT_COUNT_PLACEHOLDER/$SPRINT_COUNT/" "$ARTIFACTS/state.json" > "$STATE_TMP"') plus a subsequent mv of the temp file into place (e.g., contains mv "$STATE_TMP" "$ARTIFACTS/state.json"). Ensure these checks reference the same content variable and the test name 'archon-adversarial-dev init-workspace should avoid non-portable sed -i'.
🤖 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/defaults/bundled-defaults.test.ts`:
- Around line 122-129: The test for 'archon-adversarial-dev init-workspace
should avoid non-portable sed -i' is too narrow; update the assertions that
inspect BUNDLED_WORKFLOWS['archon-adversarial-dev'] (variable content) to assert
there is no use of any sed -i form (use a negative match against a regex
covering -i, -i''/"" and -i with an arg) and also assert the script uses the
safe two-step atomic replace: the redirected sed invocation (existing check for
'sed "s/SPRINT_COUNT_PLACEHOLDER/$SPRINT_COUNT/" "$ARTIFACTS/state.json" >
"$STATE_TMP"') plus a subsequent mv of the temp file into place (e.g., contains
mv "$STATE_TMP" "$ARTIFACTS/state.json"). Ensure these checks reference the same
content variable and the test name 'archon-adversarial-dev init-workspace should
avoid non-portable sed -i'.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5e15a5cf-eea3-4884-bed3-b7cb77e07e60
📒 Files selected for processing (2)
.archon/workflows/defaults/archon-adversarial-dev.yamlpackages/workflows/src/defaults/bundled-defaults.test.ts
|
Thanks for this, @LaplaceYoung — the fix is correct and the regression test is well-shaped. One blocker before merge: the bundled-defaults file wasn't regenerated, and the PR's own test reads from there. Per CLAUDE.md:
Right now `packages/workflows/src/defaults/bundled-defaults.generated.ts` still contains the old `sed -i "s/..."` literal — so:
To unblockFrom the repo root, on your branch: ```bash Once that lands, `bun run validate` should pass locally and we can merge. Really appreciate the fix — this hit a real macOS pain point. |
|
I didnot take my mac mini with me recently. So I wonder would u mind me seeking help from codex in the this PR? |
|
To use Codex here, create an environment for this repo. |
1 similar comment
|
To use Codex here, create an environment for this repo. |
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.
|
Pushed the regenerated bundle on your behalf (also merged in current PR #1345 closed as duplicate. Should be ready to merge once CI goes green. |
* fix(workflows): fail loudly on SDK isError results (coleam00#1208) (coleam00#1291) Previously, `dag-executor` only failed nodes/iterations when the SDK returned an `error_max_budget_usd` result. Every other `isError: true` subtype — including `error_during_execution` — was silently `break`ed out of the stream with whatever partial output had accumulated, letting failed runs masquerade as successful ones with empty output. This is the most likely explanation for the "5-second crash" symptom in coleam00#1208: iterations finish instantly with empty text, the loop keeps going, and only the `claude.result_is_error` log tips the user off. Changes: - Capture the SDK's `errors: string[]` detail on result messages (previously discarded) and surface it through `MessageChunk.errors`. - Log `errors`, `stopReason` alongside `errorSubtype` in `claude.result_is_error` so users can see what actually failed. - Throw from both the general node path and the loop iteration path on any `isError: true` result, including the subtype and SDK errors detail in the thrown message. Note: this does not implement auto-retry. See PR comments on coleam00#1121 and the analysis on coleam00#1208 — a retry-with-fresh-session approach for loop iterations is not obviously correct until we see what `error_during_execution` actually carries in the reporter's env. This change is the observability + fail-loud step that has to come first so that signal is no longer silent. Co-authored-by: Claude Opus 4.7 <noreply@anthropic.com> (cherry picked from commit 4c6ddd9) * fix(db): throw on corrupt commands JSON instead of silent empty fallback (coleam00#1033) * fix(db): throw on corrupt commands JSON instead of silent empty fallback (coleam00#967) getCodebaseCommands() silently returned {} when the commands column contained corrupt JSON. Callers had no way to distinguish 'no commands' from 'unreadable data', violating fail-fast principles. Now throws a descriptive error with the codebase ID and a recovery hint. The error is still logged for observability before throwing. Adds two test cases: corrupt JSON throws, valid JSON string parses. * fix: include parse error in log for better diagnostics (cherry picked from commit 39a05b7) * fix(isolation): raise worktree git-operation timeout to 5m (coleam00#1306) All 15 worktree git-subprocess timeouts in WorktreeProvider were hardcoded at 30000ms. Repos with heavy post-checkout hooks (lint, dependency install, submodule init) routinely exceed that budget and fail worktree creation. Consolidate them onto a single GIT_OPERATION_TIMEOUT_MS constant at 5 min. Generous enough to cover reported cases while still catching genuine hangs (credential prompts in non-TTY, stalled fetches). Chosen over the config-key approach in coleam00#1029 to avoid adding permanent .archon/config.yaml surface for a problem a raised default solves cleanly. If 5 min turns out to also be too tight for real-world use, we'll revisit. Closes coleam00#1119 Supersedes coleam00#1029 Co-authored-by: Shay Elmualem <12733941+norbinsh@users.noreply.github.com> (cherry picked from commit cc78071) * fix(web,server): show real platform connection status in Settings (coleam00#1061) The Settings page's Platform Connections section hardcoded every platform except Web to 'Not configured', so users couldn't tell whether their Slack/ Telegram/Discord/GitHub/Gitea/GitLab adapters had actually started. - Server: /api/health now returns an activePlatforms array populated live as each adapter's start() resolves. Passed into registerApiRoutes so the reference stays mutable — Telegram starts after the HTTP listener is already accepting requests, so a snapshot would miss it. - Web: SettingsPage.PlatformConnectionsSection now reads activePlatforms from /api/health and looks each platform up in a Set. Also adds Gitea and GitLab to the list (they already ship as adapters). Closes coleam00#1031 Co-authored-by: Lior Franko <liorfr@dreamgroup.com> (cherry picked from commit 08de8ee) * fix: initialize options.hooks before merging YAML node hooks (coleam00#1177) When a workflow node defines hooks (PreToolUse/PostToolUse) in YAML but no hooks exist yet on the options object, applyNodeConfig crashes with "undefined is not an object" because it tries to assign properties on the undefined options.hooks. Initialize options.hooks to {} before the merge loop. Reproduces with: archon workflow run archon-architect (which uses per-node hooks extensively). Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com> (cherry picked from commit 7ea3214) * fix: detect completion signal in any XML tag, not just <promise> (coleam00#1126) (coleam00#1184) * fix: detect completion signal in any XML tag, not just <promise> (coleam00#1126) Loop nodes with `until:` reported max_iterations_reached when the AI wrapped the completion signal in XML tags other than `<promise>` (e.g., `<COMPLETE>ALL_CLEAN</COMPLETE>`). The three existing regex patterns all missed this format, causing the loop to exhaust iterations and fail. Changes: - Add generic XML-wrapped signal pattern to `detectCompletionSignal` - Extend `stripCompletionTags` to strip matched XML-wrapped signals from output - Pass `loop.until` to `stripCompletionTags` call site in dag-executor - Add unit tests for detection and stripping of XML-wrapped signals - Add integration test for loop completing on final iteration with XML tags Fixes coleam00#1126 * fix: address review findings for completion signal detection - Update detectCompletionSignal JSDoc to document all three detection formats - Update stripCompletionTags JSDoc to mention the `until` parameter - Remove superfluous `m` flag from xmlWrappedPattern (no anchors, no effect) - Document that XML tag names are matched independently (intentional permissiveness) - Add test: detects signal in mismatched XML tags (permissive behavior) - Add test: strips both <promise> and XML-tagged signal in same chunk - Add assertion in DAG integration test that raw XML tags don't appear in sent messages * simplify: reduce complexity in changed files * fix: require matching XML tag names in completion-signal detection Follow-up to the initial broadening in this PR. The first version of the regex accepted mismatched open/close tags (e.g. `<COMPLETE>X</done>`) which was a small false-positive surface when the AI interleaves tags in prose. Tightens both detectCompletionSignal and stripCompletionTags to capture the tag name and enforce it on the close via \1 backreference. Case-insensitivity on the tag name is preserved. Test updates: - Flip the "permissive mismatch" case to assert strict rejection with a comment explaining the guard. - Add a case-insensitive matching case to lock that behavior in. No behavior change for workflows that use matching tags (the overwhelming common case) or for <promise>...</promise>. Behavior change is limited to the narrow "open tag and close tag disagree" case, which only happens when the AI is confused — in which case we'd rather report max_iterations_reached and let the author inspect than silently call the loop complete. (cherry picked from commit bc25dee) * fix(web): allow deleting nodes from Workflow Builder (coleam00#971) (coleam00#1113) * fix(web): allow deleting nodes from Workflow Builder (coleam00#971) Three independent gaps prevented users from deleting nodes added to the Workflow Builder canvas: dropped nodes were never auto-selected so keyboard shortcuts silently no-oped, no right-click context menu existed, and the Delete Node button was buried in the Advanced tab (hidden below the viewport for Prompt/Command, completely absent for Bash since bash nodes have no Advanced tab). Fixes coleam00#971. * fix(web): push undo snapshot before adding nodes on canvas Call onPushSnapshot() before setNodes() in both onDrop and quick-add handlers so that node additions are captured by undo/redo history. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix(web): address PR coleam00#1113 review feedback - Hold nodes/edges in refs so handleNodeDeleteById and onPushSnapshot can't capture stale pre-drop state (fixes undo-stack correctness). - Clamp context-menu x/y to viewport so right-click near edges stays fully on-screen. - Drop non-conformant role=menu/menuitem from the single-item context menu; rely on the native button for accessibility. - Extend isInputTarget() to cover ARIA combobox/textbox/searchbox so Backspace in Radix/shadcn widgets never nukes a node. - Extract handleBuilderKeydown as a pure function and add tests covering the Delete/Backspace + isInputTarget invariant. - Remove issue-number references from code comments per CLAUDE.md. - Document the new delete affordances in the Workflow Builder docs. - Inline context-menu dismissal, rename pointer handler, drop unused deps in keyboardActions useMemo. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com> (cherry picked from commit d7f36b2) * 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> (cherry picked from commit 817186d) * fix(deps): override transitive axios to ^1.15.0 for CVE-2025-62718 (coleam00#1330) axios <1.15.0 can be coerced to bypass NO_PROXY rules via hostname normalization, enabling SSRF in the right network shape. Archon pulls axios transitively through @slack/bolt (^1.12.0) and @slack/web-api (^1.13.5); before this change bun.lock resolved axios@1.13.6 — within the vulnerable range. Adding "axios": "^1.15.0" to the root package.json overrides bumps the transitive resolution to axios@1.15.1 (latest compatible 1.x). Both Slack range specs accept it without API surface changes — no downstream code touches axios directly. Supersedes coleam00#1153. Credits @stefans71 for identifying and reporting the vulnerability; their PR was stale on the lockfile (0.3.5 → 0.3.6 drift on dev), so this is a fresh one-line re-do on current dev. Closes coleam00#1053. Co-authored-by: Stefans71 <stefans71@users.noreply.github.com> (cherry picked from commit ae2d936) * fix(cli): surface stale-workspace registration error instead of fake "not a git repo" (coleam00#1332) * fix(cli): surface stale-workspace registration error instead of fake "not a git repo" When workflowRunCommand auto-registers an unregistered repo, a stale ~/.archon/workspaces/<owner>/<repo>/source symlink (pointing to an old checkout) causes createProjectSourceSymlink() in @archon/paths to throw: Source symlink at <linkPath> already points to <existing>, expected <target> The CLI caught that in a try/catch, logged it at warn level, continued with `codebase = null`, and then the isolation / resume branches hit their "codebase missing" fallback and threw the generic: Cannot create worktree: not in a git repository. That message is false — the repo is valid; the Archon workspace entry is stale. It sends users down the wrong diagnostic path (checking git config, permissions, etc.) instead of pointing at the workspace dir. Fix: preserve the registration error on a new `codebaseRegistrationError` local, and at both fallback sites (resume + worktree-creation) check it before the generic "not a git repo" branch. When set, throw a truthful: Cannot {create worktree,resume}: repository registration failed. Error: <original message> Hint: Remove the stale workspace entry at <dir> and retry, or use --no-worktree to skip isolation. The hint's exact path comes from a small parser that extracts the workspace directory from the known "Source symlink at …" format; when the message shape doesn't match (future error text changes), the parser returns null and we fall back to a generic "check registration under <archon-home>/workspaces" hint — safe degradation. Regression test in workflow.test.ts asserts the new error message and negatively asserts the old "not in a git repository" string is gone. Supersedes coleam00#1157 — that PR was draft + CONFLICTING against current dev, and also mentioned Windows test-compat changes that weren't in the diff (pruned scope). This is a fresh re-do focused strictly on coleam00#1146. Closes coleam00#1146. Co-authored-by: Bortlesboat <Bortlesboat@users.noreply.github.com> * review: add resume-path test, null-fallback test, update troubleshooting docs Addresses multi-agent review feedback on this PR: - Add regression test for the --resume fallback site (the worktree-create site was already covered; the resume site had identical wiring but zero test coverage). - Add test for the unrecognized-error-shape branch of buildRegistrationFailureError so the generic workspace hint is pinned (prevents accidental inversion of the stale-entry vs generic-hint ternary). - Update the troubleshooting page to key on the new "Cannot create worktree: repository registration failed." message. Users hitting the new error won't find the page under the old heading, and the "In the future..." note is obsolete now that the error itself contains the cleanup path. - Trim both new docblocks: keep the load-bearing cross-package error string contract in extractStaleWorkspaceEntry, drop narration of what the code already shows. Drop the "Before this helper existed..." paragraph from buildRegistrationFailureError — that's CHANGELOG material. Drop PR-reference suffix from the test section divider. * review: guard getArchonHome in hint + export parser for direct tests Two follow-up fixes to the multi-agent review commit (f32f002): CodeRabbit finding — unguarded getArchonHome() in the fallback hint. If getArchonHome() ever throws (misconfigured env vars, permission issues on the resolution path), the registration-failure Error would never get constructed: we'd throw a secondary home-resolution error that masks the root cause. Wrap the fallback branch in try/catch — prefer losing the exact path in the hint over replacing the actionable registration error. A safe generic hint ("Check your Archon workspace registration and retry") takes over when getArchonHome() throws. The original error.message is always embedded verbatim in the re-thrown Error. S2 — export extractStaleWorkspaceEntry for direct table tests. The parser is where the cross-package string contract with @archon/paths actually lives; direct tests against it are cheaper than end-to-end CLI tests and pin the edge cases: - POSIX path with forward slashes (typical unix user) - Windows path with backslashes (verifies Math.max(lastIndexOf / , lastIndexOf \)) - Unrelated error message (no prefix) → null - Prefix matches but delimiter missing → null - Source path without any separator → null (guards against returning empty string, which would produce a nonsense "Remove the stale workspace entry at " hint) - Empty string → null Six new cases in the test file. The claim of Windows support in the PR description is now actually verified. * fix(test): make generic-hint assertion path-separator agnostic Windows test runner (CI) hit: Expected to contain: "Check your Archon workspace registration under /home/test/.archon/workspaces" Received: "... under \home\test\.archon\workspaces and retry, ..." path.join normalizes to `\` on Windows and `/` on POSIX. The test hardcoded forward slashes in the expected substring. Split into two separator-agnostic asserts: the prefix up to "under", then `/workspaces\b/` regex for the final path segment. Behavior doesn't change — the hint still gets the full path.join'd workspaces dir on either platform. --------- Co-authored-by: Bortlesboat <Bortlesboat@users.noreply.github.com> (cherry picked from commit 056707d) * fix(server,web,workflows): web approval gates auto-resume + reject-with-reason dialog (coleam00#1329) * fix(server,web,workflows): web approval gates auto-resume + reject-with-reason dialog Fixes three tightly-coupled bugs that made web approval gates unusable: 1. orchestrator-agent did not pass parentConversationId to executeWorkflow for any web-dispatched foreground / interactive / resumable run. Without that field, findResumableRunByParentConversation (the machinery the CLI relies on for resume) couldn't find the paused run from the same conversation on a follow-up message, and the approve/reject API handlers had no conversation to dispatch back to. 2. POST /api/workflows/runs/:runId/{approve,reject} recorded the decision and returned "Send a message to continue the workflow." — the workflow never actually resumed. Added tryAutoResumeAfterGate() that mirrors what workflowApproveCommand / workflowRejectCommand already do on the CLI: look up the parent conversation, dispatch `/workflow run <name> <userMessage>` back through dispatchToOrchestrator. Failures are non-fatal — the user can still send a manual message as a fallback. 3. The during-streaming cancel-check in dag-executor aborted any streaming node whenever the run status left 'running', including the legitimate transition to 'paused' that an approval node performs. A concurrent AI node in the same DAG layer now tolerates 'paused' and finishes its own stream; only truly terminal / unknown states (null, cancelled, failed, completed) abort the in-flight stream. Web UI: ConfirmRunActionDialog gains an optional reasonInput prop (label + placeholder) that renders a textarea and passes the trimmed value to onConfirm. WorkflowRunCard (dashboard) and WorkflowProgressCard (chat) both use it for Reject now — the chat card was still on window.confirm, which was both inconsistent with the dashboard and couldn't collect a reason. The trimmed reason threads through to $REJECTION_REASON in the workflow's on_reject prompt. Supersedes coleam00#1147. @jonasvanderhaegen surfaced the root cause and shape of the fix; that PR was 87 commits stale and pre-dated the reject-UX upgrade (coleam00#1261 area), so this is a fresh re-do on current dev. Tests: - packages/server/src/routes/api.workflow-runs.test.ts — 5 new cases: approve with parent dispatches; approve without parent returns "Send a message"; approve with deleted parent conversation skips safely; reject dispatches on-reject flows; reject that cancels (no on_reject) does NOT dispatch. - packages/core/src/orchestrator/orchestrator.test.ts — updated the two synthesizedPrompt-dispatch tests for the new executeWorkflow arity. Closes coleam00#1131. Co-authored-by: Jonas Vanderhaegen <7755555+jonasvanderhaegen@users.noreply.github.com> * fix: address multi-agent review findings for web approval auto-resume C1 (critical) — cross-adapter misrouting guard tryAutoResumeAfterGate now checks parentConv.platform_type === 'web' before dispatching. Non-web parents (Slack/Telegram/GitHub/Discord) being approved from the dashboard skip auto-resume rather than dispatching a Slack thread_ts or Telegram chat_id through the web adapter's lock manager. C2 (critical) — fire-and-forget dispatch replaced with await void dispatchToOrchestrator() meant the "Resuming workflow." response fired before async work completed, and the outer try/catch couldn't observe dispatch failures. Changed to await; response now accurately reflects dispatch outcome. I1 — replaced logPrefix string-template (which produced 3-segment api.workflow_*.dispatched event names violating {domain}.{action}_{state}) with literal event names per action, branched inside the helper. Accepts action: 'approve' | 'reject' instead. I2 — corrected misleading "foreground/interactive" qualifier in the approve-endpoint comment; background web dispatches also set parent_conversation_id via the pre-created run, so they auto-resume too. I3 — extracted shouldContinueStreamingForStatus() as a small exported policy and added 7 unit tests covering running/paused/null/cancelled/ failed/completed/unknown. Full-integration coverage of the paused- tolerance invariant would require manipulating the 10s CANCEL_CHECK_INTERVAL_MS, which is flaky-prone; unit test of the policy function captures the same invariant deterministically. I4 — updated approval-nodes.md and authoring-workflows.md to reflect that Web UI approve/reject now auto-resumes (no "send a follow-up message" copy), documented the reject-with-reason dialog and $REJECTION_REASON flow, and called out the cross-platform caveat. S1 — rewrote streaming status check as positive shouldContinue safe-list via the extracted policy function, matching the inline comment. S2 — inlined handleReject on the dashboard rather than squeezing rejectWorkflowRun through runAction with a closure; keeps runAction narrow for the single-arg lifecycle actions. S5 — new regression test covering the non-web-parent skip path (slack-platform parent → dispatch skipped → response falls back to "Send a message to continue"). S6 — removed stale reference to runAction in ConfirmRunActionDialog's onConfirm JSDoc (no longer accurate now that WorkflowProgressCard calls the dialog without runAction). S7 — fixed misleading "user can resume manually by sending any message" docstring (resume is triggered by re-running the workflow command, not by an arbitrary message). Skipped as out-of-scope: S3 — cancelWorkflowRun rowCount check (pre-existing defect; separate PR) S4 — tightening expect.anything() to UUID regex (deferred) S8 — 12-positional-arg executeWorkflow → options-bag refactor (tracked follow-up) bun run validate green locally; 68 tests in api.workflow-runs.test.ts (up from 67), 173 in dag-executor.test.ts (up from 166). * review: close I1/I2/I3/I4/I6 — paused tolerance in loop + emitter, resume test, useId I1 (loop inter-iteration check) — dag-executor.ts:1715 Used `!== 'running'` in the loop node's between-iteration status check. A sibling approval node pausing the run in the same topological layer would abort the loop mid-iteration with "Loop node '<id>' stopped at iteration N (paused)". Switched to the shared shouldContinueStreamingForStatus helper so paused is tolerated — same semantics the streaming check got. Extended inline comment explains the sibling-layer concurrency reason. I2 (skipIfStatusChanged emitter unregister) — dag-executor.ts:2886 At DAG-finalization writes the helper correctly skipped writing on any non-running state (paused included — don't mark a paused run complete), but it *also* called getWorkflowEventEmitter().unregisterRun() which broke SSE observability for a run that's still live (waiting for user approval). Split the two responsibilities: skip the write for all non-running states, but only unregister the emitter for terminal states (cancelled / deleted / completed / failed). `paused` keeps the emitter registered so resume stays visible on the dashboard. I3 (foreground_resume_detected branch untested) — orchestrator-agent.test.ts That branch was modified as part of the original fix (added parentConversationId as 11th positional arg) but no existing test configured mockFindResumableRunByParentConversation to return non-null. A positional mistake (e.g. accidentally swapping issueContext and parentConversationId) would silently break auto-resume with no failing test. New regression test configures the mock, asserts both the cwd comes from the resumable run's working_path AND parentConversationId is passed correctly at position 10. I4 (null-parent log level) — api.ts tryAutoResumeAfterGate `getConversationById` returning null is a data-integrity signal (the parent conversation was deleted while the run was paused) — worth surfacing at info level so operators notice, not hiding at debug. Missing platform_conversation_id on an existing row would be an unusual DB state and stays at debug. Added `parentDeleted: boolean` to the log context so the two cases are distinguishable in observability. I6 (hardcoded DOM id) — ConfirmRunActionDialog.tsx `id="confirm-run-action-reason"` collided when multiple dialog instances share the same page (Radix portals mitigate in practice but the code was fragile). Switched to React.useId() so each instance gets a unique id — htmlFor/id wiring preserved. S11 (arity-only assertion) — orchestrator-agent.test.ts:1092 area The interactive-workflow-on-web test asserted mockExecuteWorkflow was called, but nothing about the args. Added a specific assertion that position 10 (parentConversationId) equals 'conv-1' (the caller conversation id) — pins the wiring that I1/I2 depend on being correct. Deferred (from review S1-S10, I5, I7): - S1 (ExecuteWorkflowOptions bag) — tracked as standalone follow-up; 12 positional args with 2 adjacent optionals is a real maintenance hazard but the refactor deserves its own PR. - S7 (WHY comment on non-web else branch) — review text says the branch "correctly omits" parentConversationId but the code passes it; the combination with the web-parent guard in tryAutoResumeAfterGate is intentional. Not adding a justify-what-we-don't-do comment. - S2/S3/S4/S5/S8/S9/S10 — pure polish (event-map ternary, platformConvId inlining, shared constant for REJECTION_REASON_INPUT, onChange arrow shorthand, discriminated union, docblock trim, suffix comment drop) - I5 (soften "Resuming workflow." to "— check the dashboard for progress") — users clicking from the dashboard are already on the dashboard; the current text is accurate (enqueue completed) and concise. - I7 (test dispatch-throws path) — covered implicitly by the try/catch branch of tryAutoResumeAfterGate returning false; a direct test would require mocking handleMessage to throw and would couple to dispatchToOrchestrator internals. bun run validate green; 189 dag-executor tests, 98 orchestrator-agent tests, 68 api.workflow-runs tests — all the new cases pass. --------- Co-authored-by: Jonas Vanderhaegen <7755555+jonasvanderhaegen@users.noreply.github.com> (cherry picked from commit d5c1cd9) * feat(providers): autodetect canonical binary install paths for Claude and Codex (coleam00#1361) Both binary resolvers previously stopped at env-var + explicit config and threw a "not found" error when neither was set. Users who followed the upstream-recommended install flow (Anthropic's `curl install.sh` for Claude, `npm install -g @openai/codex`) still had to manually set either `CLAUDE_BIN_PATH` / `CODEX_BIN_PATH` or the corresponding config field before any workflow could run. Add a tier-N autodetect step between the explicit config tier and the install-instructions throw. Purely additive: env and config still win when set (precedence covered by new tests). On autodetect miss, the same install-instructions error fires as before. Claude probe list (verified against docs.claude.com "Uninstall Claude Code → Native installation" section): - $HOME/.local/bin/claude (mac/linux native installer) - $USERPROFILE\.local\bin\claude.exe (Windows native installer) Codex probe list (verified against openai/codex README; npm global- install puts the binary at `{npm_prefix}/bin/<name>` on POSIX, `{npm_prefix}\<name>.cmd` on Windows): - $HOME/.npm-global/bin/codex (user-set `npm config set prefix`) - /opt/homebrew/bin/codex (mac arm64 with homebrew-node) - /usr/local/bin/codex (mac intel / linux system node) - %APPDATA%\npm\codex.cmd (Windows npm global default) - $HOME\.npm-global\codex.cmd (Windows user-set prefix) Not probed (explicit override still required): - Custom npm prefixes — `npm root -g` would need a subprocess per resolve, too much surface for a probe helper - `brew install --cask codex` — cask layout isn't a PATH binary - Manual GitHub Releases extracts — placement is user-determined - `~/.bun/bin/codex` — not documented in openai/codex README Pi provider intentionally has no equivalent change: the Pi SDK is bundled into the archon binary (no subprocess), so there's no "binary" to resolve. Pi auth lives at `~/.pi/agent/auth.json` which the SDK already finds by default, and the PR A shim (`PI_PACKAGE_DIR`) handles the package-dir case via Pi's own documented escape hatch. E2E verified: removed both config entries from ~/.archon/config.yaml, rebuilt compiled binary, ran `archon workflow run archon-assist` and a Codex workflow. Logs showed `source: 'autodetect'` for both, responses returned cleanly. (cherry picked from commit b99cee4) * fix(providers/test): use os.homedir() instead of $HOME in claude binary autodetect test The native-installer autodetect test computed its expected path from process.env.HOME, but the implementation uses node:os homedir(). On Windows, HOME is typically unset (Windows uses USERPROFILE), so the test fell back to '/Users/test' while the resolver returned the real home dir — making the spy's path-equality check fail and breaking CI on windows-latest. Mirror the implementation by importing homedir() from node:os and joining with node:path so the expected path matches the actual platform-resolved home and separator. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> (cherry picked from commit f9f8775) * fix(server): contain Discord login failure so it doesn't kill the server (coleam00#1365) Reported in coleam00#1365: a user running `archon serve` with DISCORD_BOT_TOKEN set but the "Message Content Intent" toggle disabled in the Discord Developer Portal saw the entire server crash with `Used disallowed intents`. Discord rejects the gateway connection (close code 4014) when a privileged intent is requested without being enabled, and the unguarded `await discord.start()` propagated the error all the way up, taking the web UI down with it. Wrap discord.start() in try/catch — log the failure with an actionable hint (special-cased for the disallowed-intent error) and continue running. Other adapters and the web UI come up regardless. The shutdown handler already uses optional chaining (`discord?.stop()`) so nulling discord after a failed start is safe. Other adapters (Telegram, Slack, GitHub, Gitea, GitLab) have the same unguarded-start pattern but are out of scope for this fix — addressing them is tracked separately. Also expanded the Discord setup docs with a caution callout that names the exact error string and the new log event so users can grep for both. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> (cherry picked from commit 5957c6e) --------- Co-authored-by: Cole Medin <cole@dynamous.ai> Co-authored-by: Claude Opus 4.7 <noreply@anthropic.com> Co-authored-by: Kagura <kagura.chen28@gmail.com> Co-authored-by: Rasmus Widing <152263317+Wirasm@users.noreply.github.com> Co-authored-by: Shay Elmualem <12733941+norbinsh@users.noreply.github.com> Co-authored-by: Lior Franko <lior.franko@ironsrc.com> Co-authored-by: Lior Franko <liorfr@dreamgroup.com> Co-authored-by: Alex Siri <alexsiri7@gmail.com> Co-authored-by: Ahmed <44034059+medevs@users.noreply.github.com> 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: Stefans71 <stefans71@users.noreply.github.com> Co-authored-by: Bortlesboat <Bortlesboat@users.noreply.github.com> Co-authored-by: Jonas Vanderhaegen <7755555+jonasvanderhaegen@users.noreply.github.com>
…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>
Summary
Fixes the
archon-adversarial-devinit-workspace step for macOS BSDsedcompatibility.Issue #1103 reports that
sed -ifails on macOS because BSD sed requires a backup extension, while GNU sed accepts bare-i.This PR replaces the non-portable in-place edit with a portable temp-file pattern:
sed -i s/.../.../ filesed s/.../.../ file > file.tmp && mv file.tmp fileChanges
.archon/workflows/defaults/archon-adversarial-dev.yamlinit-workspaceto writestate.jsonvia temp file (state.json.tmp) and atomic move.packages/workflows/src/defaults/bundled-defaults.test.tssed -ifor this replacement.Validation
bun test packages/workflows/src/defaults/bundled-defaults.test.tsCloses #1103
Summary by CodeRabbit
Chores
Tests