diff --git a/codex-rs/Cargo.lock b/codex-rs/Cargo.lock index d5bab66c66..3dc8a91130 100644 --- a/codex-rs/Cargo.lock +++ b/codex-rs/Cargo.lock @@ -1865,6 +1865,7 @@ dependencies = [ "codex-rollout", "codex-sandboxing", "codex-secrets", + "codex-shell", "codex-shell-command", "codex-shell-escalation", "codex-state", @@ -2568,6 +2569,24 @@ dependencies = [ "tracing", ] +[[package]] +name = "codex-shell" +version = "0.0.0" +dependencies = [ + "anyhow", + "codex-otel", + "codex-protocol", + "codex-utils-pty", + "libc", + "pretty_assertions", + "serde", + "tempfile", + "tokio", + "tracing", + "uuid", + "which 8.0.0", +] + [[package]] name = "codex-shell-command" version = "0.0.0" @@ -2575,6 +2594,7 @@ dependencies = [ "anyhow", "base64 0.22.1", "codex-protocol", + "codex-shell", "codex-utils-absolute-path", "once_cell", "pretty_assertions", @@ -2668,6 +2688,7 @@ dependencies = [ "codex-code-mode", "codex-features", "codex-protocol", + "codex-shell", "codex-utils-absolute-path", "codex-utils-pty", "pretty_assertions", diff --git a/codex-rs/Cargo.toml b/codex-rs/Cargo.toml index 2d80bbc69e..319e41e9fa 100644 --- a/codex-rs/Cargo.toml +++ b/codex-rs/Cargo.toml @@ -24,6 +24,7 @@ members = [ "config", "shell-command", "shell-escalation", + "shell", "skills", "core", "core-skills", @@ -149,6 +150,7 @@ codex-sandboxing = { path = "sandboxing" } codex-secrets = { path = "secrets" } codex-shell-command = { path = "shell-command" } codex-shell-escalation = { path = "shell-escalation" } +codex-shell = { path = "shell" } codex-skills = { path = "skills" } codex-state = { path = "state" } codex-stdio-to-uds = { path = "stdio-to-uds" } diff --git a/codex-rs/core/Cargo.toml b/codex-rs/core/Cargo.toml index ad82b6c086..9519166d6f 100644 --- a/codex-rs/core/Cargo.toml +++ b/codex-rs/core/Cargo.toml @@ -51,6 +51,7 @@ codex-protocol = { workspace = true } codex-rollout = { workspace = true } codex-rmcp-client = { workspace = true } codex-sandboxing = { workspace = true } +codex-shell = { workspace = true } codex-state = { workspace = true } codex-terminal-detection = { workspace = true } codex-tools = { workspace = true } diff --git a/codex-rs/core/src/codex.rs b/codex-rs/core/src/codex.rs index 47c6cf8600..21d189594c 100644 --- a/codex-rs/core/src/codex.rs +++ b/codex-rs/core/src/codex.rs @@ -290,6 +290,7 @@ use crate::rollout::policy::EventPersistenceMode; use crate::session_startup_prewarm::SessionStartupPrewarmHandle; use crate::shell; use crate::shell_snapshot::ShellSnapshot; +use crate::shell_snapshot::spawn_stale_snapshot_cleanup; use crate::skills_watcher::SkillsWatcher; use crate::skills_watcher::SkillsWatcherEvent; use crate::state::ActiveTurn; @@ -1400,7 +1401,7 @@ impl Session { windows_sandbox_level: session_configuration.windows_sandbox_level, }) .with_unified_exec_shell_mode_for_session( - crate::tools::spec::tool_user_shell_type(user_shell), + user_shell.shell_type, shell_zsh_path, main_execve_wrapper_exe, ) @@ -1750,25 +1751,19 @@ impl Session { shell::default_user_shell() }; // Create the mutable state for the Session. - let shell_snapshot_tx = if config.features.enabled(Feature::ShellSnapshot) { + if config.features.enabled(Feature::ShellSnapshot) { if let Some(snapshot) = session_configuration.inherited_shell_snapshot.clone() { - let (tx, rx) = watch::channel(Some(snapshot)); - default_shell.shell_snapshot = rx; - tx + default_shell.set_shell_snapshot(Some(snapshot)); } else { - ShellSnapshot::start_snapshotting( + default_shell.start_snapshotting( config.codex_home.clone(), conversation_id, session_configuration.cwd.to_path_buf(), - &mut default_shell, session_telemetry.clone(), - ) + ); + spawn_stale_snapshot_cleanup(config.codex_home.clone(), conversation_id); } - } else { - let (tx, rx) = watch::channel(None); - default_shell.shell_snapshot = rx; - tx - }; + } let thread_name = match session_index::find_thread_name_by_id(&config.codex_home, &conversation_id) .instrument(info_span!( @@ -1884,7 +1879,6 @@ impl Session { hooks, rollout: Mutex::new(rollout_recorder), user_shell: Arc::new(default_shell), - shell_snapshot_tx, show_raw_agent_reasoning: config.show_raw_agent_reasoning, exec_policy, auth_manager: Arc::clone(&auth_manager), @@ -2325,14 +2319,13 @@ impl Session { return; } - ShellSnapshot::refresh_snapshot( + self.services.user_shell.refresh_snapshot( codex_home.to_path_buf(), self.conversation_id, next_cwd.to_path_buf(), - self.services.user_shell.as_ref().clone(), - self.services.shell_snapshot_tx.clone(), self.services.session_telemetry.clone(), ); + spawn_stale_snapshot_cleanup(codex_home.to_path_buf(), self.conversation_id); } pub(crate) async fn update_settings( @@ -5472,7 +5465,7 @@ async fn spawn_review_thread( windows_sandbox_level: parent_turn_context.windows_sandbox_level, }) .with_unified_exec_shell_mode_for_session( - crate::tools::spec::tool_user_shell_type(sess.services.user_shell.as_ref()), + sess.services.user_shell.shell_type, sess.services.shell_zsh_path.as_ref(), sess.services.main_execve_wrapper_exe.as_ref(), ) diff --git a/codex-rs/core/src/codex_tests.rs b/codex-rs/core/src/codex_tests.rs index 24c24eb935..c74ae5f4bf 100644 --- a/codex-rs/core/src/codex_tests.rs +++ b/codex-rs/core/src/codex_tests.rs @@ -2675,7 +2675,6 @@ pub(crate) async fn make_session_and_context() -> (Session, TurnContext) { }), rollout: Mutex::new(None), user_shell: Arc::new(default_user_shell()), - shell_snapshot_tx: watch::channel(None).0, show_raw_agent_reasoning: config.show_raw_agent_reasoning, exec_policy, auth_manager: auth_manager.clone(), @@ -3512,7 +3511,6 @@ pub(crate) async fn make_session_and_context_with_dynamic_tools_and_rx( }), rollout: Mutex::new(None), user_shell: Arc::new(default_user_shell()), - shell_snapshot_tx: watch::channel(None).0, show_raw_agent_reasoning: config.show_raw_agent_reasoning, exec_policy, auth_manager: Arc::clone(&auth_manager), diff --git a/codex-rs/core/src/environment_context_tests.rs b/codex-rs/core/src/environment_context_tests.rs index 073f1fe169..57ce05e611 100644 --- a/codex-rs/core/src/environment_context_tests.rs +++ b/codex-rs/core/src/environment_context_tests.rs @@ -5,11 +5,7 @@ use core_test_support::test_path_buf; use pretty_assertions::assert_eq; fn fake_shell() -> Shell { - Shell { - shell_type: ShellType::Bash, - shell_path: PathBuf::from("/bin/bash"), - shell_snapshot: crate::shell::empty_shell_snapshot_receiver(), - } + Shell::new(ShellType::Bash, PathBuf::from("/bin/bash")) } #[test] @@ -219,11 +215,7 @@ fn equals_except_shell_compares_cwd_differences() { fn equals_except_shell_ignores_shell() { let context1 = EnvironmentContext::new( Some(PathBuf::from("/repo")), - Shell { - shell_type: ShellType::Bash, - shell_path: "/bin/bash".into(), - shell_snapshot: crate::shell::empty_shell_snapshot_receiver(), - }, + Shell::new(ShellType::Bash, "/bin/bash".into()), /*current_date*/ None, /*timezone*/ None, /*network*/ None, @@ -231,11 +223,7 @@ fn equals_except_shell_ignores_shell() { ); let context2 = EnvironmentContext::new( Some(PathBuf::from("/repo")), - Shell { - shell_type: ShellType::Zsh, - shell_path: "/bin/zsh".into(), - shell_snapshot: crate::shell::empty_shell_snapshot_receiver(), - }, + Shell::new(ShellType::Zsh, "/bin/zsh".into()), /*current_date*/ None, /*timezone*/ None, /*network*/ None, diff --git a/codex-rs/core/src/lib.rs b/codex-rs/core/src/lib.rs index 8ffc7acfb7..bdb9154d12 100644 --- a/codex-rs/core/src/lib.rs +++ b/codex-rs/core/src/lib.rs @@ -72,7 +72,6 @@ mod sandbox_tags; pub mod sandboxing; mod session_prefix; mod session_startup_prewarm; -mod shell_detect; pub mod skills; pub(crate) use skills::SkillError; pub(crate) use skills::SkillInjections; diff --git a/codex-rs/core/src/shell.rs b/codex-rs/core/src/shell.rs index 1943712565..25bf7d59cb 100644 --- a/codex-rs/core/src/shell.rs +++ b/codex-rs/core/src/shell.rs @@ -1,385 +1,6 @@ -use crate::shell_detect::detect_shell_type; -use crate::shell_snapshot::ShellSnapshot; -use serde::Deserialize; -use serde::Serialize; -use std::path::PathBuf; -use std::sync::Arc; -use tokio::sync::watch; - -#[derive(Debug, PartialEq, Eq, Clone, Serialize, Deserialize)] -pub enum ShellType { - Zsh, - Bash, - PowerShell, - Sh, - Cmd, -} - -#[derive(Debug, Clone, Serialize, Deserialize)] -pub struct Shell { - pub(crate) shell_type: ShellType, - pub(crate) shell_path: PathBuf, - #[serde( - skip_serializing, - skip_deserializing, - default = "empty_shell_snapshot_receiver" - )] - pub(crate) shell_snapshot: watch::Receiver>>, -} - -impl Shell { - pub fn name(&self) -> &'static str { - match self.shell_type { - ShellType::Zsh => "zsh", - ShellType::Bash => "bash", - ShellType::PowerShell => "powershell", - ShellType::Sh => "sh", - ShellType::Cmd => "cmd", - } - } - - /// Takes a string of shell and returns the full list of command args to - /// use with `exec()` to run the shell command. - pub fn derive_exec_args(&self, command: &str, use_login_shell: bool) -> Vec { - match self.shell_type { - ShellType::Zsh | ShellType::Bash | ShellType::Sh => { - let arg = if use_login_shell { "-lc" } else { "-c" }; - vec![ - self.shell_path.to_string_lossy().to_string(), - arg.to_string(), - command.to_string(), - ] - } - ShellType::PowerShell => { - let mut args = vec![self.shell_path.to_string_lossy().to_string()]; - if !use_login_shell { - args.push("-NoProfile".to_string()); - } - - args.push("-Command".to_string()); - args.push(command.to_string()); - args - } - ShellType::Cmd => { - let mut args = vec![self.shell_path.to_string_lossy().to_string()]; - args.push("/c".to_string()); - args.push(command.to_string()); - args - } - } - } - - /// Return the shell snapshot if existing. - pub fn shell_snapshot(&self) -> Option> { - self.shell_snapshot.borrow().clone() - } -} - -pub(crate) fn empty_shell_snapshot_receiver() -> watch::Receiver>> { - let (_tx, rx) = watch::channel(None); - rx -} - -impl PartialEq for Shell { - fn eq(&self, other: &Self) -> bool { - self.shell_type == other.shell_type && self.shell_path == other.shell_path - } -} - -impl Eq for Shell {} - -#[cfg(unix)] -fn get_user_shell_path() -> Option { - let uid = unsafe { libc::getuid() }; - use std::ffi::CStr; - use std::mem::MaybeUninit; - use std::ptr; - - let mut passwd = MaybeUninit::::uninit(); - - // We cannot use getpwuid here: it returns pointers into libc-managed - // storage, which is not safe to read concurrently on all targets (the musl - // static build used by the CLI can segfault when parallel callers race on - // that buffer). getpwuid_r keeps the passwd data in caller-owned memory. - let suggested_buffer_len = unsafe { libc::sysconf(libc::_SC_GETPW_R_SIZE_MAX) }; - let buffer_len = usize::try_from(suggested_buffer_len) - .ok() - .filter(|len| *len > 0) - .unwrap_or(1024); - let mut buffer = vec![0; buffer_len]; - - loop { - let mut result = ptr::null_mut(); - let status = unsafe { - libc::getpwuid_r( - uid, - passwd.as_mut_ptr(), - buffer.as_mut_ptr().cast(), - buffer.len(), - &mut result, - ) - }; - - if status == 0 { - if result.is_null() { - return None; - } - - let passwd = unsafe { passwd.assume_init_ref() }; - if passwd.pw_shell.is_null() { - return None; - } - - let shell_path = unsafe { CStr::from_ptr(passwd.pw_shell) } - .to_string_lossy() - .into_owned(); - return Some(PathBuf::from(shell_path)); - } - - if status != libc::ERANGE { - return None; - } - - // Retry with a larger buffer until libc can materialize the passwd entry. - let new_len = buffer.len().checked_mul(2)?; - if new_len > 1024 * 1024 { - return None; - } - buffer.resize(new_len, 0); - } -} - -#[cfg(not(unix))] -fn get_user_shell_path() -> Option { - None -} - -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) - && file_exists(&default_shell_path).is_some() - { - 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| Shell { - shell_type: ShellType::Zsh, - shell_path, - shell_snapshot: empty_shell_snapshot_receiver(), - }) -} - -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| Shell { - shell_type: ShellType::Bash, - shell_path, - shell_snapshot: empty_shell_snapshot_receiver(), - }) -} - -fn get_sh_shell(path: Option<&PathBuf>) -> Option { - let shell_path = get_shell_path(ShellType::Sh, path, "sh", vec!["/bin/sh"]); - - shell_path.map(|shell_path| Shell { - shell_type: ShellType::Sh, - shell_path, - shell_snapshot: empty_shell_snapshot_receiver(), - }) -} - -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| Shell { - shell_type: ShellType::PowerShell, - shell_path, - shell_snapshot: empty_shell_snapshot_receiver(), - }) -} - -fn get_cmd_shell(path: Option<&PathBuf>) -> Option { - let shell_path = get_shell_path(ShellType::Cmd, path, "cmd", vec![]); - - shell_path.map(|shell_path| Shell { - shell_type: ShellType::Cmd, - shell_path, - shell_snapshot: empty_shell_snapshot_receiver(), - }) -} - -fn ultimate_fallback_shell() -> Shell { - if cfg!(windows) { - Shell { - shell_type: ShellType::Cmd, - shell_path: PathBuf::from("cmd.exe"), - shell_snapshot: empty_shell_snapshot_receiver(), - } - } else { - Shell { - shell_type: ShellType::Sh, - shell_path: PathBuf::from("/bin/sh"), - shell_snapshot: empty_shell_snapshot_receiver(), - } - } -} - -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(ultimate_fallback_shell()) -} - -pub fn get_shell(shell_type: ShellType, path: Option<&PathBuf>) -> Option { - match shell_type { - ShellType::Zsh => get_zsh_shell(path), - ShellType::Bash => get_bash_shell(path), - ShellType::PowerShell => get_powershell_shell(path), - ShellType::Sh => get_sh_shell(path), - ShellType::Cmd => get_cmd_shell(path), - } -} - -pub fn default_user_shell() -> Shell { - default_user_shell_from_path(get_user_shell_path()) -} - -fn default_user_shell_from_path(user_shell_path: Option) -> Shell { - if cfg!(windows) { - get_shell(ShellType::PowerShell, /*path*/ None).unwrap_or(ultimate_fallback_shell()) - } else { - let user_default_shell = user_shell_path - .and_then(|shell| detect_shell_type(&shell)) - .and_then(|shell_type| get_shell(shell_type, /*path*/ None)); - - let shell_with_fallback = if cfg!(target_os = "macos") { - user_default_shell - .or_else(|| get_shell(ShellType::Zsh, /*path*/ None)) - .or_else(|| get_shell(ShellType::Bash, /*path*/ None)) - } else { - user_default_shell - .or_else(|| get_shell(ShellType::Bash, /*path*/ None)) - .or_else(|| get_shell(ShellType::Zsh, /*path*/ None)) - }; - - shell_with_fallback.unwrap_or(ultimate_fallback_shell()) - } -} - -#[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(if cfg!(windows) { - "C:\\windows\\System32\\WindowsPowerShell\\v1.0\\powershell.exe" - } else { - "/usr/local/bin/pwsh" - })), - 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) - ); - assert_eq!( - detect_shell_type(&PathBuf::from("/bin/sh")), - Some(ShellType::Sh) - ); - assert_eq!(detect_shell_type(&PathBuf::from("sh")), Some(ShellType::Sh)); - assert_eq!( - detect_shell_type(&PathBuf::from("cmd")), - Some(ShellType::Cmd) - ); - assert_eq!( - detect_shell_type(&PathBuf::from("cmd.exe")), - Some(ShellType::Cmd) - ); - } -} - -#[cfg(test)] -#[cfg(unix)] -#[path = "shell_tests.rs"] -mod tests; +pub use codex_shell::Shell; +pub use codex_shell::ShellType; +pub use codex_shell::default_user_shell; +pub use codex_shell::detect_shell_type; +pub use codex_shell::get_shell; +pub use codex_shell::get_shell_by_model_provided_path; diff --git a/codex-rs/core/src/shell_detect.rs b/codex-rs/core/src/shell_detect.rs deleted file mode 100644 index 3595ab3469..0000000000 --- a/codex-rs/core/src/shell_detect.rs +++ /dev/null @@ -1,24 +0,0 @@ -use crate::shell::ShellType; -use std::path::Path; -use std::path::PathBuf; - -pub(crate) fn detect_shell_type(shell_path: &PathBuf) -> Option { - match shell_path.as_os_str().to_str() { - Some("zsh") => Some(ShellType::Zsh), - Some("sh") => Some(ShellType::Sh), - Some("cmd") => Some(ShellType::Cmd), - 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 { - let shell_name_path = Path::new(shell_name); - if shell_name_path != Path::new(shell_path) { - return detect_shell_type(&shell_name_path.to_path_buf()); - } - } - None - } - } -} diff --git a/codex-rs/core/src/shell_snapshot.rs b/codex-rs/core/src/shell_snapshot.rs index 29b50cb9e8..096f3cd6e8 100644 --- a/codex-rs/core/src/shell_snapshot.rs +++ b/codex-rs/core/src/shell_snapshot.rs @@ -1,490 +1,17 @@ use std::io::ErrorKind; use std::path::Path; use std::path::PathBuf; -use std::process::Stdio; -use std::sync::Arc; -use std::time::Duration; use std::time::SystemTime; use crate::rollout::list::find_thread_path_by_id_str; -use crate::shell::Shell; -use crate::shell::ShellType; -use crate::shell::get_shell; -use anyhow::Context; use anyhow::Result; -use anyhow::anyhow; -use anyhow::bail; -use codex_otel::SessionTelemetry; use codex_protocol::ThreadId; +pub use codex_shell::SNAPSHOT_DIR; +pub use codex_shell::SNAPSHOT_RETENTION; +pub use codex_shell::ShellSnapshot; +use codex_shell::remove_snapshot_file; +pub use codex_shell::snapshot_session_id_from_file_name; use tokio::fs; -use tokio::process::Command; -use tokio::sync::watch; -use tokio::time::timeout; -use tracing::Instrument; -use tracing::info_span; - -#[derive(Clone, Debug, PartialEq, Eq)] -pub struct ShellSnapshot { - pub path: PathBuf, - pub cwd: PathBuf, -} - -const SNAPSHOT_TIMEOUT: Duration = Duration::from_secs(10); -const SNAPSHOT_RETENTION: Duration = Duration::from_secs(60 * 60 * 24 * 3); // 3 days retention. -const SNAPSHOT_DIR: &str = "shell_snapshots"; -const EXCLUDED_EXPORT_VARS: &[&str] = &["PWD", "OLDPWD"]; - -impl ShellSnapshot { - pub fn start_snapshotting( - codex_home: PathBuf, - session_id: ThreadId, - session_cwd: PathBuf, - shell: &mut Shell, - session_telemetry: SessionTelemetry, - ) -> watch::Sender>> { - let (shell_snapshot_tx, shell_snapshot_rx) = watch::channel(None); - shell.shell_snapshot = shell_snapshot_rx; - - Self::spawn_snapshot_task( - codex_home, - session_id, - session_cwd, - shell.clone(), - shell_snapshot_tx.clone(), - session_telemetry, - ); - - shell_snapshot_tx - } - - pub fn refresh_snapshot( - codex_home: PathBuf, - session_id: ThreadId, - session_cwd: PathBuf, - shell: Shell, - shell_snapshot_tx: watch::Sender>>, - session_telemetry: SessionTelemetry, - ) { - Self::spawn_snapshot_task( - codex_home, - session_id, - session_cwd, - shell, - shell_snapshot_tx, - session_telemetry, - ); - } - - fn spawn_snapshot_task( - codex_home: PathBuf, - session_id: ThreadId, - session_cwd: PathBuf, - snapshot_shell: Shell, - shell_snapshot_tx: watch::Sender>>, - session_telemetry: SessionTelemetry, - ) { - let snapshot_span = info_span!("shell_snapshot", thread_id = %session_id); - tokio::spawn( - async move { - let timer = session_telemetry.start_timer("codex.shell_snapshot.duration_ms", &[]); - let snapshot = ShellSnapshot::try_new( - &codex_home, - session_id, - session_cwd.as_path(), - &snapshot_shell, - ) - .await - .map(Arc::new); - let success = snapshot.is_ok(); - let success_tag = if success { "true" } else { "false" }; - let _ = timer.map(|timer| timer.record(&[("success", success_tag)])); - let mut counter_tags = vec![("success", success_tag)]; - if let Some(failure_reason) = snapshot.as_ref().err() { - counter_tags.push(("failure_reason", *failure_reason)); - } - session_telemetry.counter("codex.shell_snapshot", /*inc*/ 1, &counter_tags); - let _ = shell_snapshot_tx.send(snapshot.ok()); - } - .instrument(snapshot_span), - ); - } - - async fn try_new( - codex_home: &Path, - session_id: ThreadId, - session_cwd: &Path, - shell: &Shell, - ) -> std::result::Result { - // File to store the snapshot - let extension = match shell.shell_type { - ShellType::PowerShell => "ps1", - _ => "sh", - }; - let nonce = SystemTime::now() - .duration_since(SystemTime::UNIX_EPOCH) - .map(|duration| duration.as_nanos()) - .unwrap_or(0); - let path = codex_home - .join(SNAPSHOT_DIR) - .join(format!("{session_id}.{nonce}.{extension}")); - let temp_path = codex_home - .join(SNAPSHOT_DIR) - .join(format!("{session_id}.tmp-{nonce}")); - - // Clean the (unlikely) leaked snapshot files. - let codex_home = codex_home.to_path_buf(); - let cleanup_session_id = session_id; - tokio::spawn(async move { - if let Err(err) = cleanup_stale_snapshots(&codex_home, cleanup_session_id).await { - tracing::warn!("Failed to clean up shell snapshots: {err:?}"); - } - }); - - // Make the new snapshot. - let temp_path = - match write_shell_snapshot(shell.shell_type.clone(), &temp_path, session_cwd).await { - Ok(path) => { - tracing::info!("Shell snapshot successfully created: {}", path.display()); - path - } - Err(err) => { - tracing::warn!( - "Failed to create shell snapshot for {}: {err:?}", - shell.name() - ); - return Err("write_failed"); - } - }; - - let temp_snapshot = Self { - path: temp_path.clone(), - cwd: session_cwd.to_path_buf(), - }; - - if let Err(err) = validate_snapshot(shell, &temp_snapshot.path, session_cwd).await { - tracing::error!("Shell snapshot validation failed: {err:?}"); - remove_snapshot_file(&temp_snapshot.path).await; - return Err("validation_failed"); - } - - if let Err(err) = fs::rename(&temp_snapshot.path, &path).await { - tracing::warn!("Failed to finalize shell snapshot: {err:?}"); - remove_snapshot_file(&temp_snapshot.path).await; - return Err("write_failed"); - } - - Ok(Self { - path, - cwd: session_cwd.to_path_buf(), - }) - } -} - -impl Drop for ShellSnapshot { - fn drop(&mut self) { - if let Err(err) = std::fs::remove_file(&self.path) { - tracing::warn!( - "Failed to delete shell snapshot at {:?}: {err:?}", - self.path - ); - } - } -} - -async fn write_shell_snapshot( - shell_type: ShellType, - output_path: &Path, - cwd: &Path, -) -> Result { - if shell_type == ShellType::PowerShell || shell_type == ShellType::Cmd { - bail!("Shell snapshot not supported yet for {shell_type:?}"); - } - let shell = get_shell(shell_type.clone(), /*path*/ None) - .with_context(|| format!("No available shell for {shell_type:?}"))?; - - let raw_snapshot = capture_snapshot(&shell, cwd).await?; - let snapshot = strip_snapshot_preamble(&raw_snapshot)?; - - if let Some(parent) = output_path.parent() { - let parent_display = parent.display(); - fs::create_dir_all(parent) - .await - .with_context(|| format!("Failed to create snapshot parent {parent_display}"))?; - } - - let snapshot_path = output_path.display(); - fs::write(output_path, snapshot) - .await - .with_context(|| format!("Failed to write snapshot to {snapshot_path}"))?; - - Ok(output_path.to_path_buf()) -} - -async fn capture_snapshot(shell: &Shell, cwd: &Path) -> Result { - let shell_type = shell.shell_type.clone(); - match shell_type { - ShellType::Zsh => run_shell_script(shell, &zsh_snapshot_script(), cwd).await, - ShellType::Bash => run_shell_script(shell, &bash_snapshot_script(), cwd).await, - ShellType::Sh => run_shell_script(shell, &sh_snapshot_script(), cwd).await, - ShellType::PowerShell => run_shell_script(shell, powershell_snapshot_script(), cwd).await, - ShellType::Cmd => bail!("Shell snapshotting is not yet supported for {shell_type:?}"), - } -} - -fn strip_snapshot_preamble(snapshot: &str) -> Result { - let marker = "# Snapshot file"; - let Some(start) = snapshot.find(marker) else { - bail!("Snapshot output missing marker {marker}"); - }; - - Ok(snapshot[start..].to_string()) -} - -async fn validate_snapshot(shell: &Shell, snapshot_path: &Path, cwd: &Path) -> Result<()> { - let snapshot_path_display = snapshot_path.display(); - let script = format!("set -e; . \"{snapshot_path_display}\""); - run_script_with_timeout( - shell, - &script, - SNAPSHOT_TIMEOUT, - /*use_login_shell*/ false, - cwd, - ) - .await - .map(|_| ()) -} - -async fn run_shell_script(shell: &Shell, script: &str, cwd: &Path) -> Result { - run_script_with_timeout( - shell, - script, - SNAPSHOT_TIMEOUT, - /*use_login_shell*/ true, - cwd, - ) - .await -} - -async fn run_script_with_timeout( - shell: &Shell, - script: &str, - snapshot_timeout: Duration, - use_login_shell: bool, - cwd: &Path, -) -> Result { - let args = shell.derive_exec_args(script, use_login_shell); - let shell_name = shell.name(); - - // Handler is kept as guard to control the drop. The `mut` pattern is required because .args() - // returns a ref of handler. - let mut handler = Command::new(&args[0]); - handler.args(&args[1..]); - handler.stdin(Stdio::null()); - handler.current_dir(cwd); - #[cfg(unix)] - unsafe { - handler.pre_exec(|| { - codex_utils_pty::process_group::detach_from_tty()?; - Ok(()) - }); - } - handler.kill_on_drop(true); - let output = timeout(snapshot_timeout, handler.output()) - .await - .map_err(|_| anyhow!("Snapshot command timed out for {shell_name}"))? - .with_context(|| format!("Failed to execute {shell_name}"))?; - - if !output.status.success() { - let status = output.status; - let stderr = String::from_utf8_lossy(&output.stderr); - bail!("Snapshot command exited with status {status}: {stderr}"); - } - - Ok(String::from_utf8_lossy(&output.stdout).into_owned()) -} - -fn excluded_exports_regex() -> String { - EXCLUDED_EXPORT_VARS.join("|") -} - -fn zsh_snapshot_script() -> String { - let excluded = excluded_exports_regex(); - let script = r##"if [[ -n "$ZDOTDIR" ]]; then - rc="$ZDOTDIR/.zshrc" -else - rc="$HOME/.zshrc" -fi -[[ -r "$rc" ]] && . "$rc" -print '# Snapshot file' -print '# Unset all aliases to avoid conflicts with functions' -print 'unalias -a 2>/dev/null || true' -print '# Functions' -functions -print '' -setopt_count=$(setopt | wc -l | tr -d ' ') -print "# setopts $setopt_count" -setopt | sed 's/^/setopt /' -print '' -alias_count=$(alias -L | wc -l | tr -d ' ') -print "# aliases $alias_count" -alias -L -print '' -export_lines=$(export -p | awk ' -/^(export|declare -x|typeset -x) / { - line=$0 - name=line - sub(/^(export|declare -x|typeset -x) /, "", name) - sub(/=.*/, "", name) - if (name ~ /^(EXCLUDED_EXPORTS)$/) { - next - } - if (name ~ /^[A-Za-z_][A-Za-z0-9_]*$/) { - print line - } -}') -export_count=$(printf '%s\n' "$export_lines" | sed '/^$/d' | wc -l | tr -d ' ') -print "# exports $export_count" -if [[ -n "$export_lines" ]]; then - print -r -- "$export_lines" -fi -"##; - script.replace("EXCLUDED_EXPORTS", &excluded) -} - -fn bash_snapshot_script() -> String { - let excluded = excluded_exports_regex(); - let script = r##"if [ -z "$BASH_ENV" ] && [ -r "$HOME/.bashrc" ]; then - . "$HOME/.bashrc" -fi -echo '# Snapshot file' -echo '# Unset all aliases to avoid conflicts with functions' -unalias -a 2>/dev/null || true -echo '# Functions' -declare -f -echo '' -bash_opts=$(set -o | awk '$2=="on"{print $1}') -bash_opt_count=$(printf '%s\n' "$bash_opts" | sed '/^$/d' | wc -l | tr -d ' ') -echo "# setopts $bash_opt_count" -if [ -n "$bash_opts" ]; then - printf 'set -o %s\n' $bash_opts -fi -echo '' -alias_count=$(alias -p | wc -l | tr -d ' ') -echo "# aliases $alias_count" -alias -p -echo '' -export_lines=$( - while IFS= read -r name; do - if [[ "$name" =~ ^(EXCLUDED_EXPORTS)$ ]]; then - continue - fi - if [[ ! "$name" =~ ^[A-Za-z_][A-Za-z0-9_]*$ ]]; then - continue - fi - declare -xp "$name" 2>/dev/null || true - done < <(compgen -e) -) -export_count=$(printf '%s\n' "$export_lines" | sed '/^$/d' | wc -l | tr -d ' ') -echo "# exports $export_count" -if [ -n "$export_lines" ]; then - printf '%s\n' "$export_lines" -fi -"##; - script.replace("EXCLUDED_EXPORTS", &excluded) -} - -fn sh_snapshot_script() -> String { - let excluded = excluded_exports_regex(); - let script = r##"if [ -n "$ENV" ] && [ -r "$ENV" ]; then - . "$ENV" -fi -echo '# Snapshot file' -echo '# Unset all aliases to avoid conflicts with functions' -unalias -a 2>/dev/null || true -echo '# Functions' -if command -v typeset >/dev/null 2>&1; then - typeset -f -elif command -v declare >/dev/null 2>&1; then - declare -f -fi -echo '' -if set -o >/dev/null 2>&1; then - sh_opts=$(set -o | awk '$2=="on"{print $1}') - sh_opt_count=$(printf '%s\n' "$sh_opts" | sed '/^$/d' | wc -l | tr -d ' ') - echo "# setopts $sh_opt_count" - if [ -n "$sh_opts" ]; then - printf 'set -o %s\n' $sh_opts - fi -else - echo '# setopts 0' -fi -echo '' -if alias >/dev/null 2>&1; then - alias_count=$(alias | wc -l | tr -d ' ') - echo "# aliases $alias_count" - alias - echo '' -else - echo '# aliases 0' -fi -if export -p >/dev/null 2>&1; then - export_lines=$(export -p | awk ' -/^(export|declare -x|typeset -x) / { - line=$0 - name=line - sub(/^(export|declare -x|typeset -x) /, "", name) - sub(/=.*/, "", name) - if (name ~ /^(EXCLUDED_EXPORTS)$/) { - next - } - if (name ~ /^[A-Za-z_][A-Za-z0-9_]*$/) { - print line - } -}') - export_count=$(printf '%s\n' "$export_lines" | sed '/^$/d' | wc -l | tr -d ' ') - echo "# exports $export_count" - if [ -n "$export_lines" ]; then - printf '%s\n' "$export_lines" - fi -else - export_count=$(env | sort | awk -F= '$1 ~ /^[A-Za-z_][A-Za-z0-9_]*$/ { count++ } END { print count }') - echo "# exports $export_count" - env | sort | while IFS='=' read -r key value; do - case "$key" in - ""|[0-9]*|*[!A-Za-z0-9_]*|EXCLUDED_EXPORTS) continue ;; - esac - escaped=$(printf "%s" "$value" | sed "s/'/'\"'\"'/g") - printf "export %s='%s'\n" "$key" "$escaped" - done -fi -"##; - script.replace("EXCLUDED_EXPORTS", &excluded) -} - -fn powershell_snapshot_script() -> &'static str { - r##"$ErrorActionPreference = 'Stop' -Write-Output '# Snapshot file' -Write-Output '# Unset all aliases to avoid conflicts with functions' -Write-Output 'Remove-Item Alias:* -ErrorAction SilentlyContinue' -Write-Output '# Functions' -Get-ChildItem Function: | ForEach-Object { - "function {0} {{`n{1}`n}}" -f $_.Name, $_.Definition -} -Write-Output '' -$aliases = Get-Alias -Write-Output ("# aliases " + $aliases.Count) -$aliases | ForEach-Object { - "Set-Alias -Name {0} -Value {1}" -f $_.Name, $_.Definition -} -Write-Output '' -$envVars = Get-ChildItem Env: -Write-Output ("# exports " + $envVars.Count) -$envVars | ForEach-Object { - $escaped = $_.Value -replace "'", "''" - "`$env:{0}='{1}'" -f $_.Name, $escaped -} -"## -} /// Removes shell snapshots that either lack a matching session rollout file or /// whose rollouts have not been updated within the retention window. @@ -547,22 +74,12 @@ pub async fn cleanup_stale_snapshots(codex_home: &Path, active_session_id: Threa Ok(()) } -async fn remove_snapshot_file(path: &Path) { - if let Err(err) = fs::remove_file(path).await { - tracing::warn!("Failed to delete shell snapshot at {:?}: {err:?}", path); - } -} - -fn snapshot_session_id_from_file_name(file_name: &str) -> Option<&str> { - let (stem, extension) = file_name.rsplit_once('.')?; - match extension { - "sh" | "ps1" => Some( - stem.split_once('.') - .map_or(stem, |(session_id, _generation)| session_id), - ), - _ if extension.starts_with("tmp-") => Some(stem), - _ => None, - } +pub(crate) fn spawn_stale_snapshot_cleanup(codex_home: PathBuf, active_session_id: ThreadId) { + tokio::spawn(async move { + if let Err(err) = cleanup_stale_snapshots(&codex_home, active_session_id).await { + tracing::warn!("Failed to clean up shell snapshots: {err:?}"); + } + }); } #[cfg(test)] diff --git a/codex-rs/core/src/shell_snapshot_tests.rs b/codex-rs/core/src/shell_snapshot_tests.rs index ff700ff7a6..79b27f2c23 100644 --- a/codex-rs/core/src/shell_snapshot_tests.rs +++ b/codex-rs/core/src/shell_snapshot_tests.rs @@ -1,101 +1,26 @@ use super::*; +use anyhow::Result; use pretty_assertions::assert_eq; #[cfg(unix)] use std::os::unix::ffi::OsStrExt; +use std::path::Path; +use std::path::PathBuf; #[cfg(unix)] -use std::process::Command; -#[cfg(target_os = "linux")] -use std::process::Command as StdCommand; - +use std::time::Duration; +#[cfg(unix)] +use std::time::SystemTime; use tempfile::tempdir; -#[cfg(unix)] -struct BlockingStdinPipe { - original: i32, - write_end: i32, -} - -#[cfg(unix)] -impl BlockingStdinPipe { - fn install() -> Result { - let mut fds = [0i32; 2]; - if unsafe { libc::pipe(fds.as_mut_ptr()) } == -1 { - return Err(std::io::Error::last_os_error()).context("create stdin pipe"); - } - - let original = unsafe { libc::dup(libc::STDIN_FILENO) }; - if original == -1 { - let err = std::io::Error::last_os_error(); - unsafe { - libc::close(fds[0]); - libc::close(fds[1]); - } - return Err(err).context("dup stdin"); - } - - if unsafe { libc::dup2(fds[0], libc::STDIN_FILENO) } == -1 { - let err = std::io::Error::last_os_error(); - unsafe { - libc::close(fds[0]); - libc::close(fds[1]); - libc::close(original); - } - return Err(err).context("replace stdin"); - } - - unsafe { - libc::close(fds[0]); - } - - Ok(Self { - original, - write_end: fds[1], - }) - } -} - -#[cfg(unix)] -impl Drop for BlockingStdinPipe { - fn drop(&mut self) { - unsafe { - libc::dup2(self.original, libc::STDIN_FILENO); - libc::close(self.original); - libc::close(self.write_end); - } - } -} - -#[cfg(not(target_os = "windows"))] -fn assert_posix_snapshot_sections(snapshot: &str) { - assert!(snapshot.contains("# Snapshot file")); - assert!(snapshot.contains("aliases ")); - assert!(snapshot.contains("exports ")); - assert!( - snapshot.contains("PATH"), - "snapshot should capture a PATH export" - ); - assert!(snapshot.contains("setopts ")); -} - -async fn get_snapshot(shell_type: ShellType) -> Result { - let dir = tempdir()?; - let path = dir.path().join("snapshot.sh"); - write_shell_snapshot(shell_type, &path, dir.path()).await?; - let content = fs::read_to_string(&path).await?; - Ok(content) -} - -#[test] -fn strip_snapshot_preamble_removes_leading_output() { - let snapshot = "noise\n# Snapshot file\nexport PATH=/bin\n"; - let cleaned = strip_snapshot_preamble(snapshot).expect("snapshot marker exists"); - assert_eq!(cleaned, "# Snapshot file\nexport PATH=/bin\n"); -} - -#[test] -fn strip_snapshot_preamble_requires_marker() { - let result = strip_snapshot_preamble("missing header"); - assert!(result.is_err()); +async fn write_rollout_stub(codex_home: &Path, session_id: ThreadId) -> Result { + let dir = codex_home + .join("sessions") + .join("2025") + .join("01") + .join("01"); + fs::create_dir_all(&dir).await?; + let path = dir.join(format!("rollout-2025-01-01T00-00-00-{session_id}.jsonl")); + fs::write(&path, "").await?; + Ok(path) } #[test] @@ -120,286 +45,6 @@ fn snapshot_file_name_parser_supports_legacy_and_suffixed_names() { ); } -#[cfg(unix)] -#[test] -fn bash_snapshot_filters_invalid_exports() -> Result<()> { - let output = Command::new("/bin/bash") - .arg("-c") - .arg(bash_snapshot_script()) - .env("BASH_ENV", "/dev/null") - .env("VALID_NAME", "ok") - .env("PWD", "/tmp/stale") - .env("NEXTEST_BIN_EXE_codex-write-config-schema", "/path/to/bin") - .env("BAD-NAME", "broken") - .output()?; - - assert!(output.status.success()); - - let stdout = String::from_utf8_lossy(&output.stdout); - assert!(stdout.contains("VALID_NAME")); - assert!(!stdout.contains("PWD=/tmp/stale")); - assert!(!stdout.contains("NEXTEST_BIN_EXE_codex-write-config-schema")); - assert!(!stdout.contains("BAD-NAME")); - - Ok(()) -} - -#[cfg(unix)] -#[test] -fn bash_snapshot_preserves_multiline_exports() -> Result<()> { - let multiline_cert = "-----BEGIN CERTIFICATE-----\nabc\n-----END CERTIFICATE-----"; - let output = Command::new("/bin/bash") - .arg("-c") - .arg(bash_snapshot_script()) - .env("BASH_ENV", "/dev/null") - .env("MULTILINE_CERT", multiline_cert) - .output()?; - - assert!(output.status.success()); - - let stdout = String::from_utf8_lossy(&output.stdout); - assert!( - stdout.contains("MULTILINE_CERT=") || stdout.contains("MULTILINE_CERT"), - "snapshot should include the multiline export name" - ); - - let dir = tempdir()?; - let snapshot_path = dir.path().join("snapshot.sh"); - std::fs::write(&snapshot_path, stdout.as_bytes())?; - - let validate = Command::new("/bin/bash") - .arg("-c") - .arg("set -e; . \"$1\"") - .arg("bash") - .arg(&snapshot_path) - .env("BASH_ENV", "/dev/null") - .output()?; - - assert!( - validate.status.success(), - "snapshot validation failed: {}", - String::from_utf8_lossy(&validate.stderr) - ); - - Ok(()) -} - -#[cfg(unix)] -#[tokio::test] -async fn try_new_creates_and_deletes_snapshot_file() -> Result<()> { - let dir = tempdir()?; - let shell = Shell { - shell_type: ShellType::Bash, - shell_path: PathBuf::from("/bin/bash"), - shell_snapshot: crate::shell::empty_shell_snapshot_receiver(), - }; - - let snapshot = ShellSnapshot::try_new(dir.path(), ThreadId::new(), dir.path(), &shell) - .await - .expect("snapshot should be created"); - let path = snapshot.path.clone(); - assert!(path.exists()); - assert_eq!(snapshot.cwd, dir.path().to_path_buf()); - - drop(snapshot); - - assert!(!path.exists()); - - Ok(()) -} - -#[cfg(unix)] -#[tokio::test] -async fn try_new_uses_distinct_generation_paths() -> Result<()> { - let dir = tempdir()?; - let session_id = ThreadId::new(); - let shell = Shell { - shell_type: ShellType::Bash, - shell_path: PathBuf::from("/bin/bash"), - shell_snapshot: crate::shell::empty_shell_snapshot_receiver(), - }; - - let initial_snapshot = ShellSnapshot::try_new(dir.path(), session_id, dir.path(), &shell) - .await - .expect("initial snapshot should be created"); - let refreshed_snapshot = ShellSnapshot::try_new(dir.path(), session_id, dir.path(), &shell) - .await - .expect("refreshed snapshot should be created"); - let initial_path = initial_snapshot.path.clone(); - let refreshed_path = refreshed_snapshot.path.clone(); - - assert_ne!(initial_path, refreshed_path); - assert_eq!(initial_path.exists(), true); - assert_eq!(refreshed_path.exists(), true); - - drop(initial_snapshot); - - assert_eq!(initial_path.exists(), false); - assert_eq!(refreshed_path.exists(), true); - - drop(refreshed_snapshot); - - assert_eq!(refreshed_path.exists(), false); - - Ok(()) -} - -#[cfg(unix)] -#[tokio::test] -async fn snapshot_shell_does_not_inherit_stdin() -> Result<()> { - let _stdin_guard = BlockingStdinPipe::install()?; - - let dir = tempdir()?; - let home = dir.path(); - let read_status_path = home.join("stdin-read-status"); - let read_status_display = read_status_path.display(); - // Persist the startup `read` exit status so the test can assert whether - // bash saw EOF on stdin after the snapshot process exits. - let bashrc = format!("read -t 1 -r ignored\nprintf '%s' \"$?\" > \"{read_status_display}\"\n"); - fs::write(home.join(".bashrc"), bashrc).await?; - - let shell = Shell { - shell_type: ShellType::Bash, - shell_path: PathBuf::from("/bin/bash"), - shell_snapshot: crate::shell::empty_shell_snapshot_receiver(), - }; - - let home_display = home.display(); - let script = format!( - "HOME=\"{home_display}\"; export HOME; {}", - bash_snapshot_script() - ); - let output = run_script_with_timeout( - &shell, - &script, - Duration::from_secs(2), - /*use_login_shell*/ true, - home, - ) - .await - .context("run snapshot command")?; - let read_status = fs::read_to_string(&read_status_path) - .await - .context("read stdin probe status")?; - - assert_eq!( - read_status, "1", - "expected shell startup read to see EOF on stdin; status={read_status:?}" - ); - - assert!( - output.contains("# Snapshot file"), - "expected snapshot marker in output; output={output:?}" - ); - - Ok(()) -} - -#[cfg(target_os = "linux")] -#[tokio::test] -async fn timed_out_snapshot_shell_is_terminated() -> Result<()> { - use std::process::Stdio; - use tokio::time::Duration as TokioDuration; - use tokio::time::Instant; - use tokio::time::sleep; - - let dir = tempdir()?; - let pid_path = dir.path().join("pid"); - let script = format!("echo $$ > \"{}\"; sleep 30", pid_path.display()); - - let shell = Shell { - shell_type: ShellType::Sh, - shell_path: PathBuf::from("/bin/sh"), - shell_snapshot: crate::shell::empty_shell_snapshot_receiver(), - }; - - let err = run_script_with_timeout( - &shell, - &script, - Duration::from_secs(1), - /*use_login_shell*/ true, - dir.path(), - ) - .await - .expect_err("snapshot shell should time out"); - assert!( - err.to_string().contains("timed out"), - "expected timeout error, got {err:?}" - ); - - let pid = fs::read_to_string(&pid_path) - .await - .expect("snapshot shell writes its pid before timing out") - .trim() - .parse::()?; - - let deadline = Instant::now() + TokioDuration::from_secs(1); - loop { - let kill_status = StdCommand::new("kill") - .arg("-0") - .arg(pid.to_string()) - .stderr(Stdio::null()) - .stdout(Stdio::null()) - .status()?; - if !kill_status.success() { - break; - } - if Instant::now() >= deadline { - panic!("timed out snapshot shell is still alive after grace period"); - } - sleep(TokioDuration::from_millis(50)).await; - } - - Ok(()) -} - -#[cfg(target_os = "macos")] -#[tokio::test] -async fn macos_zsh_snapshot_includes_sections() -> Result<()> { - let snapshot = get_snapshot(ShellType::Zsh).await?; - assert_posix_snapshot_sections(&snapshot); - Ok(()) -} - -#[cfg(target_os = "linux")] -#[tokio::test] -async fn linux_bash_snapshot_includes_sections() -> Result<()> { - let snapshot = get_snapshot(ShellType::Bash).await?; - assert_posix_snapshot_sections(&snapshot); - Ok(()) -} - -#[cfg(target_os = "linux")] -#[tokio::test] -async fn linux_sh_snapshot_includes_sections() -> Result<()> { - let snapshot = get_snapshot(ShellType::Sh).await?; - assert_posix_snapshot_sections(&snapshot); - Ok(()) -} - -#[cfg(target_os = "windows")] -#[ignore] -#[tokio::test] -async fn windows_powershell_snapshot_includes_sections() -> Result<()> { - let snapshot = get_snapshot(ShellType::PowerShell).await?; - assert!(snapshot.contains("# Snapshot file")); - assert!(snapshot.contains("aliases ")); - assert!(snapshot.contains("exports ")); - Ok(()) -} - -async fn write_rollout_stub(codex_home: &Path, session_id: ThreadId) -> Result { - let dir = codex_home - .join("sessions") - .join("2025") - .join("01") - .join("01"); - fs::create_dir_all(&dir).await?; - let path = dir.join(format!("rollout-2025-01-01T00-00-00-{session_id}.jsonl")); - fs::write(&path, "").await?; - Ok(path) -} - #[tokio::test] async fn cleanup_stale_snapshots_removes_orphans_and_keeps_live() -> Result<()> { let dir = tempdir()?; @@ -476,7 +121,7 @@ fn set_file_mtime(path: &Path, age: Duration) -> Result<()> { .saturating_sub(age.as_secs()); let tv_sec = now .try_into() - .map_err(|_| anyhow!("Snapshot mtime is out of range for libc::timespec"))?; + .map_err(|_| anyhow::anyhow!("Snapshot mtime is out of range for libc::timespec"))?; let ts = libc::timespec { tv_sec, tv_nsec: 0 }; let times = [ts, ts]; let c_path = std::ffi::CString::new(path.as_os_str().as_bytes())?; diff --git a/codex-rs/core/src/state/service.rs b/codex-rs/core/src/state/service.rs index a7c384202f..f8e8311e28 100644 --- a/codex-rs/core/src/state/service.rs +++ b/codex-rs/core/src/state/service.rs @@ -1,5 +1,3 @@ -use std::sync::Arc; - use crate::RolloutRecorder; use crate::SkillsManager; use crate::agent::AgentControl; @@ -22,9 +20,9 @@ use codex_mcp::mcp_connection_manager::McpConnectionManager; use codex_otel::SessionTelemetry; use codex_rollout::state_db::StateDbHandle; use std::path::PathBuf; +use std::sync::Arc; use tokio::sync::Mutex; use tokio::sync::RwLock; -use tokio::sync::watch; use tokio_util::sync::CancellationToken; pub(crate) struct SessionServices { @@ -39,7 +37,6 @@ pub(crate) struct SessionServices { pub(crate) hooks: Hooks, pub(crate) rollout: Mutex>, pub(crate) user_shell: Arc, - pub(crate) shell_snapshot_tx: watch::Sender>>, pub(crate) show_raw_agent_reasoning: bool, pub(crate) exec_policy: Arc, pub(crate) auth_manager: Arc, diff --git a/codex-rs/core/src/tools/handlers/shell_tests.rs b/codex-rs/core/src/tools/handlers/shell_tests.rs index fee47c2147..40bee160c2 100644 --- a/codex-rs/core/src/tools/handlers/shell_tests.rs +++ b/codex-rs/core/src/tools/handlers/shell_tests.rs @@ -9,7 +9,6 @@ use crate::exec_env::create_env; use crate::sandboxing::SandboxPermissions; use crate::shell::Shell; use crate::shell::ShellType; -use crate::shell_snapshot::ShellSnapshot; use crate::tools::context::FunctionToolOutput; use crate::tools::context::ToolInvocation; use crate::tools::context::ToolPayload; @@ -22,42 +21,25 @@ use codex_shell_command::powershell::try_find_powershell_executable_blocking; use codex_shell_command::powershell::try_find_pwsh_executable_blocking; use serde_json::json; use tokio::sync::Mutex; -use tokio::sync::watch; /// The logic for is_known_safe_command() has heuristics for known shells, /// so we must ensure the commands generated by [ShellCommandHandler] can be /// recognized as safe if the `command` is safe. #[test] fn commands_generated_by_shell_command_handler_can_be_matched_by_is_known_safe_command() { - let bash_shell = Shell { - shell_type: ShellType::Bash, - shell_path: PathBuf::from("/bin/bash"), - shell_snapshot: crate::shell::empty_shell_snapshot_receiver(), - }; + let bash_shell = Shell::new(ShellType::Bash, PathBuf::from("/bin/bash")); assert_safe(&bash_shell, "ls -la"); - let zsh_shell = Shell { - shell_type: ShellType::Zsh, - shell_path: PathBuf::from("/bin/zsh"), - shell_snapshot: crate::shell::empty_shell_snapshot_receiver(), - }; + let zsh_shell = Shell::new(ShellType::Zsh, PathBuf::from("/bin/zsh")); assert_safe(&zsh_shell, "ls -la"); if let Some(path) = try_find_powershell_executable_blocking() { - let powershell = Shell { - shell_type: ShellType::PowerShell, - shell_path: path.to_path_buf(), - shell_snapshot: crate::shell::empty_shell_snapshot_receiver(), - }; + let powershell = Shell::new(ShellType::PowerShell, path.to_path_buf()); assert_safe(&powershell, "ls -Name"); } if let Some(path) = try_find_pwsh_executable_blocking() { - let pwsh = Shell { - shell_type: ShellType::PowerShell, - shell_path: path.to_path_buf(), - shell_snapshot: crate::shell::empty_shell_snapshot_receiver(), - }; + let pwsh = Shell::new(ShellType::PowerShell, path.to_path_buf()); assert_safe(&pwsh, "ls -Name"); } } @@ -124,15 +106,7 @@ async fn shell_command_handler_to_exec_params_uses_session_shell_and_turn_contex #[test] fn shell_command_handler_respects_explicit_login_flag() { - let (_tx, shell_snapshot) = watch::channel(Some(Arc::new(ShellSnapshot { - path: PathBuf::from("/tmp/snapshot.sh"), - cwd: PathBuf::from("/tmp"), - }))); - let shell = Shell { - shell_type: ShellType::Bash, - shell_path: PathBuf::from("/bin/bash"), - shell_snapshot, - }; + let shell = Shell::new(ShellType::Bash, PathBuf::from("/bin/bash")); let login_command = ShellCommandHandler::base_command( &shell, diff --git a/codex-rs/core/src/tools/handlers/unified_exec.rs b/codex-rs/core/src/tools/handlers/unified_exec.rs index 98d7d717fc..e3438ed5b5 100644 --- a/codex-rs/core/src/tools/handlers/unified_exec.rs +++ b/codex-rs/core/src/tools/handlers/unified_exec.rs @@ -392,11 +392,10 @@ pub(crate) fn get_command( match shell_mode { UnifiedExecShellMode::Direct => { - let model_shell = args.shell.as_ref().map(|shell_str| { - let mut shell = get_shell_by_model_provided_path(&PathBuf::from(shell_str)); - shell.shell_snapshot = crate::shell::empty_shell_snapshot_receiver(); - shell - }); + let model_shell = args + .shell + .as_ref() + .map(|shell_str| get_shell_by_model_provided_path(&PathBuf::from(shell_str))); let shell = model_shell.as_ref().unwrap_or(session_shell.as_ref()); Ok(shell.derive_exec_args(&args.cmd, use_login_shell)) } diff --git a/codex-rs/core/src/tools/runtimes/mod.rs b/codex-rs/core/src/tools/runtimes/mod.rs index 53890a89b7..d15328672b 100644 --- a/codex-rs/core/src/tools/runtimes/mod.rs +++ b/codex-rs/core/src/tools/runtimes/mod.rs @@ -38,7 +38,7 @@ pub(crate) fn build_sandbox_command( /// POSIX-only helper: for commands produced by `Shell::derive_exec_args` /// for Bash/Zsh/sh of the form `[shell_path, "-lc", "