fix: populate required fields in FunctionDeclaration json_schema fallback#5000
Closed
giulio-leone wants to merge 2 commits intogoogle:mainfrom
Closed
fix: populate required fields in FunctionDeclaration json_schema fallback#5000giulio-leone wants to merge 2 commits intogoogle:mainfrom
giulio-leone wants to merge 2 commits intogoogle:mainfrom
Conversation
Collaborator
|
/gemini review |
rohityan
added
the
needs review
Collaborator
|
Hi @giulio-leone , Thank you for your contribution! We appreciate you taking the time to submit this pull request. Your 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. |
Collaborator
|
Hi @GWeale , can you please review this. |
Contributor
Author
|
Thanks @rohityan. Quick maintainer note on the current head: the effective PR checks are now The cancelled entry is from the superseded mypy matrix run; the latest Happy to adjust anything else if helpful during review. |
giulio-leone
force-pushed
the
fix/required-fields-json-schema-fallback
branch
from
71720a0 to
d03672d
Compare
giulio-leone
force-pushed
the
fix/required-fields-json-schema-fallback
branch
from
d03672d to
d7f1343
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 google#4798
Clear the pyink failure on PR google#4812 after validating the json_schema fallback behavior with targeted pytest and a runtime reproduction script.
giulio-leone
force-pushed
the
fix/required-fields-json-schema-fallback
branch
from
d7f1343 to
e9783d7
Compare
3 tasks
Collaborator
|
Started internal review process |
copybara-service Bot
pushed a commit
that referenced
this pull request
Apr 13, 2026
…back Merge #5000 Fixes #4798 — `required` fields lost in `FunctionDeclaration` when the `parameters_json_schema` fallback path is used. Co-authored-by: Xuan Yang <xygoogle@google.com> COPYBARA_INTEGRATE_REVIEW=#5000 from giulio-leone:fix/required-fields-json-schema-fallback e9783d7 PiperOrigin-RevId: 899243799
Collaborator
|
Thank you @giulio-leone for your contribution! 🎉 Your changes have been successfully imported and merged via Copybara in commit 9b9faa4. Closing this PR as the changes are now in the main branch. |
9 tasks
sasha-gitg
pushed a commit
that referenced
this pull request
Apr 21, 2026
…back Merge #5000 Fixes #4798 — `required` fields lost in `FunctionDeclaration` when the `parameters_json_schema` fallback path is used. Co-authored-by: Xuan Yang <xygoogle@google.com> COPYBARA_INTEGRATE_REVIEW=#5000 from giulio-leone:fix/required-fields-json-schema-fallback e9783d7 PiperOrigin-RevId: 899243799 Change-Id: I24d85f4f082d7e06f16ebd3684207eb743210d48
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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