Dereference $ref in tool schemas for MCP client compatibility#2814
Dereference $ref in tool schemas for MCP client compatibility#2814
Conversation
marvin-context-protocol
Bot
added
bug
|
Warning Rate limit exceeded@jlowin has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 13 minutes and 7 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
WalkthroughThe docs add inline notes stating FastMCP automatically dereferences $ref entries and inlines complex Pydantic models for MCP client compatibility. In code, dereference_refs now inlines $ref targets, preserves sibling keywords (description, default, examples) via a new _merge_ref_siblings helper, and removes $defs after merging. resolve_root_ref was extended to inline root-level $ref entries. compress_schema now calls dereference_refs up-front, removes the public prune_defs parameter, and drives subsequent optimization via prune_titles and prune_additional_properties. Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (6)
pyproject.tomlis excluded by none and included by nonetests/tools/test_tool.pyis excluded by none and included by nonetests/tools/test_tool_transform.pyis excluded by none and included by nonetests/utilities/openapi/test_schemas.pyis excluded by none and included by nonetests/utilities/test_json_schema.pyis excluded by none and included by noneuv.lockis excluded by!**/*.lockand included by none
📒 Files selected for processing (4)
docs/servers/tools.mdxsrc/fastmcp/tools/tool.pysrc/fastmcp/tools/tool_transform.pysrc/fastmcp/utilities/json_schema.py
🧰 Additional context used
📓 Path-based instructions (4)
docs/**/*.mdx
📄 CodeRabbit inference engine (docs/.cursor/rules/mintlify.mdc)
docs/**/*.mdx: Use clear, direct language appropriate for technical audiences
Write in second person ('you') for instructions and procedures in MDX documentation
Use active voice over passive voice in MDX technical documentation
Employ present tense for current states and future tense for outcomes in MDX documentation
Maintain consistent terminology throughout all MDX documentation
Keep sentences concise while providing necessary context in MDX documentation
Use parallel structure in lists, headings, and procedures in MDX documentation
Lead with the most important information using inverted pyramid structure in MDX documentation
Use progressive disclosure in MDX documentation: present basic concepts before advanced ones
Break complex procedures into numbered steps in MDX documentation
Include prerequisites and context before instructions in MDX documentation
Provide expected outcomes for each major step in MDX documentation
End sections with next steps or related information in MDX documentation
Use descriptive, keyword-rich headings for navigation and SEO in MDX documentation
Focus on user goals and outcomes rather than system features in MDX documentation
Anticipate common questions and address them proactively in MDX documentation
Include troubleshooting for likely failure points in MDX documentation
Provide multiple pathways (beginner vs advanced) but offer an opinionated path to avoid overwhelming users in MDX documentation
Always include complete, runnable code examples that users can copy and execute in MDX documentation
Show proper error handling and edge case management in MDX code examples
Use realistic data instead of placeholder values in MDX code examples
Include expected outputs and results for verification in MDX code examples
Test all code examples thoroughly before publishing in MDX documentation
Specify language and include filename when relevant in MDX code examples
Add explanatory comments for complex logic in MDX code examples
Document all API...
Files:
docs/servers/tools.mdx
docs/**/*.{md,mdx,json}
📄 CodeRabbit inference engine (AGENTS.md)
Documentation uses Mintlify framework. Files must be in docs.json to be included. Never modify docs/python-sdk/** (auto-generated)
Files:
docs/servers/tools.mdx
docs/**/*.{md,mdx}
📄 CodeRabbit inference engine (AGENTS.md)
docs/**/*.{md,mdx}: Code examples in documentation must explain before showing code and make blocks fully runnable (include imports)
Documentation structure: Headers form navigation guide with logical H2/H3 hierarchy. Content should be user-focused with sections motivating features (why) before mechanics (how). Use prose over code comments for important information
Never use 'This isn't...' or 'not just...' constructions in writing - state what something IS directly. Avoid defensive writing patterns
Files:
docs/servers/tools.mdx
src/fastmcp/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
src/fastmcp/**/*.py: Python ≥ 3.10 with full type annotations required
Prioritize readable, understandable code - clarity over cleverness. Avoid obfuscated or confusing patterns even if shorter
Follow existing patterns and maintain consistency in code implementation
Be intentional about re-exports - don't blindly re-export everything to parent namespaces. Core types defining a module's purpose should be exported. Specialized features can live in submodules. Only re-export to fastmcp.* for most fundamental types
Never use bare except - be specific with exception types
Files:
src/fastmcp/tools/tool.pysrc/fastmcp/tools/tool_transform.pysrc/fastmcp/utilities/json_schema.py
🧠 Learnings (3)
📚 Learning: 2025-12-25T15:53:07.656Z
Learnt from: CR
Repo: jlowin/fastmcp PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-25T15:53:07.656Z
Learning: Applies to src/fastmcp/**/*.py : Python ≥ 3.10 with full type annotations required
Applied to files:
docs/servers/tools.mdxsrc/fastmcp/tools/tool.py
📚 Learning: 2025-12-25T15:53:07.656Z
Learnt from: CR
Repo: jlowin/fastmcp PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-25T15:53:07.656Z
Learning: Applies to src/fastmcp/**/*.py : Follow existing patterns and maintain consistency in code implementation
Applied to files:
docs/servers/tools.mdx
📚 Learning: 2025-12-25T15:53:07.656Z
Learnt from: CR
Repo: jlowin/fastmcp PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-25T15:53:07.656Z
Learning: Applies to src/fastmcp/**/*.py : Be intentional about re-exports - don't blindly re-export everything to parent namespaces. Core types defining a module's purpose should be exported. Specialized features can live in submodules. Only re-export to fastmcp.* for most fundamental types
Applied to files:
src/fastmcp/tools/tool.py
🧬 Code graph analysis (2)
src/fastmcp/tools/tool.py (1)
src/fastmcp/utilities/json_schema.py (1)
compress_schema(286-323)
src/fastmcp/tools/tool_transform.py (1)
src/fastmcp/utilities/json_schema.py (1)
compress_schema(286-323)
🪛 Ruff (0.14.10)
src/fastmcp/utilities/json_schema.py
45-45: Consider moving this statement to an else block
(TRY300)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Run tests: Python 3.10 on ubuntu-latest
- GitHub Check: Run tests: Python 3.10 on windows-latest
- GitHub Check: Run tests: Python 3.13 on ubuntu-latest
- GitHub Check: Run tests with lowest-direct dependencies
🔇 Additional comments (5)
docs/servers/tools.mdx (1)
177-179: LGTM! Clear documentation of automatic dereferencing behavior.The note effectively communicates the automatic $ref dereferencing feature and its purpose for MCP client compatibility. The placement in the Arguments section is appropriate since this affects how tool schemas are generated.
src/fastmcp/tools/tool.py (1)
37-37: LGTM! Import simplification aligns with centralized dereferencing.The removal of
resolve_root_refimport is correct now thatcompress_schemahandles all $ref dereferencing internally viadereference_refs.src/fastmcp/tools/tool_transform.py (1)
683-683: LGTM! Updated to use new compress_schema API.Both callsites correctly updated to use
compress_schema()without the removedprune_defsparameter. The new behavior automatically dereferences all$refentries viadereference_refs(), which achieves the same goal of eliminating$defsfrom the output schema while being more thorough about inlining definitions.Also applies to: 866-866
src/fastmcp/utilities/json_schema.py (2)
9-51: LGTM! Well-designed dereferencing with fallback for circular schemas.The
dereference_refsfunction properly handles the common case of inlining all$refentries using jsonref'sreplace_refs, and gracefully falls back toresolve_root_reffor self-referencing/circular schemas that can't be fully dereferenced. Theproxies=Falseandlazy_load=Falseparameters ensure immediate resolution to plain dicts, which is correct for the use case.The automatic removal of
$defsafter successful dereferencing (lines 42-43) is appropriate since the definitions are now inlined.
53-89: LGTM! Fallback handles MCP spec requirements for circular schemas.The
resolve_root_reffunction provides a sensible fallback for self-referencing schemas where full dereferencing isn't possible. By resolving only the root-level$refwhile preserving$defsfor nested references, it ensures the schema has an explicit"type": "object"at the root level (MCP spec requirement) while maintaining correctness for circular references.The conditions (line 77) appropriately guard against resolving when not needed.
![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: 809f2392db
ℹ️ 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".
| # Use jsonref to resolve all $ref references | ||
| # proxies=False returns plain dicts (not proxy objects) | ||
| # lazy_load=False resolves immediately | ||
| dereferenced = replace_refs(schema, proxies=False, lazy_load=False) | ||
|
|
There was a problem hiding this comment.
replace_refs replaces any {"$ref": ...} object with the referenced schema, which drops sibling keywords on that node (e.g., description, default, examples, field-level constraints). Pydantic commonly emits those siblings when a field references a model with its own metadata, so dereferencing here can silently strip that metadata from tool schemas and change what clients see. Consider merging the referenced schema with the local node (minus $ref) or reapplying local keys after deref so field-level annotations survive inlining.
Useful? React with 👍 / 👎.
jlowin
added
enhancement
Pydantic places description, default, and examples as siblings to $ref. The jsonref library drops these when inlining. This fix walks both the original and dereferenced schemas in parallel, merging siblings back in. Also fixes type annotations per CodeRabbit feedback.
jlowin
force-pushed
the
fix/dereference-input-schemas
branch
from
e02a495 to
73ae9e3
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/fastmcp/utilities/json_schema.py (2)
48-48: Replace assert with runtime check or type narrowing cast.Using
assertfor type narrowing is problematic because assertions can be disabled with Python's-Oflag, causing the type narrowing to be lost. If_merge_ref_siblingsunexpectedly returns a non-dict at the top level, production code could silently fail.♻️ Proposed fix using typing.cast
+from typing import Any, cast + # ... dereferenced = replace_refs(schema, proxies=False, lazy_load=False) # Merge sibling keywords that were lost during dereferencing # Pydantic puts description, default, examples as siblings to $ref merged = _merge_ref_siblings(schema, dereferenced) - # Type assertion: top-level schema is always a dict - assert isinstance(merged, dict) - dereferenced = merged + # Top-level schema is always a dict after merging + dereferenced = cast(dict[str, Any], merged)
100-109: Simplify the complex list comprehension for better readability.This nested list comprehension with conditional zip operations is difficult to understand and maintain, violating the coding guideline "Prioritize readable, understandable code - clarity over cleverness."
♻️ Proposed refactor for clarity
- elif isinstance(original, list) and isinstance(dereferenced, list): - # Process list items in parallel - return [ - _merge_ref_siblings(o, d) if i < len(original) else d - for i, (o, d) in enumerate( - zip(original, dereferenced, strict=False) - if len(original) <= len(dereferenced) - else zip(original[: len(dereferenced)], dereferenced, strict=False) - ) - ] + (dereferenced[len(original) :] if len(dereferenced) > len(original) else []) + elif isinstance(original, list) and isinstance(dereferenced, list): + # Process list items in parallel + result = [] + min_len = min(len(original), len(dereferenced)) + + # Merge items at matching indices + for i in range(min_len): + result.append(_merge_ref_siblings(original[i], dereferenced[i])) + + # Append any remaining dereferenced items + if len(dereferenced) > min_len: + result.extend(dereferenced[min_len:]) + + return resultAs per coding guidelines, clarity should be prioritized over clever one-liners.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
tests/utilities/test_json_schema.pyis excluded by none and included by none
📒 Files selected for processing (1)
src/fastmcp/utilities/json_schema.py
🧰 Additional context used
📓 Path-based instructions (1)
src/fastmcp/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
src/fastmcp/**/*.py: Python ≥ 3.10 with full type annotations required
Prioritize readable, understandable code - clarity over cleverness. Avoid obfuscated or confusing patterns even if shorter
Follow existing patterns and maintain consistency in code implementation
Be intentional about re-exports - don't blindly re-export everything to parent namespaces. Core types defining a module's purpose should be exported. Specialized features can live in submodules. Only re-export to fastmcp.* for most fundamental types
Never use bare except - be specific with exception types
Files:
src/fastmcp/utilities/json_schema.py
🧠 Learnings (1)
📚 Learning: 2025-12-25T15:53:07.656Z
Learnt from: CR
Repo: jlowin/fastmcp PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-25T15:53:07.656Z
Learning: Applies to src/fastmcp/**/*.py : Python ≥ 3.10 with full type annotations required
Applied to files:
src/fastmcp/utilities/json_schema.py
🪛 Ruff (0.14.10)
src/fastmcp/utilities/json_schema.py
55-55: Consider moving this statement to an else block
(TRY300)
🔇 Additional comments (1)
src/fastmcp/utilities/json_schema.py (1)
347-384: LGTM - Dereferencing-first approach is well-structured.The updated flow correctly prioritizes dereferencing before optimization:
dereference_refsinlines all$refentries and removes$defs(MCP compatibility)- Parameter pruning operates on the dereferenced schema
- Additional optimizations run only when requested
Removing the
prune_defsparameter from the public API is appropriate since dereferencing now handles definition cleanup.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/fastmcp/utilities/json_schema.py (2)
47-49: Avoid assert for runtime validation.Using
assertfor type validation is problematic because assertions can be disabled with Python's-Oflag andAssertionErrordoesn't provide descriptive context. Since this is a runtime safety check (not a development-time invariant), use a proper type guard or conditional check with a descriptive error.♻️ Proposed fix
- # Type assertion: top-level schema is always a dict - assert isinstance(merged, dict) - dereferenced = merged + # Top-level schema should always be a dict after merging + if not isinstance(merged, dict): + raise TypeError(f"Expected dict after merging, got {type(merged).__name__}") + dereferenced = merged
100-109: Simplify the list handling logic for better readability.The current list-processing logic is overly complex with nested conditionals in the zip operation and a redundant length check on line 103. The condition
if i < len(original)is always true within the enumerate because of how the zip is constructed. Additionally, usingstrict=Falsemay mask bugs where the lists are expected to have the same structure.Per the coding guidelines: "Prioritize readable, understandable code - clarity over cleverness."
♻️ Proposed simplification
- elif isinstance(original, list) and isinstance(dereferenced, list): - # Process list items in parallel - return [ - _merge_ref_siblings(o, d) if i < len(original) else d - for i, (o, d) in enumerate( - zip(original, dereferenced, strict=False) - if len(original) <= len(dereferenced) - else zip(original[: len(dereferenced)], dereferenced, strict=False) - ) - ] + (dereferenced[len(original) :] if len(dereferenced) > len(original) else []) + elif isinstance(original, list) and isinstance(dereferenced, list): + # Process list items in parallel, handling different lengths + result = [] + for i in range(len(dereferenced)): + if i < len(original): + result.append(_merge_ref_siblings(original[i], dereferenced[i])) + else: + result.append(dereferenced[i]) + return result
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
tests/utilities/test_json_schema.pyis excluded by none and included by none
📒 Files selected for processing (2)
docs/servers/tools.mdxsrc/fastmcp/utilities/json_schema.py
🚧 Files skipped from review as they are similar to previous changes (1)
- docs/servers/tools.mdx
🧰 Additional context used
📓 Path-based instructions (1)
src/fastmcp/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
src/fastmcp/**/*.py: Python ≥ 3.10 with full type annotations required
Prioritize readable, understandable code - clarity over cleverness. Avoid obfuscated or confusing patterns even if shorter
Follow existing patterns and maintain consistency in code implementation
Be intentional about re-exports - don't blindly re-export everything to parent namespaces. Core types defining a module's purpose should be exported. Specialized features can live in submodules. Only re-export to fastmcp.* for most fundamental types
Never use bare except - be specific with exception types
Files:
src/fastmcp/utilities/json_schema.py
🧠 Learnings (2)
📓 Common learnings
Learnt from: CR
Repo: jlowin/fastmcp PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-25T15:53:07.656Z
Learning: Applies to src/fastmcp/**/*.py : Python ≥ 3.10 with full type annotations required
Learnt from: CR
Repo: jlowin/fastmcp PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-25T15:53:07.656Z
Learning: Applies to src/fastmcp/**/*.py : Follow existing patterns and maintain consistency in code implementation
📚 Learning: 2025-12-25T15:53:07.656Z
Learnt from: CR
Repo: jlowin/fastmcp PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-25T15:53:07.656Z
Learning: Applies to src/fastmcp/**/*.py : Python ≥ 3.10 with full type annotations required
Applied to files:
src/fastmcp/utilities/json_schema.py
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Run tests: Python 3.10 on windows-latest
🔇 Additional comments (2)
src/fastmcp/utilities/json_schema.py (2)
153-153: LGTM! Type annotations are correct.The updated type annotations properly use
dict[str, Any]syntax (Python 3.10+) and maintain consistency throughout the module. The removal ofprune_defsfromcompress_schema's public API is appropriate sincedereference_refsnow handles removing$defsafter inlining.Also applies to: 175-179, 348-352
369-382: LGTM! Implementation correctly dereferences and optimizes.The logic flow is correct:
- Dereference all
$refentries upfront (line 369) for MCP client compatibility- Prune specific parameters if requested (lines 372-373)
- Apply optimizations only when needed (lines 376-382)
The hardcoded
prune_defs=Falseon line 381 is appropriate sincedereference_refsalready removes$defsafter inlining all references.
| def _merge_ref_siblings( | ||
| original: dict[str, Any] | list | Any, | ||
| dereferenced: dict[str, Any] | list | Any, | ||
| ) -> dict[str, Any] | list | Any: | ||
| """Merge sibling keywords from original $ref nodes into dereferenced schema. | ||
|
|
||
| When jsonref resolves $ref, it replaces the entire node with the referenced | ||
| definition, losing any sibling keywords like description, default, or examples. | ||
| This function walks both trees in parallel and merges those siblings back. | ||
|
|
||
| Args: | ||
| original: The original schema with $ref and potential siblings | ||
| dereferenced: The schema after jsonref processing | ||
|
|
||
| Returns: | ||
| The dereferenced schema with sibling keywords restored | ||
| """ | ||
| if isinstance(original, dict) and isinstance(dereferenced, dict): | ||
| # Check if original had a $ref with siblings | ||
| if "$ref" in original: | ||
| siblings = {k: v for k, v in original.items() if k not in ("$ref", "$defs")} | ||
| if siblings: | ||
| # Merge siblings into dereferenced, siblings take precedence | ||
| # (local description overrides referenced definition's description) | ||
| merged = dict(dereferenced) | ||
| merged.update(siblings) | ||
| return merged | ||
|
|
||
| # Recurse into nested structures | ||
| result = {} | ||
| for key, value in dereferenced.items(): | ||
| if key in original: | ||
| result[key] = _merge_ref_siblings(original[key], value) | ||
| else: | ||
| result[key] = value | ||
| return result | ||
|
|
||
| elif isinstance(original, list) and isinstance(dereferenced, list): | ||
| # Process list items in parallel | ||
| return [ | ||
| _merge_ref_siblings(o, d) if i < len(original) else d | ||
| for i, (o, d) in enumerate( | ||
| zip(original, dereferenced, strict=False) | ||
| if len(original) <= len(dereferenced) | ||
| else zip(original[: len(dereferenced)], dereferenced, strict=False) | ||
| ) | ||
| ] + (dereferenced[len(original) :] if len(dereferenced) > len(original) else []) | ||
|
|
||
| return dereferenced | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -type f -name "*test*.py" | head -20Repository: jlowin/fastmcp
Length of output: 135
🏁 Script executed:
rg -l "json_schema|_merge_ref_siblings|dereference_refs" --type=py | grep -i testRepository: jlowin/fastmcp
Length of output: 40
🏁 Script executed:
rg -n "_merge_ref_siblings" --type=pyRepository: jlowin/fastmcp
Length of output: 440
🏁 Script executed:
cat -n ./src/fastmcp/utilities/tests.pyRepository: jlowin/fastmcp
Length of output: 11290
🏁 Script executed:
rg -n "json_schema|dereference" ./src/fastmcp/utilities/tests.py -A 5 -B 2Repository: jlowin/fastmcp
Length of output: 40
🏁 Script executed:
rg -n "def test" ./src/fastmcp/utilities/tests.pyRepository: jlowin/fastmcp
Length of output: 104
🏁 Script executed:
find . -type f -name "*test_json*" -o -name "*json*test*" | grep -E '\.py$'Repository: jlowin/fastmcp
Length of output: 40
🏁 Script executed:
rg -l "dereference_refs" --type=pyRepository: jlowin/fastmcp
Length of output: 94
🏁 Script executed:
find . -path "*/tests/*" -name "*.py" -type f | grep -E "(json|schema)" | head -20Repository: jlowin/fastmcp
Length of output: 40
🏁 Script executed:
find . -path "*/test*" -name "*.py" -type f | head -30Repository: jlowin/fastmcp
Length of output: 169
🏁 Script executed:
rg -n "dereference_refs|_merge_ref_siblings|json_schema" --type=py -l | grep -i testRepository: jlowin/fastmcp
Length of output: 40
🏁 Script executed:
cat -n src/fastmcp/utilities/json_schema.py | head -60Repository: jlowin/fastmcp
Length of output: 2907
🏁 Script executed:
rg -n "import.*json_schema|from.*json_schema" --type=pyRepository: jlowin/fastmcp
Length of output: 1341
🏁 Script executed:
find . -type d -name tests | xargs -I {} find {} -name "*.py" -type fRepository: jlowin/fastmcp
Length of output: 102
🏁 Script executed:
rg -n "json_schema|dereference_refs|_merge_ref_siblings" ./examples/testing_demo/tests/test_server.py -A 3 -B 2Repository: jlowin/fastmcp
Length of output: 40
🏁 Script executed:
cat -n ./examples/testing_demo/tests/test_server.py | head -100Repository: jlowin/fastmcp
Length of output: 3863
🏁 Script executed:
find . -type f -name "conftest.py" -o -type f -path "*/tests/test_*.py" | head -20Repository: jlowin/fastmcp
Length of output: 102
🏁 Script executed:
find . -type f -name "*.py" -path "*/test*" | xargs grep -l "compress_schema\|dereference" 2>/dev/nullRepository: jlowin/fastmcp
Length of output: 42
🏁 Script executed:
rg -r "def compress_schema|def dereference_refs" --type=py -A 2Repository: jlowin/fastmcp
Length of output: 454
🏁 Script executed:
find . -type f -name "test_*.py" -o -type f -name "*_test.py" | wc -lRepository: jlowin/fastmcp
Length of output: 59
🏁 Script executed:
cat -n src/fastmcp/utilities/json_schema.pyRepository: jlowin/fastmcp
Length of output: 18076
Add comprehensive test coverage for the dereference/merge logic.
The dereference_refs and _merge_ref_siblings functions implement critical logic for inlining $ref definitions while preserving sibling keywords (description, default, examples). No tests currently exist for these functions. Add tests covering:
- Simple cases with siblings alongside
$ref - Nested structures with mixed
$refand siblings - Lists containing schemas with
$ref - Edge cases like empty dicts/lists and mismatched structures
- Root-level
$refresolution (resolve_root_ref)
The _merge_ref_siblings function now follows $ref pointers into $defs to preserve siblings on nested references (e.g., a field with default inside a referenced definition). Added tests for list handling (anyOf) and nested sibling preservation.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/fastmcp/utilities/json_schema.py (1)
6-6: Remove redundantlazy_load=Falseparameter.The project uses
jsonref>=1.1.0. Thelazy_loadparameter only applies whenproxies=True, but this code usesproxies=Falseto get plain dict objects. Sincelazy_loadis ignored withproxies=False, remove it to avoid confusion:dereferenced = replace_refs(schema, proxies=False)Also applies to: 42-42
🧹 Nitpick comments (2)
src/fastmcp/utilities/json_schema.py (2)
64-82: Simplify overly broad type hints.The type annotations
dict[str, Any] | list | Anyare redundant becauseAnysubsumes all types. This makes the union meaningless and defeats the purpose of type hints.♻️ Proposed type hint improvement
-def _merge_ref_siblings( - original: dict[str, Any] | list | Any, - dereferenced: dict[str, Any] | list | Any, - defs: dict[str, Any], -) -> dict[str, Any] | list | Any: +def _merge_ref_siblings( + original: Any, + dereferenced: Any, + defs: dict[str, Any], +) -> Any:Alternatively, if you want to be more specific about expected types:
+from typing import Any, Union + +_SchemaNode = Union[dict[str, Any], list[Any], str, int, float, bool, None] + def _merge_ref_siblings( - original: dict[str, Any] | list | Any, - dereferenced: dict[str, Any] | list | Any, + original: _SchemaNode, + dereferenced: _SchemaNode, defs: dict[str, Any], -) -> dict[str, Any] | list | Any: +) -> _SchemaNode:Based on coding guidelines requiring full type annotations for Python ≥ 3.10.
114-123: Simplify overly complex list handling logic.The conditional expression on lines 119-121 is unnecessarily complex and hard to understand. Since
zip(..., strict=False)already handles mismatched lengths by stopping at the shorter sequence, the explicit slicing in theelsebranch is redundant.♻️ Proposed simplification
elif isinstance(original, list) and isinstance(dereferenced, list): # Process list items in parallel + min_len = min(len(original), len(dereferenced)) return [ - _merge_ref_siblings(o, d, defs) if i < len(original) else d - for i, (o, d) in enumerate( - zip(original, dereferenced, strict=False) - if len(original) <= len(dereferenced) - else zip(original[: len(dereferenced)], dereferenced, strict=False) - ) - ] + (dereferenced[len(original) :] if len(dereferenced) > len(original) else []) + _merge_ref_siblings(o, d, defs) + for o, d in zip(original, dereferenced, strict=False) + ] + dereferenced[min_len:]This achieves the same result with clearer logic:
zipwithstrict=Falsepairs up elements from both lists- Any extra elements in
dereferencedare appended without modificationBased on coding guidelines: "Prioritize readable, understandable code - clarity over cleverness."
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
tests/utilities/test_json_schema.pyis excluded by none and included by none
📒 Files selected for processing (1)
src/fastmcp/utilities/json_schema.py
🧰 Additional context used
📓 Path-based instructions (1)
src/fastmcp/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
src/fastmcp/**/*.py: Python ≥ 3.10 with full type annotations required
Prioritize readable, understandable code - clarity over cleverness. Avoid obfuscated or confusing patterns even if shorter
Follow existing patterns and maintain consistency in code implementation
Be intentional about re-exports - don't blindly re-export everything to parent namespaces. Core types defining a module's purpose should be exported. Specialized features can live in submodules. Only re-export to fastmcp.* for most fundamental types
Never use bare except - be specific with exception types
Files:
src/fastmcp/utilities/json_schema.py
🧠 Learnings (2)
📓 Common learnings
Learnt from: CR
Repo: jlowin/fastmcp PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-25T15:53:07.656Z
Learning: Applies to src/fastmcp/**/*.py : Python ≥ 3.10 with full type annotations required
📚 Learning: 2025-12-25T15:53:07.656Z
Learnt from: CR
Repo: jlowin/fastmcp PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-25T15:53:07.656Z
Learning: Applies to src/fastmcp/**/*.py : Python ≥ 3.10 with full type annotations required
Applied to files:
src/fastmcp/utilities/json_schema.py
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Run tests: Python 3.10 on windows-latest
- GitHub Check: Run tests: Python 3.13 on ubuntu-latest
- GitHub Check: Run tests: Python 3.10 on ubuntu-latest
- GitHub Check: Run tests with lowest-direct dependencies
🔇 Additional comments (3)
src/fastmcp/utilities/json_schema.py (3)
13-17: LGTM!The docstring clearly explains the problem (MCP clients not handling $ref properly) and the solution (preserving sibling keywords during dereferencing).
167-167: LGTM!Type hints properly updated to
dict[str, Any], consistent with Python ≥ 3.10 requirements from coding guidelines.Also applies to: 189-189, 193-193, 362-362, 366-366
383-396: LGTM!The flow is correct:
dereference_refsinlines all definitions and removes$defs(for MCP client compatibility), then subsequent optimizations are applied. Settingprune_defs=Falseon line 395 is appropriate since$defswere already removed.
Added visited-node tracking to protect against potential infinite recursion if $defs contain circular references. Also simplified type hints and list handling logic.
Test Failure AnalysisSummary: The test Root Cause:
The stack trace shows the hang occurs at: File "diskcache/core.py", line 623, in _con
con = self._local.con = sqlite3.connect(...)Suggested Solution: The best fix is to ensure each test uses an isolated database directory. This can be done by:
import pytest
from pathlib import Path
@pytest.fixture
def temp_oauth_storage(tmp_path):
"""Provide isolated storage for OAuth tests."""
from key_value.aio.stores.disk import DiskStore
from key_value.aio.wrappers.encryption import FernetEncryptionWrapper
from cryptography.fernet import Fernet
from fastmcp.server.auth.oauth_proxy import derive_jwt_key
storage_encryption_key = derive_jwt_key(
high_entropy_material="test-secret",
salt="fastmcp-storage-encryption-key",
)
storage = FernetEncryptionWrapper(
key_value=DiskStore(directory=tmp_path / "oauth-proxy"),
fernet=Fernet(key=storage_encryption_key),
)
return storage
def test_base_authority_from_parameter(self, temp_oauth_storage):
"""Test that base_authority can be set via parameter."""
provider = AzureProvider(
client_id="env-client-id",
client_secret="env-secret",
tenant_id="env-tenant-id",
base_url="https://myserver.com",
required_scopes=["read"],
base_authority="login.microsoftonline.us",
jwt_signing_key="test-secret",
client_storage=temp_oauth_storage, # Add this parameter
)
# ... rest of test
This ensures each test gets its own isolated SQLite database in a temporary directory that's automatically cleaned up after the test. Detailed AnalysisRelevant log excerpts: Why Windows specifically?
Why this test and not others?
Related Files
|
|
@jlowin: thank you for your amazing work on this repo! Any chance this could get released sooner than v3? I keep running into this issue when parsing outputs from Gemini 3 Pro, and having this fix in FastMCP would be amazing. I upgraded to 2.14.3 but it didn't fix this. Let me know if I should do something else. |
|
Hi @debnil-mercor - I've opened #2861 and will try release in the next couple days |
|
@jlowin: thanks so much for the quick response! Let me know if I can help accelerate this. |
2 participants
MCP clients like VS Code Copilot and Claude Desktop don't properly handle
$refin tool input schemas—they strip$defsbefore sending schemas to the LLM, leaving dangling references that break enum parameters and complex types.FastMCP now automatically dereferences all
$refentries, inlining definitions directly into the schema. This happens incompress_schema()at tool creation time and again into_mcp_tool()at serving time (to catch OpenAPI tools that bypasscompress_schema).For circular/self-referencing schemas that can't be fully dereferenced, FastMCP falls back to resolving only the root-level
$refwhile preserving$defsfor nested references.Follow-up to #2807, addresses #2236