fix: populate required fields in FunctionDeclaration json_schema fallback#4812
fix: populate required fields in FunctionDeclaration json_schema fallback#4812giulio-leone wants to merge 2 commits intogoogle:mainfrom
Conversation
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
|
Response from ADK Triaging Agent Hello @giulio-leone, thank you for creating this PR! Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). You can find more information at https://cla.developers.google.com/. Thanks! |
|
Hi @giulio-leone , Thank you for your contribution! It appears you haven't yet signed the Contributor License Agreement (CLA). Please visit https://cla.developers.google.com/ to complete the signing process. Once the CLA is signed, we'll be able to proceed with the review of your PR. Thank you! |
cd48cd4 to
ed192a1
Compare
|
@rohityan Thanks for the heads-up! I'll get the Google CLA signed. Will follow up once it's done. |
|
I have signed the Google CLA. Could you please re-run the CLA check? Thank you! |
16dd249 to
9002a03
Compare
|
Hi @rohityan — the CLA is now signed and passing ✅ (it was a |
9d5c8cc to
8643f46
Compare
|
Hi @giulio-leone , our PR has been received by the team and is currently under review. We will provide feedback as soon as we have an update to share. |
|
Hi @Jacksunwei , can you please review this. LGTM |
|
Rebased this branch onto the latest Local validation I ran:
|
54906af to
2c8f536
Compare
…back When _parse_schema_from_parameter raises ValueError for complex union types (e.g. list[str] | None), from_function_with_options falls back to the parameters_json_schema branch. This branch was missing two things: 1. The _get_required_fields() call to populate declaration.parameters.required 2. Default value propagation from inspect.Parameter to Schema.default/nullable Without these, the LLM sees all parameters as optional and may omit required ones. This fix: - Adds _get_required_fields() to the elif branch (mirrors primary path) - Propagates non-None defaults to schema.default - Sets schema.nullable=True for parameters defaulting to None Includes regression test with list[str] | None parameter type. Fixes #4798
Clear the pyink failure on PR #4812 after validating the json_schema fallback behavior with targeted pytest and a runtime reproduction script.
2c8f536 to
71720a0
Compare
|
Refreshed this branch onto current Validation (two consecutive clean passes, no code changes between passes):
Direct runtime proof for the actual fallback case from #4798 using: complex_tool(query: str, mode: str = "default", tags: list[str] | None = None)
Worktree stayed clean after pass 2. |
✅ Autonomous Validation — Pass 2/2 CleanBranch: Test Results (double-pass, no code changes between passes)
Runner: Validated autonomously — double-pass strict protocol |
Clear the pyink failure on PR google#4812 after validating the json_schema fallback behavior with targeted pytest and a runtime reproduction script.
Summary
Fixes #4798 —
requiredfields lost inFunctionDeclarationwhen theparameters_json_schemafallback path is used.Root Cause
from_function_with_options()has two code paths for building function declarations:parameters_properties): calls_get_required_fields()✅parameters_json_schema): triggered when_parse_schema_from_parameterraisesValueErrorfor complex union types (e.g.list[str] | None) — never called_get_required_fields()❌Additionally, the fallback path didn't propagate
defaultvalues frominspect.Parameterto the generatedSchema, so_get_required_fields()(which checksschema.default is None) would treat defaulted parameters as required.Impact
The LLM sees all parameters as optional and may omit required ones. Observed in production: Gemini Flash omitted the mandatory
queryparameter when calling a tool, because the schema had norequiredfield.Fix
Three changes to the
elif parameters_json_schemabranch infrom_function_with_options():_get_required_fields()callrequiredfield (mirrors primary path)schema.default_get_required_fields()correctly excludes defaulted paramsschema.nullable=TrueforNone-default params_get_required_fields()correctly excludes nullable paramsTest
Added regression test
test_required_fields_set_in_json_schema_fallbackthat verifies:query(no default) → required ✅mode(default='default') → not required ✅tags: list[str] | None = None→ not required ✅Full test suite: 4726 passed, 0 failures