fix: reject free-form objects in strict schemas instead of silently emptying them - #4277
fix: reject free-form objects in strict schemas instead of silently emptying them#4277abhay-codes07 wants to merge 9 commits into
Conversation
![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: aed496e006
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
seratch
left a comment
seratch
left a comment
There was a problem hiding this comment.
Thanks for the contribution. The underlying issue is valid, but the current presence-based object-shape whitelist still silently changes schema meaning. Please revise the conversion so an object that has no properties declared at that object level and no explicit additionalProperties: false falls back to non-strict mode after any already-supported normalization. Do not treat anyOf, multi-branch allOf, enum, or const as sufficient evidence that the wrapper can be closed.
Please also preserve whether MCP properties were declared before MCPUtil adds its synthetic properties: {}, so a root free-form MCP schema takes the same fallback. Add regression coverage for a root MCP object, a composed wrapper, and a non-empty object enum/const, asserting that their original non-strict meaning is preserved.
|
Thanks, both points were right and both are fixed in 7bc786e. The whitelist did still change meaning. I checked the cases you named before changing anything, and each one accepted a value before conversion and rejected it after:
The rule is now the one you described: an object is closed only when it declares To keep the already-supported normalizations working, the check moved to the end of the conversion instead of running before the composition keywords are handled. A
The synthetic
Regression coverage for the three you asked for, plus the normalization cases so the fallback cannot quietly widen later:
Ten of these fail on the previous revision and pass now. I also removed the test from that revision which asserted
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7bc786e19e
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
The automated review found three real regressions in the previous revision. All three reproduced, and all three are fixed in 92f9da1. 1. A redundant single-entry {"type": "object", "properties": {"a": {"type": "string"}}, "allOf": [{"type": "object"}]}The bare 2. The MCP
The shim is restored after a successful conversion, which is only reachable when the server already closed the root. 3. Typeless roots were exposed to the object-only check. The same pop meant an annotation-only schema never reached the new branch and was served strict without an object envelope:
Only a root that declares The free-form behaviour you asked for is unchanged by these fixes:
Three new tests, each failing on the previous revision:
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 92f9da1fbc
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
An object schema that declares no `properties` and no `additionalProperties`
accepts arbitrary keys. Strict conversion defaulted it to
`additionalProperties: false`, which narrows it to "the empty object is the
only valid value". The tool is then served to the model as strict with a
parameter that can never carry any content, and nothing reports a problem.
{"keysAndValues": {"type": "object", "description": "key/value pairs"}}
became
{"keysAndValues": {"type": "object", "description": "...",
"additionalProperties": false}}
so `{"visible": false}` is rejected and only `{}` validates.
This is inconsistent with how the same intent is handled elsewhere. A Python
tool annotated `dict[str, Any]` produces `additionalProperties: true`, which
already raises UserError. Only schemas that arrive without the keyword, which
is the common shape from MCP servers and hand-written schemas, were silently
narrowed.
Treat a free-form object the same way: raise UserError. MCP tool conversion
already falls back to serving the original schema as non-strict when strict
conversion fails, so those tools go back to accepting arbitrary keys. Function
tools and output types now get an actionable error rather than a tool that
silently cannot receive data.
An object is only considered free-form when it carries none of the keywords
that constrain its contents, so `properties: {}` (a no-argument tool), an
explicit `additionalProperties: false`, and objects shaped by `$ref`, `allOf`,
`anyOf`, `patternProperties` and friends are unaffected.
This matches the symptom reported in openai#1681, where nested object arguments were
flattened by the model. The schema it received required that parameter to be
empty.
Addresses review feedback. The previous revision decided free-form-ness
from a whitelist of keywords that were taken as evidence the object could
be closed. That still changed schema meaning: an object whose contents are
described by `anyOf`, a multi-branch `allOf`, `enum` or `const` has no
`properties` map at its own level, so adding `additionalProperties: false`
rejected the very values those branches describe.
{"type": "object", "anyOf": [{"properties": {"a": {"type": "string"}}}]}
accepted {"a": "x"} before conversion and rejected it after. The same held
for multi-branch allOf, object enum, object const, patternProperties and
propertyNames.
An object is now closed only when it declares `properties` at its own level
or already carries an explicit `additionalProperties: false`. Everything
else falls back to non-strict. The check moved to the end of the conversion
so the existing normalizations still run first: a `$ref` and a single-entry
`allOf` both lift `properties` up to this level and re-enter the function,
so those keep converting to strict as before.
Also stop MCP's synthetic `properties: {}` from masking a free-form root.
That key is added because the OpenAI spec wants one, not because the server
said the tool takes no arguments, so strict conversion now runs against what
the server actually sent. A root of `{"type": "object"}` falls back, while a
server that explicitly declares `properties: {}` still converts to a strict
no-argument tool.
…t-allOf roots
Three follow-ups from automated review on the previous revision, each a
regression that revision introduced.
A single-entry `allOf` branch was converted before being merged into its
parent, so a branch judged in isolation could look free-form even when the
parent already declared the properties. `{"type": "object", "properties":
{"a": ...}, "allOf": [{"type": "object"}]}` became a conversion error, and
therefore a non-strict fallback, instead of converting. The branch is now
merged first and the merged schema re-entered, which is also where the
existing normalization already expected to happen.
Dropping MCP's synthetic `properties: {}` before conversion also dropped it
from the served schema when conversion then succeeded. A server root that is
already closed, such as `{"type": "object", "additionalProperties": false}`,
was served strict without the `properties` key the OpenAI tool schema shape
requires. The shim is restored after a successful conversion.
That same drop exposed typeless roots to the object-only check. An
annotation-only schema such as `{"description": "Test tool"}` passed through
untouched and was served as a strict non-object schema. Only a root that
declares `type: object` is now exposed as free-form; a schema that never
states its type keeps its historical no-argument treatment rather than
having its meaning changed here.
92f9da1 to
6ba6834
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6ba6834a32
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Five more findings from automated review, each a case where the previous
revision rejected or closed a schema it should not have.
Definitions are no longer judged on their own. A `$defs` entry is a
template: a broad base is routinely narrowed by the keys a `$ref` site
supplies, and an unreferenced one has no validation effect at all.
Rejecting there turned ordinary schemas into conversion errors, so
definitions keep the historical behaviour and the check runs at the use
site, where the merged shape is known.
`{"type": "object", "maxProperties": 0}` already forbids every key, so
closing it changes nothing and it now converts instead of being rejected.
An empty `properties` map is no longer read as "this object is empty"
when a keyword beside it describes the contents another way. With
`patternProperties`, `propertyNames`, `enum` and friends the object still
accepts keys, so closing it rejected valid values.
On the MCP side the synthetic `properties: {}` is now always removed
before conversion, not only for roots that declare `type: object`. A root
reached through composition, such as `{"allOf": [{"type": "object"}]}`,
was otherwise closed as a no-argument tool and lost its composed meaning.
Because that also exposes roots that never declare an object at all, a
successful conversion must now produce a closed object envelope; anything
else is served non-strict rather than handing the provider a strict
schema it cannot use.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 88e83e99d6
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Three more findings from automated review.
A bare `$ref` to a free-form definition was still narrowed. Definitions
are exempt from the rejection because they are templates, but the walk
that follows still closed them, so a property referencing one directly
ended up pointing at an object that accepts only `{}`. Definitions that
cannot be closed are now recorded before the walk, and a bare `$ref` to
one is rejected, since it is never inlined and so never re-checked at the
use site.
A bound on the number of keys says arbitrary keys are expected, so
`maxProperties` and `minProperties` now count as describing the contents.
`{"type": "object", "properties": {}, "maxProperties": 1}` accepts one
arbitrary key and was being closed down to the empty object.
An MCP root that is already closed but never declared its type, such as
`{"additionalProperties": false}`, is normalized to an object envelope
rather than treated as a failed conversion. Removing the synthetic
`properties` shim left it without a type, so the new envelope check
rejected a root the previous path handled.
|
All Codex threads are now addressed and resolved. Eight findings across two rounds, each reproduced before being fixed. Definitions are no longer judged in isolation. This was the serious one: the rejection ran while walking
A definition is a template. A broad base is routinely narrowed by the keys a A bare Empty
MCP roots. The synthetic
Sixteen tests now cover this, thirteen of which fail on the preceding commits. Full suite 6404 passed, no new failures against One thing worth saying plainly: this PR has needed four rounds of correction, and every round was a case where my change rejected or narrowed a schema that was previously fine. The blast radius is wider than the one-line bug it started from. If you would rather not carry that risk, closing it is a reasonable call and I will not push back. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7a1bb41260
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
anujbolewar
left a comment
anujbolewar
left a comment
There was a problem hiding this comment.
Good — a closed root that never declared its type is still an object, and normalizing it avoids the empty-schema rejection. Handles the list-of-types case cleanly. One caution: _is_strict_object_root mutates the schema dict in place via the included property assignment, so wrap it expects a freshly copied schema and never shares that dict with a readonly reference — going from deepcopy on the caller it is safe, just worth stating so future callers do not pass a shared ref.
Four more findings from automated review, three of them holes in the
ref-tracking added by the previous commit. The root cause was structural:
the tracked set was a separate parameter, so any recursion site that
forgot to pass it silently lost the information. The registry now lives
on the budget object, which already reaches every recursive call because
the DoS bound depends on it, and records definitions by object identity,
so nested `$defs` need no path bookkeeping.
That closes the two propagation holes: an `allOf` branch that is a bare
`$ref` to a free-form definition now falls back instead of converting
against the closed definition, and a bare `$ref` to a definition nested
under another definition's `$defs` is caught the same way.
A `$ref` with siblings that do not shape the object is also rejected at
the merge site. The merged result inherits the `additionalProperties:
false` the definition walk added, so the final decision cannot see the
problem; the merge site now requires the siblings to declare real
properties, close the object explicitly, or constrain it to the empty
object. `{"$ref": ..., "properties": {}}` and an annotation-only sibling
fall back, while a `$ref` narrowed by real properties keeps converting.
Empty-object literals are recognized as already closed: `const: {}` and
`enum` entries that are all `{}` admit only the empty object, so closing
them preserves meaning exactly, like `maxProperties: 0`.
|
The four remaining Codex threads are fixed in 9e95a48 and resolved. Three of them were holes in the ref-tracking added by the previous commit, and the root cause was structural rather than three separate oversights: the tracked set was its own parameter, so any recursion site that missed it silently dropped the information. The registry now rides on the conversion budget, which already reaches every recursive call because the DoS bound depends on it, and records definitions by object identity, so nested The sibling case is handled at the merge site, since the merged result inherits the
And empty-object literals are now recognized as already closed: Seven new tests, six failing on the previous commit; the seventh pins the real-properties case that must keep converting. I also re-ran the full 23-case behavioural sweep across every scenario raised on this PR so far: all as expected. Full suite 6410 passed, no new failures against |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9e95a489ee
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Four more findings from automated review.
A definition whose interior holds a free-form node at a value position is
now recorded as tainted. The definition carve-out was propagating into
every position inside a definition, so a nested field like
{"x": {"type": "object"}} was silently closed while a `$ref` kept
pointing at it. A sibling merge only reshapes the top level of the
referenced subtree, so no reference to a tainted definition can ever be
made strict; both the bare-ref and merge sites now reject them.
Unreferenced definitions with such interiors stay harmless.
Ref chains are followed before judging a bare reference: an alias
definition that is itself a lone `$ref` is resolved through to its
target, with a seen-set to stop cycles, so aliases to free-form or
tainted definitions no longer slip past the check.
Required names outside the declared properties are rejected. Conversion
overwrote `required` with the property list, so a schema requiring an
undeclared key was silently converted into one that forbids that key.
`dependentRequired` and draft-07 `dependencies` now count as describing
an object's contents, so an empty `properties` map next to them is not
read as "this object is empty".
|
The four round-six threads are fixed in 339c1ce and resolved. Free-form fields inside referenced definitions. The carve-out was propagating into every position inside a definition, exactly as flagged. A definition whose interior holds a free-form node at a value position is now recorded as tainted, and since a sibling merge only reshapes the top level of a referenced subtree, both the bare-ref and merge sites reject references to tainted definitions unconditionally. Alias chains. Undeclared Dependency keywords. Six new tests, five failing on the previous commit; the sixth pins the unreferenced-tainted-definition case that must keep converting. The cumulative behavioural sweep is now 25 cases covering every scenario raised across all six review rounds, all passing. Full suite 6416 passed, no new failures against |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 339c1ce27d
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
One more finding from automated review, plus a latent inverse it implied.
The free-form and tainted registries were populated while the definition
walk visited each entry, which made the outcome depend on declaration
order. An annotated alias appearing before its target, such as
$defs.Outer = {"$ref": "#/$defs/Inner", "description": "d"} declared
ahead of $defs.Inner = {"type": "object"}, was inlined and closed before
Inner was ever recorded, so a bare reference to Outer converted as
strict and accepted only the empty object. The inverse ordering had the
opposite defect: a bare alias declared after its target caused a
conversion failure even when nothing referenced the alias.
Both registries are now precomputed from the untouched tree before
anything is walked or mutated: every definition entry is collected, each
is seeded from what it says on its own, and alias and interior
references are iterated to a fixpoint. Declaration order cannot matter
because nothing has been inlined or closed yet when the registries are
read.
With the registries authoritative up front, the walk-time checks inside
definitions no longer raise; they record the enclosing definition as
tainted instead, so a template is only ever a problem once something
references it, in either order.
|
The round-seven thread is fixed in 08b0094 and resolved, taking the suggestion as stated: the registries are now precomputed order-independently. Before anything is walked or mutated, every definition entry in the tree is collected, seeded from what it says on its own, and alias and interior references are iterated to a fixpoint. Declaration order cannot matter because nothing has been inlined or closed yet when the registries are read. Verifying the reported case and its neighbours:
The fourth row is a latent inverse the walk-time approach also had: order dependence cut both ways, and the reversed declaration wrongly failed a schema whose alias nothing referenced. With the registries authoritative up front, the walk-time checks inside definitions no longer raise at all; they record the enclosing definition as tainted, so a template is only a problem once something references it, in either order. Four new tests, two failing on the previous commit and two pinning the harmless cases. The cumulative sweep is now 30 cases across all seven rounds, all passing. Full suite 6425 passed, no new failures; ruff, full mypy and pyright clean. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 08b0094820
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Three more findings from automated review, plus a reviewer note.
Literal values are no longer mistaken for schema nodes. The precomputed
interior walk descended into `const`, `enum`, `default` and `examples`
values, so a data literal that happened to look like an open schema,
such as `{"const": {"type": "object"}}`, tainted the definition that
contained it and pushed working references to non-strict.
The undeclared-required judgment now waits for pending merges. A node
carrying a `$ref` or a single-entry `allOf` gets more properties merged
onto it and re-enters conversion, so both the required-subset check and
the `required` overwrite are deferred to the merged shape. Previously
`{"properties": {}, "required": ["a"], "allOf": [{"properties": {"a":
...}}]}` was rejected before the branch that supplies `a` could merge,
and the overwrite destroyed the original `required` before it could be
judged at all, which also let a branch that does not supply the key
slip through.
Typeless definitions with a properties map are classified as objects by
the precomputed registries, mirroring the normalization the conversion
walk applies, so `{"properties": {}, "propertyNames": ...}` referenced
bare is caught instead of being closed into an empty object.
Also made `_is_strict_object_root` a pure predicate, per review: the
typeless-root normalization now happens explicitly at the call site, on
the copy that branch owns, instead of as a side effect inside the check.
|
The three round-eight threads are fixed in d4a7134 and resolved, along with the review note about the mutating predicate. Literals are no longer read as schemas. The precomputed interior walk descended into Required judgment waits for pending merges. With a
Typeless definitions with a properties map are classified as objects by the precomputed registries, mirroring the normalization the walk applies, so @anujbolewar on the mutating predicate: agreed, and fixed in the same commit. Seven new tests, five failing on the previous commit and two pinning behaviour that must not change. The cumulative sweep is 36 cases across eight review rounds, all passing. Full suite 6426 passed, no new failures; ruff, full mypy and pyright clean. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d4a7134f4c
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if _is_unclosable_object(entry): | ||
| budget.free_form_definition_ids.add(id(entry)) |
There was a problem hiding this comment.
When a $defs entry is shaped by a single-entry allOf, this precompute step marks the raw definition as free-form before the converter's existing single-allOf merge can lift the branch properties onto it. A schema like {"$defs":{"Base":{"type":"object","allOf":[{"properties":{"a":{"type":"string"}}}]}},"type":"object","properties":{"f":{"$ref":"#/$defs/Base"}}} now raises (or makes MCP fall back non-strict) even though the definition is strictable after normalization; defer this classification until after ref/allOf normalization, or account for that normalization here. Fresh evidence in this revision is that the new registry is populated before the normalization pass runs on definition roots. .agents/references/function-and-output-schema.mdL17-L18
Useful? React with 👍 / 👎.
| declared = json_schema.get("properties") | ||
| if not is_dict(declared): | ||
| return True |
There was a problem hiding this comment.
When a referenced definition has type: "object" plus a $ref sibling that supplies the actual properties, this branch still classifies the raw definition as free-form because it looks only for a local properties map before the later $ref merge runs. For example, Base = {"type":"object","$ref":"#/$defs/S","description":"d"} where S declares property a is converted into a strict object during the definition walk, but a bare $ref to Base is then rejected from the stale registry; account for pending $ref normalization before recording the definition as free-form. .agents/references/function-and-output-schema.mdL17-L18
Useful? React with 👍 / 👎.
|
|
||
| # Keywords whose values are data literals, not schemas. A literal such as | ||
| # `{"const": {"type": "object"}}` must never have its value mistaken for a schema node. | ||
| _LITERAL_VALUE_KEYWORDS = frozenset({"const", "default", "enum", "examples"}) |
There was a problem hiding this comment.
When a referenced definition contains an OpenAPI-style singular example annotation whose value is an object literal, this new precompute walk still descends into that literal because only examples is skipped. For a definition like {"type":"object","properties":{"tag":{"type":"string","example":{"type":"object"}}}}, the literal is mistaken for a free-form schema node, so a bare $ref to the otherwise strictable definition raises or makes MCP fall back non-strict even though example has no validation effect; include example in the literal-keyword skip list or traverse only schema-valued keywords. .agents/references/function-and-output-schema.mdL18-L18
Useful? React with 👍 / 👎.
| # Restore the shape the OpenAI spec wants. Only reachable when the root was | ||
| # already closed by the server, e.g. ``additionalProperties: false``. | ||
| converted["properties"] = {} | ||
| converted.setdefault("required", []) |
There was a problem hiding this comment.
When an MCP root omits properties but is already closed and carries required, such as {"type":"object","additionalProperties":false,"required":["a"]}, the shim is removed before conversion, so ensure_strict_json_schema() returns the undeclared required name untouched. This block then restores properties: {} while setdefault preserves required: ["a"], marking the tool strict with a required key that is not declared in properties; reject this shape or reset required consistently before setting strict_json_schema=True. .agents/references/function-and-output-schema.mdL17-L17
Useful? React with 👍 / 👎.
Summary
An object schema that declares no
propertiesand noadditionalPropertiesaccepts arbitrary keys. Strict conversion defaulted it toadditionalProperties: false, which narrows it to "the empty object is the only valid value". The tool is then handed to the model as strict, with a parameter that can never carry any content, and nothing reports a problem.{"type": "object", "properties": { "target": {"type": "string"}, "keysAndValues": {"type": "object", "description": "key/value pairs"}}}keysAndValuescomes out ofensure_strict_json_schemaas:{"type": "object", "description": "key/value pairs", "additionalProperties": False}Validating candidate values against that:
{}{"visible": false}Additional properties are not allowed{"a": 1, "b": "x"}The parameter is required, and the only value it can hold is
{}.Why this is inconsistent rather than just a strict-mode limitation
The same intent is already handled correctly when it arrives spelled differently. A Python tool annotated
dict[str, Any]makes Pydantic emitadditionalProperties: true, and that path raisesUserErrortoday:Only schemas that arrive without the keyword were silently narrowed. That is the common shape from MCP servers and hand-written schemas, so the loud path covers the case users are least likely to hit and the silent path covers the case they are most likely to hit.
Fix
Treat a free-form object the same way as an explicit
additionalProperties: trueand raiseUserError.That choice is what makes this a real fix rather than a different error. MCP tool conversion already falls back to serving the original schema as non-strict when strict conversion raises:
So affected MCP tools go back to accepting arbitrary keys:
strict_json_schema{}{"visible": false}TrueFalseFunction tools and output types now get an actionable error instead of a tool that silently cannot receive data.
An object counts as free-form only when it carries none of the keywords that constrain its contents, so these are unaffected:
properties: {}, a no-argument tool, still converts to a strict empty objectadditionalProperties: falseis preserved, since that is the caller stating the empty object really is the only valid value$ref,allOf,anyOf,oneOf,patternProperties,propertyNames,enum,constand friends still convert normallyIssue number
Matches the symptom in #1681, where nested object arguments came back flattened. That report concluded the model was ignoring a correct schema. The schema the model received actually required that parameter to be empty, so there was no way to express the nested value.
Test plan
Three tests fail without the source change and pass with it:
test_free_form_object_property_is_rejected_instead_of_silently_emptiedtest_free_form_object_root_is_rejectedtest_free_form_object_arg_falls_back_to_non_strict_schema, which asserts the user-visible outcome, that the MCP tool is served non-strict and keeps accepting arbitrary keysSeven more pin the cases that must keep working, and pass both with and without the change: empty
properties, explicitadditionalProperties: false, objects shaped byallOf/anyOf/patternProperties/enum, and ordinary nested objects still converting to strict.make format/make lintmake mypymain, none in the touched filesmake pyrightuv run pytest tests/test_strict_schema.py tests/mcp/make testsThe full suite was run on Windows, where a set of sandbox symlink and tracing timing tests fail regardless of this change. I diffed the failing set against
mainat the same commit: no new failures. The one test that differed between runs,tests/test_tracing.py::test_simple_tracing, fails 5 out of 5 times on unmodifiedmainwhen run in isolation, so it is part of that pre-existing set rather than something this change introduced.Checks
.agents/skills/code-change-verification/scripts/run.sh/reviewbefore submitting this PRThe verification script is a bash script that shells out to
make. I ran the underlying steps individually instead, with the results above.