diff --git a/codex-rs/core/src/mcp_connection_manager.rs b/codex-rs/core/src/mcp_connection_manager.rs index 938d6d0b2bf3..eac8485048a8 100644 --- a/codex-rs/core/src/mcp_connection_manager.rs +++ b/codex-rs/core/src/mcp_connection_manager.rs @@ -1440,7 +1440,12 @@ async fn make_rmcp_client( } => { let command_os: OsString = command.into(); let args_os: Vec = args.into_iter().map(Into::into).collect(); - RmcpClient::new_stdio_client(command_os, args_os, env, &env_vars, cwd) + let env_os = env.map(|env| { + env.into_iter() + .map(|(key, value)| (key.into(), value.into())) + .collect::>() + }); + RmcpClient::new_stdio_client(command_os, args_os, env_os, &env_vars, cwd) .await .map_err(|err| StartupOutcomeError::from(anyhow!(err))) } diff --git a/codex-rs/rmcp-client/src/program_resolver.rs b/codex-rs/rmcp-client/src/program_resolver.rs index e62336e263df..d9be2741c7e7 100644 --- a/codex-rs/rmcp-client/src/program_resolver.rs +++ b/codex-rs/rmcp-client/src/program_resolver.rs @@ -13,18 +13,13 @@ use std::collections::HashMap; use std::ffi::OsString; -#[cfg(windows)] -use std::env; -#[cfg(windows)] -use tracing::debug; - /// Resolves a program to its executable path on Unix systems. /// /// Unix systems handle PATH resolution and script execution natively through /// the kernel's shebang (`#!`) mechanism, so this function simply returns /// the program name unchanged. #[cfg(unix)] -pub fn resolve(program: OsString, _env: &HashMap) -> std::io::Result { +pub fn resolve(program: OsString, _env: &HashMap) -> std::io::Result { Ok(program) } @@ -38,25 +33,22 @@ pub fn resolve(program: OsString, _env: &HashMap) -> std::io::Re /// This enables tools like `npx`, `pnpm`, and `yarn` to work correctly on Windows /// without requiring users to specify full paths or extensions in their configuration. #[cfg(windows)] -pub fn resolve(program: OsString, env: &HashMap) -> std::io::Result { +pub fn resolve(program: OsString, env: &HashMap) -> std::io::Result { // Get current directory for relative path resolution - let cwd = env::current_dir() + let cwd = std::env::current_dir() .map_err(|e| std::io::Error::other(format!("Failed to get current directory: {e}")))?; // Extract PATH from environment for search locations - let search_path = env.get("PATH"); + let search_path = env.get(std::ffi::OsStr::new("PATH")); // Attempt resolution via which crate match which::which_in(&program, search_path, &cwd) { Ok(resolved) => { - debug!("Resolved {:?} to {:?}", program, resolved); + tracing::debug!("Resolved {program:?} to {resolved:?}"); Ok(resolved.into_os_string()) } Err(e) => { - debug!( - "Failed to resolve {:?}: {}. Using original path", - program, e - ); + tracing::debug!("Failed to resolve {program:?}: {e}. Using original path"); // Fallback to original program - let Command::new() handle the error Ok(program) } @@ -146,7 +138,7 @@ mod tests { // Held to prevent the temporary directory from being deleted. _temp_dir: TempDir, program_name: String, - mcp_env: HashMap, + mcp_env: HashMap, } impl TestExecutableEnv { @@ -160,10 +152,10 @@ mod tests { // Build a clean environment with the temp dir in the PATH. let mut extra_env = HashMap::new(); - extra_env.insert("PATH".to_string(), Self::build_path(dir_path)); + extra_env.insert(OsString::from("PATH"), Self::build_path_env_var(dir_path)); #[cfg(windows)] - extra_env.insert("PATHEXT".to_string(), Self::ensure_cmd_extension()); + extra_env.insert(OsString::from("PATHEXT"), Self::ensure_cmd_extension()); let mcp_env = create_env_for_mcp_server(Some(extra_env), &[]); @@ -202,20 +194,30 @@ mod tests { } /// Prepends the given directory to the system's PATH variable. - fn build_path(dir: &Path) -> String { - let current = std::env::var("PATH").unwrap_or_default(); - let sep = if cfg!(windows) { ";" } else { ":" }; - format!("{}{sep}{current}", dir.to_string_lossy()) + fn build_path_env_var(dir: &Path) -> OsString { + let mut path = OsString::from(dir.as_os_str()); + if let Some(current) = std::env::var_os("PATH") { + let sep = if cfg!(windows) { ";" } else { ":" }; + path.push(sep); + path.push(current); + } + path } /// Ensures `.CMD` is in the `PATHEXT` variable on Windows for script discovery. #[cfg(windows)] - fn ensure_cmd_extension() -> String { - let current = std::env::var("PATHEXT").unwrap_or_default(); - if current.to_uppercase().contains(".CMD") { + fn ensure_cmd_extension() -> OsString { + let current = std::env::var_os("PATHEXT").unwrap_or_default(); + if current + .to_string_lossy() + .to_ascii_uppercase() + .contains(".CMD") + { current } else { - format!(".CMD;{current}") + let mut path_ext = OsString::from(".CMD;"); + path_ext.push(current); + path_ext } } } diff --git a/codex-rs/rmcp-client/src/rmcp_client.rs b/codex-rs/rmcp-client/src/rmcp_client.rs index 55a3603ed7a6..aa460c21b94f 100644 --- a/codex-rs/rmcp-client/src/rmcp_client.rs +++ b/codex-rs/rmcp-client/src/rmcp_client.rs @@ -390,7 +390,7 @@ enum TransportRecipe { Stdio { program: OsString, args: Vec, - env: Option>, + env: Option>, env_vars: Vec, cwd: Option, }, @@ -478,7 +478,7 @@ impl RmcpClient { pub async fn new_stdio_client( program: OsString, args: Vec, - env: Option>, + env: Option>, env_vars: &[String], cwd: Option, ) -> io::Result { diff --git a/codex-rs/rmcp-client/src/utils.rs b/codex-rs/rmcp-client/src/utils.rs index df09b0f8be86..cb906aaaa954 100644 --- a/codex-rs/rmcp-client/src/utils.rs +++ b/codex-rs/rmcp-client/src/utils.rs @@ -5,16 +5,17 @@ use reqwest::header::HeaderName; use reqwest::header::HeaderValue; use std::collections::HashMap; use std::env; +use std::ffi::OsString; pub(crate) fn create_env_for_mcp_server( - extra_env: Option>, + extra_env: Option>, env_vars: &[String], -) -> HashMap { +) -> HashMap { DEFAULT_ENV_VARS .iter() .copied() .chain(env_vars.iter().map(String::as_str)) - .filter_map(|var| env::var(var).ok().map(|value| (var.to_string(), value))) + .filter_map(|var| env::var_os(var).map(|value| (OsString::from(var), value))) .chain(extra_env.unwrap_or_default()) .collect() } @@ -140,7 +141,7 @@ mod tests { use pretty_assertions::assert_eq; use serial_test::serial; - use std::ffi::OsString; + use std::ffi::OsStr; struct EnvVarGuard { key: String, @@ -148,10 +149,10 @@ mod tests { } impl EnvVarGuard { - fn set(key: &str, value: &str) -> Self { + fn set(key: &str, value: impl AsRef) -> Self { let original = std::env::var_os(key); unsafe { - std::env::set_var(key, value); + std::env::set_var(key, value.as_ref()); } Self { key: key.to_string(), @@ -177,9 +178,12 @@ mod tests { #[tokio::test] async fn create_env_honors_overrides() { let value = "custom".to_string(); - let env = - create_env_for_mcp_server(Some(HashMap::from([("TZ".into(), value.clone())])), &[]); - assert_eq!(env.get("TZ"), Some(&value)); + let expected = OsString::from(&value); + let env = create_env_for_mcp_server( + Some(HashMap::from([(OsString::from("TZ"), expected.clone())])), + &[], + ); + assert_eq!(env.get(OsStr::new("TZ")), Some(&expected)); } #[test] @@ -187,8 +191,24 @@ mod tests { fn create_env_includes_additional_whitelisted_variables() { let custom_var = "EXTRA_RMCP_ENV"; let value = "from-env"; + let expected = OsString::from(value); let _guard = EnvVarGuard::set(custom_var, value); let env = create_env_for_mcp_server(None, &[custom_var.to_string()]); - assert_eq!(env.get(custom_var), Some(&value.to_string())); + assert_eq!(env.get(OsStr::new(custom_var)), Some(&expected)); + } + + #[cfg(unix)] + #[test] + #[serial(extra_rmcp_env)] + fn create_env_preserves_path_when_it_is_not_utf8() { + use std::os::unix::ffi::OsStrExt; + + let raw_path = std::ffi::OsStr::from_bytes(b"/tmp/codex-\xFF/bin"); + let expected = raw_path.to_os_string(); + let _guard = EnvVarGuard::set("PATH", raw_path); + + let env = create_env_for_mcp_server(None, &[]); + + assert_eq!(env.get(OsStr::new("PATH")), Some(&expected)); } } diff --git a/codex-rs/rmcp-client/tests/process_group_cleanup.rs b/codex-rs/rmcp-client/tests/process_group_cleanup.rs index bb033af875fa..2d6c220ce85b 100644 --- a/codex-rs/rmcp-client/tests/process_group_cleanup.rs +++ b/codex-rs/rmcp-client/tests/process_group_cleanup.rs @@ -73,8 +73,8 @@ async fn drop_kills_wrapper_process_group() -> Result<()> { ), ], Some(HashMap::from([( - "CHILD_PID_FILE".to_string(), - child_pid_file_str, + OsString::from("CHILD_PID_FILE"), + OsString::from(child_pid_file_str), )])), &[], None,