Skip to content

Commit 7e3f76e

Browse files
tzolovtzolov
committed
fix: align tool error handling with MCP specification
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 #538 Related to #422 Signed-off-by: Christian Tzolov <christian.tzolov@broadcom.com>
1 parent 7cf5907 commit 7e3f76e

7 files changed

Lines changed: 157 additions & 91 deletions

File tree

‎mcp-test/src/main/java/io/modelcontextprotocol/AbstractMcpClientServerIntegrationTests.java‎

Lines changed: 50 additions & 32 deletions
Original file line numberDiff line numberDiff line change
@@ -4,14 +4,6 @@
44

55
package io.modelcontextprotocol;
66

7-
import static net.javacrumbs.jsonunit.assertj.JsonAssertions.assertThatJson;
8-
import static net.javacrumbs.jsonunit.assertj.JsonAssertions.json;
9-
import static org.assertj.core.api.Assertions.assertThat;
10-
import static org.assertj.core.api.Assertions.assertThatExceptionOfType;
11-
import static org.assertj.core.api.Assertions.assertWith;
12-
import static org.awaitility.Awaitility.await;
13-
import static org.mockito.Mockito.mock;
14-
157
import java.net.URI;
168
import java.net.http.HttpClient;
179
import java.net.http.HttpRequest;
@@ -29,9 +21,6 @@
2921
import java.util.function.Function;
3022
import java.util.stream.Collectors;
3123

32-
import org.junit.jupiter.params.ParameterizedTest;
33-
import org.junit.jupiter.params.provider.ValueSource;
34-
3524
import io.modelcontextprotocol.client.McpClient;
3625
import io.modelcontextprotocol.common.McpTransportContext;
3726
import io.modelcontextprotocol.server.McpServer;
@@ -59,9 +48,18 @@
5948
import io.modelcontextprotocol.spec.McpSchema.Tool;
6049
import io.modelcontextprotocol.util.Utils;
6150
import net.javacrumbs.jsonunit.core.Option;
51+
import org.junit.jupiter.params.ParameterizedTest;
52+
import org.junit.jupiter.params.provider.ValueSource;
6253
import reactor.core.publisher.Mono;
6354
import reactor.test.StepVerifier;
6455

