diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 6f9e969f..862b9207 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -91,6 +91,8 @@ tests/fm-tangle-guard.test.sh # primary-checkout tangle detection, r 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 +tests/fm-spawn-worktree-symlink.test.sh # worktree wait-loop path canonicalization: a symlinked project path (e.g. /tmp -> /private/tmp) must not misdetect the project as the worktree +tests/fm-gotmp.test.sh # per-task GOTMPDIR temp root: fm-spawn tasktmp= meta contract and fm-teardown cleanup of the recorded temp root tests/fm-update.test.sh # fast-forward-only self-update, reread, nudge, dedup, and skip-safety tests tests/fm-secondmate-sync.test.sh # local-HEAD secondmate sync, no-fetch, bootstrap nudge gating, and spawn hook tests tests/fm-secondmate-harness.test.sh # secondmate-vs-crewmate harness resolution, optional secondmate model/effort pins, primary-to-secondmate config inheritance, and config-push tests @@ -99,9 +101,9 @@ tests/fm-secondmate-safety.test.sh # secondmate home safety, idle charter tests/fm-teardown.test.sh # fm-teardown.sh landed-work safety and reminder checks: fork-remote allow, squash/content landings, dirty and unlanded refusals, PR-head metadata, no-pr= branch discovery, tasks-axi/manual backlog reminder, --force override tests/fm-review-diff.test.sh # fm-review-diff.sh authoritative review diff coverage: recorded pr_head=, fetched refs/pull//head, no-pr local branch behavior, and warning fallback tests/fm-pr-merge.test.sh # fm-pr-merge.sh records pr= and available pr_head= before merging, parses PR URLs into gh-axi number/--repo calls, defaults to squash, preserves explicit merge methods, rejects malformed URLs and repo overrides, and propagates real merge failures -tests/fm-crew-state.test.sh # fm-crew-state.sh current-state reconciliation: run-step authority including closed panes, stale needs-decision/blocked superseded by a resumed run, genuine-parked, cross-branch runs-list attribution, pane/status-log fallback, scout skip, torn-down/missing-meta graceful +tests/fm-crew-state.test.sh # fm-crew-state.sh current-state reconciliation: run-step authority including closed panes, stale needs-decision/blocked superseded by a resumed run, genuine-parked, cross-branch runs-list attribution, bounded run-lookup retry/backoff recovering a raced `axi status` or coarse runs-list lookup, pane/status-log fallback, scout skip, torn-down/missing-meta graceful tests/fm-backend.test.sh # runtime-backend abstraction: fm-backend.sh selection/meta/dispatch helpers, shell-portable sourced backend matching, and old-vs-new fake-tool command-log conformance for fm-send/fm-peek/fm-spawn/fm-teardown -tests/fm-backend-tmux-smoke.test.sh # real (private-socket) tmux smoke test for the tmux adapter: create/duplicate-refuse, send text + Enter, send literal + key, bounded capture, live-window resolve, kill +tests/fm-backend-tmux-smoke.test.sh # real (private-socket) tmux smoke test for the tmux adapter: create/duplicate-refuse, new-window trailing-colon session target (renumber-windows index-in-use guard), send text + Enter, send literal + key, bounded capture, live-window resolve, kill tests/fm-backend-herdr.test.sh # fake herdr CLI unit tests for the experimental herdr adapter, including version/tool gates, target parsing, send/capture, structural composer-state verification, slash-submit retry regression coverage, native busy state, per-home workspace-label resolution, default-tab prune safety, restored-layout husk replacement, and verified CLI bug workarounds tests/fm-backend-herdr-smoke.test.sh # real herdr adapter smoke test, skipped when herdr or jq is unavailable, using an isolated throwaway HERDR_SESSION and guarded session cleanup, including live-agent duplicate refusal and no-agent husk replacement tests/fm-backend-autodetect-smoke.test.sh # real herdr auto-detection smoke test, skipped when herdr, jq, or treehouse is unavailable, using the same guarded session cleanup diff --git a/bin/backends/tmux.sh b/bin/backends/tmux.sh index bf2f9674..12db826e 100644 --- a/bin/backends/tmux.sh +++ b/bin/backends/tmux.sh @@ -69,13 +69,22 @@ fm_backend_tmux_container_ensure() { # 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). +# +# The new-window target is "$ses:" (trailing colon), NOT the bare "$ses". A bare +# session target resolves to that session's CURRENT window, so tmux inserts +# next-up-from-current; with `renumber-windows on` (tmux >= 3.7) that can land on +# an occupied index and fail "index in use". The trailing colon is the documented +# "pick the next free window index in this session" form and is unambiguous across +# tmux versions/configs, so it replaces the external PATH shim that used to rewrite +# bare session targets. Behavior is identical to the bare form when the bare form +# succeeds (both append at the next free index). fm_backend_tmux_create_task() { # local ses=$1 wname=$2 proj_abs=$3 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" + tmux new-window -d -t "$ses:" -n "$wname" -c "$proj_abs" } # fm_backend_tmux_current_path: the live pane's current working directory, or diff --git a/bin/fm-crew-state.sh b/bin/fm-crew-state.sh index 5c11c6ae..750bac68 100755 --- a/bin/fm-crew-state.sh +++ b/bin/fm-crew-state.sh @@ -307,13 +307,22 @@ log_reports_ci_ready() { # " []" separated by runs of # spaces (verified: no quoting, so splitting on the first two whitespace runs # is exact) - but branch + coarse status is exactly what this predicate needs: -# is a run for THIS branch active right now. Echoes the first (most recent) -# matching row's status word (running/completed/cancelled/failed), or empty -# when the branch has no run within FM_CREW_STATE_RUNS_LIMIT rows. -nm_runs_status_for_branch() { # +# is a run for THIS branch active right now. Sets COARSE_STATUS to the first +# (most recent) matching row's status word (running/completed/cancelled/failed), +# or empty when the branch has no run within FM_CREW_STATE_RUNS_LIMIT rows. +# Results come back through globals rather than stdout because the retry loop +# below also needs NM_RUNS_RESPONDED - whether the bounded `no-mistakes runs` +# call produced any output at all - and a command-substitution subshell could +# not report that second fact. `no-mistakes runs` always emits text when the +# CLI is alive (even with zero runs it prints a "no runs" line), so an empty +# capture reliably means the call timed out, never a legitimate empty answer. +nm_runs_status_for_branch() { # ; sets COARSE_STATUS + NM_RUNS_RESPONDED local branch=$1 out row st rest br + COARSE_STATUS="" + NM_RUNS_RESPONDED=0 out=$(nm_run runs --limit "$FM_CREW_STATE_RUNS_LIMIT") [ -n "$out" ] || return 0 + NM_RUNS_RESPONDED=1 while IFS= read -r row; do row=$(trim "$row") [ -n "$row" ] || continue @@ -322,7 +331,7 @@ nm_runs_status_for_branch() { # rest=$(trim "$rest") br=${rest%% *} if [ "$br" = "$branch" ]; then - printf '%s' "$st" + COARSE_STATUS=$st return 0 fi done <<< "$out" @@ -340,28 +349,68 @@ HAVE_RUN=0 # run-step block below skips the TOON field parsing entirely for this crew. RUN_SOURCE=full COARSE_STATUS="" +NM_RUNS_RESPONDED=0 +# Bounded retry/backoff around the run-attribution lookup. During an actively +# running pipeline the bounded no-mistakes calls can lose a race - time out to +# empty (the CLI is busy serving the run) - and leave HAVE_RUN=0. The caller +# would then fall through to the possibly-stale status log, which defeats the +# watcher's provably-working absorption and surfaces a validating crew as +# stale every poll (per-minute false stale wakes during long validations). +# Re-attempt a few times with a short backoff before accepting a +# non-authoritative verdict. Only an unresponsive attempt is retried, and an +# empty result from EITHER bounded call - `axi status` or the coarse +# `no-mistakes runs` list - is that timeout/race signature, because both +# commands emit non-empty text whenever the CLI is alive (`runs` prints a "no +# runs" line even with zero runs). An attempt is authoritative (no retry) only +# when every call that left HAVE_RUN=0 actually answered: `axi status` +# reported another branch's run AND the coarse runs list answered with no row +# for this branch. That definitive "no run for this branch" breaks the loop +# without retrying, because retrying cannot change an authoritative answer and +# would only add hot-path latency for the steady-state implementing crew +# (branch created, validation not yet started). +# This is the upstream replacement for the local state/.crew-state-retry.sh +# (FM_CREW_STATE_BIN) production mitigation, which wrapped this whole helper +# with the same retry-until-run-step/pane semantics. +# With FM_CREW_STATE_RETRIES=0 the loop runs exactly once, byte-identical to the +# original single-attempt behavior (used by the test suite to stay fast). +CREW_STATE_RETRIES=${FM_CREW_STATE_RETRIES:-2} +case "$CREW_STATE_RETRIES" in ''|*[!0-9]*) CREW_STATE_RETRIES=2 ;; esac +CREW_STATE_RETRY_DELAY=${FM_CREW_STATE_RETRY_DELAY:-2} +case "$CREW_STATE_RETRY_DELAY" in ''|*[!0-9]*) CREW_STATE_RETRY_DELAY=2 ;; esac # Scouts and secondmates never drive a no-mistakes validation of their own # worktree, so skip the lookup for them and read state from pane/log directly. if [ "$KIND" = ship ] && [ -n "$CREW_BRANCH" ] && command -v no-mistakes >/dev/null 2>&1; then - RUN_OUT=$(nm_run axi status) - if [ -n "$RUN_OUT" ]; then - run_branch=$(strip_quotes "$(nm_field branch)") - if [ -n "$run_branch" ] && [ "$run_branch" = "$CREW_BRANCH" ]; then - HAVE_RUN=1 - else - # The active-or-most-recent run is for another branch (the CLI is alive - # and answered; only the attribution missed) - try the coarse fallback. - # Deliberately nested inside `[ -n "$RUN_OUT" ]`: an empty/timed-out - # primary call means the CLI itself did not respond, so retrying it - # immediately with a second bounded call would just double the wait - # for no better answer. - COARSE_STATUS=$(nm_runs_status_for_branch "$CREW_BRANCH") - if [ -n "$COARSE_STATUS" ]; then + attempt=0 + while : ; do + cli_responded=0 + RUN_OUT=$(nm_run axi status) + if [ -n "$RUN_OUT" ]; then + cli_responded=1 + run_branch=$(strip_quotes "$(nm_field branch)") + if [ -n "$run_branch" ] && [ "$run_branch" = "$CREW_BRANCH" ]; then HAVE_RUN=1 - RUN_SOURCE=coarse + else + # The active-or-most-recent run is for another branch (the CLI is alive + # and answered; only the attribution missed) - try the coarse fallback. + # Deliberately nested inside `[ -n "$RUN_OUT" ]`: an empty/timed-out + # primary call means the CLI itself did not respond within this attempt, + # so the coarse re-query would just double the wait; the retry loop below + # is what gives the CLI another chance after a backoff instead. + nm_runs_status_for_branch "$CREW_BRANCH" + if [ -n "$COARSE_STATUS" ]; then + HAVE_RUN=1 + RUN_SOURCE=coarse + elif [ "$NM_RUNS_RESPONDED" = 0 ]; then + cli_responded=0 + fi fi fi - fi + [ "$HAVE_RUN" = 1 ] && break + [ "$cli_responded" = 0 ] || break + [ "$attempt" -lt "$CREW_STATE_RETRIES" ] || break + attempt=$((attempt + 1)) + [ "$CREW_STATE_RETRY_DELAY" -gt 0 ] && sleep "$CREW_STATE_RETRY_DELAY" + done fi # --- run-step authoritative path ------------------------------------------- diff --git a/bin/fm-spawn.sh b/bin/fm-spawn.sh index bf3b30fd..ffd5692f 100755 --- a/bin/fm-spawn.sh +++ b/bin/fm-spawn.sh @@ -785,11 +785,21 @@ 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. + # Compare CANONICALIZED paths: the backend reports the pane's real cwd (e.g. tmux's + # #{pane_current_path} resolves symlinks), while PROJ_ABS was built with `pwd` and can + # still carry a symlink component (on macOS /tmp -> /private/tmp, /var -> /private/var). + # Comparing the raw strings would then read as "already moved" while the pane is still + # at the project, setting WT to the project dir and tripping validate_spawn_worktree's + # isolation check. Canonicalize both sides so the loop tracks a genuine cwd change. + PROJ_ABS_REAL=$(cd "$PROJ_ABS" 2>/dev/null && pwd -P) || PROJ_ABS_REAL=$PROJ_ABS for _ in $(seq 1 60); do p=$(spawn_current_path "$T" || true) - if [ -n "$p" ] && [ "$p" != "$PROJ_ABS" ]; then - WT="$p" - break + if [ -n "$p" ]; then + p_real=$(cd "$p" 2>/dev/null && pwd -P) || p_real=$p + if [ "$p_real" != "$PROJ_ABS_REAL" ]; then + WT="$p" + break + fi fi sleep 1 done diff --git a/docs/configuration.md b/docs/configuration.md index 623219b3..b18d7bd0 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -237,6 +237,8 @@ FM_CHECK_INTERVAL=300 # seconds between slow checks (merge polls or the X-mode FM_CHECK_TIMEOUT=30 # seconds allowed per slow check script FM_CREW_STATE_NM_TIMEOUT=10 # seconds allowed per no-mistakes query inside fm-crew-state.sh FM_CREW_STATE_RUNS_LIMIT=200 # recent no-mistakes runs rows scanned when cross-branch attribution falls back from axi status +FM_CREW_STATE_RETRIES=2 # extra run-lookup attempts after a bounded no-mistakes call (axi status or the coarse runs list) times out to empty; 0 = single attempt (fully answered no-run verdicts never retry) +FM_CREW_STATE_RETRY_DELAY=2 # seconds slept between those run-lookup retry attempts FM_CREW_STATE_BIN=bin/fm-crew-state.sh # test override for the current-state reader used by provably-working watcher triage FMX_PAIRING_TOKEN= # X mode pairing token; .env opt-in authorizes replies and eligible lifecycle actions FMX_RELAY_URL=https://myfirstmate.io # optional X relay override, mainly for local relay development diff --git a/docs/herdr-backend.md b/docs/herdr-backend.md index b513ac72..03c9876f 100644 --- a/docs/herdr-backend.md +++ b/docs/herdr-backend.md @@ -35,7 +35,7 @@ You do not need to attach for routine supervision: `bin/fm-peek.sh fm-` read Verify it works by spawning a trivial task with `--backend herdr` and confirming the task's meta records `backend=herdr` plus `herdr_session=`, `herdr_workspace_id=`, `herdr_tab_id=`, and `herdr_pane_id=`; the workspace for your home should show the new `fm-` tab. -Limitations: herdr is experimental, not yet used for `bin/fm-bootstrap.sh`'s required-tools list (the version/tool gate happens at spawn time instead), and carries the known gaps documented below (a small-`--lines` capture bug with a built-in workaround, and a `pane_cwd`-adjacent worktree-discovery symlink fragility) - see "Known gaps and follow-up notes" at the end of this document. +Limitations: herdr is experimental, not yet used for `bin/fm-bootstrap.sh`'s required-tools list (the version/tool gate happens at spawn time instead), and carries the known gaps documented below (a small-`--lines` capture bug with a built-in workaround) - see "Known gaps and follow-up notes" at the end of this document. ## Status: experimental @@ -376,8 +376,8 @@ This is a test-harness-only concern - `fm_backend_herdr_composer_state` and `fm_ A genuine `events.subscribe`-driven push is a reasonable follow-up, not implemented here. - **`bin/fm-bootstrap.sh`'s required-tools list is unchanged.** It still unconditionally requires `tmux`, and does not yet conditionally add `herdr` and `jq` when a backend selection resolves to herdr. The version/tool gate happens at spawn time instead and refuses loudly, so this is bootstrap-detection polish, not a functional gap. -- **Worktree-discovery isolation guard is symlink-fragile for a project path under a symlinked prefix (e.g. macOS's `/tmp` -> `/private/tmp`).** Discovered while building the runtime-backend-auto-detection real smoke test (`tests/fm-backend-autodetect-smoke.test.sh`), which needed a scratch project. `fm-spawn.sh`'s `PROJ_ABS` is a LOGICAL `cd && pwd` (symlink components kept), while herdr's `foreground_cwd` (and real tmux's `pane_current_path`, on the same OS-level cwd primitive) report the PHYSICALLY resolved path. - When the project itself lives under a symlinked directory, the very first worktree-discovery poll sees two different strings for the identical starting directory and the isolation guard false-refuses the spawn as "not isolated" before `treehouse get` ever moves the pane - backend-agnostic, not specific to herdr. Worked around in the test by resolving its scratch `TMP_ROOT` through `pwd -P` before use; the underlying `fm-spawn.sh` path-comparison gap (worth resolving `PROJ_ABS` physically, or comparing physically-resolved forms in the isolation guard) is unfixed and worth a dedicated follow-up. +- **RESOLVED: worktree-discovery isolation guard was symlink-fragile for a project path under a symlinked prefix (e.g. macOS's `/tmp` -> `/private/tmp`).** Discovered while building the runtime-backend-auto-detection real smoke test (`tests/fm-backend-autodetect-smoke.test.sh`), which needed a scratch project. `fm-spawn.sh`'s `PROJ_ABS` is a LOGICAL `cd && pwd` (symlink components kept), while herdr's `foreground_cwd` (and real tmux's `pane_current_path`, on the same OS-level cwd primitive) report the PHYSICALLY resolved path. + When the project itself lived under a symlinked directory, the very first worktree-discovery poll saw two different strings for the identical starting directory and the isolation guard false-refused the spawn as "not isolated" before `treehouse get` ever moved the pane - backend-agnostic, not specific to herdr. Fixed upstream: `fm-spawn.sh`'s wait loop now canonicalizes both sides (`pwd -P`) before comparing, so the loop only breaks on a genuine cwd move; regression-covered by `tests/fm-spawn-worktree-symlink.test.sh`. The autodetect smoke test's own `pwd -P` resolution of its scratch `TMP_ROOT` remains as belt-and-braces. - **RESOLVED: a restart's restored-layout husk no longer needs a manual pane close before respawn.** See "Respawn idempotency: a restored task tab is a husk, not a duplicate" above for the fix (`fm_backend_herdr_pane_agent_state`, `fm_backend_herdr_create_task`'s close-and-replace). Left over from that fix: the `dead` (`pane_not_found`) husk classification is exercised only at the unit level, never against the real binary - killing a pane's process on a live server was observed to make herdr reap the whole tab immediately (never leaving a dead-but-still-listed pane for the duplicate check to find), and a real session restart was never observed to produce one either. It remains a conservative, defensively-coded path for a herdr failure mode (e.g. a restored process that fails to start) nobody has reproduced against the real binary yet. diff --git a/docs/scripts.md b/docs/scripts.md index 5b550698..f19a8915 100644 --- a/docs/scripts.md +++ b/docs/scripts.md @@ -30,7 +30,7 @@ Each file also starts with a short header comment. | `fm-watch-arm.sh` | Verified per-home watcher re-arm; reports `started`, `healthy`, or `FAILED`; `--restart` relaunches only this home's watcher | | `fm-watch.sh` | Singleton-safe always-on watcher and default pull-based event source; uses backend-native busy state when available before the shared regex fallback, absorbs no-verb signal and stale wakes only when the crew is provably working, checks that evidence before trusting stale status-log terminality, queues and exits for actionable wakes, and reverts to daemon-owned one-shot behavior while `state/.afk` exists | | `fm-supervise-daemon.sh` | Presence-gated sub-supervisor for walk-away (`/afk`) supervision: wraps `fm-watch.sh`, uses the shared wake classifier, backend-aware stale rechecks, and tmux/herdr supervisor injection, self-handles routine wakes in bash, and escalates only captain-relevant events as one verified, batched, single-line digest prefixed with a sentinel marker | -| `fm-crew-state.sh` | Print one stable current-state line for a crew by reconciling its matching no-mistakes run-step, including coarse cross-branch attribution from `no-mistakes runs`, even when the pane has closed, with backend-aware pane fallback that corroborates native idle/unknown verdicts before the status log | +| `fm-crew-state.sh` | Print one stable current-state line for a crew by reconciling its matching no-mistakes run-step, including coarse cross-branch attribution from `no-mistakes runs`, even when the pane has closed, retrying the run lookup with a bounded backoff when a busy `axi status` or coarse `no-mistakes runs` call times out to empty, with backend-aware pane fallback that corroborates native idle/unknown verdicts before the status log | | `fm-tangle-lib.sh` | Shared default-branch resolution and primary-checkout tangle classification sourced by bootstrap and guard | | `fm-supervision-lib.sh` | Shared grace-based "in-flight work exists but no watcher has a fresh beacon" status and predicate used by `fm-guard.sh`; `fm-turnend-guard.sh` uses it for banner fields and relies on `fm-wake-lib.sh` for live watcher lock health | | `fm-ff-lib.sh` | Shared guarded fast-forward helper for `/updatefirstmate` origin pulls and no-fetch local secondmate syncs | diff --git a/tests/fm-backend-tmux-smoke.test.sh b/tests/fm-backend-tmux-smoke.test.sh index 0d47b83e..b35f1c72 100755 --- a/tests/fm-backend-tmux-smoke.test.sh +++ b/tests/fm-backend-tmux-smoke.test.sh @@ -60,6 +60,56 @@ if fm_backend_tmux_create_task "$SESSION" "$WINDOW" "$HOME" 2>/dev/null; then fi pass "real tmux: fm_backend_tmux_create_task creates a window and refuses a duplicate" +# --- new-window target form (bug 3) ------------------------------------------ +# A bare `-t ` target resolves to the session's CURRENT window, so tmux +# inserts next-up-from-current; on tmux >= 3.7 with `renumber-windows on` that can +# land on an occupied index and fail "index in use". The adapter must target +# ":" so tmux picks the next free index unambiguously (this also replaces +# the external PATH shim that used to rewrite bare session targets). +# +# Guard 1 (build-independent): capture the new-window invocation and assert the +# trailing-colon session target, never the bare session name. A `tmux` function +# shadows the PATH shim just for these calls (an `if`, not a `case`, and defined +# at top level - bash 3.2 cannot parse a function containing `case` inside $(...)), +# then it is unset so the rest of the suite uses the real socketed tmux again. +CAP_LOG="$SHIM_DIR/newwindow.args" +: > "$CAP_LOG" +# shellcheck disable=SC2329 # invoked indirectly by fm_backend_tmux_create_task +tmux() { + if [ "${1:-}" = new-window ]; then shift; printf '%s\n' "$*" >> "$CAP_LOG"; return 0; fi + return 0 # list-windows etc: empty output -> the duplicate check finds nothing +} +fm_backend_tmux_create_task "capture-ses" "fm-cap" "/tmp" +unset -f tmux +newwin_args=$(cat "$CAP_LOG") +case "$newwin_args" in + *"-t capture-ses: "*) : ;; + *"-t capture-ses "*) fail "create_task used a BARE session target (bug 3): $newwin_args" ;; + *) fail "create_task new-window args were not as expected: $newwin_args" ;; +esac +pass "fm_backend_tmux_create_task targets ':' (next free index), not the bare session" + +# Guard 2 (real tmux behavioral): with renumber-windows on and a non-last active +# window, create_task must still succeed and the window must appear. +RENUM_SES="renum" +tmux new-session -d -s "$RENUM_SES" -x 200 -y 50 || fail "real tmux: new-session (renum) failed" +tmux set-option -t "$RENUM_SES" renumber-windows on +tmux set-option -t "$RENUM_SES" base-index 0 +fm_backend_tmux_create_task "$RENUM_SES" "fm-r1" "$HOME" || fail "create_task fm-r1 failed under renumber-windows" +fm_backend_tmux_create_task "$RENUM_SES" "fm-r2" "$HOME" || fail "create_task fm-r2 failed under renumber-windows" +fm_backend_tmux_create_task "$RENUM_SES" "fm-r3" "$HOME" || fail "create_task fm-r3 failed under renumber-windows" +# Select a MIDDLE (non-last) window active, so a bare `-t ` would target +# an occupied index; create_task must still succeed via the trailing-colon form. +tmux select-window -t "$RENUM_SES:1" 2>/dev/null || tmux select-window -t "$RENUM_SES" +fm_backend_tmux_create_task "$RENUM_SES" "fm-r4" "$HOME" \ + || fail "create_task fm-r4 failed with a non-last active window under renumber-windows (bug 3)" +for w in fm-r1 fm-r2 fm-r3 fm-r4; do + tmux list-windows -t "$RENUM_SES" -F '#{window_name}' | grep -qx "$w" \ + || fail "window $w not visible after create_task under renumber-windows" +done +tmux kill-session -t "$RENUM_SES" 2>/dev/null || true +pass "real tmux: create_task succeeds under renumber-windows with a non-last active window" + # --- send text + Enter ------------------------------------------------------- tmux send-keys -t "$TARGET" "cd /tmp && PS1='smoke\$ '" Enter diff --git a/tests/fm-crew-state.test.sh b/tests/fm-crew-state.test.sh index 7e88bbc6..dcf6bc8f 100755 --- a/tests/fm-crew-state.test.sh +++ b/tests/fm-crew-state.test.sh @@ -36,6 +36,14 @@ CREW_STATE="$ROOT/bin/fm-crew-state.sh" TMP_ROOT=$(fm_test_tmproot fm-crew-state) fm_git_identity fmtest fmtest@example.invalid +# The helper now retries the bounded run-attribution lookup with a backoff before +# accepting a non-authoritative verdict (the fix for the race that surfaced +# validating crews as stale every poll). These hermetic fakes are deterministic, +# so a retry never changes the outcome - it would only add real sleeps. Pin the +# backoff to 0 so the suite stays fast; the dedicated retry cases below set their +# own FM_CREW_STATE_RETRIES to exercise the loop explicitly. +export FM_CREW_STATE_RETRY_DELAY=0 + # A real git repo checked out on , so the helper's branch attribution # (git symbolic-ref) resolves like it would for a live crew worktree. make_repo_on_branch() { # @@ -672,15 +680,21 @@ SH toolbin=$(make_no_timeout_toolbin "$d") fm_write_meta "$d/state/feat-timeout.meta" "window=fm:fm-feat-timeout" "worktree=$d/wt" "kind=ship" FM_FAKE_BUSY=1 + # A hanging no-mistakes: the perl bound must cap EACH attempt at NM_TIMEOUT, and + # the run-attribution retry (2 extra attempts, no backoff here) must still fire on + # the timed-out-to-empty path - that empty result IS the race the retry exists to + # cover. So the CLI is called 1 + FM_CREW_STATE_RETRIES times, each perl-bounded, + # and the total stays bounded (never the unbounded hang). start=$SECONDS - out=$(FM_FAKE_NM_CALLS="$calls_file" PATH="$d/fakebin:$toolbin" FM_STATE_OVERRIDE="$d/state" FM_CREW_STATE_NM_TIMEOUT=1 "$CREW_STATE" feat-timeout) + out=$(FM_FAKE_NM_CALLS="$calls_file" PATH="$d/fakebin:$toolbin" FM_STATE_OVERRIDE="$d/state" \ + FM_CREW_STATE_NM_TIMEOUT=1 FM_CREW_STATE_RETRIES=2 FM_CREW_STATE_RETRY_DELAY=0 "$CREW_STATE" feat-timeout) elapsed=$((SECONDS - start)) assert_contains "$out" "state: working" "timed-out no-mistakes falls back to pane" assert_contains "$out" "source: pane" "timed-out no-mistakes -> pane source" - [ "$elapsed" -lt 5 ] || fail "perl timeout did not bound no-mistakes calls (elapsed ${elapsed}s)" + [ "$elapsed" -lt 8 ] || fail "perl timeout did not bound no-mistakes calls (elapsed ${elapsed}s)" calls=$(awk 'END { print NR + 0 }' "$calls_file" 2>/dev/null || echo 0) - [ "$calls" -eq 1 ] || fail "empty no-mistakes status triggered extra lookups ($calls calls)" - pass "no timeout command uses perl bound" + [ "$calls" -eq 3 ] || fail "retry should re-attempt the timed-out lookup 1+2 times (got $calls calls)" + pass "no timeout command uses perl bound; retry re-attempts the timed-out lookup within the bound" } # (i) kind=scout skips the run lookup entirely (its deliverable is a report). @@ -769,6 +783,145 @@ EOF pass "crew_is_provably_working still surfaces a genuinely stopped crew (safety property preserved)" } +# Bug-5 regression: the bounded run-attribution lookup can lose a race against an +# actively-running pipeline (the CLI is busy and `axi status` times out to empty). +# The retry/backoff must re-attempt and recover this crew's own run-step, so a +# validating crew reads `working / run-step` instead of falling through to the +# possibly-stale status log (which defeated the watcher's provably-working +# absorption and caused per-minute false stale wakes). A stateful fake returns +# empty on the first call, then this branch's run. +race_recovery_fakebin() { # [] + local d=$1 empty_until=$2 runs_empty_until=${3:-0} fb="$1/fakebin" + mkdir -p "$fb" + : > "$d/calln" + : > "$d/runs-calln" + cat > "$fb/no-mistakes" <> "$fb/no-mistakes" <<'SH' +case "${1:-}" in + axi) + if [ "${2:-}" = status ] && [ "${3:-}" != --run ]; then + n=$(cat "$calln" 2>/dev/null || echo 0); n=$((n + 1)); printf '%s' "$n" > "$calln" + [ "$n" -le "$empty_until" ] && exit 0 # race lost on this attempt: empty + printf '%s\n' "${FM_FAKE_AXI_STATUS:-}" + fi ;; + runs) + n=$(cat "$runs_calln" 2>/dev/null || echo 0); n=$((n + 1)); printf '%s' "$n" > "$runs_calln" + [ "$n" -le "$runs_empty_until" ] && exit 0 # coarse race lost: empty + printf '%s\n' "${FM_FAKE_RUNS_LIST:-}" ;; +esac +exit 0 +SH + cat > "$fb/tmux" <<'SH' +#!/usr/bin/env bash +set -u +case "${1:-}" in + display-message) printf '%%1\n' ;; + capture-pane) + if [ "${FM_FAKE_BUSY:-0}" = 1 ]; then printf 'work\nesc to interrupt\n'; else printf 'quiet\n> \n'; fi ;; +esac +exit 0 +SH + chmod +x "$fb/no-mistakes" "$fb/tmux" +} + +test_retry_recovers_raced_run_lookup() { + reset_fakes + local d out; d=$(new_case retry-recovers) + make_repo_on_branch "$d/wt" fm/feat-race + race_recovery_fakebin "$d" 1 + fm_write_meta "$d/state/feat-race.meta" "window=fm:fm-feat-race" "worktree=$d/wt" "kind=ship" + FM_FAKE_AXI_STATUS="$(run_running fm/feat-race)" + FM_FAKE_BUSY=0 + # With retries on, the second attempt attributes the run: working / run-step. + out=$(PATH="$d/fakebin:$PATH" FM_STATE_OVERRIDE="$d/state" \ + FM_CREW_STATE_RETRIES=2 FM_CREW_STATE_RETRY_DELAY=0 "$CREW_STATE" feat-race) + assert_contains "$out" "state: working" "retry recovered the raced run lookup (working)" + assert_contains "$out" "source: run-step" "retry recovered the authoritative run-step source" + + # With retries OFF, the same first-call-empty race is NOT recovered: the single + # attempt sees empty and falls through off the run-step path. This pins the retry + # as the load-bearing fix, not incidental behavior. + : > "$d/calln" + out=$(PATH="$d/fakebin:$PATH" FM_STATE_OVERRIDE="$d/state" \ + FM_CREW_STATE_RETRIES=0 "$CREW_STATE" feat-race) + assert_not_contains "$out" "source: run-step" "without retry the raced lookup is not recovered" + pass "bounded retry recovers a run lookup that lost the race against an active pipeline" +} + +# The same race can hit the COARSE call instead: `axi status` answers with +# another branch's run (the CLI is alive), but the bounded `no-mistakes runs` +# list itself times out to empty on this attempt. Empty runs output is the +# timeout signature - the real command always prints text when the CLI is +# alive, even with zero runs - so the attempt must count as unresponsive and +# retry, never be mistaken for an authoritative "no run for this branch". +# Otherwise a validating crew whose active run is not the repo's most recent +# one (concurrent multi-crew validations, the motivating busy-CLI scenario) +# falls through to the possibly-stale log and false-surfaces. +test_retry_recovers_raced_coarse_runs_lookup() { + reset_fakes + local d out; d=$(new_case retry-recovers-coarse) + make_repo_on_branch "$d/wt" fm/feat-coarse + race_recovery_fakebin "$d" 0 1 + fm_write_meta "$d/state/feat-coarse.meta" "window=fm:fm-feat-coarse" "worktree=$d/wt" "kind=ship" + FM_FAKE_AXI_STATUS="$(run_running fm/other-crew)" + FM_FAKE_RUNS_LIST=" running fm/feat-coarse bbbbbbb 2026-07-02 22:05" + FM_FAKE_BUSY=0 + # With retries on, the second attempt's runs list answers and attributes this + # branch's own run: working / run-step (via the coarse fallback). + out=$(PATH="$d/fakebin:$PATH" FM_STATE_OVERRIDE="$d/state" \ + FM_CREW_STATE_RETRIES=2 FM_CREW_STATE_RETRY_DELAY=0 "$CREW_STATE" feat-coarse) + assert_contains "$out" "state: working" "retry recovered the raced coarse runs lookup (working)" + assert_contains "$out" "source: run-step" "coarse-recovered run -> run-step source" + + # With retries OFF, the same first-runs-call-empty race is NOT recovered, + # pinning the retry as the load-bearing fix on this path too. + race_recovery_fakebin "$d" 0 1 + out=$(PATH="$d/fakebin:$PATH" FM_STATE_OVERRIDE="$d/state" \ + FM_CREW_STATE_RETRIES=0 "$CREW_STATE" feat-coarse) + assert_not_contains "$out" "source: run-step" "without retry the raced coarse lookup is not recovered" + pass "bounded retry covers a coarse runs-list call that lost the race" +} + +# Retry-scope regression: only the empty/timed-out signature (from either +# bounded call) is retried. When the CLI answers with another branch's run AND +# the authoritative runs list answers with rows but none for this branch, that +# is a definitive "no run for this branch" - the steady state of every +# implementing ship crew - so the lookup must make exactly ONE `axi status` +# call, never burning retries and backoff sleeps on an answer that cannot +# change. The same call-counting fake then pins the counterfactual: an empty +# first attempt still retries and recovers. +test_authoritative_no_run_answer_not_retried() { + reset_fakes + local d out calls; d=$(new_case no-retry-authoritative) + make_repo_on_branch "$d/wt" fm/feat-norun + race_recovery_fakebin "$d" 0 + fm_write_meta "$d/state/feat-norun.meta" "window=fm:fm-feat-norun" "worktree=$d/wt" "kind=ship" + FM_FAKE_AXI_STATUS="$(run_running fm/other-crew)" + FM_FAKE_RUNS_LIST=" running fm/other-crew aaaaaaa 2026-07-02 22:10" + FM_FAKE_BUSY=0 + out=$(PATH="$d/fakebin:$PATH" FM_STATE_OVERRIDE="$d/state" \ + FM_CREW_STATE_RETRIES=2 FM_CREW_STATE_RETRY_DELAY=0 "$CREW_STATE" feat-norun) + assert_not_contains "$out" "source: run-step" "another branch's run is not attributed to this crew" + calls=$(cat "$d/calln" 2>/dev/null || echo 0) + [ "$calls" -eq 1 ] || fail "authoritative no-run answer must not be retried (got $calls axi status calls)" + + race_recovery_fakebin "$d" 1 + FM_FAKE_AXI_STATUS="$(run_running fm/feat-norun)" + out=$(PATH="$d/fakebin:$PATH" FM_STATE_OVERRIDE="$d/state" \ + FM_CREW_STATE_RETRIES=2 FM_CREW_STATE_RETRY_DELAY=0 "$CREW_STATE" feat-norun) + assert_contains "$out" "source: run-step" "empty/timed-out attempt is still retried and recovered" + calls=$(cat "$d/calln" 2>/dev/null || echo 0) + [ "$calls" -eq 2 ] || fail "empty attempt should retry once then recover (got $calls axi status calls)" + pass "authoritative no-run answer breaks without retrying; empty attempts still retry" +} + # Usage error (no id) is the one non-zero exit. test_usage_error() { reset_fakes @@ -804,6 +957,9 @@ test_torn_down_worktree test_missing_meta test_provably_working_via_runs_list_fallback test_not_provably_working_when_stopped +test_retry_recovers_raced_run_lookup +test_retry_recovers_raced_coarse_runs_lookup +test_authoritative_no_run_answer_not_retried test_usage_error echo "all fm-crew-state tests passed" diff --git a/tests/fm-spawn-worktree-symlink.test.sh b/tests/fm-spawn-worktree-symlink.test.sh new file mode 100755 index 00000000..70d16917 --- /dev/null +++ b/tests/fm-spawn-worktree-symlink.test.sh @@ -0,0 +1,105 @@ +#!/usr/bin/env bash +# Regression test for fm-spawn.sh's treehouse-worktree wait loop path comparison. +# +# The loop waits for the pane's cwd to move from the project to the freshly-got +# worktree. The backend reports the pane's REAL cwd (tmux's #{pane_current_path} +# resolves symlinks), but PROJ_ABS was built with `pwd` and can still carry a +# symlink component (macOS /tmp -> /private/tmp, /var -> /private/var; any +# symlinked parent). Comparing the raw strings then reads as "already moved" +# on the very first poll - while the pane is STILL at the project - so the loop +# adopts the project dir as the worktree and validate_spawn_worktree aborts the +# spawn with "did not yield an isolated worktree". The fix canonicalizes both +# sides before comparing. +# +# This drives the REAL wait loop with a fake tmux whose #{pane_current_path} +# returns the project's CANONICAL path on the first read (pane not yet moved, +# symlink-mismatched vs PROJ_ABS) and the worktree on subsequent reads. An +# explicit symlink layer guarantees PROJ_ABS keeps a symlink component on every +# platform, so `pwd` and `pwd -P` genuinely differ. +set -u + +# shellcheck source=tests/lib.sh +. "$(dirname "${BASH_SOURCE[0]}")/lib.sh" + +SPAWN="$ROOT/bin/fm-spawn.sh" +TMP_ROOT=$(fm_test_tmproot fm-spawn-symlink) +fm_git_identity fmtest fmtest@example.invalid + +# A fake tmux: session/window ops are no-ops; #{pane_current_path} is dynamic. +make_symlink_fakebin() { # + local dir=$1 fakebin + fakebin=$(fm_fakebin "$dir") + cat > "$fakebin/tmux" <<'SH' +#!/usr/bin/env bash +set -u +case "$*" in + *"#{pane_current_path}"*) + n=$(cat "$FM_FAKE_PANE_CALLS" 2>/dev/null || echo 0) + n=$((n + 1)); printf '%s' "$n" > "$FM_FAKE_PANE_CALLS" + # Read 1: pane is STILL at the project, reported in canonical form (the + # symlink-mismatch vs PROJ_ABS that triggers the bug). Read 2+: it has moved + # into the worktree. + if [ "$n" -le 1 ]; then printf '%s\n' "$FM_FAKE_PANE_PROJECT" + else printf '%s\n' "$FM_FAKE_PANE_WORKTREE"; fi + exit 0 ;; +esac +case "${1:-}" in + display-message) printf 'firstmate\n'; exit 0 ;; + list-windows|has-session|new-session|new-window|kill-window|send-keys) exit 0 ;; +esac +exit 0 +SH + chmod +x "$fakebin/tmux" + fm_fake_exit0 "$fakebin" treehouse + printf '%s\n' "$fakebin" +} + +test_symlinked_project_path_does_not_misdetect_worktree() { + local case_dir realbase symbase home proj wt fakebin id out status meta + case_dir="$TMP_ROOT/sym-case" + realbase="$case_dir/real" + symbase="$case_dir/link" + mkdir -p "$realbase" + # A real repo + worktree under realbase; reference them through a symlink so + # PROJ_ABS (built with `pwd`) keeps the symlink while the pane reports canonical. + proj="$realbase/project" + wt="$realbase/wt" + fm_git_worktree "$proj" "$wt" "wt-sym" + ln -s "$realbase" "$symbase" + + home="$symbase/home" + mkdir -p "$home/data" "$home/state" "$home/config" + printf 'claude\n' > "$home/config/crew-harness" + id=spawn-sym-s1 + mkdir -p "$home/data/$id" + printf 'brief for %s\n' "$id" > "$home/data/$id/brief.md" + + fakebin=$(make_symlink_fakebin "$case_dir/fake") + + out=$(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 TMUX="fake,1,0" \ + FM_FAKE_PANE_CALLS="$case_dir/pane.calls" \ + FM_FAKE_PANE_PROJECT="$(cd "$symbase/project" && pwd -P)" \ + FM_FAKE_PANE_WORKTREE="$(cd "$wt" && pwd -P)" \ + PATH="$fakebin:$PATH" \ + "$SPAWN" "$id" "$symbase/project" 2>&1) + status=$? + + expect_code 0 "$status" "spawn through a symlinked project path should not misdetect the worktree" + assert_not_contains "$out" "did not yield an isolated worktree" \ + "symlink path mismatch tripped validate_spawn_worktree (bug 4)" + assert_contains "$out" "spawned $id" "spawn did not report success" + meta="$home/state/$id.meta" + assert_present "$meta" "meta was not written" + # The recorded worktree must be the real worktree, never the project dir. + assert_grep "worktree=$(cd "$wt" && pwd -P)" "$meta" "meta worktree is not the real worktree" + assert_no_grep "worktree=$(cd "$symbase/project" && pwd -P)" "$meta" \ + "meta worktree was misdetected as the project dir" + pass "fm-spawn canonicalizes paths, so a symlinked project path does not misdetect the worktree" +} + +test_symlinked_project_path_does_not_misdetect_worktree + +echo "# all fm-spawn-worktree-symlink tests passed" diff --git a/tests/lib.sh b/tests/lib.sh index 6e425367..49cfb044 100644 --- a/tests/lib.sh +++ b/tests/lib.sh @@ -94,10 +94,14 @@ SH # --- deterministic git identity and fixtures -------------------------------- # fm_git_identity [name] [email]: export a fixed author/committer identity so -# fixture commits never depend on the host git config. +# fixture commits never depend on the host git config. Also neutralizes the +# host's core.excludesFile: a global ignore that matches fixture files (e.g. a +# ~/.gitignore_global ignoring `.gitignore` itself) would silently drop them +# from fixture commits and break check-ignore-based guards under test. fm_git_identity() { export GIT_AUTHOR_NAME=${1:-fmtest} GIT_AUTHOR_EMAIL=${2:-fmtest@example.invalid} export GIT_COMMITTER_NAME=$GIT_AUTHOR_NAME GIT_COMMITTER_EMAIL=$GIT_AUTHOR_EMAIL + export GIT_CONFIG_COUNT=1 GIT_CONFIG_KEY_0=core.excludesFile GIT_CONFIG_VALUE_0=/dev/null } # fm_git_init_commit : create a git repo at with a README and one