Skip to content

fix: validate and coerce function tool argument types#4664

Open
yuvrajangadsingh wants to merge 12 commits intogoogle:mainfrom
yuvrajangadsingh:fix/enforce-arg-types-4612
Open

fix: validate and coerce function tool argument types#4664
yuvrajangadsingh wants to merge 12 commits intogoogle:mainfrom
yuvrajangadsingh:fix/enforce-arg-types-4612

Conversation

@yuvrajangadsingh
Copy link

Fixes #4612

_preprocess_args currently only handles Pydantic model conversion but passes all other types through without any validation. This means a function declared with num: int happily accepts "foobar", 3.14159, or None at runtime.

This PR extends _preprocess_args to validate and coerce all annotated argument types using pydantic.TypeAdapter. When coercion fails, the validation error is returned to the LLM so it can self-correct and retry (same pattern as the existing missing-mandatory-args check).

What changed:

  • _preprocess_args now returns (args, validation_errors) tuple
  • Uses pydantic.TypeAdapter.validate_python() for all non-Pydantic-BaseModel annotated params
  • Compatible types get coerced automatically (e.g. "42" -> 42 for int, "red" -> Color.RED for enum)
  • Incompatible types produce a validation error returned to the LLM
  • Existing Pydantic BaseModel handling is unchanged (graceful failure with warning log)
  • All 3 call sites updated: FunctionTool.run_async, CrewAITool.run_async, sync tool path in functions.py

Before (no validation):

def set_num(num: int):
    print(type(num), num)

# LLM sends num=3.14159 -> passes through as float, no error
# LLM sends num="foobar" -> passes through as string, no error

After (validates and coerces):

# LLM sends num="42" -> coerced to int 42, function runs
# LLM sends num="foobar" -> error returned to LLM:
#   "Parameter 'num': expected type '<class 'int'>', validation error: ..."
# LLM can then retry with correct types

Tests: 17 new tests covering coercion (str->int, str->float, str->enum, list coercion, Optional handling, bool coercion), validation errors (invalid str for int, None for required int, multiple param errors), and run_async integration (error returned to LLM, valid coercion invokes function, enum validation). All 1776 existing tests continue to pass.

_preprocess_args now uses pydantic.TypeAdapter to validate and coerce
all annotated argument types (primitives, enums, containers), not just
Pydantic models. Invalid arguments return a descriptive error to the
LLM so it can self-correct and retry, matching the existing pattern
for missing mandatory args.

- Coerces compatible types (e.g. str "42" -> int 42, str "red" -> Color.RED)
- Returns validation errors to LLM for incompatible types (e.g. "foobar" -> int)
- Existing Pydantic BaseModel handling unchanged (graceful failure)
- Updated all 3 call sites (FunctionTool, CrewAITool, sync tool path)
@yuvrajangadsingh
Copy link
Author

@cuihaoleo this implements the TypeAdapter-based validation you suggested in the issue comments. validation errors go back to the LLM so it can self-correct (the LangChain approach you mentioned). existing Pydantic model behavior is unchanged.

@llalitkumarrr @xuanyang15 would appreciate a review when you get a chance.

@gemini-code-assist
Copy link
Contributor

Summary of Changes

Hello, 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 robustness of function tool argument handling by introducing comprehensive type validation and coercion. Previously, tools would accept incorrect argument types without validation, leading to runtime errors. Now, arguments are validated against their type annotations using Pydantic's TypeAdapter, and compatible types are automatically coerced. If validation fails, detailed error messages are returned to the LLM, enabling it to understand and correct its tool calls, thereby improving the reliability and self-correction capabilities of LLM-powered agents.

