From eaf1fdf0b741dca928d18ea453a6ecde069955b8 Mon Sep 17 00:00:00 2001 From: Curt <38527447+cpjet64@users.noreply.github.com> Date: Thu, 23 Oct 2025 17:20:30 -0400 Subject: [PATCH 1/5] [fix][windows-path]: preserve PATH order; include core env vars Context: * Address open issues related to PATH/PATHEXT and Windows env propagation. * Prepending to PATH changed users' tool precedence (e.g., virtualenv). MCP servers on Windows missed critical env vars (COMSPEC, SYSTEMROOT, etc.). Change: * Append apply_patch temp dir to PATH instead of prepending (arg0, Node wrapper). * Expand Windows DEFAULT_ENV_VARS for MCP client to include COMSPEC, SYSTEMROOT, PROGRAMFILES, APPDATA, etc. Risk: * Low: only affects PATH ordering (preserved) and adds allowed env passthrough. No privilege broadening. Tests: * Existing rmcp-client unit tests pass. Verification: * Ran cargo fmt. * Built and ran tests: `cargo test -p codex-rmcp-client` (all passed). `cargo check -p codex-arg0` OK. --- codex-cli/bin/codex.js | 3 ++- codex-rs/arg0/src/lib.rs | 4 +++- codex-rs/rmcp-client/src/utils.rs | 16 ++++++++++++++++ 3 files changed, 21 insertions(+), 2 deletions(-) diff --git a/codex-cli/bin/codex.js b/codex-cli/bin/codex.js index 805be85af8..fdbf7486ec 100644 --- a/codex-cli/bin/codex.js +++ b/codex-cli/bin/codex.js @@ -73,9 +73,10 @@ const binaryPath = path.join(archRoot, "codex", codexBinaryName); function getUpdatedPath(newDirs) { const pathSep = process.platform === "win32" ? ";" : ":"; const existingPath = process.env.PATH || ""; + // Preserve caller PATH precedence by appending any additional dirs. const updatedPath = [ - ...newDirs, ...existingPath.split(pathSep).filter(Boolean), + ...newDirs, ].join(pathSep); return updatedPath; } diff --git a/codex-rs/arg0/src/lib.rs b/codex-rs/arg0/src/lib.rs index e70ff2df64..58d2d5b348 100644 --- a/codex-rs/arg0/src/lib.rs +++ b/codex-rs/arg0/src/lib.rs @@ -179,9 +179,11 @@ fn prepend_path_entry_for_apply_patch() -> std::io::Result { const PATH_SEPARATOR: &str = ";"; let path_element = path.display(); + // Append our temp dir to PATH rather than prepending to avoid + // disturbing user PATH ordering (e.g., virtualenvs, shims). let updated_path_env_var = match std::env::var("PATH") { Ok(existing_path) => { - format!("{path_element}{PATH_SEPARATOR}{existing_path}") + format!("{existing_path}{PATH_SEPARATOR}{path_element}") } Err(_) => { format!("{path_element}") diff --git a/codex-rs/rmcp-client/src/utils.rs b/codex-rs/rmcp-client/src/utils.rs index 17e050cb6f..9321ab5255 100644 --- a/codex-rs/rmcp-client/src/utils.rs +++ b/codex-rs/rmcp-client/src/utils.rs @@ -171,11 +171,27 @@ pub(crate) const DEFAULT_ENV_VARS: &[&str] = &[ #[cfg(windows)] pub(crate) const DEFAULT_ENV_VARS: &[&str] = &[ + // Core path resolution "PATH", "PATHEXT", + // Shell and system roots + "COMSPEC", + "SYSTEMROOT", + "SYSTEMDRIVE", + // User context and profiles "USERNAME", "USERDOMAIN", "USERPROFILE", + "HOMEDRIVE", + "HOMEPATH", + // Program locations + "PROGRAMFILES", + "PROGRAMFILES(X86)", + "PROGRAMDATA", + // App data and caches + "LOCALAPPDATA", + "APPDATA", + // Temp locations "TEMP", "TMP", ]; From 12ad73089632dce6541f04d42ae240e838b6027b Mon Sep 17 00:00:00 2001 From: Curt <38527447+cpjet64@users.noreply.github.com> Date: Thu, 23 Oct 2025 17:39:17 -0400 Subject: [PATCH 2/5] [fix][windows-env]: add Path alias and common shell vars Context: * Some Windows programs (incl. MCP servers/plugins) read canonical `Path` key. * Users had to manually set PWSH/POWERSHELL; include when present. Change: * In rmcp-client env builder, ensure `Path` mirrors `PATH` on Windows. * Add PROGRAMW6432, POWERSHELL, PWSH to the default pass-through list. Risk: * Low; only adds env keys when available. No privilege expansion. Tests: * Added Windows unit test verifying PATH/Path aliasing. Verification: * cargo fmt; cargo clippy -p codex-rmcp-client; cargo test -p codex-rmcp-client (all ok). --- codex-rs/rmcp-client/src/utils.rs | 32 +++++++++++++++++++++++++++++-- 1 file changed, 30 insertions(+), 2 deletions(-) diff --git a/codex-rs/rmcp-client/src/utils.rs b/codex-rs/rmcp-client/src/utils.rs index 9321ab5255..7d4c54d068 100644 --- a/codex-rs/rmcp-client/src/utils.rs +++ b/codex-rs/rmcp-client/src/utils.rs @@ -76,13 +76,27 @@ pub(crate) fn create_env_for_mcp_server( extra_env: Option>, env_vars: &[String], ) -> HashMap { - DEFAULT_ENV_VARS + let mut map: 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))) .chain(extra_env.unwrap_or_default()) - .collect() + .collect(); + + #[cfg(windows)] + { + // Some Windows programs look up the canonical-cased key `Path`. + // Ensure both `PATH` and `Path` are present with identical values. + if let Some(path_val) = map.get("PATH").cloned() { + map.entry("Path".to_string()).or_insert(path_val); + } else if let Ok(system_path) = env::var("PATH") { + map.insert("PATH".to_string(), system_path.clone()); + map.entry("Path".to_string()).or_insert(system_path); + } + } + + map } pub(crate) fn build_default_headers( @@ -187,6 +201,7 @@ pub(crate) const DEFAULT_ENV_VARS: &[&str] = &[ // Program locations "PROGRAMFILES", "PROGRAMFILES(X86)", + "PROGRAMW6432", "PROGRAMDATA", // App data and caches "LOCALAPPDATA", @@ -194,6 +209,9 @@ pub(crate) const DEFAULT_ENV_VARS: &[&str] = &[ // Temp locations "TEMP", "TMP", + // Common shells/pwsh hints + "POWERSHELL", + "PWSH", ]; #[cfg(test)] @@ -239,6 +257,16 @@ mod tests { } } + #[test] + #[cfg(windows)] + fn windows_path_alias_is_present() { + let _guard = EnvVarGuard::set("PATH", r"C:\\Windows;C:\\Windows\\System32"); + let env = create_env_for_mcp_server(None, &[]); + assert!(env.contains_key("PATH")); + assert!(env.contains_key("Path")); + assert_eq!(env.get("PATH"), env.get("Path")); + } + #[tokio::test] async fn create_env_honors_overrides() { let value = "custom".to_string(); From 2fcff53f25d105d719ea8c7d00d5d50a48ac435c Mon Sep 17 00:00:00 2001 From: Curt <38527447+cpjet64@users.noreply.github.com> Date: Thu, 23 Oct 2025 17:48:29 -0400 Subject: [PATCH 3/5] [chore][lint]: avoid unused_mut on non-Windows; drop redundant clone - Conditionally declare `mut` only on Windows in env builder. - Remove unnecessary `clone()` flagged by clippy. Validated with: cargo clippy -p codex-rmcp-client -D warnings. --- codex-rs/rmcp-client/src/utils.rs | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/codex-rs/rmcp-client/src/utils.rs b/codex-rs/rmcp-client/src/utils.rs index 7d4c54d068..5d69ad30b3 100644 --- a/codex-rs/rmcp-client/src/utils.rs +++ b/codex-rs/rmcp-client/src/utils.rs @@ -76,6 +76,7 @@ pub(crate) fn create_env_for_mcp_server( extra_env: Option>, env_vars: &[String], ) -> HashMap { + #[cfg(windows)] let mut map: HashMap = DEFAULT_ENV_VARS .iter() .copied() @@ -84,6 +85,15 @@ pub(crate) fn create_env_for_mcp_server( .chain(extra_env.unwrap_or_default()) .collect(); + #[cfg(not(windows))] + let map: 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))) + .chain(extra_env.unwrap_or_default()) + .collect(); + #[cfg(windows)] { // Some Windows programs look up the canonical-cased key `Path`. From e2d234f42ff91d0407e7b7d169bb34b24714b70e Mon Sep 17 00:00:00 2001 From: Curt <38527447+cpjet64@users.noreply.github.com> Date: Thu, 23 Oct 2025 18:06:33 -0400 Subject: [PATCH 4/5] [fix][windows-path]: mirror Path <-> PATH consistently - On Windows, if only `Path` is provided (extra env/whitelist), ensure `PATH` is set to the same value (and vice versa). Avoids HashMap ordering races. Validated: clippy clean, rmcp-client tests pass. --- codex-rs/rmcp-client/src/utils.rs | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) diff --git a/codex-rs/rmcp-client/src/utils.rs b/codex-rs/rmcp-client/src/utils.rs index 5d69ad30b3..348c955ab8 100644 --- a/codex-rs/rmcp-client/src/utils.rs +++ b/codex-rs/rmcp-client/src/utils.rs @@ -96,13 +96,14 @@ pub(crate) fn create_env_for_mcp_server( #[cfg(windows)] { - // Some Windows programs look up the canonical-cased key `Path`. - // Ensure both `PATH` and `Path` are present with identical values. - if let Some(path_val) = map.get("PATH").cloned() { - map.entry("Path".to_string()).or_insert(path_val); - } else if let Ok(system_path) = env::var("PATH") { - map.insert("PATH".to_string(), system_path.clone()); - map.entry("Path".to_string()).or_insert(system_path); + if let Some(v) = map + .get("PATH") + .cloned() + .or_else(|| map.get("Path").cloned()) + .or_else(|| env::var("PATH").ok()) + { + map.insert("PATH".to_string(), v.clone()); + map.insert("Path".to_string(), v); } } From ef3220c9e1be741dcce86e90d8bb35c4ce09e76d Mon Sep 17 00:00:00 2001 From: cpjet64 Date: Tue, 28 Oct 2025 21:21:22 -0400 Subject: [PATCH 5/5] =?UTF-8?q?[refactor][split]:=20path-precedence-only?= =?UTF-8?q?=20branch=20=E2=80=94=20revert=20rmcp-client=20utils=20to=20ups?= =?UTF-8?q?tream\n\nKeep=20only=20PATH=20precedence=20change=20in=20arg0?= =?UTF-8?q?=20and=20Node=20wrapper;=20move=20env=20var/alias=20changes=20o?= =?UTF-8?q?ut=20of=20this=20branch.?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- codex-rs/rmcp-client/src/utils.rs | 59 ++----------------------------- 1 file changed, 2 insertions(+), 57 deletions(-) diff --git a/codex-rs/rmcp-client/src/utils.rs b/codex-rs/rmcp-client/src/utils.rs index 348c955ab8..17e050cb6f 100644 --- a/codex-rs/rmcp-client/src/utils.rs +++ b/codex-rs/rmcp-client/src/utils.rs @@ -76,38 +76,13 @@ pub(crate) fn create_env_for_mcp_server( extra_env: Option>, env_vars: &[String], ) -> HashMap { - #[cfg(windows)] - let mut map: HashMap = DEFAULT_ENV_VARS + 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))) .chain(extra_env.unwrap_or_default()) - .collect(); - - #[cfg(not(windows))] - let map: 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))) - .chain(extra_env.unwrap_or_default()) - .collect(); - - #[cfg(windows)] - { - if let Some(v) = map - .get("PATH") - .cloned() - .or_else(|| map.get("Path").cloned()) - .or_else(|| env::var("PATH").ok()) - { - map.insert("PATH".to_string(), v.clone()); - map.insert("Path".to_string(), v); - } - } - - map + .collect() } pub(crate) fn build_default_headers( @@ -196,33 +171,13 @@ pub(crate) const DEFAULT_ENV_VARS: &[&str] = &[ #[cfg(windows)] pub(crate) const DEFAULT_ENV_VARS: &[&str] = &[ - // Core path resolution "PATH", "PATHEXT", - // Shell and system roots - "COMSPEC", - "SYSTEMROOT", - "SYSTEMDRIVE", - // User context and profiles "USERNAME", "USERDOMAIN", "USERPROFILE", - "HOMEDRIVE", - "HOMEPATH", - // Program locations - "PROGRAMFILES", - "PROGRAMFILES(X86)", - "PROGRAMW6432", - "PROGRAMDATA", - // App data and caches - "LOCALAPPDATA", - "APPDATA", - // Temp locations "TEMP", "TMP", - // Common shells/pwsh hints - "POWERSHELL", - "PWSH", ]; #[cfg(test)] @@ -268,16 +223,6 @@ mod tests { } } - #[test] - #[cfg(windows)] - fn windows_path_alias_is_present() { - let _guard = EnvVarGuard::set("PATH", r"C:\\Windows;C:\\Windows\\System32"); - let env = create_env_for_mcp_server(None, &[]); - assert!(env.contains_key("PATH")); - assert!(env.contains_key("Path")); - assert_eq!(env.get("PATH"), env.get("Path")); - } - #[tokio::test] async fn create_env_honors_overrides() { let value = "custom".to_string();