fix(agent): handle duplicate MCP tool names#14217
Merged
Merged
Conversation
dosubot
Bot
added
the
size:M
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds an MCPToolBinding wrapper and threads an optional timeout through tool session call signatures and dispatch; generates indexed OpenAI function names for MCP tools and stores bindings keyed by those names while updating the metadata generator to accept an explicit function_name. ChangesMCP tool binding + metadata + dispatch
Sequence DiagramsequenceDiagram
participant Agent as Agent
participant Registry as ToolRegistry
participant Schema as SchemaGen
participant Dispatcher as Dispatcher
participant Session as ToolSession
Agent->>Agent: compute indexed_name = "{tnm}_{tool_idx}"
Agent->>Registry: create MCPToolBinding(session, original_name)
Agent->>Schema: mcp_tool_metadata_to_openai_tool(..., function_name=indexed_name)
Schema-->>Agent: OpenAI tool schema (function.name = indexed_name)
Agent->>Registry: store MCPToolBinding keyed by indexed_name
Registry->>Dispatcher: tool_call(indexed_name, arguments, timeout)
Dispatcher->>Dispatcher: detect MCPToolBinding
Dispatcher->>Session: session.tool_call(original_name, arguments, timeout)
Session-->>Dispatcher: result
Dispatcher-->>Registry: result
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
agent/component/agent_with_tools.py (1)
100-109: Log the indexed MCP registrations.This new registration path is where duplicate names get disambiguated, so a debug/info log with the MCP server id, original tool name, and indexed name would make collisions and dispatch issues much easier to trace.
As per coding guidelines,
**/*.py: Add logging for new flows.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@agent/component/agent_with_tools.py` around lines 100 - 109, Add a debug/info log inside the loop that registers MCP tools to record the MCP server id, the original tool name (tnm) and the generated indexed name (indexed_name) to aid tracing of disambiguation; locate the registration block where tool_idx is computed and MCPServerService.get_by_id(...) and MCPToolCallSession(...) are used, and log before appending to self.tool_meta / assigning self.tools (use the module or class logger consistent with existing codebase and include mcp["mcp_id"], tnm, and indexed_name in the message).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@agent/tools/base.py`:
- Around line 55-66: The new timeout parameter added to tool_call is being
dropped because tool_call_async lacks a timeout argument and the calls to
thread_pool_exec inside tool_call_async use a hardcoded 60; update
tool_call_async signature to accept timeout: float | int = 10, have tool_call
pass the timeout through to tool_call_async, and replace the hardcoded 60 in the
thread_pool_exec calls (for MCPToolBinding.session.tool_call and
MCPToolCallSession.tool_call) with the passed-in timeout so callers can control
MCP execution time; reference methods: tool_call, tool_call_async and classes:
MCPToolBinding, MCPToolCallSession and the thread_pool_exec invocations.
---
Nitpick comments:
In `@agent/component/agent_with_tools.py`:
- Around line 100-109: Add a debug/info log inside the loop that registers MCP
tools to record the MCP server id, the original tool name (tnm) and the
generated indexed name (indexed_name) to aid tracing of disambiguation; locate
the registration block where tool_idx is computed and
MCPServerService.get_by_id(...) and MCPToolCallSession(...) are used, and log
before appending to self.tool_meta / assigning self.tools (use the module or
class logger consistent with existing codebase and include mcp["mcp_id"], tnm,
and indexed_name in the message).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b10234c6-1dc5-4365-8dc7-8c3d36a18b20
📒 Files selected for processing (3)
agent/component/agent_with_tools.pyagent/tools/base.pycommon/mcp_tool_call_conn.py
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
agent/tools/base.py (1)
63-66: Add branch-level logging for the new MCP binding flow.For the new
MCPToolBindingdispatch path, loggingoriginal_nameandtimeoutwould make debugging collisions/timeouts much easier.Suggested patch
tool_obj = self.tools_map[name] if isinstance(tool_obj, MCPToolBinding): + logging.info( + "[ToolCall] dispatch type=MCPToolBinding llm_name=%s mcp_name=%s timeout=%s", + name, tool_obj.original_name, timeout + ) resp = await thread_pool_exec(tool_obj.session.tool_call, tool_obj.original_name, arguments, timeout) elif isinstance(tool_obj, MCPToolCallSession): + logging.info( + "[ToolCall] dispatch type=MCPToolCallSession name=%s timeout=%s", + name, timeout + ) resp = await thread_pool_exec(tool_obj.tool_call, name, arguments, timeout)As per coding guidelines, "
**/*.py: Add logging for new flows".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@agent/tools/base.py` around lines 63 - 66, Add branch-level logging in the MCPToolBinding dispatch path: inside the branch that checks isinstance(tool_obj, MCPToolBinding) (where thread_pool_exec is called with tool_obj.session.tool_call, tool_obj.original_name, arguments, timeout), log tool_obj.original_name and timeout (and any concise context like the session or caller) before invoking thread_pool_exec so collisions/timeouts are easier to debug; keep the MCPToolCallSession branch unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@agent/tools/base.py`:
- Around line 58-66: The default timeout in tool_call_async (currently set to
10) causes MCP-backed tools (instances of MCPToolBinding and MCPToolCallSession)
to be forcibly limited by thread_pool_exec -> Future.result(timeout=...), which
is a regression for callers that omit timeout; update the callers in
rag/llm/chat_model.py (the four places that call tool_call_async without
timeout) to pass an explicit, appropriate timeout value based on each tool's
expected runtime, or change the caller sites to forward a configured timeout
parameter (e.g., per-tool or per-call timeout) into tool_call_async(name,
arguments, timeout) so MCPToolBinding.session.tool_call and
MCPToolCallSession.tool_call are not prematurely canceled; ensure
thread_pool_exec continues to receive the propagated timeout.
---
Nitpick comments:
In `@agent/tools/base.py`:
- Around line 63-66: Add branch-level logging in the MCPToolBinding dispatch
path: inside the branch that checks isinstance(tool_obj, MCPToolBinding) (where
thread_pool_exec is called with tool_obj.session.tool_call,
tool_obj.original_name, arguments, timeout), log tool_obj.original_name and
timeout (and any concise context like the session or caller) before invoking
thread_pool_exec so collisions/timeouts are easier to debug; keep the
MCPToolCallSession branch unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 049c812e-a840-4170-a85d-b4cfdf6aef90
📒 Files selected for processing (1)
agent/tools/base.py
Contributor
Author
|
Hi maintainers, just wanted to gently follow up on this PR. This PR fixes an MCP tool-name collision issue: when multiple MCP servers have tools with the same name, the current logic may overwrite previously registered tools. Could @JinHai-CN please take a look when convenient? Thanks! |
Collaborator
|
Appreciations! Fix CI failure please. |
- rename the async timeout parameter to request_timeout
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #14217 +/- ##
==========================================
- Coverage 98.11% 94.16% -3.95%
==========================================
Files 10 10
Lines 690 703 +13
Branches 108 112 +4
==========================================
- Hits 677 662 -15
- Misses 4 25 +21
- Partials 9 16 +7 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
tmimmanuel
pushed a commit
to tmimmanuel/ragflow
that referenced
this pull request
May 14, 2026
### What problem does this PR solve? When multiple MCP servers expose tools with the same name, the agent currently registers those tools using their original MCP names. This can lead to two issues: - later MCP tools may overwrite earlier ones in the agent tool map - duplicate function names may be exposed to the LLM This PR fixes duplicate MCP tool-name handling by applying the same indexed naming strategy already used for native agent tools. Native tools are exposed with generated names such as `<tool_name>_<index>` to avoid collisions, and MCP tools now follow the same convention for consistency. Specifically, this PR: - assigns unique indexed function names to MCP tools exposed to the LLM - preserves each MCP tool's original server-side name in an `MCPToolBinding` - dispatches MCP calls using the original MCP tool name while keeping the indexed name in the agent tool map - allows MCP metadata conversion to override only the OpenAI function name without modifying the original MCP tool metadata ### Type of change - [x] Bug Fix (non-breaking change which fixes an issue) ### Validation The validation was performed using two MCP servers. Both servers exposed a tool with the same name: `mcp0`. Both tools take no input parameters. **MCP Server One:** <img width="1780" height="625" alt="ONE" src="https://github.com/user-attachments/assets/801a2654-fc10-4b71-b31c-81841fd40c55" /> **MCP Server Two:** <img width="1777" height="624" alt="Second" src="https://github.com/user-attachments/assets/c095151d-7bdf-47c8-9bfe-6aaf4a01b944" /> **Before the fix:** When invoking `mcp0`, only the `mcp0` tool from the MCP server injected later could be called successfully. As shown below, both `mcp0` tools were present, but only the later-registered one was actually invokable. <img width="694" height="935" alt="Three" src="https://github.com/user-attachments/assets/3b9d7ab2-1765-492c-b8e0-bf05a69933ca" /> **After the fix:** Both `mcp0` tools can now be invoked correctly. <img width="737" height="1095" alt="F" src="https://github.com/user-attachments/assets/6e896627-2b7f-41bb-becc-daa0c73ff58f" /> <img width="730" height="1090" alt="six" src="https://github.com/user-attachments/assets/aba75593-26ae-4e3b-951d-b45ff177fd32" />
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What problem does this PR solve?
When multiple MCP servers expose tools with the same name, the agent currently registers those tools using their original MCP names. This can lead to two issues:
This PR fixes duplicate MCP tool-name handling by applying the same indexed naming strategy already used for native agent tools. Native tools are exposed with generated names such as
<tool_name>_<index>to avoid collisions, and MCP tools now follow the same convention for consistency.Specifically, this PR:
MCPToolBindingType of change
Validation
The validation was performed using two MCP servers. Both servers exposed a tool with the same name:
mcp0. Both tools take no input parameters.MCP Server One:
MCP Server Two:
Before the fix:
When invoking
mcp0, only themcp0tool from the MCP server injected later could be called successfully. As shown below, bothmcp0tools were present, but only the later-registered one was actually invokable.After the fix:
Both
mcp0tools can now be invoked correctly.