56+
import static net.javacrumbs.jsonunit.assertj.JsonAssertions.assertThatJson;
57+
import static net.javacrumbs.jsonunit.assertj.JsonAssertions.json;
58+
import static org.assertj.core.api.Assertions.assertThat;
59+
import static org.assertj.core.api.Assertions.assertWith;
60+
import static org.awaitility.Awaitility.await;
61+
import static org.mockito.Mockito.mock;
62+
6563
public abstract class AbstractMcpClientServerIntegrationTests {
6664

6765
protected ConcurrentHashMap<String, McpClient.SyncSpec> clientBuilders = new ConcurrentHashMap<>();
@@ -108,8 +106,8 @@ void testCreateMessageWithoutSamplingCapabilities(String clientType) {
108106
McpServerFeatures.AsyncToolSpecification tool = McpServerFeatures.AsyncToolSpecification.builder()
109107
.tool(Tool.builder().name("tool1").description("tool1 description").inputSchema(emptyJsonSchema).build())
110108
.callHandler((exchange, request) -> {
111-
exchange.createMessage(mock(McpSchema.CreateMessageRequest.class)).block();
112-
return Mono.just(mock(CallToolResult.class));
109+
return exchange.createMessage(mock(McpSchema.CreateMessageRequest.class))
110+
.then(Mono.just(mock(CallToolResult.class)));
113111
})
114112
.build();
115113

@@ -122,13 +120,15 @@ void testCreateMessageWithoutSamplingCapabilities(String clientType) {
122120

123121
assertThat(client.initialize()).isNotNull();
124122

125-
try {
126-
client.callTool(new McpSchema.CallToolRequest("tool1", Map.of()));
127-
}
128-
catch (McpError e) {
129-
assertThat(e).isInstanceOf(McpError.class)
130-
.hasMessage("Client must be configured with sampling capabilities");
131-
}
123+
McpSchema.CallToolResult callToolResult = client.callTool(new McpSchema.CallToolRequest("tool1", Map.of()));
124+
125+
// Tool errors should be reported within the result object, not as MCP
126+
// protocol-level errors. This allows the LLM to see and potentially
127+
// handle the error.
128+
assertThat(callToolResult).isNotNull();
129+
assertThat(callToolResult.isError()).isTrue();
130+
assertThat(callToolResult.content()).containsExactly(new McpSchema.TextContent(
131+
"Error calling tool: Client must be configured with sampling capabilities"));
132132
}
133133
finally {
134134
server.closeGracefully().block();
@@ -338,9 +338,16 @@ void testCreateMessageWithRequestTimeoutFail(String clientType) throws Interrupt
338338
InitializeResult initResult = mcpClient.initialize();
339339
assertThat(initResult).isNotNull();
340340

341-
assertThatExceptionOfType(McpError.class).isThrownBy(() -> {
342-
mcpClient.callTool(new McpSchema.CallToolRequest("tool1", Map.of()));
343-
}).withMessageContaining("1000ms");
341+
McpSchema.CallToolResult callToolResult = mcpClient
342+
.callTool(new McpSchema.CallToolRequest("tool1", Map.of()));
343+
344+
// Tool errors should be reported within the result object, not as MCP
345+
// protocol-level errors. This allows the LLM to see and potentially
346+
// handle the error.
347+
assertThat(callToolResult).isNotNull();
348+
assertThat(callToolResult.isError()).isTrue();
349+
assertThat(callToolResult.content()).containsExactly(new McpSchema.TextContent(
350+
"Error calling tool: Did not observe any item or terminal signal within 1000ms in 'source(MonoCreate)' (and no fallback has been configured)"));
344351
}
345352
finally {
346353
mcpServer.closeGracefully().block();
@@ -556,9 +563,16 @@ void testCreateElicitationWithRequestTimeoutFail(String clientType) {
556563
InitializeResult initResult = mcpClient.initialize();
557564
assertThat(initResult).isNotNull();
558565

559-
assertThatExceptionOfType(McpError.class).isThrownBy(() -> {
560-
mcpClient.callTool(new McpSchema.CallToolRequest("tool1", Map.of()));
561-
}).withMessageContaining("within 1000ms");
566+
McpSchema.CallToolResult callToolResult = mcpClient
567+
.callTool(new McpSchema.CallToolRequest("tool1", Map.of()));
568+
569+
// Tool errors should be reported within the result object, not as MCP
570+
// protocol-level errors. This allows the LLM to see and potentially
571+
// handle the error.
572+
assertThat(callToolResult).isNotNull();
573+
assertThat(callToolResult.isError()).isTrue();
574+
assertThat(callToolResult.content()).containsExactly(new McpSchema.TextContent(
575+
"Error calling tool: Did not observe any item or terminal signal within 1000ms in 'source(MonoCreate)' (and no fallback has been configured)"));
562576

563577
ElicitResult elicitResult = resultRef.get();
564578
assertThat(elicitResult).isNull();
@@ -842,12 +856,16 @@ void testThrowingToolCallIsCaughtBeforeTimeout(String clientType) {
842856
InitializeResult initResult = mcpClient.initialize();
843857
assertThat(initResult).isNotNull();
844858

845-
// We expect the tool call to fail immediately with the exception raised by
846-
// the offending tool
847-
// instead of getting back a timeout.
848-
assertThatExceptionOfType(McpError.class)
849-
.isThrownBy(() -> mcpClient.callTool(new McpSchema.CallToolRequest("tool1", Map.of())))
850-
.withMessageContaining("Timeout on blocking read");
859+
McpSchema.CallToolResult callToolResult = mcpClient
860+
.callTool(new McpSchema.CallToolRequest("tool1", Map.of()));
861+
862+
// Tool errors should be reported within the result object, not as MCP
863+
// protocol-level errors. This allows the LLM to see and potentially
864+
// handle the error.
865+
assertThat(callToolResult).isNotNull();
866+
assertThat(callToolResult.isError()).isTrue();
867+
assertThat(callToolResult.content()).containsExactly(new McpSchema.TextContent(
868+
"Error calling tool: Timeout on blocking read for 1000000000 NANOSECONDS"));
851869
}
852870
finally {
853871
mcpServer.closeGracefully();

‎mcp/src/main/java/io/modelcontextprotocol/server/McpAsyncServer.java‎

Lines changed: 23 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -29,10 +29,12 @@
2929
import io.modelcontextprotocol.spec.McpError;
3030
import io.modelcontextprotocol.spec.McpSchema;
3131
import io.modelcontextprotocol.spec.McpSchema.CallToolResult;
32+
import io.modelcontextprotocol.spec.McpSchema.JSONRPCResponse;
3233
import io.modelcontextprotocol.spec.McpSchema.LoggingLevel;
3334
import io.modelcontextprotocol.spec.McpSchema.LoggingMessageNotification;
3435
import io.modelcontextprotocol.spec.McpSchema.ResourceTemplate;
3536
import io.modelcontextprotocol.spec.McpSchema.SetLevelRequest;
37+
import io.modelcontextprotocol.spec.McpSchema.TextContent;
3638
import io.modelcontextprotocol.spec.McpSchema.Tool;
3739
import io.modelcontextprotocol.spec.McpServerSession;
3840
import io.modelcontextprotocol.spec.McpServerTransportProvider;
@@ -376,6 +378,11 @@ public Mono<CallToolResult> apply(McpAsyncServerExchange exchange, McpSchema.Cal
376378

377379
return this.delegateCallToolResult.apply(exchange, request).map(result -> {
378380

381+
if (result.isError() != null && result.isError()) {
382+
// If the tool call resulted in an error, skip further validation
383+
return result;
384+
}
385+
379386
if (outputSchema == null) {
380387
if (result.structuredContent() != null) {
381388
logger.warn(
@@ -507,11 +514,23 @@ private McpRequestHandler<CallToolResult> toolsCallRequestHandler() {
507514
.findAny();
508515

509516
if (toolSpecification.isEmpty()) {
510-
return Mono.error(new McpError("Tool not found: " + callToolRequest.name()));
517+
return Mono.error(new McpError(new JSONRPCResponse.JSONRPCError(McpSchema.ErrorCodes.INVALID_PARAMS,
518+
"Unknown tool: invalid_tool_name", "Tool not found: " + callToolRequest.name())));
519+
}
520+
else {
521+
return toolSpecification.get().callHandler().apply(exchange, callToolRequest).onErrorResume(error -> {
522+
logger.error("Error calling tool: {}", callToolRequest.name(), error);
523+
524+
// Tool errors should be reported within the result object, not as MCP
525+
// protocol-level errors. This allows the LLM to see and potentially
526+
// handle the error.
527+
return Mono.just(CallToolResult.builder()
528+
.isError(true)
529+
.content(List
530+
.of(new TextContent("Error calling tool: " + Utils.findRootCause(error).getMessage())))
531+
.build());
532+
});
511533
}
512-
513-
return toolSpecification.map(tool -> Mono.defer(() -> tool.callHandler().apply(exchange, callToolRequest)))
514-
.orElse(Mono.error(new McpError("Tool not found: " + callToolRequest.name())));
515534
};
516535
}
517536

‎mcp/src/main/java/io/modelcontextprotocol/server/McpStatelessAsyncServer.java‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -249,6 +249,11 @@ public Mono<CallToolResult> apply(McpTransportContext transportContext, McpSchem
249249

250250
return this.delegateHandler.apply(transportContext, request).map(result -> {
251251

252+
if (result.isError() != null && result.isError()) {
253+
// If the tool call resulted in an error, skip further validation
254+
return result;
255+
}
256+
252257
if (outputSchema == null) {
253258
if (result.structuredContent() != null) {
254259
logger.warn(

‎mcp/src/main/java/io/modelcontextprotocol/util/Utils.java‎

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@
99
import java.net.URI;
1010
import java.util.Collection;
1111
import java.util.Map;
12+
import java.util.Objects;
1213

1314
/**
1415
* Miscellaneous utility methods.
@@ -107,4 +108,13 @@ private static boolean isUnderBaseUri(URI baseUri, URI endpointUri) {
107108
return endpointPath.startsWith(basePath);
108109
}
109110

111+
public static Throwable findRootCause(Throwable throwable) {
112+
Objects.requireNonNull(throwable);
113+
Throwable rootCause = throwable;
114+
while (rootCause.getCause() != null && rootCause.getCause() != rootCause) {
115+
rootCause = rootCause.getCause();
116+
}
117+
return rootCause;
118+
}
119+
110120
}

‎mcp/src/test/java/io/modelcontextprotocol/server/AbstractMcpClientServerIntegrationTests.java‎

Lines changed: 51 additions & 33 deletions
Original file line numberDiff line numberDiff line change
@@ -4,15 +4,6 @@
44

55
package io.modelcontextprotocol.server;
66

7-
import static net.javacrumbs.jsonunit.assertj.JsonAssertions.assertThatJson;
8-
import static net.javacrumbs.jsonunit.assertj.JsonAssertions.json;
9-
import static org.assertj.core.api.Assertions.assertThat;
10-
import static org.assertj.core.api.Assertions.assertThatExceptionOfType;
11-
import static org.assertj.core.api.Assertions.assertWith;
12-
import static org.awaitility.Awaitility.await;
13-
import static org.mockito.Mockito.mock;
14-
15-
import io.modelcontextprotocol.common.McpTransportContext;
167
import java.net.URI;
178
import java.net.http.HttpClient;
189
import java.net.http.HttpRequest;
@@ -30,10 +21,8 @@
3021
import java.util.function.Function;
3122
import java.util.stream.Collectors;
3223

33-
import org.junit.jupiter.params.ParameterizedTest;
34-
import org.junit.jupiter.params.provider.ValueSource;
35-
3624
import io.modelcontextprotocol.client.McpClient;
25+
import io.modelcontextprotocol.common.McpTransportContext;
3726
import io.modelcontextprotocol.spec.McpError;
3827
import io.modelcontextprotocol.spec.McpSchema;
3928
import io.modelcontextprotocol.spec.McpSchema.CallToolResult;
@@ -55,9 +44,18 @@
5544
import io.modelcontextprotocol.spec.McpSchema.Tool;
5645
import io.modelcontextprotocol.util.Utils;
5746
import net.javacrumbs.jsonunit.core.Option;
47+
import org.junit.jupiter.params.ParameterizedTest;
48+
import org.junit.jupiter.params.provider.ValueSource;
5849
import reactor.core.publisher.Mono;
5950
import reactor.test.StepVerifier;
6051

52+
import static net.javacrumbs.jsonunit.assertj.JsonAssertions.assertThatJson;
53+
import static net.javacrumbs.jsonunit.assertj.JsonAssertions.json;
54+
import static org.assertj.core.api.Assertions.assertThat;
55+
import static org.assertj.core.api.Assertions.assertWith;
56+
import static org.awaitility.Awaitility.await;
57+
import static org.mockito.Mockito.mock;
58+
6159
public abstract class AbstractMcpClientServerIntegrationTests {
6260

6361
protected ConcurrentHashMap<String, McpClient.SyncSpec> clientBuilders = new ConcurrentHashMap<>();
@@ -104,8 +102,8 @@ void testCreateMessageWithoutSamplingCapabilities(String clientType) {
104102
McpServerFeatures.AsyncToolSpecification tool = McpServerFeatures.AsyncToolSpecification.builder()
105103
.tool(Tool.builder().name("tool1").description("tool1 description").inputSchema(emptyJsonSchema).build())
106104
.callHandler((exchange, request) -> {
107-
exchange.createMessage(mock(McpSchema.CreateMessageRequest.class)).block();
108-
return Mono.just(mock(CallToolResult.class));
105+
return exchange.createMessage(mock(McpSchema.CreateMessageRequest.class))
106+
.then(Mono.just(mock(CallToolResult.class)));
109107
})
110108
.build();
111109

@@ -118,13 +116,15 @@ void testCreateMessageWithoutSamplingCapabilities(String clientType) {
118116

119117
assertThat(client.initialize()).isNotNull();
120118

121-
try {
122-
client.callTool(new McpSchema.CallToolRequest("tool1", Map.of()));
123-
}
124-
catch (McpError e) {
125-
assertThat(e).isInstanceOf(McpError.class)
126-
.hasMessage("Client must be configured with sampling capabilities");
127-
}
119+
McpSchema.CallToolResult callToolResult = client.callTool(new McpSchema.CallToolRequest("tool1", Map.of()));
120+
121+
// Tool errors should be reported within the result object, not as MCP
122+
// protocol-level errors. This allows the LLM to see and potentially
123+
// handle the error.
124+
assertThat(callToolResult).isNotNull();
125+
assertThat(callToolResult.isError()).isTrue();
126+
assertThat(callToolResult.content()).containsExactly(new McpSchema.TextContent(
127+
"Error calling tool: Client must be configured with sampling capabilities"));
128128
}
129129
finally {
130130
server.closeGracefully().block();
@@ -334,9 +334,16 @@ void testCreateMessageWithRequestTimeoutFail(String clientType) throws Interrupt
334334
InitializeResult initResult = mcpClient.initialize();
335335
assertThat(initResult).isNotNull();
336336

337-
assertThatExceptionOfType(McpError.class).isThrownBy(() -> {
338-
mcpClient.callTool(new McpSchema.CallToolRequest("tool1", Map.of()));
339-
}).withMessageContaining("1000ms");
337+
McpSchema.CallToolResult callToolResult = mcpClient
338+
.callTool(new McpSchema.CallToolRequest("tool1", Map.of()));
339+
340+
// Tool errors should be reported within the result object, not as MCP
341+
// protocol-level errors. This allows the LLM to see and potentially
342+
// handle the error.
343+
assertThat(callToolResult).isNotNull();
344+
assertThat(callToolResult.isError()).isTrue();
345+
assertThat(callToolResult.content()).containsExactly(new McpSchema.TextContent(
346+
"Error calling tool: Did not observe any item or terminal signal within 1000ms in 'source(MonoCreate)' (and no fallback has been configured)"));
340347
}
341348
finally {
342349
mcpServer.closeGracefully().block();
@@ -552,9 +559,16 @@ void testCreateElicitationWithRequestTimeoutFail(String clientType) {
552559
InitializeResult initResult = mcpClient.initialize();
553560
assertThat(initResult).isNotNull();
554561

555-
assertThatExceptionOfType(McpError.class).isThrownBy(() -> {
556-
mcpClient.callTool(new McpSchema.CallToolRequest("tool1", Map.of()));
557-
}).withMessageContaining("within 1000ms");
562+
McpSchema.CallToolResult callToolResult = mcpClient
563+
.callTool(new McpSchema.CallToolRequest("tool1", Map.of()));
564+
565+
// Tool errors should be reported within the result object, not as MCP
566+
// protocol-level errors. This allows the LLM to see and potentially
567+
// handle the error.
568+
assertThat(callToolResult).isNotNull();
569+
assertThat(callToolResult.isError()).isTrue();
570+
assertThat(callToolResult.content()).containsExactly(new McpSchema.TextContent(
571+
"Error calling tool: Did not observe any item or terminal signal within 1000ms in 'source(MonoCreate)' (and no fallback has been configured)"));
558572

559573
ElicitResult elicitResult = resultRef.get();
560574
assertThat(elicitResult).isNull();
@@ -838,12 +852,16 @@ void testThrowingToolCallIsCaughtBeforeTimeout(String clientType) {
838852
InitializeResult initResult = mcpClient.initialize();
839853
assertThat(initResult).isNotNull();
840854

841-
// We expect the tool call to fail immediately with the exception raised by
842-
// the offending tool
843-
// instead of getting back a timeout.
844-
assertThatExceptionOfType(McpError.class)
845-
.isThrownBy(() -> mcpClient.callTool(new McpSchema.CallToolRequest("tool1", Map.of())))
846-
.withMessageContaining("Timeout on blocking read");
855+
McpSchema.CallToolResult callToolResult = mcpClient
856+
.callTool(new McpSchema.CallToolRequest("tool1", Map.of()));
857+
858+
// Tool errors should be reported within the result object, not as MCP
859+
// protocol-level errors. This allows the LLM to see and potentially
860+
// handle the error.
861+
assertThat(callToolResult).isNotNull();
862+
assertThat(callToolResult.isError()).isTrue();
863+
assertThat(callToolResult.content()).containsExactly(new McpSchema.TextContent(
864+
"Error calling tool: Timeout on blocking read for 1000000000 NANOSECONDS"));
847865
}
848866
finally {
849867
mcpServer.closeGracefully();

0 commit comments

Comments
 (0)