From 8002594ee3ad32bc7dd9c2465949212bef1c017d Mon Sep 17 00:00:00 2001 From: pakrym-oai Date: Fri, 27 Mar 2026 13:58:29 -0700 Subject: [PATCH] Normalize /mcp tool grouping for hyphenated server names (#15946) Fix display for servers with special characters. --- codex-rs/core/src/mcp/mod.rs | 26 +++ codex-rs/core/src/mcp/mod_tests.rs | 8 + codex-rs/core/src/mcp_connection_manager.rs | 21 +- codex-rs/tui/src/history_cell.rs | 170 +++++++++++---- ...sts_tools_for_hyphenated_server_names.snap | 16 ++ codex-rs/tui_app_server/src/history_cell.rs | 195 ++++++++++++------ ...sts_tools_for_hyphenated_server_names.snap | 16 ++ 7 files changed, 333 insertions(+), 119 deletions(-) create mode 100644 codex-rs/tui/src/snapshots/codex_tui__history_cell__tests__mcp_tools_output_lists_tools_for_hyphenated_server_names.snap create mode 100644 codex-rs/tui_app_server/src/snapshots/codex_tui_app_server__history_cell__tests__mcp_tools_output_lists_tools_for_hyphenated_server_names.snap diff --git a/codex-rs/core/src/mcp/mod.rs b/codex-rs/core/src/mcp/mod.rs index a9d2388f70..afeb5647ea 100644 --- a/codex-rs/core/src/mcp/mod.rs +++ b/codex-rs/core/src/mcp/mod.rs @@ -33,6 +33,32 @@ const MCP_TOOL_NAME_DELIMITER: &str = "__"; pub(crate) const CODEX_APPS_MCP_SERVER_NAME: &str = "codex_apps"; const CODEX_CONNECTORS_TOKEN_ENV_VAR: &str = "CODEX_CONNECTORS_TOKEN"; +/// The Responses API requires tool names to match `^[a-zA-Z0-9_-]+$`. +/// MCP server/tool names are user-controlled, so sanitize the fully-qualified +/// name we expose to the model by replacing any disallowed character with `_`. +pub(crate) fn sanitize_responses_api_tool_name(name: &str) -> String { + let mut sanitized = String::with_capacity(name.len()); + for c in name.chars() { + if c.is_ascii_alphanumeric() || c == '_' { + sanitized.push(c); + } else { + sanitized.push('_'); + } + } + + if sanitized.is_empty() { + "_".to_string() + } else { + sanitized + } +} + +pub fn qualified_mcp_tool_name_prefix(server_name: &str) -> String { + sanitize_responses_api_tool_name(&format!( + "{MCP_TOOL_NAME_PREFIX}{MCP_TOOL_NAME_DELIMITER}{server_name}{MCP_TOOL_NAME_DELIMITER}" + )) +} + #[derive(Debug, Clone, Default, PartialEq, Eq)] pub struct ToolPluginProvenance { plugin_display_names_by_connector_id: HashMap>, diff --git a/codex-rs/core/src/mcp/mod_tests.rs b/codex-rs/core/src/mcp/mod_tests.rs index 855e71e1f2..d1bbf68f60 100644 --- a/codex-rs/core/src/mcp/mod_tests.rs +++ b/codex-rs/core/src/mcp/mod_tests.rs @@ -52,6 +52,14 @@ fn split_qualified_tool_name_returns_server_and_tool() { ); } +#[test] +fn qualified_mcp_tool_name_prefix_sanitizes_server_names_without_lowercasing() { + assert_eq!( + qualified_mcp_tool_name_prefix("Some-Server"), + "mcp__Some_Server__".to_string() + ); +} + #[test] fn split_qualified_tool_name_rejects_invalid_names() { assert_eq!(split_qualified_tool_name("other__alpha__do_thing"), None); diff --git a/codex-rs/core/src/mcp_connection_manager.rs b/codex-rs/core/src/mcp_connection_manager.rs index eac8485048..b03f1a7a12 100644 --- a/codex-rs/core/src/mcp_connection_manager.rs +++ b/codex-rs/core/src/mcp_connection_manager.rs @@ -22,6 +22,7 @@ use std::time::Instant; use crate::mcp::CODEX_APPS_MCP_SERVER_NAME; use crate::mcp::ToolPluginProvenance; use crate::mcp::auth::McpAuthStatusEntry; +use crate::mcp::sanitize_responses_api_tool_name; use anyhow::Context; use anyhow::Result; use anyhow::anyhow; @@ -104,26 +105,6 @@ const MCP_TOOLS_LIST_DURATION_METRIC: &str = "codex.mcp.tools.list.duration_ms"; const MCP_TOOLS_FETCH_UNCACHED_DURATION_METRIC: &str = "codex.mcp.tools.fetch_uncached.duration_ms"; const MCP_TOOLS_CACHE_WRITE_DURATION_METRIC: &str = "codex.mcp.tools.cache_write.duration_ms"; -/// The Responses API requires tool names to match `^[a-zA-Z0-9_-]+$`. -/// MCP server/tool names are user-controlled, so sanitize the fully-qualified -/// name we expose to the model by replacing any disallowed character with `_`. -fn sanitize_responses_api_tool_name(name: &str) -> String { - let mut sanitized = String::with_capacity(name.len()); - for c in name.chars() { - if c.is_ascii_alphanumeric() || c == '_' { - sanitized.push(c); - } else { - sanitized.push('_'); - } - } - - if sanitized.is_empty() { - "_".to_string() - } else { - sanitized - } -} - fn sha1_hex(s: &str) -> String { let mut hasher = Sha1::new(); hasher.update(s.as_bytes()); diff --git a/codex-rs/tui/src/history_cell.rs b/codex-rs/tui/src/history_cell.rs index 2dee1e846b..1be1c5342b 100644 --- a/codex-rs/tui/src/history_cell.rs +++ b/codex-rs/tui/src/history_cell.rs @@ -42,6 +42,7 @@ use base64::Engine; use codex_core::config::Config; use codex_core::config::types::McpServerTransportConfig; use codex_core::mcp::McpManager; +use codex_core::mcp::qualified_mcp_tool_name_prefix; use codex_core::plugins::PluginsManager; use codex_core::web_search::web_search_detail; use codex_otel::RuntimeMetricsSummary; @@ -1824,7 +1825,7 @@ pub(crate) fn new_mcp_tools_output( servers.sort_by(|(a, _), (b, _)| a.cmp(b)); for (server, cfg) in servers { - let prefix = format!("mcp__{server}__"); + let prefix = qualified_mcp_tool_name_prefix(server); let mut names: Vec = tools .keys() .filter(|k| k.starts_with(&prefix)) @@ -2544,7 +2545,6 @@ mod tests { use codex_core::config::Config; use codex_core::config::ConfigBuilder; use codex_core::config::types::McpServerConfig; - use codex_core::config::types::McpServerTransportConfig; use codex_otel::RuntimeMetricTotals; use codex_otel::RuntimeMetricsSummary; use codex_protocol::ThreadId; @@ -2582,6 +2582,88 @@ mod tests { std::env::temp_dir() } + fn stdio_server_config( + command: &str, + args: Vec<&str>, + env: Option>, + env_vars: Vec<&str>, + ) -> McpServerConfig { + let mut table = toml::Table::new(); + table.insert( + "command".to_string(), + toml::Value::String(command.to_string()), + ); + if !args.is_empty() { + table.insert( + "args".to_string(), + toml::Value::Array( + args.into_iter() + .map(|arg| toml::Value::String(arg.to_string())) + .collect(), + ), + ); + } + if let Some(env) = env { + table.insert("env".to_string(), string_map_to_toml_value(env)); + } + if !env_vars.is_empty() { + table.insert( + "env_vars".to_string(), + toml::Value::Array( + env_vars + .into_iter() + .map(|name| toml::Value::String(name.to_string())) + .collect(), + ), + ); + } + + toml::Value::Table(table) + .try_into() + .expect("test stdio MCP config should deserialize") + } + + fn streamable_http_server_config( + url: &str, + bearer_token_env_var: Option<&str>, + http_headers: Option>, + env_http_headers: Option>, + ) -> McpServerConfig { + let mut table = toml::Table::new(); + table.insert("url".to_string(), toml::Value::String(url.to_string())); + if let Some(bearer_token_env_var) = bearer_token_env_var { + table.insert( + "bearer_token_env_var".to_string(), + toml::Value::String(bearer_token_env_var.to_string()), + ); + } + if let Some(http_headers) = http_headers { + table.insert( + "http_headers".to_string(), + string_map_to_toml_value(http_headers), + ); + } + if let Some(env_http_headers) = env_http_headers { + table.insert( + "env_http_headers".to_string(), + string_map_to_toml_value(env_http_headers), + ); + } + + toml::Value::Table(table) + .try_into() + .expect("test streamable_http MCP config should deserialize") + } + + fn string_map_to_toml_value(entries: HashMap) -> toml::Value { + toml::Value::Table( + entries + .into_iter() + .map(|(key, value)| (key, toml::Value::String(value))) + .collect(), + ) + } + fn render_lines(lines: &[Line<'static>]) -> Vec { lines .iter() @@ -2897,25 +2979,7 @@ mod tests { let mut config = test_config().await; let mut env = HashMap::new(); env.insert("TOKEN".to_string(), "secret".to_string()); - let stdio_config = McpServerConfig { - transport: McpServerTransportConfig::Stdio { - command: "docs-server".to_string(), - args: vec![], - env: Some(env), - env_vars: vec!["APP_TOKEN".to_string()], - cwd: None, - }, - enabled: true, - required: false, - disabled_reason: None, - startup_timeout_sec: None, - tool_timeout_sec: None, - enabled_tools: None, - disabled_tools: None, - scopes: None, - oauth_resource: None, - tools: HashMap::new(), - }; + let stdio_config = stdio_server_config("docs-server", vec![], Some(env), vec!["APP_TOKEN"]); let mut servers = config.mcp_servers.get().clone(); servers.insert("docs".to_string(), stdio_config); @@ -2923,24 +2987,12 @@ mod tests { headers.insert("Authorization".to_string(), "Bearer secret".to_string()); let mut env_headers = HashMap::new(); env_headers.insert("X-API-Key".to_string(), "API_KEY_ENV".to_string()); - let http_config = McpServerConfig { - transport: McpServerTransportConfig::StreamableHttp { - url: "https://example.com/mcp".to_string(), - bearer_token_env_var: Some("MCP_TOKEN".to_string()), - http_headers: Some(headers), - env_http_headers: Some(env_headers), - }, - enabled: true, - required: false, - disabled_reason: None, - startup_timeout_sec: None, - tool_timeout_sec: None, - enabled_tools: None, - disabled_tools: None, - scopes: None, - oauth_resource: None, - tools: HashMap::new(), - }; + let http_config = streamable_http_server_config( + "https://example.com/mcp", + Some("MCP_TOKEN"), + Some(headers), + Some(env_headers), + ); servers.insert("http".to_string(), http_config); config .mcp_servers @@ -2988,6 +3040,46 @@ mod tests { insta::assert_snapshot!(rendered); } + #[tokio::test] + async fn mcp_tools_output_lists_tools_for_hyphenated_server_names() { + let mut config = test_config().await; + let mut servers = config.mcp_servers.get().clone(); + servers.insert( + "some-server".to_string(), + stdio_server_config("docs-server", vec!["--stdio"], None, vec![]), + ); + config + .mcp_servers + .set(servers) + .expect("test mcp servers should accept any configuration"); + + let tools = HashMap::from([( + "mcp__some_server__lookup".to_string(), + Tool { + description: None, + name: "lookup".to_string(), + title: None, + input_schema: serde_json::json!({"type": "object", "properties": {}}), + output_schema: None, + annotations: None, + icons: None, + meta: None, + }, + )]); + + let auth_statuses: HashMap = HashMap::new(); + let cell = new_mcp_tools_output( + &config, + tools, + HashMap::new(), + HashMap::new(), + &auth_statuses, + ); + let rendered = render_lines(&cell.display_lines(120)).join("\n"); + + insta::assert_snapshot!(rendered); + } + #[test] fn empty_agent_message_cell_transcript() { let cell = AgentMessageCell::new(vec![Line::default()], false); diff --git a/codex-rs/tui/src/snapshots/codex_tui__history_cell__tests__mcp_tools_output_lists_tools_for_hyphenated_server_names.snap b/codex-rs/tui/src/snapshots/codex_tui__history_cell__tests__mcp_tools_output_lists_tools_for_hyphenated_server_names.snap new file mode 100644 index 0000000000..d3ca4c3984 --- /dev/null +++ b/codex-rs/tui/src/snapshots/codex_tui__history_cell__tests__mcp_tools_output_lists_tools_for_hyphenated_server_names.snap @@ -0,0 +1,16 @@ +--- +source: tui/src/history_cell.rs +assertion_line: 3080 +expression: rendered +--- +/mcp + +🔌 MCP Tools + + • some-server + • Status: enabled + • Auth: Unsupported + • Command: docs-server --stdio + • Tools: lookup + • Resources: (none) + • Resource templates: (none) diff --git a/codex-rs/tui_app_server/src/history_cell.rs b/codex-rs/tui_app_server/src/history_cell.rs index 7e37a6bf98..37d52b4f9d 100644 --- a/codex-rs/tui_app_server/src/history_cell.rs +++ b/codex-rs/tui_app_server/src/history_cell.rs @@ -45,6 +45,8 @@ use codex_core::config::types::McpServerTransportConfig; #[cfg(test)] use codex_core::mcp::McpManager; #[cfg(test)] +use codex_core::mcp::qualified_mcp_tool_name_prefix; +#[cfg(test)] use codex_core::plugins::PluginsManager; use codex_core::web_search::web_search_detail; use codex_otel::RuntimeMetricsSummary; @@ -1831,7 +1833,7 @@ pub(crate) fn new_mcp_tools_output( servers.sort_by(|(a, _), (b, _)| a.cmp(b)); for (server, cfg) in servers { - let prefix = format!("mcp__{server}__"); + let prefix = qualified_mcp_tool_name_prefix(server); let mut names: Vec = tools .keys() .filter(|k| k.starts_with(&prefix)) @@ -2773,7 +2775,6 @@ mod tests { use codex_core::config::ConfigBuilder; use codex_core::config::types::McpServerConfig; use codex_core::config::types::McpServerDisabledReason; - use codex_core::config::types::McpServerTransportConfig; use codex_otel::RuntimeMetricTotals; use codex_otel::RuntimeMetricsSummary; use codex_protocol::ThreadId; @@ -2811,6 +2812,88 @@ mod tests { std::env::temp_dir() } + fn stdio_server_config( + command: &str, + args: Vec<&str>, + env: Option>, + env_vars: Vec<&str>, + ) -> McpServerConfig { + let mut table = toml::Table::new(); + table.insert( + "command".to_string(), + toml::Value::String(command.to_string()), + ); + if !args.is_empty() { + table.insert( + "args".to_string(), + toml::Value::Array( + args.into_iter() + .map(|arg| toml::Value::String(arg.to_string())) + .collect(), + ), + ); + } + if let Some(env) = env { + table.insert("env".to_string(), string_map_to_toml_value(env)); + } + if !env_vars.is_empty() { + table.insert( + "env_vars".to_string(), + toml::Value::Array( + env_vars + .into_iter() + .map(|name| toml::Value::String(name.to_string())) + .collect(), + ), + ); + } + + toml::Value::Table(table) + .try_into() + .expect("test stdio MCP config should deserialize") + } + + fn streamable_http_server_config( + url: &str, + bearer_token_env_var: Option<&str>, + http_headers: Option>, + env_http_headers: Option>, + ) -> McpServerConfig { + let mut table = toml::Table::new(); + table.insert("url".to_string(), toml::Value::String(url.to_string())); + if let Some(bearer_token_env_var) = bearer_token_env_var { + table.insert( + "bearer_token_env_var".to_string(), + toml::Value::String(bearer_token_env_var.to_string()), + ); + } + if let Some(http_headers) = http_headers { + table.insert( + "http_headers".to_string(), + string_map_to_toml_value(http_headers), + ); + } + if let Some(env_http_headers) = env_http_headers { + table.insert( + "env_http_headers".to_string(), + string_map_to_toml_value(env_http_headers), + ); + } + + toml::Value::Table(table) + .try_into() + .expect("test streamable_http MCP config should deserialize") + } + + fn string_map_to_toml_value(entries: HashMap) -> toml::Value { + toml::Value::Table( + entries + .into_iter() + .map(|(key, value)| (key, toml::Value::String(value))) + .collect(), + ) + } + fn render_lines(lines: &[Line<'static>]) -> Vec { lines .iter() @@ -3126,25 +3209,7 @@ mod tests { let mut config = test_config().await; let mut env = HashMap::new(); env.insert("TOKEN".to_string(), "secret".to_string()); - let stdio_config = McpServerConfig { - transport: McpServerTransportConfig::Stdio { - command: "docs-server".to_string(), - args: vec![], - env: Some(env), - env_vars: vec!["APP_TOKEN".to_string()], - cwd: None, - }, - enabled: true, - required: false, - disabled_reason: None, - startup_timeout_sec: None, - tool_timeout_sec: None, - enabled_tools: None, - disabled_tools: None, - scopes: None, - oauth_resource: None, - tools: HashMap::new(), - }; + let stdio_config = stdio_server_config("docs-server", vec![], Some(env), vec!["APP_TOKEN"]); let mut servers = config.mcp_servers.get().clone(); servers.insert("docs".to_string(), stdio_config); @@ -3152,24 +3217,12 @@ mod tests { headers.insert("Authorization".to_string(), "Bearer secret".to_string()); let mut env_headers = HashMap::new(); env_headers.insert("X-API-Key".to_string(), "API_KEY_ENV".to_string()); - let http_config = McpServerConfig { - transport: McpServerTransportConfig::StreamableHttp { - url: "https://example.com/mcp".to_string(), - bearer_token_env_var: Some("MCP_TOKEN".to_string()), - http_headers: Some(headers), - env_http_headers: Some(env_headers), - }, - enabled: true, - required: false, - disabled_reason: None, - startup_timeout_sec: None, - tool_timeout_sec: None, - enabled_tools: None, - disabled_tools: None, - scopes: None, - oauth_resource: None, - tools: HashMap::new(), - }; + let http_config = streamable_http_server_config( + "https://example.com/mcp", + Some("MCP_TOKEN"), + Some(headers), + Some(env_headers), + ); servers.insert("http".to_string(), http_config); config .mcp_servers @@ -3218,30 +3271,52 @@ mod tests { } #[tokio::test] - async fn mcp_tools_output_from_statuses_renders_status_only_servers() { + async fn mcp_tools_output_lists_tools_for_hyphenated_server_names() { let mut config = test_config().await; - let servers = HashMap::from([( - "plugin_docs".to_string(), - McpServerConfig { - transport: McpServerTransportConfig::Stdio { - command: "docs-server".to_string(), - args: vec!["--stdio".to_string()], - env: None, - env_vars: vec![], - cwd: None, - }, - enabled: false, - required: false, - disabled_reason: Some(McpServerDisabledReason::Unknown), - startup_timeout_sec: None, - tool_timeout_sec: None, - enabled_tools: None, - disabled_tools: None, - scopes: None, - oauth_resource: None, - tools: HashMap::new(), + let mut servers = config.mcp_servers.get().clone(); + servers.insert( + "some-server".to_string(), + stdio_server_config("docs-server", vec!["--stdio"], None, vec![]), + ); + config + .mcp_servers + .set(servers) + .expect("test mcp servers should accept any configuration"); + + let tools = HashMap::from([( + "mcp__some_server__lookup".to_string(), + Tool { + description: None, + name: "lookup".to_string(), + title: None, + input_schema: serde_json::json!({"type": "object", "properties": {}}), + output_schema: None, + annotations: None, + icons: None, + meta: None, }, )]); + + let auth_statuses: HashMap = HashMap::new(); + let cell = new_mcp_tools_output( + &config, + tools, + HashMap::new(), + HashMap::new(), + &auth_statuses, + ); + let rendered = render_lines(&cell.display_lines(120)).join("\n"); + + insta::assert_snapshot!(rendered); + } + + #[tokio::test] + async fn mcp_tools_output_from_statuses_renders_status_only_servers() { + let mut config = test_config().await; + let mut plugin_docs = stdio_server_config("docs-server", vec!["--stdio"], None, vec![]); + plugin_docs.enabled = false; + plugin_docs.disabled_reason = Some(McpServerDisabledReason::Unknown); + let servers = HashMap::from([("plugin_docs".to_string(), plugin_docs)]); config .mcp_servers .set(servers) diff --git a/codex-rs/tui_app_server/src/snapshots/codex_tui_app_server__history_cell__tests__mcp_tools_output_lists_tools_for_hyphenated_server_names.snap b/codex-rs/tui_app_server/src/snapshots/codex_tui_app_server__history_cell__tests__mcp_tools_output_lists_tools_for_hyphenated_server_names.snap new file mode 100644 index 0000000000..c859344b32 --- /dev/null +++ b/codex-rs/tui_app_server/src/snapshots/codex_tui_app_server__history_cell__tests__mcp_tools_output_lists_tools_for_hyphenated_server_names.snap @@ -0,0 +1,16 @@ +--- +source: tui_app_server/src/history_cell.rs +assertion_line: 3310 +expression: rendered +--- +/mcp + +🔌 MCP Tools + + • some-server + • Status: enabled + • Auth: Unsupported + • Command: docs-server --stdio + • Tools: lookup + • Resources: (none) + • Resource templates: (none)