diff --git a/bin/fm-spawn.sh b/bin/fm-spawn.sh index bd760060..c6359262 100755 --- a/bin/fm-spawn.sh +++ b/bin/fm-spawn.sh @@ -786,23 +786,40 @@ spawn_send_key() { # esac } if [ "$KIND" != secondmate ] && [ "$BACKEND" != orca ]; then - spawn_send_text_line "$T" 'treehouse get' - - # Wait for the treehouse subshell: the pane's cwd moves from the project to the worktree. - for _ in $(seq 1 60); do - p=$(spawn_current_path "$T" || true) - if [ -n "$p" ] && [ "$p" != "$PROJ_ABS" ]; then - WT="$p" - break - fi - sleep 1 - done - if [ -z "$WT" ]; then - echo "error: treehouse get did not enter a worktree within 60s; inspect window $T" >&2 + # Authoritatively LEASE the worktree instead of scraping the pane's cwd. + # + # The prior approach sent `treehouse get` into the pane and then polled + # `pane_current_path` until it differed from PROJ_ABS, taking that first + # differing path as the worktree. That heuristic is unreliable: the pane can + # momentarily report a directory that is neither PROJ_ABS nor the worktree - + # most commonly the session's default working directory, which is the + # firstmate home where fm-spawn runs - before `treehouse get` has entered its + # subshell. WT then latched onto that spurious path (observed: every crew's + # meta recorded worktree= instead of the real leased + # worktree), which broke fm-teardown (`treehouse return` on a non-pool path), + # merge detection, and branch resolution. + # + # `treehouse get --lease` reserves a pool worktree durably and prints ONLY + # its absolute path to stdout (all banners go to stderr), so WT is exactly + # the leased worktree path. treehouse resolves the pool from the working + # directory, so run it from the project, mirroring fm-teardown's + # `treehouse return`. A leased worktree is protected from `treehouse prune` + # for the task's lifetime; teardown's `treehouse return --force "$WT"` + # releases it, exactly as it did for an interactively-acquired worktree. + set +e + WT=$(cd "$PROJ_ABS" && treehouse get --lease --lease-holder "fm-$ID" 2>/dev/null) + TH_STATUS=$? + set -e + if [ "$TH_STATUS" -ne 0 ] || [ -z "$WT" ]; then + echo "error: treehouse get --lease did not yield a worktree for $PROJ_ABS; inspect window $T" >&2 exit 1 fi - validate_spawn_worktree "treehouse get" "$T" + validate_spawn_worktree "treehouse get --lease" "$T" + + # Move the task pane into the leased worktree so the agent runs there; the + # GOTMPDIR export and launch command below are sent into this same shell. + spawn_send_text_line "$T" "cd $(shell_quote "$WT")" fi # Per-task temp root: /tmp/fm-/ with Go's build temp nested at gotmp/. Go won't diff --git a/bin/fm-teardown.sh b/bin/fm-teardown.sh index b52c4e75..8b49fb01 100755 --- a/bin/fm-teardown.sh +++ b/bin/fm-teardown.sh @@ -362,6 +362,25 @@ canonical_existing_dir() { ( cd "$target" && pwd -P ) } +# A recorded worktree is safe to act on (branch cleanup + `treehouse return`) +# only when it is a real, registered worktree of the task's project AND is not +# the firstmate repo root or the active firstmate home. The second guard matters +# for firstmate-on-itself tasks whose meta was written before the worktree-path +# fix: such a meta can record the runtime home (a genuine worktree of the +# firstmate repo, so registered) as the "worktree", and teardown must never +# detach/branch-delete or hand that runtime home to `treehouse return`. +safe_task_worktree() { # + local project=$1 wt=$2 wt_abs root_abs home_abs + worktree_registered_for_project "$project" "$wt" || return 1 + wt_abs=$(canonical_existing_dir "$wt") || return 1 + root_abs=$(cd "$FM_ROOT" && pwd -P) || return 1 + [ "$wt_abs" != "$root_abs" ] || return 1 + if home_abs=$(cd "$FM_HOME" 2>/dev/null && pwd -P); then + [ "$wt_abs" != "$home_abs" ] || return 1 + fi + return 0 +} + require_orca_worktree_path_match() { local worktree_id=$1 inspected=$2 resolved inspected_abs resolved_abs resolved=$(fm_backend_worktree_path orca "$worktree_id") || { @@ -782,19 +801,30 @@ if [ "$BACKEND" = orca ] && [ "$KIND" != secondmate ]; then fi [ -z "$T_ORCA" ] || fm_backend_kill "$BACKEND" "$T" "$(meta_value "$META" zellij_tab_id)" "fm-$ID" 2>/dev/null || true fm_backend_remove_worktree "$BACKEND" "$ORCA_WORKTREE_ID" -elif [ -d "$WT" ] && [ "$KIND" != secondmate ]; then - branch=$(git -C "$WT" rev-parse --abbrev-ref HEAD 2>/dev/null || echo HEAD) - if [ "$branch" != "HEAD" ]; then - if git -C "$WT" checkout --detach -q 2>/dev/null; then - git -C "$WT" branch -D "$branch" >/dev/null 2>&1 || true +elif [ "$KIND" != secondmate ]; then + # Return the leased worktree to the pool. Degrade gracefully rather than + # aborting: a missing, stale, or non-treehouse WT (e.g. a meta written before + # 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 + branch=$(git -C "$WT" rev-parse --abbrev-ref HEAD 2>/dev/null || echo HEAD) + if [ "$branch" != "HEAD" ]; then + if git -C "$WT" checkout --detach -q 2>/dev/null; then + git -C "$WT" branch -D "$branch" >/dev/null 2>&1 || true + fi fi + # Remove our hook file so a reused pool worktree cannot fire signals for a dead task. + rm -f "$WT/.claude/settings.local.json" "$WT/.opencode/plugins/fm-turn-end.js" "$WT/.fm-grok-turnend" + # Kills remaining processes in the worktree (including the agent), resets, returns + # to pool. treehouse resolves the pool from the working directory, so run it from + # the project. Non-fatal: a failed return warns rather than aborting teardown. + if ! ( cd "$PROJ" && treehouse return --force "$WT" ); then + echo "warning: treehouse return failed for worktree '$WT'; the pool slot may still be leased - inspect with 'treehouse status'. Continuing teardown." >&2 + fi + else + echo "warning: recorded worktree '${WT:-}' for $ID is missing, not a directory, or not a treehouse-managed worktree of $PROJ; skipping worktree return and continuing teardown (window will be killed and volatile state cleared)." >&2 fi - # Remove our hook file so a reused pool worktree cannot fire signals for a dead task. - rm -f "$WT/.claude/settings.local.json" "$WT/.opencode/plugins/fm-turn-end.js" "$WT/.fm-grok-turnend" - # Kills remaining processes in the worktree (including the agent), resets, returns - # to pool. treehouse resolves the pool from the working directory, so run it from - # the project. - ( cd "$PROJ" && treehouse return --force "$WT" ) fi if [ "$BACKEND" != orca ]; then diff --git a/tests/fm-backend.test.sh b/tests/fm-backend.test.sh index 8254cfb8..0fb2d717 100755 --- a/tests/fm-backend.test.sh +++ b/tests/fm-backend.test.sh @@ -709,7 +709,17 @@ esac exit 0 SH chmod +x "$fb/tmux" - fm_fake_exit0 "$fb" treehouse + # fm-spawn leases the worktree out-of-band with `treehouse get --lease` and + # reads the path from stdout; echo the fake worktree so WT resolves to it. + cat > "$fb/treehouse" <> "\${FM_TMUX_LOG:?}" +if [ "\${1:-}" = get ]; then + printf '%s\\n' "$wt" +fi +exit 0 +SH + chmod +x "$fb/treehouse" printf '%s\n' "$fb" } @@ -725,43 +735,49 @@ run_spawn_case() { # } test_spawn_conformance_old_vs_new() { - local old_bin fb proj wt data id log_old log_new out_old out_new - local state_old state_new config_old config_new - old_bin=$(build_old_bin spawn-old) + # NOTE: fm-spawn's worktree resolution intentionally changed after the P1 + # backend extraction: it no longer sends `treehouse get` into the pane and + # polls pane_current_path (which could latch onto a spurious cwd like the + # firstmate home). It now leases the worktree out-of-band with + # `treehouse get --lease` - whose stdout is the authoritative leased path - + # and sends `cd ` into the pane. So this is no longer a byte- + # identical old-vs-new comparison for spawn; it asserts the new mechanism + # records the leased worktree and drives the pane correctly. + local fb proj wt data id log_new out_new state_new config_new proj="$TMP_ROOT/spawn-project"; wt="$TMP_ROOT/spawn-wt"; data="$TMP_ROOT/spawn-data" id="spawnconform1" fm_git_worktree "$proj" "$wt" "fm/$id" fb=$(make_spawn_fakebin "$TMP_ROOT/spawn-fake" "$wt") mkdir -p "$data/$id" printf 'test brief content\n' > "$data/$id/brief.md" - state_old="$TMP_ROOT/spawn-state-old"; state_new="$TMP_ROOT/spawn-state-new" - config_old="$TMP_ROOT/spawn-config-old"; config_new="$TMP_ROOT/spawn-config-new" - mkdir -p "$state_old" "$state_new" "$config_old" "$config_new" - log_old="$TMP_ROOT/spawn-old.log"; log_new="$TMP_ROOT/spawn-new.log" + state_new="$TMP_ROOT/spawn-state-new"; config_new="$TMP_ROOT/spawn-config-new" + mkdir -p "$state_new" "$config_new" + log_new="$TMP_ROOT/spawn-new.log" - out_old=$(run_spawn_case "$old_bin" "$fb" "$log_old" "$state_old" "$data" "$config_old" "$proj" -- "$id" "$proj" claude 2>&1) - local rc_old=$? out_new=$(run_spawn_case "$ROOT" "$fb" "$log_new" "$state_new" "$data" "$config_new" "$proj" -- "$id" "$proj" claude 2>&1) local rc_new=$? - expect_code 0 "$rc_old" "old fm-spawn.sh should succeed"$'\n'"$out_old" expect_code 0 "$rc_new" "new fm-spawn.sh should succeed"$'\n'"$out_new" - [ "$out_old" = "$out_new" ] || fail "fm-spawn.sh stdout differs old vs new"$'\n'"--- old ---"$'\n'"$out_old"$'\n'"--- new ---"$'\n'"$out_new" assert_contains "$out_new" "spawned $id harness=claude kind=ship mode=no-mistakes yolo=off window=firstmate:fm-$id worktree=$wt" \ - "spawn output missing the expected summary line" + "spawn output missing the expected summary line (should record the leased worktree)" - diff -u "$log_old" "$log_new" > "$TMP_ROOT/spawn-diff.txt" 2>&1 \ - || fail "fm-spawn.sh: tmux command log differs old vs new"$'\n'"$(cat "$TMP_ROOT/spawn-diff.txt")" + # The leased worktree is recorded in the task meta verbatim. + assert_grep "worktree=$wt" "$state_new/$id.meta" "spawn meta did not record the leased worktree path" + # The worktree was leased out-of-band (get --lease), not sent into the pane. + assert_contains "$(cat "$log_new")" "treehouse"$'\x1f''get'$'\x1f''--lease' \ + "spawn did not lease the worktree with treehouse get --lease" + # The pane is moved into the leased worktree with a cd send. + assert_contains "$(cat "$log_new")" $'\x1f'"cd '$wt'" \ + "spawn did not send 'cd ' into the pane" # Sanity: the log actually captured the session/window lifecycle so an # accidentally-empty log (e.g. a fake tmux path typo) cannot pass silently. assert_contains "$(cat "$log_new")" $'\x1f''new-window' "spawn tmux log missing new-window" - assert_contains "$(cat "$log_new")" $'\x1f''treehouse get' "spawn tmux log missing the treehouse get send" assert_contains "$(cat "$log_new")" $'\x1f''-l'$'\x1f'"CLAUDE_CODE_ENABLE_PROMPT_SUGGESTION=false claude --dangerously-skip-permissions \"\$(cat '$data/$id/brief.md')\"" \ "spawn tmux log missing the literal launch-command send" rm -rf "/tmp/fm-$id" - pass "fm-spawn.sh: tmux command log and printed summary line are byte-identical old vs new for a ship-task claude spawn" + pass "fm-spawn.sh: leases the worktree with treehouse get --lease, records it in meta, and drives the pane (cd + launch)" } # --- old vs new: fm-teardown.sh ---------------------------------------------- diff --git a/tests/fm-grok-harness.test.sh b/tests/fm-grok-harness.test.sh index 655efe2f..83c3f97c 100755 --- a/tests/fm-grok-harness.test.sh +++ b/tests/fm-grok-harness.test.sh @@ -25,7 +25,18 @@ esac exit 0 SH chmod +x "$fakebin/tmux" - fm_fake_exit0 "$fakebin" treehouse gh-axi gh + # fm-spawn leases the worktree via `treehouse get --lease`, whose stdout is the + # leased path. Echo the worktree run_grok_spawn advertises via FM_FAKE_PANE_PATH. + cat > "$fakebin/treehouse" <<'SH' +#!/usr/bin/env bash +if [ "${1:-}" = get ]; then + printf '%s\n' "${FM_FAKE_PANE_PATH:-}" + exit 0 +fi +exit 0 +SH + chmod +x "$fakebin/treehouse" + fm_fake_exit0 "$fakebin" gh-axi gh printf '%s\n' "$fakebin" } diff --git a/tests/fm-secondmate-harness.test.sh b/tests/fm-secondmate-harness.test.sh index 1a314c05..b16d3998 100755 --- a/tests/fm-secondmate-harness.test.sh +++ b/tests/fm-secondmate-harness.test.sh @@ -441,6 +441,18 @@ esac exit 0 SH chmod +x "$fakebin/tmux" + # A crew/scout spawn leases its worktree via `treehouse get --lease` (stdout is + # the leased path); echo the FM_FAKE_PANE_PATH the caller advertises. Harmless + # for secondmate spawns, which skip the worktree-lease step entirely. + cat > "$fakebin/treehouse" <<'SH' +#!/usr/bin/env bash +if [ "${1:-}" = get ]; then + printf '%s\n' "${FM_FAKE_PANE_PATH:-}" + exit 0 +fi +exit 0 +SH + chmod +x "$fakebin/treehouse" printf '%s\n' "$fakebin" } diff --git a/tests/fm-spawn-dispatch-profile.test.sh b/tests/fm-spawn-dispatch-profile.test.sh index 5b0bad7a..289e10df 100755 --- a/tests/fm-spawn-dispatch-profile.test.sh +++ b/tests/fm-spawn-dispatch-profile.test.sh @@ -42,7 +42,18 @@ esac exit 0 SH chmod +x "$fakebin/tmux" - fm_fake_exit0 "$fakebin" treehouse + # fm-spawn leases the worktree with `treehouse get --lease`, whose stdout is + # the leased path. Echo the same worktree run_spawn advertises via + # FM_FAKE_PANE_PATH so WT resolves to the real isolated git worktree. + cat > "$fakebin/treehouse" <<'SH' +#!/usr/bin/env bash +if [ "${1:-}" = get ]; then + printf '%s\n' "${FM_FAKE_PANE_PATH:-}" + exit 0 +fi +exit 0 +SH + chmod +x "$fakebin/treehouse" printf '%s\n' "$fakebin" } diff --git a/tests/fm-spawn-worktree-lease.test.sh b/tests/fm-spawn-worktree-lease.test.sh new file mode 100755 index 00000000..9e865b9e --- /dev/null +++ b/tests/fm-spawn-worktree-lease.test.sh @@ -0,0 +1,121 @@ +#!/usr/bin/env bash +# Behavior test for fm-spawn.sh's worktree resolution (the worktree-path fix). +# +# Regression guard for the bug where every crew's state/.meta recorded +# worktree= instead of the crew's real treehouse +# worktree. The old code sent `treehouse get` into the pane and then polled +# `pane_current_path` until it differed from PROJ_ABS, taking that first +# differing directory as the worktree - which could latch onto the session's +# default working directory (the firstmate home) before the treehouse subshell +# was entered. fm-spawn now leases the worktree authoritatively with +# `treehouse get --lease`, whose stdout is exactly the leased path, so the meta +# must record that path verbatim. +# +# The suite fakes tmux and treehouse (like every other spawn suite except the +# real-tmux smoke test): tmux is a no-op terminal, and `treehouse get --lease` +# echoes a pre-created real git worktree so fm-spawn's isolated-worktree +# validation still passes. +set -u + +# shellcheck source=tests/lib.sh +. "$(dirname "${BASH_SOURCE[0]}")/lib.sh" +fm_git_identity fmtest fmtest@example.invalid + +SPAWN="$ROOT/bin/fm-spawn.sh" +TMP_ROOT=$(fm_test_tmproot fm-spawn-worktree-lease) + +# Build a spawn sandbox. Sets up a firstmate home with a project git repo, a +# brief, and a fakebin with no-op tmux + a treehouse mock whose `get --lease` +# prints a pre-created worktree of the project. Echoes the case dir. +make_spawn_case() { + local name=$1 id=$2 case_dir home project leased fakebin + case_dir="$TMP_ROOT/$name" + home="$case_dir/home" + project="$home/projects/repo" + leased="$case_dir/leased-worktree" + fakebin="$case_dir/fakebin" + mkdir -p "$home/state" "$home/config" "$home/data/$id" "$fakebin" + + # A real project repo so treehouse can add a worktree of it, and the leased + # worktree fm-spawn will validate as a genuine isolated worktree. + fm_git_init_commit "$project" + git -C "$project" worktree add -q -b "fm-lease-$id" "$leased" + + # Registry line so fm-project-mode.sh resolves a delivery mode cleanly. + printf '%s\n' "- repo [no-mistakes] - test project (added 2026-01-01)" \ + > "$home/data/projects.md" + + # Minimal brief; fm-spawn only needs it to exist and be catted into the pane. + printf '%s\n' "Test brief for $id." > "$home/data/$id/brief.md" + + # No-op tmux: list-windows prints nothing (so the duplicate-window guard + # passes), has-session succeeds (so container-ensure reuses "firstmate"), + # everything else exits 0. pane_current_path is never polled anymore. + cat > "$fakebin/tmux" <<'SH' +#!/usr/bin/env bash +case "${1:-}" in + list-windows) exit 0 ;; +esac +exit 0 +SH + + # treehouse mock: `get --lease ...` prints the pre-created worktree path + # (banners would go to stderr in the real tool); every other subcommand is a + # no-op. FM_TEST_LEASE_PATH carries the path the test expects recorded. + cat > "$fakebin/treehouse" < "$case_dir/stdout" 2> "$case_dir/stderr" + rc=$? + set -e + + # Per-task temp root fm-spawn creates unconditionally; clean it up. + rm -rf "/tmp/fm-$id" 2>/dev/null || true + + expect_code 0 "$rc" "spawn should succeed with a leased worktree ($(cat "$case_dir/stderr"))" + assert_present "$home/state/$id.meta" "spawn did not write the task meta" + meta_wt=$(grep '^worktree=' "$home/state/$id.meta" | cut -d= -f2-) + [ "$meta_wt" = "$leased" ] \ + || fail "meta worktree is '$meta_wt', expected the leased treehouse path '$leased'" + # The launch cwd / firstmate home must never be recorded as the worktree. + [ "$meta_wt" != "$home" ] \ + || fail "meta worktree is the firstmate home (the original bug)" + pass "spawn records the leased treehouse worktree path in the task meta" +} + +test_meta_records_leased_worktree_path diff --git a/tests/fm-tangle-guard.test.sh b/tests/fm-tangle-guard.test.sh index 92a697d8..bb910fff 100755 --- a/tests/fm-tangle-guard.test.sh +++ b/tests/fm-tangle-guard.test.sh @@ -169,7 +169,18 @@ esac exit 0 SH chmod +x "$fakebin/tmux" - fm_fake_exit0 "$fakebin" treehouse + # fm-spawn resolves the worktree from `treehouse get --lease` stdout, so echo + # the caller-controlled FM_FAKE_PANE_PATH to steer WT to the path under test + # (a non-git dir, a primary subdir, or a genuine isolated worktree). + cat > "$fakebin/treehouse" <<'SH' +#!/usr/bin/env bash +if [ "${1:-}" = get ]; then + printf '%s\n' "${FM_FAKE_PANE_PATH:-}" + exit 0 +fi +exit 0 +SH + chmod +x "$fakebin/treehouse" printf '%s\n' "$fakebin" } diff --git a/tests/fm-teardown.test.sh b/tests/fm-teardown.test.sh index a3e8b7ed..66acf539 100755 --- a/tests/fm-teardown.test.sh +++ b/tests/fm-teardown.test.sh @@ -655,6 +655,78 @@ test_gh_error_and_content_absent_refuses() { pass "gh lookup error with content not in default refuses (fail-safe)" } +# Override the treehouse mock so `treehouse return` fails, simulating treehouse +# refusing a worktree it does not manage (the "is not managed by treehouse" +# choke the robustness fix must survive). +add_treehouse_return_failure() { + local case_dir=$1 + cat > "$case_dir/fakebin/treehouse" <<'SH' +#!/usr/bin/env bash +if [ "${1:-}" = return ]; then + echo "error: worktree is not managed by treehouse" >&2 + exit 1 +fi +exit 0 +SH + chmod +x "$case_dir/fakebin/treehouse" +} + +# A meta that records a directory which is NOT a registered worktree of the +# project (reproduces a pre-fix meta that recorded the launch cwd / firstmate +# home instead of the leased worktree) must not brick teardown: it should skip +# the worktree return, warn, and still kill the window and clear volatile state. +test_stale_nontreehouse_worktree_degrades_gracefully() { + local case_dir rc other + case_dir=$(make_case stale-nontreehouse) + # Standalone clone of origin: HEAD is on origin/main (landed, so the + # landed-work check passes to the worktree-return step) but the clone is not a + # worktree registered in the project's `git worktree list`. + other="$case_dir/not-a-pool-worktree" + git clone -q "$case_dir/origin.git" "$other" + fm_write_meta "$case_dir/state/task-x1.meta" \ + "window=fm-task-x1" \ + "worktree=$other" \ + "project=$case_dir/project" \ + "kind=ship" \ + "mode=no-mistakes" + add_treehouse_return_failure "$case_dir" + + set +e + run_teardown "$case_dir" > "$case_dir/stdout" 2> "$case_dir/stderr" + rc=$? + set -e + + expect_code 0 "$rc" "stale-nontreehouse: teardown must not abort on a non-treehouse worktree" + grep -q "not a treehouse-managed worktree" "$case_dir/stderr" \ + || fail "stale-nontreehouse: expected a graceful warning about the non-treehouse worktree"$'\n'"$(cat "$case_dir/stderr")" + assert_absent "$case_dir/state/task-x1.meta" "stale-nontreehouse: meta should be cleared after teardown" + pass "teardown degrades gracefully when the recorded worktree is not a treehouse worktree" +} + +# When the recorded worktree IS a registered worktree but `treehouse return` +# itself fails (e.g. already returned, or a transient treehouse error), teardown +# must warn and continue rather than aborting mid-cleanup. +test_registered_worktree_return_failure_is_nonfatal() { + local case_dir rc + case_dir=$(make_case return-failure-nonfatal) + write_meta "$case_dir" no-mistakes ship + wt_commit "$case_dir" "shippable work" + git -C "$case_dir/wt" push -q origin fm/task-x1 + git -C "$case_dir/project" fetch -q origin + add_treehouse_return_failure "$case_dir" + + set +e + run_teardown "$case_dir" > "$case_dir/stdout" 2> "$case_dir/stderr" + rc=$? + set -e + + expect_code 0 "$rc" "return-failure-nonfatal: a failed treehouse return must not abort teardown" + grep -q "treehouse return failed" "$case_dir/stderr" \ + || fail "return-failure-nonfatal: expected a non-fatal warning about the failed return"$'\n'"$(cat "$case_dir/stderr")" + assert_absent "$case_dir/state/task-x1.meta" "return-failure-nonfatal: meta should be cleared after teardown" + pass "a failed treehouse return warns but does not abort teardown" +} + test_local_only_force_overrides_unpushed() { local case_dir rc case_dir=$(make_case force-override) @@ -690,3 +762,5 @@ test_content_in_default_fallback_allows test_content_fallback_refreshes_stale_origin_ref test_dirty_worktree_refuses test_gh_error_and_content_absent_refuses +test_stale_nontreehouse_worktree_degrades_gracefully +test_registered_worktree_return_failure_is_nonfatal