Consider the current implementation of validateToolContextSupport compared from 1.0.0 and 1.0.0-M6

    private void validateToolContextSupport(@Nullable ToolContext toolContext) {
        var isNonEmptyToolContextProvided = toolContext != null && !CollectionUtils.isEmpty(toolContext.getContext());
        var isToolContextAcceptedByMethod = Stream.of(this.toolMethod.getParameterTypes())
            .anyMatch(type -> ClassUtils.isAssignable(type, ToolContext.class));
        if (isToolContextAcceptedByMethod && !isNonEmptyToolContextProvided) {
            throw new IllegalArgumentException("ToolContext is required by the method as an argument");
        }
    }

M6

    private void validateToolContextSupport(@Nullable ToolContext toolContext) {
        var isToolContextRequired = toolContext != null && !CollectionUtils.isEmpty(toolContext.getContext());
        var isToolContextAcceptedByMethod = Stream.of(toolMethod.getParameterTypes())
            .anyMatch(type -> ClassUtils.isAssignable(type, ToolContext.class));
        if (isToolContextRequired && !isToolContextAcceptedByMethod) {
            throw new IllegalArgumentException("ToolContext is not supported by the method as an argument");
        }
    }

  1. Both have the same serious bug also refered in #3355 , it seems that the writer does not know the semantics of ClassUtils.isAssignable(left, right) => right is assinable to left, so left is the "super" type. All this code has the arguments in incorrect order.

  2. More, in the case of a toolContext not being provided, the change in the validation logic now make it fail due to error 1 M6 : No context => isToolContextRequired = false
    => isToolContextRequired && !isToolContextAcceptedByMethod = false => no exception 1.0.0 : No context => isNonEmptyToolContextProvided = false + isNonEmptyToolContextProvided = true (because of error 1) => isToolContextAcceptedByMethod && !isNonEmptyToolContextProvided = true => exception is thrown !

Comment From: sunyuhan1998

Hi @jcgouveia , I checked the code and I agree with you and pushed a PR to try to fix it :https://github.com/spring-projects/spring-ai/pull/3478, can you help to review this PR?thanks.

Comment From: ilayaperumalg

@jcgouveia Thanks for reporting the issue and the detailed writeup! Please feel free to contribute your suggestions as pull request as well.

@sunyuhan1998 Thanks for the PR!

Comment From: ilayaperumalg

Hi @jcgouveia @sunyuhan1998, I want to bring up a specific use case into discussion related to the logic applied as part of this change. With this tool context validation check, if a chat client has multiple Method tool callbacks but one of them doesn't need tool context to be processed, with a non-empty tool context value, the validation would fail.

Comment From: sunyuhan1998

Hi @jcgouveia @sunyuhan1998, I want to bring up a specific use case into discussion related to the logic applied as part of this change. With this tool context validation check, if a chat client has multiple Method tool callbacks but one of them doesn't need tool context to be processed, with a non-empty tool context value, the validation would fail.

I think I understand what you mean. I believe the key issue lies in the following code:

https://github.com/spring-projects/spring-ai/blob/5cb5c90a0c2621e0a2776b4f7478eb0eefcacdf6/spring-ai-model/src/main/java/org/springframework/ai/model/tool/DefaultToolCallingManager.java#L222-L230

It does not take into account whether the Method of the ToolCallback itself accepts ToolContext as a parameter.

I think we have two ways to solve this issue:

  1. Adjust the call(String toolInput, @Nullable ToolContext toolContext) method in MethodToolCallback so that if toolContext is null, it internally converts to a call to call(String toolInput).

  2. Modify the DefaultToolCallingManager to determine which call method to invoke based on the actual situation during execution.

I prefer the second approach because the first one violates the definition in the ToolCallback interface, which specifies that toolContext should be non-null in the call(String toolInput, @Nullable ToolContext toolContext) method.

I think I can submit a PR to try to fix this issue, do we need to open a new issue?

Comment From: sunyuhan1998

if a chat client has multiple Method tool callbacks but one of them doesn't need tool context to be processed, with a non-empty tool context value, the validation would fail.

Hi @ilayaperumalg , I have submitted an additional PR https://github.com/spring-projects/spring-ai/pull/3521 to try and fix the issue, could you help review it?thanks.