Convert mounted servers to MountedProvider#2635
Conversation
- Remove get_http_routes from Provider (unused) - Remove ProviderLifespanConfig, _base_lifespan, _register_tasks - Remove supports_tasks flag from Provider.__init__ - Consolidate all docket registration in server._docket_lifespan() - Simplify lifespan() to take no parameters - Move MountedProvider to separate module
marvin-context-protocol
Bot
added
enhancement
WalkthroughThe PR replaces the mounted-servers model with a provider-centric design: it adds a Provider interface and Components/TaskComponents containers, introduces MountedProvider to expose a mounted FastMCP server with optional prefixing and name mappings, and updates server and ComponentService logic to iterate over and delegate to providers instead of MountedServer instances. Provider lifespans drive lifecycle management, provider-provided tools/resources/templates/prompts/tasks are registered into the docket with prefixed keys, public provider exports moved under fastmcp.server.providers, and legacy MountedServer and resource-prefix utilities were removed. Possibly related PRs
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ 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 |
![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
🧹 Nitpick comments (2)
src/fastmcp/server/server.py (1)
884-901: Redundant exception handling for provider lookups.The
get_toolmethod catchesNotFoundErrorfrom providers, but per theProviderinterface contract,get_tool()returnsNonewhen not found (not raising an exception). The same pattern appears inget_resource,get_resource_template, andget_prompt.While this defensive coding won't cause issues, it's inconsistent with the documented provider semantics: "Return
Nonefromget_*methods to indicate 'I don't have it'".If you want to keep defensive handling, consider catching a broader
Exceptionto handle provider implementation bugs, or simplify to only check forNone:- except NotFoundError: - continue + except Exception: + continue # Provider failed, try nextOr trust the provider contract:
for provider in self._providers: - try: - tool = await provider.get_tool(key) - if tool is not None: - return tool - except NotFoundError: - continue + tool = await provider.get_tool(key) + if tool is not None: + return toolsrc/fastmcp/providers/mounted.py (1)
218-236: Potential performance concern withget_resources()for single lookup.The
get_resource()method callsself.server.get_resources()which retrieves all resources just to check if one exists. For servers with many resources, this could be inefficient.However, I understand this is intentional to preserve the distinction between concrete resources and template-generated resources for task execution purposes.
If performance becomes a concern, consider using the resource manager directly:
# More efficient: check resource manager directly if unprefixed not in self.server._resource_manager._resources: return None resource = self.server._resource_manager._resources[unprefixed]This mirrors what
get_tasks()does for direct manager access.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (4)
tests/client/test_client.pyis excluded by none and included by nonetests/server/test_mount.pyis excluded by none and included by nonetests/server/test_providers.pyis excluded by none and included by nonetests/tools/test_tool_manager.pyis excluded by none and included by none
📒 Files selected for processing (7)
examples/providers/sqlite/README.md(2 hunks)examples/providers/sqlite/setup_db.py(1 hunks)src/fastmcp/contrib/component_manager/component_service.py(7 hunks)src/fastmcp/providers/__init__.py(1 hunks)src/fastmcp/providers/base.py(12 hunks)src/fastmcp/providers/mounted.py(1 hunks)src/fastmcp/server/server.py(21 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
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/providers/__init__.pysrc/fastmcp/contrib/component_manager/component_service.pysrc/fastmcp/providers/mounted.pysrc/fastmcp/providers/base.pysrc/fastmcp/server/server.py
src/fastmcp/**/__init__.py
📄 CodeRabbit inference engine (AGENTS.md)
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
Files:
src/fastmcp/providers/__init__.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/providers/__init__.py
📚 Learning: 2025-11-26T21:51:44.174Z
Learnt from: CR
Repo: jlowin/fastmcp PR: 0
File: .cursor/rules/core-mcp-objects.mdc:0-0
Timestamp: 2025-11-26T21:51:44.174Z
Learning: Review and update related Manager classes (ToolManager, ResourceManager, PromptManager) when modifying MCP object definitions
Applied to files:
src/fastmcp/providers/base.py
🪛 Ruff (0.14.8)
src/fastmcp/providers/mounted.py
195-195: Consider moving this statement to an else block
(TRY300)
299-299: Consider moving this statement to an else block
(TRY300)
src/fastmcp/server/server.py
901-901: Avoid specifying long messages outside the exception class
(TRY003)
991-991: Avoid specifying long messages outside the exception class
(TRY003)
1031-1031: Avoid specifying long messages outside the exception class
(TRY003)
1071-1071: Avoid specifying long messages outside the exception class
(TRY003)
1592-1592: Avoid specifying long messages outside the exception class
(TRY003)
1593-1593: Avoid specifying long messages outside the exception class
(TRY003)
1704-1704: Avoid specifying long messages outside the exception class
(TRY003)
1705-1705: Avoid specifying long messages outside the exception class
(TRY003)
1723-1723: Avoid specifying long messages outside the exception class
(TRY003)
1724-1724: Avoid specifying long messages outside the exception class
(TRY003)
1828-1828: Avoid specifying long messages outside the exception class
(TRY003)
1829-1829: 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.13 on ubuntu-latest
- GitHub Check: Run tests with lowest-direct dependencies
- GitHub Check: Run tests: Python 3.10 on ubuntu-latest
- GitHub Check: Run tests: Python 3.10 on windows-latest
🔇 Additional comments (16)
examples/providers/sqlite/setup_db.py (1)
7-7: Update aligns with new provider path structure.The docstring update correctly reflects the new file location and is consistent with the provider-centric restructuring introduced in this PR.
examples/providers/sqlite/README.md (1)
15-15: Consistent path updates reflect new provider directory structure.All path references have been correctly updated from
examples/dynamic_tools_sqlite/toexamples/providers/sqlite/, maintaining consistency across usage examples, setup instructions, and runtime modification examples.Also applies to: 18-18, 53-53, 56-56
src/fastmcp/providers/__init__.py (1)
1-34: Well-structured module initialization.The re-exports are appropriate for this module's purpose -
Components,Provider, andMountedProviderare core types that define the providers abstraction. The docstring example demonstrates the intended usage pattern clearly.src/fastmcp/server/server.py (5)
478-492: Type safety concern withtype: ignorecomments on.fnaccess.The code accesses
.fnattribute on tools, resources, templates, and prompts returned fromprovider.get_tasks(), but these are typed as baseTool,Resource, etc. The# type: ignore[attr-defined]comments suppress the error, but if a provider returns non-function-based components fromget_tasks(), this will fail at runtime.The base
Provider.get_tasks()already filters forFunctionTool,FunctionResource, etc., but custom provider implementations could return non-function types.Consider adding runtime checks or narrowing the type annotations in
Components:# Option 1: Add runtime check before access for tool in tasks.tools: if hasattr(tool, 'fn'): named_fn = _create_named_fn_wrapper(tool.fn, tool.key) docket.register(named_fn)Alternatively, the
Componentsdataclass could use more specific types likeSequence[FunctionTool]if only function-based components are expected.
546-548: LGTM!The provider lifespan management using
AsyncExitStackis correct. Provider lifespans are entered in registration order and will be properly exited in reverse order when the stack unwinds.
1182-1208: LGTM!The deduplication logic correctly implements the documented semantics:
- Local (static) components take precedence for execution
- Later providers override earlier ones when there are duplicates
- Provider components appear first in the list for visibility
The comments clearly explain the intentional ordering behavior.
1572-1593: LGTM!The provider tool execution logic correctly:
- Uses
reversed()to give later providers precedence- Validates tool existence before calling
- Allows validation errors to propagate unmasked
- Wraps unexpected exceptions with optional detail masking
2699-2720: Clean deprecation handling foras_proxyparameter.The implementation properly:
- Emits a
DeprecationWarningwhenas_proxyis used- Still honors the flag for backward compatibility
- Documents the migration path in the warning message
src/fastmcp/providers/mounted.py (4)
57-77: LGTM!The initialization is clean with proper super() call. The reverse mapping for tool names enables efficient lookup when stripping prefixes.
89-108: LGTM!The
_strip_tool_prefixmethod correctly handles:
- Tool name overrides via
_reverse_tool_names- Prefix pattern matching
- No-prefix case (always matches)
Returning
Noneto signal "not this provider's responsibility" aligns with the Provider interface semantics.
360-370: LGTM!The recursive task collection correctly handles nested providers. Each level applies its own prefix, so deeply nested mounts get properly prefixed at each level (e.g.,
parent_child_grandchild_tool).
376-387: LGTM!The lifespan correctly invokes only the wrapped server's user-defined lifespan, not its full lifecycle manager. This avoids duplicate Docket/Worker creation since the parent server handles all background task infrastructure.
src/fastmcp/providers/base.py (4)
41-51: LGTM!The
Componentsdataclass provides a clean container for task-eligible components. UsingSequencewith empty tuple defaults is appropriate for immutability and flexibility.
54-72: Well-documented Provider interface.The docstring clearly explains:
- The
Nonereturn convention for "not found"- Precedence rules (static components win)
- Error handling semantics (list_* degrades gracefully, execution methods propagate)
This makes the contract clear for custom provider implementations.
227-268: LGTM!The default
get_tasks()implementation correctly:
- Filters for function-based component types that can be registered with Docket
- Excludes components with
task_config.mode == "forbidden"- Uses
list_*methods which is appropriate for generic providersNote:
MountedProvideroverrides this to bypass middleware, which is the correct design for mounted servers.
274-298: LGTM!The default lifespan implementation is a no-op that can be overridden. The docstring provides a clear example of the expected pattern with proper try/finally for cleanup.
jlowin
changed the title
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
examples/providers/sqlite/server.py (1)
77-91: Method signatures don't match the updated Provider base class.The
list_toolsandget_toolmethods include acontext: Contextparameter, but the baseProviderclass (insrc/fastmcp/server/providers/base.py) has updated these methods to be context-free:
async def list_tools(self) -> Sequence[Tool]async def get_tool(self, name: str) -> Tool | NoneThis signature mismatch means these methods won't be called correctly by the provider infrastructure.
Apply this diff to fix the method signatures:
- async def list_tools(self, context: Context) -> list[Tool]: + async def list_tools(self) -> list[Tool]: async with aiosqlite.connect(self.db_path) as db: db.row_factory = aiosqlite.Row async with db.execute("SELECT * FROM tools WHERE enabled = 1") as cursor: rows = await cursor.fetchall() return [self._make_tool(row) for row in rows] - async def get_tool(self, context: Context, name: str) -> Tool | None: + async def get_tool(self, name: str) -> Tool | None: async with aiosqlite.connect(self.db_path) as db: db.row_factory = aiosqlite.Row async with db.execute(Also remove the unused
Contextimport from line 24 if no longer needed elsewhere.
🧹 Nitpick comments (4)
src/fastmcp/server/server.py (2)
1573-1594: Consider consolidating error handling patterns.The provider call_tool error handling duplicates the pattern from
_execute_tool. While this is functional, you might consider extracting a shared error handling utility in a future refactor. The current implementation is correct and the static analysis hints about TRY003 (long exception messages) can be safely ignored as the messages provide useful debugging context.
1690-1725: Provider resource/template reads iterate providers twice - potential optimization.The
_read_resourcemethod iterates through providers twice: once for concrete resources (lines 1691-1705) and once for templates (lines 1708-1724). If a provider is known to not have the resource, it will be queried again for templates.This is functionally correct but could be optimized in a future iteration by combining the lookups. For now, this is acceptable given the clarity of the current implementation.
src/fastmcp/server/providers/mounted.py (2)
73-77: Potential silent collision iftool_namescontains duplicate values.If two original tool names map to the same custom name (e.g.,
{"a": "x", "b": "x"}), only one entry survives in_reverse_tool_names. This could cause one tool to become unreachable when stripping prefixes.Consider validating uniqueness of values:
self.tool_names = tool_names or {} + if len(self.tool_names) != len(set(self.tool_names.values())): + raise ValueError("tool_names values must be unique") self._reverse_tool_names = {v: k for k, v in self.tool_names.items()}
183-207: Tool retrieval and execution logic is correct.The prefix-stripping before delegation and re-prefixing after retrieval ensures proper routing.
The static analysis hint at line 195 (TRY300) suggests moving
return toolinto anelseblock, which can marginally improve clarity but is stylistic:try: tool = await self.server.get_tool(unprefixed) # Return with prefixed key for parent's filter checking prefixed_key = name # The name we received is already the prefixed form if tool.key != prefixed_key: tool = tool.model_copy(key=prefixed_key) - return tool except NotFoundError: return None + else: + return tool
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
tests/server/test_mount.pyis excluded by none and included by nonetests/server/test_providers.pyis excluded by none and included by none
📒 Files selected for processing (7)
examples/providers/sqlite/server.py(1 hunks)src/fastmcp/__init__.py(0 hunks)src/fastmcp/contrib/component_manager/component_service.py(7 hunks)src/fastmcp/server/providers/__init__.py(1 hunks)src/fastmcp/server/providers/base.py(12 hunks)src/fastmcp/server/providers/mounted.py(1 hunks)src/fastmcp/server/server.py(22 hunks)
💤 Files with no reviewable changes (1)
- src/fastmcp/init.py
🧰 Additional context used
📓 Path-based instructions (2)
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/contrib/component_manager/component_service.pysrc/fastmcp/server/server.pysrc/fastmcp/server/providers/__init__.pysrc/fastmcp/server/providers/mounted.pysrc/fastmcp/server/providers/base.py
src/fastmcp/**/__init__.py
📄 CodeRabbit inference engine (AGENTS.md)
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
Files:
src/fastmcp/server/providers/__init__.py
🧠 Learnings (1)
📚 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/server/providers/__init__.py
🧬 Code graph analysis (3)
src/fastmcp/contrib/component_manager/component_service.py (2)
src/fastmcp/server/providers/mounted.py (1)
MountedProvider(25-387)src/fastmcp/server/server.py (8)
tool(1904-1919)tool(1922-1937)tool(1939-2081)has_resource_prefix(3056-3092)remove_resource_prefix(3012-3053)prompt(2295-2307)prompt(2310-2322)prompt(2324-2474)
examples/providers/sqlite/server.py (2)
src/fastmcp/server/context.py (1)
Context(117-1003)src/fastmcp/server/providers/base.py (1)
Provider(55-299)
src/fastmcp/server/providers/__init__.py (2)
src/fastmcp/server/providers/base.py (2)
Components(43-52)Provider(55-299)src/fastmcp/server/providers/mounted.py (1)
MountedProvider(25-387)
🪛 Ruff (0.14.8)
src/fastmcp/server/server.py
901-901: Avoid specifying long messages outside the exception class
(TRY003)
991-991: Avoid specifying long messages outside the exception class
(TRY003)
1031-1031: Avoid specifying long messages outside the exception class
(TRY003)
1071-1071: Avoid specifying long messages outside the exception class
(TRY003)
1592-1592: Avoid specifying long messages outside the exception class
(TRY003)
1593-1593: Avoid specifying long messages outside the exception class
(TRY003)
1704-1704: Avoid specifying long messages outside the exception class
(TRY003)
1705-1705: Avoid specifying long messages outside the exception class
(TRY003)
1723-1723: Avoid specifying long messages outside the exception class
(TRY003)
1724-1724: Avoid specifying long messages outside the exception class
(TRY003)
1828-1828: Avoid specifying long messages outside the exception class
(TRY003)
1829-1829: Avoid specifying long messages outside the exception class
(TRY003)
src/fastmcp/server/providers/mounted.py
195-195: Consider moving this statement to an else block
(TRY300)
299-299: 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.13 on ubuntu-latest
- GitHub Check: Run tests: Python 3.10 on windows-latest
- GitHub Check: Run tests with lowest-direct dependencies
🔇 Additional comments (22)
src/fastmcp/server/providers/base.py (4)
1-53: LGTM! Clean module structure and Components dataclass.The module docstring with example is helpful, and the
Componentsdataclass withSequencetypes and empty tuple defaults is well-designed for immutability and type safety.
55-93: LGTM! Provider base class with sensible defaults.The Provider class documentation clearly explains the semantics (return
Noneto indicate "I don't have it"), error handling behavior, and precedence rules. Default implementations returning empty lists/None allow subclasses to implement only what they need.
228-269: Consider whether filtering formode != "forbidden"is sufficient.The
get_tasks()method filters components wheretask_config.mode != "forbidden". This correctly excludes forbidden components from Docket registration. The logic is sound, though the imports inside the method suggest circular import avoidance - this is acceptable.
275-299: LGTM! Clean lifespan implementation.The
lifespan()async context manager follows the standard pattern with a simpleyield. The docstring clearly explains the expected usage for subclasses.examples/providers/sqlite/server.py (1)
23-25: LGTM! Import path updated correctly.The import path for
Providerhas been correctly updated to usefastmcp.server.providersinstead of the old location.src/fastmcp/contrib/component_manager/component_service.py (3)
10-11: LGTM! Import updated for provider-centric architecture.The import of
MountedProviderfrom the new providers module is correct.
44-55: LGTM! Provider-based tool routing with correct control flow.The logic correctly:
- Iterates providers in reverse order (later providers win)
- Filters for
MountedProviderinstances- Checks for prefix match before delegating
- Uses
else: continueto skip non-matching prefixes
108-121: LGTM! Resource routing control flow is now correct.The
else: continueat lines 119-120 is correctly aligned with the innerif has_resource_prefix(...)check, ensuring the loop continues to the next provider when the prefix doesn't match. This addresses the previously reported control flow issue.src/fastmcp/server/providers/__init__.py (1)
1-35: LGTM! Intentional public API surface for providers module.The module correctly exports the core types (
Components,Provider,MountedProvider) that define the provider abstraction. The__all__declaration makes the public API explicit. Based on learnings, this follows the guideline to be intentional about re-exports and export core types that define a module's purpose.src/fastmcp/server/server.py (5)
84-84: LGTM! Provider import updated to new module path.The import of
Providerfromfastmcp.server.providersaligns with the new provider-centric architecture.
478-492: LGTM! Provider task registration with proper name wrapping.The Docket registration for provider components correctly uses
_create_named_fn_wrapperto create unique function names, avoiding collisions between mounted servers with identically-named functions. Thetype: ignore[attr-defined]comments are justified sinceget_tasks()filters for function-based variants.
546-548: LGTM! Provider lifespans integrated into server lifecycle.Provider lifespans are correctly entered via the
AsyncExitStack, ensuring proper startup/shutdown sequencing with the server's lifespan.
1182-1208: LGTM! Correct deduplication logic - local components take precedence.The implementation correctly:
- Collects local tools first
- Collects provider tools (later providers override earlier)
- Removes provider tools that conflict with local tools
- Returns provider tools first in list for visibility, but local tools win for execution
2699-2720: LGTM! Clean deprecation path for as_proxy parameter.The deprecation warning is clear and provides migration guidance. The backward compatibility handling that creates a proxy when
as_proxy=Trueis a thoughtful transition path.src/fastmcp/server/providers/mounted.py (8)
1-23: Clean import structure with proper type checking guard.The use of
TYPE_CHECKINGto guard theFastMCPimport avoids circular dependencies at runtime while maintaining type safety.
83-133: Prefix handling logic is sound.The helper methods correctly handle the bidirectional prefix transformations. The local imports at lines 115 and 129 appropriately avoid circular dependencies.
139-172: Efficient prefix application with conditional copying.The
model_copyapproach avoids unnecessary object creation when no changes are needed. The pattern is consistent across all component types.
213-248: Resource methods correctly distinguish concrete resources from templates.The
get_resourcemethod explicitly documents that it only returns concrete resources (line 221-222), preserving templates for proper task execution. The middleware delegation inread_resourceis consistent with the tool pattern.
254-276: Template handling properly delegates to the server's middleware.The
read_resource_templatecorrectly delegates toread_resourcesince the server's middleware handles template resolution internally.
287-311: Prompt methods follow the same pattern as tools.The same TRY300 suggestion applies at line 299 as noted for the tool methods. The implementation is consistent and correct.
317-370: Direct manager access is intentional and well-documented.The docstring clearly explains the rationale for bypassing middleware during task registration. The recursive collection from nested providers (lines 361-366) properly handles hierarchical provider structures with correct prefix propagation.
376-387: Lifecycle correctly starts only the user-defined lifespan.The distinction between the user's lifespan and the full
_lifespan_manager()(which includes Docket) is important—the parent server handles background tasks centrally.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/fastmcp/server/providers/mounted.py (2)
57-77: Consider validating uniqueness oftool_namesvalues.If
tool_namescontains duplicate values (e.g.,{"foo": "bar", "baz": "bar"}), the reverse mapping at line 77 will silently overwrite, causing only one original name to be recoverable. This could lead to unexpected behavior when resolving custom names back to originals.If this is a concern, consider adding validation:
self.tool_names = tool_names or {} + if len(self.tool_names) != len(set(self.tool_names.values())): + raise ValueError("tool_names values must be unique") self._reverse_tool_names = {v: k for k, v in self.tool_names.items()}
305-358: Tight coupling via private attribute access is documented but worth noting.The direct access to internal manager dictionaries (
_tool_manager._tools,_resource_manager._resources, etc.) tightly couples this provider to the server's internal structure. The docstring explains this is intentional to bypass middleware during registration.Consider adding a dedicated non-middleware method on
FastMCP(e.g.,_get_raw_tasks()) to encapsulate this access pattern, reducing the surface area of private attribute dependencies:# In FastMCP class: def _get_raw_tasks(self) -> Components: """Return raw task-eligible components without middleware.""" # Centralized access to internal managers ...This would make future refactoring of internal structures easier.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/fastmcp/server/providers/mounted.py(1 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/providers/mounted.py
🧠 Learnings (1)
📓 Common learnings
Learnt from: CR
Repo: jlowin/fastmcp PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-17T03:06:14.522Z
Learning: Applies to tests/**/*.py : Pass FastMCP servers directly to clients for in-memory testing instead of using network transport
⏰ 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.13 on ubuntu-latest
- GitHub Check: Run tests: Python 3.10 on ubuntu-latest
- GitHub Check: Run tests: Python 3.10 on windows-latest
- GitHub Check: Run tests with lowest-direct dependencies
🔇 Additional comments (8)
src/fastmcp/server/providers/mounted.py (8)
1-23: LGTM!Clean module structure with proper
TYPE_CHECKINGguard for theFastMCPimport to avoid circular dependencies. Import organization is appropriate.
83-108: LGTM!The prefix handling logic correctly prioritizes
tool_namesoverrides before checking the prefix pattern, and the fallback for unprefixed providers (line 107-108) is appropriate.
183-199: LGTM!The early-exit optimization at line 186 prevents unnecessary middleware calls when the prefix doesn't match. The
get_toolimplementation correctly goes throughlist_tools()to ensure middleware participation as stated in the PR objectives.
234-244: LGTM!Good use of specific exception handling with
NotFoundErrorrather than a bareexcept. The fallback toNonewhen resource is not found allows the provider chain to continue searching.
255-272: LGTM!The template matching logic correctly uses the unprefixed URI against unprefixed templates from middleware, then returns the prefixed version. Delegation to
read_resourcefor reading is appropriate since the server's middleware handles template resolution internally.
278-299: LGTM!Prompt methods follow the same pattern as tools, consistently using underscore-separated prefixes. The middleware call ensures proper execution through the mounted server's chain.
364-375: LGTM!The lifespan context manager correctly delegates to the mounted server's user lifespan while allowing the parent server's Docket to handle background tasks. The docstring clearly explains this distinction.
139-172: Themodel_copycalls in these methods are valid. Pydantic v2model_copyaccepts anupdatedictionary for field changes, but FastMCPComponent overridesmodel_copywith a custom implementation that explicitly accepts akeyparameter. All four model classes (Tool, Resource, ResourceTemplate, Prompt) inherit from FastMCPComponent and therefore support this API. The code follows the codebase's design pattern correctly.Likely an incorrect or invalid review comment.
Test Failure AnalysisSummary: The test Root Cause: Commit 58d7484 modified Suggested Solution: Revert the
Why this fixes it: Resource templates need special handling during task execution. When Detailed AnalysisThe test failure occurs in: Test code (line 280-289): async def test_mounted_resource_template_task(self, parent_server):
async with Client(parent_server) as client:
task = await client.read_resource("child://child/item/99.json", task=True)
assert not task.returned_immediately
result = await task.result()
assert '"itemId": "99"' in result[0].textExecution flow that causes the hang:
Key insight from previous code (5466bb7): # Only check concrete resources (not templates that match the URI)
# This preserves the original template for task executionThis comment was removed in 58d7484, but the reasoning still applies. Related Files
|
Reverts template-checking in get_resource that broke task execution. Tasks need access to the original template, not instantiated resources.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/fastmcp/server/providers/mounted.py (1)
110-133: Inline imports resolve circular dependencies—consider extracting utilities to a separate module for improved maintainability.The imports at lines 115 and 129 work around a circular dependency between
mounted.pyandserver.py. While functional and well-documented, this pattern can make code harder to maintain and breaks static analysis tools. Consider movingadd_resource_prefix,has_resource_prefix, andremove_resource_prefixto a dedicated utility module (e.g.,fastmcp.server.resource_prefix_utils) to eliminate the circular dependency.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/fastmcp/server/providers/mounted.py(1 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/providers/mounted.py
🧠 Learnings (1)
📓 Common learnings
Learnt from: CR
Repo: jlowin/fastmcp PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-17T03:06:14.522Z
Learning: Applies to tests/**/*.py : Pass FastMCP servers directly to clients for in-memory testing instead of using network transport
⏰ 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). (5)
- GitHub Check: Run tests: Python 3.10 on ubuntu-latest
- GitHub Check: Run tests: Python 3.13 on ubuntu-latest
- GitHub Check: Run tests with lowest-direct dependencies
- GitHub Check: Run tests: Python 3.10 on windows-latest
- GitHub Check: Analyze (python)
🔇 Additional comments (7)
src/fastmcp/server/providers/mounted.py (7)
1-23: LGTM!The imports and module documentation are well-structured and clear.
83-108: LGTM!The prefix handling logic correctly supports both custom name mappings and standard prefixing, with proper early-exit routing for non-matching names.
178-199: LGTM!The tool methods correctly route through the mounted server's middleware chain while properly handling prefix matching and early exits for non-matching names.
224-234: LGTM!The resource reading correctly routes through middleware and handles errors gracefully. The code assumes
_read_resource_middlewarereturns a list, with line 232 extracting the first content item.
268-289: LGTM!The prompt methods follow the same consistent pattern as tools, properly routing through middleware with prefix handling.
354-365: LGTM!The lifespan method correctly starts only the user-defined lifespan, avoiding duplicate Docket initialization. The comment clearly explains the design decision.
139-145: Clarify prefixing behavior whentool_namesmapping is applied.The
tool_namesmapping allows direct customization of tool keys, but this bypasses the standard prefix logic. When a tool name is found intool_names, the mapped value is used directly asnew_keywithout applying_add_tool_prefix(). This differs from the fallback case and from_prefix_resource(), which always applies a prefix. Either apply the prefix to the mapped value for consistency, or update the docstring to clarify that the mapping takes precedence over prefixing.
- Add resource prefix functions (add/remove/has_resource_prefix) to mounted.py - Deprecate import_server with warning to use mount() instead - Add tool_names uniqueness validation in MountedProvider
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
src/fastmcp/server/server.py (2)
478-492: Inconsistent key selection for Docket registration may cause collisions.The code uses different attributes for different component types:
- Tools:
tool.key(line 482)- Resources:
resource.name(line 485)- Templates:
template.name(line 488)- Prompts:
prompt.key(line 491)For resources and templates, using
.nameinstead of.key(the URI) could cause collisions if multiple resources from different providers happen to have the same display name. Consider using.keyconsistently for all component types to ensure uniqueness.🔎 Suggested fix:
for resource in tasks.resources: - named_fn = _create_named_fn_wrapper(resource.fn, resource.name) # type: ignore[attr-defined] + named_fn = _create_named_fn_wrapper(resource.fn, resource.key) # type: ignore[attr-defined] docket.register(named_fn) for template in tasks.templates: - named_fn = _create_named_fn_wrapper(template.fn, template.name) # type: ignore[attr-defined] + named_fn = _create_named_fn_wrapper(template.fn, template.key) # type: ignore[attr-defined] docket.register(named_fn)
1690-1725: Consider combining the two provider loops for resources and templates.The code has two separate reverse iterations over providers: one for concrete resources (lines 1691-1705) and one for templates (lines 1708-1724). While functionally correct, this could be slightly more efficient by checking both in a single loop.
However, the current separation is clear and maintainable, so this is optional.
src/fastmcp/server/providers/mounted.py (1)
221-227: Consider efficiency ofget_toolimplementation.The current implementation calls
list_tools()(which invokes middleware and prefixes all tools) just to find a single tool. While functionally correct, this could be inefficient for servers with many tools.A potential optimization would be to directly look up the unprefixed name in the mounted server's tools and apply prefixing only to the result. However, this would require ensuring middleware is properly invoked for the single-tool case, which may not be worth the added complexity.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
tests/server/test_server.pyis excluded by none and included by none
📒 Files selected for processing (3)
src/fastmcp/contrib/component_manager/component_service.py(7 hunks)src/fastmcp/server/providers/mounted.py(1 hunks)src/fastmcp/server/server.py(24 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/providers/mounted.pysrc/fastmcp/contrib/component_manager/component_service.py
🧬 Code graph analysis (2)
src/fastmcp/server/server.py (2)
src/fastmcp/server/providers/base.py (14)
Provider(55-299)get_tasks(228-269)lifespan(276-299)get_prompt(196-206)list_tools(75-80)get_tool(82-92)get_resource(117-127)get_resource_template(136-150)list_resources(110-115)list_resource_templates(129-134)list_prompts(189-194)call_tool(94-108)read_resource(152-167)render_prompt(208-222)src/fastmcp/server/providers/mounted.py (15)
get_tasks(333-386)lifespan(393-403)get_prompt(311-317)list_tools(216-219)get_tool(221-227)get_resource(248-260)get_resource_template(283-295)list_resources(243-246)list_resource_templates(278-281)list_prompts(306-309)call_tool(229-237)read_resource(262-272)render_prompt(319-327)MountedProvider(67-403)add_resource_prefix(29-37)
src/fastmcp/contrib/component_manager/component_service.py (2)
src/fastmcp/server/context.py (1)
fastmcp(169-174)src/fastmcp/server/providers/mounted.py (3)
MountedProvider(67-403)has_resource_prefix(55-64)remove_resource_prefix(40-52)
🪛 Ruff (0.14.8)
src/fastmcp/server/server.py
901-901: Avoid specifying long messages outside the exception class
(TRY003)
991-991: Avoid specifying long messages outside the exception class
(TRY003)
1031-1031: Avoid specifying long messages outside the exception class
(TRY003)
1071-1071: Avoid specifying long messages outside the exception class
(TRY003)
1592-1592: Avoid specifying long messages outside the exception class
(TRY003)
1593-1593: Avoid specifying long messages outside the exception class
(TRY003)
1704-1704: Avoid specifying long messages outside the exception class
(TRY003)
1705-1705: Avoid specifying long messages outside the exception class
(TRY003)
1723-1723: Avoid specifying long messages outside the exception class
(TRY003)
1724-1724: Avoid specifying long messages outside the exception class
(TRY003)
1828-1828: Avoid specifying long messages outside the exception class
(TRY003)
1829-1829: Avoid specifying long messages outside the exception class
(TRY003)
src/fastmcp/server/providers/mounted.py
35-35: Avoid specifying long messages outside the exception class
(TRY003)
46-46: Avoid specifying long messages outside the exception class
(TRY003)
61-61: Avoid specifying long messages outside the exception class
(TRY003)
120-120: 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 windows-latest
- GitHub Check: Run tests: Python 3.10 on ubuntu-latest
- GitHub Check: Run tests: Python 3.13 on ubuntu-latest
- GitHub Check: Run tests with lowest-direct dependencies
🔇 Additional comments (14)
src/fastmcp/contrib/component_manager/component_service.py (3)
10-12: LGTM on imports.The imports correctly bring in
MountedProviderfrom the providers module and the prefix utility functions from mounted.py.
109-122: Control flow is now correct.The
else: continueblock at lines 120-121 is properly aligned with the innerif has_resource_prefix(...)check, ensuring that when a provider has a prefix but the key doesn't match, the loop continues to the next provider. This is consistent with the tool and prompt methods.
177-188: LGTM on prompt enable/disable logic.The control flow is consistent with the tool methods, properly continuing to the next provider when the prefix doesn't match.
src/fastmcp/server/server.py (5)
546-548: LGTM on provider lifespan management.Using
AsyncExitStackensures proper startup in registration order and cleanup in reverse order when the server shuts down.
740-743: LGTM on defensive prompt lookup.Wrapping
get_promptin a try/except allows the handler to gracefully continue when the prompt doesn't exist, matching the check pattern used elsewhere.
1182-1208: Clear and well-documented aggregation logic.The implementation correctly:
- Gives local tools precedence over provider tools
- Allows later providers to override earlier ones (forward iteration)
- Returns provider tools first in the list for visibility
The code is well-commented and the behavior is clear.
1572-1594: LGTM on provider tool execution.The reverse iteration ensures later providers take precedence (matching the "last wins" semantics). Error handling properly distinguishes validation errors (never masked) from execution errors (optionally masked).
2762-2770: LGTM on deprecation handling.The deprecation warning correctly guides users to use
mount()instead while maintaining backward compatibility for existing code.src/fastmcp/server/providers/mounted.py (6)
29-64: LGTM on URI prefix utility functions.The implementations correctly:
- Validate URI format before processing
- Use
re.escape()to prevent regex injection- Handle edge cases (empty prefix returns original URI)
- Return the original URI if prefix doesn't match in
remove_resource_prefix
115-121: LGTM on initialization with uniqueness validation.The validation at lines 119-120 correctly ensures
tool_namesvalues are unique before building the reverse mapping, preventing silent overwrites that would cause unpredictable tool resolution. This addresses the previous review feedback.
177-183: LGTM on tool prefixing logic.The method correctly:
- Checks for explicit name overrides first via
tool_names- Falls back to standard prefixing using
_add_tool_prefix- Avoids unnecessary copies when the key doesn't change
333-386: Direct manager access is intentional for task registration.The comment at lines 336-338 explains that direct manager access is intentional to avoid triggering middleware during Docket registration. While this creates coupling to internal structures (as noted in past reviews), it's a pragmatic trade-off.
The implementation correctly:
- Filters for function-based components only
- Respects
task_config.mode != "forbidden"- Recursively collects from nested providers with proper prefixing
392-403: LGTM on lifespan management.The implementation correctly starts only the mounted server's user-defined lifespan, leaving Docket management to the parent server. This prevents duplicate task registration and worker conflicts.
268-272: SwallowingNotFoundErroris intentional but consider logging.The
try/exceptcatchesNotFoundErrorand returnsNone, allowing the parent server to try other providers. This is correct for the provider chain pattern.However, other exceptions from
_read_resource_middlewarewould propagate up, which is appropriate for actual errors vs. "not found" situations.
- Remove unused _is_mounted flag (MountedProvider.lifespan() calls _lifespan not _lifespan_manager, so the flag was never checked) - Fix provider iteration: change reversed() to forward order in execution methods (_call_tool, _read_resource_middleware, _get_prompt_content_middleware) to match documented "first non-None wins" semantics - Fix ComponentService to handle prefix-less mounted servers using _strip_tool_prefix()/_strip_resource_prefix() methods - Update conflict resolution tests to expect first-registered provider wins - Add regression tests for Docket behavior and prefix-less ComponentService
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
src/fastmcp/server/server.py (2)
874-891: Provider iteration order is now consistent with_call_tool.The previous review flagged an inconsistency where
get_toolused forward order while_call_toolusedreversed(). Looking at the current code, both methods now iterateself._providersin forward order (lines 883 and 1563), which resolves the previously flagged issue. The "first registered provider wins" semantics are now consistent.
2708-2710: Missing_is_mountedflag on mounted server.The
mount()method creates aMountedProviderbut doesn't setserver._is_mounted = True. While the new provider-based architecture may handle this differently, setting this flag would:
- Allow code to detect whether a server is mounted
- Prevent issues if the mounted server is accidentally run independently later
#!/bin/bash # Check if _is_mounted is used elsewhere in the codebase rg -n '_is_mounted' --type=py
🧹 Nitpick comments (2)
src/fastmcp/server/server.py (2)
468-482: Missing exception handling for provider task registration.The provider iteration for Docket registration lacks exception handling, unlike the similar patterns in
get_tools,get_resources, etc. which catch exceptions and optionally continue based onmounted_components_raise_on_load_error. Ifprovider.get_tasks()fails, the entire server startup will fail.Consider adding consistent error handling:
🔎 Suggested fix:
# Register provider components for provider in self._providers: - tasks = await provider.get_tasks() - for tool in tasks.tools: - named_fn = _create_named_fn_wrapper(tool.fn, tool.key) # type: ignore[attr-defined] - docket.register(named_fn) - for resource in tasks.resources: - named_fn = _create_named_fn_wrapper(resource.fn, resource.name) # type: ignore[attr-defined] - docket.register(named_fn) - for template in tasks.templates: - named_fn = _create_named_fn_wrapper(template.fn, template.name) # type: ignore[attr-defined] - docket.register(named_fn) - for prompt in tasks.prompts: - named_fn = _create_named_fn_wrapper(prompt.fn, prompt.key) # type: ignore[attr-defined] - docket.register(named_fn) + try: + tasks = await provider.get_tasks() + for tool in tasks.tools: + named_fn = _create_named_fn_wrapper(tool.fn, tool.key) # type: ignore[attr-defined] + docket.register(named_fn) + for resource in tasks.resources: + named_fn = _create_named_fn_wrapper(resource.fn, resource.name) # type: ignore[attr-defined] + docket.register(named_fn) + for template in tasks.templates: + named_fn = _create_named_fn_wrapper(template.fn, template.name) # type: ignore[attr-defined] + docket.register(named_fn) + for prompt in tasks.prompts: + named_fn = _create_named_fn_wrapper(prompt.fn, prompt.key) # type: ignore[attr-defined] + docket.register(named_fn) + except Exception as e: + provider_name = getattr(provider, "server", provider).__class__.__name__ + logger.warning(f"Failed to register tasks from provider {provider_name!r}: {e}") + if fastmcp.settings.mounted_components_raise_on_load_error: + raise
1680-1715: Consider consolidating provider resource and template lookup.The two separate loops (lines 1681-1695 for resources, 1698-1714 for templates) have nearly identical exception handling. While functionally correct, this could be simplified. However, the current implementation is clear and the separation makes the priority chain explicit.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
tests/server/test_mount.pyis excluded by none and included by none
📒 Files selected for processing (2)
src/fastmcp/contrib/component_manager/component_service.py(7 hunks)src/fastmcp/server/server.py(24 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/fastmcp/contrib/component_manager/component_service.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/server/server.py
🪛 Ruff (0.14.8)
src/fastmcp/server/server.py
891-891: Avoid specifying long messages outside the exception class
(TRY003)
981-981: Avoid specifying long messages outside the exception class
(TRY003)
1021-1021: Avoid specifying long messages outside the exception class
(TRY003)
1061-1061: Avoid specifying long messages outside the exception class
(TRY003)
1582-1582: Avoid specifying long messages outside the exception class
(TRY003)
1583-1583: Avoid specifying long messages outside the exception class
(TRY003)
1694-1694: Avoid specifying long messages outside the exception class
(TRY003)
1695-1695: Avoid specifying long messages outside the exception class
(TRY003)
1713-1713: Avoid specifying long messages outside the exception class
(TRY003)
1714-1714: Avoid specifying long messages outside the exception class
(TRY003)
1818-1818: Avoid specifying long messages outside the exception class
(TRY003)
1819-1819: 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/server.py (12)
115-129: LGTM! Clean helper for Docket name mapping.The wrapper correctly preserves the original function's metadata via
@functools.wrapswhile allowing a custom__name__for Docket registration. The local import offunctoolsis acceptable given this is a rarely-called utility.
536-538: LGTM! Provider lifespans correctly managed.Using
AsyncExitStackensures proper cleanup of provider lifespans in reverse order on exit, which is the expected behavior for nested context managers.
730-733: LGTM! Specific exception handling.Catching
NotFoundErrorspecifically (rather than a bareexcept) correctly follows the coding guidelines and allows the handler to fall through to subsequent logic.
904-941: LGTM! Clear fallback chain for resource/template lookup.The method correctly implements a priority chain: local resources → local templates → provider resources → provider templates. Catching
NotFoundErrorspecifically and continuing is the right approach.
1172-1198: LGTM! Correct conflict resolution semantics.The implementation correctly implements:
- Local tools always win over provider tools (line 1193-1194 removes conflicts)
- Among providers, later providers override earlier ones (line 1186)
- Provider tools appear first in the list for visibility, but local tools take execution precedence
The exception handling with
except Exceptionpluslogger.exceptionis appropriate for this aggregation pattern.
1552-1584: LGTM! Robust tool execution with proper error handling.The execution flow correctly:
- Prioritizes local tools (first-registered wins)
- Uses forward provider iteration (consistent with
get_tool)- Handles errors appropriately: validation errors are never masked, while other exceptions respect
_mask_error_detailsThe
ToolErrorwrapping pattern with conditional detail masking is a good security practice.
1792-1819: LGTM! Consistent prompt execution pattern.The prompt execution follows the same well-structured pattern as tool execution: local-first lookup, forward provider iteration, and proper exception handling with
_mask_error_detailssupport.
2712-2760: LGTM! Proper deprecation pattern.The deprecation warning with
stacklevel=2correctly points to the caller. The method remains functional for backward compatibility while guiding users towardmount().
2693-2706: LGTM! Clean deprecation with backward compatibility.The
as_proxydeprecation correctly:
- Emits a warning with guidance on the new approach
- Still honors the flag for backward compatibility
- Provides a clear migration path
1004-1021: LGTM! Consistent resource template lookup.The method correctly implements local-first lookup with provider fallback, maintaining consistency with other getter methods.
84-84: LGTM! Clean import for new provider module.The import correctly brings in
Providerfrom the new providers package.
1113-1122: Remove misleading docstring reference to provider HTTP routes.The docstring states this method returns routes "including from providers," but providers don't expose HTTP routes and the implementation only returns locally registered routes. Update the docstring to reflect the actual behavior.
…stration - Create TaskComponents dataclass with FunctionTool/FunctionResource/etc. types for proper typing of get_tasks() return value - Add try/except wrapper around provider.get_tasks() in _docket_lifespan for consistent error handling (warn + continue or raise based on settings) - Remove type: ignore comments from server.py task registration loop
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/fastmcp/server/providers/mounted.py (1)
337-390: Direct access to private manager internals breaks encapsulation.Lines 355, 359, 366, and 373 directly access private attributes like
self.server._tool_manager._tools, bypassing encapsulation. While the comment on line 354 explains this is intentional to avoid middleware, this creates tight coupling and fragility.Based on the past review comment, consider adding a public method on FastMCP or the managers to retrieve task-eligible components:
🔎 Suggested approach
# On FastMCP async def get_task_eligible_components(self) -> TaskComponents: """Return components eligible for task registration (bypassing middleware).""" from fastmcp.prompts.prompt import FunctionPrompt from fastmcp.resources.resource import FunctionResource from fastmcp.resources.template import FunctionResourceTemplate from fastmcp.tools.tool import FunctionTool tools = [ t for t in self._tool_manager._tools.values() if isinstance(t, FunctionTool) and t.task_config.mode != "forbidden" ] resources = [ r for r in self._resource_manager._resources.values() if isinstance(r, FunctionResource) and r.task_config.mode != "forbidden" ] templates = [ t for t in self._resource_manager._templates.values() if isinstance(t, FunctionResourceTemplate) and t.task_config.mode != "forbidden" ] prompts = [ p for p in self._prompt_manager._prompts.values() if isinstance(p, FunctionPrompt) and p.task_config.mode != "forbidden" ] return TaskComponents(tools=tools, resources=resources, templates=templates, prompts=prompts)Then in MountedProvider.get_tasks():
async def get_tasks(self) -> TaskComponents: """Return task-eligible components with prefixes applied.""" components = await self.server.get_task_eligible_components() tools = [self._prefix_tool(t) for t in components.tools] resources = [self._prefix_resource(r) for r in components.resources] templates = [self._prefix_template(t) for t in components.templates] prompts = [self._prefix_prompt(p) for p in components.prompts] # Recursively get tasks from nested providers for provider in self.server._providers: nested = await provider.get_tasks() tools.extend(self._prefix_tool(t) for t in nested.tools) resources.extend(self._prefix_resource(r) for r in nested.resources) templates.extend(self._prefix_template(t) for t in nested.templates) prompts.extend(self._prefix_prompt(p) for p in nested.prompts) return TaskComponents(tools=tools, resources=resources, templates=templates, prompts=prompts)
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/fastmcp/server/providers/__init__.py(1 hunks)src/fastmcp/server/providers/base.py(13 hunks)src/fastmcp/server/providers/mounted.py(1 hunks)src/fastmcp/server/server.py(24 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/fastmcp/server/providers/init.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/server/server.pysrc/fastmcp/server/providers/mounted.pysrc/fastmcp/server/providers/base.py
🪛 Ruff (0.14.8)
src/fastmcp/server/server.py
905-905: Avoid specifying long messages outside the exception class
(TRY003)
995-995: Avoid specifying long messages outside the exception class
(TRY003)
1035-1035: Avoid specifying long messages outside the exception class
(TRY003)
1075-1075: Avoid specifying long messages outside the exception class
(TRY003)
1596-1596: Avoid specifying long messages outside the exception class
(TRY003)
1597-1597: Avoid specifying long messages outside the exception class
(TRY003)
1708-1708: Avoid specifying long messages outside the exception class
(TRY003)
1709-1709: Avoid specifying long messages outside the exception class
(TRY003)
1727-1727: Avoid specifying long messages outside the exception class
(TRY003)
1728-1728: Avoid specifying long messages outside the exception class
(TRY003)
1832-1832: Avoid specifying long messages outside the exception class
(TRY003)
1833-1833: Avoid specifying long messages outside the exception class
(TRY003)
src/fastmcp/server/providers/mounted.py
39-39: Avoid specifying long messages outside the exception class
(TRY003)
50-50: Avoid specifying long messages outside the exception class
(TRY003)
65-65: Avoid specifying long messages outside the exception class
(TRY003)
124-124: 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.13 on ubuntu-latest
- GitHub Check: Run tests: Python 3.10 on ubuntu-latest
- GitHub Check: Run tests: Python 3.10 on windows-latest
- GitHub Check: Run tests with lowest-direct dependencies
🔇 Additional comments (6)
src/fastmcp/server/providers/mounted.py (2)
103-125: LGTM!The initialization logic is correct. The uniqueness validation for
tool_namesvalues (lines 123-124) properly addresses the past review concern, ensuring the reverse mapping won't have collisions.
396-407: LGTM!The lifespan implementation correctly starts only the mounted server's user-defined lifespan (line 406), avoiding double initialization of Docket and allowing the parent server to manage background tasks.
src/fastmcp/server/server.py (3)
867-905: LGTM!The tool lookup methods correctly prioritize local tools over provider tools, and use consistent forward iteration order for provider queries. The error handling allows graceful degradation when providers fail to load.
2726-2819: Deprecation properly implemented.The deprecation warning (lines 2770-2774) and documentation clearly guide users toward using
mount()instead. The import fromfastmcp.server.providers.mounted(line 2768) maintains functionality during the deprecation period.
2657-2724: Remove the outdated_is_mountedsuggestion.The
_is_mountedflag suggested in the past review comment does not exist in the codebase and is unnecessary with the current provider-centric architecture. Mounted servers are now tracked via theMountedProviderandself._providerslist. Settingserver._is_mounted = Truewould add dead code that is never referenced or checked. Themount()implementation is already complete and correct as-is.Likely an incorrect or invalid review comment.
src/fastmcp/server/providers/base.py (1)
1-316: LGTM! Well-designed provider abstraction.The Provider base class provides a clean, context-free interface with sensible defaults:
- Components and TaskComponents containers separate runtime components from task-eligible ones
- Method signatures are simplified (no Context parameter required)
- get_tasks() provides filtering logic that subclasses can override
- lifespan() context manager enables provider-specific setup/teardown
This design enables flexible provider implementations while maintaining consistency across the codebase.
| # Register provider components | ||
| for provider in self._providers: | ||
| try: | ||
| tasks = await provider.get_tasks() | ||
| for tool in tasks.tools: | ||
| named_fn = _create_named_fn_wrapper(tool.fn, tool.key) | ||
| docket.register(named_fn) | ||
| for resource in tasks.resources: | ||
| named_fn = _create_named_fn_wrapper( | ||
| resource.fn, resource.name | ||
| ) | ||
| docket.register(named_fn) | ||
| for template in tasks.templates: | ||
| named_fn = _create_named_fn_wrapper( | ||
| template.fn, template.name | ||
| ) | ||
| docket.register(named_fn) | ||
| for prompt in tasks.prompts: | ||
| named_fn = _create_named_fn_wrapper(prompt.fn, prompt.key) | ||
| docket.register(named_fn) | ||
| except Exception as e: | ||
| provider_name = getattr( | ||
| provider, "server", provider | ||
| ).__class__.__name__ | ||
| logger.warning( | ||
| f"Failed to register tasks from provider {provider_name!r}: {e}" | ||
| ) | ||
| if fastmcp.settings.mounted_components_raise_on_load_error: | ||
| raise |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's find where Resource, ResourceTemplate, Tool, and Prompt are defined
find . -type f -name "*.py" | head -20
git ls-files "*.py" | grep -E "(model|task|provider|resource|tool|prompt)" | head -30Repository: jlowin/fastmcp
Length of output: 2152
🏁 Script executed:
# Search for Resource and ResourceTemplate class definitions
rg -n "class Resource" --type=py -A 10 | head -50
rg -n "class ResourceTemplate" --type=py -A 10 | head -50
rg -n "class Tool" --type=py -A 10 | head -50
rg -n "class Prompt" --type=py -A 10 | head -50Repository: jlowin/fastmcp
Length of output: 10591
🏁 Script executed:
# Check how docket.register is called with resources and templates in the code
rg -n "\.name|\.key" src/fastmcp/server/server.py -B 2 -A 2 | head -60Repository: jlowin/fastmcp
Length of output: 2793
🏁 Script executed:
# Look at the actual code snippet to understand the context better
sed -n '468,496p' src/fastmcp/server/server.pyRepository: jlowin/fastmcp
Length of output: 1618
🏁 Script executed:
# Get the Resource class definition
cat -n src/fastmcp/resources/resource.py | head -80Repository: jlowin/fastmcp
Length of output: 2864
🏁 Script executed:
# Get the ResourceTemplate class full definition
cat -n src/fastmcp/resources/template.py | head -150Repository: jlowin/fastmcp
Length of output: 6123
🏁 Script executed:
# Get the Tool class definition
cat -n src/fastmcp/tools/tool.py | grep -A 30 "^ 123"Repository: jlowin/fastmcp
Length of output: 1306
🏁 Script executed:
# Get the Prompt class definition
rg -n "class Prompt\(" --type=py -A 30 src/fastmcp/prompts/Repository: jlowin/fastmcp
Length of output: 2073
🏁 Script executed:
# Find FastMCPComponent definition
rg -n "class FastMCPComponent" --type=py -A 50Repository: jlowin/fastmcp
Length of output: 3800
🏁 Script executed:
# Also check if Resource class has explicit .key or .name properties
cat -n src/fastmcp/resources/resource.py | sed -n '80,250p'Repository: jlowin/fastmcp
Length of output: 7403
🏁 Script executed:
# Check what exceptions _create_named_fn_wrapper might raise
rg -n "_create_named_fn_wrapper" --type=py -B 5 -A 20 src/fastmcp/server/Repository: jlowin/fastmcp
Length of output: 5170
Inconsistent Docket registration keys and bare exception handling.
Lines 477 and 482 register resources and templates using .name, while lines 473 and 486 use .key for tools and prompts. Since resources and templates are indexed by .key elsewhere in the codebase (lines 966, 1006), this inconsistency could cause lookup failures. Use .key consistently across all component types.
Additionally, line 488 uses a bare except Exception which violates the coding guideline requiring specific exception types. Replace with appropriate exception type(s).
🤖 Prompt for AI Agents
In src/fastmcp/server/server.py around lines 468 to 496, fix two issues:
register resources and templates using .key (not .name) so all component types
consistently use .key for docket registration, and replace the bare "except
Exception" with a narrow exception handler that targets likely failure modes
from provider.get_tasks and registration (for example AttributeError, TypeError,
ValueError, RuntimeError) — catch those explicitly (e.g., except
(AttributeError, TypeError, ValueError, RuntimeError) as e) and keep the
existing logging/raise behavior.
jlowin
added
feature
jlowin
added
enhancement
1 participant
Converts mounted servers to use the Provider abstraction, enabling them to participate fully in the provider system.
Key change:
FastMCP.mount()now creates aMountedProviderthat wraps the mounted server. This means mounted servers:list_*methods