fix: parse MCP server names with underscores#2746
Conversation
|
Thanks @puneetdixit200 for taking the time to contribute. This repository is currently observing a maintainer-managed contribution gate in dry-run mode, so this pull request is staying open. When enforcement is enabled, pull requests from contributors who are not listed in Please read |
![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 the MCP tool name resolution logic in crates/tui/src/mcp.rs to correctly handle server names containing underscores by matching prefixes against known connections or configured servers, and adds a corresponding unit test. The reviewer suggested optimizing this lookup by using rmatch_indices instead of match_indices to iterate from right to left, which finds the longest matching server name first and allows for an early return.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| let mut resolved = None; | ||
| for (idx, _) in rest.match_indices('_') { | ||
| let server = &rest[..idx]; | ||
| let tool = &rest[idx + 1..]; | ||
| if server.is_empty() || tool.is_empty() { | ||
| continue; | ||
| } | ||
| if self.connections.contains_key(server) || self.config.servers.contains_key(server) { | ||
| resolved = Some((server, tool)); | ||
| } | ||
| } | ||
| if let Some((server, tool)) = resolved { | ||
| return Ok((server, tool)); | ||
| } |
There was a problem hiding this comment.
Since we want to resolve the tool name against the longest known server-name prefix, we can optimize this lookup by iterating backwards using rmatch_indices instead of match_indices.
By searching from right to left, the first match we find that corresponds to a known server is guaranteed to be the longest prefix. This allows us to return early immediately, avoiding redundant map lookups and eliminating the need for the temporary resolved variable.
for (idx, _) in rest.rmatch_indices('_') {
let server = &rest[..idx];
let tool = &rest[idx + 1..];
if server.is_empty() || tool.is_empty() {
continue;
}
if self.connections.contains_key(server) || self.config.servers.contains_key(server) {
return Ok((server, tool));
}
}![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| #[tokio::test] | ||
| async fn mcp_pool_call_tool_preserves_server_names_with_underscores() { |
There was a problem hiding this comment.
resolved on every match, so the last (longest-prefix) match wins. This is the stated intent, but there is no test that verifies the tie-breaking when two registered servers share an underscore prefix (e.g. both my and my_db are in the pool and the call is mcp_my_db_execute_sql). Without that test, a future refactor that switches the loop to pick the first match would silently change routing for users with overlapping server names. Consider adding a dedicated test for this boundary.
| #[tokio::test] | |
| async fn mcp_pool_call_tool_preserves_server_names_with_underscores() { | |
| /// Verify longest-prefix wins when two server names overlap. | |
| #[tokio::test] | |
| async fn mcp_pool_parse_prefixed_name_longest_prefix_wins() { | |
| // Server "my" is registered; "my_db" is also registered. | |
| // mcp_my_db_execute_sql must route to "my_db", not "my". | |
| let pool = McpPool::new(McpConfig { | |
| timeouts: McpTimeouts::default(), | |
| servers: { | |
| let mut m = HashMap::new(); | |
| m.insert("my".to_string(), Default::default()); | |
| m.insert("my_db".to_string(), Default::default()); | |
| m | |
| }, | |
| }); | |
| let (server, tool) = pool.parse_prefixed_name("mcp_my_db_execute_sql").unwrap(); | |
| assert_eq!(server, "my_db"); | |
| assert_eq!(tool, "execute_sql"); | |
| } | |
| #[tokio::test] | |
| async fn mcp_pool_call_tool_preserves_server_names_with_underscores() { |
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| let mut resolved = None; | ||
| for (idx, _) in rest.match_indices('_') { | ||
| let server = &rest[..idx]; | ||
| let tool = &rest[idx + 1..]; | ||
| if server.is_empty() || tool.is_empty() { | ||
| continue; | ||
| } | ||
| if self.connections.contains_key(server) || self.config.servers.contains_key(server) { | ||
| resolved = Some((server, tool)); | ||
| } | ||
| } | ||
| if let Some((server, tool)) = resolved { | ||
| return Ok((server, tool)); | ||
| } |
There was a problem hiding this comment.
Adding a new server whose name is a prefix-extension of an existing one (e.g. registering my_db after my already exists) silently re-routes any mcp_my_db_* tool call that was previously landing on server my. The tool on my named db_<anything> becomes unreachable through the prefixed name the moment my_db is added to the pool. The longest-match semantics are correct per the PR description, but this behavioral change is invisible at the call site — callers get a successful response from a different server, or an "unknown tool" error if my_db doesn't expose that tool. A doc comment on parse_prefixed_name stating the longest-prefix rule and its consequence for overlapping server names would help future maintainers.
|
Thanks @puneetdixit200 — this correctly identified and fixed the MCP server-name underscore parsing bug from #2744. The v0.9.0 integration branch ( Verification recorded for the landed fix: Closing this PR as superseded by the credited integration-branch harvest, not because the report or fix was ignored. |
parse_prefixed_name now matches the qualified mcp_<server>_<tool> name against the set of registered server names (connections + configured servers) and prefers the longest matching server name, instead of naively splitting on the first underscore. Tools on servers whose names contain underscores (e.g. "my_db") are now reachable, and an overlapping pair like "my" and "my_db" routes to the correct server. Falls back to the legacy first-underscore split when no registered server matches, preserving backward compatibility. Harvested from PR Hmbown#2747 by @cyq1017; supersedes the equivalent fix in PR Hmbown#2746 by @puneetdixit200. Both contributors diagnosed and fixed issue Hmbown#2744; Hmbown#2747 landed for its longest-match tie-break test coverage. Fixes Hmbown#2744. Co-authored-by: cyq1017 <61975706+cyq1017@users.noreply.github.com> Co-authored-by: puneetdixit200 <236133619+puneetdixit200@users.noreply.github.com>
…mbown#2757 harvests and Hmbown#2742/Hmbown#2751/Hmbown#2755 dispositions Log the new community-PR harvests in CHANGELOG.md and crates/tui/CHANGELOG.md (MCP underscore server names, Xiaomi MiMo pricing, hydrated deferred-tool render, Token Plan region docs) with contributor credit, and update docs/V0_9_0_EXECUTION_MAP.md with evidence-backed dispositions for the newly-reviewed PRs, including the deferred Hmbown#2742 and forwarded Hmbown#2751/Hmbown#2755.
2 participants
Closes #2744.
This resolves prefixed MCP tool names against the longest known server-name prefix before falling back to the legacy single-underscore split. That keeps existing
mcp_server_toolnames working while allowing servers likemy_dbto routemcp_my_db_execute_sqlto servermy_dband toolexecute_sql.AI-assisted with human review.
Verification
CARGO_HOME=/tmp/codewhale-2744-cargo-home CARGO_TARGET_DIR=/tmp/codewhale-2744-target cargo test -p codewhale-tui mcp_pool_call_tool_preserves_server_names_with_underscores -- --nocapturefailed withFailed to find MCP server: myCARGO_HOME=/tmp/codewhale-2744-cargo-home CARGO_TARGET_DIR=/tmp/codewhale-2744-target cargo test -p codewhale-tui mcp_pool_call_tool_preserves_server_names_with_underscores -- --nocaptureCARGO_HOME=/tmp/codewhale-2744-cargo-home CARGO_TARGET_DIR=/tmp/codewhale-2744-target cargo test -p codewhale-tui mcp_pool_call_tool_preserves -- --nocaptureCARGO_HOME=/tmp/codewhale-2744-cargo-home CARGO_TARGET_DIR=/tmp/codewhale-2744-target cargo fmt --checkgit diff --checkGreptile Summary
This PR fixes MCP tool routing for server names that contain underscores (e.g.
my_db) by replacing the naive first-underscore split with a loop that matches the longest known server-name prefix before falling back to the legacy behaviour.parse_prefixed_namenow iterates every_position in the suffix, checks each candidate prefix againstself.connectionsandself.config.servers, and keeps the last (longest) match — makingmcp_my_db_execute_sqlroute to servermy_dband toolexecute_sqlrather than servermy.mcp_pool_call_tool_preserves_server_names_with_underscores) exercises the new path end-to-end with a scripted transport.Confidence Score: 4/5
Safe to merge; the core logic correctly implements longest-prefix matching and the existing test suite exercises the primary scenario.
The fix handles the target case correctly and preserves backward compatibility through the legacy fallback. The main gap is that no test covers the overlapping-server-name scenario (both
myandmy_dbregistered simultaneously), so the longest-prefix tie-breaking is untested and could be broken by a future refactor without a test failure. The silent re-routing risk when server names share a prefix is also undocumented.crates/tui/src/mcp.rs — specifically the new loop in parse_prefixed_name and its interaction with overlapping server-name prefixes.
Important Files Changed
Flowchart
%%{init: {'theme': 'neutral'}}%% flowchart TD A["parse_prefixed_name(prefixed_name)"] --> B{"starts_with('mcp_')?"} B -- No --> ERR1["bail! Invalid MCP tool name"] B -- Yes --> C["rest = prefixed_name[4..]"] C --> D["resolved = None"] D --> E["for each '_' position idx in rest"] E --> F["server = rest[..idx], tool = rest[idx+1..]"] F --> G{"server or tool empty?"} G -- Yes --> E G -- No --> H{"server in connections or config?"} H -- No --> E H -- Yes --> I["resolved = Some((server, tool)) // overwrites: longest prefix wins"] I --> E E -- done --> J{"resolved is Some?"} J -- Yes --> K["return Ok((server, tool)) // longest-prefix match"] J -- No --> L["rest.split_once('_')"] L -- None --> ERR2["bail! Invalid MCP tool name format"] L -- Some --> M["return Ok((server, tool)) // legacy first-underscore fallback"]Reviews (1): Last reviewed commit: "fix: parse MCP server names with undersc..." | Re-trigger Greptile