fix(llm): apply tool defaults across all schema shapes; keep response formats sentinel-free#6444
fix(llm): apply tool defaults across all schema shapes; keep response formats sentinel-free#6444u9g wants to merge 11 commits into
Conversation
| # Treat any parameter with a default value as optional. If the parameter’s type doesn't | ||
| # support None, the default will be used instead. | ||
| t = json_schema.get("type") | ||
| if isinstance(t, str): | ||
| json_schema["type"] = [t, "null"] | ||
|
|
||
| elif isinstance(t, list): | ||
| types = t.copy() | ||
| if "null" not in types: | ||
| types.append("null") | ||
|
|
||
| json_schema["type"] = types |
There was a problem hiding this comment.
Can you verify it doesn't break existing behaviour? I think the reasoning behind that is that users using default values on parameters don't want to require the LLMs to generate a value for it
There was a problem hiding this comment.
Updated in 80a6538. I kept the existing default-as-null schema behavior, so the LLM is still not required to generate a value it cannot know. The new validate_response_format helper recursively replaces null with the Pydantic default before validation, while preserving null for fields whose declared type is actually nullable. The focused response-format and tool suites pass (101 tests).
There was a problem hiding this comment.
It seems that validate_response_format is not used anywhere, except tests?
There was a problem hiding this comment.
Sorry this was an LLM comment, after this comment was sent, I added to claude code a manual guard to never autosend comments without human approval. Me and theo chatted offline. The idea that we agreed on is that we will make the schema for defaults be type | null and then if the model gives null then we will put in the default value.
There was a problem hiding this comment.
The intent is clear, thanks! My comment was about validate_response_format being defined, but not used — it's in the PR changes, not just comment above.
P.S. I should add such guard as well — Claude has also needlessly commented once on my behalf.
There was a problem hiding this comment.
You are right. This PR was hanging on the assumption that people would call a different function to normalize the nulls out of their structured outputs into the defaults. We no longer do that.
b960f6c to
2ba5b76
Compare
u9g
changed the title
…ema_defaults The helper is not specific to response formats; prepare it for reuse by tool argument preparation.
Tool argument preparation only replaced null with the parameter default for top-level parameters, so a defaulted non-nullable field inside a nested model argument failed validation. Reuse the recursive _inject_schema_defaults pass (shared with structured responses) instead of walking the signature. A null for a required parameter with no default is now rejected by pydantic validation instead of a hand-written ValueError; both surface as ToolError.
_json_schema_matches evaluated anyOf variants against the raw pydantic schema, where defaulted fields are optional and unknown keys are ignored, so a payload written for one variant could match a sibling variant whose required keys happened to be present. Pydantic then validated the wrong model and silently dropped the extra field. The strict wire schema closes objects (additionalProperties: false), so payload keys identify the variant. Mirror that in the matcher: a key outside a variant's declared properties rules the variant out, unless the schema explicitly opens the object (extra=allow models, dict values).
_inject_schema_defaults only recursed through properties and items, so null sentinels under additionalProperties (dict[str, Model]) or prefixItems (fixed tuples) survived to validation and were rejected, even though the strict wire schema advertises them as nullable via $defs. Recurse into both container shapes: properties wins for declared keys with additionalProperties covering the rest, and prefixItems covers positional slots with items as the tail.
_json_schema_allows_null returned true if any of type/anyOf/oneOf/$ref mentioned null, but json schema keywords compose conjunctively: null is a valid instance only when every constraint present accepts it. The disjunction overwrote genuine nulls with defaults when nullability was expressed through enum membership (Literal["x", None]), and missed enum/const/allOf entirely. Rewrite as a conjunction that defaults to true and rejects null when any keyword excludes it. Empty (Any) schemas now count as nullable, so a defaulted Any field keeps a genuine null instead of the default.
The strict encoder only appended "null" to a bare type, so defaulted fields whose schema carried a const/enum (Literal), a top-level anyOf (union), or a $ref (nested model / enum) either contradicted themselves (null still rejected by const/enum) or never advertised the sentinel at all. The LLM was then forced to invent a value it could not know. Replace the type-append with _add_null_sentinel, which adds a null branch to a union, appends null to a plain type without value constraints, and otherwise wraps the schema in an anyOf null branch so null is a valid instance without violating the original constraints. Pop title/discriminator first so a wrapped schema doesn't retain them.
Response formats are decoded by users with plain pydantic validation, so the wire schema must be exactly what validation accepts. Drop the null sentinel from to_openai_response_format: defaulted fields stay required and non-nullable, and the model produces their values. Tool schemas keep the sentinel via to_strict_json_schema's new null_sentinel_for_defaults flag, since _prepare_function_arguments resolves it before validation. validate_response_format is removed: with a faithful response schema there is nothing to resolve.
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
A documented tool param with a nested-model or enum default arrives as
{"$ref", "default", "description"}. The null-sentinel wrap ran before
the $ref unravel and moved the whole schema into the anyOf variant,
leaving a $ref with a sibling description, which strict mode rejects.
Keep siblings on the wrapper (allowed next to anyOf) and the ref alone
inside.
u9g
changed the title
3 participants
Summary
Strict JSON schemas require every property and don't support the
defaultkeyword. This PR settles how defaulted fields are handled on each of the two encode paths.Tool schemas keep the null sentinel, and it now works for every schema shape. A defaulted field additionally accepts
null, meaning "use the Python default" — the LLM never has to invent a value it can't know. This is safe because the framework controls the decode:_prepare_function_argumentsresolves the sentinel before validation.Literal(const/enum), unions (anyOf), and$ref-typed defaults (nested models, enums) previously either contradicted themselves or never became nullable.dict[str, Model]values, and tuple items are decoded (previously a top-level-only signature walk).properties, matching the closed-object semantics of the strict wire schema — prevents a payload meant for variant B from silently matching variant A and dropping data.Literal["x", None]) are preserved instead of being replaced with the default.$ref-typed field (e.g. a documented nested-model param), sibling keys likedescriptionstay on the wrapper — strict mode forbids siblings on$refbut allows them next toanyOf.Response format schemas get no sentinel.
to_openai_response_formatoutput is decoded by users with plain Pydantic validation, so the wire schema must be exactly what validation accepts. Defaulted fields stay required and non-nullable; the model produces their values and the Python default never fires.to_strict_json_schematakes anull_sentinel_for_defaultsflag (tools pass it; response formats don't).Behavior changes
response_format: the model can no longer sendnullfor a defaulted non-nullable field. Previously bare-type defaulted fields were widened to["type", "null"], producing payloads that plainmodel_validaterejects.nullfor a required parameter with no default now surfaces as a PydanticValidationError(still wrapped inToolError) instead of a hand-writtenValueError.Known limitation
Field(default_factory=...)emits no schemadefault, so factory-defaulted tool fields get no sentinel and remain required on the wire. Left for a follow-up.Testing