Skip to content

feat(cli): extend --detach to workflow approve/reject/resume control verbs - #2204

Open
buun-dev wants to merge 5 commits into
coleam00:devfrom
buun-dev:feat/detach-control-verbs
Open

feat(cli): extend --detach to workflow approve/reject/resume control verbs#2204
buun-dev wants to merge 5 commits into
coleam00:devfrom
buun-dev:feat/detach-control-verbs

Conversation

@buun-dev

@buun-dev buun-dev commented Jul 21, 2026

Copy link
Copy Markdown
Contributor

Summary

  • Problem: --detach existed only on workflow run. The control verbs (approve, reject, resume) hosted the executor in the calling shell, so a reaped shell — a harness task, a closed terminal, a dropped SSH session — left the run wedged mid-resume: approval recorded, workflow never continued.
  • Why it matters: These are exactly the commands automation drives. An approve that records the decision and then dies leaves a run paused forever with no signal that anything went wrong.
  • What changed: --detach now works on all three control verbs. The parent validates the run read-only and spawns a detached child that owns every state mutation in its own process group. The precondition gate is extracted into assertApprovable/assertRejectable in @archon/core so the parent's precheck and the operation itself are literally the same code. --detach --json gains continues: true, and the child is now spawned with the caller's --cwd.
  • What did not change (scope boundary): No change to the non-detached paths of any verb. No change to resumeWorkflow's gate (its precheck already called the shared operation). No change to the chat/HTTP/manage_run surfaces beyond the behavior-preserving extraction. workflow run --detach is untouched.

UX Journey

Before

  Operator                 archon (your shell)          Workflow run
  ────────                 ───────────────────          ────────────
  approve <id> ─────────▶  records approval ──────────▶ paused → resolved
                           resumes inline
                           runs the executor ─────────▶ running ...........
        shell dies ──────▶ ✗ process killed              ✗ WEDGED
                                                          (approved, never resumed)

  approve <id> --detach    [not supported — flag only existed on `workflow run`]

After

  Operator                 archon parent            detached child          Workflow run
  ────────                 ─────────────            ──────────────          ────────────
  approve <id> --detach ▶  [*precheck: READ-ONLY*]
                           4 gates, zero writes
                              │
                              ├─ refused? ──────────────────────────────▶ untouched
                              │  {ok:false} to your face, NOTHING spawned
                              │
                              └─ ok ─▶ spawn ─────▶ records approval ────▶ paused → resolved
                           {ok:true,                resumes inline
                            continues:true}         runs executor ───────▶ running ......
                           exits                       │
        shell dies ──────▶ (already gone)              └─ keeps running ──▶ ✓ completed

Architecture Diagram

Before

  packages/cli/src/commands/workflow.ts
    workflowApproveCommand ──┐
    workflowRejectCommand  ──┼─▶ runDetachedControlCommand
    workflowResumeCommand  ──┘        │
                                      ├─▶ [inline precheck: status ONLY]   ← partial copy
                                      └─▶ spawnDetachedWorkflowRun(process.cwd())
                                                                  ▲
                                                     caller's --cwd discarded here

  packages/core/src/operations/workflow-operations.ts
    approveWorkflow ──▶ [inline gate: status, context, child_workflow, resolved]
    rejectWorkflow  ──▶ [inline gate: status, child_workflow, resolved]

  The two gates are unrelated code. They already drifted 3 checks apart.

After

  packages/cli/src/commands/workflow.ts
    workflowApproveCommand ──┐
    workflowRejectCommand  ──┼─▶ runDetachedControlCommand(runId, action, json, [+]cwd, precheck)
    workflowResumeCommand  ──┘        │
                                      ├─▶ precheck ===▶ assertApprovable / assertRejectable
                                      └─▶ spawnDetachedWorkflowRun(cwd ?? process.cwd())  [~]

  packages/core/src/operations/workflow-operations.ts
    [+] assertApprovable(run): ApprovalContext            ◀=== CLI precheck
    [+] assertRejectable(run): ApprovalContext | undefined ◀=== CLI precheck
    approveWorkflow [~] ===▶ assertApprovable
    rejectWorkflow  [~] ===▶ assertRejectable

  ONE gate. The parent cannot validate less than the child enforces.

Connection inventory (list every module-to-module edge, mark changes):

