Add tool_names parameter to mount() for name overrides#2619
Conversation
WalkthroughAdds an optional tool_names mapping to MountedServer and the mount API, propagating that mapping through mounting, registration, listing, and invocation paths. Observable tool/prompt/template keys are computed by checking tool_names overrides first, then applying a mount prefix, then falling back to the original key. Registration and nested mounting flows pass tool_names into _docket_lifespan and _register_mounted_server_functions. Call routing performs reverse-lookup against tool_names before applying standard prefix stripping. Possibly related PRs
Pre-merge checks and finishing touches✅ Passed checks (5 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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/fastmcp/server/server.py (1)
509-572: Docket registration for mounted tools ignorestool_namesoverrides, breaking task execution
_register_mounted_server_functionsderivesfn_namesolely fromprefixandtool.key(line 524), while bothget_tools(line 940-941) and_list_tools(line 1226-1227) check formounted.tool_namesoverrides. When a mounted tool has atool_namesoverride, the registered Docket function name will use the original prefixed key, buthandle_tool_as_taskretrieves the tool viaget_tool()(which applies the override), then callsdocket.add(tool.key)with the overridden key. This causes Docket to look for a function named by the override, but the registered function has the original prefixed name—task execution will fail.Pass the full
MountedServer(includingtool_names) into_register_mounted_server_functionsand apply the same override logic used inget_toolsand_list_toolswhen derivingfn_namefor tools.
🧹 Nitpick comments (4)
src/fastmcp/server/server.py (4)
1217-1237: Consistent override behavior in_list_tools, consider de‑duplicating key logicThe same
tool_names→prefix→ original key precedence as inget_toolsis correctly applied here, and updatingtool.keyonly when it changes keeps things tidy. To avoid future drift, you might consider extracting a small helper like_mounted_tool_key(mounted, original_key)and using it in bothget_toolsand_list_tools.
1646-1666: Reverse lookup for overridden tool names in_call_toolis sound; possible micro‑optimizationThe reverse mapping from
tool_namesoverride back to the original key, with prefix fallback in theelseblock, is logically correct and keeps compatibility with both overrides and the existingprefix_scheme.If
tool_namesmappings ever become large or_call_toolis extremely hot, you could precompute a reverse dict onMountedServer(e.g.overrides_by_value) instead of scanningtool_names.items()on every call, but that’s an optional micro‑optimization.
2676-2751:mount(..., tool_names=...)API extension is coherent; consider doc tweakThe new
tool_names: dict[str, str] | Noneparameter and its propagation intoMountedServergive callers precise control over mounted tool names, and the docstring correctly describes the mapping as original‑name → custom‑name.Since the earlier bullets describe prefix behavior unconditionally, you might consider clarifying that those examples describe the default behavior when
tool_namesis not used, and that overrides take precedence over prefixing on a per‑tool basis. That would make the interaction betweenprefixandtool_namesexplicit for readers of the docstring.
3005-3010:MountedServer.tool_namesfield is well‑typed; potential place for a reverse mapAdding
tool_names: dict[str, str] | None = Nonehere is consistent with its use sites and satisfies the full‑annotation guideline. If you later decide to optimize_call_toollookups, this dataclass would also be a natural place to attach a precomputed reverse mapping (override → original) derived fromtool_names.
📜 Review details
Configuration used: CodeRabbit 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 (1)
src/fastmcp/server/server.py(8 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*.py: Write Python code with Python ≥3.10 and include full type annotations
Use specific exception types in error handling - never use bareexcept
Prioritize readable, understandable code - clarity over cleverness; avoid obfuscated or confusing patterns even if they're shorter
Files:
src/fastmcp/server/server.py
🧬 Code graph analysis (1)
src/fastmcp/server/server.py (3)
src/fastmcp/resources/resource.py (1)
key(149-156)src/fastmcp/resources/template.py (1)
key(221-228)src/fastmcp/utilities/components.py (1)
key(66-73)
⏰ 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 (2)
src/fastmcp/server/server.py (2)
935-947: Override vs prefix resolution inget_toolslooks correctThe precedence
tool_names→prefix→ original key is clear and matches the intended semantics, andmodel_copy(key=new_key)keeps the tool’s key consistent with the dict key. This hunk looks good.
1495-1512: Prompt key prefixing refactor is behavior‑preservingThe explicit
if mounted.prefix: key = ... else: key = prompt.keyplusmodel_copyonly when the key changes is equivalent to the previous behavior and keeps the intent clearer without affecting behavior.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/fastmcp/server/server.py (2)
2686-2739: Clarify the tool_names mapping direction in documentation.The documentation states "Keys are the original tool names from the mounted server" but doesn't explicitly clarify that values are the new/override names. Consider adding an example for clarity.
Apply this diff to improve the documentation:
- tool_names: Optional mapping of original tool names to custom names. Use this - to override prefixed names. Keys are the original tool names from the - mounted server. + tool_names: Optional mapping of original tool names to custom names. Use this + to override prefixed names. Keys are the original tool names from the + mounted server, and values are the custom names to use instead of the + prefix-based names. Example: {"get_user": "fetch_user"} renames + "get_user" to "fetch_user" (without applying the prefix).
2686-2761: Consider: Add validation for tool_names mappings.The current implementation doesn't validate that:
- Keys in
tool_namescorrespond to actual tools in the mounted server- Values in
tool_namesdon't collide with existing tool names- Override names are valid identifiers
While the PR description mentions length validation was intentionally omitted (relying on MCP SDK warnings), consider if basic validation would improve the developer experience:
def mount( self, server: FastMCP[LifespanResultT], prefix: str | None = None, as_proxy: bool | None = None, tool_names: dict[str, str] | None = None, ) -> None: """...""" from fastmcp.server.proxy import FastMCPProxy # Validate tool_names if provided if tool_names: if not all(isinstance(k, str) and isinstance(v, str) for k, v in tool_names.items()): raise ValueError("tool_names must be a dict[str, str]") if not all(v.strip() for v in tool_names.values()): raise ValueError("tool_names override values cannot be empty strings") # ... rest of implementationThis would catch obvious mistakes at mount time rather than at runtime when a tool is called.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/fastmcp/server/server.py(12 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*.py: Write Python code with Python ≥3.10 and include full type annotations
Use specific exception types in error handling - never use bareexcept
Prioritize readable, understandable code - clarity over cleverness; avoid obfuscated or confusing patterns even if they're shorter
Files:
src/fastmcp/server/server.py
🧬 Code graph analysis (1)
src/fastmcp/server/server.py (4)
src/fastmcp/tools/tool.py (1)
FunctionTool(275-419)src/fastmcp/resources/resource.py (1)
key(149-156)src/fastmcp/resources/template.py (1)
key(221-228)src/fastmcp/utilities/components.py (1)
key(66-73)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Run tests: Python 3.10 on windows-latest
- GitHub Check: Run tests: Python 3.13 on ubuntu-latest
- GitHub Check: Run tests: Python 3.10 on ubuntu-latest
- GitHub Check: Run tests with lowest-direct dependencies
🔇 Additional comments (6)
src/fastmcp/server/server.py (6)
3019-3019: LGTM!The
tool_namesfield is properly typed and initialized with a sensible default.
466-469: LGTM!Correctly propagates
tool_namesto the registration function for task execution support.
509-582: Verify: Should tool_names apply only to tools, not prompts/resources/templates?The
tool_namesoverride is applied to tools (lines 531-537) but not to prompts (line 547), resources (line 559), or templates (line 569), which only use prefix-based naming. This seems intentional based on the parameter name, but it creates an asymmetry in the API.Consider:
- Is this the intended behavior?
- Should the documentation in
mount()clarify thattool_namesonly affects tools?- Would users expect similar override capability for prompts?
941-965: LGTM!The tool naming logic correctly applies overrides before prefixes and properly creates copies to avoid mutating the original tools.
1209-1258: LGTM!The implementation correctly applies tool_names overrides and includes a nice optimization to only create a copy when the key actually changes.
1647-1702: Verify: Potential name collisions between overrides and prefixed names.The reverse lookup logic correctly handles tool_names overrides (lines 1660-1671), but consider this scenario:
- Server A mounted with
prefix="api", has tool"get_user"→ exposed as"api_get_user"- Server B mounted with
tool_names={"foo": "api_get_user"}→ tool"foo"exposed as"api_get_user"Both servers now expose a tool named
"api_get_user". The reverse iteration order (line 1657) means "last mounted wins," which is consistent with the existing behavior for non-prefixed mounts. However, this could be confusing for users.Consider:
- Should the documentation warn about potential collisions?
- Should
mount()validate that override names don't collide with existing tool names?For validation, you could add a check after line 2761:
# Optional: Validate no collisions with existing tools if tool_names: existing_keys = set(await self.get_tools()) collisions = set(tool_names.values()) & existing_keys if collisions: logger.warning(f"Tool name overrides {collisions} collide with existing tool names")Note: This would require making
mount()async or deferring validation.
When mounting servers with prefixes, auto-generated tool names can become unwieldy (e.g.,
api_v2_users_get_user). This adds atool_namesparameter tomount()that lets users override specific tool names:The override is applied instead of (not in addition to) the prefix, giving full control over the final name. The MCP SDK already warns for names > 128 chars (SEP-986), so no custom validation was added.
Closes #2596