Skip to content

fix(server): align ProtocolError re-throw with spec error classification #1769

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
felixweinberger wants to merge 1 commit into
base: main
Choose a base branch
from
+91 −52
Open

fix(server): align ProtocolError re-throw with spec error classification #1769

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions .changeset/protocol-error-rethrow.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
---
'@modelcontextprotocol/server': minor
---

Re-throw all `ProtocolError` instances from `tools/call` handler as JSON-RPC errors instead of wrapping them in `isError: true` results.

**Breaking change:** Output validation failures (missing or schema-mismatched `structuredContent`) now surface as JSON-RPC `InternalError` rejections instead of `{ isError: true }` results. Input validation failures continue to return `{ isError: true }` per the MCP spec's tool-execution-error classification.

This also means tool handlers that deliberately `throw new ProtocolError(...)` will now propagate that as a JSON-RPC error, matching the python-sdk behavior.
14 changes: 14 additions & 0 deletions docs/migration-SKILL.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -163,6 +163,20 @@ if (error instanceof SdkError && error.code === SdkErrorCode.ClientHttpFailedToO
}
```

### Tool error classification (McpServer tools/call)

`McpServer` now re-throws any `ProtocolError` from the `tools/call` handler as a JSON-RPC error. Previously only `UrlElicitationRequired` was re-thrown; other protocol errors were wrapped as `{ isError: true }` results.

Behavior changes in `callTool` results:

- Input validation failure: `{ isError: true }` → `{ isError: true }` (unchanged)
- Output validation failure: `{ isError: true }` → throws `ProtocolError` (`InternalError`)
- Task-required without task: `{ isError: true }` → throws `ProtocolError` (`MethodNotFound`)
- Handler throws `ProtocolError`: `{ isError: true }` → re-thrown as JSON-RPC error
- Handler throws plain `Error`: `{ isError: true }` → `{ isError: true }` (unchanged)

Migration: if code checks `result.isError` to detect output-schema violations or deliberate `ProtocolError` throws, add a `try/catch` around `callTool`. If a handler throws `ProtocolError` expecting tool-level wrapping, change it to throw a plain `Error`.

### OAuth error consolidation

Individual OAuth error classes replaced with single `OAuthError` class and `OAuthErrorCode` enum:
Expand Down
38 changes: 38 additions & 0 deletions docs/migration.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -607,6 +607,44 @@ try {
}
```

#### Tool error classification

The `tools/call` handler in `McpServer` now re-throws any `ProtocolError` as a JSON-RPC error instead of wrapping it in an `isError: true` result. Previously, only `UrlElicitationRequired` was re-thrown.

This aligns error surfaces with the MCP spec's classification:

- **Input validation failure** — unchanged, still returns `{ isError: true }` (spec classifies this as a tool-execution error)
- **Output validation failure** — now throws `ProtocolError` with `InternalError` code (was `{ isError: true }`)
- **Task-required tool called without task** — now throws `ProtocolError` with `MethodNotFound` code (was `{ isError: true }`)
- **Handler throws `ProtocolError`** — now re-thrown as a JSON-RPC error (was `{ isError: true }`)
- **Handler throws plain `Error`** — unchanged, still returns `{ isError: true }`

**Before (v1):**

```typescript
const result = await client.callTool({ name: 'test', arguments: {} });
if (result.isError) {
// caught output-schema mismatches, task misconfig, handler ProtocolErrors
}
```

**After (v2):**

```typescript
try {
const result = await client.callTool({ name: 'test', arguments: {} });
if (result.isError) {
// only input validation and ordinary handler exceptions land here
}
} catch (error) {
if (error instanceof ProtocolError) {
// output validation failure, task misconfig, or handler-thrown ProtocolError
}
}
```

If your tool handler was throwing `ProtocolError` expecting it to be wrapped as `isError: true`, throw a plain `Error` instead.

#### New `SdkErrorCode` enum

