Skip to content

feat: Improve validation in MethodToolCallbackProvider #2964

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed

Conversation

tzolov
Copy link
Contributor

@tzolov tzolov commented May 1, 2025

This ensures that validation errors are caught early during object construction rather than later when methods are called, providing better error feedback.

  • Add validation for tool-annotated methods during construction
  • Validate duplicate tool names in constructor instead of only at getToolCallbacks() time
  • Add comprehensive test suite for MethodToolCallbackProvider

This ensures that validation errors are caught early during object construction
rather than later when methods are called, providing better error feedback.

- Add validation for tool-annotated methods during construction
- Validate duplicate tool names in constructor instead of only at getToolCallbacks() time
- Add comprehensive test suite for MethodToolCallbackProvider

Signed-off-by: Christian Tzolov <[email protected]>
@tzolov tzolov added this to the 1.0.0-RC1 milestone May 1, 2025
.toList();

if (toolMethods.isEmpty()) {
throw new IllegalStateException("No @Tool annotated methods found in " + toolObject);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider extending this message to include "Did you mean to pass a ToolCallback or ToolCallbackProvider? If so, you have to use .toolCallbacks() instead of .tool()".

It could also be a warn log message.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we are seeing that this is a pretty critical fix, the app won't work as expected so I think an exception rather than a warn message is appropriate. agree wrt to the log message.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree. Also they could mean to call defaultToolCallbacks or defaultTool.

validateToolCallbacks(getToolCallbacks());
}

private void assertToolAnnotatedMethodsPresent(List<Object> toolObjects) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

very nice!

@markpollack
Copy link
Member

merged in 6c52c99

kept the exception, can change if needed. let's see how this plays.

@markpollack markpollack closed this May 1, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants