Skip to content

Commit 210a813

Browse files
tzolovtzolov
committed
fix: Skip structured output validation for error tool results
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. - 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. These tests use npx to download and run the MCP "everything" server locally. The first test execution will download the everything server scripts and cache them locally, which can take more than 15 seconds. Subsequent test runs will use the cached version and execute faster. Resolves #538 Related to #422 Signed-off-by: Christian Tzolov <christian.tzolov@broadcom.com>
1 parent f4380e7 commit 210a813

File tree

11 files changed

+353
-80
lines changed

11 files changed

+353
-80
lines changed

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

Lines changed: 73 additions & 13 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;
@@ -56,12 +45,23 @@
5645
import io.modelcontextprotocol.spec.McpSchema.Role;
5746
import io.modelcontextprotocol.spec.McpSchema.Root;
5847
import io.modelcontextprotocol.spec.McpSchema.ServerCapabilities;
48+
import io.modelcontextprotocol.spec.McpSchema.TextContent;
5949
import io.modelcontextprotocol.spec.McpSchema.Tool;
6050
import io.modelcontextprotocol.util.Utils;
6151
import net.javacrumbs.jsonunit.core.Option;
52+
import org.junit.jupiter.params.ParameterizedTest;
53+
import org.junit.jupiter.params.provider.ValueSource;
6254
import reactor.core.publisher.Mono;
6355
import reactor.test.StepVerifier;
6456

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

