diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 6e066fa8..e5b9ba61 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -87,7 +87,7 @@ tests/fm-session-start.test.sh # fm-session-start.sh: ABSENT vs empty tests/fm-grok-harness.test.sh # grok adapter spawn hook, token guard, teardown cleanup, and session-lock detection tests tests/fm-fleet-sync.test.sh # project clone refresh: safe detached recovery, STUCK drift reports, benign skips, and bootstrap relay tests/fm-x-mode.test.sh # X-mode poll, inbox context round-trip, reply threading, dismiss, completion follow-up counters/caps, dry-run preview, and .env-presence activation tests -tests/fm-tangle-guard.test.sh # primary-checkout tangle detection, read-only remediation suppression, and spawn/brief isolation tests +tests/fm-tangle-guard.test.sh # primary-checkout tangle detection, read-only remediation suppression, spawn/brief isolation, and fm-spawn tmux window construction (append-form creation, name pinning, window-id targeting) tests tests/fm-brief.test.sh # fm-brief.sh bash -n parse regression guard (issue #166) and clean no-mistakes/direct-PR/local-only brief generation tests tests/fm-spawn-batch.test.sh # batch dispatch and FM_HOME project-path scoping tests tests/fm-spawn-dispatch-profile.test.sh # concrete dispatch profile flags: active-profile backstop, harness/model/effort meta, launch templates, batch forwarding, and secondmate exemption diff --git a/bin/backends/tmux.sh b/bin/backends/tmux.sh index bf2f9674..2af5a1e6 100644 --- a/bin/backends/tmux.sh +++ b/bin/backends/tmux.sh @@ -68,14 +68,28 @@ fm_backend_tmux_container_ensure() { # fm_backend_tmux_create_task: create the task's window in , # refusing an existing in . Mirrors fm-spawn.sh's # duplicate-check-then-new-window sequence, including the exact error text -# (session:window, matching how fm-spawn.sh composed its own $T). -fm_backend_tmux_create_task() { # - local ses=$1 wname=$2 proj_abs=$3 +# (session:window, matching how fm-spawn.sh composed its own $T). Prints the +# created window's stable window id on stdout for the caller to target. +# +# Robustness (fm-spawn tmux window handling under a non-default captain config): +# - Capture a STABLE window id with -P -F '#{window_id}', and let tmux append +# at the next free index by targeting the session with a trailing colon +# ("$ses:"), so a non-default base-index (e.g. base-index 1) cannot collide. +# - PIN the window name by disabling automatic-rename and allow-rename on the +# new window: the captain's tmux may rename the window away from fm- once +# treehouse cd's into the worktree, which would break name-based targeting. +# The returned window id lets callers target the window even if its name is ever +# lost, so worktree discovery cannot fall back to the active client's window. +fm_backend_tmux_create_task() { # -> prints window id + local ses=$1 wname=$2 proj_abs=$3 wid if tmux list-windows -t "$ses" -F '#{window_name}' | grep -qx "$wname"; then echo "error: window $ses:$wname already exists" >&2 return 1 fi - tmux new-window -d -t "$ses" -n "$wname" -c "$proj_abs" + wid=$(tmux new-window -dP -F '#{window_id}' -t "$ses:" -n "$wname" -c "$proj_abs") || return 1 + tmux set-window-option -t "$wid" automatic-rename off 2>/dev/null || true + tmux set-window-option -t "$wid" allow-rename off 2>/dev/null || true + printf '%s\n' "$wid" } # fm_backend_tmux_current_path: the live pane's current working directory, or diff --git a/bin/fm-spawn.sh b/bin/fm-spawn.sh index bd760060..338a4762 100755 --- a/bin/fm-spawn.sh +++ b/bin/fm-spawn.sh @@ -661,7 +661,14 @@ case "$BACKEND" in tmux) SES=$(fm_backend_tmux_container_ensure) T="$SES:$W" - fm_backend_tmux_create_task "$SES" "$W" "$PROJ_ABS" || exit 1 + # #134 robustness (tmux): fm_backend_tmux_create_task captures a stable window + # id and pins the window name (automatic-rename/allow-rename off) so a captain's + # non-default tmux config cannot rename the window away from fm- once + # treehouse cd's into the worktree. WT_TARGET carries that stable id for the + # rename-critical worktree-detection steps below; the persisted window= handle + # stays $T (the name form), which is safe now that rename is disabled. + WID=$(fm_backend_tmux_create_task "$SES" "$W" "$PROJ_ABS") || exit 1 + WT_TARGET="$WID" ;; herdr) # fm_backend_herdr_workspace_label resolves the target workspace from @@ -750,6 +757,12 @@ EOF T="$ORCA_TERMINAL" ;; esac +# #134 robustness: only tmux needs a worktree-detection target distinct from $T - +# its rename-safe stable window id, set as WT_TARGET=$WID in the tmux branch above. +# Every other backend addresses its pane/surface by the id already in $T, so default +# WT_TARGET to $T for them (and for any future backend) - the shared treehouse-get + +# worktree-detection steps below must never reference an unbound WT_TARGET under set -u. +: "${WT_TARGET:=$T}" spawn_send_text_line() { # case "$BACKEND" in tmux) fm_backend_tmux_send_text_line "$1" "$2" ;; @@ -786,11 +799,15 @@ spawn_send_key() { # esac } if [ "$KIND" != secondmate ] && [ "$BACKEND" != orca ]; then - spawn_send_text_line "$T" 'treehouse get' + spawn_send_text_line "$WT_TARGET" 'treehouse get' # Wait for the treehouse subshell: the pane's cwd moves from the project to the worktree. + # Target the stable window id, not the name: if the name is ever lost (e.g. an + # automatic-rename slips through), display-message -t falls back to the + # active client's window, which would misread firstmate's OWN pane path as the + # worktree and tangle a hook into the primary checkout. The window id never lies. for _ in $(seq 1 60); do - p=$(spawn_current_path "$T" || true) + p=$(spawn_current_path "$WT_TARGET" || true) if [ -n "$p" ] && [ "$p" != "$PROJ_ABS" ]; then WT="$p" break diff --git a/tests/fm-backend.test.sh b/tests/fm-backend.test.sh index 8254cfb8..94d1a59a 100755 --- a/tests/fm-backend.test.sh +++ b/tests/fm-backend.test.sh @@ -713,56 +713,20 @@ SH printf '%s\n' "$fb" } -run_spawn_case() { # -- - local bin=$1 fb=$2 log=$3 state=$4 data=$5 config=$6 proj=$7; shift 7 - [ "${1:-}" = -- ] && shift - : > "$log" - env PATH="$fb:$PATH" FM_ROOT_OVERRIDE="$bin" \ - FM_STATE_OVERRIDE="$state" FM_DATA_OVERRIDE="$data" FM_CONFIG_OVERRIDE="$config" \ - FM_PROJECTS_OVERRIDE="$TMP_ROOT/unused-projects" \ - FM_SPAWN_NO_GUARD=1 TMUX="fake,1,0" FM_TMUX_LOG="$log" \ - "$bin/bin/fm-spawn.sh" "$@" -} - -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) - 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" - - 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" - - 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")" - - # 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" -} +# NOTE: the old-vs-new spawn command-log conformance test that used to live here +# was retired. It asserted the P1 backend refactor was a byte-for-byte pure +# extraction of the spawn window-creation/targeting sequence, but that sequence +# is now DELIBERATELY changed: fm-spawn drives the tmux backend to capture a +# stable window id, pin the window name (automatic-rename/allow-rename off), and +# target that id for the rename-critical spawn steps (robustness under a +# captain's non-default tmux config). A byte-identical old-vs-new diff can no +# longer hold there by design. That intended sequence is now authoritatively and +# comprehensively verified - via a recording fake-tmux - by +# tests/fm-tangle-guard.test.sh ("fm-spawn: appends windows by session-colon, +# pins the name, and targets the window id"), and the real tmux create/kill path +# by tests/fm-backend-tmux-smoke.test.sh. The send/peek/teardown conformance +# tests below remain pure extractions and stay. (make_spawn_fakebin is retained; +# test_spawn_default_backend_writes_no_meta_field still uses it.) # --- old vs new: fm-teardown.sh ---------------------------------------------- @@ -963,7 +927,6 @@ test_resolve_selector_three_forms test_backend_of_selector_matches_explicit_target_meta test_send_conformance_old_vs_new test_peek_conformance_old_vs_new -test_spawn_conformance_old_vs_new test_teardown_conformance_old_vs_new test_spawn_refuses_unknown_backend_flag test_spawn_refuses_unknown_fm_backend_env diff --git a/tests/fm-tangle-guard.test.sh b/tests/fm-tangle-guard.test.sh index 92a697d8..a6f24eaa 100755 --- a/tests/fm-tangle-guard.test.sh +++ b/tests/fm-tangle-guard.test.sh @@ -214,8 +214,96 @@ test_spawn_isolation_abort() { pass "fm-spawn: aborts unless the resolved worktree is a genuine, isolated worktree" } +# --- GUARD 1c: fm-spawn tmux window construction ---------------------------- + +# The prevention guard also depends on fm-spawn building robust tmux commands +# under a non-default tmux config (base-index 1, automatic-rename on). A RECORDING +# fake tmux logs every invocation and returns a sentinel window id, so these +# assertions pin the command construction deterministically, with no live tmux: +# - window creation targets the session with a trailing colon (append form), so +# tmux appends at the next free index instead of the active window index, which +# collides under base-index 1; +# - the window id is captured (-P -F #{window_id}) and automatic-rename/allow-rename +# are disabled so the fm- name survives treehouse cd'ing into the worktree; +# - the treehouse-get send-keys and the worktree wait loop target that stable +# window id, never the (possibly-renamed) name - a lost name would let +# display-message fall back to the active client's window and misread firstmate's +# OWN pane as the worktree, tangling a hook into the primary checkout. +make_spawn_record_fakebin() { + local dir=$1 fakebin + fakebin=$(fm_fakebin "$dir") + cat > "$fakebin/tmux" <<'SH' +#!/usr/bin/env bash +set -u +[ -n "${FM_TMUX_REC:-}" ] && printf 'tmux %s\n' "$*" >> "$FM_TMUX_REC" +case "$*" in + *"#{pane_current_path}"*) printf '%s\n' "${FM_FAKE_PANE_PATH:-}"; exit 0 ;; +esac +case "${1:-}" in + display-message) printf 'firstmate\n'; exit 0 ;; + new-window) printf '%s\n' "@spawnwid"; exit 0 ;; + list-windows) exit 0 ;; + has-session|new-session|send-keys|set-window-option) exit 0 ;; +esac +exit 0 +SH + chmod +x "$fakebin/tmux" + fm_fake_exit0 "$fakebin" treehouse + printf '%s\n' "$fakebin" +} + +run_spawn_record() { + local home=$1 id=$2 proj=$3 pane=$4 fakebin=$5 rec=$6 + mkdir -p "$home/data/$id" + printf 'brief\n' > "$home/data/$id/brief.md" + FM_ROOT_OVERRIDE='' FM_HOME="$home" \ + FM_STATE_OVERRIDE="$home/state" FM_DATA_OVERRIDE="$home/data" \ + FM_PROJECTS_OVERRIDE="$home/projects" FM_CONFIG_OVERRIDE="$home/config" \ + FM_SPAWN_NO_GUARD=1 FM_FAKE_PANE_PATH="$pane" TMUX="fake,1,0" \ + FM_TMUX_REC="$rec" \ + PATH="$fakebin:$PATH" \ + "$ROOT/bin/fm-spawn.sh" "$id" "$proj" codex 2>&1 +} + +test_spawn_tmux_window_construction() { + local home proj fakebin rec wt out status + home="$TMP_ROOT/spawn-rec-home" + mkdir -p "$home/data" + proj=$(make_repo "$TMP_ROOT/spawn-rec-proj") + fakebin=$(make_spawn_record_fakebin "$TMP_ROOT/spawn-rec-fake") + rec="$TMP_ROOT/spawn-rec.log" + : > "$rec" + wt="$TMP_ROOT/spawn-rec-wt" + git -C "$proj" worktree add -q --detach "$wt" >/dev/null 2>&1 + + out=$(run_spawn_record "$home" rec-win-gg7 "$proj" "$wt" "$fakebin" "$rec"); status=$? + expect_code 0 "$status" "spawn into a genuine worktree should succeed" + assert_contains "$out" "spawned rec-win-gg7" "recording spawn did not report success" + + # Bug 1 fix: append-form window creation (trailing colon on the session target). + assert_grep "new-window -dP -F #{window_id} -t firstmate: -n fm-rec-win-gg7" "$rec" \ + "new-window must append at the session (trailing colon) and capture the window id" + assert_no_grep "new-window -dP -F #{window_id} -t firstmate -n" "$rec" \ + "new-window must not target the bare session name (collides under base-index 1)" + + # Bug 2 fix (a): pin the window name against automatic-rename / allow-rename. + assert_grep "set-window-option -t @spawnwid automatic-rename off" "$rec" \ + "must disable automatic-rename on the spawned window" + assert_grep "set-window-option -t @spawnwid allow-rename off" "$rec" \ + "must disable allow-rename on the spawned window" + + # Bug 2 fix (b): treehouse-get and the worktree wait loop target the stable id. + assert_grep "send-keys -t @spawnwid treehouse get Enter" "$rec" \ + "treehouse get must be sent to the stable window id" + assert_grep "display-message -p -t @spawnwid #{pane_current_path}" "$rec" \ + "the worktree wait loop must query the stable window id, not the name" + + pass "fm-spawn: appends windows by session-colon, pins the name, and targets the window id" +} + test_lib_classification test_guard_banner test_bootstrap_line test_brief_assertion_precedes_branch test_spawn_isolation_abort +test_spawn_tmux_window_construction