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,