Overhaul shell detection and centralize command generation for unified exec (#6577)

This fixes command display for unified exec. All `cd`s and `ls`es are
now parsed.

<img width="452" height="237" alt="image"
src="https://github.com/user-attachments/assets/ce92d81f-f74c-485a-9b34-1eaa29290ec6"
/>

Deletes a ton of tests that were doing nothing from shell.rs.

---------

Co-authored-by: Pavel Krymets <pavel@krymets.com>

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<PathBuf>, // 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<PathBuf> {
     use libc::getpwuid;
     use libc::getuid;
     use std::ffi::CStr;
@@ -78,75 +92,164 @@ 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<PathBuf> {
+    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<PathBuf> {
+    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<PathBuf> {
+    // 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<ZshShell> {
+    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<BashShell> {
+    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<PowerShellConfig> {
+    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<Shell> {
+    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<ShellType> {
+    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).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(get_shell)
+            .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 +259,30 @@ 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")
@@ -164,292 +291,42 @@ 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::<Vec<_>>();
-
-            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 = "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::<Vec<_>>();
-
-            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 {
+#[cfg(windows)]
+mod tests {
     use super::*;
 
+    #[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"),
+        };
+
+        assert!(shell_path.ends_with("pwsh.exe") || shell_path.ends_with("powershell.exe"));
+    }
+
     #[test]
-    fn test_format_default_shell_invocation_powershell() {
-        use std::path::PathBuf;
+    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"),
+        };
 
-        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::<Vec<_>>();
-
-            // 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::<Vec<_>>();
-            assert_eq!(command, expected, "input: {input:?} config: {config:?}");
-        }
+        assert!(shell_path.ends_with("pwsh.exe") || shell_path.ends_with("powershell.exe"));
     }
 }