fix(server): align ProtocolError re-throw with spec error classification#1769
fix(server): align ProtocolError re-throw with spec error classification#1769felixweinberger wants to merge 7 commits intomainfrom
Conversation
Re-throw all ProtocolError instances from the tools/call handler as JSON-RPC errors. Previously only UrlElicitationRequired was re-thrown; other ProtocolErrors thrown inside the try block (output validation, task misconfiguration) were wrapped as isError: true tool results. Per the MCP spec's error classification: - Input validation failures are tool-execution errors (isError: true) - Output validation failures are server errors (JSON-RPC InternalError) - Task misconfiguration is a protocol mismatch (JSON-RPC error) Changes: - validateToolInput now throws plain Error so input validation stays tool-level (isError: true) - validateToolOutput now uses InternalError code instead of InvalidParams (output validation failure is a server-side bug, not client fault) - catch block re-throws any ProtocolError, matching python-sdk semantics and allowing tool handlers to deliberately throw protocol-level errors
🦋 Changeset detectedLatest commit: 4633cb1 The changes in this PR will be included in the next version bump. This PR includes changesets to release 5 packages
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 |
@modelcontextprotocol/client
@modelcontextprotocol/server
@modelcontextprotocol/express
@modelcontextprotocol/fastify
@modelcontextprotocol/hono
@modelcontextprotocol/node
commit: |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
The code change itself is small and logically sound, but this is an explicitly breaking change to core error handling in the tools/call handler that warrants human sign-off — particularly the design choice to keep MethodNotFound for task-required-without-task (vs InvalidParams), and whether the changeset should be minor given the breaking nature.
Extended reasoning...
Overview
This PR broadens the catch block in McpServer's tools/call handler from only re-throwing UrlElicitationRequired to re-throwing all ProtocolError instances. It also downgrades validateToolInput from throwing ProtocolError to plain Error (so input validation stays as isError: true), and changes output validation error codes from InvalidParams to InternalError. Documentation and tests are updated accordingly.
Security risks
No security concerns. The change affects error classification, not authorization or data exposure.
Level of scrutiny
This is a self-described breaking change to a core code path. The tools/call handler is exercised by every tool invocation. Consumers relying on result.isError to detect output validation failures or handler-thrown ProtocolErrors will see different behavior. The changeset marks it as minor but the PR description and migration docs both call it breaking — a human should verify the semver classification. The files are also covered by CODEOWNERS (@modelcontextprotocol/typescript-sdk).
Other factors
The two bug reports are both pre-existing issues not introduced by this PR. The code logic is correct and well-motivated (aligning with spec classification and Python SDK behavior). Tests are updated to match. Migration docs are thorough. The concern is purely about the design-level decisions that a maintainer should validate.
bhosmer-ant
left a comment
bhosmer-ant
left a comment
There was a problem hiding this comment.
The spec-alignment reasoning is correct (input validation → tool result, output validation → InternalError, task-required → InvalidParams all check out against MCP spec §Tools/Error Handling). Bughunter found two issues with the broadened catch-all:
Inline comment below on the main one (getTask InvalidParams now exposed).
Also — pre-existing consistency gap this PR exposes (not in diff, at ~L313): handleAutomaticTaskPolling throws plain Error('No task store provided for task-capable tool.') for a server misconfig, while the analogous taskSupport mismatch at L177 uses ProtocolError(InternalError). Both are server-side config bugs, but with this PR's broadened re-throw, L177 now becomes a JSON-RPC error while L313 stays {isError: true}. Worth aligning while you're here?
Test-coverage nits (non-blocking):
- Tests at L1429/1551/6432 assert error messages but not codes — since the PR's thesis is code alignment, consider
.rejects.toMatchObject({ code: ProtocolErrorCode.X, message: /.../ })(matching the existing pattern at L1824) - The new auto-polling output validation path (commit 3f29f35, mcp.ts:335-339) has no test coverage
| @@ -206,8 +206,8 @@ export class McpServer { | |||
| await this.validateToolOutput(tool, result, request.params.name); | |||
| return result; | |||
| } catch (error) { | |||
There was a problem hiding this comment.
This broadened catch now re-throws ProtocolError(InvalidParams) from RequestTaskStore.getTask (taskManager.ts:666-668) when a task vanishes mid-polling at L327. Before this PR, that was wrapped as {isError: true}; now it surfaces as a JSON-RPC InvalidParams error — but a task going missing during automatic polling isn't the client's fault.
Options:
- Catch-and-convert in
handleAutomaticTaskPollingbefore L327'sgetTaskresult is consumed - Or fix the underlying
RequestTaskStore.getTaskto throwInternalErrorinstead ofInvalidParamsfor task-not-found
Also: the null-check at L328-330 (if (!task) throw new ProtocolError(InternalError, ...)) is dead code — getTask throws before it can return null.
…ling Addresses review from @bhosmer-ant on #1769: - handleAutomaticTaskPolling: wrap getTask in try/catch and convert ProtocolError(InvalidParams) to InternalError when a task vanishes mid-poll. A task going missing during automatic polling is a server-side issue — the client didn't request a task. - handleAutomaticTaskPolling: change plain Error('No task store...') to ProtocolError(InternalError) for consistency with the analogous taskSupport config check at L177. - Update three tests to assert error codes (not just messages) using toMatchObject, matching the pattern at L1824. Follow-up (non-blocking): test coverage for the auto-polling output validation path added in 3f29f35.
2 participants
Re-throws all
ProtocolErrorinstances from thetools/callhandler as JSON-RPC errors. Previously onlyUrlElicitationRequiredwas re-thrown; otherProtocolErrors thrown inside the try block (output validation, task misconfiguration) were silently wrapped asisError: truetool results.Motivation and Context
The MCP spec classifies tool errors into two categories:
isError: true): API failures, input validation errors, business logic errorsThe current catch block only re-throws
UrlElicitationRequired, which means output validation failures (a server-side bug) and task misconfiguration errors get demoted to tool-levelisError: trueresults when they should be protocol-level JSON-RPC errors.This also means tool handlers that deliberately
throw new ProtocolError(...)get their intent overridden — the python-sdk re-throws allMCPErrorin the equivalent path.How Has This Been Tested?
Updated existing integration tests to reflect the new behavior. All tests pass locally.
Breaking Changes
Yes — output validation failures and task-required-without-task now throw
ProtocolErrorinstead of returning{ isError: true }. See migration guide updates.Input validation behavior is unchanged (still
isError: true, per spec).Types of changes
Checklist
Additional context
Surfaced while triaging #1674, which proposed the broadened re-throw but would have also promoted input validation to JSON-RPC (spec violation). This PR applies the broadening while keeping input validation tool-level by changing
validateToolInputto throw plainErrorinstead ofProtocolError.