fix(bin): refuse teardown of a worktree reassigned to another task - #2
Merged
Merged
Conversation
fm-teardown.sh terminated processes in, and returned, the worktree path recorded in a task's metadata without checking the worktree still belonged to that task. After a partial failure - the pool return succeeded but a later step (a Herdr pane close) failed, so the task record stayed on disk - treehouse could hand the freed slot to the next task before teardown was rerun. The rerun then killed the new task's agent and deleted its branch with no warning or refusal. Ownership proof: every non-secondmate spawn now writes a gitignored .fm-task-owner marker into the task worktree naming the task and the state directory holding its records. Teardown reads it before killing processes in, resetting, or returning the worktree, and refuses loudly - naming the other task - when it belongs to someone else. The marker is rewritten by every spawn, so it names whichever task took the worktree last; it speaks only for a task that still has a record where it says that task's records live, so a leftover never wedges an unrelated teardown. A worktree with no live marker falls back to another live record in this home claiming the same path, then to the fm/<task-id> branch. The refusal forces, stashes, and discards nothing, and rerunning after the records are reconciled completes. Rerun convergence: a successful pool return or Orca worktree removal is recorded as worktree_returned=1 in the task record, so a rerun after a later failure skips every worktree step instead of repeating it, reaping only the task's own temp root. A return that succeeded but could not be recorded fails loudly rather than leaving the two records disagreeing silently. Both script headers carry the new contracts. Regression tests cover an ordinary teardown still reaping and returning its own worktree, a rerun refusing loudly on a worktree a second task has since acquired or marked, a rerun completing cleanly once the records are reconciled, and spawn marking the settled worktree out of git's view.
…turned-worktree relaunch refusal
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Intent
Fix: bin/fm-teardown.sh can kill another task's agent when it is re-run after a partial failure, because it terminates processes in, and returns, the worktree path recorded in the task's meta without verifying the worktree still belongs to that task.
Observed 2026-09-05 on the Herdr backend, but the defect is backend-independent:
fm-teardown.sh Areturned A's treehouse worktree to the pool ("Worktree returned to pool."), then failed the Herdr pane close ("herdr presentation cleanup target is the captain's active tab; refusing a close that cannot preserve focus") and exited non-zero with "retaining every durable task record", so state/A.meta (with worktree=) stayed on disk.fm-spawn.sh Bran; treehouse handed task B the same now-free slot-1 worktree.fm-teardown.sh Awas re-run. It terminated every lingering process in the recorded slot-1 path ("Terminated lingering processes: bash, claude, ..."), which were now B's agent, and returned the worktree again. B died with no status line and its branch was deleted by the pool return, with no warning or refusal.Required behavior:
Firstmate repo rules that apply to this change (it touches firstmate's own shared tracked material in bin/): follow .agents/skills/firstmate-coding-guidelines/SKILL.md - one sentence per line in Markdown, plain dashes never em dashes, bin/*.sh must pass bin/fm-lint.sh (pinned shellcheck 0.11.0), colocate regression tests in tests/ named .test.sh following tests/lib.sh conventions and extend an existing test file where one already covers the subject, tests must exercise behavior through the executable and never assert on implementation source bytes, never add an agent co-author to commits, read the header comment of every script touched first because each header is the single owner of that script's contract and must be updated when the contract changes, keep the change minimal and focused on this defect and do not refactor surrounding code, and do not touch data/, state/, config/, or projects/ of any firstmate home.
Implementation decisions made while doing the work, which a reviewer reading only the diff would not know:
Test evidence and known environment limitation:
What Changed
bin/fm-spawn.shnow writes a gitignored.fm-task-ownermarker (task=<id>,state=<state dir>) into every non-secondmate task worktree on every spawn, andbin/fm-teardown.shchecks that marker (falling back to another live task record claiming the same path, then the worktree'sfm/<id>branch) before killing processes in, resetting, or returning the recorded worktree; when the evidence binds the worktree to a different task, teardown refuses loudly, names that task, and forces, stashes, or discards nothing, including under--force.worktree_returned=1instate/<id>.metaso a rerun after a later failure (pane close, presentation cleanup) skips every worktree step and reaps only the task's own tasktmp; a failed Orca removal now aborts with the record unmarked, andbin/fm-spawn.sh --relaunchandbin/fm-control.sh's safe checkpoint refuse to relaunch a task whose worktree was already returned.tests/fm-teardown.test.sh,tests/fm-spawn-worktree-settle.test.sh, andtests/fm-control-relaunch.test.shcover an ordinary teardown still reaping and returning its own worktree, a rerun after a real post-return failure refusing to touch a successor task's worktree, a rerun after reconciliation completing cleanly, spawn marking the settled worktree, and the relaunch refusal; script headers anddocs/agent-control.md/docs/architecture.mddocument the ownership check and rerun-convergence contract.Risk Assessment
✅ Low: The re-review confirms each fix-round change against the code: the marker-signal test can now only pass through the marker path, the Orca removal failure fails closed with the record retained as the header states, and relaunch refuses a returned-marked record in both fm-spawn and fm-control before touching the agent, leaving no reachable path to the original cross-task teardown that I could substantiate.
Testing
Ran the three touched suites plus the endpoint-safety and Orca suites; all new and existing cases pass except five real-lsof reap cases in tests/fm-teardown.test.sh that fail identically on the base commit because lsof is not installed on this host. Captured end-user CLI transcripts of bin/fm-teardown.sh for the incident scenario on both target and base: on target the rerun refuses loudly, names the task that now owns the worktree from both the cross-record and marker signals, and returns the slot once, while on base the rerun reaps the successor's process and returns the slot twice. Also captured the persisted meta showing worktree_returned=1 after a partial failure and a reconciled rerun completing without a second return.
Evidence: Evidence index (what each file shows)
Source: Evidence index (what each file shows)
Evidence: Target: rerun refusal transcript, cross-record signal (bin/fm-teardown.sh stderr)
Source: Target: rerun refusal transcript, cross-record signal (bin/fm-teardown.sh stderr)
REFUSED: worktree /tmp/fm-teardown-tests.Rx7PUW/reassigned-worktree-refusal/wt is no longer task task-x1's: task task-x2's own record claims the same worktree. Killing its processes or returning it would destroy another task's work. Reconcile the task records first (clear the stale worktree binding on task-x1), then rerun teardown; nothing was killed, returned, or discarded.Evidence: Target: rerun refusal transcript, .fm-task-owner marker signal (bin/fm-teardown.sh stderr)
Source: Target: rerun refusal transcript, .fm-task-owner marker signal (bin/fm-teardown.sh stderr)
REFUSED: worktree /tmp/fm-teardown-tests.Rx7PUW/reassigned-worktree-refusal/wt is no longer task task-x1's: it is marked as task task-x2's in /tmp/fm-teardown-tests.Rx7PUW/reassigned-worktree-refusal/state. Killing its processes or returning it would destroy another task's work. Reconcile the task records first (clear the stale worktree binding on task-x1), then rerun teardown; nothing was killed, returned, or discarded.Evidence: Target: pool return log across the partial run and both refused reruns (exactly one return)
Source: Target: pool return log across the partial run and both refused reruns (exactly one return)
return --force /tmp/fm-teardown-tests.Rx7PUW/reassigned-worktree-refusal/wtEvidence: Base c03bfbe: same scenario reproduces the defect (rerun reaps the successor's process, slot returned twice)
Source: Base c03bfbe: same scenario reproduces the defect (rerun reaps the successor's process, slot returned twice)
second.stderr: teardown: reaping leaked worktree process(es) for task-x1: 423829 treehouse.log: return --force /tmp/fm-teardown-tests.E8MSjo/reassigned-worktree-refusal/wt return --force /tmp/fm-teardown-tests.E8MSjo/reassigned-worktree-refusal/wtEvidence: Persisted state: meta after a real partial failure and the converged rerun
Source: Persisted state: meta after a real partial failure and the converged rerun
## state/task-x1.meta after the partial failure (record retained and converged) window=firstmate:fm-task-x1 endpoint_task_id=task-x1 worktree=/tmp/fm-teardown-tests.O3KPz8/converged-meta-demo/wt project=/tmp/fm-teardown-tests.O3KPz8/converged-meta-demo/project kind=ship mode=no-mistakes worktree_returned=1 ## rerun without reconciling: exit and stderr exit=1 teardown: worktree /tmp/fm-teardown-tests.O3KPz8/converged-meta-demo/wt was already returned for task-x1 by an earlier run; skipping every worktree step ## treehouse calls after the unreconciled rerun (must still be one) return --force /tmp/fm-teardown-tests.O3KPz8/converged-meta-demo/wtEvidence: Target: reconciled rerun completes and skips the worktree steps
Source: Target: reconciled rerun completes and skips the worktree steps
stderr: teardown: worktree .../reconciled-rerun/wt was already returned for task-x1 by an earlier run; skipping every worktree step stdout: teardown task-x1 complete (window firstmate:fm-task-x1, worktree .../reconciled-rerun/wt) treehouse.log: one return onlyEvidence: Target: ordinary teardown still reaps its own process and returns its own worktree
Source: Target: ordinary teardown still reaps its own process and returns its own worktree
teardown: reaping leaked worktree process(es) for task-x1: 414987 teardown task-x1 complete (window firstmate:fm-task-x1, worktree .../own-worktree-return/wt) treehouse.log: return --force .../own-worktree-return/wtEvidence: New regression cases: pass on target, fail on base scripts
Source: New regression cases: pass on target, fail on base scripts
target: ok - an ordinary teardown still reaps and returns the worktree that is its own ok - a rerun after a post-return failure refuses loudly instead of tearing down a reassigned worktree ok - a rerun after the records are reconciled completes without repeating the worktree steps base c03bfbe: ok - an ordinary teardown still reaps and returns the worktree that is its own not ok - reassigned-worktree-refusal: the rerun killed the successor task's process not ok - reconciled-rerun: the rerun returned the already-returned worktree againEvidence: Full tests/fm-teardown.test.sh run on target
Source: Full tests/fm-teardown.test.sh run on target
Evidence: Reap cases after the lsof failure: target vs base (identical, lsof missing on host)
Source: Reap cases after the lsof failure: target vs base (identical, lsof missing on host)
Evidence: tests/fm-control-relaunch.test.sh run (relaunch refuses a returned worktree)
Source: tests/fm-control-relaunch.test.sh run (relaunch refuses a returned worktree)
Evidence: tests/fm-spawn-worktree-settle.test.sh run (spawn writes the owner marker)
Source: tests/fm-spawn-worktree-settle.test.sh run (spawn writes the owner marker)
Evidence: tests/fm-backend-orca.test.sh run (Orca removal failure keeps the record)
Source: tests/fm-backend-orca.test.sh run (Orca removal failure keeps the record)
Evidence: tests/fm-teardown-endpoint-safety.test.sh run
Source: tests/fm-teardown-endpoint-safety.test.sh run
Pipeline
Updates from git push no-mistakes
✅ **intent** - passed
✅ No issues found.
✅ **Rebase** - passed
✅ No issues found.
🔧 **Review** - 2 issues found → auto-fixed (3) ✅
bin/fm-spawn.sh:2721- The relaunch path can still bind task A to a worktree another task now holds, and the new marker then makes A's teardown treat that worktree as its own. Sequence: fm-teardown.sh A returns slot-1 and records worktree_returned=1, then fails at the pane close; treehouse leases slot-1 to B; A's endpoint now reads 'dead' (the return killed its agent), so bin/fm-control.sh relaunch A passes the agent-free check and fm-spawn.sh --relaunch adopts the recorded worktree unconditionally (RELAUNCH_WT at bin/fm-spawn.sh:1047-1051 only checks the directory exists), overwrites B's .fm-task-owner with task=A at bin/fm-spawn.sh:2369, and preserve_relaunch_meta drops worktree_returned. A's next teardown now sees a marker naming itself, passes assert_worktree_owned_by_task, reaps B's agent and returns B's worktree - the original defect via a sibling path. The intent's note that 'a relaunch drops it when it rebinds a worktree' is not what the code does: relaunch never rebinds, it reuses the recorded path. Recommended boundary: in the --relaunch adoption block (next to the missing-worktree refusal at bin/fm-spawn.sh:1048), refuse when the prior meta carries worktree_returned=1, stating the worktree was already returned and teardown must be rerun; keep dropping the key only for the non-relaunch spawn that genuinely acquires a new worktree.bin/fm-teardown.sh:2861- In the Orca branch record_worktree_returned runs unconditionally after fm_backend_remove_worktree, whose exit status is discarded. Iforca worktree rmfails (tool missing, id not found, transient error), the meta still gains worktree_returned=1, so a rerun after a later failure skips every worktree step and the final rm of the meta leaves an Orca worktree that was never removed, with the record having claimed otherwise. The treehouse branch gates the record on a successful return; the Orca branch should do the same, e.g.if fm_backend_remove_worktree "$BACKEND" "$ORCA_WORKTREE_ID"; then record_worktree_returned || {...}; fi(or fail loudly on removal failure, matching the header's 'never leave the two records disagreeing silently' contract).🔧 Fix: refuse relaunch into returned worktree; gate Orca return record
1 error still open:
bin/fm-teardown.sh:2868- The fix round turned an Orca removal failure from a fail-closed abort into a warn-and-continue path that then deletes the task record. Under the script's set -eu the base's barefm_backend_remove_worktreecall exited teardown on failure and kept state/<id>.meta; now the else branch only prints a warning and execution falls through to the unconditionalrm -f ... "$STATE/$ID.meta"near line 3002. Trace: backend=orca,orca worktree rmfails (tool missing, id not found, transient error) -> warning says 'a rerun retries the removal' -> teardown exits 0 and removes the meta -> no record remains to rerun from and the Orca worktree is orphaned. This regresses base behavior and makes the new header sentence at line 50 ('leaves the record unmarked, so a rerun retries the removal') false. Fix: fail loudly in the else branch, e.g.echo "error: Orca worktree $WT (${ORCA_WORKTREE_ID:-no id}) could not be removed for $ID; retaining the task record so a rerun retries the removal" >&2; exit 1, matching the treehouse return-failure path at line ~2895 and the header contract.🔧 Fix: fail closed when Orca worktree removal fails
2 issues (1 warning, 1 info) still open:
tests/fm-teardown.test.sh:2764- The marker-signal sub-case cannot prove the marker check works. Before the third run the worktree is still on branch fm/task-x2 (checked out earlier in the test) and task-x2.meta still exists, so assert_worktree_owned_by_task's branch fallback ('it is on task task-x2's branch') refuses with 'task-x2' in stderr even if the marker path were broken (e.g. marker_owner_still_recorded always returning 1, or worktree_owner_marker_value returning nothing). The assertions only check exit != 0, 'task-x2' in stderr, and the return count, all of which the branch fallback also satisfies. Fix: before the third run detach the worktree (git -C "$case_dir/wt" checkout -q --detach) or switch it to a non-fm/ branch so only the marker remains, and assert the marker-specific evidence text ('is marked as task task-x2') rather than just the task id.bin/fm-spawn.sh:1055- Relaunch refuses only on the meta's worktree_returned=1 and never reads the worktree's own .fm-task-owner marker. If teardown's pool return succeeds but record_worktree_returned fails (teardown exits 1 with the record unmarked and the marker already removed), a later task can lease the slot and write its marker, and a subsequent relaunch of the retired task would still adopt the path and overwrite that marker plus clear the other task's harness wiring. The path is narrow (a filesystem error in the state dir, followed by an operator relaunching instead of reconciling as the error instructs, and the relaunch's shell-in-worktree check surviving the return), so this is noted as a residual gap rather than a blocker; reading the marker in the --relaunch adoption block and refusing when it names another task would close it cheaply.🔧 Fix: isolate marker-signal refusal test from branch fallback
✅ Re-checked - no issues remain.
✅ **Test** - passed
✅ No issues found.
bash tests/fm-teardown.test.sh(full suite on target; all cases pass up to and including the three new ownership cases, then stops at the pre-existing lsof-dependent leaked-process-reap failure)bash tests/fm-control-relaunch.test.sh(pass, including the two new returned-worktree refusal cases)bash tests/fm-spawn-worktree-settle.test.sh(pass, including the new task-owner marker case)bash tests/fm-teardown-endpoint-safety.test.sh(pass)bash tests/fm-backend-orca.test.sh(pass, includingfm-teardown.sh backend=orca: preserves metadata on remove ok:false JSON, which covers the fail-closed Orca removal path)run-selected-teardown-tests.sh tests test_ordinary_teardown_reaps_and_returns_its_own_worktree test_rerun_after_partial_failure_refuses_a_reassigned_worktree test_rerun_after_reconciled_records_completes_cleanlyon target: all three pass, transcripts capturedSame three test functions run against the base commit c03bfbe scripts (extracted withgit archiveto a temp dir): reassigned-worktree case fails withthe rerun killed the successor task's process, reconciled-rerun case fails withthe rerun returned the already-returned worktree again, ordinary case passesThe ten reap cases after the lsof failure (test_leaked_worktree_process_is_reapedthroughtest_run_abort_precedes_process_reap_precedes_worktree_removal) run one at a time on target and on base: identical results, five pre-existing failures caused by missing lsof on this host, five passManual checkshow-converged-meta.sh: one real partial-failure teardown, then printed state/task-x1.meta showingworktree_returned=1, then an unreconciled rerun that skips the worktree steps with the pool return count still onegit status --porcelain --ignoredin the worktree: clean, no transient artifacts left✅ **Document** - passed
✅ No issues found.
✅ **Lint** - passed
✅ No issues found.
✅ **Push** - passed
✅ No issues found.