Skip to content

fix(model/openaimodel): pin function tools to non-strict validation - #1389

Open
hanorik wants to merge 1 commit into
google:mainfrom
hanorik:fix/openaimodel-strict-tools
Open

fix(model/openaimodel): pin function tools to non-strict validation#1389
hanorik wants to merge 1 commit into
google:mainfrom
hanorik:fix/openaimodel-strict-tools

Conversation

@hanorik

@hanorik hanorik commented Aug 22, 2026

Copy link
Copy Markdown
Contributor

Problem:

convertFunctionDeclaration never set FunctionToolParam.Strict. openai-go
tags that field json:"strict,omitzero", so leaving it unset drops the key from
the request body entirely, and the Responses API reads an absent strict as
"attempt strict mode; if the schema cannot be made compatible, fall back to
non-strict, best-effort function calling". The validation mode a tool runs under
was therefore inferred by the server from the shape of whatever parameter schema
the caller happened to produce, rather than chosen by this package.

That makes the mode inconsistent and invisible. Tools built with
functiontool.New get additionalProperties:false and a full required list
from jsonschema-go, so they ran strict; tools declared with a hand-written
genai.Schema emit no additionalProperties and never did. Adding omitempty
to a single Go struct field silently flipped that tool from validated to best
effort. The API reports the mode it settled on on the response tool, which this
package discards, so there was no way to tell which you got.

Solution:

Pin Strict to false, making the mode this package's decision rather than an
emergent property of the caller's struct tags. This matches adk-python's
Responses path. Pinning it on is the breaking direction: strict requires every
property to appear in required, so an optional argument would have to be
declared nullable ("type": ["string", "null"]) and listed there anyway,
changing what existing agents send. The trade is that tools whose schema already
qualified lose strict validation and drop to best effort; the package doc and
the conversion site record this and tell callers to validate arguments in the
tool.

Testing Plan

Unit Tests: added TestConvertFunctionDeclarationPinsStrictOff (all three
parameter paths), TestConvertFunctionDeclarationMarshalsStrict (guards the
omitzero wire format), TestConvertFunctionDeclarationKeepsOptionalParameters
(pins that enforceStrictOpenAISchema is not applied to tool parameters),
TestBuildOpenAIParams_ToolsPinStrictOff (asserts on the marshalled request
body), plus a strict assertion in TestConvertTools.

$ go test -race -count=1 ./model/openaimodel/
ok  google.golang.org/adk/v2/model/openaimodel  1.391s

@hanorik
hanorik requested review from a team and karolpiotrowicz August 22, 2026 21:09
@karolpiotrowicz

Copy link
Copy Markdown
Contributor

The change is right, and there is a measurement that makes a stronger case for it than the description does.

I ran the three-way comparison against the live Responses API. With strict absent the API does not fall back to non-strict for a schema that has an optional argument. It rewrites the caller's required to list every property and runs strict anyway:

$ curl -sS https://api.openai.com/v1/responses \
    -H "Authorization: Bearer $OPENAI_API_KEY" -H "Content-Type: application/json" \
    -d '{"model":"gpt-4o-mini","max_output_tokens":64,
         "input":"What is the weather in Paris? Do not specify units.",
         "tool_choice":{"type":"function","name":"get_weather"},
         "tools":[{"type":"function","name":"get_weather","description":"d",
           "parameters":{"type":"object",
             "properties":{"city":{"type":"string"},
                           "units":{"type":"string","description":"optional"}},
             "required":["city"],"additionalProperties":false}}]}'

strict absent  ->  echoed strict=true   echoed required=["city","units"]   arguments {"city":"Paris","units":""}
strict: false  ->  echoed strict=false  echoed required=["city"]           arguments {"city":"Paris"}
strict: true   ->  HTTP 400 invalid_function_parameters:
                   "'required' is required to be supplied and to be an array
                    including every key in properties. Missing 'units'."

So the pre-change behavior was not only that the mode was chosen by the server and invisible. For any tool with an optional argument the API forced a value into that argument: the empty string in units is the model being made to fill a field the prompt told it to leave out. Setting strict: false at tools.go:110 removes that, which is worth saying in the description — it makes this a bug fix rather than a consistency argument, and it corrects the premise there that the API falls back whenever the schema cannot be made compatible. For this schema it did not fall back.

One thing I would fix before merge. TestConvertFunctionDeclarationKeepsOptionalParameters is the test that pins "do not rewrite required", and it exercises only the genai.Schema branch — it passes Parameters:, never ParametersJsonSchema:. But functiontool.New populates ParametersJsonSchema at function.go:166, so every tool built the idiomatic way takes the other branch, which the test does not reach. I checked the other ParametersJsonSchema fixtures in the package and none of them would catch it either — each one either has no properties or already lists every property in required. Adding enforceStrictOpenAISchema(paramsMap) to that branch leaves the whole suite green.

Smaller: the exception list at doc.go:28-29 reads as closed — "unless the argument struct uses omitempty or omitzero" — and there is a third case. A map-typed field emits additionalProperties as a schema rather than false on the nested object, so it is strict-incompatible too:

Tags map[string]string  ->  "tags":{"additionalProperties":{"type":"string"},"type":"object"}

The rest of the doc claim holds, for what it is worth: I ran each declaration shape through convertFunctionDeclaration and all-required structs, pointer-optional structs, nested structs and slice fields do emit strict-compatible schemas, so the validation those tools lose is real.

@hanorik
hanorik force-pushed the fix/openaimodel-strict-tools branch from 907ce14 to 8daa97f Compare August 24, 2026 15:31
@karolpiotrowicz

Copy link
Copy Markdown
Contributor

The test fix holds up. I injected enforceStrictOpenAISchema(paramsMap) into the ParametersJsonSchema branch and the new case fails on it with required = [city units], want [city]. Restoring the previous version of the file, that same mutant passes, and so does the rest of the suite. Omitting units from the fixture's required is what makes it work, and the comment saying so is worth keeping.

One correction on the new comment text, though. tools.go:81-82 says genai.Schema emits "nullable": true, "which strict rejects". It does emit that — I ran a Nullable: true schema through convertFunctionDeclaration and got "units":{"nullable":true,"type":"string"} — but strict accepts it and ignores it rather than rejecting it. Sent with additionalProperties:false and both properties in required, so the nullable form is the only variable:

strict:true, units = {"nullable":true,"type":"string"}   -> accepted, arguments {"city":"Paris","units":""}
strict:true, units = {"type":["null","string"]}          -> accepted, arguments {"city":"Paris","units":null}

Same prompt as before, "What is the weather in Paris? Do not specify units.", tool call forced, gpt-4o-mini. I did not try other models, so I only know this holds there.

Your conclusion is unaffected and I think it gets stronger: you still cannot express an optional argument through genai.Schema under strict, so pinning on would still break those callers. The failure just is not a 400 they would notice — it is the same silent forced empty value the rest of this change is about.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants