Skip to content

Commit f59dbaf

Browse files
tzolovtzolov
committed
fix: Skip structured output validation for error tool results
- 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 <christian.tzolov@broadcom.com>
1 parent 346d767 commit f59dbaf

File tree

8 files changed

+99
-207
lines changed

8 files changed

+99
-207
lines changed

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

Lines changed: 26 additions & 40 deletions
Original file line numberDiff line numberDiff line change
@@ -45,6 +45,7 @@
4545
import io.modelcontextprotocol.spec.McpSchema.Role;
4646
import io.modelcontextprotocol.spec.McpSchema.Root;
4747
import io.modelcontextprotocol.spec.McpSchema.ServerCapabilities;
48+
import io.modelcontextprotocol.spec.McpSchema.TextContent;
4849
import io.modelcontextprotocol.spec.McpSchema.Tool;
4950
import io.modelcontextprotocol.util.Utils;
5051
import net.javacrumbs.jsonunit.core.Option;
@@ -56,6 +57,7 @@
5657
import static net.javacrumbs.jsonunit.assertj.JsonAssertions.assertThatJson;
5758
import static net.javacrumbs.jsonunit.assertj.JsonAssertions.json;
5859
import static org.assertj.core.api.Assertions.assertThat;
60+
import static org.assertj.core.api.Assertions.assertThatExceptionOfType;
5961
import static org.assertj.core.api.Assertions.assertWith;
6062
import static org.awaitility.Awaitility.await;
6163
import static org.mockito.Mockito.mock;
@@ -120,15 +122,13 @@ void testCreateMessageWithoutSamplingCapabilities(String clientType) {
120122

121123
assertThat(client.initialize()).isNotNull();
122124

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"));
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+
}
132132
}
133133
finally {
134134
server.closeGracefully().block();
@@ -338,16 +338,9 @@ void testCreateMessageWithRequestTimeoutFail(String clientType) throws Interrupt
338338
InitializeResult initResult = mcpClient.initialize();
339339
assertThat(initResult).isNotNull();
340340

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)"));
341+
assertThatExceptionOfType(McpError.class).isThrownBy(() -> {
342+
mcpClient.callTool(new McpSchema.CallToolRequest("tool1", Map.of()));
343+
}).withMessageContaining("1000ms");
351344
}
352345
finally {
353346
mcpServer.closeGracefully().block();
@@ -563,16 +556,9 @@ void testCreateElicitationWithRequestTimeoutFail(String clientType) {
563556
InitializeResult initResult = mcpClient.initialize();
564557
assertThat(initResult).isNotNull();
565558

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)"));
559+
assertThatExceptionOfType(McpError.class).isThrownBy(() -> {
560+
mcpClient.callTool(new McpSchema.CallToolRequest("tool1", Map.of()));
561+
}).withMessageContaining("within 1000ms");
576562

577563
ElicitResult elicitResult = resultRef.get();
578564
assertThat(elicitResult).isNull();
@@ -856,16 +842,12 @@ void testThrowingToolCallIsCaughtBeforeTimeout(String clientType) {
856842
InitializeResult initResult = mcpClient.initialize();
857843
assertThat(initResult).isNotNull();
858844

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"));
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");
869851
}
870852
finally {
871853
mcpServer.closeGracefully();
@@ -1471,7 +1453,11 @@ void testStructuredOutputWithInHandlerError(String clientType) {
14711453
McpServerFeatures.SyncToolSpecification tool = McpServerFeatures.SyncToolSpecification.builder()
14721454
.tool(calculatorTool)
14731455
.callHandler((exchange, request) -> {
1474-
throw new RuntimeException("Simulated in-handler error");
1456+
1457+
return CallToolResult.builder()
1458+
.isError(true)
1459+
.content(List.of(new TextContent("Error calling tool: Simulated in-handler error")))
1460+
.build();
14751461
})
14761462
.build();
14771463

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

Lines changed: 13 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -19,10 +19,12 @@
1919
import io.modelcontextprotocol.server.McpServer.StatelessSyncSpecification;
2020
import io.modelcontextprotocol.server.McpStatelessServerFeatures;
2121
import io.modelcontextprotocol.server.McpStatelessSyncServer;
22+
import io.modelcontextprotocol.spec.McpError;
2223
import io.modelcontextprotocol.spec.McpSchema;
2324
import io.modelcontextprotocol.spec.McpSchema.CallToolResult;
2425
import io.modelcontextprotocol.spec.McpSchema.InitializeResult;
2526
import io.modelcontextprotocol.spec.McpSchema.ServerCapabilities;
27+
import io.modelcontextprotocol.spec.McpSchema.TextContent;
2628
import io.modelcontextprotocol.spec.McpSchema.Tool;
2729
import net.javacrumbs.jsonunit.core.Option;
2830
import org.junit.jupiter.params.ParameterizedTest;
@@ -32,6 +34,7 @@
3234
import static net.javacrumbs.jsonunit.assertj.JsonAssertions.assertThatJson;
3335
import static net.javacrumbs.jsonunit.assertj.JsonAssertions.json;
3436
import static org.assertj.core.api.Assertions.assertThat;
37+
import static org.assertj.core.api.Assertions.assertThatExceptionOfType;
3538
import static org.awaitility.Awaitility.await;
3639

3740
public abstract class AbstractStatelessIntegrationTests {
@@ -156,16 +159,12 @@ void testThrowingToolCallIsCaughtBeforeTimeout(String clientType) {
156159
InitializeResult initResult = mcpClient.initialize();
157160
assertThat(initResult).isNotNull();
158161

159-
McpSchema.CallToolResult callToolResult = mcpClient
160-
.callTool(new McpSchema.CallToolRequest("tool1", Map.of()));
161-
162-
// Tool errors should be reported within the result object, not as MCP
163-
// protocol-level errors. This allows the LLM to see and potentially
164-
// handle the error.
165-
assertThat(callToolResult).isNotNull();
166-
assertThat(callToolResult.isError()).isTrue();
167-
assertThat(callToolResult.content()).containsExactly(new McpSchema.TextContent(
168-
"Error calling tool: Timeout on blocking read for 1000000000 NANOSECONDS"));
162+
// We expect the tool call to fail immediately with the exception raised by
163+
// the offending tool
164+
// instead of getting back a timeout.
165+
assertThatExceptionOfType(McpError.class)
166+
.isThrownBy(() -> mcpClient.callTool(new McpSchema.CallToolRequest("tool1", Map.of())))
167+
.withMessageContaining("Timeout on blocking read");
169168
}
170169
finally {
171170
mcpServer.closeGracefully();
@@ -372,9 +371,10 @@ void testStructuredOutputWithInHandlerError(String clientType) {
372371
McpStatelessServerFeatures.SyncToolSpecification tool = McpStatelessServerFeatures.SyncToolSpecification
373372
.builder()
374373
.tool(calculatorTool)
375-
.callHandler((exchange, request) -> {
376-
throw new RuntimeException("Simulated in-handler error");
377-
})
374+
.callHandler((exchange, request) -> CallToolResult.builder()
375+
.isError(true)
376+
.content(List.of(new TextContent("Error calling tool: Simulated in-handler error")))
377+
.build())
378378
.build();
379379

380380
var mcpServer = prepareSyncServerBuilder().serverInfo("test-server", "1.0.0")

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

Lines changed: 2 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -517,20 +517,8 @@ private McpRequestHandler<CallToolResult> toolsCallRequestHandler() {
517517
return Mono.error(new McpError(new JSONRPCResponse.JSONRPCError(McpSchema.ErrorCodes.INVALID_PARAMS,
518518
"Unknown tool: invalid_tool_name", "Tool not found: " + callToolRequest.name())));
519519
}
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-
});
533-
}
520+
521+
return toolSpecification.get().callHandler().apply(exchange, callToolRequest);
534522
};
535523
}
536524

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

Lines changed: 1 addition & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -385,28 +385,8 @@ private McpStatelessRequestHandler<CallToolResult> toolsCallRequestHandler() {
385385
return Mono.error(new McpError(new JSONRPCResponse.JSONRPCError(McpSchema.ErrorCodes.INVALID_PARAMS,
386386
"Unknown tool: invalid_tool_name", "Tool not found: " + callToolRequest.name())));
387387
}
388-
else {
389-
return toolSpecification.get().callHandler().apply(ctx, callToolRequest).onErrorResume(error -> {
390-
logger.error("Error calling tool: {}", callToolRequest.name(), error);
391-
392-
// TODO: Should we handle the McpError+JsonRcpError specaially (e.g.
393-
// propagate)
394-
// or always return a CallToolResult with isError=true?
395-
if (error instanceof McpError mcpError && mcpError.getJsonRpcError() != null) {
396-
// If the error is already an MCP error, propagate it as is
397-
return Mono.error(mcpError);
398-
}
399388

400-
// Tool errors should be reported within the result object, not as MCP
401-
// protocol-level errors. This allows the LLM to see and potentially
402-
// handle the error.
403-
return Mono.just(CallToolResult.builder()
404-
.isError(true)
405-
.content(List
406-
.of(new TextContent("Error calling tool: " + Utils.findRootCause(error).getMessage())))
407-
.build());
408-
});
409-
}
389+
return toolSpecification.get().callHandler().apply(ctx, callToolRequest);
410390
};
411391
}
412392