Highlights

  • Enhanced Argument Validation and Coercion: The _preprocess_args method now comprehensively validates and coerces all annotated argument types for function tools using pydantic.TypeAdapter, moving beyond just Pydantic model conversions.
  • Improved Error Handling for LLMs: Tool invocation functions (FunctionTool.run_async, CrewAITool.run_async, and the sync tool path) now capture and return detailed validation errors to the LLM, enabling it to self-correct and retry tool calls with proper argument types.
  • Updated _preprocess_args Signature: The _preprocess_args method's return signature has been updated to tuple[dict[str, Any], list[str]], providing both the processed arguments and any encountered validation errors.
  • Comprehensive Test Coverage: A new test file has been added with 17 new tests covering various coercion scenarios (e.g., string to int/float/enum, list coercion, optional handling) and validation error cases, ensuring the robustness of the new logic.

🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console.

Changelog
  • src/google/adk/flows/llm_flows/functions.py
    • Modified the run_sync_tool function to capture and handle validation errors returned by tool._preprocess_args, returning an error message to the caller if issues are found.
  • src/google/adk/tools/crewai_tool.py
    • Updated the run_async method to receive validation errors from self._preprocess_args and return a structured error response to the LLM if any validation failures occur.
  • src/google/adk/tools/function_tool.py
    • Refactored the _preprocess_args method to return a tuple containing processed arguments and a list of validation error strings.
    • Implemented pydantic.TypeAdapter within _preprocess_args to validate and coerce primitive types, enums, and container types based on function annotations.
    • Preserved existing Pydantic BaseModel conversion logic with its graceful failure mechanism.
    • Modified the run_async method to check for and propagate validation errors from _preprocess_args back to the LLM.
  • tests/unittests/tools/test_function_tool_arg_validation.py
    • Added a new test file containing extensive tests for argument coercion (e.g., string to int/float, enum, list) and validation error scenarios (e.g., invalid string for int, None for required int, multiple parameter errors).
    • Included integration tests for run_async to verify that invalid arguments result in error messages returned to the LLM and valid coercions correctly invoke the function.
  • tests/unittests/tools/test_function_tool_pydantic.py
    • Updated calls to tool._preprocess_args in existing tests to correctly unpack the new (args, errors) tuple return value.
Activity
  • The pull request introduces 17 new tests specifically designed to cover various argument coercion and validation scenarios, including string-to-numeric, string-to-enum, list coercion, optional handling, and boolean coercion.
  • Validation error handling is tested for cases like invalid string-to-int conversion, None for required integers, and multiple parameter errors.
  • Integration tests confirm that run_async correctly returns validation errors to the LLM and successfully invokes functions with valid, coerced arguments.
  • All 1776 existing tests continue to pass, ensuring no regressions.
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.

@adk-bot adk-bot added the core [Component] This issue is related to the core interface and implementation label Feb 28, 2026
Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a 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 argument type validation and coercion for function tools. The use of pydantic.TypeAdapter is a solid approach for this. The changes are well-implemented across the affected files, and the new tests provide good coverage for the new functionality.

I have a couple of suggestions to improve maintainability and robustness: one is to refactor the duplicated error message generation into a helper method, and the other is to use more specific exception handling instead of a broad except Exception. Overall, this is a great improvement to the framework's reliability.

@yuvrajangadsingh
Copy link
Author

@gemini-code-assist addressed both suggestions in 08c9f55:

  1. narrowed except Exception to except (TypeError, NameError) for the TypeAdapter fallback
  2. extracted _build_validation_error_response helper on FunctionTool, all 3 call sites now use it

/gemini review

Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a 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 robust type validation and coercion for function tool arguments by leveraging pydantic.TypeAdapter. This is a great improvement that will prevent runtime errors from incorrect argument types passed by the LLM and allow it to self-correct. The changes are well-implemented across FunctionTool, CrewAITool, and the synchronous tool path, and are accompanied by a comprehensive new test suite. I have a few suggestions to further improve the robustness and consistency of the implementation.

@yuvrajangadsingh
Copy link
Author

@gemini-code-assist addressed all 3 suggestions in 2d5b34f:

  1. pydantic model None check now uses is_optional too, consistent with the other types
  2. (TypeError, NameError) fallback now logs a warning instead of silent skip
  3. strengthened multi-param test: asserts exact error count (3) and checks all param names. pydantic rejects int->str in lax mode too, so all 3 params fail.