The new `SdkErrorCode` enum contains string-valued codes for local SDK errors:
Expand Down
15 changes: 7 additions & 8 deletions packages/server/src/server/mcp.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -206,8 +206,8 @@ export class McpServer {
await this.validateToolOutput(tool, result, request.params.name);
return result;
} catch (error) {
if (error instanceof ProtocolError && error.code === ProtocolErrorCode.UrlElicitationRequired) {
throw error; // Return the error to the caller without wrapping in CallToolResult
if (error instanceof ProtocolError) {
throw error;
}
Comment on lines 208 to 211
Copy link

@claude claude bot Mar 26, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟣 The MethodNotFound (-32601) error code at line 186 for task-required-without-task is semantically incorrect per JSON-RPC: -32601 means "the method does not exist," but tools/call exists and the tool was found — the request is just missing the task parameter. This is a pre-existing error code choice, but this PR makes it client-visible by broadening the catch block to re-throw all ProtocolErrors. Consider using InvalidParams (-32602), which is already used for analogous cases in this handler (tool not found at line 164, tool disabled at line 167).

Extended reasoning...

What the bug is

At line 186, ProtocolErrorCode.MethodNotFound (-32601) is used when a tool with taskSupport: "required" is called without task augmentation. Per the JSON-RPC 2.0 spec, -32601 means "The method does not exist / is not available." However, in this scenario the tools/call method absolutely exists, the handler was found, and the specific tool was located — the client simply omitted the task parameter from the request.

Why this matters now

Before this PR, this error was caught by the old catch block (which only re-threw UrlElicitationRequired) and wrapped into an isError: true tool result. The JSON-RPC error code was irrelevant because it never reached the client as a protocol-level error. After this PR, the broadened catch at line 209 (if (error instanceof ProtocolError) { throw error; }) re-throws it as a JSON-RPC error response, making the -32601 code directly visible to clients.

Step-by-step proof

  1. Server registers a tool with taskSupport: "required" via registerToolTask.
  2. Client sends tools/call for that tool WITHOUT a task field in the request params.
  3. At line 184, taskSupport === "required" && !isTaskRequest evaluates to true.
  4. A ProtocolError with MethodNotFound code is thrown (line 185-188).
  5. The catch block at line 209 sees it is a ProtocolError and re-throws it.
  6. The JSON-RPC layer sends a -32601 MethodNotFound error response to the client.
  7. A well-behaved client receiving -32601 may conclude that the tools/call method is not supported by this server, which is incorrect.

Internal inconsistency

The same handler uses InvalidParams (-32602) for analogous situations: "tool not found" (line 164) and "tool disabled" (line 167) both use ProtocolErrorCode.InvalidParams. The task-required case is structurally the same — the request is valid but a required parameter is missing — yet uses a different error code.

Impact

A client receiving -32601 could misinterpret it as the server not supporting tools/call at all, rather than understanding that the specific invocation is missing the task parameter. In practice, the error message text ("requires task augmentation") clarifies the issue, so clients parsing the message string will understand. But clients that dispatch on error codes alone will be misled.

Mitigation

The error code at line 186 is pre-existing and was not introduced by this PR. The PR author explicitly documented this as MethodNotFound in the migration guides, suggesting it may be an intentional design choice (interpreting "not available" as "this invocation mode is not available"). However, InvalidParams (-32602, "Invalid method parameter(s)") would be more semantically correct and consistent with how the same handler treats other invalid-request scenarios. The fix would be changing ProtocolErrorCode.MethodNotFound to ProtocolErrorCode.InvalidParams at line 186.

return this.createToolError(error instanceof Error ? error.message : String(error));
}
Comment on lines 206 to 213
Copy link

@claude claude bot Mar 26, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟣 Pre-existing bug: handleAutomaticTaskPolling (line 335) returns the task result from ctx.task.store.getTaskResult(taskId) without ever calling validateToolOutput(). Tools with taskSupport: "optional", an outputSchema, and no task augmentation from the client will have their results bypass output schema validation entirely. This PR improves output validation error surfacing, but that improvement has no effect on this code path since validateToolOutput is never invoked.

Extended reasoning...

What the bug is

The handleAutomaticTaskPolling method in McpServer handles the case where a tool has taskSupport: "optional" but the client calls it without task augmentation. In this path, the server automatically creates a task, polls until completion, and returns the final result. However, this method never calls validateToolOutput() on the result before returning it.

The specific code path

When a tool call arrives at the tools/call handler with taskSupport: "optional" and no request.params.task, execution flows to line ~193: return await this.handleAutomaticTaskPolling(tool, request, ctx). Inside handleAutomaticTaskPolling (lines 307-336), after polling completes, the method returns (await ctx.task.store.getTaskResult(taskId)) as CallToolResult at line 335 -- casting the raw store result directly to CallToolResult without any schema validation.

In contrast, the normal (non-task) execution path explicitly calls await this.validateToolOutput(tool, result, request.params.name) at line 206 before returning the result.

Step-by-step proof

  1. Register a tool with taskSupport: "optional", an outputSchema (e.g., z.object({ count: z.number() })), and a task handler that stores malformed structuredContent (e.g., { count: "not-a-number" }).
  2. A client calls this tool WITHOUT task augmentation (request.params.task is falsy).
  3. The tools/call handler enters the handleAutomaticTaskPolling branch at line 193.
  4. handleAutomaticTaskPolling validates input, creates the task via tool.executor, then polls until task.status === "completed".
  5. At line 335, it returns ctx.task.store.getTaskResult(taskId) -- the malformed structured content reaches the client unvalidated.
  6. If the same tool were called with task augmentation (normal path), validateToolOutput at line 206 would catch the schema mismatch and throw a ProtocolError(InternalError).

Impact

A task handler could store structuredContent that does not conform to the tool's outputSchema, and it would be returned to the client without any validation. This undermines the contract that outputSchema is supposed to enforce. The impact is limited to the specific scenario of taskSupport: "optional" tools called without task augmentation, but it creates an inconsistency where the same tool validates output in one path but not another.

Relationship to this PR

This bug is pre-existing -- the handleAutomaticTaskPolling method is not modified by this PR. However, it is adjacent: this PR correctly changes output validation failures from { isError: true } to ProtocolError(InternalError), but that improvement has zero effect on the automatic polling path since validateToolOutput is never called there.

How to fix

Add a validateToolOutput call in handleAutomaticTaskPolling before returning the result:

const result = (await ctx.task.store.getTaskResult(taskId)) as CallToolResult;
await this.validateToolOutput(tool, result, request.params.name);
return result;

Expand Down Expand Up @@ -251,10 +251,9 @@ export class McpServer {

const parseResult = await validateStandardSchema(tool.inputSchema, args ?? {});
if (!parseResult.success) {
throw new ProtocolError(
ProtocolErrorCode.InvalidParams,
`Input validation error: Invalid arguments for tool ${toolName}: ${parseResult.error}`
);
// Per spec, input validation failures are tool-execution errors (isError: true),
// not protocol errors — throw plain Error so the catch wraps it as a tool result.
throw new Error(`Input validation error: Invalid arguments for tool ${toolName}: ${parseResult.error}`);
}

return parseResult.data as unknown as Args;
Expand All @@ -279,7 +278,7 @@ export class McpServer {

if (!result.structuredContent) {
throw new ProtocolError(
ProtocolErrorCode.InvalidParams,
ProtocolErrorCode.InternalError,
`Output validation error: Tool ${toolName} has an output schema but no structured content was provided`
);
}
Expand All @@ -288,7 +287,7 @@ export class McpServer {
const parseResult = await validateStandardSchema(tool.outputSchema, result.structuredContent);
if (!parseResult.success) {
throw new ProtocolError(
ProtocolErrorCode.InvalidParams,
ProtocolErrorCode.InternalError,
`Output validation error: Invalid structured content for tool ${toolName}: ${parseResult.error}`
);
}
Expand Down
67 changes: 23 additions & 44 deletions test/integration/test/server/mcp.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1418,25 +1418,15 @@ describe('Zod v4', () => {

await Promise.all([client.connect(clientTransport), mcpServer.server.connect(serverTransport)]);

// Call the tool and expect it to throw an error
const result = await client.callTool({
name: 'test',
arguments: {
input: 'hello'
}
});

expect(result.isError).toBe(true);
expect(result.content).toEqual(
expect.arrayContaining([
{
type: 'text',
text: expect.stringContaining(
'Output validation error: Tool test has an output schema but no structured content was provided'
)
// Output validation failure is a server-side bug → JSON-RPC InternalError
await expect(
client.callTool({
name: 'test',
arguments: {
input: 'hello'
}
])
);
})
).rejects.toThrow('Output validation error: Tool test has an output schema but no structured content was provided');
});
/***
* Test: Tool with Output Schema Must Provide Structured Content
Expand Down Expand Up @@ -1550,23 +1540,15 @@ describe('Zod v4', () => {

await Promise.all([client.connect(clientTransport), mcpServer.server.connect(serverTransport)]);

// Call the tool and expect it to throw a server-side validation error
const result = await client.callTool({
name: 'test',
arguments: {
input: 'hello'
}
});

expect(result.isError).toBe(true);
expect(result.content).toEqual(
expect.arrayContaining([
{
type: 'text',
text: expect.stringContaining('Output validation error: Invalid structured content for tool test')
// Output validation failure is a server-side bug → JSON-RPC InternalError
await expect(
client.callTool({
name: 'test',
arguments: {
input: 'hello'
}
])
);
})
).rejects.toThrow(/Output validation error: Invalid structured content for tool test/);
});

/***
Expand Down Expand Up @@ -6441,16 +6423,13 @@ describe('Zod v4', () => {

await Promise.all([client.connect(clientTransport), mcpServer.connect(serverTransport)]);

// Call the tool WITHOUT task augmentation - should return error
const result = await client.callTool({
name: 'long-running-task',
arguments: { input: 'test data' }
});

// Should receive error result
expect(result.isError).toBe(true);
const content = result.content as TextContent[];
expect(content[0]!.text).toContain('requires task augmentation');
// Call the tool WITHOUT task augmentation - should throw JSON-RPC error
await expect(
client.callTool({
name: 'long-running-task',
arguments: { input: 'test data' }
})
).rejects.toThrow(/requires task augmentation/);

taskStore.cleanup();
});
Expand Down
Loading