diff --git a/mcp-core/src/main/java/io/modelcontextprotocol/server/DefaultMcpStatelessServerHandler.java b/mcp-core/src/main/java/io/modelcontextprotocol/server/DefaultMcpStatelessServerHandler.java index 58c85bd3b..7be1bcfad 100644 --- a/mcp-core/src/main/java/io/modelcontextprotocol/server/DefaultMcpStatelessServerHandler.java +++ b/mcp-core/src/main/java/io/modelcontextprotocol/server/DefaultMcpStatelessServerHandler.java @@ -12,6 +12,7 @@ import reactor.core.publisher.Mono; import java.util.Map; +import java.util.Objects; class DefaultMcpStatelessServerHandler implements McpStatelessServerHandler { @@ -44,8 +45,11 @@ public Mono handleRequest(McpTransportContext transpo error = mcpError.getJsonRpcError(); } else { + // The cause chain may contain internal details: log it, but only send + // the message to the client + logger.warn("Error handling request {} [{}]", request.id(), request.method(), t); error = new McpSchema.JSONRPCResponse.JSONRPCError(McpSchema.ErrorCodes.INTERNAL_ERROR, - t.getMessage(), null); + Objects.requireNonNullElse(t.getMessage(), "Internal error"), null); } return Mono.just(new McpSchema.JSONRPCResponse(McpSchema.JSONRPC_VERSION, request.id(), null, error)); }); diff --git a/mcp-core/src/main/java/io/modelcontextprotocol/spec/McpClientSession.java b/mcp-core/src/main/java/io/modelcontextprotocol/spec/McpClientSession.java index 0ba7ab3b8..63732c6a8 100644 --- a/mcp-core/src/main/java/io/modelcontextprotocol/spec/McpClientSession.java +++ b/mcp-core/src/main/java/io/modelcontextprotocol/spec/McpClientSession.java @@ -14,6 +14,7 @@ import java.time.Duration; import java.util.Map; +import java.util.Objects; import java.util.UUID; import java.util.concurrent.ConcurrentHashMap; import java.util.concurrent.atomic.AtomicLong; @@ -167,11 +168,17 @@ else if (message instanceof McpSchema.JSONRPCRequest request) { logger.debug("Received request: {}", request); handleIncomingRequest(request).onErrorResume(error -> { - McpSchema.JSONRPCResponse.JSONRPCError jsonRpcError = (error instanceof McpError mcpError - && mcpError.getJsonRpcError() != null) ? mcpError.getJsonRpcError() - // TODO: add error message through the data field - : new McpSchema.JSONRPCResponse.JSONRPCError(McpSchema.ErrorCodes.INTERNAL_ERROR, - error.getMessage(), McpError.aggregateExceptionMessages(error)); + McpSchema.JSONRPCResponse.JSONRPCError jsonRpcError; + if (error instanceof McpError mcpError && mcpError.getJsonRpcError() != null) { + jsonRpcError = mcpError.getJsonRpcError(); + } + else { + // The cause chain may contain internal details: log it, but only send + // the message to the server + logger.warn("Error handling request {} [{}]", request.id(), request.method(), error); + jsonRpcError = new McpSchema.JSONRPCResponse.JSONRPCError(McpSchema.ErrorCodes.INTERNAL_ERROR, + Objects.requireNonNullElse(error.getMessage(), "Internal error"), null); + } var errorResponse = new McpSchema.JSONRPCResponse(McpSchema.JSONRPC_VERSION, request.id(), null, jsonRpcError); diff --git a/mcp-core/src/main/java/io/modelcontextprotocol/spec/McpServerSession.java b/mcp-core/src/main/java/io/modelcontextprotocol/spec/McpServerSession.java index 241f7d8b5..fc313bef2 100644 --- a/mcp-core/src/main/java/io/modelcontextprotocol/spec/McpServerSession.java +++ b/mcp-core/src/main/java/io/modelcontextprotocol/spec/McpServerSession.java @@ -6,6 +6,7 @@ import java.time.Duration; import java.util.Map; +import java.util.Objects; import java.util.concurrent.ConcurrentHashMap; import java.util.concurrent.atomic.AtomicInteger; import java.util.concurrent.atomic.AtomicLong; @@ -224,10 +225,7 @@ public Mono handle(McpSchema.JSONRPCMessage message) { else if (message instanceof McpSchema.JSONRPCRequest request) { logger.debug("Received request: {}", request); return handleIncomingRequest(request, transportContext).onErrorResume(error -> { - McpSchema.JSONRPCResponse.JSONRPCError jsonRpcError = (error instanceof McpError mcpError - && mcpError.getJsonRpcError() != null) ? mcpError.getJsonRpcError() - : new McpSchema.JSONRPCResponse.JSONRPCError(McpSchema.ErrorCodes.INTERNAL_ERROR, - error.getMessage(), McpError.aggregateExceptionMessages(error)); + McpSchema.JSONRPCResponse.JSONRPCError jsonRpcError = toJsonRpcError(request, error); var errorResponse = new McpSchema.JSONRPCResponse(McpSchema.JSONRPC_VERSION, request.id(), null, jsonRpcError); // TODO: Should the error go to SSE or back as POST return? @@ -285,18 +283,28 @@ private Mono handleIncomingRequest(McpSchema.JSONRPCR } return resultMono .map(result -> new McpSchema.JSONRPCResponse(McpSchema.JSONRPC_VERSION, request.id(), result, null)) - .onErrorResume(error -> { - McpSchema.JSONRPCResponse.JSONRPCError jsonRpcError = (error instanceof McpError mcpError - && mcpError.getJsonRpcError() != null) ? mcpError.getJsonRpcError() - // TODO: add error message through the data field - : new McpSchema.JSONRPCResponse.JSONRPCError(McpSchema.ErrorCodes.INTERNAL_ERROR, - error.getMessage(), McpError.aggregateExceptionMessages(error)); - return Mono.just( - new McpSchema.JSONRPCResponse(McpSchema.JSONRPC_VERSION, request.id(), null, jsonRpcError)); - }); + .onErrorResume(error -> Mono.just(new McpSchema.JSONRPCResponse(McpSchema.JSONRPC_VERSION, request.id(), + null, toJsonRpcError(request, error)))); }); } + /** + * Converts a request handler failure into the JSON-RPC error sent to the client. + * Errors other than {@link McpError} are logged, and only their message is sent: + * their cause chain may contain internal details that must not reach the client. + * @param request the request that failed + * @param error the failure + * @return the JSON-RPC error to send to the client + */ + private McpSchema.JSONRPCResponse.JSONRPCError toJsonRpcError(McpSchema.JSONRPCRequest request, Throwable error) { + if (error instanceof McpError mcpError && mcpError.getJsonRpcError() != null) { + return mcpError.getJsonRpcError(); + } + logger.warn("Error handling request {} [{}]", request.id(), request.method(), error); + return new McpSchema.JSONRPCResponse.JSONRPCError(McpSchema.ErrorCodes.INTERNAL_ERROR, + Objects.requireNonNullElse(error.getMessage(), "Internal error"), null); + } + /** * Handles an incoming JSON-RPC notification by routing it to the appropriate handler. * @param notification The incoming JSON-RPC notification diff --git a/mcp-core/src/main/java/io/modelcontextprotocol/spec/McpStreamableServerSession.java b/mcp-core/src/main/java/io/modelcontextprotocol/spec/McpStreamableServerSession.java index 95f8959f5..fe2b6d798 100644 --- a/mcp-core/src/main/java/io/modelcontextprotocol/spec/McpStreamableServerSession.java +++ b/mcp-core/src/main/java/io/modelcontextprotocol/spec/McpStreamableServerSession.java @@ -6,6 +6,7 @@ import java.time.Duration; import java.util.Map; +import java.util.Objects; import java.util.UUID; import java.util.concurrent.ConcurrentHashMap; import java.util.concurrent.atomic.AtomicLong; @@ -179,10 +180,18 @@ public Mono responseStream(McpSchema.JSONRPCRequest jsonrpcRequest, McpStr .map(result -> new McpSchema.JSONRPCResponse(McpSchema.JSONRPC_VERSION, jsonrpcRequest.id(), result, null)) .onErrorResume(e -> { - McpSchema.JSONRPCResponse.JSONRPCError jsonRpcError = (e instanceof McpError mcpError - && mcpError.getJsonRpcError() != null) ? mcpError.getJsonRpcError() - : new McpSchema.JSONRPCResponse.JSONRPCError(McpSchema.ErrorCodes.INTERNAL_ERROR, - e.getMessage(), McpError.aggregateExceptionMessages(e)); + McpSchema.JSONRPCResponse.JSONRPCError jsonRpcError; + if (e instanceof McpError mcpError && mcpError.getJsonRpcError() != null) { + jsonRpcError = mcpError.getJsonRpcError(); + } + else { + // The cause chain may contain internal details: log it, but only + // send + // the message to the client + logger.warn("Error handling request {} [{}]", jsonrpcRequest.id(), jsonrpcRequest.method(), e); + jsonRpcError = new McpSchema.JSONRPCResponse.JSONRPCError(McpSchema.ErrorCodes.INTERNAL_ERROR, + Objects.requireNonNullElse(e.getMessage(), "Internal error"), null); + } var errorResponse = new McpSchema.JSONRPCResponse(McpSchema.JSONRPC_VERSION, jsonrpcRequest.id(), null, jsonRpcError); diff --git a/mcp-core/src/test/java/io/modelcontextprotocol/server/DefaultMcpStatelessServerHandlerTests.java b/mcp-core/src/test/java/io/modelcontextprotocol/server/DefaultMcpStatelessServerHandlerTests.java index 267aca504..a351df345 100644 --- a/mcp-core/src/test/java/io/modelcontextprotocol/server/DefaultMcpStatelessServerHandlerTests.java +++ b/mcp-core/src/test/java/io/modelcontextprotocol/server/DefaultMcpStatelessServerHandlerTests.java @@ -7,9 +7,11 @@ import io.modelcontextprotocol.common.McpTransportContext; import io.modelcontextprotocol.spec.McpSchema; import org.junit.jupiter.api.Test; +import reactor.core.publisher.Mono; import reactor.test.StepVerifier; import java.util.Collections; +import java.util.Map; import static org.assertj.core.api.Assertions.assertThat; @@ -37,4 +39,42 @@ void testHandleRequestWithUnregisteredMethod() { }).verifyComplete(); } + @Test + void testHandleRequestWithFailingHandler() { + McpStatelessRequestHandler failingHandler = (transportContext, params) -> Mono + .error(new IllegalStateException("Tool failed", + new IllegalArgumentException("connect to db.internal:5432 failed"))); + DefaultMcpStatelessServerHandler handler = new DefaultMcpStatelessServerHandler( + Map.of("tools/call", failingHandler), Collections.emptyMap()); + + McpSchema.JSONRPCRequest request = new McpSchema.JSONRPCRequest(McpSchema.JSONRPC_VERSION, "tools/call", + "test-id-123", null); + + StepVerifier.create(handler.handleRequest(McpTransportContext.EMPTY, request)).assertNext(response -> { + assertThat(response.id()).isEqualTo("test-id-123"); + assertThat(response.result()).isNull(); + assertThat(response.error()).isNotNull(); + assertThat(response.error().code()).isEqualTo(McpSchema.ErrorCodes.INTERNAL_ERROR); + assertThat(response.error().message()).isEqualTo("Tool failed"); + assertThat(response.error().data()).isNull(); + }).verifyComplete(); + } + + @Test + void testHandleRequestWithFailingHandlerWithoutMessage() { + McpStatelessRequestHandler failingHandler = (transportContext, params) -> Mono + .error(new IllegalStateException()); + DefaultMcpStatelessServerHandler handler = new DefaultMcpStatelessServerHandler( + Map.of("tools/call", failingHandler), Collections.emptyMap()); + + McpSchema.JSONRPCRequest request = new McpSchema.JSONRPCRequest(McpSchema.JSONRPC_VERSION, "tools/call", + "test-id-123", null); + + StepVerifier.create(handler.handleRequest(McpTransportContext.EMPTY, request)).assertNext(response -> { + assertThat(response.error()).isNotNull(); + assertThat(response.error().code()).isEqualTo(McpSchema.ErrorCodes.INTERNAL_ERROR); + assertThat(response.error().message()).isEqualTo("Internal error"); + }).verifyComplete(); + } + } diff --git a/mcp-core/src/test/java/io/modelcontextprotocol/spec/McpClientSessionTests.java b/mcp-core/src/test/java/io/modelcontextprotocol/spec/McpClientSessionTests.java index 3de06f503..d059c8468 100644 --- a/mcp-core/src/test/java/io/modelcontextprotocol/spec/McpClientSessionTests.java +++ b/mcp-core/src/test/java/io/modelcontextprotocol/spec/McpClientSessionTests.java @@ -246,30 +246,23 @@ void testRequestHandlerThrowsGenericException() { "test-id", null); transport.simulateIncomingMessage(request); - // Verify: The response should contain INTERNAL_ERROR with aggregated exception - // messages in data field + // Verify: The response should contain INTERNAL_ERROR with the exception message McpSchema.JSONRPCMessage sentMessage = transport.getLastSentMessage(); assertThat(sentMessage).isInstanceOf(McpSchema.JSONRPCResponse.class); McpSchema.JSONRPCResponse response = (McpSchema.JSONRPCResponse) sentMessage; assertThat(response.error()).isNotNull(); assertThat(response.error().code()).isEqualTo(McpSchema.ErrorCodes.INTERNAL_ERROR); assertThat(response.error().message()).isEqualTo("Something went wrong"); - // Verify data field contains aggregated exception messages - assertThat(response.error().data()).isNotNull(); - assertThat(response.error().data().toString()).contains("RuntimeException"); - assertThat(response.error().data().toString()).contains("Something went wrong"); + assertThat(response.error().data()).isNull(); session.close(); } @Test - void testRequestHandlerThrowsExceptionWithCause() { - // Setup: Create a request handler that throws an exception with a cause chain - String testMethod = "test.chainedError"; - RuntimeException rootCause = new IllegalArgumentException("Root cause message"); - RuntimeException middleCause = new IllegalStateException("Middle cause message", rootCause); - RuntimeException topException = new RuntimeException("Top level message", middleCause); - McpClientSession.RequestHandler failingHandler = params -> Mono.error(topException); + void testRequestHandlerThrowsExceptionWithoutMessage() { + // Setup: Create a request handler that throws an exception with a null message + String testMethod = "test.noMessageError"; + McpClientSession.RequestHandler failingHandler = params -> Mono.error(new IllegalStateException()); var transport = new MockMcpClientTransport(); var session = new McpClientSession(TIMEOUT, transport, Map.of(testMethod, failingHandler), Map.of(), @@ -280,22 +273,13 @@ void testRequestHandlerThrowsExceptionWithCause() { "test-id", null); transport.simulateIncomingMessage(request); - // Verify: The response should contain INTERNAL_ERROR with full exception chain - // in data field + // Verify: An error response is still sent McpSchema.JSONRPCMessage sentMessage = transport.getLastSentMessage(); assertThat(sentMessage).isInstanceOf(McpSchema.JSONRPCResponse.class); McpSchema.JSONRPCResponse response = (McpSchema.JSONRPCResponse) sentMessage; assertThat(response.error()).isNotNull(); assertThat(response.error().code()).isEqualTo(McpSchema.ErrorCodes.INTERNAL_ERROR); - assertThat(response.error().message()).isEqualTo("Top level message"); - // Verify data field contains the full exception chain - String dataString = response.error().data().toString(); - assertThat(dataString).contains("RuntimeException"); - assertThat(dataString).contains("Top level message"); - assertThat(dataString).contains("IllegalStateException"); - assertThat(dataString).contains("Middle cause message"); - assertThat(dataString).contains("IllegalArgumentException"); - assertThat(dataString).contains("Root cause message"); + assertThat(response.error().message()).isEqualTo("Internal error"); session.close(); } diff --git a/mcp-test/src/main/java/io/modelcontextprotocol/AbstractMcpClientServerIntegrationTests.java b/mcp-test/src/main/java/io/modelcontextprotocol/AbstractMcpClientServerIntegrationTests.java index e320a168b..5b7d909b1 100644 --- a/mcp-test/src/main/java/io/modelcontextprotocol/AbstractMcpClientServerIntegrationTests.java +++ b/mcp-test/src/main/java/io/modelcontextprotocol/AbstractMcpClientServerIntegrationTests.java @@ -60,7 +60,9 @@ import static net.javacrumbs.jsonunit.assertj.JsonAssertions.json; import static org.assertj.core.api.Assertions.assertThat; import static org.assertj.core.api.Assertions.assertThatExceptionOfType; +import static org.assertj.core.api.Assertions.assertThatThrownBy; import static org.assertj.core.api.Assertions.assertWith; +import static org.assertj.core.api.InstanceOfAssertFactories.type; import static org.awaitility.Awaitility.await; import static org.mockito.Mockito.mock; @@ -576,6 +578,67 @@ void testCreateElicitationWithRequestTimeoutFail(String clientType) { } } + @ParameterizedTest(name = "{0} : {displayName} ") + @MethodSource("clientsForTesting") + void testToolErrorDoesNotSendCauseChain(String clientType) { + + var clientBuilder = clientBuilders.get(clientType); + + McpServerFeatures.AsyncToolSpecification tool = McpServerFeatures.AsyncToolSpecification.builder() + .tool(Tool.builder().name("tool1").description("tool1 description").inputSchema(EMPTY_JSON_SCHEMA).build()) + .callHandler((exchange, + request) -> Mono.error(new IllegalStateException("Tool failed", + new IllegalArgumentException("connect to db.internal:5432 failed")))) + .build(); + + var mcpServer = prepareAsyncServerBuilder().serverInfo("test-server", "1.0.0").tools(tool).build(); + + try (var mcpClient = clientBuilder.clientInfo(new McpSchema.Implementation("Sample client", "0.0.0")).build()) { + + assertThatThrownBy(() -> mcpClient.callTool(new McpSchema.CallToolRequest("tool1", Map.of()))) + .isInstanceOf(McpError.class) + .extracting("jsonRpcError") + .asInstanceOf(type(McpSchema.JSONRPCResponse.JSONRPCError.class)) + .satisfies(error -> { + assertThat(error.code()).isEqualTo(McpSchema.ErrorCodes.INTERNAL_ERROR); + assertThat(error.message()).isEqualTo("Tool failed"); + assertThat(error.data()).isNull(); + }); + } + finally { + mcpServer.closeGracefully().block(); + } + } + + @ParameterizedTest(name = "{0} : {displayName} ") + @MethodSource("clientsForTesting") + void testToolErrorWithoutMessage(String clientType) { + + var clientBuilder = clientBuilders.get(clientType); + + McpServerFeatures.AsyncToolSpecification tool = McpServerFeatures.AsyncToolSpecification.builder() + .tool(Tool.builder().name("tool1").description("tool1 description").inputSchema(EMPTY_JSON_SCHEMA).build()) + .callHandler((exchange, request) -> Mono.error(new IllegalStateException())) + .build(); + + var mcpServer = prepareAsyncServerBuilder().serverInfo("test-server", "1.0.0").tools(tool).build(); + + try (var mcpClient = clientBuilder.clientInfo(new McpSchema.Implementation("Sample client", "0.0.0")).build()) { + + assertThatThrownBy(() -> mcpClient.callTool(new McpSchema.CallToolRequest("tool1", Map.of()))) + .isInstanceOf(McpError.class) + .extracting("jsonRpcError") + .asInstanceOf(type(McpSchema.JSONRPCResponse.JSONRPCError.class)) + .satisfies(error -> { + assertThat(error.code()).isEqualTo(McpSchema.ErrorCodes.INTERNAL_ERROR); + assertThat(error.message()).isEqualTo("Internal error"); + }); + } + finally { + mcpServer.closeGracefully().block(); + } + } + // --------------------------------------- // Roots Tests // ---------------------------------------