Skip to content

fix: remove dereference=True while calling compress_schema#3774

Closed
Jazzcort wants to merge 1 commit intoPrefectHQ:release/2.xfrom
Jazzcort:revoke-dereference-json
Closed

fix: remove dereference=True while calling compress_schema#3774
Jazzcort wants to merge 1 commit intoPrefectHQ:release/2.xfrom
Jazzcort:revoke-dereference-json

Conversation

@Jazzcort
Copy link
Copy Markdown

@Jazzcort Jazzcort commented Apr 6, 2026

Description

This addresses the issues found in #3760. Currently, if a self-referencing Pydantic data type is buried in a nested field, calling dereference_refs() introduces a circular reference to the final schema. This results in a serialization error that crashes the server during tools/list requests.

I checked v3.2.0 and noticed that dereference=True is no longer being passed to compress_schema(), which essentially reverts #3170. We should implement this same behavior for the 2.x versions. While the community will eventually migrate to 3.x, I think we still need to pull this fix in now because the bug causes a complete server crash whenever tools/list is called.

Contribution type

  • Bug fix (simple, well-scoped fix for a clearly broken behavior)
  • Documentation improvement
  • Enhancement (maintainers typically implement enhancements — see CONTRIBUTING.md)

Checklist

  • This PR addresses an existing issue (or fixes a self-evident bug)
  • I have read CONTRIBUTING.md
  • I have added tests that cover my changes
  • I have run uv run prek run --all-files and all checks pass
  • I have self-reviewed my changes
  • If I used an LLM, it followed the repo's contributing conventions (not generic output)

This addresses the issues found in PrefectHQ#3760. Currently, if a self-referencing Pydantic
data type is buried in a nested field, calling dereference_refs() introduces a
circular reference to the final schema. This results in a serialization error that
crashes the server during tools/list requests.

I checked v3.2.0 and noticed that dereference=True is no longer being passed to
compress_schema(), which essentially reverts PrefectHQ#3170. We should implement this same
behavior for the 2.x versions. While the community will eventually migrate to 3.x,
I think we still need to pull this fix in now because the bug causes a complete
server crash whenever tools/list is called.
@marvin-context-protocol marvin-context-protocol Bot added bug Something isn't working. Reports of errors, unexpected behavior, or broken functionality. server Related to FastMCP server implementation or server-side functionality. high-priority labels Apr 6, 2026
Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 0a53f1cfa0

ℹ️ 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".

Comment thread src/fastmcp/tools/tool.py
Comment on lines 500 to 503
input_schema,
prune_params=prune_params,
prune_titles=True,
dereference=True,
)
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Keep input schemas dereferenced for client compatibility

Removing dereference=True here changes tool parameter schemas back to $ref/$defs form for common Pydantic inputs (e.g., user: UserInput), which regresses the compatibility behavior introduced for MCP clients that do not resolve JSON Schema refs. The codebase still documents this as required for clients like Copilot/Claude (src/fastmcp/utilities/json_schema.py) and existing tool-manager expectations assume inlined parameter objects, so this will make affected tools unusable in those clients rather than just changing formatting.

Useful? React with 👍 / 👎.

@marvin-context-protocol
Copy link
Copy Markdown
Contributor

Test Failure Analysis

Summary: The PR removes dereference=True from compress_schema() in tool.py but only partially updated the test suite — test_tool_manager.py and several tests in test_tool_transform.py still assert the old dereferenced (inlined) schema behavior.

Root Cause: The PR changed how tool input schemas are built: previously compress_schema(..., dereference=True) would inline all $refs so schemas had flat properties with no $defs. After removing dereference=True, Pydantic models produce schemas with $defs and $ref entries. tests/tools/test_tool.py was updated to match, but these tests were not:

  • tests/tools/test_tool_manager.py:79test_pydantic_model_function accesses tool.parameters["properties"]["user"]["properties"], which is now a $ref, not an inlined object → KeyError: 'properties'
  • tests/tools/test_tool_transform.py:220test_hidden_param_prunes_defs asserts '$defs' not in schema, but the schema now contains $defs
  • tests/tools/test_tool_transform.py:1576test_transform_tool_with_complex_defs_pruning makes the same assertion
  • More tests in test_tool_transform.py that expect the snapshot to contain inlined properties (not $ref entries)

