From ff7513cd8318fbc908f5f9718f503a5ad6f56ed2 Mon Sep 17 00:00:00 2001 From: pakrym-oai Date: Tue, 26 May 2026 08:21:15 -0700 Subject: [PATCH] Move MCP tool naming mode into manager (#21576) ## Why The `non_prefixed_mcp_tool_names` feature should be applied where MCP tools become model-visible, not by remapping names later in core. Keeping the decision in `McpConnectionManager` construction makes `ToolInfo` the single shaped view that spec building, deferred tool search, routing, and unavailable-tool placeholders can consume directly. This also preserves the existing external behavior while the feature is off, and keeps the feature-on behavior for code mode and hooks explicit at the manager boundary. ## What Changed - Add `McpToolNameMode` to `codex-mcp` and flow it through `McpConfig` into `McpConnectionManager::new`. - Normalize MCP `ToolInfo` names in the manager using either legacy-prefixed namespaces or non-prefixed namespaces; the legacy path adds `mcp__` without restoring the old trailing namespace suffix. - Remove the core-side MCP name remapping path so specs, tool search, session resolution, and unavailable-tool placeholder construction use the manager-provided `ToolName` values directly. - Keep code mode flattening on the `__` namespace separator. - Preserve hook compatibility by giving non-prefixed MCP hook names legacy `mcp__...` matcher aliases. - Add/adjust integration and unit coverage for non-prefixed code-mode behavior, hook matching with the feature on and off, and manager-level legacy prefixing. ## Testing - `cargo test -p codex-mcp --lib` - `cargo test -p codex-core --lib tools::spec::tests -- --nocapture` - `cargo test -p codex-core --lib mcp_tools -- --nocapture` - `cargo test -p codex-core --lib mcp_tool_exposure -- --nocapture` - `cargo test -p codex-core --test all mcp_tool -- --nocapture` - `cargo test -p codex-core --test all search_tool -- --nocapture` - `cargo test -p codex-core --test all hooks_mcp -- --nocapture` - `cargo test -p codex-core --test all code_mode_uses_non_prefixed_mcp_tool_names_when_feature_enabled -- --nocapture` - `cargo test -p codex-tools` - `cargo test -p codex-features` --- .../app-server/tests/suite/v2/mcp_tool.rs | 2 +- codex-rs/codex-mcp/src/codex_apps.rs | 6 +- codex-rs/codex-mcp/src/connection_manager.rs | 21 +- .../codex-mcp/src/connection_manager_tests.rs | 210 +++++++++++++++--- codex-rs/codex-mcp/src/mcp/mod.rs | 5 + codex-rs/codex-mcp/src/mcp/mod_tests.rs | 1 + codex-rs/codex-mcp/src/tools.rs | 43 +++- codex-rs/core/config.schema.json | 6 + codex-rs/core/src/config/config_tests.rs | 21 ++ codex-rs/core/src/config/mod.rs | 5 + codex-rs/core/src/connectors.rs | 1 + codex-rs/core/src/mcp_tool_call.rs | 6 +- codex-rs/core/src/mcp_tool_call_tests.rs | 20 +- codex-rs/core/src/mcp_tool_exposure_test.rs | 6 +- codex-rs/core/src/session/mcp.rs | 1 + codex-rs/core/src/session/session.rs | 2 + codex-rs/core/src/session/tests.rs | 2 + codex-rs/core/src/tools/handlers/mcp.rs | 113 +++++++++- .../core/src/tools/handlers/tool_search.rs | 6 +- codex-rs/core/src/tools/spec_plan_tests.rs | 21 +- .../core/tests/common/apps_test_server.rs | 6 +- codex-rs/core/tests/suite/code_mode.rs | 65 +++++- codex-rs/core/tests/suite/hooks_mcp.rs | 126 ++++++++--- codex-rs/core/tests/suite/plugins.rs | 2 +- codex-rs/core/tests/suite/rmcp_client.rs | 28 +-- codex-rs/core/tests/suite/search_tool.rs | 14 +- codex-rs/core/tests/suite/sqlite_state.rs | 2 +- codex-rs/core/tests/suite/truncation.rs | 6 +- codex-rs/features/src/lib.rs | 8 + codex-rs/tools/src/code_mode.rs | 2 +- 30 files changed, 611 insertions(+), 146 deletions(-) diff --git a/codex-rs/app-server/tests/suite/v2/mcp_tool.rs b/codex-rs/app-server/tests/suite/v2/mcp_tool.rs index 46e9b2bf07..a21dd0d9f0 100644 --- a/codex-rs/app-server/tests/suite/v2/mcp_tool.rs +++ b/codex-rs/app-server/tests/suite/v2/mcp_tool.rs @@ -403,7 +403,7 @@ url = "{mcp_server_url}/mcp" #[tokio::test(flavor = "multi_thread", worker_threads = 2)] async fn mcp_tool_call_completion_notification_contains_truncated_large_result() -> Result<()> { let call_id = "call-large-mcp"; - let namespace = format!("mcp__{TEST_SERVER_NAME}__"); + let namespace = format!("mcp__{TEST_SERVER_NAME}"); let responses = vec![ responses::sse(vec![ responses::ev_response_created("resp-1"), diff --git a/codex-rs/codex-mcp/src/codex_apps.rs b/codex-rs/codex-mcp/src/codex_apps.rs index 772f3fc42f..eb88ffa6b1 100644 --- a/codex-rs/codex-mcp/src/codex_apps.rs +++ b/codex-rs/codex-mcp/src/codex_apps.rs @@ -20,7 +20,7 @@ use serde::Serialize; use sha1::Digest; use sha1::Sha1; -pub(crate) const CODEX_APPS_TOOLS_CACHE_SCHEMA_VERSION: u8 = 2; +pub(crate) const CODEX_APPS_TOOLS_CACHE_SCHEMA_VERSION: u8 = 3; #[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] pub struct CodexAppsToolsCacheKey { @@ -127,9 +127,9 @@ pub(crate) fn normalize_codex_apps_callable_namespace( if server_name == CODEX_APPS_MCP_SERVER_NAME && let Some(connector_name) = connector_name { - format!("mcp__{}__{}", server_name, sanitize_name(connector_name)) + format!("{}__{}", server_name, sanitize_name(connector_name)) } else { - format!("mcp__{server_name}__") + server_name.to_string() } } diff --git a/codex-rs/codex-mcp/src/connection_manager.rs b/codex-rs/codex-mcp/src/connection_manager.rs index f647aee1b9..91ed8b1b64 100644 --- a/codex-rs/codex-mcp/src/connection_manager.rs +++ b/codex-rs/codex-mcp/src/connection_manager.rs @@ -33,7 +33,7 @@ use crate::server::EffectiveMcpServer; use crate::server::McpServerMetadata; use crate::tools::ToolInfo; use crate::tools::filter_tools; -use crate::tools::normalize_tools_for_model; +use crate::tools::normalize_tools_for_model_with_prefix; use crate::tools::tool_with_model_visible_input_schema; use anyhow::Context; use anyhow::Result; @@ -73,6 +73,7 @@ pub struct McpConnectionManager { server_metadata: HashMap, tool_plugin_provenance: Arc, host_owned_codex_apps_enabled: bool, + prefix_mcp_tool_names: bool, elicitation_requests: ElicitationRequestManager, startup_cancellation_token: CancellationToken, } @@ -81,19 +82,26 @@ impl McpConnectionManager { pub fn new_uninitialized( approval_policy: &Constrained, permission_profile: &Constrained, + prefix_mcp_tool_names: bool, ) -> Self { - Self::new_uninitialized_with_permission_profile(approval_policy, permission_profile.get()) + Self::new_uninitialized_with_permission_profile( + approval_policy, + permission_profile.get(), + prefix_mcp_tool_names, + ) } pub fn new_uninitialized_with_permission_profile( approval_policy: &Constrained, permission_profile: &PermissionProfile, + prefix_mcp_tool_names: bool, ) -> Self { Self { clients: HashMap::new(), server_metadata: HashMap::new(), tool_plugin_provenance: Arc::new(ToolPluginProvenance::default()), host_owned_codex_apps_enabled: false, + prefix_mcp_tool_names, elicitation_requests: ElicitationRequestManager::new( approval_policy.value(), permission_profile.clone(), @@ -180,6 +188,7 @@ impl McpConnectionManager { codex_home: PathBuf, codex_apps_tools_cache_key: CodexAppsToolsCacheKey, host_owned_codex_apps_enabled: bool, + prefix_mcp_tool_names: bool, client_elicitation_capability: ElicitationCapability, tool_plugin_provenance: ToolPluginProvenance, auth: Option<&CodexAuth>, @@ -292,6 +301,7 @@ impl McpConnectionManager { server_metadata, tool_plugin_provenance, host_owned_codex_apps_enabled, + prefix_mcp_tool_names, elicitation_requests: elicitation_requests.clone(), startup_cancellation_token: cancel_token.clone(), }; @@ -381,7 +391,7 @@ impl McpConnectionManager { .map(|tool| self.with_server_metadata(tool)), ); } - normalize_tools_for_model(tools) + normalize_tools_for_model_with_prefix(tools, self.prefix_mcp_tool_names) } /// Force-refresh codex apps tools by bypassing the in-process cache. @@ -432,7 +442,10 @@ impl McpConnectionManager { tool.tool = tool_with_model_visible_input_schema(&tool.tool); self.with_server_metadata(tool) }); - Ok(normalize_tools_for_model(tools)) + Ok(normalize_tools_for_model_with_prefix( + tools, + self.prefix_mcp_tool_names, + )) } fn with_server_metadata(&self, mut tool: ToolInfo) -> ToolInfo { diff --git a/codex-rs/codex-mcp/src/connection_manager_tests.rs b/codex-rs/codex-mcp/src/connection_manager_tests.rs index 181d95d0ec..bf524cceaa 100644 --- a/codex-rs/codex-mcp/src/connection_manager_tests.rs +++ b/codex-rs/codex-mcp/src/connection_manager_tests.rs @@ -14,7 +14,7 @@ use crate::server::McpServerOrigin; use crate::tools::ToolFilter; use crate::tools::ToolInfo; use crate::tools::filter_tools; -use crate::tools::normalize_tools_for_model; +use crate::tools::normalize_tools_for_model_with_prefix; use crate::tools::tool_with_model_visible_input_schema; use codex_config::Constrained; use codex_config::McpServerConfig; @@ -37,13 +37,12 @@ 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(), supports_parallel_tool_calls: false, server_origin: None, callable_name: tool_name.to_string(), - callable_namespace: tool_namespace, + callable_namespace: server_name.to_string(), namespace_description: None, tool: Tool { name: tool_name.to_string().into(), @@ -97,7 +96,10 @@ fn model_tool_names(tools: &[ToolInfo]) -> HashSet { } fn model_tool_name_len(name: &ToolName) -> usize { - name.namespace.as_deref().map_or(0, str::len) + name.name.len() + name.namespace + .as_deref() + .map_or(0, |namespace| namespace.len() + "__".len()) + + name.name.len() } fn is_code_mode_compatible_tool_name(name: &ToolName) -> bool { @@ -301,13 +303,14 @@ fn test_normalize_tools_short_non_duplicated_names() { create_test_tool("server1", "tool2"), ]; - let model_tools = normalize_tools_for_model(tools); + let model_tools = + normalize_tools_for_model_with_prefix(tools, /*prefix_mcp_tool_names*/ true); assert_eq!( model_tool_names(&model_tools), HashSet::from([ - ToolName::namespaced("mcp__server1__", "tool1"), - ToolName::namespaced("mcp__server1__", "tool2") + ToolName::namespaced("mcp__server1", "tool1"), + ToolName::namespaced("mcp__server1", "tool2") ]) ); } @@ -319,12 +322,13 @@ fn test_normalize_tools_duplicated_names_skipped() { create_test_tool("server1", "duplicate_tool"), ]; - let model_tools = normalize_tools_for_model(tools); + let model_tools = + normalize_tools_for_model_with_prefix(tools, /*prefix_mcp_tool_names*/ true); // Only the first tool should remain, the second is skipped assert_eq!( model_tool_names(&model_tools), - HashSet::from([ToolName::namespaced("mcp__server1__", "duplicate_tool")]) + HashSet::from([ToolName::namespaced("mcp__server1", "duplicate_tool")]) ); } @@ -343,7 +347,8 @@ fn test_normalize_tools_long_names_same_server() { ), ]; - let model_tools = normalize_tools_for_model(tools); + let model_tools = + normalize_tools_for_model_with_prefix(tools, /*prefix_mcp_tool_names*/ true); assert_eq!(model_tools.len(), 2); @@ -353,7 +358,7 @@ fn test_normalize_tools_long_names_same_server() { assert!( names .iter() - .all(|name| name.namespace.as_deref() == Some("mcp__my_server__")) + .all(|name| name.namespace.as_deref() == Some("mcp__my_server")) ); assert!( names.iter().all(is_code_mode_compatible_tool_name), @@ -365,14 +370,15 @@ fn test_normalize_tools_long_names_same_server() { fn test_normalize_tools_sanitizes_invalid_characters() { let tools = vec![create_test_tool("server.one", "tool.two-three")]; - let model_tools = normalize_tools_for_model(tools); + let model_tools = + normalize_tools_for_model_with_prefix(tools, /*prefix_mcp_tool_names*/ true); assert_eq!(model_tools.len(), 1); let tool = model_tools.into_iter().next().expect("one tool"); let model_name = tool.canonical_tool_name(); assert_eq!( model_name, - ToolName::namespaced("mcp__server_one__", "tool_two_three") + ToolName::namespaced("mcp__server_one", "tool_two_three") ); assert_eq!( ToolName::namespaced(tool.callable_namespace.clone(), tool.callable_name.clone()), @@ -381,7 +387,7 @@ fn test_normalize_tools_sanitizes_invalid_characters() { // The callable parts are sanitized for model-visible tool calls, but the raw // MCP name is preserved for the actual MCP call. assert_eq!(tool.server_name, "server.one"); - assert_eq!(tool.callable_namespace, "mcp__server_one__"); + assert_eq!(tool.callable_namespace, "mcp__server_one"); assert_eq!(tool.callable_name, "tool_two_three"); assert_eq!(tool.tool.name, "tool.two-three"); @@ -395,19 +401,59 @@ fn test_normalize_tools_sanitizes_invalid_characters() { fn test_normalize_tools_keeps_hyphenated_mcp_tools_callable() { let tools = vec![create_test_tool("music-studio", "get-strudel-guide")]; - let model_tools = normalize_tools_for_model(tools); + let model_tools = + normalize_tools_for_model_with_prefix(tools, /*prefix_mcp_tool_names*/ true); assert_eq!(model_tools.len(), 1); let tool = model_tools.into_iter().next().expect("one tool"); assert_eq!( tool.canonical_tool_name(), - ToolName::namespaced("mcp__music_studio__", "get_strudel_guide") + ToolName::namespaced("mcp__music_studio", "get_strudel_guide") ); - assert_eq!(tool.callable_namespace, "mcp__music_studio__"); + 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_normalize_tools_disambiguates_reserved_unprefixed_namespaces() { + let tools = vec![ + create_test_tool("tools", "list"), + create_test_tool("web", "search"), + ]; + + let model_tools = + normalize_tools_for_model_with_prefix(tools, /*prefix_mcp_tool_names*/ false); + + assert_eq!(model_tools.len(), 2); + let namespaces = model_tools + .iter() + .map(|tool| tool.callable_namespace.as_str()) + .collect::>(); + assert_eq!(namespaces.len(), 2); + assert!( + namespaces + .iter() + .all(|namespace| !matches!(*namespace, "tools" | "web")), + "reserved namespaces should be disambiguated: {namespaces:?}" + ); + assert!( + namespaces + .iter() + .any(|namespace| namespace.starts_with("tools_")) + ); + assert!( + namespaces + .iter() + .any(|namespace| namespace.starts_with("web_")) + ); + let model_names = model_tool_names(&model_tools); + assert!( + model_names.iter().all(is_code_mode_compatible_tool_name), + "model-visible names must be code-mode compatible: {model_names:?}" + ); +} + #[test] fn test_normalize_tools_disambiguates_sanitized_namespace_collisions() { let tools = vec![ @@ -415,7 +461,8 @@ fn test_normalize_tools_disambiguates_sanitized_namespace_collisions() { create_test_tool("basic_server", "query"), ]; - let model_tools = normalize_tools_for_model(tools); + let model_tools = + normalize_tools_for_model_with_prefix(tools, /*prefix_mcp_tool_names*/ true); assert_eq!(model_tools.len(), 2); let mut namespaces = model_tools @@ -445,7 +492,8 @@ fn test_normalize_tools_disambiguates_sanitized_tool_name_collisions() { create_test_tool("server", "tool_name"), ]; - let model_tools = normalize_tools_for_model(tools); + let model_tools = + normalize_tools_for_model_with_prefix(tools, /*prefix_mcp_tool_names*/ true); assert_eq!(model_tools.len(), 2); let raw_tool_names = model_tools @@ -691,8 +739,11 @@ async fn list_all_tools_uses_startup_snapshot_while_client_is_pending() { .shared(); let approval_policy = Constrained::allow_any(AskForApproval::OnFailure); let permission_profile = Constrained::allow_any(PermissionProfile::default()); - let mut manager = - McpConnectionManager::new_uninitialized(&approval_policy, &permission_profile); + let mut manager = McpConnectionManager::new_uninitialized( + &approval_policy, + &permission_profile, + /*prefix_mcp_tool_names*/ true, + ); manager.clients.insert( CODEX_APPS_MCP_SERVER_NAME.to_string(), AsyncManagedClient { @@ -709,13 +760,97 @@ async fn list_all_tools_uses_startup_snapshot_while_client_is_pending() { .iter() .find(|tool| { tool.canonical_tool_name() - == ToolName::namespaced("mcp__codex_apps__", "calendar_create_event") + == ToolName::namespaced("mcp__codex_apps", "calendar_create_event") }) .expect("tool from startup cache"); assert_eq!(tool.server_name, CODEX_APPS_MCP_SERVER_NAME); assert_eq!(tool.callable_name, "calendar_create_event"); } +#[tokio::test] +async fn list_all_tools_accepts_canonical_namespaced_tool_names() { + let startup_tools = vec![create_test_tool("rmcp", "echo")]; + let pending_client = futures::future::pending::>() + .boxed() + .shared(); + let approval_policy = Constrained::allow_any(AskForApproval::OnFailure); + let permission_profile = Constrained::allow_any(PermissionProfile::default()); + let mut manager = McpConnectionManager::new_uninitialized( + &approval_policy, + &permission_profile, + /*prefix_mcp_tool_names*/ false, + ); + manager.clients.insert( + "rmcp".to_string(), + AsyncManagedClient { + client: pending_client, + startup_snapshot: Some(startup_tools), + startup_complete: Arc::new(std::sync::atomic::AtomicBool::new(false)), + tool_plugin_provenance: Arc::new(ToolPluginProvenance::default()), + cancel_token: CancellationToken::new(), + }, + ); + + let tools = manager.list_all_tools().await; + let tool = tools + .iter() + .find(|tool| tool.canonical_tool_name() == ToolName::namespaced("rmcp", "echo")) + .expect("split MCP tool namespace and name should resolve"); + + let expected = ("rmcp", "rmcp", "echo", "echo"); + assert_eq!( + ( + tool.server_name.as_str(), + tool.callable_namespace.as_str(), + tool.callable_name.as_str(), + tool.tool.name.as_ref(), + ), + expected + ); +} + +#[tokio::test] +async fn list_all_tools_applies_legacy_mcp_prefix_by_default() { + let startup_tools = vec![create_test_tool("rmcp", "echo")]; + let pending_client = futures::future::pending::>() + .boxed() + .shared(); + let approval_policy = Constrained::allow_any(AskForApproval::OnFailure); + let permission_profile = Constrained::allow_any(PermissionProfile::default()); + let mut manager = McpConnectionManager::new_uninitialized( + &approval_policy, + &permission_profile, + /*prefix_mcp_tool_names*/ true, + ); + manager.clients.insert( + "rmcp".to_string(), + AsyncManagedClient { + client: pending_client, + startup_snapshot: Some(startup_tools), + startup_complete: Arc::new(std::sync::atomic::AtomicBool::new(false)), + tool_plugin_provenance: Arc::new(ToolPluginProvenance::default()), + cancel_token: CancellationToken::new(), + }, + ); + + let tools = manager.list_all_tools().await; + let tool = tools + .iter() + .find(|tool| tool.canonical_tool_name() == ToolName::namespaced("mcp__rmcp", "echo")) + .expect("legacy-prefixed MCP tool name should resolve"); + + let expected = ("rmcp", "mcp__rmcp", "echo", "echo"); + assert_eq!( + ( + tool.server_name.as_str(), + tool.callable_namespace.as_str(), + tool.callable_name.as_str(), + tool.tool.name.as_ref(), + ), + expected + ); +} + #[tokio::test] async fn list_all_tools_blocks_while_client_is_pending_without_startup_snapshot() { let pending_client = futures::future::pending::>() @@ -723,8 +858,11 @@ async fn list_all_tools_blocks_while_client_is_pending_without_startup_snapshot( .shared(); let approval_policy = Constrained::allow_any(AskForApproval::OnFailure); let permission_profile = Constrained::allow_any(PermissionProfile::default()); - let mut manager = - McpConnectionManager::new_uninitialized(&approval_policy, &permission_profile); + let mut manager = McpConnectionManager::new_uninitialized( + &approval_policy, + &permission_profile, + /*prefix_mcp_tool_names*/ true, + ); manager.clients.insert( CODEX_APPS_MCP_SERVER_NAME.to_string(), AsyncManagedClient { @@ -748,8 +886,11 @@ async fn list_all_tools_does_not_block_when_startup_snapshot_cache_hit_is_empty( .shared(); let approval_policy = Constrained::allow_any(AskForApproval::OnFailure); let permission_profile = Constrained::allow_any(PermissionProfile::default()); - let mut manager = - McpConnectionManager::new_uninitialized(&approval_policy, &permission_profile); + let mut manager = McpConnectionManager::new_uninitialized( + &approval_policy, + &permission_profile, + /*prefix_mcp_tool_names*/ true, + ); manager.clients.insert( CODEX_APPS_MCP_SERVER_NAME.to_string(), AsyncManagedClient { @@ -782,8 +923,11 @@ async fn list_all_tools_uses_startup_snapshot_when_client_startup_fails() { .shared(); let approval_policy = Constrained::allow_any(AskForApproval::OnFailure); let permission_profile = Constrained::allow_any(PermissionProfile::default()); - let mut manager = - McpConnectionManager::new_uninitialized(&approval_policy, &permission_profile); + let mut manager = McpConnectionManager::new_uninitialized( + &approval_policy, + &permission_profile, + /*prefix_mcp_tool_names*/ true, + ); let startup_complete = Arc::new(std::sync::atomic::AtomicBool::new(true)); manager.clients.insert( CODEX_APPS_MCP_SERVER_NAME.to_string(), @@ -801,7 +945,7 @@ async fn list_all_tools_uses_startup_snapshot_when_client_startup_fails() { .iter() .find(|tool| { tool.canonical_tool_name() - == ToolName::namespaced("mcp__codex_apps__", "calendar_create_event") + == ToolName::namespaced("mcp__codex_apps", "calendar_create_event") }) .expect("tool from startup cache"); assert_eq!(tool.server_name, CODEX_APPS_MCP_SERVER_NAME); @@ -817,8 +961,11 @@ async fn list_all_tools_adds_server_metadata_to_cached_tools() { .shared(); let approval_policy = Constrained::allow_any(AskForApproval::OnFailure); let permission_profile = Constrained::allow_any(PermissionProfile::default()); - let mut manager = - McpConnectionManager::new_uninitialized(&approval_policy, &permission_profile); + let mut manager = McpConnectionManager::new_uninitialized( + &approval_policy, + &permission_profile, + /*prefix_mcp_tool_names*/ true, + ); manager.server_metadata.insert( server_name.to_string(), McpServerMetadata { @@ -927,6 +1074,7 @@ async fn no_local_runtime_fails_local_stdio_but_keeps_local_http_server() { is_workspace_account: false, }, /*host_owned_codex_apps_enabled*/ false, + /*prefix_mcp_tool_names*/ true, ElicitationCapability::default(), ToolPluginProvenance::default(), /*auth*/ None, diff --git a/codex-rs/codex-mcp/src/mcp/mod.rs b/codex-rs/codex-mcp/src/mcp/mod.rs index 1a06d9b9a0..407ece427e 100644 --- a/codex-rs/codex-mcp/src/mcp/mod.rs +++ b/codex-rs/codex-mcp/src/mcp/mod.rs @@ -132,6 +132,9 @@ pub struct McpConfig { /// ChatGPT auth is checked separately at runtime before the host-owned apps /// MCP server is added. pub apps_enabled: bool, + /// Whether model-visible MCP tool namespaces should keep the legacy + /// `mcp__` prefix. + pub prefix_mcp_tool_names: bool, /// Client-side elicitation capabilities advertised during MCP initialization. pub client_elicitation_capability: ElicitationCapability, /// Config-backed MCP servers keyed by server name. @@ -288,6 +291,7 @@ pub async fn read_mcp_resource( config.codex_home.clone(), codex_apps_tools_cache_key(auth), host_owned_codex_apps_enabled, + config.prefix_mcp_tool_names, config.client_elicitation_capability.clone(), tool_plugin_provenance(config), auth, @@ -361,6 +365,7 @@ pub async fn collect_mcp_server_status_snapshot_with_detail( config.codex_home.clone(), codex_apps_tools_cache_key(auth), host_owned_codex_apps_enabled, + config.prefix_mcp_tool_names, config.client_elicitation_capability.clone(), tool_plugin_provenance, auth, diff --git a/codex-rs/codex-mcp/src/mcp/mod_tests.rs b/codex-rs/codex-mcp/src/mcp/mod_tests.rs index a8260de6c7..d4d888f202 100644 --- a/codex-rs/codex-mcp/src/mcp/mod_tests.rs +++ b/codex-rs/codex-mcp/src/mcp/mod_tests.rs @@ -28,6 +28,7 @@ fn test_mcp_config(codex_home: PathBuf) -> McpConfig { codex_linux_sandbox_exe: None, use_legacy_landlock: false, apps_enabled: false, + prefix_mcp_tool_names: true, client_elicitation_capability: ElicitationCapability::default(), configured_mcp_servers: HashMap::new(), plugin_ids_by_mcp_server_name: HashMap::new(), diff --git a/codex-rs/codex-mcp/src/tools.rs b/codex-rs/codex-mcp/src/tools.rs index 5b2fba2ef8..4f6e5775d4 100644 --- a/codex-rs/codex-mcp/src/tools.rs +++ b/codex-rs/codex-mcp/src/tools.rs @@ -25,6 +25,8 @@ use crate::mcp::sanitize_responses_api_tool_name; pub(crate) const MCP_TOOLS_CACHE_WRITE_DURATION_METRIC: &str = "codex.mcp.tools.cache_write.duration_ms"; +const LEGACY_MCP_TOOL_NAME_PREFIX: &str = "mcp__"; + #[derive(Debug, Clone, Serialize, Deserialize)] pub struct ToolInfo { /// Raw MCP server name used for routing the tool call. @@ -141,7 +143,13 @@ pub(crate) fn filter_tools(tools: Vec, filter: &ToolFilter) -> Vec(tools: I) -> Vec +/// +/// When `prefix_mcp_tool_names` is true, the historical `mcp__` namespace +/// prefix is added without restoring the old trailing `__` namespace suffix. +pub(crate) fn normalize_tools_for_model_with_prefix( + tools: I, + prefix_mcp_tool_names: bool, +) -> Vec where I: IntoIterator, { @@ -163,8 +171,19 @@ where continue; } + let mut callable_namespace = callable_namespace_with_prefix( + &sanitize_responses_api_tool_name(&tool.callable_namespace), + prefix_mcp_tool_names, + ); + if !prefix_mcp_tool_names + && RESERVED_UNPREFIXED_MCP_NAMESPACES.contains(&callable_namespace.as_str()) + { + callable_namespace = + append_namespace_hash_suffix(&callable_namespace, &raw_namespace_identity); + } + candidates.push(CallableToolCandidate { - callable_namespace: sanitize_responses_api_tool_name(&tool.callable_namespace), + callable_namespace, callable_name: sanitize_responses_api_tool_name(&tool.callable_name), raw_namespace_identity, raw_tool_identity, @@ -226,6 +245,7 @@ where &candidate.callable_name, &candidate.raw_tool_identity, &mut used_names, + MCP_TOOL_NAME_DELIMITER.len(), ); candidate.tool.callable_namespace = callable_namespace; candidate.tool.callable_name = callable_name; @@ -247,6 +267,15 @@ const MCP_TOOL_NAME_DELIMITER: &str = "__"; const MAX_TOOL_NAME_LENGTH: usize = 64; const CALLABLE_NAME_HASH_LEN: usize = 12; const META_OPENAI_FILE_PARAMS: &str = "openai/fileParams"; +const RESERVED_UNPREFIXED_MCP_NAMESPACES: &[&str] = &["tools", "web"]; + +fn callable_namespace_with_prefix(namespace: &str, prefix_mcp_tool_names: bool) -> String { + if !prefix_mcp_tool_names || namespace.starts_with(LEGACY_MCP_TOOL_NAME_PREFIX) { + namespace.to_string() + } else { + format!("{LEGACY_MCP_TOOL_NAME_PREFIX}{namespace}") + } +} fn mask_input_schema_for_file_path_params(input_schema: &mut JsonValue, file_params: &[String]) { let Some(properties) = input_schema @@ -331,9 +360,10 @@ fn fit_callable_parts_with_hash( namespace: &str, tool_name: &str, raw_identity: &str, + reserved_len: usize, ) -> (String, String) { let suffix = callable_name_hash_suffix(raw_identity); - let max_tool_len = MAX_TOOL_NAME_LENGTH.saturating_sub(namespace.len()); + let max_tool_len = MAX_TOOL_NAME_LENGTH.saturating_sub(namespace.len() + reserved_len); if max_tool_len >= suffix.len() { let prefix_len = max_tool_len - suffix.len(); return ( @@ -342,7 +372,7 @@ fn fit_callable_parts_with_hash( ); } - let max_namespace_len = MAX_TOOL_NAME_LENGTH - suffix.len(); + let max_namespace_len = MAX_TOOL_NAME_LENGTH.saturating_sub(suffix.len() + reserved_len); (truncate_name(namespace, max_namespace_len), suffix) } @@ -351,9 +381,10 @@ fn unique_callable_parts( tool_name: &str, raw_identity: &str, used_names: &mut HashSet, + reserved_len: usize, ) -> (String, String) { let model_name = format!("{namespace}{tool_name}"); - if model_name.len() <= MAX_TOOL_NAME_LENGTH && used_names.insert(model_name) { + if model_name.len() + reserved_len <= MAX_TOOL_NAME_LENGTH && used_names.insert(model_name) { return (namespace.to_string(), tool_name.to_string()); } @@ -365,7 +396,7 @@ fn unique_callable_parts( format!("{raw_identity}\0{attempt}") }; let (namespace, tool_name) = - fit_callable_parts_with_hash(namespace, tool_name, &hash_input); + fit_callable_parts_with_hash(namespace, tool_name, &hash_input, reserved_len); let model_name = format!("{namespace}{tool_name}"); if used_names.insert(model_name) { return (namespace, tool_name); diff --git a/codex-rs/core/config.schema.json b/codex-rs/core/config.schema.json index 8b653609de..88d467e09d 100644 --- a/codex-rs/core/config.schema.json +++ b/codex-rs/core/config.schema.json @@ -500,6 +500,9 @@ "network_proxy": { "$ref": "#/definitions/FeatureToml_for_NetworkProxyConfigToml" }, + "non_prefixed_mcp_tool_names": { + "type": "boolean" + }, "personality": { "type": "boolean" }, @@ -4423,6 +4426,9 @@ "network_proxy": { "$ref": "#/definitions/FeatureToml_for_NetworkProxyConfigToml" }, + "non_prefixed_mcp_tool_names": { + "type": "boolean" + }, "personality": { "type": "boolean" }, diff --git a/codex-rs/core/src/config/config_tests.rs b/codex-rs/core/src/config/config_tests.rs index 58b6dcc0da..59937106c4 100644 --- a/codex-rs/core/src/config/config_tests.rs +++ b/codex-rs/core/src/config/config_tests.rs @@ -5304,6 +5304,27 @@ async fn to_mcp_config_preserves_apps_feature_from_config() -> std::io::Result<( Ok(()) } +#[tokio::test] +async fn to_mcp_config_flows_mcp_tool_prefix_from_feature() -> std::io::Result<()> { + let codex_home = TempDir::new()?; + let mut config = Config::load_from_base_config_with_overrides( + ConfigToml::default(), + ConfigOverrides::default(), + codex_home.abs(), + ) + .await?; + let plugins_manager = PluginsManager::new(codex_home.path().to_path_buf()); + + let mcp_config = config.to_mcp_config(&plugins_manager).await; + assert!(mcp_config.prefix_mcp_tool_names); + + let _ = config.features.enable(Feature::NonPrefixedMcpToolNames); + let mcp_config = config.to_mcp_config(&plugins_manager).await; + assert!(!mcp_config.prefix_mcp_tool_names); + + Ok(()) +} + #[tokio::test] async fn to_mcp_config_preserves_auth_elicitation_feature_from_config() -> std::io::Result<()> { let codex_home = TempDir::new()?; diff --git a/codex-rs/core/src/config/mod.rs b/codex-rs/core/src/config/mod.rs index 261850c8a0..97e0fd993f 100644 --- a/codex-rs/core/src/config/mod.rs +++ b/codex-rs/core/src/config/mod.rs @@ -1334,6 +1334,7 @@ impl Config { codex_linux_sandbox_exe: self.codex_linux_sandbox_exe.clone(), use_legacy_landlock: self.features.use_legacy_landlock(), apps_enabled: self.features.enabled(Feature::Apps), + prefix_mcp_tool_names: self.prefix_mcp_tool_names(), client_elicitation_capability: if self.features.enabled(Feature::AuthElicitation) { ElicitationCapability { form: Some(FormElicitationCapability::default()), @@ -1350,6 +1351,10 @@ impl Config { } } + pub(crate) fn prefix_mcp_tool_names(&self) -> bool { + !self.features.enabled(Feature::NonPrefixedMcpToolNames) + } + pub async fn rebuild_preserving_session_layers( &self, refreshed_config: &Config, diff --git a/codex-rs/core/src/connectors.rs b/codex-rs/core/src/connectors.rs index 8593686c70..09717bb121 100644 --- a/codex-rs/core/src/connectors.rs +++ b/codex-rs/core/src/connectors.rs @@ -276,6 +276,7 @@ pub async fn list_accessible_connectors_from_mcp_tools_with_environment_manager( config.codex_home.to_path_buf(), codex_apps_tools_cache_key(auth.as_ref()), host_owned_codex_apps_enabled, + mcp_config.prefix_mcp_tool_names, mcp_config.client_elicitation_capability, ToolPluginProvenance::default(), auth.as_ref(), diff --git a/codex-rs/core/src/mcp_tool_call.rs b/codex-rs/core/src/mcp_tool_call.rs index 5888acad31..e514fa0d03 100644 --- a/codex-rs/core/src/mcp_tool_call.rs +++ b/codex-rs/core/src/mcp_tool_call.rs @@ -110,7 +110,7 @@ pub(crate) async fn handle_mcp_tool_call( call_id: String, server: String, tool_name: String, - hook_tool_name: String, + hook_tool_name: HookToolName, arguments: String, ) -> HandledMcpToolCall { // Parse the `arguments` as JSON. An empty string is OK, but invalid JSON @@ -1158,7 +1158,7 @@ async fn maybe_request_mcp_tool_approval( turn_context: &Arc, call_id: &str, invocation: &McpInvocation, - hook_tool_name: &str, + hook_tool_name: &HookToolName, metadata: Option<&McpToolApprovalMetadata>, approval_mode: AppToolApproval, ) -> Option { @@ -1193,7 +1193,7 @@ async fn maybe_request_mcp_tool_approval( turn_context, call_id, PermissionRequestPayload { - tool_name: HookToolName::new(hook_tool_name), + tool_name: hook_tool_name.clone(), tool_input: invocation .arguments .clone() diff --git a/codex-rs/core/src/mcp_tool_call_tests.rs b/codex-rs/core/src/mcp_tool_call_tests.rs index 0d6b8ff1ac..de5276b26e 100644 --- a/codex-rs/core/src/mcp_tool_call_tests.rs +++ b/codex-rs/core/src/mcp_tool_call_tests.rs @@ -5,6 +5,7 @@ use crate::session::tests::make_session_and_context; use crate::session::tests::make_session_and_context_with_rx; use crate::state::ActiveTurn; use crate::test_support::models_manager_with_provider; +use crate::tools::hook_names::HookToolName; use crate::turn_metadata::McpTurnMetadataContext; use codex_config::CONFIG_TOML_FILE; use codex_config::config_toml::ConfigToml; @@ -1270,6 +1271,7 @@ async fn install_host_owned_codex_apps_manager(session: &Session, turn_context: turn_context.config.codex_home.to_path_buf(), codex_mcp::codex_apps_tools_cache_key(auth.as_ref()), /*host_owned_codex_apps_enabled*/ true, + turn_context.config.prefix_mcp_tool_names(), rmcp::model::ElicitationCapability::default(), codex_mcp::ToolPluginProvenance::default(), auth.as_ref(), @@ -2293,7 +2295,7 @@ async fn approve_mode_skips_when_annotations_do_not_require_approval() { &turn_context, "call-1", &invocation, - "mcp__test__tool", + &HookToolName::new("mcp__test__tool"), Some(&metadata), AppToolApproval::Approve, ) @@ -2367,7 +2369,7 @@ async fn guardian_mode_skips_auto_when_annotations_do_not_require_approval() { &turn_context, "call-guardian", &invocation, - "mcp__test__tool", + &HookToolName::new("mcp__test__tool"), Some(&metadata), AppToolApproval::Auto, ) @@ -2424,7 +2426,7 @@ async fn permission_request_hook_allows_mcp_tool_call() { &turn_context, "call-mcp-hook", &invocation, - "mcp__memory__create_entities", + &HookToolName::new("mcp__memory__create_entities"), Some(&metadata), AppToolApproval::Auto, ) @@ -2486,7 +2488,7 @@ async fn permission_request_hook_uses_hook_tool_name_without_metadata() { &turn_context, "call-mcp-hook-no-metadata", &invocation, - "mcp__memory__create_entities", + &HookToolName::new("mcp__memory__create_entities"), /*metadata*/ None, AppToolApproval::Auto, ) @@ -2566,7 +2568,7 @@ async fn permission_request_hook_runs_after_remembered_mcp_approval() { &turn_context, "call-mcp-remembered", &invocation, - "mcp__memory__create_entities", + &HookToolName::new("mcp__memory__create_entities"), Some(&metadata), AppToolApproval::Auto, ) @@ -2647,7 +2649,7 @@ async fn guardian_mode_mcp_denial_returns_rationale_message() { &turn_context, "call-guardian-deny", &invocation, - "mcp__test__tool", + &HookToolName::new("mcp__test__tool"), Some(&metadata), AppToolApproval::Auto, ) @@ -2705,7 +2707,7 @@ async fn prompt_mode_waits_for_approval_when_annotations_do_not_require_approval &turn_context, "call-prompt", &invocation, - "mcp__test__tool", + &HookToolName::new("mcp__test__tool"), Some(&metadata), AppToolApproval::Prompt, ) @@ -2761,7 +2763,7 @@ async fn full_access_mode_skips_mcp_tool_approval_for_all_approval_modes() { &turn_context, "call-2", &invocation, - "mcp__test__tool", + &HookToolName::new("mcp__test__tool"), Some(&metadata), approval_mode, ) @@ -2849,7 +2851,7 @@ async fn approve_mode_skips_guardian_in_every_permission_mode() { &turn_context, "call-3", &invocation, - "mcp__test__tool", + &HookToolName::new("mcp__test__tool"), Some(&metadata), AppToolApproval::Approve, ) diff --git a/codex-rs/core/src/mcp_tool_exposure_test.rs b/codex-rs/core/src/mcp_tool_exposure_test.rs index fba7fe1f84..367baaf4a0 100644 --- a/codex-rs/core/src/mcp_tool_exposure_test.rs +++ b/codex-rs/core/src/mcp_tool_exposure_test.rs @@ -70,7 +70,7 @@ fn numbered_mcp_tools(count: usize) -> Vec { make_mcp_tool( "rmcp", &tool_name, - "mcp__rmcp__", + "mcp__rmcp", &tool_name, /*connector_id*/ None, /*connector_name*/ None, @@ -127,7 +127,7 @@ async fn always_defer_feature_defers_apps_too() { make_mcp_tool( "rmcp", "tool", - "mcp__rmcp__", + "mcp__rmcp", "tool", /*connector_id*/ None, /*connector_name*/ None, @@ -156,7 +156,7 @@ async fn always_defer_feature_defers_apps_too() { .as_ref() .expect("MCP tools should be discoverable through tool_search"); let deferred_tool_names = tool_names(deferred_tools); - assert!(deferred_tool_names.contains(&ToolName::namespaced("mcp__rmcp__", "tool"))); + assert!(deferred_tool_names.contains(&ToolName::namespaced("mcp__rmcp", "tool"))); assert!(deferred_tool_names.contains(&ToolName::namespaced( "mcp__codex_apps__calendar", "_create_event" diff --git a/codex-rs/core/src/session/mcp.rs b/codex-rs/core/src/session/mcp.rs index 40cb45d2ac..a775c8b836 100644 --- a/codex-rs/core/src/session/mcp.rs +++ b/codex-rs/core/src/session/mcp.rs @@ -353,6 +353,7 @@ impl Session { config.codex_home.to_path_buf(), codex_apps_tools_cache_key(auth.as_ref()), host_owned_codex_apps_enabled, + mcp_config.prefix_mcp_tool_names, mcp_config.client_elicitation_capability, tool_plugin_provenance, auth.as_ref(), diff --git a/codex-rs/core/src/session/session.rs b/codex-rs/core/src/session/session.rs index e430202ae1..75eb7d0653 100644 --- a/codex-rs/core/src/session/session.rs +++ b/codex-rs/core/src/session/session.rs @@ -978,6 +978,7 @@ impl Session { McpConnectionManager::new_uninitialized_with_permission_profile( &config.permissions.approval_policy, config.permissions.permission_profile(), + config.prefix_mcp_tool_names(), ), )), mcp_startup_cancellation_token: Mutex::new(CancellationToken::new()), @@ -1156,6 +1157,7 @@ impl Session { config.codex_home.to_path_buf(), codex_apps_tools_cache_key(auth), host_owned_codex_apps_enabled, + config.prefix_mcp_tool_names(), client_elicitation_capability, tool_plugin_provenance, auth, diff --git a/codex-rs/core/src/session/tests.rs b/codex-rs/core/src/session/tests.rs index ac62a3482c..428e762244 100644 --- a/codex-rs/core/src/session/tests.rs +++ b/codex-rs/core/src/session/tests.rs @@ -4519,6 +4519,7 @@ pub(crate) async fn make_session_and_context() -> (Session, TurnContext) { McpConnectionManager::new_uninitialized_with_permission_profile( &config.permissions.approval_policy, config.permissions.permission_profile(), + config.prefix_mcp_tool_names(), ), )), mcp_startup_cancellation_token: Mutex::new(CancellationToken::new()), @@ -6348,6 +6349,7 @@ where McpConnectionManager::new_uninitialized_with_permission_profile( &config.permissions.approval_policy, config.permissions.permission_profile(), + config.prefix_mcp_tool_names(), ), )), mcp_startup_cancellation_token: Mutex::new(CancellationToken::new()), diff --git a/codex-rs/core/src/tools/handlers/mcp.rs b/codex-rs/core/src/tools/handlers/mcp.rs index 0d5c158272..09eba1730d 100644 --- a/codex-rs/core/src/tools/handlers/mcp.rs +++ b/codex-rs/core/src/tools/handlers/mcp.rs @@ -9,7 +9,10 @@ use crate::tools::context::ToolInvocation; use crate::tools::context::ToolPayload; use crate::tools::context::boxed_tool_output; use crate::tools::flat_tool_name; +use crate::tools::hook_names::HookToolName; use crate::tools::registry::CoreToolRuntime; +use crate::tools::registry::PostToolUsePayload; +use crate::tools::registry::PreToolUsePayload; use crate::tools::registry::ToolExecutor; use crate::tools::registry::ToolTelemetryTags; use crate::tools::tool_search_entry::ToolSearchInfo; @@ -20,6 +23,11 @@ use codex_tools::ToolName; use codex_tools::ToolSearchSourceInfo; use codex_tools::ToolSpec; use codex_tools::mcp_tool_to_responses_api_tool; +use serde_json::Map; +use serde_json::Value; + +const LEGACY_MCP_TOOL_NAME_PREFIX: &str = "mcp__"; +const MCP_TOOL_NAME_DELIMITER: &str = "__"; pub struct McpHandler { tool_info: ToolInfo, @@ -31,6 +39,29 @@ impl McpHandler { let spec = create_tool_spec(&tool_info)?; Ok(Self { tool_info, spec }) } + + fn hook_tool_name(&self) -> HookToolName { + HookToolName::new(ensure_mcp_prefix(&join_tool_name(&self.tool_name()))) + } +} + +fn join_tool_name(tool_name: &ToolName) -> String { + match tool_name.namespace.as_deref() { + Some(namespace) => { + let namespace = namespace.trim_end_matches('_'); + let name = tool_name.name.trim_start_matches('_'); + format!("{namespace}{MCP_TOOL_NAME_DELIMITER}{name}") + } + None => tool_name.name.clone(), + } +} + +fn ensure_mcp_prefix(name: &str) -> String { + if name.starts_with(LEGACY_MCP_TOOL_NAME_PREFIX) { + name.to_string() + } else { + format!("{LEGACY_MCP_TOOL_NAME_PREFIX}{name}") + } } #[async_trait::async_trait] @@ -84,7 +115,7 @@ impl ToolExecutor for McpHandler { call_id.clone(), self.tool_info.server_name.clone(), self.tool_info.tool.name.to_string(), - self.tool_name().to_string(), + self.hook_tool_name(), payload, ) .await; @@ -138,6 +169,58 @@ impl CoreToolRuntime for McpHandler { tags }) } + + fn pre_tool_use_payload(&self, invocation: &ToolInvocation) -> Option { + let ToolPayload::Function { arguments } = &invocation.payload else { + return None; + }; + + Some(PreToolUsePayload { + tool_name: self.hook_tool_name(), + tool_input: mcp_hook_tool_input(arguments), + }) + } + + fn with_updated_hook_input( + &self, + mut invocation: ToolInvocation, + updated_input: Value, + ) -> Result { + invocation.payload = match invocation.payload { + ToolPayload::Function { .. } => ToolPayload::Function { + arguments: serde_json::to_string(&updated_input).map_err(|err| { + FunctionCallError::RespondToModel(format!( + "failed to serialize rewritten MCP arguments: {err}" + )) + })?, + }, + payload => { + return Err(FunctionCallError::RespondToModel(format!( + "tool {} does not support hook input rewriting for payload {payload:?}", + self.tool_name() + ))); + } + }; + Ok(invocation) + } + fn post_tool_use_payload( + &self, + invocation: &ToolInvocation, + result: &dyn crate::tools::context::ToolOutput, + ) -> Option { + let ToolPayload::Function { .. } = &invocation.payload else { + return None; + }; + + let tool_response = + result.post_tool_use_response(&invocation.call_id, &invocation.payload)?; + Some(PostToolUsePayload { + tool_name: self.hook_tool_name(), + tool_use_id: invocation.call_id.clone(), + tool_input: result.post_tool_use_input(&invocation.payload)?, + tool_response, + }) + } } fn create_tool_spec(tool_info: &ToolInfo) -> Result { @@ -166,6 +249,14 @@ fn create_tool_spec(tool_info: &ToolInfo) -> Result })) } +fn mcp_hook_tool_input(raw_arguments: &str) -> Value { + if raw_arguments.trim().is_empty() { + return Value::Object(Map::new()); + } + + serde_json::from_str(raw_arguments).unwrap_or_else(|_| Value::String(raw_arguments.to_string())) +} + fn build_mcp_search_text(info: &ToolInfo) -> String { let tool_name = info.canonical_tool_name(); let mut schema_properties = info @@ -233,7 +324,7 @@ mod tests { use tokio::sync::Mutex; #[tokio::test] - async fn mcp_pre_tool_use_payload_uses_model_tool_name_and_raw_args() { + async fn mcp_pre_tool_use_payload_uses_prefixed_tool_name_and_raw_args() { let payload = ToolPayload::Function { arguments: json!({ "entities": [{ @@ -244,7 +335,7 @@ mod tests { .to_string(), }; let (session, turn) = make_session_and_context().await; - let handler = McpHandler::new(tool_info("memory", "mcp__memory__", "create_entities")) + let handler = McpHandler::new(tool_info("memory", "memory", "create_entities")) .expect("MCP tool spec should build"); assert_eq!( handler.pre_tool_use_payload(&ToolInvocation { @@ -253,7 +344,7 @@ mod tests { cancellation_token: tokio_util::sync::CancellationToken::new(), tracker: Arc::new(Mutex::new(TurnDiffTracker::new())), call_id: "call-mcp-pre".to_string(), - tool_name: codex_tools::ToolName::namespaced("mcp__memory__", "create_entities"), + tool_name: codex_tools::ToolName::namespaced("memory", "create_entities"), source: ToolCallSource::Direct, payload, }), @@ -275,7 +366,7 @@ mod tests { arguments: json!({ "message": "hello" }).to_string(), }; let (session, turn) = make_session_and_context().await; - let handler = McpHandler::new(tool_info("foo", "mcp__foo__", "exec_command")) + let handler = McpHandler::new(tool_info("foo", "mcp__foo", "exec_command")) .expect("MCP tool spec should build"); assert_eq!( @@ -285,7 +376,7 @@ mod tests { cancellation_token: tokio_util::sync::CancellationToken::new(), tracker: Arc::new(Mutex::new(TurnDiffTracker::new())), call_id: "call-mcp-pre-builtin-like".to_string(), - tool_name: codex_tools::ToolName::namespaced("mcp__foo__", "exec_command"), + tool_name: codex_tools::ToolName::namespaced("mcp__foo", "exec_command"), source: ToolCallSource::Direct, payload, }), @@ -302,7 +393,7 @@ mod tests { arguments: json!({ "message": "hello" }).to_string(), }; let (session, turn) = make_session_and_context().await; - let handler = McpHandler::new(tool_info("foo", "mcp__foo__", "exec_command")) + let handler = McpHandler::new(tool_info("foo", "mcp__foo", "exec_command")) .expect("MCP tool spec should build"); let invocation = handler @@ -313,7 +404,7 @@ mod tests { cancellation_token: tokio_util::sync::CancellationToken::new(), tracker: Arc::new(Mutex::new(TurnDiffTracker::new())), call_id: "call-mcp-rewrite-builtin-like".to_string(), - tool_name: codex_tools::ToolName::namespaced("mcp__foo__", "exec_command"), + tool_name: codex_tools::ToolName::namespaced("mcp__foo", "exec_command"), source: ToolCallSource::Direct, payload, }, @@ -328,7 +419,7 @@ mod tests { } #[tokio::test] - async fn mcp_post_tool_use_payload_uses_model_tool_name_args_and_result() { + async fn mcp_post_tool_use_payload_uses_prefixed_tool_name_args_and_result() { let payload = ToolPayload::Function { arguments: json!({ "path": "/tmp/notes.txt" }).to_string(), }; @@ -352,7 +443,7 @@ mod tests { truncation_policy: codex_utils_output_truncation::TruncationPolicy::Bytes(1024), }; let (session, turn) = make_session_and_context().await; - let handler = McpHandler::new(tool_info("filesystem", "mcp__filesystem__", "read_file")) + let handler = McpHandler::new(tool_info("filesystem", "filesystem", "read_file")) .expect("MCP tool spec should build"); let invocation = ToolInvocation { session: session.into(), @@ -360,7 +451,7 @@ mod tests { cancellation_token: tokio_util::sync::CancellationToken::new(), tracker: Arc::new(Mutex::new(TurnDiffTracker::new())), call_id: "call-mcp-post".to_string(), - tool_name: codex_tools::ToolName::namespaced("mcp__filesystem__", "read_file"), + tool_name: codex_tools::ToolName::namespaced("filesystem", "read_file"), source: ToolCallSource::Direct, payload, }; diff --git a/codex-rs/core/src/tools/handlers/tool_search.rs b/codex-rs/core/src/tools/handlers/tool_search.rs index 27a0a1c58d..107723aa15 100644 --- a/codex-rs/core/src/tools/handlers/tool_search.rs +++ b/codex-rs/core/src/tools/handlers/tool_search.rs @@ -197,8 +197,8 @@ mod tests { tools, vec![ LoadableToolSpec::Namespace(ResponsesApiNamespace { - name: "mcp__calendar__".to_string(), - description: "Tools in the mcp__calendar__ namespace.".to_string(), + name: "mcp__calendar".to_string(), + description: "Tools in the mcp__calendar namespace.".to_string(), tools: vec![ ResponsesApiNamespaceTool::Function(ResponsesApiTool { name: "create_event".to_string(), @@ -256,7 +256,7 @@ mod tests { supports_parallel_tool_calls: false, server_origin: None, callable_name: tool_name.to_string(), - callable_namespace: format!("mcp__{server_name}__"), + callable_namespace: format!("mcp__{server_name}"), namespace_description: None, tool: Tool { name: tool_name.to_string().into(), diff --git a/codex-rs/core/src/tools/spec_plan_tests.rs b/codex-rs/core/src/tools/spec_plan_tests.rs index 32913bf2e7..59d6954890 100644 --- a/codex-rs/core/src/tools/spec_plan_tests.rs +++ b/codex-rs/core/src/tools/spec_plan_tests.rs @@ -447,7 +447,7 @@ async fn mcp_and_tool_search_follow_direct_and_deferred_tool_exposure() { let direct_mcp = probe_with( |_| {}, ToolPlanInputs { - mcp_tools: Some(vec![mcp_tool("direct", "mcp__direct__", "lookup")]), + mcp_tools: Some(vec![mcp_tool("direct", "mcp__direct", "lookup")]), ..ToolPlanInputs::default() }, ) @@ -458,12 +458,12 @@ async fn mcp_and_tool_search_follow_direct_and_deferred_tool_exposure() { "read_mcp_resource", ]); assert_eq!( - direct_mcp.namespace_function_names("mcp__direct__"), + direct_mcp.namespace_function_names("mcp__direct"), &["lookup".to_string()] ); let searchable_mcp = ToolPlanInputs { - deferred_mcp_tools: Some(vec![mcp_tool("searchable", "mcp__searchable__", "lookup")]), + deferred_mcp_tools: Some(vec![mcp_tool("searchable", "mcp__searchable", "lookup")]), ..ToolPlanInputs::default() }; @@ -512,7 +512,10 @@ async fn mcp_and_tool_search_follow_direct_and_deferred_tool_exposure() { ) .await; enabled.assert_visible_contains(&["tool_search"]); - enabled.assert_registered_contains(&["tool_search", "mcp__searchable__lookup"]); + enabled.assert_registered_contains(&[ + "tool_search", + &ToolName::namespaced("mcp__searchable", "lookup").to_string(), + ]); } #[tokio::test] @@ -520,18 +523,14 @@ async fn invalid_mcp_tools_are_not_registered() { let plan = probe_with( |_| {}, ToolPlanInputs { - mcp_tools: Some(vec![invalid_mcp_tool( - "invalid", - "mcp__invalid__", - "lookup", - )]), + mcp_tools: Some(vec![invalid_mcp_tool("invalid", "mcp__invalid", "lookup")]), ..ToolPlanInputs::default() }, ) .await; - plan.assert_visible_lacks(&["mcp__invalid__"]); - plan.assert_registered_lacks(&["mcp__invalid__lookup"]); + plan.assert_visible_lacks(&["mcp__invalid"]); + plan.assert_registered_lacks(&[&ToolName::namespaced("mcp__invalid", "lookup").to_string()]); } #[tokio::test] diff --git a/codex-rs/core/tests/common/apps_test_server.rs b/codex-rs/core/tests/common/apps_test_server.rs index 702f96ef2d..2f2875867b 100644 --- a/codex-rs/core/tests/common/apps_test_server.rs +++ b/codex-rs/core/tests/common/apps_test_server.rs @@ -29,9 +29,9 @@ const SEARCHABLE_TOOL_COUNT: usize = 100; const CALENDAR_CREATE_EVENT_TOOL_NAME: &str = "calendar_create_event"; pub const CALENDAR_EXTRACT_TEXT_TOOL_NAME: &str = "calendar_extract_text"; const CALENDAR_LIST_EVENTS_TOOL_NAME: &str = "calendar_list_events"; -pub const DIRECT_CALENDAR_CREATE_EVENT_TOOL: &str = "mcp__codex_apps__calendar_create_event"; -pub const DIRECT_CALENDAR_LIST_EVENTS_TOOL: &str = "mcp__codex_apps__calendar_list_events"; -pub const DIRECT_CALENDAR_EXTRACT_TEXT_TOOL: &str = "mcp__codex_apps__calendar_extract_text"; +pub const DIRECT_CALENDAR_CREATE_EVENT_TOOL: &str = "mcp__codex_apps__calendar__create_event"; +pub const DIRECT_CALENDAR_LIST_EVENTS_TOOL: &str = "mcp__codex_apps__calendar__list_events"; +pub const DIRECT_CALENDAR_EXTRACT_TEXT_TOOL: &str = "mcp__codex_apps__calendar__extract_text"; pub const SEARCH_CALENDAR_NAMESPACE: &str = "mcp__codex_apps__calendar"; pub const SEARCH_CALENDAR_CREATE_TOOL: &str = "_create_event"; pub const SEARCH_CALENDAR_EXTRACT_TEXT_TOOL: &str = "_extract_text"; diff --git a/codex-rs/core/tests/suite/code_mode.rs b/codex-rs/core/tests/suite/code_mode.rs index a1dda52ec1..3cd5c67249 100644 --- a/codex-rs/core/tests/suite/code_mode.rs +++ b/codex-rs/core/tests/suite/code_mode.rs @@ -200,7 +200,11 @@ async fn run_code_mode_turn_with_rmcp_model( code: &str, model: &'static str, ) -> Result<(TestCodex, ResponseMock)> { - run_code_mode_turn_with_rmcp_config(server, prompt, code, model, /*code_mode_only*/ false).await + run_code_mode_turn_with_rmcp_config( + server, prompt, code, model, /*code_mode_only*/ false, + /*non_prefixed_mcp_tool_names*/ false, + ) + .await } async fn run_code_mode_turn_with_rmcp_mode( @@ -209,8 +213,15 @@ async fn run_code_mode_turn_with_rmcp_mode( code: &str, code_mode_only: bool, ) -> Result<(TestCodex, ResponseMock)> { - run_code_mode_turn_with_rmcp_config(server, prompt, code, "test-gpt-5.1-codex", code_mode_only) - .await + run_code_mode_turn_with_rmcp_config( + server, + prompt, + code, + "test-gpt-5.1-codex", + code_mode_only, + /*non_prefixed_mcp_tool_names*/ false, + ) + .await } async fn run_code_mode_turn_with_rmcp_config( @@ -219,6 +230,7 @@ async fn run_code_mode_turn_with_rmcp_config( code: &str, model: &'static str, code_mode_only: bool, + non_prefixed_mcp_tool_names: bool, ) -> Result<(TestCodex, ResponseMock)> { let rmcp_test_server_bin = stdio_server_bin()?; let mut builder = test_codex().with_model(model).with_config(move |config| { @@ -227,6 +239,9 @@ async fn run_code_mode_turn_with_rmcp_config( } else { config.features.enable(Feature::CodeMode) }; + if non_prefixed_mcp_tool_names { + let _ = config.features.enable(Feature::NonPrefixedMcpToolNames); + } let mut servers = config.mcp_servers.get().clone(); servers.insert( @@ -2598,6 +2613,50 @@ contentLength=0" Ok(()) } +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn code_mode_uses_non_prefixed_mcp_tool_names_when_feature_enabled() -> Result<()> { + skip_if_no_network!(Ok(())); + + let server = responses::start_mock_server().await; + let code = r#" +const result = await tools.rmcp__echo({ message: "ping" }); +text(JSON.stringify({ + hasNonPrefixedEcho: typeof tools.rmcp__echo === "function", + hasPrefixedEcho: typeof tools.mcp__rmcp__echo === "function", + echo: result.structuredContent?.echo ?? "missing", +})); +"#; + + let (_test, second_mock) = run_code_mode_turn_with_rmcp_config( + &server, + "use exec to inspect non-prefixed MCP names", + code, + "test-gpt-5.1-codex", + /*code_mode_only*/ false, + /*non_prefixed_mcp_tool_names*/ true, + ) + .await?; + + let req = second_mock.single_request(); + let (output, success) = custom_tool_output_body_and_success(&req, "call-1"); + assert_ne!( + success, + Some(false), + "exec non-prefixed rmcp access failed unexpectedly: {output}" + ); + let parsed: Value = serde_json::from_str(&output)?; + assert_eq!( + parsed, + serde_json::json!({ + "hasNonPrefixedEcho": true, + "hasPrefixedEcho": false, + "echo": "ECHOING: ping", + }) + ); + + Ok(()) +} + #[tokio::test(flavor = "multi_thread", worker_threads = 2)] async fn code_mode_exposes_namespaced_mcp_tools_on_global_tools_object() -> Result<()> { skip_if_no_network!(Ok(())); diff --git a/codex-rs/core/tests/suite/hooks_mcp.rs b/codex-rs/core/tests/suite/hooks_mcp.rs index 5ee2cb05d7..940db7cf4c 100644 --- a/codex-rs/core/tests/suite/hooks_mcp.rs +++ b/codex-rs/core/tests/suite/hooks_mcp.rs @@ -9,6 +9,7 @@ use codex_config::types::AppToolApproval; use codex_config::types::McpServerConfig; use codex_config::types::McpServerTransportConfig; use codex_core::config::Config; +use codex_features::Feature; use core_test_support::hooks::trust_discovered_hooks; use core_test_support::responses::ev_assistant_message; use core_test_support::responses::ev_completed; @@ -26,11 +27,18 @@ use serde_json::Value; use serde_json::json; const RMCP_SERVER: &str = "rmcp"; -const RMCP_NAMESPACE: &str = "mcp__rmcp__"; +const RMCP_PREFIXED_NAMESPACE: &str = "mcp__rmcp"; +const RMCP_UNPREFIXED_NAMESPACE: &str = "rmcp"; const RMCP_ECHO_TOOL_NAME: &str = "mcp__rmcp__echo"; -const RMCP_HOOK_MATCHER: &str = "mcp__rmcp__.*"; +const RMCP_HOOK_MATCHER: &str = RMCP_ECHO_TOOL_NAME; const RMCP_ECHO_MESSAGE: &str = "hook e2e ping"; +fn enable_mcp_tool_name_features(config: &mut Config, prefix_mcp_tool_names: bool) { + if !prefix_mcp_tool_names { + let _ = config.features.enable(Feature::NonPrefixedMcpToolNames); + } +} + fn write_pre_tool_use_hook(home: &Path, reason: &str) -> Result<()> { let script_path = home.join("pre_tool_use_hook.py"); let log_path = home.join("pre_tool_use_hook_log.jsonl"); @@ -207,13 +215,35 @@ fn enable_hooks_and_rmcp_server( config: &mut Config, rmcp_test_server_bin: String, approval_mode: AppToolApproval, + prefix_mcp_tool_names: bool, ) { trust_discovered_hooks(config); + enable_mcp_tool_name_features(config, prefix_mcp_tool_names); insert_rmcp_test_server(config, rmcp_test_server_bin, approval_mode); } #[tokio::test(flavor = "multi_thread", worker_threads = 2)] -async fn pre_tool_use_blocks_mcp_tool_before_execution() -> Result<()> { +async fn pre_tool_use_blocks_mcp_tool_before_execution_with_legacy_prefixed_names() -> Result<()> { + pre_tool_use_blocks_mcp_tool_before_execution( + /*prefix_mcp_tool_names*/ true, + RMCP_PREFIXED_NAMESPACE, + ) + .await +} + +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn pre_tool_use_blocks_mcp_tool_before_execution_with_non_prefixed_names() -> Result<()> { + pre_tool_use_blocks_mcp_tool_before_execution( + /*prefix_mcp_tool_names*/ false, + RMCP_UNPREFIXED_NAMESPACE, + ) + .await +} + +async fn pre_tool_use_blocks_mcp_tool_before_execution( + prefix_mcp_tool_names: bool, + mcp_namespace: &'static str, +) -> Result<()> { skip_if_no_network!(Ok(())); let server = start_mock_server().await; @@ -224,7 +254,7 @@ async fn pre_tool_use_blocks_mcp_tool_before_execution() -> Result<()> { vec![ sse(vec![ ev_response_created("resp-1"), - ev_function_call_with_namespace(call_id, RMCP_NAMESPACE, "echo", &arguments), + ev_function_call_with_namespace(call_id, mcp_namespace, "echo", &arguments), ev_completed("resp-1"), ]), sse(vec![ @@ -245,7 +275,12 @@ async fn pre_tool_use_blocks_mcp_tool_before_execution() -> Result<()> { } }) .with_config(move |config| { - enable_hooks_and_rmcp_server(config, rmcp_test_server_bin, AppToolApproval::Approve); + enable_hooks_and_rmcp_server( + config, + rmcp_test_server_bin, + AppToolApproval::Approve, + prefix_mcp_tool_names, + ); }) .build(&server) .await?; @@ -256,10 +291,9 @@ async fn pre_tool_use_blocks_mcp_tool_before_execution() -> Result<()> { let requests = responses.requests(); assert_eq!(requests.len(), 2); let output_item = requests[1].function_call_output(call_id); - let output = output_item - .get("output") - .and_then(Value::as_str) - .expect("blocked MCP tool output string"); + let Some(output) = output_item.get("output").and_then(Value::as_str) else { + panic!("blocked MCP tool output should be a string: {output_item:?}"); + }; assert!( output.contains(&format!( "Tool call blocked by PreToolUse hook: {block_reason}. Tool: {RMCP_ECHO_TOOL_NAME}" @@ -283,9 +317,12 @@ async fn pre_tool_use_blocks_mcp_tool_before_execution() -> Result<()> { "tool_input": { "message": RMCP_ECHO_MESSAGE }, }) ); - let transcript_path = hook_inputs[0]["transcript_path"] - .as_str() - .expect("pre tool use hook transcript_path"); + let Some(transcript_path) = hook_inputs[0]["transcript_path"].as_str() else { + panic!( + "pre tool use hook transcript_path should be a string: {:?}", + hook_inputs[0]["transcript_path"] + ); + }; assert!( Path::new(transcript_path).exists(), "pre tool use hook transcript_path should be materialized on disk", @@ -306,7 +343,7 @@ async fn pre_tool_use_rewrites_mcp_tool_before_execution() -> Result<()> { &server, sse(vec![ ev_response_created("resp-1"), - ev_function_call_with_namespace(call_id, RMCP_NAMESPACE, "echo", &arguments), + ev_function_call_with_namespace(call_id, RMCP_PREFIXED_NAMESPACE, "echo", &arguments), ev_completed("resp-1"), ]), ) @@ -329,7 +366,12 @@ async fn pre_tool_use_rewrites_mcp_tool_before_execution() -> Result<()> { } }) .with_config(move |config| { - enable_hooks_and_rmcp_server(config, rmcp_test_server_bin, AppToolApproval::Approve); + enable_hooks_and_rmcp_server( + config, + rmcp_test_server_bin, + AppToolApproval::Approve, + /*prefix_mcp_tool_names*/ true, + ); }) .build(&server) .await?; @@ -339,10 +381,9 @@ async fn pre_tool_use_rewrites_mcp_tool_before_execution() -> Result<()> { let final_request = final_mock.single_request(); let output_item = final_request.function_call_output(call_id); - let output = output_item - .get("output") - .and_then(Value::as_str) - .expect("MCP tool output string"); + let Some(output) = output_item.get("output").and_then(Value::as_str) else { + panic!("MCP tool output should be a string: {output_item:?}"); + }; assert!( output.contains(&format!("ECHOING: {rewritten_message}")), "MCP tool should execute the rewritten input", @@ -365,7 +406,29 @@ async fn pre_tool_use_rewrites_mcp_tool_before_execution() -> Result<()> { } #[tokio::test(flavor = "multi_thread", worker_threads = 2)] -async fn post_tool_use_records_mcp_tool_payload_and_context() -> Result<()> { +async fn post_tool_use_records_mcp_tool_payload_and_context_with_legacy_prefixed_names() +-> Result<()> { + post_tool_use_records_mcp_tool_payload_and_context( + /*prefix_mcp_tool_names*/ true, + RMCP_PREFIXED_NAMESPACE, + ) + .await +} + +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn post_tool_use_records_mcp_tool_payload_and_context_with_non_prefixed_names() -> Result<()> +{ + post_tool_use_records_mcp_tool_payload_and_context( + /*prefix_mcp_tool_names*/ false, + RMCP_UNPREFIXED_NAMESPACE, + ) + .await +} + +async fn post_tool_use_records_mcp_tool_payload_and_context( + prefix_mcp_tool_names: bool, + mcp_namespace: &'static str, +) -> Result<()> { skip_if_no_network!(Ok(())); let server = start_mock_server().await; @@ -375,7 +438,7 @@ async fn post_tool_use_records_mcp_tool_payload_and_context() -> Result<()> { &server, sse(vec![ ev_response_created("resp-1"), - ev_function_call_with_namespace(call_id, RMCP_NAMESPACE, "echo", &arguments), + ev_function_call_with_namespace(call_id, mcp_namespace, "echo", &arguments), ev_completed("resp-1"), ]), ) @@ -399,7 +462,12 @@ async fn post_tool_use_records_mcp_tool_payload_and_context() -> Result<()> { } }) .with_config(move |config| { - enable_hooks_and_rmcp_server(config, rmcp_test_server_bin, AppToolApproval::Approve); + enable_hooks_and_rmcp_server( + config, + rmcp_test_server_bin, + AppToolApproval::Approve, + prefix_mcp_tool_names, + ); }) .build(&server) .await?; @@ -415,10 +483,9 @@ async fn post_tool_use_records_mcp_tool_payload_and_context() -> Result<()> { "follow-up request should include MCP post tool use additional context", ); let output_item = final_request.function_call_output(call_id); - let output = output_item - .get("output") - .and_then(Value::as_str) - .expect("MCP tool output string"); + let Some(output) = output_item.get("output").and_then(Value::as_str) else { + panic!("MCP tool output should be a string: {output_item:?}"); + }; assert!( output.contains(&format!("ECHOING: {RMCP_ECHO_MESSAGE}")), "MCP tool output should still reach the model", @@ -449,9 +516,12 @@ async fn post_tool_use_records_mcp_tool_payload_and_context() -> Result<()> { }, }) ); - let transcript_path = hook_inputs[0]["transcript_path"] - .as_str() - .expect("post tool use hook transcript_path"); + let Some(transcript_path) = hook_inputs[0]["transcript_path"].as_str() else { + panic!( + "post tool use hook transcript_path should be a string: {:?}", + hook_inputs[0]["transcript_path"] + ); + }; assert!( Path::new(transcript_path).exists(), "post tool use hook transcript_path should be materialized on disk", diff --git a/codex-rs/core/tests/suite/plugins.rs b/codex-rs/core/tests/suite/plugins.rs index 45fdf3df07..917ab08554 100644 --- a/codex-rs/core/tests/suite/plugins.rs +++ b/codex-rs/core/tests/suite/plugins.rs @@ -340,7 +340,7 @@ async fn explicit_plugin_mentions_inject_plugin_guidance() -> Result<()> { "expected plugin app tools to become visible for this turn: {request_tools:?}" ); let echo_tool = request - .tool_by_name("mcp__sample__", "echo") + .tool_by_name("mcp__sample", "echo") .expect("plugin MCP tool should be present"); let echo_description = echo_tool .get("description") diff --git a/codex-rs/core/tests/suite/rmcp_client.rs b/codex-rs/core/tests/suite/rmcp_client.rs index 864fb1e07a..260edb3f64 100644 --- a/codex-rs/core/tests/suite/rmcp_client.rs +++ b/codex-rs/core/tests/suite/rmcp_client.rs @@ -382,7 +382,7 @@ async fn call_cwd_tool( server_name: &str, call_id: &str, ) -> anyhow::Result { - let namespace = format!("mcp__{server_name}__"); + let namespace = format!("mcp__{server_name}"); mount_sse_once( server, responses::sse(vec![ @@ -467,7 +467,7 @@ async fn stdio_server_round_trip() -> anyhow::Result<()> { let call_id = "call-123"; let server_name = "rmcp"; - let namespace = format!("mcp__{server_name}__"); + let namespace = format!("mcp__{server_name}"); let call_mock = mount_sse_once( &server, @@ -723,7 +723,7 @@ async fn stdio_mcp_tool_call_includes_sandbox_state_meta() -> anyhow::Result<()> let call_id = "sandbox-meta-call"; let server_name = "rmcp"; - let namespace = format!("mcp__{server_name}__"); + let namespace = format!("mcp__{server_name}"); let call_mock = mount_sse_once( &server, @@ -817,7 +817,7 @@ async fn stdio_mcp_parallel_tool_calls_default_false_runs_serially() -> anyhow:: let first_call_id = "sync-serial-1"; let second_call_id = "sync-serial-2"; let server_name = "rmcp"; - let namespace = format!("mcp__{server_name}__"); + let namespace = format!("mcp__{server_name}"); let args = json!({ "sleep_after_ms": 100 }).to_string(); mount_sse_once( @@ -930,7 +930,7 @@ async fn stdio_mcp_read_only_tool_calls_run_concurrently_without_server_opt_in() let first_call_id = "sync-read-only-1"; let second_call_id = "sync-read-only-2"; let server_name = "rmcp"; - let namespace = format!("mcp__{server_name}__"); + let namespace = format!("mcp__{server_name}"); // The stdio MCP test server holds each sync call at this barrier until both // calls arrive. A serial scheduler times out inside the server instead of // returning the structured `{ "result": "ok" }` result asserted below. @@ -1025,7 +1025,7 @@ async fn stdio_mcp_parallel_tool_calls_opt_in_runs_concurrently() -> anyhow::Res let first_call_id = "sync-1"; let second_call_id = "sync-2"; let server_name = "rmcp"; - let namespace = format!("mcp__{server_name}__"); + let namespace = format!("mcp__{server_name}"); let args = json!({ "sleep_after_ms": 100, "barrier": { @@ -1110,7 +1110,7 @@ async fn stdio_image_responses_round_trip() -> anyhow::Result<()> { let call_id = "img-1"; let server_name = "rmcp"; - let namespace = format!("mcp__{server_name}__"); + let namespace = format!("mcp__{server_name}"); // First stream: model decides to call the image tool. mount_sse_once( @@ -1247,7 +1247,7 @@ async fn stdio_image_responses_preserve_original_detail_metadata() -> anyhow::Re let call_id = "img-original-detail-1"; let server_name = "rmcp"; - let namespace = format!("mcp__{server_name}__"); + let namespace = format!("mcp__{server_name}"); mount_sse_once( &server, @@ -1333,7 +1333,7 @@ async fn stdio_image_responses_are_sanitized_for_text_only_model() -> anyhow::Re let call_id = "img-text-only-1"; let server_name = "rmcp"; - let namespace = format!("mcp__{server_name}__"); + let namespace = format!("mcp__{server_name}"); let text_only_model_slug = "rmcp-text-only-model"; let models_mock = mount_models_once( @@ -1480,7 +1480,7 @@ async fn stdio_server_propagates_whitelisted_env_vars() -> anyhow::Result<()> { let call_id = "call-1234"; let server_name = "rmcp_whitelist"; - let namespace = format!("mcp__{server_name}__"); + let namespace = format!("mcp__{server_name}"); mount_sse_once( &server, @@ -1594,7 +1594,7 @@ async fn stdio_server_propagates_explicit_local_env_var_source() -> anyhow::Resu let server = responses::start_mock_server().await; let call_id = "call-local-source"; let server_name = "rmcp_local_source"; - let namespace = format!("mcp__{server_name}__"); + let namespace = format!("mcp__{server_name}"); let env_name = "MCP_TEST_LOCAL_SOURCE"; let expected_env_value = "propagated-explicit-local-source"; @@ -1687,7 +1687,7 @@ async fn remote_stdio_env_var_source_does_not_copy_local_env() -> anyhow::Result let server = responses::start_mock_server().await; let call_id = "call-remote-source"; let server_name = "rmcp_remote_source"; - let namespace = format!("mcp__{server_name}__"); + let namespace = format!("mcp__{server_name}"); let env_name = "MCP_TEST_REMOTE_SOURCE_ONLY"; mount_sse_once( @@ -1859,7 +1859,7 @@ async fn streamable_http_tool_call_round_trip() -> anyhow::Result<()> { let call_id = "call-456"; let server_name = "rmcp_http"; - let namespace = format!("mcp__{server_name}__"); + let namespace = format!("mcp__{server_name}"); mount_sse_once( &server, @@ -2026,7 +2026,7 @@ async fn streamable_http_with_oauth_round_trip_impl() -> anyhow::Result<()> { let call_id = "call-789"; let server_name = "rmcp_http_oauth"; - let namespace = format!("mcp__{server_name}__"); + let namespace = format!("mcp__{server_name}"); mount_sse_once( &server, diff --git a/codex-rs/core/tests/suite/search_tool.rs b/codex-rs/core/tests/suite/search_tool.rs index 26531e9d2c..bbf16b3657 100644 --- a/codex-rs/core/tests/suite/search_tool.rs +++ b/codex-rs/core/tests/suite/search_tool.rs @@ -1059,13 +1059,13 @@ async fn tool_search_indexes_only_enabled_non_app_mcp_tools() -> Result<()> { "non-app MCP tools should be hidden before search in large-search mode: {first_request_tools:?}" ); assert!( - !first_request_tools.iter().any(|name| name == "mcp__rmcp__"), + !first_request_tools.iter().any(|name| name == "mcp__rmcp"), "non-app MCP namespace 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 echo_output = json!({ "tools": echo_tools }); - let rmcp_echo_tool = namespace_child_tool(&echo_output, "mcp__rmcp__", "echo") + let rmcp_echo_tool = namespace_child_tool(&echo_output, "mcp__rmcp", "echo") .expect("tool_search should return rmcp echo as a namespace child tool"); assert_eq!( rmcp_echo_tool.get("type").and_then(Value::as_str), @@ -1075,7 +1075,7 @@ async fn tool_search_indexes_only_enabled_non_app_mcp_tools() -> Result<()> { 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__")) + .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()); @@ -1111,7 +1111,7 @@ async fn tool_search_surfaced_mcp_tool_errors_are_returned_to_model() -> Result< ]), sse(vec![ ev_response_created("resp-2"), - ev_function_call_with_namespace(tool_call_id, "mcp__rmcp__", "echo", "{}"), + ev_function_call_with_namespace(tool_call_id, "mcp__rmcp", "echo", "{}"), ev_completed("resp-2"), ]), sse(vec![ @@ -1213,12 +1213,12 @@ async fn tool_search_surfaced_mcp_tool_errors_are_returned_to_model() -> Result< "first request should advertise tool_search: {first_request_tools:?}" ); assert!( - !first_request_tools.iter().any(|name| name == "mcp__rmcp__"), + !first_request_tools.iter().any(|name| name == "mcp__rmcp"), "deferred rmcp namespace should not be directly exposed before search: {first_request_tools:?}" ); assert!( - tool_search_output_has_namespace_child(&requests[1], search_call_id, "mcp__rmcp__", "echo"), + tool_search_output_has_namespace_child(&requests[1], search_call_id, "mcp__rmcp", "echo"), "tool_search should return the rmcp echo tool" ); @@ -1324,7 +1324,7 @@ async fn tool_search_uses_non_app_mcp_server_instructions_as_namespace_descripti let tools = tool_search_output_tools(&requests[1], search_call_id); let rmcp_namespace = tools .iter() - .find(|tool| tool.get("name").and_then(Value::as_str) == Some("mcp__rmcp__")) + .find(|tool| tool.get("name").and_then(Value::as_str) == Some("mcp__rmcp")) .expect("tool_search should return the rmcp namespace"); assert_eq!( rmcp_namespace.get("description").and_then(Value::as_str), diff --git a/codex-rs/core/tests/suite/sqlite_state.rs b/codex-rs/core/tests/suite/sqlite_state.rs index f5dd75670d..6f7a51069a 100644 --- a/codex-rs/core/tests/suite/sqlite_state.rs +++ b/codex-rs/core/tests/suite/sqlite_state.rs @@ -330,7 +330,7 @@ async fn mcp_call_marks_thread_memory_mode_polluted_when_configured() -> Result< let server = start_mock_server().await; let call_id = "call-123"; let server_name = "rmcp"; - let namespace = format!("mcp__{server_name}__"); + let namespace = format!("mcp__{server_name}"); mount_sse_once( &server, responses::sse(vec![ diff --git a/codex-rs/core/tests/suite/truncation.rs b/codex-rs/core/tests/suite/truncation.rs index b98c438f40..04b59a3331 100644 --- a/codex-rs/core/tests/suite/truncation.rs +++ b/codex-rs/core/tests/suite/truncation.rs @@ -343,7 +343,7 @@ async fn mcp_tool_call_output_exceeds_limit_truncated_for_model() -> Result<()> let call_id = "rmcp-truncated"; let server_name = "rmcp"; - let namespace = format!("mcp__{server_name}__"); + let namespace = format!("mcp__{server_name}"); // Build a very large message to exceed 10KiB once serialized. let large_msg = "long-message-with-newlines-".repeat(6000); @@ -446,7 +446,7 @@ async fn mcp_image_output_preserves_image_and_no_text_summary() -> Result<()> { let call_id = "rmcp-image-no-trunc"; let server_name = "rmcp"; - let namespace = format!("mcp__{server_name}__"); + let namespace = format!("mcp__{server_name}"); mount_sse_once( &server, @@ -733,7 +733,7 @@ async fn mcp_tool_call_output_not_truncated_with_custom_limit() -> Result<()> { let call_id = "rmcp-untruncated"; let server_name = "rmcp"; - let namespace = format!("mcp__{server_name}__"); + let namespace = format!("mcp__{server_name}"); let large_msg = "a".repeat(80_000); let args_json = serde_json::json!({ "message": large_msg }); diff --git a/codex-rs/features/src/lib.rs b/codex-rs/features/src/lib.rs index 4db7b56d25..cd50de4330 100644 --- a/codex-rs/features/src/lib.rs +++ b/codex-rs/features/src/lib.rs @@ -136,6 +136,8 @@ pub enum Feature { ToolSearch, /// Always defer MCP tools behind tool_search instead of exposing small sets directly. ToolSearchAlwaysDeferMcpTools, + /// Expose MCP model-visible namespaces without the legacy `mcp__` prefix. + NonPrefixedMcpToolNames, /// Enable discoverable tool suggestions for apps. ToolSuggest, /// Enable plugins. @@ -961,6 +963,12 @@ pub const FEATURES: &[FeatureSpec] = &[ stage: Stage::UnderDevelopment, default_enabled: false, }, + FeatureSpec { + id: Feature::NonPrefixedMcpToolNames, + key: "non_prefixed_mcp_tool_names", + stage: Stage::UnderDevelopment, + default_enabled: false, + }, FeatureSpec { id: Feature::UnavailableDummyTools, key: "unavailable_dummy_tools", diff --git a/codex-rs/tools/src/code_mode.rs b/codex-rs/tools/src/code_mode.rs index 052edbac59..4876486f61 100644 --- a/codex-rs/tools/src/code_mode.rs +++ b/codex-rs/tools/src/code_mode.rs @@ -147,7 +147,7 @@ pub fn code_mode_name_for_tool_name(tool_name: &ToolName) -> String { Some(namespace) if namespace.ends_with('_') || tool_name.name.starts_with('_') => { format!("{namespace}{}", tool_name.name) } - Some(namespace) => format!("{namespace}_{}", tool_name.name), + Some(namespace) => format!("{namespace}__{}", tool_name.name), None => tool_name.name.clone(), } }