diff --git a/codex-rs/app-server-protocol/schema/typescript/LegacyAppPathString.ts b/codex-rs/app-server-protocol/schema/typescript/LegacyAppPathString.ts index 04e465b8a..e39784a8b 100644 --- a/codex-rs/app-server-protocol/schema/typescript/LegacyAppPathString.ts +++ b/codex-rs/app-server-protocol/schema/typescript/LegacyAppPathString.ts @@ -11,10 +11,10 @@ * * When converting from [`PathUri`], "native" refers to the supplied * [`PathConvention`], which may be foreign to the operating system running - * this process. The inner string is private so path-producing code must convert - * from [`AbsolutePathBuf`] or use [`Self::from_path_uri`] instead of bypassing - * the intended conversion boundary. Non-UTF-8 paths are converted to UTF-8 - * lossily because this API value is serialized as a JSON string. + * this process. The inner string is private so path-producing code must use a + * path conversion method instead of bypassing the intended conversion + * boundary. Non-UTF-8 paths are converted to UTF-8 lossily because this API + * value is serialized as a JSON string. * * Deserialization accepts any UTF-8 string without interpreting or validating * it. That unrestricted construction path is intentionally available only to diff --git a/codex-rs/cli/src/doctor.rs b/codex-rs/cli/src/doctor.rs index 5e5a1e94b..5f60a96ff 100644 --- a/codex-rs/cli/src/doctor.rs +++ b/codex-rs/cli/src/doctor.rs @@ -1520,19 +1520,35 @@ async fn mcp_check_from_servers(servers: &HashMap) -> D if disabled_server { continue; } - if let Some(cwd) = cwd - && !cwd.exists() - { - missing_env.push(format!("{name}: cwd does not exist ({})", cwd.display())); - } - if command.trim().is_empty() { + let command_is_empty = command.trim().is_empty(); + if command_is_empty { missing_env.push(format!("{name}: stdio command is empty")); - } else if let Err(err) = - stdio_command_resolves(command, cwd.as_deref(), env.as_ref()) - { - missing_env.push(format!( - "{name}: stdio command {command:?} is not resolvable ({err})" - )); + } + if server.is_local_environment() { + let host_native_cwd = cwd.as_ref().map(|cwd| Path::new(cwd.as_str())); + if let Some(cwd) = host_native_cwd + && !cwd.exists() + { + missing_env.push(format!("{name}: cwd does not exist ({})", cwd.display())); + } + if !command_is_empty + && let Err(err) = + stdio_command_resolves(command, host_native_cwd, env.as_ref()) + { + missing_env.push(format!( + "{name}: stdio command {command:?} is not resolvable ({err})" + )); + } + } else { + match cwd { + Some(cwd) if cwd.to_inferred_path_uri().is_none() => { + missing_env + .push(format!("{name}: remote stdio cwd is not absolute ({cwd})")); + } + None => missing_env + .push(format!("{name}: remote stdio requires an explicit cwd")), + Some(_) => {} + } } if let Some(env) = env { for key in env.keys().filter(|key| key.trim().is_empty()) { @@ -1541,10 +1557,12 @@ async fn mcp_check_from_servers(servers: &HashMap) -> D } for env_var in env_vars { if env_var.is_remote_source() { - missing_env.push(format!( - "{name}: env_vars entry `{}` uses source `remote`, which requires remote MCP stdio", - env_var.name() - )); + if server.is_local_environment() { + missing_env.push(format!( + "{name}: env_vars entry `{}` uses source `remote`, which requires remote MCP stdio", + env_var.name() + )); + } } else if !env_var_present(env_var.name()) { missing_env.push(format!("{name}: env var {} is not set", env_var.name())); } @@ -3863,6 +3881,70 @@ mod tests { })); } + #[tokio::test] + async fn mcp_check_skips_host_path_checks_for_remote_stdio() { + #[cfg(not(windows))] + let cwd = r"C:\Users\openai\share"; + #[cfg(windows)] + let cwd = "/home/openai/share"; + let cwd = toml::Value::String(cwd.to_string()); + let remote_server: McpServerConfig = toml::from_str(&format!( + r#" + command = "definitely-missing-codex-doctor-mcp" + environment_id = "remote" + cwd = {cwd} + required = true + env_vars = [{{ name = "REMOTE_ONLY_TOKEN", source = "remote" }}] + "#, + )) + .expect("should deserialize 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"); + } + + #[tokio::test] + async fn mcp_check_validates_remote_stdio_cwd() { + let missing_cwd: McpServerConfig = toml::from_str( + r#" + command = "echo" + environment_id = "remote" + required = true + "#, + ) + .expect("should deserialize remote MCP config without cwd"); + let relative_cwd: McpServerConfig = toml::from_str( + r#" + command = "echo" + environment_id = "remote" + cwd = "relative" + required = true + "#, + ) + .expect("should deserialize remote MCP config with relative cwd"); + let servers = HashMap::from([ + ("missing".to_string(), missing_cwd), + ("relative".to_string(), relative_cwd), + ]); + + let check = mcp_check_from_servers(&servers).await; + + assert_eq!(check.status, CheckStatus::Fail); + assert!( + check + .details + .contains(&"missing: remote stdio requires an explicit cwd".to_string()) + ); + assert!( + check + .details + .contains(&"relative: remote stdio cwd is not absolute (relative)".to_string()) + ); + } + #[cfg(unix)] #[test] fn read_probe_file_rejects_unreadable_file() { diff --git a/codex-rs/cli/src/mcp_cmd.rs b/codex-rs/cli/src/mcp_cmd.rs index 1466f1039..7baee7d34 100644 --- a/codex-rs/cli/src/mcp_cmd.rs +++ b/codex-rs/cli/src/mcp_cmd.rs @@ -640,7 +640,7 @@ async fn run_list(config_overrides: &CliConfigOverrides, list_args: ListArgs) -> let env_display = format_env_display(env.as_ref(), env_vars); let cwd_display = cwd .as_ref() - .map(|path| path.display().to_string()) + .map(ToString::to_string) .filter(|value| !value.is_empty()) .unwrap_or_else(|| "-".to_string()); let status = format_mcp_status(cfg); @@ -897,7 +897,7 @@ async fn run_get(config_overrides: &CliConfigOverrides, get_args: GetArgs) -> Re println!(" args: {args_display}"); let cwd_display = cwd .as_ref() - .map(|path| path.display().to_string()) + .map(ToString::to_string) .filter(|value| !value.is_empty()) .unwrap_or_else(|| "-".to_string()); println!(" cwd: {cwd_display}"); diff --git a/codex-rs/codex-mcp/src/plugin_config_tests.rs b/codex-rs/codex-mcp/src/plugin_config_tests.rs index 0ba8f42f3..44f7e8208 100644 --- a/codex-rs/codex-mcp/src/plugin_config_tests.rs +++ b/codex-rs/codex-mcp/src/plugin_config_tests.rs @@ -7,6 +7,7 @@ use codex_config::McpServerConfig; use codex_config::McpServerEnvVar; use codex_config::McpServerOAuthConfig; use codex_config::McpServerTransportConfig; +use codex_utils_path_uri::LegacyAppPathString; use pretty_assertions::assert_eq; use std::collections::BTreeMap; use std::collections::HashMap; @@ -31,7 +32,7 @@ fn stdio_server( args: Vec::new(), env: None, env_vars, - cwd: Some(cwd.to_path_buf()), + cwd: Some(LegacyAppPathString::from_path(cwd)), }, environment_id: environment_id.to_string(), enabled: true, diff --git a/codex-rs/codex-mcp/src/rmcp_client.rs b/codex-rs/codex-mcp/src/rmcp_client.rs index d6c2590ba..6c8b9a241 100644 --- a/codex-rs/codex-mcp/src/rmcp_client.rs +++ b/codex-rs/codex-mcp/src/rmcp_client.rs @@ -645,6 +645,7 @@ async fn make_rmcp_client( )) as Arc }; + let cwd = cwd.map(codex_utils_path_uri::LegacyAppPathString::into_string); RmcpClient::new_stdio_client(command_os, args_os, env_os, &env_vars, cwd, launcher) .await .map_err(|err| StartupOutcomeError::from(anyhow!(err))) diff --git a/codex-rs/codex-mcp/src/runtime.rs b/codex-rs/codex-mcp/src/runtime.rs index ff5c65db0..e52048349 100644 --- a/codex-rs/codex-mcp/src/runtime.rs +++ b/codex-rs/codex-mcp/src/runtime.rs @@ -66,9 +66,6 @@ impl McpRuntimeContext { .environment_manager .get_environment(&config.environment_id) { - if !config.is_local_environment() { - ensure_remote_stdio_cwd(server_name, config)?; - } return Ok(Some(environment)); } @@ -88,27 +85,6 @@ impl McpRuntimeContext { } } -fn ensure_remote_stdio_cwd( - server_name: &str, - config: &codex_config::McpServerConfig, -) -> Result<(), String> { - let codex_config::McpServerTransportConfig::Stdio { cwd, .. } = &config.transport else { - return Ok(()); - }; - let Some(cwd) = cwd else { - return Err(format!( - "remote stdio MCP server `{server_name}` requires an absolute cwd" - )); - }; - if cwd.is_absolute() { - return Ok(()); - } - Err(format!( - "remote stdio MCP server `{server_name}` requires an absolute cwd, got `{}`", - cwd.display() - )) -} - pub(crate) fn emit_duration(metric: &str, duration: Duration, tags: &[(&str, &str)]) { if let Some(metrics) = codex_otel::global() { let _ = metrics.record_duration(metric, duration, tags); @@ -123,6 +99,7 @@ mod tests { use codex_config::McpServerConfig; use codex_config::McpServerTransportConfig; use codex_exec_server::EnvironmentManager; + use codex_utils_path_uri::LegacyAppPathString; use pretty_assertions::assert_eq; use super::*; @@ -236,7 +213,7 @@ mod tests { let McpServerTransportConfig::Stdio { cwd, .. } = &mut remote_stdio.transport else { unreachable!("stdio helper should build stdio transport"); }; - *cwd = Some(std::env::temp_dir()); + *cwd = Some(LegacyAppPathString::from_path(&std::env::temp_dir())); for resolved_runtime in [ runtime_context.resolve_server_environment("stdio", &remote_stdio), runtime_context.resolve_server_environment("http", &http_server("remote")), @@ -264,32 +241,4 @@ mod tests { }; assert!(resolved_runtime.is_some()); } - - #[tokio::test] - async fn remote_stdio_requires_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(PathBuf::from("relative")); - - let error = match runtime_context.resolve_server_environment("stdio", &remote_stdio) { - Ok(_) => panic!("remote stdio MCP should require absolute cwd"), - Err(error) => error, - }; - assert_eq!( - error, - "remote stdio MCP server `stdio` requires an absolute cwd, got `relative`" - ); - } } diff --git a/codex-rs/config/src/mcp_edit.rs b/codex-rs/config/src/mcp_edit.rs index 9f007de11..1d896e5f3 100644 --- a/codex-rs/config/src/mcp_edit.rs +++ b/codex-rs/config/src/mcp_edit.rs @@ -146,7 +146,7 @@ fn serialize_mcp_server(config: &McpServerConfig) -> TomlItem { entry["env_vars"] = array_from_env_vars(env_vars); } if let Some(cwd) = cwd { - entry["cwd"] = value(cwd.to_string_lossy().to_string()); + entry["cwd"] = value(cwd.as_str()); } } McpServerTransportConfig::StreamableHttp { diff --git a/codex-rs/config/src/mcp_types.rs b/codex-rs/config/src/mcp_types.rs index 0d30ba8b2..42142212c 100644 --- a/codex-rs/config/src/mcp_types.rs +++ b/codex-rs/config/src/mcp_types.rs @@ -2,9 +2,9 @@ use std::collections::HashMap; use std::fmt; -use std::path::PathBuf; use std::time::Duration; +use codex_utils_path_uri::LegacyAppPathString; use schemars::JsonSchema; use serde::Deserialize; use serde::Deserializer; @@ -224,7 +224,7 @@ pub struct RawMcpServerConfig { #[serde(default)] pub env_vars: Option>, #[serde(default)] - pub cwd: Option, + pub cwd: Option, pub http_headers: Option>, #[serde(default)] pub env_http_headers: Option>, @@ -358,7 +358,6 @@ impl TryFrom for McpServerConfig { let environment_id = environment_id.unwrap_or_else(|| DEFAULT_MCP_SERVER_ENVIRONMENT_ID.to_string()); - validate_remote_stdio_cwd(&transport, &environment_id)?; Ok(Self { transport, @@ -395,30 +394,6 @@ const fn default_enabled() -> bool { true } -fn validate_remote_stdio_cwd( - transport: &McpServerTransportConfig, - environment_id: &str, -) -> Result<(), String> { - if environment_id == DEFAULT_MCP_SERVER_ENVIRONMENT_ID { - return Ok(()); - } - let McpServerTransportConfig::Stdio { cwd, .. } = transport else { - return Ok(()); - }; - let Some(cwd) = cwd else { - return Err(format!( - "remote stdio MCP servers require an absolute cwd when environment_id is `{environment_id}`" - )); - }; - if cwd.is_absolute() { - return Ok(()); - } - Err(format!( - "remote stdio MCP servers require an absolute cwd when environment_id is `{environment_id}`, got `{}`", - cwd.display() - )) -} - #[derive(Serialize, Deserialize, Debug, Clone, PartialEq, JsonSchema)] #[serde(untagged, deny_unknown_fields, rename_all = "snake_case")] pub enum McpServerTransportConfig { @@ -432,7 +407,7 @@ pub enum McpServerTransportConfig { #[serde(default, skip_serializing_if = "Vec::is_empty")] env_vars: Vec, #[serde(default, skip_serializing_if = "Option::is_none")] - cwd: Option, + cwd: Option, }, /// https://modelcontextprotocol.io/specification/2025-06-18/basic/transports#streamable-http StreamableHttp { diff --git a/codex-rs/config/src/mcp_types_tests.rs b/codex-rs/config/src/mcp_types_tests.rs index 5f3933ce4..d3ec9d77e 100644 --- a/codex-rs/config/src/mcp_types_tests.rs +++ b/codex-rs/config/src/mcp_types_tests.rs @@ -1,7 +1,8 @@ use super::*; +use codex_utils_path_uri::LegacyAppPathString; use pretty_assertions::assert_eq; use std::collections::HashMap; -use std::path::PathBuf; +use std::path::Path; #[test] fn deserialize_stdio_command_server_config() { @@ -52,38 +53,12 @@ fn deserialize_stdio_command_server_config_with_args() { } #[test] -fn deserialize_remote_stdio_server_requires_absolute_cwd() { - let missing_cwd = toml::from_str::( - r#" - command = "echo" - environment_id = "remote" - "#, - ) - .expect_err("remote stdio MCP should require cwd"); - assert!( - missing_cwd - .to_string() - .contains("remote stdio MCP servers require an absolute cwd"), - "unexpected error: {missing_cwd}" - ); - - let relative_cwd = toml::from_str::( - r#" - command = "echo" - environment_id = "remote" - cwd = "relative" - "#, - ) - .expect_err("remote stdio MCP should require absolute cwd"); - assert!( - relative_cwd.to_string().contains("got `relative`"), - "unexpected error: {relative_cwd}" - ); -} - -#[test] -fn deserialize_remote_stdio_server_accepts_absolute_cwd() { - let cwd = std::env::temp_dir(); +fn deserialize_remote_stdio_server_accepts_foreign_absolute_cwd() { + #[cfg(not(windows))] + let cwd = r"C:\Users\openai\share"; + #[cfg(windows)] + let cwd = "/home/openai/share"; + let expected_cwd = LegacyAppPathString::from_path(Path::new(cwd)); let cfg: McpServerConfig = match toml::from_str(&format!( r#" command = "echo" @@ -102,7 +77,7 @@ fn deserialize_remote_stdio_server_accepts_absolute_cwd() { args: vec![], env: None, env_vars: Vec::new(), - cwd: Some(cwd), + cwd: Some(expected_cwd), } ); } @@ -223,7 +198,7 @@ fn deserialize_stdio_command_server_config_with_cwd() { args: vec![], env: None, env_vars: Vec::new(), - cwd: Some(PathBuf::from("/tmp")), + cwd: Some(LegacyAppPathString::from_path(Path::new("/tmp"))), } ); } diff --git a/codex-rs/core/config.schema.json b/codex-rs/core/config.schema.json index a10d01a8a..31dcc667d 100644 --- a/codex-rs/core/config.schema.json +++ b/codex-rs/core/config.schema.json @@ -1324,6 +1324,9 @@ ], "description": "One action binding value in config.\n\nThis accepts either:\n\n1. A single key spec string (`\"ctrl-a\"`). 2. A list of key spec strings (`[\"ctrl-a\", \"alt-a\"]`).\n\nAn empty list explicitly unbinds the action in that scope. Because an explicit empty list is still a configured value, runtime resolution must not fall through to global or built-in defaults for that action." }, + "LegacyAppPathString": { + "type": "string" + }, "MarketplaceConfig": { "additionalProperties": false, "properties": { @@ -2440,8 +2443,12 @@ "type": "string" }, "cwd": { - "default": null, - "type": "string" + "allOf": [ + { + "$ref": "#/definitions/LegacyAppPathString" + } + ], + "default": null }, "default_tools_approval_mode": { "allOf": [ diff --git a/codex-rs/core/src/config/config_tests.rs b/codex-rs/core/src/config/config_tests.rs index 9b4b6e93f..67cb0932d 100644 --- a/codex-rs/core/src/config/config_tests.rs +++ b/codex-rs/core/src/config/config_tests.rs @@ -89,6 +89,7 @@ use codex_protocol::protocol::MultiAgentVersion; use codex_protocol::protocol::NetworkAccess; use codex_protocol::protocol::RealtimeVoice; use codex_protocol::protocol::SandboxPolicy; +use codex_utils_path_uri::LegacyAppPathString; use serde::Deserialize; use tempfile::tempdir; @@ -5554,6 +5555,7 @@ async fn load_global_mcp_servers_returns_empty_if_missing() -> anyhow::Result<() #[tokio::test] async fn replace_mcp_servers_round_trips_entries() -> anyhow::Result<()> { let codex_home = TempDir::new()?; + let expected_cwd = LegacyAppPathString::from_path(codex_home.path()); let mut servers = BTreeMap::new(); servers.insert( @@ -5564,7 +5566,7 @@ async fn replace_mcp_servers_round_trips_entries() -> anyhow::Result<()> { args: vec!["hello".to_string()], env: None, env_vars: Vec::new(), - cwd: Some(codex_home.path().to_path_buf()), + cwd: Some(expected_cwd.clone()), }, environment_id: "remote".to_string(), enabled: true, @@ -5603,7 +5605,7 @@ async fn replace_mcp_servers_round_trips_entries() -> anyhow::Result<()> { assert_eq!(args, &vec!["hello".to_string()]); assert!(env.is_none()); assert!(env_vars.is_empty()); - assert_eq!(cwd, &Some(codex_home.path().to_path_buf())); + assert_eq!(cwd, &Some(expected_cwd)); } other => panic!("unexpected transport {other:?}"), } @@ -6104,6 +6106,7 @@ async fn replace_mcp_servers_serializes_cwd() -> anyhow::Result<()> { let codex_home = TempDir::new()?; let cwd_path = PathBuf::from("/tmp/codex-mcp"); + let cwd = LegacyAppPathString::from_path(&cwd_path); let servers = BTreeMap::from([( "docs".to_string(), McpServerConfig { @@ -6112,7 +6115,7 @@ async fn replace_mcp_servers_serializes_cwd() -> anyhow::Result<()> { args: Vec::new(), env: None, env_vars: Vec::new(), - cwd: Some(cwd_path.clone()), + cwd: Some(cwd.clone()), }, environment_id: codex_config::DEFAULT_MCP_SERVER_ENVIRONMENT_ID.to_string(), enabled: true, @@ -6147,7 +6150,7 @@ async fn replace_mcp_servers_serializes_cwd() -> anyhow::Result<()> { let docs = loaded.get("docs").expect("docs entry"); match &docs.transport { McpServerTransportConfig::Stdio { cwd, .. } => { - assert_eq!(cwd.as_deref(), Some(Path::new("/tmp/codex-mcp"))); + assert_eq!(cwd, &Some(LegacyAppPathString::from_path(&cwd_path))); } other => panic!("unexpected transport {other:?}"), } diff --git a/codex-rs/core/src/config/edit/document_helpers.rs b/codex-rs/core/src/config/edit/document_helpers.rs index 5b99306eb..3aed9a49b 100644 --- a/codex-rs/core/src/config/edit/document_helpers.rs +++ b/codex-rs/core/src/config/edit/document_helpers.rs @@ -69,7 +69,7 @@ fn serialize_mcp_server_table(config: &McpServerConfig) -> TomlTable { entry["env_vars"] = array_from_env_vars(env_vars); } if let Some(cwd) = cwd { - entry["cwd"] = value(cwd.to_string_lossy().to_string()); + entry["cwd"] = value(cwd.as_str()); } } McpServerTransportConfig::StreamableHttp { diff --git a/codex-rs/core/tests/suite/rmcp_client.rs b/codex-rs/core/tests/suite/rmcp_client.rs index 95f7fbb93..78fa73ce5 100644 --- a/codex-rs/core/tests/suite/rmcp_client.rs +++ b/codex-rs/core/tests/suite/rmcp_client.rs @@ -27,6 +27,7 @@ use codex_exec_server::HttpRequestParams; use codex_login::CodexAuth; use codex_mcp::MCP_SANDBOX_STATE_META_CAPABILITY; use codex_models_manager::manager::RefreshStrategy; +use codex_utils_path_uri::LegacyAppPathString; use codex_protocol::config_types::ReasoningSummary; use codex_protocol::models::PermissionProfile; @@ -297,7 +298,7 @@ fn stdio_transport_with_cwd( args: Vec::new(), env, env_vars, - cwd, + cwd: cwd.map(|cwd| LegacyAppPathString::from_path(&cwd)), } } diff --git a/codex-rs/ext/mcp/src/executor_plugin/provider_tests.rs b/codex-rs/ext/mcp/src/executor_plugin/provider_tests.rs index b154b060b..07d662185 100644 --- a/codex-rs/ext/mcp/src/executor_plugin/provider_tests.rs +++ b/codex-rs/ext/mcp/src/executor_plugin/provider_tests.rs @@ -18,6 +18,7 @@ use codex_plugin::manifest::PluginManifest; use codex_plugin::manifest::PluginManifestMcpServers; use codex_plugin::manifest::PluginManifestPaths; use codex_utils_absolute_path::AbsolutePathBuf; +use codex_utils_path_uri::LegacyAppPathString; use codex_utils_path_uri::PathUri; use pretty_assertions::assert_eq; use std::collections::HashMap; @@ -169,7 +170,7 @@ async fn reads_declared_config_only_through_executor_file_system() { args: Vec::new(), env: None, env_vars: Vec::new(), - cwd: Some(plugin_root.to_path_buf()), + cwd: Some(LegacyAppPathString::from_path(plugin_root.as_path())), }, environment_id: "executor-test".to_string(), enabled: true, @@ -223,7 +224,7 @@ async fn reads_manifest_object_config_without_executor_file_system_access() { args: Vec::new(), env: None, env_vars: Vec::new(), - cwd: Some(plugin_root.to_path_buf()), + cwd: Some(LegacyAppPathString::from_path(plugin_root.as_path())), }, environment_id: "executor-test".to_string(), enabled: true, diff --git a/codex-rs/rmcp-client/src/rmcp_client.rs b/codex-rs/rmcp-client/src/rmcp_client.rs index 4a9b89de3..f5c7da529 100644 --- a/codex-rs/rmcp-client/src/rmcp_client.rs +++ b/codex-rs/rmcp-client/src/rmcp_client.rs @@ -2,7 +2,6 @@ use std::collections::HashMap; use std::ffi::OsString; use std::future::Future; use std::io; -use std::path::PathBuf; use std::sync::Arc; use std::sync::atomic::AtomicUsize; use std::sync::atomic::Ordering; @@ -354,7 +353,7 @@ impl RmcpClient { args: Vec, env: Option>, env_vars: &[McpServerEnvVar], - cwd: Option, + cwd: Option, launcher: Arc, ) -> io::Result { let transport_recipe = TransportRecipe::Stdio { diff --git a/codex-rs/rmcp-client/src/stdio_server_launcher.rs b/codex-rs/rmcp-client/src/stdio_server_launcher.rs index dfd2420db..bc1a8924d 100644 --- a/codex-rs/rmcp-client/src/stdio_server_launcher.rs +++ b/codex-rs/rmcp-client/src/stdio_server_launcher.rs @@ -15,6 +15,7 @@ use std::collections::HashMap; use std::ffi::OsString; use std::future::Future; use std::io; +use std::path::Path; use std::path::PathBuf; use std::process::Stdio; use std::sync::Arc; @@ -35,6 +36,7 @@ use codex_exec_server::ExecEnvPolicy; use codex_exec_server::ExecParams; use codex_exec_server::ExecProcess; use codex_protocol::config_types::ShellEnvironmentPolicyInherit; +use codex_utils_path_uri::LegacyAppPathString; use codex_utils_path_uri::PathUri; #[cfg(unix)] use codex_utils_pty::process_group::kill_process_group; @@ -82,7 +84,7 @@ pub struct StdioServerCommand { args: Vec, env: Option>, env_vars: Vec, - cwd: Option, + cwd: Option, } /// Client-side rmcp transport for a launched MCP stdio server. @@ -149,7 +151,7 @@ impl StdioServerCommand { args: Vec, env: Option>, env_vars: Vec, - cwd: Option, + cwd: Option, ) -> Self { Self { program, @@ -247,7 +249,7 @@ impl LocalStdioServerLauncher { } = command; let program_name = program.to_string_lossy().into_owned(); let envs = create_env_for_mcp_server(env, &env_vars).map_err(io::Error::other)?; - let cwd = cwd.unwrap_or(fallback_cwd); + let cwd = cwd.map(PathBuf::from).unwrap_or(fallback_cwd); let resolved_program = program_resolver::resolve(program, &envs, &cwd).map_err(io::Error::other)?; @@ -479,6 +481,9 @@ impl ExecutorStdioServerLauncher { "executor stdio server requires an explicit cwd", )); }; + let cwd: PathUri = LegacyAppPathString::from_path(Path::new(&cwd)) + .try_into() + .map_err(|err| io::Error::new(io::ErrorKind::InvalidInput, err))?; let program_name = program.to_string_lossy().into_owned(); let envs = create_env_overlay_for_remote_mcp_server(env, &env_vars); let remote_env_vars = remote_mcp_env_var_names(&env_vars); @@ -488,7 +493,6 @@ impl ExecutorStdioServerLauncher { // before sending an executor request. let argv = Self::process_api_argv(&program, &args).map_err(io::Error::other)?; let env = Self::process_api_env(envs).map_err(io::Error::other)?; - let cwd = PathUri::from_host_native_path(cwd)?; let process_id = ExecutorProcessTransport::next_process_id(); // Start the MCP server process on the executor with raw pipes. `tty=false` // keeps stdout as a clean protocol stream, while `pipe_stdin=true` lets diff --git a/codex-rs/rmcp-client/tests/foreign_stdio_cwd.rs b/codex-rs/rmcp-client/tests/foreign_stdio_cwd.rs new file mode 100644 index 000000000..84aca7b92 --- /dev/null +++ b/codex-rs/rmcp-client/tests/foreign_stdio_cwd.rs @@ -0,0 +1,67 @@ +use std::ffi::OsString; +use std::sync::Arc; +use std::sync::Mutex; + +use codex_exec_server::ExecBackend; +use codex_exec_server::ExecBackendFuture; +use codex_exec_server::ExecParams; +use codex_exec_server::ExecServerError; +use codex_rmcp_client::ExecutorStdioServerLauncher; +use codex_rmcp_client::RmcpClient; +use codex_utils_path_uri::PathUri; +use pretty_assertions::assert_eq; + +#[derive(Default)] +struct RecordingExecBackend { + params: Mutex>, +} + +impl ExecBackend for RecordingExecBackend { + fn start(&self, params: ExecParams) -> ExecBackendFuture<'_> { + let mut recorded_params = match self.params.lock() { + Ok(recorded_params) => recorded_params, + Err(poisoned) => poisoned.into_inner(), + }; + *recorded_params = Some(params); + Box::pin(async { + Err(ExecServerError::Protocol( + "stop after recording executor request".to_string(), + )) + }) + } +} + +#[tokio::test] +async fn executor_stdio_forwards_foreign_absolute_cwd_as_path_uri() { + #[cfg(not(windows))] + let cwd = r"C:\Users\openai\share"; + #[cfg(windows)] + let cwd = "/home/openai/share"; + #[cfg(not(windows))] + let expected_cwd: PathUri = "file:///C:/Users/openai/share" + .parse() + .expect("expected cwd should be a path URI"); + #[cfg(windows)] + let expected_cwd: PathUri = "file:///home/openai/share" + .parse() + .expect("expected cwd should be a path URI"); + let backend = Arc::new(RecordingExecBackend::default()); + let launcher = Arc::new(ExecutorStdioServerLauncher::new(backend.clone())); + + let _ = RmcpClient::new_stdio_client( + OsString::from("echo"), + Vec::new(), + /*env*/ None, + &[], + Some(cwd.to_string()), + launcher, + ) + .await; + let params = backend + .params + .lock() + .expect("recorded params lock should not be poisoned") + .take() + .expect("executor start request should be recorded"); + assert_eq!(params.cwd, expected_cwd); +} diff --git a/codex-rs/tui/src/history_cell/mcp.rs b/codex-rs/tui/src/history_cell/mcp.rs index 599a44f09..b71af3d25 100644 --- a/codex-rs/tui/src/history_cell/mcp.rs +++ b/codex-rs/tui/src/history_cell/mcp.rs @@ -414,7 +414,7 @@ pub(crate) fn new_mcp_tools_output( lines.push(vec![" • Command: ".into(), cmd_display.into()].into()); if let Some(cwd) = cwd.as_ref() { - lines.push(vec![" • Cwd: ".into(), cwd.display().to_string().into()].into()); + lines.push(vec![" • Cwd: ".into(), cwd.to_string().into()].into()); } let env_display = format_env_display(env.as_ref(), env_vars); diff --git a/codex-rs/utils/path-uri/src/api_path_string.rs b/codex-rs/utils/path-uri/src/api_path_string.rs index 3e33f316c..e4fdbe7c2 100644 --- a/codex-rs/utils/path-uri/src/api_path_string.rs +++ b/codex-rs/utils/path-uri/src/api_path_string.rs @@ -7,6 +7,7 @@ use serde::Deserialize; use serde::Serialize; use serde::Serializer; use std::fmt; +use std::path::Path; use thiserror::Error; use ts_rs::TS; @@ -18,10 +19,10 @@ use ts_rs::TS; /// /// When converting from [`PathUri`], "native" refers to the supplied /// [`PathConvention`], which may be foreign to the operating system running -/// this process. The inner string is private so path-producing code must convert -/// from [`AbsolutePathBuf`] or use [`Self::from_path_uri`] instead of bypassing -/// the intended conversion boundary. Non-UTF-8 paths are converted to UTF-8 -/// lossily because this API value is serialized as a JSON string. +/// this process. The inner string is private so path-producing code must use a +/// path conversion method instead of bypassing the intended conversion +/// boundary. Non-UTF-8 paths are converted to UTF-8 lossily because this API +/// value is serialized as a JSON string. /// /// Deserialization accepts any UTF-8 string without interpreting or validating /// it. That unrestricted construction path is intentionally available only to @@ -35,9 +36,14 @@ use ts_rs::TS; pub struct LegacyAppPathString(String); impl LegacyAppPathString { + /// Preserves path text without interpreting it using the current host. + pub fn from_path(path: &Path) -> Self { + Self(path.to_string_lossy().into_owned()) + } + /// Renders an absolute path using the current host's path convention. pub fn from_abs_path(path: &AbsolutePathBuf) -> Self { - Self(path.to_string_lossy().into_owned()) + Self::from_path(path.as_path()) } /// Renders a path URI using the requested native path convention. diff --git a/codex-rs/utils/path-uri/src/api_path_string_tests.rs b/codex-rs/utils/path-uri/src/api_path_string_tests.rs index 3439ca11e..c71a65abf 100644 --- a/codex-rs/utils/path-uri/src/api_path_string_tests.rs +++ b/codex-rs/utils/path-uri/src/api_path_string_tests.rs @@ -496,6 +496,23 @@ fn foreign_absolute_syntax_deserializes_without_host_interpretation() { } } +#[test] +fn from_path_preserves_foreign_absolute_path_for_uri_conversion() { + #[cfg(not(windows))] + let (foreign_path, expected_uri) = (r"C:\Users\openai\share", "file:///C:/Users/openai/share"); + #[cfg(windows)] + let (foreign_path, expected_uri) = ("/home/openai/share", "file:///home/openai/share"); + + let path: PathUri = LegacyAppPathString::from_path(std::path::Path::new(foreign_path)) + .try_into() + .expect("foreign absolute path should convert"); + + assert_eq!( + path, + PathUri::parse(expected_uri).expect("valid expected URI") + ); +} + #[test] fn renders_an_absolute_path_using_the_host_convention() { #[cfg(unix)]