Conversation
Keep .key as the standard lookup interface for all components but implement it as a computed property instead of a stored field. - Remove _key private attribute and custom model_copy(key=...) - .key returns .name for tools/prompts, str(.uri) for resources, .uri_template for templates - Use .key universally for component lookups in managers - MountedProvider: prefix URIs only for resources/templates, not names - Docket registration: tools/prompts use .key, resources use .name (matches fn.__name__ for function lookup)
Keep .key as the standard lookup interface for all components but implement it as a computed property instead of a stored field. - Remove _key private attribute and custom model_copy(key=...) - .key returns .name for tools/prompts, str(.uri) for resources, .uri_template for templates - Use .key universally for component lookups and Docket registration - MountedProvider: prefix URIs only for resources/templates, not names - Add _backend_* fields to proxy classes to preserve original identifiers for backend calls when prefixed via import_server
WalkthroughAdds backend-aware identifiers to proxy classes (ProxyTool, ProxyResource, ProxyTemplate, ProxyPrompt) with corresponding Possibly related PRs
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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 |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/fastmcp/server/proxy.py (1)
413-419: Consider extracting the error message to a constant or helper.The static analysis tool flags TRY003 for the inline error message. While this is a minor style issue, you could optionally extract it to improve consistency.
🔎 Optional: Extract error message
+class ProxyResource(Resource, MirroredComponent): + """...""" + _EMPTY_CONTENT_ERROR = "Remote server returned empty content for {uri}" + # ... existing code ... async def read(self) -> ResourceContent: # ... if not result: - raise ResourceError( - f"Remote server returned empty content for {backend_uri}" - ) + raise ResourceError(self._EMPTY_CONTENT_ERROR.format(uri=backend_uri))
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
tests/server/tasks/test_server_tasks_parameter.pyis excluded by none and included by none
📒 Files selected for processing (3)
src/fastmcp/server/proxy.py(9 hunks)src/fastmcp/server/server.py(2 hunks)src/fastmcp/server/tasks/handlers.py(3 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
src/fastmcp/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
src/fastmcp/**/*.py: Write Python code with ≥3.10 type annotations required throughout
Never use bare except - be specific with exception types
Files:
src/fastmcp/server/server.pysrc/fastmcp/server/tasks/handlers.pysrc/fastmcp/server/proxy.py
🧬 Code graph analysis (2)
src/fastmcp/server/server.py (3)
src/fastmcp/resources/template.py (1)
key(221-228)src/fastmcp/utilities/components.py (1)
key(66-73)src/fastmcp/resources/resource.py (1)
key(276-283)
src/fastmcp/server/proxy.py (3)
src/fastmcp/utilities/components.py (1)
model_copy(99-120)src/fastmcp/exceptions.py (1)
ResourceError(14-15)src/fastmcp/prompts/prompt_manager.py (1)
get_prompt(53-58)
🪛 Ruff (0.14.8)
src/fastmcp/server/proxy.py
417-419: Avoid specifying long messages outside the exception class
(TRY003)
⏰ 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 (12)
src/fastmcp/server/proxy.py (7)
283-283: LGTM! Backend-aware dispatch for ProxyTool.The
_backend_namefield andmodel_copyoverride correctly preserve the original tool name for backend calls when the local name is prefixed. This ensures proper routing to the remote server.Also applies to: 289-296
343-344: Correct usage of backend identifier for tool calls.Using
self._backend_name or self.nameensures the tool call is routed with the original backend identifier when available, falling back to the local name otherwise.
364-364: LGTM! Backend-aware dispatch for ProxyResource.The pattern mirrors
ProxyToolcorrectly. The_backend_uriis captured on first URI change and used for backend read operations.Also applies to: 377-384
442-442: LGTM! Backend-aware dispatch for ProxyTemplate.Consistent implementation preserving
_backend_uri_templateon first change. The pattern aligns with the other proxy types.Also applies to: 448-455
487-488: Correct template parameterization using backend template.The
backend_templateis correctly used for formatting the parameterized URI, ensuring the remote server receives the expected URI pattern.
534-534: LGTM! Backend-aware dispatch for ProxyPrompt.Completes the consistent pattern across all proxy types. The
_backend_namefield andmodel_copyoverride follow the same approach asProxyTool.Also applies to: 540-547
578-580: Correct usage of backend identifier for prompt rendering.The fallback pattern
self._backend_name or self.nameensures proper routing to the remote server.src/fastmcp/server/server.py (2)
445-445: LGTM! Consistent key-based docket registration for local components.For resources,
.keyreturnsstr(self.uri), and for templates,.keyreturnsself.uri_template. This aligns docket registrations with the canonical identifiers used for lookups, ensuring tasks are correctly dispatched.Also applies to: 452-452
461-463: LGTM! Consistent key-based docket registration for provider components.Provider resources and templates now use
.keyfor docket registration, matching the local component approach and ensuring uniform task dispatch behavior.src/fastmcp/server/tasks/handlers.py (3)
102-107: LGTM! Tool task queuing uses canonical key.Using
tool.keyensures the docket lookup matches the registered name, correctly handling prefixed tools from mounted servers.
208-213: LGTM! Prompt task queuing uses canonical key.Consistent with the tool handler, using
prompt.keyensures proper dispatch for both local and mounted prompts.
312-326: LGTM! Resource task queuing uses canonical key.Both template-based and direct resource paths now use
resource.keyfor docket dispatch, aligning with the key-based registration in_docket_lifespan.
- .key is now a read-only computed property:
- Tools/Prompts: returns .name
- Resources: returns str(.uri)
- Templates: returns .uri_template
- Prefixing uses model_copy(update={...}) to change underlying field
- Resource/template names are NOT prefixed, only URIs
- Move import_server tests to tests/deprecated/
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/fastmcp/server/providers/mounted.py (1)
199-205: LGTM! URI template prefixing correctly implemented.The method properly prefixes only the
uri_templatefield usingmodel_copy(update={"uri_template": new_template}). The docstring correctly clarifies that the name field is not prefixed.Optional: Minor redundancy in condition
Line 201 checks
if self.prefix and template.uri_template:, buturi_templateis a required field inResourceTemplate, so the second condition is always true. You could simplify to:- if self.prefix and template.uri_template: + if self.prefix:This doesn't affect correctness, just a minor simplification.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (8)
tests/deprecated/__init__.pyis excluded by none and included by nonetests/deprecated/conftest.pyis excluded by none and included by nonetests/deprecated/test_import_server.pyis excluded by none and included by nonetests/resources/test_resource_manager.pyis excluded by none and included by nonetests/server/test_mount.pyis excluded by none and included by nonetests/server/test_server.pyis excluded by none and included by nonetests/tools/test_tool_manager.pyis excluded by none and included by nonetests/utilities/test_components.pyis excluded by none and included by none
📒 Files selected for processing (6)
src/fastmcp/resources/resource.py(1 hunks)src/fastmcp/resources/resource_manager.py(2 hunks)src/fastmcp/resources/template.py(1 hunks)src/fastmcp/server/providers/mounted.py(1 hunks)src/fastmcp/server/server.py(3 hunks)src/fastmcp/utilities/components.py(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/fastmcp/resources/resource_manager.py
🚧 Files skipped from review as they are similar to previous changes (1)
- src/fastmcp/server/server.py
🧰 Additional context used
📓 Path-based instructions (1)
src/fastmcp/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
src/fastmcp/**/*.py: Write Python code with ≥3.10 type annotations required throughout
Never use bare except - be specific with exception types
Files:
src/fastmcp/resources/resource.pysrc/fastmcp/resources/template.pysrc/fastmcp/server/providers/mounted.pysrc/fastmcp/utilities/components.py
🧠 Learnings (2)
📚 Learning: 2025-12-17T03:06:14.522Z
Learnt from: CR
Repo: jlowin/fastmcp PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-17T03:06:14.522Z
Learning: Applies to src/fastmcp/**/__init__.py : Be intentional about re-exports; core types that define a module's purpose should be exported; specialized features can live in submodules; only re-export to fastmcp.* for fundamental types
Applied to files:
src/fastmcp/utilities/components.py
📚 Learning: 2025-12-17T03:06:14.522Z
Learnt from: CR
Repo: jlowin/fastmcp PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-17T03:06:14.522Z
Learning: Applies to src/fastmcp/**/*.py : Write Python code with ≥3.10 type annotations required throughout
Applied to files:
src/fastmcp/utilities/components.py
🧬 Code graph analysis (2)
src/fastmcp/server/providers/mounted.py (4)
src/fastmcp/server/proxy.py (4)
model_copy(289-296)model_copy(377-384)model_copy(448-455)model_copy(540-547)src/fastmcp/resources/resource.py (1)
Resource(133-278)src/fastmcp/resources/template.py (1)
ResourceTemplate(92-223)src/fastmcp/prompts/prompt.py (1)
Prompt(113-230)
src/fastmcp/utilities/components.py (1)
src/fastmcp/server/server.py (1)
name(362-363)
⏰ 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). (2)
- GitHub Check: Run tests with lowest-direct dependencies
- GitHub Check: Run tests: Python 3.10 on windows-latest
🔇 Additional comments (6)
src/fastmcp/utilities/components.py (1)
59-68: LGTM! Clean simplification of the key property.The shift to a computed
.keyproperty that returnsself.nameby default is much cleaner than maintaining a stored_keyattribute. The docstring clearly documents the override pattern for subclasses.src/fastmcp/resources/resource.py (1)
276-278: LGTM! URI-based key is the right identifier for resources.The key property correctly returns
str(self.uri)as the lookup identifier for resources, which aligns with the resource identification model.src/fastmcp/resources/template.py (1)
221-223: LGTM! URI template-based key is consistent with resource identification.The key property correctly returns
self.uri_templateas the lookup identifier for templates, maintaining consistency with the Resource pattern of URI-based identification.src/fastmcp/server/providers/mounted.py (3)
181-189: LGTM! Prefixing logic correctly uses model_copy with update.The method properly handles both tool_names overrides and default prefixing, using
model_copy(update={"name": new_name})to update the name field. The optimization to return the original tool when unchanged is good. The updated docstring correctly notes that name changes affect.key.
191-197: LGTM! URI prefixing correctly updates the uri field.The method properly prefixes only the URI (not the name), using
model_copy(update={"uri": new_uri}). Pydantic will validate and coerce the string toAnyUrl. The docstring correctly clarifies that the name field is not prefixed.
207-212: LGTM! Prompt prefixing follows the same pattern as tool prefixing.The method correctly updates the prompt name using
model_copy(update={"name": new_name}), with proper optimization to return the original when unchanged. The docstring accurately notes that name changes affect.key.
Test Failure AnalysisSummary: Integration test timeout caused by GitHub API rate limiting (429 error), not a code issue. Root Cause: The test The test appears to hang waiting for a response from GitHub's MCP API, which is rate limiting requests. This is an external API issue, not a bug in the PR's changes. Suggested Solution: The PR changes themselves are sound - they simplify the Options to address this:
Since this PR is already merged and the failure is environmental (not caused by the code changes), no immediate action is required unless this becomes a recurring issue. Detailed AnalysisError Location: tests/integration_tests/test_github_mcp_remote.py:99 Stack Trace: The timeout occurs in the asyncio event loop while waiting for the GitHub API response. The 429 error appears during session teardown ( PR Changes Review: The changes in this PR modify how Related Files
|
Test Failure AnalysisSummary: The Windows test suite is timing out (5s timeout) during initialization of in the test due to SQLite database locking issues. Root Cause: The test creates an which initializes a at line 821 of . On Windows, the initialization is hitting SQLite database locking contention, likely due to:
Suggested Solution: This issue is not directly related to the PR changes (which focused on simplifying the
Recommendation: Option 2 (in-memory storage for tests) is cleanest and will make tests faster and more reliable across all platforms. Detailed AnalysisTest that timed out: Stack trace location: The timeout occurs when Why this happens on Windows: Windows uses mandatory file locking (the OS enforces locks), while Unix systems use advisory locking (applications cooperate). This makes Windows more susceptible to SQLite locking issues in parallel test scenarios. References: Related Files
|
Test Failure AnalysisSummary: The Windows test suite is timing out (5s timeout) during initialization of Root Cause: The test creates an
Suggested Solution: This issue is not directly related to the PR changes (which focused on simplifying the
Recommendation: Option 2 (in-memory storage for tests) is cleanest and will make tests faster and more reliable across all platforms. Detailed AnalysisTest that timed out: Stack trace location: The timeout occurs when Why this happens on Windows: Windows uses mandatory file locking (the OS enforces locks), while Unix systems use advisory locking (applications cooperate). This makes Windows more susceptible to SQLite locking issues in parallel test scenarios. All tests are using the same References: Related Files
|
|
@jlowin I see this merged into 3.X branch. Any recommendations for folks running on 2.14.X where prefix is broken? |
|
This wasn't a bug fix. What issue are you referring to? |
|
Oh I see - that was an inadvertent outcome of this change. We're not planning to back port this, it would be complicated. I recommend using the 3.0 b1 if you need that middleware to work! |
|
:( just upgraded to .14.X from .10.X |
The
.keyproperty now serves as a universal lookup interface for all components, implemented as a computed property rather than a stored field.Before: Components had a
_keyprivate attribute and custommodel_copy(key=...)for setting keys separately from names/URIs. This created complexity and confusion about when to use.keyvs.name.After:
.keyis computed from the underlying identifier:.keyreturns.name.keyreturnsstr(.uri).keyreturns.uri_templateWhen prefixing components via
model_copy(update={"name": "prefixed_tool"}), the.keyautomatically updates. Resource/template names are NOT prefixed—only their URIs are prefixed.Also moves
import_servertests totests/deprecated/with warning suppression.