From c209bfab8a69b1e5e5af433740e11f3c8d4611a1 Mon Sep 17 00:00:00 2001 From: Matthew Zeng Date: Mon, 6 Apr 2026 14:09:27 -0700 Subject: [PATCH 1/9] update --- codex-rs/core/src/codex.rs | 88 ++++--- codex-rs/core/src/codex_tests.rs | 232 ++++++++---------- codex-rs/core/src/tools/code_mode/mod.rs | 2 +- .../core/src/tools/handlers/tool_search.rs | 13 +- codex-rs/core/src/tools/js_repl/mod.rs | 2 +- codex-rs/core/src/tools/router.rs | 6 +- codex-rs/core/src/tools/router_tests.rs | 8 +- codex-rs/core/src/tools/spec.rs | 21 +- codex-rs/core/src/tools/spec_tests.rs | 47 ++-- codex-rs/core/tests/suite/search_tool.rs | 129 +++++++++- codex-rs/tools/src/lib.rs | 9 +- codex-rs/tools/src/tool_discovery.rs | 131 +++++++--- codex-rs/tools/src/tool_discovery_tests.rs | 48 +++- codex-rs/tools/src/tool_registry_plan.rs | 34 +-- .../tools/src/tool_registry_plan_tests.rs | 139 ++++++----- .../tools/src/tool_registry_plan_types.rs | 6 +- 16 files changed, 597 insertions(+), 318 deletions(-) diff --git a/codex-rs/core/src/codex.rs b/codex-rs/core/src/codex.rs index 758b592047a0..4153618c21f4 100644 --- a/codex-rs/core/src/codex.rs +++ b/codex-rs/core/src/codex.rs @@ -436,7 +436,7 @@ pub(crate) const INITIAL_SUBMIT_ID: &str = ""; pub(crate) const SUBMISSION_CHANNEL_CAPACITY: usize = 512; const CYBER_VERIFY_URL: &str = "https://chatgpt.com/cyber"; const CYBER_SAFETY_URL: &str = "https://developers.openai.com/codex/concepts/cyber-safety"; -const DIRECT_APP_TOOL_EXPOSURE_THRESHOLD: usize = 100; +const DIRECT_MCP_TOOL_EXPOSURE_THRESHOLD: usize = 100; impl Codex { /// Spawn a new [`Codex`] and initialize the session. @@ -6510,6 +6510,42 @@ fn connector_inserted_in_messages( connector_count == 1 && skill_count == 0 && mention_names_lower.contains(&mention_slug) } +struct McpToolExposure { + direct_tools: HashMap, + deferred_tools: Option>, +} + +fn build_mcp_tool_exposure( + all_mcp_tools: &HashMap, + connectors: Option<&[connectors::AppInfo]>, + explicitly_enabled_connectors: &[connectors::AppInfo], + config: &Config, + tools_config: &ToolsConfig, +) -> McpToolExposure { + let mut deferred_tools = filter_non_codex_apps_mcp_tools_only(all_mcp_tools); + if let Some(connectors) = connectors { + deferred_tools.extend(filter_codex_apps_mcp_tools( + all_mcp_tools, + connectors, + config, + )); + } + + if !tools_config.search_tool || deferred_tools.len() < DIRECT_MCP_TOOL_EXPOSURE_THRESHOLD { + return McpToolExposure { + direct_tools: deferred_tools, + deferred_tools: None, + }; + } + + let direct_tools = + filter_codex_apps_mcp_tools(all_mcp_tools, explicitly_enabled_connectors, config); + McpToolExposure { + direct_tools, + deferred_tools: Some(deferred_tools), + } +} + fn filter_codex_apps_mcp_tools( mcp_tools: &HashMap, connectors: &[connectors::AppInfo], @@ -6725,7 +6761,7 @@ pub(crate) async fn built_tools( ) -> CodexResult> { let mcp_connection_manager = sess.services.mcp_connection_manager.read().await; let has_mcp_servers = mcp_connection_manager.has_servers(); - let mut mcp_tools = mcp_connection_manager + let all_mcp_tools = mcp_connection_manager .list_all_tools() .or_cancel(cancellation_token) .await?; @@ -6740,7 +6776,7 @@ pub(crate) async fn built_tools( let apps_enabled = turn_context.apps_enabled(); let accessible_connectors = - apps_enabled.then(|| connectors::accessible_connectors_from_mcp_tools(&mcp_tools)); + apps_enabled.then(|| connectors::accessible_connectors_from_mcp_tools(&all_mcp_tools)); let accessible_connectors_with_enabled_state = accessible_connectors.as_ref().map(|connectors| { connectors::with_app_enabled_state(connectors.clone(), &turn_context.config) @@ -6786,57 +6822,39 @@ pub(crate) async fn built_tools( None }; - let app_tools = connectors.as_ref().map(|connectors| { - filter_codex_apps_mcp_tools(&mcp_tools, connectors, &turn_context.config) - }); - - if let Some(connectors) = connectors.as_ref() { + let explicitly_enabled = if let Some(connectors) = connectors.as_ref() { let skill_name_counts_lower = skills_outcome.map_or_else(HashMap::new, |outcome| { build_skill_name_counts(&outcome.skills, &outcome.disabled_paths).1 }); - let explicitly_enabled = filter_connectors_for_input( + filter_connectors_for_input( connectors, input, &effective_explicitly_enabled_connectors, &skill_name_counts_lower, - ); - - let mut selected_mcp_tools = filter_non_codex_apps_mcp_tools_only(&mcp_tools); - selected_mcp_tools.extend(filter_codex_apps_mcp_tools( - &mcp_tools, - explicitly_enabled.as_ref(), - &turn_context.config, - )); - - mcp_tools = selected_mcp_tools; - } - - // Expose app tools directly when tool_search is disabled, or when tool_search - // is enabled but the accessible app tool set stays below the direct-exposure threshold. - let expose_app_tools_directly = !turn_context.tools_config.search_tool - || app_tools - .as_ref() - .is_some_and(|tools| tools.len() < DIRECT_APP_TOOL_EXPOSURE_THRESHOLD); - if expose_app_tools_directly && let Some(app_tools) = app_tools.as_ref() { - mcp_tools.extend(app_tools.clone()); - } - let app_tools = if expose_app_tools_directly { - None + ) } else { - app_tools + Vec::new() }; + let mcp_tool_exposure = build_mcp_tool_exposure( + &all_mcp_tools, + connectors.as_deref(), + explicitly_enabled.as_slice(), + &turn_context.config, + &turn_context.tools_config, + ); Ok(Arc::new(ToolRouter::from_config( &turn_context.tools_config, ToolRouterParams { mcp_tools: has_mcp_servers.then(|| { - mcp_tools + mcp_tool_exposure + .direct_tools .into_iter() .map(|(name, tool)| (name, tool.tool)) .collect() }), - app_tools, + deferred_mcp_tools: mcp_tool_exposure.deferred_tools, discoverable_tools, dynamic_tools: turn_context.dynamic_tools.as_slice(), }, diff --git a/codex-rs/core/src/codex_tests.rs b/codex-rs/core/src/codex_tests.rs index 274607095c1d..bd0f6f75657d 100644 --- a/codex-rs/core/src/codex_tests.rs +++ b/codex-rs/core/src/codex_tests.rs @@ -298,7 +298,7 @@ fn test_tool_runtime(session: Arc, turn_context: Arc) -> T &turn_context.tools_config, crate::tools::router::ToolRouterParams { mcp_tools: None, - app_tools: None, + deferred_mcp_tools: None, discoverable_tools: None, dynamic_tools: turn_context.dynamic_tools.as_slice(), }, @@ -424,6 +424,41 @@ fn make_mcp_tool( } } +fn numbered_mcp_tools(count: usize) -> HashMap { + (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, + ), + ) + }) + .collect() +} + +fn tools_config_for_mcp_tool_exposure(search_tool: bool) -> ToolsConfig { + let config = test_config(); + let model_info = ModelsManager::construct_model_info_offline_for_tests( + "gpt-5-codex", + &config.to_models_manager_config(), + ); + let features = Features::with_defaults(); + let available_models = Vec::new(); + let mut tools_config = ToolsConfig::new(&ToolsConfigParams { + model_info: &model_info, + available_models: &available_models, + features: &features, + web_search_mode: Some(WebSearchMode::Cached), + session_source: SessionSource::Cli, + sandbox_policy: &SandboxPolicy::DangerFullAccess, + windows_sandbox_level: WindowsSandboxLevel::Disabled, + }); + tools_config.search_tool = search_tool; + tools_config +} + #[test] fn validated_network_policy_amendment_host_allows_normalized_match() { let amendment = NetworkPolicyAmendment { @@ -738,156 +773,93 @@ fn collect_explicit_app_ids_from_skill_items_skips_plain_mentions_with_skill_con } #[test] -fn non_app_mcp_tools_remain_visible_without_search_selection() { - let mcp_tools = HashMap::from([ - ( - "mcp__codex_apps__calendar_create_event".to_string(), - make_mcp_tool( - CODEX_APPS_MCP_SERVER_NAME, - "calendar_create_event", - Some("calendar"), - Some("Calendar"), - ), - ), - ( - "mcp__rmcp__echo".to_string(), - make_mcp_tool( - "rmcp", "echo", /*connector_id*/ None, /*connector_name*/ None, - ), - ), - ]); - - let mut selected_mcp_tools = mcp_tools - .iter() - .filter(|(_, tool)| tool.server_name != CODEX_APPS_MCP_SERVER_NAME) - .map(|(name, tool)| (name.clone(), tool.clone())) - .collect::>(); - - let connectors = connectors::accessible_connectors_from_mcp_tools(&mcp_tools); - let explicitly_enabled_connectors = HashSet::new(); - let connectors = filter_connectors_for_input( - &connectors, - &[user_message("run echo")], - &explicitly_enabled_connectors, - &HashMap::new(), - ); +fn mcp_tool_exposure_directly_exposes_small_effective_tool_sets() { let config = test_config(); - selected_mcp_tools.extend(filter_codex_apps_mcp_tools( + let tools_config = tools_config_for_mcp_tool_exposure(/*search_tool*/ true); + let mcp_tools = numbered_mcp_tools(DIRECT_MCP_TOOL_EXPOSURE_THRESHOLD - 1); + + let exposure = build_mcp_tool_exposure( &mcp_tools, - &connectors, + /*connectors*/ None, + &[], &config, - )); + &tools_config, + ); - let mut tool_names: Vec = selected_mcp_tools.into_keys().collect(); - tool_names.sort(); - assert_eq!(tool_names, vec!["mcp__rmcp__echo".to_string()]); + 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!(exposure.deferred_tools.is_none()); } #[test] -fn search_tool_selection_keeps_codex_apps_tools_without_mentions() { - let selected_tool_names = [ - "mcp__codex_apps__calendar_create_event".to_string(), - "mcp__rmcp__echo".to_string(), - ]; - let mcp_tools = HashMap::from([ - ( - "mcp__codex_apps__calendar_create_event".to_string(), - make_mcp_tool( - CODEX_APPS_MCP_SERVER_NAME, - "calendar_create_event", - Some("calendar"), - Some("Calendar"), - ), - ), - ( - "mcp__rmcp__echo".to_string(), - make_mcp_tool( - "rmcp", "echo", /*connector_id*/ None, /*connector_name*/ None, - ), - ), - ]); - - let mut selected_mcp_tools = mcp_tools - .iter() - .filter(|(name, _)| selected_tool_names.contains(name)) - .map(|(name, tool)| (name.clone(), tool.clone())) - .collect::>(); - let connectors = connectors::accessible_connectors_from_mcp_tools(&mcp_tools); - let explicitly_enabled_connectors = HashSet::new(); - let connectors = filter_connectors_for_input( - &connectors, - &[user_message("run the selected tools")], - &explicitly_enabled_connectors, - &HashMap::new(), - ); +fn mcp_tool_exposure_searches_large_effective_tool_sets() { let config = test_config(); - selected_mcp_tools.extend(filter_codex_apps_mcp_tools( + let tools_config = tools_config_for_mcp_tool_exposure(/*search_tool*/ true); + let mcp_tools = numbered_mcp_tools(DIRECT_MCP_TOOL_EXPOSURE_THRESHOLD); + + let exposure = build_mcp_tool_exposure( &mcp_tools, - &connectors, + /*connectors*/ None, + &[], &config, - )); - - let mut tool_names: Vec = selected_mcp_tools.into_keys().collect(); - tool_names.sort(); - assert_eq!( - tool_names, - vec![ - "mcp__codex_apps__calendar_create_event".to_string(), - "mcp__rmcp__echo".to_string(), - ] + &tools_config, ); + + assert!(exposure.direct_tools.is_empty()); + let deferred_tools = exposure + .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); } #[test] -fn apps_mentions_add_codex_apps_tools_to_search_selected_set() { - let selected_tool_names = ["mcp__rmcp__echo".to_string()]; - let mcp_tools = HashMap::from([ - ( - "mcp__codex_apps__calendar_create_event".to_string(), - make_mcp_tool( - CODEX_APPS_MCP_SERVER_NAME, - "calendar_create_event", - Some("calendar"), - Some("Calendar"), - ), - ), - ( - "mcp__rmcp__echo".to_string(), - make_mcp_tool( - "rmcp", "echo", /*connector_id*/ None, /*connector_name*/ None, - ), +fn mcp_tool_exposure_directly_exposes_explicit_apps_in_large_search_sets() { + let config = test_config(); + let tools_config = tools_config_for_mcp_tool_exposure(/*search_tool*/ true); + 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"), ), - ]); + )]); + let connectors = vec![make_connector("calendar", "Calendar")]; - let mut selected_mcp_tools = mcp_tools - .iter() - .filter(|(name, _)| selected_tool_names.contains(name)) - .map(|(name, tool)| (name.clone(), tool.clone())) - .collect::>(); - let connectors = connectors::accessible_connectors_from_mcp_tools(&mcp_tools); - let explicitly_enabled_connectors = HashSet::new(); - let connectors = filter_connectors_for_input( - &connectors, - &[user_message("use $calendar and then echo the response")], - &explicitly_enabled_connectors, - &HashMap::new(), - ); - let config = test_config(); - selected_mcp_tools.extend(filter_codex_apps_mcp_tools( + let exposure = build_mcp_tool_exposure( &mcp_tools, - &connectors, + Some(connectors.as_slice()), + connectors.as_slice(), &config, - )); + &tools_config, + ); - let mut tool_names: Vec = selected_mcp_tools.into_keys().collect(); + let mut tool_names: Vec = exposure.direct_tools.into_keys().collect(); tool_names.sort(); assert_eq!( tool_names, - vec![ - "mcp__codex_apps__calendar_create_event".to_string(), - "mcp__rmcp__echo".to_string(), - ] + vec!["mcp__codex_apps__calendar_create_event".to_string()] + ); + assert_eq!( + exposure.deferred_tools.as_ref().map(HashMap::len), + Some(DIRECT_MCP_TOOL_EXPOSURE_THRESHOLD) ); + let deferred_tools = exposure + .deferred_tools + .as_ref() + .expect("large tool sets should be discoverable through tool_search"); + assert!(deferred_tools.contains_key("mcp__codex_apps__calendar_create_event")); + assert!(deferred_tools.contains_key("mcp__rmcp__tool_0")); } #[tokio::test] @@ -5125,7 +5097,7 @@ async fn fatal_tool_error_stops_turn_and_reports_error() { .list_all_tools() .await }; - let app_tools = Some(tools.clone()); + let deferred_mcp_tools = Some(tools.clone()); let router = ToolRouter::from_config( &turn_context.tools_config, crate::tools::router::ToolRouterParams { @@ -5135,7 +5107,7 @@ async fn fatal_tool_error_stops_turn_and_reports_error() { .map(|(name, tool)| (name, tool.tool)) .collect(), ), - app_tools, + deferred_mcp_tools, discoverable_tools: None, dynamic_tools: turn_context.dynamic_tools.as_slice(), }, diff --git a/codex-rs/core/src/tools/code_mode/mod.rs b/codex-rs/core/src/tools/code_mode/mod.rs index 21fca4689ee2..3d8cdabd2be6 100644 --- a/codex-rs/core/src/tools/code_mode/mod.rs +++ b/codex-rs/core/src/tools/code_mode/mod.rs @@ -266,7 +266,7 @@ async fn build_nested_router(exec: &ExecContext) -> ToolRouter { &nested_tools_config, ToolRouterParams { mcp_tools: Some(mcp_tools), - app_tools: None, + deferred_mcp_tools: None, discoverable_tools: None, dynamic_tools: exec.turn.dynamic_tools.as_slice(), }, diff --git a/codex-rs/core/src/tools/handlers/tool_search.rs b/codex-rs/core/src/tools/handlers/tool_search.rs index dd203e93b5ef..c1cb320397de 100644 --- a/codex-rs/core/src/tools/handlers/tool_search.rs +++ b/codex-rs/core/src/tools/handlers/tool_search.rs @@ -80,7 +80,9 @@ impl ToolHandler for ToolSearchHandler { results .into_iter() .filter_map(|result| entries.get(result.document.id)) - .map(|(_name, tool)| ToolSearchResultSource { + .map(|(name, tool)| ToolSearchResultSource { + qualified_tool_name: name.as_str(), + server_name: tool.server_name.as_str(), tool_namespace: tool.tool_namespace.as_str(), tool_name: tool.tool_name.as_str(), tool: &tool.tool, @@ -129,6 +131,15 @@ fn build_search_text(name: &str, info: &ToolInfo) -> String { parts.push(connector_description.to_string()); } + parts.extend( + info.plugin_display_names + .iter() + .map(String::as_str) + .map(str::trim) + .filter(|name| !name.is_empty()) + .map(str::to_string), + ); + parts.extend( info.tool .input_schema diff --git a/codex-rs/core/src/tools/js_repl/mod.rs b/codex-rs/core/src/tools/js_repl/mod.rs index 0cfbd2a60ccb..b192d7994815 100644 --- a/codex-rs/core/src/tools/js_repl/mod.rs +++ b/codex-rs/core/src/tools/js_repl/mod.rs @@ -1571,7 +1571,7 @@ impl JsReplManager { .map(|(name, tool)| (name, tool.tool)) .collect(), ), - app_tools: None, + deferred_mcp_tools: None, discoverable_tools: None, dynamic_tools: exec.turn.dynamic_tools.as_slice(), }, diff --git a/codex-rs/core/src/tools/router.rs b/codex-rs/core/src/tools/router.rs index 4fd64715bb10..88d78468feeb 100644 --- a/codex-rs/core/src/tools/router.rs +++ b/codex-rs/core/src/tools/router.rs @@ -41,7 +41,7 @@ pub struct ToolRouter { pub(crate) struct ToolRouterParams<'a> { pub(crate) mcp_tools: Option>, - pub(crate) app_tools: Option>, + pub(crate) deferred_mcp_tools: Option>, pub(crate) discoverable_tools: Option>, pub(crate) dynamic_tools: &'a [DynamicToolSpec], } @@ -50,14 +50,14 @@ impl ToolRouter { pub fn from_config(config: &ToolsConfig, params: ToolRouterParams<'_>) -> Self { let ToolRouterParams { mcp_tools, - app_tools, + deferred_mcp_tools, discoverable_tools, dynamic_tools, } = params; let builder = build_specs_with_discoverable_tools( config, mcp_tools, - app_tools, + deferred_mcp_tools, discoverable_tools, dynamic_tools, ); diff --git a/codex-rs/core/src/tools/router_tests.rs b/codex-rs/core/src/tools/router_tests.rs index 641adb56de03..cb4ecd46fad0 100644 --- a/codex-rs/core/src/tools/router_tests.rs +++ b/codex-rs/core/src/tools/router_tests.rs @@ -25,7 +25,7 @@ async fn js_repl_tools_only_blocks_direct_tool_calls() -> anyhow::Result<()> { .await .list_all_tools() .await; - let app_tools = Some(mcp_tools.clone()); + let deferred_mcp_tools = Some(mcp_tools.clone()); let router = ToolRouter::from_config( &turn.tools_config, ToolRouterParams { @@ -35,7 +35,7 @@ async fn js_repl_tools_only_blocks_direct_tool_calls() -> anyhow::Result<()> { .map(|(name, tool)| (name, tool.tool)) .collect(), ), - app_tools, + deferred_mcp_tools, discoverable_tools: None, dynamic_tools: turn.dynamic_tools.as_slice(), }, @@ -83,7 +83,7 @@ async fn js_repl_tools_only_allows_js_repl_source_calls() -> anyhow::Result<()> .await .list_all_tools() .await; - let app_tools = Some(mcp_tools.clone()); + let deferred_mcp_tools = Some(mcp_tools.clone()); let router = ToolRouter::from_config( &turn.tools_config, ToolRouterParams { @@ -93,7 +93,7 @@ async fn js_repl_tools_only_allows_js_repl_source_calls() -> anyhow::Result<()> .map(|(name, tool)| (name, tool.tool)) .collect(), ), - app_tools, + deferred_mcp_tools, discoverable_tools: None, dynamic_tools: turn.dynamic_tools.as_slice(), }, diff --git a/codex-rs/core/src/tools/spec.rs b/codex-rs/core/src/tools/spec.rs index 90b9a382aaa8..f7c028da2f66 100644 --- a/codex-rs/core/src/tools/spec.rs +++ b/codex-rs/core/src/tools/spec.rs @@ -5,12 +5,11 @@ use crate::tools::handlers::multi_agents_common::DEFAULT_WAIT_TIMEOUT_MS; use crate::tools::handlers::multi_agents_common::MAX_WAIT_TIMEOUT_MS; use crate::tools::handlers::multi_agents_common::MIN_WAIT_TIMEOUT_MS; use crate::tools::registry::ToolRegistryBuilder; -use codex_mcp::mcp::CODEX_APPS_MCP_SERVER_NAME; use codex_mcp::mcp_connection_manager::ToolInfo; use codex_protocol::dynamic_tools::DynamicToolSpec; use codex_tools::DiscoverableTool; use codex_tools::ToolHandlerKind; -use codex_tools::ToolRegistryPlanAppTool; +use codex_tools::ToolRegistryPlanDeferredTool; use codex_tools::ToolRegistryPlanParams; use codex_tools::ToolUserShellType; use codex_tools::ToolsConfig; @@ -32,7 +31,7 @@ pub(crate) fn tool_user_shell_type(user_shell: &Shell) -> ToolUserShellType { pub(crate) fn build_specs_with_discoverable_tools( config: &ToolsConfig, mcp_tools: Option>, - app_tools: Option>, + deferred_mcp_tools: Option>, discoverable_tools: Option>, dynamic_tools: &[DynamicToolSpec], ) -> ToolRegistryBuilder { @@ -68,10 +67,11 @@ pub(crate) fn build_specs_with_discoverable_tools( use crate::tools::handlers::multi_agents_v2::WaitAgentHandler as WaitAgentHandlerV2; let mut builder = ToolRegistryBuilder::new(); - let app_tool_sources = app_tools.as_ref().map(|app_tools| { - app_tools - .values() - .map(|tool| ToolRegistryPlanAppTool { + let deferred_mcp_tool_sources = deferred_mcp_tools.as_ref().map(|tools| { + tools + .iter() + .map(|(qualified_tool_name, tool)| ToolRegistryPlanDeferredTool { + qualified_tool_name: qualified_tool_name.as_str(), tool_name: tool.tool_name.as_str(), tool_namespace: tool.tool_namespace.as_str(), server_name: tool.server_name.as_str(), @@ -86,7 +86,7 @@ pub(crate) fn build_specs_with_discoverable_tools( config, ToolRegistryPlanParams { mcp_tools: mcp_tools.as_ref(), - app_tools: app_tool_sources.as_deref(), + deferred_mcp_tools: deferred_mcp_tool_sources.as_deref(), discoverable_tools: discoverable_tools.as_deref(), dynamic_tools, default_agent_type_description: &default_agent_type_description, @@ -95,7 +95,6 @@ pub(crate) fn build_specs_with_discoverable_tools( min_timeout_ms: MIN_WAIT_TIMEOUT_MS, max_timeout_ms: MAX_WAIT_TIMEOUT_MS, }, - codex_apps_mcp_server_name: CODEX_APPS_MCP_SERVER_NAME, }, ); let shell_handler = Arc::new(ShellHandler); @@ -207,9 +206,9 @@ pub(crate) fn build_specs_with_discoverable_tools( } ToolHandlerKind::ToolSearch => { if tool_search_handler.is_none() { - tool_search_handler = app_tools + tool_search_handler = deferred_mcp_tools .as_ref() - .map(|app_tools| Arc::new(ToolSearchHandler::new(app_tools.clone()))); + .map(|tools| Arc::new(ToolSearchHandler::new(tools.clone()))); } if let Some(tool_search_handler) = tool_search_handler.as_ref() { builder.register_handler(handler.name, tool_search_handler.clone()); diff --git a/codex-rs/core/src/tools/spec_tests.rs b/codex-rs/core/src/tools/spec_tests.rs index 4615b453195e..c1971c3c56f6 100644 --- a/codex-rs/core/src/tools/spec_tests.rs +++ b/codex-rs/core/src/tools/spec_tests.rs @@ -174,13 +174,13 @@ fn model_info_from_models_json(slug: &str) -> ModelInfo { fn build_specs( config: &ToolsConfig, mcp_tools: Option>, - app_tools: Option>, + deferred_mcp_tools: Option>, dynamic_tools: &[DynamicToolSpec], ) -> ToolRegistryBuilder { build_specs_with_discoverable_tools( config, mcp_tools, - app_tools, + deferred_mcp_tools, /*discoverable_tools*/ None, dynamic_tools, ) @@ -229,7 +229,7 @@ fn get_memory_requires_feature_flag() { let (tools, _) = build_specs( &tools_config, /*mcp_tools*/ None, - /*app_tools*/ None, + /*deferred_mcp_tools*/ None, &[], ) .build(); @@ -261,7 +261,7 @@ fn assert_model_tools( &tools_config, ToolRouterParams { mcp_tools: None, - app_tools: None, + deferred_mcp_tools: None, discoverable_tools: None, dynamic_tools: &[], }, @@ -521,7 +521,7 @@ fn test_build_specs_default_shell_present() { let (tools, _) = build_specs( &tools_config, Some(HashMap::new()), - /*app_tools*/ None, + /*deferred_mcp_tools*/ None, &[], ) .build(); @@ -627,7 +627,7 @@ fn tool_suggest_requires_apps_and_plugins_features() { let (tools, _) = build_specs_with_discoverable_tools( &tools_config, /*mcp_tools*/ None, - /*app_tools*/ None, + /*deferred_mcp_tools*/ None, discoverable_tools.clone(), &[], ) @@ -643,7 +643,7 @@ fn tool_suggest_requires_apps_and_plugins_features() { } #[test] -fn search_tool_description_handles_no_enabled_apps() { +fn search_tool_description_handles_no_enabled_mcp_tools() { let model_info = search_capable_model_info(); let mut features = Features::with_defaults(); features.enable(Feature::Apps); @@ -672,7 +672,7 @@ fn search_tool_description_handles_no_enabled_apps() { }; assert!(description.contains("None currently enabled.")); - assert!(!description.contains("{{app_descriptions}}")); + assert!(!description.contains("{{source_descriptions}}")); } #[test] @@ -725,7 +725,7 @@ fn search_tool_description_falls_back_to_connector_name_without_description() { } #[test] -fn search_tool_registers_namespaced_app_tool_aliases() { +fn search_tool_registers_namespaced_mcp_tool_aliases() { let model_info = search_capable_model_info(); let mut features = Features::with_defaults(); features.enable(Feature::Apps); @@ -779,15 +779,30 @@ fn search_tool_registers_namespaced_app_tool_aliases() { plugin_display_names: Vec::new(), }, ), + ( + "mcp__rmcp__echo".to_string(), + ToolInfo { + server_name: "rmcp".to_string(), + tool_name: "echo".to_string(), + tool_namespace: "rmcp".to_string(), + tool: mcp_tool("echo", "Echo", serde_json::json!({"type": "object"})), + connector_id: None, + connector_name: None, + connector_description: None, + plugin_display_names: Vec::new(), + }, + ), ])), &[], ) .build(); - let alias = tool_handler_key("_create_event", Some("mcp__codex_apps__calendar")); + let app_alias = tool_handler_key("_create_event", Some("mcp__codex_apps__calendar")); + let mcp_alias = tool_handler_key("echo", Some("mcp__rmcp__")); assert!(registry.has_handler(TOOL_SEARCH_TOOL_NAME, /*namespace*/ None)); - assert!(registry.has_handler(alias.as_str(), /*namespace*/ None)); + assert!(registry.has_handler(app_alias.as_str(), /*namespace*/ None)); + assert!(registry.has_handler(mcp_alias.as_str(), /*namespace*/ None)); } #[test] @@ -822,7 +837,7 @@ fn test_mcp_tool_property_missing_type_defaults_to_string() { }), ), )])), - /*app_tools*/ None, + /*deferred_mcp_tools*/ None, &[], ) .build(); @@ -880,7 +895,7 @@ fn test_mcp_tool_integer_normalized_to_number() { }), ), )])), - /*app_tools*/ None, + /*deferred_mcp_tools*/ None, &[], ) .build(); @@ -937,7 +952,7 @@ fn test_mcp_tool_array_without_items_gets_default_string_items() { }), ), )])), - /*app_tools*/ None, + /*deferred_mcp_tools*/ None, &[], ) .build(); @@ -998,7 +1013,7 @@ fn test_mcp_tool_anyof_defaults_to_string() { }), ), )])), - /*app_tools*/ None, + /*deferred_mcp_tools*/ None, &[], ) .build(); @@ -1072,7 +1087,7 @@ fn test_get_openai_tools_mcp_tools_with_additional_properties_schema() { }), ), )])), - /*app_tools*/ None, + /*deferred_mcp_tools*/ None, &[], ) .build(); diff --git a/codex-rs/core/tests/suite/search_tool.rs b/codex-rs/core/tests/suite/search_tool.rs index d41785bd4eae..39402500a6fb 100644 --- a/codex-rs/core/tests/suite/search_tool.rs +++ b/codex-rs/core/tests/suite/search_tool.rs @@ -2,6 +2,8 @@ #![allow(clippy::unwrap_used, clippy::expect_used)] use anyhow::Result; +use codex_config::types::McpServerConfig; +use codex_config::types::McpServerTransportConfig; use codex_core::config::Config; use codex_features::Feature; use codex_login::CodexAuth; @@ -24,15 +26,18 @@ use core_test_support::responses::mount_sse_sequence; use core_test_support::responses::sse; use core_test_support::responses::start_mock_server; use core_test_support::skip_if_no_network; +use core_test_support::stdio_server_bin; use core_test_support::test_codex::TestCodexBuilder; use core_test_support::test_codex::test_codex; use core_test_support::wait_for_event; use pretty_assertions::assert_eq; use serde_json::Value; use serde_json::json; +use std::collections::HashMap; +use std::time::Duration; const SEARCH_TOOL_DESCRIPTION_SNIPPETS: [&str; 2] = [ - "You have access to all the tools of the following apps/connectors", + "You have access to tools from the following MCP servers/connectors", "- Calendar: Plan events and manage your calendar.", ]; const TOOL_SEARCH_TOOL_NAME: &str = "tool_search"; @@ -165,7 +170,7 @@ async fn search_tool_flag_adds_tool_search() -> Result<()> { "parameters": { "type": "object", "properties": { - "query": {"type": "string", "description": "Search query for apps tools."}, + "query": {"type": "string", "description": "Search query for MCP tools."}, "limit": {"type": "number", "description": "Maximum number of tools to return (defaults to 8)."}, }, "required": ["query"], @@ -581,3 +586,123 @@ async fn tool_search_returns_deferred_tools_without_follow_up_tool_injection() - Ok(()) } + +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn tool_search_indexes_only_enabled_non_app_mcp_tools() -> Result<()> { + skip_if_no_network!(Ok(())); + + let server = start_mock_server().await; + let apps_server = AppsTestServer::mount_searchable(&server).await?; + let echo_call_id = "tool-search-echo"; + let image_call_id = "tool-search-image"; + let mock = mount_sse_sequence( + &server, + vec![ + sse(vec![ + ev_response_created("resp-1"), + ev_tool_search_call( + echo_call_id, + &json!({ + "query": "Echo back the provided message and include environment data.", + "limit": 8, + }), + ), + ev_tool_search_call( + image_call_id, + &json!({ + "query": "Return a single image content block.", + "limit": 8, + }), + ), + ev_completed("resp-1"), + ]), + sse(vec![ + ev_response_created("resp-2"), + ev_assistant_message("msg-1", "done"), + ev_completed("resp-2"), + ]), + ], + ) + .await; + + let rmcp_test_server_bin = stdio_server_bin()?; + let mut builder = + configured_builder(apps_server.chatgpt_base_url.clone()).with_config(move |config| { + let mut servers = config.mcp_servers.get().clone(); + servers.insert( + "rmcp".to_string(), + McpServerConfig { + transport: McpServerTransportConfig::Stdio { + command: rmcp_test_server_bin, + args: Vec::new(), + env: None, + env_vars: Vec::new(), + cwd: None, + }, + enabled: true, + required: false, + disabled_reason: None, + startup_timeout_sec: Some(Duration::from_secs(10)), + tool_timeout_sec: None, + enabled_tools: Some(vec!["echo".to_string(), "image".to_string()]), + disabled_tools: Some(vec!["image".to_string()]), + scopes: None, + oauth_resource: None, + tools: HashMap::new(), + }, + ); + config + .mcp_servers + .set(servers) + .expect("test mcp servers should accept any configuration"); + }); + let test = builder.build(&server).await?; + + test.submit_turn_with_policies( + "Find the rmcp echo and image tools.", + AskForApproval::Never, + SandboxPolicy::DangerFullAccess, + ) + .await?; + + let requests = mock.requests(); + assert_eq!(requests.len(), 2); + + let first_request_tools = tool_names(&requests[0].body_json()); + assert!( + first_request_tools + .iter() + .any(|name| name == TOOL_SEARCH_TOOL_NAME), + "first request should advertise tool_search: {first_request_tools:?}" + ); + assert!( + !first_request_tools + .iter() + .any(|name| name == "mcp__rmcp__echo"), + "non-app MCP tools should be hidden before search in large-search mode: {first_request_tools:?}" + ); + + let echo_tools = tool_search_output_tools(&requests[1], echo_call_id); + let rmcp_echo_tools = echo_tools + .iter() + .filter(|tool| tool.get("name").and_then(Value::as_str) == Some("mcp__rmcp__")) + .flat_map(|namespace| namespace.get("tools").and_then(Value::as_array)) + .flatten() + .filter_map(|tool| tool.get("name").and_then(Value::as_str).map(str::to_string)) + .collect::>(); + assert_eq!(rmcp_echo_tools, vec!["echo".to_string()]); + + let image_tools = tool_search_output_tools(&requests[1], image_call_id); + let found_rmcp_image_tool = image_tools + .iter() + .filter(|tool| tool.get("name").and_then(Value::as_str) == Some("mcp__rmcp__")) + .flat_map(|namespace| namespace.get("tools").and_then(Value::as_array)) + .flatten() + .any(|tool| tool.get("name").and_then(Value::as_str).is_some()); + assert!( + !found_rmcp_image_tool, + "disabled non-app MCP tools should not be searchable: {image_tools:?}" + ); + + Ok(()) +} diff --git a/codex-rs/tools/src/lib.rs b/codex-rs/tools/src/lib.rs index 16e84700bb20..5998b08e9110 100644 --- a/codex-rs/tools/src/lib.rs +++ b/codex-rs/tools/src/lib.rs @@ -99,21 +99,22 @@ pub use tool_discovery::DiscoverableToolType; pub use tool_discovery::TOOL_SEARCH_DEFAULT_LIMIT; pub use tool_discovery::TOOL_SEARCH_TOOL_NAME; pub use tool_discovery::TOOL_SUGGEST_TOOL_NAME; -pub use tool_discovery::ToolSearchAppInfo; -pub use tool_discovery::ToolSearchAppSource; pub use tool_discovery::ToolSearchResultSource; +pub use tool_discovery::ToolSearchSource; +pub use tool_discovery::ToolSearchSourceInfo; pub use tool_discovery::ToolSuggestEntry; -pub use tool_discovery::collect_tool_search_app_infos; pub use tool_discovery::collect_tool_search_output_tools; +pub use tool_discovery::collect_tool_search_source_infos; pub use tool_discovery::collect_tool_suggest_entries; pub use tool_discovery::create_tool_search_tool; pub use tool_discovery::create_tool_suggest_tool; pub use tool_discovery::filter_tool_suggest_discoverable_tools_for_client; +pub use tool_discovery::tool_search_output_namespace_and_name; pub use tool_registry_plan::build_tool_registry_plan; pub use tool_registry_plan_types::ToolHandlerKind; pub use tool_registry_plan_types::ToolHandlerSpec; pub use tool_registry_plan_types::ToolRegistryPlan; -pub use tool_registry_plan_types::ToolRegistryPlanAppTool; +pub use tool_registry_plan_types::ToolRegistryPlanDeferredTool; pub use tool_registry_plan_types::ToolRegistryPlanParams; pub use tool_spec::ConfiguredToolSpec; pub use tool_spec::ResponsesApiWebSearchFilters; diff --git a/codex-rs/tools/src/tool_discovery.rs b/codex-rs/tools/src/tool_discovery.rs index 6b2bb3de2f29..2f95d5a81256 100644 --- a/codex-rs/tools/src/tool_discovery.rs +++ b/codex-rs/tools/src/tool_discovery.rs @@ -16,13 +16,13 @@ pub const TOOL_SEARCH_DEFAULT_LIMIT: usize = 8; pub const TOOL_SUGGEST_TOOL_NAME: &str = "tool_suggest"; #[derive(Clone, Debug, PartialEq, Eq)] -pub struct ToolSearchAppInfo { +pub struct ToolSearchSourceInfo { pub name: String, pub description: Option, } #[derive(Clone, Copy, Debug, PartialEq, Eq)] -pub struct ToolSearchAppSource<'a> { +pub struct ToolSearchSource<'a> { pub server_name: &'a str, pub connector_name: Option<&'a str>, pub connector_description: Option<&'a str>, @@ -30,6 +30,8 @@ pub struct ToolSearchAppSource<'a> { #[derive(Clone, Copy, Debug, PartialEq)] pub struct ToolSearchResultSource<'a> { + pub qualified_tool_name: &'a str, + pub server_name: &'a str, pub tool_namespace: &'a str, pub tool_name: &'a str, pub tool: &'a rmcp::model::Tool, @@ -143,12 +145,15 @@ pub struct ToolSuggestEntry { pub app_connector_ids: Vec, } -pub fn create_tool_search_tool(app_tools: &[ToolSearchAppInfo], default_limit: usize) -> ToolSpec { +pub fn create_tool_search_tool( + searchable_sources: &[ToolSearchSourceInfo], + default_limit: usize, +) -> ToolSpec { let properties = BTreeMap::from([ ( "query".to_string(), JsonSchema::String { - description: Some("Search query for apps tools.".to_string()), + description: Some("Search query for MCP tools.".to_string()), }, ), ( @@ -161,22 +166,22 @@ pub fn create_tool_search_tool(app_tools: &[ToolSearchAppInfo], default_limit: u ), ]); - let mut app_descriptions = BTreeMap::new(); - for app_tool in app_tools { - app_descriptions - .entry(app_tool.name.clone()) + let mut source_descriptions = BTreeMap::new(); + for source in searchable_sources { + source_descriptions + .entry(source.name.clone()) .and_modify(|existing: &mut Option| { if existing.is_none() { - *existing = app_tool.description.clone(); + *existing = source.description.clone(); } }) - .or_insert(app_tool.description.clone()); + .or_insert(source.description.clone()); } - let app_descriptions = if app_descriptions.is_empty() { + let source_descriptions = if source_descriptions.is_empty() { "None currently enabled.".to_string() } else { - app_descriptions + source_descriptions .into_iter() .map(|(name, description)| match description { Some(description) => format!("- {name}: {description}"), @@ -187,7 +192,7 @@ pub fn create_tool_search_tool(app_tools: &[ToolSearchAppInfo], default_limit: u }; let description = format!( - "# Apps (Connectors) tool discovery\n\nSearches over apps/connectors tool metadata with BM25 and exposes matching tools for the next model call.\n\nYou have access to all the tools of the following apps/connectors:\n{app_descriptions}\nSome of the tools may not have been provided to you upfront, and you should use this tool (`{TOOL_SEARCH_TOOL_NAME}`) to search for the required tools and load them for the apps mentioned above. For the apps mentioned above, always use `{TOOL_SEARCH_TOOL_NAME}` instead of `list_mcp_resources` or `list_mcp_resource_templates` for tool discovery." + "# MCP tool discovery\n\nSearches over MCP tool metadata with BM25 and exposes matching tools for the next model call.\n\nYou have access to tools from the following MCP servers/connectors:\n{source_descriptions}\nSome of the tools may not have been provided to you upfront, and you should use this tool (`{TOOL_SEARCH_TOOL_NAME}`) to search for the required MCP tools. For MCP tool discovery, always use `{TOOL_SEARCH_TOOL_NAME}` instead of `list_mcp_resources` or `list_mcp_resource_templates`." ); ToolSpec::ToolSearch { @@ -205,9 +210,17 @@ pub fn collect_tool_search_output_tools<'a>( tool_sources: impl IntoIterator>, ) -> Result, serde_json::Error> { let grouped = tool_sources.into_iter().fold( - BTreeMap::<&'a str, Vec>>::new(), + BTreeMap::)>>::new(), |mut grouped, tool| { - grouped.entry(tool.tool_namespace).or_default().push(tool); + let (tool_namespace, tool_name) = tool_search_output_namespace_and_name( + tool.qualified_tool_name, + tool.tool_namespace, + tool.tool_name, + ); + grouped + .entry(tool_namespace) + .or_default() + .push((tool_name, tool)); grouped }, ); @@ -219,20 +232,28 @@ pub fn collect_tool_search_output_tools<'a>( }; let description = first_tool + .1 .connector_description .map(str::to_string) .or_else(|| { first_tool + .1 .connector_name .map(str::trim) .filter(|connector_name| !connector_name.is_empty()) .map(|connector_name| format!("Tools for working with {connector_name}.")) + }) + .or_else(|| { + Some(format!( + "Tools from the {} MCP server.", + first_tool.1.server_name + )) }); let tools = tools .iter() .map(|tool| { - mcp_tool_to_deferred_responses_api_tool(tool.tool_name.to_string(), tool.tool) + mcp_tool_to_deferred_responses_api_tool(tool.0.clone(), tool.1.tool) .map(ResponsesApiNamespaceTool::Function) }) .collect::, _>>()?; @@ -247,29 +268,77 @@ pub fn collect_tool_search_output_tools<'a>( Ok(results) } -pub fn collect_tool_search_app_infos<'a>( - app_tools: impl IntoIterator>, - codex_apps_server_name: &str, -) -> Vec { - app_tools +pub fn tool_search_output_namespace_and_name( + qualified_tool_name: &str, + tool_namespace: &str, + tool_name: &str, +) -> (String, String) { + if let Some(child_name) = qualified_tool_name.strip_prefix(tool_namespace) + && child_name == tool_name + { + return (tool_namespace.to_string(), tool_name.to_string()); + } + + if let Some((namespace, child_name)) = + split_qualified_mcp_tool_name_for_search(qualified_tool_name) + { + return (namespace.to_string(), child_name.to_string()); + } + + (tool_namespace.to_string(), tool_name.to_string()) +} + +fn split_qualified_mcp_tool_name_for_search(qualified_tool_name: &str) -> Option<(&str, &str)> { + let stripped = qualified_tool_name.strip_prefix("mcp__")?; + let server_name_len = stripped.find("__")?; + let child_name_start = server_name_len + "__".len(); + let child_name = stripped.get(child_name_start..)?; + if child_name.is_empty() { + return None; + } + + let namespace_len = "mcp__".len() + server_name_len + "__".len(); + Some((&qualified_tool_name[..namespace_len], child_name)) +} + +pub fn collect_tool_search_source_infos<'a>( + searchable_tools: impl IntoIterator>, +) -> Vec { + searchable_tools .into_iter() - .filter(|tool| tool.server_name == codex_apps_server_name) .filter_map(|tool| { - let name = tool + if let Some(name) = tool .connector_name .map(str::trim) - .filter(|connector_name| !connector_name.is_empty())? - .to_string(); - let description = tool - .connector_description - .map(str::trim) - .filter(|connector_description| !connector_description.is_empty()) - .map(str::to_string); - Some(ToolSearchAppInfo { name, description }) + .filter(|connector_name| !connector_name.is_empty()) + { + let description = connector_description(tool.connector_description); + return Some(ToolSearchSourceInfo { + name: name.to_string(), + description, + }); + } + + let name = tool.server_name.trim(); + if name.is_empty() { + return None; + } + + Some(ToolSearchSourceInfo { + name: name.to_string(), + description: None, + }) }) .collect() } +fn connector_description(description: Option<&str>) -> Option { + description + .map(str::trim) + .filter(|description| !description.is_empty()) + .map(str::to_string) +} + pub fn create_tool_suggest_tool(discoverable_tools: &[ToolSuggestEntry]) -> ToolSpec { let discoverable_tool_ids = discoverable_tools .iter() diff --git a/codex-rs/tools/src/tool_discovery_tests.rs b/codex-rs/tools/src/tool_discovery_tests.rs index 97756f5657df..2dd4e44b6570 100644 --- a/codex-rs/tools/src/tool_discovery_tests.rs +++ b/codex-rs/tools/src/tool_discovery_tests.rs @@ -25,23 +25,23 @@ fn mcp_tool(name: &str, description: &str) -> Tool { } #[test] -fn create_tool_search_tool_deduplicates_and_renders_enabled_apps() { +fn create_tool_search_tool_deduplicates_and_renders_enabled_sources() { assert_eq!( create_tool_search_tool( &[ - ToolSearchAppInfo { + ToolSearchSourceInfo { name: "Google Drive".to_string(), description: Some( "Use Google Drive as the single entrypoint for Drive, Docs, Sheets, and Slides work." .to_string(), ), }, - ToolSearchAppInfo { + ToolSearchSourceInfo { name: "Google Drive".to_string(), description: None, }, - ToolSearchAppInfo { - name: "Slack".to_string(), + ToolSearchSourceInfo { + name: "docs".to_string(), description: None, }, ], @@ -49,7 +49,7 @@ fn create_tool_search_tool_deduplicates_and_renders_enabled_apps() { ), ToolSpec::ToolSearch { execution: "client".to_string(), - description: "# Apps (Connectors) tool discovery\n\nSearches over apps/connectors tool metadata with BM25 and exposes matching tools for the next model call.\n\nYou have access to all the tools of the following apps/connectors:\n- Google Drive: Use Google Drive as the single entrypoint for Drive, Docs, Sheets, and Slides work.\n- Slack\nSome of the tools may not have been provided to you upfront, and you should use this tool (`tool_search`) to search for the required tools and load them for the apps mentioned above. For the apps mentioned above, always use `tool_search` instead of `list_mcp_resources` or `list_mcp_resource_templates` for tool discovery.".to_string(), + description: "# MCP tool discovery\n\nSearches over MCP tool metadata with BM25 and exposes matching tools for the next model call.\n\nYou have access to tools from the following MCP servers/connectors:\n- Google Drive: Use Google Drive as the single entrypoint for Drive, Docs, Sheets, and Slides work.\n- docs\nSome of the tools may not have been provided to you upfront, and you should use this tool (`tool_search`) to search for the required MCP tools. For MCP tool discovery, always use `tool_search` instead of `list_mcp_resources` or `list_mcp_resource_templates`.".to_string(), parameters: JsonSchema::Object { properties: BTreeMap::from([ ( @@ -64,7 +64,7 @@ fn create_tool_search_tool_deduplicates_and_renders_enabled_apps() { ( "query".to_string(), JsonSchema::String { - description: Some("Search query for apps tools.".to_string()), + description: Some("Search query for MCP tools.".to_string()), }, ), ]), @@ -160,9 +160,12 @@ fn collect_tool_search_output_tools_groups_results_by_namespace() { let calendar_create_event = mcp_tool("calendar-create-event", "Create a calendar event."); let gmail_read_email = mcp_tool("gmail-read-email", "Read an email."); let calendar_list_events = mcp_tool("calendar-list-events", "List calendar events."); + let docs_search = mcp_tool("search", "Search docs."); let tools = collect_tool_search_output_tools([ ToolSearchResultSource { + qualified_tool_name: "mcp__codex_apps__calendar_create_event", + server_name: "codex_apps", tool_namespace: "mcp__codex_apps__calendar", tool_name: "_create_event", tool: &calendar_create_event, @@ -170,6 +173,8 @@ fn collect_tool_search_output_tools_groups_results_by_namespace() { connector_description: Some("Plan events"), }, ToolSearchResultSource { + qualified_tool_name: "mcp__codex_apps__gmail_read_email", + server_name: "codex_apps", tool_namespace: "mcp__codex_apps__gmail", tool_name: "_read_email", tool: &gmail_read_email, @@ -177,12 +182,23 @@ fn collect_tool_search_output_tools_groups_results_by_namespace() { connector_description: Some("Read mail"), }, ToolSearchResultSource { + qualified_tool_name: "mcp__codex_apps__calendar_list_events", + server_name: "codex_apps", tool_namespace: "mcp__codex_apps__calendar", tool_name: "_list_events", tool: &calendar_list_events, connector_name: Some("Calendar"), connector_description: Some("Plan events"), }, + ToolSearchResultSource { + qualified_tool_name: "mcp__docs__search", + server_name: "docs", + tool_namespace: "docs", + tool_name: "search", + tool: &docs_search, + connector_name: None, + connector_description: None, + }, ]) .expect("collect tool search output tools"); @@ -235,6 +251,22 @@ fn collect_tool_search_output_tools_groups_results_by_namespace() { output_schema: None, })], }), + ToolSearchOutputTool::Namespace(ResponsesApiNamespace { + name: "mcp__docs__".to_string(), + description: "Tools from the docs MCP server.".to_string(), + tools: vec![ResponsesApiNamespaceTool::Function(ResponsesApiTool { + name: "search".to_string(), + description: "Search docs.".to_string(), + strict: false, + defer_loading: Some(true), + parameters: JsonSchema::Object { + properties: Default::default(), + required: None, + additional_properties: None, + }, + output_schema: None, + })], + }), ], ); } @@ -244,6 +276,8 @@ fn collect_tool_search_output_tools_falls_back_to_connector_name_description() { let gmail_batch_read_email = mcp_tool("gmail-batch-read-email", "Read multiple emails."); let tools = collect_tool_search_output_tools([ToolSearchResultSource { + qualified_tool_name: "mcp__codex_apps__gmail_batch_read_email", + server_name: "codex_apps", tool_namespace: "mcp__codex_apps__gmail", tool_name: "_batch_read_email", tool: &gmail_batch_read_email, diff --git a/codex-rs/tools/src/tool_registry_plan.rs b/codex-rs/tools/src/tool_registry_plan.rs index cc706f24fca6..9d5792762310 100644 --- a/codex-rs/tools/src/tool_registry_plan.rs +++ b/codex-rs/tools/src/tool_registry_plan.rs @@ -8,13 +8,13 @@ use crate::TOOL_SUGGEST_TOOL_NAME; use crate::ToolHandlerKind; use crate::ToolRegistryPlan; use crate::ToolRegistryPlanParams; -use crate::ToolSearchAppSource; +use crate::ToolSearchSource; use crate::ToolSpec; use crate::ToolsConfig; use crate::ViewImageToolOptions; use crate::WebSearchToolOptions; use crate::collect_code_mode_tool_definitions; -use crate::collect_tool_search_app_infos; +use crate::collect_tool_search_source_infos; use crate::collect_tool_suggest_entries; use crate::create_apply_patch_freeform_tool; use crate::create_apply_patch_json_tool; @@ -58,6 +58,7 @@ use crate::mcp_tool_to_responses_api_tool; use crate::request_permissions_tool_description; use crate::request_user_input_tool_description; use crate::tool_registry_plan_types::agent_type_description; +use crate::tool_search_output_namespace_and_name; use codex_protocol::openai_models::ApplyPatchToolType; use codex_protocol::openai_models::ConfigShellToolType; use rmcp::model::Tool as McpTool; @@ -228,26 +229,31 @@ pub fn build_tool_registry_plan( } if config.search_tool - && let Some(app_tools) = params.app_tools + && let Some(deferred_mcp_tools) = params.deferred_mcp_tools { - let search_app_infos = collect_tool_search_app_infos( - app_tools.iter().map(|tool| ToolSearchAppSource { - server_name: tool.server_name, - connector_name: tool.connector_name, - connector_description: tool.connector_description, - }), - params.codex_apps_mcp_server_name, - ); + let search_source_infos = + collect_tool_search_source_infos(deferred_mcp_tools.iter().map(|tool| { + ToolSearchSource { + server_name: tool.server_name, + connector_name: tool.connector_name, + connector_description: tool.connector_description, + } + })); plan.push_spec( - create_tool_search_tool(&search_app_infos, TOOL_SEARCH_DEFAULT_LIMIT), + create_tool_search_tool(&search_source_infos, TOOL_SEARCH_DEFAULT_LIMIT), /*supports_parallel_tool_calls*/ true, config.code_mode_enabled, ); plan.register_handler(TOOL_SEARCH_TOOL_NAME, ToolHandlerKind::ToolSearch); - for tool in app_tools { + for tool in deferred_mcp_tools { + let (tool_namespace, tool_name) = tool_search_output_namespace_and_name( + tool.qualified_tool_name, + tool.tool_namespace, + tool.tool_name, + ); plan.register_handler( - format!("{}:{}", tool.tool_namespace, tool.tool_name), + format!("{tool_namespace}:{tool_name}"), ToolHandlerKind::Mcp, ); } diff --git a/codex-rs/tools/src/tool_registry_plan_tests.rs b/codex-rs/tools/src/tool_registry_plan_tests.rs index fd7ab1007377..d136209df862 100644 --- a/codex-rs/tools/src/tool_registry_plan_tests.rs +++ b/codex-rs/tools/src/tool_registry_plan_tests.rs @@ -9,7 +9,7 @@ use crate::ResponsesApiTool; use crate::ResponsesApiWebSearchFilters; use crate::ResponsesApiWebSearchUserLocation; use crate::ToolHandlerSpec; -use crate::ToolRegistryPlanAppTool; +use crate::ToolRegistryPlanDeferredTool; use crate::ToolsConfigParams; use crate::WaitAgentTimeoutOptions; use crate::mcp_call_tool_result_output_schema; @@ -56,7 +56,7 @@ fn test_full_toolset_specs_for_gpt5_codex_unified_exec_web_search() { let (tools, _) = build_specs( &config, /*mcp_tools*/ None, - /*app_tools*/ None, + /*deferred_mcp_tools*/ None, &[], ); @@ -157,7 +157,7 @@ fn test_build_specs_collab_tools_enabled() { let (tools, _) = build_specs( &tools_config, /*mcp_tools*/ None, - /*app_tools*/ None, + /*deferred_mcp_tools*/ None, &[], ); @@ -198,7 +198,7 @@ fn test_build_specs_multi_agent_v2_uses_task_names_and_hides_resume() { let (tools, _) = build_specs( &tools_config, /*mcp_tools*/ None, - /*app_tools*/ None, + /*deferred_mcp_tools*/ None, &[], ); @@ -366,7 +366,7 @@ fn test_build_specs_enable_fanout_enables_agent_jobs_and_collab_tools() { let (tools, _) = build_specs( &tools_config, /*mcp_tools*/ None, - /*app_tools*/ None, + /*deferred_mcp_tools*/ None, &[], ); @@ -400,7 +400,7 @@ fn view_image_tool_omits_detail_without_original_detail_feature() { let (tools, _) = build_specs( &tools_config, /*mcp_tools*/ None, - /*app_tools*/ None, + /*deferred_mcp_tools*/ None, &[], ); let view_image = find_tool(&tools, VIEW_IMAGE_TOOL_NAME); @@ -432,7 +432,7 @@ fn view_image_tool_includes_detail_with_original_detail_feature() { let (tools, _) = build_specs( &tools_config, /*mcp_tools*/ None, - /*app_tools*/ None, + /*deferred_mcp_tools*/ None, &[], ); let view_image = find_tool(&tools, VIEW_IMAGE_TOOL_NAME); @@ -475,7 +475,7 @@ fn test_build_specs_agent_job_worker_tools_enabled() { let (tools, _) = build_specs( &tools_config, /*mcp_tools*/ None, - /*app_tools*/ None, + /*deferred_mcp_tools*/ None, &[], ); @@ -511,7 +511,7 @@ fn request_user_input_description_reflects_default_mode_feature_flag() { let (tools, _) = build_specs( &tools_config, /*mcp_tools*/ None, - /*app_tools*/ None, + /*deferred_mcp_tools*/ None, &[], ); let request_user_input_tool = find_tool(&tools, REQUEST_USER_INPUT_TOOL_NAME); @@ -533,7 +533,7 @@ fn request_user_input_description_reflects_default_mode_feature_flag() { let (tools, _) = build_specs( &tools_config, /*mcp_tools*/ None, - /*app_tools*/ None, + /*deferred_mcp_tools*/ None, &[], ); let request_user_input_tool = find_tool(&tools, REQUEST_USER_INPUT_TOOL_NAME); @@ -560,7 +560,7 @@ fn request_permissions_requires_feature_flag() { let (tools, _) = build_specs( &tools_config, /*mcp_tools*/ None, - /*app_tools*/ None, + /*deferred_mcp_tools*/ None, &[], ); assert_lacks_tool_name(&tools, "request_permissions"); @@ -579,7 +579,7 @@ fn request_permissions_requires_feature_flag() { let (tools, _) = build_specs( &tools_config, /*mcp_tools*/ None, - /*app_tools*/ None, + /*deferred_mcp_tools*/ None, &[], ); let request_permissions_tool = find_tool(&tools, "request_permissions"); @@ -607,7 +607,7 @@ fn request_permissions_tool_is_independent_from_additional_permissions() { let (tools, _) = build_specs( &tools_config, /*mcp_tools*/ None, - /*app_tools*/ None, + /*deferred_mcp_tools*/ None, &[], ); @@ -632,7 +632,7 @@ fn js_repl_requires_feature_flag() { let (tools, _) = build_specs( &tools_config, /*mcp_tools*/ None, - /*app_tools*/ None, + /*deferred_mcp_tools*/ None, &[], ); @@ -665,7 +665,7 @@ fn js_repl_enabled_adds_tools() { let (tools, _) = build_specs( &tools_config, /*mcp_tools*/ None, - /*app_tools*/ None, + /*deferred_mcp_tools*/ None, &[], ); @@ -694,7 +694,7 @@ fn image_generation_tools_require_feature_and_supported_model() { let (default_tools, _) = build_specs( &default_tools_config, /*mcp_tools*/ None, - /*app_tools*/ None, + /*deferred_mcp_tools*/ None, &[], ); assert!( @@ -716,7 +716,7 @@ fn image_generation_tools_require_feature_and_supported_model() { let (supported_tools, _) = build_specs( &supported_tools_config, /*mcp_tools*/ None, - /*app_tools*/ None, + /*deferred_mcp_tools*/ None, &[], ); assert_contains_tool_names(&supported_tools, &["image_generation"]); @@ -741,7 +741,7 @@ fn image_generation_tools_require_feature_and_supported_model() { let (tools, _) = build_specs( &tools_config, /*mcp_tools*/ None, - /*app_tools*/ None, + /*deferred_mcp_tools*/ None, &[], ); assert!( @@ -770,7 +770,7 @@ fn web_search_mode_cached_sets_external_web_access_false() { let (tools, _) = build_specs( &tools_config, /*mcp_tools*/ None, - /*app_tools*/ None, + /*deferred_mcp_tools*/ None, &[], ); @@ -805,7 +805,7 @@ fn web_search_mode_live_sets_external_web_access_true() { let (tools, _) = build_specs( &tools_config, /*mcp_tools*/ None, - /*app_tools*/ None, + /*deferred_mcp_tools*/ None, &[], ); @@ -854,7 +854,7 @@ fn web_search_config_is_forwarded_to_tool_spec() { let (tools, _) = build_specs( &tools_config, /*mcp_tools*/ None, - /*app_tools*/ None, + /*deferred_mcp_tools*/ None, &[], ); @@ -894,7 +894,7 @@ fn web_search_tool_type_text_and_image_sets_search_content_types() { let (tools, _) = build_specs( &tools_config, /*mcp_tools*/ None, - /*app_tools*/ None, + /*deferred_mcp_tools*/ None, &[], ); @@ -928,7 +928,7 @@ fn mcp_resource_tools_are_hidden_without_mcp_servers() { let (tools, _) = build_specs( &tools_config, /*mcp_tools*/ None, - /*app_tools*/ None, + /*deferred_mcp_tools*/ None, &[], ); @@ -958,7 +958,7 @@ fn mcp_resource_tools_are_included_when_mcp_servers_are_present() { let (tools, _) = build_specs( &tools_config, Some(HashMap::new()), - /*app_tools*/ None, + /*deferred_mcp_tools*/ None, &[], ); @@ -991,7 +991,7 @@ fn test_parallel_support_flags() { let (tools, _) = build_specs( &tools_config, /*mcp_tools*/ None, - /*app_tools*/ None, + /*deferred_mcp_tools*/ None, &[], ); @@ -1017,7 +1017,7 @@ fn test_test_model_info_includes_sync_tool() { let (tools, _) = build_specs( &tools_config, /*mcp_tools*/ None, - /*app_tools*/ None, + /*deferred_mcp_tools*/ None, &[], ); @@ -1064,7 +1064,7 @@ fn test_build_specs_mcp_tools_converted() { }), ), )])), - /*app_tools*/ None, + /*deferred_mcp_tools*/ None, &[], ); @@ -1146,7 +1146,12 @@ fn test_build_specs_mcp_tools_sorted_by_name() { ), ]); - let (tools, _) = build_specs(&tools_config, Some(tools_map), /*app_tools*/ None, &[]); + let (tools, _) = build_specs( + &tools_config, + Some(tools_map), + /*deferred_mcp_tools*/ None, + &[], + ); let mcp_names: Vec<_> = tools .iter() @@ -1162,7 +1167,7 @@ fn test_build_specs_mcp_tools_sorted_by_name() { } #[test] -fn search_tool_description_lists_each_codex_apps_connector_once() { +fn search_tool_description_lists_each_mcp_source_once() { let model_info = search_capable_model_info(); let mut features = Features::with_defaults(); features.enable(Feature::Apps); @@ -1178,7 +1183,7 @@ fn search_tool_description_lists_each_codex_apps_connector_once() { windows_sandbox_level: WindowsSandboxLevel::Disabled, }); - let (tools, _) = build_specs( + let (tools, handlers) = build_specs( &tools_config, Some(HashMap::from([ ( @@ -1195,29 +1200,36 @@ fn search_tool_description_lists_each_codex_apps_connector_once() { ), ])), Some(vec![ - app_tool( + deferred_mcp_tool( + "mcp__codex_apps__calendar_create_event", "_create_event", "mcp__codex_apps__calendar", CODEX_APPS_MCP_SERVER_NAME, Some("Calendar"), Some("Plan events and manage your calendar."), ), - app_tool( + deferred_mcp_tool( + "mcp__codex_apps__calendar_list_events", "_list_events", "mcp__codex_apps__calendar", CODEX_APPS_MCP_SERVER_NAME, Some("Calendar"), Some("Plan events and manage your calendar."), ), - app_tool( + deferred_mcp_tool( + "mcp__codex_apps__gmail_search_threads", "_search_threads", "mcp__codex_apps__gmail", CODEX_APPS_MCP_SERVER_NAME, Some("Gmail"), Some("Find and summarize email threads."), ), - app_tool( - "echo", "rmcp", "rmcp", /*connector_name*/ None, + deferred_mcp_tool( + "mcp__rmcp__echo", + "echo", + "rmcp", + "rmcp", + /*connector_name*/ None, /*connector_description*/ None, ), ]), @@ -1237,14 +1249,25 @@ fn search_tool_description_lists_each_codex_apps_connector_once() { .count(), 1 ); + assert!(description.contains("- rmcp")); assert!(!description.contains("mcp__rmcp__echo")); + + assert!(handlers.contains(&ToolHandlerSpec { + name: "mcp__codex_apps__calendar:_create_event".to_string(), + kind: ToolHandlerKind::Mcp, + })); + assert!(handlers.contains(&ToolHandlerSpec { + name: "mcp__rmcp__:echo".to_string(), + kind: ToolHandlerKind::Mcp, + })); } #[test] fn search_tool_requires_model_capability_and_feature_flag() { let model_info = search_capable_model_info(); - let app_tools = Some(vec![app_tool( - "calendar_create_event", + let deferred_mcp_tools = Some(vec![deferred_mcp_tool( + "mcp__codex_apps__calendar_create_event", + "_create_event", "mcp__codex_apps__calendar", CODEX_APPS_MCP_SERVER_NAME, Some("Calendar"), @@ -1268,7 +1291,7 @@ fn search_tool_requires_model_capability_and_feature_flag() { let (tools, _) = build_specs( &tools_config, /*mcp_tools*/ None, - app_tools.clone(), + deferred_mcp_tools.clone(), &[], ); assert_lacks_tool_name(&tools, TOOL_SEARCH_TOOL_NAME); @@ -1285,7 +1308,7 @@ fn search_tool_requires_model_capability_and_feature_flag() { let (tools, _) = build_specs( &tools_config, /*mcp_tools*/ None, - app_tools.clone(), + deferred_mcp_tools.clone(), &[], ); assert_lacks_tool_name(&tools, TOOL_SEARCH_TOOL_NAME); @@ -1301,7 +1324,12 @@ fn search_tool_requires_model_capability_and_feature_flag() { sandbox_policy: &SandboxPolicy::DangerFullAccess, windows_sandbox_level: WindowsSandboxLevel::Disabled, }); - let (tools, _) = build_specs(&tools_config, /*mcp_tools*/ None, app_tools, &[]); + let (tools, _) = build_specs( + &tools_config, + /*mcp_tools*/ None, + deferred_mcp_tools, + &[], + ); assert_contains_tool_names(&tools, &[TOOL_SEARCH_TOOL_NAME]); } @@ -1326,7 +1354,7 @@ fn tool_suggest_is_not_registered_without_feature_flag() { let (tools, _) = build_specs_with_discoverable_tools( &tools_config, /*mcp_tools*/ None, - /*app_tools*/ None, + /*deferred_mcp_tools*/ None, Some(vec![discoverable_connector( "connector_2128aebfecb84f64a069897515042a44", "Google Calendar", @@ -1365,7 +1393,7 @@ fn tool_suggest_can_be_registered_without_search_tool() { let (tools, _) = build_specs_with_discoverable_tools( &tools_config, /*mcp_tools*/ None, - /*app_tools*/ None, + /*deferred_mcp_tools*/ None, Some(vec![discoverable_connector( "connector_2128aebfecb84f64a069897515042a44", "Google Calendar", @@ -1432,7 +1460,7 @@ fn tool_suggest_description_lists_discoverable_tools() { let (tools, _) = build_specs_with_discoverable_tools( &tools_config, /*mcp_tools*/ None, - /*app_tools*/ None, + /*deferred_mcp_tools*/ None, Some(discoverable_tools), &[], ); @@ -1527,7 +1555,7 @@ fn code_mode_augments_mcp_tool_descriptions_with_namespaced_sample() { }), ), )])), - /*app_tools*/ None, + /*deferred_mcp_tools*/ None, &[], ); @@ -1563,7 +1591,7 @@ fn code_mode_augments_builtin_tool_descriptions_with_typed_sample() { let (tools, _) = build_specs( &tools_config, /*mcp_tools*/ None, - /*app_tools*/ None, + /*deferred_mcp_tools*/ None, &[], ); let ToolSpec::Function(ResponsesApiTool { description, .. }) = @@ -1598,7 +1626,7 @@ fn code_mode_only_exec_description_includes_full_nested_tool_details() { let (tools, _) = build_specs( &tools_config, /*mcp_tools*/ None, - /*app_tools*/ None, + /*deferred_mcp_tools*/ None, &[], ); let ToolSpec::Freeform(FreeformTool { description, .. }) = &find_tool(&tools, "exec").spec @@ -1634,7 +1662,7 @@ fn code_mode_exec_description_omits_nested_tool_details_when_not_code_mode_only( let (tools, _) = build_specs( &tools_config, /*mcp_tools*/ None, - /*app_tools*/ None, + /*deferred_mcp_tools*/ None, &[], ); let ToolSpec::Freeform(FreeformTool { description, .. }) = &find_tool(&tools, "exec").spec @@ -1694,13 +1722,13 @@ fn search_capable_model_info() -> ModelInfo { fn build_specs<'a>( config: &ToolsConfig, mcp_tools: Option>, - app_tools: Option>>, + deferred_mcp_tools: Option>>, dynamic_tools: &[DynamicToolSpec], ) -> (Vec, Vec) { build_specs_with_discoverable_tools( config, mcp_tools, - app_tools, + deferred_mcp_tools, /*discoverable_tools*/ None, dynamic_tools, ) @@ -1709,7 +1737,7 @@ fn build_specs<'a>( fn build_specs_with_discoverable_tools<'a>( config: &ToolsConfig, mcp_tools: Option>, - app_tools: Option>>, + deferred_mcp_tools: Option>>, discoverable_tools: Option>, dynamic_tools: &[DynamicToolSpec], ) -> (Vec, Vec) { @@ -1717,12 +1745,11 @@ fn build_specs_with_discoverable_tools<'a>( config, ToolRegistryPlanParams { mcp_tools: mcp_tools.as_ref(), - app_tools: app_tools.as_deref(), + deferred_mcp_tools: deferred_mcp_tools.as_deref(), discoverable_tools: discoverable_tools.as_deref(), dynamic_tools, default_agent_type_description: DEFAULT_AGENT_TYPE_DESCRIPTION, wait_agent_timeouts: wait_agent_timeout_options(), - codex_apps_mcp_server_name: CODEX_APPS_MCP_SERVER_NAME, }, ); (plan.specs, plan.handlers) @@ -1761,14 +1788,16 @@ fn discoverable_connector(id: &str, name: &str, description: &str) -> Discoverab })) } -fn app_tool<'a>( +fn deferred_mcp_tool<'a>( + qualified_tool_name: &'a str, tool_name: &'a str, tool_namespace: &'a str, server_name: &'a str, connector_name: Option<&'a str>, connector_description: Option<&'a str>, -) -> ToolRegistryPlanAppTool<'a> { - ToolRegistryPlanAppTool { +) -> ToolRegistryPlanDeferredTool<'a> { + ToolRegistryPlanDeferredTool { + qualified_tool_name, tool_name, tool_namespace, server_name, diff --git a/codex-rs/tools/src/tool_registry_plan_types.rs b/codex-rs/tools/src/tool_registry_plan_types.rs index d15cf15d5ab9..6d5908476e36 100644 --- a/codex-rs/tools/src/tool_registry_plan_types.rs +++ b/codex-rs/tools/src/tool_registry_plan_types.rs @@ -58,16 +58,16 @@ pub struct ToolRegistryPlan { #[derive(Debug, Clone, Copy)] pub struct ToolRegistryPlanParams<'a> { pub mcp_tools: Option<&'a HashMap>, - pub app_tools: Option<&'a [ToolRegistryPlanAppTool<'a>]>, + pub deferred_mcp_tools: Option<&'a [ToolRegistryPlanDeferredTool<'a>]>, pub discoverable_tools: Option<&'a [DiscoverableTool]>, pub dynamic_tools: &'a [DynamicToolSpec], pub default_agent_type_description: &'a str, pub wait_agent_timeouts: WaitAgentTimeoutOptions, - pub codex_apps_mcp_server_name: &'a str, } #[derive(Debug, Clone, Copy)] -pub struct ToolRegistryPlanAppTool<'a> { +pub struct ToolRegistryPlanDeferredTool<'a> { + pub qualified_tool_name: &'a str, pub tool_name: &'a str, pub tool_namespace: &'a str, pub server_name: &'a str, From db20b96727d9dd1d5a862da6a014f41a8c6b3c69 Mon Sep 17 00:00:00 2001 From: Matthew Zeng Date: Mon, 6 Apr 2026 19:19:35 -0700 Subject: [PATCH 2/9] update --- .../codex-mcp/src/mcp_connection_manager.rs | 20 ++++----- .../src/mcp_connection_manager_tests.rs | 6 +-- codex-rs/core/src/codex_tests.rs | 2 +- .../core/src/tools/handlers/tool_search.rs | 3 +- codex-rs/core/src/tools/spec.rs | 5 +-- codex-rs/core/src/tools/spec_tests.rs | 2 +- codex-rs/tools/src/lib.rs | 1 - codex-rs/tools/src/tool_discovery.rs | 43 +------------------ codex-rs/tools/src/tool_discovery_tests.rs | 7 +-- codex-rs/tools/src/tool_registry_plan.rs | 8 +--- .../tools/src/tool_registry_plan_tests.rs | 9 +--- .../tools/src/tool_registry_plan_types.rs | 1 - 12 files changed, 19 insertions(+), 88 deletions(-) diff --git a/codex-rs/codex-mcp/src/mcp_connection_manager.rs b/codex-rs/codex-mcp/src/mcp_connection_manager.rs index 9a995a5c76a1..8dc974ada0f5 100644 --- a/codex-rs/codex-mcp/src/mcp_connection_manager.rs +++ b/codex-rs/codex-mcp/src/mcp_connection_manager.rs @@ -143,14 +143,7 @@ where let mut seen_raw_names = HashSet::new(); let mut qualified_tools = HashMap::new(); for tool in tools { - let qualified_name_raw = if tool.server_name != CODEX_APPS_MCP_SERVER_NAME { - format!( - "mcp{}{}{}{}", - MCP_TOOL_NAME_DELIMITER, tool.server_name, MCP_TOOL_NAME_DELIMITER, tool.tool_name - ) - } else { - format!("{}{}", tool.tool_namespace, tool.tool_name) - }; + let qualified_name_raw = format!("{}{}", tool.tool_namespace, tool.tool_name); if !seen_raw_names.insert(qualified_name_raw.clone()) { warn!("skipping duplicated tool {}", qualified_name_raw); continue; @@ -1255,9 +1248,9 @@ fn normalize_codex_apps_tool_name( } fn normalize_codex_apps_namespace(server_name: &str, connector_name: Option<&str>) -> String { - if server_name != CODEX_APPS_MCP_SERVER_NAME { - server_name.to_string() - } else if let Some(connector_name) = connector_name { + if server_name == CODEX_APPS_MCP_SERVER_NAME + && let Some(connector_name) = connector_name + { format!( "mcp{}{}{}{}", MCP_TOOL_NAME_DELIMITER, @@ -1266,7 +1259,10 @@ fn normalize_codex_apps_namespace(server_name: &str, connector_name: Option<&str sanitize_name(connector_name) ) } else { - server_name.to_string() + format!( + "mcp{}{}{}", + MCP_TOOL_NAME_DELIMITER, server_name, MCP_TOOL_NAME_DELIMITER + ) } } diff --git a/codex-rs/codex-mcp/src/mcp_connection_manager_tests.rs b/codex-rs/codex-mcp/src/mcp_connection_manager_tests.rs index b34a5cc0437d..6af95cc58dca 100644 --- a/codex-rs/codex-mcp/src/mcp_connection_manager_tests.rs +++ b/codex-rs/codex-mcp/src/mcp_connection_manager_tests.rs @@ -10,11 +10,7 @@ fn create_test_tool(server_name: &str, tool_name: &str) -> ToolInfo { ToolInfo { server_name: server_name.to_string(), tool_name: tool_name.to_string(), - tool_namespace: if server_name == CODEX_APPS_MCP_SERVER_NAME { - format!("mcp__{server_name}__") - } else { - server_name.to_string() - }, + tool_namespace: format!("mcp__{server_name}__"), tool: Tool { name: tool_name.to_string().into(), title: None, diff --git a/codex-rs/core/src/codex_tests.rs b/codex-rs/core/src/codex_tests.rs index bd0f6f75657d..acb32d8f4647 100644 --- a/codex-rs/core/src/codex_tests.rs +++ b/codex-rs/core/src/codex_tests.rs @@ -399,7 +399,7 @@ fn make_mcp_tool( .map(|connector_name| format!("mcp__{server_name}__{connector_name}")) .unwrap_or_else(|| server_name.to_string()) } else { - server_name.to_string() + format!("mcp__{server_name}__") }; ToolInfo { diff --git a/codex-rs/core/src/tools/handlers/tool_search.rs b/codex-rs/core/src/tools/handlers/tool_search.rs index c1cb320397de..7edcfeceab79 100644 --- a/codex-rs/core/src/tools/handlers/tool_search.rs +++ b/codex-rs/core/src/tools/handlers/tool_search.rs @@ -80,8 +80,7 @@ impl ToolHandler for ToolSearchHandler { results .into_iter() .filter_map(|result| entries.get(result.document.id)) - .map(|(name, tool)| ToolSearchResultSource { - qualified_tool_name: name.as_str(), + .map(|(_, tool)| ToolSearchResultSource { server_name: tool.server_name.as_str(), tool_namespace: tool.tool_namespace.as_str(), tool_name: tool.tool_name.as_str(), diff --git a/codex-rs/core/src/tools/spec.rs b/codex-rs/core/src/tools/spec.rs index f7c028da2f66..c130e261dad2 100644 --- a/codex-rs/core/src/tools/spec.rs +++ b/codex-rs/core/src/tools/spec.rs @@ -69,9 +69,8 @@ pub(crate) fn build_specs_with_discoverable_tools( let mut builder = ToolRegistryBuilder::new(); let deferred_mcp_tool_sources = deferred_mcp_tools.as_ref().map(|tools| { tools - .iter() - .map(|(qualified_tool_name, tool)| ToolRegistryPlanDeferredTool { - qualified_tool_name: qualified_tool_name.as_str(), + .values() + .map(|tool| ToolRegistryPlanDeferredTool { tool_name: tool.tool_name.as_str(), tool_namespace: tool.tool_namespace.as_str(), server_name: tool.server_name.as_str(), diff --git a/codex-rs/core/src/tools/spec_tests.rs b/codex-rs/core/src/tools/spec_tests.rs index c1971c3c56f6..bc508f13543d 100644 --- a/codex-rs/core/src/tools/spec_tests.rs +++ b/codex-rs/core/src/tools/spec_tests.rs @@ -784,7 +784,7 @@ fn search_tool_registers_namespaced_mcp_tool_aliases() { ToolInfo { server_name: "rmcp".to_string(), tool_name: "echo".to_string(), - tool_namespace: "rmcp".to_string(), + tool_namespace: "mcp__rmcp__".to_string(), tool: mcp_tool("echo", "Echo", serde_json::json!({"type": "object"})), connector_id: None, connector_name: None, diff --git a/codex-rs/tools/src/lib.rs b/codex-rs/tools/src/lib.rs index 5998b08e9110..ea2f20832ebb 100644 --- a/codex-rs/tools/src/lib.rs +++ b/codex-rs/tools/src/lib.rs @@ -109,7 +109,6 @@ pub use tool_discovery::collect_tool_suggest_entries; pub use tool_discovery::create_tool_search_tool; pub use tool_discovery::create_tool_suggest_tool; pub use tool_discovery::filter_tool_suggest_discoverable_tools_for_client; -pub use tool_discovery::tool_search_output_namespace_and_name; pub use tool_registry_plan::build_tool_registry_plan; pub use tool_registry_plan_types::ToolHandlerKind; pub use tool_registry_plan_types::ToolHandlerSpec; diff --git a/codex-rs/tools/src/tool_discovery.rs b/codex-rs/tools/src/tool_discovery.rs index 2f95d5a81256..8f91f4a67a08 100644 --- a/codex-rs/tools/src/tool_discovery.rs +++ b/codex-rs/tools/src/tool_discovery.rs @@ -30,7 +30,6 @@ pub struct ToolSearchSource<'a> { #[derive(Clone, Copy, Debug, PartialEq)] pub struct ToolSearchResultSource<'a> { - pub qualified_tool_name: &'a str, pub server_name: &'a str, pub tool_namespace: &'a str, pub tool_name: &'a str, @@ -212,15 +211,10 @@ pub fn collect_tool_search_output_tools<'a>( let grouped = tool_sources.into_iter().fold( BTreeMap::)>>::new(), |mut grouped, tool| { - let (tool_namespace, tool_name) = tool_search_output_namespace_and_name( - tool.qualified_tool_name, - tool.tool_namespace, - tool.tool_name, - ); grouped - .entry(tool_namespace) + .entry(tool.tool_namespace.to_string()) .or_default() - .push((tool_name, tool)); + .push((tool.tool_name.to_string(), tool)); grouped }, ); @@ -268,39 +262,6 @@ pub fn collect_tool_search_output_tools<'a>( Ok(results) } -pub fn tool_search_output_namespace_and_name( - qualified_tool_name: &str, - tool_namespace: &str, - tool_name: &str, -) -> (String, String) { - if let Some(child_name) = qualified_tool_name.strip_prefix(tool_namespace) - && child_name == tool_name - { - return (tool_namespace.to_string(), tool_name.to_string()); - } - - if let Some((namespace, child_name)) = - split_qualified_mcp_tool_name_for_search(qualified_tool_name) - { - return (namespace.to_string(), child_name.to_string()); - } - - (tool_namespace.to_string(), tool_name.to_string()) -} - -fn split_qualified_mcp_tool_name_for_search(qualified_tool_name: &str) -> Option<(&str, &str)> { - let stripped = qualified_tool_name.strip_prefix("mcp__")?; - let server_name_len = stripped.find("__")?; - let child_name_start = server_name_len + "__".len(); - let child_name = stripped.get(child_name_start..)?; - if child_name.is_empty() { - return None; - } - - let namespace_len = "mcp__".len() + server_name_len + "__".len(); - Some((&qualified_tool_name[..namespace_len], child_name)) -} - pub fn collect_tool_search_source_infos<'a>( searchable_tools: impl IntoIterator>, ) -> Vec { diff --git a/codex-rs/tools/src/tool_discovery_tests.rs b/codex-rs/tools/src/tool_discovery_tests.rs index 2dd4e44b6570..ef39d87c2fa7 100644 --- a/codex-rs/tools/src/tool_discovery_tests.rs +++ b/codex-rs/tools/src/tool_discovery_tests.rs @@ -164,7 +164,6 @@ fn collect_tool_search_output_tools_groups_results_by_namespace() { let tools = collect_tool_search_output_tools([ ToolSearchResultSource { - qualified_tool_name: "mcp__codex_apps__calendar_create_event", server_name: "codex_apps", tool_namespace: "mcp__codex_apps__calendar", tool_name: "_create_event", @@ -173,7 +172,6 @@ fn collect_tool_search_output_tools_groups_results_by_namespace() { connector_description: Some("Plan events"), }, ToolSearchResultSource { - qualified_tool_name: "mcp__codex_apps__gmail_read_email", server_name: "codex_apps", tool_namespace: "mcp__codex_apps__gmail", tool_name: "_read_email", @@ -182,7 +180,6 @@ fn collect_tool_search_output_tools_groups_results_by_namespace() { connector_description: Some("Read mail"), }, ToolSearchResultSource { - qualified_tool_name: "mcp__codex_apps__calendar_list_events", server_name: "codex_apps", tool_namespace: "mcp__codex_apps__calendar", tool_name: "_list_events", @@ -191,9 +188,8 @@ fn collect_tool_search_output_tools_groups_results_by_namespace() { connector_description: Some("Plan events"), }, ToolSearchResultSource { - qualified_tool_name: "mcp__docs__search", server_name: "docs", - tool_namespace: "docs", + tool_namespace: "mcp__docs__", tool_name: "search", tool: &docs_search, connector_name: None, @@ -276,7 +272,6 @@ fn collect_tool_search_output_tools_falls_back_to_connector_name_description() { let gmail_batch_read_email = mcp_tool("gmail-batch-read-email", "Read multiple emails."); let tools = collect_tool_search_output_tools([ToolSearchResultSource { - qualified_tool_name: "mcp__codex_apps__gmail_batch_read_email", server_name: "codex_apps", tool_namespace: "mcp__codex_apps__gmail", tool_name: "_batch_read_email", diff --git a/codex-rs/tools/src/tool_registry_plan.rs b/codex-rs/tools/src/tool_registry_plan.rs index 9d5792762310..15ee19c88a48 100644 --- a/codex-rs/tools/src/tool_registry_plan.rs +++ b/codex-rs/tools/src/tool_registry_plan.rs @@ -58,7 +58,6 @@ use crate::mcp_tool_to_responses_api_tool; use crate::request_permissions_tool_description; use crate::request_user_input_tool_description; use crate::tool_registry_plan_types::agent_type_description; -use crate::tool_search_output_namespace_and_name; use codex_protocol::openai_models::ApplyPatchToolType; use codex_protocol::openai_models::ConfigShellToolType; use rmcp::model::Tool as McpTool; @@ -247,13 +246,8 @@ pub fn build_tool_registry_plan( plan.register_handler(TOOL_SEARCH_TOOL_NAME, ToolHandlerKind::ToolSearch); for tool in deferred_mcp_tools { - let (tool_namespace, tool_name) = tool_search_output_namespace_and_name( - tool.qualified_tool_name, - tool.tool_namespace, - tool.tool_name, - ); plan.register_handler( - format!("{tool_namespace}:{tool_name}"), + format!("{}:{}", tool.tool_namespace, tool.tool_name), ToolHandlerKind::Mcp, ); } diff --git a/codex-rs/tools/src/tool_registry_plan_tests.rs b/codex-rs/tools/src/tool_registry_plan_tests.rs index d136209df862..20cdd856ec68 100644 --- a/codex-rs/tools/src/tool_registry_plan_tests.rs +++ b/codex-rs/tools/src/tool_registry_plan_tests.rs @@ -1201,7 +1201,6 @@ fn search_tool_description_lists_each_mcp_source_once() { ])), Some(vec![ deferred_mcp_tool( - "mcp__codex_apps__calendar_create_event", "_create_event", "mcp__codex_apps__calendar", CODEX_APPS_MCP_SERVER_NAME, @@ -1209,7 +1208,6 @@ fn search_tool_description_lists_each_mcp_source_once() { Some("Plan events and manage your calendar."), ), deferred_mcp_tool( - "mcp__codex_apps__calendar_list_events", "_list_events", "mcp__codex_apps__calendar", CODEX_APPS_MCP_SERVER_NAME, @@ -1217,7 +1215,6 @@ fn search_tool_description_lists_each_mcp_source_once() { Some("Plan events and manage your calendar."), ), deferred_mcp_tool( - "mcp__codex_apps__gmail_search_threads", "_search_threads", "mcp__codex_apps__gmail", CODEX_APPS_MCP_SERVER_NAME, @@ -1225,9 +1222,8 @@ fn search_tool_description_lists_each_mcp_source_once() { Some("Find and summarize email threads."), ), deferred_mcp_tool( - "mcp__rmcp__echo", "echo", - "rmcp", + "mcp__rmcp__", "rmcp", /*connector_name*/ None, /*connector_description*/ None, @@ -1266,7 +1262,6 @@ fn search_tool_description_lists_each_mcp_source_once() { fn search_tool_requires_model_capability_and_feature_flag() { let model_info = search_capable_model_info(); let deferred_mcp_tools = Some(vec![deferred_mcp_tool( - "mcp__codex_apps__calendar_create_event", "_create_event", "mcp__codex_apps__calendar", CODEX_APPS_MCP_SERVER_NAME, @@ -1789,7 +1784,6 @@ fn discoverable_connector(id: &str, name: &str, description: &str) -> Discoverab } fn deferred_mcp_tool<'a>( - qualified_tool_name: &'a str, tool_name: &'a str, tool_namespace: &'a str, server_name: &'a str, @@ -1797,7 +1791,6 @@ fn deferred_mcp_tool<'a>( connector_description: Option<&'a str>, ) -> ToolRegistryPlanDeferredTool<'a> { ToolRegistryPlanDeferredTool { - qualified_tool_name, tool_name, tool_namespace, server_name, diff --git a/codex-rs/tools/src/tool_registry_plan_types.rs b/codex-rs/tools/src/tool_registry_plan_types.rs index 6d5908476e36..b4219e1c9c50 100644 --- a/codex-rs/tools/src/tool_registry_plan_types.rs +++ b/codex-rs/tools/src/tool_registry_plan_types.rs @@ -67,7 +67,6 @@ pub struct ToolRegistryPlanParams<'a> { #[derive(Debug, Clone, Copy)] pub struct ToolRegistryPlanDeferredTool<'a> { - pub qualified_tool_name: &'a str, pub tool_name: &'a str, pub tool_namespace: &'a str, pub server_name: &'a str, From 6aa78918886475d58ac5dbba519c23925ce00a24 Mon Sep 17 00:00:00 2001 From: Matthew Zeng Date: Tue, 7 Apr 2026 00:13:15 -0700 Subject: [PATCH 3/9] update --- .../app-server/src/codex_message_processor.rs | 69 ++---- .../tests/suite/v2/mcp_server_status.rs | 18 +- codex-rs/codex-mcp/src/lib.rs | 1 + codex-rs/codex-mcp/src/mcp/mod.rs | 184 ++++++++++++---- .../codex-mcp/src/mcp_connection_manager.rs | 94 +++------ .../src/mcp_connection_manager_tests.rs | 124 ++++++++--- codex-rs/codex-mcp/src/mcp_tool_names.rs | 197 ++++++++++++++++++ codex-rs/core/src/codex_tests.rs | 4 +- codex-rs/core/src/connectors_tests.rs | 12 +- .../core/src/tools/handlers/tool_search.rs | 7 +- codex-rs/core/src/tools/spec.rs | 4 +- codex-rs/core/src/tools/spec_tests.rs | 16 +- 12 files changed, 524 insertions(+), 206 deletions(-) create mode 100644 codex-rs/codex-mcp/src/mcp_tool_names.rs diff --git a/codex-rs/app-server/src/codex_message_processor.rs b/codex-rs/app-server/src/codex_message_processor.rs index 86c2218bc8d7..6b669ad6ebbb 100644 --- a/codex-rs/app-server/src/codex_message_processor.rs +++ b/codex-rs/app-server/src/codex_message_processor.rs @@ -245,11 +245,11 @@ use codex_login::default_client::set_default_client_residency_requirement; use codex_login::login_with_api_key; use codex_login::request_device_code; use codex_login::run_login_server; +use codex_mcp::mcp::McpServerStatusSnapshot; use codex_mcp::mcp::auth::discover_supported_scopes; use codex_mcp::mcp::auth::resolve_oauth_scopes; -use codex_mcp::mcp::collect_mcp_snapshot; +use codex_mcp::mcp::collect_mcp_server_status_snapshot; use codex_mcp::mcp::effective_mcp_servers; -use codex_mcp::mcp::qualified_mcp_tool_name_prefix; use codex_models_manager::collaboration_mode_presets::CollaborationModesConfig; use codex_protocol::ThreadId; use codex_protocol::config_types::CollaborationMode; @@ -5190,54 +5190,20 @@ impl CodexMessageProcessor { mcp_config: codex_mcp::mcp::McpConfig, auth: Option, ) { - let snapshot = collect_mcp_snapshot( + let snapshot = collect_mcp_server_status_snapshot( &mcp_config, auth.as_ref(), request_id.request_id.to_string(), ) .await; - // Rebuild the tool list per original server name instead of using - // `group_tools_by_server()`: qualified tool names are sanitized for the - // Responses API, so a config key like `some-server` is encoded as the - // `mcp__some_server__` prefix. Matching with the original server name's - // sanitized prefix preserves `/mcp` output for hyphenated names. let effective_servers = effective_mcp_servers(&mcp_config, auth.as_ref()); - let mut sanitized_prefix_counts = HashMap::::new(); - for name in effective_servers.keys() { - let prefix = qualified_mcp_tool_name_prefix(name); - *sanitized_prefix_counts.entry(prefix).or_default() += 1; - } - let tools_by_server = effective_servers - .keys() - .map(|name| { - let prefix = qualified_mcp_tool_name_prefix(name); - // If multiple server names normalize to the same prefix, the - // qualified tool namespace is ambiguous (for example - // `some-server` and `some_server` both become - // `mcp__some_server__`). In that case, avoid attributing the - // same tools to multiple servers. - let tools = if sanitized_prefix_counts - .get(&prefix) - .copied() - .unwrap_or_default() - == 1 - { - snapshot - .tools - .iter() - .filter_map(|(qualified_name, tool)| { - qualified_name - .strip_prefix(&prefix) - .map(|tool_name| (tool_name.to_string(), tool.clone())) - }) - .collect::>() - } else { - HashMap::new() - }; - (name.clone(), tools) - }) - .collect::>(); + let McpServerStatusSnapshot { + tools_by_server, + resources, + resource_templates, + auth_statuses, + } = snapshot; let mut server_names: Vec = config .mcp_servers @@ -5247,9 +5213,9 @@ impl CodexMessageProcessor { // effective runtime config even when they are not user-declared in // `config.mcp_servers`. .chain(effective_servers.keys().cloned()) - .chain(snapshot.auth_statuses.keys().cloned()) - .chain(snapshot.resources.keys().cloned()) - .chain(snapshot.resource_templates.keys().cloned()) + .chain(auth_statuses.keys().cloned()) + .chain(resources.keys().cloned()) + .chain(resource_templates.keys().cloned()) .collect(); server_names.sort(); server_names.dedup(); @@ -5290,14 +5256,9 @@ impl CodexMessageProcessor { .map(|name| McpServerStatus { name: name.clone(), tools: tools_by_server.get(name).cloned().unwrap_or_default(), - resources: snapshot.resources.get(name).cloned().unwrap_or_default(), - resource_templates: snapshot - .resource_templates - .get(name) - .cloned() - .unwrap_or_default(), - auth_status: snapshot - .auth_statuses + resources: resources.get(name).cloned().unwrap_or_default(), + resource_templates: resource_templates.get(name).cloned().unwrap_or_default(), + auth_status: auth_statuses .get(name) .cloned() .unwrap_or(CoreMcpAuthStatus::Unsupported) diff --git a/codex-rs/app-server/tests/suite/v2/mcp_server_status.rs b/codex-rs/app-server/tests/suite/v2/mcp_server_status.rs index 8b3844b521c4..1b1d580530ef 100644 --- a/codex-rs/app-server/tests/suite/v2/mcp_server_status.rs +++ b/codex-rs/app-server/tests/suite/v2/mcp_server_status.rs @@ -128,8 +128,7 @@ impl ServerHandler for McpStatusServer { } #[tokio::test] -async fn mcp_server_status_list_does_not_duplicate_tools_for_sanitized_name_collisions() --> Result<()> { +async fn mcp_server_status_list_keeps_tools_for_sanitized_name_collisions() -> Result<()> { let server = create_mock_responses_server_sequence_unchecked(Vec::new()).await; let (dash_server_url, dash_server_handle) = start_mcp_server("dash_lookup").await?; let (underscore_server_url, underscore_server_handle) = @@ -179,11 +178,22 @@ url = "{underscore_server_url}/mcp" let status_tools = response .data .iter() - .map(|status| (status.name.as_str(), status.tools.keys().count())) + .map(|status| { + ( + status.name.as_str(), + status.tools.keys().cloned().collect::>(), + ) + }) .collect::>(); assert_eq!( status_tools, - BTreeMap::from([("some-server", 0), ("some_server", 0)]) + BTreeMap::from([ + ("some-server", BTreeSet::from(["dash_lookup".to_string()])), + ( + "some_server", + BTreeSet::from(["underscore_lookup".to_string()]) + ) + ]) ); dash_server_handle.abort(); diff --git a/codex-rs/codex-mcp/src/lib.rs b/codex-rs/codex-mcp/src/lib.rs index 149578c967c3..c263ff48e1f5 100644 --- a/codex-rs/codex-mcp/src/lib.rs +++ b/codex-rs/codex-mcp/src/lib.rs @@ -1,2 +1,3 @@ pub mod mcp; pub mod mcp_connection_manager; +pub(crate) mod mcp_tool_names; diff --git a/codex-rs/codex-mcp/src/mcp/mod.rs b/codex-rs/codex-mcp/src/mcp/mod.rs index b843bbadc02c..0e9cb35940a8 100644 --- a/codex-rs/codex-mcp/src/mcp/mod.rs +++ b/codex-rs/codex-mcp/src/mcp/mod.rs @@ -18,6 +18,7 @@ use codex_protocol::mcp::Resource; use codex_protocol::mcp::ResourceTemplate; use codex_protocol::mcp::Tool; use codex_protocol::protocol::AskForApproval; +use codex_protocol::protocol::McpAuthStatus; use codex_protocol::protocol::McpListToolsResponseEvent; use codex_protocol::protocol::SandboxPolicy; use codex_rmcp_client::OAuthCredentialsStoreMode; @@ -34,9 +35,8 @@ const MCP_TOOL_NAME_DELIMITER: &str = "__"; pub 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 `_`. +/// Model-visible MCP tool names must also be valid code-mode identifiers, so +/// MCP server/tool names are sanitized to ASCII alphanumeric and `_`. pub(crate) fn sanitize_responses_api_tool_name(name: &str) -> String { let mut sanitized = String::with_capacity(name.len()); for c in name.chars() { @@ -331,6 +331,68 @@ pub async fn collect_mcp_snapshot( snapshot } +#[derive(Debug, Clone)] +pub struct McpServerStatusSnapshot { + pub tools_by_server: HashMap>, + pub resources: HashMap>, + pub resource_templates: HashMap>, + pub auth_statuses: HashMap, +} + +pub async fn collect_mcp_server_status_snapshot( + config: &McpConfig, + auth: Option<&CodexAuth>, + submit_id: String, +) -> McpServerStatusSnapshot { + let mcp_servers = effective_mcp_servers(config, auth); + let tool_plugin_provenance = tool_plugin_provenance(config); + if mcp_servers.is_empty() { + return McpServerStatusSnapshot { + tools_by_server: HashMap::new(), + resources: HashMap::new(), + resource_templates: HashMap::new(), + auth_statuses: HashMap::new(), + }; + } + + let auth_status_entries = + compute_auth_statuses(mcp_servers.iter(), config.mcp_oauth_credentials_store_mode).await; + + let (tx_event, rx_event) = unbounded(); + drop(rx_event); + + let sandbox_state = SandboxState { + sandbox_policy: SandboxPolicy::new_read_only_policy(), + codex_linux_sandbox_exe: config.codex_linux_sandbox_exe.clone(), + sandbox_cwd: env::current_dir().unwrap_or_else(|_| PathBuf::from("/")), + use_legacy_landlock: config.use_legacy_landlock, + }; + + let (mcp_connection_manager, cancel_token) = McpConnectionManager::new( + &mcp_servers, + config.mcp_oauth_credentials_store_mode, + auth_status_entries.clone(), + &config.approval_policy, + submit_id, + tx_event, + sandbox_state, + config.codex_home.clone(), + codex_apps_tools_cache_key(auth), + tool_plugin_provenance, + ) + .await; + + let snapshot = collect_mcp_server_status_snapshot_from_manager( + &mcp_connection_manager, + auth_status_entries, + ) + .await; + + cancel_token.cancel(); + + snapshot +} + pub fn split_qualified_tool_name(qualified_name: &str) -> Option<(String, String)> { let mut parts = qualified_name.split(MCP_TOOL_NAME_DELIMITER); let prefix = parts.next()?; @@ -360,39 +422,35 @@ pub fn group_tools_by_server( grouped } -pub async fn collect_mcp_snapshot_from_manager( - mcp_connection_manager: &McpConnectionManager, - auth_status_entries: HashMap, -) -> McpListToolsResponseEvent { - let (tools, resources, resource_templates) = tokio::join!( - mcp_connection_manager.list_all_tools(), - mcp_connection_manager.list_all_resources(), - mcp_connection_manager.list_all_resource_templates(), - ); - - let auth_statuses = auth_status_entries - .iter() - .map(|(name, entry)| (name.clone(), entry.auth_status)) - .collect::>(); - - let tools = tools - .into_iter() - .filter_map(|(name, tool)| match serde_json::to_value(tool.tool) { - Ok(value) => match Tool::from_mcp_value(value) { - Ok(tool) => Some((name, tool)), - Err(err) => { - tracing::warn!("Failed to convert MCP tool '{name}': {err}"); - None - } - }, +fn protocol_tool_from_rmcp_tool(name: &str, tool: &rmcp::model::Tool) -> Option { + match serde_json::to_value(tool) { + Ok(value) => match Tool::from_mcp_value(value) { + Ok(tool) => Some(tool), Err(err) => { - tracing::warn!("Failed to serialize MCP tool '{name}': {err}"); + tracing::warn!("Failed to convert MCP tool '{name}': {err}"); None } - }) - .collect::>(); + }, + Err(err) => { + tracing::warn!("Failed to serialize MCP tool '{name}': {err}"); + None + } + } +} - let resources = resources +fn auth_statuses_from_entries( + auth_status_entries: &HashMap, +) -> HashMap { + auth_status_entries + .iter() + .map(|(name, entry)| (name.clone(), entry.auth_status)) + .collect::>() +} + +fn convert_mcp_resources( + resources: HashMap>, +) -> HashMap> { + resources .into_iter() .map(|(name, resources)| { let resources = resources @@ -425,9 +483,13 @@ pub async fn collect_mcp_snapshot_from_manager( .collect::>(); (name, resources) }) - .collect::>(); + .collect::>() +} - let resource_templates = resource_templates +fn convert_mcp_resource_templates( + resource_templates: HashMap>, +) -> HashMap> { + resource_templates .into_iter() .map(|(name, templates)| { let templates = templates @@ -461,13 +523,61 @@ pub async fn collect_mcp_snapshot_from_manager( .collect::>(); (name, templates) }) + .collect::>() +} + +async fn collect_mcp_server_status_snapshot_from_manager( + mcp_connection_manager: &McpConnectionManager, + auth_status_entries: HashMap, +) -> McpServerStatusSnapshot { + let (tools, resources, resource_templates) = tokio::join!( + mcp_connection_manager.list_all_tools(), + mcp_connection_manager.list_all_resources(), + mcp_connection_manager.list_all_resource_templates(), + ); + + let mut tools_by_server = HashMap::>::new(); + for (_qualified_name, 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; + }; + tools_by_server + .entry(tool_info.server_name) + .or_default() + .insert(raw_tool_name, tool); + } + + McpServerStatusSnapshot { + tools_by_server, + resources: convert_mcp_resources(resources), + resource_templates: convert_mcp_resource_templates(resource_templates), + auth_statuses: auth_statuses_from_entries(&auth_status_entries), + } +} + +pub async fn collect_mcp_snapshot_from_manager( + mcp_connection_manager: &McpConnectionManager, + auth_status_entries: HashMap, +) -> McpListToolsResponseEvent { + let (tools, resources, resource_templates) = tokio::join!( + mcp_connection_manager.list_all_tools(), + mcp_connection_manager.list_all_resources(), + mcp_connection_manager.list_all_resource_templates(), + ); + + let tools = tools + .into_iter() + .filter_map(|(name, tool)| { + protocol_tool_from_rmcp_tool(&name, &tool.tool).map(|tool| (name, tool)) + }) .collect::>(); McpListToolsResponseEvent { tools, - resources, - resource_templates, - auth_statuses, + resources: convert_mcp_resources(resources), + resource_templates: convert_mcp_resource_templates(resource_templates), + auth_statuses: auth_statuses_from_entries(&auth_status_entries), } } diff --git a/codex-rs/codex-mcp/src/mcp_connection_manager.rs b/codex-rs/codex-mcp/src/mcp_connection_manager.rs index 8dc974ada0f5..15fe4884a45b 100644 --- a/codex-rs/codex-mcp/src/mcp_connection_manager.rs +++ b/codex-rs/codex-mcp/src/mcp_connection_manager.rs @@ -3,8 +3,8 @@ //! The [`McpConnectionManager`] owns one [`codex_rmcp_client::RmcpClient`] per //! configured server (keyed by the *server name*). It offers convenience //! helpers to query the available tools across *all* servers and returns them -//! in a single aggregated map using the fully-qualified tool name -//! `""` as the key. +//! in a single aggregated map using the model-visible fully-qualified tool name +//! as the key. use std::borrow::Cow; use std::collections::HashMap; @@ -25,8 +25,8 @@ use crate::mcp::ToolPluginProvenance; use crate::mcp::auth::McpAuthStatusEntry; use crate::mcp::configured_mcp_servers; use crate::mcp::effective_mcp_servers; -use crate::mcp::sanitize_responses_api_tool_name; use crate::mcp::tool_plugin_provenance; +pub(crate) use crate::mcp_tool_names::qualify_tools; use anyhow::Context; use anyhow::Result; use anyhow::anyhow; @@ -89,13 +89,8 @@ use codex_login::CodexAuth; use codex_utils_plugins::mcp_connector::is_connector_id_allowed; use codex_utils_plugins::mcp_connector::sanitize_name; -/// Delimiter used to separate the server name from the tool name in a fully -/// qualified tool name. -/// -/// OpenAI requires tool names to conform to `^[a-zA-Z0-9_-]+$`, so we must -/// choose a delimiter from this character set. +/// Delimiter used to separate MCP tool-name parts. const MCP_TOOL_NAME_DELIMITER: &str = "__"; -const MAX_TOOL_NAME_LENGTH: usize = 64; /// Default timeout for initializing MCP server & initially listing tools. pub const DEFAULT_STARTUP_TIMEOUT: Duration = Duration::from_secs(30); @@ -103,7 +98,7 @@ pub const DEFAULT_STARTUP_TIMEOUT: Duration = Duration::from_secs(30); /// Default timeout for individual tool calls. const DEFAULT_TOOL_TIMEOUT: Duration = Duration::from_secs(120); -const CODEX_APPS_TOOLS_CACHE_SCHEMA_VERSION: u8 = 1; +const CODEX_APPS_TOOLS_CACHE_SCHEMA_VERSION: u8 = 2; const CODEX_APPS_TOOLS_CACHE_DIR: &str = "cache/codex_apps_tools"; 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"; @@ -135,50 +130,17 @@ pub fn codex_apps_tools_cache_key(auth: Option<&CodexAuth>) -> CodexAppsToolsCac } } -fn qualify_tools(tools: I) -> HashMap -where - I: IntoIterator, -{ - let mut used_names = HashSet::new(); - let mut seen_raw_names = HashSet::new(); - let mut qualified_tools = HashMap::new(); - for tool in tools { - let qualified_name_raw = format!("{}{}", tool.tool_namespace, tool.tool_name); - if !seen_raw_names.insert(qualified_name_raw.clone()) { - warn!("skipping duplicated tool {}", qualified_name_raw); - continue; - } - - // Start from a "pretty" name (sanitized), then deterministically disambiguate on - // collisions by appending a hash of the *raw* (unsanitized) qualified name. This - // ensures tools like `foo.bar` and `foo_bar` don't collapse to the same key. - let mut qualified_name = sanitize_responses_api_tool_name(&qualified_name_raw); - - // Enforce length constraints early; use the raw name for the hash input so the - // output remains stable even when sanitization changes. - if qualified_name.len() > MAX_TOOL_NAME_LENGTH { - let sha1_str = sha1_hex(&qualified_name_raw); - let prefix_len = MAX_TOOL_NAME_LENGTH - sha1_str.len(); - qualified_name = format!("{}{}", &qualified_name[..prefix_len], sha1_str); - } - - if used_names.contains(&qualified_name) { - warn!("skipping duplicated tool {}", qualified_name); - continue; - } - - used_names.insert(qualified_name.clone()); - qualified_tools.insert(qualified_name, tool); - } - - qualified_tools -} - #[derive(Debug, Clone, Serialize, Deserialize)] pub struct ToolInfo { + /// Raw MCP server name used for routing the tool call. pub server_name: String, - pub tool_name: String, - pub tool_namespace: String, + /// Model-visible tool name used in Responses API tool declarations. + #[serde(rename = "tool_name", alias = "callable_name")] + pub callable_name: String, + /// Model-visible namespace used for deferred tool loading. + #[serde(rename = "tool_namespace", alias = "callable_namespace")] + pub callable_namespace: String, + /// Raw MCP tool definition; `tool.name` is sent back to the MCP server. pub tool: Tool, pub connector_id: Option, pub connector_name: Option, @@ -805,14 +767,14 @@ impl McpConnectionManager { /// fully-qualified name for the tool. #[instrument(level = "trace", skip_all)] pub async fn list_all_tools(&self) -> HashMap { - let mut tools = HashMap::new(); + let mut tools = Vec::new(); for managed_client in self.clients.values() { let Some(server_tools) = managed_client.listed_tools().await else { continue; }; - tools.extend(qualify_tools(server_tools)); + tools.extend(server_tools); } - tools + qualify_tools(tools) } /// Force-refresh codex apps tools by bypassing the in-process cache. @@ -1212,7 +1174,7 @@ fn normalize_codex_apps_tool_title( value.to_string() } -fn normalize_codex_apps_tool_name( +fn normalize_codex_apps_callable_name( server_name: &str, tool_name: &str, connector_id: Option<&str>, @@ -1247,7 +1209,10 @@ fn normalize_codex_apps_tool_name( tool_name } -fn normalize_codex_apps_namespace(server_name: &str, connector_name: Option<&str>) -> String { +fn normalize_codex_apps_callable_namespace( + server_name: &str, + connector_name: Option<&str>, +) -> String { if server_name == CODEX_APPS_MCP_SERVER_NAME && let Some(connector_name) = connector_name { @@ -1259,10 +1224,7 @@ fn normalize_codex_apps_namespace(server_name: &str, connector_name: Option<&str sanitize_name(connector_name) ) } else { - format!( - "mcp{}{}{}", - MCP_TOOL_NAME_DELIMITER, server_name, MCP_TOOL_NAME_DELIMITER - ) + format!("mcp{MCP_TOOL_NAME_DELIMITER}{server_name}{MCP_TOOL_NAME_DELIMITER}") } } @@ -1591,14 +1553,16 @@ async fn list_tools_for_client_uncached( .tools .into_iter() .map(|tool| { - let tool_name = normalize_codex_apps_tool_name( + let callable_name = normalize_codex_apps_callable_name( server_name, &tool.tool.name, tool.connector_id.as_deref(), tool.connector_name.as_deref(), ); - let tool_namespace = - normalize_codex_apps_namespace(server_name, tool.connector_name.as_deref()); + let callable_namespace = normalize_codex_apps_callable_namespace( + server_name, + tool.connector_name.as_deref(), + ); let connector_name = tool.connector_name; let connector_description = tool.connector_description; let mut tool_def = tool.tool; @@ -1611,8 +1575,8 @@ async fn list_tools_for_client_uncached( } ToolInfo { server_name: server_name.to_owned(), - tool_name, - tool_namespace, + callable_name, + callable_namespace, tool: tool_def, connector_id: tool.connector_id, connector_name, diff --git a/codex-rs/codex-mcp/src/mcp_connection_manager_tests.rs b/codex-rs/codex-mcp/src/mcp_connection_manager_tests.rs index 6af95cc58dca..3fb232f52a42 100644 --- a/codex-rs/codex-mcp/src/mcp_connection_manager_tests.rs +++ b/codex-rs/codex-mcp/src/mcp_connection_manager_tests.rs @@ -7,10 +7,11 @@ use std::sync::Arc; use tempfile::tempdir; fn create_test_tool(server_name: &str, tool_name: &str) -> ToolInfo { + let tool_namespace = format!("mcp__{server_name}__"); ToolInfo { server_name: server_name.to_string(), - tool_name: tool_name.to_string(), - tool_namespace: format!("mcp__{server_name}__"), + callable_name: tool_name.to_string(), + callable_namespace: tool_namespace, tool: Tool { name: tool_name.to_string().into(), title: None, @@ -142,16 +143,12 @@ fn test_qualify_tools_long_names_same_server() { let mut keys: Vec<_> = qualified_tools.keys().cloned().collect(); keys.sort(); - assert_eq!(keys[0].len(), 64); - assert_eq!( - keys[0], - "mcp__my_server__extremel119a2b97664e41363932dc84de21e2ff1b93b3e9" - ); - - assert_eq!(keys[1].len(), 64); - assert_eq!( - keys[1], - "mcp__my_server__yet_anot419a82a89325c1b477274a41f8c65ea5f3a7f341" + assert!(keys.iter().all(|key| key.len() == 64)); + assert!(keys.iter().all(|key| key.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:?}" ); } @@ -164,17 +161,94 @@ fn test_qualify_tools_sanitizes_invalid_characters() { 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!( + format!("{}{}", tool.callable_namespace, tool.callable_name), + qualified_name + ); - // The key is sanitized for OpenAI, but we keep original parts for the actual MCP call. + // The key and 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.tool_name, "tool.two-three"); + 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 .chars() - .all(|c| c.is_ascii_alphanumeric() || c == '_' || c == '-'), - "qualified name must be Responses API compatible: {qualified_name:?}" + .all(|c| c.is_ascii_alphanumeric() || c == '_'), + "qualified name must be code-mode compatible: {qualified_name:?}" + ); +} + +#[test] +fn test_qualify_tools_keeps_hyphenated_mcp_tools_callable() { + let tools = vec![create_test_tool("music-studio", "get-strudel-guide")]; + + let qualified_tools = qualify_tools(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!(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() { + let tools = vec![ + create_test_tool("basic-server", "lookup"), + create_test_tool("basic_server", "query"), + ]; + + let qualified_tools = qualify_tools(tools); + + assert_eq!(qualified_tools.len(), 2); + let mut namespaces = qualified_tools + .values() + .map(|tool| tool.callable_namespace.as_str()) + .collect::>(); + namespaces.sort(); + namespaces.dedup(); + assert_eq!(namespaces.len(), 2); + + let raw_servers = qualified_tools + .values() + .map(|tool| tool.server_name.as_str()) + .collect::>(); + assert_eq!(raw_servers, HashSet::from(["basic-server", "basic_server"])); + assert!( + qualified_tools + .keys() + .all(|key| key.chars().all(|c| c.is_ascii_alphanumeric() || c == '_')), + "qualified names must be code-mode compatible: {qualified_tools:?}" + ); +} + +#[test] +fn test_qualify_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); + + assert_eq!(qualified_tools.len(), 2); + let raw_tool_names = qualified_tools + .values() + .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() + .map(|tool| tool.callable_name.as_str()) + .collect::>(); + assert_eq!(callable_tool_names.len(), 2); } #[test] @@ -241,7 +315,7 @@ fn filter_tools_applies_per_server_filters() { assert_eq!(filtered.len(), 1); assert_eq!(filtered[0].server_name, "server1"); - assert_eq!(filtered[0].tool_name, "tool_a"); + assert_eq!(filtered[0].callable_name, "tool_a"); } #[test] @@ -258,12 +332,12 @@ fn codex_apps_tools_cache_is_overwritten_by_last_write() { write_cached_codex_apps_tools(&cache_context, &tools_gateway_1); let cached_gateway_1 = read_cached_codex_apps_tools(&cache_context).expect("cache entry exists for first write"); - assert_eq!(cached_gateway_1[0].tool_name, "one"); + assert_eq!(cached_gateway_1[0].callable_name, "one"); write_cached_codex_apps_tools(&cache_context, &tools_gateway_2); let cached_gateway_2 = read_cached_codex_apps_tools(&cache_context).expect("cache entry exists for second write"); - assert_eq!(cached_gateway_2[0].tool_name, "two"); + assert_eq!(cached_gateway_2[0].callable_name, "two"); } #[test] @@ -290,8 +364,8 @@ fn codex_apps_tools_cache_is_scoped_per_user() { let read_user_2 = read_cached_codex_apps_tools(&cache_context_user_2).expect("cache entry for user two"); - assert_eq!(read_user_1[0].tool_name, "one"); - assert_eq!(read_user_2[0].tool_name, "two"); + assert_eq!(read_user_1[0].callable_name, "one"); + assert_eq!(read_user_2[0].callable_name, "two"); assert_ne!( cache_context_user_1.cache_path(), cache_context_user_2.cache_path(), @@ -326,7 +400,7 @@ fn codex_apps_tools_cache_filters_disallowed_connectors() { let cached = read_cached_codex_apps_tools(&cache_context).expect("cache entry exists for user"); assert_eq!(cached.len(), 1); - assert_eq!(cached[0].tool_name, "allowed_tool"); + assert_eq!(cached[0].callable_name, "allowed_tool"); assert_eq!(cached[0].connector_id.as_deref(), Some("calendar")); } @@ -391,7 +465,7 @@ fn startup_cached_codex_apps_tools_loads_from_disk_cache() { assert_eq!(startup_tools.len(), 1); assert_eq!(startup_tools[0].server_name, CODEX_APPS_MCP_SERVER_NAME); - assert_eq!(startup_tools[0].tool_name, "calendar_search"); + assert_eq!(startup_tools[0].callable_name, "calendar_search"); } #[tokio::test] @@ -420,7 +494,7 @@ async fn list_all_tools_uses_startup_snapshot_while_client_is_pending() { .get("mcp__codex_apps__calendar_create_event") .expect("tool from startup cache"); assert_eq!(tool.server_name, CODEX_APPS_MCP_SERVER_NAME); - assert_eq!(tool.tool_name, "calendar_create_event"); + assert_eq!(tool.callable_name, "calendar_create_event"); } #[tokio::test] @@ -499,7 +573,7 @@ async fn list_all_tools_uses_startup_snapshot_when_client_startup_fails() { .get("mcp__codex_apps__calendar_create_event") .expect("tool from startup cache"); assert_eq!(tool.server_name, CODEX_APPS_MCP_SERVER_NAME); - assert_eq!(tool.tool_name, "calendar_create_event"); + assert_eq!(tool.callable_name, "calendar_create_event"); } #[test] diff --git a/codex-rs/codex-mcp/src/mcp_tool_names.rs b/codex-rs/codex-mcp/src/mcp_tool_names.rs new file mode 100644 index 000000000000..7827cb681ecb --- /dev/null +++ b/codex-rs/codex-mcp/src/mcp_tool_names.rs @@ -0,0 +1,197 @@ +//! Allocates model-visible MCP tool names while preserving raw MCP identities. + +use std::collections::HashMap; +use std::collections::HashSet; + +use sha1::Digest; +use sha1::Sha1; +use tracing::warn; + +use crate::mcp::sanitize_responses_api_tool_name; +use crate::mcp_connection_manager::ToolInfo; + +const MCP_TOOL_NAME_DELIMITER: &str = "__"; +const MAX_TOOL_NAME_LENGTH: usize = 64; +const CALLABLE_NAME_HASH_LEN: usize = 12; + +fn sha1_hex(s: &str) -> String { + let mut hasher = Sha1::new(); + hasher.update(s.as_bytes()); + let sha1 = hasher.finalize(); + format!("{sha1:x}") +} + +fn callable_name_hash_suffix(raw_identity: &str) -> String { + let hash = sha1_hex(raw_identity); + format!("_{}", &hash[..CALLABLE_NAME_HASH_LEN]) +} + +fn append_hash_suffix(value: &str, raw_identity: &str) -> String { + format!("{value}{}", callable_name_hash_suffix(raw_identity)) +} + +fn append_namespace_hash_suffix(namespace: &str, raw_identity: &str) -> String { + if let Some(namespace) = namespace.strip_suffix(MCP_TOOL_NAME_DELIMITER) { + format!( + "{}{}{}", + namespace, + callable_name_hash_suffix(raw_identity), + MCP_TOOL_NAME_DELIMITER + ) + } else { + append_hash_suffix(namespace, raw_identity) + } +} + +fn truncate_name(value: &str, max_len: usize) -> String { + value.chars().take(max_len).collect() +} + +fn fit_callable_parts_with_hash( + namespace: &str, + tool_name: &str, + raw_identity: &str, +) -> (String, String) { + let suffix = callable_name_hash_suffix(raw_identity); + let max_tool_len = MAX_TOOL_NAME_LENGTH.saturating_sub(namespace.len()); + if max_tool_len >= suffix.len() { + let prefix_len = max_tool_len - suffix.len(); + return ( + namespace.to_string(), + format!("{}{}", truncate_name(tool_name, prefix_len), suffix), + ); + } + + let max_namespace_len = MAX_TOOL_NAME_LENGTH - suffix.len(); + (truncate_name(namespace, max_namespace_len), suffix) +} + +fn unique_callable_parts( + namespace: &str, + 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); + } + + let mut attempt = 0_u32; + loop { + let hash_input = if attempt == 0 { + raw_identity.to_string() + } else { + format!("{raw_identity}\0{attempt}") + }; + 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); + } + attempt = attempt.saturating_add(1); + } +} + +#[derive(Debug)] +struct CallableToolCandidate { + tool: ToolInfo, + raw_namespace_identity: String, + raw_tool_identity: String, + callable_namespace: String, + callable_name: String, +} + +pub(crate) fn qualify_tools(tools: I) -> HashMap +where + I: IntoIterator, +{ + let mut seen_raw_names = HashSet::new(); + let mut candidates = Vec::new(); + for tool in tools { + let raw_namespace_identity = format!( + "{}\0{}\0{}", + tool.server_name, + tool.callable_namespace, + tool.connector_id.as_deref().unwrap_or_default() + ); + let raw_tool_identity = format!( + "{}\0{}\0{}", + raw_namespace_identity, tool.callable_name, tool.tool.name + ); + if !seen_raw_names.insert(raw_tool_identity.clone()) { + warn!("skipping duplicated tool {}", tool.tool.name); + continue; + } + + candidates.push(CallableToolCandidate { + callable_namespace: sanitize_responses_api_tool_name(&tool.callable_namespace), + callable_name: sanitize_responses_api_tool_name(&tool.callable_name), + raw_namespace_identity, + raw_tool_identity, + tool, + }); + } + + let mut namespace_identities_by_base = HashMap::>::new(); + for candidate in &candidates { + namespace_identities_by_base + .entry(candidate.callable_namespace.clone()) + .or_default() + .insert(candidate.raw_namespace_identity.clone()); + } + let colliding_namespaces = namespace_identities_by_base + .into_iter() + .filter_map(|(namespace, identities)| (identities.len() > 1).then_some(namespace)) + .collect::>(); + for candidate in &mut candidates { + if colliding_namespaces.contains(&candidate.callable_namespace) { + candidate.callable_namespace = append_namespace_hash_suffix( + &candidate.callable_namespace, + &candidate.raw_namespace_identity, + ); + } + } + + let mut tool_identities_by_base = HashMap::<(String, String), HashSet>::new(); + for candidate in &candidates { + tool_identities_by_base + .entry(( + candidate.callable_namespace.clone(), + candidate.callable_name.clone(), + )) + .or_default() + .insert(candidate.raw_tool_identity.clone()); + } + let colliding_tools = tool_identities_by_base + .into_iter() + .filter_map(|(key, identities)| (identities.len() > 1).then_some(key)) + .collect::>(); + for candidate in &mut candidates { + if colliding_tools.contains(&( + candidate.callable_namespace.clone(), + candidate.callable_name.clone(), + )) { + candidate.callable_name = + append_hash_suffix(&candidate.callable_name, &candidate.raw_tool_identity); + } + } + + 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(); + for mut candidate in candidates { + let (callable_namespace, callable_name, qualified_name) = unique_callable_parts( + &candidate.callable_namespace, + &candidate.callable_name, + &candidate.raw_tool_identity, + &mut used_names, + ); + candidate.tool.callable_namespace = callable_namespace; + candidate.tool.callable_name = callable_name; + qualified_tools.insert(qualified_name, candidate.tool); + } + qualified_tools +} diff --git a/codex-rs/core/src/codex_tests.rs b/codex-rs/core/src/codex_tests.rs index acb32d8f4647..79ae5befe976 100644 --- a/codex-rs/core/src/codex_tests.rs +++ b/codex-rs/core/src/codex_tests.rs @@ -404,8 +404,8 @@ fn make_mcp_tool( ToolInfo { server_name: server_name.to_string(), - tool_name: tool_name.to_string(), - tool_namespace, + callable_name: tool_name.to_string(), + callable_namespace: tool_namespace, tool: Tool { name: tool_name.to_string().into(), title: None, diff --git a/codex-rs/core/src/connectors_tests.rs b/codex-rs/core/src/connectors_tests.rs index 893ab243caa4..1be6fff10752 100644 --- a/codex-rs/core/src/connectors_tests.rs +++ b/codex-rs/core/src/connectors_tests.rs @@ -110,8 +110,8 @@ fn codex_app_tool( ToolInfo { server_name: CODEX_APPS_MCP_SERVER_NAME.to_string(), - tool_name: tool_name.to_string(), - tool_namespace, + callable_name: tool_name.to_string(), + callable_namespace: tool_namespace, tool: test_tool_definition(tool_name), connector_id: Some(connector_id.to_string()), connector_name: connector_name.map(ToOwned::to_owned), @@ -188,8 +188,8 @@ fn accessible_connectors_from_mcp_tools_carries_plugin_display_names() { "mcp__sample__echo".to_string(), ToolInfo { server_name: "sample".to_string(), - tool_name: "echo".to_string(), - tool_namespace: "sample".to_string(), + callable_name: "echo".to_string(), + callable_namespace: "sample".to_string(), tool: test_tool_definition("echo"), connector_id: None, connector_name: None, @@ -312,8 +312,8 @@ fn accessible_connectors_from_mcp_tools_preserves_description() { "mcp__codex_apps__calendar_create_event".to_string(), ToolInfo { server_name: CODEX_APPS_MCP_SERVER_NAME.to_string(), - tool_name: "calendar_create_event".to_string(), - tool_namespace: "mcp__codex_apps__calendar".to_string(), + callable_name: "calendar_create_event".to_string(), + callable_namespace: "mcp__codex_apps__calendar".to_string(), tool: Tool { name: "calendar_create_event".to_string().into(), title: None, diff --git a/codex-rs/core/src/tools/handlers/tool_search.rs b/codex-rs/core/src/tools/handlers/tool_search.rs index 7edcfeceab79..114e6768078b 100644 --- a/codex-rs/core/src/tools/handlers/tool_search.rs +++ b/codex-rs/core/src/tools/handlers/tool_search.rs @@ -82,8 +82,8 @@ impl ToolHandler for ToolSearchHandler { .filter_map(|result| entries.get(result.document.id)) .map(|(_, tool)| ToolSearchResultSource { server_name: tool.server_name.as_str(), - tool_namespace: tool.tool_namespace.as_str(), - tool_name: tool.tool_name.as_str(), + tool_namespace: tool.callable_namespace.as_str(), + tool_name: tool.callable_name.as_str(), tool: &tool.tool, connector_name: tool.connector_name.as_deref(), connector_description: tool.connector_description.as_deref(), @@ -102,7 +102,8 @@ impl ToolHandler for ToolSearchHandler { fn build_search_text(name: &str, info: &ToolInfo) -> String { let mut parts = vec![ name.to_string(), - info.tool_name.clone(), + info.callable_name.clone(), + info.tool.name.to_string(), info.server_name.clone(), ]; diff --git a/codex-rs/core/src/tools/spec.rs b/codex-rs/core/src/tools/spec.rs index c130e261dad2..bb6735f70e44 100644 --- a/codex-rs/core/src/tools/spec.rs +++ b/codex-rs/core/src/tools/spec.rs @@ -71,8 +71,8 @@ pub(crate) fn build_specs_with_discoverable_tools( tools .values() .map(|tool| ToolRegistryPlanDeferredTool { - tool_name: tool.tool_name.as_str(), - tool_namespace: tool.tool_namespace.as_str(), + tool_name: tool.callable_name.as_str(), + tool_namespace: tool.callable_namespace.as_str(), server_name: tool.server_name.as_str(), connector_name: tool.connector_name.as_deref(), connector_description: tool.connector_description.as_deref(), diff --git a/codex-rs/core/src/tools/spec_tests.rs b/codex-rs/core/src/tools/spec_tests.rs index bc508f13543d..88b51807e0af 100644 --- a/codex-rs/core/src/tools/spec_tests.rs +++ b/codex-rs/core/src/tools/spec_tests.rs @@ -699,8 +699,8 @@ fn search_tool_description_falls_back_to_connector_name_without_description() { "mcp__codex_apps__calendar_create_event".to_string(), ToolInfo { server_name: CODEX_APPS_MCP_SERVER_NAME.to_string(), - tool_name: "_create_event".to_string(), - tool_namespace: "mcp__codex_apps__calendar".to_string(), + callable_name: "_create_event".to_string(), + callable_namespace: "mcp__codex_apps__calendar".to_string(), tool: mcp_tool( "calendar_create_event", "Create calendar event", @@ -749,8 +749,8 @@ fn search_tool_registers_namespaced_mcp_tool_aliases() { "mcp__codex_apps__calendar_create_event".to_string(), ToolInfo { server_name: CODEX_APPS_MCP_SERVER_NAME.to_string(), - tool_name: "_create_event".to_string(), - tool_namespace: "mcp__codex_apps__calendar".to_string(), + callable_name: "_create_event".to_string(), + callable_namespace: "mcp__codex_apps__calendar".to_string(), tool: mcp_tool( "calendar-create-event", "Create calendar event", @@ -766,8 +766,8 @@ fn search_tool_registers_namespaced_mcp_tool_aliases() { "mcp__codex_apps__calendar_list_events".to_string(), ToolInfo { server_name: CODEX_APPS_MCP_SERVER_NAME.to_string(), - tool_name: "_list_events".to_string(), - tool_namespace: "mcp__codex_apps__calendar".to_string(), + callable_name: "_list_events".to_string(), + callable_namespace: "mcp__codex_apps__calendar".to_string(), tool: mcp_tool( "calendar-list-events", "List calendar events", @@ -783,8 +783,8 @@ fn search_tool_registers_namespaced_mcp_tool_aliases() { "mcp__rmcp__echo".to_string(), ToolInfo { server_name: "rmcp".to_string(), - tool_name: "echo".to_string(), - tool_namespace: "mcp__rmcp__".to_string(), + callable_name: "echo".to_string(), + callable_namespace: "mcp__rmcp__".to_string(), tool: mcp_tool("echo", "Echo", serde_json::json!({"type": "object"})), connector_id: None, connector_name: None, From b7eb11b26c8e9ff9ca0096e8f1ebdf811a4b059b Mon Sep 17 00:00:00 2001 From: Matthew Zeng Date: Tue, 7 Apr 2026 22:10:28 -0700 Subject: [PATCH 4/9] update --- .../app-server/tests/suite/v2/mcp_server_status.rs | 13 ++++++++++--- codex-rs/codex-mcp/src/mcp/mod.rs | 3 ++- 2 files changed, 12 insertions(+), 4 deletions(-) diff --git a/codex-rs/app-server/tests/suite/v2/mcp_server_status.rs b/codex-rs/app-server/tests/suite/v2/mcp_server_status.rs index 5c43c4ff8154..44efec4ed3f8 100644 --- a/codex-rs/app-server/tests/suite/v2/mcp_server_status.rs +++ b/codex-rs/app-server/tests/suite/v2/mcp_server_status.rs @@ -38,9 +38,9 @@ use tokio::time::timeout; const DEFAULT_READ_TIMEOUT: Duration = Duration::from_secs(10); #[tokio::test] -async fn mcp_server_status_list_returns_tools_for_hyphenated_server_names() -> Result<()> { +async fn mcp_server_status_list_returns_raw_server_and_tool_names() -> Result<()> { let server = create_mock_responses_server_sequence_unchecked(Vec::new()).await; - let (mcp_server_url, mcp_server_handle) = start_mcp_server("lookup").await?; + let (mcp_server_url, mcp_server_handle) = start_mcp_server("look-up.raw").await?; let codex_home = TempDir::new()?; write_mock_responses_config_toml( codex_home.path(), @@ -85,7 +85,14 @@ url = "{mcp_server_url}/mcp" assert_eq!(status.name, "some-server"); assert_eq!( status.tools.keys().cloned().collect::>(), - BTreeSet::from(["lookup".to_string()]) + BTreeSet::from(["look-up.raw".to_string()]) + ); + assert_eq!( + status + .tools + .get("look-up.raw") + .map(|tool| tool.name.as_str()), + Some("look-up.raw") ); mcp_server_handle.abort(); diff --git a/codex-rs/codex-mcp/src/mcp/mod.rs b/codex-rs/codex-mcp/src/mcp/mod.rs index 2bd305a6b361..dfe30e6d6bd2 100644 --- a/codex-rs/codex-mcp/src/mcp/mod.rs +++ b/codex-rs/codex-mcp/src/mcp/mod.rs @@ -593,10 +593,11 @@ async fn collect_mcp_server_status_snapshot_from_manager( let Some(tool) = protocol_tool_from_rmcp_tool(&raw_tool_name, &tool_info.tool) else { continue; }; + let tool_name = tool.name.clone(); tools_by_server .entry(tool_info.server_name) .or_default() - .insert(raw_tool_name, tool); + .insert(tool_name, tool); } McpServerStatusSnapshot { From 87368eb2fc5dd2cf84b893439540ddb74671e133 Mon Sep 17 00:00:00 2001 From: Matthew Zeng Date: Tue, 7 Apr 2026 23:27:10 -0700 Subject: [PATCH 5/9] update --- codex-rs/tools/src/tool_registry_plan_tests.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/codex-rs/tools/src/tool_registry_plan_tests.rs b/codex-rs/tools/src/tool_registry_plan_tests.rs index 52c2a8366c14..3ce26fdbfc6b 100644 --- a/codex-rs/tools/src/tool_registry_plan_tests.rs +++ b/codex-rs/tools/src/tool_registry_plan_tests.rs @@ -485,7 +485,7 @@ fn disabled_environment_omits_environment_backed_tools() { let (tools, _) = build_specs( &tools_config, /*mcp_tools*/ None, - /*app_tools*/ None, + /*deferred_mcp_tools*/ None, &[], ); From f2538bc80b89905a6bf40d2b6612a8276406b357 Mon Sep 17 00:00:00 2001 From: Matthew Zeng Date: Wed, 8 Apr 2026 12:15:11 -0700 Subject: [PATCH 6/9] codex: address PR review feedback (#16944) --- codex-rs/core/src/codex.rs | 71 +------------------------ codex-rs/core/src/codex_tests.rs | 3 ++ codex-rs/core/src/lib.rs | 1 + codex-rs/core/src/mcp_tool_exposure.rs | 73 ++++++++++++++++++++++++++ codex-rs/tools/src/tool_discovery.rs | 14 ++--- 5 files changed, 83 insertions(+), 79 deletions(-) create mode 100644 codex-rs/core/src/mcp_tool_exposure.rs diff --git a/codex-rs/core/src/codex.rs b/codex-rs/core/src/codex.rs index 13a78775a9fe..b9debc1011a5 100644 --- a/codex-rs/core/src/codex.rs +++ b/codex-rs/core/src/codex.rs @@ -25,6 +25,7 @@ use crate::config::ManagedFeatures; use crate::connectors; use crate::exec_policy::ExecPolicyManager; use crate::installation_id::resolve_installation_id; +use crate::mcp_tool_exposure::build_mcp_tool_exposure; use crate::parse_turn_item; use crate::path_utils::normalize_for_native_workdir; use crate::realtime_conversation::RealtimeConversationManager; @@ -73,9 +74,7 @@ use codex_login::auth_env_telemetry::collect_auth_env_telemetry; use codex_login::default_client::originator; use codex_mcp::McpConnectionManager; use codex_mcp::SandboxState; -use codex_mcp::ToolInfo as McpToolInfo; use codex_mcp::codex_apps_tools_cache_key; -use codex_mcp::filter_non_codex_apps_mcp_tools_only; #[cfg(test)] use codex_models_manager::collaboration_mode_presets::CollaborationModesConfig; use codex_models_manager::manager::ModelsManager; @@ -322,7 +321,6 @@ use crate::util::backoff; use crate::windows_sandbox::WindowsSandboxLevelExt; use codex_async_utils::OrCancelExt; use codex_git_utils::get_git_repo_root; -use codex_mcp::CODEX_APPS_MCP_SERVER_NAME; use codex_mcp::compute_auth_statuses; use codex_mcp::with_codex_apps_mcp; use codex_otel::SessionTelemetry; @@ -434,8 +432,6 @@ pub(crate) const INITIAL_SUBMIT_ID: &str = ""; pub(crate) const SUBMISSION_CHANNEL_CAPACITY: usize = 512; const CYBER_VERIFY_URL: &str = "https://chatgpt.com/cyber"; const CYBER_SAFETY_URL: &str = "https://developers.openai.com/codex/concepts/cyber-safety"; -const DIRECT_MCP_TOOL_EXPOSURE_THRESHOLD: usize = 100; - impl Codex { /// Spawn a new [`Codex`] and initialize the session. pub(crate) async fn spawn(args: CodexSpawnArgs) -> CodexResult { @@ -6581,71 +6577,6 @@ fn connector_inserted_in_messages( connector_count == 1 && skill_count == 0 && mention_names_lower.contains(&mention_slug) } -struct McpToolExposure { - direct_tools: HashMap, - deferred_tools: Option>, -} - -fn build_mcp_tool_exposure( - all_mcp_tools: &HashMap, - connectors: Option<&[connectors::AppInfo]>, - explicitly_enabled_connectors: &[connectors::AppInfo], - config: &Config, - tools_config: &ToolsConfig, -) -> McpToolExposure { - let mut deferred_tools = filter_non_codex_apps_mcp_tools_only(all_mcp_tools); - if let Some(connectors) = connectors { - deferred_tools.extend(filter_codex_apps_mcp_tools( - all_mcp_tools, - connectors, - config, - )); - } - - if !tools_config.search_tool || deferred_tools.len() < DIRECT_MCP_TOOL_EXPOSURE_THRESHOLD { - return McpToolExposure { - direct_tools: deferred_tools, - deferred_tools: None, - }; - } - - let direct_tools = - filter_codex_apps_mcp_tools(all_mcp_tools, explicitly_enabled_connectors, config); - McpToolExposure { - direct_tools, - deferred_tools: Some(deferred_tools), - } -} - -fn filter_codex_apps_mcp_tools( - mcp_tools: &HashMap, - connectors: &[connectors::AppInfo], - config: &Config, -) -> HashMap { - let allowed: HashSet<&str> = connectors - .iter() - .map(|connector| connector.id.as_str()) - .collect(); - - mcp_tools - .iter() - .filter(|(_, tool)| { - if tool.server_name != CODEX_APPS_MCP_SERVER_NAME { - return false; - } - let Some(connector_id) = codex_apps_connector_id(tool) else { - return false; - }; - allowed.contains(connector_id) && connectors::codex_app_tool_is_enabled(config, tool) - }) - .map(|(name, tool)| (name.clone(), tool.clone())) - .collect() -} - -fn codex_apps_connector_id(tool: &McpToolInfo) -> Option<&str> { - tool.connector_id.as_deref() -} - pub(crate) fn build_prompt( input: Vec, router: &ToolRouter, diff --git a/codex-rs/core/src/codex_tests.rs b/codex-rs/core/src/codex_tests.rs index 1cc84e0cb47d..f87d60bbdac8 100644 --- a/codex-rs/core/src/codex_tests.rs +++ b/codex-rs/core/src/codex_tests.rs @@ -10,11 +10,14 @@ use crate::config_loader::RequirementSource; use crate::config_loader::Sourced; use crate::exec::ExecCapturePolicy; use crate::function_tool::FunctionCallError; +use crate::mcp_tool_exposure::DIRECT_MCP_TOOL_EXPOSURE_THRESHOLD; +use crate::mcp_tool_exposure::build_mcp_tool_exposure; use crate::shell::default_user_shell; use crate::tools::format_exec_output_str; use codex_features::Features; use codex_login::CodexAuth; +use codex_mcp::CODEX_APPS_MCP_SERVER_NAME; use codex_mcp::ToolInfo; use codex_model_provider_info::ModelProviderInfo; use codex_models_manager::bundled_models_response; diff --git a/codex-rs/core/src/lib.rs b/codex-rs/core/src/lib.rs index 83232de734f1..7233e7431839 100644 --- a/codex-rs/core/src/lib.rs +++ b/codex-rs/core/src/lib.rs @@ -45,6 +45,7 @@ pub use landlock::spawn_command_under_linux_sandbox; pub(crate) mod mcp; mod mcp_skill_dependencies; mod mcp_tool_approval_templates; +mod mcp_tool_exposure; mod network_policy_decision; pub(crate) mod network_proxy_loader; pub use mcp::McpManager; diff --git a/codex-rs/core/src/mcp_tool_exposure.rs b/codex-rs/core/src/mcp_tool_exposure.rs new file mode 100644 index 000000000000..d7858c17f0d1 --- /dev/null +++ b/codex-rs/core/src/mcp_tool_exposure.rs @@ -0,0 +1,73 @@ +use std::collections::HashMap; +use std::collections::HashSet; + +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; +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) fn build_mcp_tool_exposure( + all_mcp_tools: &HashMap, + connectors: Option<&[connectors::AppInfo]>, + explicitly_enabled_connectors: &[connectors::AppInfo], + config: &Config, + tools_config: &ToolsConfig, +) -> McpToolExposure { + let mut deferred_tools = filter_non_codex_apps_mcp_tools_only(all_mcp_tools); + if let Some(connectors) = connectors { + deferred_tools.extend(filter_codex_apps_mcp_tools( + all_mcp_tools, + connectors, + config, + )); + } + + if !tools_config.search_tool || deferred_tools.len() < DIRECT_MCP_TOOL_EXPOSURE_THRESHOLD { + return McpToolExposure { + direct_tools: deferred_tools, + deferred_tools: None, + }; + } + + let direct_tools = + filter_codex_apps_mcp_tools(all_mcp_tools, explicitly_enabled_connectors, config); + McpToolExposure { + direct_tools, + deferred_tools: Some(deferred_tools), + } +} + +fn filter_codex_apps_mcp_tools( + mcp_tools: &HashMap, + connectors: &[connectors::AppInfo], + config: &Config, +) -> HashMap { + let allowed: HashSet<&str> = connectors + .iter() + .map(|connector| connector.id.as_str()) + .collect(); + + mcp_tools + .iter() + .filter(|(_, tool)| { + if tool.server_name != CODEX_APPS_MCP_SERVER_NAME { + return false; + } + let Some(connector_id) = tool.connector_id.as_deref() else { + return false; + }; + allowed.contains(connector_id) && connectors::codex_app_tool_is_enabled(config, tool) + }) + .map(|(name, tool)| (name.clone(), tool.clone())) + .collect() +} diff --git a/codex-rs/tools/src/tool_discovery.rs b/codex-rs/tools/src/tool_discovery.rs index 1bb9e9f5544e..594d4011b862 100644 --- a/codex-rs/tools/src/tool_discovery.rs +++ b/codex-rs/tools/src/tool_discovery.rs @@ -269,10 +269,13 @@ pub fn collect_tool_search_source_infos<'a>( .map(str::trim) .filter(|connector_name| !connector_name.is_empty()) { - let description = connector_description(tool.connector_description); return Some(ToolSearchSourceInfo { name: name.to_string(), - description, + description: tool + .connector_description + .map(str::trim) + .filter(|description| !description.is_empty()) + .map(str::to_string), }); } @@ -289,13 +292,6 @@ pub fn collect_tool_search_source_infos<'a>( .collect() } -fn connector_description(description: Option<&str>) -> Option { - description - .map(str::trim) - .filter(|description| !description.is_empty()) - .map(str::to_string) -} - pub fn create_tool_suggest_tool(discoverable_tools: &[ToolSuggestEntry]) -> ToolSpec { let discoverable_tool_ids = discoverable_tools .iter() From ab3c36f5e0c1d0aed659f6114ad8ef39c0289b24 Mon Sep 17 00:00:00 2001 From: Matthew Zeng Date: Wed, 8 Apr 2026 13:32:44 -0700 Subject: [PATCH 7/9] update --- codex-rs/core/src/codex_tests.rs | 1 + codex-rs/core/src/tools/spec_tests.rs | 1 + codex-rs/tools/src/tool_registry_plan_tests.rs | 1 + 3 files changed, 3 insertions(+) diff --git a/codex-rs/core/src/codex_tests.rs b/codex-rs/core/src/codex_tests.rs index c622a78948b1..628345a02435 100644 --- a/codex-rs/core/src/codex_tests.rs +++ b/codex-rs/core/src/codex_tests.rs @@ -462,6 +462,7 @@ fn tools_config_for_mcp_tool_exposure(search_tool: bool) -> ToolsConfig { model_info: &model_info, available_models: &available_models, features: &features, + image_generation_tool_auth_allowed: true, web_search_mode: Some(WebSearchMode::Cached), session_source: SessionSource::Cli, sandbox_policy: &SandboxPolicy::DangerFullAccess, diff --git a/codex-rs/core/src/tools/spec_tests.rs b/codex-rs/core/src/tools/spec_tests.rs index 6050ed64b296..25ccef0f19df 100644 --- a/codex-rs/core/src/tools/spec_tests.rs +++ b/codex-rs/core/src/tools/spec_tests.rs @@ -170,6 +170,7 @@ fn multi_agent_v2_tools_config() -> ToolsConfig { model_info: &model_info, available_models: &available_models, features: &features, + image_generation_tool_auth_allowed: true, web_search_mode: Some(WebSearchMode::Cached), session_source: SessionSource::Cli, sandbox_policy: &SandboxPolicy::DangerFullAccess, diff --git a/codex-rs/tools/src/tool_registry_plan_tests.rs b/codex-rs/tools/src/tool_registry_plan_tests.rs index a06214da3706..13ef18495e5f 100644 --- a/codex-rs/tools/src/tool_registry_plan_tests.rs +++ b/codex-rs/tools/src/tool_registry_plan_tests.rs @@ -1624,6 +1624,7 @@ fn code_mode_preserves_nullable_and_literal_mcp_input_shapes() { model_info: &model_info, available_models: &available_models, features: &features, + image_generation_tool_auth_allowed: true, web_search_mode: Some(WebSearchMode::Cached), session_source: SessionSource::Cli, sandbox_policy: &SandboxPolicy::DangerFullAccess, From 67987670dd2a7a97535774906ca1bc25f9d0a49f Mon Sep 17 00:00:00 2001 From: Matthew Zeng Date: Wed, 8 Apr 2026 16:45:51 -0700 Subject: [PATCH 8/9] codex: address latest MCP tool-search feedback (#16944) --- codex-rs/codex-mcp/src/mcp_tool_names.rs | 5 +++ .../core/src/tools/handlers/tool_search.rs | 39 +++++++++++-------- 2 files changed, 27 insertions(+), 17 deletions(-) diff --git a/codex-rs/codex-mcp/src/mcp_tool_names.rs b/codex-rs/codex-mcp/src/mcp_tool_names.rs index 7827cb681ecb..5a323dfe7163 100644 --- a/codex-rs/codex-mcp/src/mcp_tool_names.rs +++ b/codex-rs/codex-mcp/src/mcp_tool_names.rs @@ -103,6 +103,11 @@ struct CallableToolCandidate { callable_name: String, } +/// Returns a qualified-name lookup for MCP tools. +/// +/// Raw MCP server/tool names are kept on each [`ToolInfo`] for protocol calls, while +/// `callable_namespace` / `callable_name` are sanitized and, when necessary, hashed so +/// every model-visible `mcp__namespace__tool` name is unique and <= 64 bytes. pub(crate) fn qualify_tools(tools: I) -> HashMap where I: IntoIterator, diff --git a/codex-rs/core/src/tools/handlers/tool_search.rs b/codex-rs/core/src/tools/handlers/tool_search.rs index 23c07ef72f8a..1c715e43fca9 100644 --- a/codex-rs/core/src/tools/handlers/tool_search.rs +++ b/codex-rs/core/src/tools/handlers/tool_search.rs @@ -6,21 +6,36 @@ use crate::tools::registry::ToolHandler; use crate::tools::registry::ToolKind; use bm25::Document; use bm25::Language; +use bm25::SearchEngine; use bm25::SearchEngineBuilder; use codex_mcp::ToolInfo; use codex_tools::TOOL_SEARCH_DEFAULT_LIMIT; use codex_tools::TOOL_SEARCH_TOOL_NAME; use codex_tools::ToolSearchResultSource; use codex_tools::collect_tool_search_output_tools; -use std::collections::HashMap; pub struct ToolSearchHandler { - tools: HashMap, + entries: Vec<(String, ToolInfo)>, + search_engine: SearchEngine, } impl ToolSearchHandler { - pub fn new(tools: HashMap) -> Self { - Self { tools } + pub fn new(tools: std::collections::HashMap) -> Self { + let mut entries: Vec<(String, ToolInfo)> = tools.into_iter().collect(); + entries.sort_by(|a, b| a.0.cmp(&b.0)); + + let documents: Vec> = entries + .iter() + .enumerate() + .map(|(idx, (name, info))| Document::new(idx, build_search_text(name, info))) + .collect(); + let search_engine = + SearchEngineBuilder::::with_documents(Language::English, documents).build(); + + Self { + entries, + search_engine, + } } } @@ -60,26 +75,16 @@ impl ToolHandler for ToolSearchHandler { )); } - let mut entries: Vec<(String, ToolInfo)> = self.tools.clone().into_iter().collect(); - entries.sort_by(|a, b| a.0.cmp(&b.0)); - - if entries.is_empty() { + if self.entries.is_empty() { return Ok(ToolSearchOutput { tools: Vec::new() }); } - let documents: Vec> = entries - .iter() - .enumerate() - .map(|(idx, (name, info))| Document::new(idx, build_search_text(name, info))) - .collect(); - let search_engine = - SearchEngineBuilder::::with_documents(Language::English, documents).build(); - let results = search_engine.search(query, limit); + let results = self.search_engine.search(query, limit); let tools = collect_tool_search_output_tools( results .into_iter() - .filter_map(|result| entries.get(result.document.id)) + .filter_map(|result| self.entries.get(result.document.id)) .map(|(_, tool)| ToolSearchResultSource { server_name: tool.server_name.as_str(), tool_namespace: tool.callable_namespace.as_str(), From 90b56c54098b5ff0830adfff10995ea84d34e011 Mon Sep 17 00:00:00 2001 From: Matthew Zeng Date: Wed, 8 Apr 2026 17:23:19 -0700 Subject: [PATCH 9/9] codex: carry direct MCP tools as ToolInfo (#16944) --- codex-rs/core/src/codex.rs | 10 ++---- codex-rs/core/src/codex_tests.rs | 5 +-- codex-rs/core/src/tools/code_mode/mod.rs | 4 +-- codex-rs/core/src/tools/js_repl/mod.rs | 4 +-- codex-rs/core/src/tools/router.rs | 33 +------------------- codex-rs/core/src/tools/router_tests.rs | 8 ++--- codex-rs/core/src/tools/spec.rs | 38 ++++++++++++++++++++--- codex-rs/core/src/tools/spec_tests.rs | 39 +++++++++++++++--------- 8 files changed, 67 insertions(+), 74 deletions(-) diff --git a/codex-rs/core/src/codex.rs b/codex-rs/core/src/codex.rs index 85f10b4138b0..2f358d84d0c0 100644 --- a/codex-rs/core/src/codex.rs +++ b/codex-rs/core/src/codex.rs @@ -6861,19 +6861,13 @@ pub(crate) async fn built_tools( &turn_context.config, &turn_context.tools_config, ); - let mcp_tool_router_inputs = has_mcp_servers - .then(|| crate::tools::router::map_mcp_tool_infos(&mcp_tool_exposure.direct_tools)); + let direct_mcp_tools = has_mcp_servers.then_some(mcp_tool_exposure.direct_tools); Ok(Arc::new(ToolRouter::from_config( &turn_context.tools_config, ToolRouterParams { deferred_mcp_tools: mcp_tool_exposure.deferred_tools, - mcp_tools: mcp_tool_router_inputs - .as_ref() - .map(|inputs| inputs.mcp_tools.clone()), - tool_namespaces: mcp_tool_router_inputs - .as_ref() - .map(|inputs| inputs.tool_namespaces.clone()), + mcp_tools: direct_mcp_tools, discoverable_tools, dynamic_tools: turn_context.dynamic_tools.as_slice(), }, diff --git a/codex-rs/core/src/codex_tests.rs b/codex-rs/core/src/codex_tests.rs index 628345a02435..f55a46f3032a 100644 --- a/codex-rs/core/src/codex_tests.rs +++ b/codex-rs/core/src/codex_tests.rs @@ -309,7 +309,6 @@ fn test_tool_runtime(session: Arc, turn_context: Arc) -> T crate::tools::router::ToolRouterParams { mcp_tools: None, deferred_mcp_tools: None, - tool_namespaces: None, discoverable_tools: None, dynamic_tools: turn_context.dynamic_tools.as_slice(), }, @@ -5270,13 +5269,11 @@ async fn fatal_tool_error_stops_turn_and_reports_error() { .await }; let deferred_mcp_tools = Some(tools.clone()); - let mcp_tool_router_inputs = crate::tools::router::map_mcp_tool_infos(&tools); let router = ToolRouter::from_config( &turn_context.tools_config, crate::tools::router::ToolRouterParams { deferred_mcp_tools, - mcp_tools: Some(mcp_tool_router_inputs.mcp_tools), - tool_namespaces: Some(mcp_tool_router_inputs.tool_namespaces), + mcp_tools: Some(tools), discoverable_tools: None, dynamic_tools: turn_context.dynamic_tools.as_slice(), }, diff --git a/codex-rs/core/src/tools/code_mode/mod.rs b/codex-rs/core/src/tools/code_mode/mod.rs index dd521ba729c9..c0320387f4c4 100644 --- a/codex-rs/core/src/tools/code_mode/mod.rs +++ b/codex-rs/core/src/tools/code_mode/mod.rs @@ -258,14 +258,12 @@ async fn build_nested_router(exec: &ExecContext) -> ToolRouter { .await .list_all_tools() .await; - let mcp_tool_router_inputs = crate::tools::router::map_mcp_tool_infos(&mcp_tools); ToolRouter::from_config( &nested_tools_config, ToolRouterParams { deferred_mcp_tools: None, - mcp_tools: Some(mcp_tool_router_inputs.mcp_tools), - tool_namespaces: Some(mcp_tool_router_inputs.tool_namespaces), + mcp_tools: Some(mcp_tools), discoverable_tools: None, dynamic_tools: exec.turn.dynamic_tools.as_slice(), }, diff --git a/codex-rs/core/src/tools/js_repl/mod.rs b/codex-rs/core/src/tools/js_repl/mod.rs index a0dd6dba5a0a..f3acf13d5335 100644 --- a/codex-rs/core/src/tools/js_repl/mod.rs +++ b/codex-rs/core/src/tools/js_repl/mod.rs @@ -1561,14 +1561,12 @@ impl JsReplManager { .await .list_all_tools() .await; - let mcp_tool_router_inputs = crate::tools::router::map_mcp_tool_infos(&mcp_tools); let router = ToolRouter::from_config( &exec.turn.tools_config, crate::tools::router::ToolRouterParams { deferred_mcp_tools: None, - mcp_tools: Some(mcp_tool_router_inputs.mcp_tools), - tool_namespaces: Some(mcp_tool_router_inputs.tool_namespaces), + mcp_tools: Some(mcp_tools), discoverable_tools: None, dynamic_tools: exec.turn.dynamic_tools.as_slice(), }, diff --git a/codex-rs/core/src/tools/router.rs b/codex-rs/core/src/tools/router.rs index f4e71d4ce73c..ce59eebe0193 100644 --- a/codex-rs/core/src/tools/router.rs +++ b/codex-rs/core/src/tools/router.rs @@ -16,10 +16,8 @@ use codex_protocol::models::SearchToolCallParams; use codex_protocol::models::ShellToolCallParams; use codex_tools::ConfiguredToolSpec; use codex_tools::DiscoverableTool; -use codex_tools::ToolNamespace; use codex_tools::ToolSpec; use codex_tools::ToolsConfig; -use rmcp::model::Tool; use std::collections::HashMap; use std::sync::Arc; use tracing::instrument; @@ -41,45 +39,17 @@ pub struct ToolRouter { } pub(crate) struct ToolRouterParams<'a> { - pub(crate) mcp_tools: Option>, + pub(crate) mcp_tools: Option>, pub(crate) deferred_mcp_tools: Option>, - pub(crate) tool_namespaces: Option>, pub(crate) discoverable_tools: Option>, pub(crate) dynamic_tools: &'a [DynamicToolSpec], } -pub(crate) struct McpToolRouterInputs { - pub(crate) mcp_tools: HashMap, - pub(crate) tool_namespaces: HashMap, -} - -pub(crate) fn map_mcp_tool_infos(mcp_tools: &HashMap) -> McpToolRouterInputs { - McpToolRouterInputs { - mcp_tools: mcp_tools - .iter() - .map(|(name, tool)| (name.clone(), tool.tool.clone())) - .collect(), - tool_namespaces: mcp_tools - .iter() - .map(|(name, tool)| { - ( - name.clone(), - ToolNamespace { - name: tool.callable_namespace.clone(), - description: tool.server_instructions.clone(), - }, - ) - }) - .collect(), - } -} - impl ToolRouter { pub fn from_config(config: &ToolsConfig, params: ToolRouterParams<'_>) -> Self { let ToolRouterParams { mcp_tools, deferred_mcp_tools, - tool_namespaces, discoverable_tools, dynamic_tools, } = params; @@ -87,7 +57,6 @@ impl ToolRouter { config, mcp_tools, deferred_mcp_tools, - tool_namespaces, discoverable_tools, dynamic_tools, ); diff --git a/codex-rs/core/src/tools/router_tests.rs b/codex-rs/core/src/tools/router_tests.rs index ec8b7c2db0e6..e54981b3b1c3 100644 --- a/codex-rs/core/src/tools/router_tests.rs +++ b/codex-rs/core/src/tools/router_tests.rs @@ -26,13 +26,11 @@ async fn js_repl_tools_only_blocks_direct_tool_calls() -> anyhow::Result<()> { .list_all_tools() .await; let deferred_mcp_tools = Some(mcp_tools.clone()); - let mcp_tool_router_inputs = super::map_mcp_tool_infos(&mcp_tools); let router = ToolRouter::from_config( &turn.tools_config, ToolRouterParams { deferred_mcp_tools, - mcp_tools: Some(mcp_tool_router_inputs.mcp_tools), - tool_namespaces: Some(mcp_tool_router_inputs.tool_namespaces), + mcp_tools: Some(mcp_tools), discoverable_tools: None, dynamic_tools: turn.dynamic_tools.as_slice(), }, @@ -81,13 +79,11 @@ async fn js_repl_tools_only_allows_js_repl_source_calls() -> anyhow::Result<()> .list_all_tools() .await; let deferred_mcp_tools = Some(mcp_tools.clone()); - let mcp_tool_router_inputs = super::map_mcp_tool_infos(&mcp_tools); let router = ToolRouter::from_config( &turn.tools_config, ToolRouterParams { deferred_mcp_tools, - mcp_tools: Some(mcp_tool_router_inputs.mcp_tools), - tool_namespaces: Some(mcp_tool_router_inputs.tool_namespaces), + mcp_tools: Some(mcp_tools), discoverable_tools: None, dynamic_tools: turn.dynamic_tools.as_slice(), }, diff --git a/codex-rs/core/src/tools/spec.rs b/codex-rs/core/src/tools/spec.rs index ed124c0c47bd..bfad3cdb3531 100644 --- a/codex-rs/core/src/tools/spec.rs +++ b/codex-rs/core/src/tools/spec.rs @@ -29,11 +29,36 @@ pub(crate) fn tool_user_shell_type(user_shell: &Shell) -> ToolUserShellType { } } +struct McpToolPlanInputs { + mcp_tools: HashMap, + tool_namespaces: HashMap, +} + +fn map_mcp_tools_for_plan(mcp_tools: &HashMap) -> McpToolPlanInputs { + McpToolPlanInputs { + mcp_tools: mcp_tools + .iter() + .map(|(name, tool)| (name.clone(), tool.tool.clone())) + .collect(), + tool_namespaces: mcp_tools + .iter() + .map(|(name, tool)| { + ( + name.clone(), + ToolNamespace { + name: tool.callable_namespace.clone(), + description: tool.server_instructions.clone(), + }, + ) + }) + .collect(), + } +} + pub(crate) fn build_specs_with_discoverable_tools( config: &ToolsConfig, - mcp_tools: Option>, + mcp_tools: Option>, deferred_mcp_tools: Option>, - tool_namespaces: Option>, discoverable_tools: Option>, dynamic_tools: &[DynamicToolSpec], ) -> ToolRegistryBuilder { @@ -69,6 +94,7 @@ pub(crate) fn build_specs_with_discoverable_tools( use crate::tools::handlers::multi_agents_v2::WaitAgentHandler as WaitAgentHandlerV2; let mut builder = ToolRegistryBuilder::new(); + let mcp_tool_plan_inputs = mcp_tools.as_ref().map(map_mcp_tools_for_plan); let deferred_mcp_tool_sources = deferred_mcp_tools.as_ref().map(|tools| { tools .values() @@ -86,9 +112,13 @@ pub(crate) fn build_specs_with_discoverable_tools( let plan = build_tool_registry_plan( config, ToolRegistryPlanParams { - mcp_tools: mcp_tools.as_ref(), + mcp_tools: mcp_tool_plan_inputs + .as_ref() + .map(|inputs| &inputs.mcp_tools), deferred_mcp_tools: deferred_mcp_tool_sources.as_deref(), - tool_namespaces: tool_namespaces.as_ref(), + tool_namespaces: mcp_tool_plan_inputs + .as_ref() + .map(|inputs| &inputs.tool_namespaces), discoverable_tools: discoverable_tools.as_deref(), dynamic_tools, default_agent_type_description: &default_agent_type_description, diff --git a/codex-rs/core/src/tools/spec_tests.rs b/codex-rs/core/src/tools/spec_tests.rs index 25ccef0f19df..9e519607aefb 100644 --- a/codex-rs/core/src/tools/spec_tests.rs +++ b/codex-rs/core/src/tools/spec_tests.rs @@ -53,6 +53,20 @@ fn mcp_tool(name: &str, description: &str, input_schema: serde_json::Value) -> r } } +fn mcp_tool_info(tool: rmcp::model::Tool) -> ToolInfo { + ToolInfo { + server_name: "test_server".to_string(), + callable_name: tool.name.to_string(), + callable_namespace: "mcp__test_server__".to_string(), + server_instructions: None, + tool, + connector_id: None, + connector_name: None, + plugin_display_names: Vec::new(), + connector_description: None, + } +} + fn discoverable_connector(id: &str, name: &str, description: &str) -> DiscoverableTool { let slug = name.replace(' ', "-").to_lowercase(); DiscoverableTool::Connector(Box::new(AppInfo { @@ -208,7 +222,7 @@ 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>, + mcp_tools: Option>, deferred_mcp_tools: Option>, dynamic_tools: &[DynamicToolSpec], ) -> ToolRegistryBuilder { @@ -216,7 +230,6 @@ fn build_specs( config, mcp_tools, deferred_mcp_tools, - /*tool_namespaces*/ None, /*discoverable_tools*/ None, dynamic_tools, ) @@ -301,7 +314,6 @@ fn assert_model_tools( ToolRouterParams { mcp_tools: None, deferred_mcp_tools: None, - tool_namespaces: None, discoverable_tools: None, dynamic_tools: &[], }, @@ -709,7 +721,6 @@ fn tool_suggest_requires_apps_and_plugins_features() { &tools_config, /*mcp_tools*/ None, /*deferred_mcp_tools*/ None, - /*tool_namespaces*/ None, discoverable_tools.clone(), &[], ) @@ -916,7 +927,7 @@ fn test_mcp_tool_property_missing_type_defaults_to_string() { &tools_config, Some(HashMap::from([( "dash/search".to_string(), - mcp_tool( + mcp_tool_info(mcp_tool( "search", "Search docs", serde_json::json!({ @@ -925,7 +936,7 @@ fn test_mcp_tool_property_missing_type_defaults_to_string() { "query": {"description": "search query"} } }), - ), + )), )])), /*deferred_mcp_tools*/ None, &[], @@ -976,14 +987,14 @@ fn test_mcp_tool_preserves_integer_schema() { &tools_config, Some(HashMap::from([( "dash/paginate".to_string(), - mcp_tool( + mcp_tool_info(mcp_tool( "paginate", "Pagination", serde_json::json!({ "type": "object", "properties": {"page": {"type": "integer"}} }), - ), + )), )])), /*deferred_mcp_tools*/ None, &[], @@ -1035,14 +1046,14 @@ fn test_mcp_tool_array_without_items_gets_default_string_items() { &tools_config, Some(HashMap::from([( "dash/tags".to_string(), - mcp_tool( + mcp_tool_info(mcp_tool( "tags", "Tags", serde_json::json!({ "type": "object", "properties": {"tags": {"type": "array"}} }), - ), + )), )])), /*deferred_mcp_tools*/ None, &[], @@ -1096,7 +1107,7 @@ fn test_mcp_tool_anyof_defaults_to_string() { &tools_config, Some(HashMap::from([( "dash/value".to_string(), - mcp_tool( + mcp_tool_info(mcp_tool( "value", "AnyOf Value", serde_json::json!({ @@ -1105,7 +1116,7 @@ fn test_mcp_tool_anyof_defaults_to_string() { "value": {"anyOf": [{"type": "string"}, {"type": "number"}]} } }), - ), + )), )])), /*deferred_mcp_tools*/ None, &[], @@ -1161,7 +1172,7 @@ fn test_get_openai_tools_mcp_tools_with_additional_properties_schema() { &tools_config, Some(HashMap::from([( "test_server/do_something_cool".to_string(), - mcp_tool( + mcp_tool_info(mcp_tool( "do_something_cool", "Do something cool", serde_json::json!({ @@ -1187,7 +1198,7 @@ fn test_get_openai_tools_mcp_tools_with_additional_properties_schema() { } } }), - ), + )), )])), /*deferred_mcp_tools*/ None, &[],