fix(spawn): make tmux window handling robust under non-default config#134
Open
Deeds67 wants to merge 2 commits into
Open
fix(spawn): make tmux window handling robust under non-default config#134Deeds67 wants to merge 2 commits into
Deeds67 wants to merge 2 commits into
Conversation
Two bugs surface under base-index 1 + automatic-rename on (reproduced on
macOS tmux):
1. Window-creation collision. `new-window -t "$SES"` (no trailing colon)
targets the session's active window index instead of appending; under
base-index 1 with a window already at index 1, tmux fails with
"create window failed: index 1 in use" and no crew window is created.
Target "$SES:" so tmux appends at the next free index.
2. Lost window name -> worktree misread. With automatic-rename on, once
`treehouse get` cd's the pane into the worktree, tmux renames the window
away from fm-<id>. The wait loop's `display-message -t "$SES:$W"` then
can't find it by name and falls back to the active client's window,
returning firstmate's own pane path (the primary checkout) as the
supposed worktree -- so the turn-end hook and recorded worktree land in
the primary. Capture the stable window id from `new-window -P -F
'#{window_id}'`, disable automatic-rename/allow-rename on it so the name
sticks, and target the wait-loop send-keys/display-message by that id.
Add a deterministic tangle-guard test (recording fake tmux) pinning the
append-form creation, name pinning, and id-based targeting.
… CONTRIBUTING coverage list
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Intent
The developer was working on the firstmate codebase as an ordinary software project, tasking a coding agent with applying a small robustness fix to bin/fm-spawn.sh's tmux window handling to address two empirically-reproduced bugs that surface under a non-default tmux config (base-index 1 and automatic-rename on): a window-creation collision (fixed by targeting the session with a trailing colon so tmux appends at the next free index) and a dangerous lost-window-name issue where a renamed window caused the wait-loop to misread firstmate's own primary checkout as the worktree (fixed by capturing the stable window id, disabling automatic-rename/allow-rename, and targeting by id). They required the change be confined to that one script plus an optional deterministic test, that bash -n and shellcheck pass, and that the behavior test suite (especially the spawn/tangle tests) pass, with the work shipping through the no-mistakes contribution pipeline. After the code was committed and verified locally, the pipeline was blocked by a no-mistakes gate transport error (invalid gate path: '.'); the developer fixed gate infrastructure on their end (restarting the daemon and clearing the stale ref), and when that did not resolve it, told the agent to stand by while they worked the deeper no-mistakes transport/path issue from the firstmate side, instructing it not to retry the gate or change anything.
What Changed
bin/fm-spawn.sh: create the task window by targeting the session with a trailing colon so tmux appends at the next free index instead of colliding whenbase-indexis non-zero, and capture the resulting stable window id fromnew-window -P.bin/fm-spawn.sh: disableautomatic-rename/allow-renameon the new window and target the worktree wait loop andtreehouse getsend-keys by the captured window id, so a window rename can no longer make the wait loop misread firstmate's own primary checkout as the worktree.test_spawn_tmux_window_constructiontotests/fm-tangle-guard.test.shcovering the window-construction behavior, and listed it in theCONTRIBUTING.mdcoverage list.Risk Assessment
✅ Low: A small, well-bounded robustness fix confined to one script's tmux handling plus a hermetic deterministic test; the only finding is a low-practical-risk consistency gap where the launch and persisted meta target still use name-based addressing the fix otherwise hardened against.
Testing
Baseline (the full tmux-gated e2e suite) already passed; on top of that I confirmed the change's added deterministic test passes and then demonstrated the intent end-to-end on a real tmux server configured with the exact bug-triggering options (base-index 1, automatic-rename on, allow-rename on). Running the real fm-spawn.sh there, it spawns successfully, resolves the correct isolated worktree, and the window retains its fm-<id> name despite the rename config; a side-by-side run against the actual pre-fix base script shows that code deterministically failing under the same conditions, and an instrumented copy proves the most dangerous mode — the old name-based query silently returning firstmate's own primary checkout as the worktree and slipping past the isolation guard. Bug 1's hard collision does not surface as an error on the installed tmux 3.6b for a bare session name (it does for an explicitly occupied index), but the colon-append form the fix adopts is the canonical version-independent append and is pinned by the new test — non-actionable. This is a shell/CLI/tmux change with no rendered UI surface, so evidence is CLI transcripts rather than screenshots. Overall: all tests pass and the user intent is demonstrated working end-to-end.Evidence: Focused real-tmux mechanism demo (annotated before/after for both bugs)
BUG 1 — old form 'new-window -t firstmate:1' => create window failed: index 1 in use; new form 'new-window -t firstmate:' => created @1 at next free index, and CAPTURED the id BUG 2 (name lost to allow-rename): >>> OLD code by NAME (firstmate:fm-job) -> /private/tmp/.../primary *** DANGER: firstmate's PRIMARY checkout, NOT the worktree *** >>> NEW code by id (@1) -> /private/tmp/.../worktree CORRECT: the real worktree BUG 2 fix (a) — pinned window after same rename trigger: name stayed 'fm-pinned'; firstmate:fm-pinned -> /private/tmp/.../worktreeEvidence: End-to-end: real fm-spawn.sh under bug config (PASS)
spawned e2e-fix-zz9 harness=codex kind=ship mode=no-mistakes yolo=off window=firstmate:fm-e2e-fix-zz9 worktree=/private/tmp/fm-e2e.../wt meta: window=firstmate:fm-e2e-fix-zz9 / worktree=/private/tmp/fm-e2e.../wt live windows: 2: fm-e2e-fix-zz9 (@1) cwd=/private/tmp/fm-e2e.../wt PASS: resolved the WORKTREE, not the primary checkout PASS: window kept its name 'fm-e2e-fix-zz9' (automatic-rename pinned off) === END-TO-END RESULT: PASS ===Evidence: Before/after vs pre-fix base script (base fails 3/3, fix correct 3/3)
BEFORE (base 81c94db): BASE#1 exit=1 aborted; BASE#2 exit=1 aborted; BASE#3 exit=1 aborted AFTER (target e00cc40): FIXED#1 CORRECT (real worktree); FIXED#2 CORRECT; FIXED#3 CORRECTEvidence: Instrumented base proves primary-checkout misread
DEBUG[base]: wait-loop resolved WT=/private/tmp/.../primary (PROJ_ABS=/private/tmp/.../proj) spawned ba-task-qq1 ... worktree=/private/tmp/.../primary <-- pre-fix records firstmate's OWN checkout as the worktree; isolation guard did not catch itPipeline
Updates from git push no-mistakes
✅ **intent** - passed
✅ No issues found.
✅ **Rebase** - passed
✅ No issues found.
bin/fm-spawn.sh:502- The fix hardens the worktree wait loop and thetreehouse getsend-keys to use the stable$WID, with a comment warning that a lost window name makesdisplay-message -t <bad-name>fall back to the active client's window. But the two highest-stakes operations still use the name-based$T($SES:$W): the agent launch at lines 502/504 (tmux send-keys -t "$T" -l "$LAUNCH"thenEnter) and the persisted supervision target at line 479 (window=$Tin meta, consumed by fm-peek/fm-send/fm-teardown/fm-watch/fm-ff-lib as a tmux target for the whole task lifetime). In the exact scenario the fix defends against — the name lost before the rename-disable settles (e.g. anallow-renameescape sequence in the create→disable gap) — the launch keys could be sent to the wrong/active pane (potentially firstmate's own session) and the recordedwindow=target would be broken for all later supervision. In practice the risk is low:new-window -nauto-disables automatic-rename for the window,allow-rename offis set before treehouse/agent run, so by launch time the name is well-pinned and the residual race is a microsecond window with a bare shell. Recommend using$WIDfor the launch send-keys and the persisted meta target for full consistency with the fix's own rationale; note this changes the documentedwindow=<session:window>meta/output format (line 30) to a@id, which is a valid tmux target everywhere but is the reason this warrants author sign-off rather than a blind change.✅ **Test** - passed
✅ No issues found.
command -v tmux >/dev/null || { echo "tmux is required for e2e tests" >&2; exit 1; }; tmux -V; rc=0; for t in tests/*.test.sh; do echo "== $t =="; bash "$t" || rc=1; done; exit "$rc"bash tests/fm-tangle-guard.test.sh— full file incl. the newtest_spawn_tmux_window_construction; all 6 cases okFull behavior suite spot-checked: ran eachtests/*.test.sh; every file that completed reported its finalok/all ... passedline (matches the green baseline run)Real-tmux mechanism demo on an isolated server withbase-index 1; automatic-rename on; allow-rename on: old formnew-window -t firstmate:1collides (index 1 in use); new formnew-window -dP -F '#{window_id}' -t 'firstmate:'appends and captures the idReal-tmux Bug-2 reproduction: after a worktree subshell emits a window-rename escape,display-message -p -t firstmate:fm-jobsilently returns firstmate's PRIMARY checkout, whiledisplay-message -p -t @<id>returns the real worktree;set-window-option automatic-rename/allow-rename offkeeps the name pinnedEnd-to-end with the REALbin/fm-spawn.shunder the bug config (faketreehouse getcd'ing into a genuine git worktree, fakecodex): exit 0,worktree=resolves to the worktree, meta correct, window keeps namefm-<id>(e2e-spawn-nondefault-tmux.sh)Before/after: basefm-spawn.shfrom81c94db(staged with current libs) vs the fixed worktree script, 3 runs each — base fails 3/3, fixed resolves the real worktree 3/3 (e2e-before-after.sh)Instrumented analysis copy of the base script: proves the pre-fix wait-loop resolvesWT=<primary checkout>and recordsworktree=<primary>, with the isolation guard not catching it✅ **Document** - passed
✅ No issues found.
✅ **Lint** - passed
✅ No issues found.
✅ **Push** - passed
✅ No issues found.