fix(mcp): rename duplicate tool names across MCP servers instead of raising an error#3037
fix(mcp): rename duplicate tool names across MCP servers instead of raising an error#3037anishesg wants to merge 3 commits intoopenai:mainfrom
Conversation
…aising an error When multiple MCP servers exposed tools with the same name, `MCPUtil.get_all_function_tools` in `src/agents/mcp/util.py` raised a `UserError` and refused to proceed. This was overly strict: the conflict is resolvable and users should be able to use tools from both servers simultaneously. Signed-off-by: anish k <ak8686@princeton.edu>
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 95ca2ca713
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| prefixed_name = f"{server.name}_{tool.name}" | ||
| logger.warning( | ||
| f"Duplicate tool name '{tool.name}' found across MCP servers. " | ||
| f"Renaming to '{prefixed_name}' for server '{server.name}'." | ||
| ) | ||
| tool = dataclasses.replace(tool, name=prefixed_name) |
There was a problem hiding this comment.
Renaming collisions to "{server.name}_{tool.name}" can still produce duplicate names (for example, if another server already exposes that prefixed name, or multiple servers share the same server.name), so this no longer guarantees unique dispatch keys. Downstream lookup uses last-wins semantics in build_function_tool_lookup_map (src/agents/_tool_identity.py), which means one tool can silently shadow another and route calls to the wrong server instead of failing fast. Please detect post-rename collisions and either generate a truly unique name or raise a UserError.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
good catch. added a post-rename collision check: after computing the final names for all tools (prefixed or not), we count duplicates again and raise a UserError with the offending names before building the tool list. added a test covering the exact scenario you described (server "A" + tool "B_C" vs server "A_B" + tool "C" → both land on "A_B_C").
There was a problem hiding this comment.
updated the same check to also cover collisions against agent.tools names (caught in the follow-up comment below). Both cases now go through the same all_collisions set before raising.
…Error
When prefixing with {server.name}_{tool.name} still produces duplicate
final names (e.g. server 'A' + tool 'B_C' and server 'A_B' + tool 'C'
both resolve to 'A_B_C'), compute the complete set of final names before
building the tool list and raise a UserError immediately if any remain
ambiguous. A test covering this path is added alongside the existing
rename tests.
Signed-off-by: anish k <ak8686@princeton.edu>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d71fa59770
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| post_rename_duplicates = {n for n, c in final_name_counts.items() if c > 1} | ||
| if post_rename_duplicates: | ||
| raise UserError( | ||
| "Tool name collision could not be resolved automatically by prefixing with the " | ||
| "server name. The following names remain ambiguous after renaming: " |
There was a problem hiding this comment.
The post-rename collision check only counts names from per_server, so it can still produce a function-tool name that collides with an agent-defined tool (for example, a local tool named serverA_run when duplicate MCP run tools are auto-prefixed). AgentBase.get_all_tools() then merges MCP and local tools, and lookup uses last-wins semantics in build_function_tool_lookup_map, so the wrong implementation can be dispatched silently. This is a behavioral regression from the prior fail-fast duplicate handling for multi-server MCP conflicts; include agent.tools names in this check (or generate guaranteed-unique names) before returning.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
fixed — now collecting agent.tools names before the collision check and including them in agent_collisions. If any final MCP tool name (post-prefix) hits an existing agent tool name, it's added to all_collisions and we raise UserError. Added a test that exercises exactly the scenario you described (two MCP servers both expose run → renamed to serverA_run, which clashes with a local serverA_run function tool).
…isting agent.tools to prevent silent shadowing Signed-off-by: anish k <ak8686@princeton.edu>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 31c71388e9
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| agent_tool_names: set[str] = { | ||
| name | ||
| for t in agent.tools | ||
| if isinstance((name := getattr(t, "name", None)), str) and name | ||
| } |
There was a problem hiding this comment.
The new collision guard derives agent_tool_names from raw agent.tools .name values, then intersects those names with MCP final names, but function-tool dispatch is namespace-aware (build_function_tool_lookup_map keys include namespace). That means valid configurations now fail fast: e.g., a namespaced local tool serverA_run (via tool_namespace) does not conflict with a bare MCP serverA_run, yet this check still raises UserError. This is a regression introduced by the commit because it rejects non-ambiguous tool sets before runtime.
Useful? React with 👍 / 👎.
|
Thanks for this suggestion. We're thinking of an opt-in approach like this: #3019 |
2 participants
When multiple MCP servers exposed tools with the same name,
MCPUtil.get_all_function_toolsinsrc/agents/mcp/util.pyraised aUserErrorand refused to proceed. This was overly strict: the conflict is resolvable and users should be able to use tools from both servers simultaneously.The fix changes the collision-handling logic to rename conflicting tools to
{server_name}_{tool_name}rather than aborting. Only tools whose names actually collide across servers get prefixed; tools with unique names are left untouched. Alogger.warningis emitted for every renamed tool so the rename is visible in logs without being fatal.The underlying server call still uses the original (unprefixed) tool name, so the server protocol is unaffected.
dataclasses.replaceis used to create a newToolinstance with the prefixed name, leaving the original object intact.Two new tests in
tests/mcp/test_mcp_util.pycover the rename behavior: one checks that the returned tool names are correctly prefixed and that unique names are preserved, and another verifies that invoking a renamed tool dispatches to the server with the original tool name.Fixes #1167