Suggested Solution: There are two paths:

  1. Update the failing tests to match the new $defs/$ref schema structure. This is straightforward but note the Codex review already flagged that removing dereferencing regresses MCP client compatibility — some MCP clients (Copilot, Claude) don't resolve JSON Schema $refs at runtime.

  2. More targeted fix: Instead of removing dereferencing entirely, handle the circular reference case specifically — e.g., catch RecursionError (or detect circular schemas) and fall back to non-dereferenced output only when needed. This would fix the crash from Recursive outputSchema from native Pydantic tool crashes ListToolsResult serialization #3760 while preserving the flat schema behavior that existing tests document and MCP clients rely on.

Path 2 is likely the right approach given that the existing tests exist precisely to verify MCP client compatibility.

Detailed Analysis

Failing log excerpts:

# test_pydantic_model_function (test_tool_manager.py:79)
E       KeyError: 'properties'
# Test expects: tool.parameters["properties"]["user"]["properties"] to exist
# Actual schema now has: {"$defs": {"UserInput": {...}}, "properties": {"user": {"$ref": "#/$defs/UserInput"}}}

# test_hidden_param_prunes_defs (test_tool_transform.py:220)
E       AssertionError: assert '$defs' not in {
  '$defs': {'VisibleType': {'properties': {'x': {'type': 'integer'}}, ...}},
  'properties': {'a': {'$ref': '#/$defs/VisibleType'}}, ...
}

# test_transform_tool_with_complex_defs_pruning (test_tool_transform.py:1576)
E       AssertionError: assert '$defs' not in {
  '$defs': {'UsedType': {'properties': {'value': {'type': 'string'}}, ...}},
  'properties': {'used_param': {'$ref': '#/$defs/UsedType'}}, ...
}

The PR diff removes dereference=True from two compress_schema() calls in src/fastmcp/tools/tool.py (lines ~500 and ~561) and updates tests/tools/test_tool.py — but test_tool_manager.py and test_tool_transform.py were not updated.

The Codex review on this PR also flagged this concern at src/fastmcp/tools/tool.py:503.

Related Files
  • src/fastmcp/tools/tool.py — contains the compress_schema() calls being modified
  • tests/tools/test_tool_manager.py:79test_pydantic_model_function, fails with KeyError
  • tests/tools/test_tool_transform.py:220test_hidden_param_prunes_defs, fails with AssertionError
  • tests/tools/test_tool_transform.py:1576test_transform_tool_with_complex_defs_pruning, fails with AssertionError
  • tests/tools/test_tool.py — was updated in this PR, shows the expected new schema structure

@Jazzcort
Copy link
Copy Markdown
Author

Jazzcort commented Apr 6, 2026

Hey @jlowin, let me know what do you think about this. Do you think this is worth the fix? I can start to fix the test cases if you agree with this fix. Otherwise, you can just close this PR. 😁

@jlowin
Copy link
Copy Markdown
Member

jlowin commented Apr 6, 2026

Thanks for digging into this! The bug is real — recursive models nested in containers do break serialization.

The history here is that dereferencing was added specifically because many MCP clients (Claude, Copilot, etc.) don't resolve $ref/$defs in schemas — there are a bunch of issues documenting how contentious this was. So removing it outright fixes the recursive case but breaks the more common case.

In 3.x we solved this properly: the dereference=True keyword was removed from compress_schema() exactly where you indicate, but dereferencing was promoted to its own middleware with a public API flag — FastMCP(dereference_schemas=False) lets users opt out. The middleware also handles circular schemas gracefully instead of crashing.

Backporting that middleware to 2.x would be a fairly involved change. Since 3.x already addresses this (and a lot more), I'd recommend upgrading if you can — it's a much better experience overall. Happy to help if you run into any migration questions.

@jlowin jlowin closed this Apr 6, 2026
@Jazzcort
Copy link
Copy Markdown
Author

Jazzcort commented Apr 9, 2026

@jlowin Thank you for the detailed walkthrough of this bug! We currently have fastmcp pinned to 2.x just to streamline the process for our current release. However, we are definitely planning to upgrade to 3.x in a future release—the new features and improvements will really help simplify our implementation.

Thanks for maintaining such an awesome project! We'll be sure to contribute back if we encounter any issues or think of potential improvements while using it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working. Reports of errors, unexpected behavior, or broken functionality. high-priority server Related to FastMCP server implementation or server-side functionality.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants