Validate the input must be a struct when creating tools

[git-hulk]

This may partially resolve #330 by checking if the input is a struct type.

[gemini-code-assist]

Summary of Changes

Hello @git-hulk, I'm Gemini Code Assist¹! 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 enhances the functiontool package by implementing a crucial validation step during tool creation. It now mandates that all input arguments for a function tool must be defined as a struct, thereby standardizing the input schema and preventing potential issues with unstructured data types. This modification improves the reliability of the tool creation process and is supported by comprehensive test updates and additions.

Highlights

• Input Type Validation: Introduced a new validation check in functiontool.New to ensure that the input arguments (TArgs) for creating a tool must be a struct type. This prevents the use of primitive types directly as tool inputs.
• Issue Resolution: This change partially addresses issue [Bug] llmagent is only compatible with functiontool where input is a struct #330, improving the robustness of tool creation by enforcing a structured input format.
• Test Updates: Existing test cases in function_test.go were updated to conform to the new validation rule, converting primitive input types to wrapper struct types. New test cases were added to specifically verify that functiontool.New correctly rejects non-struct input types.

Using Gemini Code Assist

The 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 /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature	Command	Description
Code Review	/gemini review	Performs a code review for the current pull request in its current state.
Pull Request Summary	/gemini summary	Provides a summary of the current pull request in its current state.
Comment	@gemini-code-assist	Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help	/gemini help	Displays a list of available commands.

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 .gemini/ folder in the base of the repository. Detailed instructions can be found here.

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

1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩

[gemini-code-assist]

Code Review

This pull request adds a validation check to ensure that function tool inputs are structs. The reflection-based implementation in function.go is solid and correctly handles pointers. The tests are also well-updated, with new cases in TestNew_InvalidInputType to cover the new validation.

I found a minor copy-paste error in a new test case that would cause a compilation failure, and I've provided a suggestion to fix it.

Additionally, it appears tool/tool_test.go was not updated and contains a test that will now fail. You'll likely need to update it to use a struct-based tool to get the build to pass.

[git-hulk]

@mazas-google Thanks for your review. I have applied the review suggestions.

[mazas-google]

LGTM

[kdroste-google]

LGTM

[kdroste-google]

LGTM

[git-hulk]

Sorry for missing the lint error in CI, I have enabled the CI on my side and confirmed it works well: https://github.com/git-hulk/adk-go/actions/runs/19690326139/job/56404514448.

By the way, is it possible to enable CI on every push operation instead of only for the main branch? This will be easier for contributors to handle the CI issue before waiting for maintainers to approve the workflow. cc @mazas-google I'll be happy to submit a PR to do that if it sounds good to you.

[dpasiukevich]

maps are also supported right now. Restricting validation to a struct only, that will break function tools that use maps:

updateUserPreference(ctx tool.Context, args map[string]string) (map[string]string, error)

[git-hulk]

maps are also supported right now. Restricting validation to a struct only, that will break function tools that use maps:

updateUserPreference(ctx tool.Context, args map[string]string) (map[string]string, error)

@dpasiukevich I also considered this situation while fixing this issue. But it seems map[string]string as the input type should not work for now, because it cannot correctly resolve the fields of the input schema when the type is map[string]string. So it's impossible for LLM to infer the correct fields for the function call. Please correct me if my understanding is wrong.

[dpasiukevich]

For example if we have this go func func updateUserPreference(ctx tool.Context, args map[string]string) (*updateUserPreferenceResult, error) { ... }

This will generate the following jsonschema for input

          {
            "parametersJsonSchema": {
              "type": "object",
              "additionalProperties": {
                "type": "string"
              }
            },

it's impossible for LLM to infer the correct fields for the function call

It all depends on the LLM instruction. For example if there's such instruction

"You are an agent that can update user preferences. When a user asks to set a preference, identify the preference key and the desired value. For example, in 'set my theme to dark', the key is 'theme' and the value is 'dark'. Then, call the 'update_user_preference' tool with these values."

Then LLM will define both fields/keys and values.

I agree, using structs is more straightforward as it defines a concrete schema.
But using maps gives more flexibility as LLM will and can define both keys and the values.

IMO we should restrict the functiontool Args to types which can be converted to json object (structs and maps come to my mind).

[git-hulk]

@dpasiukevich Aha, I see. It did work if we explicitly specified the key-value pairs in the instruction. It looks reasonable, but it should be hard to use from the user side.

So, should I relax the input validation by allowing the map type?

[dpasiukevich]

So, should I relax the input validation by allowing the map type?

Yes, we should allow anything which jsonschema-go converts to json object (only maps and structs come to my mind, but if we missed anything, we can expand validation until there's a proper solution for other json types).

it should be hard to use from the user side.

Just to understand your perspective -- why would it be hard to use from the user side?

This instruction does the whole thing:

When a user asks to set a preference, identify the preference key and the desired value. For example, in 'set my theme to dark', the key is 'theme' and the value is 'dark'. Then, call the 'update_user_preference' tool with these values.

Maps are the only option is the keys/fields are not known in the instruction and LLM has to infer them.
If only structs are allowed, it would still be possible to make a map within struct, but then why limiting it on top level?

[git-hulk]

@dpasiukevich Thanks for your clarification.

Yes, we should allow anything which jsonschema-go converts to json object (only maps and structs come to my mind, but if we missed anything, we can expand validation until there's a proper solution for other json types).

No problem.

Just to understand your perspective -- why would it be hard to use from the user side?

Yes, my point is user must know what field name and type can be used in the instruction, or the LLM may infer the wrong type for this field.

[git-hulk]

@dpasiukevich Could have a look again.

[git-hulk]

@dpasiukevich Could you please take a look at it to see if anything else needs to be addressed?

[kdroste-google]

LGTM

[dpasiukevich]

Apologies, this PR got lost in the inbox.

One small nit + could you PTAL:

And we're good to merge. Thanks!

[git-hulk]

Apologies, this PR got lost in the inbox.

One small nit + could you PTAL:

And we're good to merge. Thanks!

@dpasiukevich Done, sorry for didn't notice this format issue.

[kdroste-google]

LGTM