diff --git a/AGENTS.md b/AGENTS.md index 6939d146b00e..9b1788007d9f 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -108,6 +108,15 @@ See `codex-rs/tui/styles.md`. ## Tests +When reviewing or adding tests: + +- Ask whether the test makes sense. +- Ask whether it actually asserts something valuable that you care about. +- Ask how much the test would need to change if the surrounding system were refactored. +- Ask whether you or someone else would notice the regression if Codex auto-updated the test. + +If a unit test mostly covers deep implementation details, static data, removed behavior, or requires heavy monkeypatching, prefer deleting it and writing an integration test that protects meaningful end-to-end behavior instead. + ### Snapshot tests This repo uses snapshot tests (via `insta`), especially in `codex-rs/tui`, to validate rendered output. diff --git a/codex-rs/codex-mcp/src/codex_apps.rs b/codex-rs/codex-mcp/src/codex_apps.rs index 0a7981fb0d5f..ff5098a9ef02 100644 --- a/codex-rs/codex-mcp/src/codex_apps.rs +++ b/codex-rs/codex-mcp/src/codex_apps.rs @@ -21,7 +21,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 { @@ -115,7 +115,7 @@ pub(crate) fn normalize_codex_apps_callable_name( && let Some(stripped) = tool_name.strip_prefix(&connector_name) && !stripped.is_empty() { - return stripped.to_string(); + return stripped.strip_prefix('_').unwrap_or(stripped).to_string(); } if let Some(connector_id) = connector_id @@ -125,7 +125,7 @@ pub(crate) fn normalize_codex_apps_callable_name( && let Some(stripped) = tool_name.strip_prefix(&connector_id) && !stripped.is_empty() { - return stripped.to_string(); + return stripped.strip_prefix('_').unwrap_or(stripped).to_string(); } tool_name @@ -138,9 +138,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_tests.rs b/codex-rs/codex-mcp/src/connection_manager_tests.rs index 01b1161b730a..9c3c9fb91013 100644 --- a/codex-rs/codex-mcp/src/connection_manager_tests.rs +++ b/codex-rs/codex-mcp/src/connection_manager_tests.rs @@ -35,7 +35,7 @@ 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}__"); + let tool_namespace = server_name.to_string(); ToolInfo { server_name: server_name.to_string(), callable_name: tool_name.to_string(), @@ -71,6 +71,18 @@ fn create_test_tool_with_connector( tool } +fn create_codex_apps_test_tool(tool_name: &str, callable_name: &str) -> ToolInfo { + let mut tool = create_test_tool_with_connector( + CODEX_APPS_MCP_SERVER_NAME, + tool_name, + "calendar", + Some("Calendar"), + ); + tool.callable_name = callable_name.to_string(); + tool.callable_namespace = "codex_apps__calendar".to_string(); + tool +} + fn create_codex_apps_tools_cache_context( codex_home: PathBuf, account_id: Option<&str>, @@ -276,8 +288,8 @@ fn test_qualify_tools_short_non_duplicated_names() { let qualified_tools = qualify_tools(tools); assert_eq!(qualified_tools.len(), 2); - assert!(qualified_tools.contains_key("mcp__server1__tool1")); - assert!(qualified_tools.contains_key("mcp__server1__tool2")); + assert!(qualified_tools.contains_key("server1__tool1")); + assert!(qualified_tools.contains_key("server1__tool2")); } #[test] @@ -291,7 +303,7 @@ fn test_qualify_tools_duplicated_names_skipped() { // Only the first tool should remain, the second is skipped assert_eq!(qualified_tools.len(), 1); - assert!(qualified_tools.contains_key("mcp__server1__duplicate_tool")); + assert!(qualified_tools.contains_key("server1__duplicate_tool")); } #[test] @@ -317,7 +329,7 @@ fn test_qualify_tools_long_names_same_server() { keys.sort(); 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.starts_with("my_server__"))); assert!( keys.iter() .all(|key| key.chars().all(|c| c.is_ascii_alphanumeric() || c == '_')), @@ -333,16 +345,16 @@ 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!(qualified_name, "server_one__tool_two_three"); assert_eq!( - format!("{}{}", tool.callable_namespace, tool.callable_name), + ToolName::namespaced(&tool.callable_namespace, &tool.callable_name).display(), qualified_name ); // The key and callable parts are sanitized for model-visible tool calls, but // the raw MCP name is preserved for the actual MCP call. assert_eq!(tool.server_name, "server.one"); - assert_eq!(tool.callable_namespace, "mcp__server_one__"); + assert_eq!(tool.callable_namespace, "server_one"); assert_eq!(tool.callable_name, "tool_two_three"); assert_eq!(tool.tool.name, "tool.two-three"); @@ -362,8 +374,8 @@ fn test_qualify_tools_keeps_hyphenated_mcp_tools_callable() { 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!(qualified_name, "music_studio__get_strudel_guide"); + assert_eq!(tool.callable_namespace, "music_studio"); assert_eq!(tool.callable_name, "get_strudel_guide"); assert_eq!(tool.tool.name, "get-strudel-guide"); } @@ -624,10 +636,7 @@ fn startup_cached_codex_apps_tools_loads_from_disk_cache() { Some("account-one"), Some("user-one"), ); - let cached_tools = vec![create_test_tool( - CODEX_APPS_MCP_SERVER_NAME, - "calendar_search", - )]; + let cached_tools = vec![create_codex_apps_test_tool("calendar_search", "search")]; write_cached_codex_apps_tools(&cache_context, &cached_tools); let startup_snapshot = load_startup_cached_codex_apps_tools_snapshot( @@ -638,14 +647,14 @@ 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].callable_name, "calendar_search"); + assert_eq!(startup_tools[0].callable_name, "search"); } #[tokio::test] async fn list_all_tools_uses_startup_snapshot_while_client_is_pending() { - let startup_tools = vec![create_test_tool( - CODEX_APPS_MCP_SERVER_NAME, + let startup_tools = vec![create_codex_apps_test_tool( "calendar_create_event", + "create_event", )]; let pending_client = futures::future::pending::>() .boxed() @@ -667,10 +676,10 @@ async fn list_all_tools_uses_startup_snapshot_while_client_is_pending() { let tools = manager.list_all_tools().await; let tool = tools - .get("mcp__codex_apps__calendar_create_event") + .get("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"); + assert_eq!(tool.callable_name, "create_event"); } #[tokio::test] @@ -695,11 +704,11 @@ async fn resolve_tool_info_accepts_canonical_namespaced_tool_names() { ); let tool = manager - .resolve_tool_info(&ToolName::namespaced("mcp__rmcp__", "echo")) + .resolve_tool_info(&ToolName::namespaced("rmcp", "echo")) .await .expect("split MCP tool namespace and name should resolve"); - let expected = ("rmcp", "mcp__rmcp__", "echo", "echo"); + let expected = ("rmcp", "rmcp", "echo", "echo"); assert_eq!( ( tool.server_name.as_str(), @@ -764,9 +773,9 @@ async fn list_all_tools_does_not_block_when_startup_snapshot_cache_hit_is_empty( #[tokio::test] async fn list_all_tools_uses_startup_snapshot_when_client_startup_fails() { - let startup_tools = vec![create_test_tool( - CODEX_APPS_MCP_SERVER_NAME, + let startup_tools = vec![create_codex_apps_test_tool( "calendar_create_event", + "create_event", )]; let failed_client = futures::future::ready::>(Err( StartupOutcomeError::Failed { @@ -793,10 +802,10 @@ async fn list_all_tools_uses_startup_snapshot_when_client_startup_fails() { let tools = manager.list_all_tools().await; let tool = tools - .get("mcp__codex_apps__calendar_create_event") + .get("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"); + assert_eq!(tool.callable_name, "create_event"); } #[test] diff --git a/codex-rs/codex-mcp/src/mcp/mod.rs b/codex-rs/codex-mcp/src/mcp/mod.rs index 3cfd4d01e194..32f5c92abecf 100644 --- a/codex-rs/codex-mcp/src/mcp/mod.rs +++ b/codex-rs/codex-mcp/src/mcp/mod.rs @@ -41,7 +41,6 @@ use crate::connection_manager::McpConnectionManager; use crate::runtime::McpRuntimeEnvironment; pub const CODEX_APPS_MCP_SERVER_NAME: &str = "codex_apps"; -const MCP_TOOL_NAME_PREFIX: &str = "mcp"; const MCP_TOOL_NAME_DELIMITER: &str = "__"; const CODEX_CONNECTORS_TOKEN_ENV_VAR: &str = "CODEX_CONNECTORS_TOKEN"; @@ -59,9 +58,10 @@ impl McpSnapshotDetail { } pub fn qualified_mcp_tool_name_prefix(server_name: &str) -> String { - sanitize_responses_api_tool_name(&format!( - "{MCP_TOOL_NAME_PREFIX}{MCP_TOOL_NAME_DELIMITER}{server_name}{MCP_TOOL_NAME_DELIMITER}" - )) + format!( + "{}{MCP_TOOL_NAME_DELIMITER}", + sanitize_responses_api_tool_name(server_name) + ) } /// Returns true when MCP permission prompts should resolve as approved instead diff --git a/codex-rs/codex-mcp/src/mcp/mod_tests.rs b/codex-rs/codex-mcp/src/mcp/mod_tests.rs index fa5cbf1f7adb..10e8b21992b3 100644 --- a/codex-rs/codex-mcp/src/mcp/mod_tests.rs +++ b/codex-rs/codex-mcp/src/mcp/mod_tests.rs @@ -36,7 +36,7 @@ fn test_mcp_config(codex_home: PathBuf) -> McpConfig { fn qualified_mcp_tool_name_prefix_sanitizes_server_names_without_lowercasing() { assert_eq!( qualified_mcp_tool_name_prefix("Some-Server"), - "mcp__Some_Server__".to_string() + "Some_Server__".to_string() ); } diff --git a/codex-rs/codex-mcp/src/tools.rs b/codex-rs/codex-mcp/src/tools.rs index 9b677e8a07c7..2ee8f937d90c 100644 --- a/codex-rs/codex-mcp/src/tools.rs +++ b/codex-rs/codex-mcp/src/tools.rs @@ -134,7 +134,7 @@ pub(crate) fn filter_tools(tools: Vec, filter: &ToolFilter) -> Vec(tools: I) -> HashMap where I: IntoIterator, @@ -327,17 +327,22 @@ fn fit_callable_parts_with_hash( 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), - ); + + for prefix_len in (0..=tool_name.chars().count()).rev() { + let candidate_name = format!("{}{}", truncate_name(tool_name, prefix_len), suffix); + if qualified_name(namespace, &candidate_name).len() <= MAX_TOOL_NAME_LENGTH { + return (namespace.to_string(), candidate_name); + } } - let max_namespace_len = MAX_TOOL_NAME_LENGTH - suffix.len(); - (truncate_name(namespace, max_namespace_len), suffix) + for namespace_len in (0..=namespace.chars().count()).rev() { + let candidate_namespace = truncate_name(namespace, namespace_len); + if qualified_name(&candidate_namespace, &suffix).len() <= MAX_TOOL_NAME_LENGTH { + return (candidate_namespace, suffix); + } + } + + (String::new(), suffix) } fn unique_callable_parts( @@ -346,9 +351,15 @@ fn unique_callable_parts( 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 qualified_display_name = qualified_name(namespace, tool_name); + if qualified_display_name.len() <= MAX_TOOL_NAME_LENGTH + && used_names.insert(qualified_display_name.clone()) + { + return ( + namespace.to_string(), + tool_name.to_string(), + qualified_display_name, + ); } let mut attempt = 0_u32; @@ -360,10 +371,14 @@ fn unique_callable_parts( }; let (namespace, tool_name) = fit_callable_parts_with_hash(namespace, tool_name, &hash_input); - let qualified_name = format!("{namespace}{tool_name}"); + let qualified_name = qualified_name(&namespace, &tool_name); if used_names.insert(qualified_name.clone()) { return (namespace, tool_name, qualified_name); } attempt = attempt.saturating_add(1); } } + +fn qualified_name(namespace: &str, tool_name: &str) -> String { + ToolName::namespaced(namespace, tool_name).display() +} diff --git a/codex-rs/core/src/connectors_tests.rs b/codex-rs/core/src/connectors_tests.rs index b3538d1ff062..c3775193c465 100644 --- a/codex-rs/core/src/connectors_tests.rs +++ b/codex-rs/core/src/connectors_tests.rs @@ -112,7 +112,7 @@ fn codex_app_tool( ) -> ToolInfo { let tool_namespace = connector_name .map(sanitize_name) - .map(|connector_name| format!("mcp__{CODEX_APPS_MCP_SERVER_NAME}__{connector_name}")) + .map(|connector_name| format!("{CODEX_APPS_MCP_SERVER_NAME}__{connector_name}")) .unwrap_or_else(|| CODEX_APPS_MCP_SERVER_NAME.to_string()); ToolInfo { @@ -175,7 +175,7 @@ fn merge_connectors_replaces_plugin_placeholder_name_with_accessible_name() { fn accessible_connectors_from_mcp_tools_carries_plugin_display_names() { let tools = HashMap::from([ ( - "mcp__codex_apps__calendar_list_events".to_string(), + "codex_apps__calendar__list_events".to_string(), codex_app_tool( "calendar_list_events", "calendar", @@ -184,7 +184,7 @@ fn accessible_connectors_from_mcp_tools_carries_plugin_display_names() { ), ), ( - "mcp__codex_apps__calendar_create_event".to_string(), + "codex_apps__calendar__create_event".to_string(), codex_app_tool( "calendar_create_event", "calendar", @@ -193,7 +193,7 @@ fn accessible_connectors_from_mcp_tools_carries_plugin_display_names() { ), ), ( - "mcp__sample__echo".to_string(), + "sample__echo".to_string(), ToolInfo { server_name: "sample".to_string(), callable_name: "echo".to_string(), @@ -242,7 +242,7 @@ async fn refresh_accessible_connectors_cache_from_mcp_tools_writes_latest_instal let cache_key = accessible_connectors_cache_key(&config, /*auth*/ None); let tools = HashMap::from([ ( - "mcp__codex_apps__calendar_list_events".to_string(), + "codex_apps__calendar__list_events".to_string(), codex_app_tool( "calendar_list_events", "calendar", @@ -251,7 +251,7 @@ async fn refresh_accessible_connectors_cache_from_mcp_tools_writes_latest_instal ), ), ( - "mcp__codex_apps__openai_hidden".to_string(), + "codex_apps__openai_hidden".to_string(), codex_app_tool( "openai_hidden", "connector_openai_hidden", @@ -318,11 +318,11 @@ fn merge_connectors_unions_and_dedupes_plugin_display_names() { #[test] fn accessible_connectors_from_mcp_tools_preserves_description() { let mcp_tools = HashMap::from([( - "mcp__codex_apps__calendar_create_event".to_string(), + "codex_apps__calendar__create_event".to_string(), ToolInfo { server_name: CODEX_APPS_MCP_SERVER_NAME.to_string(), callable_name: "calendar_create_event".to_string(), - callable_namespace: "mcp__codex_apps__calendar".to_string(), + callable_namespace: "codex_apps__calendar".to_string(), server_instructions: None, tool: Tool { name: "calendar_create_event".to_string().into(), diff --git a/codex-rs/core/src/mcp_tool_call_tests.rs b/codex-rs/core/src/mcp_tool_call_tests.rs index 524138f017d4..e2a3a85bb85c 100644 --- a/codex-rs/core/src/mcp_tool_call_tests.rs +++ b/codex-rs/core/src/mcp_tool_call_tests.rs @@ -1907,7 +1907,7 @@ async fn approve_mode_skips_when_annotations_do_not_require_approval() { &turn_context, "call-1", &invocation, - "mcp__test__tool", + "test__tool", Some(&metadata), AppToolApproval::Approve, ) @@ -1980,7 +1980,7 @@ async fn guardian_mode_skips_auto_when_annotations_do_not_require_approval() { &turn_context, "call-guardian", &invocation, - "mcp__test__tool", + "test__tool", Some(&metadata), AppToolApproval::Auto, ) @@ -1995,7 +1995,7 @@ async fn permission_request_hook_allows_mcp_tool_call() { let log_path = install_mcp_permission_request_hook( &mut session, &turn_context, - "mcp__memory__.*", + "memory__.*", &serde_json::json!({ "hookSpecificOutput": { "hookEventName": "PermissionRequest", @@ -2036,7 +2036,7 @@ async fn permission_request_hook_allows_mcp_tool_call() { &turn_context, "call-mcp-hook", &invocation, - "mcp__memory__create_entities", + "memory__create_entities", Some(&metadata), AppToolApproval::Auto, ) @@ -2057,7 +2057,7 @@ async fn permission_request_hook_allows_mcp_tool_call() { "transcript_path": null, "model": turn_context.model_info.slug, "permission_mode": "default", - "tool_name": "mcp__memory__create_entities", + "tool_name": "memory__create_entities", "hook_event_name": "PermissionRequest", "tool_input": { "entities": [{ @@ -2075,7 +2075,7 @@ async fn permission_request_hook_uses_hook_tool_name_without_metadata() { let log_path = install_mcp_permission_request_hook( &mut session, &turn_context, - "mcp__memory__.*", + "memory__.*", &serde_json::json!({ "hookSpecificOutput": { "hookEventName": "PermissionRequest", @@ -2096,7 +2096,7 @@ async fn permission_request_hook_uses_hook_tool_name_without_metadata() { &turn_context, "call-mcp-hook-no-metadata", &invocation, - "mcp__memory__create_entities", + "memory__create_entities", /*metadata*/ None, AppToolApproval::Auto, ) @@ -2117,7 +2117,7 @@ async fn permission_request_hook_uses_hook_tool_name_without_metadata() { "transcript_path": null, "model": turn_context.model_info.slug, "permission_mode": "default", - "tool_name": "mcp__memory__create_entities", + "tool_name": "memory__create_entities", "hook_event_name": "PermissionRequest", "tool_input": { "entities": [] } })] @@ -2130,7 +2130,7 @@ async fn permission_request_hook_runs_after_remembered_mcp_approval() { let log_path = install_mcp_permission_request_hook( &mut session, &turn_context, - "mcp__memory__.*", + "memory__.*", &serde_json::json!({ "hookSpecificOutput": { "hookEventName": "PermissionRequest", @@ -2173,7 +2173,7 @@ async fn permission_request_hook_runs_after_remembered_mcp_approval() { &turn_context, "call-mcp-remembered", &invocation, - "mcp__memory__create_entities", + "memory__create_entities", Some(&metadata), AppToolApproval::Auto, ) @@ -2253,7 +2253,7 @@ async fn guardian_mode_mcp_denial_returns_rationale_message() { &turn_context, "call-guardian-deny", &invocation, - "mcp__test__tool", + "test__tool", Some(&metadata), AppToolApproval::Auto, ) @@ -2310,7 +2310,7 @@ async fn prompt_mode_waits_for_approval_when_annotations_do_not_require_approval &turn_context, "call-prompt", &invocation, - "mcp__test__tool", + "test__tool", Some(&metadata), AppToolApproval::Prompt, ) @@ -2385,7 +2385,7 @@ async fn approve_mode_blocks_when_arc_returns_interrupt_for_model() { &turn_context, "call-2", &invocation, - "mcp__test__tool", + "test__tool", Some(&metadata), AppToolApproval::Approve, ) @@ -2457,7 +2457,7 @@ async fn custom_approve_mode_blocks_when_arc_returns_interrupt_for_model() { &turn_context, "call-2-custom", &invocation, - "mcp__test__tool", + "test__tool", Some(&metadata), AppToolApproval::Approve, ) @@ -2529,7 +2529,7 @@ async fn approve_mode_blocks_when_arc_returns_interrupt_without_annotations() { &turn_context, "call-3", &invocation, - "mcp__test__tool", + "test__tool", Some(&metadata), AppToolApproval::Approve, ) @@ -2611,7 +2611,7 @@ async fn full_access_mode_skips_arc_monitor_for_all_approval_modes() { &turn_context, "call-2", &invocation, - "mcp__test__tool", + "test__tool", Some(&metadata), approval_mode, ) @@ -2701,7 +2701,7 @@ async fn approve_mode_skips_arc_and_guardian_when_guardian_reviewer_is_enabled() &turn_context, "call-3", &invocation, - "mcp__test__tool", + "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 cbd4d3b29c76..85df59e297f0 100644 --- a/codex-rs/core/src/mcp_tool_exposure_test.rs +++ b/codex-rs/core/src/mcp_tool_exposure_test.rs @@ -48,10 +48,10 @@ fn make_mcp_tool( let tool_namespace = if server_name == CODEX_APPS_MCP_SERVER_NAME { connector_name .map(sanitize_name) - .map(|connector_name| format!("mcp__{server_name}__{connector_name}")) + .map(|connector_name| format!("{server_name}__{connector_name}")) .unwrap_or_else(|| server_name.to_string()) } else { - format!("mcp__{server_name}__") + server_name.to_string() }; ToolInfo { @@ -82,7 +82,7 @@ fn numbered_mcp_tools(count: usize) -> HashMap { .map(|index| { let tool_name = format!("tool_{index}"); ( - format!("mcp__rmcp__{tool_name}"), + format!("rmcp__{tool_name}"), make_mcp_tool( "rmcp", &tool_name, /*connector_id*/ None, /*connector_name*/ None, ), @@ -165,7 +165,7 @@ async fn directly_exposes_explicit_apps_without_deferred_overlap() { let tools_config = tools_config_for_mcp_tool_exposure(/*search_tool*/ true).await; let mut mcp_tools = numbered_mcp_tools(DIRECT_MCP_TOOL_EXPOSURE_THRESHOLD - 1); mcp_tools.extend([( - "mcp__codex_apps__calendar_create_event".to_string(), + "codex_apps__calendar__create_event".to_string(), make_mcp_tool( CODEX_APPS_MCP_SERVER_NAME, "calendar_create_event", @@ -187,7 +187,7 @@ async fn directly_exposes_explicit_apps_without_deferred_overlap() { tool_names.sort(); assert_eq!( tool_names, - vec!["mcp__codex_apps__calendar_create_event".to_string()] + vec!["codex_apps__calendar__create_event".to_string()] ); assert_eq!( exposure.deferred_tools.as_ref().map(HashMap::len), @@ -203,8 +203,8 @@ async fn directly_exposes_explicit_apps_without_deferred_overlap() { .all(|direct_tool_name| !deferred_tools.contains_key(direct_tool_name)), "direct tools should not also be deferred: {tool_names:?}" ); - assert!(!deferred_tools.contains_key("mcp__codex_apps__calendar_create_event")); - assert!(deferred_tools.contains_key("mcp__rmcp__tool_0")); + assert!(!deferred_tools.contains_key("codex_apps__calendar__create_event")); + assert!(deferred_tools.contains_key("rmcp__tool_0")); } #[tokio::test] @@ -217,13 +217,13 @@ async fn always_defer_feature_preserves_explicit_apps() { let tools_config = tools_config_for_mcp_tool_exposure(/*search_tool*/ true).await; let mcp_tools = HashMap::from([ ( - "mcp__rmcp__tool".to_string(), + "rmcp__tool".to_string(), make_mcp_tool( "rmcp", "tool", /*connector_id*/ None, /*connector_name*/ None, ), ), ( - "mcp__codex_apps__calendar_create_event".to_string(), + "codex_apps__calendar__create_event".to_string(), make_mcp_tool( CODEX_APPS_MCP_SERVER_NAME, "calendar_create_event", @@ -246,12 +246,12 @@ async fn always_defer_feature_preserves_explicit_apps() { direct_tool_names.sort(); assert_eq!( direct_tool_names, - vec!["mcp__codex_apps__calendar_create_event".to_string()] + vec!["codex_apps__calendar__create_event".to_string()] ); let deferred_tools = exposure .deferred_tools .as_ref() .expect("MCP tools should be discoverable through tool_search"); - assert!(deferred_tools.contains_key("mcp__rmcp__tool")); - assert!(!deferred_tools.contains_key("mcp__codex_apps__calendar_create_event")); + assert!(deferred_tools.contains_key("rmcp__tool")); + assert!(!deferred_tools.contains_key("codex_apps__calendar__create_event")); } diff --git a/codex-rs/core/src/tools/handlers/mcp.rs b/codex-rs/core/src/tools/handlers/mcp.rs index 568e4561583c..cbe6cadbffe2 100644 --- a/codex-rs/core/src/tools/handlers/mcp.rs +++ b/codex-rs/core/src/tools/handlers/mcp.rs @@ -142,12 +142,12 @@ 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, }), Some(PreToolUsePayload { - tool_name: HookToolName::new("mcp__memory__create_entities"), + tool_name: HookToolName::new("memory__create_entities"), tool_input: json!({ "entities": [{ "name": "Ada", @@ -191,14 +191,14 @@ 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, }; assert_eq!( McpHandler.post_tool_use_payload(&invocation, &output), Some(PostToolUsePayload { - tool_name: HookToolName::new("mcp__filesystem__read_file"), + tool_name: HookToolName::new("filesystem__read_file"), tool_use_id: "call-mcp-post".to_string(), tool_input: json!({ "path": { diff --git a/codex-rs/core/src/tools/handlers/tool_search.rs b/codex-rs/core/src/tools/handlers/tool_search.rs index f38b4ee88321..5f20c6e9d470 100644 --- a/codex-rs/core/src/tools/handlers/tool_search.rs +++ b/codex-rs/core/src/tools/handlers/tool_search.rs @@ -202,11 +202,11 @@ mod tests { let handler = handler_from_tools( Some(&std::collections::HashMap::from([ ( - "mcp__calendar__create_event".to_string(), + "calendar__create_event".to_string(), tool_info("calendar", "create_event", "Create events"), ), ( - "mcp__calendar__list_events".to_string(), + "calendar__list_events".to_string(), tool_info("calendar", "list_events", "List events"), ), ])), @@ -226,8 +226,8 @@ mod tests { tools, vec![ LoadableToolSpec::Namespace(ResponsesApiNamespace { - name: "mcp__calendar__".to_string(), - description: "Tools in the mcp__calendar__ namespace.".to_string(), + name: "calendar".to_string(), + description: "Tools in the calendar namespace.".to_string(), tools: vec![ ResponsesApiNamespaceTool::Function(ResponsesApiTool { name: "create_event".to_string(), @@ -380,7 +380,7 @@ mod tests { .map(|index| { let tool_name = format!("tool_{index:03}"); ( - format!("mcp__{server_name}__{tool_name}"), + format!("{server_name}__{tool_name}"), tool_info(server_name, &tool_name, description_prefix), ) }) @@ -391,7 +391,7 @@ mod tests { ToolInfo { server_name: server_name.to_string(), callable_name: tool_name.to_string(), - callable_namespace: format!("mcp__{server_name}__"), + callable_namespace: server_name.to_string(), server_instructions: None, tool: Tool { name: tool_name.to_string().into(), diff --git a/codex-rs/core/src/tools/registry_tests.rs b/codex-rs/core/src/tools/registry_tests.rs index d44c3d0f9b8b..c49f113c6a45 100644 --- a/codex-rs/core/src/tools/registry_tests.rs +++ b/codex-rs/core/src/tools/registry_tests.rs @@ -23,7 +23,7 @@ impl ToolHandler for TestHandler { fn handler_looks_up_namespaced_aliases_explicitly() { let plain_handler = Arc::new(TestHandler) as Arc; let namespaced_handler = Arc::new(TestHandler) as Arc; - let namespace = "mcp__codex_apps__gmail"; + let namespace = "codex_apps__gmail"; let tool_name = "gmail_get_recent_emails"; let plain_name = codex_tools::ToolName::plain(tool_name); let namespaced_name = codex_tools::ToolName::namespaced(namespace, tool_name); @@ -35,7 +35,7 @@ fn handler_looks_up_namespaced_aliases_explicitly() { let plain = registry.handler(&plain_name); let namespaced = registry.handler(&namespaced_name); let missing_namespaced = registry.handler(&codex_tools::ToolName::namespaced( - "mcp__codex_apps__calendar", + "codex_apps__calendar", tool_name, )); diff --git a/codex-rs/core/src/tools/router_tests.rs b/codex-rs/core/src/tools/router_tests.rs index ac859d9aa61c..c1dc1283b014 100644 --- a/codex-rs/core/src/tools/router_tests.rs +++ b/codex-rs/core/src/tools/router_tests.rs @@ -55,7 +55,7 @@ async fn parallel_support_does_not_match_namespaced_local_tool_names() -> anyhow .expect("test session should expose a parallel shell-like tool"); assert!(!router.tool_supports_parallel(&ToolCall { - tool_name: ToolName::namespaced("mcp__server__", parallel_tool_name), + tool_name: ToolName::namespaced("server", parallel_tool_name), call_id: "call-namespaced-tool".to_string(), payload: ToolPayload::Function { arguments: "{}".to_string(), @@ -76,7 +76,7 @@ async fn build_tool_call_uses_namespace_for_registry_name() -> anyhow::Result<() ResponseItem::FunctionCall { id: None, name: tool_name.clone(), - namespace: Some("mcp__codex_apps__calendar".to_string()), + namespace: Some("codex_apps__calendar".to_string()), arguments: "{}".to_string(), call_id: "call-namespace".to_string(), }, @@ -86,7 +86,7 @@ async fn build_tool_call_uses_namespace_for_registry_name() -> anyhow::Result<() assert_eq!( call.tool_name, - ToolName::namespaced("mcp__codex_apps__calendar", tool_name) + ToolName::namespaced("codex_apps__calendar", tool_name) ); assert_eq!(call.call_id, "call-namespace"); match call.payload { @@ -115,7 +115,7 @@ async fn mcp_parallel_support_uses_exact_payload_server() -> anyhow::Result<()> ); let deferred_call = ToolCall { - tool_name: ToolName::namespaced("mcp__echo__", "query_with_delay"), + tool_name: ToolName::namespaced("echo", "query_with_delay"), call_id: "call-deferred".to_string(), payload: ToolPayload::Mcp { server: "echo".to_string(), @@ -126,7 +126,7 @@ async fn mcp_parallel_support_uses_exact_payload_server() -> anyhow::Result<()> assert!(router.tool_supports_parallel(&deferred_call)); let different_server_call = ToolCall { - tool_name: ToolName::namespaced("mcp__hello_echo__", "query_with_delay"), + tool_name: ToolName::namespaced("hello_echo", "query_with_delay"), call_id: "call-other-server".to_string(), payload: ToolPayload::Mcp { server: "hello_echo".to_string(), diff --git a/codex-rs/core/src/tools/spec_tests.rs b/codex-rs/core/src/tools/spec_tests.rs index afa586dc62cf..69673ccbe248 100644 --- a/codex-rs/core/src/tools/spec_tests.rs +++ b/codex-rs/core/src/tools/spec_tests.rs @@ -61,7 +61,7 @@ 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(), + callable_namespace: "test_server".to_string(), server_instructions: None, tool, connector_id: None, @@ -133,7 +133,7 @@ fn deferred_responses_api_tool_serializes_with_defer_loading() { let serialized = serde_json::to_value(ToolSpec::Function( mcp_tool_to_deferred_responses_api_tool( - &ToolName::namespaced("mcp__codex_apps__", "lookup_order"), + &ToolName::namespaced("codex_apps", "lookup_order"), &tool, ) .expect("convert deferred tool"), @@ -893,11 +893,11 @@ async fn search_tool_description_falls_back_to_connector_name_without_descriptio &tools_config, /*mcp_tools*/ None, Some(HashMap::from([( - "mcp__codex_apps__calendar_create_event".to_string(), + "codex_apps__calendar__create_event".to_string(), ToolInfo { server_name: CODEX_APPS_MCP_SERVER_NAME.to_string(), - callable_name: "_create_event".to_string(), - callable_namespace: "mcp__codex_apps__calendar".to_string(), + callable_name: "create_event".to_string(), + callable_namespace: "codex_apps__calendar".to_string(), server_instructions: None, tool: mcp_tool( "calendar_create_event", @@ -945,11 +945,11 @@ async fn search_tool_registers_namespaced_mcp_tool_aliases() { /*mcp_tools*/ None, Some(HashMap::from([ ( - "mcp__codex_apps__calendar_create_event".to_string(), + "codex_apps__calendar__create_event".to_string(), ToolInfo { server_name: CODEX_APPS_MCP_SERVER_NAME.to_string(), - callable_name: "_create_event".to_string(), - callable_namespace: "mcp__codex_apps__calendar".to_string(), + callable_name: "create_event".to_string(), + callable_namespace: "codex_apps__calendar".to_string(), server_instructions: None, tool: mcp_tool( "calendar-create-event", @@ -963,11 +963,11 @@ async fn search_tool_registers_namespaced_mcp_tool_aliases() { }, ), ( - "mcp__codex_apps__calendar_list_events".to_string(), + "codex_apps__calendar__list_events".to_string(), ToolInfo { server_name: CODEX_APPS_MCP_SERVER_NAME.to_string(), - callable_name: "_list_events".to_string(), - callable_namespace: "mcp__codex_apps__calendar".to_string(), + callable_name: "list_events".to_string(), + callable_namespace: "codex_apps__calendar".to_string(), server_instructions: None, tool: mcp_tool( "calendar-list-events", @@ -981,11 +981,11 @@ async fn search_tool_registers_namespaced_mcp_tool_aliases() { }, ), ( - "mcp__rmcp__echo".to_string(), + "rmcp__echo".to_string(), ToolInfo { server_name: "rmcp".to_string(), callable_name: "echo".to_string(), - callable_namespace: "mcp__rmcp__".to_string(), + callable_namespace: "rmcp".to_string(), server_instructions: None, tool: mcp_tool("echo", "Echo", serde_json::json!({"type": "object"})), connector_id: None, @@ -999,8 +999,8 @@ async fn search_tool_registers_namespaced_mcp_tool_aliases() { ) .build(); - let app_alias = ToolName::namespaced("mcp__codex_apps__calendar", "_create_event"); - let mcp_alias = ToolName::namespaced("mcp__rmcp__", "echo"); + let app_alias = ToolName::namespaced("codex_apps__calendar", "create_event"); + let mcp_alias = ToolName::namespaced("rmcp", "echo"); assert!(registry.has_handler(&ToolName::plain(TOOL_SEARCH_TOOL_NAME))); assert!(registry.has_handler(&app_alias)); @@ -1025,7 +1025,7 @@ async fn tool_search_entries_skip_namespace_outputs_when_namespace_tools_are_dis }); tools_config.namespace_tools = false; let mcp_tools = HashMap::from([( - "mcp__test_server__echo".to_string(), + "test_server__echo".to_string(), mcp_tool_info(mcp_tool( "echo", "Echo", @@ -1084,7 +1084,7 @@ async fn direct_mcp_tools_register_namespaced_handlers() { let (_, registry) = build_specs( &tools_config, Some(HashMap::from([( - "mcp__test_server__echo".to_string(), + "test_server__echo".to_string(), mcp_tool_info(mcp_tool( "echo", "Echo", @@ -1096,8 +1096,8 @@ async fn direct_mcp_tools_register_namespaced_handlers() { ) .build(); - assert!(registry.has_handler(&ToolName::namespaced("mcp__test_server__", "echo"))); - assert!(!registry.has_handler(&ToolName::plain("mcp__test_server__echo"))); + assert!(registry.has_handler(&ToolName::namespaced("test_server", "echo"))); + assert!(!registry.has_handler(&ToolName::plain("test_server__echo"))); } #[tokio::test] @@ -1118,7 +1118,7 @@ async fn unavailable_mcp_tools_are_exposed_as_dummy_function_tools() { windows_sandbox_level: WindowsSandboxLevel::Disabled, }); - let unavailable_tool = ToolName::namespaced("mcp__codex_apps__calendar", "_create_event"); + let unavailable_tool = ToolName::namespaced("codex_apps__calendar", "create_event"); let (tools, registry) = build_specs_with_unavailable_tools( &tools_config, /*mcp_tools*/ None, @@ -1128,7 +1128,7 @@ async fn unavailable_mcp_tools_are_exposed_as_dummy_function_tools() { ) .build(); - let tool = find_tool(&tools, "mcp__codex_apps__calendar_create_event"); + let tool = find_tool(&tools, "codex_apps__calendar__create_event"); let ToolSpec::Function(ResponsesApiTool { description, parameters, @@ -1143,10 +1143,10 @@ async fn unavailable_mcp_tools_are_exposed_as_dummy_function_tools() { Some(AdditionalProperties::Boolean(false)) ); assert!(registry.has_handler(&ToolName::namespaced( - "mcp__codex_apps__calendar", - "_create_event" + "codex_apps__calendar", + "create_event" ))); - assert!(!registry.has_handler(&ToolName::plain("mcp__codex_apps__calendar_create_event"))); + assert!(!registry.has_handler(&ToolName::plain("codex_apps__calendar__create_event"))); } #[tokio::test] diff --git a/codex-rs/core/src/unavailable_tool.rs b/codex-rs/core/src/unavailable_tool.rs index 39ba21f9b86a..07f57be31900 100644 --- a/codex-rs/core/src/unavailable_tool.rs +++ b/codex-rs/core/src/unavailable_tool.rs @@ -39,7 +39,10 @@ pub(crate) fn collect_unavailable_called_tools( } fn should_collect_unavailable_tool(name: &str, namespace: Option<&str>) -> bool { - namespace.is_some_and(|namespace| namespace.starts_with("mcp__")) || name.starts_with("mcp__") + // New histories preserve the namespace split, so any missing namespaced call + // is eligible for a placeholder. Keep the flattened MCP branch for rollouts + // written before namespaced MCP calls were preserved in history. + namespace.is_some() || name.starts_with("mcp__") } #[cfg(test)] @@ -58,11 +61,12 @@ mod tests { } #[test] - fn collect_unavailable_called_tools_detects_mcp_function_calls() { + fn collect_unavailable_called_tools_detects_namespaced_and_legacy_mcp_calls() { let input = vec![ function_call("shell", /*namespace*/ None), function_call("mcp__server__lookup", /*namespace*/ None), function_call("_create_event", Some("mcp__codex_apps__calendar")), + function_call("lookup", Some("calendar")), ]; let tools = collect_unavailable_called_tools(&input, &HashSet::new()); @@ -70,6 +74,7 @@ mod tests { assert_eq!( tools, vec![ + ToolName::namespaced("calendar", "lookup"), ToolName::namespaced("mcp__codex_apps__calendar", "_create_event"), ToolName::plain("mcp__server__lookup"), ] diff --git a/codex-rs/core/tests/suite/code_mode.rs b/codex-rs/core/tests/suite/code_mode.rs index 3bcb37e7b277..f455b350eac7 100644 --- a/codex-rs/core/tests/suite/code_mode.rs +++ b/codex-rs/core/tests/suite/code_mode.rs @@ -367,7 +367,7 @@ async fn code_mode_only_guides_all_tools_search_and_calls_deferred_app_tools() - "exec", r#" const tool = ALL_TOOLS.find( - ({ name }) => name === "mcp__codex_apps__calendar_timezone_option_99" + ({ name }) => name === "codex_apps__calendar_timezone_option_99" ); if (!tool) { text(JSON.stringify({ found: false })); @@ -2075,7 +2075,7 @@ async fn code_mode_can_use_mcp_image_result_with_image_helper() -> Result<()> { let server = responses::start_mock_server().await; let code = r#" -const out = await tools.mcp__rmcp__image_scenario({ +const out = await tools.rmcp_image_scenario({ scenario: "image_only_original_detail", }); const imageItem = out.content.find((item) => item.type === "image"); @@ -2176,7 +2176,7 @@ async fn code_mode_can_print_structured_mcp_tool_result_fields() -> Result<()> { let server = responses::start_mock_server().await; let code = r#" -const { content, structuredContent, isError } = await tools.mcp__rmcp__echo({ +const { content, structuredContent, isError } = await tools.rmcp_echo({ message: "ping", }); text( @@ -2214,7 +2214,7 @@ async fn code_mode_only_can_call_mcp_tool() -> Result<()> { let server = responses::start_mock_server().await; let code = r#" -const result = await tools.mcp__rmcp__echo({ message: "ping" }); +const result = await tools.rmcp_echo({ message: "ping" }); text(`echo=${result.structuredContent?.echo ?? "missing"}`); "#; @@ -2244,12 +2244,12 @@ async fn code_mode_exposes_mcp_tools_on_global_tools_object() -> Result<()> { let server = responses::start_mock_server().await; let code = r#" -const { content, structuredContent, isError } = await tools.mcp__rmcp__echo({ +const { content, structuredContent, isError } = await tools.rmcp_echo({ message: "ping", }); text( - `hasEcho=${String(Object.keys(tools).includes("mcp__rmcp__echo"))}\n` + - `echoType=${typeof tools.mcp__rmcp__echo}\n` + + `hasEcho=${String(Object.keys(tools).includes("rmcp_echo"))}\n` + + `echoType=${typeof tools.rmcp_echo}\n` + `echo=${structuredContent?.echo ?? "missing"}\n` + `isError=${String(isError)}\n` + `contentLength=${content.length}` @@ -2287,7 +2287,7 @@ async fn code_mode_exposes_namespaced_mcp_tools_on_global_tools_object() -> Resu let code = r#" text(JSON.stringify({ hasExecCommand: typeof tools.exec_command === "function", - hasNamespacedEcho: typeof tools.mcp__rmcp__echo === "function", + hasNamespacedEcho: typeof tools.rmcp_echo === "function", })); "#; @@ -2321,7 +2321,7 @@ async fn code_mode_exposes_normalized_illegal_mcp_tool_names() -> Result<()> { let server = responses::start_mock_server().await; let code = r#" -const result = await tools.mcp__rmcp__echo_tool({ message: "ping" }); +const result = await tools.rmcp_echo_tool({ message: "ping" }); text(`echo=${result.structuredContent.echo}`); "#; @@ -2502,7 +2502,7 @@ async fn code_mode_exports_all_tools_metadata_for_namespaced_mcp_tools() -> Resu let server = responses::start_mock_server().await; let code = r#" const tool = ALL_TOOLS.find( - ({ name }) => name === "mcp__rmcp__echo" + ({ name }) => name === "rmcp_echo" ); text(JSON.stringify(tool)); "#; @@ -2525,12 +2525,12 @@ text(JSON.stringify(tool)); assert_eq!( parsed, serde_json::json!({ - "name": "mcp__rmcp__echo", + "name": "rmcp_echo", "description": concat!( "Echo back the provided message and include environment data.\n\n", "exec tool declaration:\n", "```ts\n", - "declare const tools: { mcp__rmcp__echo(args: { env_var?: string; message: string; }): ", + "declare const tools: { rmcp_echo(args: { env_var?: string; message: string; }): ", "Promise>; };\n", "```", ), @@ -2701,7 +2701,7 @@ async fn code_mode_can_print_content_only_mcp_tool_result_fields() -> Result<()> let server = responses::start_mock_server().await; let code = r#" -const { content, structuredContent, isError } = await tools.mcp__rmcp__image_scenario({ +const { content, structuredContent, isError } = await tools.rmcp_image_scenario({ scenario: "text_only", caption: "caption from mcp", }); @@ -2744,7 +2744,7 @@ async fn code_mode_can_print_error_mcp_tool_result_fields() -> Result<()> { let server = responses::start_mock_server().await; let code = r#" -const { content, structuredContent, isError } = await tools.mcp__rmcp__echo({}); +const { content, structuredContent, isError } = await tools.rmcp_echo({}); const firstText = content[0]?.text ?? ""; const mentionsMissingMessage = firstText.includes("missing field") && firstText.includes("message"); diff --git a/codex-rs/core/tests/suite/hooks_mcp.rs b/codex-rs/core/tests/suite/hooks_mcp.rs index 2157630e02b0..bbe8d8ff1f85 100644 --- a/codex-rs/core/tests/suite/hooks_mcp.rs +++ b/codex-rs/core/tests/suite/hooks_mcp.rs @@ -26,9 +26,9 @@ use serde_json::Value; use serde_json::json; const RMCP_SERVER: &str = "rmcp"; -const RMCP_NAMESPACE: &str = "mcp__rmcp__"; -const RMCP_ECHO_TOOL_NAME: &str = "mcp__rmcp__echo"; -const RMCP_HOOK_MATCHER: &str = "mcp__rmcp__.*"; +const RMCP_NAMESPACE: &str = "rmcp"; +const RMCP_ECHO_TOOL_NAME: &str = "rmcp__echo"; +const RMCP_HOOK_MATCHER: &str = "rmcp__.*"; const RMCP_ECHO_MESSAGE: &str = "hook e2e ping"; fn write_pre_tool_use_hook(home: &Path, reason: &str) -> Result<()> { diff --git a/codex-rs/core/tests/suite/openai_file_mcp.rs b/codex-rs/core/tests/suite/openai_file_mcp.rs index ac49b5334b3e..13762d6caf9f 100644 --- a/codex-rs/core/tests/suite/openai_file_mcp.rs +++ b/codex-rs/core/tests/suite/openai_file_mcp.rs @@ -30,9 +30,9 @@ use wiremock::matchers::header; use wiremock::matchers::method; use wiremock::matchers::path; -const DOCUMENT_EXTRACT_NAMESPACE: &str = "mcp__codex_apps__calendar"; -const DOCUMENT_EXTRACT_TOOL: &str = "_extract_text"; -const DOCUMENT_EXTRACT_HOOK_MATCHER: &str = "mcp__codex_apps__calendar_extract_text"; +const DOCUMENT_EXTRACT_NAMESPACE: &str = "codex_apps__calendar"; +const DOCUMENT_EXTRACT_TOOL: &str = "extract_text"; +const DOCUMENT_EXTRACT_HOOK_MATCHER: &str = "codex_apps__calendar__extract_text"; fn configure_apps(config: &mut Config, chatgpt_base_url: &str) { if let Err(err) = config.features.enable(Feature::Apps) { diff --git a/codex-rs/core/tests/suite/plugins.rs b/codex-rs/core/tests/suite/plugins.rs index 5b83d3b13663..00b42c0904f8 100644 --- a/codex-rs/core/tests/suite/plugins.rs +++ b/codex-rs/core/tests/suite/plugins.rs @@ -347,11 +347,11 @@ async fn explicit_plugin_mentions_inject_plugin_guidance() -> Result<()> { assert!( request_tools .iter() - .any(|name| name == "mcp__codex_apps__google_calendar"), + .any(|name| name == "codex_apps__google_calendar"), "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("sample", "echo") .expect("plugin MCP tool should be present"); let echo_description = echo_tool .get("description") @@ -362,7 +362,7 @@ async fn explicit_plugin_mentions_inject_plugin_guidance() -> Result<()> { "expected plugin MCP provenance in tool description: {echo_description:?}" ); let calendar_tool = request - .tool_by_name("mcp__codex_apps__google_calendar", "_create_event") + .tool_by_name("codex_apps__google_calendar", "create_event") .expect("plugin app tool should be present"); let calendar_description = calendar_tool .get("description") @@ -471,8 +471,8 @@ async fn plugin_mcp_tools_are_listed() -> Result<()> { let mut available_tools: Vec<&str> = tool_list.tools.keys().map(String::as_str).collect(); available_tools.sort_unstable(); assert!( - tool_list.tools.contains_key("mcp__sample__echo") - && tool_list.tools.contains_key("mcp__sample__image"), + tool_list.tools.contains_key("sample__echo") + && tool_list.tools.contains_key("sample__image"), "expected plugin MCP tools to be listed; discovered tools: {available_tools:?}" ); diff --git a/codex-rs/core/tests/suite/rmcp_client.rs b/codex-rs/core/tests/suite/rmcp_client.rs index 0947f4fba76e..6e4f7d0cbadf 100644 --- a/codex-rs/core/tests/suite/rmcp_client.rs +++ b/codex-rs/core/tests/suite/rmcp_client.rs @@ -336,7 +336,7 @@ async fn call_cwd_tool( server_name: &str, call_id: &str, ) -> anyhow::Result { - let namespace = format!("mcp__{server_name}__"); + let namespace = server_name.to_string(); mount_sse_once( server, responses::sse(vec![ @@ -421,7 +421,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 = server_name.to_string(); let call_mock = mount_sse_once( &server, @@ -730,7 +730,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 = server_name.to_string(); let tool_name = format!("{namespace}sandbox_meta"); let call_mock = mount_sse_once( @@ -848,7 +848,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 = server_name.to_string(); let args = json!({ "sleep_after_ms": 100 }).to_string(); mount_sse_once( @@ -957,7 +957,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 = server_name.to_string(); let args = json!({ "sleep_after_ms": 100, "barrier": { @@ -1039,8 +1039,8 @@ async fn stdio_image_responses_round_trip() -> anyhow::Result<()> { let call_id = "img-1"; let server_name = "rmcp"; - let tool_name = format!("mcp__{server_name}__image"); - let namespace = format!("mcp__{server_name}__"); + let tool_name = format!("{server_name}__image"); + let namespace = server_name.to_string(); // First stream: model decides to call the image tool. mount_sse_once( @@ -1176,8 +1176,8 @@ async fn stdio_image_responses_preserve_original_detail_metadata() -> anyhow::Re let call_id = "img-original-detail-1"; let server_name = "rmcp"; - let tool_name = format!("mcp__{server_name}__image_scenario"); - let namespace = format!("mcp__{server_name}__"); + let tool_name = format!("{server_name}__image_scenario"); + let namespace = server_name.to_string(); mount_sse_once( &server, @@ -1263,7 +1263,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 = server_name.to_string(); let text_only_model_slug = "rmcp-text-only-model"; let models_mock = mount_models_once( @@ -1408,7 +1408,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 = server_name.to_string(); mount_sse_once( &server, @@ -1522,7 +1522,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 = server_name.to_string(); let env_name = "MCP_TEST_LOCAL_SOURCE"; let expected_env_value = "propagated-explicit-local-source"; @@ -1615,7 +1615,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 = server_name.to_string(); let env_name = "MCP_TEST_REMOTE_SOURCE_ONLY"; mount_sse_once( @@ -1787,7 +1787,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 = server_name.to_string(); mount_sse_once( &server, @@ -1954,8 +1954,8 @@ async fn streamable_http_with_oauth_round_trip_impl() -> anyhow::Result<()> { let call_id = "call-789"; let server_name = "rmcp_http_oauth"; - let tool_name = format!("mcp__{server_name}__echo"); - let namespace = format!("mcp__{server_name}__"); + let tool_name = format!("{server_name}__echo"); + let namespace = server_name.to_string(); 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 b4edf24668ef..bf46217866a6 100644 --- a/codex-rs/core/tests/suite/search_tool.rs +++ b/codex-rs/core/tests/suite/search_tool.rs @@ -47,11 +47,11 @@ const SEARCH_TOOL_DESCRIPTION_SNIPPETS: [&str; 2] = [ "- Calendar: Plan events and manage your calendar.", ]; const TOOL_SEARCH_TOOL_NAME: &str = "tool_search"; -const CALENDAR_CREATE_TOOL: &str = "mcp__codex_apps__calendar_create_event"; -const CALENDAR_LIST_TOOL: &str = "mcp__codex_apps__calendar_list_events"; -const SEARCH_CALENDAR_NAMESPACE: &str = "mcp__codex_apps__calendar"; -const SEARCH_CALENDAR_CREATE_TOOL: &str = "_create_event"; -const SEARCH_CALENDAR_LIST_TOOL: &str = "_list_events"; +const CALENDAR_CREATE_TOOL: &str = "codex_apps__calendar__create_event"; +const CALENDAR_LIST_TOOL: &str = "codex_apps__calendar__list_events"; +const SEARCH_CALENDAR_NAMESPACE: &str = "codex_apps__calendar"; +const SEARCH_CALENDAR_CREATE_TOOL: &str = "create_event"; +const SEARCH_CALENDAR_LIST_TOOL: &str = "list_events"; fn tool_names(body: &Value) -> Vec { body.get("tools") @@ -235,7 +235,7 @@ async fn always_defer_feature_hides_small_app_tool_sets() -> Result<()> { "small app tool sets should be deferred behind tool_search: {tools:?}" ); assert!( - tools.iter().all(|name| !name.starts_with("mcp__")), + tools.iter().all(|name| !name.starts_with("codex_apps__")), "MCP tools should not be directly exposed: {tools:?}" ); @@ -996,19 +996,17 @@ async fn tool_search_indexes_only_enabled_non_app_mcp_tools() -> Result<()> { "first request should advertise tool_search: {first_request_tools:?}" ); assert!( - !first_request_tools - .iter() - .any(|name| name == "mcp__rmcp__echo"), + !first_request_tools.iter().any(|name| name == "rmcp__echo"), "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 == "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, "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), @@ -1018,7 +1016,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("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()); diff --git a/codex-rs/core/tests/suite/sqlite_state.rs b/codex-rs/core/tests/suite/sqlite_state.rs index 8250f5493dea..1bc9c42b96fd 100644 --- a/codex-rs/core/tests/suite/sqlite_state.rs +++ b/codex-rs/core/tests/suite/sqlite_state.rs @@ -328,7 +328,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 = server_name.to_string(); mount_sse_once( &server, responses::sse(vec![ diff --git a/codex-rs/core/tests/suite/tools.rs b/codex-rs/core/tests/suite/tools.rs index b85607454061..e781d1a72f2c 100644 --- a/codex-rs/core/tests/suite/tools.rs +++ b/codex-rs/core/tests/suite/tools.rs @@ -349,6 +349,184 @@ async fn historical_unavailable_mcp_call_is_exposed_as_placeholder_tool() -> Res Ok(()) } +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn historical_unavailable_namespaced_call_is_exposed_as_placeholder_tool() -> Result<()> { + skip_if_no_network!(Ok(())); + + let historical_call_id = "historical-namespaced-call"; + let retry_call_id = "retry-namespaced-call"; + let unavailable_tool_namespace = "legacy_tools"; + let unavailable_tool_name = "missing_tool"; + let unavailable_tool_display_name = "legacy_tools__missing_tool"; + let server = start_mock_server().await; + let codex_home = Arc::new(TempDir::new()?); + let mut builder = test_codex() + .with_model("gpt-5.4") + .with_home(Arc::clone(&codex_home)) + .with_config(|config| { + config + .features + .enable(Feature::UnavailableDummyTools) + .expect("unavailable dummy tools should be enabled for this test"); + }); + let test = builder.build(&server).await?; + + let first_turn_mock = mount_sse_sequence( + &server, + vec![ + sse(vec![ + ev_response_created("resp-1"), + ev_namespaced_function_call( + historical_call_id, + unavailable_tool_namespace, + unavailable_tool_name, + r#"{}"#, + ), + ev_completed("resp-1"), + ]), + sse(vec![ + ev_response_created("resp-2"), + ev_assistant_message("msg-1", "done"), + ev_completed("resp-2"), + ]), + ], + ) + .await; + + test.submit_turn("call a namespaced tool").await?; + let rollout_path = test.codex.rollout_path().context("rollout path")?; + assert_eq!(first_turn_mock.requests().len(), 2); + drop(test); + + let retry_mock = mount_sse_sequence( + &server, + vec![ + sse(vec![ + ev_response_created("resp-3"), + ev_namespaced_function_call( + retry_call_id, + unavailable_tool_namespace, + unavailable_tool_name, + r#"{}"#, + ), + ev_completed("resp-3"), + ]), + sse(vec![ + ev_response_created("resp-4"), + ev_assistant_message("msg-2", "done"), + ev_completed("resp-4"), + ]), + ], + ) + .await; + + let mut resume_builder = test_codex().with_model("gpt-5.4").with_config(|config| { + config + .features + .enable(Feature::UnavailableDummyTools) + .expect("unavailable dummy tools should be enabled for this test"); + }); + let test = resume_builder + .resume(&server, codex_home, rollout_path) + .await?; + + test.submit_turn("retry the namespaced tool").await?; + + let requests = retry_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 == unavailable_tool_display_name), + "historical unavailable namespaced call should add a placeholder tool; got {first_request_tools:?}" + ); + let output_text = requests[1] + .function_call_output_text(retry_call_id) + .context("placeholder tool output present")?; + assert!(output_text.contains("not currently available")); + assert!( + !output_text.contains("unsupported call"), + "placeholder handler should answer instead of falling back to unsupported call: {output_text}" + ); + + Ok(()) +} + +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +#[serial(mcp_test_value)] +async fn reserved_mcp_namespace_is_not_exposed() -> Result<()> { + skip_if_no_network!(Ok(())); + + let server = start_mock_server().await; + let response_mock = mount_sse_once( + &server, + sse(vec![ + ev_response_created("resp-1"), + ev_assistant_message("msg-1", "done"), + ev_completed("resp-1"), + ]), + ) + .await; + let rmcp_test_server_bin = match stdio_server_bin() { + Ok(bin) => bin, + Err(err) => { + eprintln!("test_stdio_server binary not available, skipping test: {err}"); + return Ok(()); + } + }; + + let mut builder = test_codex().with_config(move |config| { + let mut servers = config.mcp_servers.get().clone(); + for server_name in ["rmcp", "tools"] { + servers.insert( + server_name.to_string(), + McpServerConfig { + transport: McpServerTransportConfig::Stdio { + command: rmcp_test_server_bin.clone(), + args: Vec::new(), + env: Some(HashMap::new()), + env_vars: Vec::new(), + cwd: None, + }, + experimental_environment: None, + enabled: true, + required: false, + supports_parallel_tool_calls: false, + disabled_reason: None, + startup_timeout_sec: Some(Duration::from_secs(10)), + tool_timeout_sec: None, + default_tools_approval_mode: None, + enabled_tools: Some(vec!["echo".to_string()]), + disabled_tools: None, + 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("which tools are available?").await?; + + let tools = tool_names(&response_mock.single_request().body_json()); + assert!( + tools.iter().any(|name| name == "rmcp"), + "non-reserved MCP namespace should remain visible; got {tools:?}" + ); + assert!( + !tools.iter().any(|name| name == "tools"), + "reserved MCP namespace should be skipped; got {tools:?}" + ); + + Ok(()) +} + #[tokio::test(flavor = "multi_thread", worker_threads = 2)] async fn shell_escalated_permissions_rejected_then_ok() -> Result<()> { skip_if_no_network!(Ok(())); diff --git a/codex-rs/core/tests/suite/truncation.rs b/codex-rs/core/tests/suite/truncation.rs index 0fd072a70f74..bd8c6e87ce55 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 = server_name.to_string(); // Build a very large message to exceed 10KiB once serialized. let large_msg = "long-message-with-newlines-".repeat(6000); @@ -445,7 +445,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 = server_name.to_string(); mount_sse_once( &server, @@ -726,7 +726,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 = server_name.to_string(); let large_msg = "a".repeat(80_000); let args_json = serde_json::json!({ "message": large_msg }); diff --git a/codex-rs/protocol/src/models.rs b/codex-rs/protocol/src/models.rs index 198a191d4e9c..20723bac86a0 100644 --- a/codex-rs/protocol/src/models.rs +++ b/codex-rs/protocol/src/models.rs @@ -2090,8 +2090,8 @@ mod tests { fn function_call_deserializes_optional_namespace() { let item: ResponseItem = serde_json::from_value(serde_json::json!({ "type": "function_call", - "name": "mcp__codex_apps__gmail_get_recent_emails", - "namespace": "mcp__codex_apps__gmail", + "name": "get_recent_emails", + "namespace": "codex_apps__gmail", "arguments": "{\"top_k\":5}", "call_id": "call-1", })) @@ -2101,8 +2101,8 @@ mod tests { item, ResponseItem::FunctionCall { id: None, - name: "mcp__codex_apps__gmail_get_recent_emails".to_string(), - namespace: Some("mcp__codex_apps__gmail".to_string()), + name: "get_recent_emails".to_string(), + namespace: Some("codex_apps__gmail".to_string()), arguments: "{\"top_k\":5}".to_string(), call_id: "call-1".to_string(), } @@ -2616,7 +2616,7 @@ mod tests { execution: "client".to_string(), tools: vec![serde_json::json!({ "type": "function", - "name": "mcp__codex_apps__calendar_create_event", + "name": "codex_apps__calendar__create_event", "description": "Create a calendar event.", "defer_loading": true, "parameters": { @@ -2637,7 +2637,7 @@ mod tests { execution: "client".to_string(), tools: vec![serde_json::json!({ "type": "function", - "name": "mcp__codex_apps__calendar_create_event", + "name": "codex_apps__calendar__create_event", "description": "Create a calendar event.", "defer_loading": true, "parameters": { @@ -2661,7 +2661,7 @@ mod tests { "execution": "client", "tools": [{ "type": "function", - "name": "mcp__codex_apps__calendar_create_event", + "name": "codex_apps__calendar__create_event", "description": "Create a calendar event.", "defer_loading": true, "parameters": { diff --git a/codex-rs/protocol/src/tool_name.rs b/codex-rs/protocol/src/tool_name.rs index d09bc1ce72d4..bcd97a063f7a 100644 --- a/codex-rs/protocol/src/tool_name.rs +++ b/codex-rs/protocol/src/tool_name.rs @@ -34,7 +34,7 @@ impl ToolName { pub fn display(&self) -> String { match &self.namespace { - Some(namespace) => format!("{namespace}{}", self.name), + Some(namespace) => flatten_namespaced_tool_name(namespace, &self.name), None => self.name.clone(), } } @@ -43,12 +43,20 @@ impl ToolName { impl fmt::Display for ToolName { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { match &self.namespace { - Some(namespace) => write!(f, "{namespace}{}", self.name), + Some(namespace) => f.write_str(&flatten_namespaced_tool_name(namespace, &self.name)), None => f.write_str(&self.name), } } } +fn flatten_namespaced_tool_name(namespace: &str, name: &str) -> String { + if namespace.ends_with("__") || name.starts_with('_') { + format!("{namespace}{name}") + } else { + format!("{namespace}__{name}") + } +} + impl From for ToolName { fn from(name: String) -> Self { Self::plain(name) @@ -60,3 +68,25 @@ impl From<&str> for ToolName { Self::plain(name) } } + +#[cfg(test)] +mod tests { + use super::ToolName; + + #[test] + fn display_flattens_namespaced_tools_with_double_underscore_separator() { + assert_eq!(ToolName::namespaced("foo_", "bar").display(), "foo___bar"); + } + + #[test] + fn display_preserves_legacy_flattened_namespaced_tools() { + assert_eq!( + ToolName::namespaced("mcp__rmcp__", "echo").display(), + "mcp__rmcp__echo" + ); + assert_eq!( + ToolName::namespaced("mcp__codex_apps__calendar", "_create_event").display(), + "mcp__codex_apps__calendar_create_event" + ); + } +} diff --git a/codex-rs/tools/src/responses_api_tests.rs b/codex-rs/tools/src/responses_api_tests.rs index 3ce13ebfe8c3..62022de2a63e 100644 --- a/codex-rs/tools/src/responses_api_tests.rs +++ b/codex-rs/tools/src/responses_api_tests.rs @@ -108,7 +108,7 @@ fn mcp_tool_to_deferred_responses_api_tool_sets_defer_loading() { assert_eq!( mcp_tool_to_deferred_responses_api_tool( - &ToolName::namespaced("mcp__codex_apps__", "lookup_order"), + &ToolName::namespaced("codex_apps", "lookup_order"), &tool, ) .expect("convert deferred tool"), @@ -133,7 +133,7 @@ fn mcp_tool_to_deferred_responses_api_tool_sets_defer_loading() { #[test] fn loadable_tool_spec_namespace_serializes_with_deferred_child_tools() { let namespace = LoadableToolSpec::Namespace(ResponsesApiNamespace { - name: "mcp__codex_apps__calendar".to_string(), + name: "codex_apps__calendar".to_string(), description: "Plan events".to_string(), tools: vec![ResponsesApiNamespaceTool::Function(ResponsesApiTool { name: "create_event".to_string(), @@ -155,7 +155,7 @@ fn loadable_tool_spec_namespace_serializes_with_deferred_child_tools() { value, json!({ "type": "namespace", - "name": "mcp__codex_apps__calendar", + "name": "codex_apps__calendar", "description": "Plan events", "tools": [ { diff --git a/codex-rs/tools/src/tool_definition_tests.rs b/codex-rs/tools/src/tool_definition_tests.rs index 0d4d97f5ce9e..da6c2c398386 100644 --- a/codex-rs/tools/src/tool_definition_tests.rs +++ b/codex-rs/tools/src/tool_definition_tests.rs @@ -22,9 +22,9 @@ fn tool_definition() -> ToolDefinition { #[test] fn renamed_overrides_name_only() { assert_eq!( - tool_definition().renamed("mcp__orders__lookup_order".to_string()), + tool_definition().renamed("orders__lookup_order".to_string()), ToolDefinition { - name: "mcp__orders__lookup_order".to_string(), + name: "orders__lookup_order".to_string(), ..tool_definition() } ); diff --git a/codex-rs/tools/src/tool_registry_plan.rs b/codex-rs/tools/src/tool_registry_plan.rs index 1da71ab04ada..b8b2e00fe7c5 100644 --- a/codex-rs/tools/src/tool_registry_plan.rs +++ b/codex-rs/tools/src/tool_registry_plan.rs @@ -68,6 +68,7 @@ use crate::tool_registry_plan_types::agent_type_description; use codex_protocol::openai_models::ApplyPatchToolType; use codex_protocol::openai_models::ConfigShellToolType; use std::collections::BTreeMap; +use std::collections::HashSet; pub fn build_tool_registry_plan( config: &ToolsConfig, @@ -260,53 +261,6 @@ pub fn build_tool_registry_plan( plan.register_handler("request_permissions", ToolHandlerKind::RequestPermissions); } - let deferred_dynamic_tools = params - .dynamic_tools - .iter() - .filter(|tool| tool.defer_loading && (config.namespace_tools || tool.namespace.is_none())) - .collect::>(); - let deferred_mcp_tools_for_search = if config.namespace_tools { - params.deferred_mcp_tools - } else { - None - }; - - if config.search_tool - && (deferred_mcp_tools_for_search.is_some() || !deferred_dynamic_tools.is_empty()) - { - let mut search_source_infos = deferred_mcp_tools_for_search - .map(|deferred_mcp_tools| { - 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, - } - })) - }) - .unwrap_or_default(); - - if !deferred_dynamic_tools.is_empty() { - search_source_infos.push(ToolSearchSourceInfo { - name: "Dynamic tools".to_string(), - description: Some("Tools provided by the current Codex thread.".to_string()), - }); - } - - plan.push_spec( - 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); - - if let Some(deferred_mcp_tools) = deferred_mcp_tools_for_search { - for tool in deferred_mcp_tools { - plan.register_handler(tool.name.clone(), ToolHandlerKind::Mcp); - } - } - } - if config.tool_suggest && let Some(discoverable_tools) = params.discoverable_tools.filter(|tools| !tools.is_empty()) @@ -506,8 +460,96 @@ pub fn build_tool_registry_plan( } } + let deferred_dynamic_tools = params + .dynamic_tools + .iter() + .filter(|tool| tool.defer_loading && (config.namespace_tools || tool.namespace.is_none())) + .collect::>(); + let mut claimed_non_mcp_top_level_names = + claimed_non_mcp_top_level_names(&plan, params.dynamic_tools); + if config.search_tool + && (params.deferred_mcp_tools.is_some() || !deferred_dynamic_tools.is_empty()) + { + claimed_non_mcp_top_level_names.insert(TOOL_SEARCH_TOOL_NAME.to_string()); + } + let mut skipped_mcp_namespaces = HashSet::new(); + let mut should_expose_mcp_namespace = |namespace: &str| { + if reserved_mcp_namespace(namespace) || claimed_non_mcp_top_level_names.contains(namespace) + { + if skipped_mcp_namespaces.insert(namespace.to_string()) { + tracing::warn!( + "skipping MCP namespace `{namespace}` because that top-level tool name is reserved or already claimed" + ); + } + return false; + } + true + }; + + let deferred_mcp_tools_for_search = if config.namespace_tools { + params.deferred_mcp_tools.map(|tools| { + tools + .iter() + .filter(|tool| { + tool.name + .namespace + .as_deref() + .is_some_and(&mut should_expose_mcp_namespace) + }) + .collect::>() + }) + } else { + None + }; + + if config.search_tool + && (deferred_mcp_tools_for_search.is_some() || !deferred_dynamic_tools.is_empty()) + { + let mut search_source_infos = deferred_mcp_tools_for_search + .as_ref() + .map(|deferred_mcp_tools| { + 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, + } + })) + }) + .unwrap_or_default(); + + if !deferred_dynamic_tools.is_empty() { + search_source_infos.push(ToolSearchSourceInfo { + name: "Dynamic tools".to_string(), + description: Some("Tools provided by the current Codex thread.".to_string()), + }); + } + + plan.push_spec( + 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); + + if let Some(deferred_mcp_tools) = deferred_mcp_tools_for_search.as_ref() { + for tool in deferred_mcp_tools { + plan.register_handler(tool.name.clone(), ToolHandlerKind::Mcp); + } + } + } + if let Some(mcp_tools) = params.mcp_tools { - let mut entries = mcp_tools.to_vec(); + let mut entries = mcp_tools + .iter() + .filter(|tool| { + tool.name + .namespace + .as_deref() + .is_some_and(&mut should_expose_mcp_namespace) + }) + .cloned() + .collect::>(); entries.sort_by_key(|tool| tool.name.display()); let mut namespace_entries = BTreeMap::new(); @@ -615,6 +657,25 @@ fn compare_code_mode_tools( .then_with(|| left.name.cmp(&right.name)) } +fn claimed_non_mcp_top_level_names( + plan: &ToolRegistryPlan, + dynamic_tools: &[codex_protocol::dynamic_tools::DynamicToolSpec], +) -> HashSet { + plan.specs + .iter() + .map(|tool| tool.name().to_string()) + .chain( + dynamic_tools + .iter() + .map(|tool| tool.namespace.clone().unwrap_or_else(|| tool.name.clone())), + ) + .collect() +} + +fn reserved_mcp_namespace(namespace: &str) -> bool { + matches!(namespace, "functions" | "tools" | "web") +} + fn code_mode_namespace_name<'a>( tool: &codex_code_mode::ToolDefinition, namespace_descriptions: &'a BTreeMap, diff --git a/codex-rs/tools/src/tool_registry_plan_tests.rs b/codex-rs/tools/src/tool_registry_plan_tests.rs index 9564c3eb8664..200aca239506 100644 --- a/codex-rs/tools/src/tool_registry_plan_tests.rs +++ b/codex-rs/tools/src/tool_registry_plan_tests.rs @@ -1205,16 +1205,16 @@ fn namespace_specs_are_hidden_when_namespace_tools_are_disabled() { let (tools, handlers) = build_specs( &tools_config, Some(HashMap::from([( - ToolName::namespaced("mcp__sample__", "echo"), + ToolName::namespaced("sample", "echo"), mcp_tool("echo", "Echo", serde_json::json!({"type": "object"})), )])), /*deferred_mcp_tools*/ None, &[], ); - assert_lacks_tool_name(&tools, "mcp__sample__"); + assert_lacks_tool_name(&tools, "sample"); assert!(handlers.contains(&ToolHandlerSpec { - name: ToolName::namespaced("mcp__sample__", "echo"), + name: ToolName::namespaced("sample", "echo"), kind: ToolHandlerKind::Mcp, })); } @@ -1374,7 +1374,7 @@ fn search_tool_description_lists_each_mcp_source_once() { &tools_config, Some(HashMap::from([ ( - ToolName::namespaced("mcp__codex_apps__calendar", "_create_event"), + ToolName::namespaced("codex_apps__calendar", "create_event"), mcp_tool( "calendar_create_event", "Create calendar event", @@ -1382,37 +1382,34 @@ fn search_tool_description_lists_each_mcp_source_once() { ), ), ( - ToolName::namespaced("mcp__rmcp__", "echo"), + ToolName::namespaced("rmcp", "echo"), mcp_tool("echo", "Echo", serde_json::json!({"type": "object"})), ), ])), Some(vec![ deferred_mcp_tool( - "_create_event", - "mcp__codex_apps__calendar", + "create_event", + "codex_apps__calendar", CODEX_APPS_MCP_SERVER_NAME, Some("Calendar"), Some("Plan events and manage your calendar."), ), deferred_mcp_tool( - "_list_events", - "mcp__codex_apps__calendar", + "list_events", + "codex_apps__calendar", CODEX_APPS_MCP_SERVER_NAME, Some("Calendar"), Some("Plan events and manage your calendar."), ), deferred_mcp_tool( - "_search_threads", - "mcp__codex_apps__gmail", + "search_threads", + "codex_apps__gmail", CODEX_APPS_MCP_SERVER_NAME, Some("Gmail"), Some("Find and summarize email threads."), ), deferred_mcp_tool( - "echo", - "mcp__rmcp__", - "rmcp", - /*connector_name*/ None, + "echo", "rmcp", "rmcp", /*connector_name*/ None, /*connector_description*/ None, ), ]), @@ -1433,14 +1430,14 @@ fn search_tool_description_lists_each_mcp_source_once() { 1 ); assert!(description.contains("- rmcp")); - assert!(!description.contains("mcp__rmcp__echo")); + assert!(!description.contains("rmcp__echo")); assert!(handlers.contains(&ToolHandlerSpec { - name: ToolName::namespaced("mcp__codex_apps__calendar", "_create_event"), + name: ToolName::namespaced("codex_apps__calendar", "create_event"), kind: ToolHandlerKind::Mcp, })); assert!(handlers.contains(&ToolHandlerSpec { - name: ToolName::namespaced("mcp__rmcp__", "echo"), + name: ToolName::namespaced("rmcp", "echo"), kind: ToolHandlerKind::Mcp, })); } @@ -1449,8 +1446,8 @@ fn search_tool_description_lists_each_mcp_source_once() { fn search_tool_requires_model_capability_and_enabled_feature() { let model_info = search_capable_model_info(); let deferred_mcp_tools = Some(vec![deferred_mcp_tool( - "_create_event", - "mcp__codex_apps__calendar", + "create_event", + "codex_apps__calendar", CODEX_APPS_MCP_SERVER_NAME, Some("Calendar"), /*connector_description*/ None, @@ -1540,8 +1537,8 @@ fn search_tool_is_hidden_when_only_deferred_namespace_tools_are_available() { &tools_config, /*mcp_tools*/ None, Some(vec![deferred_mcp_tool( - "_create_event", - "mcp__codex_apps__calendar", + "create_event", + "codex_apps__calendar", CODEX_APPS_MCP_SERVER_NAME, Some("Calendar"), Some("Plan events and manage your calendar."), @@ -1691,6 +1688,53 @@ fn search_tool_keeps_plain_deferred_dynamic_tools_when_namespace_tools_are_disab })); } +#[test] +fn mcp_namespace_colliding_with_dynamic_namespace_is_skipped() { + let model_info = model_info(); + let features = Features::with_defaults(); + let available_models = Vec::new(); + let tools_config = ToolsConfig::new(&ToolsConfigParams { + 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, + permission_profile: &PermissionProfile::Disabled, + windows_sandbox_level: WindowsSandboxLevel::Disabled, + }); + let dynamic_tools = vec![DynamicToolSpec { + namespace: Some("shared".to_string()), + name: "automation_update".to_string(), + description: "Create or update automations.".to_string(), + input_schema: json!({"type": "object", "properties": {}}), + defer_loading: false, + }]; + + let (tools, handlers) = build_specs( + &tools_config, + Some(HashMap::from([( + ToolName::namespaced("shared", "echo"), + mcp_tool("echo", "Echo", json!({"type": "object"})), + )])), + /*deferred_mcp_tools*/ None, + &dynamic_tools, + ); + + assert_eq!( + namespace_function_names(&tools, "shared"), + vec!["automation_update".to_string()] + ); + assert!(!handlers.contains(&ToolHandlerSpec { + name: ToolName::namespaced("shared", "echo"), + kind: ToolHandlerKind::Mcp, + })); + assert!(handlers.contains(&ToolHandlerSpec { + name: ToolName::namespaced("shared", "automation_update"), + kind: ToolHandlerKind::DynamicTool, + })); +} + #[test] fn tool_suggest_is_not_registered_without_feature_flag() { let model_info = search_capable_model_info(); @@ -1916,7 +1960,7 @@ fn code_mode_augments_mcp_tool_descriptions_with_namespaced_sample() { let (tools, _) = build_specs( &tools_config, Some(HashMap::from([( - ToolName::namespaced("mcp__sample__", "echo"), + ToolName::namespaced("sample", "echo"), mcp_tool( "echo", "Echo text", @@ -1935,7 +1979,7 @@ fn code_mode_augments_mcp_tool_descriptions_with_namespaced_sample() { ); let ResponsesApiTool { description, .. } = - find_namespace_function_tool(&tools, "mcp__sample__", "echo"); + find_namespace_function_tool(&tools, "sample", "echo"); assert_eq!( description, @@ -1943,7 +1987,7 @@ fn code_mode_augments_mcp_tool_descriptions_with_namespaced_sample() { exec tool declaration: ```ts -declare const tools: { mcp__sample__echo(args: { message: string; }): Promise; }; +declare const tools: { sample_echo(args: { message: string; }): Promise; }; ```"# ); } @@ -1969,7 +2013,7 @@ fn code_mode_preserves_nullable_and_literal_mcp_input_shapes() { let (tools, _) = build_specs( &tools_config, Some(HashMap::from([( - ToolName::namespaced("mcp__sample__", "fn"), + ToolName::namespaced("sample", "fn"), mcp_tool( "fn", "Sample fn", @@ -2020,13 +2064,12 @@ fn code_mode_preserves_nullable_and_literal_mcp_input_shapes() { &[], ); - let ResponsesApiTool { description, .. } = - find_namespace_function_tool(&tools, "mcp__sample__", "fn"); + let ResponsesApiTool { description, .. } = find_namespace_function_tool(&tools, "sample", "fn"); assert!(description.contains( r#"exec tool declaration: ```ts -declare const tools: { mcp__sample__fn(args: { open?: Array<{ lineno?: number | null; ref_id: string; }> | null; response_length?: "short" | "medium" | "long"; tagged_list?: Array<{ kind: "tagged"; scope: "one" | "two"; variant: "alpha" | "beta"; }> | null; }): Promise; }; +declare const tools: { sample_fn(args: { open?: Array<{ lineno?: number | null; ref_id: string; }> | null; response_length?: "short" | "medium" | "long"; tagged_list?: Array<{ kind: "tagged"; scope: "one" | "two"; variant: "alpha" | "beta"; }> | null; }): Promise; }; ```"# )); } @@ -2311,7 +2354,7 @@ fn code_mode_augments_mcp_tool_descriptions_with_structured_output_sample() { let (tools, _) = build_specs( &tools_config, Some(HashMap::from([( - ToolName::namespaced("mcp__sample__", "echo"), + ToolName::namespaced("sample", "echo"), tool, )])), /*deferred_mcp_tools*/ None, @@ -2319,7 +2362,7 @@ fn code_mode_augments_mcp_tool_descriptions_with_structured_output_sample() { ); let ResponsesApiTool { description, .. } = - find_namespace_function_tool(&tools, "mcp__sample__", "echo"); + find_namespace_function_tool(&tools, "sample", "echo"); assert_eq!( description, @@ -2327,7 +2370,7 @@ fn code_mode_augments_mcp_tool_descriptions_with_structured_output_sample() { exec tool declaration: ```ts -declare const tools: { mcp__sample__echo(args: { message: string; }): Promise>; }; +declare const tools: { sample_echo(args: { message: string; }): Promise>; }; ```"# ); } diff --git a/codex-rs/tools/src/tool_spec_tests.rs b/codex-rs/tools/src/tool_spec_tests.rs index 0b986256265e..919d7e813aa3 100644 --- a/codex-rs/tools/src/tool_spec_tests.rs +++ b/codex-rs/tools/src/tool_spec_tests.rs @@ -38,12 +38,12 @@ fn tool_spec_name_covers_all_variants() { ); assert_eq!( ToolSpec::Namespace(ResponsesApiNamespace { - name: "mcp__demo__".to_string(), + name: "demo".to_string(), description: "Demo tools".to_string(), tools: Vec::new(), }) .name(), - "mcp__demo__" + "demo" ); assert_eq!( ToolSpec::ToolSearch { @@ -178,7 +178,7 @@ fn create_tools_json_for_responses_api_includes_top_level_name() { fn namespace_tool_spec_serializes_expected_wire_shape() { assert_eq!( serde_json::to_value(ToolSpec::Namespace(ResponsesApiNamespace { - name: "mcp__demo__".to_string(), + name: "demo".to_string(), description: "Demo tools".to_string(), tools: vec![ResponsesApiNamespaceTool::Function(ResponsesApiTool { name: "lookup_order".to_string(), @@ -199,7 +199,7 @@ fn namespace_tool_spec_serializes_expected_wire_shape() { .expect("serialize namespace tool"), json!({ "type": "namespace", - "name": "mcp__demo__", + "name": "demo", "description": "Demo tools", "tools": [ {