/gemini review

Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a 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 robust type validation and coercion for function tool arguments using pydantic.TypeAdapter. This is a significant improvement that prevents runtime errors from invalid data types passed by the LLM and enables self-correction by providing clear validation errors. The changes are well-implemented across FunctionTool and CrewAITool, and the new unit tests are comprehensive, covering various coercion and validation scenarios. The separation of concerns, preserving the existing graceful failure for Pydantic models while enforcing stricter validation for other types, is a sensible design choice.

@yuvrajangadsingh
Copy link
Author

@gemini-code-assist applied in 7507121. error messages now show int instead of <class 'int'>.

/gemini review

Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a 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 significant improvement by adding type validation and coercion for function tool arguments using pydantic.TypeAdapter. The changes are well-implemented, returning validation errors to the LLM for self-correction, and are accompanied by a comprehensive new test suite. I've identified a minor issue in a new test case and a small logging style improvement. Overall, this is a great enhancement to the tool's robustness.

@yuvrajangadsingh
Copy link
Author

@gemini-code-assist addressed in 6ed9ba1:

  1. test assertion (int->str): pushed back on this one. pydantic.TypeAdapter(str).validate_python(123) actually raises ValidationError in pydantic v2, it does NOT coerce int to str. verified locally. the test with 3 errors is correct.

  2. deferred logging: applied, using %s formatting instead of f-string.

/gemini review

Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a 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 significant improvement by adding validation and coercion for function tool argument types using pydantic.TypeAdapter. The changes are well-implemented, robust, and thoroughly tested. My review found one minor area for improvement to ensure compatibility with newer Python syntax for optional types. Overall, this is an excellent contribution that enhances the reliability of tool function calls.

@yuvrajangadsingh
Copy link
Author

@gemini-code-assist the get_origin(param.annotation) is Union check is pre-existing code from before this PR, not something I introduced. and in practice it doesn't cause issues here because TypeAdapter handles int | None natively: TypeAdapter(int | None).validate_python(None) returns None correctly, no unwrapping needed.

fixing the Union vs UnionType handling across the whole method would be a good follow-up but it's out of scope for this PR since it affects the pre-existing Pydantic model path too.

/gemini review

Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a 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 improvement by adding type validation and coercion for function tool arguments. The use of pydantic.TypeAdapter is a solid approach for this. The changes are well-implemented across FunctionTool, CrewAITool, and llm_flows/functions.py, ensuring consistent behavior. The new unit tests in test_function_tool_arg_validation.py are comprehensive and cover a wide range of scenarios, which is great for ensuring the new logic is robust. I have one suggestion to improve code clarity by removing a small piece of duplicated logic. Overall, this is a great enhancement that will improve the reliability of tool calls.

@yuvrajangadsingh
Copy link
Author

@gemini-code-assist applied in e35c130. hoisted the Optional None check before the Pydantic branch so it covers both paths in one place.

/gemini review

Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a 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 robust argument type validation and coercion for function tools using pydantic.TypeAdapter, which is an excellent enhancement for tool reliability. The core logic changes in function_tool.py are well-structured, and the new unit tests in test_function_tool_arg_validation.py are comprehensive. The necessary updates at the call sites in crewai_tool.py and functions.py correctly integrate the new error handling mechanism. I have one high-severity suggestion concerning src/google/adk/flows/llm_flows/functions.py to refactor the tool execution logic. This will address duplicated logic and inconsistent behavior between synchronous and asynchronous tool executions, leading to a more robust and maintainable implementation.

@yuvrajangadsingh
Copy link
Author

@gemini-code-assist the sync/async duplication in _call_tool_in_thread_pool is pre-existing code, not something introduced by this PR. I only updated the existing call site to handle the new tuple return from _preprocess_args.

refactoring the entire sync tool execution path to go through run_async is a valid improvement but it's a separate concern with its own risk surface. out of scope here.

