Skip to content

fix: record real leased worktree in fm-spawn meta; harden fm-teardown#264

Open
prodempsey wants to merge 1 commit into
kunchenguid:mainfrom
prodempsey:fm/fmspawn-lease-pr
Open

fix: record real leased worktree in fm-spawn meta; harden fm-teardown#264
prodempsey wants to merge 1 commit into
kunchenguid:mainfrom
prodempsey:fm/fmspawn-lease-pr

Conversation

@prodempsey

Copy link
Copy Markdown

fix: record real leased worktree in fm-spawn meta; harden fm-teardown

Summary

bin/fm-spawn.sh recorded the wrong worktree path in every crew's meta, which broke teardown, merge/PR detection, and branch resolution and forced hand-teardown of every crew.
This change replaces the fragile pane-cwd polling heuristic with an authoritative treehouse get --lease acquire, whose stdout is exactly the leased worktree path, and hardens bin/fm-teardown.sh so a missing, stale, or non-treehouse worktree in the meta degrades gracefully instead of bricking teardown.

Root cause

The old spawn path sent treehouse get into the task pane and then polled tmux's #{pane_current_path}, taking the first value that differed from the project path as the worktree.
That is a fork/exec race: between sending treehouse get and the treehouse subshell actually entering the pooled worktree, the pane can transiently report the session's default working directory - the firstmate home where fm-spawn itself runs.
The poll latched onto that spurious transient cwd, so worktree=<firstmate home> was written into the meta instead of the real leased worktree.
Downstream, fm-teardown then ran treehouse return on a non-pool path (refused), and merge/PR detection and branch resolution operated on the wrong directory, so every crew had to be torn down by hand.

The fix

fm-spawn now leases the worktree out-of-band with treehouse get --lease --lease-holder "fm-<id>", run from the project so treehouse resolves the correct pool.
--lease prints only the leased worktree's absolute path to stdout (all banners go to stderr), so the captured value is authoritative rather than scraped.
If no worktree is leased, spawn exits 1 immediately - no 60s poll, no timeout, no chance of a spurious path.
The pane is then moved into the leased worktree with an explicit cd <worktree> send.
The leased worktree is protected from treehouse prune for the task's lifetime, and teardown's existing treehouse return --force "$WT" releases the lease exactly as it did for an interactively-acquired worktree.
The secondmate and Orca spawn paths are unchanged.

Why --lease beats a hardened poll

A hardened poll (longer timeout, cwd sanity checks) would still be inferring the worktree from an observable side effect that races with the pane.
--lease removes the inference entirely: treehouse returns the path it reserved, so there is exactly one source of truth and no window in which a transient cwd can be mistaken for the worktree.
It also makes the reservation durable against treehouse prune, which the poll never did.
This design - authoritative acquire plus fail-fast exit 1 - is Captain-approved.

Teardown hardening

fm-teardown adds safe_task_worktree(), which permits branch cleanup and treehouse return only when the recorded worktree is a real, registered worktree of the task's project AND is neither the firstmate repo root nor the active firstmate home.
The second guard matters for firstmate-on-itself tasks whose meta was written before this fix: such a meta could record the runtime home (itself a genuine, registered worktree of the firstmate repo) as the worktree, and teardown must never detach, branch-delete, or hand the runtime home to treehouse return.
Teardown now also degrades gracefully: a missing, stale, or non-treehouse worktree warns on stderr and continues - the window is still killed and volatile state cleared - instead of aborting when treehouse return refuses an unmanaged path.
A failed treehouse return likewise warns rather than aborting, so a stuck pool slot never blocks the rest of teardown.

Tests

  • New tests/fm-spawn-worktree-lease.test.sh asserts the spawn meta records the leased path, that the worktree is leased with treehouse get --lease rather than sent into the pane, and that the pane is driven with cd <worktree>.
  • tests/fm-teardown.test.sh gains graceful-degradation and non-fatal-return cases (non-treehouse worktree skipped, failed return warns but continues).
  • The rewritten test_spawn_conformance_old_vs_new in tests/fm-backend.test.sh drops the byte-identical old-vs-new tmux-log comparison (no longer meaningful after the mechanism change) and instead asserts the new --lease mechanism, meta recording, and pane drive.
  • The existing spawn mocks in tests/fm-grok-harness.test.sh, tests/fm-secondmate-harness.test.sh, tests/fm-spawn-dispatch-profile.test.sh, and tests/fm-tangle-guard.test.sh are updated to echo a fake leased path for the --lease flow.

