Fix: Non-existent tool, disabled tool and inputSchema validation return MCP protocol level instead of CallToolResult with isError: true (#1044)
This commit is contained in:
parent
874aa27a89
commit
7387c44bf5
3 changed files with 150 additions and 116 deletions
10
package-lock.json
generated
10
package-lock.json
generated
|
|
@ -52,19 +52,11 @@
|
|||
"node": ">=18"
|
||||
},
|
||||
"peerDependencies": {
|
||||
"@cfworker/json-schema": "^4.1.1",
|
||||
"ajv": "^8.17.1",
|
||||
"ajv-formats": "^3.0.1"
|
||||
"@cfworker/json-schema": "^4.1.1"
|
||||
},
|
||||
"peerDependenciesMeta": {
|
||||
"@cfworker/json-schema": {
|
||||
"optional": true
|
||||
},
|
||||
"ajv": {
|
||||
"optional": true
|
||||
},
|
||||
"ajv-formats": {
|
||||
"optional": true
|
||||
}
|
||||
}
|
||||
},
|
||||
|
|
|
|||
|
|
@ -928,37 +928,53 @@ describe('tool()', () => {
|
|||
|
||||
await Promise.all([client.connect(clientTransport), mcpServer.server.connect(serverTransport)]);
|
||||
|
||||
await expect(
|
||||
client.request(
|
||||
{
|
||||
method: 'tools/call',
|
||||
params: {
|
||||
const result = await client.request(
|
||||
{
|
||||
method: 'tools/call',
|
||||
params: {
|
||||
name: 'test',
|
||||
arguments: {
|
||||
name: 'test',
|
||||
arguments: {
|
||||
name: 'test',
|
||||
value: 'not a number'
|
||||
}
|
||||
value: 'not a number'
|
||||
}
|
||||
},
|
||||
CallToolResultSchema
|
||||
)
|
||||
).rejects.toThrow(/Invalid arguments/);
|
||||
}
|
||||
},
|
||||
CallToolResultSchema
|
||||
);
|
||||
|
||||
await expect(
|
||||
client.request(
|
||||
expect(result.isError).toBe(true);
|
||||
expect(result.content).toEqual(
|
||||
expect.arrayContaining([
|
||||
{
|
||||
method: 'tools/call',
|
||||
params: {
|
||||
name: 'test (new api)',
|
||||
arguments: {
|
||||
name: 'test',
|
||||
value: 'not a number'
|
||||
}
|
||||
type: 'text',
|
||||
text: expect.stringContaining('Input validation error: Invalid arguments for tool test')
|
||||
}
|
||||
])
|
||||
);
|
||||
|
||||
const result2 = await client.request(
|
||||
{
|
||||
method: 'tools/call',
|
||||
params: {
|
||||
name: 'test (new api)',
|
||||
arguments: {
|
||||
name: 'test',
|
||||
value: 'not a number'
|
||||
}
|
||||
},
|
||||
CallToolResultSchema
|
||||
)
|
||||
).rejects.toThrow(/Invalid arguments/);
|
||||
}
|
||||
},
|
||||
CallToolResultSchema
|
||||
);
|
||||
|
||||
expect(result2.isError).toBe(true);
|
||||
expect(result2.content).toEqual(
|
||||
expect.arrayContaining([
|
||||
{
|
||||
type: 'text',
|
||||
text: expect.stringContaining('Input validation error: Invalid arguments for tool test (new api)')
|
||||
}
|
||||
])
|
||||
);
|
||||
});
|
||||
|
||||
/***
|
||||
|
|
@ -1152,14 +1168,24 @@ describe('tool()', () => {
|
|||
await Promise.all([client.connect(clientTransport), mcpServer.server.connect(serverTransport)]);
|
||||
|
||||
// Call the tool and expect it to throw an error
|
||||
await expect(
|
||||
client.callTool({
|
||||
name: 'test',
|
||||
arguments: {
|
||||
input: 'hello'
|
||||
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'
|
||||
)
|
||||
}
|
||||
})
|
||||
).rejects.toThrow(/Tool test has an output schema but no structured content was provided/);
|
||||
])
|
||||
);
|
||||
});
|
||||
/***
|
||||
* Test: Tool with Output Schema Must Provide Structured Content
|
||||
|
|
@ -1274,14 +1300,22 @@ describe('tool()', () => {
|
|||
await Promise.all([client.connect(clientTransport), mcpServer.server.connect(serverTransport)]);
|
||||
|
||||
// Call the tool and expect it to throw a server-side validation error
|
||||
await expect(
|
||||
client.callTool({
|
||||
name: 'test',
|
||||
arguments: {
|
||||
input: 'hello'
|
||||
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')
|
||||
}
|
||||
})
|
||||
).rejects.toThrow(/Invalid structured content for tool test/);
|
||||
])
|
||||
);
|
||||
});
|
||||
|
||||
/***
|
||||
|
|
@ -1552,17 +1586,25 @@ describe('tool()', () => {
|
|||
|
||||
await Promise.all([client.connect(clientTransport), mcpServer.server.connect(serverTransport)]);
|
||||
|
||||
await expect(
|
||||
client.request(
|
||||
const result = await client.request(
|
||||
{
|
||||
method: 'tools/call',
|
||||
params: {
|
||||
name: 'nonexistent-tool'
|
||||
}
|
||||
},
|
||||
CallToolResultSchema
|
||||
);
|
||||
|
||||
expect(result.isError).toBe(true);
|
||||
expect(result.content).toEqual(
|
||||
expect.arrayContaining([
|
||||
{
|
||||
method: 'tools/call',
|
||||
params: {
|
||||
name: 'nonexistent-tool'
|
||||
}
|
||||
},
|
||||
CallToolResultSchema
|
||||
)
|
||||
).rejects.toThrow(/Tool nonexistent-tool not found/);
|
||||
type: 'text',
|
||||
text: expect.stringContaining('Tool nonexistent-tool not found')
|
||||
}
|
||||
])
|
||||
);
|
||||
});
|
||||
|
||||
/***
|
||||
|
|
|
|||
|
|
@ -126,73 +126,55 @@ export class McpServer {
|
|||
|
||||
this.server.setRequestHandler(CallToolRequestSchema, async (request, extra): Promise<CallToolResult> => {
|
||||
const tool = this._registeredTools[request.params.name];
|
||||
if (!tool) {
|
||||
throw new McpError(ErrorCode.InvalidParams, `Tool ${request.params.name} not found`);
|
||||
}
|
||||
|
||||
if (!tool.enabled) {
|
||||
throw new McpError(ErrorCode.InvalidParams, `Tool ${request.params.name} disabled`);
|
||||
}
|
||||
|
||||
let result: CallToolResult;
|
||||
|
||||
if (tool.inputSchema) {
|
||||
const parseResult = await tool.inputSchema.safeParseAsync(request.params.arguments);
|
||||
if (!parseResult.success) {
|
||||
throw new McpError(
|
||||
ErrorCode.InvalidParams,
|
||||
`Invalid arguments for tool ${request.params.name}: ${parseResult.error.message}`
|
||||
);
|
||||
try {
|
||||
if (!tool) {
|
||||
throw new McpError(ErrorCode.InvalidParams, `Tool ${request.params.name} not found`);
|
||||
}
|
||||
|
||||
const args = parseResult.data;
|
||||
const cb = tool.callback as ToolCallback<ZodRawShape>;
|
||||
try {
|
||||
if (!tool.enabled) {
|
||||
throw new McpError(ErrorCode.InvalidParams, `Tool ${request.params.name} disabled`);
|
||||
}
|
||||
|
||||
if (tool.inputSchema) {
|
||||
const cb = tool.callback as ToolCallback<ZodRawShape>;
|
||||
const parseResult = await tool.inputSchema.safeParseAsync(request.params.arguments);
|
||||
if (!parseResult.success) {
|
||||
throw new McpError(
|
||||
ErrorCode.InvalidParams,
|
||||
`Input validation error: Invalid arguments for tool ${request.params.name}: ${parseResult.error.message}`
|
||||
);
|
||||
}
|
||||
|
||||
const args = parseResult.data;
|
||||
|
||||
result = await Promise.resolve(cb(args, extra));
|
||||
} catch (error) {
|
||||
result = {
|
||||
content: [
|
||||
{
|
||||
type: 'text',
|
||||
text: error instanceof Error ? error.message : String(error)
|
||||
}
|
||||
],
|
||||
isError: true
|
||||
};
|
||||
}
|
||||
} else {
|
||||
const cb = tool.callback as ToolCallback<undefined>;
|
||||
try {
|
||||
} else {
|
||||
const cb = tool.callback as ToolCallback<undefined>;
|
||||
result = await Promise.resolve(cb(extra));
|
||||
} catch (error) {
|
||||
result = {
|
||||
content: [
|
||||
{
|
||||
type: 'text',
|
||||
text: error instanceof Error ? error.message : String(error)
|
||||
}
|
||||
],
|
||||
isError: true
|
||||
};
|
||||
}
|
||||
}
|
||||
|
||||
if (tool.outputSchema && !result.isError) {
|
||||
if (!result.structuredContent) {
|
||||
throw new McpError(
|
||||
ErrorCode.InvalidParams,
|
||||
`Tool ${request.params.name} has an output schema but no structured content was provided`
|
||||
);
|
||||
}
|
||||
|
||||
// if the tool has an output schema, validate structured content
|
||||
const parseResult = await tool.outputSchema.safeParseAsync(result.structuredContent);
|
||||
if (!parseResult.success) {
|
||||
throw new McpError(
|
||||
ErrorCode.InvalidParams,
|
||||
`Invalid structured content for tool ${request.params.name}: ${parseResult.error.message}`
|
||||
);
|
||||
if (tool.outputSchema && !result.isError) {
|
||||
if (!result.structuredContent) {
|
||||
throw new McpError(
|
||||
ErrorCode.InvalidParams,
|
||||
`Output validation error: Tool ${request.params.name} has an output schema but no structured content was provided`
|
||||
);
|
||||
}
|
||||
|
||||
// if the tool has an output schema, validate structured content
|
||||
const parseResult = await tool.outputSchema.safeParseAsync(result.structuredContent);
|
||||
if (!parseResult.success) {
|
||||
throw new McpError(
|
||||
ErrorCode.InvalidParams,
|
||||
`Output validation error: Invalid structured content for tool ${request.params.name}: ${parseResult.error.message}`
|
||||
);
|
||||
}
|
||||
}
|
||||
} catch (error) {
|
||||
return this.createToolError(error instanceof Error ? error.message : String(error));
|
||||
}
|
||||
|
||||
return result;
|
||||
|
|
@ -201,6 +183,24 @@ export class McpServer {
|
|||
this._toolHandlersInitialized = true;
|
||||
}
|
||||
|
||||
/**
|
||||
* Creates a tool error result.
|
||||
*
|
||||
* @param errorMessage - The error message.
|
||||
* @returns The tool error result.
|
||||
*/
|
||||
private createToolError(errorMessage: string): CallToolResult {
|
||||
return {
|
||||
content: [
|
||||
{
|
||||
type: 'text',
|
||||
text: errorMessage
|
||||
}
|
||||
],
|
||||
isError: true
|
||||
};
|
||||
}
|
||||
|
||||
private _completionHandlerInitialized = false;
|
||||
|
||||
private setCompletionRequestHandler() {
|
||||
|
|
|
|||
Loading…
Reference in a new issue