diff --git a/codex-rs/cli/src/doctor.rs b/codex-rs/cli/src/doctor.rs index ca25b3b1c..ef587cd1d 100644 --- a/codex-rs/cli/src/doctor.rs +++ b/codex-rs/cli/src/doctor.rs @@ -3457,6 +3457,26 @@ mod tests { })); } + #[tokio::test] + async fn mcp_check_does_not_probe_environment_stdio_on_the_host() { + let remote_server: McpServerConfig = toml::from_str( + r#" + command = "remote-only-command" + environment_id = "remote" + cwd = "C:\\plugins\\demo" + required = true + env_vars = [{ name = "REMOTE_ONLY_TOKEN", source = "remote" }] + "#, + ) + .expect("remote MCP config"); + let servers = HashMap::from([("remote".to_string(), remote_server)]); + + let check = mcp_check_from_servers(&servers).await; + + assert_eq!(check.status, CheckStatus::Ok); + assert_eq!(check.summary, "MCP configuration is locally consistent"); + } + #[test] fn provider_specific_auth_allows_non_openai_provider_without_env_key() { let check = provider_specific_auth_check( diff --git a/codex-rs/codex-mcp/src/lib.rs b/codex-rs/codex-mcp/src/lib.rs index b56a4f907..b4d397eac 100644 --- a/codex-rs/codex-mcp/src/lib.rs +++ b/codex-rs/codex-mcp/src/lib.rs @@ -46,7 +46,7 @@ pub use mcp::hosted_plugin_runtime_mcp_server_config; pub use mcp::tool_plugin_provenance; pub use plugin_config::PluginMcpConfigParseOutcome; pub use plugin_config::PluginMcpServerParseError; -pub use plugin_config::PluginMcpServerPlacement; +pub use plugin_config::parse_executor_plugin_mcp_config; pub use plugin_config::parse_plugin_mcp_config; pub use mcp::McpServerStatusSnapshot; diff --git a/codex-rs/codex-mcp/src/plugin_config.rs b/codex-rs/codex-mcp/src/plugin_config.rs index 7c53de3e4..968d6a64d 100644 --- a/codex-rs/codex-mcp/src/plugin_config.rs +++ b/codex-rs/codex-mcp/src/plugin_config.rs @@ -1,22 +1,24 @@ use codex_config::McpServerConfig; use codex_config::McpServerEnvVar; use codex_config::McpServerTransportConfig; +use codex_utils_path_uri::LegacyAppPathString; +use codex_utils_path_uri::PathUri; use serde::Deserialize; use serde_json::Map as JsonMap; use serde_json::Value as JsonValue; use std::collections::BTreeMap; -use std::path::Component; use std::path::Path; -use std::path::PathBuf; use tracing::warn; -/// Placement applied while normalizing MCP servers declared by a plugin. #[derive(Clone, Copy, Debug)] -pub enum PluginMcpServerPlacement<'a> { - /// Preserve declared placement, resolving a relative working directory below the plugin root. - Declared, - /// Bind stdio servers to one environment and default their working directory to the plugin root. - Environment { environment_id: &'a str }, +enum PluginMcpSource<'a> { + Host { + root: &'a Path, + }, + Environment { + root: &'a PathUri, + environment_id: &'a str, + }, } /// One plugin MCP server that could not be normalized into runtime configuration. @@ -62,13 +64,44 @@ impl PluginMcpFile { pub fn parse_plugin_mcp_config( plugin_root: &Path, contents: &str, - placement: PluginMcpServerPlacement<'_>, +) -> Result { + parse_plugin_mcp_config_from(contents, PluginMcpSource::Host { root: plugin_root }) +} + +/// Parses executor-owned plugin MCP config without interpreting the plugin root +/// as a path on the orchestrator host. +pub fn parse_executor_plugin_mcp_config( + plugin_root: &PathUri, + contents: &str, + environment_id: &str, +) -> Result { + parse_plugin_mcp_config_from( + contents, + PluginMcpSource::Environment { + root: plugin_root, + environment_id, + }, + ) +} + +impl PluginMcpSource<'_> { + fn display(self) -> String { + match self { + Self::Host { root } => root.display().to_string(), + Self::Environment { root, .. } => root.to_string(), + } + } +} + +fn parse_plugin_mcp_config_from( + contents: &str, + source: PluginMcpSource<'_>, ) -> Result { let parsed = serde_json::from_str::(contents)?; let mut outcome = PluginMcpConfigParseOutcome::default(); for (name, config_value) in parsed.into_mcp_servers() { - match normalize_plugin_mcp_server(plugin_root, config_value, placement) { + match normalize_plugin_mcp_server(config_value, source) { Ok(config) => { outcome.servers.insert(name, config); } @@ -82,12 +115,15 @@ pub fn parse_plugin_mcp_config( } fn normalize_plugin_mcp_server( - plugin_root: &Path, value: JsonValue, - placement: PluginMcpServerPlacement<'_>, + source: PluginMcpSource<'_>, ) -> Result { - let mut object = normalize_plugin_mcp_server_value(plugin_root, value, placement); - if let PluginMcpServerPlacement::Environment { environment_id } = placement { + let mut object = normalize_plugin_mcp_server_value(value, source); + if let PluginMcpSource::Environment { + root, + environment_id, + } = source + { object.insert( "environment_id".to_string(), JsonValue::String(environment_id.to_string()), @@ -96,15 +132,13 @@ fn normalize_plugin_mcp_server( match object.remove("cwd") { Some(JsonValue::String(cwd)) => object.insert( "cwd".to_string(), - JsonValue::String( - executor_plugin_cwd(plugin_root, &cwd)? - .to_string_lossy() - .into_owned(), - ), + JsonValue::String(environment_cwd(root, Some(&cwd))?.into_string()), ), Some(JsonValue::Null) | None => object.insert( "cwd".to_string(), - JsonValue::String(plugin_root.to_string_lossy().into_owned()), + JsonValue::String( + environment_cwd(root, /*configured_cwd*/ None)?.into_string(), + ), ), Some(value) => object.insert("cwd".to_string(), value), }; @@ -113,29 +147,28 @@ fn normalize_plugin_mcp_server( let mut config = serde_json::from_value::(JsonValue::Object(object)) .map_err(|err| err.to_string())?; - if matches!(placement, PluginMcpServerPlacement::Environment { .. }) { + if matches!(source, PluginMcpSource::Environment { .. }) { bind_environment_env_vars(&mut config)?; } Ok(config) } -fn executor_plugin_cwd(plugin_root: &Path, configured_cwd: &str) -> Result { - let cwd = Path::new(configured_cwd); - if cwd.is_absolute() { - return Ok(cwd.to_path_buf()); - } - if cwd.components().any(|component| { - matches!( - component, - Component::ParentDir | Component::RootDir | Component::Prefix(_) - ) - }) { +fn environment_cwd( + root: &PathUri, + configured_cwd: Option<&str>, +) -> Result { + let Some(configured_cwd) = configured_cwd else { + return Ok(root.clone().into()); + }; + let cwd = PathUri::parse(configured_cwd) + .or_else(|_| root.join(configured_cwd)) + .map_err(|err| format!("invalid cwd `{configured_cwd}`: {err}"))?; + if !cwd.starts_with(root) { return Err(format!( - "relative cwd `{configured_cwd}` must remain within plugin root `{}`", - plugin_root.display() + "cwd `{configured_cwd}` must remain within plugin root `{root}`" )); } - Ok(plugin_root.join(cwd)) + Ok(cwd.into()) } fn bind_environment_env_vars(config: &mut McpServerConfig) -> Result<(), String> { @@ -175,9 +208,8 @@ fn bind_environment_env_vars(config: &mut McpServerConfig) -> Result<(), String> } fn normalize_plugin_mcp_server_value( - plugin_root: &Path, value: JsonValue, - placement: PluginMcpServerPlacement<'_>, + source: PluginMcpSource<'_>, ) -> JsonMap { let mut object = match value { JsonValue::Object(object) => object, @@ -188,8 +220,9 @@ fn normalize_plugin_mcp_server_value( match transport_type.as_str() { "http" | "streamable_http" | "streamable-http" | "stdio" => {} other => { + let plugin_display = source.display(); warn!( - plugin = %plugin_root.display(), + plugin = %plugin_display, transport = other, "plugin MCP server uses an unknown transport type" ); @@ -199,8 +232,9 @@ fn normalize_plugin_mcp_server_value( if let Some(JsonValue::Object(mut oauth)) = object.remove("oauth") { if oauth.remove("callbackPort").is_some() { + let plugin_display = source.display(); warn!( - plugin = %plugin_root.display(), + plugin = %plugin_display, "plugin MCP server OAuth callbackPort is ignored; Codex uses global MCP OAuth callback settings" ); } @@ -214,13 +248,13 @@ fn normalize_plugin_mcp_server_value( } } - if matches!(placement, PluginMcpServerPlacement::Declared) + if let PluginMcpSource::Host { root } = source && let Some(JsonValue::String(cwd)) = object.get("cwd") && !Path::new(cwd).is_absolute() { object.insert( "cwd".to_string(), - JsonValue::String(plugin_root.join(cwd).display().to_string()), + JsonValue::String(root.join(cwd).display().to_string()), ); } diff --git a/codex-rs/codex-mcp/src/plugin_config_tests.rs b/codex-rs/codex-mcp/src/plugin_config_tests.rs index 44f7e8208..d95cd5095 100644 --- a/codex-rs/codex-mcp/src/plugin_config_tests.rs +++ b/codex-rs/codex-mcp/src/plugin_config_tests.rs @@ -1,6 +1,6 @@ use super::PluginMcpConfigParseOutcome; use super::PluginMcpServerParseError; -use super::PluginMcpServerPlacement; +use super::parse_executor_plugin_mcp_config; use super::parse_plugin_mcp_config; use codex_config::DEFAULT_MCP_SERVER_ENVIRONMENT_ID; use codex_config::McpServerConfig; @@ -8,6 +8,7 @@ use codex_config::McpServerEnvVar; use codex_config::McpServerOAuthConfig; use codex_config::McpServerTransportConfig; use codex_utils_path_uri::LegacyAppPathString; +use codex_utils_path_uri::PathUri; use pretty_assertions::assert_eq; use std::collections::BTreeMap; use std::collections::HashMap; @@ -20,10 +21,14 @@ fn plugin_root() -> PathBuf { .join("plugin-root") } +fn plugin_root_uri(plugin_root: &Path) -> PathUri { + PathUri::from_host_native_path(plugin_root).expect("plugin root URI") +} + fn stdio_server( command: &str, environment_id: &str, - cwd: &Path, + cwd: LegacyAppPathString, env_vars: Vec, ) -> McpServerConfig { McpServerConfig { @@ -32,7 +37,7 @@ fn stdio_server( args: Vec::new(), env: None, env_vars, - cwd: Some(LegacyAppPathString::from_path(cwd)), + cwd: Some(cwd), }, environment_id: environment_id.to_string(), enabled: true, @@ -56,8 +61,8 @@ fn declared_placement_preserves_local_plugin_normalization() { let plugin_root = plugin_root(); let expected_stdio = stdio_server( "demo-mcp", - "configured-environment", - &plugin_root.join("scripts"), + DEFAULT_MCP_SERVER_ENVIRONMENT_ID, + LegacyAppPathString::from_path(&plugin_root.join("scripts")), Vec::new(), ); let expected_http = McpServerConfig { @@ -91,7 +96,6 @@ fn declared_placement_preserves_local_plugin_normalization() { "demo": { "type": "stdio", "command": "demo-mcp", - "environment_id": "configured-environment", "cwd": "scripts" }, "hosted": { @@ -100,7 +104,6 @@ fn declared_placement_preserves_local_plugin_normalization() { "oauth": {"clientId": "client-id", "callbackPort": 9876} } }"#, - PluginMcpServerPlacement::Declared, ) .expect("parse plugin MCP config"); @@ -119,8 +122,9 @@ fn declared_placement_preserves_local_plugin_normalization() { #[test] fn environment_placement_forces_authority_and_defaults_null_cwd() { let plugin_root = plugin_root(); - let outcome = parse_plugin_mcp_config( - &plugin_root, + let plugin_root_uri = plugin_root_uri(&plugin_root); + let outcome = parse_executor_plugin_mcp_config( + &plugin_root_uri, r#"{ "$schema":"https://example.com/plugin-mcp.schema.json", "mcpServers":{"demo":{ @@ -130,9 +134,7 @@ fn environment_placement_forces_authority_and_defaults_null_cwd() { "env_vars":["EXECUTOR_TOKEN", {"name":"OTHER_TOKEN"}] }} }"#, - PluginMcpServerPlacement::Environment { - environment_id: "executor-1", - }, + "executor-1", ) .expect("parse plugin MCP config"); @@ -144,7 +146,7 @@ fn environment_placement_forces_authority_and_defaults_null_cwd() { stdio_server( "demo-mcp", "executor-1", - &plugin_root, + plugin_root_uri.into(), vec![ McpServerEnvVar::Config { name: "EXECUTOR_TOKEN".to_string(), @@ -165,12 +167,11 @@ fn environment_placement_forces_authority_and_defaults_null_cwd() { #[test] fn environment_placement_resolves_relative_cwd_beneath_plugin_root() { let plugin_root = plugin_root(); - let outcome = parse_plugin_mcp_config( - &plugin_root, + let plugin_root_uri = plugin_root_uri(&plugin_root); + let outcome = parse_executor_plugin_mcp_config( + &plugin_root_uri, r#"{"demo":{"command":"demo-mcp","cwd":"scripts"}}"#, - PluginMcpServerPlacement::Environment { - environment_id: "executor-1", - }, + "executor-1", ) .expect("parse plugin MCP config"); @@ -182,7 +183,39 @@ fn environment_placement_resolves_relative_cwd_beneath_plugin_root() { stdio_server( "demo-mcp", "executor-1", - &plugin_root.join("scripts"), + plugin_root_uri + .join("scripts") + .expect("plugin cwd URI") + .into(), + Vec::new(), + ), + )]), + errors: Vec::new(), + } + ); +} + +#[test] +fn executor_environment_placement_resolves_foreign_uri_cwd() { + let plugin_root = PathUri::parse("file:///C:/plugins/demo").expect("plugin root URI"); + let outcome = parse_executor_plugin_mcp_config( + &plugin_root, + r#"{"demo":{"command":"demo-mcp","cwd":"scripts"}}"#, + "executor-1", + ) + .expect("parse plugin MCP config"); + + assert_eq!( + outcome, + PluginMcpConfigParseOutcome { + servers: BTreeMap::from([( + "demo".to_string(), + stdio_server( + "demo-mcp", + "executor-1", + LegacyAppPathString::from( + plugin_root.join("scripts").expect("executor cwd URI"), + ), Vec::new(), ), )]), @@ -194,12 +227,11 @@ fn environment_placement_resolves_relative_cwd_beneath_plugin_root() { #[test] fn environment_placement_rejects_relative_cwd_that_escapes_package() { let plugin_root = plugin_root(); - let outcome = parse_plugin_mcp_config( - &plugin_root, + let plugin_root_uri = plugin_root_uri(&plugin_root); + let outcome = parse_executor_plugin_mcp_config( + &plugin_root_uri, r#"{"demo":{"command":"demo-mcp","cwd":"../outside"}}"#, - PluginMcpServerPlacement::Environment { - environment_id: "executor-1", - }, + "executor-1", ) .expect("parse plugin MCP config"); @@ -210,8 +242,7 @@ fn environment_placement_rejects_relative_cwd_that_escapes_package() { errors: vec![PluginMcpServerParseError { name: "demo".to_string(), message: format!( - "relative cwd `../outside` must remain within plugin root `{}`", - plugin_root.display() + "cwd `../outside` must remain within plugin root `{plugin_root_uri}`" ), }], } @@ -221,12 +252,10 @@ fn environment_placement_rejects_relative_cwd_that_escapes_package() { #[test] fn environment_placement_rejects_orchestrator_env_vars() { let plugin_root = plugin_root(); - let outcome = parse_plugin_mcp_config( - &plugin_root, + let outcome = parse_executor_plugin_mcp_config( + &plugin_root_uri(&plugin_root), r#"{"demo":{"command":"demo-mcp","env_vars":[{"name":"TOKEN","source":"local"}]}}"#, - PluginMcpServerPlacement::Environment { - environment_id: "executor-1", - }, + "executor-1", ) .expect("parse plugin MCP config"); @@ -247,12 +276,11 @@ fn environment_placement_rejects_orchestrator_env_vars() { #[test] fn local_environment_placement_preserves_local_env_vars() { let plugin_root = plugin_root(); - let outcome = parse_plugin_mcp_config( - &plugin_root, + let plugin_root_uri = plugin_root_uri(&plugin_root); + let outcome = parse_executor_plugin_mcp_config( + &plugin_root_uri, r#"{"demo":{"command":"demo-mcp","env_vars":["TOKEN",{"name":"OTHER","source":"local"}]}}"#, - PluginMcpServerPlacement::Environment { - environment_id: DEFAULT_MCP_SERVER_ENVIRONMENT_ID, - }, + DEFAULT_MCP_SERVER_ENVIRONMENT_ID, ) .expect("parse plugin MCP config"); @@ -264,7 +292,7 @@ fn local_environment_placement_preserves_local_env_vars() { stdio_server( "demo-mcp", DEFAULT_MCP_SERVER_ENVIRONMENT_ID, - &plugin_root, + plugin_root_uri.into(), vec![ McpServerEnvVar::Name("TOKEN".to_string()), McpServerEnvVar::Config { @@ -282,12 +310,10 @@ fn local_environment_placement_preserves_local_env_vars() { #[test] fn local_environment_placement_rejects_remote_env_vars() { let plugin_root = plugin_root(); - let outcome = parse_plugin_mcp_config( - &plugin_root, + let outcome = parse_executor_plugin_mcp_config( + &plugin_root_uri(&plugin_root), r#"{"demo":{"command":"demo-mcp","env_vars":[{"name":"TOKEN","source":"remote"}]}}"#, - PluginMcpServerPlacement::Environment { - environment_id: DEFAULT_MCP_SERVER_ENVIRONMENT_ID, - }, + DEFAULT_MCP_SERVER_ENVIRONMENT_ID, ) .expect("parse plugin MCP config"); diff --git a/codex-rs/codex-mcp/src/runtime.rs b/codex-rs/codex-mcp/src/runtime.rs index e48bdc7f2..ce8d4156a 100644 --- a/codex-rs/codex-mcp/src/runtime.rs +++ b/codex-rs/codex-mcp/src/runtime.rs @@ -13,7 +13,6 @@ use codex_exec_server::Environment; use codex_exec_server::EnvironmentManager; use codex_protocol::models::PermissionProfile; use codex_utils_path_uri::PathUri; - use serde::Deserialize; use serde::Serialize; @@ -225,6 +224,36 @@ mod tests { } } + #[tokio::test] + async fn remote_stdio_accepts_foreign_absolute_cwd() { + let runtime_context = McpRuntimeContext::new( + Arc::new( + EnvironmentManager::create_for_tests( + Some("ws://127.0.0.1:8765".to_string()), + /*local_runtime_paths*/ None, + ) + .await, + ), + PathBuf::from("/tmp"), + ); + let mut remote_stdio = stdio_server("remote"); + let McpServerTransportConfig::Stdio { cwd, .. } = &mut remote_stdio.transport else { + unreachable!("stdio helper should build stdio transport"); + }; + *cwd = Some( + PathUri::parse("file:///C:/plugins/demo") + .expect("foreign cwd URI") + .into(), + ); + + let resolved_runtime = + match runtime_context.resolve_server_environment("stdio", &remote_stdio) { + Ok(resolved_runtime) => resolved_runtime, + Err(error) => panic!("foreign cwd should resolve: {error}"), + }; + assert!(resolved_runtime.is_some()); + } + #[tokio::test] async fn local_stdio_accepts_local_environment_when_available() { let runtime_context = McpRuntimeContext::new( diff --git a/codex-rs/core-plugins/src/loader.rs b/codex-rs/core-plugins/src/loader.rs index 6fdc3f1a2..db99256f3 100644 --- a/codex-rs/core-plugins/src/loader.rs +++ b/codex-rs/core-plugins/src/loader.rs @@ -28,7 +28,6 @@ use codex_core_skills::config_rules::skill_config_rules_from_stack; use codex_core_skills::loader::SkillRoot; use codex_core_skills::loader::load_skills_from_roots; use codex_exec_server::LOCAL_FS; -use codex_mcp::PluginMcpServerPlacement; use codex_mcp::parse_plugin_mcp_config; use codex_plugin::AppConnectorId; use codex_plugin::AppDeclaration; @@ -1282,17 +1281,16 @@ async fn load_mcp_servers_from_file( let Ok(contents) = tokio::fs::read_to_string(mcp_config_path.as_path()).await else { return PluginMcpDiscovery::default(); }; - let parsed = - match parse_plugin_mcp_config(plugin_root, &contents, PluginMcpServerPlacement::Declared) { - Ok(parsed) => parsed, - Err(err) => { - warn!( - path = %mcp_config_path.display(), - "failed to parse plugin MCP config: {err}" - ); - return PluginMcpDiscovery::default(); - } - }; + let parsed = match parse_plugin_mcp_config(plugin_root, &contents) { + Ok(parsed) => parsed, + Err(err) => { + warn!( + path = %mcp_config_path.display(), + "failed to parse plugin MCP config: {err}" + ); + return PluginMcpDiscovery::default(); + } + }; for error in parsed.errors { warn!( plugin = %plugin_root.display(), @@ -1311,11 +1309,7 @@ fn load_mcp_servers_from_manifest_object( plugin_root: &Path, object_config: &str, ) -> PluginMcpDiscovery { - let parsed = match parse_plugin_mcp_config( - plugin_root, - object_config, - PluginMcpServerPlacement::Declared, - ) { + let parsed = match parse_plugin_mcp_config(plugin_root, object_config) { Ok(parsed) => parsed, Err(err) => { warn!( diff --git a/codex-rs/ext/mcp/src/executor_plugin/provider.rs b/codex-rs/ext/mcp/src/executor_plugin/provider.rs index d3f264fcc..9b8a9878e 100644 --- a/codex-rs/ext/mcp/src/executor_plugin/provider.rs +++ b/codex-rs/ext/mcp/src/executor_plugin/provider.rs @@ -2,8 +2,7 @@ use codex_config::McpServerConfig; use codex_config::McpServerTransportConfig; use codex_core_plugins::ResolvedExecutorPlugin; use codex_exec_server::ExecutorFileSystem; -use codex_mcp::PluginMcpServerPlacement; -use codex_mcp::parse_plugin_mcp_config; +use codex_mcp::parse_executor_plugin_mcp_config; use codex_plugin::PluginResourceLocator; use codex_plugin::ResolvedPlugin; use codex_plugin::ResolvedPluginLocation; @@ -11,7 +10,6 @@ use codex_plugin::manifest::PluginManifestMcpServers; use codex_utils_path_uri::PathUri; use codex_utils_path_uri::PathUriParseError; use std::io; -use std::path::PathBuf; use thiserror::Error; const DEFAULT_MCP_CONFIG_FILE: &str = ".mcp.json"; @@ -116,17 +114,13 @@ async fn load_from_file_system( (contents, config_path) } }; - let plugin_root_path = PathBuf::from(plugin_root.inferred_native_path_string()); - let parsed = parse_plugin_mcp_config( - plugin_root_path.as_path(), - &contents, - PluginMcpServerPlacement::Environment { environment_id }, - ) - .map_err(|source| ExecutorPluginMcpProviderError::ParseConfig { - plugin_id: plugin_id.to_string(), - path: config_path, - source, - })?; + let parsed = parse_executor_plugin_mcp_config(plugin_root, &contents, environment_id).map_err( + |source| ExecutorPluginMcpProviderError::ParseConfig { + plugin_id: plugin_id.to_string(), + path: config_path, + source, + }, + )?; for error in parsed.errors { tracing::warn!(