Test evidence

Verified in the crew worktree on 2026-07-04, rebased onto origin/main at dc37d58.

  • shellcheck bin/*.sh bin/backends/*.sh tests/*.sh (the exact CI command): clean, exit 0.
  • bash -n on every touched script: clean.
  • The fix's own tests pass: fm-spawn-worktree-lease.test.sh, fm-teardown.test.sh, and the rewritten fm-backend.test.sh conformance case.
  • Full behavior suite compared against a clean origin/main (dc37d58) baseline run in the same environment.
    The branch is strictly better than baseline: fm-backend.test.sh goes FAIL -> PASS, because its test_spawn_conformance_old_vs_new was already failing on origin/main (an upstream GOTMPDIR change had broken the byte-identical old-vs-new assertion) and this change rewrites that test to the new mechanism.
  • The 9 remaining suite failures are pre-existing on origin/main, identical on both baseline and this branch, and all live in files this change does not touch: fm-backend-cmux, fm-backend-orca, fm-backend-zellij (backend-binary/home-hash env dependencies), fm-gotmp, fm-fleet-sync, fm-session-start, fm-turnend-guard, fm-secondmate-safety, and fm-secondmate-lifecycle-e2e (environment/tooling/seed dependencies).
    They are unrelated to this change.

Note for the pusher

The commit preserves its original Co-Authored-By: Claude Opus 4.8 (1M context) trailer from the archived fm/fmspawn-worktree-fix commit a5f477c.
The firstmate repo house style forbids an agent co-author trailer, so consider git commit --amend to strip it before pushing.

fm-spawn recorded worktree=<firstmate home> (the launch cwd) in every crew's
meta instead of the crew's real treehouse worktree. It sent `treehouse get`
into the pane and polled `pane_current_path` until it differed from PROJ_ABS,
taking that first differing path as the worktree - but the pane can briefly
sit in the session's default working directory (the firstmate home) before the
treehouse subshell is entered, so WT latched onto that spurious path. The bad
worktree broke fm-teardown (`treehouse return` on a non-pool path), merge/PR
detection, and branch resolution, forcing hand-teardown of every crew.

fm-spawn now leases the worktree authoritatively with `treehouse get --lease`,
whose stdout is exactly the leased path, then sends `cd <worktree>` into the
pane. Secondmate and Orca paths are unchanged. Teardown's existing
`treehouse return --force` releases the lease as before.

fm-teardown is also hardened: a missing, stale, or non-treehouse worktree in
the meta (e.g. a pre-fix meta) now warns and continues - killing the window and
clearing volatile state - instead of aborting when `treehouse return` refuses
an unmanaged path. A safe-worktree guard prevents branch cleanup / return from
ever touching the runtime home for a firstmate-on-itself meta.

Tests: new fm-spawn-worktree-lease asserts the meta records the leased path;
fm-teardown gains graceful-degradation and non-fatal-return cases; the existing
spawn mocks (fm-backend, fm-tangle-guard, fm-grok-harness,
fm-secondmate-harness, fm-spawn-dispatch-profile) are updated for the --lease
flow, and the fm-backend spawn conformance check now asserts the new mechanism.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 999b82f802

ℹ️ 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".

Comment thread bin/fm-teardown.sh
# the worktree-path fix, which recorded the launch cwd instead of the leased
# worktree) must not brick teardown - warn and fall through so the window is
# still killed and volatile state cleared.
if [ -n "$WT" ] && [ -d "$WT" ] && safe_task_worktree "$PROJ" "$WT"; then

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 Badge Skip invalid recorded worktrees before safety checks

In the normal non-Orca teardown path, this new guard is only reached after the dirty/unpushed checks have already run against $WT. When a pre-fix meta recorded the launch cwd or firstmate home and that directory is a git checkout with dirty or unpushed work, fm-teardown.sh exits with REFUSED before it reaches this graceful skip, so the window and meta remain stuck even though the recorded path is not safe to return. Apply the same safe_task_worktree decision before the landed-work checks, or otherwise bypass those checks for invalid recorded worktrees.

Useful? React with 👍 / 👎.

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.

1 participant