Fix silent-failure and misleading-error classes across the tool surface - #125
Open
tony wants to merge 20 commits into
Open
Fix silent-failure and misleading-error classes across the tool surface#125tony wants to merge 20 commits into
tony wants to merge 20 commits into
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #125 +/- ##
==========================================
+ Coverage 86.24% 86.54% +0.29%
==========================================
Files 46 46
Lines 4042 4272 +230
Branches 599 645 +46
==========================================
+ Hits 3486 3697 +211
- Misses 404 411 +7
- Partials 152 164 +12 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Calling a tool above the server's `LIBTMUX_SAFETY` tier reported `Unknown tool: 'kill_pane'` — the server denying that its own gated tool exists. An agent told a tool is absent reports the capability as missing instead of naming the setting that enables it. Two gates were intended and only one worked. FastMCP's native `disable()` enforces the tier; `SafetyMiddleware` was meant to explain it. `get_tool()` answers `None` for a disabled tool, so the guard reading `if tool and not allowed` never ran for precisely the tools it was written for, and dispatch raised `NotFoundError` instead. Off-tier names now resolve against `_list_tools()`, which retains disabled tools with their tags. The batch wrappers carried the same defect through a second call path: `_get_allowed_tool_tier` also read `get_tool` and raised "Unknown tool" on `None`, so a gated tool and a misspelled one produced byte-identical rows. It hands the operation on instead of duplicating the lookup — nested calls already run with `run_middleware=True`, so one source of truth decides which of the two it is. The message also hardcoded `LIBTMUX_SAFETY=destructive` for every denial, so a readonly server answered a `send_keys` call by advising the strongest tier — telling a user to grant `kill_server` rights in order to type into a pane. Denials now name the required tier and the tier in force. Restores an audit property the middleware ordering is built around: a tier denial must raise inside `SafetyMiddleware` so `AuditMiddleware`, sitting outside it, records it as a denial. While the denial never fired, blocked calls were audited as unknown-tool errors. `on_call_tool` now fails closed with no FastMCP context. It previously fell through to the tool, which is fail-open in a gate whose top tier includes `kill_server`, and was masked only because the native gate made the dispatch fail anyway. The existing suite documented the dead path rather than testing it, so 921 passing tests never called a gated tool end to end. Adds that coverage per tier and through the batch wrapper, plus a contract test pinning the private `_list_tools()` behavior the explanation depends on.
tony
force-pushed
the
mcp-improvements
branch
2 times, most recently
from
August 25, 2026 01:06
0735203 to
9d6f7be
Compare
Two defects on the same seam: an operation failed and the server told the agent something that was not true. `send_keys` dropped any payload starting with `-` and returned `Keys sent to pane %N`. tmux read the payload as flags and rejected the command; `Pane.send_keys` builds the argv with no `--` separator and discards tmux's result, and the wrapper returned a hardcoded success string. Reachable with ordinary input — `--help` typed into a REPL, a negative number, a pasted diff line. The argv now ends flag parsing with `--` and a failed send raises with tmux's own stderr. All three call sites built argv separately and shared none of it; they now share one builder, which also fixes the timed batch path (it surfaced the error but still failed to deliver). `wait_for_text` crashed with `ValueError: invalid literal for int() with base 10: ''` when its pane died mid-wait. tmux expands every field of a vanished pane to the empty string and three `int()` calls on the poll path took it raw — the same degrade-don't-fail rule the comment three lines above already states for `alternate_on`. Fixing only the `int()` calls would have swapped the crash for a wrong answer: `pane_dead` is itself blanked, so it reads back as `"0"` and cannot report the death. A killed pane would then have failed the pid comparison and been reported as *respawned*. A live pane always has a `pane_pid`, so an empty one is the reliable gone signal, and the wait now ends with `pane %N died`. `_read_history_limit` had the same defect one call over — its guard covered an empty list but not an empty string. There were no tests for `_parse_pane_state` at all, which is why both defects survived. The three history tests that asserted on `Pane.send_keys` calls now observe the argv boundary, which is where every path already goes.
`list_sessions`, `list_windows` and `list_panes` validated the operator
half of a Django-style filter key and never the field half, so
`filters={"nosuch_field__contains": "x"}` returned `[]`. A key with no
`__` at all was not checked by anything — the loop only entered its
branch when one was present, and bound the field to `_field`, the name
that means "deliberately unused".
libtmux's `QueryList` resolves a key by attribute traversal and treats
a miss as "no match", so a misspelled field silently filtered every row
out and the empty result was indistinguishable from a real one. That is
the worst shape in this class: not an error the agent can react to, but
a confident wrong answer shaped like a right one.
Field names are now checked against the object being filtered, with
near-misses suggested, mirroring the operator error that was already
good. Validation covers only the leading segment, so nested traversal
like `active_window__window_name__contains` keeps working — the check
rejects what the type cannot have rather than whitelisting.
The type is a parameter rather than read off the first item, so an
empty list still validates. That is exactly when a typo most needs
reporting.
A pane whose current directory contains a newline makes libtmux fail to parse `-F` output, and because every pane lookup enumerates panes, the whole server stops resolving — healthy panes included. It reached the agent as `Unexpected error: ValueError: zip() argument 2 is shorter than argument 1`, logged at ERROR, naming nothing it could act on. The agent could not repair it through the MCP either: every tool that could have moved the pane out needed the same enumeration. It is now an expected failure that names the cause, says the blast radius is server-wide rather than one pane, and gives the command that locates the offender. Matched on the message because the raise site is a stdlib `zip` with no dedicated exception type. The parse itself is fixed upstream in tmux-python/libtmux#752, but this diagnosis is kept rather than deferred: the floor is `libtmux>=0.62.0` and the installed version is not this package's to choose. `exc.PaneNotFound` prefixes its own message and the mapper prefixed it again, so the most frequently hit error in the server read `Pane not found: Pane not found: %9999`.
When the tmux binary running this server is older than the one that created a socket, `get_server_info` returned `is_alive=False, session_count=0` and `list_sessions` returned `[]`, both without error. An agent reads that as "the user's work is gone". Two of four calls answered with a confident falsehood; `list_panes` did error. `Server.is_alive()` answers `False` both for a socket with no daemon and for a live server this binary cannot speak to, and `Server.sessions` degrades to `[]` in both cases. libtmux's own `sessions` docstring points at `is_alive` to tell the two apart, but it cannot — they collapse to the same `False`. tmux distinguishes them on stderr, so `_probe_liveness` reads that instead of the boolean. `list_sessions` probes only when the listing came back empty: a server that listed anything cannot be unreachable, so the common path keeps its single round trip on the most-called discovery tool. `ServerInfo` gains `unreachable_reason`. When set, `is_alive=False` means "could not ask", not "not running", and `session_count=0` carries no information. `list_servers`' socket scan shares the probe. The trigger is an ordinary tmux upgrade — sockets outlive the binary that made them. Verified against a real 3.2a client and a 3.7c server. Also: `exit_copy_mode` on a pane that was not in a mode returned a full `PaneInfo`, reading as confirmation the pane had left copy mode, while tmux said `not in a mode` and exited 1. `Pane.send_keys(copy_mode_cmd=...)` discards that result the same way the ordinary send path did. Both copy-mode call sites now share a helper that raises with tmux's stderr; no `--` is needed there because every command is a module constant rather than caller text. The liveness tests drive a crafted result rather than a second tmux binary, so they assert the discrimination without depending on which tmux a CI job installed.
The entry scan covered `patterns` and not `stop`, so waiting on a pane that already showed a failure marker returned a bare `timeout` with no sign the marker had been there the whole time. The realistic shape: an agent runs a build, it fails, the agent waits for the next build without clearing, and reads `timeout` as "still running" when the honest answer is "the previous run already failed". `WaitForTextResult` gains `stop_matched_at_entry`, kept separate from `matched_at_entry` because a stale success marker and a stale failure marker call for opposite reactions. A stale stop hit still does not end the wait — only a fresh one does — so this is a diagnostic, not a behavior change. The rationale is the one already written above `matched_at_entry`'s own entry scan: an agent must be able to tell "already there" from "never arrived". That reasoning was applied to the success patterns and not to the stop patterns three lines away. Also corrects the server instructions. `stop=[] bails` was parsed by two independent readers as "the empty list bails". `stop=[]` is accepted and behaves like `stop=null`; it is a stop *hit* that returns immediately.
The directory scan held each socket's full path in the entry it was iterating and reported only its name, so `socket_path` was null on every scanned row. Passing that same socket through `extra_socket_paths` then listed it a second time carrying the opposite half of its identity, with nothing tying the two rows together — an agent could not tell they were one server. Scanned rows now carry both fields, and extras are deduplicated against the scan by resolved path, which also survives symlinks and relative paths. A socket whose name does not round-trip through `tmux -L` — one holding a newline — raised inside `get_server_info` and was dropped from the listing by a bare `continue`, so a live server vanished without a word. It now falls back to the path probe, which always works.
Output that laps a pane's `history-limit` returned no lines with `lines_missed=False`, while tmux still held dozens of them. The field built to report exactly this loss reported its opposite. Two defects compounded. `_cursor_anchor_lost` has no overflow case: `history_size` climbs to the limit and then stays pinned while rows are evicted off the top, so none of its three tests fire. And the fingerprint degenerates to a single hash whenever the anchor was the last row — the normal case, because an agent starts tailing an idle pane and the anchor is the shell prompt. The uniqueness guard asks whether a candidate is unique *in the current buffer*, not unique *in time*, so once the flood evicted the anchor its one surviving twin was the prompt currently on screen: one candidate, guard satisfied, false match far below the real anchor. Everything above it was dropped as "already seen". Matches are now rejected on position past `anchor_abs` — tmux evicts only from the top, so a surviving anchor can only move earlier. That is necessary but not sufficient: an anchor taken at the bottom of an already-saturated history occupies the same row as the current prompt, and the two candidates genuinely overlap. So a single-hash fingerprint on a saturated history additionally refuses to match inside the visible region. Declining costs a conservative `lines_missed=True`, which stays honest — a saturated history means rows were evicted whether or not the anchor itself survived — and panes away from their limit are untouched. Saturation is asked via the existing trim-risk heuristic, not `history_size == history_limit`: measured on tmux 3.7c, a pane with `history-limit 20` pins at `history_size 19`, so an exact comparison never fires on the very panes this guards. The existing regression test worked around the defect rather than catching it. Its docstring records that "the flood alone is not deterministic — tmux 3.6 retains enough of the original prompt that `_find_unique_cursor_match` re-anchors on the surviving hash" and adds a `clear-history` to force anchor destruction, so the flood-only path that real agents hit was never covered. It is now, without help. Measured before/after on a 20-line history: a 5-line burst returned 0 lines claiming nothing was missed, and now returns the visible content flagged `lines_missed=True`; a 50000-line history is unchanged.
tmux does not unify `-g` listings across its session and window trees, so `show-hooks -g` omits pane-level hooks that `show-hooks -gw` holds. A merge existed to paper over that and its own comment said so — but it was gated on the caller passing `scope="server"` explicitly. The natural call, `show_hooks()`, leaves `scope` at its default of `None` and skipped the merge, so asking "what hooks are configured?" the obvious way returned an incomplete list with no sign anything was omitted, and `show_hook()` on a missing name then contradicted it. The server instructions point agents straight at that pair. The test named for this behavior only exercised the explicit-scope path, so the default one — the one agents actually take — was never covered.
A capture large enough to hit the 1 MB backstop produced `RuntimeError:
Tool capture_pane has an output schema but did not return structured
content` — a transport-level failure delivering no data at all, which
is worse than the truncation the limiter exists to perform and worse
than having no limiter. `capture_pane`'s docstring advertises
`max_lines=None` for a complete capture, so the documented way to ask
for everything was the way to break it. Size-driven, not `None`-driven:
a large explicit `max_lines` fails identically.
The limiter rebuilds the result when it truncates, and that rebuild
dropped `structured_content` alongside `is_error`. The `is_error` half
was already fixed, and this class's own docstring spells out why —
"MCP clients then validate the truncated text against the tool's output
schema and fail with a transport-level error". The successful-response
half was left standing with the same consequence and no `is_error` to
restore.
Truncated successes now carry structured content. Only the
`{"result": str}` shape fastmcp gives a `-> str` tool is rebuildable;
model- and list-shaped payloads carry the oversize inside their own
fields and cannot be trimmed from the flattened text, so those return
an actionable tool error telling the agent to narrow its range rather
than a response its client will reject outright.
Verified against a real >1 MB capture: `max_lines=None` returns 954 KB
of truncated data with structured content intact instead of raising.
Tail preservation, the dropped-line count, and the per-tool caps were
correct throughout; only the over-cap rebuild was broken.
`search_panes` searches only the visible screen unless `content_start`
is given, but reported `matches: []` alongside `truncated: false` — an
active claim that nothing was left out, for a search that never looked
at scrollback. Its docstring described "visible terminal scrollback
content", which reads as scrollback and is what led two readers to
expect it.
The result now carries `searched_scope`, and `truncated` is documented
as describing the `limit` and per-pane line caps only. The default
stays visible-only deliberately: the tool fans out across every pane on
the server, so defaulting to scrollback would multiply cost by history
depth times pane count and make identical calls take wildly different
times depending on how long panes had been alive. Telling the agent
what was searched is strictly more useful than a slow complete answer,
because it also reveals the knob.
Also: `paste_text("")` errored with `no buffer libtmux_mcp_..._paste`
because tmux creates no buffer for empty content — an error for a
no-op, naming an internal buffer the caller never chose. It is now a
no-op success.
And `send_keys` documents the roughly 16 KB tmux ceiling that surfaces
as `command too long`, pointing at `paste_text` for larger payloads,
plus a warning against verifying a write by string-comparing captured
text: tmux renders combining marks and zero-width joiners as `<XXXX>`
placeholders, so `école` and emoji come back transformed even when the
bytes were delivered correctly.
`show_option(option="history-limit", scope="session")` returned `value: null` while 50000 was in force. tmux resolves inherited values with `-A` and libtmux has always accepted `include_inherited`, but the tool never exposed it — so an agent could ask "is this set at this exact scope?" and never "what is in force here?", which is what a question like "is mouse mode on?" actually means. `include_inherited` is now a parameter, and the result carries `scope_queried` so a `null` reads as "not set at THIS scope" rather than "not set anywhere". `show_environment` no longer encodes removal in the key. tmux prints a removed variable as `-NAME`; the dash was kept and the value set to boolean `true`, so `variables["KRB5CCNAME"]` raised `KeyError` while `variables["-KRB5CCNAME"]` answered `true` — the inverse of the truth, for a variable that is explicitly unset — and every consumer had to type-check a `str | bool` mapping. `variables` now holds only names that are set, mapped to their values, with removed names listed separately. `search_panes` rejects `offset < 0` and `limit < 1` instead of clamping or answering with an empty page. `limit=0` returned `matches: []`, which an agent cannot tell from a genuine miss, and a negative offset was clamped to zero while being echoed back unchanged. Also records the measured false-positive band for the capture_since saturation guard: on a 50000-line limit it reports a loss from roughly 92% full, and is clean at 0/10/50/80/88%. Growth in `history_size` would narrow it, but a burst that saturates midway both grows and evicts, so trusting growth would reopen the silent loss that guard closes.
`run_command` assumed a cooperative shell sitting at a prompt, and neither verified nor documented it. That assumption holds in tests and breaks in a live session, in two ways. A full-screen program owns the pane's keyboard, so the exit-status wrapper is consumed as ITS keystrokes. Measured against `less`: `s=$?...` became less's save-to-file command, a fragment escaped to a shell, and the pane was left on less's help screen. In `vi` the same payload lands in the buffer, where `:`-prefixed fragments are commands that edit and write files. `alternate_on` was readable before the call the whole time — `snapshot_pane` already reports it — so the tool now reads it and refuses, naming the occupant and pointing at `send_keys`. The guard is deliberately narrow. There is no reliable "pane is busy" signal: `pane_current_command` is the foreground process, which is legitimately non-shell for the entire duration of any long command an agent wants to run, so refusing on that would break the main use. `alternate_on` means a program has taken the whole grid, which is never a state where a shell wrapper makes sense. A timeout does not cancel the command. The keystrokes are already in the pane's input buffer, so a blocked shell runs them whenever it next reads a line — verified: a command reporting `timed_out=True` executed once the blocking `sleep` returned. An agent reading `timed_out` alone concludes it did not run and retries, which is how a `git push` or a migration runs twice, the second time unwatched. The result now carries `command_may_still_run`, and the docstring states both preconditions. The wrapper is sent through the checked send path, so a rejected send surfaces as an error rather than as a timeout. Also: `Invalid buffer name: 'yanked'` misattributed. tmux accepts that name; it is this server that only touches buffers it allocated, because tmux buffers can hold OS clipboard history. The message says so, and states that a copy-mode yank or tmux's own `buffer0` is unreachable by design rather than leaving that to be inferred.
Reading `tmux://sessions/nosuchsession` answered `Internal error: Session not found: nosuchsession`. That is a caller naming something that does not exist, reported as a server fault. `ToolErrorResultMiddleware` exists to remove exactly that wrapper — its docstring says so — but it intercepts `tools/call` only, while the fastmcp transform it inherits from serves EVERY message kind. So the defect survived one fork over, on resources. Fixed at the transform rather than by adding a resource hook. The property is "an expected failure is never an internal error", and it has to hold on every path that transform serves, not just the one that was noticed. An error this server raises deliberately to describe a caller-caused failure now maps to `-32002` on a resource read and `-32602` elsewhere, with its own message intact. The predicate is a named tuple rather than `ExpectedToolError` alone: the resource handlers raise fastmcp's `ResourceError`, so a check on our own type matched nothing and looked like it had worked. A test pins the negative control — a genuine `RuntimeError` still reads as `-32603 Internal error:` — so this reclassifies caller mistakes without hiding server faults. Also documents that a resource URI's path segment is percent-decoded before lookup. With sessions named `pct name` and `pct%20name` both live, `tmux://sessions/pct%20name` returns the former: the wrong session, silently. Only `pct%2520name` reaches the latter. A space needs no encoding, which is what hides it. Concatenating a `session_name` straight from `list_sessions` is the obvious construction and the one that breaks, so a ready-made `uri` field on the models is the better fix and is left as follow-up rather than landed unverified.
Four tools returned success for an operation that did not do what the
caller asked. In each the confirming signal was already reachable and
simply not consulted.
`pipe_pane` to an unwritable path. tmux hands the pipe command to a
shell and reports success whatever that shell then does, so a redirect
into a missing directory produced `Piping pane %N to ...` and no file
ever appeared — and a stale file already there would then read back as
if it were live capture. The destination is checked before piping.
`#{pane_pipe}` looks like the obvious discriminator and is not:
measured, it reads `1` immediately after a doomed pipe because the
shell has been spawned and has not yet failed, and only `0` some
200 ms later. Reading it here would be a check that never fires.
`respawn_pane` with a shell that cannot run. tmux does not fail a
respawn whose command cannot be executed: the new process dies at once
and takes the pane with it — and its window, session and server, if it
was the last one. Measured, a mistyped shell path destroyed the entire
server while the tool returned a `PaneInfo` for a pane that no longer
existed. The program is checked before respawning, because catching it
afterwards can only report the loss and even that races the dying
process.
`respawn_pane` returning the process it replaced. `pane_pid` changes at
once but `pane_current_command` lags it by ~14 ms, so the model
described the process about to be replaced. Not cosmetic:
`test_respawn_pane_replaces_shell` asserts on that field and has been
flaking since it was written, absorbed by `--reruns=2`.
Three predicates were measured for that wait and all three fail. Pid
change is necessary but not sufficient (stale 15/15 runs). Command
change never fires when a pane is respawned as itself. Two consecutive
equal reads is the worst: the pre-change value is itself stable, so a
fast poll debounces onto the OLD value and returns it confidently —
0/6 stale at a 20 ms poll, 2/6 at 5 ms, 3/6 at 1 ms, correct only by
accident of the interval. What ships matches the requested command's
basename and is interval-independent. A commandless respawn needs no
wait at all, structurally: `spawn.c` guards its default-command
fallback on `sc->argc == 0 && (~sc->flags & SPAWN_RESPAWN)`, so a
respawn skips it and reuses the pane's existing argv.
`paste_text` with a trailing newline. Bracketed paste — the default —
holds the newline in the shell's edit buffer instead of submitting.
Correct terminal behavior and a safe default, but `Text pasted` reads
as "your command ran", and the text is not inert: it executes when
Enter next reaches that pane from any source, out of order with the
call. The result says so and names both ways to submit.
`test_capture_pane_truncates_tail_preserving` failed intermittently in full-suite runs and passed in isolation, which reads as a draw race and is not one. It borrowed the shared pane fixture, and an earlier test splitting that window left the pane a few rows tall; `capture_pane` then returned fewer lines than `max_lines`, nothing was truncated, and the header assertion failed. Measured: 24 rows gives 24 captured lines and a header, 3 rows gives 2 lines and none. Deterministic given the height — the line count lands on 2, not on 5 or 7, which is what rules out a partially drawn pane, since a draw race would give the right count with wrong content. So this one is a test-isolation bug rather than a product defect, the opposite conclusion to the respawn flake fixed alongside it, which was a real defect hiding behind `--reruns=2`. Both were reached by measuring rather than by guessing, and guessing would have got them wrong in opposite directions. The measurement is in the docstring so the next reader does not re-diagnose it as timing.
…reen `alternate_on` turned out to be necessary rather than sufficient, and the counterexample is reachable through this server's own tooling: `less` viewing a `pipe_pane` capture decides the file is binary and prompts "may be a binary file. See it anyway?" BEFORE entering the alternate screen. It owns the keyboard with `alternate_on=0`, so the exit-status wrapper was typed into it and reported a clean `exit_status=0` — the exact outcome the guard exists to prevent, with no timeout and no `command_may_still_run` to soften it. Confirmed by answering the prompt: pressing `y` flips `alternate_on` to 1. The general test — "the foreground command is not the process tmux started" — was implemented and measured across five pane states, which it separated correctly, and then rejected: this suite refused it because a pane running `bash` inside a `zsh` pane has a perfectly good prompt, as do `sudo -s`, `ssh` and `nix-shell`. "The foreground process isn't the one tmux started" is true and does not answer "is there a prompt". So a small deny-list of pagers and editors instead, where typing a shell wrapper is destructive rather than merely wrong. Incomplete by construction, and the cost is measured rather than assumed: against a `python` REPL the call is not refused, `timed_out` and `command_may_still_run` both fire correctly, and the REPL is left at a `...` continuation where the next input from anyone is swallowed. State survives, nothing is destroyed. Destructive programs are refused; merely disruptive ones are carried by `command_may_still_run`.
`test_capture_pane_truncates_tail_preserving` was moved to a dedicated window on the theory that a shared pane had been shrunk by an earlier test's split. That mechanism is impossible: libtmux's `server` and `session` fixtures are function-scoped, measured as two tests landing on different sockets with both panes at 24 rows, so no earlier test can resize a later one's pane. A 3-row pane does reproduce the symptom, which is what made it look like the cause. Demonstrating that a hypothesis CAN produce a symptom is not evidence that it did, and the check that would have refuted it — reading the fixture scope — was one `rg` away and never run. Reverted rather than left in as a fix that fixes nothing. The correction stays in the test's docstring and CHANGES alongside the two other candidates measured away: the retry budget needs 0.24 s against 2 s even under eightfold load, and randomized ordering survives in isolation. What is actually happening is suite-wide. Run the suite as CI does, `pytest -n auto`, with `--reruns` off, and every run fails — two independent sets of runs give 1/5/2/3/3 and 6/4/1 failures, a different sample each time, well over a dozen distinct tests in the union. Most drive a real `zsh` and wait inside a budget tuned on an idle machine. The family spans at least `tests/test_pane_tools.py` and `tests/test_history.py`, so it is not one module's problem. The same suite in the default configuration reports `968 passed, 6 skipped, 7 rerun`. Three configurations are each blind to this — fixed-order with reruns on, random-order with reruns off but serial, and CI's parallel with reruns on — which is why three separate per-test explanations were proposed and measured away before the shape was visible. Fixing the budgets is a larger piece of work than this branch should absorb; naming the property, with the measurements, is not.
Eleven sampled runs of the parallel suite put all but one failure in `tests/test_pane_tools.py`, which invites narrowing the follow-up to that module. Counting the at-risk pattern instead — drives a real shell AND waits on output — finds it across several modules, with `test_pane_tools.py` holding 78-90% of the wait sites depending on how they are counted. Its dominance in every sample is a density effect, not exclusivity, and a thin tail elsewhere is exactly what the single observed failure in `tests/test_history.py` represents. Sampling was reading a distribution's mode as its support; a density count settles in one pass what eleven runs could not, and points the fix at the right scope.
Recorded as 78-90%; independently recomputed at 77-79% across three patterns, and 75% counting `retry_until` alone. The range is now 75-85%, which covers every measure taken. The spread has a cause worth keeping. A pattern that counts `wait_for_text` calls inflates `test_pane_tools.py`, because `wait_for_text` is the tool UNDER TEST in that module — 110 occurrences of it there are the subject of the assertions rather than waits supporting them. The metric measured something adjacent to the claim in exactly the place the adjacency was greatest, which is the same shape as the signal errors catalogued elsewhere in this branch. `retry_until` alone is the cleaner proxy. The claim being scoped — dominant but not exclusive, so scope the fix by the pattern rather than the filename — survives all of them, which is why it is worth stating and no single figure is.
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
2 participants
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.
Bug-hunting pass over the MCP tool surface. A second Claude instance drove the server as a black-box MCP client — twelve phases, including a 9-version tmux matrix run twice — while this one reproduced and fixed what it found. Every finding was reproduced independently before a fix landed, and every fix was re-tested by the black-box instance against a worktree of the pushed branch.
26 fixes across 17 commits. Each commit passes the full chain (
ruff check,ruff format,uv run mypy .,pytest --reruns 0,just build-docs), and CI is green across tmux 3.2a, 3.3a, 3.4, 3.5, 3.6 and master.The pattern
Nearly every high-severity finding is one defect in a different module: an operation failed, or could not be answered, and the server returned something success-shaped that was not true.
Unknown tool: 'kill_pane'— it exists and is gatedsend_keyswith a-prefixKeys sent to pane %N— nothing was sent[]— indistinguishable from "nothing matched"ValueError: invalid literal for int()is_alive=False,list_sessions=[], no errorhistory-limitlines_missed=Falseshow_hooks()Internal error: Session not found: …pipe_paneto an unwritable pathPiping pane %N to …— no file, everrespawn_panewith a bad shellpaste_textwith a trailing newlineText pasted— the command had not runThe error path was less defended than the happy path. The
ExpectedToolError+suggestion+meta.error_typemachinery here is good; it was simply not reached by the cases that mattered most.Three observations shaped how the fixes were written:
Not crashing is not the same as being right. Making the pane-state
int()calls tolerant would have removed the crash and kept the lie —pane_deadis itself blanked for a vanished pane, so a killed pane would then have been reported as respawned.When a fix targets a code path rather than a property, the property stays broken wherever the path forks.
ToolErrorResultMiddlewarestripsInternal error:ontools/call; the transform it inherits serves every message kind, so resources kept reporting caller mistakes as server faults. Fixed at the fork, not the path. A subsequent audit of the three multi-site defect classes found one previously-unexaminedget_tool()caller (benign) and confirmed the rest.Several tests asserted the property but never on the path users take, which is how 921 passing tests coexisted with these. There was no
_parse_pane_statetest at all; the hook test only used an explicitscope="server"; the response limiter'sis_errorbranch was fixed and documented while its success twin was left; and thecapture_sinceflood test documents the defect in its own docstring — "the flood alone is not deterministic …_find_unique_cursor_matchre-anchors on the surviving hash" — then adds aclear-historyto force the failure it wanted. Each was written from the fix's point of view rather than the caller's.Fixed
disable()makesget_tool()answerNone, so the middleware'sif tool and not allowedguard never ran for the tools it was written for. The message also hardcodedLIBTMUX_SAFETY=destructivefor every denial. Same defect in the batch wrapper. Denials reach the audit log as denials again;on_call_toolfails closed with no FastMCP context.send_keysno longer drops text starting with-—--terminates flag parsing, a failed send raises with tmux's stderr, and all three call sites share one argv builder.is_alive()collapses "no daemon" and "cannot speak to this server" into the sameFalse; tmux distinguishes them on stderr.ServerInfogainsunreachable_reason.capture_sincereports the loss when a pane laps its history limit. The fingerprint degenerates to a single hash whenever the anchor is the shell prompt — the normal case for an agent tailing an idle pane — and the uniqueness guard asks whether a candidate is unique in the current buffer, not in time.show_hooks()merges both hook trees on the default scope.structured_contentalongsideis_error.run_commandrequires a shell at a prompt — it refuses a pane on the alternate screen rather than typing its wrapper intoless/vi— and reportscommand_may_still_run, because a timeout does not cancel: the keystrokes run whenever the shell next reads a line.alternate_onalone proved necessary-but-not-sufficient —lessviewing apipe_panecapture prompts "may be a binary file" before entering the alternate screen — so a small deny-list of pagers and editors covers that, with its residual gap measured and documented.pipe_panechecks its destination before piping.#{pane_pipe}looks like the discriminator and is not — measured, it reads1immediately after a doomed pipe and only0some 200 ms later.respawn_panechecks the program before respawning, and returns the new process rather than the one it replaced.paste_textsays when a bracketed trailing newline was not submitted, and is a no-op for empty text.search_panesreportssearched_scopeand rejectsoffset < 0/limit < 1;show_optiongainsinclude_inherited;show_environmentseparates removed names from set ones;list_serversrows carry a complete identity;wait_for_textreports a stop marker already on screen; a rejected copy-mode command is no longer success; a newline in a directory name is diagnosed;Pane not found:is no longer said twice; the buffer-name error no longer calls a valid tmux name invalid.Upstream
The newline-in-a-path parse bug is rooted in libtmux and fixed in tmux-python/libtmux#752. It cannot ship here until libtmux releases and this floor moves, so this branch carries the diagnosis instead.
Separately, libtmux's
session_check_name()rejects empty names,.and:, while tmux has no window equivalent. The two halves have different standing and should not be relaxed together:new-session -d -s ''— accepted on 3.2a, rejected on 3.3a, 3.4, 3.5, 3.6 and 3.7, accepted again from 3.7a. On six of nine supported versions libtmux's rejection matches tmux.Where a fix was rejected, and why
Three plausible fixes were measured and discarded rather than shipped, each of which passed its own first test:
capture_sincefingerprint by position alone. Sound, and it fixed one repro — but on a saturated history the anchor's old row and the current prompt's row are the same number, so position cannot discriminate. Needed a second predicate.pipe_paneverifying#{pane_pipe}after piping. Passed the success case; reading it after a doomed pipe returns1, because the shell has been spawned and has not yet failed. A check that could never fire.respawn_panewaiting for two consecutive equal command reads. The pre-change value is itself stable for ~14 ms, so a fast poll debounces onto the OLD value and returns it confidently: 0/6 stale at a 20 ms poll, 2/6 at 5 ms, 3/6 at 1 ms — correct only by accident of the interval. Two other predicates (pid-change, command-change) fail too. What ships matches the requested command's basename and is interval-independent.The suite reports green in CI while two to five tests fail per parallel run. Run it as CI does —
pytest -n auto— but with--rerunsoff, and every run fails. Two independent measurement sets: 1/5/2/3/3 failures across five runs, and 6/4/1 across three. A different sample each time, well over a dozen distinct tests in the union, spanning at leasttests/test_pane_tools.pyandtests/test_history.py. Most drive a realzshand wait inside a budget tuned on an idle machine. The same suite in the default configuration reports968 passed, 6 skipped, **7 rerun**— the rerun count is the only visible trace.This supersedes three separate per-test explanations proposed during this branch, all measured away: a shared pane shrunk by another test's split (impossible — libtmux's
server/sessionfixtures are function-scoped, measured as two tests on different sockets with both panes at 24 rows), a retry budget too tight (the marker needs 0.24 s under eightfold load against 2 s), and test ordering (survives randomization in isolation). One change made on the strength of the first theory was reverted rather than left in as a fix that fixes nothing.Three configurations are each blind to this — fixed-order with reruns on, random-order with reruns off but serial, and CI's parallel with reruns on — which is why a single failing test kept reading as a local defect. Fixing the budgets is larger than this branch should absorb; the measurements are recorded so whoever takes it starts from data. Scope that work by the pattern — drives a real shell and waits on output — not by the filename: sampled runs put nearly everything in
tests/test_pane_tools.py, but counting the pattern finds it across several modules with that one holding roughly 75-85% of the wait sites. Its dominance in every sample is density, not exclusivity. (The spread is instructive too: a pattern countingwait_for_textcalls inflates that module, becausewait_for_textis the tool under test there — the metric measures something adjacent to the claim exactly where the adjacency is greatest.retry_untilalone is the cleaner proxy.)_reports_status_after_shell_state_change[errexit_false]at 3-of-5 is the closest to deterministic.It also matters beyond this suite:
--reruns=2is the mechanism that concealed a genuinerespawn_panedefect found in this same branch, which had been flaking since the day its test was written.Confirmed, reproduced, not fixed here
capture_since's fingerprint could be extended with rows above the anchor, removing the conservative branch above. Needs a cursor version bump; deliberately deferred.start_directoryis silently ignored — tmux falls back to the default directory, and every relative path in that pane then resolves against the wrong root. Genuine design question: tmux's fallback is deliberate, so "fail" and "report the divergence" are different products.pct nameandpct%20nameboth live,tmux://sessions/pct%20namesilently returns the wrong one. Documented; a ready-madeurifield on the models is the real fix.show_optionon an unset user option changes contract at tmux 3.3a.run_command.outputis the whole visible pane, not the command's output.pipe_paneaccepts onlyoutput_path, so tmux's "filter a live stream" use is unrepresentable.move_window(destination_index=)is typedstr. Note for whoever adds agt/ltfilter operator: over string-typed fields a naive comparison makes'9' > '10'true.tmux_binis process-env only whilesocket_nameis per-call, so a server whose binary is not onPATHcannot be reached.capture_pane/capture_since/run_command.mutatingtier can create sessions, windows and panes but reap none of them. Changing this changes what a safety tier permits, so it wants an explicit decision.Worth protecting under refactor
Called out independently by the black-box instance:
capture_since's cursor carries identity, position, content-anchor and geometry, so respawn, resize, cross-pane and corruption are each structurally detectable; the wait ceiling reportseffective_timeoutinstead of clamping silently; batch tools isolate per-row errors, refuse to nest, refuse to batch self-bounded waits, and enforce tier before execution (verified by side effect, not by message);search_panespushes the match into tmux's own#{C:}and its fast and slow paths agree on every probe, with format-string injection inert; audit redaction digests payloads to length + SHA-256 prefix on every path including both nested ones; thesuppress_historycontract holds exactly as documented, verified against real histfiles; stale-socket filtering uses anAF_UNIXconnect rather than spawning tmux per socket; the tool surface is byte-identical across all 9 tmux versions.Notes for contributors
LIBTMUX_TMUX_BINto pin the binary per server, and pointingPYTHONPATHat a worktree of the branch under test, are what made a 9-version matrix tractable. Without a pinned build most of a QA pass is spent deciding whether a result is real — a developer's MCP client config runs the working tree, and a long-lived server freezes it at process start.Two regression tests here were themselves version-fragile before being rewritten. The first
send_keystest asserted on pane contents and failed on 3.2a and 3.4 while passing on 3.6, because it depended on the shell echoing un-submitted text; it now asserts at the argv boundary. The liveness tests drive a crafted result rather than a second tmux binary.