fix: force additionalProperties=false on all objects in strict schemas - #3805
fix: force additionalProperties=false on all objects in strict schemas#3805Bhumika-1432006 wants to merge 4 commits into
Conversation
_ensure_strict_json_schema() only set additionalProperties=False when
the key was missing from the schema entirely:
if typ == "object" and "additionalProperties" not in json_schema:
json_schema["additionalProperties"] = False
Pydantic models with `extra="allow"` (or fields that produce a
`Dict[str, ...]`-shaped schema) already have `additionalProperties`
set - to `True`, or to a nested schema - so this branch never fired
for them, and the resulting schema was sent to the API as-is.
The Responses/Chat Completions APIs require `additionalProperties:
false` on every object in a strict schema, unconditionally. A schema
with `additionalProperties: true` (or a nested schema) is rejected
with a 400: "'additionalProperties' is required to be supplied and
to be false", which surfaces from client.beta.chat.completions.parse()
and client.responses.parse() whenever the response model (or a
nested model) uses `extra="allow"`.
Since there's no schema shape where the API accepts anything other
than `additionalProperties: false`, the fix removes the
`not in json_schema` guard so the value is always normalized,
overriding whatever Pydantic produced.
Fixes openai#2740
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ff9b33e747
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
The previous commit made additionalProperties=False unconditional for
every object, which is correct for extra="allow" models (where
Pydantic sets additionalProperties=True) but wrong for a
Dict[str, ...]-shaped field or mapping RootModel: Pydantic represents
those with a schema describing the values' type, e.g.
{"type": "string"}, not a boolean.
Overwriting that schema with False would silently turn the field into
an object that only accepts {}, changing its declared contract
instead of surfacing the actual limitation - the API has no way to
represent an arbitrary-key mapping in a strict schema.
_ensure_strict_json_schema now only normalizes additionalProperties
when it's already a bool (True -> False, or filling in the missing
key), and raises a TypeError describing the limitation when it's a
schema value.
Addresses the automated review feedback on openai#3805.
|
Good catch — fixed in f9467c2. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f9467c26b6
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
The previous commit treated every additionalProperties=True the same
as extra="allow" and coerced it to False. But True is also what
Pydantic emits for a Dict[str, Any]-shaped field, an Any-valued
mapping RootModel, or a bare extra="allow" model with zero declared
fields - none of which have a fixed set of properties to close the
object around. Forcing False in those cases silently turns the
object into one that only accepts {}, the same silent-contract-change
bug as the schema-valued additionalProperties case fixed previously.
additionalProperties=True is now only corrected to False when the
schema also has at least one declared property (a real extra="allow"
model with named fields, where closing to those fields is a
reasonable strict-schema approximation). Every other non-False value
- a schema, or True with no declared properties - raises the same
TypeError as before, describing the object as accepting arbitrary
keys, which strict schemas can't represent.
Addresses further automated review feedback on openai#3805.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 642214206f
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
…es exist
Three issues from the last round of review:
1. A Pydantic v2 model with extra="allow" and a typed
__pydantic_extra__ (validating the extra values) produces a
schema-valued additionalProperties, e.g. {"type": "integer"},
instead of True - but it still has declared properties to close
the object around, exactly like the untyped extra="allow" case.
The previous check only forgave additional_properties is True,
so this got incorrectly rejected. Simplified the condition to key
only on whether properties are declared, regardless of whether
additionalProperties is True or a schema - which was the correct
discriminator all along and subsumes the True-specific check.
2. The new test's `from pydantic import RootModel` was a module-level
import. RootModel doesn't exist on Pydantic v1, so this raised
ImportError during test collection on that lane, before the
PYDANTIC_V1 skip in the test body ever got a chance to run -
breaking collection for the whole file under
./scripts/test-pydantic-v1. Moved the import inside the test
function, after the skip.
3. test_dict_any_field_raises_instead_of_silently_allowing_only_empty_object
assumed Pydantic v2's `additionalProperties: True` representation
for Dict[str, Any]. Pydantic v1 omits the key entirely for an
Any-valued dict, so _ensure_strict_json_schema takes the
missing-key path there and returns additionalProperties: false
without raising. Added a PYDANTIC_V1 skip.
Added test_typed_extra_is_forced_false_just_like_untyped_extra_allow
covering (1). Full tests/lib/test_pydantic.py: 9 passed.
|
@codex review |
|
Codex Review: Didn't find any major issues. Bravo. Reviewed commit: ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback". |
|
Security review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
1 participant
Summary
to_strict_json_schema()/_ensure_strict_json_schema()insrc/openai/lib/_pydantic.pyis supposed to normalize a Pydantic-generated JSON schema so it satisfies the Responses/Chat Completions strict-schema requirement that every object haveadditionalProperties: false. It only did this when the key was completely absent from the schema:A Pydantic model with
model_config = ConfigDict(extra="allow")already producesadditionalProperties: Truein its generated schema (that's exactly whatextra="allow"means to Pydantic), so this branch never fired for it, and the schema was sent to the API withadditionalProperties: truestill in place.Fixes #2740.
Reproduction (from the issue)
Fix
The discriminator that matters is whether the schema has at least one declared property - i.e. a fixed shape to close the object around:
FalseadditionalPropertiesthere is corrected toFalse. This covers plainextra="allow"(additionalProperties: True) andextra="allow"with a typed__pydantic_extra__(additionalPropertiesis a schema, e.g.{"type": "integer"}) - forbidding keys beyond the declared ones doesn't change what those declared fields accept.Dict[str, ...]-shaped field, a mappingRootModel, aDict[str, Any]-shaped field, anAny-valued mappingRootModel, or a bareextra="allow"model with zero fields. The API can't represent "any key, any/typed value" in a strict schema, so this now raises a clearTypeErrorinstead of silently forcingadditionalProperties: false, which would turn the object into one that only accepts{}- changing its declared contract instead of reporting the real limitation.This can't regress a previously-working case: every schema shape this raises for was already going to be rejected by the API with a 400 before this fix - just with the API's own opaque error at request time, rather than this library's own clearer error before the request is even sent.
Review history
This PR went through several rounds of automated review, each catching a real edge case:
additionalProperties(Dict[str, str], mappingRootModel) - the first version forcedadditionalProperties: Falseunconditionally, silently discarding the value schema. Fixed by raising instead of overwriting non-boolean values.additionalProperties: Truewith no declared properties (Dict[str, Any],Any-valued mappingRootModel, bareextra="allow"with no fields) - indistinguishable from safeextra="allow"-with-fields by value alone. Fixed by keying the decision on declared properties instead of the boolean.extra="allow"+ typed__pydantic_extra__) produce a schema-valuedadditionalPropertieswith declared properties - the fix from (2) still rejected these since it special-casedTrue. Simplified further: the discriminator is declared properties alone, regardless of whetheradditionalPropertiesisTrueor a schema.from pydantic import RootModelin the new test raisedImportErrorduring collection on the v1 lane (RootModeldoesn't exist there), before thePYDANTIC_V1skip in the test body could run - breaking the whole file's collection under./scripts/test-pydantic-v1. Fixed by moving the import inside the test, after the skip.additionalProperties: Truerepresentation forDict[str, Any]; Pydantic v1 omits the key entirely there, taking the missing-key path instead. Added aPYDANTIC_V1skip.Tests
tests/lib/test_pydantic.pynow covers:test_additional_properties_is_forced_false_even_when_extra_allow-extra="allow"with a declared field ->False.test_typed_extra_is_forced_false_just_like_untyped_extra_allow-extra="allow"with a typed__pydantic_extra__(schema-valuedadditionalProperties) and a declared field ->False.test_empty_extra_allow_model_raises_instead_of_forcing_empty_object-extra="allow"with zero declared fields -> raises.test_dict_field_raises_instead_of_silently_dropping_value_schema-Dict[str, str]field -> raises.test_dict_any_field_raises_instead_of_silently_allowing_only_empty_object-Dict[str, Any]field -> raises (skipped on Pydantic v1, which represents this differently).test_mapping_root_model_raises_instead_of_silently_dropping_value_schema-RootModel[Dict[str, str]]-> raises (skipped on Pydantic v1,RootModelimport is local to avoid breaking v1 collection).Full
tests/lib/test_pydantic.pysuite: 9 passed. No existing snapshot changed - none of the current tests exercise a model with a non-boolean or fieldless-TrueadditionalProperties, so this remains a pure bugfix with no behavior change for existing schemas.ruff check,ruff format --check, andmypyare clean on the changed files.Notes for reviewers
This only touches the top-level normalization step in
_ensure_strict_json_schema; the recursive walk intoproperties,items,anyOf,allOf, and$refexpansion is untouched, so nested objects (which already go through this same function recursively) get the same treatment for free.