fix(mcp): use longest-prefix matching in parseMcpToolName for server names with underscores#28033
fix(mcp): use longest-prefix matching in parseMcpToolName for server names with underscores#28033bisma-nawaz wants to merge 1 commit into
Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses an issue where MCP tool names containing underscores were incorrectly parsed due to a naive regex split. By introducing an optional registry of known server names, the system can now perform longest-prefix matching to unambiguously identify the server component. This change ensures that wildcard routing and policy matching function correctly for servers with complex names while preserving existing behavior for legacy call sites. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize the Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counterproductive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
|
📊 PR Size: size/M
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates parseMcpToolName to support longest-prefix matching using a list of known server names, which allows correctly parsing MCP tool names when the server name contains underscores. It also updates LocalAgentExecutor to pass the known server names from the tool registry and adds comprehensive unit tests. The reviewer identified a performance issue where parentToolRegistry.getAllTools() is called repeatedly inside a loop, creating an O(N^2) bottleneck during initialization. Caching the known server names on the registry instance is recommended to resolve this.
| const knownServerNames = Array.from( | ||
| new Set( | ||
| parentToolRegistry | ||
| .getAllTools() | ||
| .filter( | ||
| (t): t is DiscoveredMCPTool => t instanceof DiscoveredMCPTool, | ||
| ) | ||
| .map((t) => t.serverName), | ||
| ), | ||
| ); | ||
| const parsed = parseMcpToolName(toolName, knownServerNames); |
There was a problem hiding this comment.
Calling parentToolRegistry.getAllTools() is an expensive operation because it performs configuration resolution, metadata construction, alias expansion, and sorting for all registered tools. Since registerToolByName is called inside a loop over all available tools, computing knownServerNames on every iteration introduces an
We should cache the computed knownServerNames on the parentToolRegistry instance to ensure it is only computed once.
const registry = parentToolRegistry as any;
registry._knownServerNamesCache ??= Array.from(
new Set(
parentToolRegistry
.getAllTools()
.filter(
(t): t is DiscoveredMCPTool => t instanceof DiscoveredMCPTool,
)
.map((t) => t.serverName),
),
);
const parsed = parseMcpToolName(toolName, registry._knownServerNamesCache);
🛑 Action Required: Evaluation ApprovalSteering changes have been detected in this PR. To prevent regressions, a maintainer must approve the evaluation run before this PR can be merged. Maintainers:
Once approved, the evaluation results will be posted here automatically. |
gemini-cli
Bot
added
the
area/agent
1 participant
What
Adds an optional
knownServerNamesparameter toparseMcpToolNamethat enables longest-prefix matching, fixing incorrect tool routing when MCP server names contain underscores.Why
Fixes #27981.
The current regex
withoutPrefix.match(/^([^_]+)_(.+)$/)stops at the first underscore, so a server namedmy_serveris misidentified asmy. For example:mcp_my_server_my_tool{ serverName: 'my', toolName: 'server_my_tool' }{ serverName: 'my_server', toolName: 'my_tool' }Underscores are common in server names (
github_copilot,google_workspace,my_custom_server), making this a broad correctness issue affecting wildcard routing and policy matching.How
parseMcpToolNamenow accepts an optionalknownServerNames?: string[]second argument. When provided, the function sorts the known names longest-first and uses prefix matching to unambiguously identify the server component. The original first-underscore regex is used as a fallback so all existing call sites without a registry remain backward compatible.local-executor.ts— the call site that routesmcp_<server>_*wildcards — now extracts the registered server names fromparentToolRegistryand passes them toparseMcpToolName, fixing the wildcard case for underscore servers.Testing
8 new tests added to
mcp-tool.test.ts:knownServerNamesmcp_my_server_*wildcards when server name has underscoresmyvsmy_server)All 70 tests in
mcp-tool.test.tspass.