From be45b979f9eff9046f6676189d48e1382124c663 Mon Sep 17 00:00:00 2001 From: jimmyfraiture Date: Fri, 5 Sep 2025 10:39:21 -0700 Subject: [PATCH] Unify into POSIX --- codex-rs/core/src/codex.rs | 12 +- codex-rs/core/src/shell.rs | 363 +++++++++++++++++-------------------- 2 files changed, 166 insertions(+), 209 deletions(-) diff --git a/codex-rs/core/src/codex.rs b/codex-rs/core/src/codex.rs index 067d112ba1..f233ef6173 100644 --- a/codex-rs/core/src/codex.rs +++ b/codex-rs/core/src/codex.rs @@ -106,7 +106,6 @@ use crate::safety::SafetyCheck; use crate::safety::assess_command_safety; use crate::safety::assess_safety_for_untrusted_command; use crate::shell; -use crate::shell::Shell; use crate::shell::ShellSnapshot; use crate::turn_diff_tracker::TurnDiffTracker; use crate::user_instructions::UserInstructions; @@ -411,10 +410,7 @@ impl Session { ..Default::default() }; - let shell_snapshot = match &default_shell { - Shell::Zsh(zsh) => zsh.shell_snapshot.clone(), - _ => None, - }; + let shell_snapshot = default_shell.get_snapshot(); // Handle MCP manager result and record any startup failures. let (mcp_connection_manager, failed_clients) = match mcp_res { @@ -2314,7 +2310,7 @@ fn should_translate_shell_command( || shell_policy.use_profile || matches!( shell, - crate::shell::Shell::Zsh(zsh) if zsh.shell_snapshot.is_some() + crate::shell::Shell::Posix(shell) if shell.shell_snapshot.is_some() ) } @@ -2963,9 +2959,9 @@ mod tests { } fn zsh_shell(shell_snapshot: Option) -> shell::Shell { - shell::Shell::Zsh(shell::ZshShell { + shell::Shell::Posix(shell::PosixShell { shell_path: "/bin/zsh".to_string(), - zshrc_path: "/Users/example/.zshrc".to_string(), + rc_path: "/Users/example/.zshrc".to_string(), shell_snapshot, }) } diff --git a/codex-rs/core/src/shell.rs b/codex-rs/core/src/shell.rs index 3adacc8cc9..29b5cb617f 100644 --- a/codex-rs/core/src/shell.rs +++ b/codex-rs/core/src/shell.rs @@ -6,13 +6,6 @@ use std::path::PathBuf; use tracing::trace; use uuid::Uuid; -#[derive(Debug, PartialEq, Eq, Clone, Serialize, Deserialize)] -pub struct ZshShell { - pub(crate) shell_path: String, - pub(crate) zshrc_path: String, - pub(crate) shell_snapshot: Option, -} - #[derive(Debug, PartialEq, Eq, Clone, Serialize, Deserialize)] pub struct ShellSnapshot { pub(crate) path: PathBuf, @@ -25,9 +18,9 @@ impl ShellSnapshot { } #[derive(Debug, PartialEq, Eq, Clone, Serialize, Deserialize)] -pub struct BashShell { - shell_path: String, - bashrc_path: String, +pub struct PosixShell { + pub(crate) shell_path: String, + pub(crate) rc_path: String, pub(crate) shell_snapshot: Option, } @@ -39,8 +32,7 @@ pub struct PowerShellConfig { #[derive(Debug, PartialEq, Eq, Clone, Serialize, Deserialize)] pub enum Shell { - Zsh(ZshShell), - Bash(BashShell), + Posix(PosixShell), PowerShell(PowerShellConfig), Unknown, } @@ -48,18 +40,29 @@ pub enum Shell { impl Shell { pub fn format_default_shell_invocation(&self, command: Vec) -> Option> { match self { - Shell::Zsh(zsh) => format_shell_invocation_with_rc( - &command, - &zsh.shell_path, - &zsh.zshrc_path, - &zsh.shell_snapshot, - ), - Shell::Bash(bash) => format_shell_invocation_with_rc( - &command, - &bash.shell_path, - &bash.bashrc_path, - &bash.shell_snapshot, - ), + Shell::Posix(shell) => { + let joined = strip_bash_lc(&command) + .or_else(|| shlex::try_join(command.iter().map(|s| s.as_str())).ok())?; + + let mut source_path = Path::new(&shell.rc_path); + + let session_cmd = if let Some(shell_snapshot) = &shell.shell_snapshot + && shell_snapshot.path.exists() + { + source_path = shell_snapshot.path.as_path(); + "-c".to_string() + } else { + "-lc".to_string() + }; + + let rc_command = if source_path.exists() { + format!("source {} && ({joined})", source_path.to_string_lossy()) + } else { + joined + }; + + Some(vec![shell.shell_path.clone(), session_cmd, rc_command]) + } Shell::PowerShell(ps) => { // If model generated a bash command, prefer a detected bash fallback if let Some(script) = strip_bash_lc(&command) { @@ -111,45 +114,20 @@ impl Shell { 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::Bash(bash) => std::path::Path::new(&bash.shell_path) + Shell::Posix(shell) => Path::new(&shell.rc_path) .file_name() .map(|s| s.to_string_lossy().to_string()), Shell::PowerShell(ps) => Some(ps.exe.clone()), Shell::Unknown => None, } } -} -fn format_shell_invocation_with_rc( - command: &Vec, - shell_path: &str, - rc_path: &str, - shell_snapshot: &Option, -) -> Option> { - let joined = strip_bash_lc(command) - .or_else(|| shlex::try_join(command.iter().map(|s| s.as_str())).ok())?; - - let mut source_path = Path::new(rc_path); - - let session_cmd = if let Some(shell_snapshot) = shell_snapshot - && shell_snapshot.path.exists() - { - source_path = shell_snapshot.path.as_path(); - "-c".to_string() - } else { - "-lc".to_string() - }; - - let rc_command = if source_path.exists() { - format!("source {} && ({joined})", source_path.to_string_lossy()) - } else { - joined - }; - - Some(vec![shell_path.to_string(), session_cmd, rc_command]) + pub fn get_snapshot(&self) -> Option { + match self { + Shell::Posix(shell) => shell.shell_snapshot.clone(), + _ => None, + } + } } fn strip_bash_lc(command: &Vec) -> Option { @@ -182,27 +160,26 @@ async fn detect_default_user_shell(session_id: Uuid) -> Shell { let home_path = CStr::from_ptr((*pw).pw_dir).to_string_lossy().into_owned(); let snapshot_path = - ensure_zsh_snapshot(&shell_path, Path::new(&home_path), session_id).await; + snapshots::ensure_posix_snapshot(&shell_path, Path::new(&home_path), session_id) + .await; if snapshot_path.is_none() { trace!("failed to prepare zsh snapshot; using live profile"); } let shell_snapshot = snapshot_path.map(ShellSnapshot::new); - if shell_path.ends_with("/zsh") { - return Shell::Zsh(ZshShell { - shell_path, - zshrc_path: format!("{home_path}/.zshrc"), - shell_snapshot, - }); - } + let rc_path = if shell_path.ends_with("/zsh") { + format!("{home_path}/.zshrc") + } else if shell_path.ends_with("/bash") { + format!("{home_path}/.bashrc") + } else { + return Shell::Unknown; + }; - if shell_path.ends_with("/bash") { - return Shell::Bash(BashShell { - shell_path, - bashrc_path: format!("{home_path}/.bashrc"), - shell_snapshot, - }); - } + return Shell::Posix(PosixShell { + shell_path, + rc_path, + shell_snapshot, + }); } } Shell::Unknown @@ -258,133 +235,134 @@ pub async fn default_user_shell(session_id: Uuid) -> Shell { Shell::Unknown } -#[cfg(target_os = "macos")] -fn zsh_profile_paths(home: &Path) -> Vec { - [".zshenv", ".zprofile", ".zshrc", ".zlogin"] - .into_iter() - .map(|name| home.join(name)) - .collect() -} +#[cfg(unix)] +mod snapshots { + use super::*; -#[cfg(target_os = "macos")] -fn zsh_profile_source_script(home: &Path) -> String { - zsh_profile_paths(home) - .into_iter() - .map(|profile| { - let profile_string = profile.to_string_lossy().into_owned(); - let quoted = shlex::try_quote(&profile_string) - .map(|cow| cow.into_owned()) - .unwrap_or(profile_string.clone()); + fn zsh_profile_paths(home: &Path) -> Vec { + [".zshenv", ".zprofile", ".zshrc", ".zlogin"] + .into_iter() + .map(|name| home.join(name)) + .collect() + } - format!("[ -f {quoted} ] && source {quoted}") - }) - .collect::>() - .join("; ") -} + fn posix_profile_source_script(home: &Path) -> String { + zsh_profile_paths(home) + .into_iter() + .map(|profile| { + let profile_string = profile.to_string_lossy().into_owned(); + let quoted = shlex::try_quote(&profile_string) + .map(|cow| cow.into_owned()) + .unwrap_or(profile_string.clone()); -#[cfg(target_os = "macos")] -async fn ensure_zsh_snapshot(shell_path: &str, home: &Path, session_id: Uuid) -> Option { - let snapshot_path = home - .join(".codex") - .join(format!("codex_shell_snapshot_{session_id}.zsh")); + format!("[ -f {quoted} ] && source {quoted}") + }) + .collect::>() + .join("; ") + } - // Check if an update in the profile requires to re-generate the snapshot. - let snapshot_is_stale = async { - let snapshot_metadata = tokio::fs::metadata(&snapshot_path).await.ok()?; - let snapshot_modified = snapshot_metadata.modified().ok()?; + pub(crate) async fn ensure_posix_snapshot( + shell_path: &str, + home: &Path, + session_id: Uuid, + ) -> Option { + let snapshot_path = home + .join(".codex") + .join(format!("codex_shell_snapshot_{session_id}.zsh")); - for profile in zsh_profile_paths(home) { - let Ok(profile_metadata) = tokio::fs::metadata(&profile).await else { - continue; - }; + // Check if an update in the profile requires to re-generate the snapshot. + let snapshot_is_stale = async { + let snapshot_metadata = tokio::fs::metadata(&snapshot_path).await.ok()?; + let snapshot_modified = snapshot_metadata.modified().ok()?; - let Ok(profile_modified) = profile_metadata.modified() else { - return Some(true); - }; + for profile in zsh_profile_paths(home) { + let Ok(profile_metadata) = tokio::fs::metadata(&profile).await else { + continue; + }; - if profile_modified > snapshot_modified { - return Some(true); + let Ok(profile_modified) = profile_metadata.modified() else { + return Some(true); + }; + + if profile_modified > snapshot_modified { + return Some(true); + } + } + + Some(false) + } + .await + .unwrap_or(true); + + if !snapshot_is_stale { + return Some(snapshot_path); + } + + match regenerate_posix_snapshot(shell_path, home, &snapshot_path).await { + Ok(()) => Some(snapshot_path), + Err(err) => { + tracing::warn!("failed to generate zsh snapshot: {err}"); + None } } - - Some(false) - } - .await - .unwrap_or(true); - - if !snapshot_is_stale { - return Some(snapshot_path); } - match regenerate_zsh_snapshot(shell_path, home, &snapshot_path).await { - Ok(()) => Some(snapshot_path), - Err(err) => { - tracing::warn!("failed to generate zsh snapshot: {err}"); - None + async fn regenerate_posix_snapshot( + shell_path: &str, + home: &Path, + snapshot_path: &Path, + ) -> std::io::Result<()> { + // Use `emulate -L sh` instead of `set -o posix` so we work on zsh builds + // that disable that option. Guard `alias -p` with `|| true` so the script + // keeps a zero exit status even if aliases are disabled. + let mut capture_script = String::new(); + let profile_sources = posix_profile_source_script(home); + if !profile_sources.is_empty() { + capture_script.push_str(&format!("{profile_sources}; ")); } - } -} -#[cfg(target_os = "macos")] -async fn regenerate_zsh_snapshot( - shell_path: &str, - home: &Path, - snapshot_path: &Path, -) -> std::io::Result<()> { - // Use `emulate -L sh` instead of `set -o posix` so we work on zsh builds - // that disable that option. Guard `alias -p` with `|| true` so the script - // keeps a zero exit status even if aliases are disabled. - let mut capture_script = String::new(); - let profile_sources = zsh_profile_source_script(home); - if !profile_sources.is_empty() { - capture_script.push_str(&format!("{profile_sources}; ")); - } + let zshrc = home.join(".zshrc"); - let zshrc = home.join(".zshrc"); + capture_script.push_str( + &format!("source {}/.zshrc; setopt posixbuiltins; export -p; {{ alias | sed 's/^/alias /'; }} 2>/dev/null || true", zshrc.display()), + ); + let output = tokio::process::Command::new(shell_path) + .arg("-lc") + .arg(capture_script) + .env("HOME", home) + .output() + .await?; - capture_script.push_str( - &format!("source {}/.zshrc; setopt posixbuiltins; export -p; {{ alias | sed 's/^/alias /'; }} 2>/dev/null || true", zshrc.display()), - ); - let output = tokio::process::Command::new(shell_path) - .arg("-lc") - .arg(capture_script) - .env("HOME", home) - .output() - .await?; + if !output.status.success() { + return Err(std::io::Error::other(format!( + "snapshot capture exited with status {}", + output.status + ))); + } - if !output.status.success() { - return Err(std::io::Error::other(format!( - "snapshot capture exited with status {}", - output.status - ))); - } + let mut contents = String::from("# Generated by Codex. Do not edit.\n"); - let mut contents = String::from("# Generated by Codex. Do not edit.\n"); + contents.push_str(&String::from_utf8_lossy(&output.stdout)); + contents.push('\n'); - contents.push_str(&String::from_utf8_lossy(&output.stdout)); - contents.push('\n'); + if let Some(parent) = snapshot_path.parent() { + tokio::fs::create_dir_all(parent).await?; + } - if let Some(parent) = snapshot_path.parent() { - tokio::fs::create_dir_all(parent).await?; - } + let tmp_path = snapshot_path.with_extension("tmp"); + tokio::fs::write(&tmp_path, contents).await?; - let tmp_path = snapshot_path.with_extension("tmp"); - tokio::fs::write(&tmp_path, contents).await?; - - #[cfg(unix)] - { // Restrict the snapshot to user read/write so that environment variables or aliases // that may contain secrets are not exposed to other users on the system. use std::os::unix::fs::PermissionsExt; let permissions = std::fs::Permissions::from_mode(0o600); tokio::fs::set_permissions(&tmp_path, permissions).await?; - } - tokio::fs::rename(&tmp_path, snapshot_path).await?; - Ok(()) + tokio::fs::rename(&tmp_path, snapshot_path).await?; + Ok(()) + } } -#[cfg(target_os = "macos")] pub(crate) fn delete_shell_snapshot(path: &Path) { if let Err(err) = std::fs::remove_file(path) { trace!(?path, %err, "failed to delete shell snapshot"); @@ -414,9 +392,9 @@ mod tests { let shell_path = String::from_utf8_lossy(&shell.stdout).trim().to_string(); if shell_path.ends_with("/zsh") { match default_user_shell(Uuid::new_v4()).await { - Shell::Zsh(zsh) => { - assert_eq!(zsh.shell_path, shell_path); - assert_eq!(zsh.zshrc_path, format!("{home}/.zshrc")); + Shell::Posix(shell) => { + assert_eq!(shell.shell_path, shell_path); + assert_eq!(shell.rc_path, format!("{home}/.zshrc")); } other => panic!("unexpected shell returned: {other:?}"), } @@ -425,9 +403,9 @@ mod tests { #[tokio::test] async fn test_run_with_profile_zshrc_not_exists() { - let shell = Shell::Zsh(ZshShell { + let shell = Shell::Posix(PosixShell { shell_path: "/bin/zsh".to_string(), - zshrc_path: "/does/not/exist/.zshrc".to_string(), + rc_path: "/does/not/exist/.zshrc".to_string(), shell_snapshot: None, }); let actual_cmd = shell.format_default_shell_invocation(vec!["myecho".to_string()]); @@ -441,24 +419,6 @@ mod tests { ); } - #[tokio::test] - async fn test_run_with_profile_bashrc_not_exists() { - let shell = Shell::Bash(BashShell { - shell_path: "/bin/bash".to_string(), - bashrc_path: "/does/not/exist/.bashrc".to_string(), - shell_snapshot: None, - }); - let actual_cmd = shell.format_default_shell_invocation(vec!["myecho".to_string()]); - assert_eq!( - actual_cmd, - Some(vec![ - "/bin/bash".to_string(), - "-lc".to_string(), - "myecho".to_string() - ]) - ); - } - #[tokio::test] async fn test_run_with_profile_bash_escaping_and_execution() { let shell_path = "/bin/bash"; @@ -500,9 +460,9 @@ mod tests { "#, ) .unwrap(); - let shell = Shell::Bash(BashShell { + let shell = Shell::Posix(PosixShell { shell_path: shell_path.to_string(), - bashrc_path: bashrc_path.to_str().unwrap().to_string(), + rc_path: bashrc_path.to_str().unwrap().to_string(), shell_snapshot: None, }); @@ -553,6 +513,7 @@ mod tests { #[cfg(target_os = "macos")] mod macos_tests { use super::*; + use crate::shell::snapshots::ensure_posix_snapshot; #[tokio::test] async fn test_snapshot_generation_uses_session_id_and_cleanup() { @@ -569,7 +530,7 @@ mod macos_tests { .unwrap(); let session_id = Uuid::new_v4(); - let snapshot_path = ensure_zsh_snapshot(shell_path, temp_home.path(), session_id) + let snapshot_path = ensure_posix_snapshot(shell_path, temp_home.path(), session_id) .await .expect("snapshot path"); @@ -581,7 +542,7 @@ mod macos_tests { assert!(filename.contains(&session_id.to_string())); assert!(snapshot_path.exists()); - let snapshot_path_second = ensure_zsh_snapshot(shell_path, temp_home.path(), session_id) + let snapshot_path_second = ensure_posix_snapshot(shell_path, temp_home.path(), session_id) .await .expect("snapshot path"); assert_eq!(snapshot_path, snapshot_path_second); @@ -600,9 +561,9 @@ mod macos_tests { let snapshot_path = temp_dir.path().join("snapshot.zsh"); std::fs::write(&snapshot_path, "export SNAPSHOT_READY=1").unwrap(); - let shell = Shell::Zsh(ZshShell { + let shell = Shell::Posix(PosixShell { shell_path: "/bin/zsh".to_string(), - zshrc_path: { + rc_path: { let path = temp_dir.path().join(".zshrc"); std::fs::write(&path, "# test zshrc").unwrap(); path.to_string_lossy().to_string() @@ -675,9 +636,9 @@ mod macos_tests { "#, ) .unwrap(); - let shell = Shell::Zsh(ZshShell { + let shell = Shell::Posix(PosixShell { shell_path: shell_path.to_string(), - zshrc_path: zshrc_path.to_str().unwrap().to_string(), + rc_path: zshrc_path.to_str().unwrap().to_string(), shell_snapshot: None, });