/gemini review

Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a 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 significant improvement by adding validation and type coercion for function tool arguments using pydantic.TypeAdapter. This makes tool execution more robust by catching type mismatches from the LLM early and providing feedback for self-correction. The changes are well-structured, with the core logic encapsulated in FunctionTool._preprocess_args, and updates are correctly propagated to CrewAITool and the synchronous tool execution path. The addition of a comprehensive new test suite (test_function_tool_arg_validation.py) is excellent and covers various coercion and validation scenarios. I have one suggestion to improve compatibility with modern Python type hints.

@yuvrajangadsingh
Copy link
Author

@gemini-code-assist applied in cb12206. added types.UnionType handling for Python 3.10+ T | None syntax. the Optional unwrap now works for both Optional[T] and T | None.

/gemini review

Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a 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 significant improvement by adding type validation and coercion for function tool arguments using pydantic.TypeAdapter. This makes tool calls more robust and allows the LLM to self-correct based on validation feedback. The changes are well-implemented across FunctionTool and its usages, and are accompanied by a comprehensive new test suite.

My review includes two suggestions for src/google/adk/tools/function_tool.py to further improve the error handling:

  1. Make Pydantic BaseModel validation failures consistent with other types by returning validation errors to the LLM instead of just logging a warning.
  2. Refine the validation error messages to be more concise and easier for the LLM to parse by using structured error details from Pydantic's ValidationError.

Overall, this is a valuable and well-executed change.

@yuvrajangadsingh
Copy link
Author

@gemini-code-assist pushing back on both:

  1. Pydantic model error handling: the graceful-failure behavior for Pydantic models is pre-existing and intentional. changing it to return validation errors would be a behavioral change for existing users who rely on the pass-through behavior. out of scope for this PR.

  2. verbose ValidationError: the full error string from pydantic is actually useful for the LLM since it includes the exact field paths and expected types. truncating to just msg would lose context. the LLM can parse multi-line text fine.

/gemini review

Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a 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 argument type validation and coercion for function tools using pydantic.TypeAdapter. This will improve the robustness of tool calls by catching type mismatches from the LLM and allowing it to self-correct. The changes are well-structured, touching FunctionTool, CrewAITool, and the synchronous tool path, and are accompanied by a comprehensive new test suite. My review found one issue in the new tests regarding an incorrect assumption about Pydantic's type coercion behavior.

@yuvrajangadsingh
Copy link
Author

@gemini-code-assist this is incorrect. pydantic v2 (2.12.5) does NOT coerce int to str in lax mode:

>>> pydantic.TypeAdapter(str).validate_python(123)
pydantic_core._pydantic_core.ValidationError: 1 validation error for str
  Input should be a valid string [type=string_type, input_value=123, input_type=int]

the test asserting 3 errors is correct. all three params fail validation: int->str, "not_a_number"->int, "not_a_bool"->bool.

/gemini review

Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a 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 improvement by adding argument type validation and coercion for function tools using pydantic.TypeAdapter. The changes are well-structured, and the addition of comprehensive unit tests is commendable. The core logic in _preprocess_args is robust, handling various types, including optional types, and provides clear validation errors to the LLM for self-correction. The updates to FunctionTool, CrewAITool, and the LLM flow functions are consistent with the new error handling mechanism. I have one suggestion to improve consistency in error handling for Pydantic models.

@yuvrajangadsingh
Copy link
Author

@gemini-code-assist these are all repeat suggestions that have been addressed or explicitly declined in previous rounds:

  • int->str coercion: already disproved with actual pydantic 2.12.5 output. TypeAdapter(str).validate_python(123) raises ValidationError. the test is correct.
  • Pydantic model error handling: intentionally preserving pre-existing behavior, out of scope.
  • verbose ValidationError: full error string is more useful for LLM self-correction.

no further changes needed on these. waiting for human review.

@gemini-code-assist
Copy link
Contributor

Thank you for the detailed explanations and for addressing the previous suggestions.

