-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Conversation
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]>
.toList(); | ||
|
||
if (toolMethods.isEmpty()) { | ||
throw new IllegalStateException("No @Tool annotated methods found in " + toolObject); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
very nice!
merged in 6c52c99 kept the exception, can change if needed. let's see how this plays. |
This ensures that validation errors are caught early during object construction rather than later when methods are called, providing better error feedback.