Repository navigation
fix(core): skip malformed MCP tools instead of failing the server - #58350
Conversation
The MCP SDK validates tools/list as a whole, so one tool that violates the schema failed every tool on the server and left it failed. Check each tool in tools/list responses on the transport, before the SDK sees them, against the same era-specific schema the SDK applies, drop the ones it would reject and log a warning naming the server and tool. The SDK still paginates and caches the remaining tools, so callTool output validation keeps working in both protocol eras.
![opencode-agent[bot]](https://avatars.githubusercontent.com/in/1549082?s=60&v=4){kind=small}
There was a problem hiding this comment.
The fix works. On the base, the new test fails in both the legacy and modern protocols with Invalid result for tools/list. With this change it passes, and the valid tools still load and can be called. The rest of test/mcp.test.ts passes, except a descendant-termination test that times out on the base too. typecheck:tsc passes for packages/core. I couldn't run it end to end, because the CLI server won't start in this environment on either checkout.
Public API: unchanged.
The approach is sound. It reuses the SDK's own validator for the negotiated protocol version, and pagination, the tool cache and the retry path are untouched. One small inline point: the filter is applied to any response with a tools array, not only replies to tools/list requests.
| const onmessage = transport.onmessage | ||
| transport.onmessage = (message, extra) => { | ||
| if ( | ||
| isJSONRPCResultResponse(message) && |
There was a problem hiding this comment.
Non-blocking: this matches any JSON-RPC result that has a tools array, not only replies to tools/list requests. For example, a tools/call result whose server adds a top-level tools field would have that field filtered or changed here. If that matters, you could track the ids of outgoing tools/list requests (by wrapping transport.send) and filter only those replies.
Same behaviour as the cherry-picked guard, minus the parts current main no longer needs: mcp 1.x root-model unwrapping (Hermes pins mcp 2.x), the separate request ledger class, and pragma-no-cover try/excepts around code that cannot fail. One outer except in the read proxy still guarantees SDK drift never breaks a session. get_mcp_status reads _dropped_tools with getattr like its _registered_tool_names neighbour, so status fakes without the slot keep working. Tests trimmed to two invariants driven through a real ClientSession: one rejected tool costs that tool (named), not the page; a custom-method result carrying a tools list passes through untouched. Invalid case is a missing inputSchema, the shape the pinned SDK rejects in every protocol era. Port reference: anomalyco/opencode#58350 (skip malformed MCP tools instead of failing the server).
The SDK validates a
tools/listresult as a whole, so one tool that breaks the schema (say aninputSchemawhosetypeisn't"object") fails the whole list, and since we list tools during connect, the whole server shows as failed.Now each tool is checked on its own and the bad ones are dropped with a warning naming the server and tool. The rest load normally.
It runs as a transport message handler ahead of the SDK, so
listToolsitself is unchanged: pagination, the tool cache thatcallTooluses for output validation, and the modern-era header retry all keep working. The check is the SDK's owntools/listvalidator for the negotiated protocol version, so a tool is only dropped when the SDK would have rejected it anyway. That validator is a protected method (_wireCodec()), so an SDK update that renames it will show up as a type error.If the result is broken for some reason other than its tools, it's left alone and the SDK reports the real error.