6767
protected ConcurrentHashMap<String, McpClient.SyncSpec> clientBuilders = new ConcurrentHashMap<>();
@@ -108,8 +108,8 @@ void testCreateMessageWithoutSamplingCapabilities(String clientType) {
108108
McpServerFeatures.AsyncToolSpecification tool = McpServerFeatures.AsyncToolSpecification.builder()
109109
.tool(Tool.builder().name("tool1").description("tool1 description").inputSchema(emptyJsonSchema).build())
110110
.callHandler((exchange, request) -> {
111-
exchange.createMessage(mock(McpSchema.CreateMessageRequest.class)).block();
112-
return Mono.just(mock(CallToolResult.class));
111+
return exchange.createMessage(mock(McpSchema.CreateMessageRequest.class))
112+
.then(Mono.just(mock(CallToolResult.class)));
113113
})
114114
.build();
115115

@@ -1434,6 +1434,66 @@ void testStructuredOutputValidationSuccess(String clientType) {
14341434

14351435
@ParameterizedTest(name = "{0} : {displayName} ")
14361436
@ValueSource(strings = { "httpclient", "webflux" })
1437+
void testStructuredOutputWithInHandlerError(String clientType) {
1438+
var clientBuilder = clientBuilders.get(clientType);
1439+
1440+
// Create a tool with output schema
1441+
Map<String, Object> outputSchema = Map.of(
1442+
"type", "object", "properties", Map.of("result", Map.of("type", "number"), "operation",
1443+
Map.of("type", "string"), "timestamp", Map.of("type", "string")),
1444+
"required", List.of("result", "operation"));
1445+
1446+
Tool calculatorTool = Tool.builder()
1447+
.name("calculator")
1448+
.description("Performs mathematical calculations")
1449+
.outputSchema(outputSchema)
1450+
.build();
1451+
1452+
// Handler that throws an exception to simulate an error
1453+
McpServerFeatures.SyncToolSpecification tool = McpServerFeatures.SyncToolSpecification.builder()
1454+
.tool(calculatorTool)
1455+
.callHandler((exchange, request) -> {
1456+
1457+
return CallToolResult.builder()
1458+
.isError(true)
1459+
.content(List.of(new TextContent("Error calling tool: Simulated in-handler error")))
1460+
.build();
1461+
})
1462+
.build();
1463+
1464+
var mcpServer = prepareSyncServerBuilder().serverInfo("test-server", "1.0.0")
1465+
.capabilities(ServerCapabilities.builder().tools(true).build())
1466+
.tools(tool)
1467+
.build();
1468+
1469+
try (var mcpClient = clientBuilder.build()) {
1470+
InitializeResult initResult = mcpClient.initialize();
1471+
assertThat(initResult).isNotNull();
1472+
1473+
// Verify tool is listed with output schema
1474+
var toolsList = mcpClient.listTools();
1475+
assertThat(toolsList.tools()).hasSize(1);
1476+
assertThat(toolsList.tools().get(0).name()).isEqualTo("calculator");
1477+
// Note: outputSchema might be null in sync server, but validation still works
1478+
1479+
// Call tool with valid structured output
1480+
CallToolResult response = mcpClient
1481+
.callTool(new McpSchema.CallToolRequest("calculator", Map.of("expression", "2 + 3")));
1482+
1483+
assertThat(response).isNotNull();
1484+
assertThat(response.isError()).isTrue();
1485+
assertThat(response.content()).isNotEmpty();
1486+
assertThat(response.content())
1487+
.containsExactly(new McpSchema.TextContent("Error calling tool: Simulated in-handler error"));
1488+
assertThat(response.structuredContent()).isNull();
1489+
}
1490+
finally {
1491+
mcpServer.closeGracefully();
1492+
}
1493+
}
1494+
1495+
@ParameterizedTest(name = "{0} : {displayName} ")
1496+
@ValueSource(strings = { "httpclient" })
14371497
void testStructuredOutputValidationFailure(String clientType) {
14381498

14391499
var clientBuilder = clientBuilders.get(clientType);

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

Lines changed: 67 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -4,12 +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.awaitility.Awaitility.await;
12-
137
import java.net.URI;
148
import java.net.http.HttpClient;
159
import java.net.http.HttpRequest;
@@ -20,9 +14,6 @@
2014
import java.util.concurrent.ConcurrentHashMap;
2115
import java.util.concurrent.atomic.AtomicReference;
2216

23-
import org.junit.jupiter.params.ParameterizedTest;
24-
import org.junit.jupiter.params.provider.ValueSource;
25-
2617
import io.modelcontextprotocol.client.McpClient;
2718
import io.modelcontextprotocol.server.McpServer.StatelessAsyncSpecification;
2819
import io.modelcontextprotocol.server.McpServer.StatelessSyncSpecification;
@@ -33,10 +24,19 @@
3324
import io.modelcontextprotocol.spec.McpSchema.CallToolResult;
3425
import io.modelcontextprotocol.spec.McpSchema.InitializeResult;
3526
import io.modelcontextprotocol.spec.McpSchema.ServerCapabilities;
27+
import io.modelcontextprotocol.spec.McpSchema.TextContent;
3628
import io.modelcontextprotocol.spec.McpSchema.Tool;
3729
import net.javacrumbs.jsonunit.core.Option;
30+
import org.junit.jupiter.params.ParameterizedTest;
31+
import org.junit.jupiter.params.provider.ValueSource;
3832
import reactor.core.publisher.Mono;
3933

34+
import static net.javacrumbs.jsonunit.assertj.JsonAssertions.assertThatJson;
35+
import static net.javacrumbs.jsonunit.assertj.JsonAssertions.json;
36+
import static org.assertj.core.api.Assertions.assertThat;
37+
import static org.assertj.core.api.Assertions.assertThatExceptionOfType;
38+
import static org.awaitility.Awaitility.await;
39+
4040
public abstract class AbstractStatelessIntegrationTests {
4141

4242
protected ConcurrentHashMap<String, McpClient.SyncSpec> clientBuilders = new ConcurrentHashMap<>();
@@ -350,6 +350,64 @@ void testStructuredOutputValidationSuccess(String clientType) {
350350
}
351351
}
352352

353+
@ParameterizedTest(name = "{0} : {displayName} ")
354+
@ValueSource(strings = { "httpclient", "webflux" })
355+
void testStructuredOutputWithInHandlerError(String clientType) {
356+
var clientBuilder = clientBuilders.get(clientType);
357+
358+
// Create a tool with output schema
359+
Map<String, Object> outputSchema = Map.of(
360+
"type", "object", "properties", Map.of("result", Map.of("type", "number"), "operation",
361+
Map.of("type", "string"), "timestamp", Map.of("type", "string")),
362+
"required", List.of("result", "operation"));
363+
364+
Tool calculatorTool = Tool.builder()
365+
.name("calculator")
366+
.description("Performs mathematical calculations")
367+
.outputSchema(outputSchema)
368+
.build();
369+
370+
// Handler that throws an exception to simulate an error
371+
McpStatelessServerFeatures.SyncToolSpecification tool = McpStatelessServerFeatures.SyncToolSpecification
372+
.builder()
373+
.tool(calculatorTool)
374+
.callHandler((exchange, request) -> CallToolResult.builder()
375+
.isError(true)
376+
.content(List.of(new TextContent("Error calling tool: Simulated in-handler error")))
377+
.build())
378+
.build();
379+
380+
var mcpServer = prepareSyncServerBuilder().serverInfo("test-server", "1.0.0")
381+
.capabilities(ServerCapabilities.builder().tools(true).build())
382+
.tools(tool)
383+
.build();
384+
385+
try (var mcpClient = clientBuilder.build()) {
386+
InitializeResult initResult = mcpClient.initialize();
387+
assertThat(initResult).isNotNull();
388+
389+
// Verify tool is listed with output schema
390+
var toolsList = mcpClient.listTools();
391+
assertThat(toolsList.tools()).hasSize(1);
392+
assertThat(toolsList.tools().get(0).name()).isEqualTo("calculator");
393+
// Note: outputSchema might be null in sync server, but validation still works
394+
395+
// Call tool with valid structured output
396+
CallToolResult response = mcpClient
397+
.callTool(new McpSchema.CallToolRequest("calculator", Map.of("expression", "2 + 3")));
398+
399+
assertThat(response).isNotNull();
400+
assertThat(response.isError()).isTrue();
401+
assertThat(response.content()).isNotEmpty();
402+
assertThat(response.content())
403+
.containsExactly(new McpSchema.TextContent("Error calling tool: Simulated in-handler error"));
404+
assertThat(response.structuredContent()).isNull();
405+
}
406+
finally {
407+
mcpServer.closeGracefully();
408+
}
409+
}
410+
353411
@ParameterizedTest(name = "{0} : {displayName} ")
354412
@ValueSource(strings = { "httpclient", "webflux" })
355413
void testStructuredOutputValidationFailure(String clientType) {

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

Lines changed: 10 additions & 3 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 (Boolean.TRUE.equals(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,11 @@ 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())));
511519
}
512520

513-
return toolSpecification.map(tool -> Mono.defer(() -> tool.callHandler().apply(exchange, callToolRequest)))
514-
.orElse(Mono.error(new McpError("Tool not found: " + callToolRequest.name())));
521+
return toolSpecification.get().callHandler().apply(exchange, callToolRequest);
515522
};
516523
}
517524

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