Regarding your points in comment #13:

  • int->str coercion: You are absolutely correct. My apologies for the oversight. I've re-verified Pydantic v2.12.5 behavior, and TypeAdapter(str).validate_python(123) indeed raises a ValidationError. The test asserting 3 errors is correct.
  • Pydantic model error handling: I understand and agree with your decision to maintain the existing graceful-failure behavior for Pydantic models to avoid breaking changes for current users. This is a pragmatic approach for this PR's scope.
  • Verbose ValidationError: Your reasoning for keeping the full, verbose error string for the LLM's self-correction capabilities is sound. Providing more context can indeed be beneficial for the LLM to understand and rectify its tool calls.

It looks like all the points we discussed have been thoroughly considered and addressed or declined with valid justifications. I appreciate your diligent work on this pull request.

…params

Remove the Pydantic model special case from _preprocess_args so
TypeAdapter handles primitives, enums, Pydantic models, and containers
in one path. Skip framework-managed params (_ignore_params) to avoid
validating injected values like credential=None.
@yuvrajangadsingh
Copy link
Author

@gemini-code-assist reconsidered the Pydantic model inconsistency point and agreed. fb7827d removes the special case so TypeAdapter handles all types uniformly. also skips framework-managed params (_ignore_params) to avoid validating injected values like credential=None.

/gemini review

Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a 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 robust type validation and coercion for function tool arguments using pydantic.TypeAdapter. This is a great improvement, as it prevents runtime errors from invalid argument types provided by the LLM and provides clear feedback for self-correction. The implementation is clean and the new tests provide good coverage. I have one suggestion to cache TypeAdapter instances for a performance improvement.

@yuvrajangadsingh
Copy link
Author

@gemini-code-assist applied in 3b62f63. TypeAdapter instances are now cached per target_type on the FunctionTool instance, so they're only created once per param type across invocations.

/gemini review

Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a 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 significant improvement by adding validation and coercion for function tool argument types using pydantic.TypeAdapter. This enhances the robustness of tool calls by catching type mismatches and providing actionable feedback to the LLM for self-correction. The changes are well-implemented across the affected files, and the addition of a comprehensive test suite in test_function_tool_arg_validation.py is excellent. I have one suggestion to simplify the Optional type handling in function_tool.py by leveraging more of Pydantic's capabilities, which would improve code clarity and maintainability.

@yuvrajangadsingh
Copy link
Author

@gemini-code-assist good call. applied in 8ac8cb9. removed the entire Optional unwrap block since TypeAdapter handles Optional[T] and T | None natively. also dropped the now-unused sys, get_args, get_origin, and _UNION_TYPES imports.

/gemini review

Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a 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 significant improvement by adding validation and type coercion for function tool arguments using pydantic.TypeAdapter. This makes the tools more robust against incorrect argument types provided by the LLM and allows for self-correction. The changes are well-implemented across FunctionTool, CrewAITool, and the synchronous tool path. The addition of a comprehensive test suite for the new validation logic is also a great contribution.

My review includes one high-severity comment regarding a potential bug in the TypeAdapter caching logic that could cause validation to be skipped for certain argument types.

@yuvrajangadsingh
Copy link
Author

@gemini-code-assist good catch. applied in 91e5a9d. cache lookup now uses try/except KeyError+TypeError so unhashable types like Union[int, str] still get validated, just not cached.

/gemini review

Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a 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 is a great improvement, introducing robust type validation and coercion for function tool arguments using pydantic.TypeAdapter. This will prevent runtime type errors and allow the LLM to self-correct. The changes are well-implemented across FunctionTool and its consumers, and are accompanied by a comprehensive new test suite. I have one suggestion to improve error handling for invalid type annotations to make developer errors more visible.

@yuvrajangadsingh
Copy link
Author

@gemini-code-assist pushing back on this one. TypeError/NameError from TypeAdapter means a broken type annotation (forward reference, etc), which is a developer bug, not an LLM mistake. returning it as a validation error to the LLM would just confuse it since it can't fix the tool definition. logging a warning is the right call here, it surfaces the issue for the developer without polluting the LLM's error feedback loop.

