From 936574d6265c4ec05880be37c830f93522359f15 Mon Sep 17 00:00:00 2001 From: Laran Evans Date: Fri, 10 Apr 2026 08:32:46 -0700 Subject: [PATCH] Fix silent null-arg tool dispatch causing runaway tool-call loops MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Related: https://github.com/spring-projects/spring-ai/issues/5754 Related: https://github.com/spring-projects/spring-ai/issues/3333 Related: https://github.com/spring-projects/spring-ai/issues/2383 Related: https://github.com/spring-projects/spring-ai/issues/4464 Related: https://github.com/spring-projects/spring-ai/issues/4617 The "Handle the possible null parameter situation in streaming mode" logic added in b059cdf9 silently replaced null or empty tool-call arguments with "{}". When a tool has required parameters, the downstream MethodToolCallback then called the method with null for every required argument and silently returned whatever the tool produced. Many tool implementations return a valid-looking but empty result in that case, which the model interprets as a transient failure and retries — often with the identical call. Combined with the absence of any iteration limit on Spring AI's tool-call recursion (#3333), this can produce multi-million-token runaway loops in a single turn. Fix: 1. DefaultToolCallingManager: when tool arguments are null or empty, raise a ToolExecutionException and route it through the standard ToolExecutionExceptionProcessor so the resulting error becomes a proper tool response. The model can then adjust its approach rather than retry blindly. Well-formed tool calls in the same batch still execute normally. 2. MethodToolCallback.buildMethodArguments: when a required parameter (default: @ToolParam.required = true) is missing from the tool input map, throw a ToolExecutionException with a clear "Missing required parameter" message. Previously, buildTypedArgument silently returned null for missing values, allowing method invocation to proceed with null arguments. Optional parameters marked with @ToolParam(required = false) still pass null through unchanged, and zero-parameter tools are unaffected. 3. Sync/AsyncMcpToolCallback: apply the same fix as (1) for MCP callbacks that had the identical silent-"{}" fallback. Tests: - DefaultToolCallingManagerTest: the two tests that previously asserted the buggy behavior (shouldHandleNullArgumentsInStreamMode, shouldHandleEmptyArgumentsInStreamMode) are rewritten to assert that the tool callback is NOT invoked and that the conversation history contains a tool response with the error from the exception processor. A new test (shouldExecuteValidToolsWhileReturningErrorForMalformedTool) verifies that a batch containing a malformed call and a valid call processes both independently. - MethodToolCallbackExceptionHandlingTest: new tests verify that (a) missing required parameters throw ToolExecutionException and the underlying method is never invoked, (b) optional parameters are still allowed to be null, and (c) zero-param tools remain callable with "{}". - SyncMcpToolCallbackTests / AsyncMcpToolCallbackTest: rewritten null/empty input tests to assert the new error path and verify that the MCP client is never invoked. - DefaultToolCallingManagerTests#whenMixedMethodToolCallsInChatResponse ThenExecute was implicitly relying on the silent-null behavior by calling TestGenericClass.call(String) with an empty args map. Updated to supply a value for the required parameter. Loop safety of the new tests: every new test invokes its callback exactly once and asserts on the single result. There is no recursion, retry loop, or chat model involved, so the tests cannot themselves reproduce the runaway loop they guard against. Signed-off-by: Laran Evans --- .../ai/mcp/AsyncMcpToolCallback.java | 14 +- .../ai/mcp/SyncMcpToolCallback.java | 14 +- .../ai/mcp/AsyncMcpToolCallbackTest.java | 59 +++--- .../ai/mcp/SyncMcpToolCallbackTests.java | 41 +++-- .../model/tool/DefaultToolCallingManager.java | 36 ++-- .../ai/tool/method/MethodToolCallback.java | 25 +++ .../tool/DefaultToolCallingManagerTest.java | 174 +++++++++++++++--- .../tool/DefaultToolCallingManagerTests.java | 17 +- ...thodToolCallbackExceptionHandlingTest.java | 111 +++++++++++ 9 files changed, 393 insertions(+), 98 deletions(-) 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"; + } + + } + }