Fix: Return CallToolResult with isError for tool errors to conform with MCP spec #354
Conversation
…rowing exceptions Update server-side tool call handling to conform with MCP specification: - handleCallTool now returns CallToolResult with isError=true for non-existent tools - Added exception handling to catch tool execution errors and return them as CallToolResult with isError=true - Updated client-side tests to expect CallToolResult with isError instead of protocol-level exceptions - Updated server-side integration tests to verify new error handling behavior
There was a problem hiding this comment.
Pull Request Overview
This PR refactors error handling in the MCP server to return errors as CallToolResult objects with isError=true instead of throwing exceptions. The change aligns tool call error handling with the MCP protocol specification.
- Changed
handleCallToolin Server.kt to return error results instead of throwing exceptions for missing tools and tool execution failures - Updated test cases across TypeScript integration tests (STDIO and SSE) and Kotlin integration tests to validate error responses instead of asserting on thrown exceptions
- Removed
assertThrowsimports from test files where they are no longer needed
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| kotlin-sdk-server/src/commonMain/kotlin/io/modelcontextprotocol/kotlin/sdk/server/Server.kt | Modified handleCallTool to return CallToolResult with isError=true for missing tools and exceptions, added exception handling with proper re-throwing of CancellationException |
| kotlin-sdk-test/src/jvmTest/kotlin/io/modelcontextprotocol/kotlin/sdk/integration/typescript/stdio/KotlinClientTsServerEdgeCasesTestStdio.kt | Updated tests for non-existent tool and invalid arguments to validate error responses instead of exceptions |
| kotlin-sdk-test/src/jvmTest/kotlin/io/modelcontextprotocol/kotlin/sdk/integration/typescript/sse/KotlinClientTsServerEdgeCasesTestSse.kt | Updated tests for non-existent tool and invalid arguments to validate error responses instead of exceptions |
| kotlin-sdk-test/src/jvmTest/kotlin/io/modelcontextprotocol/kotlin/sdk/integration/kotlin/AbstractToolIntegrationTest.kt | Updated tests for non-existent tool and exception handling to validate error responses instead of exceptions |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| logger.error { "Tool not found: ${request.name}" } | ||
| throw IllegalArgumentException("Tool not found: ${request.name}") | ||
| return CallToolResult( | ||
| content = listOf(TextContent(text = "Tool ${request.name} not found")), |
There was a problem hiding this comment.
Inconsistent error message format. The log message uses 'Tool not found: ${request.name}' (line 524), but the returned content uses 'Tool ${request.name} not found'. These should match for consistency. Consider changing to 'Tool not found: ${request.name}' to align with the logging and other error messages in the codebase (e.g., 'Prompt not found: ${request.name}' on line 556).
| content = listOf(TextContent(text = "Tool ${request.name} not found")), | |
| content = listOf(TextContent(text = "Tool not found: ${request.name}")), |
| client.callTool(errorToolName, exceptionArgs) | ||
| } | ||
| } | ||
| val exceptionResult = client.callTool(errorToolName, exceptionArgs) as CallToolResultBase |
There was a problem hiding this comment.
Missing runBlocking wrapper for this coroutine call. Unlike line 774 where runBlocking is used to call client.callTool, this call is not wrapped in runBlocking. This inconsistency could lead to issues if the surrounding context changes. Add runBlocking wrapper for consistency with the pattern used elsewhere in the file.
|
|
||
| val msg = exception.message ?: "" | ||
| val expectedMessage = "JSONRPCError(code=InternalError, message=My exception message, data={})" | ||
| assertTrue(exceptionResult.isError ?: false, "isError should be true for exception") |
There was a problem hiding this comment.
| assertTrue(exceptionResult.isError ?: false, "isError should be true for exception") | |
| assertTrue(exceptionResult.isError == true, "isError should be true for exception") |
Elvis operator looks crypric in asserts.
| val exceptionContent = exceptionResult.content.firstOrNull { it is TextContent } as? TextContent | ||
| assertNotNull(exceptionContent, "Error content should be present in the result") | ||
|
|
||
| val exceptionText = exceptionContent.text ?: "" | ||
| assertTrue( | ||
| exceptionText.contains("Error executing tool") && exceptionText.contains("My exception message"), | ||
| "Error message should contain the exception details", | ||
| ) |
There was a problem hiding this comment.
IMO it would be more readable with kotest assertions
| val expectedMessage = "JSONRPCError(code=InternalError, message=Tool not found: non-existent-tool, data={})" | ||
| assertNotNull(result, "Tool call result should not be null") | ||
| val callResult = result as CallToolResult | ||
| assertTrue(callResult.isError ?: false, "isError should be true for non-existent tool") |
Update server-side tool error handling to return
CallToolResultwithisError: trueinstead of throwing protocol-level exceptions, conforming with MCP specificationMotivation and Context
The MCP specification states:
This PR updates the kotlin sdk to match this behavior
How Has This Been Tested?
All tests pass successfully with the new behavior
Breaking Changes
None
Types of changes
Checklist
Additional context
The TypeScript SDK was recently updated #1044 to conform with the MCP specification regarding tool error handling. That’s why we got exceptions in the integration tests