feat(mcp): S2 — hand-rolled stdio client (JSON-RPC) + lifecycle#30
Merged
Conversation
Second slice (docs/MCP.md). Still wired to nothing: S3 is the router line in agent.js. Split pure/IO the way the codebase already does (sse.js thin IO, translate.js pure), so the parts most likely to be wrong are testable without spawning: - mcpProtocol.js (pure) — newline-delimited JSON-RPC framing and tool-result flattening. Both are genuinely easy to get wrong: a pipe does not respect message boundaries, so a framer that assumes "one chunk = one message" works until a big tools/list arrives; and a stray non-JSON log line on stdout (a well-known MCP footgun) must be skipped rather than desync the stream. Results are typed content blocks but tool_result is a STRING, so non-text degrades to a placeholder (never raw base64), failures take the agent's `ERROR: ` prefix, and the whole thing is capped — the generic tool path bounds nothing. - mcpClient.js (IO) — spawn, handshake, request/response, teardown. Four things it owes the agent that the generic tool path does not provide: timeouts (tools run sequentially, so one wedged server would hang the whole run), a capped flattened string, call() that NEVER throws (a failure returns an `ERROR: ` string so one bad server cannot break the turn loop), and deterministic reaping via reapMcp() for New Chat / deactivate — a detached child otherwise outlives the window, exactly the reason reapCommands() exists. Security choices: shell:false (args are argv, never re-parsed by a shell), and server→client requests (sampling/*, elicitation/*) are explicitly REFUSED rather than ignored — a server must not be able to drive our model or prompt the user. This module deliberately does not decide whether a server MAY start; that gate is S4. Tested against a real subprocess, not mocks: test/fixtures/mock-mcp-server.js speaks just enough protocol to drive the client, with tools that succeed, fail, and never answer, plus --noise/--crash/--sample modes. So the handshake, framing over a live pipe, the timeout, the sampling refusal and process teardown are all actually exercised. Verified: 15 protocol + 13 client tests pass; the full CI gate runs 18 extension suites with 0 failures; requires are child_process + the pure module only (no vscode); and `ps` is clean after the run — dispose() and reapMcp() genuinely kill the process group, asserted in-test by polling the pid. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Second MCP slice (
docs/MCP.md). Stacked on #29 — the diff here is S2 only. Still wired to nothing; S3 is the router line inagent.js.Pure / IO split
Following the codebase's own pattern (
sse.jsthin IO,translate.jspure), the parts most likely to be wrong are testable without spawning anything:mcpProtocol.js(pure) — framing + result flattening. Both are easy to get wrong:tools/listarrives. And a stray non-JSON log line on stdout — a well-known MCP footgun — must be skipped, not desync the stream.tool_resultis a string. Non-text degrades to a placeholder (never raw base64 into the transcript), failures take the agent'sERROR:prefix, and the result is capped — the generic tool path bounds nothing.mcpClient.js(IO) — spawn, handshake, request/response, teardown.Four things it owes the agent
agent.js— one wedged server would hang the entire runtool_resultis textcall()never throwsERROR: …, so one bad server can't break the turn loopreapMcp()reapCommands()existsSecurity choices
shell:false— args are passed as argv, never re-parsed by a shell.sampling/*) or prompt the user (elicitation/*). It gets a clean JSON-RPC error instead of hanging.Tested against a real subprocess, not mocks
test/fixtures/mock-mcp-server.jsspeaks just enough protocol to drive the client, with tools that succeed, fail, and never answer, plus--noise/--crash/--samplemodes. So the handshake, framing over a live pipe, the timeout, the sampling refusal, and process teardown are genuinely exercised.Verification
child_process+ the pure module only — novscodepsis clean after the run:dispose()andreapMcp()really do kill the process group, asserted in-test by polling the pid past the SIGTERM→SIGKILL graceNext
S3 wires it in —
TOOLS/TOOLS_TOKENS_ESTbecome per-run, and the router goes immediately before theunknown toolfallthrough atagent.js:442. Then S4 (trust-on-first-use + the approval card), which must not be skipped.🤖 Generated with Claude Code