From a4f7708e834fae618d2bcc78c6b104cee658e61f Mon Sep 17 00:00:00 2001 From: Adam Perry Date: Mon, 22 Jun 2026 22:30:28 +0000 Subject: [PATCH 1/9] mcp: accept foreign absolute remote stdio cwd --- codex-rs/codex-mcp/src/rmcp_client.rs | 10 +++ codex-rs/codex-mcp/src/runtime.rs | 80 +++++++++++++------ codex-rs/config/src/lib.rs | 1 + codex-rs/config/src/mcp_types.rs | 30 ++++--- codex-rs/config/src/mcp_types_tests.rs | 40 ++++++++++ .../rmcp-client/src/stdio_server_launcher.rs | 5 +- .../utils/path-uri/src/api_path_string.rs | 16 ++-- 7 files changed, 138 insertions(+), 44 deletions(-) diff --git a/codex-rs/codex-mcp/src/rmcp_client.rs b/codex-rs/codex-mcp/src/rmcp_client.rs index d6c2590ba400..08e89c8c082a 100644 --- a/codex-rs/codex-mcp/src/rmcp_client.rs +++ b/codex-rs/codex-mcp/src/rmcp_client.rs @@ -609,6 +609,13 @@ async fn make_rmcp_client( let resolved_environment = runtime_context .resolve_server_environment(server_name, &config) .map_err(|err| StartupOutcomeError::from(anyhow!(err)))?; + let resolved_remote_stdio_cwd = match &config.transport { + McpServerTransportConfig::Stdio { .. } => { + codex_config::resolve_remote_stdio_cwd(&config.transport, &config.environment_id) + .map_err(|err| StartupOutcomeError::from(anyhow!(err)))? + } + McpServerTransportConfig::StreamableHttp { .. } => None, + }; let is_local_environment = config.is_local_environment(); let McpServerConfig { transport, .. } = config; @@ -640,6 +647,9 @@ async fn make_rmcp_client( "non-local stdio MCP servers resolve an environment before launch" ); }; + let Some(_) = resolved_remote_stdio_cwd else { + unreachable!("non-local stdio MCP servers resolve a cwd before launch"); + }; Arc::new(ExecutorStdioServerLauncher::new( environment.get_exec_backend(), )) as Arc diff --git a/codex-rs/codex-mcp/src/runtime.rs b/codex-rs/codex-mcp/src/runtime.rs index ff5c65db005c..dd8518798a2e 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); @@ -127,6 +103,15 @@ mod tests { use super::*; + #[cfg(not(windows))] + const FOREIGN_CWD: &str = r"C:\Users\openai\share"; + #[cfg(not(windows))] + const FOREIGN_CWD_URI: &str = "file:///C:/Users/openai/share"; + #[cfg(windows)] + const FOREIGN_CWD: &str = "/home/openai/share"; + #[cfg(windows)] + const FOREIGN_CWD_URI: &str = "file:///home/openai/share"; + fn stdio_server(environment_id: &str) -> McpServerConfig { McpServerConfig { transport: McpServerTransportConfig::Stdio { @@ -249,6 +234,42 @@ mod tests { } } + #[tokio::test] + async fn remote_stdio_resolves_foreign_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(FOREIGN_CWD)); + + let resolved_environment = + match runtime_context.resolve_server_environment("stdio", &remote_stdio) { + Ok(resolved_environment) => resolved_environment, + Err(error) => panic!("foreign cwd environment should resolve: {error}"), + }; + assert!(resolved_environment.is_some()); + let remote_stdio_cwd = + match codex_config::resolve_remote_stdio_cwd(&remote_stdio.transport, "remote") { + Ok(Some(cwd)) => cwd, + Ok(None) => panic!("remote stdio MCP should resolve a cwd"), + Err(error) => panic!("foreign cwd should resolve: {error}"), + }; + assert_eq!( + remote_stdio_cwd, + PathUri::parse(FOREIGN_CWD_URI).expect("foreign cwd URI") + ); + } + #[tokio::test] async fn local_stdio_accepts_local_environment_when_available() { let runtime_context = McpRuntimeContext::new( @@ -283,13 +304,20 @@ mod tests { }; *cwd = Some(PathBuf::from("relative")); - let error = match runtime_context.resolve_server_environment("stdio", &remote_stdio) { + let resolved_environment = + match runtime_context.resolve_server_environment("stdio", &remote_stdio) { + Ok(resolved_environment) => resolved_environment, + Err(error) => panic!("remote stdio environment should resolve: {error}"), + }; + assert!(resolved_environment.is_some()); + let error = match codex_config::resolve_remote_stdio_cwd(&remote_stdio.transport, "remote") + { 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`" + "remote stdio MCP servers require an absolute cwd when environment_id is `remote`, got `relative`" ); } } diff --git a/codex-rs/config/src/lib.rs b/codex-rs/config/src/lib.rs index ed26757715f8..43093cae74b2 100644 --- a/codex-rs/config/src/lib.rs +++ b/codex-rs/config/src/lib.rs @@ -117,6 +117,7 @@ pub use mcp_types::McpServerOAuthConfig; pub use mcp_types::McpServerToolConfig; pub use mcp_types::McpServerTransportConfig; pub use mcp_types::RawMcpServerConfig; +pub use mcp_types::resolve_remote_stdio_cwd; pub use merge::merge_toml_values; pub use overrides::build_cli_overrides_layer; pub use plugin_edit::PluginConfigEdit; diff --git a/codex-rs/config/src/mcp_types.rs b/codex-rs/config/src/mcp_types.rs index 0d30ba8b23f4..697feeb99332 100644 --- a/codex-rs/config/src/mcp_types.rs +++ b/codex-rs/config/src/mcp_types.rs @@ -5,6 +5,8 @@ use std::fmt; use std::path::PathBuf; use std::time::Duration; +use codex_utils_path_uri::LegacyAppPathString; +use codex_utils_path_uri::PathUri; use schemars::JsonSchema; use serde::Deserialize; use serde::Deserializer; @@ -358,7 +360,7 @@ 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)?; + resolve_remote_stdio_cwd(&transport, &environment_id)?; Ok(Self { transport, @@ -395,28 +397,32 @@ const fn default_enabled() -> bool { true } -fn validate_remote_stdio_cwd( +/// Resolves a remote stdio server's cwd without applying the current host's +/// path rules. Returns `None` for local or non-stdio servers. +pub fn resolve_remote_stdio_cwd( transport: &McpServerTransportConfig, environment_id: &str, -) -> Result<(), String> { +) -> Result, String> { if environment_id == DEFAULT_MCP_SERVER_ENVIRONMENT_ID { - return Ok(()); + return Ok(None); } let McpServerTransportConfig::Stdio { cwd, .. } = transport else { - return Ok(()); + return Ok(None); }; 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() - )) + LegacyAppPathString::from_path(cwd) + .to_inferred_path_uri() + .map(Some) + .ok_or_else(|| { + 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)] diff --git a/codex-rs/config/src/mcp_types_tests.rs b/codex-rs/config/src/mcp_types_tests.rs index 5f3933ce43ee..2d084a2b04b6 100644 --- a/codex-rs/config/src/mcp_types_tests.rs +++ b/codex-rs/config/src/mcp_types_tests.rs @@ -3,6 +3,15 @@ use pretty_assertions::assert_eq; use std::collections::HashMap; use std::path::PathBuf; +#[cfg(not(windows))] +const FOREIGN_CWD: &str = r"C:\Users\openai\share"; +#[cfg(not(windows))] +const FOREIGN_CWD_URI: &str = "file:///C:/Users/openai/share"; +#[cfg(windows)] +const FOREIGN_CWD: &str = "/home/openai/share"; +#[cfg(windows)] +const FOREIGN_CWD_URI: &str = "file:///home/openai/share"; + #[test] fn deserialize_stdio_command_server_config() { let cfg: McpServerConfig = toml::from_str( @@ -107,6 +116,37 @@ fn deserialize_remote_stdio_server_accepts_absolute_cwd() { ); } +#[test] +fn deserialize_remote_stdio_server_accepts_foreign_absolute_cwd() { + let cwd = PathBuf::from(FOREIGN_CWD); + let cwd_toml = toml::Value::String(FOREIGN_CWD.to_string()); + let cfg: McpServerConfig = toml::from_str(&format!( + r#" + command = "echo" + environment_id = "remote" + cwd = {cwd_toml} + "# + )) + .expect("remote stdio MCP should accept a foreign absolute cwd"); + + assert_eq!( + resolve_remote_stdio_cwd(&cfg.transport, &cfg.environment_id), + Ok(Some( + PathUri::parse(FOREIGN_CWD_URI).expect("foreign cwd URI") + )) + ); + assert_eq!( + cfg.transport, + McpServerTransportConfig::Stdio { + command: "echo".to_string(), + args: vec![], + env: None, + env_vars: Vec::new(), + cwd: Some(cwd), + } + ); +} + #[test] fn deserialize_stdio_command_server_config_with_arg_with_args_and_env() { let cfg: McpServerConfig = toml::from_str( diff --git a/codex-rs/rmcp-client/src/stdio_server_launcher.rs b/codex-rs/rmcp-client/src/stdio_server_launcher.rs index dfd2420dbdab..5e39675d0a50 100644 --- a/codex-rs/rmcp-client/src/stdio_server_launcher.rs +++ b/codex-rs/rmcp-client/src/stdio_server_launcher.rs @@ -35,6 +35,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; @@ -479,6 +480,9 @@ impl ExecutorStdioServerLauncher { "executor stdio server requires an explicit cwd", )); }; + let cwd: PathUri = LegacyAppPathString::from_path(&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 +492,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/utils/path-uri/src/api_path_string.rs b/codex-rs/utils/path-uri/src/api_path_string.rs index 3e33f316ca05..e4fdbe7c275e 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. From 3cd7263abf1b3e77deafed663b520212f81fa9b9 Mon Sep 17 00:00:00 2001 From: Adam Perry Date: Mon, 22 Jun 2026 22:48:24 +0000 Subject: [PATCH 2/9] codex: address PR review feedback (#29493) --- codex-rs/codex-mcp/src/rmcp_client.rs | 10 --- codex-rs/codex-mcp/src/runtime.rs | 80 ------------------- codex-rs/config/src/lib.rs | 1 - codex-rs/config/src/mcp_types.rs | 31 ------- codex-rs/config/src/mcp_types_tests.rs | 70 ---------------- .../path-uri/src/api_path_string_tests.rs | 17 ++++ 6 files changed, 17 insertions(+), 192 deletions(-) diff --git a/codex-rs/codex-mcp/src/rmcp_client.rs b/codex-rs/codex-mcp/src/rmcp_client.rs index 08e89c8c082a..d6c2590ba400 100644 --- a/codex-rs/codex-mcp/src/rmcp_client.rs +++ b/codex-rs/codex-mcp/src/rmcp_client.rs @@ -609,13 +609,6 @@ async fn make_rmcp_client( let resolved_environment = runtime_context .resolve_server_environment(server_name, &config) .map_err(|err| StartupOutcomeError::from(anyhow!(err)))?; - let resolved_remote_stdio_cwd = match &config.transport { - McpServerTransportConfig::Stdio { .. } => { - codex_config::resolve_remote_stdio_cwd(&config.transport, &config.environment_id) - .map_err(|err| StartupOutcomeError::from(anyhow!(err)))? - } - McpServerTransportConfig::StreamableHttp { .. } => None, - }; let is_local_environment = config.is_local_environment(); let McpServerConfig { transport, .. } = config; @@ -647,9 +640,6 @@ async fn make_rmcp_client( "non-local stdio MCP servers resolve an environment before launch" ); }; - let Some(_) = resolved_remote_stdio_cwd else { - unreachable!("non-local stdio MCP servers resolve a cwd before launch"); - }; Arc::new(ExecutorStdioServerLauncher::new( environment.get_exec_backend(), )) as Arc diff --git a/codex-rs/codex-mcp/src/runtime.rs b/codex-rs/codex-mcp/src/runtime.rs index dd8518798a2e..f470e4a77227 100644 --- a/codex-rs/codex-mcp/src/runtime.rs +++ b/codex-rs/codex-mcp/src/runtime.rs @@ -103,15 +103,6 @@ mod tests { use super::*; - #[cfg(not(windows))] - const FOREIGN_CWD: &str = r"C:\Users\openai\share"; - #[cfg(not(windows))] - const FOREIGN_CWD_URI: &str = "file:///C:/Users/openai/share"; - #[cfg(windows)] - const FOREIGN_CWD: &str = "/home/openai/share"; - #[cfg(windows)] - const FOREIGN_CWD_URI: &str = "file:///home/openai/share"; - fn stdio_server(environment_id: &str) -> McpServerConfig { McpServerConfig { transport: McpServerTransportConfig::Stdio { @@ -234,42 +225,6 @@ mod tests { } } - #[tokio::test] - async fn remote_stdio_resolves_foreign_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(FOREIGN_CWD)); - - let resolved_environment = - match runtime_context.resolve_server_environment("stdio", &remote_stdio) { - Ok(resolved_environment) => resolved_environment, - Err(error) => panic!("foreign cwd environment should resolve: {error}"), - }; - assert!(resolved_environment.is_some()); - let remote_stdio_cwd = - match codex_config::resolve_remote_stdio_cwd(&remote_stdio.transport, "remote") { - Ok(Some(cwd)) => cwd, - Ok(None) => panic!("remote stdio MCP should resolve a cwd"), - Err(error) => panic!("foreign cwd should resolve: {error}"), - }; - assert_eq!( - remote_stdio_cwd, - PathUri::parse(FOREIGN_CWD_URI).expect("foreign cwd URI") - ); - } - #[tokio::test] async fn local_stdio_accepts_local_environment_when_available() { let runtime_context = McpRuntimeContext::new( @@ -285,39 +240,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 resolved_environment = - match runtime_context.resolve_server_environment("stdio", &remote_stdio) { - Ok(resolved_environment) => resolved_environment, - Err(error) => panic!("remote stdio environment should resolve: {error}"), - }; - assert!(resolved_environment.is_some()); - let error = match codex_config::resolve_remote_stdio_cwd(&remote_stdio.transport, "remote") - { - Ok(_) => panic!("remote stdio MCP should require absolute cwd"), - Err(error) => error, - }; - assert_eq!( - error, - "remote stdio MCP servers require an absolute cwd when environment_id is `remote`, got `relative`" - ); - } } diff --git a/codex-rs/config/src/lib.rs b/codex-rs/config/src/lib.rs index 43093cae74b2..ed26757715f8 100644 --- a/codex-rs/config/src/lib.rs +++ b/codex-rs/config/src/lib.rs @@ -117,7 +117,6 @@ pub use mcp_types::McpServerOAuthConfig; pub use mcp_types::McpServerToolConfig; pub use mcp_types::McpServerTransportConfig; pub use mcp_types::RawMcpServerConfig; -pub use mcp_types::resolve_remote_stdio_cwd; pub use merge::merge_toml_values; pub use overrides::build_cli_overrides_layer; pub use plugin_edit::PluginConfigEdit; diff --git a/codex-rs/config/src/mcp_types.rs b/codex-rs/config/src/mcp_types.rs index 697feeb99332..ccd344ab39f9 100644 --- a/codex-rs/config/src/mcp_types.rs +++ b/codex-rs/config/src/mcp_types.rs @@ -5,8 +5,6 @@ use std::fmt; use std::path::PathBuf; use std::time::Duration; -use codex_utils_path_uri::LegacyAppPathString; -use codex_utils_path_uri::PathUri; use schemars::JsonSchema; use serde::Deserialize; use serde::Deserializer; @@ -360,7 +358,6 @@ impl TryFrom for McpServerConfig { let environment_id = environment_id.unwrap_or_else(|| DEFAULT_MCP_SERVER_ENVIRONMENT_ID.to_string()); - resolve_remote_stdio_cwd(&transport, &environment_id)?; Ok(Self { transport, @@ -397,34 +394,6 @@ const fn default_enabled() -> bool { true } -/// Resolves a remote stdio server's cwd without applying the current host's -/// path rules. Returns `None` for local or non-stdio servers. -pub fn resolve_remote_stdio_cwd( - transport: &McpServerTransportConfig, - environment_id: &str, -) -> Result, String> { - if environment_id == DEFAULT_MCP_SERVER_ENVIRONMENT_ID { - return Ok(None); - } - let McpServerTransportConfig::Stdio { cwd, .. } = transport else { - return Ok(None); - }; - let Some(cwd) = cwd else { - return Err(format!( - "remote stdio MCP servers require an absolute cwd when environment_id is `{environment_id}`" - )); - }; - LegacyAppPathString::from_path(cwd) - .to_inferred_path_uri() - .map(Some) - .ok_or_else(|| { - 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 { diff --git a/codex-rs/config/src/mcp_types_tests.rs b/codex-rs/config/src/mcp_types_tests.rs index 2d084a2b04b6..2920c4910c0f 100644 --- a/codex-rs/config/src/mcp_types_tests.rs +++ b/codex-rs/config/src/mcp_types_tests.rs @@ -3,15 +3,6 @@ use pretty_assertions::assert_eq; use std::collections::HashMap; use std::path::PathBuf; -#[cfg(not(windows))] -const FOREIGN_CWD: &str = r"C:\Users\openai\share"; -#[cfg(not(windows))] -const FOREIGN_CWD_URI: &str = "file:///C:/Users/openai/share"; -#[cfg(windows)] -const FOREIGN_CWD: &str = "/home/openai/share"; -#[cfg(windows)] -const FOREIGN_CWD_URI: &str = "file:///home/openai/share"; - #[test] fn deserialize_stdio_command_server_config() { let cfg: McpServerConfig = toml::from_str( @@ -60,36 +51,6 @@ fn deserialize_stdio_command_server_config_with_args() { assert!(cfg.enabled); } -#[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(); @@ -116,37 +77,6 @@ fn deserialize_remote_stdio_server_accepts_absolute_cwd() { ); } -#[test] -fn deserialize_remote_stdio_server_accepts_foreign_absolute_cwd() { - let cwd = PathBuf::from(FOREIGN_CWD); - let cwd_toml = toml::Value::String(FOREIGN_CWD.to_string()); - let cfg: McpServerConfig = toml::from_str(&format!( - r#" - command = "echo" - environment_id = "remote" - cwd = {cwd_toml} - "# - )) - .expect("remote stdio MCP should accept a foreign absolute cwd"); - - assert_eq!( - resolve_remote_stdio_cwd(&cfg.transport, &cfg.environment_id), - Ok(Some( - PathUri::parse(FOREIGN_CWD_URI).expect("foreign cwd URI") - )) - ); - assert_eq!( - cfg.transport, - McpServerTransportConfig::Stdio { - command: "echo".to_string(), - args: vec![], - env: None, - env_vars: Vec::new(), - cwd: Some(cwd), - } - ); -} - #[test] fn deserialize_stdio_command_server_config_with_arg_with_args_and_env() { let cfg: McpServerConfig = toml::from_str( 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 3439ca11eac4..c71a65abf3df 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)] From 00bc6e5ce5aef84daa7a61b375e6c45d60b4b3b7 Mon Sep 17 00:00:00 2001 From: Adam Perry Date: Mon, 22 Jun 2026 22:59:46 +0000 Subject: [PATCH 3/9] codex: fix CI failure on PR #29493 --- .../schema/typescript/LegacyAppPathString.ts | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/codex-rs/app-server-protocol/schema/typescript/LegacyAppPathString.ts b/codex-rs/app-server-protocol/schema/typescript/LegacyAppPathString.ts index 04e465b8a29c..e39784a8b0e2 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 From e83d3b224de2c85a444c40a147bb0ff6d0020b04 Mon Sep 17 00:00:00 2001 From: Adam Perry Date: Mon, 22 Jun 2026 23:44:05 +0000 Subject: [PATCH 4/9] codex: address PR review feedback (#29493) --- codex-rs/cli/src/doctor.rs | 5 +++-- codex-rs/cli/src/mcp_cmd.rs | 4 ++-- codex-rs/codex-mcp/src/plugin_config_tests.rs | 3 ++- codex-rs/codex-mcp/src/runtime.rs | 3 ++- codex-rs/config/src/mcp_edit.rs | 2 +- codex-rs/config/src/mcp_types.rs | 6 +++--- codex-rs/config/src/mcp_types_tests.rs | 15 ++++++++++----- codex-rs/core/config.schema.json | 11 +++++++++-- codex-rs/core/src/config/config_tests.rs | 11 +++++++---- codex-rs/core/src/config/edit/document_helpers.rs | 2 +- codex-rs/core/tests/suite/rmcp_client.rs | 3 ++- .../ext/mcp/src/executor_plugin/provider_tests.rs | 5 +++-- codex-rs/rmcp-client/src/rmcp_client.rs | 4 ++-- codex-rs/rmcp-client/src/stdio_server_launcher.rs | 11 +++++++---- codex-rs/tui/src/history_cell/mcp.rs | 2 +- 15 files changed, 55 insertions(+), 32 deletions(-) diff --git a/codex-rs/cli/src/doctor.rs b/codex-rs/cli/src/doctor.rs index 5e5a1e94be29..f79f063a4e2c 100644 --- a/codex-rs/cli/src/doctor.rs +++ b/codex-rs/cli/src/doctor.rs @@ -1520,7 +1520,8 @@ async fn mcp_check_from_servers(servers: &HashMap) -> D if disabled_server { continue; } - if let Some(cwd) = cwd + 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())); @@ -1528,7 +1529,7 @@ async fn mcp_check_from_servers(servers: &HashMap) -> D if command.trim().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()) + stdio_command_resolves(command, host_native_cwd, env.as_ref()) { missing_env.push(format!( "{name}: stdio command {command:?} is not resolvable ({err})" diff --git a/codex-rs/cli/src/mcp_cmd.rs b/codex-rs/cli/src/mcp_cmd.rs index 1466f1039479..7baee7d3498c 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 0ba8f42f3595..44f7e8208ccf 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/runtime.rs b/codex-rs/codex-mcp/src/runtime.rs index f470e4a77227..e52048349c02 100644 --- a/codex-rs/codex-mcp/src/runtime.rs +++ b/codex-rs/codex-mcp/src/runtime.rs @@ -99,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::*; @@ -212,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")), diff --git a/codex-rs/config/src/mcp_edit.rs b/codex-rs/config/src/mcp_edit.rs index 9f007de11304..1d896e5f3148 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 ccd344ab39f9..42142212c520 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>, @@ -407,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 2920c4910c0f..d3ec9d77e6d6 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,8 +53,12 @@ fn deserialize_stdio_command_server_config_with_args() { } #[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" @@ -72,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), } ); } @@ -193,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 a10d01a8a7c6..31dcc667d1bf 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 9b4b6e93f8f7..67cb0932de69 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 5b99306eb906..3aed9a49ba15 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 95f7fbb93148..78fa73ce50d5 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 b154b060b2b1..07d6621856ab 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 4a9b89de3c32..98919522164e 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; @@ -16,6 +15,7 @@ use codex_client::maybe_build_rustls_client_config_with_custom_ca; use codex_config::types::AuthKeyringBackendKind; use codex_config::types::McpServerEnvVar; use codex_exec_server::HttpClient; +use codex_utils_path_uri::LegacyAppPathString; use futures::FutureExt; use futures::future::BoxFuture; use oauth2::TokenResponse; @@ -354,7 +354,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 5e39675d0a50..3f767241c2c2 100644 --- a/codex-rs/rmcp-client/src/stdio_server_launcher.rs +++ b/codex-rs/rmcp-client/src/stdio_server_launcher.rs @@ -83,7 +83,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. @@ -150,7 +150,7 @@ impl StdioServerCommand { args: Vec, env: Option>, env_vars: Vec, - cwd: Option, + cwd: Option, ) -> Self { Self { program, @@ -248,7 +248,10 @@ 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(LegacyAppPathString::into_string) + .map(PathBuf::from) + .unwrap_or(fallback_cwd); let resolved_program = program_resolver::resolve(program, &envs, &cwd).map_err(io::Error::other)?; @@ -480,7 +483,7 @@ impl ExecutorStdioServerLauncher { "executor stdio server requires an explicit cwd", )); }; - let cwd: PathUri = LegacyAppPathString::from_path(&cwd) + let cwd: PathUri = cwd .try_into() .map_err(|err| io::Error::new(io::ErrorKind::InvalidInput, err))?; let program_name = program.to_string_lossy().into_owned(); diff --git a/codex-rs/tui/src/history_cell/mcp.rs b/codex-rs/tui/src/history_cell/mcp.rs index 599a44f092d8..b71af3d251a2 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); From e4ebd6e3fe86e7e394f6eb166362911f6c99ef85 Mon Sep 17 00:00:00 2001 From: Adam Perry Date: Tue, 23 Jun 2026 00:26:35 +0000 Subject: [PATCH 5/9] codex: address PR review feedback (#29493) --- codex-rs/cli/src/doctor.rs | 55 ++++++++++---- .../rmcp-client/tests/foreign_stdio_cwd.rs | 76 +++++++++++++++++++ 2 files changed, 118 insertions(+), 13 deletions(-) create mode 100644 codex-rs/rmcp-client/tests/foreign_stdio_cwd.rs diff --git a/codex-rs/cli/src/doctor.rs b/codex-rs/cli/src/doctor.rs index f79f063a4e2c..3ae6acd6eff3 100644 --- a/codex-rs/cli/src/doctor.rs +++ b/codex-rs/cli/src/doctor.rs @@ -1520,20 +1520,25 @@ async fn mcp_check_from_servers(servers: &HashMap) -> D if disabled_server { continue; } - 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.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, host_native_cwd, 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})" + )); + } } if let Some(env) = env { for key in env.keys().filter(|key| key.trim().is_empty()) { @@ -3864,6 +3869,30 @@ 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 + "#, + )) + .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"); + } + #[cfg(unix)] #[test] fn read_probe_file_rejects_unreadable_file() { 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 000000000000..d153cd6f21ae --- /dev/null +++ b/codex-rs/rmcp-client/tests/foreign_stdio_cwd.rs @@ -0,0 +1,76 @@ +use std::ffi::OsString; +use std::path::Path; +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::LegacyAppPathString; +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"; + let cwd = LegacyAppPathString::from_path(Path::new(cwd)); + let expected_cwd: PathUri = cwd + .clone() + .try_into() + .expect("foreign absolute cwd should convert to a path URI"); + let backend = Arc::new(RecordingExecBackend::default()); + let launcher = Arc::new(ExecutorStdioServerLauncher::new(backend.clone())); + + let error = match RmcpClient::new_stdio_client( + OsString::from("echo"), + Vec::new(), + /*env*/ None, + &[], + Some(cwd), + launcher, + ) + .await + { + Ok(_) => panic!("recording backend should stop executor launch"), + Err(error) => error, + }; + + assert!( + error + .to_string() + .contains("stop after recording executor request") + ); + 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); +} From 0c85a304221f283f9775ecaeac90590017aa22c6 Mon Sep 17 00:00:00 2001 From: Adam Perry Date: Tue, 23 Jun 2026 00:47:54 +0000 Subject: [PATCH 6/9] codex: address PR review feedback (#29493) --- .../rmcp-client/tests/foreign_stdio_cwd.rs | 26 +++++++------------ 1 file changed, 10 insertions(+), 16 deletions(-) diff --git a/codex-rs/rmcp-client/tests/foreign_stdio_cwd.rs b/codex-rs/rmcp-client/tests/foreign_stdio_cwd.rs index d153cd6f21ae..47dc33c9ccbc 100644 --- a/codex-rs/rmcp-client/tests/foreign_stdio_cwd.rs +++ b/codex-rs/rmcp-client/tests/foreign_stdio_cwd.rs @@ -39,15 +39,19 @@ async fn executor_stdio_forwards_foreign_absolute_cwd_as_path_uri() { 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 cwd = LegacyAppPathString::from_path(Path::new(cwd)); - let expected_cwd: PathUri = cwd - .clone() - .try_into() - .expect("foreign absolute cwd should convert to a path URI"); let backend = Arc::new(RecordingExecBackend::default()); let launcher = Arc::new(ExecutorStdioServerLauncher::new(backend.clone())); - let error = match RmcpClient::new_stdio_client( + let _ = RmcpClient::new_stdio_client( OsString::from("echo"), Vec::new(), /*env*/ None, @@ -55,17 +59,7 @@ async fn executor_stdio_forwards_foreign_absolute_cwd_as_path_uri() { Some(cwd), launcher, ) - .await - { - Ok(_) => panic!("recording backend should stop executor launch"), - Err(error) => error, - }; - - assert!( - error - .to_string() - .contains("stop after recording executor request") - ); + .await; let params = backend .params .lock() From 843c5ba18137baec228654bea02e1bb5dc6b272d Mon Sep 17 00:00:00 2001 From: Adam Perry Date: Tue, 23 Jun 2026 00:59:24 +0000 Subject: [PATCH 7/9] codex: address PR review feedback (#29493) --- codex-rs/cli/src/doctor.rs | 49 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 49 insertions(+) diff --git a/codex-rs/cli/src/doctor.rs b/codex-rs/cli/src/doctor.rs index 3ae6acd6eff3..1450e257ecdc 100644 --- a/codex-rs/cli/src/doctor.rs +++ b/codex-rs/cli/src/doctor.rs @@ -1539,6 +1539,16 @@ async fn mcp_check_from_servers(servers: &HashMap) -> D "{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()) { @@ -3893,6 +3903,45 @@ mod tests { 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() { From ecfae0baae43dd67c8b0be468963a5ce438f090c Mon Sep 17 00:00:00 2001 From: Adam Perry Date: Tue, 23 Jun 2026 01:12:38 +0000 Subject: [PATCH 8/9] codex: address PR review feedback (#29493) --- codex-rs/cli/src/doctor.rs | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/codex-rs/cli/src/doctor.rs b/codex-rs/cli/src/doctor.rs index 1450e257ecdc..5f60a96ff3f3 100644 --- a/codex-rs/cli/src/doctor.rs +++ b/codex-rs/cli/src/doctor.rs @@ -1557,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())); } @@ -3892,6 +3894,7 @@ mod tests { environment_id = "remote" cwd = {cwd} required = true + env_vars = [{{ name = "REMOTE_ONLY_TOKEN", source = "remote" }}] "#, )) .expect("should deserialize remote MCP config"); From 23a68149bc811cf41a6a074504daa3a17992c99e Mon Sep 17 00:00:00 2001 From: Adam Perry Date: Tue, 23 Jun 2026 01:18:56 +0000 Subject: [PATCH 9/9] codex: address PR review feedback (#29493) --- codex-rs/codex-mcp/src/rmcp_client.rs | 1 + codex-rs/rmcp-client/src/rmcp_client.rs | 3 +-- codex-rs/rmcp-client/src/stdio_server_launcher.rs | 12 +++++------- codex-rs/rmcp-client/tests/foreign_stdio_cwd.rs | 5 +---- 4 files changed, 8 insertions(+), 13 deletions(-) diff --git a/codex-rs/codex-mcp/src/rmcp_client.rs b/codex-rs/codex-mcp/src/rmcp_client.rs index d6c2590ba400..6c8b9a2415ad 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/rmcp-client/src/rmcp_client.rs b/codex-rs/rmcp-client/src/rmcp_client.rs index 98919522164e..f5c7da529713 100644 --- a/codex-rs/rmcp-client/src/rmcp_client.rs +++ b/codex-rs/rmcp-client/src/rmcp_client.rs @@ -15,7 +15,6 @@ use codex_client::maybe_build_rustls_client_config_with_custom_ca; use codex_config::types::AuthKeyringBackendKind; use codex_config::types::McpServerEnvVar; use codex_exec_server::HttpClient; -use codex_utils_path_uri::LegacyAppPathString; use futures::FutureExt; use futures::future::BoxFuture; use oauth2::TokenResponse; @@ -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 3f767241c2c2..bc1a8924d516 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; @@ -83,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. @@ -150,7 +151,7 @@ impl StdioServerCommand { args: Vec, env: Option>, env_vars: Vec, - cwd: Option, + cwd: Option, ) -> Self { Self { program, @@ -248,10 +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 - .map(LegacyAppPathString::into_string) - .map(PathBuf::from) - .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)?; @@ -483,7 +481,7 @@ impl ExecutorStdioServerLauncher { "executor stdio server requires an explicit cwd", )); }; - let cwd: PathUri = 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(); diff --git a/codex-rs/rmcp-client/tests/foreign_stdio_cwd.rs b/codex-rs/rmcp-client/tests/foreign_stdio_cwd.rs index 47dc33c9ccbc..84aca7b92a5e 100644 --- a/codex-rs/rmcp-client/tests/foreign_stdio_cwd.rs +++ b/codex-rs/rmcp-client/tests/foreign_stdio_cwd.rs @@ -1,5 +1,4 @@ use std::ffi::OsString; -use std::path::Path; use std::sync::Arc; use std::sync::Mutex; @@ -9,7 +8,6 @@ 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::LegacyAppPathString; use codex_utils_path_uri::PathUri; use pretty_assertions::assert_eq; @@ -47,7 +45,6 @@ async fn executor_stdio_forwards_foreign_absolute_cwd_as_path_uri() { let expected_cwd: PathUri = "file:///home/openai/share" .parse() .expect("expected cwd should be a path URI"); - let cwd = LegacyAppPathString::from_path(Path::new(cwd)); let backend = Arc::new(RecordingExecBackend::default()); let launcher = Arc::new(ExecutorStdioServerLauncher::new(backend.clone())); @@ -56,7 +53,7 @@ async fn executor_stdio_forwards_foreign_absolute_cwd_as_path_uri() { Vec::new(), /*env*/ None, &[], - Some(cwd), + Some(cwd.to_string()), launcher, ) .await;