feat(bin): add opt-in pre-acquire worktree pool safety sweep - #2827
feat(bin): add opt-in pre-acquire worktree pool safety sweep#2827ICGNU3 wants to merge 13 commits into
Conversation
Implements a pre-acquire sweep that inspects pooled worktrees before acquisition and refuses when unsafe pool state is observed. - Added bin/fm-treehouse-pool-sweep.sh with dirty detection and HEAD reachability checks against durable refs (refs/heads/*, refs/tags/*, refs/firstmate/rescue/*). - Wired sweep into bin/fm-spawn.sh after treehouse get acquisition. - Added config documentation to docs/configuration.md. - Added comprehensive test suite in tests/fm-treehouse-pool-sweep.test.sh covering: dirty worktree, staged changes, untracked files, branch/tag reachable HEAD, detached HEAD in main, reflog-only reachability, historical reproduction, remote-only refs. The sweep is disabled by default and activated via config/worktree-pool-sweep. This is a MITIGATION (not a fix) for the worktree reuse incident; it cannot close the structural gaps of direct treehouse get bypasses or cross-home race conditions. The upstream Treehouse invariant must solve those.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c2f3078b8c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| unique_count=$(git -C "$wt" rev-list --count HEAD --not --branches --tags \ | ||
| --glob='refs/firstmate/rescue/*' 2>/dev/null) || return 2 |
There was a problem hiding this comment.
Preserve commits on the checked-out branch
Captain, when an acquired pool worktree is still checked out on a local task branch with unique committed work, --branches subtracts that branch and the sweep returns safe; freshen_spawn_worktree_base then runs reset --hard origin/<default> while still on that branch, moving its only durable ref and leaving those commits reachable only through the reflog. Exclude the currently checked-out branch from this reachability proof or detach without deleting its ref before resetting, so this mitigation cannot discard unlanded commits.
AGENTS.md reference: AGENTS.md:L30-L32
Useful? React with 👍 / 👎.
Confidence Score: 4/5The PR does not yet appear safe to merge because an explicitly enabled sweep can still be silently skipped when its helper is missing or non-executable, allowing unsafe worktree state to reach the hard reset. The executable-file guard in Files Needing Attention: bin/fm-spawn.sh Reviews (4): Last reviewed commit: "no-mistakes: apply CI fixes" | Re-trigger Greptile |
…table-HEAD diagnostic
…verdicts in sweep
|
Speaking as Kun's firstmate: VISION (current main Class: opt-in. Security: none. No workflow files. Ahead 11 / behind 0. MERGEABLE / UNSTABLE. HOLD: edits Matching attestation for HEAD Waiting on green CI and hold-pair resolution, not a captain product call. |
|
Speaking as Kun's firstmate: VISION (read current main 8714c9a in full). Inspected Class: opt-in. Security: none as a CI-workflow or credential issue (no Overlap / HOLD: edits CI / NM: HEAD Workflows: already approved before this pass (CI running). Run IDs: 32622077088 (CI), 32622090057 (Require no-mistakes latest SUCCESS), 32622077051 (older NM FAILURE). Land-eligible: NO (spawn/pool overlap with the standing pair/freshen/#2804/#2760 holds, even though CI+NM are green on this HEAD). Captain-flag NOW: no. |
|
Closing as superseded, on the captain's ruling. The guard this PR implements cannot do what it was specified to do, and its own exact-head review proved it. Two structural findings: the durable-reachability gate can never refuse an attached HEAD, because Those are one finding: a consumer cannot implement this invariant. There is no inspection point before allocation, and after allocation the damage is already done. Superseded by:
The branch is preserved. Nothing here is lost, and the reasoning is recorded in the incident record rather than only in this thread. |
What Changed
bin/fm-treehouse-pool-sweep.sh, a pre-acquire safety sweep that refuses a pooled worktree that is dirty (tracked modifications, staged changes, or untracked non-ignored files), whose HEAD carries commits unreachable from durable refs (refs/heads/*,refs/tags/*,refs/firstmate/rescue/*), or whose HEAD is covered only by remote-tracking refs; distinct exit codes cover each case plus missing worktree and usage error, and unanswerable git probes fail closed under the matching exit code with their own diagnostic.bin/fm-spawn.shaftertreehouse getvalidation, passing the spawn's resolved config dir viaFM_CONFIG_OVERRIDE; a non-zero sweep exit aborts the spawn with an error naming the worktree, the exit code, andconfig/worktree-pool-sweep. The sweep ships deactivated — it stays inert unless that config file exists with a non-empty value other thanoff.treehouse getbypass, cross-home race between sweep and acquire) indocs/configuration.md, and adds shell test suites covering the sweep's behavior and itsfm-spawn.shwiring.Risk Assessment
✅ Low: The mitigation ships inert by default (proven non-trivially by both the sweep and spawn-wiring suites), the new fail-closed/diagnostic plumbing traces correctly on every exit path I constructed, and the only remaining issue is unreachable dead code with no behavioral effect.
Testing
Ran the two targeted suites for this change (
tests/fm-treehouse-pool-sweep.test.sh,tests/fm-spawn-pool-sweep-wiring.test.sh) — both pass — then drove the real fm-spawn acquisition path manually against a synthetic pool to capture the operator-visible CLI transcript: unconfigured andoffhomes spawn unchanged, an enabled sweep refuses the spawn with the diagnostic plus the "pool sweep refused worktree … (exit 2); … disable sweep in config/worktree-pool-sweep" error while leaving the abandoned pool work intact, and each documented exit code (1/2/3/4/64) emits its own distinct message. This is a CLI-only change, so the evidence is a command transcript rather than visual artifacts; no failures or flakes were seen and the worktree is clean.Evidence: Operator CLI transcript: sweep deactivated vs. enabled vs. off, plus every refusal diagnostic
Source: Operator CLI transcript: sweep deactivated vs. enabled vs. off, plus every refusal diagnostic
=== 1. Shipped state: no config/worktree-pool-sweep file === $ fm spawn demo-default <project> spawned demo-default harness=codex kind=ship mode=no-mistakes yolo=off window=firstmate:fm-demo-default worktree=/tmp/.../a/pool [exit 0] <- acquisition path unchanged; the sweep never ran === 2. Operator activates the mitigation === $ echo on > $FM_HOME/config/worktree-pool-sweep $ git -C <pool> log --oneline -1 # the work at risk 5baa05f abandoned work $ fm spawn demo-enabled <project> unsafe: HEAD contains commits not reachable from durable refs in /tmp/.../b/pool error: worktree pool sweep refused worktree /tmp/.../b/pool (exit 2); inspect unsafe state or disable sweep in config/worktree-pool-sweep [exit 1] <- spawn aborted before reusing the worktree $ cat <pool>/abandoned.txt # work still intact after the refusal abandoned lane work === 3. Explicit off === $ fm spawn demo-off <project> spawned demo-off ... worktree=/tmp/.../c/pool [exit 0] <- 'off' leaves the sweep inert --- direct invocation, one diagnostic per condition --- <dirty pool> unsafe: dirty worktree at ... [exit 1] <unreferenced HEAD> unsafe: HEAD contains commits not reachable from durable refs [exit 2] <uninspectable HEAD> unsafe: cannot inspect HEAD at ... [exit 1] <corrupt ref database> unsafe: cannot compute HEAD reachability in ... [exit 2] <remote-only coverage> unsafe: HEAD commits covered only by remote-tracking refs [exit 3] <nonexistent path> (no output) [exit 4] <no argument> error: worktree path required + usage [exit 64]Evidence: `bin/fm-treehouse-pool-sweep.sh --help` output (rendered from the file header)
Source:
bin/fm-treehouse-pool-sweep.sh --helpoutput (rendered from the file header)Pipeline
Updates from git push no-mistakes
⏭️ **intent** - skipped
✅ No issues found.
✅ **Rebase** - passed
✅ No issues found.
bin/fm-treehouse-pool-sweep.sh:170- The durable-reachability gate can never refuse an attached HEAD, so the mitigation misses the loss it is meant to prevent.git rev-list --count HEAD --not --branches --tags --glob='refs/firstmate/rescue/*'puts HEAD's own branch into the--notset whenever HEAD is attached, sounique_countis structurally always 0 andcheck_head_reachablereturns 0. Verified live:git init -b main; commit base; checkout -b lane; commit lanework; git rev-list --count HEAD --not --branches --tagsprints 0 even thoughlaneworkexists on no other ref. Concrete failing sequence: a pool worktree is returned without a clean teardown (teardown's normalgit checkout --detach+git branch -Dnever ran), so it is clean but still attached tofm/<task>carrying unpushed commits. is_dirty says clean; check_head_reachable says safe (count 0); fm-spawn.sh:2274 then callsfreshen_spawn_worktree_base, whosegit reset --hard origin/<default>(bin/fm-spawn.sh:1762) MOVES the attached branch ref, leaving those commits reachable only from the reflog - the exact "commit reachable only from a reflog is unreferenced" state the script header names as unsafe. The invariant the sweep claims ("refuse when unsafe pool state is observed") therefore holds only for detached HEADs; every attached-HEAD pool worktree is waved through by construction, and test_allows_branch_reachable_head passes trivially rather than exercising the comparison. The earliest shared boundary is the reachability computation itself: evaluate HEAD against the refs that will still exist after freshen's reset - e.g. exclude HEAD's own symbolic-ref target from the--notset, or ask directly whether HEAD's commits surviveorigin/<default>- so an attached lane branch with unique commits is actually judged instead of vacuously cleared. Flagging as ask-user because narrowing the--notset changes which worktrees the mitigation refuses, which is your scoping call.bin/fm-spawn.sh:2263- A sweep refusal leaks the pooled worktree it just refused, so each refusal permanently drains a pool slot. The sweep is invoked only afterspawn_send_text_line "$WT_TARGET" 'treehouse get'(line 2215) has already succeeded and the pane has settled in the worktree - the acquisition is complete, contradicting the "pre-acquire ... refuses to request one" framing in the script header (line 5) and --help (line 26). On refusal fm-spawn prints the error andexit 1;spawn_abort_cleanup(line 692) releases locks and cleans up only the orca path - it never runstreehouse return, and the tmux window created for the task is never killed.state/$ID.metais not published until line 2702, well after this point, so no state record exists for fm-teardown.sh to act on either (teardown_treehouse_return at bin/fm-teardown.sh:1061 is driven from that metadata). Concrete sequence: operator enables the sweep; a pool worktree fails the reachability check; spawn aborts; the worktree stays checked out and marked busy by treehouse with no owner and no path back. Retrying the spawn acquires a different slot and, since the refusal condition is a persistent property of the leaked worktree, repeats - so the pool shrinks by one per refusal until exhausted. The pre-existingvalidate_spawn_worktree "treehouse get"failure at line 2261 leaks the same way, but it fires only on a pathological pane/worktree mismatch, whereas this refusal is designed to fire on ordinary unsafe pool state. Either return the worktree before aborting (treehouse return --force "$WT", tolerating failure the way teardown does) or state explicitly that a refusal quarantines the slot for manual inspection and say so in the operator message and docs/configuration.md. Marking ask-user: quarantine-vs-return is a deliberate product decision about what should happen to an unsafe worktree.bin/fm-treehouse-pool-sweep.sh:24-usage()hand-duplicates roughly 60 lines that already exist as the file's header comment (lines 2-21), including the exit-code table verbatim twice (lines 15-21 and 49-55) and the MITIGATION/structural-gap prose twice (lines 4-12 and 28-30, 71-80). 28 scripts under bin/ instead derive usage from the header withsed -n '2,${/^#/!q;p;}' "$0" | sed 's/^# \{0,1\}//'(bin/fm-spawn.sh:204-206), which keeps the two in sync by construction. Adopting that form here removes the heredoc entirely and eliminates the drift risk between the header and --help - the same drift that produced the earlier$HOME/.firstmatevs$FM_HOMEandoff-value help inaccuracies on this branch.bin/fm-treehouse-pool-sweep.sh:98-FM_HOME="${FM_HOME:-${FM_ROOT_OVERRIDE:-$FM_ROOT}}"has a dead middle default: line 97 already setsFM_ROOT="${FM_ROOT_OVERRIDE:-$(cd "$SCRIPT_DIR/.." && pwd)}", so wheneverFM_ROOT_OVERRIDEis non-empty$FM_ROOTequals it, and when it is empty the:-falls through to$FM_ROOTanyway. The expression is exactly equivalent toFM_HOME="${FM_HOME:-$FM_ROOT}". (bin/fm-spawn.sh:212-213 carries the same redundancy, so matching it is defensible - but the simpler form is identical in behavior.)bin/fm-treehouse-pool-sweep.sh:123-is_dirtyconflates "git could not answer" with "dirty".git diff-index --quiet --ignore-submodules HEAD 2>/dev/nullexits 128 on an unborn or corrupt HEAD, and the leading!turns that failure intoreturn 0, so the sweep exits 1 and prints "unsafe: dirty worktree at <path>" for a worktree that has no uncommitted changes at all. Failing closed is right; the label is not - an operator triaging the refusal will look for uncommitted work that does not exist, when the actual condition is an unusable HEAD.check_head_reachablealready distinguishes an unanswerable question from a negative answer (lines 154, 171); doing the same here - separate the diff-index non-zero-vs-128 cases, or emit a distinct "cannot inspect HEAD" diagnostic - would make the message match reality without changing the refusal.🔧 Fix: derive sweep help from header; distinct uninspectable-HEAD diagnostic
3 infos still open:
bin/fm-treehouse-pool-sweep.sh:184- Exit 2 still conflates "git could not answer the reachability question" with "HEAD has unreachable commits" - the same mislabel class you already chose to fix for is_dirty (round 1, review-23). check_head_reachable returns 2 both when unique_count > 0 (line 164) and when rev-list fails or prints a non-numeric value (lines 160, 162), and line 184 prints "unsafe: HEAD contains commits not reachable from durable refs in <path>" for both. Concrete path, and the branch's own fixture proves it is reachable: tests/fm-treehouse-pool-sweep.test.sh:505 writes a dangling ref into .git/refs/heads/dangling, rev-list exits 128, and the operator is told to go looking for orphaned commits when the actual condition is a corrupt ref database that no amount of inspecting HEAD will explain. (The narrower is_dirty case has the same shape: a HEAD that resolves but whose tree object is unreadable makes diff-index exit 128, and line 176 calls that "dirty worktree".) Failing closed is right; the label is not. Emit a distinct diagnostic - e.g. capture rev-list's status separately and print "unsafe: cannot compute HEAD reachability in <path>" - while keeping the refusal and the exit code, so the existing test assertion at line 525 still holds.bin/fm-spawn.sh:2263- Theif [ -x "$SCRIPT_DIR/fm-treehouse-pool-sweep.sh" ]guard makes an explicitly enabled safety gate fail open with no diagnostic. If the file is absent or loses its executable bit (a copied/rsynced install, a checkout with core.fileMode off, a partial update), fm-spawn skips the sweep entirely and spawns normally - the operator who wroteoninto config/worktree-pool-sweep has no signal that the mitigation they turned on never ran. This is the only[ -x "$SCRIPT_DIR/..." ]guard in bin/; every other sibling helper (fm-ff-lib.sh, fm-wake-lib.sh, fm-guard, ...) is sourced or invoked unconditionally, so a missing file surfaces as an error rather than a silent no-op. Neither new suite covers the skip branch. Invoking it unconditionally (the|| sweep_rc=$?capture already turns a missing script into 127 and refuses) would make it fail closed and match repo convention; flagging ask-user because fail-open-on-missing-script vs fail-closed is your call about what an incomplete install should do.docs/configuration.md:143- Commit dfd7ac7 ("tighten worktree pool sweep config docs", a pipeline-authored document step) deleted the sentence "The sweep is currently disabled by default. Two live lanes hold pooled worktrees and must not be disturbed until they are free." That lane note is exactly review-18 from round 4, which you reviewed and did NOT select - the recorded decision was to keep it. The durable half survives (line 132 still says "shipped deactivated"), but the operational note about the two live lanes is gone from the branch without a decision to remove it. Either restore the sentence or confirm the removal is what you want; no source behavior is affected either way.🔧 Fix: distinguish unanswerable git probes from unsafe verdicts in sweep
1 info still open:
bin/fm-treehouse-pool-sweep.sh:126- The--cachedprobe inis_dirty(lines 126-132) is unreachable, and the test added to justify it does not exercise it. It runs only when the preceding plaingit diff-index --quiet --ignore-submodules HEADreturned 0, but that probe already reports any index-vs-HEAD delta regardless of the working tree. Verified live in a scratch repo: commitoriginal,printf staged > f && git add f, restoreprintf original > f,git update-index --refresh->git diff-index --quiet HEADexits 1 (and so does--cached). Also verified withgit update-index --assume-unchanged fstaged the same way: stillplain=1. So there is no state where the first probe says clean and the second says dirty. Consequence for the fix round's own claim:test_refuses_staged_change_restored_in_worktree(tests/fm-treehouse-pool-sweep.test.sh:171) was added specifically as "the case only the--cachedbranch catches", but it reaches exit 1 through the first probe - deleting lines 126-132 leaves it green, so it proves nothing about that branch. This is the same subsumption already reported as review-5 in an earlier round and selected for fix; it was never removed. Drop lines 126-132 and retitle the test to what it actually asserts (a staged delta whose worktree matches HEAD is still refused), or keep the branch only if a state that reaches it can be exhibited.✅ **Test** - passed
✅ No issues found.
bash tests/fm-treehouse-pool-sweep.test.sh— 23 sweep verdict/config-resolution cases, all passbash tests/fm-spawn-pool-sweep-wiring.test.sh— shipped-deactivated, enabled-refusal, and explicit-off spawn-path cases, all passManual end-to-end: drove the realbin/fm-spawn.shacquisition path against a synthetic project/origin/pool fixture (fake tmux + treehouse,FM_GATE_REFUSE_BYPASS=1) with the pool parked on a committed-but-unreferenced commit, under three config states (absent /on/off), capturing spawn stdout, exit code, pool HEAD and the survivingabandoned.txtManual:FM_HOME=<fixture> bin/fm-treehouse-pool-sweep.sh <path>against dirty, unborn-HEAD, orphan-HEAD, dangling-ref, remote-only, nonexistent, and no-argument fixtures to capture each diagnostic and exit codebin/fm-treehouse-pool-sweep.sh --help✅ **Document** - passed
✅ No issues found.
⏭️ **Lint** - skipped
✅ **Push** - passed
✅ No issues found.