diff --git a/.agents/skills/testing-mcp-with-cli-agents/SKILL.md b/.agents/skills/testing-mcp-with-cli-agents/SKILL.md index 5a59556c..ab0ca476 100644 --- a/.agents/skills/testing-mcp-with-cli-agents/SKILL.md +++ b/.agents/skills/testing-mcp-with-cli-agents/SKILL.md @@ -2,7 +2,8 @@ name: testing-mcp-with-cli-agents description: >- Test an MCP server by driving real CLI agents (Claude, Codex, Cursor, Gemini, - Grok, agy) against it, using isolated tmux sockets and send-keys instead of + Grok, agy, opencode) against it, using isolated tmux sockets and send-keys + instead of trusting unit tests alone. Use this whenever verifying MCP-server behavior end-to-end, checking that a local branch or checkout works across installed agent CLIs, comparing trunk-vs-branch MCP behavior, driving an interactive @@ -195,7 +196,17 @@ transcripts), and the **ground-truth socket state** after the run. ## Wiring a checkout into the CLIs: mcp_swap `scripts/mcp_swap.py` rewrites each CLI's config to `uv --directory run -` and preserves existing env on replacement: +` and preserves existing env on replacement. It covers eight CLIs; the +two newest are not yet driven through this harness, so +`references/cli-matrix.md` has no verified row for them: + +- **opencode** — `$XDG_CONFIG_HOME/opencode/opencode.jsonc`. JSONC, so + comments survive a swap; the entry packs argv into one `command` array + under a top-level `mcp` key, and its env table is spelled `environment`. + A scalar `command` there is a decode error that stops opencode starting. +- **pi** — `~/.pi/agent/mcp.json`. pi ships no MCP client of its own; that + file is read by the third-party `pi-mcp-adapter` extension, so a swap + does nothing until it is installed. `detect` reports this. ```console $ uv run scripts/mcp_swap.py detect # which CLIs are present diff --git a/.agents/skills/testing-mcp-with-cli-agents/references/cli-matrix.md b/.agents/skills/testing-mcp-with-cli-agents/references/cli-matrix.md index 2c29da9d..b4763dfd 100644 --- a/.agents/skills/testing-mcp-with-cli-agents/references/cli-matrix.md +++ b/.agents/skills/testing-mcp-with-cli-agents/references/cli-matrix.md @@ -75,9 +75,28 @@ below. | gemini | `gemini -p` | project `.gemini/settings.json` from cwd | `gemini mcp list` | `--approval-mode yolo` (`--skip-trust`) | no — `IneligibleTierError`, CLI unsupported for individuals | | grok | `grok -p` / `--single` | `GROK_HOME` **or** `mcp add --scope project` | `grok mcp doctor tmux --json` (real handshake) | `--permission-mode bypassPermissions` | yes | | agy | `agy -p` | hidden `--gemini_dir ` (**credentials do not follow it — copy the token in**) | none short of a model call | `--dangerously-skip-permissions` | yes | +| opencode | not yet verified | not yet verified | not yet verified | not yet verified | not yet driven through this harness | +| pi | n/a — no MCP client (see below) | n/a | n/a | n/a | n/a | ## Per-CLI detail +### opencode and pi — registered in mcp_swap, not yet driven here + +`mcp_swap` writes both, but neither has been taken through the tmux harness, so +the row above is blank rather than guessed. What is known from the source: + +- **opencode** stores MCP servers under a top-level `mcp` key in + `$XDG_CONFIG_HOME/opencode/opencode.jsonc`, as + `{"type": "local", "command": [argv...], "environment": {...}}`. `command` is + one array, not a command/args pair, and the env table is `environment` — an + `env` key is dropped in silence, while a scalar `command` fails the whole + config's decode and stops opencode starting. `opencode mcp add -- ` + is non-interactive once both a name and a `--` command are given. +- **pi** has no MCP client at all: its README says "No MCP", and the released + build contains no MCP code. `~/.pi/agent/mcp.json` is a convention of the + third-party `pi-mcp-adapter` extension. Until that package is installed, + nothing reads what a swap writes, and there is no agent behavior to drive. + ### codex — two isolation styles, both verified - **Config-less (leanest):** a home dir containing only a **copy** of the real `auth.json`, no `config.toml`, plus `-c` overrides: diff --git a/.github/dependabot.yml b/.github/dependabot.yml deleted file mode 100644 index d202a332..00000000 --- a/.github/dependabot.yml +++ /dev/null @@ -1,7 +0,0 @@ -version: 2 -updates: - - package-ecosystem: "github-actions" - directory: "/" - schedule: - # Check for updates to GitHub Actions every week - interval: "weekly" diff --git a/.github/workflows/docs.yml b/.github/workflows/docs.yml index 2c76adf1..2f067317 100644 --- a/.github/workflows/docs.yml +++ b/.github/workflows/docs.yml @@ -40,7 +40,7 @@ jobs: - name: Install uv if: env.PUBLISH == 'true' - uses: astral-sh/setup-uv@v9.0.0 + uses: astral-sh/setup-uv@v10.0.1 with: enable-cache: true diff --git a/.github/workflows/tests.yml b/.github/workflows/tests.yml index ace9b38d..b7394e9c 100644 --- a/.github/workflows/tests.yml +++ b/.github/workflows/tests.yml @@ -16,7 +16,7 @@ jobs: - uses: actions/checkout@v7 - name: Install uv - uses: astral-sh/setup-uv@v9.0.0 + uses: astral-sh/setup-uv@v10.0.1 with: enable-cache: true @@ -102,7 +102,7 @@ jobs: - uses: actions/checkout@v7 - name: Install uv - uses: astral-sh/setup-uv@v9.0.0 + uses: astral-sh/setup-uv@v10.0.1 with: enable-cache: true diff --git a/.tool-versions b/.tool-versions index d03a9772..69ad986d 100644 --- a/.tool-versions +++ b/.tool-versions @@ -1,3 +1,3 @@ just 1.58.0 -uv 0.12.1 +uv 0.12.5 python 3.14 3.13 3.12 3.11 3.10 diff --git a/AGENTS.md b/AGENTS.md index 441dd724..471e4b93 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -416,46 +416,38 @@ mention can carry the link. Leave command examples, code blocks, Mermaid node labels, and literal configuration values as code; link the surrounding prose instead. -### Code Blocks in Documentation - -When writing documentation (README, CHANGES, docs/), follow these rules for code blocks: - -**One command per code block.** This makes commands individually copyable. For sequential commands, either use separate code blocks or chain them with `&&` or `;` and `\` continuations (keeping it one logical command). - -**Put explanations outside the code block**, not as comments inside. - -### Shell Command Formatting - -**Use `console` language tag with `$ ` prefix.** +### Code Blocks + +Code blocks are paste-and-run units: pasting one block runs exactly one +intended action. Doctests and other executed examples are exempt — the test +suite runs them, nobody pastes them. + +- **One command per block.** Multiple steps may share a block only when + explicitly chained with `&&`, `;`, or `\` continuations — the chain is + then one logical command. +- **Explanations go in prose above the block**, never as `#` comments inside it. +- **Command menus are per-command blocks with prose lead-ins**, not tables. +- **Shell commands use the `console` tag with a `$ ` prefix.** This separates + interactive commands from scripts and enables prompt-aware copy. +- **Split long commands with `\`** — one flag or flag+value pair per indented + continuation line, positional arguments last. Good: -```console -$ uv run pytest -``` - -Bad: - -```bash -uv run pytest -``` - -**Split long commands with `\` for readability.** Each flag or flag+value pair gets its own continuation line, indented. Positional parameters go on the final line. - -Good: +Show the last ten commits as a graph: ```console -$ claude mcp add \ - --scope user \ - tmux -- \ - uv --directory ~/work/python/libtmux-mcp \ - run libtmux-mcp +$ git log \ + --max-count=10 \ + --graph \ + --oneline ``` Bad: ```console -$ claude mcp add --scope user tmux -- uv --directory ~/work/python/libtmux-mcp run libtmux-mcp +# Show the last ten commits as a graph +$ git log --max-count=10 --graph --oneline ``` ### Changelog Conventions @@ -522,6 +514,90 @@ When stuck in debugging loops: - FastMCP: https://github.com/jlowin/fastmcp - MCP Specification: https://modelcontextprotocol.io/ +## Comments earn their maintenance cost + +A comment ships only if it passes all three gates. Fail any: delete or rewrite. +Borderline: delete — borderline means the information is reconstructible, which +is what makes deletion cheap. + +**Loss.** Three years from now, would losing this cost a maintainer real time +rediscovering intent, an invariant, a constraint, or a failure mode the code and +tests do not already make obvious? + +**Elite.** Would SQLite, Redis, the Go standard library, or CPython write this +comment, at this length? Those projects state the constraint and stop. They do +not argue with an imagined objector. + +**Upkeep.** Will it stay true without maintenance? A comment that hand-syncs a +value the code owns — a count, an offset, a line reference, a duplicated +constant — is false the first time that value moves. + +### Ceiling + +One or two lines. A comment reaching four is either carrying several facts, in +which case split it, or arguing, in which case cut it to the fact. + +Rationale, alternatives weighed, and the story of how the code got here belong +in the commit message: timestamped, attached to the exact diff, and free to +maintain. + +A comment often holds both a constraint and the deliberation that found it. Keep +the constraint, cut the deliberation. "Runs at most once per second" survives; +"this is the right trade for now" does not. + +### Keep + +- Why over how: upstream quirks, protocol and compatibility constraints, + performance tradeoffs still part of the contract. +- Invariants, preconditions, ordering, lifetime, and concurrency requirements + that types and tests cannot express. +- Code that looks wrong but is not, so a later cleanup does not reintroduce the + bug. +- A high-level sketch of an algorithm whose local operations do not reveal the + whole. + +### Delete + +- Narration of the next lines; code translated into English. +- Restated names, types, defaults, or control flow. +- Values duplicated from the code and hand-synced. +- Justification, hedging, or apology for a choice. +- Speculation about future requirements. +- History version control already holds, including commented-out code. +- Ticket and issue numbers. They say nothing to a reader without tracker access, + and they rot when the tracker moves. Unfinished work goes in the tracker, not + the source. +- Transient observations — "currently", "for now", "the latest release" — + that go stale with no nearby edit. + +### The upkeep gate in practice + +It reaches values that track our own code. It does not reach frozen external +facts. + +Bad (Delete): + +```python +# There are 321 tests to complete for servers. +``` + +Good (Keep): + +```python +# tmux < 3.2 reports the pane ID only after the command completes, +# so this query must stay separate. +``` + +### Documentation exception + +Doctests, minimal usage examples, and param, return, and raises lines on public +API are exempt from the loss gate — they serve the caller, not the maintainer. +They are exempt from nothing else. Ceiling: a good man page entry. + +NumPy-style `Parameters`, `Returns`, and `Attributes` sections and executable +doctests fall under this exception — autodoc ships every field whether or not +you describe it, and a doctest that runs is also a test. + ## AI Slop Prevention Treat AI slop as **review-hostile noise**, not as proof that text or @@ -579,8 +655,9 @@ on unrelated code while still resolving. ### Preservation & Context -**When unsure, leave the text in place and ask.** Subjective cleanup -must never be a reason to remove load-bearing rationale. +Subjective cleanup must never remove load-bearing rationale. Adjudicate +comments with the comment policy above; borderline cases are deleted, not +kept. - **Preserve the "Why":** You MUST NOT delete comments that document invariants, protocol constraints, platform quirks, security diff --git a/CHANGES b/CHANGES index bd33a90a..82533ee0 100644 --- a/CHANGES +++ b/CHANGES @@ -6,6 +6,940 @@ _Notes on upcoming releases will be added here_ +### What's new + +#### `list_servers` is 6.7x faster, and concurrent calls stop queueing + +`list_servers` probed each live socket with its own tmux subprocess, +serially. It is strictly linear in live servers -- about 12 ms each -- +so a box with 119 of them spent **1.5 seconds** in the tool the server +instructions point at for discovery, which makes it the first call a +client makes. Every other read-only tool measures 12-58 ms. + +The probes are independent, so they now run in a pool of 16. +`ThreadPoolExecutor.map` preserves input order, so the listing stays +sorted by socket name however the probes interleave. + +That alone only bought 2x, and the reason was somewhere else entirely. +`_get_server` held the module's cache lock across `Server.is_alive()` +-- a tmux subprocess round trip -- so every concurrent tool call in the +process queued behind one another for the duration of a subprocess. The +liveness check now happens outside the lock, and two threads racing to +cache the same key agree via `setdefault`. + +That second fix is not about `list_servers`. Any concurrent use of this +server was serialised at that lock: a `call_readonly_tools_batch`, two +agents sharing a server, a wait in one pane while another is queried. + +Measured at 40 live servers, same box: 536 ms serial, 176 ms at 4 +workers, 80 ms at 16, 76 ms at 32. 16 takes the win and 32 buys +nothing. Stale entries were never the problem and still are not: 200 +extra non-socket files cost about 8 ms total. + + +#### `tmux://panes/{pane_id}` works past pane 9 + +A tmux pane id starts with `%`, and the URI layer percent-decodes every +captured template parameter before a handler sees it, so `%10` arrived +as byte `0x10` and matched nothing. `%0` through `%9` survived only by +accident -- one trailing hex digit is an *invalid* escape and passes +through unchanged. So the resource surface worked right up until a +server created its eleventh pane, while the tool surface answered +correctly for the same pane. An agent got different answers depending +on which surface it used. + +The obvious repair does not work and is worth recording so nobody +reaches for it: re-encoding the decoded byte back to `%XX` handles +`%10`-`%7f` and cannot handle `%80`-`%99`, which decode to bytes that +are not valid UTF-8 and arrive as U+FFFD with the digits already gone. +That fix passes every test written against low pane numbers and fails +once a server reaches pane 128. + +A bare number needs no escaping at any pane number, so `tmux://panes/10` +is now the spelling that always works; `%2510` keeps working for anyone +who found that workaround. The not-found error also `repr()`s the id +and says so when it is unprintable -- previously it read +`"Pane not found: "` with an invisible control character, pointing at +nothing. + + +#### `capture_since` no longer re-delivers scrollback when a pane narrows + +Narrowing a pane rewraps its history longer, and the cursor carried no +width, so the anchor's row coordinates silently stopped meaning +anything. `start` went negative by exactly the rewrap growth and +`capture-pane -S` returned that many rows of already-seen scrollback as +new output -- under `lines_missed=false`, asserting nothing had been +missed. Measured: 160 to 40 columns re-delivered 189 stale lines, and +**one** column of narrowing re-delivered three. + +Widening was already caught, which is what hid it: rewrap makes history +*shorter*, so the existing shrink branch fired. Growth from a rewrap is +indistinguishable from growth from real output, so no branch fired on +the way down. The cursor now carries `pane_width` and a change +invalidates it exactly as a `pane_pid` change does -- width being the +precise discriminator, since history growing from real output must stay +valid. A cursor minted before this field existed is treated as +invalidated rather than trusted, which self-heals on the next call. + +The trigger is a user resizing their terminal. + +#### `run_command` refuses a pane in copy mode + +Copy, view and clock mode own the keyboard while `alternate_on` stays 0 +and `pane_current_command` still reads as the shell, so both existing +arms of the busy-pane guard missed it. Measured with a client attached: +across eight trials the command never ran, the payload was consumed as +copy-mode keystrokes, and the user's scroll position was destroyed in +seven of them -- while the result reported `command_may_still_run`, +which would send an agent away from retrying a command that had never +happened. + +`#{pane_in_mode}` rides in the `display-message` round trip the guard +already makes. Refusing before sending is what preserves the scroll +position; a probe would still have typed into it. + +Without an attached client the same call fails benignly with "no +current client" and copy mode survives, so a test written on a detached +fixture sees the mild version and misses this one. + + +#### A caller string starting with `-` no longer runs a different command + +`set_environment(name="-u", value="VICTIM")` returned +`{"name": "-u", "value": "VICTIM", "status": "set"}`. What actually ran +was `tmux set-environment -u VICTIM`, which **deleted** a pre-existing +variable. Nothing was set, something was destroyed, and the result said +the opposite. + +`set_option` had it twice, and the second is worse because two +mechanisms stack. `set_option(option="-g", value="x")` let tmux eat +`-g` as the global flag, leaving `x` as the option name -- which tmux +then prefix-matched to `xterm-keys` and turned off. A caller who named +neither `xterm-keys` nor anything resembling it silently changed it and +was told `status="set"` for the option they did name. + +tmux parses flags before quoting can protect anything, and libtmux +emits `[name, value]` with no `--` terminator. Quoting itself is sound: +`A;kill-server` and `CANARY_TWO -u` were both stored verbatim as absurd +but literal names, killing nothing and unsetting nothing. The hole is +specifically a *first character* of `-` that happens to name a flag the +target command accepts -- `set_option(option="-U")` was already refused +by tmux as an invalid flag, which is what makes `-u` and `-g` the +dangerous cases rather than the obvious ones. + +Environment names are now validated against POSIX +`[A-Za-z_][A-Za-z0-9_]*`, which rejects every flag-shaped name by +construction along with the junk above; option names must not begin +with `-`, the looser rule that `@user-options` require. `display_message` +takes the third form: a format legitimately may start with `-`, so it +gets tmux's `--` terminator instead of a refusal. Previously +`format_string="-p"` was eaten as tmux's own print flag and tmux +answered with its DEFAULT message -- a plausible string answering a +question nobody asked. + +This is the `send_keys` flag bug, fixed earlier in this branch with +`--`, reappearing in three more tools. There it dropped a keystroke; +here it destroys state. + +#### `unset_environment` + +There was no way to remove a tmux environment variable. `value=""` sets +an empty string, which new panes still inherit as set-but-empty, and +`EnvironmentResult.removed` described a state no tool in the server +could produce. The only route to an unset was the flag bug above, so +closing that without this would have removed a capability rather than +a hazard. + + +#### Filters answer, or say why not -- three silent-empty paths closed + +Follow-ups on the filtering work above, all found by driving the server +as a client rather than by reading the code. + +**Operators that cannot apply to a boolean are refused.** `is_caller__in` +and `is_caller__nin` both returned zero rows against the same set -- +every row is either in or not in, so one of those answers was false +whichever way you read it -- and `is_caller__regex: ".*"` matched +nothing. Upstream, `lookup_in`, `lookup_nin` and `lookup_regex` all +guard on `isinstance(data, (str, list))` and fall off the end to +`return False` for a bool. That is not fixable from here, so the +operator is now checked against the field's declared type and refused +with a message instead of run. + +**An unparseable boolean is refused.** `is_caller: "ture"` compared a +string to `True`, matched nothing and reported no error -- the typo +bug this branch fixed for field names, one position to the right. The +accepted tokens are in the message. + +**A traversal path with no trailing operator now works.** +`active_pane__pane_id` raised "Invalid filter operator 'pane_id'" +because the last segment was taken as an operator unconditionally. +libtmux gets this right (`OpNotFound` subclasses `ValueError`, so +`QueryList` catches its own raise and defaults to `exact`), which made +the server stricter than the engine beneath it -- and it rejected from +callers the exact key form its own alias generates internally. + +The fallback is not free: read naively, a mistyped operator like +`session_name__containss` reads as an attribute path, resolves on +nothing, and filters every row out silently -- reintroducing the bug +one line below the fix. A multi-segment path that no item can resolve +is therefore an error, which the operator message is folded into. + +#### `run_command` refuses a pane running `top` + +`top` owns the keyboard but repaints the primary screen, so +`alternate_on` stays 0 for as long as it runs -- measured at 0 through +8 seconds, not a startup race. It therefore passed both arms of the +busy-pane guard and took this tool's exit-status wrapper as single-key +commands. `htop` and `watch` reach the alternate screen and were +already refused, which is what made the gap easy to miss. + + +#### Filter by what a listing actually showed you + +`list_panes(filters={"is_caller": true})` raised `Unknown filter field +'is_caller'`. That call is not an edge case: it is the workflow the +server's own instructions hand every client as the only way to answer +"which pane am I in?", since there is no whoami tool. + +Filters went straight into libtmux's `QueryList`, which resolves keys by +`getattr` on the tmux object. Two kinds of output field are invisible +there -- ones the server computes while serializing (`is_caller`) and +ones tmux exposes under a different name (`window_count`, `pane_count`, +`active_pane_id`). So the documented workflow could not work, and +neither could filtering a session list by the window count it had just +displayed. + +The old error message compounded it. "Call this tool without filters to +see available fields" pointed at a listing that shows 6 of a Session's +230 filterable fields, two of which were themselves rejected as filters +-- so following the advice returned a set that was both 97% incomplete +and partly invalid. + +Every field a tool returns is now filterable. Computed fields are +applied after serialization through the same `QueryList` operators, so +`__contains` and friends behave identically; aliased ones rewrite to +their attribute path. Because `filters` is typed `dict[str, str]`, +`"true"` is accepted wherever `true` is. The error now names the output +fields outright and counts the wider attribute set behind them. + +#### `respawn_pane` returns the new process, not the one it replaced + +`pane_pid` changes the instant `respawn-pane` returns, but +`pane_current_command` reports tmux's transient login shell for a few +tens of milliseconds before the requested command execs over it — +measured at roughly 50 ms on tmux 3.7c. The model was serialized from +the first read, so a caller checking "did my shell take effect?" against +the returned `PaneInfo` got the process that was about to be replaced. + +This was not cosmetic: `test_respawn_pane_replaces_shell` asserts on +that field and has been failing intermittently since it was written, +absorbed by the suite's `--reruns=2`. That existing test is the +regression guard, and it is now deterministic. + +Three plausible predicates were measured and all three fail. Waiting for +the pid to change is necessary but not sufficient — over 15 runs the +command was still stale at pid-change 15 times. Waiting for the command +to *change* never fires when a pane is respawned as itself. Waiting for +two consecutive equal reads is the worst of the three: the pre-change +value is itself stable for those ~14 ms, so a fast poll debounces onto +the OLD value and returns it confidently — measured at 0/6 stale with a +20 ms poll, 2/6 at 5 ms and 3/6 at 1 ms, i.e. correct only by accident +of the interval. + +What ships instead matches the requested command's basename, which is +interval-independent. Without a `shell` there is nothing to wait for, and that is structural: +tmux's `spawn.c` guards its default-command fallback on +`sc->argc == 0 && (~sc->flags & SPAWN_RESPAWN)`, so a commandless +respawn skips it and reuses the pane's existing `argv`. The new command +equals the old for any pane, including one running an editor rather +than a shell. A wrapper command whose basename never appears falls +through to a 250 ms cap, ten times the measured 26.4 ms worst case. + +#### Three tools now report what happened, not what was asked + +`pipe_pane`, `respawn_pane` and `paste_text` each returned success for +an operation that did not do what the caller asked. In all three 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 at that path would then read +back as if it were live capture. The destination is now 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 on the redirect. + +**`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 whole +server while the tool returned a `PaneInfo` describing a pane that no +longer existed. The program is now checked before respawning, since +catching it afterwards can only report the loss and even that races the +dying process. A respawn that still loses its pane raises rather than +returning stale state. + +**`paste_text` with a trailing newline.** Bracketed paste — the default +— holds the newline in the shell's edit buffer instead of submitting. +That is 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, possibly long after +the call and out of order with it. The result now says so and names +both ways to submit. + +#### Resource URIs warn that a name must be percent-encoded + +A resource URI's path segment is percent-decoded before lookup, which +is correct URI behavior — but nothing said so, and nothing round-trips +a name into a URI safely. With two sessions named `pct name` and +`pct%20name`, reading `tmux://sessions/pct%20name` returns **`pct +name`**: the wrong session, silently, with no error. Only +`tmux://sessions/pct%2520name` reaches the one whose name contains the +literal `%`. A space needs no encoding, so the trap only springs on +`%`. + +The resource docstrings now say this at the point an agent constructs a +URI. 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 session/window/pane models remains the better fix +and is recorded as follow-up rather than landed unverified. + +#### A missing resource is no longer an "Internal error" + +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 intercepted `tools/call` only, while +fastmcp's 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: an error +this server raised deliberately to describe a caller-caused failure now +maps to `-32002` on a resource read and `-32602` elsewhere, with its own +message intact. The property — an expected failure is never an internal +error — now holds on every path that transform serves, instead of on +the one path that was noticed. + +#### A non-MCP buffer name is described accurately + +`load_buffer`/`paste_buffer`/`show_buffer` answered a name like +`yanked` or tmux's own `buffer0` with `Invalid buffer name` — but those +names are perfectly valid to tmux. What is true is that they were not +allocated by this server, which only touches buffers it created because +tmux buffers can hold OS clipboard history. + +The message now says that, and the suggestion states the consequence +plainly: a buffer created outside this server — a copy-mode yank, or +`buffer0` — is not reachable by design, and `load_buffer` is the way to +stage content it can read back. + +#### `run_command` checks the pane has a shell to talk to + +`run_command` assumed a cooperative shell sitting at a prompt, and +neither verified nor documented it. The assumption holds in tests and +breaks in a live session, in two ways. + +**A program owns the keyboard.** Sent to a pane running +`less`, this tool's exit-status wrapper was consumed as *less's* +keystrokes — `s=$?` became its save-to-file command and a fragment +escaped to a shell, leaving `Cannot write to "=$?; tmux ...` on screen. +In `vi` the same payload lands in the buffer, where `:`-prefixed +fragments are commands that edit and write files. `run_command` now +reads `alternate_on` before sending and refuses, naming the program and +pointing at `send_keys` for raw input. The signal was already there: +`snapshot_pane` reports both `alternate_on` and `pane_current_command`. + +`alternate_on` alone 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, so it owns the keyboard with `alternate_on=0` and the +wrapper ran to a clean `exit_status=0`. A small deny-list of pagers and +editors covers that case. + +The obvious general test — "the foreground command is not the process +tmux started" — was implemented, measured, and rejected. It separates a +prompt from `less`, a `python` REPL and a running command cleanly, but +it also refuses a pane where the user simply ran `bash` inside a `zsh` +pane, and equally `sudo -s`, `ssh` or `nix-shell`, all of which have a +perfectly good prompt. There is no reliable way to ask tmux "is there a +prompt", so the guard errs toward letting calls through and names only +what is known harmful. It is incomplete by construction: a program not +on the list, and not yet on the alternate screen, still receives the +wrapper. + +What that costs is worth stating, because "incomplete" reads as "the +command does not run" and the real consequence is worse than that. +Measured against a `python` REPL: the call is not refused, `timed_out` +and `command_may_still_run` both fire correctly — so the agent is told +the truth — but the REPL is left at a `...` continuation with an +unterminated block, and the *next* input to that pane, from anyone, is +appended to it rather than read as a fresh statement. The pane's own +state survives; nothing is destroyed. That is the line the deny-list +draws: destructive programs are refused, merely disruptive ones are +carried by `command_may_still_run`. + +**A timed-out command is sent, not cancelled.** With the pane blocked on +a foreground command, the keystrokes sit in the terminal's input buffer +and the shell runs them whenever it next reads a line — verified: a +command that reported `timed_out=True` executed once the blocking +`sleep` returned, with no bound on when. An agent reading `timed_out` +alone concludes the command did not run and retries, which is how a +`git push`, a migration or a deploy runs twice, the second time when +nothing is watching. The result now carries `command_may_still_run`, +which says so. + +The wrapper is also sent through the checked send path, so a rejected +send surfaces as an error rather than as a timeout. + +#### `show_option` can answer what is actually in force + +`show_option(option="history-limit", scope="session")` returned +`value: null` while 50000 was in effect. 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` is readable rather than being mistaken for +"not set anywhere". + +#### `show_environment` no longer encodes removal in the key + +tmux prints a variable it has marked removed as `-NAME`. The dash was +kept in the key and the value set to boolean `true`, so +`variables["KRB5CCNAME"]` raised `KeyError` while +`variables["-KRB5CCNAME"]` answered `true` — reading as "set to true" +for a variable that is explicitly unset, and forcing every consumer to +type-check a `str | bool` mapping. + +`variables` now holds only names that are set, mapped to their values, +and removed names are listed separately under `removed`. + +#### `search_panes` rejects pagination it cannot honour + +`limit=0` returned `matches: []`, indistinguishable from a genuine +miss, and a negative `offset` was clamped to zero while being echoed +back unchanged, so the result silently did not describe the request. +Both now raise. + +#### `search_panes` says how much of each pane it read + +`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. + +The result now carries `searched_scope` (`visible` / `scrollback`), 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. + +#### `paste_text("")` is a no-op instead of an error + +tmux creates no buffer for empty content, so the follow-up +`paste-buffer` failed with `no buffer libtmux_mcp_..._paste` — an error +for a no-op, naming an internal buffer the caller never chose. + +#### `send_keys` documents its size ceiling + +tmux rejects a `send-keys` argument beyond roughly 16 KB with `command +too long`. The docstring now names that limit and points at +`paste_text`, which routes through a buffer rather than argv and takes +far more. It also warns against verifying a write by string-comparing +captured text: tmux renders combining marks and zero-width joiners as +`` placeholders, so `école` and emoji sequences come back +transformed even when the bytes were delivered correctly. + +#### An oversized response is truncated, not rejected by the client + +A capture large enough to hit the server's 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. +`capture_pane`'s own docstring advertises `max_lines=None` for a +complete capture, so the documented way to ask for everything was the +way to break it. It is 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 +had already been fixed and documented; the successful-response half was +left standing. Truncated successes now carry structured content again. + +Where the payload's shape cannot be trimmed while staying schema-valid, +the call returns an actionable tool error telling the agent to narrow +its range, rather than a response its client will reject outright. + +Tail preservation itself was correct throughout — the head is dropped, +the newest output kept, and the number of dropped lines reported. + +#### `capture_since` no longer loses a flooded pane silently + +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 the 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 cursor +fingerprint degenerates to a single hash whenever the anchor was the +last row — which is 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. Everything above it was dropped as "already seen". + +Matches are now rejected on position when they fall past `anchor_abs` — +tmux evicts only from the top, so a surviving anchor can only move +earlier. Where that is not enough (an anchor taken at the bottom of an +already-saturated history, where the two positions overlap), a +single-hash fingerprint 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 survived. Panes not near their history limit are +unaffected. + +Saturation is asked via the existing trim-risk heuristic rather than +`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 notes that "the flood alone is not +deterministic" and adds a `clear-history` to force anchor destruction, +so the flood-only path real agents hit was never covered. It is now. + +#### `show_hooks()` no longer drops hooks that `show_hook()` finds + +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 what it +was for — but it was gated on the caller passing `scope="server"` +explicitly. The natural call, `show_hooks()`, left `scope` at its +default 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 test named for this behavior only exercised the explicit-scope +path, so the default one was never covered. + +#### `list_servers` rows carry a complete identity + +The directory scan held each socket's full path 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 to tie 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. A socket whose name does not round-trip +through `tmux -L` — one containing a newline, say — is listed by path +instead of being dropped from the results without a word. + +#### `wait_for_text` reports a stop marker that was already on screen + +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. + +The server instructions described this as `stop=[] bails`, which two +independent readers parsed as "the empty list bails". `stop=[]` is +accepted and behaves like `stop=null`; it is a stop *hit* that returns +immediately, and the text now says so. + +#### A live server no longer reports as absent + +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". `list_panes` did error, +so two of four calls answered with a confident falsehood. + +`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 docstring points at +`is_alive` to tell the two apart, but it cannot — they collapse to the +same `False`. tmux itself distinguishes them on stderr, so that is read +instead of the boolean. + +`list_sessions` now raises rather than claiming a server it could not +query is empty, and names the likely cause. `ServerInfo` gains +`unreachable_reason`: when it is set, `is_alive=False` means "could not +ask", not "not running", and `session_count=0` carries no information. + +The trigger is an ordinary tmux upgrade — sockets outlive the binary +that made them. The boundary is between tmux 3.5 and 3.6, and only in +the old-client-to-new-server direction; a newer binary reads an older +server correctly. + +#### A rejected copy-mode command is no longer reported as success + +`exit_copy_mode` on a pane that was not in a mode returned a full +`PaneInfo`, which reads as confirmation the pane left copy mode. tmux +says `not in a mode` and exits 1. `Pane.send_keys(copy_mode_cmd=...)` +discards that result, the same way the ordinary send path did. +`enter_copy_mode(scroll_up=...)` used the same unchecked call. + +#### A newline in a directory name is diagnosed, not "Unexpected error" + +A pane whose current directory contains a newline makes libtmux fail to +parse `-F` output, and because every pane lookup enumerates panes, the +whole tmux 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 also could not repair it through the MCP, since every tool that +could move the pane needed the same enumeration. + +It is now an expected failure that names the cause and how to find the +offending pane. The parse itself is fixed in libtmux +([#752](https://github.com/tmux-python/libtmux/pull/752)); this +diagnosis stays useful regardless, because the installed libtmux +version is not this package's to choose. + +#### `Pane not found:` is no longer said twice + +`exc.PaneNotFound` prefixes its own message and the error mapper +prefixed it again, so the most frequently hit error in the server read +`Pane not found: Pane not found: %9999`. + +#### A typo in a filter field is an error, not an empty list + +`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, like `{"totally_bogus": "zzz"}`, was not checked by +anything. libtmux's `QueryList` resolves a filter 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 genuine one. + +Field names are now checked against the object being filtered, with +near-misses suggested: + +``` +Unknown filter field 'session_nme' in 'session_nme__contains'. +Did you mean: session_name, session_marked, session_id? +``` + +Validation covers only the leading segment of a key, so nested +traversal such as `active_window__window_name__contains` keeps working. + +#### A pane that dies mid-wait reports the death, not a parse crash + +Killing a pane while `wait_for_text` was waiting on it surfaced +`Unexpected error: ValueError: invalid literal for int() with base 10: +''`. tmux expands every field of a vanished pane to the empty string, +and three `int()` calls on the poll path took it raw. + +Fixing only the `int()` calls would have replaced the crash with a wrong +answer: `pane_dead` is one of the blanked fields, so it reads back as +`"0"` and cannot report the death itself. A killed pane would then have +failed the pid comparison instead and been reported as *respawned*. A +live pane always has a `pane_pid`, so an empty one is the reliable +signal that the pane is gone, and the wait now ends with `pane %N died; +cursor/baseline anchor is no longer valid`. + +`_read_history_limit` had the same defect one call over — its guard +covered an empty list but not an empty string — and now shares the +helper. + +#### `send_keys` no longer drops text that starts with `-` + +`send_keys(keys="-X cancel", literal=True)` returned `Keys sent to pane +%N` and sent nothing. tmux parsed the payload as flags and rejected the +command; the wrapper discarded tmux's result and returned a hardcoded +success string. Reachable with ordinary input — `--help` or `-v` typed +into a REPL, a negative number, a pasted diff line. + +The `send-keys` argv now ends flag parsing with `--`, and a failed send +raises with tmux's own stderr instead of reporting success. This covers +`send_keys` and both `send_keys_batch` paths, which each built the argv +separately; the timed batch path surfaced the error but still failed to +deliver. + +#### A gated tool now says which tier it needs + +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 +rather than naming the setting that enables it. + +Off-tier calls now name both the tier the tool requires and the tier in +force: + +``` +Tool 'kill_pane' requires safety level 'destructive', but this server is +running at 'mutating'. Restart it with LIBTMUX_SAFETY=destructive to +enable it. +``` + +The message previously 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. The tier named is now the one the tool +actually needs. + +Two gates were always intended here, and only one of them worked. +FastMCP's native `disable()` enforces the tier; `SafetyMiddleware` was +meant to explain it. Because `get_tool()` answers `None` for a disabled +tool, the middleware's `if tool and not allowed` guard never ran for the +tools it was written for. Off-tier names now resolve against the full +registry, which retains disabled tools and their tags. + +Denials also reach the audit log as denials again. The server's +middleware ordering is built so that a tier denial raises inside +`SafetyMiddleware` and is recorded by `AuditMiddleware` outside it; +while the denial never fired, a blocked call was audited as an +unknown-tool error instead. + +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 now hands the operation on instead of duplicating the lookup — +the nested call runs through the middleware, which names the tier for a +gated tool while a real typo still gets FastMCP's own error. + +`SafetyMiddleware.on_call_tool` additionally fails closed when no +FastMCP context is present. It previously fell through to the tool, +which was fail-open in a gate whose top tier includes `kill_server`, and +was masked only because the native gate made the dispatch fail anyway. + +### Documentation + +#### opencode joins the install picker + +The install widget gains an opencode panel. `opencode mcp add tmux -- ` +is non-interactive once a name and a `--` command are both given, so it is a CLI +panel rather than a paste-this-JSON one — which also sidesteps opencode's +unusual entry shape. + +### Development + +**A killed test run leaked a tmux daemon forever** + +Fixture finalizers do not run when pytest is `SIGKILL`ed, so an +interrupted run left a live tmux daemon and its socket behind +permanently. They accumulate: 119 live `libtmux_test*` servers spanning +three days were found on one development box. + +A clean run leaks none -- measured before and after, delta zero -- so +this is specifically about interrupted runs, of which a session +debugging flaky tests produces plenty. + +It is not only untidy. `list_servers` probes every live socket, so the +debris was also what made that tool spectacular rather than merely slow: +119 servers x ~12.7 ms is the 1517 ms measured there. + +`pytest_sessionstart` now reaps `libtmux_test*` sockets older than an +hour. The age gate is the load-bearing part: pytest-xdist workers all +start at once, so an unconditional reaper in one worker would kill the +servers another worker had just created. An hour is far longer than the +two-minute suite and far shorter than debris survives. + + +**The suite is green in CI while two to five tests fail per parallel run** + +Measured, and it is the finding that supersedes several smaller ones in +this branch's history. + +Run the suite as CI does — `pytest -n auto` — but with `--reruns` off, +and **every run fails**. Two independent sets of runs agree: + +``` +run 1 2 3 4 5 +A 1 5 2 3 3 failures +B 6 4 1 - - failures +``` + +A different set each time, well over a dozen distinct tests in the +union. Most send keys to a real `zsh` and wait inside a fixed budget; +under `-n auto` the tmux servers and shell startups compete and budgets +tuned on an idle machine expire. + +**Scope the fix by the pattern, not by the filename.** Eleven sampled +runs put all but one failure in `tests/test_pane_tools.py`, which +invites narrowing the work to that module. Counting the at-risk pattern +instead — drives a real shell *and* waits on output — finds it in +several modules, with `test_pane_tools.py` holding roughly 75-85% of +the wait sites. The spread is itself instructive: a pattern that counts +`wait_for_text` calls inflates that module, because `wait_for_text` is +the tool *under test* there, so the metric measures something adjacent +to the claim in precisely the place the adjacency is greatest. +`retry_until` alone is the cleaner proxy. Its dominance in every sample +is a density effect, not exclusivity, and a thin tail elsewhere is +exactly what a single observed failure in `tests/test_history.py` +represents. Sampling reads a distribution's mode as its support; the +density count settles in one pass what eleven runs could not. + +Run it in the default configuration and the same suite reports +`968 passed, 6 skipped, **7 rerun**` — green. The rerun count is the +only visible trace, and it is easy to read past. + +Three separate per-test explanations were proposed and measured away +during this branch before the shape became clear: a shared pane shrunk +by another test's split (impossible — the fixtures are function-scoped), +a retry budget too tight in the abstract (the marker needs 0.24 s +against 2 s), and test ordering (survives randomization in isolation). +Each was a local explanation for a global property, and each looked +plausible because a single failing test is exactly what a global +property looks like from inside one test. + +The configurations are why it took so long to see: fixed-order with +reruns on, random-order with reruns off but serial, and CI's parallel +with reruns on are all blind to it. Only parallel-with-reruns-off +shows it. + +One earlier change in this branch that moved a test to its own window +on the strength of the shared-pane theory has been reverted, since it +addressed a mechanism that cannot occur. + +**Fixed, and mostly not by adjusting budgets** + +A fresh six-run sample confirmed the scoping criterion above: the union +now reaches `tests/test_history.py`, so the work is scoped by the +pattern — drives a real shell AND waits — and not by the file that +happens to hold most of it. + +The dominant cause is not a budget. Twelve test helpers slept and +claimed to synchronise: `_emit_after_baseline`'s docstring said "once +the wait has armed" and its body was `await asyncio.sleep(0.2)`. Five +more said "let wait_for_text capture its baseline first" above a +`sleep(0.1)`. Losing that race is not a slow pass, it is a permanent +failure: `wait_for_text` filters each poll against the row content +captured at entry, so a marker emitted before the baseline is inside +that set and can never match afterwards. One such failure burned a +**20 second** budget — no budget would have saved it, which is exactly +what made "raise the timeouts" the wrong first instinct. + +Those twelve sites now await an event set on the wait's *second* +`_bounded_capture`. The first of those calls IS the entry capture, and +the second is issued only after its rows have been stored, so arming is +complete by the time the event fires. The helper docstrings became +true rather than aspirational. + +One test resisted that fix and needed restructuring instead: the +history-limit risk-band test raced a 200-line burst against a 2 s wait +to push `history_size` into the top 10% of `history-limit`. It now +fills history into the band *before* the wait starts, so the warning +fires on the first poll. + +The genuinely time-based ones were then raised together. Every +`retry_until` in the suite is `raises=True` — a positive wait — and for +those a larger budget costs nothing when the test passes, because the +call returns as soon as its condition holds. It only lengthens +time-to-fail. So all 56 sites under 10 s went to 10 s at once rather +than one per sampled run. `run_command`'s status-after-shell-state +test went from 2 s to 10 s for the same reason: its subject is the +reported status, not the latency. + +`send_keys_batch`'s timeout fixture was a third thing again. Its sends +are monkeypatched to return instantly, so the 0.05 s budget bounded +nothing but Python overhead — and under load that overhead pre-empted +the injected `TimeoutExpired` the test exists to exercise, so the test +failed while measuring the wrong mechanism. +One budget is deliberately left small and now says so in the code: +`test_run_command_reports_unclamped_timeout` passes a timeout that it +then asserts is echoed back verbatim, so widening it breaks the +assertion. The sweep above caught it, which is the argument for +sweeping rather than editing constants by hand. + +A follow-up pass caught the cost of getting that rule slightly wrong. +Raising a budget is free only when the call returns as soon as its +condition holds. Three sites are the opposite -- they spend the whole +budget on purpose, to prove something does NOT happen -- and raising +those turned a ceiling into a bill: + +| site | before | after | +|---|---|---| +| `wait_for_text` case `silent_pane_still_times_out` | 20.0 s | 1.0 s | +| `pipe_pane` "the file must not grow" | 10.0 s | 1.0 s | +| `wait_for_text` tail-cap (pattern never appears) | 6.0 s | 3.0 s | + +The `pipe_pane` one is the instructive shape: it is a +`retry_until(..., raises=True)` *inside* `pytest.raises(WaitTimeout)`, +so it reads as a positive wait and behaves as a negative one. Serial +suite time went from 139 s to 117 s, and the slowest single test from +20.0 s to 3.0 s. + +Measured before and after, `pytest -n auto --reruns 0`, same box: + +| | runs failing | failures per run | loadavg at start | +|---|---|---|---| +| before | 6 of 6 | 3, 2, 2, 1, 1, 5 | 13 - 33 | +| after | 1 of 3 | 0, 2, 0 | 25 - 45 | + +The after runs are the less favourable comparison, not the more: they +ran at roughly double the load, on a box shared with a second agent +driving tmux continuously. The two failures in that run were the same +pure-budget shape at a `timeout=5.0` the first sweep had not covered, +and are fixed by the same rule. + +**`mcp_swap.py` covers opencode and pi** + +`use-local`, `status`, `revert`, `doctor` and `detect` now reach two more agent +CLIs. + +opencode is the first config the script edits that is not plain JSON or TOML. +Its `$XDG_CONFIG_HOME/opencode/opencode.jsonc` is JSONC, its server map hangs +off a top-level `mcp` key rather than `mcpServers`, and one entry packs argv +into a single `command` array with its environment table spelled +`environment`. Getting the shape wrong is not a soft failure there: a scalar +`command` is a decode error that stops opencode starting, and an `env` key is +dropped without a word. Comments survive a swap, including one written directly +above the `command` it explains. + +pi ships no MCP client — its README says so outright, and the released build +contains no MCP code. `~/.pi/agent/mcp.json` is read by the third-party +`pi-mcp-adapter` extension, so a swap written there takes effect only once that +package is installed. `detect` says so rather than reporting a swap that cannot +do anything. + +**JSONC is edited rather than reserialized** + +The JSON writer rebuilds the whole document, which for a commented file would +mean deleting every comment in it. JSONC values now come from stdlib `json` +after comments and trailing commas are blanked in place, and writes are applied +as text splices, so every byte outside a replaced value is untouched. The +obvious dependency was measured and rejected: `json-five` round-trips comments, +but raises on the valid JSON string `"C:\\x"` and silently decodes a literal +`\u0041` to `"A"`. + +**Per-CLI behavior is declared, not branched** + +`CLIInfo` gained `container` (the key path to the server map) and `dialect` (the +entry shape) alongside `fmt`. The four `cli in (...)` membership tuples that +`get_server`, `set_server`, `delete_server` and `_all_server_specs` each carried +are gone. Two of them ended in a bare `else` that fell through to the TOML key, +so a CLI registered but forgotten in one tuple reported "no entry" instead of +failing; the other two raised `AssertionError`, which the caller did not catch. +A non-mapping at a container key now raises a `RuntimeError` naming the path for +every CLI, not just Claude. `scripts/README.md`'s extension guide described +three branch sites when there were four; it now describes the fields instead. + +## libtmux-mcp 0.1.0a20 (2026-08-09) + +libtmux-mcp 0.1.0a20 changes no tool behavior. `scripts/mcp_swap.py` gains `use-local --pr N`, which points installed agent CLIs at a pull request without a checkout and verifies the server before it rewrites any configuration, and its edits now preserve unrelated config text, file permissions, and symlink targets. ruff's curated default rule set is enabled behind a `ruff>=0.16.0` floor, taking the project from 351 enabled rules to 565 and fixing what that surfaced, and the CI workflow actions move to their current majors. In the documentation, the dataclass identifying an MCP caller describes each of its fields instead of reaching the API reference as "Alias for field number 0". + ### Documentation #### Caller identity fields are described (#105) @@ -16,6 +950,14 @@ or as a bare name carrying only its type. ### Development +**Safer pull-request testing with `mcp_swap.py`** + +`use-local --pr N` points installed agent CLIs at a pull request without a +checkout and verifies the server before changing their configuration. Swaps +preserve unrelated config text, file permissions, and symlink targets, while +retaining the original recovery data across failed or concurrent updates. +(#115) + #### CI actions updated to current majors Workflow actions moved to their current major releases: `actions/checkout` v7, diff --git a/docs/_ext/widgets/mcp_install.py b/docs/_ext/widgets/mcp_install.py index 39e6e411..88b8a867 100644 --- a/docs/_ext/widgets/mcp_install.py +++ b/docs/_ext/widgets/mcp_install.py @@ -170,6 +170,18 @@ class Panel: ), ) +#: User scope only: `opencode mcp add` writes the global config whether or +#: not a project one exists, so a Project panel would advertise a file the +#: command it prints never touches. +_OPENCODE_SCOPES: tuple[Scope, ...] = ( + Scope( + id="user", + label="User", + config_file="~/.config/opencode/opencode.jsonc", + note=None, + ), +) + _GROK_SCOPES: tuple[Scope, ...] = ( Scope( id="user", @@ -238,6 +250,12 @@ class Panel: kind="json", scopes=_ANTIGRAVITY_SCOPES, ), + Client( + id="opencode", + label="opencode", + kind="cli", + scopes=_OPENCODE_SCOPES, + ), ) @@ -377,6 +395,14 @@ def _cli_body(client: Client, scope: Scope, method: Method, cooldown: Cooldown) # ``--`` is handed to the server process verbatim. if client.id == "grok": return f"grok mcp add --scope {scope.id} tmux -- {tool_cmd}" + # opencode: ``opencode mcp add -- `` is non-interactive as + # soon as a name and a ``--`` command are both given, and it writes + # whichever config file is in scope for the current directory. The + # stored entry packs argv into a single ``command`` array rather than + # the command/args pair the JSON-kind clients use, which is why this + # is a CLI panel and not a paste-this-JSON one. + if client.id == "opencode": + return f"opencode mcp add tmux -- {tool_cmd}" # codex: CLI doesn't write project scope; the project-scope panel # uses the TOML body path (see ``_body_for``). return f"codex mcp add tmux -- {tool_cmd}" diff --git a/justfile b/justfile index 396845b4..a0cec91d 100644 --- a/justfile +++ b/justfile @@ -119,7 +119,7 @@ watch-mypy: format-markdown: prettier --parser=markdown -w *.md docs/*.md docs/**/*.md CHANGES -# Detect which CLI agents (claude/codex/cursor/gemini) exist on this machine +# Detect which agent CLIs exist on this machine [group: 'mcp'] mcp-detect: uv run scripts/mcp_swap.py detect diff --git a/pyproject.toml b/pyproject.toml index 12056fa1..036f261a 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -1,6 +1,6 @@ [project] name = "libtmux-mcp" -version = "0.1.0a19" +version = "0.1.0a20" description = "MCP server for tmux, powered by libtmux" requires-python = ">=3.10,<4.0" authors = [ diff --git a/scripts/README.md b/scripts/README.md index 08601f7d..21f07969 100644 --- a/scripts/README.md +++ b/scripts/README.md @@ -46,6 +46,43 @@ This matches Claude's conventional dev form and takes advantage of `uv run`'s automatic editable install — source edits flow through on the next invocation with no reinstall step. +### `--pr N` — point every CLI at a pull request + +Review a branch across your agents without checking it out: + +```console +$ uv run scripts/mcp_swap.py use-local --pr 114 +``` + +Each CLI's entry becomes: + +``` +command = "uvx" +args = ["--from", "git+@refs/pull/114/head", "libtmux-mcp"] +``` + +`uv` resolves the ref itself, so nothing lands on disk to refresh or +prune and `revert` restores the config with no extra cleanup. GitHub +publishes `refs/pull/N/head` on the base repository, so a pull request +from a fork needs no special handling. + +Before writing anything, the swap launches the resolved command once and +completes an MCP `initialize` round trip. A ref that does not exist, or a +dependency that cannot resolve, fails there — rather than landing in +every CLI's config and surfacing later as an opaque startup error inside +each agent. Pass `--no-preflight` to skip the probe when offline. + +`gh` confirms the number exists and labels the output; resolution does +not depend on it, so an unauthenticated `gh` degrades to an unlabelled +swap rather than a failure. + +A branch whose dependencies need resolver flags can carry them as +environment, the same way any other setting travels: + +```console +$ uv run scripts/mcp_swap.py use-local --pr 114 --env UV_NO_CONFIG=1 +``` + ### `--scope {user,project}` (Claude only) Claude's `~/.claude.json` supports two config scopes for MCP servers: @@ -94,14 +131,18 @@ the user-level fallback; the project entry stays. `revert` without ### Scope -Covers four CLIs and their canonical **global** config paths: +Covers eight CLIs and their canonical **global** config paths: -| CLI | Config | Format | -|--------|-------------------------------|--------| -| Claude | `~/.claude.json` | JSON (per-project keying) | -| Codex | `~/.codex/config.toml` | TOML (format-preserving via `tomlkit`) | -| Cursor | `~/.cursor/mcp.json` | JSON | -| Gemini | `~/.gemini/settings.json` | JSON | +| CLI | Config | Format | +|-----|--------|--------| +| Claude | `~/.claude.json` | JSON (per-project keying) | +| Codex | `~/.codex/config.toml` | TOML (format-preserving via `tomlkit`) | +| Cursor | `~/.cursor/mcp.json` | JSON | +| Gemini | `~/.gemini/settings.json` | JSON | +| Grok | `~/.grok/config.toml` | TOML (same shape as Codex) | +| agy | `~/.gemini/config/mcp_config.json` | JSON | +| opencode | `$XDG_CONFIG_HOME/opencode/opencode.jsonc` | JSONC (comments preserved) | +| pi | `~/.pi/agent/mcp.json` | JSONC (read by `pi-mcp-adapter`, not by pi) | Claude's config is keyed per-project under the repo's absolute path — the script writes only under the current repo's key, leaving other projects' @@ -109,11 +150,22 @@ entries untouched. #### Out of scope (use the CLI's native command) -- **Workspace / project-local configs** for Cursor and Gemini - (`$PWD/.cursor/mcp.json`, `$PWD/.gemini/settings.json`). When - workspace precedence matters, use `cursor mcp add` / `gemini mcp add` - directly — workspace files take precedence over the global ones this - script writes. +- **Workspace / project-local configs** for Cursor, Gemini and opencode + (`$PWD/.cursor/mcp.json`, `$PWD/.gemini/settings.json`, + `$PWD/opencode.json`). When workspace precedence matters, use + `cursor mcp add` / `gemini mcp add` directly — workspace files take + precedence over the global ones this script writes. opencode has no + non-interactive project-scope add (`opencode mcp add` writes the global + file), so edit `$PWD/opencode.json` by hand. +- **opencode's sibling global files.** opencode merges `config.json`, + `opencode.json` and `opencode.jsonc` from the same directory, with + `.jsonc` winning. This script writes `.jsonc`, so its entry is the one + that takes effect, but a stale `mcp.` in a sibling + `opencode.json` merges underneath rather than being shadowed. +- **pi without `pi-mcp-adapter`.** pi ships no MCP client. The file this + script writes is read by that third-party extension, so until it is + installed the swap has no effect — `detect` reports this rather than + claiming otherwise. - **Custom binary install locations.** Detection is `shutil.which` plus the file existing at the configured global path. Homebrew, npm prefixes (`~/.npm-global/bin`), and the canonical local-install @@ -122,7 +174,27 @@ entries untouched. ### Extending to a new CLI -Add an entry to the `CLIS` table in `mcp_swap.py` and extend the three -per-CLI branches in `get_server` / `set_server` / `delete_server`. Tests -in `tests/test_mcp_swap.py` use a `fake_home` fixture that monkeypatches -`CLIS`, so the extension pattern is already established. +Add an entry to the `CLIS` table in `mcp_swap.py`. Each `CLIInfo` carries +the three things that vary per CLI, so no `get_server` / `set_server` / +`delete_server` / `_all_server_specs` branch needs touching: + +- `fmt` — `json`, `jsonc` or `toml`, selecting the reader and writer +- `container` — the key path to the server map, e.g. `("mcpServers",)` + or `("mcp",)` +- `dialect` — the shape of one entry: `standard` (scalar `command`, + sibling `args`, optional `env`), `claude` (adds `type` and always + writes `env`), or `opencode` (one `command` array, env under + `environment`) + +Add the name to `CLIName` and `ALL_CLIS` too — a CLI in `CLIS` but not +`ALL_CLIS` has its state entries dropped on load, so `revert` forgets the +swap and leaves the config rewritten. + +A dialect no existing CLI speaks needs a branch in +`McpServerSpec.to_entry_dict` and its mirror in `_spec_from_entry`; that +mirror is what keeps `is_local_uv_directory`, `local_repo_path` and +`pr_ref` working, and those drive the "already local" short-circuit. + +Tests in `tests/test_mcp_swap.py` use a `fake_home` fixture that +monkeypatches `CLIS` wholesale, so every new CLI must be added there as +well — `test_fake_home_covers_every_registered_cli` enforces it. diff --git a/scripts/mcp_swap.py b/scripts/mcp_swap.py index 5b4355e4..78428a08 100755 --- a/scripts/mcp_swap.py +++ b/scripts/mcp_swap.py @@ -3,12 +3,13 @@ # requires-python = ">=3.10" # dependencies = ["tomlkit>=0.13"] # /// -"""Swap MCP server configs across Claude / Codex / Cursor / Gemini / Grok / agy. +"""Swap MCP server configs across every installed agent CLI. Use when you want every installed agent CLI to run a local checkout of an MCP server (editable) instead of a pinned release. ``use-local`` rewrites each CLI's config to invoke the checkout via ``uv --directory run -``; ``revert`` restores from the timestamped backup the swap wrote. +``, or a pull request's head via ``uvx`` with ``--pr``; ``revert`` +restores from the timestamped backup the swap wrote. Swapping a layer that is already swapped keeps that first backup rather than taking a new one, so ``revert`` always lands on the pre-swap config. @@ -25,6 +26,7 @@ $ uv run scripts/mcp_swap.py status $ uv run scripts/mcp_swap.py use-local --dry-run $ uv run scripts/mcp_swap.py use-local +$ uv run scripts/mcp_swap.py use-local --pr 115 $ uv run scripts/mcp_swap.py revert ``` @@ -35,16 +37,35 @@ - **Global configs only.** Writes to ``~/.cursor/mcp.json``, ``~/.claude.json``, ``~/.codex/config.toml``, ``~/.gemini/settings.json``, ``~/.grok/config.toml`` (TOML - ``mcp_servers``, same shape as Codex), and + ``mcp_servers``, same shape as Codex), ``~/.gemini/config/mcp_config.json`` (agy / Antigravity CLI, JSON ``mcpServers`` — the shared-config file the CLI reads, sibling to the - ``config.json`` it loads at startup). Workspace / project-local configs - (``$PWD/.cursor/mcp.json``, ``$PWD/.gemini/settings.json``, - per-project ``projects..mcpServers`` entries inside - ``~/.claude.json`` *are* recognised for Claude only) are NOT - walked — workspace files for Cursor/Gemini are silently ignored. + ``config.json`` it loads at startup), + ``$XDG_CONFIG_HOME/opencode/opencode.jsonc`` (JSONC ``mcp``, comments + preserved) and ``~/.pi/agent/mcp.json`` (JSONC too -- the adapter that + reads it strips comments). Workspace / project-local + configs (``$PWD/.cursor/mcp.json``, ``$PWD/.gemini/settings.json``, + ``$PWD/opencode.json``, per-project ``projects..mcpServers`` + entries inside ``~/.claude.json`` *are* recognised for Claude only) + are NOT walked — workspace files for the others are silently ignored. When workspace precedence matters, run the CLI's own - ``cursor mcp add ...`` / ``gemini mcp add ...`` directly. + ``cursor mcp add ...`` / ``gemini mcp add ...`` directly. opencode has + no non-interactive project-scope add -- ``opencode mcp add`` writes the + global file -- so edit ``$PWD/opencode.json`` by hand for that. + +- **opencode reads three global files.** ``config.json``, + ``opencode.json`` and ``opencode.jsonc`` in the same directory are all + loaded and merged, with ``.jsonc`` winning. This script owns + ``.jsonc`` — the file opencode itself writes to — so its entry is the + one that takes effect. A stale ``mcp.`` left in a sibling + ``opencode.json`` still merges underneath rather than being shadowed + outright; remove it by hand if that matters. + +- **pi has no MCP client of its own.** Its README says so, and the + released build ships no MCP code. ``~/.pi/agent/mcp.json`` is read by + the third-party ``pi-mcp-adapter`` extension, so a swap written there + takes effect only once that package is installed. ``detect`` says as + much rather than reporting a swap that cannot do anything. - **Claude scope.** ``use-local`` and ``revert`` accept ``--scope {user,project}``. The default ``project`` writes the @@ -53,8 +74,8 @@ pre-flag behaviour. ``--scope user`` writes Claude's top-level ``mcpServers`` fallback so every project that has no per-project override picks up the swap; useful when QA-ing a branch across - many directories. Codex, Cursor, Gemini, Grok, and agy have no per-project - layer in their config files; the flag is silently coerced to + many directories. Every other CLI here has no per-project layer in + the config file this script writes; the flag is silently coerced to ``user`` for them. Both Claude scopes can coexist with independent backups; full ``revert`` unwinds in LIFO order. - **Simple binary detection.** Probing is ``shutil.which()`` @@ -71,12 +92,17 @@ from __future__ import annotations import argparse +import contextlib import dataclasses import difflib +import fcntl import json import os import pathlib +import re import shutil +import stat +import subprocess import sys import tempfile import time @@ -85,8 +111,23 @@ import tomlkit import tomlkit.items -CLIName = t.Literal["claude", "codex", "cursor", "gemini", "grok", "agy"] -ALL_CLIS: tuple[CLIName, ...] = ("claude", "codex", "cursor", "gemini", "grok", "agy") +CLIName = t.Literal[ + "claude", "codex", "cursor", "gemini", "grok", "agy", "opencode", "pi" +] +ALL_CLIS: tuple[CLIName, ...] = ( + "claude", + "codex", + "cursor", + "gemini", + "grok", + "agy", + "opencode", + "pi", +) + +#: Width of the CLI-name column in ``detect`` output, derived rather +#: than hardcoded so adding a longer name cannot silently misalign it. +_CLI_COLUMN = max(len(name) for name in ALL_CLIS) + 1 #: Claude config scope: ``"user"`` targets the user/system-level top-level #: ``mcpServers`` fallback that applies to every project without its own @@ -185,6 +226,18 @@ def _xdg_state_home() -> pathlib.Path: # --------------------------------------------------------------------------- +#: Per-entry shape a CLI expects under its server map. ``standard`` is +#: the Claude-Desktop lineage every CLI here started from — scalar +#: ``command``, sibling ``args`` list, optional ``env`` table. +#: ``claude`` is that shape plus an explicit ``type``/``env`` that +#: Claude writes even when empty. ``opencode`` packs argv into a single +#: ``command`` array and spells the environment table ``environment``. +#: Dialects exist because the shape is not implied by the file format: +#: two CLIs sharing ``fmt="json"`` can still disagree about how one +#: entry is spelled. +Dialect = t.Literal["standard", "claude", "opencode"] + + @dataclasses.dataclass(frozen=True) class CLIInfo: """Static descriptor for a CLI's config file and discovery heuristics.""" @@ -192,7 +245,28 @@ class CLIInfo: name: CLIName binary: str config_path: pathlib.Path - fmt: t.Literal["json", "toml"] + fmt: t.Literal["json", "jsonc", "toml"] + #: Key path from the document root down to the mapping of server + #: name -> entry. A path rather than a single key so a CLI that + #: nests deeper needs no new branch in the four functions that + #: read, write, delete and enumerate entries. + container: tuple[str, ...] + #: Entry shape written and read back for this CLI. + dialect: Dialect + + +def _xdg_config_home() -> pathlib.Path: + """``$XDG_CONFIG_HOME`` when absolute, else ``~/.config``. + + The spec requires these variables to be absolute and says to ignore + them otherwise. A relative value would resolve against the working + directory, so the swap would record a backup path that revert could + no longer find from anywhere else. + """ + raw = os.environ.get("XDG_CONFIG_HOME") + if raw and pathlib.Path(raw).is_absolute(): + return pathlib.Path(raw) + return pathlib.Path.home() / ".config" CLIS: dict[CLIName, CLIInfo] = { @@ -201,39 +275,98 @@ class CLIInfo: binary="claude", config_path=pathlib.Path.home() / ".claude.json", fmt="json", + container=("mcpServers",), + dialect="claude", ), "codex": CLIInfo( name="codex", binary="codex", config_path=pathlib.Path.home() / ".codex" / "config.toml", fmt="toml", + container=("mcp_servers",), + dialect="standard", ), "cursor": CLIInfo( name="cursor", binary="cursor-agent", config_path=pathlib.Path.home() / ".cursor" / "mcp.json", fmt="json", + container=("mcpServers",), + dialect="standard", ), "gemini": CLIInfo( name="gemini", binary="gemini", config_path=pathlib.Path.home() / ".gemini" / "settings.json", fmt="json", + container=("mcpServers",), + dialect="standard", ), "grok": CLIInfo( name="grok", binary="grok", config_path=pathlib.Path.home() / ".grok" / "config.toml", fmt="toml", + container=("mcp_servers",), + dialect="standard", ), "agy": CLIInfo( name="agy", binary="agy", config_path=(pathlib.Path.home() / ".gemini" / "config" / "mcp_config.json"), fmt="json", + container=("mcpServers",), + dialect="standard", + ), + "opencode": CLIInfo( + name="opencode", + binary="opencode", + # opencode reads config.json, opencode.json and opencode.jsonc from + # this directory and merges all three, with .jsonc winning. It writes + # to the first that exists, defaulting to .jsonc — so that is the one + # file a swap can own without being shadowed. + config_path=_xdg_config_home() / "opencode" / "opencode.jsonc", + fmt="jsonc", + container=("mcp",), + dialect="opencode", + ), + "pi": CLIInfo( + name="pi", + binary="pi", + # Read by the pi-mcp-adapter extension, not by pi itself; see + # PI_ADAPTER_DIR. Claude-Desktop schema, so the standard dialect. + # The adapter parses through strip-json-comments with trailing + # commas allowed, so the file is JSONC despite the .json suffix. + config_path=pathlib.Path.home() / ".pi" / "agent" / "mcp.json", + fmt="jsonc", + container=("mcpServers",), + dialect="standard", ), } +#: Written into an opencode config this script creates from nothing. +#: opencode injects the same line itself on first load; seeding it here +#: keeps the swap from being followed by a surprise rewrite. +OPENCODE_SCHEMA_URL = "https://opencode.ai/config.json" + +#: pi ships no MCP client — its README says "No MCP" outright, and the +#: released build contains no MCP code at all. MCP reaches pi only +#: through the third-party ``pi-mcp-adapter`` extension, which is what +#: reads ``~/.pi/agent/mcp.json``. The swap writes that file because it +#: is the one pi-family location with a settled schema, but until the +#: adapter is installed pi does not read it, so ``detect`` says so +#: instead of reporting a swap that cannot take effect. +PI_ADAPTER_DIR = ( + pathlib.Path.home() / ".pi" / "agent" / "npm" / "node_modules" / "pi-mcp-adapter" +) +PI_ADAPTER_HINT = "needs the pi-mcp-adapter package; pi has no built-in MCP client" + + +#: A ``--from`` argument pointing at a pull request's head commit. +#: GitHub publishes ``refs/pull//head`` on the *base* repository, so +#: one URL serves same-repo and fork pull requests alike. +PR_REF_RE = re.compile(r"git\+(?P.+?)@refs/pull/(?P\d+)/head") + @dataclasses.dataclass class McpServerSpec: @@ -243,17 +376,28 @@ class McpServerSpec: args: list[str] = dataclasses.field(default_factory=list) env: dict[str, str] = dataclasses.field(default_factory=dict) - def to_json_dict(self, *, include_stdio_type: bool = False) -> dict[str, t.Any]: - """Serialize to the JSON shape (Claude-extended when ``include_stdio_type``).""" - # Claude's format always includes ``type`` and ``env`` (even when empty); - # Cursor/Gemini omit both. include_stdio_type selects Claude shape. - if include_stdio_type: + def to_entry_dict(self, dialect: Dialect = "standard") -> dict[str, t.Any]: + """Serialize to the entry shape ``dialect`` expects.""" + # Claude's format always includes ``type`` and ``env`` (even when + # empty); the standard shape omits both when there is nothing to say. + if dialect == "claude": return { "type": "stdio", "command": self.command, "args": list(self.args), "env": dict(self.env), } + if dialect == "opencode": + # One array for argv, and the table is "environment" -- an + # "env" key here is dropped in silence, and a scalar command + # is a decode error that takes the whole config down with it. + local: dict[str, t.Any] = { + "type": "local", + "command": [self.command, *self.args], + } + if self.env: + local["environment"] = dict(self.env) + return local out: dict[str, t.Any] = {"command": self.command, "args": list(self.args)} if self.env: out["env"] = dict(self.env) @@ -275,6 +419,16 @@ def local_repo_path(self) -> pathlib.Path | None: return None return pathlib.Path(self.args[i + 1]) + def pr_ref(self) -> tuple[str, int] | None: + """Return ``(repo_url, pr_number)`` for a ``uvx`` pull-request spec.""" + if self.command != "uvx": + return None + for arg in self.args: + match = PR_REF_RE.fullmatch(arg) + if match: + return match.group("url"), int(match.group("number")) + return None + @dataclasses.dataclass class SwapEntry: @@ -297,6 +451,354 @@ class SwapEntry: #: ``Lib/sched.py`` uses to break ties on ``Event(time, priority, #: sequence, …)``. seq_no: int + #: Exact destination changed by the swap. ``config_path`` may be a + #: symlink that is later repointed, so it is not sufficient recovery + #: identity. Older state entries omit this field and fall back to + #: ``config_path`` during revert. + target_path: str | None = None + + +class SwapStateError(RuntimeError): + """Swap state is unsafe to use for a mutating operation.""" + + +# --------------------------------------------------------------------------- +# JSONC — comments and trailing commas, edited without reserializing +# --------------------------------------------------------------------------- +# +# tomlkit gives TOML a format-preserving round trip; JSONC has no +# equivalent on PyPI that is safe to depend on here. ``json-five`` was +# measured first and rejected: it raises on ``"C:\\x"`` and silently +# decodes the literal six characters ``\u0041`` to ``"A"`` — both valid +# JSON that stdlib reads correctly, and the second is exactly the silent +# rewrite this script exists to avoid. +# +# So values come from stdlib ``json`` (correct escape semantics) and +# edits are applied as text splices located by an offset-preserving +# scanner. Every byte outside a replaced value survives untouched, which +# is the same technique opencode's own config writer uses via +# ``jsonc-parser``'s ``modify()``. + +_JSON_WS = " \t\n\r" + +#: Longest inline rendering of a scalar list before it is broken across +#: lines. A swapped ``command`` array is the common case and reads +#: better on one line, which is how these configs are written by hand. +_INLINE_WIDTH = 88 + + +def _jsonc_blank_comments(text: str) -> str: + """Replace comment bytes with spaces, preserving every offset. + + Scanning rather than matching a regex is the whole point: ``//`` + inside a URL and ``/*`` inside a Windows path are string content, not + comments, and only a scanner that tracks string state can tell them + apart. Offsets are preserved so a span found in the blanked text + addresses the same bytes in the original. + """ + out = list(text) + i, n = 0, len(text) + in_string = False + while i < n: + char = text[i] + if in_string: + if char == "\\": + i += 2 + continue + if char == '"': + in_string = False + i += 1 + elif char == '"': + in_string = True + i += 1 + elif char == "/" and i + 1 < n and text[i + 1] == "/": + while i < n and text[i] != "\n": + out[i] = " " + i += 1 + elif char == "/" and i + 1 < n and text[i + 1] == "*": + end = text.find("*/", i + 2) + end = n if end == -1 else end + 2 + for j in range(i, end): + if out[j] != "\n": + out[j] = " " + i = end + else: + i += 1 + return "".join(out) + + +def _jsonc_blank_trailing_commas(blanked: str) -> str: + """Blank trailing commas so stdlib :func:`json.loads` accepts the text.""" + out = list(blanked) + i, n = 0, len(blanked) + in_string = False + last_comma = -1 + while i < n: + char = blanked[i] + if in_string: + if char == "\\": + i += 2 + continue + if char == '"': + in_string = False + i += 1 + continue + if char == '"': + in_string = True + last_comma = -1 + elif char == ",": + last_comma = i + elif char in "}]": + if last_comma != -1: + out[last_comma] = " " + last_comma = -1 + elif char not in _JSON_WS: + last_comma = -1 + i += 1 + return "".join(out) + + +def _jsonc_loads(text: str) -> t.Any: + """Parse JSONC text into plain Python objects.""" + if not text.strip(): + return {} + return json.loads(_jsonc_blank_trailing_commas(_jsonc_blank_comments(text))) + + +class _JsoncScanner: + """Locate value spans inside comment-blanked JSON text.""" + + def __init__(self, text: str) -> None: + self.text = text + self.pos = 0 + + def skip_ws(self) -> None: + """Advance past insignificant whitespace.""" + while self.pos < len(self.text) and self.text[self.pos] in _JSON_WS: + self.pos += 1 + + def read_string(self) -> str: + """Consume one string token and return its raw text, quotes included.""" + start = self.pos + self.pos += 1 + while self.pos < len(self.text): + char = self.text[self.pos] + if char == "\\": + self.pos += 2 + continue + self.pos += 1 + if char == '"': + break + return self.text[start : self.pos] + + def read_value(self) -> tuple[int, int]: + """Consume one value and return its ``(start, end)`` span.""" + self.skip_ws() + start = self.pos + char = self.text[self.pos] + if char == '"': + self.read_string() + elif char in "{[": + self._read_container() + else: + while ( + self.pos < len(self.text) + and self.text[self.pos] not in ",}]" + and self.text[self.pos] not in _JSON_WS + ): + self.pos += 1 + return start, self.pos + + def _read_container(self) -> None: + self.pos += 1 + depth = 1 + while self.pos < len(self.text) and depth: + char = self.text[self.pos] + if char == '"': + self.read_string() + continue + if char in "{[": + depth += 1 + elif char in "}]": + depth -= 1 + self.pos += 1 + + def read_members(self, obj_start: int) -> list[_JsoncMember]: + """Enumerate an object's members. ``obj_start`` indexes its ``{``.""" + self.pos = obj_start + 1 + found: list[_JsoncMember] = [] + while True: + self.skip_ws() + if self.pos >= len(self.text) or self.text[self.pos] == "}": + return found + if self.text[self.pos] == ",": + self.pos += 1 + continue + member_start = self.pos + raw_key = self.read_string() + self.skip_ws() + self.pos += 1 # the ':' + value_start, value_end = self.read_value() + found.append( + _JsoncMember( + key=json.loads(raw_key), + start=member_start, + end=value_end, + value_start=value_start, + value_end=value_end, + ) + ) + + +class _JsoncMember(t.NamedTuple): + """One ``"key": value`` pair located inside a JSONC document. + + Attributes + ---------- + key : str + The decoded member name. + start : int + Offset of the opening quote of the key. + end : int + Offset just past the value — the end of the whole member. + value_start : int + Offset of the first byte of the value. + value_end : int + Offset just past the last byte of the value. + """ + + key: str + start: int + end: int + value_start: int + value_end: int + + +def _jsonc_render(value: t.Any, depth: int, *, ensure_ascii: bool) -> str: + """Render ``value`` as JSON text indented for nesting ``depth``.""" + pad = " " * depth + if isinstance(value, list) and all( + isinstance(item, (str, int, float, bool)) or item is None for item in value + ): + inline = json.dumps(value, ensure_ascii=ensure_ascii) + if len(inline) + len(pad) <= _INLINE_WIDTH: + return inline + return json.dumps(value, indent=2, ensure_ascii=ensure_ascii).replace( + "\n", "\n" + pad + ) + + +def _jsonc_object_span(blanked: str, path: tuple[str, ...]) -> tuple[int, int] | None: + """Return the span of the object reached by ``path``, or ``None``.""" + scanner = _JsoncScanner(blanked) + scanner.skip_ws() + if scanner.pos >= len(blanked) or blanked[scanner.pos] != "{": + return None + cursor = scanner.pos + for key in path: + match = next( + (m for m in _JsoncScanner(blanked).read_members(cursor) if m.key == key), + None, + ) + if match is None or blanked[match.value_start] != "{": + return None + cursor = match.value_start + tail = _JsoncScanner(blanked) + tail.pos = cursor + return tail.read_value() + + +def _jsonc_next_edit( + text: str, + data: t.Mapping[str, t.Any], + path: tuple[str, ...], + *, + ensure_ascii: bool, +) -> tuple[int, int, str] | None: + """Find the one next splice that brings ``path`` closer to ``data``.""" + blanked = _jsonc_blank_comments(text) + span = _jsonc_object_span(blanked, path) + if span is None: + return None + obj_start, obj_end = span + members = _JsoncScanner(blanked).read_members(obj_start) + by_key = {member.key: member for member in members} + depth = len(path) + 1 + pad = " " * depth + + for key, value in data.items(): + member = by_key.get(key) + if member is None: + body = _jsonc_render(value, depth, ensure_ascii=ensure_ascii) + # Escape the key like any other value: written raw, a backslash + # or quote in a server name emits text that cannot be parsed + # back, so the member is never found and the merge re-inserts + # it until the pass ceiling, holding the swap lock throughout. + name = json.dumps(key, ensure_ascii=ensure_ascii) + if members: + tail = members[-1].end + return tail, tail, f",\n{pad}{name}: {body}" + if blanked[obj_start + 1 : obj_end - 1].strip(): + return None + # Blanking hid any comment the object holds, so measure the + # interior in the original text and splice after it, not over it. + interior = text[obj_start + 1 : obj_end - 1] + anchor = obj_start + 1 + len(interior.rstrip()) + closing = " " * (depth - 1) + return anchor, obj_end - 1, f"\n{pad}{name}: {body}\n{closing}" + current = json.loads( + _jsonc_blank_trailing_commas(blanked[member.value_start : member.value_end]) + ) + if isinstance(value, dict) and isinstance(current, dict): + nested = _jsonc_next_edit( + text, value, (*path, key), ensure_ascii=ensure_ascii + ) + if nested is not None: + return nested + elif current != value: + return ( + member.value_start, + member.value_end, + _jsonc_render(value, depth, ensure_ascii=ensure_ascii), + ) + + for index, member in enumerate(members): + if member.key in data: + continue + # Exactly one delimiter leaves with the member: the comma before + # it, or, for the first member which has none, the comma after. + if index: + return members[index - 1].end, member.end, "" + # Read that comma out of the blanked text -- one inside a comment + # is not a delimiter, and a real one behind a comment still is. + trailing = blanked[member.end : obj_end] + drop_to = member.end + if trailing.lstrip(_JSON_WS).startswith(","): + drop_to += trailing.index(",") + 1 + return obj_start + 1, drop_to, "" + return None + + +def _jsonc_merge(text: str, data: t.Mapping[str, t.Any], *, ensure_ascii: bool) -> str: + """Reconcile ``data`` into ``text``, rewriting only members that differ. + + Applies one splice at a time and rescans, so offsets are always + computed against current text rather than patched up after the fact. + Config files are small enough that the extra passes do not matter and + the invariant is worth far more than the cycles. + """ + if not text.strip(): + return json.dumps(dict(data), indent=2, ensure_ascii=ensure_ascii) + "\n" + # One splice per member, plus slack; a config that needs more than + # this has a pathology worth surfacing rather than looping on. + for _ in range(10_000): + edit = _jsonc_next_edit(text, data, (), ensure_ascii=ensure_ascii) + if edit is None: + return text + start, end, replacement = edit + text = text[:start] + replacement + text[end:] + msg = "JSONC merge did not converge" + raise RuntimeError(msg) # --------------------------------------------------------------------------- @@ -305,34 +807,93 @@ class SwapEntry: def load_config(info: CLIInfo) -> t.Any: - """Parse a CLI's config file (JSON or TOML) into an editable structure. + """Parse a CLI's config file (JSON, JSONC or TOML) into an editable structure. Empty JSON files are treated as empty objects so first-run MCP configs can be seeded with their initial server entry. """ raw = info.config_path.read_bytes() + if info.fmt == "jsonc": + return _jsonc_loads(raw.decode()) if info.fmt == "json": text = raw.decode().strip() return json.loads(text) if text else {} return tomlkit.parse(raw.decode()) -def dump_config_bytes(info: CLIInfo, config: t.Any) -> bytes: - """Serialize an edited config back to bytes in its original format.""" - if info.fmt == "json": - return (json.dumps(config, indent=2) + "\n").encode() - return tomlkit.dumps(config).encode() +def _json_trailer(original: bytes) -> str: + """Return the newline a rewritten JSON config should end with. + + Claude writes ``~/.claude.json`` without a trailing newline, so + appending one unconditionally grows the file by a byte on every swap + and shows as a diff hunk in a region the swap never touched. Empty + bytes mean a file being seeded, which gets the conventional newline. + """ + if not original: + return "\n" + return "\n" if original.endswith(b"\n") else "" + + +def dump_config_bytes(info: CLIInfo, config: t.Any, *, original: bytes) -> bytes: + """Serialize an edited config back to bytes in its original format. + + ``original`` is the file's pre-edit bytes, or empty when seeding a + new one. The parsed structure does not record the byte-level + conventions of the file it came from, so they are carried over from + the source instead. Required rather than defaulted: a caller that + omitted it would silently start rewriting regions it never touched, + which is the defect this parameter exists to prevent. tomlkit + preserves those conventions itself; only the JSON writer needs it. + """ + # Dispatched on the exact format rather than "not json": a third + # format reaching the TOML writer by fall-through would silently + # write TOML bytes into a JSON file. + if info.fmt == "toml": + return tomlkit.dumps(config).encode() + if info.fmt == "jsonc": + # The merge derives its output from the original text, so the + # file's own trailing-newline convention carries over untouched + # and needs no _json_trailer fixup. + source = original.decode() + try: + return _jsonc_merge(source, config, ensure_ascii=False).encode() + except UnicodeEncodeError: + return _jsonc_merge(source, config, ensure_ascii=True).encode() + trailer = _json_trailer(original) + # ensure_ascii would re-escape every non-ASCII character in the file, + # including config text the swap never read. + text = json.dumps(config, indent=2, ensure_ascii=False) + trailer + try: + return text.encode() + except UnicodeEncodeError: + # A lone surrogate — a JS writer slicing a string mid-pair — has no + # UTF-8 encoding. Escaping the document is then the only form that + # can be written at all. + return (json.dumps(config, indent=2) + trailer).encode() def atomic_write(path: pathlib.Path, data: bytes) -> None: - """Write bytes to ``path`` via tempfile + ``os.replace`` to avoid partial writes.""" - path.parent.mkdir(parents=True, exist_ok=True) - fd, tmp_name = tempfile.mkstemp(prefix=path.name + ".", dir=str(path.parent)) + """Write bytes to ``path`` without replacing a symlinked config. + + Parameters + ---------- + path : pathlib.Path + Destination path. A symlink resolves to its final target so the + write preserves every link in the chain. + data : bytes + Bytes to write atomically. + """ + target = path.resolve() if path.is_symlink() else path + target.parent.mkdir(parents=True, exist_ok=True) + mode = stat.S_IMODE(target.stat().st_mode) if target.exists() else None + fd, tmp_name = tempfile.mkstemp(prefix=target.name + ".", dir=str(target.parent)) tmp = pathlib.Path(tmp_name) try: with os.fdopen(fd, "wb") as fh: + if mode is not None: + os.fchmod(fh.fileno(), mode) fh.write(data) - tmp.replace(path) + tmp.replace(target) except Exception: tmp.unlink(missing_ok=True) raise @@ -483,6 +1044,72 @@ def _claude_user_servers( return existing +@t.overload +def _server_map( + info: CLIInfo, config: t.Any, *, create: t.Literal[True] +) -> dict[str, t.Any]: ... + + +@t.overload +def _server_map( + info: CLIInfo, config: t.Any, *, create: t.Literal[False] +) -> dict[str, t.Any] | None: ... + + +def _server_map( + info: CLIInfo, config: t.Any, *, create: bool +) -> dict[str, t.Any] | None: + """Walk ``info.container`` to the mapping holding this CLI's entries. + + Returns ``None`` when the path is absent and ``create`` is false. + Intermediate levels are created on demand so a nested container needs + no special case; TOML gets tomlkit tables so the written document + keeps its formatting. + + Raises + ------ + RuntimeError + A key along the path holds something other than a mapping. + Reported rather than overwritten — a swap must never discard + config it cannot interpret. + """ + node: dict[str, t.Any] = config + for depth, key in enumerate(info.container): + child = node.get(key) + if child is None: + if not create: + return None + child = tomlkit.table() if info.fmt == "toml" else {} + node[key] = child + elif not isinstance(child, dict): + path = ".".join(info.container[: depth + 1]) + msg = ( + f"{info.config_path}: {path} is a {type(child).__name__}, " + f"expected a table of server entries" + ) + raise RuntimeError(msg) + node = child + return node + + +def _as_toml_table(entry: dict[str, t.Any]) -> tomlkit.items.Table: + """Render one entry dict as a tomlkit table. + + Nested mappings (``env``) become sub-tables so the written document + keeps TOML's own structure instead of an inline dict literal. + """ + table = tomlkit.table() + for key, value in entry.items(): + if isinstance(value, dict): + sub = tomlkit.table() + for sub_key, sub_value in value.items(): + sub[sub_key] = sub_value + table[key] = sub + else: + table[key] = value + return table + + def get_server( cli: CLIName, config: t.Any, @@ -506,13 +1133,12 @@ def get_server( if not node: return None entry = node.get("mcpServers", {}).get(name) - elif cli in ("cursor", "gemini", "agy"): - entry = config.get("mcpServers", {}).get(name) - else: # cli in ("codex", "grok") - entry = config.get("mcp_servers", {}).get(name) + else: + servers = _server_map(CLIS[cli], config, create=False) + entry = servers.get(name) if servers else None if entry is None: return None - return _spec_from_entry(entry, fmt=CLIS[cli].fmt) + return _spec_from_entry(entry, info=CLIS[cli]) def set_server( @@ -536,37 +1162,23 @@ def set_server( if scope == "user": servers = _claude_user_servers(config, create=True) had = name in servers - servers[name] = spec.to_json_dict(include_stdio_type=True) + servers[name] = spec.to_entry_dict("claude") return "replaced" if had else "added" node = _claude_project_node(config, repo, create=True) servers = node.setdefault("mcpServers", {}) had = name in servers - servers[name] = spec.to_json_dict(include_stdio_type=True) - return "replaced" if had else "added" - if cli in ("cursor", "gemini", "agy"): - servers = config.setdefault("mcpServers", {}) - had = name in servers - servers[name] = spec.to_json_dict() - return "replaced" if had else "added" - if cli in ("codex", "grok"): - # tomlkit: top-level tables are accessed via dict protocol too. - mcp_servers = config.get("mcp_servers") - if mcp_servers is None: - mcp_servers = tomlkit.table() - config["mcp_servers"] = mcp_servers - had = name in mcp_servers - table = tomlkit.table() - table["command"] = spec.command - table["args"] = list(spec.args) - if spec.env: - env_tbl = tomlkit.table() - for k, v in spec.env.items(): - env_tbl[k] = v - table["env"] = env_tbl - mcp_servers[name] = table + servers[name] = spec.to_entry_dict("claude") return "replaced" if had else "added" - msg = f"unreachable: unknown CLI {cli!r}" - raise AssertionError(msg) + info = CLIS[cli] + if info.dialect == "opencode" and not config: + # Seeding from nothing: opencode rewrites the file on load to add + # this line, so writing it now avoids an immediate second edit. + config["$schema"] = OPENCODE_SCHEMA_URL + servers = _server_map(info, config, create=True) + had = name in servers + entry = spec.to_entry_dict(info.dialect) + servers[name] = _as_toml_table(entry) if info.fmt == "toml" else entry + return "replaced" if had else "added" def delete_server( @@ -594,33 +1206,45 @@ def delete_server( return False servers = node.get("mcpServers", {}) return servers.pop(name, None) is not None - if cli in ("cursor", "gemini", "agy"): - return config.get("mcpServers", {}).pop(name, None) is not None - if cli in ("codex", "grok"): - mcp_servers = config.get("mcp_servers") - if mcp_servers is None: - return False - if name in mcp_servers: - del mcp_servers[name] - return True + servers = _server_map(CLIS[cli], config, create=False) + if servers is None or name not in servers: return False - msg = f"unreachable: unknown CLI {cli!r}" - raise AssertionError(msg) + del servers[name] + return True + +def _spec_from_entry(entry: t.Any, *, info: CLIInfo) -> McpServerSpec: + """Convert a raw config entry (dict or tomlkit Table) into an McpServerSpec. -def _spec_from_entry(entry: t.Any, *, fmt: t.Literal["json", "toml"]) -> McpServerSpec: - """Convert a raw config entry (dict or tomlkit Table) into an McpServerSpec.""" + Every dialect is normalised down to the portable scalar-command + shape, so the helpers that reason about a spec — + :meth:`McpServerSpec.is_local_uv_directory`, :meth:`McpServerSpec.pr_ref`, + ``_points_at`` — stay dialect-agnostic. Skipping this is not a + cosmetic loss: an unsplit array command makes the "already local, no + change" check miss, and every run rewrites a config it did not need + to touch. + """ # tomlkit items quack like dicts/lists; coerce to plain Python for our spec. - if fmt == "toml": + if info.fmt == "toml": entry = ( tomlkit.items.Table.unwrap(entry) if isinstance(entry, tomlkit.items.Table) else dict(entry) ) - command = str(entry.get("command", "")) - raw_args = entry.get("args", []) - args = [str(a) for a in raw_args] if raw_args else [] - raw_env = entry.get("env") or {} + if info.dialect == "opencode": + raw_command = entry.get("command", []) + argv = ( + [str(part) for part in raw_command] + if isinstance(raw_command, (list, tuple)) + else [str(raw_command)] + ) + command, args = (argv[0], argv[1:]) if argv else ("", []) + raw_env = entry.get("environment") or {} + else: + command = str(entry.get("command", "")) + raw_args = entry.get("args", []) + args = [str(a) for a in raw_args] if raw_args else [] + raw_env = entry.get("env") or {} env = {str(k): str(v) for k, v in dict(raw_env).items()} return McpServerSpec(command=command, args=args, env=env) @@ -665,12 +1289,167 @@ def build_local_spec(repo: pathlib.Path, entry: str) -> McpServerSpec: ) +def build_pr_spec(repo_url: str, pr: int, entry: str) -> McpServerSpec: + """Build the ``uvx --from git+@refs/pull//head `` spec. + + Nothing is checked out: ``uv`` resolves the ref itself, so a swap + leaves no worktree to refresh or prune and ``revert`` needs no + cleanup beyond restoring the config. + """ + return McpServerSpec( + command="uvx", + args=["--from", f"git+{repo_url}@refs/pull/{pr}/head", entry], + ) + + +def _run_text(argv: list[str], cwd: pathlib.Path | None = None) -> str: + """Run ``argv`` and return stdout, raising on a non-zero exit.""" + return subprocess.run( + argv, + cwd=None if cwd is None else str(cwd), + capture_output=True, + text=True, + check=True, + ).stdout + + +def remote_https_url(repo: pathlib.Path, remote: str = "origin") -> str: + """Return ``https:////`` for a repo's git remote. + + Normalizes the spellings git accepts — ``git@host:owner/name.git``, + an ``ssh://`` or ``git+ssh://`` scheme, an embedded user, a trailing + ``.git`` — because the pull-request ref is fetched over https however + the working copy was cloned. + """ + try: + raw = _run_text(["git", "-C", str(repo), "remote", "get-url", remote]) + except (OSError, subprocess.CalledProcessError) as exc: + msg = f"cannot read git remote {remote!r} in {repo}" + raise RuntimeError(msg) from exc + return _normalize_remote_url(raw.strip()) + + +def _normalize_remote_url(url: str) -> str: + """Rewrite any git remote spelling as a plain https URL. + + Examples + -------- + >>> _normalize_remote_url("git+ssh://git@github.com/o/n.git") + 'https://github.com/o/n' + >>> _normalize_remote_url("git@github.com:o/n.git") + 'https://github.com/o/n' + >>> _normalize_remote_url("https://github.com/o/n") + 'https://github.com/o/n' + """ + url = url.removeprefix("git+") + if url.startswith("ssh://"): + url = "https://" + url.removeprefix("ssh://") + elif "://" not in url and ":" in url: + host, _, path = url.partition(":") + url = f"https://{host}/{path}" + scheme, sep, rest = url.partition("://") + authority, slash, path = rest.partition("/") + return f"{scheme}{sep}{authority.rpartition('@')[2]}{slash}{path}".removesuffix( + ".git" + ) + + +def gh_pr_summary(repo: pathlib.Path, pr: int) -> dict[str, t.Any] | None: + """Return ``gh``'s view of a pull request, or ``None`` when unreadable. + + Used to confirm the number exists and to label output. Resolution + does not depend on it: the ref and URL come from git, so a missing + or unauthenticated ``gh`` degrades to an unlabelled swap rather than + a failure. + """ + try: + out = _run_text( + [ + "gh", + "pr", + "view", + str(pr), + "--json", + "number,title,state,headRefName,isCrossRepository", + ], + cwd=repo, + ) + except (OSError, subprocess.CalledProcessError): + return None + try: + loaded = json.loads(out) + except json.JSONDecodeError: + return None + return loaded if isinstance(loaded, dict) else None + + +#: One MCP ``initialize`` request, newline-framed for stdio. +_INITIALIZE_FRAME = ( + json.dumps( + { + "jsonrpc": "2.0", + "id": 1, + "method": "initialize", + "params": { + "protocolVersion": "2025-06-18", + "capabilities": {}, + "clientInfo": {"name": "mcp_swap-preflight", "version": "1"}, + }, + } + ) + + "\n" +) + + +def preflight_spec(spec: McpServerSpec, *, timeout: float = 300.0) -> str | None: + """Launch ``spec`` and complete one MCP ``initialize`` round trip. + + Returns ``None`` when the server answered, otherwise a reason to + show the operator. A pull-request spec resolves its dependencies at + launch time, inside whichever agent starts it, so an unresolvable + ref would otherwise land in every config and surface later as an + opaque startup failure in each one. + + Closing stdin after the frame lets a well-behaved stdio server exit + on its own, which keeps this free of signal handling. + """ + try: + proc = subprocess.Popen( + [spec.command, *spec.args], + stdin=subprocess.PIPE, + stdout=subprocess.PIPE, + stderr=subprocess.PIPE, + env={**os.environ, **spec.env}, + text=True, + ) + except OSError as exc: + return f"could not launch {spec.command}: {exc}" + + try: + out, err = proc.communicate(_INITIALIZE_FRAME, timeout=timeout) + except subprocess.TimeoutExpired: + proc.kill() + proc.communicate() + return f"no MCP response within {timeout:.0f}s" + + for line in out.splitlines(): + try: + message = json.loads(line) + except json.JSONDecodeError: + continue + if isinstance(message, dict) and message.get("id") == 1 and "result" in message: + return None + + tail = "\n".join(err.strip().splitlines()[-3:]) + return tail or "server exited without answering initialize" + + # --------------------------------------------------------------------------- # State file # --------------------------------------------------------------------------- -def load_state() -> dict[tuple[CLIName, Scope], SwapEntry]: +def load_state(*, strict: bool = False) -> dict[tuple[CLIName, Scope], SwapEntry]: """Read the swap-state file, returning an empty mapping when absent. The state file's schema is internal — no compatibility contract — @@ -678,23 +1457,68 @@ def load_state() -> dict[tuple[CLIName, Scope], SwapEntry]: (those that don't parse as ``cli:scope``) and entries with a non-coercible ``seq_no`` or missing required fields are dropped silently so a hand-edited file cannot crash the script. + + A file that will not parse at all is reported rather than dropped + silently: it means the record of every swap is gone, so ``revert`` + is about to say there is nothing to unwind while swapped configs + and their backups sit on disk. Saying so is what lets the operator + go find those backups. Mutating callers pass ``strict=True`` so an + unreadable or malformed record blocks changes instead of being + overwritten as empty state. """ if not STATE_FILE.exists(): return {} - raw = json.loads(STATE_FILE.read_text()) + try: + raw = json.loads(STATE_FILE.read_text()) + except (OSError, ValueError) as exc: + message = f"swap state unreadable ({STATE_FILE}): {exc}" + print(message, file=sys.stderr) + if strict: + raise SwapStateError(message) from exc + return {} + if not isinstance(raw, dict): + if strict: + message = f"swap state has invalid shape: {STATE_FILE}" + print(message, file=sys.stderr) + raise SwapStateError(message) + return {} entries = raw.get("entries", {}) + if not isinstance(entries, dict): + if strict: + message = f"swap state has invalid entries: {STATE_FILE}" + print(message, file=sys.stderr) + raise SwapStateError(message) + entries = {} out: dict[tuple[CLIName, Scope], SwapEntry] = {} for k, v in entries.items(): parsed = _parse_state_key(k) if parsed is None: + if strict: + message = f"swap state has invalid key {k!r}: {STATE_FILE}" + print(message, file=sys.stderr) + raise SwapStateError(message) continue entry = _parse_state_entry(v) if entry is None: + if strict: + message = f"swap state has invalid entry {k!r}: {STATE_FILE}" + print(message, file=sys.stderr) + raise SwapStateError(message) continue out[parsed] = entry return out +@contextlib.contextmanager +def _state_lock() -> t.Iterator[None]: + """Serialize config mutations that share the swap state file.""" + STATE_DIR.mkdir(parents=True, exist_ok=True) + fd = os.open(STATE_DIR / "state.lock", os.O_RDWR | os.O_CREAT, 0o600) + with os.fdopen(fd, "a+b") as lock_file: + fcntl.flock(lock_file.fileno(), fcntl.LOCK_EX) + yield + + def save_state(entries: dict[tuple[CLIName, Scope], SwapEntry]) -> None: """Write the swap-state file atomically.""" STATE_DIR.mkdir(parents=True, exist_ok=True) @@ -707,13 +1531,10 @@ def save_state(entries: dict[tuple[CLIName, Scope], SwapEntry]) -> None: atomic_write(STATE_FILE, (json.dumps(payload, indent=2) + "\n").encode("utf-8")) -def clear_state(keys: t.Iterable[tuple[CLIName, Scope]]) -> None: - """Remove the given ``(cli, scope)`` keys; delete the file if empty.""" - current = load_state() - for key in keys: - current.pop(key, None) - if current: - save_state(current) +def _save_or_clear_state(entries: dict[tuple[CLIName, Scope], SwapEntry]) -> None: + """Persist ``entries``, removing the state file when the mapping is empty.""" + if entries: + save_state(entries) elif STATE_FILE.exists(): STATE_FILE.unlink() @@ -768,8 +1589,10 @@ def cmd_detect(args: argparse.Namespace) -> int: extra.append("binary missing") if not p.config_found: extra.append(f"config missing: {CLIS[p.cli].config_path}") + if p.cli == "pi" and not PI_ADAPTER_DIR.is_dir(): + extra.append(PI_ADAPTER_HINT) suffix = f" ({', '.join(extra)})" if extra else "" - print(f" [{flag}] {p.cli:<7}{suffix}") + print(f" [{flag}] {p.cli:<{_CLI_COLUMN}}{suffix}") return 0 @@ -838,27 +1661,44 @@ def cmd_status(args: argparse.Namespace) -> int: print( f"[{cli}] {server} = {spec.command} {' '.join(spec.args)} ({tag})" ) - except RuntimeError as exc: + except (RuntimeError, ValueError, OSError) as exc: print(f"[{cli}] {exc}", file=sys.stderr) continue return 0 def _describe_spec(spec: McpServerSpec, repo: pathlib.Path) -> str: - """Return a short label classifying a spec (local/pypi-pin/other).""" + """Return a short label classifying a spec (local/PR/pypi-pin/other).""" if spec.is_local_uv_directory(): local = spec.local_repo_path() if local and local.resolve() == repo.resolve(): return "local: this repo" return f"local: {local}" + pr = spec.pr_ref() + if pr is not None: + # Checked before the pin branch below: a PR ref contains `@`, + # which that branch would report as a version pin. + return f"PR #{pr[1]}: {pr[0]}" if spec.command == "uvx": pinned = next((a for a in spec.args if "==" in a or "@" in a), None) return f"pypi pin: {pinned}" if pinned else "pypi (unpinned)" return "other" -def cmd_use_local(args: argparse.Namespace) -> int: - """Rewrite each target CLI's config to run the repo's checkout via ``uv``. +def _points_at( + current: McpServerSpec, target: McpServerSpec, repo: pathlib.Path +) -> bool: + """Return True when ``current`` already runs what ``target`` describes.""" + if target.pr_ref() is not None: + return current.pr_ref() == target.pr_ref() + return current.is_local_uv_directory() and current.local_repo_path() == repo + + +def _cmd_use_local(args: argparse.Namespace) -> int: + """Rewrite each target CLI's config to run the repo, or a pull request. + + Without ``--pr`` the entry runs the repo's checkout via ``uv``; with + it, the pull request's head via ``uvx``. The optional ``--scope`` flag selects Claude's user-level fallback vs. per-project override; see :data:`Scope`. The flag is silently @@ -868,9 +1708,30 @@ def cmd_use_local(args: argparse.Namespace) -> int: server, default_entry = resolve_repo_meta(repo) server = args.server or server entry = args.entry or default_entry - spec = build_local_spec(repo, entry) extra_env = dict(args.env or []) + pr = getattr(args, "pr", None) + if pr is None: + spec = build_local_spec(repo, entry) + else: + try: + spec = build_pr_spec(remote_https_url(repo), pr, entry) + except RuntimeError as exc: + print(exc, file=sys.stderr) + return 1 + spec = dataclasses.replace(spec, env=dict(extra_env)) + summary = gh_pr_summary(repo, pr) + if summary is None: + print(f"PR #{pr}: gh could not read it — swapping anyway", file=sys.stderr) + else: + fork = " (fork)" if summary.get("isCrossRepository") else "" + print( + f"PR #{summary.get('number', pr)} [{summary.get('state', '?')}]" + f"{fork} {summary.get('headRefName', '?')} — " + f"{summary.get('title', '')}", + file=sys.stderr, + ) + hint = _naming_hint(repo, server) if hint: print(hint, file=sys.stderr) @@ -880,8 +1741,17 @@ def cmd_use_local(args: argparse.Namespace) -> int: print("no CLIs detected — nothing to do", file=sys.stderr) return 1 + # Runs under --dry-run too: resolving the ref is the only signal a + # dry run can give about whether the swap would actually start. + if pr is not None and not args.no_preflight: + print(f"preflight: {spec.command} {' '.join(spec.args)}", file=sys.stderr) + failure = preflight_spec(spec) + if failure is not None: + print(f"preflight failed, nothing written:\n{failure}", file=sys.stderr) + return 1 + ts = time.strftime("%Y%m%d%H%M%S") - state = load_state() + state = load_state(strict=True) had_error = 0 for cli in targets: scope = _normalize_scope(cli, args.scope) @@ -889,23 +1759,28 @@ def cmd_use_local(args: argparse.Namespace) -> int: info = CLIS[cli] if not info.config_path.exists(): print(f"[{label}] skip — config not found at {info.config_path}") + had_error = 1 continue - # Wrap the read + shape-guarded mutation in try/except RuntimeError - # so a malformed Claude config (top-level mcpServers / projects not a - # mapping) surfaces as a clean per-CLI error instead of an uncaught - # traceback. Same per-CLI continuation pattern the inner write-failure - # handler below uses. + target_path = info.config_path.resolve() + target_info = dataclasses.replace(info, config_path=target_path) + # Wrap the read + shape-guarded mutation so an unreadable config + # surfaces as a clean per-CLI error instead of an uncaught traceback. + # The three arms are the three ways it fails: a shape this script + # rejects raises RuntimeError, an unparseable one raises ValueError + # (JSON, TOML and UTF-8 decode errors all derive from it), and an + # unopenable one raises OSError. Same trio ``doctor`` catches, and + # the same per-CLI continuation the write-failure handler below uses. try: - original_bytes = info.config_path.read_bytes() - config = load_config(info) + original_bytes = target_path.read_bytes() + config = load_config(target_info) current = get_server(cli, config, server, repo, scope=scope) if ( current - and current.is_local_uv_directory() - and current.local_repo_path() == repo + and _points_at(current, spec, repo) and all(current.env.get(k) == v for k, v in extra_env.items()) ): - print(f"[{label}] already local (this repo) — no change") + where = "local (this repo)" if pr is None else f"PR #{pr}" + print(f"[{label}] already {where} — no change") continue # Preserve the existing entry's env on replacement. ``build_local_spec`` # writes an empty env, so without this merge a swap would silently drop @@ -920,8 +1795,8 @@ def cmd_use_local(args: argparse.Namespace) -> int: else spec ) action = set_server(cli, config, server, cli_spec, repo, scope=scope) - new_bytes = dump_config_bytes(info, config) - except RuntimeError as exc: + new_bytes = dump_config_bytes(info, config, original=original_bytes) + except (RuntimeError, ValueError, OSError) as exc: print(f"[{label}] {exc}", file=sys.stderr) had_error = 1 continue @@ -966,22 +1841,21 @@ def cmd_use_local(args: argparse.Namespace) -> int: backup_suffix = f"{BACKUP_SUFFIX_PREFIX}{ts}" if cli == "claude": backup_suffix += f"-{scope}" - backup_path = write_new_backup( - info.config_path.with_suffix(info.config_path.suffix + backup_suffix), - original_bytes, - ) + # A backup that cannot be written must abort this CLI rather + # than degrade into a swap with nothing to revert to — an + # unwritable directory is the case that produces both. + try: + backup_path = write_new_backup( + info.config_path.with_suffix( + info.config_path.suffix + backup_suffix + ), + original_bytes, + ) + except OSError as exc: + print(f"[{label}] cannot write backup: {exc}", file=sys.stderr) + had_error = 1 + continue backup_note = f"backup: {backup_path}" - try: - atomic_write(info.config_path, new_bytes) - _revalidate(info) - except Exception as exc: - atomic_write(info.config_path, original_bytes) - print( - f"[{label}] write failed ({exc}); backup at {backup_path}", - file=sys.stderr, - ) - had_error = 1 - continue if prior is not None and backup_path == prior_backup: # ``swapped_at`` mirrors the timestamp in the backup filename # and ``seq_no`` fixes the backup's place in the unwind @@ -990,27 +1864,76 @@ def cmd_use_local(args: argparse.Namespace) -> int: else: seq_no = max((e.seq_no for e in state.values()), default=-1) + 1 swapped_at = ts - state[(cli, scope)] = SwapEntry( + next_state = dict(state) + next_state[(cli, scope)] = SwapEntry( config_path=str(info.config_path), backup_path=str(backup_path), server=server, action=action, swapped_at=swapped_at, seq_no=seq_no, + target_path=str(target_path), ) + try: + save_state(next_state) + except OSError as exc: + print( + f"[{label}] cannot save recovery state ({exc}); config unchanged; " + f"backup at {backup_path}", + file=sys.stderr, + ) + had_error = 1 + continue + previous_state = state + state = next_state + try: + atomic_write(target_path, new_bytes) + _revalidate(target_info) + except Exception as exc: + try: + atomic_write(target_path, original_bytes) + except Exception as rollback_exc: + rollback_note = f"; rollback failed ({rollback_exc})" + else: + rollback_note = "; original config restored" + try: + _save_or_clear_state(previous_state) + except OSError as state_exc: + rollback_note += f"; recovery state cleanup failed ({state_exc})" + else: + state = previous_state + print( + f"[{label}] write failed ({exc}){rollback_note}; " + f"backup at {backup_path}", + file=sys.stderr, + ) + had_error = 1 + continue print(f"[{label}] {action}; {backup_note}") - if not args.dry_run: - save_state(state) return had_error +def cmd_use_local(args: argparse.Namespace) -> int: + """Run :func:`_cmd_use_local` under the shared mutation lock.""" + if args.dry_run: + return _cmd_use_local(args) + try: + with _state_lock(): + return _cmd_use_local(args) + except SwapStateError: + return 1 + except OSError as exc: + print(f"swap state unavailable: {exc}", file=sys.stderr) + return 1 + + def _revalidate(info: CLIInfo) -> None: """Re-parse the file after writing; raise on failure.""" load_config(info) -def cmd_revert(args: argparse.Namespace) -> int: +def _cmd_revert(args: argparse.Namespace) -> int: """Restore each target CLI's config from the backup recorded in the state file. Without ``--scope``, every recorded entry for the targeted CLIs is @@ -1019,14 +1942,14 @@ def cmd_revert(args: argparse.Namespace) -> int: the matching scope is reverted; the parameter is silently coerced to ``"user"`` for non-Claude CLIs. """ - state = load_state() + state = load_state(strict=True) # Without --cli, revert every CLI that has any recorded swap. targets = list(args.cli) if args.cli else list({cli for cli, _scope in state}) if not targets: print("no recorded swaps — nothing to revert", file=sys.stderr) return 1 - reverted: list[tuple[CLIName, Scope]] = [] + had_error = 0 for cli in targets: if args.scope is not None: wanted_scopes: tuple[Scope, ...] = (_normalize_scope(cli, args.scope),) @@ -1059,28 +1982,56 @@ def cmd_revert(args: argparse.Namespace) -> int: entry = state[key] label = f"{sc_cli}:{sc_scope}" if sc_cli == "claude" else sc_cli backup = pathlib.Path(entry.backup_path) - dest = pathlib.Path(entry.config_path) + dest = pathlib.Path(entry.target_path or entry.config_path) if not backup.exists(): print(f"[{label}] backup missing: {backup}", file=sys.stderr) - continue + had_error = 1 + break if args.dry_run: print(f"[{label}] would restore {dest} from {backup}") continue - atomic_write(dest, backup.read_bytes()) - # Backup served its purpose; LIFO unwind for this layer is - # complete. Delete on success, keep on error — same idiom - # CPython's ``tempfile.NamedTemporaryFile`` uses - # (Lib/tempfile.py:614-618). If ``atomic_write`` had raised, - # this line wouldn't run and the backup would survive for - # post-mortem; on success the backup is redundant and would - # otherwise accumulate forever across swap/revert cycles. - backup.unlink() + try: + atomic_write(dest, backup.read_bytes()) + except OSError as exc: + print(f"[{label}] restore failed: {exc}", file=sys.stderr) + had_error = 1 + break + next_state = dict(state) + next_state.pop(key) + try: + _save_or_clear_state(next_state) + except OSError as exc: + print( + f"[{label}] restored, but recovery state could not be updated: " + f"{exc}", + file=sys.stderr, + ) + had_error = 1 + break + state = next_state + try: + backup.unlink() + except OSError as exc: + print( + f"[{label}] restored; backup cleanup failed: {exc}", file=sys.stderr + ) + had_error = 1 print(f"[{label}] restored from {backup}") - reverted.append(key) + return had_error - if not args.dry_run and reverted: - clear_state(reverted) - return 0 + +def cmd_revert(args: argparse.Namespace) -> int: + """Run :func:`_cmd_revert` under the shared mutation lock.""" + if args.dry_run: + return _cmd_revert(args) + try: + with _state_lock(): + return _cmd_revert(args) + except SwapStateError: + return 1 + except OSError as exc: + print(f"swap state unavailable: {exc}", file=sys.stderr) + return 1 # --------------------------------------------------------------------------- @@ -1110,6 +2061,19 @@ def _env_pair(raw: str) -> tuple[str, str]: return key, value +def _pr_number(raw: str) -> int: + """Parse a ``--pr`` argument as a pull-request number, or raise for argparse.""" + try: + number = int(raw) + except ValueError: + msg = f"--pr expects a number, got {raw!r}" + raise argparse.ArgumentTypeError(msg) from None + if number < 1: + msg = f"--pr expects a positive number, got {number}" + raise argparse.ArgumentTypeError(msg) + return number + + def _config_present_clis() -> list[CLIName]: """CLIs whose config file exists — enough to *read* entries (no binary needed). @@ -1140,17 +2104,15 @@ def _add(raw: t.Any) -> None: for name, entry in raw.items(): if not isinstance(entry, dict): continue - out[str(name)] = _spec_from_entry(entry, fmt=CLIS[cli].fmt) + out[str(name)] = _spec_from_entry(entry, info=CLIS[cli]) if cli == "claude": _add(_claude_user_servers(config, create=False)) node = _claude_project_node(config, repo, create=False) if node: _add(node.get("mcpServers")) - elif cli in ("cursor", "gemini", "agy"): - _add(config.get("mcpServers")) - else: # codex, grok - _add(config.get("mcp_servers")) + else: + _add(_server_map(CLIS[cli], config, create=False)) return out @@ -1318,8 +2280,30 @@ def build_parser() -> argparse.ArgumentParser: ) ps.set_defaults(func=cmd_status) - pu = sub.add_parser("use-local", help="rewrite configs to run this checkout") + pu = sub.add_parser( + "use-local", help="rewrite configs to run this checkout, or a pull request" + ) pu.add_argument("--repo", default=".", help="repo root (default: .)") + pu.add_argument( + "--pr", + type=_pr_number, + metavar="N", + help=( + "Point the CLIs at pull request N instead of the working copy. " + "Writes 'uvx --from git+@refs/pull/N/head ', so " + "nothing is checked out and 'revert' needs no cleanup. The ref " + "lives on the base repo, so fork PRs work unchanged." + ), + ) + pu.add_argument( + "--no-preflight", + action="store_true", + help=( + "Skip the MCP initialize round trip --pr runs before writing. " + "The probe resolves the ref once so a bad PR fails here instead " + "of inside every agent; skip it when offline or already warm." + ), + ) pu.add_argument( "--server", help="MCP server name (default: derived from pyproject.toml)" ) diff --git a/src/libtmux_mcp/__about__.py b/src/libtmux_mcp/__about__.py index 24b7c76f..fcf3b6c8 100644 --- a/src/libtmux_mcp/__about__.py +++ b/src/libtmux_mcp/__about__.py @@ -4,7 +4,7 @@ __title__ = "libtmux-mcp" __package_name__ = "libtmux_mcp" -__version__ = "0.1.0a19" +__version__ = "0.1.0a20" __description__ = "MCP server for tmux, powered by libtmux" __author__ = "Tony Narlock" __email__ = "tony@git-pull.com" diff --git a/src/libtmux_mcp/_utils.py b/src/libtmux_mcp/_utils.py index 587e2d85..9ba8f2e0 100644 --- a/src/libtmux_mcp/_utils.py +++ b/src/libtmux_mcp/_utils.py @@ -7,23 +7,26 @@ from __future__ import annotations import dataclasses +import difflib import functools import json import logging import os import pathlib +import re import threading import typing as t from fastmcp.exceptions import ToolError from libtmux import exc -from libtmux._internal.query_list import LOOKUP_NAME_MAP +from libtmux._internal.query_list import LOOKUP_NAME_MAP, QueryList from libtmux.server import Server if t.TYPE_CHECKING: from libtmux.pane import Pane from libtmux.session import Session from libtmux.window import Window + from pydantic import BaseModel from libtmux_mcp.models import PaneInfo, SessionInfo, WindowInfo @@ -470,6 +473,40 @@ def _caller_is_strictly_on_server( } +#: POSIX portable environment variable name. +_ENV_NAME_RE = re.compile(r"^[A-Za-z_][A-Za-z0-9_]*$") + + +def _raise_if_flag_like(label: str, value: str) -> None: + """Refuse a caller string tmux would parse as a flag. + + tmux reads flags before quoting can protect anything, and libtmux + emits ``[name, value]`` with no ``--`` terminator. So a leading + ``-`` substitutes one command for another silently: measured, + ``set_environment(name="-u", value="VICTIM")`` UNSET ``VICTIM`` and + reported ``status="set"``, and ``set_option(option="-g", value="x")`` + turned off ``xterm-keys`` because tmux prefix-matched ``x``. + """ + if value.startswith("-"): + msg = ( + f"{label} may not begin with '-': tmux parses it as a flag, so " + f"the call would run a different command than the one requested " + f"(got {value!r})." + ) + raise ExpectedToolError(msg) + + +def _raise_if_not_env_name(name: str) -> None: + """Refuse an environment variable name tmux or POSIX cannot hold.""" + if not _ENV_NAME_RE.match(name): + msg = ( + f"Environment variable name must match [A-Za-z_][A-Za-z0-9_]* " + f"(got {name!r}). tmux stores anything else verbatim as an " + "unusable name, and a leading '-' is read as a flag." + ) + raise ExpectedToolError(msg) + + def _tmux_argv(server: Server, *tmux_args: str) -> list[str]: """Build a full tmux argv list honouring ``socket_name`` and ``socket_path``. @@ -549,22 +586,32 @@ def _get_server( cache_key = (socket_name, socket_path, tmux_bin) with _server_cache_lock: - if cache_key in _server_cache: - cached = _server_cache[cache_key] - if not cached.is_alive(): + cached = _server_cache.get(cache_key) + + # ``is_alive()`` is a tmux subprocess round trip. Holding the cache + # lock across it serialises every concurrent tool call in this + # process behind one another -- measured, it capped a 16-way + # parallel socket scan at about 2x instead of 8x. + if cached is not None: + if cached.is_alive(): + return cached + with _server_cache_lock: + if _server_cache.get(cache_key) is cached: del _server_cache[cache_key] - if cache_key not in _server_cache: - kwargs: dict[str, t.Any] = {} - if socket_name is not None: - kwargs["socket_name"] = socket_name - if socket_path is not None: - kwargs["socket_path"] = socket_path - if tmux_bin is not None: - kwargs["tmux_bin"] = tmux_bin - _server_cache[cache_key] = Server(**kwargs) - - return _server_cache[cache_key] + kwargs: dict[str, t.Any] = {} + if socket_name is not None: + kwargs["socket_name"] = socket_name + if socket_path is not None: + kwargs["socket_path"] = socket_path + if tmux_bin is not None: + kwargs["tmux_bin"] = tmux_bin + server = Server(**kwargs) + + # Two threads racing to fill the same key both build a valid handle; + # ``setdefault`` makes them agree on which one the cache keeps. + with _server_cache_lock: + return _server_cache.setdefault(cache_key, server) def _invalidate_server( @@ -841,10 +888,175 @@ def _coerce_dict_arg( return value +@functools.cache +def _filterable_fields(obj_type: type) -> frozenset[str]: + """Attribute names a filter key may begin with. + + ``QueryList`` resolves a key by ``getattr`` traversal and treats a + miss as "no match", so an unknown field silently filters every row + out and an empty result is indistinguishable from a typo. + + Deliberately permissive: it rejects names the type cannot have and + accepts everything else, because ``__`` traversal into a nested + object is legitimate and only the first segment is checkable here. + """ + names = {name for name in dir(obj_type) if not name.startswith("_")} + if dataclasses.is_dataclass(obj_type): + names |= {field.name for field in dataclasses.fields(obj_type)} + return frozenset(names) + + +_MODEL_FIELD_ALIASES: dict[str, str] = { + "window_count": "session_windows", + "pane_count": "window_panes", + "active_pane_id": "active_pane__pane_id", +} +"""Output fields tmux exposes under a different attribute name.""" + + +def _admits_bool(annotation: t.Any) -> bool: + """Whether a model field's annotation can hold a bool.""" + return annotation is bool or bool in t.get_args(annotation) + + +_BOOL_TRUE = frozenset({"true", "1", "yes"}) +_BOOL_FALSE = frozenset({"false", "0", "no"}) + +#: Operators that mean anything against a bool. The rest are string or +#: collection tests; libtmux's lookups fall through to ``return False`` +#: for a bool, so allowing them would answer every query with an empty +#: list -- including contradictory pairs like ``__in``/``__nin``. +_BOOL_OPERATORS = frozenset({"exact", "eq"}) + + +def _coerce_model_value(key: str, value: t.Any, annotation: t.Any) -> t.Any: + """Coerce a filter value to what the model field actually holds. + + ``filters`` is typed ``dict[str, str]``, so a bool field is + addressed as ``"true"``; comparing that to ``True`` never matches. + An unrecognised token is rejected rather than compared as a string, + which would report "nothing matched" for a typo. + """ + if isinstance(value, str) and _admits_bool(annotation): + lowered = value.strip().lower() + if lowered in _BOOL_TRUE: + return True + if lowered in _BOOL_FALSE: + return False + msg = ( + f"Filter '{key}' takes a boolean, got {value!r}. Use one of: " + f"{', '.join(sorted(_BOOL_TRUE | _BOOL_FALSE))}." + ) + raise ExpectedToolError(msg) + return value + + +def _path_resolves(item: t.Any, path: str) -> bool: + """Whether ``path``'s ``__``-separated segments resolve on ``item``. + + ``None`` ends the walk only at an INTERMEDIATE segment, where there + is genuinely nothing to traverse into. On the terminal segment it is + an ordinary value -- tmux leaves many format fields empty, so + ``active_pane__pane_start_command`` is None on every shell pane -- + and treating that as unresolvable turns a true empty result into a + false error. + """ + current = item + segments = path.split("__") + last = len(segments) - 1 + for i, segment in enumerate(segments): + try: + current = getattr(current, segment) + except Exception: # noqa: BLE001 - any failure means "no such path" + return False + if current is None: + return i == last + return True + + +def _attribute_access_error(probe: list[t.Any], field: str) -> str | None: + """Message if ``field`` raises on every probed item, else ``None``. + + libtmux keeps removed properties around so they raise a message + naming the replacement. ``dir()`` still lists them, so they reach + callers as filterable; ``QueryList`` swallows the raise and answers + an empty list. Surfacing libtmux's own message is what makes the + refusal useful. + + One item settles it: the raise comes from the class, so it cannot + differ per instance. + """ + if not probe: + return None + try: + getattr(probe[0], field) + except Exception as exc: # noqa: BLE001 - reported, not handled + return str(exc) + return None + + +def _unknown_field_message( + key: str, + field: str, + allowed_fields: frozenset[str], + model_fields: t.Mapping[str, t.Any], + obj_type: type, +) -> str: + """Build the error for a filter key naming no known field.""" + msg = f"Unknown filter field '{field}' in '{key}'." + known = sorted(set(allowed_fields) | set(model_fields)) + close = difflib.get_close_matches(field, known, n=3) + if close: + msg += f" Did you mean: {', '.join(close)}?" + return ( + f"{msg} Every field this tool returns is filterable: " + f"{', '.join(sorted(model_fields))}. libtmux " + f"{obj_type.__name__} attributes are accepted too, though tmux " + "leaves many of them empty." + ) + + +def _raise_if_path_unresolvable( + probe: list[t.Any], + field_path: str, + key: str, + valid_ops: list[str], + *, + operator_parsed: bool, +) -> None: + """Reject a multi-segment path no item can resolve. + + Guards the traversal fallback: without this, a mistyped operator + (``session_name__containss``) reads as a path, resolves on nothing + and filters every row out -- the silent-empty answer this module + exists to prevent. Only provable when something is there to probe, + so an empty list is left alone. + """ + if "__" not in field_path: + return + if not probe or any(_path_resolves(item, field_path) for item in probe): + return + msg = f"Filter '{key}' names no attribute path on any item." + if not operator_parsed: + # Only a key with no operator can be a mistyped one. When an + # operator WAS parsed off, blaming the last path segment for + # not being one denies the operator the caller supplied. + trailing = field_path.rsplit("__", 1)[1] + close = difflib.get_close_matches(trailing, valid_ops, n=3) + msg += ( + f" '{trailing}' is not a filter operator either; did you mean '{close[0]}'?" + if close + else f" '{trailing}' is not a filter operator either." + ) + raise ExpectedToolError(msg) + + def _apply_filters( items: t.Any, filters: dict[str, str] | str | None, serializer: t.Callable[..., M], + obj_type: type, + model_type: type[BaseModel], ) -> list[M]: """Apply QueryList filters and serialize results. @@ -858,6 +1070,14 @@ def _apply_filters( If None or empty, all items are returned. serializer : callable Serializer function to convert each item to a model. + obj_type : type + libtmux class of the filtered items, used to validate filter + field names. Taken as a parameter rather than read off the + first item so an empty list still validates -- an empty result + is exactly when a typo most needs reporting. + model_type : type + Model ``serializer`` returns. Its fields are filterable too, so + that filtering by what a listing displayed always works. Returns ------- @@ -867,7 +1087,8 @@ def _apply_filters( Raises ------ ExpectedToolError - If a filter key uses an invalid lookup operator. + If a filter key uses an invalid lookup operator or names a + field the object cannot have. """ coerced = _coerce_dict_arg("filters", filters) if not coerced: @@ -875,18 +1096,58 @@ def _apply_filters( filters = coerced valid_ops = sorted(LOOKUP_NAME_MAP.keys()) - for key in filters: + allowed_fields = _filterable_fields(obj_type) + model_fields = model_type.model_fields + attr_filters: dict[str, t.Any] = {} + model_filters: dict[str, t.Any] = {} + probe = list(items) + + for key, value in filters.items(): + # A trailing segment that is not an operator is part of the + # attribute path, matching QueryList: it treats an unknown + # trailing segment as a path and defaults the operator to + # ``exact``, so ``active_pane__pane_id`` traverses. + field_path, op = key, "" if "__" in key: - _field, op = key.rsplit("__", 1) - if op not in LOOKUP_NAME_MAP: + lhs, trailing = key.rsplit("__", 1) + if trailing in LOOKUP_NAME_MAP: + field_path, op = lhs, trailing + + field = field_path.split("__", 1)[0] + if field in allowed_fields: + removed = _attribute_access_error(probe, field) + if removed is not None: + msg = f"Filter field '{field}' cannot be read: {removed}" + raise ExpectedToolError(msg) + _raise_if_path_unresolvable( + probe, field_path, key, valid_ops, operator_parsed=bool(op) + ) + attr_filters[key] = value + elif field in _MODEL_FIELD_ALIASES: + attr_filters[_MODEL_FIELD_ALIASES[field] + key[len(field) :]] = value + elif field in model_fields: + annotation = model_fields[field].annotation + if _admits_bool(annotation) and op and op not in _BOOL_OPERATORS: msg = ( - f"Invalid filter operator '{op}' in '{key}'. " - f"Valid operators: {', '.join(valid_ops)}" + f"Operator '{op}' does not apply to boolean field " + f"'{field}'. Use {' or '.join(sorted(_BOOL_OPERATORS))}, " + "or omit the operator." ) raise ExpectedToolError(msg) + # Computed server-side, so it exists only after serializing. + model_filters[key] = _coerce_model_value(key, value, annotation) + else: + raise ExpectedToolError( + _unknown_field_message( + key, field, allowed_fields, model_fields, obj_type + ) + ) - filtered = items.filter(**filters) - return [serializer(item) for item in filtered] + filtered = items.filter(**attr_filters) if attr_filters else items + results = [serializer(item) for item in filtered] + if model_filters: + results = list(QueryList(results).filter(**model_filters)) + return results def _serialize_session(session: Session) -> SessionInfo: @@ -1029,6 +1290,69 @@ def _serialize_pane(pane: Pane) -> PaneInfo: R = t.TypeVar("R") +#: tmux stderr fragments that mean the socket genuinely has no daemon +#: behind it. Anything else on a failed ``list-sessions`` -- a protocol +#: mismatch, a permission error -- means a server that exists and cannot +#: be talked to, which is a different answer. +_NO_SERVER_MARKERS = ( + "no server running", + "no such file or directory", + "error connecting to", +) + + +def _probe_liveness(server: Server) -> tuple[bool, str | None]: + """Return ``(alive, unreachable_reason)`` for *server*. + + ``Server.is_alive()`` answers False for a socket with no daemon AND + for a live server this tmux binary cannot speak to, and + ``Server.sessions`` degrades to ``[]`` in both cases. libtmux's own + docstring points at ``is_alive`` to tell those apart, but it cannot: + both collapse to the same False. + + The difference matters because they warrant opposite reactions. "No + server" is a fact an agent can act on; "cannot reach the server" over + a socket whose daemon is running -- an ordinary tmux upgrade leaves + sockets older than the binary -- reported as False tells the agent + the user's work is gone. tmux distinguishes them on stderr, so read + it rather than the boolean. + """ + try: + result = server.cmd("list-sessions") + except Exception as err: # noqa: BLE001 - probe must not raise + return False, str(err) + + if result.returncode == 0: + return True, None + + detail = " ".join(result.stderr).strip() if result.stderr else "" + lowered = detail.lower() + if any(marker in lowered for marker in _NO_SERVER_MARKERS): + return False, None + return False, detail or f"tmux exited with status {result.returncode}" + + +def _undouble(prefix: str, text: str) -> str: + """Drop *prefix* from *text* when the wrapper is about to add it back.""" + return text.removeprefix(prefix) + + +def _is_format_newline_parse_error(e: BaseException) -> bool: + """Detect libtmux failing to parse a format value containing a newline. + + libtmux <= 0.62.0 splits ``-F`` output one line per object, so a + newline inside any value (a pane's current directory, most reachably) + splits that record and its strict ``zip`` raises. It surfaces as a + bare ``ValueError`` and would otherwise reach the agent as + "Unexpected error", logged at ERROR, naming nothing it can act on. + + Matched on the message because the raise site is a stdlib ``zip`` + with no dedicated exception type. Kept even once the floor moves + past the libtmux fix: the installed version is not ours to choose. + """ + return isinstance(e, ValueError) and "zip()" in str(e) + + def _map_exception_to_tool_error(fn_name: str, e: BaseException) -> ToolError: """Translate a libtmux / unexpected exception into a ``ToolError``. @@ -1066,9 +1390,21 @@ def _map_exception_to_tool_error(fn_name: str, e: BaseException) -> ToolError: ) if isinstance(e, exc.PaneNotFound): return ExpectedToolError( - f"Pane not found: {e}", + f"Pane not found: {_undouble('Pane not found: ', str(e))}", suggestion="Call list_panes to discover valid pane ids.", ) + if _is_format_newline_parse_error(e): + return ExpectedToolError( + "tmux listing could not be parsed: a format value contains a " + "newline, almost always a pane whose current directory has one " + "in its name. Every pane on this server is affected, not just " + "that one, because pane lookup enumerates them all.", + suggestion=( + "Find it with: tmux list-panes -a -F " + "'#{pane_id} #{pane_current_path}' | cat -A — then move or " + "rename that directory. Upgrading libtmux also fixes it." + ), + ) if isinstance(e, exc.LibTmuxException): return ExpectedToolError(f"tmux error: {e}") logger.exception("unexpected error in MCP tool %s", fn_name) diff --git a/src/libtmux_mcp/middleware.py b/src/libtmux_mcp/middleware.py index 9226bf2a..d0da40e8 100644 --- a/src/libtmux_mcp/middleware.py +++ b/src/libtmux_mcp/middleware.py @@ -32,6 +32,7 @@ import time import typing as t +from fastmcp.exceptions import PromptError, ResourceError from fastmcp.server.middleware import Middleware, MiddlewareContext from fastmcp.server.middleware.error_handling import ( ErrorHandlingMiddleware, @@ -40,7 +41,8 @@ from fastmcp.server.middleware.response_limiting import ResponseLimitingMiddleware from fastmcp.tools.base import ToolResult from libtmux import exc as libtmux_exc -from mcp.types import CallToolRequestParams, TextContent +from mcp import McpError +from mcp.types import CallToolRequestParams, ErrorData, TextContent from pydantic import ValidationError as PydanticValidationError from libtmux_mcp._utils import ( @@ -51,25 +53,65 @@ ExpectedToolError, ) +#: Errors this server raises deliberately to describe a CALLER-caused +#: failure. They must never reach the ``-32603`` "Internal error" path, +#: whichever message kind raised them: ``ExpectedToolError`` from tools, +#: and fastmcp's ``ResourceError`` / ``PromptError`` from the resource +#: and prompt handlers, which this package raises only for a bad target +#: or a missing object. +_CALLER_CAUSED_ERRORS: tuple[type[Exception], ...] = ( + ExpectedToolError, + ResourceError, + PromptError, +) + +logger = logging.getLogger(__name__) + _TIER_LEVELS: dict[str, int] = { TAG_READONLY: 0, TAG_MUTATING: 1, TAG_DESTRUCTIVE: 2, } +#: Reverse of :data:`_TIER_LEVELS`, so a middleware configured with an +#: unrecognized tier can still *name* the tier it fell back to. +_LEVEL_TIERS: dict[int, str] = {level: tier for tier, level in _TIER_LEVELS.items()} + + +def _highest_tier(tags: t.Collection[str]) -> str | None: + """Return the highest safety tier named in *tags*, or None if untagged.""" + found = [tier for tier in _TIER_LEVELS if tier in tags] + if not found: + return None + return max(found, key=lambda tier: _TIER_LEVELS[tier]) + class SafetyMiddleware(Middleware): - """Gate tools by safety tier. + """Explain tier denials that ``_enable_allowed_tools`` enforces. + + FastMCP's native ``disable()`` is the enforcement gate and holds + even for a call that skips the middleware chain. It also makes + ``get_tool()`` answer **None**, so FastMCP reports an off-tier call + as ``Unknown tool`` -- the server denying its own gated tool exists. + This gate names the required tier instead, resolving such names + against :meth:`_tier_snapshot` (the registry keeps disabled tools). + + Denials must raise *here* so :class:`AuditMiddleware`, which sits + outside, records them as denials rather than unknown-tool errors. Parameters ---------- max_tier : str - Maximum allowed tier. One of ``TAG_READONLY``, ``TAG_MUTATING``, - or ``TAG_DESTRUCTIVE``. + Maximum allowed tier. Unrecognized values fall back to + ``TAG_READONLY``. """ def __init__(self, max_tier: str = TAG_MUTATING) -> None: self.max_level = _TIER_LEVELS.get(max_tier, 0) + #: Normalized tier name, so a denial reports where the server + #: stands rather than echoing an unrecognized env value back. + self.max_tier = _LEVEL_TIERS[self.max_level] + self._tier_by_tool: dict[str, str] | None = None def _is_allowed(self, tags: set[str]) -> bool: """Return True if the tool's tags fall within the allowed tier. @@ -84,6 +126,55 @@ def _is_allowed(self, tags: set[str]) -> bool: return False return found_tier + def _denial_message(self, tool_name: str, required_tier: str | None) -> str: + """Name the required tier and the active one. + + The message this replaced hardcoded ``destructive`` for every + denial, so a readonly server answered ``send_keys`` by advising + kill_server rights in order to type into a pane. + """ + if required_tier is None: + return ( + f"Tool {tool_name!r} declares no safety tier and is blocked. " + "This is a bug in the server, not a configuration problem; " + "please report it." + ) + return ( + f"Tool {tool_name!r} requires safety level {required_tier!r}, but " + f"this server is running at {self.max_tier!r}. Restart it with " + f"LIBTMUX_SAFETY={required_tier} to enable it." + ) + + async def _tier_snapshot(self, fastmcp: t.Any) -> dict[str, str]: + """Map every registered tool name to its tier, disabled included. + + Built once. ``_list_tools()`` is FastMCP-private, so a failure + degrades to an empty map: the call then falls through to the + stock ``NotFoundError``, losing the explanation but nothing + else. ``tests/test_server.py`` pins the behavior so a fastmcp + bump fails in CI rather than silently reverting this gate. + """ + if self._tier_by_tool is not None: + return self._tier_by_tool + + snapshot: dict[str, str] = {} + try: + registered = await fastmcp._list_tools() + except Exception: + logger.warning( + "safety tier snapshot unavailable; off-tier calls will " + "report 'unknown tool'", + exc_info=True, + ) + else: + for tool in registered: + tier = _highest_tier(tool.tags) + if tier is not None: + snapshot[tool.name] = tier + + self._tier_by_tool = snapshot + return snapshot + async def on_list_tools( self, context: MiddlewareContext, @@ -98,16 +189,36 @@ async def on_call_tool( context: MiddlewareContext, call_next: t.Any, ) -> t.Any: - """Block execution of tools above the safety tier.""" - if context.fastmcp_context: - tool = await context.fastmcp_context.fastmcp.get_tool(context.message.name) - if tool and not self._is_allowed(tool.tags): - msg = ( - f"Tool '{context.message.name}' is not available at the " - f"current safety level. Set LIBTMUX_SAFETY=destructive " - f"to enable destructive tools." + """Block execution of tools above the safety tier. + + Fail-closed except for a name the registry has never heard of, + which is a typo and deserves FastMCP's own ``NotFoundError``. + """ + tool_name = context.message.name + + if context.fastmcp_context is None: + # No registry to consult: deny, since the top tier includes + # kill_server. + msg = ( + f"Tool {tool_name!r} was called without a FastMCP context, so " + "its safety tier cannot be verified. Call it through MCP." + ) + raise ExpectedToolError(msg) + + fastmcp = context.fastmcp_context.fastmcp + + tool = await fastmcp.get_tool(tool_name) + if tool is not None: + if not self._is_allowed(tool.tags): + raise ExpectedToolError( + self._denial_message(tool_name, _highest_tier(tool.tags)) ) - raise ExpectedToolError(msg) + return await call_next(context) + + # Invisible to ``get_tool``: gated by tier, or nonexistent. + gated_tier = (await self._tier_snapshot(fastmcp)).get(tool_name) + if gated_tier is not None: + raise ExpectedToolError(self._denial_message(tool_name, gated_tier)) return await call_next(context) @@ -430,6 +541,35 @@ def _log_error(self, error: Exception, context: MiddlewareContext) -> None: except Exception: self.logger.exception("Error in error callback") + def _transform_error( + self, + error: Exception, + context: MiddlewareContext, + ) -> Exception: + """Keep caller-caused failures out of the ``-32603`` catch-all. + + The base transform funnels every unrecognized exception into + ``"Internal error: ..."``. That is right for a bug and wrong for + "that session does not exist". It also runs for EVERY message + kind, so intercepting only ``tools/call`` below left resources + reporting a caller's mistake as a server fault: + ``tmux://sessions/nosuchsession`` answered + ``Internal error: Session not found: nosuchsession``. + + Fixed at the fork rather than by adding one resource hook. The + property is "an expected failure is never an internal error", + and it has to hold on every path this transform serves. + """ + if self.transform_errors and isinstance(error, _CALLER_CAUSED_ERRORS): + # Mirror the base class's own convention: -32002 is the MCP + # code for a resource miss, -32602 says the caller's + # arguments were wrong. Neither prefixes the message, which + # already names the object that was not found. + method = context.method or "" + code = -32002 if method.startswith("resources/") else -32602 + return McpError(ErrorData(code=code, message=str(error))) + return super()._transform_error(error, context) + async def on_call_tool( self, context: MiddlewareContext, @@ -812,6 +952,34 @@ async def on_call_tool( _TRUNCATION_HEADER_TEMPLATE = "[... truncated {dropped} bytes ...]\n" +def _restructure_truncated( + original: dict[str, t.Any], + truncated: ToolResult, +) -> ToolResult | None: + """Re-attach structured content to a truncated success result. + + Only the single-string result shape is rebuildable: fastmcp wraps a + ``-> str`` tool as ``{"result": "..."}``, so swapping in the + truncated text keeps the payload schema-valid. Model- and + list-shaped results carry the oversize inside their own fields and + cannot be trimmed from the flattened text, so they return None and + the caller reports a tool error instead of an invalid response. + """ + if set(original) != {"result"} or not isinstance(original["result"], str): + return None + text = next( + (block.text for block in truncated.content if isinstance(block, TextContent)), + None, + ) + if text is None: + return None + return ToolResult( + content=truncated.content, + structured_content={"result": text}, + meta=truncated.meta, + ) + + class TailPreservingResponseLimitingMiddleware(ResponseLimitingMiddleware): """Response-limiter that keeps the tail of oversized output. @@ -860,15 +1028,42 @@ async def _capture( return t.cast("ToolResult", inner) result = await super().on_call_tool(context, _capture) - if result is not inner and isinstance(inner, ToolResult) and inner.is_error: - # The base class truncated and rebuilt the result; restore - # the error flag it dropped. + if result is inner or not isinstance(inner, ToolResult): + return result + + # The base class truncated and rebuilt the result, dropping both + # ``is_error`` and ``structured_content``. + if inner.is_error: return ToolResult( content=result.content, meta=result.meta, is_error=True, ) - return result + if inner.structured_content is None: + return result + + # A SUCCESSFUL oversized response is the other half of the same + # defect: the tool declares an output schema, the rebuilt result + # carries no structured content, and a spec-compliant client + # raises a transport-level error instead of delivering truncated + # data. That is worse than the truncation this middleware exists + # to perform, and worse than having no middleware at all. + rebuilt = _restructure_truncated(inner.structured_content, result) + if rebuilt is not None: + return rebuilt + # Shape we cannot rebuild: say so as a tool error the agent can + # act on, rather than emitting a response its client will reject. + msg = ( + "Response exceeded the server's size limit and could not be " + "truncated while satisfying this tool's output schema. Re-run " + "with a narrower range (for example a smaller max_lines, or a " + "less negative start)." + ) + return ToolResult( + content=[TextContent(type="text", text=msg)], + meta=result.meta, + is_error=True, + ) def _truncate_to_result( self, diff --git a/src/libtmux_mcp/models.py b/src/libtmux_mcp/models.py index 90733671..422dbece 100644 --- a/src/libtmux_mcp/models.py +++ b/src/libtmux_mcp/models.py @@ -200,13 +200,41 @@ class ServerInfo(BaseModel): socket_path: str | None = Field(default=None, description="Socket path") session_count: int = Field(description="Number of sessions") version: str | None = Field(default=None, description="tmux version") + unreachable_reason: str | None = Field( + default=None, + description=( + "Why a server that exists could not be queried, e.g. this tmux " + "binary being older than the one that created the socket. When " + "set, is_alive=False means 'could not ask', NOT 'not running', " + "and session_count=0 carries no information." + ), + ) class OptionResult(BaseModel): """Result of a show_option call.""" option: str = Field(description="Option name") - value: t.Any = Field(description="Option value") + value: t.Any = Field( + description=( + "Option value. ``null`` means NOT SET AT THE SCOPE QUERIED, " + "which is not the same as not set: an option inherited from " + "a wider scope reads as null unless ``include_inherited`` " + "was passed." + ) + ) + scope_queried: str = Field( + default="server", + description="Scope this answer describes, so null is readable.", + ) + include_inherited: bool = Field( + default=False, + description=( + "True when inherited values were resolved (tmux ``-A``), so " + "the value is the one in force rather than only one set at " + "this scope." + ), + ) class OptionSetResult(BaseModel): @@ -220,15 +248,29 @@ class OptionSetResult(BaseModel): class EnvironmentResult(BaseModel): """Result of a show_environment call.""" - variables: dict[str, str | bool] = Field(description="Environment variable mapping") + variables: dict[str, str] = Field( + description="Variables that are SET, mapped to their values." + ) + removed: list[str] = Field( + default_factory=list, + description=( + "Names tmux marks as explicitly REMOVED from the environment " + "(it prints them as ``-NAME``). These are not in ``variables``. " + "Distinct from a name that simply never appears, which tmux " + "does not report at all." + ), + ) class EnvironmentSetResult(BaseModel): """Result of a set_environment call.""" name: str = Field(description="Variable name") - value: str = Field(description="Value that was set") - status: str = Field(description="Operation status") + value: str | None = Field( + default=None, + description="Value that was set; null when the variable was unset", + ) + status: str = Field(description="Operation status: 'set' or 'unset'") class WaitForTextResult(BaseModel): @@ -299,6 +341,17 @@ class WaitForTextResult(BaseModel): "``alternate_screen``." ), ) + stop_matched_at_entry: bool = Field( + default=False, + description=( + "True when a ``stop`` pattern was already on screen before the " + "wait began. The wait only stops on a FRESH stop hit, so this " + "does not end it -- but a failure marker left from an earlier " + "run is the usual reason a ``timeout`` outcome is misread as " + "'still running'. Read it as: check whether you are waiting on " + "a run that already failed." + ), + ) matched_at_entry: bool = Field( default=False, description=( @@ -366,6 +419,18 @@ class RunCommandResult(BaseModel): description="Shell exit status, or None when the command timed out", ) timed_out: bool = Field(description="True when the wait timed out") + command_may_still_run: bool = Field( + default=False, + description=( + "True when the wait timed out, meaning the command was SENT " + "but not observed to finish. It is not cancelled: the " + "keystrokes sit in the pane's input buffer and the shell " + "runs them whenever it next reads a line, which may be long " + "after this call returned. Do NOT retry a non-idempotent " + "command on this result -- that is how a `git push` or a " + "migration runs twice. Check the pane first." + ), + ) elapsed_seconds: float = Field(description="Time spent waiting in seconds") output: list[str] = Field( default_factory=list, @@ -646,11 +711,24 @@ class SearchPanesResult(BaseModel): default_factory=list, description="PaneContentMatch entries for this page.", ) + searched_scope: t.Literal["visible", "scrollback"] = Field( + default="visible", + description=( + "How much of each pane was read. ``visible`` is the default " + "and means ONLY the on-screen rows were searched, so a match " + "that has scrolled off is not reported and ``matches: []`` " + "does not mean the text is absent. Pass ``content_start`` " + "(e.g. -500) to search scrollback." + ), + ) truncated: bool = Field( default=False, description=( "True when the result set was truncated by ``limit`` or " - "by ``max_matched_lines_per_pane`` on any pane." + "by ``max_matched_lines_per_pane`` on any pane. It describes " + "those caps ONLY -- it never reports rows left unread " + "because the search was scoped to the visible screen; read " + "``searched_scope`` for that." ), ) truncated_panes: list[str] = Field( diff --git a/src/libtmux_mcp/resources/hierarchy.py b/src/libtmux_mcp/resources/hierarchy.py index 0dcfd45e..b0cea487 100644 --- a/src/libtmux_mcp/resources/hierarchy.py +++ b/src/libtmux_mcp/resources/hierarchy.py @@ -31,6 +31,41 @@ _TEXT_MIME = "text/plain" +def _normalize_pane_id(pane_id: str) -> str: + """Accept ``10`` as well as ``%10`` in a resource URI. + + The URI layer percent-decodes every captured template parameter + (fastmcp ``resources/template.py``), and a tmux pane id starts with + ``%``. ``%10`` therefore arrives as the single byte ``0x10``. + ``%0``-``%9`` survive only because one trailing hex digit is an + INVALID escape and passes through, so the whole surface works right + up until a server has created its eleventh pane. + + Re-encoding the decoded byte is not a fix: ``%80``-``%99`` decode to + bytes that are not valid UTF-8 and arrive as U+FFFD, so the digits + are already gone. That repair passes every test written against low + pane numbers and fails once a pane id reaches 128. + + A bare number needs no escaping at any pane number, so that is the + spelling this accepts. ``%2510`` (a caller who pre-encoded the + percent) still decodes to ``%10`` and keeps working. + """ + if pane_id and not pane_id.startswith("%"): + return f"%{pane_id}" + return pane_id + + +def _raise_pane_not_found(pane_id: str) -> t.NoReturn: + """Report a missing pane, naming a mangled id rather than hiding it.""" + msg = f"Pane not found: {pane_id!r}" + if not pane_id.isprintable(): + msg += ( + " -- the id was percent-decoded by the URI layer. Address the " + "pane by its bare number instead, e.g. 'tmux://panes/10'." + ) + raise ResourceError(msg) + + def register(mcp: FastMCP) -> None: """Register hierarchy resources with the FastMCP instance.""" @@ -67,10 +102,20 @@ def get_session( ) -> str: """Get details of a specific tmux session. + .. warning:: + + Percent-encode the name when building this URI. The path + segment is percent-DECODED before lookup, so a session + literally named ``pct%20name`` is reached only as + ``tmux://sessions/pct%2520name`` — pasting the raw name + reads the session named ``pct name`` instead, silently and + with no error. A space needs no encoding, which is what + makes a literal ``%`` easy to miss. + Parameters ---------- session_name : str - The session name. + The session name, percent-encoded. socket_name : str, optional tmux socket name. Defaults to LIBTMUX_SOCKET env var. @@ -101,6 +146,10 @@ def get_session_windows( ) -> str: """List all windows in a tmux session. + The session name is percent-decoded before lookup; see + :func:`get_session` for why a name containing ``%`` must be + encoded before it goes into this URI. + Parameters ---------- session_name : str @@ -175,7 +224,9 @@ def get_pane(pane_id: str, socket_name: str | None = None) -> str: Parameters ---------- pane_id : str - The pane ID (e.g. '%1'). + Pane number or id. Prefer the bare number ('10'): + a URI-escaped '%10' is decoded to a control character + before it reaches here. socket_name : str, optional tmux socket name. Defaults to LIBTMUX_SOCKET env var. @@ -185,10 +236,10 @@ def get_pane(pane_id: str, socket_name: str | None = None) -> str: JSON object of pane details (MIME: ``application/json``). """ server = _get_server(socket_name=socket_name) + pane_id = _normalize_pane_id(pane_id) pane = server.panes.get(pane_id=pane_id, default=None) if pane is None: - msg = f"Pane not found: {pane_id}" - raise ResourceError(msg) + _raise_pane_not_found(pane_id) return json.dumps(_serialize_pane(pane).model_dump(), indent=2) @@ -203,7 +254,9 @@ def get_pane_content(pane_id: str, socket_name: str | None = None) -> str: Parameters ---------- pane_id : str - The pane ID (e.g. '%1'). + Pane number or id. Prefer the bare number ('10'): + a URI-escaped '%10' is decoded to a control character + before it reaches here. socket_name : str, optional tmux socket name. Defaults to LIBTMUX_SOCKET env var. @@ -213,10 +266,10 @@ def get_pane_content(pane_id: str, socket_name: str | None = None) -> str: Plain text captured pane content (MIME: ``text/plain``). """ server = _get_server(socket_name=socket_name) + pane_id = _normalize_pane_id(pane_id) pane = server.panes.get(pane_id=pane_id, default=None) if pane is None: - msg = f"Pane not found: {pane_id}" - raise ResourceError(msg) + _raise_pane_not_found(pane_id) lines = pane.capture_pane() return "\n".join(lines) diff --git a/src/libtmux_mcp/server.py b/src/libtmux_mcp/server.py index c5772f1b..a38407c3 100644 --- a/src/libtmux_mcp/server.py +++ b/src/libtmux_mcp/server.py @@ -108,7 +108,7 @@ "WAIT, DON'T POLL: run_command for authored commands needing " "status; wait_for_channel for custom tmux wait-for; capture_since " "for tailing; wait_for_text for output you don't author " - "(patterns=null=any output; stop=[] bails); " + "(patterns=null=any output; a stop hit returns at once); " "send_keys_batch for raw input." ) @@ -436,8 +436,11 @@ def _enable_allowed_tools() -> None: if _mcp_visibility_configured: return - # Use FastMCP's native visibility system as primary gate, - # with the SafetyMiddleware as a secondary layer for clear error messages. + # This is the ENFORCEMENT gate: it holds even for a call that skips + # the middleware chain (``call_tool`` accepts + # ``run_middleware=False``). ``SafetyMiddleware`` is the + # EXPLANATION gate -- disabling makes ``get_tool`` answer None, so + # FastMCP would otherwise report a gated tool as ``Unknown tool``. allowed_tags = {TAG_READONLY} if _safety_level in {TAG_MUTATING, TAG_DESTRUCTIVE}: allowed_tags.add(TAG_MUTATING) diff --git a/src/libtmux_mcp/tools/batch_tools.py b/src/libtmux_mcp/tools/batch_tools.py index 2091f4b4..2b52780e 100644 --- a/src/libtmux_mcp/tools/batch_tools.py +++ b/src/libtmux_mcp/tools/batch_tools.py @@ -125,8 +125,14 @@ async def _get_allowed_tool_tier( tool = await fastmcp.get_tool(operation.tool) if tool is None: - msg = f"Unknown tool: {operation.tool!r}" - raise ExpectedToolError(msg) + # None means nonexistent OR disabled by tier, so raising + # "Unknown tool" here denied that a gated tool exists. Hand it + # on instead: the nested call runs with ``run_middleware=True``, + # letting ``SafetyMiddleware`` name the tier and FastMCP still + # raise ``NotFoundError`` for a typo. Nothing is skipped -- + # visibility follows tier tags, so an invisible tool is + # off-tier by construction and is denied before these checks. + return # ``max_tier`` is a CEILING, so a readonly tool is reachable through # every batch wrapper, not only the readonly one. The batch loop is diff --git a/src/libtmux_mcp/tools/buffer_tools.py b/src/libtmux_mcp/tools/buffer_tools.py index 6663d7e7..1fb23bb3 100644 --- a/src/libtmux_mcp/tools/buffer_tools.py +++ b/src/libtmux_mcp/tools/buffer_tools.py @@ -125,15 +125,29 @@ def _validate_buffer_name(name: str) -> str: >>> _validate_buffer_name("clipboard") Traceback (most recent call last): ... - libtmux_mcp._utils.ExpectedToolError: Invalid buffer name: 'clipboard' + libtmux_mcp._utils.ExpectedToolError: 'clipboard' is not an MCP-allocated buffer >>> _validate_buffer_name("libtmux_mcp_shortuuid_buf") Traceback (most recent call last): ... - libtmux_mcp._utils.ExpectedToolError: Invalid buffer name: 'libtmux_mcp_...' + libtmux_mcp._utils.ExpectedToolError: 'libtmux_mcp_...' is not an MCP-... """ if not _BUFFER_NAME_RE.fullmatch(name): - msg = f"Invalid buffer name: {name!r}" - raise ExpectedToolError(msg) + # Not "invalid": tmux accepts any of these names happily. It is + # this server that only touches buffers it allocated, because + # tmux buffers can hold OS clipboard history and a tool that + # reads arbitrary ones is a clipboard reader. + msg = f"{name!r} is not an MCP-allocated buffer" + raise ExpectedToolError( + msg, + suggestion=( + "This server only reads and writes buffers it created " + "(libtmux_mcp_<32-hex>_