Fix wait_for_text's blind spot and bound every wait - #100
Conversation
Code reviewFound 6 issues (1 confirmed by repro):
libtmux-mcp/src/libtmux_mcp/tools/pane_tools/wait.py Lines 238 to 252 in 6bd82ce
libtmux-mcp/src/libtmux_mcp/prompts/recipes.py Lines 166 to 171 in 6bd82ce
libtmux-mcp/src/libtmux_mcp/prompts/recipes.py Lines 138 to 143 in 6bd82ce
libtmux-mcp/src/libtmux_mcp/tools/pane_tools/wait.py Lines 517 to 520 in 6bd82ce
libtmux-mcp/src/libtmux_mcp/tools/pane_tools/wait.py Lines 588 to 592 in 6bd82ce
libtmux-mcp/src/libtmux_mcp/tools/pane_tools/wait.py Lines 712 to 716 in 6bd82ce 🤖 Generated with Claude Code - If this code review was useful, please react with 👍. Otherwise, react with 👎. |
6bd82ce to
2993411
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #100 +/- ##
==========================================
+ Coverage 85.58% 86.13% +0.55%
==========================================
Files 44 46 +2
Lines 3379 3593 +214
Branches 477 516 +39
==========================================
+ Hits 2892 3095 +203
Misses 354 354
- Partials 133 144 +11 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Code reviewFound 3 issues (re-checked against the current, rebased branch):
libtmux-mcp/src/libtmux_mcp/tools/pane_tools/wait.py Lines 238 to 254 in 2993411
libtmux-mcp/src/libtmux_mcp/tools/pane_tools/wait.py Lines 589 to 594 in 2993411
libtmux-mcp/src/libtmux_mcp/tools/pane_tools/wait.py Lines 517 to 520 in 2993411 Generated with Claude Code |
Code reviewFound 5 issues:
libtmux-mcp/src/libtmux_mcp/tools/pane_tools/__init__.py Lines 93 to 97 in 3a95d89
libtmux-mcp/src/libtmux_mcp/prompts/recipes.py Lines 167 to 172 in 3a95d89
Lines 8 to 24 in 3a95d89
libtmux-mcp/src/libtmux_mcp/tools/wait_for_tools.py Lines 173 to 182 in 3a95d89
Lines 76 to 78 in 3a95d89 Generated with Claude Code |
why: run_command now clamps its timeout to the shared wait ceiling, exactly like wait_for_text and wait_for_channel, but it was still registered with TAG_MUTATING alone. The batch loop is serial with no aggregate deadline and MAX_BATCH_OPERATIONS is 1000, so a batched run_command multiplied that ceiling by the operation count — the same cap amplifier the tag exists to close for the wait tools. Raised in review of PR #100. what: - register run_command with TAG_SELF_BOUNDED alongside TAG_MUTATING, with the rationale comment mirroring wait_for_text's - assert the real registration carries the tag and that _get_allowed_tool_tier rejects it at every wrapper's max_tier - update the history-inheritance test: the generic batch wrapper now rejects a nested run_command instead of inheriting the suppression default, and the schema audit expects the added tag - CHANGES: note run-command can no longer be nested in a batch wrapper, and point at send-keys-batch for command sequences
why: wait_for_channel's comment claimed to mirror wait_for_text's pattern. That stopped being true earlier in PR #100, when the wait_for_text path moved to native asyncio.create_subprocess_exec because a thread blocked in libtmux's untimed Popen.communicate() cannot be cancelled and hangs interpreter shutdown. wait_for_channel still runs subprocess.run on a worker thread, so the comment pointed a reader at code that no longer looks like this one. what: - rewrite the comment to state that this path runs subprocess.run on a worker thread and that wait_for_text no longer does, and why - no behavior change
why: PR #100 removes `risk_band_warned` from WaitForTextResult, but the unreleased entry documented only the `patterns` rename and the `wait_for_content_change` removal. The field existed so clients that cannot surface MCP warning notifications could still see that matching was best-effort; losing it silently is a buried breaking change. what: - Add a `### Breaking changes` subheading for the dropped `risk_band_warned` field on WaitForTextResult - State that the trim-risk signal remains a warning notification only, with no result-field fallback for clients that cannot surface it - Show a `# Before` / `# After` migration using the shipped `outcome` and `matched_at_entry` fields, and point deterministic waits at wait-for-channel
why: The wait-ceiling entry added in PR #100 cited `RunCommandResult.effective_timeout` with the `{class}` role. It is a pydantic field, so the reference did not resolve to the attribute's autodoc anchor and rendered without a link. what: - Switch the `effective_timeout` reference to `{attr}`, matching the `SessionInfo.active_pane_id` precedent elsewhere in the file
why: The interrupt_gracefully recipe emitted stop=["^C", "Interrupt"]
on a wait_for_text call that also passes regex=True, so the marker
compiled to a start-anchored "C". It never matched the literal ^C echo
a shell prints on SIGINT — the entire point of the marker — and it
false-fired outcome="stopped" on any line starting with C
("Compiling", "Cloning"). Review finding on PR #100.
what:
- Escape the caret in the emitted recipe text so agents are told to
pass stop=["\^C", "Interrupt"], matching how the sibling patterns
argument on the preceding line already escapes its glyphs
- Keep regex=True: patterns on the same call genuinely needs regex
- Add tests/test_prompts.py regression guard that extracts the
emitted stop marker, compiles it, and asserts it matches a literal
^C while not matching "Compiling"
Code reviewFresh pass over the six commits since the last review ( Found 2 issues:
Lines 45 to 62 in dd33e4d
libtmux-mcp/src/libtmux_mcp/tools/pane_tools/wait.py Lines 237 to 254 in dd33e4d Below the reporting threshold but still open, both in Generated with Claude Code |
why: run_command now clamps its timeout to the shared wait ceiling, exactly like wait_for_text and wait_for_channel, but it was still registered with TAG_MUTATING alone. The batch loop is serial with no aggregate deadline and MAX_BATCH_OPERATIONS is 1000, so a batched run_command multiplied that ceiling by the operation count — the same cap amplifier the tag exists to close for the wait tools. Raised in review of PR #100. what: - register run_command with TAG_SELF_BOUNDED alongside TAG_MUTATING, with the rationale comment mirroring wait_for_text's - assert the real registration carries the tag and that _get_allowed_tool_tier rejects it at every wrapper's max_tier - update the history-inheritance test: the generic batch wrapper now rejects a nested run_command instead of inheriting the suppression default, and the schema audit expects the added tag - CHANGES: note run-command can no longer be nested in a batch wrapper, and point at send-keys-batch for command sequences
why: wait_for_channel's comment claimed to mirror wait_for_text's pattern. That stopped being true earlier in PR #100, when the wait_for_text path moved to native asyncio.create_subprocess_exec because a thread blocked in libtmux's untimed Popen.communicate() cannot be cancelled and hangs interpreter shutdown. wait_for_channel still runs subprocess.run on a worker thread, so the comment pointed a reader at code that no longer looks like this one. what: - rewrite the comment to state that this path runs subprocess.run on a worker thread and that wait_for_text no longer does, and why - no behavior change
why: The interrupt_gracefully recipe emitted stop=["^C", "Interrupt"]
on a wait_for_text call that also passes regex=True, so the marker
compiled to a start-anchored "C". It never matched the literal ^C echo
a shell prints on SIGINT — the entire point of the marker — and it
false-fired outcome="stopped" on any line starting with C
("Compiling", "Cloning"). Review finding on PR #100.
what:
- Escape the caret in the emitted recipe text so agents are told to
pass stop=["\^C", "Interrupt"], matching how the sibling patterns
argument on the preceding line already escapes its glyphs
- Keep regex=True: patterns on the same call genuinely needs regex
- Add tests/test_prompts.py regression guard that extracts the
emitted stop marker, compiles it, and asserts it matches a literal
^C while not matching "Compiling"
dd33e4d to
3e2c6e8
Compare
why: run_command now clamps its timeout to the shared wait ceiling, exactly like wait_for_text and wait_for_channel, but it was still registered with TAG_MUTATING alone. The batch loop is serial with no aggregate deadline and MAX_BATCH_OPERATIONS is 1000, so a batched run_command multiplied that ceiling by the operation count — the same cap amplifier the tag exists to close for the wait tools. Raised in review of PR #100. what: - register run_command with TAG_SELF_BOUNDED alongside TAG_MUTATING, with the rationale comment mirroring wait_for_text's - assert the real registration carries the tag and that _get_allowed_tool_tier rejects it at every wrapper's max_tier - update the history-inheritance test: the generic batch wrapper now rejects a nested run_command instead of inheriting the suppression default, and the schema audit expects the added tag - CHANGES: note run-command can no longer be nested in a batch wrapper, and point at send-keys-batch for command sequences
why: wait_for_channel's comment claimed to mirror wait_for_text's pattern. That stopped being true earlier in PR #100, when the wait_for_text path moved to native asyncio.create_subprocess_exec because a thread blocked in libtmux's untimed Popen.communicate() cannot be cancelled and hangs interpreter shutdown. wait_for_channel still runs subprocess.run on a worker thread, so the comment pointed a reader at code that no longer looks like this one. what: - rewrite the comment to state that this path runs subprocess.run on a worker thread and that wait_for_text no longer does, and why - no behavior change
why: The interrupt_gracefully recipe emitted stop=["^C", "Interrupt"]
on a wait_for_text call that also passes regex=True, so the marker
compiled to a start-anchored "C". It never matched the literal ^C echo
a shell prints on SIGINT — the entire point of the marker — and it
false-fired outcome="stopped" on any line starting with C
("Compiling", "Cloning"). Review finding on PR #100.
what:
- Escape the caret in the emitted recipe text so agents are told to
pass stop=["\^C", "Interrupt"], matching how the sibling patterns
argument on the preceding line already escapes its glyphs
- Keep regex=True: patterns on the same call genuinely needs regex
- Add tests/test_prompts.py regression guard that extracts the
emitted stop marker, compiles it, and asserts it matches a literal
^C while not matching "Compiling"
46b40de to
365b65d
Compare
why: run_command now clamps its timeout to the shared wait ceiling, exactly like wait_for_text and wait_for_channel, but it was still registered with TAG_MUTATING alone. The batch loop is serial with no aggregate deadline and MAX_BATCH_OPERATIONS is 1000, so a batched run_command multiplied that ceiling by the operation count — the same cap amplifier the tag exists to close for the wait tools. Raised in review of PR #100. what: - register run_command with TAG_SELF_BOUNDED alongside TAG_MUTATING, with the rationale comment mirroring wait_for_text's - assert the real registration carries the tag and that _get_allowed_tool_tier rejects it at every wrapper's max_tier - update the history-inheritance test: the generic batch wrapper now rejects a nested run_command instead of inheriting the suppression default, and the schema audit expects the added tag - CHANGES: note run-command can no longer be nested in a batch wrapper, and point at send-keys-batch for command sequences
why: wait_for_channel's comment claimed to mirror wait_for_text's pattern. That stopped being true earlier in PR #100, when the wait_for_text path moved to native asyncio.create_subprocess_exec because a thread blocked in libtmux's untimed Popen.communicate() cannot be cancelled and hangs interpreter shutdown. wait_for_channel still runs subprocess.run on a worker thread, so the comment pointed a reader at code that no longer looks like this one. what: - rewrite the comment to state that this path runs subprocess.run on a worker thread and that wait_for_text no longer does, and why - no behavior change
why: The interrupt_gracefully recipe emitted stop=["^C", "Interrupt"]
on a wait_for_text call that also passes regex=True, so the marker
compiled to a start-anchored "C". It never matched the literal ^C echo
a shell prints on SIGINT — the entire point of the marker — and it
false-fired outcome="stopped" on any line starting with C
("Compiling", "Cloning"). Review finding on PR #100.
what:
- Escape the caret in the emitted recipe text so agents are told to
pass stop=["\^C", "Interrupt"], matching how the sibling patterns
argument on the preceding line already escapes its glyphs
- Keep regex=True: patterns on the same call genuinely needs regex
- Add tests/test_prompts.py regression guard that extracts the
emitted stop marker, compiles it, and asserts it matches a literal
^C while not matching "Compiling"
365b65d to
12622d4
Compare
why: run_command now clamps its timeout to the shared wait ceiling, exactly like wait_for_text and wait_for_channel, but it was still registered with TAG_MUTATING alone. The batch loop is serial with no aggregate deadline and MAX_BATCH_OPERATIONS is 1000, so a batched run_command multiplied that ceiling by the operation count — the same cap amplifier the tag exists to close for the wait tools. Raised in review of PR #100. what: - register run_command with TAG_SELF_BOUNDED alongside TAG_MUTATING, with the rationale comment mirroring wait_for_text's - assert the real registration carries the tag and that _get_allowed_tool_tier rejects it at every wrapper's max_tier - update the history-inheritance test: the generic batch wrapper now rejects a nested run_command instead of inheriting the suppression default, and the schema audit expects the added tag - CHANGES: note run-command can no longer be nested in a batch wrapper, and point at send-keys-batch for command sequences
12622d4 to
59d9ed5
Compare
why: wait_for_channel's comment claimed to mirror wait_for_text's pattern. That stopped being true earlier in PR #100, when the wait_for_text path moved to native asyncio.create_subprocess_exec because a thread blocked in libtmux's untimed Popen.communicate() cannot be cancelled and hangs interpreter shutdown. wait_for_channel still runs subprocess.run on a worker thread, so the comment pointed a reader at code that no longer looks like this one. what: - rewrite the comment to state that this path runs subprocess.run on a worker thread and that wait_for_text no longer does, and why - no behavior change
why: The interrupt_gracefully recipe emitted stop=["^C", "Interrupt"]
on a wait_for_text call that also passes regex=True, so the marker
compiled to a start-anchored "C". It never matched the literal ^C echo
a shell prints on SIGINT — the entire point of the marker — and it
false-fired outcome="stopped" on any line starting with C
("Compiling", "Cloning"). Review finding on PR #100.
what:
- Escape the caret in the emitted recipe text so agents are told to
pass stop=["\^C", "Interrupt"], matching how the sibling patterns
argument on the preceding line already escapes its glyphs
- Keep regex=True: patterns on the same call genuinely needs regex
- Add tests/test_prompts.py regression guard that extracts the
emitted stop marker, compiles it, and asserts it matches a literal
^C while not matching "Compiling"
768c282 to
51ba205
Compare
why: wait_for_text accepts an unbounded caller timeout and nothing in the server clamps it, so one wrong search pattern can hold an MCP connection for minutes and stall an agent session. what: - Add _wait_policy with LIBTMUX_MCP_WAIT_MAX_SECONDS (default 30 s) - Hard-clamp the resolved ceiling to [1.0, 120.0] so neither a hostile env value nor a programmatic caller can restore unbounded waits - Resolve in server.py beside the other env knobs and publish via _configure_wait_ceiling, mirroring _configure_history_defaults so tool modules never import server globals - Warn and fall back on unparseable values instead of raising - Cover resolution, clamping, and the inf/nan/garbage cases
why: wait_for_channel ran tmux through asyncio.to_thread(subprocess.run, timeout=...), which is uninterruptible. On cancellation the coroutine raised CancelledError at once while the worker thread stayed blocked in waitpid, so `tmux wait-for` kept running for the whole remainder of its budget with nobody waiting on it. Measured in-process: a 15 s wait cancelled at 2 s left the child alive another 13.07 s. Measured through the real MCP stdio server with a notifications/cancelled (what an agent TUI's Esc sends): 22.05 s of orphan behind a 25 s wait. wait_for_text was already fixed for exactly this; this path was left behind. what: - Add libtmux_mcp._tmux_proc with _kill_and_reap and _run_tmux_bounded, lifted verbatim out of pane_tools.wait._run_tmux_lines so both wait paths share one killable async subprocess - Point _run_tmux_lines at the shared helper; it now formats its own timeout and non-zero-exit messages around a TimeoutError - Move wait_for_channel off to_thread onto _run_tmux_bounded, keeping the effective_timeout clamp, both ExpectedToolError messages, and the confirmation string byte-identical - Add test_wait_for_channel_kills_tmux_child_on_cancel, which reads the live child from ps rather than trusting the tool; it fails with an orphan pid without the fix - Record the reap in CHANGES note: tmux keeps its server-side waiter registered even after the client dies, so a latch-based probe cannot see the difference — verified against raw `tmux wait-for`. The live process is the only observable, and it is what the test asserts.
why: Swapping a CLI layer that was already swapped and not yet reverted backed up `original_bytes` again — but those bytes are the script's own earlier output, not the user's config. Within one second the derived backup name collided (the timestamp is second-granular) and the write clobbered the pristine backup; across seconds a second backup was written and state repointed at it, orphaning the pristine one. Either way `revert` restored an already-swapped config, and `doctor` then told the user the orphan holding the last pristine copy was safe to delete. what: - Reuse the recorded backup on a re-swap instead of taking a new one, keeping its `seq_no` and `swapped_at` so the LIFO unwind order (fixed by what each backup captured) survives; warn when the recorded backup is gone and the new one can only capture the swapped config. - Add `write_new_backup`, which claims the path with `O_CREAT | O_EXCL` and falls back to `-1` / `-2` suffixes, so a backup file is never overwritten. - Reword doctor's orphan advice: an untracked backup can be the only surviving pre-swap copy, so it is not "safe to delete". - Cover swap -> swap -> revert (same second and different second), the Claude two-scope re-swap unwind, and the two smaller behaviours.
why: pipe-pane hands its argument to tmux's format expander before
/bin/sh sees it, and only the shell layer was escaped. An output_path
containing #{...} was substituted — as was a legacy alias, so a log
named "#Session.log" lost its #S to the session name and landed on
"<session>ession.log". Either way the file written differed from the
path the tool reported back: a silent wrong-file write.
Doubling every # is the obvious escape and is itself wrong: tmux copies
a #-run followed by [ through verbatim and never collapses ##[, so
blanket doubling corrupts those paths the same way.
what:
- Add _escape_tmux_format, escaping per #-run: leave a run alone when [
follows it, double it otherwise. Verified against tmux 3.7b across 19
literals (formats, command jobs, legacy aliases, style sequences,
already-doubled runs, bare and trailing #) — all round-trip exactly
- Apply it on top of shlex.quote in pipe_pane
- Add test_pipe_pane_writes_the_exact_path_requested, parametrized over
format substitution, command job, legacy alias, style sequence,
already-doubled and bare-hash paths. It asserts the on-disk filename,
that no sibling file appears, and that the success string names the
file actually written. Four cases fail without the fix; the style and
bare-hash cases pass both ways and guard against over-escaping
note: scope is path redirection, not RCE — pipe-pane substitutes #()
away rather than running it, verified on 3.7b.
why: run_command blocked on `tmux wait-for` through asyncio.to_thread(subprocess.run, ...), which cannot be interrupted: cancelling the call raised CancelledError at once while the worker thread stayed in an untimed waitpid, so the tmux child ran on for the rest of its budget. Measured at stock settings: a 25 s run_command cancelled at 3 s left the child alive another 22.02 s. This is the most-cancelled of the blocking waits — agents routinely bail out of a long shell command. what: - Run run_command's wait through _run_tmux_bounded, the killable async child wait_for_channel and wait_for_text already use. - Keep the timeout clamp, effective_timeout, exit_status/timed_out semantics, capture and truncation untouched; the wait-for failure message keeps its prefix and stderr detail, falling back to `exit <rc>` like wait_for_channel's. - Regression test asserts via /proc (argv vector plus exe, own pid skipped) that no `tmux -L <socket> wait-for r_*` child survives the cancel; it reports the orphan pid without the fix. - Note at signal_channel why it stays thread-based: `wait-for -S` is edge-triggered and returns in milliseconds, so its 5 s bound is ours rather than the caller's.
why: the cancelled-wait entry cited 61 seconds of orphan behind a 90-second wait. The measurement is real but was taken with LIBTMUX_MCP_WAIT_MAX_SECONDS=120; the default ceiling is 30 s, so a 90 s wait is unreachable as shipped and the number reads as impossible. The entry also named only wait_for_channel, while run_command carried the same defect. what: - Quote the two default-reachable measurements instead: 13 s behind a 15 s wait_for_channel, 22 s behind a 25 s run_command. - Retitle the entry to cover run-command. - Name the raised ceiling where the 90 s TUI figure is cited in the wait_for_channel test docstring.
why: the skill carried four claims that measurement contradicts, and a cleanup step that destroys user state. The cancellation guidance was wrong in both directions: the original text said server-side cancellation was reachable only at Layer 2, and the first correction over-swung to "just cancel the asyncio task" — a wire capture shows that sends no cancellation frame at all, so it measures the client giving up rather than the server's reap path. what: - Cancellation: document that a bare task.cancel() puts nothing on the wire, and that client.cancel(request_id) is what emits a real notifications/cancelled. Warn that the id must be read off the capture, not counted — a wrong id is accepted silently and the call runs to completion (measured: 0.1 s reap on the right id, 17 s on a wrong one, same script) - Cleanup: revert works off the swap state file, not the newest backup on disk; make the checklist conditional so it cannot tear down a pre-existing deliberate swap, and drop the un-caveated recipe line - CLI matrix: claude reaches a real tool call (was "credit blocked"); gemini now hard-fails with IneligibleTierError; agy is verified, and its --gemini_dir does not carry credentials because auth lives in the OS keyring - Promote copy-never-symlink for credentials: a symlinked token is refreshed straight through into the user's real credential file
why: the format escape landed earlier was half a fix. cmd-pipe-pane.c calls format_expand_time(), not format_expand — so tmux runs the argument through strftime as well as the #-format expander. A path containing % was still rewritten silently: '100%done.log' was created as '10025one.log' (%d -> day of month) and 'date-%Y.log' as 'date-2026.log', while the tool echoed back the requested path. Same wrong-file class as the # bug, found by reading the tmux C source rather than by probing # cases. what: - Double every % in _escape_tmux_format; %% is strftime's literal escape. Verified against tmux 3.7b: '100%done.log', 'date-%Y.log' and 'a%%b.log' all round-trip exactly - Extend test_pipe_pane_writes_the_exact_path_requested with strftime-percent-d and strftime-percent-y cases; six of the eight cases now fail without the fix - Record the two-expansion detail in the docstring and CHANGES
`tmux wait-for` exits 0 when the server shuts down without ever
signalling the channel. Measured on tmux 3.7b, that is byte-identical
to a real signal:
how the wait ended rc stderr
genuinely signalled 0 (empty)
kill-server 0 (empty)
server SIGTERM 0 (empty)
server SIGKILL 1 server exited unexpectedly
Only the SIGKILL path was already caught, so a wait on work that never
happened returned "Channel 'x' was signalled" and the agent proceeded
on a false premise. Neither the exit code nor stderr can tell the two
clean paths apart, so re-probe the server before claiming success.
`list-sessions` is the probe libtmux's own `Server.is_alive` uses, and
it is safe here specifically because it does not auto-start a server —
probing cannot resurrect the thing it asks about.
There is a narrow race: a script that signals and immediately tears the
server down now reports an error for a wait that succeeded. That error
names a true fact about the server, which beats telling the agent a
channel was signalled when nothing signalled it.
The poll loop captured from `baseline_abs - history_size + 1`, one row
BELOW where the cursor sat at entry. On a quiescent pane the cursor is
at the end of the prompt, so that is exactly where the next line of
output lands — making the case the tool is reserved for, output you did
not author, structurally unmatchable.
Measured before the fix, writing straight to the pane tty:
READY_MARK\n MISSED at 5.02s, marker on screen
\nREADY_MARK\n matched at 1.017s
READY_MARK\ntrailing\n MISSED at 5.01s, saw_new_output=true
marker inside a 20-line burst matched at 1.015s
The wait then reported `saw_new_output: false`, documented as "the pane
was silent — often the command never ran", which is backwards, and the
agent retried with a different pattern against a structural failure.
Anchor on the entry row instead and let the existing `entry_below_cursor`
snapshot suppress that row's PRE-EXISTING content by value — the same
mechanism already used for every row below it. Suppression by content
rather than by index has no blind spot: the prompt text captured at
entry stays filtered while text appended to the row afterwards matches.
Checked against the obvious way this could go wrong. A pattern broad
enough to hit the prompt (`\S`, and the prompt's own text) still does
not self-match, because the snapshot holds it. The one behaviour change
is that in-place carriage-return rewrites on the entry row now match,
which retires the docstring caveat that called them invisible.
A stale match still does not end the wait. Returning early on
`matched_at_entry` looks like free latency — it is known at ~14ms while
the call runs to the ceiling — but it would break the commonest agent
loop: run, wait for marker, re-run, wait for the SAME marker. Measured,
the wait correctly matches the fresh occurrence at 1.021s with the old
one on screen. Pinned by a test so it is not "optimised" later.
The regression tests park the pane on an `sh` prompt rather than using
the default zsh fixture. Against zsh they passed on the unfixed code:
two identical `(hsize, cursor_y)` polls are satisfied while zsh is still
starting, and its `compinit` warning then moves the cursor two rows down
so the marker misses the entry row entirely. Four of the six cases fail
without the fix; the two controls pass either way.
… topic
`_wait_policy` justified the ceiling with "stalls the MCP connection …
shared with every other tool call". That is false, and it is the false
premise that makes background tasks look like a fix. The wait tools
await throughout and FastMCP serves the stdio connection concurrently.
Measured on fastmcp 3.4.4:
tool under test interleaved median
awaits 6s (what the wait tools do) 58 3.4ms
blocks the event loop 6s (control) 1 6014ms
The control is what makes the first row evidence rather than an
assumption. The ceiling stays exactly as it is — it bounds the agent's
TURN, which MCP gives it no way to abandon mid-call — but the reason is
now the true one.
Result descriptions were carrying the entry-row bug's assumptions.
`saw_new_output` claimed a quiet pane usually means the command never
ran; it now says what the true-with-found-false case means too, which
is the shape that actually sends an agent into a retry loop.
`matched_at_entry` now explains that the wait keeps waiting for a fresh
occurrence rather than answering with the stale one.
The new topic collects which wait to reach for, how the "is this new"
predicate works, and — deliberately — the designs measured and
rejected: MCP background tasks, event-driven `pipe-pane`, and removing
the tool. Recording why they lose is what stops them being re-proposed
from intuition.
…iles
The server-death tests mint a socket per PID per case, and each run left
its socket file behind in `$TMUX_TMPDIR`. `kill-server` alone cannot fix
that: a server killed with SIGKILL never gets to remove its own socket.
The unlink has to read the path from tmux itself. `Server.socket_path`
is None on a server constructed from a `socket_name`, so the obvious
`Path(server.socket_path).unlink()` is a silent no-op — it was, until
the leftover files gave it away. Ask tmux for `#{socket_path}` while the
server is still alive to answer, and unlink that.
Folds the setup and teardown into one `_throwaway_server` context
manager so both tests share it.
…kens The corrected `saw_new_output` and `matched_at_entry` descriptions cost 93 schema tokens more than the wrong ones they replaced (measured o200k: wait_for_text 1630 -> 1723). Worth paying — the wrong `saw_new_output` text was measured driving a 3-call / 90 s / 360-token retry loop that returned nothing — but not at that price. Same two actionable branches, tighter wording: 1723 -> 1660, so accuracy now costs +30 rather than +93. Measuring this also retires the idea that `wait_for_text` has a fat docstring to trim. Its schema splits desc 312 / inputSchema 505 / outputSchema 843, so the description is 19% of the tool's cost and the result-field descriptions are half of it. There is no 1519-token docstring; cutting prose here would trade the field semantics an agent actually branches on for a rounding error.
The original complaint was that picking the wrong search text lags a
session for minutes. Re-measured now that the entry-cursor-row false
negative is fixed, it costs ONE bounded call and the result says which
of three things went wrong:
daemon printed something else saw_new_output=true, and `tail`
holds "Server listening on 8080"
when you asked for "Ready to accept"
command never ran saw_new_output=false, prompt-only tail
pane scrolling, no match saw_new_output=true, 20-line tail
The control in the same harness matched at 1.508s, so the timeouts are
the tool disagreeing with the pattern rather than the harness
manufacturing failures. The "minutes" version of this needed the
entry-row bug to keep returning zero information.
That also settles the deferred-handle design, which is now recorded as
rejected alongside tasks and pipe-pane. It would have bought SEP-2663's
"server decides to stop blocking" semantics with no client capability at
all, by reusing capture_since's opaque cursor — genuinely attractive —
but it rescues a case that already self-diagnoses in one call, and a
soft deadline would ADD calls to the good case: 5s soft-defer turns one
legitimate 45s wait into ~9 calls instead of 2.
`wait.py` carries its own async pane resolver because the canonical `_utils._resolve_pane` is synchronous and reaches tmux through an untimed `Popen.communicate()` — unusable from the event loop, and uncancellable through a thread. The two cannot share an implementation: different sync-ness, and one returns a `Pane` while the other returns a pane id. Relocating one next to the other was considered and rejected; it would move 130 lines without deduplicating anything. What they can share is the CONTRACT — which targeting argument wins, which pane you get when several could match, and which exception a miss raises — and nothing pinned it, so a change to the canonical precedence would have diverged the wait tools silently. Nine tests: five precedence arms (pane_id, window_id, session_id, session_name, no target) resolved against a session with two windows and a split pane, so "first listed" is a real choice rather than the only option, plus four miss arms asserting both raise the same type. Verified the tests can fail: mutating `_first_pane_of_window` to return the LAST pane fails exactly the four arms that route through it and leaves `by_pane_id` passing, which short-circuits before it.
Every tick sent the same constant string, `Polling pane %0 for pattern`, alongside the numeric progress/total pair. That wastes the one field a client is most likely to actually show a human: clients that surface progress at all tend to render the message, not the raw pair, and the only thing worth knowing while watching a long wait is how much budget is left. Now: `Waiting on pane %0: 1.4s elapsed, 0.6s left`. Measured over a real MCP session, a 2s wait emits 36 callbacks running from "0.0s elapsed, 2.0s left" to "2.0s elapsed, 0.0s left", all distinct. This is also the cheapest available hedge. Trialling MCP background tasks (fastmcp-tasks 4.0.0a2, SEP-2663) showed the numeric pair is dropped entirely under task execution — `make_task_context()` builds a `Context(session=None)`, so `report_progress` finds no progress token, and `GetTaskResult` has no numeric field at all. The status MESSAGE is the only thing that survives. Putting the numbers in the string costs one line and is correct either way. The test asserted the constant string, which is what a message-that-never- changes looks like when it is pinned. It now asserts the contract worth having: the pane id, both budget terms, and that the message actually changes across the wait.
The entry point wrapped the whole server import tree in a bare
`except ImportError` and printed "libtmux-mcp requires fastmcp" for
everything it caught — which is close to the one cause it cannot be,
since fastmcp is a hard dependency and the package cannot install
without it. `raise ... from None` then discarded the real exception.
Found by pointing an MCP client at the `tasks` branch, where mcp 2.0.0b2
has deleted `mcp.types`. The server exited 1 blaming fastmcp while
fastmcp was installed and importable, and the client reported only
"Connection closed". That one stderr line is the entire diagnosis
available to whoever has to fix it, so it now reads:
libtmux-mcp failed to start: cannot import 'mcp' (No module named
'mcp.types'). The installed dependency set is incomplete or
incompatible — repair it with: pip install --force-reinstall
libtmux-mcp
One message rather than a per-dependency branch: `ImportError.name`
already names the culprit, and the remedy is the same either way.
Three tests over `ImportError.name` — a submodule miss, fastmcp itself,
and an unnamed `cannot import name` — verified to fail against the old
handler.
`_client_label` walked `client_params.clientInfo`. The MCP SDK renamed that Python attribute to `client_info` between `mcp` 1.x and 2.x while keeping the camelCase WIRE alias, so what moved is the attribute, not the protocol. That interacts badly with this function's design: a miss is swallowed on purpose, because every hop is legitimately absent in unit-test contexts and background tasks. So the rename does not raise — it silently downgrades the unknown-argument suggestion from "your client (gemini-test 9.9)" to generic wording, with nothing logged. Measured against mcp 2.0.0b2 while debugging why the `tasks` branch would not start: two middleware tests failed with no other symptom. Reading both names costs one `getattr` and survives the migration in either direction. Three tests over a stub params object: camelCase only, snake_case only, and neither — the snake_case arm fails against the old accessor and the other two pass either way.
The production-schema test dumped `tool.annotations` without `by_alias=True` and then asserted camelCase keys. That asserts the SDK's PYTHON attribute spelling, not the protocol: `model_dump()` yields camelCase on mcp 1.x and snake_case on 2.x, so the test broke on an SDK upgrade while the bytes a client receives were unchanged — verified by reading `run_command`'s annotations over a real stdio session on both SDKs, identical either way: destructiveHint, idempotentHint, openWorldHint, readOnlyHint, title. `by_alias=True` asserts the names that actually go on the wire, and is the idiom `tools/batch_tools.py:68` already uses. Passes on mcp 1.28.1 and mcp 2.0.0b2 unchanged.
Eleven assertions read `tool.annotations.readOnlyHint` and friends directly. That pins the SDK's Python attribute spelling, which is not stable: `mcp` 2.x renamed every hint to snake_case while keeping the camelCase wire alias, so the protocol output is unchanged and only the Python API moved. The failure mode is quiet. fastmcp 4.x ships a deprecation shim, so on mcp 2.0.0b2 these lines still resolve at runtime — the suite passes 790 green while emitting `FastMCPDeprecationWarning` and `mypy` reports 11 `has no attribute "readOnlyHint"` errors. A suite that passes against a shim is not evidence the code is correct. New `wire_annotations()` helper in conftest dumps `by_alias=True`, so the assertions name what a client actually receives. Verified identical on both SDKs by reading `run_command`'s annotations over a real stdio session: destructiveHint, idempotentHint, openWorldHint, readOnlyHint, title. Same idiom as `tools/batch_tools.py:68`.
The rename shipped in the code and in the tool pages, but six sites
still taught the removed `pattern` argument. Found by reviewing the
branch for merge readiness; verified by issuing each documented call
against the real server, which rejects them:
Remove or correct the unrecognized argument(s): pattern.
- docs/recipes.md: five wait-for-text calls migrated to `patterns`.
The two remaining singular `pattern:` uses are search-panes, which
legitimately still takes one.
- docs/prompts.md: the interrupt_gracefully "Sample render" showed the
pre-rename call, so the page misrepresented what the server emits. It
now matches byte for byte — and a test pins that, since nothing did,
which is exactly why it drifted.
- wait_for_text's `stop` parameter told agents a hit yields
`stopped=true`. There is no such field; it is `outcome="stopped"`.
This one shipped in the inputSchema, so agents read it.
- docs/configuration.md gained LIBTMUX_MCP_WAIT_MAX_SECONDS, the only
knob for the new ceiling and previously undocumented there.
- CHANGES understated two breaking changes: batch wrappers reject all
three self-bounded tools (run-command, wait-for-text,
wait-for-channel), not just run-command; and the clamp applies to
run-command and wait-for-channel too, both of which previously
honoured `timeout` verbatim.
- The run_and_wait prompt renders `timeout=60.0` while run-command
clamps to 30 s, so it now tells the agent to read
`effective_timeout` rather than assume.
`interrupt_gracefully` told the agent to pass `stop=["\^C", "Interrupt"]`
to catch programs that trap SIGINT and keep running. `^C` cannot do
that. The terminal's line discipline echoes it whenever SIGINT is
DELIVERED, whether or not the process dies, and `stop` wins a same-tick
tie against `patterns` — so the marker systematically beat the returning
shell prompt and reported the SUCCESS path as a failure.
Measured against a plain `sleep 60` that does die, wait already running:
shell outcome matched process actually died
sh stopped ^C yes
bash stopped ^C yes
The recipe's own text called these "the markers many programs print when
they catch SIGINT and keep running" — the opposite of what happened. Its
next documented step is SIGQUIT and then `kill-pane`, so the failure
mode is destroying a pane whose command had already been interrupted.
The other ordering was wrong too, differently: with the interrupt sent
BEFORE the wait — what an agent does across two tool calls — the
returning prompt is already on screen at entry, so the wait times out
and step 3 concluded "the process is ignoring SIGINT". Also false.
No stop list can separate these; the echo is identical either way. The
recipe now brackets the interrupt with `get_pane_info` and tells the
agent to trust `pane_current_command` over the wait result. The prompt
glyphs also lost their trailing spaces, which `capture-pane` strips.
Verified: the wrong answer is gone in both orderings, 0/4 across sh and
bash. The caret-escaping test guarded a marker that no longer exists;
it is replaced by one asserting `^C` is never reintroduced, with the
measurement in the docstring.
Regenerated from the branch's current net change against main rather than edited incrementally. The section had grown to 22 entries across five headings by accretion — one per commit, several covering the same behaviour from different angles — while four user-facing changes had no entry at all: the interrupt-recipe fix, the dependency-import diagnostic, the client-identity read, and the resolver errors that now name their target. Now 15 entries that group by what a reader hits rather than by what was committed. The four cancellation, thread-free, retry-multiplication and vanished-server fixes become one entry, because they are one theme: a wait reporting something other than what happened. The pattern rename, the stop list and the ceiling become one breaking change, because they arrive together and a caller has to react to them together. No version heading, no release summary; unreleased sections carry entries only. Released sections are untouched.
886a4a8 to
fbdbd74
Compare
Summary
wait_for_textanchored one row below where the cursor sat when the call began — and on a quiescent pane that is exactly where the next line of output lands, because the cursor sits at the end of the prompt. A daemon printing a singlereadyline could not be matched at all: the wait burned its whole budget and then reported that the pane had been silent. The anchor now lands on that row, and the row's pre-existing content is suppressed by value instead of by index, so carriage-return spinners and single-line status updates become matchable too.LIBTMUX_MCP_WAIT_MAX_SECONDS(30 s default, clamped to[1, 120]) capswait_for_text,wait_for_channelandrun_command. An over-largetimeoutis clamped rather than rejected, and the result reports what was actually enforced — the agent learns the policy from the result instead of from a failed call.pattern: strwithpatterns: list[str] | None, and addstop.patterns=nullwaits for any new output, subsuming the removedwait_for_content_change. Astoplist ends the wait in milliseconds on a known failure marker instead of burning the budget.outcomeenum replaces a scatter of booleans, alongside which entry fired, whether anything new was written, whether the match was text already on screen at entry, and a byte-bounded tail. A timeout now distinguishes "the command never ran" from "output arrived and did not match" from "a pager owned the pane".wait_for_channelreported success when the tmux server shut down without ever signalling, becausetmux wait-forexits 0 for both.Changes by area
src/libtmux_mcp/tools/pane_tools/wait.pyThe core. Entry-row anchoring with content-based suppression of pre-existing text,
patterns/stophandling, the alternate-screen guard, and every tmux call spawned as a deadline-bounded async subprocess rather than reaching tmux through a worker thread.src/libtmux_mcp/_wait_policy.py,server.pyThe ceiling policy, resolved from the environment and injected at tool-registration time, mirroring the existing history-default pattern.
server.pyalso fails at startup rather than silently dropping theis_calleragent-context instructions when the MCP instruction budget is tight, while still degrading gracefully for pathological runtime values._utils.py,middleware.py,batch_tools.py,wait_for_tools.pyThe
self-boundedtag and its enforcement: excluded from readonly retry, rejected per-operation by every batch wrapper, and applied towait_for_channelandrun_commandas well aswait_for_text.wait_for_channelgains the liveness re-probe that distinguishes a signal from a server that went away.models.py,state.pyWaitForTextResultreshaped around theoutcomeenum;ContentChangeResultremoved._PaneStategainsalternate_on, tolerant of older tmux that omits the field.prompts/recipes.py, docsThe
interrupt_gracefullyrecipe no longer treats a^Cecho as evidence that a program survived SIGINT — the terminal echoes it on delivery either way, so the marker fired on the success path and the recipe's next step is escalation. It now brackets the interrupt withget_pane_infoand trustspane_current_command. A new Waiting topic covers which wait to reach for, how the "is this new" predicate works, and what a wrong pattern costs.Design decisions
ThreadPoolExecutorwithshutdown(wait=False)was considered and rejected:concurrent.futures.thread._python_exitjoins every pool worker untimed regardless, so no thread-based arrangement survives a wedged tmux at interpreter exit. Only owning a killable subprocess does.wait_for(communicate())thenkill()thenawait wait()deadlocks when a wedged tmux's grandchild still holds the stdout pipe. The reader task is torn down first and the final reap is time-boxed; the loop's child watcher reaps the pid regardless.matched_at_entryis known before the poll loop starts, so returning immediately looks like free latency. It would break the commonest agent loop — run, wait for marker, re-run, wait for the same marker — so the wait holds out for a fresh occurrence and says so in the result.outcomeas one enum, not three booleans. The output schema is re-sent on every request, so a field no agent branches on is a permanent token tax.fastmcp-tasksalpha were trialled against this tool. Neither is reachable — no shipping client negotiates the capability — and both drop the numeric progress this tool emits. Recorded in the Waiting topic so it is not re-litigated from intuition.Verification
No caller pattern reaches a tmux format string — tmux treats
#and}structurally, so an ordinary regex quantifier would corrupt field parsing:Nothing on the wait path reaches tmux through a thread:
The removed tool and its result model are gone as symbols:
Test plan
uv run ruff format --check .,uv run ruff check .,uv run mypycleanuv run pytest --reruns 0green with flake-masking reruns disabledjust build-docssucceeds; removing thewait-for-content-changepage leaves no broken refstest_wait_for_text_sees_the_entry_cursor_row— a bare line, an unterminated prompt, a trailing-line case and a carriage-return rewrite all match on the entry row; two controls pin the other directiontest_wait_for_text_does_not_match_the_prompt_on_the_entry_row— the falsifier: a broad pattern must not self-match the prompt that was already on the rowtest_wait_for_text_waits_for_a_fresh_occurrence— a stale match on screen does not short-circuit the waittest_wait_path_uses_no_worker_threads— the wait path spawns no threadstest_wait_tools_do_not_block_event_loop— the loop survives a genuinely wedged tmux; mutation-tested to fail if a blocking spawn returnstest_wait_for_text_bounds_every_tmux_call— every spawned tmux call is deadline-boundedtest_wait_for_channel_detects_a_vanished_server— a cleankill-serverand a SIGTERM are both reported as errors, with a live-server control that must still succeedtest_wait_resolver_matches_the_canonical_resolver— the wait's private resolver agrees with_utils._resolve_paneon precedence and on the exception a miss raiseswait_for_texttargeting cases, including the linked-windowMultipleObjectsReturnedtrap