Repository navigation
fix(openapi): keep media-type $defs beside component schemas - #5709
Conversation
An OpenAPI 3.1 media-type schema may declare its own $defs and reference them with #/$defs/... . When components/schemas was non-empty, the combined input schema dropped those inline definitions and the output schema replaced them with the component-derived ones, so the media-type schema's references dangled and tool calls failed validation with PointerToNowhere even though the API returned a valid body. Merge the media-type schema's own $defs with the component definitions instead: the body schema's $defs join the combined input schema, and the response schema's $defs survive beside the extracted output schema's component definitions. Refs inside those inline definitions are rewritten to #/$defs/... like every other ref.
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
This comment has been minimized.
This comment has been minimized.
marvin-context-protocol
Bot
added
bug
![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: 0d15b2d6ca
ℹ️ 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".
| } | ||
| # Add schema definitions if available | ||
| schema_defs = route.request_schemas | ||
| schema_defs = {**route.request_schemas, **body_defs} |
There was a problem hiding this comment.
$defs reuse a name
When an OpenAPI 3.1 request body declares $defs.Item while a body or non-body parameter references the distinct #/components/schemas/Item, both references are normalized to #/$defs/Item, but this right-biased merge installs only the inline definition. Even an unused inline definition now makes the generated input schema validate the component-backed value against the wrong type, so valid tool calls can be rejected (or invalid payloads admitted); collisions need separate generated names with the corresponding references rewritten.
Useful? React with 👍 / 👎.
| output_schema["$defs"] = processed_defs | ||
| # Same-named inline $defs win: the schema's #/$defs/... references | ||
| # are local to it, not to the component schemas. | ||
| output_schema["$defs"] = {**processed_defs, **inline_defs} |
There was a problem hiding this comment.
$defs reuse a name
When an OpenAPI 3.1 response schema declares $defs.Item but an output property references the distinct #/components/schemas/Item, the component reference has already been normalized to #/$defs/Item, and this merge replaces its processed component definition with the inline one. Merely adding an unused same-named inline definition therefore changes output validation and can reject a valid API response; the two source namespaces must remain distinguishable rather than unconditionally letting the inline entry win.
Useful? React with 👍 / 👎.
BrickerP
left a comment
BrickerP
left a comment
There was a problem hiding this comment.
Review
Confirmed the root cause from #5708 and that this fix addresses it.
Root cause. _replace_ref_with_defs rewrites #/components/schemas/X → #/$defs/X but does not walk a media-type schema's own $defs. On main, the combined input schema took $defs only from route.request_schemas, and extract_output_schema_from_responses did output_schema["$defs"] = processed_defs, discarding any inline definitions. Once those are gone, #/$defs/... refs in the advertised tool schema resolve against a root that no longer has them → PointerToNowhere on output validation even when the HTTP body is fine.
Input path. Collecting body_defs from the body schema and merging {**route.request_schemas, **body_defs} is the right shape. Applying _replace_ref_with_defs to each inline def is necessary because the earlier pass on body_schema does not recurse into $defs. Merge order (inline wins) matches how those local #/$defs/... refs were authored. Also correctly handles empty request_schemas + non-empty body $defs, since if schema_defs: still fires.
Output path. Capturing inline_defs before the non-object wrap is important: after wrap, $defs would sit under properties.result while #/$defs/... still resolves at the document root. Merging {**processed_defs, **inline_defs} restores both namespaces at the root for the reported case (non-empty components).
Tests. The new file covers the MRE well: input + output unit paths, ref rewriting inside inline defs, parser/flat_param_schema, and an end-to-end call_tool that would have failed with PointerToNowhere. Green CI across the matrix matches that.
Non-blocking caveats
-
Output / empty components + wrap. The merge is still gated on
if schema_definitions:. If there are no component schemas and the response schema is non-object (so it gets wrapped),inline_defsis captured but never reattached at the root — same dangling-ref failure, different trigger. Input path already handles the empty-components case via{**request_schemas, **body_defs}. Worth a small follow-up: always set root$defsfrominline_defs(andprocessed_defswhen present), especially after wrap. Not in the #5708 MRE. -
Name collisions. Flat
#/$defs/<name>collapses the component and media-type namespaces. Same-named inline vs component defs: inline wins by design here, which is correct for local#/$defs/...refs but can re-point a rewritten#/components/schemas/<same>ref at the wrong definition. Disambiguation (generated names + ref rewrite) would be a larger change; calling it out as a known edge rather than a blocker for this bugfix. No collision coverage in the new tests. -
Coverage gaps (optional). Non-object response wrap with inline
$defs, and an explicit same-name collision case, would lock in the caveats above.
Looks good to merge for the reported bug. Thanks for the clear repro, the before/after pytest notes, and the scoped change.
|
Thanks for the thorough review! Caveat 1 is fixed in c24b4f8: |
(cherry picked from commit ccc8602)
Description
Fixes #5708
An OpenAPI 3.1 media-type schema can declare its own
$defsand reference them with#/$defs/.... Whencomponents/schemaswas non-empty, the generated tool schema dropped those inline definitions: the input schema's$defscame fromroute.request_schemasalone, and the output schema's$defswere overwritten with the component-derived ones. The media-type schema's#/$defs/...references then dangled, so a tool call failed with jsonschema'sPointerToNowherewhen the result was validated against the advertised output schema, even though the API returned a valid body.The media-type schema's own
$defsnow merge with the component definitions on both the input and the output path.Why
In
fastmcp_slim/fastmcp/utilities/openapi/schemas.py:_combine_schemas_and_map_paramsassignedschema_defs = route.request_schemas(pre-fixschemas.py:716), never consulting the request body schema's own$defs.extract_output_schema_from_responsesassignedoutput_schema["$defs"] = processed_defs(pre-fixschemas.py:913), replacing whatever$defsthe response media-type schema carried.#/$defs/...references resolve against the schema root, so once the inline definitions are absent from the root$defs, they dangle.Changes
_combine_schemas_and_map_paramscollects the body schema's own$defs(schemas.py:538) and merges them withroute.request_schemas(schemas.py:726). Refs inside those inline definitions are rewritten to#/$defs/...like every other definition.extract_output_schema_from_responsescaptures the response schema's own$defsbefore the non-object wrapping hides them one level down (schemas.py:900) and merges them with the component-derived definitions (schemas.py:935); same-named inline definitions win, since the schema's#/$defs/...references are local to it.Testing
uv run pytest tests/utilities/openapi/test_inline_defs.py→ on unpatchedmain:6 failed(5 ×KeyError: 'Extra'; the tool-call test fails withPointerToNowhere: '/$defs/Extra' does not exist ...); with the fix:6 passeduv run pytest tests/utilities/openapi→293 passeduv run pytest tests/server/providers/openapi→470 passeduv run pytest -n auto→9382 passed, 9 skipped, 16 xfailed; 10 failures in proxy/connection-error tests, each reproduced identically with the changed file reverted tomain(pre-existing, unrelated)uv run prek run --all-files→ prettier, ruff check, ruff format, ty check, codespell, gitleaks all passExtrain both schemas'$defsContribution type
Checklist
uv run prek run --all-filesand all checks pass