Lines changed: 10 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -11,7 +11,9 @@
1111
import io.modelcontextprotocol.spec.McpError;
1212
import io.modelcontextprotocol.spec.McpSchema;
1313
import io.modelcontextprotocol.spec.McpSchema.CallToolResult;
14+
import io.modelcontextprotocol.spec.McpSchema.JSONRPCResponse;
1415
import io.modelcontextprotocol.spec.McpSchema.ResourceTemplate;
16+
import io.modelcontextprotocol.spec.McpSchema.TextContent;
1517
import io.modelcontextprotocol.spec.McpSchema.Tool;
1618
import io.modelcontextprotocol.spec.McpStatelessServerTransport;
1719
import io.modelcontextprotocol.util.Assert;
@@ -249,6 +251,11 @@ public Mono<CallToolResult> apply(McpTransportContext transportContext, McpSchem
249251

250252
return this.delegateHandler.apply(transportContext, request).map(result -> {
251253

254+
if (Boolean.TRUE.equals(result.isError())) {
255+
// If the tool call resulted in an error, skip further validation
256+
return result;
257+
}
258+
252259
if (outputSchema == null) {
253260
if (result.structuredContent() != null) {
254261
logger.warn(
@@ -375,11 +382,11 @@ private McpStatelessRequestHandler<CallToolResult> toolsCallRequestHandler() {
375382
.findAny();
376383

377384
if (toolSpecification.isEmpty()) {
378-
return Mono.error(new McpError("Tool not found: " + callToolRequest.name()));
385+
return Mono.error(new McpError(new JSONRPCResponse.JSONRPCError(McpSchema.ErrorCodes.INVALID_PARAMS,
386+
"Unknown tool: invalid_tool_name", "Tool not found: " + callToolRequest.name())));
379387
}
380388

381-
return toolSpecification.map(tool -> tool.callHandler().apply(ctx, callToolRequest))
382-
.orElse(Mono.error(new McpError("Tool not found: " + callToolRequest.name())));
389+
return toolSpecification.get().callHandler().apply(ctx, callToolRequest);
383390
};
384391
}
385392

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

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -4,12 +4,12 @@
44

55
package io.modelcontextprotocol.util;
66

7-
import reactor.util.annotation.Nullable;
8-
97
import java.net.URI;
108
import java.util.Collection;
119
import java.util.Map;
1210

11+
import reactor.util.annotation.Nullable;
12+
1313
/**
1414
* Miscellaneous utility methods.
1515
*

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

Lines changed: 13 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -14,10 +14,17 @@
1414
/**
1515
* Tests for the {@link McpAsyncClient} with {@link StdioClientTransport}.
1616
*
17+
* <p>
18+
* These tests use npx to download and run the MCP "everything" server locally. The first
19+
* test execution will download the everything server scripts and cache them locally,
20+
* which can take more than 15 seconds. Subsequent test runs will use the cached version
21+
* and execute faster.
22+
*
1723
* @author Christian Tzolov
1824
* @author Dariusz Jędrzejczyk
1925
*/
20-
@Timeout(15) // Giving extra time beyond the client timeout
26+
@Timeout(25) // Giving extra time beyond the client timeout to account for initial server
27+
// download
2128
class StdioMcpAsyncClientTests extends AbstractMcpAsyncClientTests {
2229

2330
@Override
@@ -40,4 +47,9 @@ protected Duration getInitializationTimeout() {
4047
return Duration.ofSeconds(20);
4148
}
4249

50+
@Override
51+
protected Duration getRequestTimeout() {
52+
return Duration.ofSeconds(25);
53+
}
54+
4355
}

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

Lines changed: 13 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -22,10 +22,17 @@
2222
/**
2323
* Tests for the {@link McpSyncClient} with {@link StdioClientTransport}.
2424
*
25+
* <p>
26+
* These tests use npx to download and run the MCP "everything" server locally. The first
27+
* test execution will download the everything server scripts and cache them locally,
28+
* which can take more than 15 seconds. Subsequent test runs will use the cached version
29+
* and execute faster.
30+
*
2531
* @author Christian Tzolov
2632
* @author Dariusz Jędrzejczyk
2733
*/
28-
@Timeout(15) // Giving extra time beyond the client timeout
34+
@Timeout(25) // Giving extra time beyond the client timeout to account for initial server
35+
// download
2936
class StdioMcpSyncClientTests extends AbstractMcpSyncClientTests {
3037

3138
@Override
@@ -71,4 +78,9 @@ protected Duration getInitializationTimeout() {
7178
return Duration.ofSeconds(10);
7279
}
7380

81+
@Override
82+
protected Duration getRequestTimeout() {
83+
return Duration.ofSeconds(25);
84+
}
85+
7486
}

0 commit comments

Comments
 (0)