Skip to content

Commit 54ef5ea

Browse files
fix(server): address review feedback on ProtocolError re-throw alignment
- Remove dead null-guard after getTask try-catch (getTask never returns null) - Add migration doc bullet for no-task-store InternalError case - Add test for output schema validation in automatic-polling path Prior commits on this branch already addressed: MethodNotFound -> InvalidParams, plain Error -> ProtocolError for no-task-store, getTask InvalidParams -> InternalError conversion, status-guarded validateToolOutput, and error-code assertions in tests.
1 parent ffc96dd commit 54ef5ea

4 files changed

Lines changed: 105 additions & 6 deletions

File tree

docs/migration-SKILL.md

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -174,6 +174,7 @@ Behavior changes in `callTool` results:
174174
- Output validation failure: `{ isError: true }` → throws `ProtocolError` (`InternalError`)
175175
- Task-required without task: `{ isError: true }` → throws `ProtocolError` (`InvalidParams`)
176176
- Handler throws `ProtocolError`: `{ isError: true }` → re-thrown as JSON-RPC error
177+
- No task store configured for `taskSupport: 'optional'` tool: `{ isError: true }` → throws `ProtocolError` (`InternalError`)
177178
- Handler throws plain `Error`: `{ isError: true }``{ isError: true }` (unchanged)
178179

179180
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`.

docs/migration.md

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -641,6 +641,7 @@ This aligns error surfaces with the MCP spec's classification:
641641
- **Output validation failure** — now throws `ProtocolError` with `InternalError` code (was `{ isError: true }`)
642642
- **Task-required tool called without task** — now throws `ProtocolError` with `InvalidParams` code (was `{ isError: true }`)
643643
- **Handler throws `ProtocolError`** — now re-thrown as a JSON-RPC error (was `{ isError: true }`)
644+
- **No task store configured for `taskSupport: 'optional'` tool** — now throws `ProtocolError` with `InternalError` code (was `{ isError: true }`)
644645
- **Handler throws plain `Error`** — unchanged, still returns `{ isError: true }`
645646

646647
**Before (v1):**

packages/server/src/server/mcp.ts

Lines changed: 1 addition & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -324,9 +324,8 @@ export class McpServer {
324324

325325
while (task.status !== 'completed' && task.status !== 'failed' && task.status !== 'cancelled') {
326326
await new Promise(resolve => setTimeout(resolve, pollInterval));
327-
let updatedTask;
328327
try {
329-
updatedTask = await ctx.task.store.getTask(taskId);
328+
task = await ctx.task.store.getTask(taskId);
330329
} catch (error) {
331330
// RequestTaskStore.getTask throws InvalidParams when the task is
332331
// missing, but a task vanishing mid-poll is a server-side issue
@@ -336,10 +335,6 @@ export class McpServer {
336335
}
337336
throw error;
338337
}
339-
if (!updatedTask) {
340-
throw new ProtocolError(ProtocolErrorCode.InternalError, `Task ${taskId} not found during polling`);
341-
}
342-
task = updatedTask;
343338
}
344339

345340
// Return the final result

test/integration/test/server/mcp.test.ts

Lines changed: 102 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6642,6 +6642,108 @@ describe('Zod v4', () => {
66426642
taskStore.cleanup();
66436643
});
66446644

6645+
test('should validate output schema for taskSupport "optional" tool result returned via automatic polling', async () => {
6646+
const taskStore = new InMemoryTaskStore();
6647+
const { releaseLatch, waitForLatch } = createLatch();
6648+
6649+
const mcpServer = new McpServer(
6650+
{
6651+
name: 'test server',
6652+
version: '1.0'
6653+
},
6654+
{
6655+
capabilities: {
6656+
tools: {},
6657+
tasks: {
6658+
requests: {
6659+
tools: {
6660+
call: {}
6661+
}
6662+
},
6663+
6664+
taskStore
6665+
}
6666+
}
6667+
}
6668+
);
6669+
6670+
const client = new Client(
6671+
{
6672+
name: 'test client',
6673+
version: '1.0'
6674+
},
6675+
{
6676+
capabilities: {
6677+
tasks: {
6678+
requests: {
6679+
tools: {
6680+
call: {}
6681+
}
6682+
}
6683+
}
6684+
}
6685+
}
6686+
);
6687+
6688+
mcpServer.experimental.tasks.registerToolTask(
6689+
'optional-task-with-output-schema',
6690+
{
6691+
description: 'An optional task with an output schema',
6692+
inputSchema: z.object({
6693+
value: z.number()
6694+
}),
6695+
outputSchema: z.object({
6696+
count: z.number()
6697+
}),
6698+
execution: {
6699+
taskSupport: 'optional'
6700+
}
6701+
},
6702+
{
6703+
createTask: async (_args, ctx) => {
6704+
const task = await ctx.task.store.createTask({ ttl: 60_000, pollInterval: 100 });
6705+
const store = ctx.task.store;
6706+
setTimeout(async () => {
6707+
await store.storeTaskResult(task.taskId, 'completed', {
6708+
content: [{ type: 'text' as const, text: 'done' }],
6709+
structuredContent: { count: 'not-a-number' }
6710+
});
6711+
releaseLatch();
6712+
}, 150);
6713+
return { task };
6714+
},
6715+
getTask: async (_args, ctx) => {
6716+
const task = await ctx.task.store.getTask(ctx.task.id);
6717+
if (!task) {
6718+
throw new Error('Task not found');
6719+
}
6720+
return task;
6721+
},
6722+
getTaskResult: async (_args, ctx) => {
6723+
const result = await ctx.task.store.getTaskResult(ctx.task.id);
6724+
return result as CallToolResult;
6725+
}
6726+
}
6727+
);
6728+
6729+
const [clientTransport, serverTransport] = InMemoryTransport.createLinkedPair();
6730+
6731+
await Promise.all([client.connect(clientTransport), mcpServer.connect(serverTransport)]);
6732+
6733+
await expect(
6734+
client.callTool({
6735+
name: 'optional-task-with-output-schema',
6736+
arguments: { value: 1 }
6737+
})
6738+
).rejects.toMatchObject({
6739+
code: ProtocolErrorCode.InternalError,
6740+
message: expect.stringMatching(/Invalid structured content/)
6741+
});
6742+
6743+
await waitForLatch();
6744+
taskStore.cleanup();
6745+
});
6746+
66456747
test('should return CreateTaskResult when tool with taskSupport "required" is called WITH task augmentation', async () => {
66466748
const taskStore = new InMemoryTaskStore();
66476749
const { releaseLatch, waitForLatch } = createLatch();

0 commit comments

Comments
 (0)