diff --git a/mcp/common/src/main/java/org/springframework/ai/mcp/AsyncMcpToolCallback.java b/mcp/common/src/main/java/org/springframework/ai/mcp/AsyncMcpToolCallback.java index a7c349d531..34de4ab906 100644 --- a/mcp/common/src/main/java/org/springframework/ai/mcp/AsyncMcpToolCallback.java +++ b/mcp/common/src/main/java/org/springframework/ai/mcp/AsyncMcpToolCallback.java @@ -107,11 +107,19 @@ public String call(String toolCallInput) { @Override public String call(String toolCallInput, @Nullable ToolContext toolContext) { - // Handle the possible null parameter situation in streaming mode. + // Reject null/empty tool call input. This typically indicates a streaming tool + // call aggregation failure upstream. Previously, it was silently replaced with + // "{}" which let the MCP call proceed with no arguments, producing silent + // failures that caused the model to retry indefinitely. Throwing a + // ToolExecutionException ensures the standard error processor surfaces a clear + // message back to the model. if (!StringUtils.hasText(toolCallInput)) { - logger.warn("Tool call arguments are null or empty for MCP tool: {}. Using empty JSON object as default.", + logger.warn("Tool call arguments are null or empty for MCP tool: {}. Surfacing as a tool error response.", this.tool.name()); - toolCallInput = "{}"; + throw new ToolExecutionException(this.getToolDefinition(), + new IllegalArgumentException("Tool call arguments were null or empty; the MCP tool '" + + this.tool.name() + "' was not invoked. This usually indicates a malformed or " + + "incomplete tool invocation from the model.")); } Map arguments = ModelOptionsUtils.jsonToMap(toolCallInput); diff --git a/mcp/common/src/main/java/org/springframework/ai/mcp/SyncMcpToolCallback.java b/mcp/common/src/main/java/org/springframework/ai/mcp/SyncMcpToolCallback.java index bb6684c634..4cce3f579e 100644 --- a/mcp/common/src/main/java/org/springframework/ai/mcp/SyncMcpToolCallback.java +++ b/mcp/common/src/main/java/org/springframework/ai/mcp/SyncMcpToolCallback.java @@ -108,11 +108,19 @@ public String call(String toolCallInput) { @Override public String call(String toolCallInput, @Nullable ToolContext toolContext) { - // Handle the possible null parameter situation in streaming mode. + // Reject null/empty tool call input. This typically indicates a streaming tool + // call aggregation failure upstream. Previously, it was silently replaced with + // "{}" which let the MCP call proceed with no arguments, producing silent + // failures that caused the model to retry indefinitely. Throwing a + // ToolExecutionException ensures the standard error processor surfaces a clear + // message back to the model. if (!StringUtils.hasText(toolCallInput)) { - logger.warn("Tool call arguments are null or empty for MCP tool: {}. Using empty JSON object as default.", + logger.warn("Tool call arguments are null or empty for MCP tool: {}. Surfacing as a tool error response.", this.tool.name()); - toolCallInput = "{}"; + throw new ToolExecutionException(this.getToolDefinition(), + new IllegalArgumentException("Tool call arguments were null or empty; the MCP tool '" + + this.tool.name() + "' was not invoked. This usually indicates a malformed or " + + "incomplete tool invocation from the model.")); } Map arguments = ModelOptionsUtils.jsonToMap(toolCallInput); diff --git a/mcp/common/src/test/java/org/springframework/ai/mcp/AsyncMcpToolCallbackTest.java b/mcp/common/src/test/java/org/springframework/ai/mcp/AsyncMcpToolCallbackTest.java index b4f098e941..fede639490 100644 --- a/mcp/common/src/test/java/org/springframework/ai/mcp/AsyncMcpToolCallbackTest.java +++ b/mcp/common/src/test/java/org/springframework/ai/mcp/AsyncMcpToolCallbackTest.java @@ -37,6 +37,7 @@ import static org.mockito.ArgumentMatchers.any; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.verifyNoInteractions; import static org.mockito.Mockito.when; @ExtendWith(MockitoExtension.class) @@ -112,60 +113,54 @@ void callShouldSucceedWithValidInput() { // prefixed } + /** + * When streaming tool call aggregation fails, the callback can receive null input. + * Previously this was silently replaced with {@code "{}"} and the MCP tool was + * invoked — producing empty-looking results that caused the model to retry + * indefinitely. The fix should throw a {@link ToolExecutionException} so the standard + * error processor surfaces a clear error to the model instead. + * + *

+ * Loop safety: the callback is invoked exactly once per assertion. There is no + * recursion or retry loop. + */ @Test - void callShouldHandleNullInput() { + void callShouldThrowOnNullInput() { when(this.tool.name()).thenReturn("testTool"); - var callToolResult = McpSchema.CallToolResult.builder() - .addTextContent("Success with empty input") - .isError(false) - .build(); - when(this.mcpClient.callTool(any(McpSchema.CallToolRequest.class))).thenReturn(Mono.just(callToolResult)); - // Act var callback = AsyncMcpToolCallback.builder() .mcpClient(this.mcpClient) .tool(this.tool) .prefixedToolName("testTool") .build(); - String result = callback.call(null); - - // Assert - assertThat(result).contains("Success with empty input"); + assertThatThrownBy(() -> callback.call(null)).isInstanceOf(ToolExecutionException.class) + .cause() + .isInstanceOf(IllegalArgumentException.class) + .hasMessageContaining("null or empty"); - // Verify empty JSON object was used - ArgumentCaptor requestCaptor = ArgumentCaptor - .forClass(McpSchema.CallToolRequest.class); - verify(this.mcpClient).callTool(requestCaptor.capture()); - assertThat(requestCaptor.getValue().arguments()).isEmpty(); + verifyNoInteractions(this.mcpClient); } + /** + * Same as {@link #callShouldThrowOnNullInput()} but for the empty-string variant. + */ @Test - void callShouldHandleEmptyInput() { + void callShouldThrowOnEmptyInput() { when(this.tool.name()).thenReturn("testTool"); - var callToolResult = McpSchema.CallToolResult.builder() - .addTextContent("Success with empty input") - .isError(false) - .build(); - when(this.mcpClient.callTool(any(McpSchema.CallToolRequest.class))).thenReturn(Mono.just(callToolResult)); - // Act var callback = AsyncMcpToolCallback.builder() .mcpClient(this.mcpClient) .tool(this.tool) .prefixedToolName("testTool") .build(); - String result = callback.call(""); - - // Assert - assertThat(result).contains("Success with empty input"); + assertThatThrownBy(() -> callback.call("")).isInstanceOf(ToolExecutionException.class) + .cause() + .isInstanceOf(IllegalArgumentException.class) + .hasMessageContaining("null or empty"); - // Verify empty JSON object was used - ArgumentCaptor requestCaptor = ArgumentCaptor - .forClass(McpSchema.CallToolRequest.class); - verify(this.mcpClient).callTool(requestCaptor.capture()); - assertThat(requestCaptor.getValue().arguments()).isEmpty(); + verifyNoInteractions(this.mcpClient); } @Test diff --git a/mcp/common/src/test/java/org/springframework/ai/mcp/SyncMcpToolCallbackTests.java b/mcp/common/src/test/java/org/springframework/ai/mcp/SyncMcpToolCallbackTests.java index e8f49cae1b..d36c891521 100644 --- a/mcp/common/src/test/java/org/springframework/ai/mcp/SyncMcpToolCallbackTests.java +++ b/mcp/common/src/test/java/org/springframework/ai/mcp/SyncMcpToolCallbackTests.java @@ -37,6 +37,7 @@ import static org.assertj.core.api.Assertions.assertThatThrownBy; import static org.mockito.ArgumentMatchers.any; import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.verifyNoInteractions; import static org.mockito.Mockito.when; @ExtendWith(MockitoExtension.class) @@ -116,12 +117,20 @@ void callShouldHandleToolContext() { assertThat(response).isNotNull(); } + /** + * When streaming tool call aggregation fails, the callback can receive null or empty + * input. Previously this was silently replaced with {@code "{}"} and the MCP tool was + * invoked — producing empty-looking results that caused the model to retry + * indefinitely. The fix should throw a {@link ToolExecutionException} so the standard + * error processor surfaces a clear error to the model instead. + * + *

+ * Loop safety: each assertion invokes the callback exactly once and verifies + * that the exception is thrown. There is no recursion or retry loop in this test. + */ @Test - void callShouldHandleNullOrEmptyInput() { + void callShouldThrowOnNullOrEmptyInput() { when(this.tool.name()).thenReturn("testTool"); - CallToolResult callResult = mock(CallToolResult.class); - when(callResult.content()).thenReturn(List.of()); - when(this.mcpClient.callTool(any(CallToolRequest.class))).thenReturn(callResult); SyncMcpToolCallback callback = SyncMcpToolCallback.builder() .mcpClient(this.mcpClient) @@ -129,17 +138,23 @@ void callShouldHandleNullOrEmptyInput() { .prefixedToolName("testClient_testTool") .build(); - // Test with null input - String responseNull = callback.call(null); - assertThat(responseNull).isEqualTo("[]"); + assertThatThrownBy(() -> callback.call(null)).isInstanceOf(ToolExecutionException.class) + .cause() + .isInstanceOf(IllegalArgumentException.class) + .hasMessageContaining("null or empty"); + + assertThatThrownBy(() -> callback.call("")).isInstanceOf(ToolExecutionException.class) + .cause() + .isInstanceOf(IllegalArgumentException.class) + .hasMessageContaining("null or empty"); - // Test with empty string input - String responseEmpty = callback.call(""); - assertThat(responseEmpty).isEqualTo("[]"); + assertThatThrownBy(() -> callback.call(" ")).isInstanceOf(ToolExecutionException.class) + .cause() + .isInstanceOf(IllegalArgumentException.class) + .hasMessageContaining("null or empty"); - // Test with whitespace-only input - String responseWhitespace = callback.call(" "); - assertThat(responseWhitespace).isEqualTo("[]"); + // Critical: the underlying MCP client must NOT have been invoked. + verifyNoInteractions(this.mcpClient); } @Test diff --git a/spring-ai-model/src/main/java/org/springframework/ai/model/tool/DefaultToolCallingManager.java b/spring-ai-model/src/main/java/org/springframework/ai/model/tool/DefaultToolCallingManager.java index bff2c9c503..45df900556 100644 --- a/spring-ai-model/src/main/java/org/springframework/ai/model/tool/DefaultToolCallingManager.java +++ b/spring-ai-model/src/main/java/org/springframework/ai/model/tool/DefaultToolCallingManager.java @@ -185,17 +185,6 @@ private InternalToolExecutionResult executeToolCall(Prompt prompt, AssistantMess String toolName = toolCall.name(); String toolInputArguments = toolCall.arguments(); - // Handle the possible null parameter situation in streaming mode. - final String finalToolInputArguments; - if (!StringUtils.hasText(toolInputArguments)) { - logger.warn("Tool call arguments are null or empty for tool: {}. Using empty JSON object as default.", - toolName); - finalToolInputArguments = "{}"; - } - else { - finalToolInputArguments = toolInputArguments; - } - ToolCallback toolCallback = toolCallbacks.stream() .filter(tool -> toolName.equals(tool.getToolDefinition().name())) .findFirst() @@ -213,6 +202,31 @@ private InternalToolExecutionResult executeToolCall(Prompt prompt, AssistantMess returnDirect = returnDirect && toolCallback.getToolMetadata().returnDirect(); } + // Null or empty tool call arguments typically indicate that streaming + // tool-call JSON aggregation failed upstream. Previously these cases + // were silently replaced with "{}", which caused tools with required + // parameters to be invoked with null values, producing empty-looking + // responses that drove the model to retry the same call indefinitely. + // Instead, surface a clear error to the model via the standard + // ToolExecutionExceptionProcessor so it can adjust its approach rather + // than silently loop. + if (!StringUtils.hasText(toolInputArguments)) { + logger.warn( + "Tool call arguments are null or empty for tool: {}. This typically indicates incomplete " + + "tool call aggregation in streaming mode. Surfacing as a tool error response.", + toolName); + ToolExecutionException ex = new ToolExecutionException(toolCallback.getToolDefinition(), + new IllegalArgumentException("Tool call arguments were null or empty; the tool was not " + + "invoked. This usually indicates a malformed or incomplete tool invocation " + + "from the model (for example, truncated or unparseable streaming arguments).")); + String toolCallResult = this.toolExecutionExceptionProcessor.process(ex); + toolResponses.add(new ToolResponseMessage.ToolResponse(toolCall.id(), toolName, + toolCallResult != null ? toolCallResult : "")); + continue; + } + + final String finalToolInputArguments = toolInputArguments; + ToolCallingObservationContext observationContext = ToolCallingObservationContext.builder() .toolDefinition(toolCallback.getToolDefinition()) .toolMetadata(toolCallback.getToolMetadata()) diff --git a/spring-ai-model/src/main/java/org/springframework/ai/tool/method/MethodToolCallback.java b/spring-ai-model/src/main/java/org/springframework/ai/tool/method/MethodToolCallback.java index 6d79b01d54..db17e210d4 100644 --- a/spring-ai-model/src/main/java/org/springframework/ai/tool/method/MethodToolCallback.java +++ b/spring-ai-model/src/main/java/org/springframework/ai/tool/method/MethodToolCallback.java @@ -19,6 +19,7 @@ import java.lang.reflect.InvocationTargetException; import java.lang.reflect.Method; import java.lang.reflect.Modifier; +import java.lang.reflect.Parameter; import java.lang.reflect.Type; import java.util.Map; import java.util.stream.Stream; @@ -31,6 +32,7 @@ import org.springframework.ai.chat.model.ToolContext; import org.springframework.ai.tool.ToolCallback; +import org.springframework.ai.tool.annotation.ToolParam; import org.springframework.ai.tool.definition.ToolDefinition; import org.springframework.ai.tool.execution.DefaultToolCallResultConverter; import org.springframework.ai.tool.execution.ToolCallResultConverter; @@ -144,10 +146,33 @@ private Object[] buildMethodArguments(Map toolInputArguments, @N return toolContext; } Object rawArgument = toolInputArguments.get(parameter.getName()); + if (rawArgument == null && isRequired(parameter)) { + // A required parameter is missing from the tool input. Previously, + // null was silently passed to the method, which often produced a + // valid-looking empty response and led the model to retry the same + // call indefinitely. Surfacing this as a ToolExecutionException + // causes the standard error processor to send a clear "missing + // required parameter" message back to the model so it can correct + // its arguments or abandon the call. + logger.warn("Required parameter '{}' is missing for tool '{}'. Surfacing as a tool error response.", + parameter.getName(), this.toolDefinition.name()); + throw new ToolExecutionException(this.getToolDefinition(), + new IllegalArgumentException("Missing required parameter '" + parameter.getName() + + "' for tool '" + this.toolDefinition.name() + + "'. The tool was not invoked. Retry the tool call with the required argument, " + + "or choose a different approach.")); + } return buildTypedArgument(rawArgument, parameter.getParameterizedType()); }).toArray(); } + private static boolean isRequired(Parameter parameter) { + // Default is required = true, both when the annotation is absent and when + // it is present without explicitly setting required = false. + ToolParam toolParam = parameter.getAnnotation(ToolParam.class); + return toolParam == null || toolParam.required(); + } + private @Nullable Object buildTypedArgument(@Nullable Object value, Type type) { if (value == null) { return null; diff --git a/spring-ai-model/src/test/java/org/springframework/ai/model/tool/DefaultToolCallingManagerTest.java b/spring-ai-model/src/test/java/org/springframework/ai/model/tool/DefaultToolCallingManagerTest.java index c23cbce484..0ea4513513 100644 --- a/spring-ai-model/src/test/java/org/springframework/ai/model/tool/DefaultToolCallingManagerTest.java +++ b/spring-ai-model/src/test/java/org/springframework/ai/model/tool/DefaultToolCallingManagerTest.java @@ -18,11 +18,13 @@ import java.util.List; import java.util.Map; +import java.util.concurrent.atomic.AtomicBoolean; import io.micrometer.observation.ObservationRegistry; import org.junit.jupiter.api.Test; import org.springframework.ai.chat.messages.AssistantMessage; +import org.springframework.ai.chat.messages.ToolResponseMessage; import org.springframework.ai.chat.messages.UserMessage; import org.springframework.ai.chat.model.ChatResponse; import org.springframework.ai.chat.model.Generation; @@ -41,9 +43,18 @@ */ class DefaultToolCallingManagerTest { + /** + * When streaming tool-call aggregation fails, {@code toolCall.arguments()} can arrive + * as {@code null}. Previously, this was silently replaced with {@code "{}"} and the + * tool was invoked with null parameters — producing empty-looking results that caused + * the model to retry indefinitely (burning tokens in a tight loop). The fix should + * short-circuit the tool call and return a clear error via the + * {@link org.springframework.ai.tool.execution.ToolExecutionExceptionProcessor} so + * the model sees a tool response explaining what went wrong. + */ @Test - void shouldHandleNullArgumentsInStreamMode() { - // Create a mock tool callback + void shouldReturnErrorResponseAndSkipToolWhenArgumentsAreNull() { + AtomicBoolean toolWasCalled = new AtomicBoolean(false); ToolCallback mockToolCallback = new ToolCallback() { @Override public ToolDefinition getToolDefinition() { @@ -61,17 +72,13 @@ public ToolMetadata getToolMetadata() { @Override public String call(String toolInput) { - // Verify the input is not null or empty - assertThat(toolInput).isNotNull(); - assertThat(toolInput).isNotEmpty(); + toolWasCalled.set(true); return "{\"result\": \"success\"}"; } }; - // Create a ToolCall with empty parameters AssistantMessage.ToolCall toolCall = new AssistantMessage.ToolCall("1", "function", "testTool", null); - // Create a ChatResponse AssistantMessage assistantMessage = AssistantMessage.builder() .content("") .properties(Map.of()) @@ -80,27 +87,39 @@ public String call(String toolInput) { Generation generation = new Generation(assistantMessage); ChatResponse chatResponse = new ChatResponse(List.of(generation)); - // Create a Prompt with tool callbacks Prompt prompt = new Prompt(List.of(new UserMessage("test"))); - // Mock the tool callbacks resolution by creating a custom ToolCallbackResolver DefaultToolCallingManager managerWithCallback = DefaultToolCallingManager.builder() .observationRegistry(ObservationRegistry.NOOP) - .toolCallbackResolver(toolName -> { - if ("testTool".equals(toolName)) { - return mockToolCallback; - } - return null; - }) + .toolCallbackResolver(toolName -> "testTool".equals(toolName) ? mockToolCallback : null) .build(); - // Verify that no exception is thrown - assertThatNoException().isThrownBy(() -> managerWithCallback.executeToolCalls(prompt, chatResponse)); + ToolExecutionResult result = managerWithCallback.executeToolCalls(prompt, chatResponse); + + // The tool callback must NOT have been invoked with null/fabricated arguments. + assertThat(toolWasCalled).isFalse(); + + // A single tool response should be produced with the error from the exception + // processor. + List conversation = result.conversationHistory(); + ToolResponseMessage lastMessage = (ToolResponseMessage) conversation.get(conversation.size() - 1); + assertThat(lastMessage.getResponses()).hasSize(1); + ToolResponseMessage.ToolResponse toolResponse = lastMessage.getResponses().get(0); + assertThat(toolResponse.id()).isEqualTo("1"); + assertThat(toolResponse.name()).isEqualTo("testTool"); + // Default processor returns a non-empty description of the error. + assertThat(toolResponse.responseData()).isNotEmpty(); + assertThat(toolResponse.responseData()).containsIgnoringCase("null or empty"); } + /** + * Same as {@link #shouldReturnErrorResponseAndSkipToolWhenArgumentsAreNull()} but for + * the empty-string variant — another way streaming aggregation can leave the tool + * arguments unusable. + */ @Test - void shouldHandleEmptyArgumentsInStreamMode() { - // Create a mock tool callback + void shouldReturnErrorResponseAndSkipToolWhenArgumentsAreEmpty() { + AtomicBoolean toolWasCalled = new AtomicBoolean(false); ToolCallback mockToolCallback = new ToolCallback() { @Override public ToolDefinition getToolDefinition() { @@ -118,17 +137,13 @@ public ToolMetadata getToolMetadata() { @Override public String call(String toolInput) { - // Verify the input is not null or empty - assertThat(toolInput).isNotNull(); - assertThat(toolInput).isNotEmpty(); + toolWasCalled.set(true); return "{\"result\": \"success\"}"; } }; - // Create a ToolCall with empty parameters AssistantMessage.ToolCall toolCall = new AssistantMessage.ToolCall("1", "function", "testTool", ""); - // Create a ChatResponse AssistantMessage assistantMessage = AssistantMessage.builder() .content("") .properties(Map.of()) @@ -137,22 +152,121 @@ public String call(String toolInput) { Generation generation = new Generation(assistantMessage); ChatResponse chatResponse = new ChatResponse(List.of(generation)); - // Create a Prompt with tool callbacks Prompt prompt = new Prompt(List.of(new UserMessage("test"))); - // Mock the tool callbacks resolution by creating a custom ToolCallbackResolver DefaultToolCallingManager managerWithCallback = DefaultToolCallingManager.builder() + .observationRegistry(ObservationRegistry.NOOP) + .toolCallbackResolver(toolName -> "testTool".equals(toolName) ? mockToolCallback : null) + .build(); + + ToolExecutionResult result = managerWithCallback.executeToolCalls(prompt, chatResponse); + + assertThat(toolWasCalled).isFalse(); + + List conversation = result.conversationHistory(); + ToolResponseMessage lastMessage = (ToolResponseMessage) conversation.get(conversation.size() - 1); + assertThat(lastMessage.getResponses()).hasSize(1); + ToolResponseMessage.ToolResponse toolResponse = lastMessage.getResponses().get(0); + assertThat(toolResponse.name()).isEqualTo("testTool"); + assertThat(toolResponse.responseData()).containsIgnoringCase("null or empty"); + } + + /** + * When a response contains a mix of well-formed and malformed tool calls, the + * well-formed ones must still execute successfully, and the malformed one must + * produce an error response rather than poisoning the whole batch. + */ + @Test + void shouldExecuteValidToolsWhileReturningErrorForMalformedTool() { + AtomicBoolean validToolCalled = new AtomicBoolean(false); + ToolCallback validToolCallback = new ToolCallback() { + @Override + public ToolDefinition getToolDefinition() { + return DefaultToolDefinition.builder() + .name("validTool") + .description("A valid tool") + .inputSchema("{\"type\": \"object\", \"properties\": {\"x\": {\"type\": \"string\"}}}") + .build(); + } + + @Override + public ToolMetadata getToolMetadata() { + return ToolMetadata.builder().build(); + } + + @Override + public String call(String toolInput) { + validToolCalled.set(true); + return "{\"ok\": true}"; + } + }; + + AtomicBoolean invalidToolCalled = new AtomicBoolean(false); + ToolCallback invalidToolCallback = new ToolCallback() { + @Override + public ToolDefinition getToolDefinition() { + return DefaultToolDefinition.builder() + .name("invalidTool") + .description("A tool that receives bad arguments") + .inputSchema("{}") + .build(); + } + + @Override + public ToolMetadata getToolMetadata() { + return ToolMetadata.builder().build(); + } + + @Override + public String call(String toolInput) { + invalidToolCalled.set(true); + return "{\"ok\": true}"; + } + }; + + AssistantMessage.ToolCall validCall = new AssistantMessage.ToolCall("1", "function", "validTool", + "{\"x\": \"hello\"}"); + AssistantMessage.ToolCall invalidCall = new AssistantMessage.ToolCall("2", "function", "invalidTool", null); + + AssistantMessage assistantMessage = AssistantMessage.builder() + .content("") + .properties(Map.of()) + .toolCalls(List.of(validCall, invalidCall)) + .build(); + Generation generation = new Generation(assistantMessage); + ChatResponse chatResponse = new ChatResponse(List.of(generation)); + + Prompt prompt = new Prompt(List.of(new UserMessage("test mixed"))); + + DefaultToolCallingManager manager = DefaultToolCallingManager.builder() .observationRegistry(ObservationRegistry.NOOP) .toolCallbackResolver(toolName -> { - if ("testTool".equals(toolName)) { - return mockToolCallback; + if ("validTool".equals(toolName)) { + return validToolCallback; + } + if ("invalidTool".equals(toolName)) { + return invalidToolCallback; } return null; }) .build(); - // Verify that no exception is thrown - assertThatNoException().isThrownBy(() -> managerWithCallback.executeToolCalls(prompt, chatResponse)); + ToolExecutionResult result = manager.executeToolCalls(prompt, chatResponse); + + assertThat(validToolCalled).isTrue(); + assertThat(invalidToolCalled).isFalse(); + + List conversation = result.conversationHistory(); + ToolResponseMessage lastMessage = (ToolResponseMessage) conversation.get(conversation.size() - 1); + assertThat(lastMessage.getResponses()).hasSize(2); + + ToolResponseMessage.ToolResponse validResponse = lastMessage.getResponses().get(0); + assertThat(validResponse.name()).isEqualTo("validTool"); + assertThat(validResponse.responseData()).contains("\"ok\""); + + ToolResponseMessage.ToolResponse invalidResponse = lastMessage.getResponses().get(1); + assertThat(invalidResponse.name()).isEqualTo("invalidTool"); + assertThat(invalidResponse.responseData()).containsIgnoringCase("null or empty"); } @Test diff --git a/spring-ai-model/src/test/java/org/springframework/ai/model/tool/DefaultToolCallingManagerTests.java b/spring-ai-model/src/test/java/org/springframework/ai/model/tool/DefaultToolCallingManagerTests.java index dfe52b126f..b9a58d862c 100644 --- a/spring-ai-model/src/test/java/org/springframework/ai/model/tool/DefaultToolCallingManagerTests.java +++ b/spring-ai-model/src/test/java/org/springframework/ai/model/tool/DefaultToolCallingManagerTests.java @@ -377,13 +377,18 @@ void whenMixedMethodToolCallsInChatResponseThenExecute() throws NoSuchMethodExce .toolContext("key", "value") .build()); + // toolA takes a required String "toolInput" — supply it explicitly so the + // call succeeds. toolB takes only a ToolContext, which the framework + // injects, so an empty JSON object is the right value for toolB. ChatResponse chatResponse = ChatResponse.builder() - .generations(List.of(new Generation(AssistantMessage.builder() - .content("") - .properties(Map.of()) - .toolCalls(List.of(new AssistantMessage.ToolCall("toolA", "function", "toolA", "{}"), - new AssistantMessage.ToolCall("toolB", "function", "toolB", "{}"))) - .build()))) + .generations( + List.of(new Generation(AssistantMessage.builder() + .content("") + .properties(Map.of()) + .toolCalls(List.of( + new AssistantMessage.ToolCall("toolA", "function", "toolA", "{\"toolInput\": \"hi\"}"), + new AssistantMessage.ToolCall("toolB", "function", "toolB", "{}"))) + .build()))) .build(); ToolResponseMessage expectedToolResponse = ToolResponseMessage.builder() diff --git a/spring-ai-model/src/test/java/org/springframework/ai/tool/method/MethodToolCallbackExceptionHandlingTest.java b/spring-ai-model/src/test/java/org/springframework/ai/tool/method/MethodToolCallbackExceptionHandlingTest.java index 819a49f410..e261819591 100644 --- a/spring-ai-model/src/test/java/org/springframework/ai/tool/method/MethodToolCallbackExceptionHandlingTest.java +++ b/spring-ai-model/src/test/java/org/springframework/ai/tool/method/MethodToolCallbackExceptionHandlingTest.java @@ -17,13 +17,16 @@ package org.springframework.ai.tool.method; import java.util.List; +import java.util.concurrent.atomic.AtomicBoolean; import org.junit.jupiter.api.Test; import org.springframework.ai.tool.annotation.Tool; +import org.springframework.ai.tool.annotation.ToolParam; import org.springframework.ai.tool.execution.ToolExecutionException; import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.assertThatCode; import static org.assertj.core.api.Assertions.assertThatThrownBy; /** @@ -73,6 +76,78 @@ void testGenericListType() throws Exception { .hasMessageContaining("Unrecognized token"); } + /** + * When streaming tool call aggregation produces an empty argument map (for example, + * when partial JSON arrives as {@code "{}"}), a required parameter will be null in + * the input map. Previously, {@code buildTypedArgument} returned null, the tool was + * invoked with null, and it often produced a valid-looking empty response. The model + * would then retry the exact same call repeatedly, causing runaway token usage. + * + *

+ * The fix must throw a {@link ToolExecutionException} so the standard error processor + * surfaces the problem to the model. + * + *

+ * Loop safety for this test: The test calls the tool callback exactly once and + * asserts that the call throws. There is no recursion, no retry loop, and no chat + * model involved — so this test cannot itself trigger a runaway loop even if the fix + * regresses. + */ + @Test + void testMissingRequiredParameterThrowsToolExecutionException() { + AtomicBoolean methodWasInvoked = new AtomicBoolean(false); + RequiredParamTools testObject = new RequiredParamTools(methodWasInvoked); + + var callback = MethodToolCallbackProvider.builder().toolObjects(testObject).build().getToolCallbacks()[0]; + + // Simulate the streaming aggregation failure scenario: the arguments JSON parsed + // successfully but the required parameter is absent from the object. + String emptyArgs = "{}"; + + assertThatThrownBy(() -> callback.call(emptyArgs)).isInstanceOf(ToolExecutionException.class) + .hasMessageContaining("Missing required parameter") + .hasMessageContaining("points"); + + // Critical: the underlying method must NOT have been invoked with null. If it + // had been, the tool would silently "succeed" and the model would loop. + assertThat(methodWasInvoked).isFalse(); + } + + /** + * Complementary test: a parameter marked {@code @ToolParam(required = false)} should + * be allowed to be null — we must not over-correct and start rejecting legitimately + * optional arguments. + */ + @Test + void testMissingOptionalParameterIsAllowed() { + OptionalParamTools testObject = new OptionalParamTools(); + + var callback = MethodToolCallbackProvider.builder().toolObjects(testObject).build().getToolCallbacks()[0]; + + // The method will be invoked with a null `note` and should return normally. + assertThatCode(() -> { + String result = callback.call("{}"); + assertThat(result).contains("note=null"); + }).doesNotThrowAnyException(); + } + + /** + * A method with no parameters at all should still be callable with an empty argument + * map — the zero-param case. This protects against regressing in the opposite + * direction. + */ + @Test + void testZeroParamToolIsCallableWithEmptyArgs() { + ZeroParamTools testObject = new ZeroParamTools(); + + var callback = MethodToolCallbackProvider.builder().toolObjects(testObject).build().getToolCallbacks()[0]; + + assertThatCode(() -> { + String result = callback.call("{}"); + assertThat(result).isEqualTo("\"pong\""); + }).doesNotThrowAnyException(); + } + public static class TestTools { @Tool(description = "Process a list of strings") @@ -82,4 +157,40 @@ public String stringList(List strings) { } + public static class RequiredParamTools { + + private final AtomicBoolean invoked; + + RequiredParamTools(AtomicBoolean invoked) { + this.invoked = invoked; + } + + @Tool(description = "Draw a path on a chart") + public String drawPath(@ToolParam(description = "Points of the path") List points) { + this.invoked.set(true); + // If this ever runs, return a value that would look like success to a model, + // demonstrating exactly the silent-failure pattern the test guards against. + return "drew path with " + (points == null ? "null" : points.size() + " points"); + } + + } + + public static class OptionalParamTools { + + @Tool(description = "Record something with an optional note") + public String record(@ToolParam(description = "An optional note", required = false) String note) { + return "note=" + note; + } + + } + + public static class ZeroParamTools { + + @Tool(description = "Ping the server") + public String ping() { + return "pong"; + } + + } + }