/gemini review

Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a 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 robust type validation and coercion for function tool arguments by leveraging pydantic.TypeAdapter. The changes ensure that arguments passed to tools at runtime match their declared types, with automatic coercion for compatible types and clear validation errors returned to the LLM for incompatible ones. The implementation is solid, includes performance caching for type adapters, and is well-tested. I've suggested a minor refactoring to improve the readability of the adapter caching logic.

@yuvrajangadsingh
Copy link
Author

@gemini-code-assist applied in ba51ee0. split the cache except into separate TypeError (unhashable, skip cache) and KeyError (cache miss, create and store) handlers.

/gemini review

Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a 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 significant improvement by adding validation and type coercion for function tool arguments using pydantic.TypeAdapter. This is a robust solution to handle type mismatches from the LLM, allowing for self-correction. The changes are well-implemented across FunctionTool, CrewAITool, and the synchronous tool path, and are supported by a comprehensive new test suite. I have one suggestion to improve the readability of the new caching logic.

Comment on lines +135 to +141
try:
adapter = self._type_adapter_cache[target_type]
except TypeError:
adapter = pydantic.TypeAdapter(target_type)
except KeyError:
adapter = pydantic.TypeAdapter(target_type)
self._type_adapter_cache[target_type] = adapter
Copy link
Contributor

Choose a reason for hiding this comment

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

medium

The logic for caching TypeAdapter instances is correct, but it can be refactored to improve readability and reduce code duplication. By combining the KeyError and TypeError exception handling, you can create the adapter once and then attempt to cache it in a nested try...except block. This makes the intent of handling both cache misses and unhashable types more explicit.

Suggested change
try:
adapter = self._type_adapter_cache[target_type]
except TypeError:
adapter = pydantic.TypeAdapter(target_type)
except KeyError:
adapter = pydantic.TypeAdapter(target_type)
self._type_adapter_cache[target_type] = adapter
try:
adapter = self._type_adapter_cache[target_type]
except (KeyError, TypeError):
# Create adapter if not in cache or type is unhashable.
adapter = pydantic.TypeAdapter(target_type)
try:
# Attempt to cache it. This will fail for unhashable types.
self._type_adapter_cache[target_type] = adapter
except TypeError:
# Can't cache unhashable types, but can still use the adapter.
pass

@yuvrajangadsingh
Copy link
Author

@gemini-code-assist this suggestion reverses the previous review's suggestion (which asked to split them apart). going to keep the current version with separate handlers since that's what the previous round asked for. no changes needed here.

waiting for human review.

@gemini-code-assist
Copy link
Contributor

You are absolutely right, and I apologize for the conflicting advice. My previous suggestion to split the TypeError and KeyError handlers was indeed the correct approach, as it properly distinguishes between unhashable types (which should not be cached) and cache misses (which should be cached). The current implementation with separate handlers is robust and correct. Thank you for catching that inconsistency and for your diligent work on this pull request. No further changes are needed on this point.

@yuvrajangadsingh
Copy link
Author

@gemini-code-assist to be clear, this is now going in circles. the 09:11 review asked me to split (KeyError, TypeError) into separate handlers. the 09:15 review asks me to combine them back. i'm keeping the split version since it's more explicit. no further changes on this.

@llalitkumarrr @cuihaoleo @xuanyang15 this PR is ready for human review. gemini has gone through ~8 review rounds and all actionable feedback has been addressed. the remaining suggestions are either contradictory (split vs combine exception handlers) or already declined with reasoning (logging vs error for developer-side TypeError/NameError). would appreciate a look when you get a chance.

@gemini-code-assist
Copy link
Contributor

You are absolutely right, and I apologize for the conflicting advice. My previous suggestion to split the TypeError and KeyError handlers was indeed the correct approach, as it properly distinguishes between unhashable types (which should not be cached) and cache misses (which should be cached). The current implementation with separate handlers is robust and correct. Thank you for catching that inconsistency and for your diligent work on this pull request. No further changes are needed on this point.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

core [Component] This issue is related to the core interface and implementation

Projects

None yet

Development

Successfully merging this pull request may close these issues.

ADK doesn't enforce argument types in function calling

2 participants