From 706490ab1b3f79ba807581b35aeeff6222e04cac Mon Sep 17 00:00:00 2001 From: Ahmed Ibrahim Date: Sat, 25 Apr 2026 06:36:07 -0700 Subject: [PATCH] [codex] Prune unused codex-mcp API and duplicate helpers (#19524) ## Why `codex-mcp` currently exposes more API than the rest of the workspace uses. Some of that surface is simply visibility that can be tightened, and some of it is public helper code that remains compiler-valid because it is exported even though no workspace caller uses it. That distinction matters: Rust does not warn on exported API just because the current workspace does not call it. This PR intentionally treats those exported-but-workspace-unreferenced paths as stale `codex-mcp` surface. The main example is MCP skill dependency collection, where the active implementation now lives in `codex-rs/core/src/mcp_skill_dependencies.rs`; keeping the older `codex-mcp` copy makes it unclear which implementation owns skill MCP installation. ## What Changed - Pruned unused `codex-mcp` re-exports from `codex-mcp/src/lib.rs`. - Removed non-runtime helper methods from `McpConnectionManager` so it stays focused on live MCP clients. - Made `ToolPluginProvenance` lookup methods crate-private. - Removed workspace-unreferenced snapshot wrapper APIs and qualified-tool grouping helpers. - Deleted the duplicate `codex-mcp` skill dependency module and tests now that skill MCP dependency handling is owned by `core`. ## Verification - `cargo check -p codex-mcp` --- codex-rs/codex-mcp/Cargo.toml | 1 - codex-rs/codex-mcp/src/lib.rs | 10 - codex-rs/codex-mcp/src/mcp/mod.rs | 125 +------------ codex-rs/codex-mcp/src/mcp/mod_tests.rs | 51 ------ .../codex-mcp/src/mcp/skill_dependencies.rs | 172 ------------------ .../src/mcp/skill_dependencies_tests.rs | 115 ------------ .../codex-mcp/src/mcp_connection_manager.rs | 22 +-- 7 files changed, 3 insertions(+), 493 deletions(-) delete mode 100644 codex-rs/codex-mcp/src/mcp/skill_dependencies.rs delete mode 100644 codex-rs/codex-mcp/src/mcp/skill_dependencies_tests.rs diff --git a/codex-rs/codex-mcp/Cargo.toml b/codex-rs/codex-mcp/Cargo.toml index a9aacb192..c3061adca 100644 --- a/codex-rs/codex-mcp/Cargo.toml +++ b/codex-rs/codex-mcp/Cargo.toml @@ -38,7 +38,6 @@ tracing = { workspace = true } url = { workspace = true } [dev-dependencies] -codex-utils-absolute-path = { workspace = true } pretty_assertions = { workspace = true } rmcp = { workspace = true, default-features = false, features = ["base64", "macros", "schemars", "server"] } tempfile = { workspace = true } diff --git a/codex-rs/codex-mcp/src/lib.rs b/codex-rs/codex-mcp/src/lib.rs index 8b77086e5..70f0b6140 100644 --- a/codex-rs/codex-mcp/src/lib.rs +++ b/codex-rs/codex-mcp/src/lib.rs @@ -5,7 +5,6 @@ pub(crate) mod mcp_tool_names; pub use mcp::CODEX_APPS_MCP_SERVER_NAME; pub use mcp::McpAuthStatusEntry; pub use mcp::McpConfig; -pub use mcp::McpManager; pub use mcp::McpOAuthLoginConfig; pub use mcp::McpOAuthLoginSupport; pub use mcp::McpOAuthScopesSource; @@ -13,30 +12,21 @@ pub use mcp::McpServerStatusSnapshot; pub use mcp::McpSnapshotDetail; pub use mcp::ResolvedMcpOAuthScopes; pub use mcp::ToolPluginProvenance; -pub use mcp::canonical_mcp_server_key; -pub use mcp::collect_mcp_server_status_snapshot; pub use mcp::collect_mcp_server_status_snapshot_with_detail; -pub use mcp::collect_mcp_snapshot; pub use mcp::collect_mcp_snapshot_from_manager; -pub use mcp::collect_mcp_snapshot_from_manager_with_detail; -pub use mcp::collect_mcp_snapshot_with_detail; -pub use mcp::collect_missing_mcp_dependencies; pub use mcp::compute_auth_statuses; pub use mcp::configured_mcp_servers; pub use mcp::discover_supported_scopes; pub use mcp::effective_mcp_servers; -pub use mcp::group_tools_by_server; pub use mcp::mcp_permission_prompt_is_auto_approved; pub use mcp::oauth_login_support; pub use mcp::qualified_mcp_tool_name_prefix; pub use mcp::read_mcp_resource; pub use mcp::resolve_oauth_scopes; pub use mcp::should_retry_without_scopes; -pub use mcp::split_qualified_tool_name; pub use mcp::tool_plugin_provenance; pub use mcp::with_codex_apps_mcp; pub use mcp_connection_manager::CodexAppsToolsCacheKey; -pub use mcp_connection_manager::DEFAULT_STARTUP_TIMEOUT; pub use mcp_connection_manager::MCP_SANDBOX_STATE_META_CAPABILITY; pub use mcp_connection_manager::McpConnectionManager; pub use mcp_connection_manager::McpRuntimeEnvironment; diff --git a/codex-rs/codex-mcp/src/mcp/mod.rs b/codex-rs/codex-mcp/src/mcp/mod.rs index 1061a6a54..bf98aeb0f 100644 --- a/codex-rs/codex-mcp/src/mcp/mod.rs +++ b/codex-rs/codex-mcp/src/mcp/mod.rs @@ -1,5 +1,4 @@ pub(crate) mod auth; -mod skill_dependencies; pub use auth::McpAuthStatusEntry; pub use auth::McpOAuthLoginConfig; pub use auth::McpOAuthLoginSupport; @@ -10,8 +9,6 @@ pub use auth::discover_supported_scopes; pub use auth::oauth_login_support; pub use auth::resolve_oauth_scopes; pub use auth::should_retry_without_scopes; -pub use skill_dependencies::canonical_mcp_server_key; -pub use skill_dependencies::collect_missing_mcp_dependencies; use std::collections::HashMap; use std::env; @@ -39,7 +36,6 @@ use serde_json::Value; use crate::mcp_connection_manager::McpConnectionManager; use crate::mcp_connection_manager::McpRuntimeEnvironment; use crate::mcp_connection_manager::codex_apps_tools_cache_key; -pub type McpManager = McpConnectionManager; const MCP_TOOL_NAME_PREFIX: &str = "mcp"; const MCP_TOOL_NAME_DELIMITER: &str = "__"; @@ -105,7 +101,7 @@ pub fn mcp_permission_prompt_is_auto_approved( /// approval/sandbox policy, locate OAuth state, and merge plugin-provided MCP /// servers. Request-scoped or auth-scoped state should not be stored here; /// thread those values explicitly into runtime entry points such as -/// [`with_codex_apps_mcp`] and [`collect_mcp_snapshot`] so config objects do +/// [`with_codex_apps_mcp`] and snapshot collection helpers so config objects do /// not go stale when auth changes. #[derive(Debug, Clone)] pub struct McpConfig { @@ -335,78 +331,6 @@ pub async fn read_mcp_resource( result } -pub async fn collect_mcp_snapshot( - config: &McpConfig, - auth: Option<&CodexAuth>, - submit_id: String, - runtime_environment: McpRuntimeEnvironment, -) -> McpListToolsResponseEvent { - collect_mcp_snapshot_with_detail( - config, - auth, - submit_id, - runtime_environment, - McpSnapshotDetail::Full, - ) - .await -} - -pub async fn collect_mcp_snapshot_with_detail( - config: &McpConfig, - auth: Option<&CodexAuth>, - submit_id: String, - runtime_environment: McpRuntimeEnvironment, - detail: McpSnapshotDetail, -) -> McpListToolsResponseEvent { - let mcp_servers = effective_mcp_servers(config, auth); - let tool_plugin_provenance = tool_plugin_provenance(config); - if mcp_servers.is_empty() { - return McpListToolsResponseEvent { - tools: HashMap::new(), - resources: HashMap::new(), - resource_templates: HashMap::new(), - auth_statuses: HashMap::new(), - }; - } - - let auth_status_entries = compute_auth_statuses( - mcp_servers.iter(), - config.mcp_oauth_credentials_store_mode, - auth, - ) - .await; - - let (tx_event, rx_event) = unbounded(); - drop(rx_event); - - let (mcp_connection_manager, cancel_token) = McpConnectionManager::new( - &mcp_servers, - config.mcp_oauth_credentials_store_mode, - auth_status_entries.clone(), - &config.approval_policy, - submit_id, - tx_event, - SandboxPolicy::new_read_only_policy(), - runtime_environment, - config.codex_home.clone(), - codex_apps_tools_cache_key(auth), - tool_plugin_provenance, - auth, - ) - .await; - - let snapshot = collect_mcp_snapshot_from_manager_with_detail( - &mcp_connection_manager, - auth_status_entries, - detail, - ) - .await; - - cancel_token.cancel(); - - snapshot -} - #[derive(Debug, Clone)] pub struct McpServerStatusSnapshot { pub tools_by_server: HashMap>, @@ -415,22 +339,6 @@ pub struct McpServerStatusSnapshot { pub auth_statuses: HashMap, } -pub async fn collect_mcp_server_status_snapshot( - config: &McpConfig, - auth: Option<&CodexAuth>, - submit_id: String, - runtime_environment: McpRuntimeEnvironment, -) -> McpServerStatusSnapshot { - collect_mcp_server_status_snapshot_with_detail( - config, - auth, - submit_id, - runtime_environment, - McpSnapshotDetail::Full, - ) - .await -} - pub async fn collect_mcp_server_status_snapshot_with_detail( config: &McpConfig, auth: Option<&CodexAuth>, @@ -487,35 +395,6 @@ pub async fn collect_mcp_server_status_snapshot_with_detail( snapshot } -pub fn split_qualified_tool_name(qualified_name: &str) -> Option<(String, String)> { - let mut parts = qualified_name.split(MCP_TOOL_NAME_DELIMITER); - let prefix = parts.next()?; - if prefix != MCP_TOOL_NAME_PREFIX { - return None; - } - let server_name = parts.next()?; - let tool_name: String = parts.collect::>().join(MCP_TOOL_NAME_DELIMITER); - if tool_name.is_empty() { - return None; - } - Some((server_name.to_string(), tool_name)) -} - -pub fn group_tools_by_server( - tools: &HashMap, -) -> HashMap> { - let mut grouped = HashMap::new(); - for (qualified_name, tool) in tools { - if let Some((server_name, tool_name)) = split_qualified_tool_name(qualified_name) { - grouped - .entry(server_name) - .or_insert_with(HashMap::new) - .insert(tool_name, tool.clone()); - } - } - grouped -} - fn protocol_tool_from_rmcp_tool(name: &str, tool: &rmcp::model::Tool) -> Option { match serde_json::to_value(tool) { Ok(value) => match Tool::from_mcp_value(value) { @@ -676,7 +555,7 @@ pub async fn collect_mcp_snapshot_from_manager( .await } -pub async fn collect_mcp_snapshot_from_manager_with_detail( +async fn collect_mcp_snapshot_from_manager_with_detail( mcp_connection_manager: &McpConnectionManager, auth_status_entries: HashMap, detail: McpSnapshotDetail, diff --git a/codex-rs/codex-mcp/src/mcp/mod_tests.rs b/codex-rs/codex-mcp/src/mcp/mod_tests.rs index 8db52a9d8..01a977077 100644 --- a/codex-rs/codex-mcp/src/mcp/mod_tests.rs +++ b/codex-rs/codex-mcp/src/mcp/mod_tests.rs @@ -25,27 +25,6 @@ fn test_mcp_config(codex_home: PathBuf) -> McpConfig { } } -fn make_tool(name: &str) -> Tool { - Tool { - name: name.to_string(), - title: None, - description: None, - input_schema: serde_json::json!({"type": "object", "properties": {}}), - output_schema: None, - annotations: None, - icons: None, - meta: None, - } -} - -#[test] -fn split_qualified_tool_name_returns_server_and_tool() { - assert_eq!( - split_qualified_tool_name("mcp__alpha__do_thing"), - Some(("alpha".to_string(), "do_thing".to_string())) - ); -} - #[test] fn qualified_mcp_tool_name_prefix_sanitizes_server_names_without_lowercasing() { assert_eq!( @@ -54,36 +33,6 @@ fn qualified_mcp_tool_name_prefix_sanitizes_server_names_without_lowercasing() { ); } -#[test] -fn split_qualified_tool_name_rejects_invalid_names() { - assert_eq!(split_qualified_tool_name("other__alpha__do_thing"), None); - assert_eq!(split_qualified_tool_name("mcp__alpha__"), None); -} - -#[test] -fn group_tools_by_server_strips_prefix_and_groups() { - let mut tools = HashMap::new(); - tools.insert("mcp__alpha__do_thing".to_string(), make_tool("do_thing")); - tools.insert( - "mcp__alpha__nested__op".to_string(), - make_tool("nested__op"), - ); - tools.insert("mcp__beta__do_other".to_string(), make_tool("do_other")); - - let mut expected_alpha = HashMap::new(); - expected_alpha.insert("do_thing".to_string(), make_tool("do_thing")); - expected_alpha.insert("nested__op".to_string(), make_tool("nested__op")); - - let mut expected_beta = HashMap::new(); - expected_beta.insert("do_other".to_string(), make_tool("do_other")); - - let mut expected = HashMap::new(); - expected.insert("alpha".to_string(), expected_alpha); - expected.insert("beta".to_string(), expected_beta); - - assert_eq!(group_tools_by_server(&tools), expected); -} - #[test] fn tool_plugin_provenance_collects_app_and_mcp_sources() { let provenance = ToolPluginProvenance::from_capability_summaries(&[ diff --git a/codex-rs/codex-mcp/src/mcp/skill_dependencies.rs b/codex-rs/codex-mcp/src/mcp/skill_dependencies.rs deleted file mode 100644 index f785fe4bd..000000000 --- a/codex-rs/codex-mcp/src/mcp/skill_dependencies.rs +++ /dev/null @@ -1,172 +0,0 @@ -use std::collections::HashMap; -use std::collections::HashSet; - -use codex_config::McpServerConfig; -use codex_config::McpServerTransportConfig; -use codex_protocol::protocol::SkillMetadata; -use codex_protocol::protocol::SkillToolDependency; -use tracing::warn; - -pub fn collect_missing_mcp_dependencies( - mentioned_skills: &[SkillMetadata], - installed: &HashMap, -) -> HashMap { - let mut missing = HashMap::new(); - let installed_keys: HashSet = installed - .iter() - .map(|(name, config)| canonical_mcp_server_key(name, config)) - .collect(); - let mut seen_canonical_keys = HashSet::new(); - - for skill in mentioned_skills { - let Some(dependencies) = skill.dependencies.as_ref() else { - continue; - }; - - for tool in &dependencies.tools { - if !tool.r#type.eq_ignore_ascii_case("mcp") { - continue; - } - let dependency_key = match canonical_mcp_dependency_key(tool) { - Ok(key) => key, - Err(err) => { - let dependency = tool.value.as_str(); - let skill_name = skill.name.as_str(); - warn!( - "unable to auto-install MCP dependency {dependency} for skill {skill_name}: {err}", - ); - continue; - } - }; - if installed_keys.contains(&dependency_key) - || seen_canonical_keys.contains(&dependency_key) - { - continue; - } - - let config = match mcp_dependency_to_server_config(tool) { - Ok(config) => config, - Err(err) => { - let dependency = dependency_key.as_str(); - let skill_name = skill.name.as_str(); - warn!( - "unable to auto-install MCP dependency {dependency} for skill {skill_name}: {err}", - ); - continue; - } - }; - - missing.insert(tool.value.clone(), config); - seen_canonical_keys.insert(dependency_key); - } - } - - missing -} - -fn canonical_mcp_key(transport: &str, identifier: &str, fallback: &str) -> String { - let identifier = identifier.trim(); - if identifier.is_empty() { - fallback.to_string() - } else { - format!("mcp__{transport}__{identifier}") - } -} - -pub fn canonical_mcp_server_key(name: &str, config: &McpServerConfig) -> String { - match &config.transport { - McpServerTransportConfig::Stdio { command, .. } => { - canonical_mcp_key("stdio", command, name) - } - McpServerTransportConfig::StreamableHttp { url, .. } => { - canonical_mcp_key("streamable_http", url, name) - } - } -} - -fn canonical_mcp_dependency_key(dependency: &SkillToolDependency) -> Result { - let transport = dependency.transport.as_deref().unwrap_or("streamable_http"); - if transport.eq_ignore_ascii_case("streamable_http") { - let url = dependency - .url - .as_ref() - .ok_or_else(|| "missing url for streamable_http dependency".to_string())?; - return Ok(canonical_mcp_key("streamable_http", url, &dependency.value)); - } - if transport.eq_ignore_ascii_case("stdio") { - let command = dependency - .command - .as_ref() - .ok_or_else(|| "missing command for stdio dependency".to_string())?; - return Ok(canonical_mcp_key("stdio", command, &dependency.value)); - } - Err(format!("unsupported transport {transport}")) -} - -fn mcp_dependency_to_server_config( - dependency: &SkillToolDependency, -) -> Result { - let transport = dependency.transport.as_deref().unwrap_or("streamable_http"); - if transport.eq_ignore_ascii_case("streamable_http") { - let url = dependency - .url - .as_ref() - .ok_or_else(|| "missing url for streamable_http dependency".to_string())?; - return Ok(McpServerConfig { - transport: McpServerTransportConfig::StreamableHttp { - url: url.clone(), - bearer_token_env_var: None, - http_headers: None, - env_http_headers: None, - }, - experimental_environment: None, - enabled: true, - required: false, - supports_parallel_tool_calls: false, - disabled_reason: None, - startup_timeout_sec: None, - tool_timeout_sec: None, - default_tools_approval_mode: None, - enabled_tools: None, - disabled_tools: None, - scopes: None, - oauth_resource: None, - tools: HashMap::new(), - }); - } - - if transport.eq_ignore_ascii_case("stdio") { - let command = dependency - .command - .as_ref() - .ok_or_else(|| "missing command for stdio dependency".to_string())?; - return Ok(McpServerConfig { - transport: McpServerTransportConfig::Stdio { - command: command.clone(), - args: Vec::new(), - env: None, - env_vars: Vec::new(), - cwd: None, - }, - experimental_environment: None, - enabled: true, - required: false, - supports_parallel_tool_calls: false, - disabled_reason: None, - startup_timeout_sec: None, - tool_timeout_sec: None, - default_tools_approval_mode: None, - enabled_tools: None, - disabled_tools: None, - scopes: None, - oauth_resource: None, - tools: HashMap::new(), - }); - } - - Err(format!("unsupported transport {transport}")) -} - -#[cfg(test)] -#[path = "skill_dependencies_tests.rs"] -mod tests; diff --git a/codex-rs/codex-mcp/src/mcp/skill_dependencies_tests.rs b/codex-rs/codex-mcp/src/mcp/skill_dependencies_tests.rs deleted file mode 100644 index 2d8390d15..000000000 --- a/codex-rs/codex-mcp/src/mcp/skill_dependencies_tests.rs +++ /dev/null @@ -1,115 +0,0 @@ -use super::*; -use codex_protocol::protocol::SkillDependencies; -use codex_protocol::protocol::SkillMetadata; -use codex_protocol::protocol::SkillScope; -use codex_utils_absolute_path::test_support::PathBufExt as _; -use codex_utils_absolute_path::test_support::test_path_buf; -use pretty_assertions::assert_eq; - -fn skill_with_tools(tools: Vec) -> SkillMetadata { - SkillMetadata { - name: "skill".to_string(), - description: "skill".to_string(), - short_description: None, - interface: None, - dependencies: Some(SkillDependencies { tools }), - path: test_path_buf("/tmp/skill").abs(), - scope: SkillScope::User, - enabled: true, - } -} - -#[test] -fn collect_missing_respects_canonical_installed_key() { - let url = "https://example.com/mcp".to_string(); - let skills = vec![skill_with_tools(vec![SkillToolDependency { - r#type: "mcp".to_string(), - value: "github".to_string(), - description: None, - transport: Some("streamable_http".to_string()), - command: None, - url: Some(url.clone()), - }])]; - let installed = HashMap::from([( - "alias".to_string(), - McpServerConfig { - transport: McpServerTransportConfig::StreamableHttp { - url, - bearer_token_env_var: None, - http_headers: None, - env_http_headers: None, - }, - experimental_environment: None, - enabled: true, - required: false, - supports_parallel_tool_calls: false, - disabled_reason: None, - startup_timeout_sec: None, - tool_timeout_sec: None, - default_tools_approval_mode: None, - enabled_tools: None, - disabled_tools: None, - scopes: None, - oauth_resource: None, - tools: HashMap::new(), - }, - )]); - - assert_eq!( - collect_missing_mcp_dependencies(&skills, &installed), - HashMap::new() - ); -} - -#[test] -fn collect_missing_dedupes_by_canonical_key_but_preserves_original_name() { - let url = "https://example.com/one".to_string(); - let skills = vec![skill_with_tools(vec![ - SkillToolDependency { - r#type: "mcp".to_string(), - value: "alias-one".to_string(), - description: None, - transport: Some("streamable_http".to_string()), - command: None, - url: Some(url.clone()), - }, - SkillToolDependency { - r#type: "mcp".to_string(), - value: "alias-two".to_string(), - description: None, - transport: Some("streamable_http".to_string()), - command: None, - url: Some(url.clone()), - }, - ])]; - - let expected = HashMap::from([( - "alias-one".to_string(), - McpServerConfig { - transport: McpServerTransportConfig::StreamableHttp { - url, - bearer_token_env_var: None, - http_headers: None, - env_http_headers: None, - }, - experimental_environment: None, - enabled: true, - required: false, - supports_parallel_tool_calls: false, - disabled_reason: None, - startup_timeout_sec: None, - tool_timeout_sec: None, - default_tools_approval_mode: None, - enabled_tools: None, - disabled_tools: None, - scopes: None, - oauth_resource: None, - tools: HashMap::new(), - }, - )]); - - assert_eq!( - collect_missing_mcp_dependencies(&skills, &HashMap::new()), - expected - ); -} diff --git a/codex-rs/codex-mcp/src/mcp_connection_manager.rs b/codex-rs/codex-mcp/src/mcp_connection_manager.rs index 4dd703c38..5a504b1c8 100644 --- a/codex-rs/codex-mcp/src/mcp_connection_manager.rs +++ b/codex-rs/codex-mcp/src/mcp_connection_manager.rs @@ -21,12 +21,8 @@ use std::time::Instant; use crate::McpAuthStatusEntry; use crate::mcp::CODEX_APPS_MCP_SERVER_NAME; -use crate::mcp::McpConfig; use crate::mcp::ToolPluginProvenance; -use crate::mcp::configured_mcp_servers; -use crate::mcp::effective_mcp_servers; use crate::mcp::mcp_permission_prompt_is_auto_approved; -use crate::mcp::tool_plugin_provenance; pub(crate) use crate::mcp_tool_names::qualify_tools; use anyhow::Context; use anyhow::Result; @@ -105,7 +101,7 @@ use codex_utils_plugins::mcp_connector::sanitize_name; const MCP_TOOL_NAME_DELIMITER: &str = "__"; /// Default timeout for initializing MCP server & initially listing tools. -pub const DEFAULT_STARTUP_TIMEOUT: Duration = Duration::from_secs(30); +const DEFAULT_STARTUP_TIMEOUT: Duration = Duration::from_secs(30); /// Default timeout for individual tool calls. const DEFAULT_TOOL_TIMEOUT: Duration = Duration::from_secs(120); @@ -689,22 +685,6 @@ impl McpRuntimeEnvironment { } impl McpConnectionManager { - pub fn configured_servers(&self, config: &McpConfig) -> HashMap { - configured_mcp_servers(config) - } - - pub fn effective_servers( - &self, - config: &McpConfig, - auth: Option<&CodexAuth>, - ) -> HashMap { - effective_mcp_servers(config, auth) - } - - pub fn tool_plugin_provenance(&self, config: &McpConfig) -> ToolPluginProvenance { - tool_plugin_provenance(config) - } - pub fn new_uninitialized( approval_policy: &Constrained, sandbox_policy: &Constrained,