Skip to content

fix: parse MCP server names with underscores #2746

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
puneetdixit200 wants to merge 1 commit into from
Closed

fix: parse MCP server names with underscores #2746

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
58 changes: 58 additions & 0 deletions crates/tui/src/mcp.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2245,6 +2245,21 @@ impl McpPool {
anyhow::bail!("Invalid MCP tool name: {prefixed_name}");
}
let rest = &prefixed_name[4..];
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));
}
Comment on lines +2248 to +2261

@gemini-code-assist gemini-code-assist Bot Jun 4, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

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));
            }
        }

Comment on lines +2248 to +2261

@greptile-apps greptile-apps Bot Jun 4, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Silent re-routing when server names share a prefix

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.

Fix in Codex Fix in Claude Code Fix in Cursor


let Some((server, tool)) = rest.split_once('_') else {
anyhow::bail!("Invalid MCP tool name format: {prefixed_name}");
};
Expand Down Expand Up @@ -3638,6 +3653,49 @@ mod tests {
);
}

#[tokio::test]
async fn mcp_pool_call_tool_preserves_server_names_with_underscores() {
Comment on lines +3656 to +3657

@greptile-apps greptile-apps Bot Jun 4, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 The loop overwrites 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.

Suggested change
#[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!

Fix in Codex Fix in Claude Code Fix in Cursor

let sent = Arc::new(Mutex::new(Vec::new()));
let transport = ScriptedValueTransport {
sent: Arc::clone(&sent),
responses: VecDeque::from([json_frame(serde_json::json!({
"jsonrpc": "2.0",
"id": 1,
"result": {"ok": true}
}))]),
};
let mut conn = test_connection(Box::new(transport));
conn.name = "my_db".to_string();
conn.tools = vec![McpTool {
name: "execute_sql".to_string(),
description: None,
input_schema: serde_json::json!({}),
}];

let mut pool = McpPool::new(McpConfig {
timeouts: McpTimeouts::default(),
servers: HashMap::new(),
});
pool.connections.insert("my_db".to_string(), conn);

let result = pool
.call_tool(
"mcp_my_db_execute_sql",
serde_json::json!({"query": "select 1"}),
)
.await
.unwrap();

assert_eq!(result, serde_json::json!({"ok": true}));
let sent = sent.lock().unwrap();
assert_eq!(sent[0]["method"], "tools/call");
assert_eq!(sent[0]["params"]["name"], "execute_sql");
assert_eq!(
sent[0]["params"]["arguments"],
serde_json::json!({"query": "select 1"})
);
}

#[tokio::test]
async fn json_rpc_session_error_is_marked_stale() {
let sent = Arc::new(Mutex::new(Vec::new()));
Expand Down
Loading