From 050b9baeb6e0e5b49d4e5a578653954bb1857d75 Mon Sep 17 00:00:00 2001 From: eddy-win Date: Wed, 20 Aug 2025 16:30:34 -0700 Subject: [PATCH] Bridge command generation to powershell when on Windows (#2319) ## What? Why? How? - When running on Windows, codex often tries to invoke bash commands, which commonly fail (unless WSL is installed) - Fix: Detect if powershell is available and, if so, route commands to it - Also add a shell_name property to environmental context for codex to default to powershell commands when running in that environment ## Testing - Tested within WSL and powershell (e.g. get top 5 largest files within a folder and validated that commands generated were powershell commands) - Tested within Zsh - Updated unit tests --------- Co-authored-by: Eddy Escardo --- codex-rs/Cargo.lock | 19 +++ codex-rs/core/Cargo.toml | 3 + codex-rs/core/src/codex.rs | 20 ++- codex-rs/core/src/environment_context.rs | 7 + codex-rs/core/src/shell.rs | 205 ++++++++++++++++++++++- codex-rs/core/tests/prompt_caching.rs | 20 ++- 6 files changed, 259 insertions(+), 15 deletions(-) diff --git a/codex-rs/Cargo.lock b/codex-rs/Cargo.lock index d6b98fcd..825b2a48 100644 --- a/codex-rs/Cargo.lock +++ b/codex-rs/Cargo.lock @@ -737,6 +737,7 @@ dependencies = [ "tree-sitter-bash", "uuid", "walkdir", + "which", "whoami", "wildmatch", "wiremock", @@ -5599,6 +5600,18 @@ version = "0.1.10" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "a751b3277700db47d3e574514de2eced5e54dc8a5436a3bf7a0b248b2cee16f3" +[[package]] +name = "which" +version = "6.0.3" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "b4ee928febd44d98f2f459a4a79bd4d928591333a494a10a868418ac1b39cf1f" +dependencies = [ + "either", + "home", + "rustix 0.38.44", + "winsafe", +] + [[package]] name = "whoami" version = "1.6.0" @@ -6011,6 +6024,12 @@ dependencies = [ "memchr", ] +[[package]] +name = "winsafe" +version = "0.0.19" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "d135d17ab770252ad95e9a872d365cf3090e3be864a34ab46f48555993efc904" + [[package]] name = "wiremock" version = "0.6.4" diff --git a/codex-rs/core/Cargo.toml b/codex-rs/core/Cargo.toml index 52c731ae..56815ba0 100644 --- a/codex-rs/core/Cargo.toml +++ b/codex-rs/core/Cargo.toml @@ -71,6 +71,9 @@ openssl-sys = { version = "*", features = ["vendored"] } [target.aarch64-unknown-linux-musl.dependencies] openssl-sys = { version = "*", features = ["vendored"] } +[target.'cfg(target_os = "windows")'.dependencies] +which = "6" + [dev-dependencies] assert_cmd = "2" core_test_support = { path = "tests/common" } diff --git a/codex-rs/core/src/codex.rs b/codex-rs/core/src/codex.rs index 57719f52..dfa10398 100644 --- a/codex-rs/core/src/codex.rs +++ b/codex-rs/core/src/codex.rs @@ -511,6 +511,7 @@ impl Session { turn_context.cwd.to_path_buf(), turn_context.approval_policy, turn_context.sandbox_policy.clone(), + sess.user_shell.clone(), ))); sess.record_conversation_items(&conversation_items).await; @@ -1070,6 +1071,7 @@ async fn submission_loop( new_cwd, new_approval_policy, new_sandbox_policy, + sess.user_shell.clone(), ))]) .await; } @@ -2051,18 +2053,20 @@ pub struct ExecInvokeArgs<'a> { pub stdout_stream: Option, } -fn maybe_run_with_user_profile( +fn maybe_translate_shell_command( params: ExecParams, sess: &Session, turn_context: &TurnContext, ) -> ExecParams { - if turn_context.shell_environment_policy.use_profile { - let command = sess + let should_translate = matches!(sess.user_shell, crate::shell::Shell::PowerShell(_)) + || turn_context.shell_environment_policy.use_profile; + + if should_translate + && let Some(command) = sess .user_shell - .format_default_shell_invocation(params.command.clone()); - if let Some(command) = command { - return ExecParams { command, ..params }; - } + .format_default_shell_invocation(params.command.clone()) + { + return ExecParams { command, ..params }; } params } @@ -2227,7 +2231,7 @@ async fn handle_container_exec_with_params( ), }; - let params = maybe_run_with_user_profile(params, sess, turn_context); + let params = maybe_translate_shell_command(params, sess, turn_context); let output_result = sess .run_exec_with_events( turn_diff_tracker, diff --git a/codex-rs/core/src/environment_context.rs b/codex-rs/core/src/environment_context.rs index b37645a3..a5a9b855 100644 --- a/codex-rs/core/src/environment_context.rs +++ b/codex-rs/core/src/environment_context.rs @@ -6,6 +6,7 @@ use crate::models::ContentItem; use crate::models::ResponseItem; use crate::protocol::AskForApproval; use crate::protocol::SandboxPolicy; +use crate::shell::Shell; use codex_protocol::config_types::SandboxMode; use std::fmt::Display; use std::path::PathBuf; @@ -28,6 +29,7 @@ pub(crate) struct EnvironmentContext { pub approval_policy: AskForApproval, pub sandbox_mode: SandboxMode, pub network_access: NetworkAccess, + pub shell: Shell, } impl EnvironmentContext { @@ -35,6 +37,7 @@ impl EnvironmentContext { cwd: PathBuf, approval_policy: AskForApproval, sandbox_policy: SandboxPolicy, + shell: Shell, ) -> Self { Self { cwd, @@ -55,6 +58,7 @@ impl EnvironmentContext { } } }, + shell, } } } @@ -69,6 +73,9 @@ impl Display for EnvironmentContext { writeln!(f, "Approval policy: {}", self.approval_policy)?; writeln!(f, "Sandbox mode: {}", self.sandbox_mode)?; writeln!(f, "Network access: {}", self.network_access)?; + if let Some(shell_name) = self.shell.name() { + writeln!(f, "Shell: {shell_name}")?; + } Ok(()) } } diff --git a/codex-rs/core/src/shell.rs b/codex-rs/core/src/shell.rs index c269b87e..3a69874e 100644 --- a/codex-rs/core/src/shell.rs +++ b/codex-rs/core/src/shell.rs @@ -1,14 +1,24 @@ +use serde::Deserialize; +use serde::Serialize; use shlex; +use std::path::PathBuf; -#[derive(Debug, PartialEq, Eq)] +#[derive(Debug, PartialEq, Eq, Clone, Serialize, Deserialize)] pub struct ZshShell { shell_path: String, zshrc_path: String, } -#[derive(Debug, PartialEq, Eq)] +#[derive(Debug, PartialEq, Eq, Clone, Serialize, Deserialize)] +pub struct PowerShellConfig { + exe: String, // Executable name or path, e.g. "pwsh" or "powershell.exe". + bash_exe_fallback: Option, // In case the model generates a bash command. +} + +#[derive(Debug, PartialEq, Eq, Clone, Serialize, Deserialize)] pub enum Shell { Zsh(ZshShell), + PowerShell(PowerShellConfig), Unknown, } @@ -33,6 +43,61 @@ impl Shell { } Some(result) } + Shell::PowerShell(ps) => { + // If model generated a bash command, prefer a detected bash fallback + if let Some(script) = strip_bash_lc(&command) { + return match &ps.bash_exe_fallback { + Some(bash) => Some(vec![ + bash.to_string_lossy().to_string(), + "-lc".to_string(), + script, + ]), + + // No bash fallback → run the script under PowerShell. + // It will likely fail (except for some simple commands), but the error + // should give a clue to the model to fix upon retry that it's running under PowerShell. + None => Some(vec![ + ps.exe.clone(), + "-NoProfile".to_string(), + "-Command".to_string(), + script, + ]), + }; + } + + // Not a bash command. If model did not generate a PowerShell command, + // turn it into a PowerShell command. + let first = command.first().map(String::as_str); + if first != Some(ps.exe.as_str()) { + // TODO (CODEX_2900): Handle escaping newlines. + if command.iter().any(|a| a.contains('\n') || a.contains('\r')) { + return Some(command); + } + + let joined = shlex::try_join(command.iter().map(|s| s.as_str())).ok(); + return joined.map(|arg| { + vec![ + ps.exe.clone(), + "-NoProfile".to_string(), + "-Command".to_string(), + arg, + ] + }); + } + + // Model generated a PowerShell command. Run it. + Some(command) + } + Shell::Unknown => None, + } + } + + pub fn name(&self) -> Option { + match self { + Shell::Zsh(zsh) => std::path::Path::new(&zsh.shell_path) + .file_name() + .map(|s| s.to_string_lossy().to_string()), + Shell::PowerShell(ps) => Some(ps.exe.clone()), Shell::Unknown => None, } } @@ -86,11 +151,51 @@ pub async fn default_user_shell() -> Shell { } } -#[cfg(not(target_os = "macos"))] +#[cfg(all(not(target_os = "macos"), not(target_os = "windows")))] pub async fn default_user_shell() -> Shell { Shell::Unknown } +#[cfg(target_os = "windows")] +pub async fn default_user_shell() -> Shell { + use tokio::process::Command; + + // Prefer PowerShell 7+ (`pwsh`) if available, otherwise fall back to Windows PowerShell. + let has_pwsh = Command::new("pwsh") + .arg("-NoLogo") + .arg("-NoProfile") + .arg("-Command") + .arg("$PSVersionTable.PSVersion.Major") + .output() + .await + .map(|o| o.status.success()) + .unwrap_or(false); + let bash_exe = if Command::new("bash.exe") + .arg("--version") + .output() + .await + .ok() + .map(|o| o.status.success()) + .unwrap_or(false) + { + which::which("bash.exe").ok() + } else { + None + }; + + if has_pwsh { + Shell::PowerShell(PowerShellConfig { + exe: "pwsh.exe".to_string(), + bash_exe_fallback: bash_exe, + }) + } else { + Shell::PowerShell(PowerShellConfig { + exe: "powershell.exe".to_string(), + bash_exe_fallback: bash_exe, + }) + } +} + #[cfg(test)] #[cfg(target_os = "macos")] mod tests { @@ -231,3 +336,97 @@ mod tests { } } } + +#[cfg(test)] +#[cfg(target_os = "windows")] +mod tests_windows { + use super::*; + + #[test] + fn test_format_default_shell_invocation_powershell() { + let cases = vec![ + ( + Shell::PowerShell(PowerShellConfig { + exe: "pwsh.exe".to_string(), + bash_exe_fallback: None, + }), + vec!["bash", "-lc", "echo hello"], + vec!["pwsh.exe", "-NoProfile", "-Command", "echo hello"], + ), + ( + Shell::PowerShell(PowerShellConfig { + exe: "powershell.exe".to_string(), + bash_exe_fallback: None, + }), + vec!["bash", "-lc", "echo hello"], + vec!["powershell.exe", "-NoProfile", "-Command", "echo hello"], + ), + ( + Shell::PowerShell(PowerShellConfig { + exe: "pwsh.exe".to_string(), + bash_exe_fallback: Some(PathBuf::from("bash.exe")), + }), + vec!["bash", "-lc", "echo hello"], + vec!["bash.exe", "-lc", "echo hello"], + ), + ( + Shell::PowerShell(PowerShellConfig { + exe: "pwsh.exe".to_string(), + bash_exe_fallback: Some(PathBuf::from("bash.exe")), + }), + vec![ + "bash", + "-lc", + "apply_patch <<'EOF'\n*** Begin Patch\n*** Update File: destination_file.txt\n-original content\n+modified content\n*** End Patch\nEOF", + ], + vec![ + "bash.exe", + "-lc", + "apply_patch <<'EOF'\n*** Begin Patch\n*** Update File: destination_file.txt\n-original content\n+modified content\n*** End Patch\nEOF", + ], + ), + ( + Shell::PowerShell(PowerShellConfig { + exe: "pwsh.exe".to_string(), + bash_exe_fallback: Some(PathBuf::from("bash.exe")), + }), + vec!["echo", "hello"], + vec!["pwsh.exe", "-NoProfile", "-Command", "echo hello"], + ), + ( + Shell::PowerShell(PowerShellConfig { + exe: "pwsh.exe".to_string(), + bash_exe_fallback: Some(PathBuf::from("bash.exe")), + }), + vec!["pwsh.exe", "-NoProfile", "-Command", "echo hello"], + vec!["pwsh.exe", "-NoProfile", "-Command", "echo hello"], + ), + ( + // TODO (CODEX_2900): Handle escaping newlines for powershell invocation. + Shell::PowerShell(PowerShellConfig { + exe: "powershell.exe".to_string(), + bash_exe_fallback: Some(PathBuf::from("bash.exe")), + }), + vec![ + "codex-mcp-server.exe", + "--codex-run-as-apply-patch", + "*** Begin Patch\n*** Update File: C:\\Users\\person\\destination_file.txt\n-original content\n+modified content\n*** End Patch", + ], + vec![ + "codex-mcp-server.exe", + "--codex-run-as-apply-patch", + "*** Begin Patch\n*** Update File: C:\\Users\\person\\destination_file.txt\n-original content\n+modified content\n*** End Patch", + ], + ), + ]; + + for (shell, input, expected_cmd) in cases { + let actual_cmd = shell + .format_default_shell_invocation(input.iter().map(|s| s.to_string()).collect()); + assert_eq!( + actual_cmd, + Some(expected_cmd.iter().map(|s| s.to_string()).collect()) + ); + } + } +} diff --git a/codex-rs/core/tests/prompt_caching.rs b/codex-rs/core/tests/prompt_caching.rs index 9f5829e1..75b7691b 100644 --- a/codex-rs/core/tests/prompt_caching.rs +++ b/codex-rs/core/tests/prompt_caching.rs @@ -8,6 +8,7 @@ use codex_core::protocol::Op; use codex_core::protocol::SandboxPolicy; use codex_core::protocol_config_types::ReasoningEffort; use codex_core::protocol_config_types::ReasoningSummary; +use codex_core::shell::default_user_shell; use codex_login::CodexAuth; use core_test_support::load_default_config_for_test; use core_test_support::load_sse_fixture_with_id; @@ -85,9 +86,15 @@ async fn prefixes_context_and_instructions_once_and_consistently_across_requests let requests = server.received_requests().await.unwrap(); assert_eq!(requests.len(), 2, "expected two POST requests"); + let shell = default_user_shell().await; + let expected_env_text = format!( - "\nCurrent working directory: {}\nApproval policy: on-request\nSandbox mode: read-only\nNetwork access: restricted\n", - cwd.path().to_string_lossy() + "\nCurrent working directory: {}\nApproval policy: on-request\nSandbox mode: read-only\nNetwork access: restricted\n{}", + cwd.path().to_string_lossy(), + match shell.name() { + Some(name) => format!("Shell: {name}\n"), + None => String::new(), + } ); let expected_ui_text = "\n\nbe consistent and helpful\n\n"; @@ -237,9 +244,14 @@ async fn overrides_turn_context_but_keeps_cached_prefix_and_key_constant() { }); // After overriding the turn context, the environment context should be emitted again // reflecting the new cwd, approval policy and sandbox settings. + let shell = default_user_shell().await; let expected_env_text_2 = format!( - "\nCurrent working directory: {}\nApproval policy: never\nSandbox mode: workspace-write\nNetwork access: enabled\n", - new_cwd.path().to_string_lossy() + "\nCurrent working directory: {}\nApproval policy: never\nSandbox mode: workspace-write\nNetwork access: enabled\n{}", + new_cwd.path().to_string_lossy(), + match shell.name() { + Some(name) => format!("Shell: {name}\n"), + None => String::new(), + } ); let expected_env_msg_2 = serde_json::json!({ "type": "message",