The MCP shim can carry a session's tool calls to a Collins that isn't wired up yet

[ghackett]

PR 2 of the session-MCP-tools plan (spec: ~/specs/collins/session-mcp-tools.md; PR 1 was the proctree upward-ancestry work in PR #201). Two new modules plus tests — deliberately inert: nothing spawns the shim or binds the socket until PR 3 wires the socket service into the app and appends --mcp-config to launched commands.

collins/mcp_shim.py — the stdio MCP server claude will spawn

• Hand-rolled newline-delimited JSON-RPC 2.0: initialize (echoes the client's protocolVersion), notifications/initialized, ping, tools/list, tools/call; unknown method with an id → -32601. No mcp PyPI dependency, keeping the debian packaging dependency-free.
• Stdlib-only and imports nothing from the rest of collins — it runs as a child of the agent CLI, so a mismatched or half-upgraded install must not be able to break session startup. Shared wire constants are mirrored by hand, with comments pointing both ways.
• Relays tools/list/tools/call to the app over the Unix socket named by COLLINS_MCP_SOCKET; the tool list lives app-side only, so adding a tool never changes the shim. The first line on every connection is a hello carrying the shim's pid — the seed for the proctree ancestry walk that maps a call to its tab in PR 3.
• Never breaks the session. Collins absent (quit, crashed, stale config): the handshake still succeeds, tools/list is empty, calls return a clean "Collins is not running" tool error. Timeouts (connect 1s, list 3s, call 15s) report "Collins did not respond in time". Any failure drops the connection and the next request redials — that lazy reconnect is what heals a Collins restart; there are no retry loops within a request.
• Stdout carries protocol bytes only; debug logging goes to COLLINS_SHIM_LOG if set.

collins/mcptools.py — the GTK-free app-side layer

• TOOLS serves only set_session_title for now — the list is app-served, and advertising tools whose handlers don't exist yet would invite calls that can only fail. open_in_editor, show_image, and notify_user join in PRs 4–5.
• validate_args() re-validates every call app-side: the socket is reachable by any local process, so the CLI's schema enforcement is not a boundary.
• Wire framing (encode_message/decode_message) with a 1 MiB line guard; runtime-dir path derivation ($XDG_RUNTIME_DIR/collins/<app_id>/, keyed by app id so the real app keeps a stable path across restarts while debug instances stay isolated); write_config() writing the --mcp-config file atomically (trust.py style, best-effort → None so callers skip the flag).
• GTK-free per the proctree.py/chats.py split, so CI (no typelibs) tests the whole protocol.

Tests

tests/test_mcp_shim.py runs the shim exactly as the CLI will — a real python -m collins.mcp_shim subprocess — against a threaded socketserver faking Collins: handshake transcript, hello pid, list/call forwarding, app-side errors becoming tool errors, degradation (env unset / socket absent / unresponsive server → timeout error with the shim still healthy), reconnect across a fake Collins restart, and a sweep asserting stdout carries nothing but complete JSON-RPC lines. One subtlety: the fake server's stop() must close established connections too, or its daemon handler threads keep serving a "dead" Collins. tests/test_mcptools.py covers the table, validation, framing guards, paths, and the config writer.

All scenarios were also hand-run green locally via a pytest-free harness (this machine has no pytest; CI is the runner of record).

No GPL modification notices needed: every file in this PR is new to the fork.

🤖 Generated with Claude Code

https://claude.ai/code/session_016J5wuRHXbUZraqsEApxRUn

[ghackett]

@claude review

[claude]

Claude finished @ghackett's task in 4m 15s —— View job

---

Reviewing PR #204

• Gather context (diff, spec description, changed files)
• Read collins/mcp_shim.py
• Read collins/mcptools.py
• Read tests/test_mcp_shim.py and tests/test_mcptools.py
• Post review feedback

Summary

This is a clean, well-scoped PR — the shim/app split, the "never break the session" degradation story, and the test coverage (real subprocess vs. a threaded fake socket server) all match the description closely and hold up under a close read. I traced the diff against origin/main via git diff 6c3cb64..HEAD (the PR's actual base) since the checkout's local main ref is stale relative to origin/main; the four files match the stated +1006/-0.

Confirmed by reading the code:

• Stdlib-only, zero imports from the rest of collins in mcp_shim.py — verified.
• _MAX_LINE (shim) and MAX_LINE (mcptools) are both 1024 * 1024, kept in sync as the comments promise.
• Timeouts match the description exactly: connect 1s, list 3s, call 15s.
• Degradation paths (_AppUnavailable → empty tool list / "Collins is not running"; _AppTimeout → "Collins did not respond in time") are handled distinctly and correctly at each call site.
• write_config's atomic tmp-file-then-replace and 0o700 runtime dir match the trust.py-style best-effort approach described.
• No modified pre-fork files in this diff, so the GPL notice requirement doesn't apply here, as the PR body notes.

Findings

Minor — inconsistent shutdown handling on the parse-error path (collins/mcp_shim.py, main(), ~lines 253–264)

try:
    msg = json.loads(line)
except ValueError:
    _error(None, -32700, "Parse error")   # not guarded
    continue
try:
    if isinstance(msg, dict):
        _handle(link, msg)
    else:
        _error(None, -32600, "Invalid Request")
except BrokenPipeError:
    return 0  # the CLI went away mid-reply

The except BrokenPipeError: return 0 only wraps the second block. If the agent CLI has already closed the shim's stdout (process exiting) and a malformed line arrives on stdin in that window, _error(None, -32700, ...) will raise an uncaught BrokenPipeError and crash the shim with a traceback instead of exiting cleanly like every other shutdown path documented in this file ("Never break the session"). Low-probability race, but easy to close by wrapping both write paths (or the whole per-line loop body) in the same try/except BrokenPipeError.

Fix this →

Nits / observations (non-blocking)

• _AppLink guards _sock/_reader with a threading.Lock, but main()'s stdin loop is strictly single-threaded — one message is fully handled before the next readline() — so the lock never sees contention today. Fine to keep as defensive plumbing if a future PR adds concurrent request handling, just noting it's currently inert.
• write_config's env dict fully replaces the MCP subprocess's environment for the mcpServers.collins entry (only COLLINS_MCP_SOCKET and PYTHONPATH are set). Since command is sys.executable (absolute path) and the shim hardcodes encoding="utf-8" everywhere, this looks safe, but it's worth double-checking against how the actual claude CLI treats a stdio server's env (merge vs. replace) once PR 3 wires this up for real.

Nothing here blocks merging — the one finding is a minor edge-case robustness gap, not a functional bug, and the test suite (which I read in full, not just skimmed) already exercises the degradation, reconnect, and stdout-hygiene scenarios thoroughly. I wasn't able to execute the tests in this sandbox (Python invocations here require approvals I don't have), consistent with the PR body's note that this machine has no pytest either — CI remains the runner of record.
· branch

[ghackett]

Addressed the finding in b933473: the whole per-line loop body now sits under the BrokenPipeError guard, so the parse-error reply shares the clean-exit path. The handler also points stdout at devnull before returning — without that, the interpreter's exit-time flush of the half-written buffer raised the same error again and left "Exception ignored" noise on stderr. Added test_broken_stdout_still_exits_cleanly, which closes the read end before sending garbage and fails against the old code.

On the nits:

• The _AppLink lock stays as deliberate plumbing: the wire protocol relies on one in-flight round trip at a time (replies correlate by id against ordered reads), and the lock is what makes that invariant hold if anything ever drives the link off the stdin thread. Documented in the class docstring.
• Agreed on env merge-vs-replace being the open question — the shim only needs its two vars plus an inherited-or-not PATH-free environment (absolute sys.executable, -m import via PYTHONPATH), so either CLI behavior works. PR 3's manual verification (/mcp listing, live tool call) will confirm it against the real CLI either way.

🤖 Generated with Claude Code

https://claude.ai/code/session_016J5wuRHXbUZraqsEApxRUn