fix: handle circular JSON Pointer $ref in dereference_refs#3896
fix: handle circular JSON Pointer $ref in dereference_refs#3896lawrence3699 wants to merge 1 commit intoPrefectHQ:mainfrom
Conversation
🤖 Generated with Claude Code
There was a problem hiding this comment.
Pull request overview
Fixes a crash in dereference_refs when encountering circular $ref expressed as JSON Pointer paths (common in .NET System.Text.Json-generated schemas), aligning behavior with the existing fallback used for $defs-based cycles.
Changes:
- Catch
RecursionErrorduringjsonref.replace_refsand fall back toresolve_root_ref()instead of crashing. - Add a regression test covering circular JSON Pointer
$ref(non-$defs) to ensuredereference_refsdoesn’t raise and preserves the circular reference.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
src/fastmcp/utilities/json_schema.py |
Expands exception handling to include RecursionError and triggers the same fallback behavior used for circular $defs schemas. |
tests/utilities/test_json_schema.py |
Adds a regression test reproducing the JSON Pointer circular $ref case seen from .NET schema generators. |
| # The circular $ref should be preserved (not inlined) | ||
| children_items = result["properties"]["nodes"]["items"]["properties"][ | ||
| "children" | ||
| ]["items"] | ||
| assert "$ref" in children_items |
There was a problem hiding this comment.
The regression test only asserts that a $ref key exists in children_items, but it doesn’t verify that the pointer-style circular reference is preserved exactly (e.g. that the value remains #/properties/nodes/items). Strengthen the assertion to check the $ref value so the test will fail if dereferencing rewrites or partially inlines the reference.
| # The circular $ref should be preserved (not inlined) | |
| children_items = result["properties"]["nodes"]["items"]["properties"][ | |
| "children" | |
| ]["items"] | |
| assert "$ref" in children_items | |
| # The circular $ref should be preserved exactly (not inlined or rewritten) | |
| children_items = result["properties"]["nodes"]["items"]["properties"][ | |
| "children" | |
| ]["items"] | |
| assert children_items["$ref"] == "#/properties/nodes/items" |
| # Self-referencing/circular schemas can't be fully dereferenced. | ||
| # RecursionError covers circular $ref using JSON Pointer paths | ||
| # (e.g. "#/properties/nodes/items") that bypass $defs-based cycle | ||
| # detection — common in schemas from .NET/System.Text.Json. | ||
| # Fall back to resolving only root-level $ref (for MCP spec compliance) | ||
| return resolve_root_ref(schema) |
There was a problem hiding this comment.
The exception-block comment says the fallback “resolv[es] only root-level $ref”, but resolve_root_ref() is a no-op when there is no root $ref (as in the JSON Pointer cycle case). Consider rewording this comment to reflect the actual behavior: fall back to resolve_root_ref() (which may leave the schema unchanged) rather than implying root refs will always be resolved.
|
Thanks @lawrence3699 — clean diagnosis and nicely scoped fix. Root cause is right: One small ask before merging — Copilot's first inline suggestion is worth taking: the regression test currently asserts assert children_items["$ref"] == "#/properties/nodes/items"catches any future regression where we might accidentally rewrite the pointer during fallback. One-line change, strictly better. Optional (feel free to skip) — the linked issue notes that .NET/ Copilot's second comment (about the existing "resolving only root-level $ref" wording) is a fair observation but it's about a pre-existing comment, not something this PR introduced — out of scope here. Once the test assertion is tightened I'll get this merged. |
jlowin
left a comment
There was a problem hiding this comment.
See my comment above — one small ask: strengthen the regression test assertion to check the pointer value is preserved verbatim (assert children_items["$ref"] == "#/properties/nodes/items"). Optional second test for non-circular JSON Pointer $ref would be nice but not blocking.
The core fix is correct and merge-ready once the assertion is tightened.
Closes #3893.
dereference_refscrashes withRecursionErrorwhen schemas contain circular$refusing JSON Pointer paths (e.g.#/properties/nodes/items) instead of$defs-based paths. This is the format emitted by .NET/C# MCP servers usingSystem.Text.Json. The existing_defs_have_cyclescheck only detects cycles within$defs, so pointer-style cycles reachjsonref.replace_refsuncaught.The fix catches
RecursionErroralongsideJsonRefErrorand falls back toresolve_root_ref, matching the existing behavior for$defs-based circular schemas. A regression test verifies the crash is gone and the circular$refis preserved in the output.🤖 Generated with Claude Code