‎mcp/src/test/java/io/modelcontextprotocol/client/StdioMcpAsyncClientTests.java‎

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -17,7 +17,7 @@
1717
* @author Christian Tzolov
1818
* @author Dariusz Jędrzejczyk
1919
*/
20-
@Timeout(15) // Giving extra time beyond the client timeout
20+
@Timeout(25) // Giving extra time beyond the client timeout
2121
class StdioMcpAsyncClientTests extends AbstractMcpAsyncClientTests {
2222

2323
@Override
@@ -40,4 +40,9 @@ protected Duration getInitializationTimeout() {
4040
return Duration.ofSeconds(20);
4141
}
4242

43+
@Override
44+
protected Duration getRequestTimeout() {
45+
return Duration.ofSeconds(25);
46+
}
47+
4348
}

‎mcp/src/test/java/io/modelcontextprotocol/client/StdioMcpSyncClientTests.java‎

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -25,7 +25,7 @@
2525
* @author Christian Tzolov
2626
* @author Dariusz Jędrzejczyk
2727
*/
28-
@Timeout(15) // Giving extra time beyond the client timeout
28+
@Timeout(25) // Giving extra time beyond the client timeout
2929
class StdioMcpSyncClientTests extends AbstractMcpSyncClientTests {
3030

3131
@Override
@@ -71,4 +71,9 @@ protected Duration getInitializationTimeout() {
7171
return Duration.ofSeconds(10);
7272
}
7373

74+
@Override
75+
protected Duration getRequestTimeout() {
76+
return Duration.ofSeconds(25);
77+
}
78+
7479
}

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