From To Status Notes
cli/commands/workflow.ts (approve precheck) core/operations/workflow-operations.assertApprovable new Replaces an inline status-only copy of one of four gates
cli/commands/workflow.ts (reject precheck) core/operations/workflow-operations.assertRejectable new Replaces an inline status-only copy of one of three gates
core.approveWorkflow core.assertApprovable new Extraction; error strings copied verbatim
core.rejectWorkflow core.assertRejectable new Extraction; error strings copied verbatim
runDetachedControlCommand spawnDetachedWorkflowRun modified Now passes the command's cwd, not process.cwd()
cli/cli.ts workflow{Approve,Reject,Resume}Command modified Threads --detach (5th arg, after upstream's cwd)
cli/commands/workflow.ts (resume precheck) core.resumeWorkflow unchanged Already called the shared operation — its single gate was never duplicated
core.{approve,reject}Workflow db/workflows.resolveApprovalGate unchanged The CAS remains the real arbiter for concurrent resolution
chat manage_run / HTTP API core.{approve,reject}Workflow unchanged Same behavior; they inherit the extraction transparently

Label Snapshot

  • Risk: risk: low
  • Size: size: M
  • Scope: cli, core, docs
  • Module: cli:workflow, core:operations

Change Metadata

  • Change type: feature (with an embedded bug fix for cwd, and a refactor for the shared gate)
  • Primary scope: cli

Linked Issue

Validation Evidence (required)

bun run check:bundled && bun run check:bundled-skill && bun run check:bundled-schema \
  && bun run check:pi-vendor-map && bun run check:capability-matrix     # all OK
bun run type-check                                                      # 10/10 packages clean
bun run lint --max-warnings 0                                           # clean
bun run format:check                                                    # clean
bun test packages/cli/src/commands/workflow.test.ts                     # 199 pass / 0 fail
bun test packages/core/src/operations/workflow-operations.test.ts       #  49 pass / 0 fail
bun test packages/core/src/orchestrator/manage-run-tool.test.ts         #  30 pass / 0 fail (unchanged)
bun run test                                                            # see caveat below
  • Evidence provided:

    • CLI suite packages/cli/src/commands/workflow.test.ts: 199 pass / 0 fail (193 pre-change baseline + 6 new).
    • Core suite packages/core/src/operations/workflow-operations.test.ts: 49 pass / 0 fail (41 baseline + 8 new).
    • The two pre-existing child_workflow refusal tests pass unchanged. That is the proof the gate extraction is behavior-preserving — the error strings were copied verbatim, so no caller-visible message changed.
    • manage-run-tool.test.ts (the only file that mock.modules workflow-operations) passes unchanged at 30/30. Because mock.module() merges rather than replaces, the two new exports keep their real implementations there; manage-run-tool.ts calls only the mocked operations, never the validators.
    • Compiled-binary verification: see Human Verification below.
  • If any command is intentionally skipped, explain why:

    • bun run validate cannot complete on a Windows host: the chain reaches bun run test:install, and scripts/test-install.sh exits immediately with Windows is not supported. Please use WSL2. The eight checks before it all pass; the && chain then short-circuits, so bun run test was run separately.
    • bun run test (full per-package sweep, all 8 test-bearing packages) produced exactly one failure: @archon/providerspathKind > returns "missing" for a broken symlink without throwing, failing with EPERM: operation not permitted, symlink. That is Windows refusing symlink creation without elevation/Developer Mode — an environment limit in a package this branch does not touch. Every other package passed.

Security Impact (required)

  • New permissions/capabilities? No. --detach re-invokes the same CLI with the same argv minus --detach/--json; the child has exactly the parent's authority.
  • New external network calls? No.
  • Secrets/tokens handling changed? No. No credential path is read, written, logged, or forwarded differently.
  • File system access scope changed? Yes — narrowed. runDetachedControlCommand previously spawned into process.cwd() regardless of the caller's --cwd. It now spawns into the cwd the caller actually asked for. This reduces the set of directories a detached child can land in and removes a case where the child could be started outside any git repo after the parent had already acked success. The run's working_path remains a non-candidate by design: a container run's working_path is a distro path the host cannot spawn into, so the child re-resolves everything by run id.

Compatibility / Migration

  • Backward compatible? Yes. --detach is purely additive on all three verbs; without it every verb behaves exactly as before. continues is an additive field on an ack that only exists under --detach --json, a combination that shipped nowhere before this PR. The extracted validators throw the same error strings the inline checks did.
  • Config/env changes? No.
  • Database migration needed? No.
  • If yes, exact upgrade steps: n/a.

Human Verification (required)

Verified against a compiled Windows binary, not bun run cli — dev mode is precisely where the Bun single-file-executable argv bug never appears, so dev-mode evidence cannot cover it.

Binary: bun build --compile --minify --target=bun-windows-x64 with BUNDLED_IS_BINARY = true; self-reports Archon CLI v0.6.0 / Platform: win32-x64 / Build: binary / Git commit: 6af8eb2a. Driven against an isolated ARCHON_HOME (throwaway SQLite DB) and a throwaway two-bash-nodes-around-one-approval-gate workflow, so the runs are real and cost no AI tokens.

  • Verified scenarios:

    1. approve <id> --detach --json → ack {ok:true, detached:true, continues:true}. Child log contains no Unknown command: /$bunfs/root/… and no Unknown command: B:/~BUN/root/…. Child log shows [after] Completed (118ms)past the gateWorkflow completed successfully.; workflow get reports "status": "completed". The run genuinely advanced — the ack alone would not have proven this, since a pre-fix binary acks success too.
    2. reject <id> --detach --json → ack continues: true; no Unknown command; child log Rejected and cancelled: smoke-detach-gate; workflow get reports "status": "cancelled".
    3. resume <id> --detach --json → ack continues: true; no Unknown command; child re-invoked, logged [before] Skipped (prior_success) and re-paused at the gate as expected.
    4. Precheck refusal on a gate this PR newly enforces in the parent — the important one. Recorded an approval with plain --json (run stays paused, gate resolved), then ran approve <id> --detach --json again: {"ok": false, "error": "Workflow run … was already approved and is awaiting resume."}, and that run's child-log file stayed byte-identical (1965 → 1965), proving nothing was spawned. Before this PR the parent acked {ok:true} here and the child died unseen.
    5. Precheck refusal on the pre-existing status gate — approve <completed-run-id> --detach --json{"ok": false, "error": "Cannot approve run with status 'completed'. Only paused runs can be approved."}; log-file count unchanged (3 → 3) and that run's log byte-identical (1718 → 1718). Nothing spawned.
  • Edge cases checked: reject deliberately tolerates a malformed/absent approval context where approve refuses (covered by unit test — it must still spawn); the child_workflow redirect is refused synchronously for both verbs (unit tests); cwd: undefined still falls back to process.cwd() so the pre-existing detach test's expect(spawnOptions?.cwd).toBe(process.cwd()) assertion passes untouched.

  • What was not verified: the child_workflow-blocked-parent redirect was not reproduced against the compiled binary (it needs a workflow: sub-run tree and is covered by unit tests at both layers, including two pre-existing tests that pass unchanged). Linux/macOS binaries were not built — the argv fix being relied on (fix(cli): drop Bun SFE virtual argv[1] from detached re-invoke (#2248) #2273) is platform-independent, but only win32-x64 was exercised end-to-end here. bun run validate was not run to completion on this host (see Validation Evidence).

Side Effects / Blast Radius (required)

  • Affected subsystems/workflows: assertApprovable/assertRejectable now sit on the approveWorkflow/rejectWorkflow path used by every surface — CLI, chat command handler, HTTP API, and the manage_run native tool. That is the honest blast radius. It is mitigated by the extraction being behavior-preserving: error strings are verbatim copies, and the pre-existing child_workflow refusal tests for both operations pass unchanged.
  • Potential unintended effects: the --detach precheck is now strictly stricter than before. A caller who previously got {ok:true} for a run with a missing approval context, a child_workflow gate, or an already-resolved gate will now get {ok:false} synchronously. That is the intended fix, but it is a visible behavior change for anyone who was (unknowingly) relying on the false success.
  • Guardrails/monitoring for early detection: every refusal path is unit-tested to assert zero spawns, so a regression that starts spawning on a refused precheck fails CI rather than silently leaking children. The continues field gives automation a machine-readable signal instead of inferred behavior.

Rollback Plan (required)

  • Fast rollback command/path: revert the PR. --detach disappears from approve/reject/resume; workflow run --detach is untouched; the operations revert to their inline gates with identical messages. No data migration, no config, nothing persisted by this change.
  • Feature flags or config toggles (if any): none — --detach is itself the opt-in. Every verb without the flag behaves exactly as before.
  • Observable failure symptoms: a control verb acking {ok:true, detached:true} while workflow get shows the run unchanged minutes later; or a detached-run-<id>.log containing Unknown command: — both indicate the child failed to re-invoke.

Risks and Mitigations

  • Risk: the shared validators are on the hot path of every approve/reject surface, so a mistake in the extraction breaks far more than the CLI.
    • Mitigation: error strings copied verbatim; the two pre-existing child_workflow refusal tests and the full 49-test core operations suite pass unchanged; eight new unit tests pin each gate's message individually.
  • Risk: adding exports to workflow-operations silently un-mocks them in any test that mock.modules that path, because Bun's mock.module() merges rather than replaces — the documented cause of test(core): 'should abandon a running run' intermittently times out on windows-latest (3 hits in one day, fully mocked test) #2240.
    • Mitigation: audited — exactly one factory exists (manage-run-tool.test.ts), it mocks only the four operations, and manage-run-tool.ts never calls the validators directly. Suite verified passing at 30/30.
  • Risk: tightening the precheck turns previously "successful" detach calls into synchronous failures for automation that relied on the false success.
    • Mitigation: this is the bug being fixed, and the failure is now loud, immediate, and carries the same message the child would have produced. Documented in the CLI reference under Detached control verbs.
  • Risk: --detach --json continues the run while bare --json does not — a genuine asymmetry that could surprise a caller.
    • Mitigation: kept deliberately (withholding continuation under --detach would spawn a child that only records a decision, which is what bare --json already does), documented in both the CLI reference and CLAUDE.md, and made machine-readable via continues: true. A unit test asserts the child argv carries neither --detach nor --json.

Summary by CodeRabbit

  • New Features

    • Added --detach support for workflow resume, approve, and reject commands.
    • Commands return immediately while continuing workflow activity in the background.
    • Detached operations validate requests before starting and provide structured JSON acknowledgments, including continuation status.
    • Background execution respects the command’s working directory and reports log locations.
  • Documentation

    • Updated CLI help and reference documentation with detached workflow behavior, validation, acknowledgments, and error handling.

@coderabbitai

coderabbitai Bot commented Jul 21, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

📝 Walkthrough

Walkthrough

Changes

Detached execution now supports workflow resume, approve, and reject. Shared validation helpers perform read-only prechecks before child spawning. The CLI forwards --detach, and tests and documentation cover acknowledgements, arguments, cwd handling, and validation failures.

Detached workflow controls

Layer / File(s) Summary
Workflow validation contracts
packages/core/src/operations/workflow-operations.ts, packages/core/src/operations/workflow-operations.test.ts
Approval and rejection preconditions are exported as shared helpers. Existing workflow operations use these helpers.
Detached control execution
packages/cli/src/commands/workflow.ts
A shared helper performs read-only validation, spawns the control command with the caller’s cwd, removes detached and JSON flags from the child, and returns acknowledgements.
CLI wiring and validation
packages/cli/src/cli.ts, packages/cli/src/commands/workflow.test.ts, packages/docs-web/src/content/docs/reference/cli.md, CLAUDE.md
The CLI forwards --detach to all three commands. Tests and documentation cover child execution, cwd propagation, precheck errors, continuation reporting, and parent-side non-mutation.

Estimated code review effort: 4 (Complex) | ~45 minutes

Sequence Diagram(s)

sequenceDiagram
  participant CLI
  participant WorkflowCommand
  participant DetachedChild
  CLI->>WorkflowCommand: dispatch workflow control command with --detach
  WorkflowCommand->>WorkflowCommand: perform read-only validation
  WorkflowCommand->>DetachedChild: spawn same command without --detach or --json
  WorkflowCommand-->>CLI: return human or JSON acknowledgement
  DetachedChild->>DetachedChild: execute workflow mutation and continuation
Loading

Possibly related issues

Possibly related PRs

  • coleam00/Archon#2112: Adds centralized approval and rejection validation reused by this detached command flow.
  • coleam00/Archon#2146: Adds the assertApprovable and assertRejectable helpers used by detached prechecks.
  • coleam00/Archon#1975: Shares detached workflow control-command handling and child-process behavior.

Suggested reviewers: wirasm

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Docstring Coverage ✅ Passed Docstring coverage is 80.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Title check ✅ Passed The title clearly identifies the extension of --detach to the workflow approve, reject, and resume commands.
Description check ✅ Passed The description completes the required template sections and provides detailed scope, architecture, validation, security, compatibility, risks, and rollback information.
✨ 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

Comment @coderabbitai help to get the list of available commands.

@Wirasm

Wirasm commented Jul 27, 2026

Copy link
Copy Markdown
Collaborator

Good direction — unattended control verbs are squarely on the roadmap (direction.md §always-on-automation), and routing all mutation into the child while the parent stays read-only is the right shape. Five things before it lands.

1. Rebase onto current dev — this one matters more than it looks.

Your branch does not contain 8c7882e8 (merged from #2273 today; I checked with git merge-base --is-ancestor). That commit fixes a bug where --detach on a compiled binary re-invoked the Bun single-file-executable's virtual argv[1] as a command: the child died with Unknown command: /$bunfs/root/archon and the parent acked success. It had been broken since --detach was introduced.

So as this PR stands, approve --detach on any brew or curl install would report success and leave the run paused forever. Your test evidence covers dev mode, where the bug never appeared. The rebase is mechanical — no overlapping hunks — but please re-verify the three verbs against a compiled binary afterwards, not just bun run cli.

2. Make the precheck a shared validator rather than a copy of one branch.

The parent's read-only precheck replicates one of the four preconditions. The other three — child_workflow, missing approval context, already-resolved — currently ack { ok: true } and then die in the child's log. That is precisely the failure this PR exists to prevent, occurring on the surface where nobody is watching.

Please extract something like assertApprovable(run) / assertRejectable(run) in packages/core/src/operations/workflow-operations.ts covering all four, and call it from both approveWorkflow/rejectWorkflow and the CLI precheck. Inlining the checks in the CLI instead isn't good enough — two copies of a four-branch gate will drift, and the drift will be silent in exactly this direction again.

3. --detach currently re-introduces the auto-resume that --json deliberately withholds.

buildDetachedRunCmd strips --json (workflow.ts:247), so the child takes the ordinary inline path and continues the run. But our documented contract is that --json on approve/reject/resume "records/validates the decision and returns a clean JSON line without the inline auto-resume (drive continuation separately)". So --detach --json silently does the opposite of --json.

That may well be the behavior you want — but it has to be stated, and callers need to be able to tell. Please:

  • update packages/docs-web/src/content/docs/reference/cli.md (the --detach row at :216 and the --json control-verb note at :290) and the CLI section of CLAUDE.md;
  • add a field to the ack — continues: true or similar — so an automation knows whether it still owns continuation;
  • add a test asserting the child argv for the --detach --json case.

4. Spawn with the command's cwd, not process.cwd().

runDetachedControlCommand should take the cwd already threaded from cli.ts and pass cwd ?? process.cwd(). As written a caller-supplied --cwd is discarded (last-wins at workflow.ts:249-251), which can strand the child outside a git repo after the parent has already acked success. Your stated rationale for avoiding working_path is unaffected — it's never a candidate here.

5. Please fill in .github/PULL_REQUEST_TEMPLATE.md and either link an issue or say none exists. CodeRabbit's template check is right about this one.


One thing worth knowing rather than fixing: the detach ack is gated only on the child having a PID, so any child that starts and immediately dies reports as a created run. That's pre-existing (#2279) and it's why #2248 hid for a whole release — but this PR widens that surface from run to the control verbs, so the shared validator in (2) is doing more work than it appears to.

buun-dev and others added 4 commits July 31, 2026 18:30
…verbs

--detach today only covers `workflow run`. But approve/reject/resume each
trigger a LONG inline execution when they act on a paused run — approve's
auto-resume, reject's on_reject rework, resume's re-run — hosted in the
calling shell. If that shell dies (a reaped automation task, a closed
terminal, a dropped SSH session), the run is left wedged mid-execution and
needs a manual recovery. That makes the control verbs unsafe to drive from
unattended automation.

Add --detach to all three verbs via a shared helper, runDetachedControlCommand:
it validates the run READ-ONLY (a precheck that refuses a non-paused run and
spawns nothing), then hands the whole command to a detached child — reusing
the existing spawnDetachedWorkflowRun, which rebuilds argv minus
--detach/--json — that owns ALL state mutation in its own process group. The
parent never mutates, so the decision is recorded exactly once, in the child
(mirroring `workflow run --detach`, where the parent likewise does nothing).

The child is spawned with the PARENT's cwd, never the run's working_path: the
child re-resolves everything by run-id, and an isolation-container run's
working_path can be a path the host cannot spawn into (ENOENT would make the
detach silently no-op).

Composes with --json (structured { ok, runId, action, detached, logPath } ack;
nothing executes in the parent). Signatures gain a `detach?` param after the
existing `cwd?`: (runId, [comment/reason], json?, cwd?, detach?).

Tests: approve/reject/resume each spawn a detached child with zero parent-side
mutation; --detach --json ack shape; precheck refuses a non-paused run and
spawns nothing.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ed control-verb gate

The CLI's read-only --detach precheck replicated only ONE of approveWorkflow's
four preconditions. The other three — missing approval context, child_workflow
gate, already-resolved — acked { ok: true } in the parent and then threw unseen
in the detached child's log: the exact failure --detach exists to prevent,
occurring on the surface nobody watches.

Extract the gate into two validators in workflow-operations and call them from
both the operations and the CLI precheck, so there is one gate rather than two
copies that drift silently in this direction again.

The two signatures differ deliberately: reject has no nodeId requirement (it
falls back to approval?.nodeId ?? 'unknown' for its audit event), so a run with
malformed approval metadata stays rejectable. resume needs no change — its
precheck already calls resumeWorkflow directly.

Error strings are copied verbatim; the pre-existing child_workflow refusal tests
pass unchanged, which is the proof the extraction is behavior-preserving.
…t in the ack

buildDetachedRunCmd strips --json from the child argv, so the child takes the
ordinary inline path and drives the run onward. Bare --json documents the
opposite — it records the decision and withholds the auto-resume so the caller
drives continuation. --detach --json therefore did the reverse of --json with
nothing saying so.

Keep the behavior (withholding continuation under --detach would leave a spawn
that only records a decision, which is what bare --json already does) and make
it legible: document the divergence in the CLI reference and CLAUDE.md, and add
continues: true to the ack so an automation can tell whether it still owns
continuation. Test asserts the child argv carries neither flag.
runDetachedControlCommand passed process.cwd() and discarded the --cwd that
cli.ts had already threaded in. buildDetachedRunCmd appends --cwd last and the
parser is last-wins, so a caller-supplied --cwd was overridden both in the spawn
option and on the child argv — stranding the child in the parent's directory,
possibly outside any git repo, after the parent had acked success.

workflowRunCommand already passes its resolved cwd; only the control verbs
regressed. Take cwd as a parameter and pass cwd ?? process.cwd().

The run's working_path remains a non-candidate: a container run's working_path
is a distro path the host cannot spawn into, so the child still re-resolves
everything by run id.
@buun-dev
buun-dev force-pushed the feat/detach-control-verbs branch from 991707c to 6af8eb2 Compare July 31, 2026 12:44
… too

The Detached control verbs section lives under `workflow approve`, so a reader
in the `workflow resume` section still saw the un-inverted contract: "--json
validates and acks but does NOT re-execute inline". Under --detach it does
re-execute. Cross-reference it where that claim is made.
@buun-dev

Copy link
Copy Markdown
Contributor Author

Thanks — this was a good catch on item 2 in particular; it was the real defect and the rest
followed from fixing it properly. Per item:

1. Rebase onto current dev — done.

The branch was 40 commits behind. Rebased clean, no conflicts. Two things came with it that
are worth naming explicitly:

  • 8c7882e8 (fix(cli): drop Bun SFE virtual argv[1] from detached re-invoke (#2248) #2273) — buildDetachedRunCmd now slices user args from argv[2] in both
    dev and binary modes. This PR was silently missing it. In a Bun single-file executable
    argv[1] is a virtual entry path (/$bunfs/root/archon, B:/~BUN/root/archon.exe on
    Windows), so the pre-rebase code fed that to the child as its first token and the child died
    with Unknown command: … — while the parent acked success. On any brew/curl install,
    approve --detach would have reported success and left the run paused forever.
  • A correction to the review's framing of the rebase as pure hygiene: it also turns the
    currently-red windows-latest check green. Those two downloadWebDist failures are not
    this PR's doing — they're the upstream flake fixed by 3044829f (test(cli): build the web-dist tarball fixture in-process instead of spawning tar #2310), whose commit
    message analyzes this PR's own CI run (29801864393).

2. The read-only precheck replicating only one of approveWorkflow's preconditions — fixed,
but not in the shape suggested.

You're right about the defect: the parent checked only status !== 'paused', so the other
three preconditions acked { ok: true } and then threw unseen in the detached child's log —
the exact failure --detach exists to prevent, relocated to the surface nobody watches.

Fixed by extracting the gate into @archon/core and calling it from both the operation and
the CLI precheck, so there is one gate rather than two copies free to drift again.

Correction on the shape: the gates are not symmetric, so this is two validators, not one.

Gate approve reject
status !== 'paused' yes yes
missing approval.nodeId yes no
approval.type === 'child_workflow' yes yes
isGateResolved(approval) yes yes

rejectWorkflow deliberately has no nodeId requirement — it falls back to
approval?.nodeId ?? 'unknown' when writing its audit event, so a run whose approval metadata
is malformed is still legitimately rejectable. Collapsing the two into a single four-branch
gate would either break reject or over-permit approve. Hence assertApprovable (four
preconditions, returns the validated ApprovalContext) and assertRejectable (three, returns
ApprovalContext | undefined).

resume needed no change at all. Its precheck already calls resumeWorkflow directly, so
its single gate (RESUMABLE_WORKFLOW_STATUSES) was never duplicated in the first place. There
was nothing to unify there.

The extraction is behavior-preserving: error strings are copied verbatim, and the two
pre-existing child_workflow refusal tests for the operations pass unchanged — that's the
proof, rather than an assertion. Eight new core unit tests pin each gate's message
individually, and four new CLI tests assert each refusal both throws and spawns nothing.

3. The --detach --json contract — documented, and made machine-readable.

You were right that it was unstated. buildDetachedRunCmd strips --json from the child argv,
so the child takes the ordinary human path — which auto-resumes. But the documented contract
for bare --json on these verbs is that it records the decision without the inline
auto-resume so the caller drives continuation. So --detach --json did the opposite of
--json, with nothing saying so.

I kept the behavior rather than "fixing" it: withholding continuation under --detach would
spawn a child that only records a decision and stops, which is what plain --json already
does, minus the point of detaching. Instead it's now stated in the CLI reference and in
CLAUDE.md, and the ack carries continues: true so an automation can read a field instead of
inferring behavior. A test asserts the child argv carries neither --detach nor --json.

All three locations you named are covered: the --detach row (:216), a new Detached control
verbs
section under workflow approve, and the --json control-verb note at :290. That last
one needed its own sentence rather than just the cross-reference — it sits in the workflow resume section and asserts --json "does not re-execute inline", which --detach inverts;
a reader in that section would never have reached the approve-section prose.

continues is true for all three verbs under --detach, because the child always drives the
verb's full inline behavior. Predicting continuation per-gate in the parent would mean
replicating executor logic there — exactly the drift item 2 exists to remove.

4. Passing the caller's --cwd to the spawned child — fixed; it was a real bug.

runDetachedControlCommand called spawnDetachedWorkflowRun(process.cwd(), …), discarding the
cwd cli.ts had already threaded in. And since buildDetachedRunCmd appends --cwd <value>
last and the parser is last-wins, the caller's --cwd was overridden in both places — the
spawn option and the child's argv. The child could land outside any git repo after the parent
had already acked success. workflowRunCommand had always passed its resolved cwd; only the
control verbs regressed. Now takes cwd as a parameter and passes cwd ?? process.cwd().

This doesn't reopen the working_path question: a container run's working_path is a distro
path the host cannot spawn into, which is why the child re-resolves everything by run id. The
choice was only ever between the caller's --cwd and the parent's process.cwd().

5. PR template + compiled-binary verification — done.

The PR body is rewritten from .github/pull_request_template.md with every section filled.

On the binary: agreed that bun run cli evidence can't cover this, since dev mode is exactly
where the SFE argv bug never appears. Verified against a compiled win32-x64 binary
(Build: binary, commit 6af8eb2a) driven against an isolated ARCHON_HOME and a throwaway
bash-nodes-around-an-approval-gate workflow:

  • approve/reject/resume --detach --json all ack continues: true, and no child log
    contains an Unknown command: /$bunfs/root/… or B:/~BUN/root/… line.
  • More importantly than the ack — a pre-fix binary acks success too — the runs actually
    advanced: approve → completed, reject → cancelled, resume → re-invoked, skipped the
    completed node, re-paused at the gate.
  • The refusal path was verified on a gate this PR newly enforces in the parent: recording an
    approval with plain --json, then re-running approve --detach --json, returns
    {"ok": false, "error": "… was already approved and is awaiting resume."} with that run's
    child-log file byte-identical before and after — proving nothing was spawned. Pre-fix, that
    case acked {ok: true} and died in the log.

One thing I did not fix, flagged rather than folded in: the detach ack is gated only on the
child having a PID, so a child that starts and immediately dies still reports as a created run
(#2279). That's pre-existing on workflow run and I've left it out of scope. But this PR does
widen that surface from run to the control verbs, which is part of why the shared validator
matters more than it looks — it moves the four/three refusal cases from "child dies unseen" to
"parent refuses synchronously", shrinking what #2279 can hide.

Also out of scope, deliberately: plan_lint-style test-double asymmetry findings elsewhere
in the tree (packages/server/src/routes/api*, packages/core/src/db/{workflows,isolation-environments}*).
This branch's diff touches none of those files; fixing upstream's test-double hygiene would
balloon a narrow CLI change across packages/server. Happy to open a separate PR if you want it.


Commits on the branch (post-rebase):

  • refactor(core): extract assertApprovable/assertRejectable as the shared control-verb gate
  • docs(cli): state that --detach --json continues the run, and signal it in the ack
  • fix(cli): spawn the detached control-verb child with the command's cwd
  • docs(cli): note the --detach inversion in the resume --json paragraph too

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🧹 Nitpick comments (2)
packages/cli/src/commands/workflow.ts (1)

2304-2306: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Warn when the child log file could not be opened.

workflowRunCommand prints an explicit warning when logPath is falsy (lines 939-943), because the child then runs with its output discarded. The control-verb path stays silent in that case. The user gets a success message with no trail if the child dies early. Add the same warning for parity.

♻️ Proposed change
     console.log(`Started '${action}' for run ${runId} in the background.`);
     console.log(`Track it with: archon workflow get ${runId}`);
-    if (logPath) console.log(`Child output: ${logPath}`);
+    if (logPath) {
+      console.log(`Child output: ${logPath}`);
+    } else {
+      console.warn('Warning: could not open a log file — child output will not be captured.');
+    }
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@packages/cli/src/commands/workflow.ts` around lines 2304 - 2306, Update the
background control-verb output in workflowRunCommand to emit the same explicit
warning used when logPath is falsy, indicating that child output is being
discarded. Preserve the existing Child output message when logPath is available
and keep the success and tracking messages unchanged.
packages/cli/src/commands/workflow.test.ts (1)

3274-3279: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Assert the approval gate mocks for detached approve/reject.

approveWorkflow uses workflowDb.resolveApprovalGate, and rejectWorkflow uses workflowDb.resolveApprovalGate or workflowDb.resolveAndCancelApprovalGate. Keeping only updateWorkflowRun asserts these two existing mocks under-enforces the “parent does not record the decision” invariant. Add the gate-call assertions to both the approve and reject detach tests.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@packages/cli/src/commands/workflow.test.ts` around lines 3274 - 3279, Update
both detached approve and detached reject tests around the existing
updateWorkflowRun and executeWorkflow assertions to also verify the relevant
approval-gate mocks were not called: resolveApprovalGate for approve, and
resolveApprovalGate or resolveAndCancelApprovalGate for reject. Preserve the
existing zero-call assertions and ensure each test checks that the parent does
not resolve or cancel the approval gate.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@packages/docs-web/src/content/docs/reference/cli.md`:
- Around line 340-350: Update the documented `runDetachedControlCommand`
response example and accompanying description to state that `logPath` may be
null when the detached workflow cannot open its log file; clarify that
automation must not assume it is always a string path.
- Around line 322-324: Update the `approve`, `reject`, and `resume`
documentation sentence to replace “the same four/three preconditions” with a
description that the parent performs the same read-only validation checks as the
operation, preserving the listed wrong-status, missing-context,
child_workflow-blocked, and already-resolved refusal cases.

---

Nitpick comments:
In `@packages/cli/src/commands/workflow.test.ts`:
- Around line 3274-3279: Update both detached approve and detached reject tests
around the existing updateWorkflowRun and executeWorkflow assertions to also
verify the relevant approval-gate mocks were not called: resolveApprovalGate for
approve, and resolveApprovalGate or resolveAndCancelApprovalGate for reject.
Preserve the existing zero-call assertions and ensure each test checks that the
parent does not resolve or cancel the approval gate.

In `@packages/cli/src/commands/workflow.ts`:
- Around line 2304-2306: Update the background control-verb output in
workflowRunCommand to emit the same explicit warning used when logPath is falsy,
indicating that child output is being discarded. Preserve the existing Child
output message when logPath is available and keep the success and tracking
messages unchanged.
🪄 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 Plus

Run ID: e408ba16-7097-4ed5-b9e1-cb0ad6a58aee

📥 Commits

Reviewing files that changed from the base of the PR and between 991707c and 6af8eb2.

📒 Files selected for processing (7)
  • CLAUDE.md
  • packages/cli/src/cli.ts
  • packages/cli/src/commands/workflow.test.ts
  • packages/cli/src/commands/workflow.ts
  • packages/core/src/operations/workflow-operations.test.ts
  • packages/core/src/operations/workflow-operations.ts
  • packages/docs-web/src/content/docs/reference/cli.md
🚧 Files skipped from review as they are similar to previous changes (1)
  • packages/cli/src/cli.ts

Comment on lines +322 to +324
`approve`, `reject`, and `resume` accept `--detach`. The parent validates the run
**read-only** — the same four/three preconditions the operation itself enforces, so a
wrong-status, missing-context, `child_workflow`-blocked, or already-resolved run is

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

Replace "four/three preconditions" with a description of the checks.

The phrase "the same four/three preconditions" exposes an internal count that differs between approve and reject, and the count will drift when the validators change. The sentence already lists the refusal cases, so state the guarantee instead of the number.

📝 Proposed wording
-`approve`, `reject`, and `resume` accept `--detach`. The parent validates the run
-**read-only** — the same four/three preconditions the operation itself enforces, so a
-wrong-status, missing-context, `child_workflow`-blocked, or already-resolved run is
-refused synchronously and nothing is spawned — then hands the whole command to a
+`approve`, `reject`, and `resume` accept `--detach`. The parent validates the run
+**read-only** with the same preconditions the operation itself enforces, so a
+wrong-status, missing-context, `child_workflow`-blocked, or already-resolved run is
+refused synchronously and nothing is spawned. The parent then hands the whole command to a
📝 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
`approve`, `reject`, and `resume` accept `--detach`. The parent validates the run
**read-only** — the same four/three preconditions the operation itself enforces, so a
wrong-status, missing-context, `child_workflow`-blocked, or already-resolved run is
`approve`, `reject`, and `resume` accept `--detach`. The parent validates the run
**read-only** with the same preconditions the operation itself enforces, so a
wrong-status, missing-context, `child_workflow`-blocked, or already-resolved run is
refused synchronously and nothing is spawned. The parent then hands the whole command to a
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@packages/docs-web/src/content/docs/reference/cli.md` around lines 322 - 324,
Update the `approve`, `reject`, and `resume` documentation sentence to replace
“the same four/three preconditions” with a description that the parent performs
the same read-only validation checks as the operation, preserving the listed
wrong-status, missing-context, child_workflow-blocked, and already-resolved
refusal cases.

Comment on lines +340 to +350
```json
{
"ok": true,
"runId": "…",
"action": "approve",
"detached": true,
"continues": true,
"workflowName": "assist",
"logPath": "~/.archon/logs/detached-run-<id>.log"
}
```

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

Document that logPath can be null.

runDetachedControlCommand puts the return value of spawnDetachedWorkflowRun into logPath. The workflow run --detach path treats a falsy value as "could not open a log file", so the field is not always a path. State that logPath is null when the log file could not be opened, so an automation does not assume a string.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@packages/docs-web/src/content/docs/reference/cli.md` around lines 340 - 350,
Update the documented `runDetachedControlCommand` response example and
accompanying description to state that `logPath` may be null when the detached
workflow cannot open its log file; clarify that automation must not assume it is
always a string path.

buun-dev added a commit to buun-dev/Archon that referenced this pull request Jul 31, 2026
…'s own

dev carried its own --detach control-verb implementation (248af22), predating
PR coleam00#2204. The PR branch carries the same feature after maintainer review:
a shared assertApprovable/assertRejectable gate instead of a status-only
precheck, `continues: true` on the --detach --json ack, and the caller's --cwd
threaded to the spawned child.

All 11 conflict hunks were supersessions rather than disagreements — same code,
reviewed version — so each resolved to the PR side:

  runDetachedControlCommand signature   4 args    -> 5 (cwd inserted)
  spawn cwd                             process.cwd() -> cwd ?? process.cwd()
  approve precheck                      status only (1 of 4) -> assertApprovable
  reject precheck                       status only (1 of 3) -> assertRejectable
  ack payload                           no continues -> continues: true
  test fixture                          metadata: {} -> a real approval context
  two test hunks                        empty on our side -> +163 lines

Resolved hunk-by-hunk, NOT `checkout --theirs`: dev's --base threading and
container-ctx-on-resume work had auto-merged into other regions of the same
file, and taking it wholesale would have dropped them silently. Verified after:
the --base references and the PR's fixes coexist.

Verified: type-check 11/11, lint --max-warnings 0, format:check, workflow.test
201 pass (199 from the PR plus 2 of dev's own, so nothing was overwritten),
workflow-operations 49 pass, and bun run test across all 8 test-bearing
packages. The sole failure is @archon/providers' broken-symlink case at EPERM —
Windows refusing symlink creation without elevation, identical on an unmerged
tree.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants