diff --git a/codex-rs/core/src/bash.rs b/codex-rs/core/src/bash.rs index edeebaba3..0ffb8e785 100644 --- a/codex-rs/core/src/bash.rs +++ b/codex-rs/core/src/bash.rs @@ -1,8 +1,13 @@ +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 { @@ -88,23 +93,16 @@ 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") || !is_well_known_sh_shell(shell) { + if !matches!(flag.as_str(), "-lc" | "-c") + || !matches!( + detect_shell_type(&PathBuf::from(shell)), + Some(ShellType::Zsh) | Some(ShellType::Bash) + ) + { 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 ba3eb6ba7..ab084c191 100644 --- a/codex-rs/core/src/command_safety/is_safe_command.rs +++ b/codex-rs/core/src/command_safety/is_safe_command.rs @@ -1,4 +1,5 @@ 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 @@ -11,12 +12,9 @@ pub fn is_known_safe_command(command: &[String]) -> bool { } }) .collect(); - #[cfg(target_os = "windows")] - { - use super::windows_safe_commands::is_safe_command_windows; - if is_safe_command_windows(&command) { - return true; - } + + 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 8279d6795..caf5c9f6e 100644 --- a/codex-rs/core/src/command_safety/mod.rs +++ b/codex-rs/core/src/command_safety/mod.rs @@ -1,4 +1,3 @@ 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 e7b2e19ff..6db2bb8f5 100644 --- a/codex-rs/core/src/environment_context.rs +++ b/codex-rs/core/src/environment_context.rs @@ -329,7 +329,6 @@ 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( @@ -338,7 +337,6 @@ 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 90f6c6b97..ed52491da 100644 --- a/codex-rs/core/src/shell.rs +++ b/codex-rs/core/src/shell.rs @@ -2,22 +2,26 @@ 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: String, - pub(crate) zshrc_path: String, + pub(crate) shell_path: PathBuf, } #[derive(Debug, PartialEq, Eq, Clone, Serialize, Deserialize)] pub struct BashShell { - pub(crate) shell_path: String, - pub(crate) bashrc_path: String, + pub(crate) shell_path: PathBuf, } #[derive(Debug, PartialEq, Eq, Clone, Serialize, Deserialize)] pub struct PowerShellConfig { - 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. + pub(crate) shell_path: PathBuf, // Executable name or path, e.g. "pwsh" or "powershell.exe". } #[derive(Debug, PartialEq, Eq, Clone, Serialize, Deserialize)] @@ -36,7 +40,10 @@ impl Shell { .file_name() .map(|s| s.to_string_lossy().to_string()) } - Shell::PowerShell(ps) => Some(ps.exe.clone()), + Shell::PowerShell(ps) => ps + .shell_path + .file_stem() + .map(|s| s.to_string_lossy().to_string()), Shell::Unknown => None, } } @@ -47,10 +54,17 @@ 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.clone(), arg.to_string(), command.to_string()] + vec![ + shell_path.to_string_lossy().to_string(), + arg.to_string(), + command.to_string(), + ] } Shell::PowerShell(ps) => { - let mut args = vec![ps.exe.clone(), "-NoLogo".to_string()]; + let mut args = vec![ + ps.shell_path.to_string_lossy().to_string(), + "-NoLogo".to_string(), + ]; if !use_login_shell { args.push("-NoProfile".to_string()); } @@ -65,7 +79,7 @@ impl Shell { } #[cfg(unix)] -fn detect_default_user_shell() -> Shell { +fn get_user_shell_path() -> Option { use libc::getpwuid; use libc::getuid; use std::ffi::CStr; @@ -78,75 +92,174 @@ fn detect_default_user_shell() -> Shell { let shell_path = CStr::from_ptr((*pw).pw_shell) .to_string_lossy() .into_owned(); - let home_path = CStr::from_ptr((*pw).pw_dir).to_string_lossy().into_owned(); + Some(PathBuf::from(shell_path)) + } else { + None + } + } +} - if shell_path.ends_with("/zsh") { - return Shell::Zsh(ZshShell { - shell_path, - zshrc_path: format!("{home_path}/.zshrc"), - }); - } +#[cfg(not(unix))] +fn get_user_shell_path() -> Option { + None +} - if shell_path.ends_with("/bash") { - return Shell::Bash(BashShell { - shell_path, - bashrc_path: format!("{home_path}/.bashrc"), - }); +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, + provided_path: Option<&PathBuf>, + binary_name: &str, + fallback_paths: Vec<&str>, +) -> Option { + // If exact provided path exists, use it + if provided_path.and_then(file_exists).is_some() { + return provided_path.cloned(); + } + + // 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(path: Option<&PathBuf>) -> Option { + let shell_path = get_shell_path(ShellType::Zsh, path, "zsh", vec!["/bin/zsh"]); + + shell_path.map(|shell_path| ZshShell { shell_path }) +} + +fn get_bash_shell(path: Option<&PathBuf>) -> Option { + let shell_path = get_shell_path(ShellType::Bash, path, "bash", vec!["/bin/bash"]); + + shell_path.map(|shell_path| BashShell { shell_path }) +} + +fn get_powershell_shell(path: Option<&PathBuf>) -> Option { + let shell_path = get_shell_path( + ShellType::PowerShell, + path, + "pwsh", + vec!["/usr/local/bin/pwsh"], + ) + .or_else(|| get_shell_path(ShellType::PowerShell, path, "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(|shell_type| get_shell(shell_type, Some(shell_path))) + .unwrap_or(Shell::Unknown) +} + +pub fn get_shell(shell_type: ShellType, path: Option<&PathBuf>) -> Option { + match shell_type { + ShellType::Zsh => get_zsh_shell(path).map(Shell::Zsh), + ShellType::Bash => get_bash_shell(path).map(Shell::Bash), + ShellType::PowerShell => get_powershell_shell(path).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 } } } - Shell::Unknown } -#[cfg(unix)] pub async fn default_user_shell() -> Shell { - 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() + if cfg!(windows) { + get_shell(ShellType::PowerShell, None).unwrap_or(Shell::Unknown) } 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, - }) + get_user_shell_path() + .and_then(|shell| detect_shell_type(&shell)) + .and_then(|shell_type| get_shell(shell_type, None)) + .unwrap_or(Shell::Unknown) } } -#[cfg(all(not(target_os = "windows"), not(unix)))] -pub async fn default_user_shell() -> Shell { - Shell::Unknown +#[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(test)] @@ -156,6 +269,34 @@ 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, None).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, None).unwrap(); + let BashShell { shell_path } = match bash_shell { + Shell::Bash(bash_shell) => bash_shell, + _ => panic!("expected bash shell"), + }; + + assert!( + shell_path == PathBuf::from("/bin/bash") + || shell_path == PathBuf::from("/usr/bin/bash"), + "shell path: {shell_path:?}", + ); + } + #[tokio::test] async fn test_current_shell_detects_zsh() { let shell = Command::new("sh") @@ -164,292 +305,44 @@ 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: shell_path.to_string(), - zshrc_path: format!("{home}/.zshrc",), + shell_path: PathBuf::from(shell_path), }) ); } } #[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:?}" - ); - } + async fn detects_powershell_as_default() { + if !cfg!(windows) { + return; } + + let powershell_shell = default_user_shell().await; + let PowerShellConfig { shell_path } = match powershell_shell { + Shell::PowerShell(powershell_shell) => powershell_shell, + _ => panic!("expected powershell shell"), + }; + + assert!(shell_path.ends_with("pwsh.exe") || shell_path.ends_with("powershell.exe")); } -} - -#[cfg(test)] -#[cfg(target_os = "macos")] -mod macos_tests { - use std::path::PathBuf; - - #[tokio::test] - async fn test_run_with_profile_escaping_and_execution() { - let shell_path = "/bin/zsh"; - - 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; - - 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 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:?}"); + fn finds_poweshell() { + if !cfg!(windows) { + return; } + + let powershell_shell = get_shell(ShellType::PowerShell, None).unwrap(); + let PowerShellConfig { shell_path } = match powershell_shell { + Shell::PowerShell(powershell_shell) => powershell_shell, + _ => panic!("expected powershell shell"), + }; + + assert!(shell_path.ends_with("pwsh.exe") || shell_path.ends_with("powershell.exe")); } } diff --git a/codex-rs/core/src/tools/events.rs b/codex-rs/core/src/tools/events.rs index 9019dac5e..5f93d51d8 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: String, + command: Vec, 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: command.to_vec(), 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.to_string()], cwd.as_path(), false).await; + emit_exec_command_begin(ctx, command, 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 3a5115f6e..39f271b2d 100644 --- a/codex-rs/core/src/tools/handlers/shell.rs +++ b/codex-rs/core/src/tools/handlers/shell.rs @@ -324,8 +324,11 @@ 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; @@ -335,27 +338,19 @@ 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: "/bin/bash".to_string(), - bashrc_path: "/home/user/.bashrc".to_string(), + shell_path: PathBuf::from("/bin/bash"), }); assert_safe(&bash_shell, "ls -la"); let zsh_shell = Shell::Zsh(ZshShell { - shell_path: "/bin/zsh".to_string(), - zshrc_path: "/home/user/.zshrc".to_string(), + shell_path: PathBuf::from("/bin/zsh"), }); assert_safe(&zsh_shell, "ls -la"); - #[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"); - } + let powershell = Shell::PowerShell(PowerShellConfig { + shell_path: PathBuf::from("pwsh.exe"), + }); + 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 97229970a..d5b4ecd71 100644 --- a/codex-rs/core/src/tools/handlers/unified_exec.rs +++ b/codex-rs/core/src/tools/handlers/unified_exec.rs @@ -5,6 +5,7 @@ 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; @@ -92,7 +93,8 @@ impl ToolHandler for UnifiedExecHandler { let Ok(params) = serde_json::from_str::(arguments) else { return true; }; - !is_known_safe_command(&["bash".to_string(), "-lc".to_string(), params.cmd]) + let command = get_command(¶ms); + !is_known_safe_command(&command) } async fn handle(&self, invocation: ToolInvocation) -> Result { @@ -125,15 +127,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) @@ -160,15 +162,14 @@ impl ToolHandler for UnifiedExecHandler { &context.call_id, None, ); - let emitter = ToolEmitter::unified_exec(cmd.clone(), cwd.clone(), true); + + let emitter = ToolEmitter::unified_exec(&command, cwd.clone(), true); emitter.emit(event_ctx, ToolEventStage::Begin).await; manager .exec_command( ExecCommandRequest { - command: &cmd, - shell: &shell, - login, + command, yield_time_ms, max_output_tokens, workdir, @@ -229,6 +230,11 @@ 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 1ee65e971..951032c07 100644 --- a/codex-rs/core/src/unified_exec/mod.rs +++ b/codex-rs/core/src/unified_exec/mod.rs @@ -63,10 +63,8 @@ impl UnifiedExecContext { } #[derive(Debug)] -pub(crate) struct ExecCommandRequest<'a> { - pub command: &'a str, - pub shell: &'a str, - pub login: bool, +pub(crate) struct ExecCommandRequest { + pub command: Vec, pub yield_time_ms: u64, pub max_output_tokens: Option, pub workdir: Option, @@ -104,7 +102,7 @@ struct SessionEntry { session_ref: Arc, turn_ref: Arc, call_id: String, - command: String, + command: Vec, cwd: PathBuf, started_at: tokio::time::Instant, } @@ -193,9 +191,7 @@ mod tests { .unified_exec_manager .exec_command( ExecCommandRequest { - command: cmd, - shell: "/bin/bash", - login: true, + command: vec!["bash".to_string(), "-lc".to_string(), cmd.to_string()], 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 df2c3aaf1..55e9102b7 100644 --- a/codex-rs/core/src/unified_exec/session_manager.rs +++ b/codex-rs/core/src/unified_exec/session_manager.rs @@ -35,23 +35,17 @@ 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( - command, + &request.command, cwd.clone(), request.with_escalated_permissions, request.justification, @@ -77,7 +71,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, ) }; @@ -97,7 +91,7 @@ impl UnifiedExecSessionManager { let exit = response.exit_code.unwrap_or(-1); Self::emit_exec_end_from_context( context, - request.command.to_string(), + &request.command, cwd, response.output.clone(), exit, @@ -222,7 +216,7 @@ impl UnifiedExecSessionManager { &self, session: UnifiedExecSession, context: &UnifiedExecContext, - command: &str, + command: &[String], cwd: PathBuf, started_at: Instant, ) -> i32 { @@ -234,7 +228,7 @@ impl UnifiedExecSessionManager { session_ref: Arc::clone(&context.session), turn_ref: Arc::clone(&context.turn), call_id: context.call_id.clone(), - command: command.to_string(), + command: command.to_vec(), cwd, started_at, }; @@ -262,7 +256,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; @@ -270,7 +264,7 @@ impl UnifiedExecSessionManager { async fn emit_exec_end_from_context( context: &UnifiedExecContext, - command: String, + command: &[String], cwd: PathBuf, aggregated_output: String, exit_code: i32, @@ -319,7 +313,7 @@ impl UnifiedExecSessionManager { pub(super) async fn open_session_with_sandbox( &self, - command: Vec, + command: &[String], cwd: PathBuf, with_escalated_permissions: Option, justification: Option, @@ -328,7 +322,7 @@ impl UnifiedExecSessionManager { let mut orchestrator = ToolOrchestrator::new(); let mut runtime = UnifiedExecRuntime::new(self); let req = UnifiedExecToolRequest::new( - command, + command.to_vec(), 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 1225b51cc..07b17f78a 100644 --- a/codex-rs/core/tests/suite/unified_exec.rs +++ b/codex-rs/core/tests/suite/unified_exec.rs @@ -214,7 +214,11 @@ async fn unified_exec_emits_exec_command_begin_event() -> Result<()> { assert_eq!( begin_event.command, - vec!["/bin/echo hello unified exec".to_string()] + vec![ + "/bin/bash".to_string(), + "-lc".to_string(), + "/bin/echo hello unified exec".to_string() + ] ); assert_eq!(begin_event.cwd, cwd.path()); @@ -654,7 +658,14 @@ 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[0], "/bin/sh -c echo ready"); + assert_eq!( + begin_events[0].command, + vec![ + "/bin/bash".to_string(), + "-lc".to_string(), + "/bin/sh -c echo ready".to_string() + ] + ); Ok(()) }