fix(protocol): send the original McpError message, not the prefixed one - #2805
Open
fszcd wants to merge 3 commits into
Open
fix(protocol): send the original McpError message, not the prefixed one#2805fszcd wants to merge 3 commits into
fszcd wants to merge 3 commits into
Conversation
A handler that throws McpError produced a wire error whose message
already carried the local `MCP error <code>:` prefix, so a v1 peer
reconstructing it via `new McpError(code, message)` displayed the prefix
twice ("MCP error -32601: MCP error -32601: ...").
Retain the constructor argument as `McpError.originalMessage` and send
that on the wire; peers add the prefix when reconstructing, matching the
v2 behavior where the constructor no longer prefixes.
Fixes modelcontextprotocol#2786
🦋 Changeset detectedLatest commit: 95a404d The changes in this PR will be included in the next version bump. Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
commit: |
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.
Root cause
McpError's constructor builds.messageasMCP error <code>: <message>. When a v1 request handler throws, the protocol layer serializederror.message— prefix included — onto the wire. A v1 peer then reconstructs withnew McpError(code, wireMessage), adding the prefix a second time:Per @pj-workspace's analysis on the issue, the wire convention is an unprefixed message: v2 servers already emit it (v2 removed the constructor prefix in #1727), and both v1 and v2 clients add the prefix when reconstructing. Only the v1 server leaks its local prefixed form. Since changing
McpError.messagein v1 would be breaking (callers may match on it), this PR takes the additive route.Fix
McpErrorretains the constructor argument as a new readonlyoriginalMessage(.messageis unchanged — no breaking change).originalMessagewhen the thrown error is anMcpError, so the wire carries exactly what the handler author wrote. Other error types keep the existingerror.message ?? 'Internal error'behavior.Effect on @pj-workspace's matrix: the v1→v1 row goes to 0 wire prefixes / 1 client prefix, matching v2→v2; v1→v2 stays clean.
Tests
New
test/issues/test_2786_mcp_error_wire_message.test.tsruns a realClient↔Serverpair overInMemoryTransport, captures the raw JSON-RPC error at the transport boundary, and asserts:error.messageisUnknown tool: nope(no prefix)MCP error -32601: Unknown tool: nope(exactly one prefix) with the rightcodeandoriginalMessageFails on unpatched
v1.x(wire message has the prefix), passes with the fix. Full suite: 53 files / 1648 tests pass; the 2test/client/stdio.test.tspipe errors reproduce identically on an unmodified checkout (Windows-local, unrelated).tsgo --noEmitandeslintclean. (prettier --check .flags 227 untouched files on this Windows checkout due to autocrlf CRLF; the three changed files pass individually and are committed as LF.)Fixes #2786