fix: populate required fields in FunctionDeclaration json_schema fallback#5000
Open
giulio-leone wants to merge 2 commits intogoogle:mainfrom
Open
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
…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.
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
2 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