Lines changed: 26 additions & 43 deletions
Original file line numberDiff line numberDiff line change
@@ -41,6 +41,7 @@
4141
import io.modelcontextprotocol.spec.McpSchema.Role;
4242
import io.modelcontextprotocol.spec.McpSchema.Root;
4343
import io.modelcontextprotocol.spec.McpSchema.ServerCapabilities;
44+
import io.modelcontextprotocol.spec.McpSchema.TextContent;
4445
import io.modelcontextprotocol.spec.McpSchema.Tool;
4546
import io.modelcontextprotocol.util.Utils;
4647
import net.javacrumbs.jsonunit.core.Option;
@@ -52,6 +53,7 @@
5253
import static net.javacrumbs.jsonunit.assertj.JsonAssertions.assertThatJson;
5354
import static net.javacrumbs.jsonunit.assertj.JsonAssertions.json;
5455
import static org.assertj.core.api.Assertions.assertThat;
56+
import static org.assertj.core.api.Assertions.assertThatExceptionOfType;
5557
import static org.assertj.core.api.Assertions.assertWith;
5658
import static org.awaitility.Awaitility.await;
5759
import static org.mockito.Mockito.mock;
@@ -116,15 +118,13 @@ void testCreateMessageWithoutSamplingCapabilities(String clientType) {
116118

117119
assertThat(client.initialize()).isNotNull();
118120

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"));
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+
}
128128
}
129129
finally {
130130
server.closeGracefully().block();
@@ -334,16 +334,9 @@ void testCreateMessageWithRequestTimeoutFail(String clientType) throws Interrupt
334334
InitializeResult initResult = mcpClient.initialize();
335335
assertThat(initResult).isNotNull();
336336

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)"));
337+
assertThatExceptionOfType(McpError.class).isThrownBy(() -> {
338+
mcpClient.callTool(new McpSchema.CallToolRequest("tool1", Map.of()));
339+
}).withMessageContaining("1000ms");
347340
}
348341
finally {
349342
mcpServer.closeGracefully().block();
@@ -559,16 +552,9 @@ void testCreateElicitationWithRequestTimeoutFail(String clientType) {
559552
InitializeResult initResult = mcpClient.initialize();
560553
assertThat(initResult).isNotNull();
561554

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)"));
555+
assertThatExceptionOfType(McpError.class).isThrownBy(() -> {
556+
mcpClient.callTool(new McpSchema.CallToolRequest("tool1", Map.of()));
557+
}).withMessageContaining("within 1000ms");
572558

573559
ElicitResult elicitResult = resultRef.get();
574560
assertThat(elicitResult).isNull();
@@ -852,16 +838,12 @@ void testThrowingToolCallIsCaughtBeforeTimeout(String clientType) {
852838
InitializeResult initResult = mcpClient.initialize();
853839
assertThat(initResult).isNotNull();
854840

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"));
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");
865847
}
866848
finally {
867849
mcpServer.closeGracefully();
@@ -1463,12 +1445,13 @@ void testStructuredOutputWithInHandlerError(String clientType) {
14631445
.outputSchema(outputSchema)
14641446
.build();
14651447

1466-
// Handler that throws an exception to simulate an error
1448+
// Handler that returns an error result
14671449
McpServerFeatures.SyncToolSpecification tool = McpServerFeatures.SyncToolSpecification.builder()
14681450
.tool(calculatorTool)
1469-
.callHandler((exchange, request) -> {
1470-
throw new RuntimeException("Simulated in-handler error");
1471-
})
1451+
.callHandler((exchange, request) -> CallToolResult.builder()
1452+
.isError(true)
1453+
.content(List.of(new TextContent("Error calling tool: Simulated in-handler error")))
1454+
.build())
14721455
.build();
14731456

14741457
var mcpServer = prepareSyncServerBuilder().serverInfo("test-server", "1.0.0")

0 commit comments

Comments
 (0)