diff --git a/codex-rs/Cargo.lock b/codex-rs/Cargo.lock index 95f4fecc4..ce10e7ef2 100644 --- a/codex-rs/Cargo.lock +++ b/codex-rs/Cargo.lock @@ -2601,7 +2601,7 @@ checksum = "0ce92ff622d6dadf7349484f42c93271a0d49b7cc4d466a936405bacbe10aa78" dependencies = [ "cfg-if", "rustix 1.0.8", - "windows-sys 0.52.0", + "windows-sys 0.59.0", ] [[package]] @@ -3505,7 +3505,7 @@ checksum = "e04d7f318608d35d4b61ddd75cbdaee86b023ebe2bd5a66ee0915f0bf93095a9" dependencies = [ "hermit-abi", "libc", - "windows-sys 0.52.0", + "windows-sys 0.59.0", ] [[package]] @@ -5294,7 +5294,7 @@ dependencies = [ "errno", "libc", "linux-raw-sys 0.4.15", - "windows-sys 0.52.0", + "windows-sys 0.59.0", ] [[package]] @@ -7475,7 +7475,7 @@ version = "0.1.9" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "cf221c93e13a30d793f7645a0e7762c55d169dbb0a49671918a2319d289b10bb" dependencies = [ - "windows-sys 0.52.0", + "windows-sys 0.59.0", ] [[package]] diff --git a/codex-rs/app-server/tests/common/lib.rs b/codex-rs/app-server/tests/common/lib.rs index 825b063c9..a095a713a 100644 --- a/codex-rs/app-server/tests/common/lib.rs +++ b/codex-rs/app-server/tests/common/lib.rs @@ -12,6 +12,8 @@ pub use auth_fixtures::write_chatgpt_auth; use codex_app_server_protocol::JSONRPCResponse; pub use core_test_support::format_with_current_shell; pub use core_test_support::format_with_current_shell_display; +pub use core_test_support::format_with_current_shell_display_non_login; +pub use core_test_support::format_with_current_shell_non_login; pub use mcp_process::McpProcess; pub use mock_model_server::create_mock_chat_completions_server; pub use mock_model_server::create_mock_chat_completions_server_unchecked; diff --git a/codex-rs/core/src/shell.rs b/codex-rs/core/src/shell.rs index 608d80632..d22b6543a 100644 --- a/codex-rs/core/src/shell.rs +++ b/codex-rs/core/src/shell.rs @@ -63,33 +63,6 @@ impl Shell { } } } - - pub(crate) fn wrap_command_with_snapshot(&self, command: &[String]) -> Vec { - let Some(snapshot) = &self.shell_snapshot else { - return command.to_vec(); - }; - - if command.is_empty() { - return command.to_vec(); - } - - match self.shell_type { - ShellType::Zsh | ShellType::Bash | ShellType::Sh => { - let mut args = self.derive_exec_args(". \"$0\" && exec \"$@\"", false); - args.push(snapshot.path.to_string_lossy().to_string()); - args.extend_from_slice(command); - args - } - ShellType::PowerShell => { - let mut args = - self.derive_exec_args("param($snapshot) . $snapshot; & @args", false); - args.push(snapshot.path.to_string_lossy().to_string()); - args.extend_from_slice(command); - args - } - ShellType::Cmd => command.to_vec(), - } - } } #[cfg(unix)] diff --git a/codex-rs/core/src/shell_snapshot.rs b/codex-rs/core/src/shell_snapshot.rs index 2c4c423f5..b27548756 100644 --- a/codex-rs/core/src/shell_snapshot.rs +++ b/codex-rs/core/src/shell_snapshot.rs @@ -257,7 +257,7 @@ mod tests { use std::os::unix::fs::PermissionsExt; #[cfg(target_os = "linux")] use std::process::Command as StdCommand; - use std::sync::Arc; + use tempfile::tempdir; #[cfg(not(target_os = "windows"))] @@ -293,53 +293,6 @@ mod tests { assert!(result.is_err()); } - #[cfg(unix)] - #[test] - fn wrap_command_with_snapshot_wraps_bash_shell() { - let snapshot_path = PathBuf::from("/tmp/snapshot.sh"); - let shell = Shell { - shell_type: ShellType::Bash, - shell_path: PathBuf::from("/bin/bash"), - shell_snapshot: Some(Arc::new(ShellSnapshot { - path: snapshot_path.clone(), - })), - }; - let original_command = vec![ - "bash".to_string(), - "-lc".to_string(), - "echo hello".to_string(), - ]; - - let wrapped = shell.wrap_command_with_snapshot(&original_command); - - let mut expected = shell.derive_exec_args(". \"$0\" && exec \"$@\"", false); - expected.push(snapshot_path.to_string_lossy().to_string()); - expected.extend_from_slice(&original_command); - - assert_eq!(wrapped, expected); - } - - #[test] - fn wrap_command_with_snapshot_preserves_cmd_shell() { - let snapshot_path = PathBuf::from("C:\\snapshot.cmd"); - let shell = Shell { - shell_type: ShellType::Cmd, - shell_path: PathBuf::from("cmd"), - shell_snapshot: Some(Arc::new(ShellSnapshot { - path: snapshot_path, - })), - }; - let original_command = vec![ - "cmd".to_string(), - "/c".to_string(), - "echo hello".to_string(), - ]; - - let wrapped = shell.wrap_command_with_snapshot(&original_command); - - assert_eq!(wrapped, original_command); - } - #[cfg(unix)] #[tokio::test] async fn try_new_creates_and_deletes_snapshot_file() -> Result<()> { diff --git a/codex-rs/core/src/tools/handlers/shell.rs b/codex-rs/core/src/tools/handlers/shell.rs index 9c306a186..ded9c7ac4 100644 --- a/codex-rs/core/src/tools/handlers/shell.rs +++ b/codex-rs/core/src/tools/handlers/shell.rs @@ -10,6 +10,7 @@ use crate::exec_policy::create_exec_approval_requirement_for_command; use crate::function_tool::FunctionCallError; use crate::is_safe_command::is_known_safe_command; use crate::protocol::ExecCommandSource; +use crate::shell::Shell; use crate::tools::context::ToolInvocation; use crate::tools::context::ToolOutput; use crate::tools::context::ToolPayload; @@ -42,13 +43,18 @@ impl ShellHandler { } impl ShellCommandHandler { + fn base_command(shell: &Shell, command: &str, login: Option) -> Vec { + let use_login_shell = login.unwrap_or(true); + shell.derive_exec_args(command, use_login_shell) + } + fn to_exec_params( params: ShellCommandToolCallParams, session: &crate::codex::Session, turn_context: &TurnContext, ) -> ExecParams { let shell = session.user_shell(); - let command = shell.derive_exec_args(¶ms.command, params.login.unwrap_or(true)); + let command = Self::base_command(shell.as_ref(), ¶ms.command, params.login); ExecParams { command, @@ -155,7 +161,7 @@ impl ToolHandler for ShellCommandHandler { serde_json::from_str::(arguments) .map(|params| { let shell = invocation.session.user_shell(); - let command = shell.derive_exec_args(¶ms.command, params.login.unwrap_or(true)); + let command = Self::base_command(shell.as_ref(), ¶ms.command, params.login); !is_known_safe_command(&command) }) .unwrap_or(true) @@ -289,6 +295,7 @@ impl ShellHandler { #[cfg(test)] mod tests { use std::path::PathBuf; + use std::sync::Arc; use codex_protocol::models::ShellCommandToolCallParams; use pretty_assertions::assert_eq; @@ -299,6 +306,7 @@ mod tests { use crate::sandboxing::SandboxPermissions; use crate::shell::Shell; use crate::shell::ShellType; + use crate::shell_snapshot::ShellSnapshot; use crate::tools::handlers::ShellCommandHandler; /// The logic for is_known_safe_command() has heuristics for known shells, @@ -372,4 +380,29 @@ mod tests { assert_eq!(exec_params.justification, justification); assert_eq!(exec_params.arg0, None); } + + #[test] + fn shell_command_handler_respects_explicit_login_flag() { + let shell = Shell { + shell_type: ShellType::Bash, + shell_path: PathBuf::from("/bin/bash"), + shell_snapshot: Some(Arc::new(ShellSnapshot { + path: PathBuf::from("/tmp/snapshot.sh"), + })), + }; + + let login_command = + ShellCommandHandler::base_command(&shell, "echo login shell", Some(true)); + assert_eq!( + login_command, + shell.derive_exec_args("echo login shell", true) + ); + + let non_login_command = + ShellCommandHandler::base_command(&shell, "echo non login shell", Some(false)); + assert_eq!( + non_login_command, + shell.derive_exec_args("echo non login shell", false) + ); + } } diff --git a/codex-rs/core/src/tools/handlers/unified_exec.rs b/codex-rs/core/src/tools/handlers/unified_exec.rs index 0d3a11da1..e81c35186 100644 --- a/codex-rs/core/src/tools/handlers/unified_exec.rs +++ b/codex-rs/core/src/tools/handlers/unified_exec.rs @@ -34,8 +34,8 @@ struct ExecCommandArgs { workdir: Option, #[serde(default)] shell: Option, - #[serde(default)] - login: Option, + #[serde(default = "default_login")] + login: bool, #[serde(default = "default_exec_yield_time_ms")] yield_time_ms: u64, #[serde(default)] @@ -66,6 +66,10 @@ fn default_write_stdin_yield_time_ms() -> u64 { 250 } +fn default_login() -> bool { + true +} + #[async_trait] impl ToolHandler for UnifiedExecHandler { fn kind(&self) -> ToolKind { @@ -125,11 +129,10 @@ impl ToolHandler for UnifiedExecHandler { )) })?; let process_id = manager.allocate_process_id().await; + let command = get_command(&args, session.user_shell()); - let command_for_intercept = get_command(&args, session.user_shell()); let ExecCommandArgs { workdir, - login, yield_time_ms, max_output_tokens, sandbox_permissions, @@ -156,7 +159,7 @@ impl ToolHandler for UnifiedExecHandler { let cwd = workdir.clone().unwrap_or_else(|| context.turn.cwd.clone()); if let Some(output) = intercept_apply_patch( - &command_for_intercept, + &command, &cwd, Some(yield_time_ms), context.session.as_ref(), @@ -177,14 +180,6 @@ impl ToolHandler for UnifiedExecHandler { &context.call_id, None, ); - let command = if login.is_none() { - context - .session - .user_shell() - .wrap_command_with_snapshot(&command_for_intercept) - } else { - command_for_intercept - }; let emitter = ToolEmitter::unified_exec( &command, cwd.clone(), @@ -258,14 +253,15 @@ impl ToolHandler for UnifiedExecHandler { } fn get_command(args: &ExecCommandArgs, session_shell: Arc) -> Vec { - if let Some(shell_str) = &args.shell { + 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 = None; - return shell.derive_exec_args(&args.cmd, args.login.unwrap_or(true)); - } + shell + }); - let use_login_shell = args.login.unwrap_or(session_shell.shell_snapshot.is_none()); - session_shell.derive_exec_args(&args.cmd, use_login_shell) + let shell = model_shell.as_ref().unwrap_or(session_shell.as_ref()); + + shell.derive_exec_args(&args.cmd, args.login) } fn format_response(response: &UnifiedExecResponse) -> String { @@ -329,7 +325,13 @@ mod tests { let command = get_command(&args, Arc::new(default_user_shell())); - assert_eq!(command[2], "echo hello"); + assert_eq!(command.last(), Some(&"echo hello".to_string())); + if command + .iter() + .any(|arg| arg.eq_ignore_ascii_case("-Command")) + { + assert!(command.contains(&"-NoProfile".to_string())); + } } #[test] diff --git a/codex-rs/core/src/tools/runtimes/mod.rs b/codex-rs/core/src/tools/runtimes/mod.rs index 2431b3c97..38bfaebe6 100644 --- a/codex-rs/core/src/tools/runtimes/mod.rs +++ b/codex-rs/core/src/tools/runtimes/mod.rs @@ -7,6 +7,7 @@ small and focused and reuses the orchestrator for approvals + sandbox + retry. use crate::exec::ExecExpiration; use crate::sandboxing::CommandSpec; use crate::sandboxing::SandboxPermissions; +use crate::shell::Shell; use crate::tools::sandboxing::ToolError; use std::collections::HashMap; use std::path::Path; @@ -38,3 +39,39 @@ pub(crate) fn build_command_spec( justification, }) } + +/// POSIX-only helper: for commands produced by `Shell::derive_exec_args` +/// for Bash/Zsh/sh of the form `[shell_path, "-lc", "