feat(engine,plugin,config): MCP engine integration — servers, namespaced tools, client/mcp.call#51
Merged
Conversation
added 5 commits
July 9, 2026 19:01
…tools MCPManager (engine/mcp.go) is the production MCPRegistry: given named MCP server configs (stdio or streamable-HTTP), it lazily connects to each on first use, lists its tools, and namespaces them mcp__<server>__<tool> (the Claude Code convention). A server that fails to connect or list tools is logged and skipped rather than failing the caller — fail-open, matching the plugin crash philosophy. Tool calls route through the same connected mcp.Client via CallTool; is_error is threaded through verbatim. Session.Config gains an MCP field (nil disables MCP entirely, matching the Hooks pattern) and Session.MCPCall exposes explicit server+tool routing for the plugin ClientAPI path landing next.
Replace plugin.ErrMCPNotImplemented in both bundled ClientAPI implementations now that MCP engine integration exists: - cmd/harness's runClientAPI.MCPCall routes through the run-mode session's engine.Session.MCPCall (late-bound, matching how SessionMessages already resolves the session lazily). - server's clientAPI.MCPCall routes through a new Options.MCP field: the same *engine.MCPManager shared with every session's engine.Config.MCP, called directly by server+tool name since MCPCallRequest names neither a session nor a namespaced tool. Update PROTOCOL.md's client/mcp.call section accordingly; client/ generate remains not-implemented (separate PR, provider-layer routing).
buildMCPManager converts config.MCPServerSpec into engine.MCPServerConfig and builds one shared *engine.MCPManager per process (nil when no servers are configured, mirroring buildPluginHost). Both harness run and harness serve build it once, thread it into every session's engine.Config.MCP (and server.Options.MCP, for the plugin ClientAPI's direct server+tool routing), and close it on shutdown with a bounded timeout — deferred before the plugin host's own Close so MCP connections outlive it, symmetric with how the host outlives the server's drain.
📦 Previous Review (superseded by newer review)Claude finished @andybons's task in 3m 49s —— View job Review: MCP engine integrationReviewed against
🔴 Connect is bound to the first caller's request context (serve mode)Inline comment →
🟡 Minor notes (non-blocking)
What's solid
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
added 4 commits
July 9, 2026 19:39
ensureConnected is a sync.Once-cached, run-exactly-once-forever operation shared by every future Session, but it was threading the very first caller's ctx straight into connectMCPServer. In serve mode that ctx is per-request: a request that happens to trigger the first connect being aborted/disconnected mid-connect canceled every server's connect, which was logged-and-skipped and, being cached, never retried — one transient cancellation permanently stripped MCP tools from the whole process. Detach with context.WithoutCancel before connecting; each server's own ConnectTimeout still bounds the connect step. Regression test drives the first caller in with an already-canceled context and asserts the server still connects (red before the fix: Tools() returned no entries).
decodeMCPBase64 silently returned nil Data on a base64 decode failure, losing the tool result's payload with no signal anywhere. Log a slog warning naming the server and tool the payload came from so the failure is at least visible — but never the payload bytes themselves, which may be arbitrarily large and are not diagnostic. Adds TestDecodeMCPBase64MalformedLogsWarning.
Close read m.clients directly, with no interaction with connectOnce. connectMCPServer populates m.clients only at the very end of the one-time connect step, so a Close racing a caller's still-in-flight first Tools()/CallTool() call would observe the zero-value nil map, close nothing, and return; the racing connect would then finish moments later and populate m.clients with a client (or, for a stdio server, a spawned child process) nobody would ever close again, since connectOnce never retries or revisits it once run — a silent leak. Have Close call ensureConnected itself before reading m.clients: sync.Once serializes concurrent Do calls, so this blocks Close until any connect already in flight elsewhere has finished and its clients are visible, closing them as it always did. If no connect was ever triggered, Close now performs it itself (bounded by each server's own ConnectTimeout) purely so it can close what it creates. Adds TestMCPManagerCloseWaitsForInFlightConnect: a gated stub server that holds the initialize response open until the test has raced Close in, then asserts (via the DELETE the client's Close issues) that the client the racing connect created was actually closed. Red before the fix (Close saw no clients yet).
… decoding engine/mcp.go namespaces every MCP tool as mcp__<server>__<tool>. That encoding is only uniquely decodable back into (server, tool) if "__" cannot occur inside server: server "a__b" tool "c" and server "a" tool "b__c" both produce the identical namespaced name mcp__a__b__c, so two unrelated servers' tools would silently collide and become indistinguishable at call time. Reject at load, loudly: a server name containing "__" anywhere, or starting with the "mcp__" namespace prefix itself. Adds table-test rows to TestLoadMCPServers covering a middle/leading/ trailing "__", a bare "mcp__" prefix, and exactly "mcp__", plus a control case confirming a single underscore is still accepted. Red before the fix (all five malformed-name rows loaded without error).
📦 Previous Review (superseded by newer review)Claude finished @andybons's task in 3m 33s —— View job Review: MCP engine integration (re-review after fixes)
Reviewed the current branch ( Prior findings — all resolved
What I re-verified as solid
One non-blocking nit (optional)
Verdict: ready to merge from my side. Clean, well-documented, faithful to the existing plugin-host patterns, and the concurrency edges are now closed with tests that would actually catch regressions. |
📦 Previous Review (superseded by newer review)Claude finished @andybons's task in 3m 27s —— View job Review: MCP engine integration (re-review after fixes)Reviewed All five findings from the prior round are resolved and each landed with a red-first regression test:
What's solid
Minor note (non-blocking)
Nothing blocking — clean, well-documented, closely mirrors the existing plugin-host patterns. Nice work. ✅ |
📦 Previous Review (superseded by newer review)Claude finished @andybons's task in 3m 7s —— View job Review: MCP engine integrationReviewed the current branch (
What I re-verified as correct
One non-blocking note (already flagged, accepted)
Verdict: clean, well-documented, faithful to the existing plugin-host patterns, concurrency edges closed with tests that would catch regressions. ✅ |
Close's leak-fix interlock (4075ef4) called ensureConnected(ctx) unconditionally to synchronize with an in-flight first connect before reading m.clients. That correctly waits out a connect already underway, but if nothing had ever triggered one, Close itself became the first caller: a harness serve process configured with MCP servers that never received a single prompt before shutdown would still live-dial every configured server just to immediately close it, up to ConnectTimeout (default 15s) of pure shutdown latency, bounded by the wrong timeout entirely (each server's own ConnectTimeout, not mcpCloseTimeout, since Close was paying for the connect rather than merely waiting on one). Close now calls connectOnce.Do with a no-op instead of the real connect closure. sync.Once still serializes every Do call the same way regardless of which function is passed: if a real connect is already in flight (or finished) elsewhere, Close's Do call blocks until it completes and m.clients is fully populated, preserving the original interlock exactly. If nothing had started, Close's no-op wins instead — near-instant, no network touched — and because connectOnce is now permanently consumed, no Tools()/CallTool()/CallServerTool call arriving after such a Close, even later, can start a connect either. Adds TestMCPManagerCloseOfNeverPromptedManagerNeverConnects: a gated stub server (never released until Cleanup) proving Close of an idle manager makes zero connection attempts and returns near-instantly, plus that a Tools() call after that Close still makes none. Red before the fix (one request reached the server). Keeps TestMCPManagerCloseWaitsForInFlightConnect passing unchanged.
|
Claude finished @andybons's task in 3m 46s —— View job Review: MCP engine integrationReviewed the branch (
What I re-verified as correct
Non-blocking observations (optional, no action needed)
Verdict: clean, well-documented, faithful to the existing plugin-host patterns, with the concurrency edges closed by tests that would actually catch regressions. Ready to merge from my side. ✅ |
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
1 participant
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.
Implements the last platform blocker for the meeseeks migration: MCP servers as first-class session tools.
mcp_serverssection declaring named servers — stdio (command/env/dir) and streamable-HTTP (url/headers) — validated at load like pluginsmcp/client, list tools, and register them as engine tools namedmcp__<server>__<tool>(Claude Code convention), routing execution through CallTool; a server that fails to connect or list is logged and skipped — never kills the session (fail-open, matching plugin crash philosophy)client/mcp.callnow reaches the same connected clients, replacingErrMCPNotImplemented(including the late-binding ClientAPI in cmd/harness)🤖 Generated with Claude Code
https://claude.ai/code/session_013B9pwUYhT4o8se9skmS5PP