fix: align tool error handling with MCP specification #540
Conversation
Implement proper error handling for tool calls according to MCP Tools' Error Handling specification (https://modelcontextprotocol.io/specification/2025-06-18/server/tools#error-handling) - Tool errors are now returned within CallToolResult with isError=true instead of throwing MCP protocol-level errors - This allows LLMs to see and handle tool errors as part of normal flow - Return proper JSON-RPC error with code -32602 in case of "Unknown tools" - Added Utils.findRootCause() helper to extract root cause from exception chains - Skip output schema validation when tool result already indicates an error - Updated all integration tests to verify new error handling behavior BREAKING CHANGE: Tool call errors are no longer thrown as McpError exceptions. They are now returned as CallToolResult objects with isError=true and error details in the content field, following MCP specification guidelines. Resolves modelcontextprotocol#538 Related to modelcontextprotocol#422 Signed-off-by: Christian Tzolov <[email protected]>
- Changed tool error handling to return CallToolResult with isError=true - Tool errors are now reported within the result object, not as MCP protocol-level errors - This allows LLMs to see and potentially handle errors gracefully - Added comprehensive tests for in-handler error scenarios - Added JavaDoc for Utils.findRootCause() method - Updated existing timeout test to expect CallToolResult instead of exception BREAKING CHANGE: Tool call errors no longer throw McpError exceptions but return error results instead. Clients should check CallToolResult.isError() to handle errors. Signed-off-by: Christian Tzolov <[email protected]>
| // TODO: Should we handle the McpError+JsonRcpError specaially (e.g. | ||
| // propagate) | ||
| // or always return a CallToolResult with isError=true? | ||
| if (error instanceof McpError mcpError && mcpError.getJsonRpcError() != null) { |
There was a problem hiding this comment.
I'm not sure this this assumption is correct? should we thread McpError + JsonRpcError content differently?
If so we have to implement this logic for the McpAsyncServer's tool handler. Otherwise remove this condition here.
@chemicL any thoughts?
There was a problem hiding this comment.
I'm guessing we shouldn't allow users to throw a generic McpError? This way we don't have to perform these checks but provide the users a framework of errors to indicate the result. So e.g. in case of logical errors they either return the CallToolResult with isError themselves or fail with a known exception type. From the outside we can't tell what is the reason so without such family of errors we'd have to assume it's a server error and we treat it as JSON-RPC error response.
There was a problem hiding this comment.
To summarize our sync conversation:
- let's keep the check for errors in the structured output wrapper
- let's keep the enhanced output and error code in case of "Unkown tool:"
But regarding the main motivation for this PR, my view is that we should discard these changes. The user is able to communicate tool calling errors since the handlers themselves allow return a CallToolResult with isError flag set to true. Any error/exception communicated from the handlers should be treated as a generic protocol error (McpError).
The motivating problem can exist in middleware libraries such as annotation processors that limit the programming freedom of the user, e.g. by allowing the tool-handling method to only return String. In such a case, these mentioned contracts (special types of exceptions) can be established at that middleware layer and the type checks can dictate the logic.
- Add validation bypass when CallToolResult.isError() is true in async/stateless servers - Fix async tool handler chaining to properly use then() instead of block() - Add comprehensive tests for structured output with in-handler errors - Improve error handling to use proper JSON-RPC error codes for unknown tools - Add findRootCause utility method for better error diagnostics - Increase test timeouts for stability in StdioMcp client tests This ensures that when a tool handler returns an error result, the structured output schema validation is skipped, preventing validation failures on error responses that don't conform to the expected output schema. Signed-off-by: Christian Tzolov <[email protected]>
|
Thanks @chemicL . I've updated the PR. |
| @@ -249,6 +251,11 @@ public Mono<CallToolResult> apply(McpTransportContext transportContext, McpSchem | |||
|
|
|||
| return this.delegateHandler.apply(transportContext, request).map(result -> { | |||
|
|
|||
| if (result.isError() != null && result.isError()) { | |||
There was a problem hiding this comment.
nit: can simplify to Boolean.TRUE.equals(result.isError())
| @@ -376,6 +378,11 @@ public Mono<CallToolResult> apply(McpAsyncServerExchange exchange, McpSchema.Cal | |||
|
|
|||
| return this.delegateCallToolResult.apply(exchange, request).map(result -> { | |||
|
|
|||
| if (result.isError() != null && result.isError()) { | |||
There was a problem hiding this comment.
nit: can simplify to Boolean.TRUE.equals(result.isError())
| * @return The root cause throwable | ||
| * @throws NullPointerException if the provided throwable is null | ||
| */ | ||
| public static Throwable findRootCause(Throwable throwable) { |
|
Thanks @chemicL I've addressed those before the merge! |
|
Rebased, squashed, cleaned and merged at improve-tool-call-error-handling c7736b6 |
fix: Skip structured output validation for error tool results
This ensures that when a tool handler returns an error result, the structured
output schema validation is skipped, preventing validation failures on error
responses that don't conform to the expected output schema.
Resolves #538
Related to #422