Fix: Return CallToolResult with isError for tool errors to conform with MCP spec (#354)
Update server-side tool error handling to return `CallToolResult` with `isError: true` instead of throwing protocol-level exceptions, conforming with MCP specification ## Motivation and Context [The MCP specification states](https://modelcontextprotocol.io/specification/2025-06-18/schema#calltoolresult): > Any errors that originate from the tool SHOULD be reported inside the result object, with isError set to true, not as an MCP protocol-level error response. Otherwise, the LLM would not be able to see that an error occurred and self-correct. 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 - [x] Bug fix (non-breaking change which fixes an issue) - [ ] New feature (non-breaking change which adds functionality) - [ ] Breaking change (fix or feature that would cause existing functionality to change) - [ ] Documentation update ## Checklist - [x] I have read the [MCP Documentation](https://modelcontextprotocol.io) - [x] My code follows the repository's style guidelines - [x] New and existing tests pass locally - [x] I have added appropriate error handling - [x] I have added or updated documentation as needed ## Additional context The TypeScript SDK was recently updated [#1044](https://github.com/modelcontextprotocol/typescript-sdk/pull/1044) to conform with the MCP specification regarding tool error handling. That’s why we got exceptions in the integration tests
This commit is contained in:
parent
c5a254b83f
commit
20e4e44509
4 changed files with 95 additions and 77 deletions
|
|
@ -21,6 +21,7 @@ import io.modelcontextprotocol.kotlin.sdk.ReadResourceRequest
|
||||||
import io.modelcontextprotocol.kotlin.sdk.ReadResourceResult
|
import io.modelcontextprotocol.kotlin.sdk.ReadResourceResult
|
||||||
import io.modelcontextprotocol.kotlin.sdk.Resource
|
import io.modelcontextprotocol.kotlin.sdk.Resource
|
||||||
import io.modelcontextprotocol.kotlin.sdk.ServerCapabilities
|
import io.modelcontextprotocol.kotlin.sdk.ServerCapabilities
|
||||||
|
import io.modelcontextprotocol.kotlin.sdk.TextContent
|
||||||
import io.modelcontextprotocol.kotlin.sdk.Tool
|
import io.modelcontextprotocol.kotlin.sdk.Tool
|
||||||
import io.modelcontextprotocol.kotlin.sdk.ToolAnnotations
|
import io.modelcontextprotocol.kotlin.sdk.ToolAnnotations
|
||||||
import io.modelcontextprotocol.kotlin.sdk.shared.ProtocolOptions
|
import io.modelcontextprotocol.kotlin.sdk.shared.ProtocolOptions
|
||||||
|
|
@ -32,6 +33,7 @@ import kotlinx.collections.immutable.minus
|
||||||
import kotlinx.collections.immutable.persistentListOf
|
import kotlinx.collections.immutable.persistentListOf
|
||||||
import kotlinx.collections.immutable.persistentMapOf
|
import kotlinx.collections.immutable.persistentMapOf
|
||||||
import kotlinx.collections.immutable.toPersistentSet
|
import kotlinx.collections.immutable.toPersistentSet
|
||||||
|
import kotlinx.coroutines.CancellationException
|
||||||
|
|
||||||
private val logger = KotlinLogging.logger {}
|
private val logger = KotlinLogging.logger {}
|
||||||
|
|
||||||
|
|
@ -515,13 +517,30 @@ public open class Server(
|
||||||
|
|
||||||
private suspend fun handleCallTool(request: CallToolRequest): CallToolResult {
|
private suspend fun handleCallTool(request: CallToolRequest): CallToolResult {
|
||||||
logger.debug { "Handling tool call request for tool: ${request.name}" }
|
logger.debug { "Handling tool call request for tool: ${request.name}" }
|
||||||
|
|
||||||
|
// Check if tool exists
|
||||||
val tool = _tools.value[request.name]
|
val tool = _tools.value[request.name]
|
||||||
?: run {
|
?: run {
|
||||||
logger.error { "Tool not found: ${request.name}" }
|
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")),
|
||||||
|
isError = true,
|
||||||
|
)
|
||||||
}
|
}
|
||||||
logger.trace { "Executing tool ${request.name} with input: ${request.arguments}" }
|
|
||||||
return tool.handler(request)
|
// Execute tool handler and catch any errors
|
||||||
|
return try {
|
||||||
|
logger.trace { "Executing tool ${request.name} with input: ${request.arguments}" }
|
||||||
|
tool.handler(request)
|
||||||
|
} catch (e: CancellationException) {
|
||||||
|
throw e
|
||||||
|
} catch (e: Exception) {
|
||||||
|
logger.error(e) { "Error executing tool ${request.name}" }
|
||||||
|
CallToolResult(
|
||||||
|
content = listOf(TextContent(text = "Error executing tool ${request.name}: ${e.message}")),
|
||||||
|
isError = true,
|
||||||
|
)
|
||||||
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
private suspend fun handleListPrompts(): ListPromptsResult {
|
private suspend fun handleListPrompts(): ListPromptsResult {
|
||||||
|
|
|
||||||
|
|
@ -21,7 +21,6 @@ import kotlinx.serialization.json.buildJsonArray
|
||||||
import kotlinx.serialization.json.buildJsonObject
|
import kotlinx.serialization.json.buildJsonObject
|
||||||
import kotlinx.serialization.json.put
|
import kotlinx.serialization.json.put
|
||||||
import org.junit.jupiter.api.Test
|
import org.junit.jupiter.api.Test
|
||||||
import org.junit.jupiter.api.assertThrows
|
|
||||||
import java.text.DecimalFormat
|
import java.text.DecimalFormat
|
||||||
import java.text.DecimalFormatSymbols
|
import java.text.DecimalFormatSymbols
|
||||||
import java.util.Locale
|
import java.util.Locale
|
||||||
|
|
@ -527,16 +526,18 @@ abstract class AbstractToolIntegrationTest : KotlinTestBase() {
|
||||||
"message" to "My exception message",
|
"message" to "My exception message",
|
||||||
)
|
)
|
||||||
|
|
||||||
val exception = assertThrows<IllegalStateException> {
|
val exceptionResult = client.callTool(errorToolName, exceptionArgs) as CallToolResultBase
|
||||||
runBlocking {
|
|
||||||
client.callTool(errorToolName, exceptionArgs)
|
|
||||||
}
|
|
||||||
}
|
|
||||||
|
|
||||||
val msg = exception.message ?: ""
|
assertTrue(exceptionResult.isError ?: false, "isError should be true for exception")
|
||||||
val expectedMessage = "JSONRPCError(code=InternalError, message=My exception message, data={})"
|
|
||||||
|
|
||||||
assertEquals(expectedMessage, msg, "Unexpected error message for exception")
|
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",
|
||||||
|
)
|
||||||
}
|
}
|
||||||
|
|
||||||
@Test
|
@Test
|
||||||
|
|
@ -770,15 +771,21 @@ abstract class AbstractToolIntegrationTest : KotlinTestBase() {
|
||||||
val nonExistentToolName = "non-existent-tool"
|
val nonExistentToolName = "non-existent-tool"
|
||||||
val arguments = mapOf("text" to "Test")
|
val arguments = mapOf("text" to "Test")
|
||||||
|
|
||||||
val exception = assertThrows<IllegalStateException> {
|
val result = runBlocking {
|
||||||
runBlocking {
|
client.callTool(nonExistentToolName, arguments)
|
||||||
client.callTool(nonExistentToolName, arguments)
|
|
||||||
}
|
|
||||||
}
|
}
|
||||||
|
|
||||||
val msg = exception.message ?: ""
|
assertNotNull(result, "Tool call result should not be null")
|
||||||
val expectedMessage = "JSONRPCError(code=InternalError, message=Tool not found: non-existent-tool, data={})"
|
val callResult = result as CallToolResult
|
||||||
|
assertTrue(callResult.isError ?: false, "isError should be true for non-existent tool")
|
||||||
|
|
||||||
assertEquals(expectedMessage, msg, "Unexpected error message for non-existent tool")
|
val textContent = callResult.content.firstOrNull { it is TextContent } as? TextContent
|
||||||
|
assertNotNull(textContent, "Error content should be present in the result")
|
||||||
|
|
||||||
|
val errorText = textContent.text ?: ""
|
||||||
|
assertTrue(
|
||||||
|
errorText.contains("non-existent-tool") && errorText.contains("not found"),
|
||||||
|
"Error message should indicate the tool was not found",
|
||||||
|
)
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
|
||||||
|
|
@ -17,7 +17,6 @@ import org.junit.jupiter.api.AfterEach
|
||||||
import org.junit.jupiter.api.BeforeEach
|
import org.junit.jupiter.api.BeforeEach
|
||||||
import org.junit.jupiter.api.Test
|
import org.junit.jupiter.api.Test
|
||||||
import org.junit.jupiter.api.Timeout
|
import org.junit.jupiter.api.Timeout
|
||||||
import org.junit.jupiter.api.assertThrows
|
|
||||||
import java.util.concurrent.TimeUnit
|
import java.util.concurrent.TimeUnit
|
||||||
import kotlin.test.assertEquals
|
import kotlin.test.assertEquals
|
||||||
import kotlin.test.assertNotNull
|
import kotlin.test.assertNotNull
|
||||||
|
|
@ -74,16 +73,19 @@ class KotlinClientTsServerEdgeCasesTestSse : TsTestBase() {
|
||||||
val nonExistentToolName = "non-existent-tool"
|
val nonExistentToolName = "non-existent-tool"
|
||||||
val arguments = mapOf("name" to "TestUser")
|
val arguments = mapOf("name" to "TestUser")
|
||||||
|
|
||||||
val exception = assertThrows<IllegalStateException> {
|
val result = client.callTool(nonExistentToolName, arguments)
|
||||||
client.callTool(nonExistentToolName, arguments)
|
assertNotNull(result, "Tool call result should not be null")
|
||||||
}
|
|
||||||
|
|
||||||
val expectedMessage =
|
val callResult = result as CallToolResult
|
||||||
"JSONRPCError(code=InvalidParams, message=MCP error -32602: Tool non-existent-tool not found, data={})"
|
assertTrue(callResult.isError ?: false, "isError should be true for non-existent tool")
|
||||||
assertEquals(
|
|
||||||
expectedMessage,
|
val textContent = callResult.content.firstOrNull { it is TextContent } as? TextContent
|
||||||
exception.message,
|
assertNotNull(textContent, "Error content should be present in the result")
|
||||||
"Unexpected error message for non-existent tool",
|
|
||||||
|
val errorText = textContent.text ?: ""
|
||||||
|
assertTrue(
|
||||||
|
errorText.contains("non-existent-tool") && errorText.contains("not found"),
|
||||||
|
"Error message should indicate the tool was not found",
|
||||||
)
|
)
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
@ -176,26 +178,20 @@ class KotlinClientTsServerEdgeCasesTestSse : TsTestBase() {
|
||||||
"name" to JsonObject(mapOf("nested" to JsonPrimitive("value"))),
|
"name" to JsonObject(mapOf("nested" to JsonPrimitive("value"))),
|
||||||
)
|
)
|
||||||
|
|
||||||
val exception = assertThrows<IllegalStateException> {
|
val result = client.callTool("greet", invalidArguments)
|
||||||
client.callTool("greet", invalidArguments)
|
assertNotNull(result, "Tool call result should not be null")
|
||||||
}
|
|
||||||
|
|
||||||
val msg = exception.message ?: ""
|
val callResult = result as CallToolResult
|
||||||
val expectedMessage = """
|
assertTrue(callResult.isError ?: false, "isError should be true for invalid arguments")
|
||||||
JSONRPCError(code=InvalidParams, message=MCP error -32602: Invalid arguments for tool greet: [
|
|
||||||
{
|
|
||||||
"code": "invalid_type",
|
|
||||||
"expected": "string",
|
|
||||||
"received": "object",
|
|
||||||
"path": [
|
|
||||||
"name"
|
|
||||||
],
|
|
||||||
"message": "Expected string, received object"
|
|
||||||
}
|
|
||||||
], data={})
|
|
||||||
""".trimIndent()
|
|
||||||
|
|
||||||
assertEquals(expectedMessage, msg, "Unexpected error message for invalid arguments")
|
val textContent = callResult.content.firstOrNull { it is TextContent } as? TextContent
|
||||||
|
assertNotNull(textContent, "Error content should be present in the result")
|
||||||
|
|
||||||
|
val errorText = textContent.text ?: ""
|
||||||
|
assertTrue(
|
||||||
|
errorText.contains("Invalid arguments") && errorText.contains("greet"),
|
||||||
|
"Error message should indicate invalid arguments for tool greet",
|
||||||
|
)
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|
|
||||||
|
|
@ -14,7 +14,6 @@ import kotlinx.serialization.json.JsonObject
|
||||||
import kotlinx.serialization.json.JsonPrimitive
|
import kotlinx.serialization.json.JsonPrimitive
|
||||||
import org.junit.jupiter.api.Test
|
import org.junit.jupiter.api.Test
|
||||||
import org.junit.jupiter.api.Timeout
|
import org.junit.jupiter.api.Timeout
|
||||||
import org.junit.jupiter.api.assertThrows
|
|
||||||
import java.util.concurrent.TimeUnit
|
import java.util.concurrent.TimeUnit
|
||||||
import kotlin.test.assertEquals
|
import kotlin.test.assertEquals
|
||||||
import kotlin.test.assertNotNull
|
import kotlin.test.assertNotNull
|
||||||
|
|
@ -31,16 +30,19 @@ class KotlinClientTsServerEdgeCasesTestStdio : TsTestBase() {
|
||||||
val nonExistentToolName = "non-existent-tool"
|
val nonExistentToolName = "non-existent-tool"
|
||||||
val arguments = mapOf("name" to "TestUser")
|
val arguments = mapOf("name" to "TestUser")
|
||||||
|
|
||||||
val exception = assertThrows<IllegalStateException> {
|
val result = client.callTool(nonExistentToolName, arguments)
|
||||||
client.callTool(nonExistentToolName, arguments)
|
assertNotNull(result, "Tool call result should not be null")
|
||||||
}
|
|
||||||
|
|
||||||
val expectedMessage =
|
val callResult = result as CallToolResult
|
||||||
"JSONRPCError(code=InvalidParams, message=MCP error -32602: Tool non-existent-tool not found, data={})"
|
assertTrue(callResult.isError ?: false, "isError should be true for non-existent tool")
|
||||||
assertEquals(
|
|
||||||
expectedMessage,
|
val textContent = callResult.content.firstOrNull { it is TextContent } as? TextContent
|
||||||
exception.message,
|
assertNotNull(textContent, "Error content should be present in the result")
|
||||||
"Unexpected error message for non-existent tool",
|
|
||||||
|
val errorText = textContent.text ?: ""
|
||||||
|
assertTrue(
|
||||||
|
errorText.contains("non-existent-tool") && errorText.contains("not found"),
|
||||||
|
"Error message should indicate the tool was not found",
|
||||||
)
|
)
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
@ -133,26 +135,20 @@ class KotlinClientTsServerEdgeCasesTestStdio : TsTestBase() {
|
||||||
"name" to JsonObject(mapOf("nested" to JsonPrimitive("value"))),
|
"name" to JsonObject(mapOf("nested" to JsonPrimitive("value"))),
|
||||||
)
|
)
|
||||||
|
|
||||||
val exception = assertThrows<IllegalStateException> {
|
val result = client.callTool("greet", invalidArguments)
|
||||||
client.callTool("greet", invalidArguments)
|
assertNotNull(result, "Tool call result should not be null")
|
||||||
}
|
|
||||||
|
|
||||||
val msg = exception.message ?: ""
|
val callResult = result as CallToolResult
|
||||||
val expectedMessage = """
|
assertTrue(callResult.isError ?: false, "isError should be true for invalid arguments")
|
||||||
JSONRPCError(code=InvalidParams, message=MCP error -32602: Invalid arguments for tool greet: [
|
|
||||||
{
|
|
||||||
"code": "invalid_type",
|
|
||||||
"expected": "string",
|
|
||||||
"received": "object",
|
|
||||||
"path": [
|
|
||||||
"name"
|
|
||||||
],
|
|
||||||
"message": "Expected string, received object"
|
|
||||||
}
|
|
||||||
], data={})
|
|
||||||
""".trimIndent()
|
|
||||||
|
|
||||||
assertEquals(expectedMessage, msg, "Unexpected error message for invalid arguments")
|
val textContent = callResult.content.firstOrNull { it is TextContent } as? TextContent
|
||||||
|
assertNotNull(textContent, "Error content should be present in the result")
|
||||||
|
|
||||||
|
val errorText = textContent.text ?: ""
|
||||||
|
assertTrue(
|
||||||
|
errorText.contains("Invalid arguments") && errorText.contains("greet"),
|
||||||
|
"Error message should indicate invalid arguments for tool greet",
|
||||||
|
)
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|
|
||||||
Loading…
Reference in a new issue