fix(bin): guard worker Git commands from primary checkouts - #3728
3264studios wants to merge 10 commits into
Conversation
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains. Reviews (3): Last reviewed commit: "no-mistakes(ci): Captain, fixed both rep..." | Re-trigger Greptile |
…Orca terminal fakes now execute the Git-guard activation command in the simulated isolated copy instead of merely acknowledging it. Both complete affected test suites pass, and `git diff --check` passes. No production behavior changed
|
Closing in favour of #3848. This branch has fallen 15 commits behind The underlying defect (worker isolation is asserted once at setup and never re-checked, so a drifted worker can run branch-switching tests in the primary checkout) is filed as #3848 with the reproduction and three candidate shapes. A narrow PR of the guard plus its test will follow whichever shape is stamped. |
Intent
A firstmate worker once did its work in the PRIMARY CHECKOUT instead of its own isolated copy, and ran a test there that creates and switches Git branches. That run died partway through on an assertion. Firstmate confirmed afterwards that the shared copy happened to be back on its main branch with the stray branch gone - which was luck. A crash one step earlier would have left the shared checkout, the one every sibling copy resolves against and every landing merges into, sitting on a stray branch. The ship instructions asserted isolation only at the START of a task, so nothing re-checked afterwards.
Asked what the guard should be, the captain ruled, verbatim: "Go with the recommendation." The recommendation he accepted was to ACCEPT the guard as a documented best-effort speed bump, honest about its known evasion routes, and to treat moving the enforcement point to something Git cannot be argued out of as the durable answer IF airtightness is later wanted. He explicitly did NOT choose hardening the wrapper into a Git command-line parser.
His reasoning, which he accepted as recorded: the incident this guard exists to prevent was a worker DRIFTING into the wrong directory - an accident, not evasion - so a documented bump delivers nearly all the value, while a parser arms race buys defence against a threat model nobody has.
He also ruled that the seven evasion routes and the temp-collision gap "must be documented in the guard itself rather than silently fixed or silently ignored." The seven accepted, documented evasion routes are core.worktree via -c/--config-env/GIT_CONFIG_*, unknown global-option arity, non-shell alias injection, an unrecognized linked-primary git dir, GIT_INDEX_FILE and related file-target redirection, absolute Git binaries, and login shells that reset PATH. The /tmp same-task-id collision and its cleanup-lifetime gap must also remain documented. The nested-isolated-worktree false positive is a real bug that must be fixed. Do not harden the wrapper into a Git command-line parser.
A separate standing rule applies to this submission: "Double check and reproduce anything before submitting an upstream PR -- keep a very high standard for principles and correctness before submitting."
What Changed
PATHGit guard before every ship and scout launch, refusing primary-checkout targets while allowing assigned worktrees, nested isolated worktrees, and disposable fixture repositories.Risk Assessment
✅ Low: The guard is well-bounded to ship/scout launches, preserves the explicitly accepted limitations, fixes nested-worktree handling, and retains recovery metadata on both guard-verification failure paths.
Testing
Completed 1 recorded test check.
Pipeline
Updates from git push no-mistakes
✅ **intent** - passed
✅ No issues found.
✅ **Rebase** - passed
✅ No issues found.
🔧 **Review** - 1 issue found → auto-fixed (2) ✅
bin/fm-spawn.sh:3329- A fresh spawn creates its endpoint/worktree and publishes provisional metadata before this timeout. If the backend accepts the PATH command but the shell never executes it, the EXIT rollback removes the metadata without closing most backends' endpoint/worktree; Orca cleanup is already disabled. The next spawn then refuses the duplicate endpoint, leaving manual recovery. Clean up the exact fresh endpoint before rolling back, or retain a durable recovery record.🔧 Fix: Captain, preserve fresh-spawn recovery records
1 warning still open:
bin/fm-spawn.sh:3323-spawn_send_text_lineis a fallible transport boundary, but this call is unguarded underset -e. For example, Zellij/cmux can write the PATH command, fail to submit Enter, clear the partial input, and return nonzero while leaving the endpoint alive. Control then skips the preservation block below, and EXIT cleanup rolls back the published metadata while retaining the endpoint/worktree. A later spawn can refuse the duplicate without a recovery record. Route send failures through the same fresh-spawn metadata-preservation path before exiting.🔧 Fix: Captain, preserve guard transport recovery records
✅ Re-checked - no issues remain.
bin/fm-test-run.sh --changed --exclude-family real-herdr-gated🔧 Fix: Fix spawn fixtures to publish guard activation proof
1 error still open:
bin/fm-test-run.sh --changed --exclude-family real-herdr-gated🔧 Fix: Execute Git guard proof in backlog spawn fixture
1 error still open:
bin/fm-test-run.sh --changed --exclude-family real-herdr-gated✅ **Document** - passed
✅ No issues found.
✅ **Lint** - passed
✅ No issues found.
✅ **Push** - passed
✅ No issues found.
Verification limits
tests/fm-remote-secondmate-lifecycle-e2e.test.shandtests/fm-remote-secondmate-trace-context.test.shwere not run because the pipeline's changed-test selection omitted them. This is adjacent coverage: the change adds a PATH-resolved guard inside the spawn path, which secondmate spawns also traverse.tomllib; the identical tree passed with Python 3.14. This is a host-toolchain artifact, not a defect in this change.PR must be raised via no-mistakeson the identical950b1977head: the successful run completed at 22:04:27Z and the failed run at 22:04:28Z after starting one second apart. The cause was not established.