Fix silent null-arg tool dispatch causing runaway tool-call loops#5790
Open
laran wants to merge 1 commit intospring-projects:mainfrom
Open
Fix silent null-arg tool dispatch causing runaway tool-call loops#5790laran wants to merge 1 commit intospring-projects:mainfrom
laran wants to merge 1 commit intospring-projects:mainfrom
Conversation
Related: spring-projects#5754 Related: spring-projects#3333 Related: spring-projects#2383 Related: spring-projects#4464 Related: spring-projects#4617 The "Handle the possible null parameter situation in streaming mode" logic added in b059cdf 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 (spring-projects#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 <laran@laranevans.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Related: #5754
Related: #3333
Related: #2383
Related: #4464
Related: #4617
The "Handle the possible null parameter situation in streaming mode" logic added in b059cdf 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:
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.
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.
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.
Thank you for taking time to contribute this pull request!
You might have already read the contributor guide, but as a reminder, please make sure to:
git commit -s) per the DCOmainbranch and squash your commitsFor more details, please check the contributor guide.
Thank you upfront!