Make tmux MCP operations bounded and truthful - #129
Open
tony wants to merge 230 commits into
Open
Conversation
why: Reviewing a branch across the agent CLIs meant checking it out, swapping to the checkout, then remembering to unwind both. uv resolves a git ref on its own, so a pull request can be swapped in without a working copy at all — which makes reverting the ordinary config restore, with nothing left on disk to prune. Resolution happens when an agent starts the server, so a bad ref would otherwise land in every config and fail opaquely inside each one. The swap now proves the command answers MCP before writing anything. what: - Add `use-local --pr N`, writing `uvx --from <remote>@refs/pull/N/head` - Complete an MCP initialize round trip before the first write, with `--no-preflight` to skip it - Read the pull request through `gh` to confirm it exists and label the output, keeping resolution independent of it - Recognize the shape in `status`, ahead of the version-pin branch that would otherwise report the ref as a pin
why: The JSON writer re-serialized the whole document to change one entry, so it escaped every non-ASCII character in the file and appended a trailing newline the file may never have had. In ~/.claude.json that reached model labels and prompt history the swap never read, turning a one-entry edit into a diff spanning the file — noise a reviewer has to read past in `--dry-run`, and a rewrite of bytes that were not ours to touch. Dropping the escaping alone would trade one defect for a worse one: a lone surrogate, which is what a JavaScript writer emits for a string sliced through a surrogate pair, has no UTF-8 encoding, and the resulting UnicodeEncodeError is not the RuntimeError the per-CLI handler catches — it would abort the whole run. what: - Write non-ASCII literally, falling back to an escaped document for the one input that cannot be encoded - Carry the source file's trailing-newline convention across the rewrite, requiring the original bytes rather than defaulting them - Assert an unmodified config round-trips byte-identical across the shapes the agent CLIs write
why: The module docstring described use-local as rewriting configs to run a local checkout, which is now only half of what it does. A reader meeting the file for the first time would not learn --pr exists. what: - Name the pull-request form alongside the checkout form - Add it to the examples block
why: The summary line named the repo's checkout as the only outcome, so it read as false for the branch immediately below it. what: - State both targets, and which flag selects the second
why: The subcommand list is where someone discovers what use-local is for, and it named only the checkout. what: - Name the pull-request target in the subparser help line
why: The note claimed all six CLIs emit JSON.stringify output. Two of them, codex and grok, are TOML and never reach this writer at all. what: - Say JSON CLIs, which is the set the note is about
why: The branch carried two entries whose prose explained mechanism — worktree pruning, the initialize round trip, escape encoding — none of which a reader needs to decide whether the change matters to them. what: - Collapse them into one entry naming what the tool can now do and what it no longer does to a config
why: The per-CLI handler caught only RuntimeError, so a config that would not parse escaped as a traceback and took the whole run with it — the other CLIs never got their swap. The comment above it already claimed a clean per-CLI error, and doctor already caught the wider set. what: - Catch ValueError and OSError alongside RuntimeError in status and use-local, matching what doctor already does - Cover malformed JSON, a truncated document, and invalid UTF-8, and that one bad config does not stop the CLIs behind it
why: load_state parsed the file with a bare json.loads, so a truncated or hand-edited one raised through every command that reads it — revert and doctor included. Its own docstring already promised a hand-edited file could not crash the script. Returning empty silently would be its own trap: it means the record of every swap is gone, so revert would report nothing to unwind while swapped configs and their backups sit on disk. Naming the file is what lets someone go find those backups. what: - Degrade to no entries when the file will not parse, or holds a shape that carries none, and say so on stderr
why: The backup write sat between the two guarded blocks, so an unwritable config directory raised a PermissionError through the whole run and the CLIs behind it never got their swap. Aborting that CLI is the right half of the trade rather than swapping anyway: the backup is the only copy of the pre-swap config, so a swap that could not take one would leave nothing to revert to. what: - Catch the failure, name it per CLI, and move on to the next
why: --pr took any int, so a typo built a ref like refs/pull/-5/head and carried it as far as the preflight. Pull requests are numbered from one, so a non-positive value can only be a mistake. what: - Parse --pr through a validator that requires a positive number, matching how --env already reports a malformed argument
why: atomic_write staged beside and replaced the config path. A config symlink into a dotfiles checkout was therefore destroyed while its target stayed stale. what: - Resolve symlinks before staging so rename stays atomic at the target - Cover link chains and swap/revert recovery with sandboxed tests
why: Concurrent swaps and partial filesystem failures could orphan the pristine backup, lose recovery state, or restore through a repointed symlink. what: - Serialize mutations and write recovery state before config changes - Restore the original target while preserving file modes - Keep recovery material on failure and return nonzero when incomplete - Add adversarial coverage for races and filesystem failures
why: The unreleased note should summarize the branch's complete user-visible result without exposing implementation detail. what: - Lead with checkout-free pull-request testing and preflight - Summarize configuration preservation and recovery guarantees
`use-local --pr N` points every installed agent CLI at a pull request without creating a checkout and validates the MCP server before changing configuration. Configuration updates preserve unrelated text, file permissions, and symlink targets. Atomic recovery retains the original backup and state through concurrent or failed swaps, while incomplete recovery returns nonzero.
Freeze the mcp_swap and ruff work from the unreleased section into the dated 0.1.0a20 entry, add its lead paragraph, and open a fresh 0.1.x unreleased placeholder above it. Bump the package version 0.1.0a19 -> 0.1.0a20 across pyproject.toml and __about__.py, and refresh uv.lock. No tool behavior changes here, so the section carries only Documentation and Development entries. MIGRATION is untouched: it has no unreleased heading to retitle, and this release documents no breaking change.
why: Three things vary per CLI -- the file format, the key path to the server map, and the shape of one entry -- but only the format was recorded on CLIInfo. The other two were spelled as `cli in (...)` membership tuples repeated across get_server, set_server, delete_server and _all_server_specs. Two of those four dispatches end in a bare `else` that falls through to the TOML `mcp_servers` key, so a CLI registered in CLIS but forgotten in one tuple reports "no entry" instead of failing; the other two raise AssertionError, which the caller's (RuntimeError, ValueError, OSError) handler does not catch. what: - Add `container` (key path to the server map) and `dialect` (entry shape) to CLIInfo, both required so a new CLI cannot be added without deciding them - Replace the four membership dispatches with one `_server_map()` accessor that walks the key path and creates intermediates on demand - Extend the non-mapping guard Claude already had to every CLI: a container key holding something other than a table now raises RuntimeError naming the path, rather than a TypeError out of setdefault - Rename `to_json_dict(include_stdio_type=)` to `to_entry_dict(dialect)` and move the TOML table build behind `_as_toml_table()`, so the two writers no longer duplicate the entry shape No behavior change for the six registered CLIs; the existing 123 mcp_swap tests pass unmodified apart from the fixture gaining the two new required fields.
why: A config format the script cannot round-trip is one it must not write. tomlkit gives TOML a format-preserving round trip; JSON goes through stdlib json.dumps, which reserializes the whole document. For a JSONC file that is doubly wrong -- json.loads rejects `//` outright, and anything that did parse would come back stripped of every comment. The obvious dependency was measured and rejected. json-five round-trips comments via its model API, but it raises on the valid JSON string "C:\\x" and silently decodes the six literal characters \u0041 to "A". stdlib json reads both correctly. A parser that quietly rewrites a value nobody touched is the exact failure this script is built to prevent, so it is not worth a PEP 723 line. what: - Parse JSONC by blanking comments and trailing commas in place -- offsets preserved -- then handing the result to stdlib json, so escape semantics are the standard library's rather than a reimplementation's - Apply writes as text splices located by a string-aware scanner, one splice at a time with a rescan between, so every byte outside a replaced value survives untouched. Same technique opencode's own writer uses through jsonc-parser's modify() - Render short scalar arrays inline so a swapped `command` stays on one line instead of exploding a dotfiles-tracked config into a large diff - Dispatch dump_config_bytes on the exact format instead of `!= "json"`, which would have sent a third format to the TOML writer and put TOML bytes in a JSON file Verified byte-identical round trips for line and block comments, trailing commas, absent final newline, non-ASCII, `//` inside a URL, `/*` inside a string, Windows paths and a literal \u escape. No CLI uses fmt="jsonc" yet; the codec lands ahead of its first consumer.
why: opencode is the seventh agent CLI on this machine and the first
whose config differs from the others in all three axes at once: the file
is JSONC, the server map hangs off `mcp` rather than `mcpServers`, and
one entry packs argv into a single `command` array with its environment
table spelled `environment`. Getting any of that wrong is not a soft
failure -- a scalar `command` is a decode error that stops opencode from
starting at all, and an `env` key is dropped without a word.
what:
- Register opencode: binary `opencode`, `$XDG_CONFIG_HOME/opencode/
opencode.jsonc` (honouring XDG the way opencode's own loader does),
fmt jsonc, container ("mcp",), dialect opencode
- Add the opencode dialect to both directions: written as
{"type": "local", "command": [argv...]} with "environment", and read
back by splitting the array into the portable command/args pair
- Seed "$schema" when creating an entry in a config that was empty;
opencode writes that line itself on first load, so writing it here
avoids a second edit landing right after the swap
- Derive the detect column width from the longest registered name
instead of a hardcoded 7, which "opencode" overflows
Splitting the array on read is what makes `is_local_uv_directory`,
`local_repo_path` and `pr_ref` keep working, and those are what the
"already local -- no change" check depends on. Without it every run
would rewrite a config that was already correct.
Verified end to end against a sandboxed HOME/XDG_CONFIG_HOME: add,
replace, revert byte-identical, second-run idempotence, a comment living
inside the replaced entry, an existing `environment` table, an empty
file, a symlinked config, --pr, and status reading each shape back.
why: pi is the eighth agent CLI here, and the only one that ships no MCP
client. Its README says "No MCP" outright, the released 0.84.1 build
contains no MCP code, and its Settings interface has no key that could
hold a server. MCP reaches pi only through the third-party
`pi-mcp-adapter` extension, which reads ~/.pi/agent/mcp.json in the
Claude-Desktop `mcpServers` schema.
That leaves one honest way to support pi. This script's value rests on
`status` telling the truth about what an agent will actually run, so
writing a file pi ignores and reporting success would cost more than not
supporting pi at all. Registering the path and naming the missing
prerequisite keeps both: the swap lands where the adapter looks, and
`detect` says why it will not take effect yet.
what:
- Register pi: binary `pi`, ~/.pi/agent/mcp.json, fmt json,
container ("mcpServers",), standard dialect -- no new dialect needed,
the adapter speaks the same shape cursor and gemini do
- `detect` appends "needs the pi-mcp-adapter package; pi has no built-in
MCP client" whenever that package is absent from
~/.pi/agent/npm/node_modules
Verified end to end against a sandboxed HOME: detect's caveat, add,
status, and revert byte-identical, with an unrelated server left alone.
why: The two new CLIs introduce axes nothing in the suite exercised: a JSONC config, a container key that is neither mcpServers nor mcp_servers, an entry that packs argv into one array, and a config read by an extension rather than by the agent. The JSONC writer also makes a stronger promise than the JSON one -- it splices text, so it owes byte fidelity rather than only value fidelity, and that has to be asserted on bytes. what: - test_fake_home_covers_every_registered_cli: the fixture replaces CLIS wholesale, so a CLI missing from it raises KeyError out of half a dozen unrelated doctor tests. Names the invariant once - Registration and set/get/delete round-trips for both CLIs, which is what proves each name reached all four container branches - opencode dialect both directions: argv packed into one array, env written as "environment", and the array split back into command+args so is_local_uv_directory, local_repo_path and pr_ref keep working - Comment fidelity: line, block and trailing comments, a comment living inside the entry being replaced, sibling servers, symlinked config, $schema seeding, and a second swap reporting no change - PRESERVED_JSONC byte-identical round-trips, including `//` inside a URL, `/*` inside a string, a Windows path and a literal \u escape -- the cases that make a naive comment-stripper corrupt a value - A parity test asserting JSONC values match stdlib json wherever stdlib can parse the body at all Verified these fail for the right reason: disabling the JSONC writer so jsonc falls through to the plain JSON one turns 8 of them red, the comment and byte-fidelity ones included.
why: Eight places enumerate the agent CLIs, and they had already drifted apart before this branch -- scripts/README.md claimed four CLIs when six were supported, and its extension guide named three per-CLI branch sites when there were four. Adding two more CLIs without reconciling them leaves the docs describing a script that no longer exists. what: - Module docstring: line 6 is the argparse description, so it no longer tries to list every CLI by name. The Scope section gains the two new config paths, opencode's three-sibling-global-files caveat, and pi's missing MCP client - scripts/README.md: the CLI table now lists all eight with their formats, and the extension guide describes CLIInfo's fmt/container/ dialect fields instead of branch sites that no longer exist. Adds the ALL_CLIS warning -- a CLI missing from it has its state dropped on load, so revert forgets the swap - docs install widget: an opencode panel. `opencode mcp add tmux -- <cmd>` is non-interactive given a name and a `--` command, so it is a CLI panel; that also avoids its array-command shape, which the shared JSON body cannot express. _cli_body falls through to codex by default, so the branch is explicit - Skill and cli-matrix: opencode added to the skill's CLI list and both new CLIs described from source. Their matrix row reads "not yet verified" rather than guessing -- that file's value is that every cell was empirically confirmed, and neither has been driven through the harness - justfile: the mcp-detect comment listed four CLIs; it now names none - CHANGES: entries under Development for the swap-script work, and under Documentation for the install-widget panel pi is deliberately absent from the install widget and has no matrix row: it cannot consume MCP, so there is nothing for a user to install into.
why: CI runs `uv run mypy .`, which covers scripts/; the chain in AGENTS.md is `uv run mypy src tests`, which does not. The opencode work was typed against the narrower invocation and broke the build. what: - Annotate the opencode entry dict, which lost its `dict[str, t.Any]` when the dialect branch was added and was then inferred narrowly enough that assigning `environment` failed - Overload `_server_map` on `create`, matching `_claude_project_node` and `_claude_user_servers`, so a create=True call is not Optional at the call site - Annotate its cursor so the walk returns a mapping rather than Any `just mypy` type-checks every .py file and would have caught this; `uv run mypy src tests` is the invocation that does not.
why: The insertion branch asks whether an object already has content by looking at the comment-blanked text, where a comment is indistinguishable from whitespace. An object holding only a comment therefore looked empty, and the insert spliced over the whole interior and took the comment with it -- silently, in a file the user wrote by hand. what: Measure the interior in the original text and anchor the splice after what it actually holds. A genuinely empty interior rstrips to nothing and the anchor collapses to the old splice point, so every previously working insert is byte-identical. Covers the same splice at the document root, where there is no enclosing member, and adds the comment-only object to the byte-fidelity cases.
why: Removing a member spliced from the end of the previous member to past the following comma, so a member between two others took the comma on both sides and left its neighbours undelimited. The next merge pass then raised JSONDecodeError, which the caller catches as a bad config, so the swap reported opencode unreadable and skipped it. Reachable without doing anything unusual: an entry carrying `enabled` or `timeout` -- both valid opencode fields the swap does not write -- hits it. what: Take exactly one delimiter with the member. Every member but the first takes the comma before it; the first takes the comma after. Read that comma out of the blanked text, so a comma inside a comment is not mistaken for the separator and a real one behind a comment is still found. Chosen over two larger alternatives after both were built and measured: across 5,508 generated documents this and a helper-based rewrite emitted identical bytes, and a third approach that also preserved the deleted member's comment corrupted files -- it stripped the newline terminating a `//` comment, pulling the closing brace inside it. A comment sitting above a removed member is still removed with it. That is unchanged, and settling it means first deciding whether such a comment documents the member or the object; re-parenting it onto the next member would leave a false statement in the user's file.
why: pi's MCP file is read by pi-mcp-adapter, which parses it through strip-json-comments with trailing commas allowed. Registering it as fmt="json" sent it to strict json.loads, so a config the adapter reads without complaint came back as a JSONDecodeError and status and use-local reported pi unreadable and skipped it. The .json suffix is misleading; the format the reader accepts is JSONC. what: fmt="jsonc". The container key and entry dialect are unchanged -- the adapter speaks the same Claude-Desktop mcpServers shape cursor and gemini do. Comments and a trailing comma now survive a swap as well.
why: The panel offered Project alongside User and named `./opencode.json` as its destination, but emitted the same command for both. `opencode mcp add` resolves its target with resolveConfigPath(Global.Path.config, true) on the non-interactive path, so it writes the global file whichever scope was picked. A reader following the Project panel would register the server for every project while believing it was scoped to one repo. what: opencode offers User only. The prose that pointed at `opencode mcp add` for workspace precedence is corrected in the same pass -- that command cannot reach a project file; editing `$PWD/opencode.json` by hand can.
The CLI table and the scope note still called it JSON, which is what the suffix says and not what the adapter reading it accepts.
why: The insertion path built the member with an f-string, so the key went in raw while every value went through json.dumps. `--server` takes an arbitrary string: give it one holding a backslash, a quote, or a newline and the emitted text does not parse back. The member is then never found on the next pass, so the merge re-inserts it until the pass ceiling -- burning CPU for over an hour while holding the exclusive swap lock, then failing with "JSONC merge did not converge". what: Render the key with json.dumps, honouring the same ensure_ascii the values use. Found by exercising the flag surface rather than the config surface; the config-shape matrix passes either way because a derived server name never contains one of these characters.
e955511 documented RETRY_TIMEOUT_SECONDS as the tuning knob for timeouts under load. It is not one here. Counted by walking the AST, all 77 retry_until call sites in this suite pass a timeout explicitly -- 73 of them the literal 10 -- so none reads the environment variable. Setting it would change nothing at all. A regex cannot count these: the predicate is usually a lambda carrying its own parentheses, so a naive match truncates and files sites that DO pass a value as defaults. The number that falsified the original attribution was already in the data. The reported symptom was "the 8s budget plus overhead" at 10.26s, a 28% fudge, while the observed first attempts clustered at 10.07, 10.15, 10.26, 10.96 -- at 10, never at 8, which is the literal rather than the default. No change is indicated, only an accurate note. The bound is a ceiling, not a spend: the polls complete in 1.4-4.3s, so 2-7x margin, and both observed failures needed loadavg above 200 on a 20-core box. CI runs far lower parallelism and has never shown it. The rerun caveat in the same section is independent and stands.
$TMUX names only the INNERMOST server. Run an agent inside tmux and point it at a second tmux, and the pane hosting its terminal belongs to the OUTER server while $TMUX describes the inner one -- so every socket comparison in the guard said "different server" and a kill of that pane was permitted, taking the caller's tty with it. That is the self-kill the guard exists to prevent. Reproduced on 3.7c: guard against the inner server True, against the outer server False. Reachable rather than theoretical: list_servers enumerates every socket, so a nested agent sees the outer one and can target it. The fix asks WHO IS ATTACHED rather than how the nesting arose. A client of the caller's own server occupies a pane of whatever hosts it, so the inner server's client_tty is the outer server's pane_tty -- measured, both /dev/pts/50. That covers a server merely attached to as well as one started from a pane, and needs no /proc, which macOS does not have; a process-tree walk would have missed the first case and silently protected only Linux. A hung probe fails closed, matching the guard's bias. A nonzero exit does not: "no such server" is an ANSWER -- one that is gone hosts nothing -- and treating it as unknown would block every destructive call for a caller whose $TMUX names a socket that has since died. That distinction is what keeps the existing unrelated-socket test passing. The regression test carries a control: an unrelated third server must stay killable, or the guard has merely stopped answering. Verified to fail on exactly the outer-server assertion with the check disabled.
The tool that destroys every session on a server had no functional test. Everything referencing it asserted its NAME sits at the right safety tier; nothing asserted it kills, or that it refuses. Both halves now. It kills a throwaway server, checked by asking the server rather than reading the return string -- a tool that answered "Server killed successfully" and killed nothing would have passed on the message alone. And it refuses when the caller is on the target, checked by the server still being alive afterwards. The refusal test takes mcp_session, not just mcp_server: the bare fixture constructs an UNSTARTED Server, so "is it still alive" answers False whether or not the kill happened. The first version of this test failed on exactly that and would otherwise have passed for the wrong reason once the assertion was inverted. Closes one of the gaps the QA instance declined -- correctly -- to test against its own live socket. A throwaway server carries no such risk.
"The test failed when I broke it" is a claim about the mutation chosen, not about the test. A multi-assertion test needs one mutation per line: the first failing assertion stops the test, so everything after it is never reached and stays unproven. Shown on kill_server's refusal test: disabling the guard falsifies the raises block and never reaches assert mcp_server.is_alive(). Only a guard that raises the RIGHT error and kills anyway falsifies that one -- so the liveness check is load-bearing, catching a tool that refuses in words and kills in fact. Applied back to the nested self-kill test, which has three rows. Removing the nesting check falsifies the OUTER row. Making it return True unconditionally falsifies the CONTROL row, so that control does catch a guard that has stopped answering. Breaking the primary realpath match falsifies NOTHING -- the inner row is satisfied by a fallback route and does not isolate that path; matches_realpath covers it instead. No behaviour changes. Both docstrings now say which mutation reaches which line, rather than leaving a reader to assume every assertion earns its place.
CHANGES lists five breaking changes for this release; MIGRATION documented three. The two missing were both added this session -- the name/title literal change and the refusal of option names containing '#' -- so the changelog promised a migration note that did not exist. Both now carry the same before/after shape as their neighbours, including the start_directory half of the first one, which is the case most likely to bite quietly: a '#' in the path expanded, the result did not exist, and the shell started in $HOME instead.
…constraint server.py, middleware.py, _utils.py, _tmux_proc.py and _progress.py. Removes fixed-bug history, rejected-alternative arguments, two issue references and a local filesystem path; keeps every tmux quirk, ordering invariant and failure mode. The redaction allowlist keeps its security boundary and its "defaults to log it" warning.
pane_tools, wait_for_tools, hook_tools, batch_tools and the rest. Cuts fixed-bug narration, rejected-alternative arguments and one issue reference; keeps every tmux-version quirk, the wait-for exit-status table and the copy_selection crash conditions. Also relocates a stale paragraph: _RESPAWN_PID_SECONDS carried the measurement for _RESPAWN_COMMAND_SECONDS (26.4ms, an order of magnitude under 0.25) while its own bound is 5.0. Both ship to the API docs.
…egisters Second pass over tests/ and scripts/mcp_swap.py, plus fixes from a peer review of the two commits before it. Six restorations. Four were registers that read as rationale but worked as decision records -- the redaction allowlist's decided-and-logged exemplars, the alwaysLoad doc URL and version floor, MAX_BATCH_OPERATIONS, and the 300s/RuntimeWarning symptom. The warning each one supported had stopped pointing at anything. Two were scope markers whose loss inverted a claim: server.py dropped "would otherwise" and so asserted the failure SafetyMiddleware exists to prevent, and _tmux_proc.py credited asyncio.wait with a kill that _kill_and_reap performs, losing the without-cancelling property that is the reason to prefer it over wait_for. Also drops a dead 14-line block documenting _EMIT_AFTER_BASELINE_SECONDS, removed in 1cf5cf1 when the tests moved to synchronising on arming, and relocates the _BLOCKING_TMUX_HELPERS doc from the constant above it.
The module docstring ships to the API docs, so the example now uses a generic path instead of the tree it was written in.
Second peer review of the tool-module and test commits. The sharpest was a hedge compressed into a false absolute: the record- separator delimiter was documented as one a path "cannot" contain, when U+241E is an ordinary printable glyph and POSIX admits any byte but NUL and slash. The original argued relative likelihood, which is the honest form; verified by creating the file. Same shape as the "would otherwise" inversion last round -- in both, a hedge was carrying the truth. Restores nine citations (cmd-show-environment.c, cmd-find.c, utf8.c, grid.c, screen.c, cmd-capture-pane.c, Lib/sched.py twice, lifecycle.py). The principle, since none was stated: a pointer is what makes a claim checkable, and four retained claims in this tree were verified against tmux 3.7c in minutes because theirs survived. cmd-capture-pane.c comes back by line number without the "post-tmux-3.0" attribution, which the clamp at :205-206 predates and which this pass cannot substantiate. Also restores the linking sentence in wait.py that made list-vs-set a consequence of including the entry cursor row rather than a third unrelated rule, the KeyError half of the environment pathology, the swept-matrix note on the copy_selection crash table, and three magnitudes: MAX_BATCH_OPERATIONS at its third site, 1785 sockets, and a 28 ms margin that separates a coin flip from a caught defect.
why: Buffer GC made a tmux round trip per cached server with an iterator open on `_server_cache`, and the paired `clear()` ran unlocked -- the only two unlocked accesses to it in the tree. A tool call caching a server inside that window raises "dictionary changed size during iteration" out of shutdown. Reproduced deterministically with a stub whose `cmd` writes to the cache; the interpreter under test is a free-threading build, so the race needs no timing luck. what: - Add `_drain_server_cache`, emptying the cache under the lock and returning what it held - `_gc_mcp_buffers` takes the resulting snapshot, so its tmux round trips stay off the lock, matching the measured rationale in `_get_server` for never holding it across a subprocess - Drop `_server_cache` and `_ServerCacheKey` from `server.py`, which no longer reaches into the cache module's internals
why: The screen that refuses uninterruptible patterns walked the parsed regex through three separate hand-maintained lists of container ops, and none of them listed `ASSERT`, `ASSERT_NOT` or `GROUPREF_EXISTS`. `(?=(a+)+$)b` passed the screen and then ran past a 2 s timer on a 60-character subject. `search_panes` carries this at readonly tier, which is the level a cautious operator picks. Variable-width lookbehind is not a vector: `re` refuses it before the screen is reached, so only the lookahead and conditional forms could carry a bomb. what: - Add `_subpatterns(op, av)`, the single table of nested pattern sequences that all three walkers now recurse through - `_first_characters` treats an unmodelled op as "assume it overlaps" instead of skipping past it, which drops the `IN`/`ANY`/`NOT_LITERAL` special cases - Cover the leaking shapes, and lookarounds an agent would really write, in the existing pattern tables
why: The unlocked cache walk shipped in 0.1.0a19, so an operator can have hit it. The regex-screen change on this branch is not a separate entry -- the screen itself is unreleased, and its deliverable already states the rule that fix makes true everywhere. what: - Add a `### Fixes` deliverable for the teardown crash
why: `_bounded_io` needed the pane-state format and parser, and reached them with three function-level imports because a module-level one cycles: the tool package's `__init__` re-exports `io`, `meta` and `capture_since`, each of which imports `_bounded_io` back. Hoisting one of the three reproduces it as an ImportError. The module was never a tool. It holds a NamedTuple, two format constants and their parsers, imports nothing from any tool, and is read by the bounded-IO layer and four pane tools alike -- so the cycle came from where the file sat, not from what it did. what: - Move `tools/pane_tools/state.py` to `_pane_state.py` - Hoist the three deferred imports; no runtime deferred import is left in `_bounded_io` - Guard the layering in `test_package_metadata`: no `libtmux_mcp/_*.py` may import from `libtmux_mcp.tools`. Shown failing on an injected import, since a function-level one costs no linter or type checker anything
why: One 2167-line file held eight unrelated concerns -- the error type and its decorators, safety tiers, argument guards, tmux exec, caller identity, the server cache, object resolution, the filter engine and serialization. 26 modules drew 46 symbols from it, so "where does this live" had one answer for all of them and reading any one concern meant scrolling past the other seven. It already knew its own seams: the concerns sit in contiguous line ranges, with two `# ---` dividers left in as evidence. The split is those slices, so no function body changed. The result is a DAG, checked by hoisting every import to module scope: `_serialize` -> `_caller` -> `_exec` -> `_errors`, `_resolve` -> `_servers` -> `_exec`. Nothing imports `_filters` or `_safety` but the tools. what: - Replace `_utils.py` with `_errors`, `_safety`, `_guards`, `_exec`, `_caller`, `_servers`, `_resolve`, `_filters`, `_serialize`, each 109-359 lines - Rewire 36 files by symbol; no compatibility shim, since the names were private and nothing outside the package could bind them - Point monkeypatch targets at the module whose namespace the lookup runs in: `_run_tmux_sync` is patched on `_servers`, where `_probe_liveness` resolves it - Replace the `utils` autodoc page with `internals`, ordered by the dependency layering, and update the architecture module map
why: The file named a module that no longer exists, and at 1502 lines it was the only route to tests for nine separate concerns -- a reader looking for the caller-identity tests had nothing to search for. Collected count is unchanged at 1181, so the split moved tests rather than dropping any. what: - Replace `tests/test_utils.py` with nine files mirroring the source modules, each carrying its own fixture classes - Move the `FakeServer` stub to `tests/conftest.py`, now that argv and liveness tests both need it, matching the existing `from tests.conftest import wire_annotations` convention
why: `_SENSITIVE_ARG_NAMES` is a deny-list over a log that records what it is given, and the module comment already names the consequence: a free-text argument added later is exposed by omission. Nothing enforced it, so the failure mode was silence. Enumerating the current surface found no leak -- all 31 logged string arguments are routing metadata, object names, paths or enums -- so this pins a property that holds today rather than fixing one that does not. what: - Add `_AUDIT_LOGGED_STRING_ARGS`, the set deliberately recorded verbatim, and assert every string parameter on every tool sits in it or in `_SENSITIVE_ARG_NAMES` - Shown failing on an injected `audit_probe_note` parameter, naming it and both remedies
why: The wall-clock assertion was a literal 10 s for a 1 s clamped wait, which is an assumption about machine speed wearing a correctness assertion's clothes. It failed once in a 151 s suite run and passed in a 91 s one; the traceback was lost, so this replaces a guess rather than fixing a diagnosed failure. Server acquisition runs before `start_time` is set, so its liveness bound is spendable ON TOP of the ceiling, and the poll loop can overshoot by one tick of two bounded reads. The budget is now that sum. It still separates a working clamp from a broken one by two orders of magnitude, which is the property the assertion exists for. what: - Compute the bound from `_LIVENESS_TIMEOUT_SECONDS` and `_TMUX_CALL_TIMEOUT_SECONDS` instead of a literal, and report both sides on failure
why: `run_command` read the exit status with `show-option`, which tmux has never registered. It resolves only through unambiguous-prefix matching -- `show-opt` and `show-optio` work identically, measured on 3.7d -- so the call survives on the accident that no other command starts with those letters. A future `show-option-...` makes it ambiguous, and the failure lands on the exit-status read, turning every `run_command` into "could not read exit status". Audited the rest of the tree against tmux 3.2a's command table: this was the only one of 28 hyphenated literals that was not an exact name or alias. `-p` is in 3.2a's `show-options` arg string, so the exact name spans the supported range. what: - `show-option` -> `show-options`
why: The screen modelled two catastrophic shapes and `(a?){20}b`
was neither, so it compiled and then backtracked exponentially --
0.76s on a 27-character line, and `(a?){20,}b` did not finish. Found
by an independent review of the previous commit, which is the honest
provenance: the traversal fix closed the hole it aimed at and left
this one.
The predicate is the repeat's MINIMUM, not its maximum. `re` breaks a
loop whose body matched nothing, and any repeat that can stop early
reaches that exit; one owed 20 iterations cannot, so it branches
consume-or-not at each. Measured on 27 characters: `(a?)*b` and
`(a?){1,20}b` 0.00s, `{15}` 0.03s, `{20}` 0.76s, `{20,}` unbounded.
The threshold is 8, which leaves three orders of magnitude of headroom
and still admits every near-miss above.
what:
- Add `_matches_empty`, and refuse a repeat whose minimum reaches
`_LARGE_MINIMUM` over a body that can match nothing
- Treat only LITERAL, NOT_LITERAL, IN, ANY and GROUPREF as consuming;
an unmodelled op counts as nullable, which can only refuse more and
is reached only under an already suspicious repeat
- Say in the module docstring that the three shapes are a model of
catastrophic backtracking, not a proof of its absence. The previous
wording claimed ambiguous repeats are refused, full stop
- Cover the four leaking forms, and the five near-misses that show the
rule discriminates on the minimum rather than blanket-refusing
why: The onboarding doc pointed at `_utils.py`, which this branch removed, and described `pane_tools.py` as one file when it has been a package for far longer than that. An agent following it looked for code that is not there. Raised by an independent review of the split, which noted the Sphinx side got every cross-reference and this did not. what: - Replace the Utils entry with the ten internal modules in dependency order, and state the rule that none may import from `tools` - List `pane_tools/` as a package and name its submodules - Add the tool modules the map never had: buffer, hook, wait_for, batch
why: The existing guard covers core-to-tools only, so a cycle among the core modules themselves would be caught by nothing: the workaround is a function-level import, which costs no linter or type checker anything and therefore survives the whole gate chain. what: - Walk the `libtmux_mcp/_*.py` import graph and fail on a cycle, reporting the path. Shown failing on an injected `_errors -> _exec -> _caller -> _serialize -> _errors`
why: The screen is unreleased, so its own gaps are branch-internal and get no `### Fixes` entry -- but the deliverable that WILL ship has to describe what it actually does. It enumerated two shapes and now refuses three, and it claimed a completeness it does not have. what: - Add the nullable-body shape, say the shapes are refused inside a lookahead or conditional too, and say plainly that they are a model rather than a proof - Add two more patterns to the "still compiles" list, including the near-miss `(a?)*`
why: Fuzzing the previous screen found 27 patterns in 3971 accepted
that still did not finish -- `(a{0,3})*b`, `(a?a?){1,20}$`,
`(a{0,3}a)*$` and relatives. Each was a repeat over a body that can
match a VARYING number of characters, which the shape list did not
name: `a{0,3}` is not "large", so the nesting rule stayed silent, and
its minimum is 0, so the nullable rule did too.
Measured the boundary rather than guessing it. Under a large repeat,
`a{2}` and `ab` are safe (one way to split), `a{1,2}`, `a{0,2}` and
`a{0,3}` are not, and `a{0,1}` is safe only because CPython breaks a
loop whose body matched nothing. Width is the predicate; the two
previous rules were special cases of it.
Two fuzz runs, different seeds and grammars, ~5700 patterns each:
27 leaks before, 0 after and 0 after. Refusals rise about 5%.
what:
- Replace `_contains_large_repeat` and `_matches_empty` with
`_width_range` / `_is_variable_width`; the module gets shorter
- Refuse a freely iterating repeat whose body varies in width, keeping
the overlapping-alternatives rule for a fixed-width body like `(a|a)+`
- Add `CONSERVATIVELY_REFUSED`: `(a?)*` and friends are refused though
CPython prunes them, because a screen that leaned on that pruning
would have to know when it stops applying
- Fix a test row that read `r"...\\w+"`, matching a literal backslash
rather than a word character
why: The entry enumerated three shapes; the screen now tests one property those were special cases of, and refuses a little more than CPython strictly requires. The deliverable has to say what ships. what: - State the varying-width rule and the fixed-width patterns it admits - Say plainly that `(a?)*` is refused though CPython finishes it
why: Exact tmux 3.7 expands break-pane's window name unsafely and the wrapper trusted a mutation before checking its command result. what: - Dispatch break-pane directly and check its result - Use a fixed 3.7 placeholder followed by an escaped rename - Re-resolve the pane and report post-mutation uncertainty - Cover hostile names and exact-version command shapes
why: The old utility API destination no longer exists, leaving its published redirect broken. what: - Redirect the legacy utility URL to the internals reference
Why: The tmux matrix ran live copy-selection cases on releases where the tool deliberately refuses to execute for server-safety reasons. What: Mark only the eight live copy-selection cases as requiring tmux 3.4 while retaining the older-version refusal coverage.
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.
Summary
capture_sincecontinuity across reflow and floods while makingwait_for_texthandle repeated lines, stops, pane death, and progress accurately.Review note
This proposal includes every commit currently reviewed in #125 plus the later
improvements-00work. A new main-targeted PR overlaps that open PR.Compatibility
Test plan
git diff --check.mainbranch.break_panefailure before rerunning the matrix.