Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
45 changes: 31 additions & 14 deletions bin/fm-spawn.sh
Original file line number Diff line number Diff line change
Expand Up @@ -786,23 +786,40 @@ spawn_send_key() { # <target> <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=<firstmate home> 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-<id>/ with Go's build temp nested at gotmp/. Go won't
Expand Down
52 changes: 41 additions & 11 deletions bin/fm-teardown.sh
Original file line number Diff line number Diff line change
Expand Up @@ -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() { # <project> <wt>
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") || {
Expand Down Expand Up @@ -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

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 👍 / 👎.

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:-<empty>}' 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
Expand Down
50 changes: 33 additions & 17 deletions tests/fm-backend.test.sh
Original file line number Diff line number Diff line change
Expand Up @@ -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" <<SH
#!/usr/bin/env bash
{ printf 'treehouse'; for a in "\$@"; do printf '\\x1f%s' "\$a"; done; printf '\\n'; } >> "\${FM_TMUX_LOG:?}"
if [ "\${1:-}" = get ]; then
printf '%s\\n' "$wt"
fi
exit 0
SH
chmod +x "$fb/treehouse"
printf '%s\n' "$fb"
}

Expand All @@ -725,43 +735,49 @@ run_spawn_case() { # <bin-root> <fakebin> <log> <state> <data> <config> <proj>
}

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 <worktree>` 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 <worktree>' 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 ----------------------------------------------
Expand Down
13 changes: 12 additions & 1 deletion tests/fm-grok-harness.test.sh
Original file line number Diff line number Diff line change
Expand Up @@ -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"
}

Expand Down
12 changes: 12 additions & 0 deletions tests/fm-secondmate-harness.test.sh
Original file line number Diff line number Diff line change
Expand Up @@ -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"
}

Expand Down
13 changes: 12 additions & 1 deletion tests/fm-spawn-dispatch-profile.test.sh
Original file line number Diff line number Diff line change
Expand Up @@ -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"
}

Expand Down
121 changes: 121 additions & 0 deletions tests/fm-spawn-worktree-lease.test.sh
Original file line number Diff line number Diff line change
@@ -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/<id>.meta recorded
# worktree=<firstmate home / launch cwd> 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" <<SH
#!/usr/bin/env bash
if [ "\${1:-}" = get ]; then
printf '%s\n' "$leased"
exit 0
fi
exit 0
SH
chmod +x "$fakebin/tmux" "$fakebin/treehouse"

printf '%s\n' "$case_dir"
}

run_spawn() {
local home=$1 fakebin=$2; shift 2
# Clear ambient overrides, pin FM_HOME to the sandbox, keep FM_ROOT on the
# real repo (so bin/ helpers resolve), force the tmux backend, and unset TMUX
# so container-ensure takes the detached-session path.
env -u TMUX \
FM_ROOT_OVERRIDE="$ROOT" \
FM_HOME="$home" \
FM_STATE_OVERRIDE='' FM_DATA_OVERRIDE='' FM_PROJECTS_OVERRIDE='' FM_CONFIG_OVERRIDE='' \
FM_BACKEND=tmux \
FM_SPAWN_NO_GUARD=1 \
PATH="$fakebin:$PATH" \
"$SPAWN" "$@"
}

test_meta_records_leased_worktree_path() {
local id case_dir home fakebin leased rc meta_wt
id="lease-probe-a1"
case_dir=$(make_spawn_case lease-basic "$id")
home="$case_dir/home"
fakebin="$case_dir/fakebin"
leased="$case_dir/leased-worktree"

set +e
run_spawn "$home" "$fakebin" "$id" projects/repo codex \
> "$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
Loading
Loading