-
Notifications
You must be signed in to change notification settings - Fork 516
functiontool: support non-struct inputs from LLM function calls #377
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
base: main
Are you sure you want to change the base?
Conversation
Automatically wrap function tools with non-struct inputs so LLM map-based function calls like {"input": <value>} are handled. Adds wrapper, tests, and docs.
Summary of ChangesHello @saurabhhhcodes, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
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.
Code Review
This pull request introduces a valuable enhancement by adding automatic support for non-struct inputs in function tools, which simplifies their creation and use. The implementation is well-structured, and the inclusion of documentation and tests is commendable. I've identified a few areas for improvement in the new wrapper.go file, primarily related to code simplification by removing unused or redundant code, and a bug in error handling where a specific validation error was being suppressed. Addressing these points will further improve the code's robustness and maintainability.
| if err1 := w.outputSchema.Validate(output); err1 != nil { | ||
| return resp, err // if it fails propagate original err. | ||
| } |
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.
There's an issue with the error handling in this block. If w.outputSchema.Validate(output) fails, the specific validation error err1 is discarded, and the original, more generic conversion error err is returned. This hides the true cause of the failure. The more specific validation error err1 should be propagated to aid in debugging.
if err1 := w.outputSchema.Validate(output); err1 != nil {
return nil, fmt.Errorf("output validation failed: %w", err1)
}
tool/functiontool/wrapper.go
Outdated
| inputSchema *jsonschema.Resolved | ||
| outputSchema *jsonschema.Resolved | ||
| handler Func[TArgs, TResults] | ||
| innerTool *functionTool[TArgs, TResults] |
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.
tool/functiontool/wrapper.go
Outdated
| if w.inputSchema != nil { | ||
| wrappedSchema.Properties["input"] = w.inputSchema.Schema() | ||
| } else { | ||
| // Use string as default if no schema | ||
| wrappedSchema.Properties["input"] = &jsonschema.Schema{Type: "string"} | ||
| } |
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.
This if-else block can be simplified. The wrapNonStructInput function is called from New, which ensures that inputSchema is non-nil; otherwise, it would have returned an error. Therefore, the else block appears to be unreachable. You can remove the conditional and directly assign w.inputSchema.Schema().
wrappedSchema.Properties["input"] = w.inputSchema.Schema()
tool/functiontool/wrapper.go
Outdated
| if wrappedSchema != nil { | ||
| decl.ParametersJsonSchema = wrappedSchema | ||
| } |
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.
…gnment, propagate validation errors
|
Requesting maintainers' review and workflow approval for this change (wrapper for non-struct functiontool inputs). Summary: Adds automatic wrapper for function tools with non-struct inputs (e.g., string/int), unit+integration tests, docs, and CHANGELOG entry. Local tests passed. Please review and approve workflows so CI can run. If you prefer different reviewer assignments or a smaller branch, let me know and I can update the PR. |
|
(Fallback) Could maintainers @dmitry and @baptmont review this PR? Top contributors identified from repo history: Dmitry Pasiukevich, Jaana Dogan, João Westerberg, Karol Droste. I attempted to add reviewers programmatically but the API returned a permissions error; please add reviewers or approve workflows when you can. Thank you!,isBackground:false} |
|
I'll monitor this PR and will merge automatically once the required maintainer approvals and CI/workflow checks complete. If maintainers prefer a smaller PR, I can split this into a minimal wrapper-only PR — please let me know. Thanks! |
Proposes automatic wrapping of function tools whose input type is a non-struct (e.g. string/int) so they accept LLM function-call arguments like {"input": }. Includes wrapper implementation, tests, docs, and a changelog entry.