diff --git a/codex-rs/rmcp-client/src/program_resolver.rs b/codex-rs/rmcp-client/src/program_resolver.rs index e62336e263..6fd25c4377 100644 --- a/codex-rs/rmcp-client/src/program_resolver.rs +++ b/codex-rs/rmcp-client/src/program_resolver.rs @@ -16,6 +16,8 @@ use std::ffi::OsString; #[cfg(windows)] use std::env; #[cfg(windows)] +use std::ffi::OsStr; +#[cfg(windows)] use tracing::debug; /// Resolves a program to its executable path on Unix systems. @@ -24,7 +26,7 @@ use tracing::debug; /// 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,13 +40,13 @@ 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() .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(OsStr::new("PATH")); // Attempt resolution via which crate match which::which_in(&program, search_path, &cwd) { @@ -146,7 +148,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 +162,13 @@ 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().into(), + ); let mcp_env = create_env_for_mcp_server(Some(extra_env), &[]); @@ -202,10 +207,14 @@ 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. diff --git a/codex-rs/rmcp-client/src/rmcp_client.rs b/codex-rs/rmcp-client/src/rmcp_client.rs index 55a3603ed7..b073480775 100644 --- a/codex-rs/rmcp-client/src/rmcp_client.rs +++ b/codex-rs/rmcp-client/src/rmcp_client.rs @@ -863,7 +863,14 @@ impl RmcpClient { cwd, } => { let program_name = program.to_string_lossy().into_owned(); - let envs = create_env_for_mcp_server(env.clone(), env_vars); + let envs = create_env_for_mcp_server( + env.clone().map(|env| { + env.into_iter() + .map(|(key, value)| (key.into(), value.into())) + .collect::>() + }), + env_vars, + ); let resolved_program = program_resolver::resolve(program.clone(), &envs)?; let mut command = Command::new(resolved_program); diff --git a/codex-rs/rmcp-client/src/utils.rs b/codex-rs/rmcp-client/src/utils.rs index df09b0f8be..15f23ff44d 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, @@ -158,6 +159,18 @@ mod tests { original, } } + + #[cfg(unix)] + fn set_os(key: &str, value: &std::ffi::OsStr) -> Self { + let original = std::env::var_os(key); + unsafe { + std::env::set_var(key, value); + } + Self { + key: key.to_string(), + original, + } + } } impl Drop for EnvVarGuard { @@ -177,9 +190,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 +203,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_os("PATH", raw_path); + + let env = create_env_for_mcp_server(None, &[]); + + assert_eq!(env.get(OsStr::new("PATH")), Some(&expected)); } }