Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -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<String, Object> arguments = ModelOptionsUtils.jsonToMap(toolCallInput);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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<String, Object> arguments = ModelOptionsUtils.jsonToMap(toolCallInput);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -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.
*
* <p>
* <b>Loop safety:</b> 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<McpSchema.CallToolRequest> 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<McpSchema.CallToolRequest> requestCaptor = ArgumentCaptor
.forClass(McpSchema.CallToolRequest.class);
verify(this.mcpClient).callTool(requestCaptor.capture());
assertThat(requestCaptor.getValue().arguments()).isEmpty();
verifyNoInteractions(this.mcpClient);
}

@Test
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -116,30 +117,44 @@ 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.
*
* <p>
* <b>Loop safety:</b> 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)
.tool(this.tool)
.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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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()
Expand All @@ -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())
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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;
Expand Down Expand Up @@ -144,10 +146,33 @@ private Object[] buildMethodArguments(Map<String, Object> 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;
Expand Down
Loading