-
Notifications
You must be signed in to change notification settings - Fork 1.6k
[Draft] preserve key field in ResponseCachingMiddleware for prefixed tools/resources/prompts #2303
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
…ls/resources/prompts The ResponseCachingMiddleware was losing prefix information when caching tools, resources, and prompts from mounted/imported servers. The root cause was that the private _key attribute wasn't being serialized by Pydantic. Changes: - Add model_serializer to FastMCPComponent to include _key in serialization - Update model_validate to restore _key from deserialized data - Add key parameter when creating cached Tool/Resource/Prompt objects - Add comprehensive test for mounted server prefix preservation Fixes #2300 Co-authored-by: William Easton <[email protected]>
WalkthroughThe caching middleware has been enhanced with three new cacheable wrapper models—CachableTool, CachableResource, and CachablePrompt—that preserve key fields and offer wrap/unwrap conversion helpers. The list caches now store these cacheable models instead of original types, with list operation handlers updated to wrap objects before caching and unwrap them when returning to callers. A new test verifies that mounted-server prefixes are preserved across multiple cached list operation calls. Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Tip 📝 Customizable high-level summaries are now available!You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.
Example:
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 |
|
/marvin im not sure i want a custom validator and serializer on the fastmcp component, we might just need to use custom cachable models in the response caching middleware, can you explore our options here and outline a recommendation? |
|
Claude finished @strawgate's task —— View job Analysis: Custom Serializers vs. Custom Cacheable ModelsI've analyzed both approaches for preserving key fields in cached responses. Here's my recommendation: Current Implementation (Custom Serializers on FastMCPComponent)Pros:
Cons:
Alternative Approach (Custom Cacheable Models in Middleware)Pros:
Cons:
Recommendation: Use Custom Cacheable Models ✅The alternative approach is architecturally superior because:
Implementation NotesIf you go with custom cacheable models:
The extra boilerplate (~100 lines) is worth the architectural cleanliness and avoiding potential bugs from changing base class behavior. |
|
/marvin do you think we could just remove key and just mutate the tool name when it gets returned from mounted servers? |
|
Claude finished @strawgate's task —— View job Analysis: Should We Remove
|
|
/marvin im not sure that's convincing, for a client to call a tool we must be able to distinguish prefixes without keys already? Can you review this again? What if we just stopped using key for bookkeeping? |
|
Claude finished @strawgate's task —— View job Analysis: Do We Actually Need the
|
|
I dont think marvin's final conclusion is valid: if you mutate the name of a tool to add a prefix, then the tool will also have that prefix in the server it was originally added in. So the ultimate (parent) server has all properly named tools, but every subserver has weirdly named tools |
|
/marvin remove key and fix merge conflicts and respond to jlowin |
|
Claude finished @strawgate's task —— View job Analysis and Implementation ✅Tasks
Response to @jlowin's ConcernYou're absolutely right - we cannot just mutate the The Architecture ProblemLooking at the code flow (server.py:1094-1134), when a tool is called:
If we mutated the child's tool name to
The
Why the Original PR Approach Was ProblematicMarvin's analysis was spot on: adding custom serializers to
Implemented Solution: Custom Cacheable Models ✅Following Marvin's recommendation and the existing pattern (
This approach:
Results
The PR now correctly preserves prefixes when caching tools, resources, and prompts from mounted servers without modifying the core component architecture. |
Instead of adding custom serializers to FastMCPComponent (which affects equality globally), use dedicated cacheable models in the middleware. This approach: - Isolates caching concerns to the middleware - Follows existing patterns (CachableReadResourceContents, CachableToolResult) - Preserves key field through serialization without side effects - Maintains architectural separation Co-authored-by: William Easton <[email protected]>
|
@claude but wouldnt we just model_copy the tool? then changing the name wouldnt change the tool in the underlying tool manager? Or return a transformed tool where the name has been modified? |
Summary
The ResponseCachingMiddleware was losing prefix information when caching tools, resources, and prompts from mounted/imported servers. The root cause was that the private
_keyattribute wasn't being serialized by Pydantic.Example
Before this fix:
After this fix, both calls correctly return
["child_example_tool"].Changes
_keyfieldkeywhen creating cacheable objectsFixes #2300
Generated with Claude Code