From a101766ff4c248f3793acfe82cbf60016f5e2d75 Mon Sep 17 00:00:00 2001 From: pakrym-oai Date: Wed, 6 May 2026 18:34:46 -0700 Subject: [PATCH 1/4] Remove string-keyed MCP tool maps --- codex-rs/codex-mcp/src/codex_apps.rs | 11 - codex-rs/codex-mcp/src/connection_manager.rs | 17 +- .../codex-mcp/src/connection_manager_tests.rs | 140 ++++---- codex-rs/codex-mcp/src/lib.rs | 1 - codex-rs/codex-mcp/src/mcp/mod.rs | 2 +- codex-rs/codex-mcp/src/tools.rs | 30 +- codex-rs/core/src/connectors.rs | 8 +- codex-rs/core/src/connectors_tests.rs | 124 +++---- codex-rs/core/src/mcp_tool_call.rs | 4 +- codex-rs/core/src/mcp_tool_exposure.rs | 32 +- codex-rs/core/src/mcp_tool_exposure_test.rs | 129 ++++---- codex-rs/core/src/plugins/injection.rs | 5 +- codex-rs/core/src/session/turn.rs | 6 +- .../core/src/tools/handlers/tool_search.rs | 31 +- codex-rs/core/src/tools/router.rs | 5 +- codex-rs/core/src/tools/spec.rs | 27 +- codex-rs/core/src/tools/spec_tests.rs | 305 ++++++++---------- codex-rs/core/src/tools/tool_search_entry.rs | 7 +- codex-rs/core/src/unavailable_tool.rs | 11 +- 19 files changed, 434 insertions(+), 461 deletions(-) diff --git a/codex-rs/codex-mcp/src/codex_apps.rs b/codex-rs/codex-mcp/src/codex_apps.rs index 0a7981fb0d5f..81643e666560 100644 --- a/codex-rs/codex-mcp/src/codex_apps.rs +++ b/codex-rs/codex-mcp/src/codex_apps.rs @@ -5,7 +5,6 @@ //! connector allow-list filtering, and the normalization that turns app //! connector/tool metadata into model-visible MCP callable names. -use std::collections::HashMap; use std::path::PathBuf; use std::time::Instant; @@ -38,16 +37,6 @@ pub fn codex_apps_tools_cache_key(auth: Option<&CodexAuth>) -> CodexAppsToolsCac } } -pub fn filter_non_codex_apps_mcp_tools_only( - mcp_tools: &HashMap, -) -> HashMap { - mcp_tools - .iter() - .filter(|(_, tool)| tool.server_name != CODEX_APPS_MCP_SERVER_NAME) - .map(|(name, tool)| (name.clone(), tool.clone())) - .collect() -} - #[derive(Clone)] pub(crate) struct CodexAppsToolsCacheContext { pub(crate) codex_home: PathBuf, diff --git a/codex-rs/codex-mcp/src/connection_manager.rs b/codex-rs/codex-mcp/src/connection_manager.rs index f93ac5e089fd..5a05092522c3 100644 --- a/codex-rs/codex-mcp/src/connection_manager.rs +++ b/codex-rs/codex-mcp/src/connection_manager.rs @@ -31,7 +31,7 @@ use crate::runtime::McpRuntimeEnvironment; use crate::runtime::emit_duration; use crate::tools::ToolInfo; use crate::tools::filter_tools; -use crate::tools::qualify_tools; +use crate::tools::normalize_tools_for_model; use crate::tools::tool_with_model_visible_input_schema; use anyhow::Context; use anyhow::Result; @@ -337,10 +337,9 @@ impl McpConnectionManager { failures } - /// Returns a single map that contains all tools. Each key is the - /// fully-qualified name for the tool. + /// Returns all tools with model-visible names normalized. #[instrument(level = "trace", skip_all)] - pub async fn list_all_tools(&self) -> HashMap { + pub async fn list_all_tools(&self) -> Vec { let mut tools = Vec::new(); for managed_client in self.clients.values() { let Some(server_tools) = managed_client.listed_tools().await else { @@ -348,15 +347,15 @@ impl McpConnectionManager { }; tools.extend(server_tools); } - qualify_tools(tools) + normalize_tools_for_model(tools) } /// Force-refresh codex apps tools by bypassing the in-process cache. /// /// On success, the refreshed tools replace the cache contents and the - /// latest filtered tool map is returned directly to the caller. On + /// latest filtered tools are returned directly to the caller. On /// failure, the existing cache remains unchanged. - pub async fn hard_refresh_codex_apps_tools_cache(&self) -> Result> { + pub async fn hard_refresh_codex_apps_tools_cache(&self) -> Result> { let managed_client = self .clients .get(CODEX_APPS_MCP_SERVER_NAME) @@ -399,7 +398,7 @@ impl McpConnectionManager { tool.tool = tool_with_model_visible_input_schema(&tool.tool); tool }); - Ok(qualify_tools(tools)) + Ok(normalize_tools_for_model(tools)) } /// Returns a single map that contains all resources. Each key is the @@ -638,7 +637,7 @@ impl McpConnectionManager { pub async fn resolve_tool_info(&self, tool_name: &ToolName) -> Option { let all_tools = self.list_all_tools().await; all_tools - .into_values() + .into_iter() .find(|tool| tool.canonical_tool_name() == *tool_name) } diff --git a/codex-rs/codex-mcp/src/connection_manager_tests.rs b/codex-rs/codex-mcp/src/connection_manager_tests.rs index f2dc85cde043..5c430bd8e56d 100644 --- a/codex-rs/codex-mcp/src/connection_manager_tests.rs +++ b/codex-rs/codex-mcp/src/connection_manager_tests.rs @@ -14,7 +14,7 @@ use crate::rmcp_client::elicitation_capability_for_server; use crate::tools::ToolFilter; use crate::tools::ToolInfo; use crate::tools::filter_tools; -use crate::tools::qualify_tools; +use crate::tools::normalize_tools_for_model; use crate::tools::tool_with_model_visible_input_schema; use codex_config::Constrained; use codex_protocol::ToolName; @@ -85,6 +85,15 @@ fn create_codex_apps_tools_cache_context( } } +fn sorted_model_tool_names(tools: &[ToolInfo]) -> Vec { + let mut names = tools + .iter() + .map(|tool| tool.canonical_tool_name().display()) + .collect::>(); + names.sort(); + names +} + #[test] fn declared_openai_file_fields_treat_names_literally() { let meta = serde_json::json!({ @@ -272,35 +281,41 @@ async fn disabled_permissions_do_not_auto_accept_elicitation_with_requested_fiel } #[test] -fn test_qualify_tools_short_non_duplicated_names() { +fn test_normalize_tools_short_non_duplicated_names() { let tools = vec![ create_test_tool("server1", "tool1"), create_test_tool("server1", "tool2"), ]; - let qualified_tools = qualify_tools(tools); + let model_tools = normalize_tools_for_model(tools); - assert_eq!(qualified_tools.len(), 2); - assert!(qualified_tools.contains_key("mcp__server1__tool1")); - assert!(qualified_tools.contains_key("mcp__server1__tool2")); + assert_eq!( + sorted_model_tool_names(&model_tools), + vec![ + "mcp__server1__tool1".to_string(), + "mcp__server1__tool2".to_string() + ] + ); } #[test] -fn test_qualify_tools_duplicated_names_skipped() { +fn test_normalize_tools_duplicated_names_skipped() { let tools = vec![ create_test_tool("server1", "duplicate_tool"), create_test_tool("server1", "duplicate_tool"), ]; - let qualified_tools = qualify_tools(tools); + let model_tools = normalize_tools_for_model(tools); // Only the first tool should remain, the second is skipped - assert_eq!(qualified_tools.len(), 1); - assert!(qualified_tools.contains_key("mcp__server1__duplicate_tool")); + assert_eq!( + sorted_model_tool_names(&model_tools), + vec!["mcp__server1__duplicate_tool".to_string()] + ); } #[test] -fn test_qualify_tools_long_names_same_server() { +fn test_normalize_tools_long_names_same_server() { let server_name = "my_server"; let tools = vec![ @@ -314,116 +329,125 @@ fn test_qualify_tools_long_names_same_server() { ), ]; - let qualified_tools = qualify_tools(tools); + let model_tools = normalize_tools_for_model(tools); - assert_eq!(qualified_tools.len(), 2); + assert_eq!(model_tools.len(), 2); - let mut keys: Vec<_> = qualified_tools.keys().cloned().collect(); - keys.sort(); + let names = sorted_model_tool_names(&model_tools); - assert!(keys.iter().all(|key| key.len() == 64)); - assert!(keys.iter().all(|key| key.starts_with("mcp__my_server__"))); + assert!(names.iter().all(|name| name.len() == 64)); + assert!( + names + .iter() + .all(|name| name.starts_with("mcp__my_server__")) + ); assert!( - keys.iter() - .all(|key| key.chars().all(|c| c.is_ascii_alphanumeric() || c == '_')), - "qualified names must be code-mode compatible: {keys:?}" + names + .iter() + .all(|name| name.chars().all(|c| c.is_ascii_alphanumeric() || c == '_')), + "model-visible names must be code-mode compatible: {names:?}" ); } #[test] -fn test_qualify_tools_sanitizes_invalid_characters() { +fn test_normalize_tools_sanitizes_invalid_characters() { let tools = vec![create_test_tool("server.one", "tool.two-three")]; - let qualified_tools = qualify_tools(tools); + let model_tools = normalize_tools_for_model(tools); - assert_eq!(qualified_tools.len(), 1); - let (qualified_name, tool) = qualified_tools.into_iter().next().expect("one tool"); - assert_eq!(qualified_name, "mcp__server_one__tool_two_three"); + assert_eq!(model_tools.len(), 1); + let tool = model_tools.into_iter().next().expect("one tool"); + let model_name = tool.canonical_tool_name().display(); + assert_eq!(model_name, "mcp__server_one__tool_two_three"); assert_eq!( format!("{}{}", tool.callable_namespace, tool.callable_name), - qualified_name + model_name ); - // The key and callable parts are sanitized for model-visible tool calls, but - // the raw MCP name is preserved for the actual MCP call. + // The callable parts are sanitized for model-visible tool calls, but the raw + // MCP name is preserved for the actual MCP call. assert_eq!(tool.server_name, "server.one"); assert_eq!(tool.callable_namespace, "mcp__server_one__"); assert_eq!(tool.callable_name, "tool_two_three"); assert_eq!(tool.tool.name, "tool.two-three"); assert!( - qualified_name + model_name .chars() .all(|c| c.is_ascii_alphanumeric() || c == '_'), - "qualified name must be code-mode compatible: {qualified_name:?}" + "model-visible name must be code-mode compatible: {model_name:?}" ); } #[test] -fn test_qualify_tools_keeps_hyphenated_mcp_tools_callable() { +fn test_normalize_tools_keeps_hyphenated_mcp_tools_callable() { let tools = vec![create_test_tool("music-studio", "get-strudel-guide")]; - let qualified_tools = qualify_tools(tools); + let model_tools = normalize_tools_for_model(tools); - assert_eq!(qualified_tools.len(), 1); - let (qualified_name, tool) = qualified_tools.into_iter().next().expect("one tool"); - assert_eq!(qualified_name, "mcp__music_studio__get_strudel_guide"); + assert_eq!(model_tools.len(), 1); + let tool = model_tools.into_iter().next().expect("one tool"); + assert_eq!( + tool.canonical_tool_name().display(), + "mcp__music_studio__get_strudel_guide" + ); assert_eq!(tool.callable_namespace, "mcp__music_studio__"); assert_eq!(tool.callable_name, "get_strudel_guide"); assert_eq!(tool.tool.name, "get-strudel-guide"); } #[test] -fn test_qualify_tools_disambiguates_sanitized_namespace_collisions() { +fn test_normalize_tools_disambiguates_sanitized_namespace_collisions() { let tools = vec![ create_test_tool("basic-server", "lookup"), create_test_tool("basic_server", "query"), ]; - let qualified_tools = qualify_tools(tools); + let model_tools = normalize_tools_for_model(tools); - assert_eq!(qualified_tools.len(), 2); - let mut namespaces = qualified_tools - .values() + assert_eq!(model_tools.len(), 2); + let mut namespaces = model_tools + .iter() .map(|tool| tool.callable_namespace.as_str()) .collect::>(); namespaces.sort(); namespaces.dedup(); assert_eq!(namespaces.len(), 2); - let raw_servers = qualified_tools - .values() + let raw_servers = model_tools + .iter() .map(|tool| tool.server_name.as_str()) .collect::>(); assert_eq!(raw_servers, HashSet::from(["basic-server", "basic_server"])); + let model_names = sorted_model_tool_names(&model_tools); assert!( - qualified_tools - .keys() - .all(|key| key.chars().all(|c| c.is_ascii_alphanumeric() || c == '_')), - "qualified names must be code-mode compatible: {qualified_tools:?}" + model_names + .iter() + .all(|name| name.chars().all(|c| c.is_ascii_alphanumeric() || c == '_')), + "model-visible names must be code-mode compatible: {model_names:?}" ); } #[test] -fn test_qualify_tools_disambiguates_sanitized_tool_name_collisions() { +fn test_normalize_tools_disambiguates_sanitized_tool_name_collisions() { let tools = vec![ create_test_tool("server", "tool-name"), create_test_tool("server", "tool_name"), ]; - let qualified_tools = qualify_tools(tools); + let model_tools = normalize_tools_for_model(tools); - assert_eq!(qualified_tools.len(), 2); - let raw_tool_names = qualified_tools - .values() + assert_eq!(model_tools.len(), 2); + let raw_tool_names = model_tools + .iter() .map(|tool| tool.tool.name.to_string()) .collect::>(); assert_eq!( raw_tool_names, HashSet::from(["tool-name".to_string(), "tool_name".to_string()]) ); - let callable_tool_names = qualified_tools - .values() + let callable_tool_names = model_tools + .iter() .map(|tool| tool.callable_name.as_str()) .collect::>(); assert_eq!(callable_tool_names.len(), 2); @@ -672,7 +696,10 @@ async fn list_all_tools_uses_startup_snapshot_while_client_is_pending() { let tools = manager.list_all_tools().await; let tool = tools - .get("mcp__codex_apps__calendar_create_event") + .iter() + .find(|tool| { + tool.canonical_tool_name().display() == "mcp__codex_apps__calendar_create_event" + }) .expect("tool from startup cache"); assert_eq!(tool.server_name, CODEX_APPS_MCP_SERVER_NAME); assert_eq!(tool.callable_name, "calendar_create_event"); @@ -798,7 +825,10 @@ async fn list_all_tools_uses_startup_snapshot_when_client_startup_fails() { let tools = manager.list_all_tools().await; let tool = tools - .get("mcp__codex_apps__calendar_create_event") + .iter() + .find(|tool| { + tool.canonical_tool_name().display() == "mcp__codex_apps__calendar_create_event" + }) .expect("tool from startup cache"); assert_eq!(tool.server_name, CODEX_APPS_MCP_SERVER_NAME); assert_eq!(tool.callable_name, "calendar_create_event"); diff --git a/codex-rs/codex-mcp/src/lib.rs b/codex-rs/codex-mcp/src/lib.rs index 83dc65dd0698..d0c3e52f8cc6 100644 --- a/codex-rs/codex-mcp/src/lib.rs +++ b/codex-rs/codex-mcp/src/lib.rs @@ -48,7 +48,6 @@ pub use mcp::oauth_login_support; pub use mcp::resolve_oauth_scopes; pub use mcp::should_retry_without_scopes; -pub use codex_apps::filter_non_codex_apps_mcp_tools_only; pub use mcp::McpPermissionPromptAutoApproveContext; pub use mcp::mcp_permission_prompt_is_auto_approved; pub use mcp::qualified_mcp_tool_name_prefix; diff --git a/codex-rs/codex-mcp/src/mcp/mod.rs b/codex-rs/codex-mcp/src/mcp/mod.rs index ae667c698a16..118c4c048473 100644 --- a/codex-rs/codex-mcp/src/mcp/mod.rs +++ b/codex-rs/codex-mcp/src/mcp/mod.rs @@ -564,7 +564,7 @@ async fn collect_mcp_server_status_snapshot_from_manager( ); let mut tools_by_server = HashMap::>::new(); - for (_qualified_name, tool_info) in tools { + for tool_info in tools { let raw_tool_name = tool_info.tool.name.to_string(); let Some(tool) = protocol_tool_from_rmcp_tool(&raw_tool_name, &tool_info.tool) else { continue; diff --git a/codex-rs/codex-mcp/src/tools.rs b/codex-rs/codex-mcp/src/tools.rs index 1a3e1a6bc230..cb3d8babae6a 100644 --- a/codex-rs/codex-mcp/src/tools.rs +++ b/codex-rs/codex-mcp/src/tools.rs @@ -1,4 +1,4 @@ -//! MCP tool metadata, filtering, schema shaping, and name qualification. +//! MCP tool metadata, filtering, schema shaping, and name normalization. //! //! Raw MCP tool identities must be preserved for protocol calls, while //! model-visible tool names must be sanitized, deduplicated, and kept within API @@ -130,12 +130,12 @@ pub(crate) fn filter_tools(tools: Vec, filter: &ToolFilter) -> Vec(tools: I) -> HashMap +/// every model-visible name is unique and <= 64 bytes. +pub(crate) fn normalize_tools_for_model(tools: I) -> Vec where I: IntoIterator, { @@ -213,9 +213,9 @@ where candidates.sort_by(|left, right| left.raw_tool_identity.cmp(&right.raw_tool_identity)); let mut used_names = HashSet::new(); - let mut qualified_tools = HashMap::new(); + let mut model_tools = Vec::new(); for mut candidate in candidates { - let (callable_namespace, callable_name, qualified_name) = unique_callable_parts( + let (callable_namespace, callable_name) = unique_callable_parts( &candidate.callable_namespace, &candidate.callable_name, &candidate.raw_tool_identity, @@ -223,9 +223,9 @@ where ); candidate.tool.callable_namespace = callable_namespace; candidate.tool.callable_name = callable_name; - qualified_tools.insert(qualified_name, candidate.tool); + model_tools.push(candidate.tool); } - qualified_tools + model_tools } #[derive(Debug)] @@ -345,10 +345,10 @@ fn unique_callable_parts( tool_name: &str, raw_identity: &str, used_names: &mut HashSet, -) -> (String, String, String) { - let qualified_name = format!("{namespace}{tool_name}"); - if qualified_name.len() <= MAX_TOOL_NAME_LENGTH && used_names.insert(qualified_name.clone()) { - return (namespace.to_string(), tool_name.to_string(), qualified_name); +) -> (String, String) { + let model_name = format!("{namespace}{tool_name}"); + if model_name.len() <= MAX_TOOL_NAME_LENGTH && used_names.insert(model_name) { + return (namespace.to_string(), tool_name.to_string()); } let mut attempt = 0_u32; @@ -360,9 +360,9 @@ fn unique_callable_parts( }; let (namespace, tool_name) = fit_callable_parts_with_hash(namespace, tool_name, &hash_input); - let qualified_name = format!("{namespace}{tool_name}"); - if used_names.insert(qualified_name.clone()) { - return (namespace, tool_name, qualified_name); + let model_name = format!("{namespace}{tool_name}"); + if used_names.insert(model_name) { + return (namespace, tool_name); } attempt = attempt.saturating_add(1); } diff --git a/codex-rs/core/src/connectors.rs b/codex-rs/core/src/connectors.rs index 0bd53a50eed0..d9d713e3e226 100644 --- a/codex-rs/core/src/connectors.rs +++ b/codex-rs/core/src/connectors.rs @@ -165,7 +165,7 @@ pub async fn list_cached_accessible_connectors_from_mcp_tools( pub(crate) fn refresh_accessible_connectors_cache_from_mcp_tools( config: &Config, auth: Option<&CodexAuth>, - mcp_tools: &HashMap, + mcp_tools: &[ToolInfo], ) { if !config.features.enabled(Feature::Apps) { return; @@ -516,12 +516,10 @@ async fn chatgpt_get_request_with_auth_provider( } } -pub(crate) fn accessible_connectors_from_mcp_tools( - mcp_tools: &HashMap, -) -> Vec { +pub(crate) fn accessible_connectors_from_mcp_tools(mcp_tools: &[ToolInfo]) -> Vec { // ToolInfo already carries plugin provenance, so app-level plugin sources // can be derived here instead of requiring a separate enrichment pass. - let tools = mcp_tools.values().filter_map(|tool| { + let tools = mcp_tools.iter().filter_map(|tool| { if tool.server_name != CODEX_APPS_MCP_SERVER_NAME { return None; } diff --git a/codex-rs/core/src/connectors_tests.rs b/codex-rs/core/src/connectors_tests.rs index 513f677e9b92..014ab1cad8d6 100644 --- a/codex-rs/core/src/connectors_tests.rs +++ b/codex-rs/core/src/connectors_tests.rs @@ -172,39 +172,30 @@ fn merge_connectors_replaces_plugin_placeholder_name_with_accessible_name() { #[test] fn accessible_connectors_from_mcp_tools_carries_plugin_display_names() { - let tools = HashMap::from([ - ( - "mcp__codex_apps__calendar_list_events".to_string(), - codex_app_tool( - "calendar_list_events", - "calendar", - /*connector_name*/ None, - &["sample", "sample"], - ), - ), - ( - "mcp__codex_apps__calendar_create_event".to_string(), - codex_app_tool( - "calendar_create_event", - "calendar", - Some("Google Calendar"), - &["beta", "sample"], - ), + let tools = vec![ + codex_app_tool( + "calendar_list_events", + "calendar", + /*connector_name*/ None, + &["sample", "sample"], ), - ( - "mcp__sample__echo".to_string(), - ToolInfo { - server_name: "sample".to_string(), - callable_name: "echo".to_string(), - callable_namespace: "sample".to_string(), - namespace_description: None, - tool: test_tool_definition("echo"), - connector_id: None, - connector_name: None, - plugin_display_names: plugin_names(&["ignored"]), - }, + codex_app_tool( + "calendar_create_event", + "calendar", + Some("Google Calendar"), + &["beta", "sample"], ), - ]); + ToolInfo { + server_name: "sample".to_string(), + callable_name: "echo".to_string(), + callable_namespace: "sample".to_string(), + namespace_description: None, + tool: test_tool_definition("echo"), + connector_id: None, + connector_name: None, + plugin_display_names: plugin_names(&["ignored"]), + }, + ]; let connectors = accessible_connectors_from_mcp_tools(&tools); @@ -238,26 +229,20 @@ async fn refresh_accessible_connectors_cache_from_mcp_tools_writes_latest_instal .expect("config should load"); let _ = config.features.set_enabled(Feature::Apps, /*enabled*/ true); let cache_key = accessible_connectors_cache_key(&config, /*auth*/ None); - let tools = HashMap::from([ - ( - "mcp__codex_apps__calendar_list_events".to_string(), - codex_app_tool( - "calendar_list_events", - "calendar", - Some("Google Calendar"), - &["calendar-plugin"], - ), + let tools = vec![ + codex_app_tool( + "calendar_list_events", + "calendar", + Some("Google Calendar"), + &["calendar-plugin"], ), - ( - "mcp__codex_apps__openai_hidden".to_string(), - codex_app_tool( - "openai_hidden", - "connector_openai_hidden", - Some("Hidden"), - &[], - ), + codex_app_tool( + "openai_hidden", + "connector_openai_hidden", + Some("Hidden"), + &[], ), - ]); + ]; let cached = with_accessible_connectors_cache_cleared(|| { refresh_accessible_connectors_cache_from_mcp_tools(&config, /*auth*/ None, &tools); @@ -315,29 +300,26 @@ fn merge_connectors_unions_and_dedupes_plugin_display_names() { #[test] fn accessible_connectors_from_mcp_tools_preserves_description() { - let mcp_tools = HashMap::from([( - "mcp__codex_apps__calendar_create_event".to_string(), - ToolInfo { - server_name: CODEX_APPS_MCP_SERVER_NAME.to_string(), - callable_name: "calendar_create_event".to_string(), - callable_namespace: "mcp__codex_apps__calendar".to_string(), - namespace_description: Some("Plan events".to_string()), - tool: Tool { - name: "calendar_create_event".to_string().into(), - title: None, - description: Some("Create a calendar event".into()), - input_schema: Arc::new(JsonObject::default()), - output_schema: None, - annotations: None, - execution: None, - icons: None, - meta: None, - }, - connector_id: Some("calendar".to_string()), - connector_name: Some("Calendar".to_string()), - plugin_display_names: Vec::new(), + let mcp_tools = vec![ToolInfo { + server_name: CODEX_APPS_MCP_SERVER_NAME.to_string(), + callable_name: "calendar_create_event".to_string(), + callable_namespace: "mcp__codex_apps__calendar".to_string(), + namespace_description: Some("Plan events".to_string()), + tool: Tool { + name: "calendar_create_event".to_string().into(), + title: None, + description: Some("Create a calendar event".into()), + input_schema: Arc::new(JsonObject::default()), + output_schema: None, + annotations: None, + execution: None, + icons: None, + meta: None, }, - )]); + connector_id: Some("calendar".to_string()), + connector_name: Some("Calendar".to_string()), + plugin_display_names: Vec::new(), + }]; assert_eq!( accessible_connectors_from_mcp_tools(&mcp_tools), diff --git a/codex-rs/core/src/mcp_tool_call.rs b/codex-rs/core/src/mcp_tool_call.rs index c7e665cb02ab..74acdc330f98 100644 --- a/codex-rs/core/src/mcp_tool_call.rs +++ b/codex-rs/core/src/mcp_tool_call.rs @@ -1477,7 +1477,7 @@ pub(crate) async fn lookup_mcp_tool_metadata( .list_all_tools() .await; let tool_info = tools - .into_values() + .into_iter() .find(|tool_info| tool_info.server_name == server && tool_info.tool.name == tool_name)?; let connector_description = if server == CODEX_APPS_MCP_SERVER_NAME { let connectors = match connectors::list_cached_accessible_connectors_from_mcp_tools( @@ -1572,7 +1572,7 @@ async fn lookup_mcp_app_usage_metadata( .list_all_tools() .await; - tools.into_values().find_map(|tool_info| { + tools.into_iter().find_map(|tool_info| { if tool_info.server_name == server && tool_info.tool.name == tool_name { Some(McpAppUsageMetadata { connector_id: tool_info.connector_id, diff --git a/codex-rs/core/src/mcp_tool_exposure.rs b/codex-rs/core/src/mcp_tool_exposure.rs index 3917a1d5e768..0bf696acdfc6 100644 --- a/codex-rs/core/src/mcp_tool_exposure.rs +++ b/codex-rs/core/src/mcp_tool_exposure.rs @@ -1,10 +1,8 @@ -use std::collections::HashMap; use std::collections::HashSet; use codex_features::Feature; use codex_mcp::CODEX_APPS_MCP_SERVER_NAME; use codex_mcp::ToolInfo as McpToolInfo; -use codex_mcp::filter_non_codex_apps_mcp_tools_only; use codex_tools::ToolsConfig; use crate::config::Config; @@ -13,12 +11,12 @@ use crate::connectors; pub(crate) const DIRECT_MCP_TOOL_EXPOSURE_THRESHOLD: usize = 100; pub(crate) struct McpToolExposure { - pub(crate) direct_tools: HashMap, - pub(crate) deferred_tools: Option>, + pub(crate) direct_tools: Vec, + pub(crate) deferred_tools: Option>, } pub(crate) fn build_mcp_tool_exposure( - all_mcp_tools: &HashMap, + all_mcp_tools: &[McpToolInfo], connectors: Option<&[connectors::AppInfo]>, explicitly_enabled_connectors: &[connectors::AppInfo], config: &Config, @@ -48,9 +46,11 @@ pub(crate) fn build_mcp_tool_exposure( let direct_tools = filter_codex_apps_mcp_tools(all_mcp_tools, explicitly_enabled_connectors, config); - for direct_tool_name in direct_tools.keys() { - deferred_tools.remove(direct_tool_name); - } + let direct_tool_names = direct_tools + .iter() + .map(McpToolInfo::canonical_tool_name) + .collect::>(); + deferred_tools.retain(|tool| !direct_tool_names.contains(&tool.canonical_tool_name())); McpToolExposure { direct_tools, @@ -58,11 +58,19 @@ pub(crate) fn build_mcp_tool_exposure( } } +fn filter_non_codex_apps_mcp_tools_only(mcp_tools: &[McpToolInfo]) -> Vec { + mcp_tools + .iter() + .filter(|tool| tool.server_name != CODEX_APPS_MCP_SERVER_NAME) + .cloned() + .collect() +} + fn filter_codex_apps_mcp_tools( - mcp_tools: &HashMap, + mcp_tools: &[McpToolInfo], connectors: &[connectors::AppInfo], config: &Config, -) -> HashMap { +) -> Vec { let allowed: HashSet<&str> = connectors .iter() .map(|connector| connector.id.as_str()) @@ -70,7 +78,7 @@ fn filter_codex_apps_mcp_tools( mcp_tools .iter() - .filter(|(_, tool)| { + .filter(|tool| { if tool.server_name != CODEX_APPS_MCP_SERVER_NAME { return false; } @@ -79,7 +87,7 @@ fn filter_codex_apps_mcp_tools( }; allowed.contains(connector_id) && connectors::codex_app_tool_is_enabled(config, tool) }) - .map(|(name, tool)| (name.clone(), tool.clone())) + .cloned() .collect() } diff --git a/codex-rs/core/src/mcp_tool_exposure_test.rs b/codex-rs/core/src/mcp_tool_exposure_test.rs index 32707e4f8ba3..0116a62e9612 100644 --- a/codex-rs/core/src/mcp_tool_exposure_test.rs +++ b/codex-rs/core/src/mcp_tool_exposure_test.rs @@ -1,4 +1,4 @@ -use std::collections::HashMap; +use std::collections::HashSet; use std::sync::Arc; use codex_connectors::metadata::sanitize_name; @@ -11,6 +11,7 @@ use codex_protocol::config_types::WebSearchMode; use codex_protocol::config_types::WindowsSandboxLevel; use codex_protocol::models::PermissionProfile; use codex_protocol::protocol::SessionSource; +use codex_tools::ToolName; use codex_tools::ToolsConfig; use codex_tools::ToolsConfigParams; use pretty_assertions::assert_eq; @@ -53,10 +54,25 @@ fn make_mcp_tool( } else { format!("mcp__{server_name}__") }; + let callable_name = if server_name == CODEX_APPS_MCP_SERVER_NAME { + let tool_name = sanitize_name(tool_name); + if let Some(connector_name) = connector_name + .map(sanitize_name) + .filter(|name| !name.is_empty()) + && let Some(stripped) = tool_name.strip_prefix(&connector_name) + && !stripped.is_empty() + { + stripped.to_string() + } else { + tool_name + } + } else { + tool_name.to_string() + }; ToolInfo { server_name: server_name.to_string(), - callable_name: tool_name.to_string(), + callable_name, callable_namespace: tool_namespace, namespace_description: None, tool: Tool { @@ -76,20 +92,24 @@ fn make_mcp_tool( } } -fn numbered_mcp_tools(count: usize) -> HashMap { +fn numbered_mcp_tools(count: usize) -> Vec { (0..count) .map(|index| { let tool_name = format!("tool_{index}"); - ( - format!("mcp__rmcp__{tool_name}"), - make_mcp_tool( - "rmcp", &tool_name, /*connector_id*/ None, /*connector_name*/ None, - ), + make_mcp_tool( + "rmcp", &tool_name, /*connector_id*/ None, /*connector_name*/ None, ) }) .collect() } +fn tool_names(tools: &[ToolInfo]) -> HashSet { + tools + .iter() + .map(codex_mcp::ToolInfo::canonical_tool_name) + .collect() +} + async fn tools_config_for_mcp_tool_exposure(search_tool: bool) -> ToolsConfig { let config = test_config().await; let model_info = @@ -124,11 +144,7 @@ async fn directly_exposes_small_effective_tool_sets() { &tools_config, ); - let mut direct_tool_names: Vec<_> = exposure.direct_tools.keys().cloned().collect(); - direct_tool_names.sort(); - let mut expected_tool_names: Vec<_> = mcp_tools.keys().cloned().collect(); - expected_tool_names.sort(); - assert_eq!(direct_tool_names, expected_tool_names); + assert_eq!(tool_names(&exposure.direct_tools), tool_names(&mcp_tools)); assert!(exposure.deferred_tools.is_none()); } @@ -151,11 +167,7 @@ async fn searches_large_effective_tool_sets() { .deferred_tools .as_ref() .expect("large tool sets should be discoverable through tool_search"); - let mut deferred_tool_names: Vec<_> = deferred_tools.keys().cloned().collect(); - deferred_tool_names.sort(); - let mut expected_tool_names: Vec<_> = mcp_tools.keys().cloned().collect(); - expected_tool_names.sort(); - assert_eq!(deferred_tool_names, expected_tool_names); + assert_eq!(tool_names(deferred_tools), tool_names(&mcp_tools)); } #[tokio::test] @@ -163,15 +175,12 @@ async fn directly_exposes_explicit_apps_without_deferred_overlap() { let config = test_config().await; let tools_config = tools_config_for_mcp_tool_exposure(/*search_tool*/ true).await; let mut mcp_tools = numbered_mcp_tools(DIRECT_MCP_TOOL_EXPOSURE_THRESHOLD - 1); - mcp_tools.extend([( - "mcp__codex_apps__calendar_create_event".to_string(), - make_mcp_tool( - CODEX_APPS_MCP_SERVER_NAME, - "calendar_create_event", - Some("calendar"), - Some("Calendar"), - ), - )]); + mcp_tools.push(make_mcp_tool( + CODEX_APPS_MCP_SERVER_NAME, + "calendar_create_event", + Some("calendar"), + Some("Calendar"), + )); let connectors = vec![make_connector("calendar", "Calendar")]; let exposure = build_mcp_tool_exposure( @@ -182,28 +191,32 @@ async fn directly_exposes_explicit_apps_without_deferred_overlap() { &tools_config, ); - let mut tool_names: Vec = exposure.direct_tools.into_keys().collect(); - tool_names.sort(); + let direct_tool_names = tool_names(&exposure.direct_tools); assert_eq!( - tool_names, - vec!["mcp__codex_apps__calendar_create_event".to_string()] + direct_tool_names, + HashSet::from([ToolName::namespaced( + "mcp__codex_apps__calendar", + "_create_event" + )]) ); assert_eq!( - exposure.deferred_tools.as_ref().map(HashMap::len), + exposure.deferred_tools.as_ref().map(Vec::len), Some(DIRECT_MCP_TOOL_EXPOSURE_THRESHOLD - 1) ); let deferred_tools = exposure .deferred_tools .as_ref() .expect("large tool sets should be discoverable through tool_search"); + let deferred_tool_names = tool_names(deferred_tools); assert!( - tool_names - .iter() - .all(|direct_tool_name| !deferred_tools.contains_key(direct_tool_name)), - "direct tools should not also be deferred: {tool_names:?}" + direct_tool_names.is_disjoint(&deferred_tool_names), + "direct tools should not also be deferred: {direct_tool_names:?}" ); - assert!(!deferred_tools.contains_key("mcp__codex_apps__calendar_create_event")); - assert!(deferred_tools.contains_key("mcp__rmcp__tool_0")); + assert!(!deferred_tool_names.contains(&ToolName::namespaced( + "mcp__codex_apps__calendar", + "_create_event" + ))); + assert!(deferred_tool_names.contains(&ToolName::namespaced("mcp__rmcp__", "tool_0"))); } #[tokio::test] @@ -214,23 +227,17 @@ async fn always_defer_feature_preserves_explicit_apps() { .enable(Feature::ToolSearchAlwaysDeferMcpTools) .expect("test config should allow feature update"); let tools_config = tools_config_for_mcp_tool_exposure(/*search_tool*/ true).await; - let mcp_tools = HashMap::from([ - ( - "mcp__rmcp__tool".to_string(), - make_mcp_tool( - "rmcp", "tool", /*connector_id*/ None, /*connector_name*/ None, - ), + let mcp_tools = vec![ + make_mcp_tool( + "rmcp", "tool", /*connector_id*/ None, /*connector_name*/ None, ), - ( - "mcp__codex_apps__calendar_create_event".to_string(), - make_mcp_tool( - CODEX_APPS_MCP_SERVER_NAME, - "calendar_create_event", - Some("calendar"), - Some("Calendar"), - ), + make_mcp_tool( + CODEX_APPS_MCP_SERVER_NAME, + "calendar_create_event", + Some("calendar"), + Some("Calendar"), ), - ]); + ]; let connectors = vec![make_connector("calendar", "Calendar")]; let exposure = build_mcp_tool_exposure( @@ -241,16 +248,22 @@ async fn always_defer_feature_preserves_explicit_apps() { &tools_config, ); - let mut direct_tool_names: Vec = exposure.direct_tools.into_keys().collect(); - direct_tool_names.sort(); + let direct_tool_names = tool_names(&exposure.direct_tools); assert_eq!( direct_tool_names, - vec!["mcp__codex_apps__calendar_create_event".to_string()] + HashSet::from([ToolName::namespaced( + "mcp__codex_apps__calendar", + "_create_event" + )]) ); let deferred_tools = exposure .deferred_tools .as_ref() .expect("MCP tools should be discoverable through tool_search"); - assert!(deferred_tools.contains_key("mcp__rmcp__tool")); - assert!(!deferred_tools.contains_key("mcp__codex_apps__calendar_create_event")); + let deferred_tool_names = tool_names(deferred_tools); + assert!(deferred_tool_names.contains(&ToolName::namespaced("mcp__rmcp__", "tool"))); + assert!(!deferred_tool_names.contains(&ToolName::namespaced( + "mcp__codex_apps__calendar", + "_create_event" + ))); } diff --git a/codex-rs/core/src/plugins/injection.rs b/codex-rs/core/src/plugins/injection.rs index 4ce50631fb0d..48e15247b5c4 100644 --- a/codex-rs/core/src/plugins/injection.rs +++ b/codex-rs/core/src/plugins/injection.rs @@ -1,5 +1,4 @@ use std::collections::BTreeSet; -use std::collections::HashMap; use codex_connectors::metadata::connector_display_label; use codex_protocol::models::ResponseItem; @@ -14,7 +13,7 @@ use codex_mcp::ToolInfo; pub(crate) fn build_plugin_injections( mentioned_plugins: &[PluginCapabilitySummary], - mcp_tools: &HashMap, + mcp_tools: &[ToolInfo], available_connectors: &[connectors::AppInfo], ) -> Vec { if mentioned_plugins.is_empty() { @@ -27,7 +26,7 @@ pub(crate) fn build_plugin_injections( .iter() .filter_map(|plugin| { let available_mcp_servers = mcp_tools - .values() + .iter() .filter(|tool| { tool.server_name != CODEX_APPS_MCP_SERVER_NAME && tool diff --git a/codex-rs/core/src/session/turn.rs b/codex-rs/core/src/session/turn.rs index 4579a4147af3..bdbd7a279324 100644 --- a/codex-rs/core/src/session/turn.rs +++ b/codex-rs/core/src/session/turn.rs @@ -195,10 +195,10 @@ pub(crate) async fn run_turn( { Ok(mcp_tools) => mcp_tools, Err(_) if turn_context.apps_enabled() => return None, - Err(_) => HashMap::new(), + Err(_) => Vec::new(), } } else { - HashMap::new() + Vec::new() }; let available_connectors = if turn_context.apps_enabled() { let connectors = codex_connectors::merge::merge_plugin_connectors_with_accessible( @@ -1247,7 +1247,7 @@ pub(crate) async fn built_tools( let exposed_tool_names = mcp_tools .iter() .chain(deferred_mcp_tools.iter()) - .flat_map(|tools| tools.keys().map(String::as_str)) + .flat_map(|tools| tools.iter().map(codex_mcp::ToolInfo::canonical_tool_name)) .collect::>(); collect_unavailable_called_tools(input, &exposed_tool_names) } else { diff --git a/codex-rs/core/src/tools/handlers/tool_search.rs b/codex-rs/core/src/tools/handlers/tool_search.rs index 59deb541169f..bc2e727dfbb0 100644 --- a/codex-rs/core/src/tools/handlers/tool_search.rs +++ b/codex-rs/core/src/tools/handlers/tool_search.rs @@ -204,19 +204,11 @@ mod tests { }), defer_loading: true, }]; - let handler = handler_from_tools( - Some(&std::collections::HashMap::from([ - ( - "mcp__calendar__create_event".to_string(), - tool_info("calendar", "create_event", "Create events"), - ), - ( - "mcp__calendar__list_events".to_string(), - tool_info("calendar", "list_events", "List events"), - ), - ])), - &dynamic_tools, - ); + let mcp_tools = vec![ + tool_info("calendar", "create_event", "Create events"), + tool_info("calendar", "list_events", "List events"), + ]; + let handler = handler_from_tools(Some(&mcp_tools), &dynamic_tools); let results = [ &handler.entries[0], &handler.entries[2], @@ -376,18 +368,11 @@ mod tests { assert!(count_results_for_server(&results, "other-server") <= TOOL_SEARCH_DEFAULT_LIMIT); } - fn numbered_tools( - server_name: &str, - description_prefix: &str, - count: usize, - ) -> std::collections::HashMap { + fn numbered_tools(server_name: &str, description_prefix: &str, count: usize) -> Vec { (0..count) .map(|index| { let tool_name = format!("tool_{index:03}"); - ( - format!("mcp__{server_name}__{tool_name}"), - tool_info(server_name, &tool_name, description_prefix), - ) + tool_info(server_name, &tool_name, description_prefix) }) .collect() } @@ -427,7 +412,7 @@ mod tests { } fn handler_from_tools( - mcp_tools: Option<&std::collections::HashMap>, + mcp_tools: Option<&[ToolInfo]>, dynamic_tools: &[DynamicToolSpec], ) -> ToolSearchHandler { ToolSearchHandler::new(build_tool_search_entries(mcp_tools, dynamic_tools)) diff --git a/codex-rs/core/src/tools/router.rs b/codex-rs/core/src/tools/router.rs index aeba3b0556ee..609fedf964bc 100644 --- a/codex-rs/core/src/tools/router.rs +++ b/codex-rs/core/src/tools/router.rs @@ -21,7 +21,6 @@ use codex_tools::ResponsesApiNamespaceTool; use codex_tools::ToolName; use codex_tools::ToolSpec; use codex_tools::ToolsConfig; -use std::collections::HashMap; use std::collections::HashSet; use std::sync::Arc; use tokio_util::sync::CancellationToken; @@ -44,8 +43,8 @@ pub struct ToolRouter { } pub(crate) struct ToolRouterParams<'a> { - pub(crate) mcp_tools: Option>, - pub(crate) deferred_mcp_tools: Option>, + pub(crate) mcp_tools: Option>, + pub(crate) deferred_mcp_tools: Option>, pub(crate) unavailable_called_tools: Vec, pub(crate) parallel_mcp_server_names: HashSet, pub(crate) discoverable_tools: Option>, diff --git a/codex-rs/core/src/tools/spec.rs b/codex-rs/core/src/tools/spec.rs index b13345f33387..aa504fe70f8a 100644 --- a/codex-rs/core/src/tools/spec.rs +++ b/codex-rs/core/src/tools/spec.rs @@ -42,17 +42,17 @@ struct McpToolPlanInputs<'a> { tool_namespaces: HashMap, } -fn map_mcp_tools_for_plan(mcp_tools: &HashMap) -> McpToolPlanInputs<'_> { +fn map_mcp_tools_for_plan(mcp_tools: &[ToolInfo]) -> McpToolPlanInputs<'_> { McpToolPlanInputs { mcp_tools: mcp_tools - .values() + .iter() .map(|tool| ToolRegistryPlanMcpTool { name: tool.canonical_tool_name(), tool: &tool.tool, }) .collect(), tool_namespaces: mcp_tools - .values() + .iter() .map(|tool| { ( tool.callable_namespace.clone(), @@ -68,8 +68,8 @@ fn map_mcp_tools_for_plan(mcp_tools: &HashMap) -> McpToolPlanI pub(crate) fn build_specs_with_discoverable_tools( config: &ToolsConfig, - mcp_tools: Option>, - deferred_mcp_tools: Option>, + mcp_tools: Option>, + deferred_mcp_tools: Option>, unavailable_called_tools: Vec, discoverable_tools: Option>, dynamic_tools: &[DynamicToolSpec], @@ -114,10 +114,10 @@ pub(crate) fn build_specs_with_discoverable_tools( use crate::tools::tool_search_entry::build_tool_search_entries_for_config; let mut builder = ToolRegistryBuilder::new(); - let mcp_tool_plan_inputs = mcp_tools.as_ref().map(map_mcp_tools_for_plan); + let mcp_tool_plan_inputs = mcp_tools.as_deref().map(map_mcp_tools_for_plan); let deferred_mcp_tool_sources = deferred_mcp_tools.as_ref().map(|tools| { tools - .values() + .iter() .map(|tool| ToolRegistryPlanDeferredTool { name: tool.canonical_tool_name(), server_name: tool.server_name.as_str(), @@ -279,7 +279,7 @@ pub(crate) fn build_specs_with_discoverable_tools( ToolHandlerKind::ToolSearch => { let entries = build_tool_search_entries_for_config( config, - deferred_mcp_tools.as_ref(), + deferred_mcp_tools.as_deref(), &deferred_dynamic_tools, ); builder.register_handler(Arc::new(ToolSearchHandler::new(entries))); @@ -305,10 +305,13 @@ pub(crate) fn build_specs_with_discoverable_tools( } } if let Some(deferred_mcp_tools) = deferred_mcp_tools.as_ref() { - for (_, tool) in deferred_mcp_tools.iter().filter(|(name, _)| { - !mcp_tools - .as_ref() - .is_some_and(|tools| tools.contains_key(*name)) + for tool in deferred_mcp_tools.iter().filter(|tool| { + let tool_name = tool.canonical_tool_name(); + !mcp_tools.as_ref().is_some_and(|direct_tools| { + direct_tools + .iter() + .any(|direct_tool| direct_tool.canonical_tool_name() == tool_name) + }) }) { builder.register_handler(Arc::new(McpHandler::new(tool.canonical_tool_name()))); } diff --git a/codex-rs/core/src/tools/spec_tests.rs b/codex-rs/core/src/tools/spec_tests.rs index 6bd796508dec..24e03fbf8a9d 100644 --- a/codex-rs/core/src/tools/spec_tests.rs +++ b/codex-rs/core/src/tools/spec_tests.rs @@ -266,8 +266,8 @@ async fn model_info_from_models_json(slug: &str) -> ModelInfo { /// Builds the tool registry builder while collecting tool specs for later serialization. fn build_specs( config: &ToolsConfig, - mcp_tools: Option>, - deferred_mcp_tools: Option>, + mcp_tools: Option>, + deferred_mcp_tools: Option>, dynamic_tools: &[DynamicToolSpec], ) -> ToolRegistryBuilder { build_specs_with_unavailable_tools( @@ -281,8 +281,8 @@ fn build_specs( fn build_specs_with_unavailable_tools( config: &ToolsConfig, - mcp_tools: Option>, - deferred_mcp_tools: Option>, + mcp_tools: Option>, + deferred_mcp_tools: Option>, unavailable_called_tools: Vec, dynamic_tools: &[DynamicToolSpec], ) -> ToolRegistryBuilder { @@ -630,7 +630,7 @@ async fn test_build_specs_default_shell_present() { }); let (tools, _) = build_specs( &tools_config, - Some(HashMap::new()), + Some(Vec::new()), /*deferred_mcp_tools*/ None, &[], ) @@ -856,7 +856,7 @@ async fn search_tool_description_handles_no_enabled_mcp_tools() { let (tools, _) = build_specs( &tools_config, /*mcp_tools*/ None, - Some(HashMap::new()), + Some(Vec::new()), &[], ) .build(); @@ -890,23 +890,20 @@ async fn search_tool_description_falls_back_to_connector_name_without_descriptio let (tools, _) = build_specs( &tools_config, /*mcp_tools*/ None, - Some(HashMap::from([( - "mcp__codex_apps__calendar_create_event".to_string(), - ToolInfo { - server_name: CODEX_APPS_MCP_SERVER_NAME.to_string(), - callable_name: "_create_event".to_string(), - callable_namespace: "mcp__codex_apps__calendar".to_string(), - namespace_description: None, - tool: mcp_tool( - "calendar_create_event", - "Create calendar event", - serde_json::json!({"type": "object"}), - ), - connector_id: Some("calendar".to_string()), - connector_name: Some("Calendar".to_string()), - plugin_display_names: Vec::new(), - }, - )])), + Some(vec![ToolInfo { + server_name: CODEX_APPS_MCP_SERVER_NAME.to_string(), + callable_name: "_create_event".to_string(), + callable_namespace: "mcp__codex_apps__calendar".to_string(), + namespace_description: None, + tool: mcp_tool( + "calendar_create_event", + "Create calendar event", + serde_json::json!({"type": "object"}), + ), + connector_id: Some("calendar".to_string()), + connector_name: Some("Calendar".to_string()), + plugin_display_names: Vec::new(), + }]), &[], ) .build(); @@ -940,55 +937,46 @@ async fn search_tool_registers_namespaced_mcp_tool_aliases() { let (_, registry) = build_specs( &tools_config, /*mcp_tools*/ None, - Some(HashMap::from([ - ( - "mcp__codex_apps__calendar_create_event".to_string(), - ToolInfo { - server_name: CODEX_APPS_MCP_SERVER_NAME.to_string(), - callable_name: "_create_event".to_string(), - callable_namespace: "mcp__codex_apps__calendar".to_string(), - namespace_description: None, - tool: mcp_tool( - "calendar-create-event", - "Create calendar event", - serde_json::json!({"type": "object"}), - ), - connector_id: Some("calendar".to_string()), - connector_name: Some("Calendar".to_string()), - plugin_display_names: Vec::new(), - }, - ), - ( - "mcp__codex_apps__calendar_list_events".to_string(), - ToolInfo { - server_name: CODEX_APPS_MCP_SERVER_NAME.to_string(), - callable_name: "_list_events".to_string(), - callable_namespace: "mcp__codex_apps__calendar".to_string(), - namespace_description: None, - tool: mcp_tool( - "calendar-list-events", - "List calendar events", - serde_json::json!({"type": "object"}), - ), - connector_id: Some("calendar".to_string()), - connector_name: Some("Calendar".to_string()), - plugin_display_names: Vec::new(), - }, - ), - ( - "mcp__rmcp__echo".to_string(), - ToolInfo { - server_name: "rmcp".to_string(), - callable_name: "echo".to_string(), - callable_namespace: "mcp__rmcp__".to_string(), - namespace_description: None, - tool: mcp_tool("echo", "Echo", serde_json::json!({"type": "object"})), - connector_id: None, - connector_name: None, - plugin_display_names: Vec::new(), - }, - ), - ])), + Some(vec![ + ToolInfo { + server_name: CODEX_APPS_MCP_SERVER_NAME.to_string(), + callable_name: "_create_event".to_string(), + callable_namespace: "mcp__codex_apps__calendar".to_string(), + namespace_description: None, + tool: mcp_tool( + "calendar-create-event", + "Create calendar event", + serde_json::json!({"type": "object"}), + ), + connector_id: Some("calendar".to_string()), + connector_name: Some("Calendar".to_string()), + plugin_display_names: Vec::new(), + }, + ToolInfo { + server_name: CODEX_APPS_MCP_SERVER_NAME.to_string(), + callable_name: "_list_events".to_string(), + callable_namespace: "mcp__codex_apps__calendar".to_string(), + namespace_description: None, + tool: mcp_tool( + "calendar-list-events", + "List calendar events", + serde_json::json!({"type": "object"}), + ), + connector_id: Some("calendar".to_string()), + connector_name: Some("Calendar".to_string()), + plugin_display_names: Vec::new(), + }, + ToolInfo { + server_name: "rmcp".to_string(), + callable_name: "echo".to_string(), + callable_namespace: "mcp__rmcp__".to_string(), + namespace_description: None, + tool: mcp_tool("echo", "Echo", serde_json::json!({"type": "object"})), + connector_id: None, + connector_name: None, + plugin_display_names: Vec::new(), + }, + ]), &[], ) .build(); @@ -1018,14 +1006,11 @@ async fn tool_search_entries_skip_namespace_outputs_when_namespace_tools_are_dis windows_sandbox_level: WindowsSandboxLevel::Disabled, }); tools_config.namespace_tools = false; - let mcp_tools = HashMap::from([( - "mcp__test_server__echo".to_string(), - mcp_tool_info(mcp_tool( - "echo", - "Echo", - serde_json::json!({"type": "object"}), - )), - )]); + let mcp_tools = vec![mcp_tool_info(mcp_tool( + "echo", + "Echo", + serde_json::json!({"type": "object"}), + ))]; let dynamic_tools = vec![ DynamicToolSpec { namespace: Some("codex_app".to_string()), @@ -1077,14 +1062,11 @@ async fn direct_mcp_tools_register_namespaced_handlers() { let (_, registry) = build_specs( &tools_config, - Some(HashMap::from([( - "mcp__test_server__echo".to_string(), - mcp_tool_info(mcp_tool( - "echo", - "Echo", - serde_json::json!({"type": "object"}), - )), - )])), + Some(vec![mcp_tool_info(mcp_tool( + "echo", + "Echo", + serde_json::json!({"type": "object"}), + ))]), /*deferred_mcp_tools*/ None, &[], ) @@ -1163,22 +1145,19 @@ async fn test_mcp_tool_property_missing_type_defaults_to_string() { let (tools, _) = build_specs( &tools_config, - Some(HashMap::from([( - "dash/search".to_string(), - mcp_tool_info_with_display_name( - "dash/search", - mcp_tool( - "search", - "Search docs", - serde_json::json!({ - "type": "object", - "properties": { - "query": {"description": "search query"} - } - }), - ), + Some(vec![mcp_tool_info_with_display_name( + "dash/search", + mcp_tool( + "search", + "Search docs", + serde_json::json!({ + "type": "object", + "properties": { + "query": {"description": "search query"} + } + }), ), - )])), + )]), /*deferred_mcp_tools*/ None, &[], ) @@ -1226,20 +1205,17 @@ async fn test_mcp_tool_preserves_integer_schema() { let (tools, _) = build_specs( &tools_config, - Some(HashMap::from([( - "dash/paginate".to_string(), - mcp_tool_info_with_display_name( - "dash/paginate", - mcp_tool( - "paginate", - "Pagination", - serde_json::json!({ - "type": "object", - "properties": {"page": {"type": "integer"}} - }), - ), + Some(vec![mcp_tool_info_with_display_name( + "dash/paginate", + mcp_tool( + "paginate", + "Pagination", + serde_json::json!({ + "type": "object", + "properties": {"page": {"type": "integer"}} + }), ), - )])), + )]), /*deferred_mcp_tools*/ None, &[], ) @@ -1288,20 +1264,17 @@ async fn test_mcp_tool_array_without_items_gets_default_string_items() { let (tools, _) = build_specs( &tools_config, - Some(HashMap::from([( - "dash/tags".to_string(), - mcp_tool_info_with_display_name( - "dash/tags", - mcp_tool( - "tags", - "Tags", - serde_json::json!({ - "type": "object", - "properties": {"tags": {"type": "array"}} - }), - ), + Some(vec![mcp_tool_info_with_display_name( + "dash/tags", + mcp_tool( + "tags", + "Tags", + serde_json::json!({ + "type": "object", + "properties": {"tags": {"type": "array"}} + }), ), - )])), + )]), /*deferred_mcp_tools*/ None, &[], ) @@ -1352,22 +1325,19 @@ async fn test_mcp_tool_anyof_defaults_to_string() { let (tools, _) = build_specs( &tools_config, - Some(HashMap::from([( - "dash/value".to_string(), - mcp_tool_info_with_display_name( - "dash/value", - mcp_tool( - "value", - "AnyOf Value", - serde_json::json!({ - "type": "object", - "properties": { - "value": {"anyOf": [{"type": "string"}, {"type": "number"}]} - } - }), - ), + Some(vec![mcp_tool_info_with_display_name( + "dash/value", + mcp_tool( + "value", + "AnyOf Value", + serde_json::json!({ + "type": "object", + "properties": { + "value": {"anyOf": [{"type": "string"}, {"type": "number"}]} + } + }), ), - )])), + )]), /*deferred_mcp_tools*/ None, &[], ) @@ -1420,39 +1390,36 @@ async fn test_get_openai_tools_mcp_tools_with_additional_properties_schema() { }); let (tools, _) = build_specs( &tools_config, - Some(HashMap::from([( - "test_server/do_something_cool".to_string(), - mcp_tool_info_with_display_name( - "test_server/do_something_cool", - mcp_tool( - "do_something_cool", - "Do something cool", - serde_json::json!({ + Some(vec![mcp_tool_info_with_display_name( + "test_server/do_something_cool", + mcp_tool( + "do_something_cool", + "Do something cool", + serde_json::json!({ + "type": "object", + "properties": { + "string_argument": {"type": "string"}, + "number_argument": {"type": "number"}, + "object_argument": { "type": "object", "properties": { - "string_argument": {"type": "string"}, - "number_argument": {"type": "number"}, - "object_argument": { + "string_property": {"type": "string"}, + "number_property": {"type": "number"} + }, + "required": ["string_property", "number_property"], + "additionalProperties": { "type": "object", "properties": { - "string_property": {"type": "string"}, - "number_property": {"type": "number"} + "addtl_prop": {"type": "string"} }, - "required": ["string_property", "number_property"], - "additionalProperties": { - "type": "object", - "properties": { - "addtl_prop": {"type": "string"} - }, - "required": ["addtl_prop"], - "additionalProperties": false - } + "required": ["addtl_prop"], + "additionalProperties": false } } - }), - ), + } + }), ), - )])), + )]), /*deferred_mcp_tools*/ None, &[], ) diff --git a/codex-rs/core/src/tools/tool_search_entry.rs b/codex-rs/core/src/tools/tool_search_entry.rs index 06b7efcd0603..a0e9a726b954 100644 --- a/codex-rs/core/src/tools/tool_search_entry.rs +++ b/codex-rs/core/src/tools/tool_search_entry.rs @@ -5,7 +5,6 @@ use codex_tools::ToolSearchResultSource; use codex_tools::ToolsConfig; use codex_tools::dynamic_tool_to_loadable_tool_spec; use codex_tools::tool_search_result_source_to_loadable_tool_spec; -use std::collections::HashMap; #[derive(Clone)] pub(crate) struct ToolSearchEntry { @@ -15,13 +14,13 @@ pub(crate) struct ToolSearchEntry { } pub(crate) fn build_tool_search_entries( - mcp_tools: Option<&HashMap>, + mcp_tools: Option<&[ToolInfo]>, dynamic_tools: &[DynamicToolSpec], ) -> Vec { let mut entries = Vec::new(); let mut mcp_tools = mcp_tools - .map(|tools| tools.values().collect::>()) + .map(|tools| tools.iter().collect::>()) .unwrap_or_default(); mcp_tools.sort_by_key(|info| info.canonical_tool_name().display()); for info in mcp_tools { @@ -55,7 +54,7 @@ pub(crate) fn build_tool_search_entries( pub(crate) fn build_tool_search_entries_for_config( config: &ToolsConfig, - mcp_tools: Option<&HashMap>, + mcp_tools: Option<&[ToolInfo]>, dynamic_tools: &[DynamicToolSpec], ) -> Vec { let mcp_tools = if config.namespace_tools { diff --git a/codex-rs/core/src/unavailable_tool.rs b/codex-rs/core/src/unavailable_tool.rs index 39ba21f9b86a..e25e47a30e11 100644 --- a/codex-rs/core/src/unavailable_tool.rs +++ b/codex-rs/core/src/unavailable_tool.rs @@ -6,7 +6,7 @@ use codex_tools::ToolName; pub(crate) fn collect_unavailable_called_tools( input: &[ResponseItem], - exposed_tool_names: &HashSet<&str>, + exposed_tool_names: &HashSet, ) -> Vec { let mut unavailable_tools = BTreeMap::new(); @@ -25,11 +25,11 @@ pub(crate) fn collect_unavailable_called_tools( Some(namespace) => ToolName::namespaced(namespace.clone(), name.clone()), None => ToolName::plain(name.clone()), }; - let display_name = tool_name.display(); - if exposed_tool_names.contains(display_name.as_str()) { + if exposed_tool_names.contains(&tool_name) { continue; } + let display_name = tool_name.display(); unavailable_tools .entry(display_name) .or_insert_with(|| tool_name); @@ -78,7 +78,10 @@ mod tests { #[test] fn collect_unavailable_called_tools_skips_currently_available_tools() { - let exposed_tool_names = HashSet::from(["mcp__server__lookup", "mcp__server__search"]); + let exposed_tool_names = HashSet::from([ + ToolName::plain("mcp__server__lookup"), + ToolName::plain("mcp__server__search"), + ]); let input = vec![ function_call("mcp__server__lookup", /*namespace*/ None), function_call("mcp__server__search", /*namespace*/ None), From 496d9d0d8d6ed763e286b11cedd64db51a34bf8f Mon Sep 17 00:00:00 2001 From: pakrym-oai Date: Wed, 6 May 2026 18:39:37 -0700 Subject: [PATCH 2/4] Compare MCP normalized tool names structurally --- .../codex-mcp/src/connection_manager_tests.rs | 66 ++++++++++--------- 1 file changed, 36 insertions(+), 30 deletions(-) diff --git a/codex-rs/codex-mcp/src/connection_manager_tests.rs b/codex-rs/codex-mcp/src/connection_manager_tests.rs index 5c430bd8e56d..9ff5fdcfe648 100644 --- a/codex-rs/codex-mcp/src/connection_manager_tests.rs +++ b/codex-rs/codex-mcp/src/connection_manager_tests.rs @@ -85,13 +85,11 @@ fn create_codex_apps_tools_cache_context( } } -fn sorted_model_tool_names(tools: &[ToolInfo]) -> Vec { - let mut names = tools +fn model_tool_names(tools: &[ToolInfo]) -> HashSet { + tools .iter() - .map(|tool| tool.canonical_tool_name().display()) - .collect::>(); - names.sort(); - names + .map(ToolInfo::canonical_tool_name) + .collect::>() } #[test] @@ -290,11 +288,11 @@ fn test_normalize_tools_short_non_duplicated_names() { let model_tools = normalize_tools_for_model(tools); assert_eq!( - sorted_model_tool_names(&model_tools), - vec![ - "mcp__server1__tool1".to_string(), - "mcp__server1__tool2".to_string() - ] + model_tool_names(&model_tools), + HashSet::from([ + ToolName::namespaced("mcp__server1__", "tool1"), + ToolName::namespaced("mcp__server1__", "tool2") + ]) ); } @@ -309,8 +307,8 @@ fn test_normalize_tools_duplicated_names_skipped() { // Only the first tool should remain, the second is skipped assert_eq!( - sorted_model_tool_names(&model_tools), - vec!["mcp__server1__duplicate_tool".to_string()] + model_tool_names(&model_tools), + HashSet::from([ToolName::namespaced("mcp__server1__", "duplicate_tool")]) ); } @@ -333,18 +331,19 @@ fn test_normalize_tools_long_names_same_server() { assert_eq!(model_tools.len(), 2); - let names = sorted_model_tool_names(&model_tools); + let names = model_tool_names(&model_tools); - assert!(names.iter().all(|name| name.len() == 64)); + assert!(names.iter().all(|name| name.display().len() == 64)); assert!( names .iter() - .all(|name| name.starts_with("mcp__my_server__")) + .all(|name| name.namespace.as_deref() == Some("mcp__my_server__")) ); assert!( - names - .iter() - .all(|name| name.chars().all(|c| c.is_ascii_alphanumeric() || c == '_')), + names.iter().all(|name| name + .display() + .chars() + .all(|c| c.is_ascii_alphanumeric() || c == '_')), "model-visible names must be code-mode compatible: {names:?}" ); } @@ -357,11 +356,14 @@ fn test_normalize_tools_sanitizes_invalid_characters() { assert_eq!(model_tools.len(), 1); let tool = model_tools.into_iter().next().expect("one tool"); - let model_name = tool.canonical_tool_name().display(); - assert_eq!(model_name, "mcp__server_one__tool_two_three"); + let model_name = tool.canonical_tool_name(); + assert_eq!( + model_name, + ToolName::namespaced("mcp__server_one__", "tool_two_three") + ); assert_eq!( format!("{}{}", tool.callable_namespace, tool.callable_name), - model_name + model_name.display() ); // The callable parts are sanitized for model-visible tool calls, but the raw @@ -373,6 +375,7 @@ fn test_normalize_tools_sanitizes_invalid_characters() { assert!( model_name + .display() .chars() .all(|c| c.is_ascii_alphanumeric() || c == '_'), "model-visible name must be code-mode compatible: {model_name:?}" @@ -388,8 +391,8 @@ fn test_normalize_tools_keeps_hyphenated_mcp_tools_callable() { assert_eq!(model_tools.len(), 1); let tool = model_tools.into_iter().next().expect("one tool"); assert_eq!( - tool.canonical_tool_name().display(), - "mcp__music_studio__get_strudel_guide" + tool.canonical_tool_name(), + ToolName::namespaced("mcp__music_studio__", "get_strudel_guide") ); assert_eq!(tool.callable_namespace, "mcp__music_studio__"); assert_eq!(tool.callable_name, "get_strudel_guide"); @@ -419,11 +422,12 @@ fn test_normalize_tools_disambiguates_sanitized_namespace_collisions() { .map(|tool| tool.server_name.as_str()) .collect::>(); assert_eq!(raw_servers, HashSet::from(["basic-server", "basic_server"])); - let model_names = sorted_model_tool_names(&model_tools); + let model_names = model_tool_names(&model_tools); assert!( - model_names - .iter() - .all(|name| name.chars().all(|c| c.is_ascii_alphanumeric() || c == '_')), + model_names.iter().all(|name| name + .display() + .chars() + .all(|c| c.is_ascii_alphanumeric() || c == '_')), "model-visible names must be code-mode compatible: {model_names:?}" ); } @@ -698,7 +702,8 @@ async fn list_all_tools_uses_startup_snapshot_while_client_is_pending() { let tool = tools .iter() .find(|tool| { - tool.canonical_tool_name().display() == "mcp__codex_apps__calendar_create_event" + tool.canonical_tool_name() + == ToolName::namespaced("mcp__codex_apps__", "calendar_create_event") }) .expect("tool from startup cache"); assert_eq!(tool.server_name, CODEX_APPS_MCP_SERVER_NAME); @@ -827,7 +832,8 @@ async fn list_all_tools_uses_startup_snapshot_when_client_startup_fails() { let tool = tools .iter() .find(|tool| { - tool.canonical_tool_name().display() == "mcp__codex_apps__calendar_create_event" + tool.canonical_tool_name() + == ToolName::namespaced("mcp__codex_apps__", "calendar_create_event") }) .expect("tool from startup cache"); assert_eq!(tool.server_name, CODEX_APPS_MCP_SERVER_NAME); From 0c1c82874c152b12fad0af3aa8309be6ac091ac3 Mon Sep 17 00:00:00 2001 From: pakrym-oai Date: Wed, 6 May 2026 18:47:53 -0700 Subject: [PATCH 3/4] Simplify MCP exposure test fixtures --- codex-rs/core/src/mcp_tool_exposure_test.rs | 49 +++++++++------------ 1 file changed, 20 insertions(+), 29 deletions(-) diff --git a/codex-rs/core/src/mcp_tool_exposure_test.rs b/codex-rs/core/src/mcp_tool_exposure_test.rs index 0116a62e9612..c3f467800a82 100644 --- a/codex-rs/core/src/mcp_tool_exposure_test.rs +++ b/codex-rs/core/src/mcp_tool_exposure_test.rs @@ -1,7 +1,6 @@ use std::collections::HashSet; use std::sync::Arc; -use codex_connectors::metadata::sanitize_name; use codex_features::Feature; use codex_features::Features; use codex_mcp::CODEX_APPS_MCP_SERVER_NAME; @@ -43,37 +42,15 @@ fn make_connector(id: &str, name: &str) -> AppInfo { fn make_mcp_tool( server_name: &str, tool_name: &str, + callable_namespace: &str, + callable_name: &str, connector_id: Option<&str>, connector_name: Option<&str>, ) -> ToolInfo { - let tool_namespace = if server_name == CODEX_APPS_MCP_SERVER_NAME { - connector_name - .map(sanitize_name) - .map(|connector_name| format!("mcp__{server_name}__{connector_name}")) - .unwrap_or_else(|| server_name.to_string()) - } else { - format!("mcp__{server_name}__") - }; - let callable_name = if server_name == CODEX_APPS_MCP_SERVER_NAME { - let tool_name = sanitize_name(tool_name); - if let Some(connector_name) = connector_name - .map(sanitize_name) - .filter(|name| !name.is_empty()) - && let Some(stripped) = tool_name.strip_prefix(&connector_name) - && !stripped.is_empty() - { - stripped.to_string() - } else { - tool_name - } - } else { - tool_name.to_string() - }; - ToolInfo { server_name: server_name.to_string(), - callable_name, - callable_namespace: tool_namespace, + callable_name: callable_name.to_string(), + callable_namespace: callable_namespace.to_string(), namespace_description: None, tool: Tool { name: tool_name.to_string().into(), @@ -97,7 +74,12 @@ fn numbered_mcp_tools(count: usize) -> Vec { .map(|index| { let tool_name = format!("tool_{index}"); make_mcp_tool( - "rmcp", &tool_name, /*connector_id*/ None, /*connector_name*/ None, + "rmcp", + &tool_name, + "mcp__rmcp__", + &tool_name, + /*connector_id*/ None, + /*connector_name*/ None, ) }) .collect() @@ -178,6 +160,8 @@ async fn directly_exposes_explicit_apps_without_deferred_overlap() { mcp_tools.push(make_mcp_tool( CODEX_APPS_MCP_SERVER_NAME, "calendar_create_event", + "mcp__codex_apps__calendar", + "_create_event", Some("calendar"), Some("Calendar"), )); @@ -229,11 +213,18 @@ async fn always_defer_feature_preserves_explicit_apps() { let tools_config = tools_config_for_mcp_tool_exposure(/*search_tool*/ true).await; let mcp_tools = vec![ make_mcp_tool( - "rmcp", "tool", /*connector_id*/ None, /*connector_name*/ None, + "rmcp", + "tool", + "mcp__rmcp__", + "tool", + /*connector_id*/ None, + /*connector_name*/ None, ), make_mcp_tool( CODEX_APPS_MCP_SERVER_NAME, "calendar_create_event", + "mcp__codex_apps__calendar", + "_create_event", Some("calendar"), Some("Calendar"), ), From d33d9a912a27de96a61f556698e97282c643901e Mon Sep 17 00:00:00 2001 From: pakrym-oai Date: Wed, 6 May 2026 19:57:58 -0700 Subject: [PATCH 4/4] Preserve MCP display name matching --- codex-rs/core/src/unavailable_tool.rs | 21 +++++++++++++++++++-- 1 file changed, 19 insertions(+), 2 deletions(-) diff --git a/codex-rs/core/src/unavailable_tool.rs b/codex-rs/core/src/unavailable_tool.rs index e25e47a30e11..aabf1f605834 100644 --- a/codex-rs/core/src/unavailable_tool.rs +++ b/codex-rs/core/src/unavailable_tool.rs @@ -9,6 +9,10 @@ pub(crate) fn collect_unavailable_called_tools( exposed_tool_names: &HashSet, ) -> Vec { let mut unavailable_tools = BTreeMap::new(); + let exposed_display_names = exposed_tool_names + .iter() + .map(ToolName::display) + .collect::>(); for item in input { let ResponseItem::FunctionCall { @@ -25,11 +29,11 @@ pub(crate) fn collect_unavailable_called_tools( Some(namespace) => ToolName::namespaced(namespace.clone(), name.clone()), None => ToolName::plain(name.clone()), }; - if exposed_tool_names.contains(&tool_name) { + let display_name = tool_name.display(); + if exposed_display_names.contains(&display_name) { continue; } - let display_name = tool_name.display(); unavailable_tools .entry(display_name) .or_insert_with(|| tool_name); @@ -92,4 +96,17 @@ mod tests { assert_eq!(tools, vec![ToolName::plain("mcp__server__missing")]); } + + #[test] + fn collect_unavailable_called_tools_matches_exposed_display_names() { + let exposed_tool_names = HashSet::from([ToolName::namespaced("mcp__server__", "lookup")]); + let input = vec![function_call( + "mcp__server__lookup", + /*namespace*/ None, + )]; + + let tools = collect_unavailable_called_tools(&input, &exposed_tool_names); + + assert_eq!(tools, Vec::new()); + } }