fix(cli): drop Bun SFE virtual argv[1] from detached re-invoke (#2248) - #2273
Conversation
📝 WalkthroughWalkthrough
ChangesDetached workflow argv
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
…m00#2248) `buildDetachedRunCmd` assumed a compiled binary has no entry-script argv[1] and sliced user args from argv[1] in binary mode. Bun single-file executables DO have an argv[1] — the virtual entry path (`/$bunfs/root/<name>`, or `B:/~BUN/root/<name>.exe` on Windows) — and report argv[0] as `bun`, not execPath. The virtual path therefore leaked in as the child's first token. Since cli.ts parses `process.argv.slice(2)` unconditionally, the detached child read it as the command and exited with `Unknown command: B:/~BUN/root/archon-windows-x64.exe`, creating no run and no worktree while the parent still reported `{ ok: true }`. User args start at argv[2] in both modes; only the command prefix differs. Verified against a real `bun build --compile` artifact: argv = ['bun', '/$bunfs/root/<name>', ...userArgs] The existing binary-mode test modelled a compiled argv with no argv[1] at all, so it certified the broken behaviour. Its fixture is corrected to the real Bun SFE shape and a Windows-shaped regression test is added; both fail against the previous implementation. Reported on Windows x64 and independently reproduced on Ubuntu 24.04 x64, so this affects every compiled binary, not just Windows. Closes coleam00#2248 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
40b0b7d to
851b69a
Compare
CLAUDE.md and AGENTS.md never referenced CONTRIBUTING.md. The links ran one way only: README.md -> CONTRIBUTING.md (humans arriving via the README) CONTRIBUTING.md -> CLAUDE.md (architecture) CLAUDE.md -> (nothing) AGENTS.md -> (nothing) CLAUDE.md and AGENTS.md are the files an agent loads automatically as project instructions; CONTRIBUTING.md never is. And because CLAUDE.md already repeats part of the PR policy (use the template, link the issue with Closes #), the section reads as the complete contributor checklist, so there is no signal to go looking for the rest. The rules that live ONLY in CONTRIBUTING.md are the ones that get missed: commit subjects in present tense and under 72 characters, and the marketplace submission process. This is not hypothetical — PR coleam00#2273 in this repo was pushed with a 78-character subject for exactly this reason. Adds a leading bullet to the Git Workflow section of both files pointing at CONTRIBUTING.md and stating explicitly that the bullets below are branch and release policy, not the full checklist. No rules are duplicated, so the two files cannot drift. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
pr: 2273
|
| Check | Status | Details |
|---|---|---|
bun run validate |
PASS | exit 0 (all nine gates) |
check:bundled |
PASS | up to date (36 commands, 21 workflows) |
check:bundled-skill / check:bundled-schema / check:pi-vendor-map / check:capability-matrix |
PASS | via validate exit 0 |
type-check |
PASS | all 8 packages exited 0 |
lint |
PASS | zero warnings (--max-warnings 0) |
format:check |
PASS | All matched files use Prettier code style! |
@archon/cli tests |
PASS | 6 batches: 425 pass / 0 fail |
| Binary build | PASS | scripts/build-binaries.sh → 74,823,712 bytes, tree restored cleanly by the EXIT trap |
Pattern Compliance
- Follows existing code structure (pure builder retained,
isBinarystill only gates the command prefix) - Type safety maintained (no new
any, no signature change) - Naming conventions followed
- Tests added for the fix, and both new assertions are mutation-verified to fail on the old implementation
- Comment corrected rather than left stale — the previous comment was the actual defect carrier
- No docs change needed (
reference/cli.mddescribes--detachbehaviourally; nothing it says was wrong) - No
CHANGELOG.mdedit needed (/releasegenerates entries)
Strengths
- Root cause is identified precisely and the corrected comment now documents the real Bun SFE argv shape — the old comment was the thing that made the bug plausible to reviewers.
- Scope discipline is exemplary:
baseCmdcorrectly left branching onisBinary, spawn options / log handling /--cwdlast-wins untouched, Windows reliability: verify the Modern Standby + --detach fixes on real hardware (code merged in #2063) #2080 explicitly excluded. - The corrected fixture is the important half of the change — the old fixture actively certified the broken behaviour.
- Both new assertions were confirmed (by me, via mutation) to fail against the pre-fix implementation. Not all "regression tests" survive that check; these do.
Historical note
git log -L on the function shows the argv.slice(1) branch was introduced by #1853, the commit that added --detach itself. So --detach has never worked on a compiled binary — i.e. it has never worked on brew/curl installs, the primary distribution channel. That is worth a "Fixed" line at the next release rather than being folded into a generic CLI bucket.
Recommendation
APPROVE / ship as-is. S1–S3 are optional polish and can land as a follow-up or be ignored. I1 deserves its own issue.
Reviewed by Claude — report: /Users/rasmus/.prp/archon-75601ef6/reviews/pr-2273-review.md
Summary
buildDetachedRunCmdassumed a compiled binary has no entry-scriptargv[1]and sliced user args fromargv[1]in binary mode. Bun single-file executables do have anargv[1]— the virtual entry path (/$bunfs/root/<name>, orB:/~BUN/root/<name>.exeon Windows) — and reportargv[0]asbun, notexecPath. That virtual path leaked in as the detached child's first token.cli.tsparsesprocess.argv.slice(2)unconditionally, so the child read the virtual path as its command and died withUnknown command: B:/~BUN/root/archon-windows-x64.exe— no run record, no worktree, no branch — while the parent still exited 0 with{ ok: true, conversationId, logPath }.--detachis therefore broken on every compiled binary, which is the primary distribution channel (brew / curl install). Reported on Windows x64 and independently reproduced on Ubuntu 24.04 x64, so this is not platform-specific.argv[2]in both modes; only the command prefix still branches onisBinary. The stale comment asserting "there is no entry-script argv[1]" is corrected. The existing binary-mode test fixture — which modelled a compiled argv with noargv[1]at all, and so certified the broken behaviour — is corrected to the real Bun SFE shape, plus a Windows-shaped regression test.baseCmdstill branches onisBinary(correct as-is). No change to spawn options, log-file handling,--cwdlast-wins appending, or the--detach/--jsonfilter. Windows reliability: verify the Modern Standby + --detach fixes on real hardware (code merged in #2063) #2080 (detached child not surviving launcher teardown) is a distinct Windows failure already addressed in v0.6.0 and is untouched here.UX Journey
Before
After
Architecture Diagram
Before
After
Connection inventory:
buildDetachedRunCmdspawnDetachedWorkflowRunspawnDetachedWorkflowRuncli.ts main()argv.slice(2)now yields the real command; previously the virtual pathworkflow.test.tsbuildDetachedRunCmdcli.ts main()Label Snapshot
risk: lowsize: XSclicli:workflowChange Metadata
bugcliLinked Issue
Validation Evidence (required)
Evidence provided:
1. Empirical confirmation of Bun SFE argv shape. Built a throwaway artifact with
bun build --compile(Bun 1.3.13, macOS arm64) and printed its argv:argv[0]isbun(not execPath) andargv[1]is the virtual entry path — directly contradicting the comment the old code relied on.2. Fix proven against that real argv:
3. New tests fail against the previous implementation (reverted the one-line change and re-ran):
The received array reproduces the reported symptom token-for-token. With the fix applied:
3 pass, 0 fail; full@archon/clisuite0 failacross all 6 batches.If any command is intentionally skipped: none skipped.
Security Impact (required)
Net effect is removing an unintended token from a spawned child's argv. The child is the same trusted
execPaththe parent is already running.Compatibility / Migration
Dev-mode (
bun+ entry script) behaviour is byte-for-byte identical: that branch already sliced fromargv[2]. Only the compiled-binary branch changes, and only from broken to working.Human Verification (required)
bun build --compileartifact rather than assumed from the report; fix output diffed against that same real argv; both new tests confirmed red against the old implementation and green against the new one; fullbun run validateexit 0.B:/~BUN/…argv covered by a dedicated test; grepped the whole repo for otherprocess.argvconsumers — the only other one iscli.ts:239, which already usesslice(2)and is the contract this now matches.--detachrun before release.Side Effects / Blast Radius (required)
archon workflow run --detachon compiled binaries. Foreground runs never call this builder.ARCHON_HOME/logs/detached-run-<conversationId>.log; a regression reappears there immediately asUnknown command: …. The two new unit tests fail loudly on any reintroduction.Rollback Plan (required)
git revert <merge-sha>— the change is one expression plus tests in a single file pair, no migrations or state.archon workflow run --detachreturns{ ok: true }butarchon workflow get <conversationId>reportsWorkflow run not found, and the detached log containsUnknown command: <virtual path>.Risks and Mitigations
argv[1]), makingslice(2)wrong in the other direction.slice(2)is now anchored to the same contractcli.ts:239uses, so the two move together — a Bun change would break the CLI's own parser first and far more loudly than the detach path. The tests document the observed shape and the Bun version it was verified against.🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
--detach/--jsonare consistently omitted, while--cwdand any additional flags are preserved.Tests