From 022c23f52907a3b1754c3a8b7b2acd9227fe86b1 Mon Sep 17 00:00:00 2001 From: jimmyfraiture Date: Mon, 8 Sep 2025 11:39:02 -0700 Subject: [PATCH] R3 --- codex-rs/core/src/codex.rs | 15 +++++---------- codex-rs/core/src/shell.rs | 20 +++++++++++++++----- 2 files changed, 20 insertions(+), 15 deletions(-) diff --git a/codex-rs/core/src/codex.rs b/codex-rs/core/src/codex.rs index 82d35eca9f..df182babb1 100644 --- a/codex-rs/core/src/codex.rs +++ b/codex-rs/core/src/codex.rs @@ -108,7 +108,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::ShellSnapshot; use crate::turn_diff_tracker::TurnDiffTracker; use crate::user_instructions::UserInstructions; use crate::user_notification::UserNotification; @@ -292,7 +291,6 @@ pub(crate) struct Session { codex_linux_sandbox_exe: Option, user_shell: shell::Shell, show_raw_agent_reasoning: bool, - shell_snapshot: Option, } /// The context needed for a single turn of the conversation. @@ -411,8 +409,6 @@ impl Session { ..Default::default() }; - 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 { Ok((mgr, failures)) => (mgr, failures), @@ -480,7 +476,6 @@ impl Session { codex_linux_sandbox_exe: config.codex_linux_sandbox_exe.clone(), user_shell: default_shell, show_raw_agent_reasoning: config.show_raw_agent_reasoning, - shell_snapshot, }); // Dispatch the SessionConfiguredEvent first and then report any errors. @@ -953,9 +948,6 @@ impl Session { impl Drop for Session { fn drop(&mut self) { self.interrupt_task(); - if let Some(shell_snapshot) = &self.shell_snapshot { - shell::delete_shell_snapshot(&shell_snapshot.path); - } } } @@ -2949,6 +2941,7 @@ mod tests { use shell::ShellSnapshot; use std::collections::HashMap; use std::path::PathBuf; + use std::sync::Arc; use std::time::Duration as StdDuration; fn text_block(s: &str) -> ContentBlock { @@ -2970,7 +2963,7 @@ mod tests { } } - fn zsh_shell(shell_snapshot: Option) -> shell::Shell { + fn zsh_shell(shell_snapshot: Option>) -> shell::Shell { shell::Shell::Posix(shell::PosixShell { shell_path: "/bin/zsh".to_string(), rc_path: "/Users/example/.zshrc".to_string(), @@ -2988,7 +2981,9 @@ mod tests { #[test] fn translates_commands_for_zsh_with_snapshot() { let policy = shell_policy_with_profile(false); - let shell = zsh_shell(Some(ShellSnapshot::new(PathBuf::from("/tmp/snapshot")))); + let shell = zsh_shell(Some(Arc::new(ShellSnapshot::new(PathBuf::from( + "/tmp/snapshot", + ))))); assert!(should_translate_shell_command(&shell, &policy)); } diff --git a/codex-rs/core/src/shell.rs b/codex-rs/core/src/shell.rs index cd7e9f0fa4..ae26e2c03a 100644 --- a/codex-rs/core/src/shell.rs +++ b/codex-rs/core/src/shell.rs @@ -3,10 +3,12 @@ use serde::Serialize; use shlex; use std::path::Path; use std::path::PathBuf; +use std::sync::Arc; use tracing::trace; use uuid::Uuid; -#[derive(Debug, PartialEq, Eq, Clone, Serialize, Deserialize)] +#[derive(Debug, PartialEq, Eq, Serialize, Deserialize)] +/// This structure cannot derive Clone or this will break the Drop implementation. pub struct ShellSnapshot { pub(crate) path: PathBuf, } @@ -17,11 +19,18 @@ impl ShellSnapshot { } } +impl Drop for ShellSnapshot { + fn drop(&mut self) { + delete_shell_snapshot(&self.path); + } +} + #[derive(Debug, PartialEq, Eq, Clone, Serialize, Deserialize)] pub struct PosixShell { pub(crate) shell_path: String, pub(crate) rc_path: String, - pub(crate) shell_snapshot: Option, + #[serde(skip_serializing, skip_deserializing)] + pub(crate) shell_snapshot: Option>, } #[derive(Debug, PartialEq, Eq, Clone, Serialize, Deserialize)] @@ -121,7 +130,7 @@ impl Shell { } } - pub fn get_snapshot(&self) -> Option { + pub fn get_snapshot(&self) -> Option> { match self { Shell::Posix(shell) => shell.shell_snapshot.clone(), _ => None, @@ -168,7 +177,8 @@ async fn detect_default_user_shell(session_id: Uuid, codex_home: &Path) -> Shell if snapshot_path.is_none() { trace!("failed to prepare posix snapshot; using live profile"); } - let shell_snapshot = snapshot_path.map(ShellSnapshot::new); + let shell_snapshot = + snapshot_path.map(|snapshot| Arc::new(ShellSnapshot::new(snapshot))); let rc_path = if shell_path.ends_with("/zsh") { format!("{home_path}/.zshrc") @@ -578,7 +588,7 @@ mod macos_tests { std::fs::write(&path, "# test zshrc").unwrap(); path.to_string_lossy().to_string() }, - shell_snapshot: Some(ShellSnapshot::new(snapshot_path.clone())), + shell_snapshot: Some(Arc::new(ShellSnapshot::new(snapshot_path.clone()))), }); let invocation = shell.format_default_shell_invocation(vec!["echo".to_string()]);