From e6995174c10cd889f6ba88e246cf2e0e2493bfee Mon Sep 17 00:00:00 2001 From: pakrym-oai Date: Thu, 13 Nov 2025 08:43:00 -0800 Subject: [PATCH] Revert "Overhaul shell detection and centralize command generation for unified exec" (#6606) Reverts openai/codex#6577 --- codex-rs/core/src/bash.rs | 24 +- .../src/command_safety/is_safe_command.rs | 10 +- codex-rs/core/src/command_safety/mod.rs | 1 + codex-rs/core/src/environment_context.rs | 2 + codex-rs/core/src/shell.rs | 551 +++++++++++------- codex-rs/core/src/tools/events.rs | 8 +- codex-rs/core/src/tools/handlers/shell.rs | 23 +- .../core/src/tools/handlers/unified_exec.rs | 22 +- codex-rs/core/src/unified_exec/mod.rs | 12 +- .../core/src/unified_exec/session_manager.rs | 26 +- codex-rs/core/tests/suite/unified_exec.rs | 15 +- 11 files changed, 411 insertions(+), 283 deletions(-) diff --git a/codex-rs/core/src/bash.rs b/codex-rs/core/src/bash.rs index 0ffb8e785..edeebaba3 100644 --- a/codex-rs/core/src/bash.rs +++ b/codex-rs/core/src/bash.rs @@ -1,13 +1,8 @@ -use std::path::PathBuf; - use tree_sitter::Node; use tree_sitter::Parser; use tree_sitter::Tree; use tree_sitter_bash::LANGUAGE as BASH; -use crate::shell::ShellType; -use crate::shell::detect_shell_type; - /// Parse the provided bash source using tree-sitter-bash, returning a Tree on /// success or None if parsing failed. pub fn try_parse_shell(shell_lc_arg: &str) -> Option { @@ -93,16 +88,23 @@ pub fn try_parse_word_only_commands_sequence(tree: &Tree, src: &str) -> Option bool { + if shell == "bash" || shell == "zsh" { + return true; + } + + let shell_name = std::path::Path::new(shell) + .file_name() + .and_then(|s| s.to_str()) + .unwrap_or(shell); + matches!(shell_name, "bash" | "zsh") +} + pub fn extract_bash_command(command: &[String]) -> Option<(&str, &str)> { let [shell, flag, script] = command else { return None; }; - if !matches!(flag.as_str(), "-lc" | "-c") - || !matches!( - detect_shell_type(&PathBuf::from(shell)), - Some(ShellType::Zsh) | Some(ShellType::Bash) - ) - { + if !matches!(flag.as_str(), "-lc" | "-c") || !is_well_known_sh_shell(shell) { return None; } Some((shell, script)) diff --git a/codex-rs/core/src/command_safety/is_safe_command.rs b/codex-rs/core/src/command_safety/is_safe_command.rs index ab084c191..ba3eb6ba7 100644 --- a/codex-rs/core/src/command_safety/is_safe_command.rs +++ b/codex-rs/core/src/command_safety/is_safe_command.rs @@ -1,5 +1,4 @@ use crate::bash::parse_shell_lc_plain_commands; -use crate::command_safety::windows_safe_commands::is_safe_command_windows; pub fn is_known_safe_command(command: &[String]) -> bool { let command: Vec = command @@ -12,9 +11,12 @@ pub fn is_known_safe_command(command: &[String]) -> bool { } }) .collect(); - - if is_safe_command_windows(&command) { - return true; + #[cfg(target_os = "windows")] + { + use super::windows_safe_commands::is_safe_command_windows; + if is_safe_command_windows(&command) { + return true; + } } if is_safe_to_call_with_exec(&command) { diff --git a/codex-rs/core/src/command_safety/mod.rs b/codex-rs/core/src/command_safety/mod.rs index caf5c9f6e..8279d6795 100644 --- a/codex-rs/core/src/command_safety/mod.rs +++ b/codex-rs/core/src/command_safety/mod.rs @@ -1,3 +1,4 @@ pub mod is_dangerous_command; pub mod is_safe_command; +#[cfg(target_os = "windows")] pub mod windows_safe_commands; diff --git a/codex-rs/core/src/environment_context.rs b/codex-rs/core/src/environment_context.rs index 6db2bb8f5..e7b2e19ff 100644 --- a/codex-rs/core/src/environment_context.rs +++ b/codex-rs/core/src/environment_context.rs @@ -329,6 +329,7 @@ mod tests { Some(workspace_write_policy(vec!["/repo"], false)), Some(Shell::Bash(BashShell { shell_path: "/bin/bash".into(), + bashrc_path: "/home/user/.bashrc".into(), })), ); let context2 = EnvironmentContext::new( @@ -337,6 +338,7 @@ mod tests { Some(workspace_write_policy(vec!["/repo"], false)), Some(Shell::Zsh(ZshShell { shell_path: "/bin/zsh".into(), + zshrc_path: "/home/user/.zshrc".into(), })), ); diff --git a/codex-rs/core/src/shell.rs b/codex-rs/core/src/shell.rs index 60c1b4123..90f6c6b97 100644 --- a/codex-rs/core/src/shell.rs +++ b/codex-rs/core/src/shell.rs @@ -2,26 +2,22 @@ use serde::Deserialize; use serde::Serialize; use std::path::PathBuf; -#[derive(Debug, PartialEq, Eq, Clone, Serialize, Deserialize)] -pub enum ShellType { - Zsh, - Bash, - PowerShell, -} - #[derive(Debug, PartialEq, Eq, Clone, Serialize, Deserialize)] pub struct ZshShell { - pub(crate) shell_path: PathBuf, + pub(crate) shell_path: String, + pub(crate) zshrc_path: String, } #[derive(Debug, PartialEq, Eq, Clone, Serialize, Deserialize)] pub struct BashShell { - pub(crate) shell_path: PathBuf, + pub(crate) shell_path: String, + pub(crate) bashrc_path: String, } #[derive(Debug, PartialEq, Eq, Clone, Serialize, Deserialize)] pub struct PowerShellConfig { - pub(crate) shell_path: PathBuf, // Executable name or path, e.g. "pwsh" or "powershell.exe". + pub(crate) exe: String, // Executable name or path, e.g. "pwsh" or "powershell.exe". + pub(crate) bash_exe_fallback: Option, // In case the model generates a bash command. } #[derive(Debug, PartialEq, Eq, Clone, Serialize, Deserialize)] @@ -40,10 +36,7 @@ impl Shell { .file_name() .map(|s| s.to_string_lossy().to_string()) } - Shell::PowerShell(ps) => ps - .shell_path - .file_stem() - .map(|s| s.to_string_lossy().to_string()), + Shell::PowerShell(ps) => Some(ps.exe.clone()), Shell::Unknown => None, } } @@ -54,17 +47,10 @@ impl Shell { match self { Shell::Zsh(ZshShell { shell_path, .. }) | Shell::Bash(BashShell { shell_path, .. }) => { let arg = if use_login_shell { "-lc" } else { "-c" }; - vec![ - shell_path.to_string_lossy().to_string(), - arg.to_string(), - command.to_string(), - ] + vec![shell_path.clone(), arg.to_string(), command.to_string()] } Shell::PowerShell(ps) => { - let mut args = vec![ - ps.shell_path.to_string_lossy().to_string(), - "-NoLogo".to_string(), - ]; + let mut args = vec![ps.exe.clone(), "-NoLogo".to_string()]; if !use_login_shell { args.push("-NoProfile".to_string()); } @@ -79,7 +65,7 @@ impl Shell { } #[cfg(unix)] -fn get_user_shell_path() -> Option { +fn detect_default_user_shell() -> Shell { use libc::getpwuid; use libc::getuid; use std::ffi::CStr; @@ -92,164 +78,75 @@ fn get_user_shell_path() -> Option { let shell_path = CStr::from_ptr((*pw).pw_shell) .to_string_lossy() .into_owned(); - Some(PathBuf::from(shell_path)) - } else { - None - } - } -} + let home_path = CStr::from_ptr((*pw).pw_dir).to_string_lossy().into_owned(); -#[cfg(not(unix))] -fn get_user_shell_path() -> Option { - None -} + if shell_path.ends_with("/zsh") { + return Shell::Zsh(ZshShell { + shell_path, + zshrc_path: format!("{home_path}/.zshrc"), + }); + } -fn file_exists(path: &PathBuf) -> Option { - if std::fs::metadata(path).is_ok_and(|metadata| metadata.is_file()) { - Some(PathBuf::from(path)) - } else { - None - } -} - -fn get_shell_path( - shell_type: ShellType, - binary_name: &str, - fallback_paths: Vec<&str>, -) -> Option { - // Check if the shell we are trying to load is user's default shell - // if just use it - - let default_shell_path = get_user_shell_path(); - if let Some(default_shell_path) = default_shell_path - && detect_shell_type(&default_shell_path) == Some(shell_type) - { - return Some(default_shell_path); - } - - if let Ok(path) = which::which(binary_name) { - return Some(path); - } - - for path in fallback_paths { - //check exists - if let Some(path) = file_exists(&PathBuf::from(path)) { - return Some(path); - } - } - - None -} - -fn get_zsh_shell() -> Option { - let shell_path = get_shell_path(ShellType::Zsh, "zsh", vec!["/bin/zsh"]); - - shell_path.map(|shell_path| ZshShell { shell_path }) -} - -fn get_bash_shell() -> Option { - let shell_path = get_shell_path(ShellType::Bash, "bash", vec!["/bin/bash"]); - - shell_path.map(|shell_path| BashShell { shell_path }) -} - -fn get_powershell_shell() -> Option { - let shell_path = get_shell_path(ShellType::PowerShell, "pwsh", vec!["/usr/local/bin/pwsh"]) - .or_else(|| get_shell_path(ShellType::PowerShell, "powershell", vec![])); - - shell_path.map(|shell_path| PowerShellConfig { shell_path }) -} - -pub fn get_shell_by_model_provided_path(shell_path: &PathBuf) -> Shell { - detect_shell_type(shell_path) - .and_then(get_shell) - .unwrap_or(Shell::Unknown) -} - -pub fn get_shell(shell_type: ShellType) -> Option { - match shell_type { - ShellType::Zsh => get_zsh_shell().map(Shell::Zsh), - ShellType::Bash => get_bash_shell().map(Shell::Bash), - ShellType::PowerShell => get_powershell_shell().map(Shell::PowerShell), - } -} - -pub fn detect_shell_type(shell_path: &PathBuf) -> Option { - match shell_path.as_os_str().to_str() { - Some("zsh") => Some(ShellType::Zsh), - Some("bash") => Some(ShellType::Bash), - Some("pwsh") => Some(ShellType::PowerShell), - Some("powershell") => Some(ShellType::PowerShell), - _ => { - let shell_name = shell_path.file_stem(); - - if let Some(shell_name) = shell_name - && shell_name != shell_path - { - detect_shell_type(&PathBuf::from(shell_name)) - } else { - None + if shell_path.ends_with("/bash") { + return Shell::Bash(BashShell { + shell_path, + bashrc_path: format!("{home_path}/.bashrc"), + }); } } } + Shell::Unknown } +#[cfg(unix)] pub async fn default_user_shell() -> Shell { - if cfg!(windows) { - get_shell(ShellType::PowerShell).unwrap_or(Shell::Unknown) + detect_default_user_shell() +} + +#[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") + .stdin(std::process::Stdio::null()) + .output() + .await + .ok() + .map(|o| o.status.success()) + .unwrap_or(false) + { + which::which("bash.exe").ok() } else { - get_user_shell_path() - .and_then(|shell| detect_shell_type(&shell)) - .and_then(get_shell) - .unwrap_or(Shell::Unknown) + 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)] -mod detect_shell_type_tests { - use super::*; - - #[test] - fn test_detect_shell_type() { - assert_eq!( - detect_shell_type(&PathBuf::from("zsh")), - Some(ShellType::Zsh) - ); - assert_eq!( - detect_shell_type(&PathBuf::from("bash")), - Some(ShellType::Bash) - ); - assert_eq!( - detect_shell_type(&PathBuf::from("pwsh")), - Some(ShellType::PowerShell) - ); - assert_eq!( - detect_shell_type(&PathBuf::from("powershell")), - Some(ShellType::PowerShell) - ); - assert_eq!(detect_shell_type(&PathBuf::from("fish")), None); - assert_eq!(detect_shell_type(&PathBuf::from("other")), None); - assert_eq!( - detect_shell_type(&PathBuf::from("/bin/zsh")), - Some(ShellType::Zsh) - ); - assert_eq!( - detect_shell_type(&PathBuf::from("/bin/bash")), - Some(ShellType::Bash) - ); - assert_eq!( - detect_shell_type(&PathBuf::from("powershell.exe")), - Some(ShellType::PowerShell) - ); - assert_eq!( - detect_shell_type(&PathBuf::from("pwsh.exe")), - Some(ShellType::PowerShell) - ); - assert_eq!( - detect_shell_type(&PathBuf::from("/usr/local/bin/pwsh")), - Some(ShellType::PowerShell) - ); - } +#[cfg(all(not(target_os = "windows"), not(unix)))] +pub async fn default_user_shell() -> Shell { + Shell::Unknown } #[cfg(test)] @@ -259,30 +156,6 @@ mod tests { use std::path::PathBuf; use std::process::Command; - #[test] - #[cfg(target_os = "macos")] - fn detects_zsh() { - let zsh_shell = get_shell(ShellType::Zsh).unwrap(); - - let ZshShell { shell_path } = match zsh_shell { - Shell::Zsh(zsh_shell) => zsh_shell, - _ => panic!("expected zsh shell"), - }; - - assert_eq!(shell_path, PathBuf::from("/bin/zsh")); - } - - #[test] - fn detects_bash() { - let bash_shell = get_shell(ShellType::Bash).unwrap(); - let BashShell { shell_path } = match bash_shell { - Shell::Bash(bash_shell) => bash_shell, - _ => panic!("expected bash shell"), - }; - - assert_eq!(shell_path, PathBuf::from("/bin/bash")); - } - #[tokio::test] async fn test_current_shell_detects_zsh() { let shell = Command::new("sh") @@ -291,42 +164,292 @@ mod tests { .output() .unwrap(); + let home = std::env::var("HOME").unwrap(); let shell_path = String::from_utf8_lossy(&shell.stdout).trim().to_string(); if shell_path.ends_with("/zsh") { assert_eq!( default_user_shell().await, Shell::Zsh(ZshShell { - shell_path: PathBuf::from(shell_path), + shell_path: shell_path.to_string(), + zshrc_path: format!("{home}/.zshrc",), }) ); } } + + #[tokio::test] + async fn test_run_with_profile_bash_escaping_and_execution() { + let shell_path = "/bin/bash"; + + let cases = vec![ + ( + vec!["myecho"], + vec![shell_path, "-lc", "source BASHRC_PATH && (myecho)"], + Some("It works!\n"), + ), + ( + vec!["bash", "-lc", "echo 'single' \"double\""], + vec![ + shell_path, + "-lc", + "source BASHRC_PATH && (echo 'single' \"double\")", + ], + Some("single double\n"), + ), + ]; + + for (input, expected_cmd, expected_output) in cases { + use std::collections::HashMap; + + use crate::exec::ExecParams; + use crate::exec::SandboxType; + use crate::exec::process_exec_tool_call; + use crate::protocol::SandboxPolicy; + + let temp_home = tempfile::tempdir().unwrap(); + let bashrc_path = temp_home.path().join(".bashrc"); + std::fs::write( + &bashrc_path, + r#" + set -x + function myecho { + echo 'It works!' + } + "#, + ) + .unwrap(); + let command = expected_cmd + .iter() + .map(|s| s.replace("BASHRC_PATH", bashrc_path.to_str().unwrap())) + .collect::>(); + + let output = process_exec_tool_call( + ExecParams { + command: command.clone(), + cwd: PathBuf::from(temp_home.path()), + timeout_ms: None, + env: HashMap::from([( + "HOME".to_string(), + temp_home.path().to_str().unwrap().to_string(), + )]), + with_escalated_permissions: None, + justification: None, + arg0: None, + }, + SandboxType::None, + &SandboxPolicy::DangerFullAccess, + temp_home.path(), + &None, + None, + ) + .await + .unwrap(); + + assert_eq!(output.exit_code, 0, "input: {input:?} output: {output:?}"); + if let Some(expected) = expected_output { + assert_eq!( + output.stdout.text, expected, + "input: {input:?} output: {output:?}" + ); + } + } + } } #[cfg(test)] -#[cfg(windows)] -mod tests { - use super::*; +#[cfg(target_os = "macos")] +mod macos_tests { + use std::path::PathBuf; #[tokio::test] - async fn detects_powershell_as_default() { - let powershell_shell = default_user_shell().await; - let PowerShellConfig { shell_path } = match powershell_shell { - Shell::PowerShell(powershell_shell) => powershell_shell, - _ => panic!("expected powershell shell"), - }; + async fn test_run_with_profile_escaping_and_execution() { + let shell_path = "/bin/zsh"; - assert!(shell_path.ends_with("pwsh.exe") || shell_path.ends_with("powershell.exe")); - } + let cases = vec![ + ( + vec!["myecho"], + vec![shell_path, "-lc", "source ZSHRC_PATH && (myecho)"], + Some("It works!\n"), + ), + ( + vec!["myecho"], + vec![shell_path, "-lc", "source ZSHRC_PATH && (myecho)"], + Some("It works!\n"), + ), + ( + vec!["bash", "-c", "echo 'single' \"double\""], + vec![ + shell_path, + "-lc", + "source ZSHRC_PATH && (bash -c \"echo 'single' \\\"double\\\"\")", + ], + Some("single double\n"), + ), + ( + vec!["bash", "-lc", "echo 'single' \"double\""], + vec![ + shell_path, + "-lc", + "source ZSHRC_PATH && (echo 'single' \"double\")", + ], + Some("single double\n"), + ), + ]; + for (input, expected_cmd, expected_output) in cases { + use std::collections::HashMap; - #[test] - fn finds_poweshell() { - let powershell_shell = get_shell(ShellType::PowerShell).unwrap(); - let PowerShellConfig { shell_path } = match powershell_shell { - Shell::PowerShell(powershell_shell) => powershell_shell, - _ => panic!("expected powershell shell"), - }; + use crate::exec::ExecParams; + use crate::exec::SandboxType; + use crate::exec::process_exec_tool_call; + use crate::protocol::SandboxPolicy; - assert!(shell_path.ends_with("pwsh.exe") || shell_path.ends_with("powershell.exe")); + let temp_home = tempfile::tempdir().unwrap(); + let zshrc_path = temp_home.path().join(".zshrc"); + std::fs::write( + &zshrc_path, + r#" + set -x + function myecho { + echo 'It works!' + } + "#, + ) + .unwrap(); + let command = expected_cmd + .iter() + .map(|s| s.replace("ZSHRC_PATH", zshrc_path.to_str().unwrap())) + .collect::>(); + + let output = process_exec_tool_call( + ExecParams { + command: command.clone(), + cwd: PathBuf::from(temp_home.path()), + timeout_ms: None, + env: HashMap::from([( + "HOME".to_string(), + temp_home.path().to_str().unwrap().to_string(), + )]), + with_escalated_permissions: None, + justification: None, + arg0: None, + }, + SandboxType::None, + &SandboxPolicy::DangerFullAccess, + temp_home.path(), + &None, + None, + ) + .await + .unwrap(); + + assert_eq!(output.exit_code, 0, "input: {input:?} output: {output:?}"); + if let Some(expected) = expected_output { + assert_eq!( + output.stdout.text, expected, + "input: {input:?} output: {output:?}" + ); + } + } + } +} + +#[cfg(test)] +#[cfg(target_os = "windows")] +mod tests_windows { + use super::*; + + #[test] + fn test_format_default_shell_invocation_powershell() { + use std::path::PathBuf; + + let cases = vec![ + ( + PowerShellConfig { + exe: "pwsh.exe".to_string(), + bash_exe_fallback: None, + }, + vec!["bash", "-lc", "echo hello"], + vec!["pwsh.exe", "-NoProfile", "-Command", "echo hello"], + ), + ( + PowerShellConfig { + exe: "powershell.exe".to_string(), + bash_exe_fallback: None, + }, + vec!["bash", "-lc", "echo hello"], + vec!["powershell.exe", "-NoProfile", "-Command", "echo hello"], + ), + ( + 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"], + ), + ( + 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", + ], + ), + ( + PowerShellConfig { + exe: "pwsh.exe".to_string(), + bash_exe_fallback: Some(PathBuf::from("bash.exe")), + }, + vec!["echo", "hello"], + vec!["pwsh.exe", "-NoProfile", "-Command", "echo hello"], + ), + ( + 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"], + ), + ( + 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 (config, input, expected_cmd) in cases { + let command = expected_cmd + .iter() + .map(|s| (*s).to_string()) + .collect::>(); + + // These tests assert the final command for each scenario now that the helper + // has been removed. The inputs remain to document the original coverage. + let expected = expected_cmd + .iter() + .map(|s| (*s).to_string()) + .collect::>(); + assert_eq!(command, expected, "input: {input:?} config: {config:?}"); + } } } diff --git a/codex-rs/core/src/tools/events.rs b/codex-rs/core/src/tools/events.rs index 5f93d51d8..9019dac5e 100644 --- a/codex-rs/core/src/tools/events.rs +++ b/codex-rs/core/src/tools/events.rs @@ -87,7 +87,7 @@ pub(crate) enum ToolEmitter { auto_approved: bool, }, UnifiedExec { - command: Vec, + command: String, cwd: PathBuf, // True for `exec_command` and false for `write_stdin`. #[allow(dead_code)] @@ -111,9 +111,9 @@ impl ToolEmitter { } } - pub fn unified_exec(command: &[String], cwd: PathBuf, is_startup_command: bool) -> Self { + pub fn unified_exec(command: String, cwd: PathBuf, is_startup_command: bool) -> Self { Self::UnifiedExec { - command: command.to_vec(), + command, cwd, is_startup_command, } @@ -218,7 +218,7 @@ impl ToolEmitter { emit_patch_end(ctx, String::new(), (*message).to_string(), false).await; } (Self::UnifiedExec { command, cwd, .. }, ToolEventStage::Begin) => { - emit_exec_command_begin(ctx, command, cwd.as_path(), false).await; + emit_exec_command_begin(ctx, &[command.to_string()], cwd.as_path(), false).await; } (Self::UnifiedExec { .. }, ToolEventStage::Success(output)) => { emit_exec_end( diff --git a/codex-rs/core/src/tools/handlers/shell.rs b/codex-rs/core/src/tools/handlers/shell.rs index 39f271b2d..3a5115f6e 100644 --- a/codex-rs/core/src/tools/handlers/shell.rs +++ b/codex-rs/core/src/tools/handlers/shell.rs @@ -324,11 +324,8 @@ impl ShellHandler { #[cfg(test)] mod tests { - use std::path::PathBuf; - use crate::is_safe_command::is_known_safe_command; use crate::shell::BashShell; - use crate::shell::PowerShellConfig; use crate::shell::Shell; use crate::shell::ZshShell; @@ -338,19 +335,27 @@ mod tests { #[test] fn commands_generated_by_shell_command_handler_can_be_matched_by_is_known_safe_command() { let bash_shell = Shell::Bash(BashShell { - shell_path: PathBuf::from("/bin/bash"), + shell_path: "/bin/bash".to_string(), + bashrc_path: "/home/user/.bashrc".to_string(), }); assert_safe(&bash_shell, "ls -la"); let zsh_shell = Shell::Zsh(ZshShell { - shell_path: PathBuf::from("/bin/zsh"), + shell_path: "/bin/zsh".to_string(), + zshrc_path: "/home/user/.zshrc".to_string(), }); assert_safe(&zsh_shell, "ls -la"); - let powershell = Shell::PowerShell(PowerShellConfig { - shell_path: PathBuf::from("pwsh.exe"), - }); - assert_safe(&powershell, "ls -Name"); + #[cfg(target_os = "windows")] + { + use crate::shell::PowerShellConfig; + + let powershell = Shell::PowerShell(PowerShellConfig { + exe: "pwsh.exe".to_string(), + bash_exe_fallback: None, + }); + assert_safe(&powershell, "ls -Name"); + } } fn assert_safe(shell: &Shell, command: &str) { diff --git a/codex-rs/core/src/tools/handlers/unified_exec.rs b/codex-rs/core/src/tools/handlers/unified_exec.rs index 6e0596d6e..fb94f23d0 100644 --- a/codex-rs/core/src/tools/handlers/unified_exec.rs +++ b/codex-rs/core/src/tools/handlers/unified_exec.rs @@ -5,7 +5,6 @@ use crate::is_safe_command::is_known_safe_command; use crate::protocol::EventMsg; use crate::protocol::ExecCommandOutputDeltaEvent; use crate::protocol::ExecOutputStream; -use crate::shell::get_shell_by_model_provided_path; use crate::tools::context::ToolInvocation; use crate::tools::context::ToolOutput; use crate::tools::context::ToolPayload; @@ -85,8 +84,7 @@ impl ToolHandler for UnifiedExecHandler { let Ok(params) = serde_json::from_str::(arguments) else { return true; }; - let command = get_command(¶ms); - !is_known_safe_command(&command) + !is_known_safe_command(&["bash".to_string(), "-lc".to_string(), params.cmd]) } async fn handle(&self, invocation: ToolInvocation) -> Result { @@ -119,15 +117,15 @@ impl ToolHandler for UnifiedExecHandler { "failed to parse exec_command arguments: {err:?}" )) })?; - - let command = get_command(&args); let ExecCommandArgs { + cmd, workdir, + shell, + login, yield_time_ms, max_output_tokens, with_escalated_permissions, justification, - .. } = args; if with_escalated_permissions.unwrap_or(false) @@ -154,14 +152,15 @@ impl ToolHandler for UnifiedExecHandler { &context.call_id, None, ); - - let emitter = ToolEmitter::unified_exec(&command, cwd.clone(), true); + let emitter = ToolEmitter::unified_exec(cmd.clone(), cwd.clone(), true); emitter.emit(event_ctx, ToolEventStage::Begin).await; manager .exec_command( ExecCommandRequest { - command, + command: &cmd, + shell: &shell, + login, yield_time_ms, max_output_tokens, workdir, @@ -222,11 +221,6 @@ impl ToolHandler for UnifiedExecHandler { } } -fn get_command(args: &ExecCommandArgs) -> Vec { - let shell = get_shell_by_model_provided_path(&PathBuf::from(args.shell.clone())); - shell.derive_exec_args(&args.cmd, args.login) -} - fn format_response(response: &UnifiedExecResponse) -> String { let mut sections = Vec::new(); diff --git a/codex-rs/core/src/unified_exec/mod.rs b/codex-rs/core/src/unified_exec/mod.rs index 3f7f7978e..b996ce79e 100644 --- a/codex-rs/core/src/unified_exec/mod.rs +++ b/codex-rs/core/src/unified_exec/mod.rs @@ -64,8 +64,10 @@ impl UnifiedExecContext { } #[derive(Debug)] -pub(crate) struct ExecCommandRequest { - pub command: Vec, +pub(crate) struct ExecCommandRequest<'a> { + pub command: &'a str, + pub shell: &'a str, + pub login: bool, pub yield_time_ms: Option, pub max_output_tokens: Option, pub workdir: Option, @@ -103,7 +105,7 @@ struct SessionEntry { session_ref: Arc, turn_ref: Arc, call_id: String, - command: Vec, + command: String, cwd: PathBuf, started_at: tokio::time::Instant, } @@ -195,7 +197,9 @@ mod tests { .unified_exec_manager .exec_command( ExecCommandRequest { - command: vec!["bash".to_string(), "-lc".to_string(), cmd.to_string()], + command: cmd, + shell: "/bin/bash", + login: true, yield_time_ms, max_output_tokens: None, workdir: None, diff --git a/codex-rs/core/src/unified_exec/session_manager.rs b/codex-rs/core/src/unified_exec/session_manager.rs index e3b17d6e9..c9763fbc4 100644 --- a/codex-rs/core/src/unified_exec/session_manager.rs +++ b/codex-rs/core/src/unified_exec/session_manager.rs @@ -36,17 +36,23 @@ use super::truncate_output_to_tokens; impl UnifiedExecSessionManager { pub(crate) async fn exec_command( &self, - request: ExecCommandRequest, + request: ExecCommandRequest<'_>, context: &UnifiedExecContext, ) -> Result { let cwd = request .workdir .clone() .unwrap_or_else(|| context.turn.cwd.clone()); + let shell_flag = if request.login { "-lc" } else { "-c" }; + let command = vec![ + request.shell.to_string(), + shell_flag.to_string(), + request.command.to_string(), + ]; let session = self .open_session_with_sandbox( - &request.command, + command, cwd.clone(), request.with_escalated_permissions, request.justification, @@ -73,7 +79,7 @@ impl UnifiedExecSessionManager { None } else { Some( - self.store_session(session, context, &request.command, cwd.clone(), start) + self.store_session(session, context, request.command, cwd.clone(), start) .await, ) }; @@ -93,7 +99,7 @@ impl UnifiedExecSessionManager { let exit = response.exit_code.unwrap_or(-1); Self::emit_exec_end_from_context( context, - &request.command, + request.command.to_string(), cwd, response.output.clone(), exit, @@ -218,7 +224,7 @@ impl UnifiedExecSessionManager { &self, session: UnifiedExecSession, context: &UnifiedExecContext, - command: &[String], + command: &str, cwd: PathBuf, started_at: Instant, ) -> i32 { @@ -230,7 +236,7 @@ impl UnifiedExecSessionManager { session_ref: Arc::clone(&context.session), turn_ref: Arc::clone(&context.turn), call_id: context.call_id.clone(), - command: command.to_vec(), + command: command.to_string(), cwd, started_at, }; @@ -258,7 +264,7 @@ impl UnifiedExecSessionManager { &entry.call_id, None, ); - let emitter = ToolEmitter::unified_exec(&entry.command, entry.cwd, true); + let emitter = ToolEmitter::unified_exec(entry.command, entry.cwd, true); emitter .emit(event_ctx, ToolEventStage::Success(output)) .await; @@ -266,7 +272,7 @@ impl UnifiedExecSessionManager { async fn emit_exec_end_from_context( context: &UnifiedExecContext, - command: &[String], + command: String, cwd: PathBuf, aggregated_output: String, exit_code: i32, @@ -315,7 +321,7 @@ impl UnifiedExecSessionManager { pub(super) async fn open_session_with_sandbox( &self, - command: &[String], + command: Vec, cwd: PathBuf, with_escalated_permissions: Option, justification: Option, @@ -324,7 +330,7 @@ impl UnifiedExecSessionManager { let mut orchestrator = ToolOrchestrator::new(); let mut runtime = UnifiedExecRuntime::new(self); let req = UnifiedExecToolRequest::new( - command.to_vec(), + command, cwd, create_env(&context.turn.shell_environment_policy), with_escalated_permissions, diff --git a/codex-rs/core/tests/suite/unified_exec.rs b/codex-rs/core/tests/suite/unified_exec.rs index 07b17f78a..1225b51cc 100644 --- a/codex-rs/core/tests/suite/unified_exec.rs +++ b/codex-rs/core/tests/suite/unified_exec.rs @@ -214,11 +214,7 @@ async fn unified_exec_emits_exec_command_begin_event() -> Result<()> { assert_eq!( begin_event.command, - vec![ - "/bin/bash".to_string(), - "-lc".to_string(), - "/bin/echo hello unified exec".to_string() - ] + vec!["/bin/echo hello unified exec".to_string()] ); assert_eq!(begin_event.cwd, cwd.path()); @@ -658,14 +654,7 @@ async fn unified_exec_skips_begin_event_for_empty_input() -> Result<()> { "expected only the initial command to emit begin event" ); assert_eq!(begin_events[0].call_id, open_call_id); - assert_eq!( - begin_events[0].command, - vec![ - "/bin/bash".to_string(), - "-lc".to_string(), - "/bin/sh -c echo ready".to_string() - ] - ); + assert_eq!(begin_events[0].command[0], "/bin/sh -c echo ready"); Ok(()) }