diff --git a/codex-rs/app-server/tests/suite/v2/turn_start_zsh_fork.rs b/codex-rs/app-server/tests/suite/v2/turn_start_zsh_fork.rs index 559be8b18c..351abdb86d 100644 --- a/codex-rs/app-server/tests/suite/v2/turn_start_zsh_fork.rs +++ b/codex-rs/app-server/tests/suite/v2/turn_start_zsh_fork.rs @@ -11,7 +11,6 @@ use app_test_support::McpProcess; use app_test_support::create_final_assistant_message_sse_response; use app_test_support::create_mock_responses_server_sequence; use app_test_support::create_mock_responses_server_sequence_unchecked; -use app_test_support::create_shell_command_sse_response; use app_test_support::to_response; use codex_app_server_protocol::CommandAction; use codex_app_server_protocol::CommandExecutionApprovalDecision; @@ -19,6 +18,7 @@ use codex_app_server_protocol::CommandExecutionRequestApprovalResponse; use codex_app_server_protocol::CommandExecutionStatus; use codex_app_server_protocol::ItemCompletedNotification; use codex_app_server_protocol::ItemStartedNotification; +use codex_app_server_protocol::JSONRPCMessage; use codex_app_server_protocol::JSONRPCResponse; use codex_app_server_protocol::RequestId; use codex_app_server_protocol::ServerRequest; @@ -35,9 +35,12 @@ use codex_core::features::Feature; use core_test_support::responses; use core_test_support::skip_if_no_network; use pretty_assertions::assert_eq; +use serde_json::json; use std::collections::BTreeMap; +use std::os::unix::fs::PermissionsExt; use std::path::Path; use tempfile::TempDir; +use tokio::time::sleep; use tokio::time::timeout; #[cfg(windows)] @@ -62,19 +65,14 @@ async fn turn_start_shell_zsh_fork_executes_command_v2() -> Result<()> { }; eprintln!("using zsh path for zsh-fork test: {}", zsh_path.display()); - // Keep the shell command in flight until we interrupt it. A fast command + // Keep the exec command in flight until we interrupt it. A fast command // like `echo hi` can finish before the interrupt arrives on faster runners, // which turns this into a test for post-command follow-up behavior instead // of interrupting an active zsh-fork command. let release_marker_escaped = release_marker.to_string_lossy().replace('\'', r#"'\''"#); let wait_for_interrupt = format!("while [ ! -f '{release_marker_escaped}' ]; do sleep 0.01; done"); - let response = create_shell_command_sse_response( - vec!["/bin/sh".to_string(), "-c".to_string(), wait_for_interrupt], - None, - Some(5000), - "call-zsh-fork", - )?; + let response = create_zsh_fork_exec_command_sse_response(&wait_for_interrupt, "call-zsh-fork")?; let no_op_response = responses::sse(vec![ responses::ev_response_created("resp-2"), responses::ev_completed("resp-2"), @@ -91,7 +89,7 @@ async fn turn_start_shell_zsh_fork_executes_command_v2() -> Result<()> { "never", &BTreeMap::from([ (Feature::ShellZshFork, true), - (Feature::UnifiedExec, false), + (Feature::UnifiedExec, true), (Feature::ShellSnapshot, false), ]), &zsh_path, @@ -163,7 +161,7 @@ async fn turn_start_shell_zsh_fork_executes_command_v2() -> Result<()> { assert_eq!(id, "call-zsh-fork"); assert_eq!(status, CommandExecutionStatus::InProgress); assert!(command.starts_with(&zsh_path.display().to_string())); - assert!(command.contains("/bin/sh -c")); + assert!(command.contains(" -lc ")); assert!(command.contains("sleep 0.01")); assert!(command.contains(&release_marker.display().to_string())); assert_eq!(cwd, workspace); @@ -174,6 +172,186 @@ async fn turn_start_shell_zsh_fork_executes_command_v2() -> Result<()> { Ok(()) } +#[tokio::test] +async fn turn_start_shell_zsh_fork_login_startup_helper_does_not_prompt_separately_v2() -> Result<()> +{ + skip_if_no_network!(Ok(())); + + let tmp = TempDir::new()?; + let codex_home = tmp.path().join("codex_home"); + std::fs::create_dir(&codex_home)?; + let workspace = tmp.path().join("workspace"); + std::fs::create_dir(&workspace)?; + let startup_helper_marker = workspace.join("startup-helper-ran"); + let startup_helper = workspace.join("startup-helper.sh"); + std::fs::write( + &startup_helper, + format!( + "#!/bin/sh\nprintf 'startup-helper-ran\\n' >> '{}'\n", + startup_helper_marker.display() + ), + )?; + std::fs::set_permissions(&startup_helper, std::fs::Permissions::from_mode(0o755))?; + std::fs::write( + workspace.join(".zprofile"), + format!( + "export SNAPSHOT_LOGIN_ENV=from-zprofile\n'{}'\n", + startup_helper.display() + ), + )?; + + let Some(zsh_path) = find_test_zsh_path()? else { + eprintln!("skipping zsh fork test: no zsh executable found"); + return Ok(()); + }; + if !supports_exec_wrapper_intercept(&zsh_path) { + eprintln!( + "skipping zsh fork test: zsh does not support EXEC_WRAPPER intercepts ({})", + zsh_path.display() + ); + return Ok(()); + } + eprintln!("using zsh path for zsh-fork test: {}", zsh_path.display()); + + let server = create_mock_responses_server_sequence(vec![ + create_zsh_fork_exec_command_sse_response_with_args( + json!({ + "cmd": "python3 -c 'import os; print(os.environ.get(\"SNAPSHOT_LOGIN_ENV\", \"missing\"))'", + "yield_time_ms": 5000, + "login": true, + "sandbox_permissions": "require_escalated", + "justification": "test login startup helper approvals", + }), + "call-zsh-fork-login-startup-helper", + )?, + create_final_assistant_message_sse_response("done")?, + ]) + .await; + create_config_toml( + &codex_home, + &server.uri(), + "on-request", + &BTreeMap::from([ + (Feature::ShellZshFork, true), + (Feature::UnifiedExec, true), + (Feature::ShellSnapshot, true), + ]), + &zsh_path, + )?; + + let mut mcp = create_zsh_test_mcp_process(&codex_home, &workspace).await?; + timeout(DEFAULT_READ_TIMEOUT, mcp.initialize()).await??; + + let start_id = mcp + .send_thread_start_request(ThreadStartParams { + model: Some("mock-model".to_string()), + cwd: Some(workspace.to_string_lossy().into_owned()), + ..Default::default() + }) + .await?; + let start_resp: JSONRPCResponse = timeout( + DEFAULT_READ_TIMEOUT, + mcp.read_stream_until_response_message(RequestId::Integer(start_id)), + ) + .await??; + let ThreadStartResponse { thread, .. } = to_response::(start_resp)?; + timeout(DEFAULT_READ_TIMEOUT, async { + loop { + if startup_helper_marker.exists() { + return Ok::<(), anyhow::Error>(()); + } + sleep(std::time::Duration::from_millis(20)).await; + } + }) + .await??; + let startup_runs_before_turn = std::fs::read_to_string(&startup_helper_marker)?; + assert_eq!(startup_runs_before_turn, "startup-helper-ran\n"); + + let turn_id = mcp + .send_turn_start_request(TurnStartParams { + thread_id: thread.id.clone(), + input: vec![V2UserInput::Text { + text: "run login zsh-fork command".to_string(), + text_elements: Vec::new(), + }], + cwd: Some(workspace.clone()), + approval_policy: Some(codex_app_server_protocol::AskForApproval::OnRequest), + sandbox_policy: Some(codex_app_server_protocol::SandboxPolicy::DangerFullAccess), + model: Some("mock-model".to_string()), + effort: Some(codex_protocol::openai_models::ReasoningEffort::Medium), + summary: Some(codex_protocol::config_types::ReasoningSummary::Auto), + ..Default::default() + }) + .await?; + let turn_resp: JSONRPCResponse = timeout( + DEFAULT_READ_TIMEOUT, + mcp.read_stream_until_response_message(RequestId::Integer(turn_id)), + ) + .await??; + let TurnStartResponse { turn } = to_response::(turn_resp)?; + + let mut saw_command_completion = false; + loop { + let message = timeout(DEFAULT_READ_TIMEOUT, mcp.read_next_message()).await??; + match message { + JSONRPCMessage::Request(request) => { + let server_req: ServerRequest = request.try_into()?; + if let ServerRequest::CommandExecutionRequestApproval { params, .. } = server_req { + panic!( + "unexpected approval during login-shell startup helper test: {:?}", + params.command + ); + } + } + JSONRPCMessage::Notification(notification) + if notification.method == "item/completed" => + { + let completed: ItemCompletedNotification = + serde_json::from_value(notification.params.expect("item/completed params"))?; + if let ThreadItem::CommandExecution { + id, + status, + aggregated_output, + .. + } = completed.item + && id == "call-zsh-fork-login-startup-helper" + { + assert_eq!(status, CommandExecutionStatus::Completed); + assert!( + aggregated_output + .as_deref() + .is_some_and(|output| output.contains("from-zprofile")), + "expected completed command output to contain restored login-shell env, got: {aggregated_output:?}" + ); + saw_command_completion = true; + } + } + JSONRPCMessage::Notification(notification) + if notification.method == "turn/completed" => + { + let completed: TurnCompletedNotification = + serde_json::from_value(notification.params.expect("turn/completed params"))?; + assert_eq!(completed.thread_id, thread.id); + assert_eq!(completed.turn.id, turn.id); + assert_eq!(completed.turn.status, TurnStatus::Completed); + assert!( + saw_command_completion, + "expected completed command execution item for login-shell startup helper test" + ); + break; + } + _ => {} + } + } + + assert_eq!( + std::fs::read_to_string(&startup_helper_marker)?, + startup_runs_before_turn + ); + + Ok(()) +} + #[tokio::test] async fn turn_start_shell_zsh_fork_exec_approval_decline_v2() -> Result<()> { skip_if_no_network!(Ok(())); @@ -191,14 +369,8 @@ async fn turn_start_shell_zsh_fork_exec_approval_decline_v2() -> Result<()> { eprintln!("using zsh path for zsh-fork test: {}", zsh_path.display()); let responses = vec![ - create_shell_command_sse_response( - vec![ - "python3".to_string(), - "-c".to_string(), - "print(42)".to_string(), - ], - None, - Some(5000), + create_zsh_fork_exec_command_sse_response( + "python3 -c 'print(42)'", "call-zsh-fork-decline", )?, create_final_assistant_message_sse_response("done")?, @@ -210,7 +382,7 @@ async fn turn_start_shell_zsh_fork_exec_approval_decline_v2() -> Result<()> { "untrusted", &BTreeMap::from([ (Feature::ShellZshFork, true), - (Feature::UnifiedExec, false), + (Feature::UnifiedExec, true), (Feature::ShellSnapshot, false), ]), &zsh_path, @@ -326,14 +498,8 @@ async fn turn_start_shell_zsh_fork_exec_approval_cancel_v2() -> Result<()> { }; eprintln!("using zsh path for zsh-fork test: {}", zsh_path.display()); - let responses = vec![create_shell_command_sse_response( - vec![ - "python3".to_string(), - "-c".to_string(), - "print(42)".to_string(), - ], - None, - Some(5000), + let responses = vec![create_zsh_fork_exec_command_sse_response( + "python3 -c 'print(42)'", "call-zsh-fork-cancel", )?]; let server = create_mock_responses_server_sequence(responses).await; @@ -343,7 +509,7 @@ async fn turn_start_shell_zsh_fork_exec_approval_cancel_v2() -> Result<()> { "untrusted", &BTreeMap::from([ (Feature::ShellZshFork, true), - (Feature::UnifiedExec, false), + (Feature::UnifiedExec, true), (Feature::ShellSnapshot, false), ]), &zsh_path, @@ -441,6 +607,204 @@ async fn turn_start_shell_zsh_fork_exec_approval_cancel_v2() -> Result<()> { Ok(()) } +#[tokio::test] +async fn turn_start_shell_zsh_fork_interrupt_kills_approved_subcommand_v2() -> Result<()> { + skip_if_no_network!(Ok(())); + + let tmp = TempDir::new()?; + let codex_home = tmp.path().join("codex_home"); + std::fs::create_dir(&codex_home)?; + let workspace = tmp.path().join("workspace"); + std::fs::create_dir(&workspace)?; + let launch_marker = workspace.join("approved-subcommand.started"); + let leaked_marker = workspace.join("approved-subcommand.leaked"); + let launch_marker_display = launch_marker.display().to_string(); + assert!( + !launch_marker_display.contains('\''), + "test workspace path should not contain single quotes: {launch_marker_display}" + ); + let leaked_marker_display = leaked_marker.display().to_string(); + assert!( + !leaked_marker_display.contains('\''), + "test workspace path should not contain single quotes: {leaked_marker_display}" + ); + + let Some(zsh_path) = find_test_zsh_path()? else { + eprintln!("skipping zsh fork interrupt cleanup test: no zsh executable found"); + return Ok(()); + }; + if !supports_exec_wrapper_intercept(&zsh_path) { + eprintln!( + "skipping zsh fork interrupt cleanup test: zsh does not support EXEC_WRAPPER intercepts ({})", + zsh_path.display() + ); + return Ok(()); + } + let zsh_path_display = zsh_path.display().to_string(); + eprintln!("using zsh path for zsh-fork test: {zsh_path_display}"); + + let shell_command = format!( + "/bin/sh -c 'echo started > \"{launch_marker_display}\" && /bin/sleep 0.5 && echo leaked > \"{leaked_marker_display}\" && exec /bin/sleep 100'" + ); + let tool_call_arguments = serde_json::to_string(&json!({ + "cmd": shell_command, + "yield_time_ms": 30_000, + }))?; + let response = responses::sse(vec![ + responses::ev_response_created("resp-1"), + responses::ev_function_call( + "call-zsh-fork-interrupt-cleanup", + "exec_command", + &tool_call_arguments, + ), + responses::ev_completed("resp-1"), + ]); + let no_op_response = responses::sse(vec![ + responses::ev_response_created("resp-2"), + responses::ev_completed("resp-2"), + ]); + let server = + create_mock_responses_server_sequence_unchecked(vec![response, no_op_response]).await; + create_config_toml( + &codex_home, + &server.uri(), + "untrusted", + &BTreeMap::from([ + (Feature::ShellZshFork, true), + (Feature::UnifiedExec, true), + (Feature::ShellSnapshot, false), + ]), + &zsh_path, + )?; + + let mut mcp = create_zsh_test_mcp_process(&codex_home, &workspace).await?; + timeout(DEFAULT_READ_TIMEOUT, mcp.initialize()).await??; + + let start_id = mcp + .send_thread_start_request(ThreadStartParams { + model: Some("mock-model".to_string()), + cwd: Some(workspace.to_string_lossy().into_owned()), + ..Default::default() + }) + .await?; + let start_resp: JSONRPCResponse = timeout( + DEFAULT_READ_TIMEOUT, + mcp.read_stream_until_response_message(RequestId::Integer(start_id)), + ) + .await??; + let ThreadStartResponse { thread, .. } = to_response::(start_resp)?; + + let turn_id = mcp + .send_turn_start_request(TurnStartParams { + thread_id: thread.id.clone(), + input: vec![V2UserInput::Text { + text: "run the long-lived command".to_string(), + text_elements: Vec::new(), + }], + cwd: Some(workspace.clone()), + approval_policy: Some(codex_app_server_protocol::AskForApproval::UnlessTrusted), + sandbox_policy: Some(codex_app_server_protocol::SandboxPolicy::WorkspaceWrite { + writable_roots: vec![workspace.clone().try_into()?], + read_only_access: codex_app_server_protocol::ReadOnlyAccess::FullAccess, + network_access: false, + exclude_tmpdir_env_var: false, + exclude_slash_tmp: false, + }), + model: Some("mock-model".to_string()), + effort: Some(codex_protocol::openai_models::ReasoningEffort::Medium), + summary: Some(codex_protocol::config_types::ReasoningSummary::Auto), + ..Default::default() + }) + .await?; + let turn_resp: JSONRPCResponse = timeout( + DEFAULT_READ_TIMEOUT, + mcp.read_stream_until_response_message(RequestId::Integer(turn_id)), + ) + .await??; + let TurnStartResponse { turn } = to_response::(turn_resp)?; + + let mut saw_target_approval = false; + while !saw_target_approval { + let server_req = timeout( + DEFAULT_READ_TIMEOUT, + mcp.read_stream_until_request_message(), + ) + .await??; + let ServerRequest::CommandExecutionRequestApproval { request_id, params } = server_req + else { + panic!("expected CommandExecutionRequestApproval request"); + }; + let approval_command = params.command.clone().unwrap_or_default(); + saw_target_approval = approval_command.contains("/bin/sh") + && approval_command.contains(&launch_marker_display) + && !approval_command.contains(&zsh_path_display); + mcp.send_response( + request_id, + serde_json::to_value(CommandExecutionRequestApprovalResponse { + decision: CommandExecutionApprovalDecision::Accept, + })?, + ) + .await?; + } + + let started_command = timeout(DEFAULT_READ_TIMEOUT, async { + loop { + let notif = mcp + .read_stream_until_notification_message("item/started") + .await?; + let started: ItemStartedNotification = + serde_json::from_value(notif.params.clone().expect("item/started params"))?; + if let ThreadItem::CommandExecution { .. } = started.item { + return Ok::(started.item); + } + } + }) + .await??; + let ThreadItem::CommandExecution { + id, + process_id, + status, + command, + cwd, + .. + } = started_command + else { + unreachable!("loop ensures we break on command execution items"); + }; + assert_eq!(id, "call-zsh-fork-interrupt-cleanup"); + assert_eq!(status, CommandExecutionStatus::InProgress); + assert!(command.starts_with(&zsh_path.display().to_string())); + assert!(command.contains(" -lc ")); + assert!(command.contains(&launch_marker_display)); + assert_eq!(cwd, workspace); + assert!(process_id.is_some(), "process id should be present"); + + timeout(DEFAULT_READ_TIMEOUT, async { + loop { + if launch_marker.exists() { + return Ok::<(), anyhow::Error>(()); + } + sleep(std::time::Duration::from_millis(20)).await; + } + }) + .await??; + + mcp.interrupt_turn_and_wait_for_aborted( + thread.id.clone(), + turn.id.clone(), + DEFAULT_READ_TIMEOUT, + ) + .await?; + + sleep(std::time::Duration::from_millis(750)).await; + assert!( + !leaked_marker.exists(), + "expected interrupt to stop approved subcommand before it wrote {leaked_marker_display}" + ); + + Ok(()) +} + #[tokio::test] async fn turn_start_shell_zsh_fork_subcommand_decline_marks_parent_declined_v2() -> Result<()> { skip_if_no_network!(Ok(())); @@ -472,16 +836,15 @@ async fn turn_start_shell_zsh_fork_subcommand_decline_marks_parent_declined_v2() first_file.display(), second_file.display() ); - let tool_call_arguments = serde_json::to_string(&serde_json::json!({ - "command": shell_command, - "workdir": serde_json::Value::Null, - "timeout_ms": 5000 + let tool_call_arguments = serde_json::to_string(&json!({ + "cmd": shell_command, + "yield_time_ms": 5000, }))?; let response = responses::sse(vec![ responses::ev_response_created("resp-1"), responses::ev_function_call( "call-zsh-fork-subcommand-decline", - "shell_command", + "exec_command", &tool_call_arguments, ), responses::ev_completed("resp-1"), @@ -502,7 +865,7 @@ async fn turn_start_shell_zsh_fork_subcommand_decline_marks_parent_declined_v2() "untrusted", &BTreeMap::from([ (Feature::ShellZshFork, true), - (Feature::UnifiedExec, false), + (Feature::UnifiedExec, true), (Feature::ShellSnapshot, false), ]), &zsh_path, @@ -744,6 +1107,31 @@ async fn create_zsh_test_mcp_process(codex_home: &Path, zdotdir: &Path) -> Resul McpProcess::new_with_env(codex_home, &[("ZDOTDIR", Some(zdotdir.as_str()))]).await } +fn create_zsh_fork_exec_command_sse_response( + command: &str, + call_id: &str, +) -> anyhow::Result { + create_zsh_fork_exec_command_sse_response_with_args( + json!({ + "cmd": command, + "yield_time_ms": 5000, + }), + call_id, + ) +} + +fn create_zsh_fork_exec_command_sse_response_with_args( + tool_call_arguments: serde_json::Value, + call_id: &str, +) -> anyhow::Result { + let tool_call_arguments = serde_json::to_string(&tool_call_arguments)?; + Ok(responses::sse(vec![ + responses::ev_response_created("resp-1"), + responses::ev_function_call(call_id, "exec_command", &tool_call_arguments), + responses::ev_completed("resp-1"), + ])) +} + fn create_config_toml( codex_home: &Path, server_uri: &str, diff --git a/codex-rs/core/src/tools/handlers/unified_exec.rs b/codex-rs/core/src/tools/handlers/unified_exec.rs index c79edf3058..11cd41b421 100644 --- a/codex-rs/core/src/tools/handlers/unified_exec.rs +++ b/codex-rs/core/src/tools/handlers/unified_exec.rs @@ -349,11 +349,19 @@ pub(crate) fn get_command( let shell = model_shell.as_ref().unwrap_or(session_shell.as_ref()); Ok(shell.derive_exec_args(&args.cmd, use_login_shell)) } - UnifiedExecShellMode::ZshFork(zsh_fork_config) => Ok(vec![ - zsh_fork_config.shell_zsh_path.to_string_lossy().to_string(), - if use_login_shell { "-lc" } else { "-c" }.to_string(), - args.cmd.clone(), - ]), + UnifiedExecShellMode::ZshFork(zsh_fork_config) => { + if args.shell.is_some() { + return Err( + "shell override is not supported when the zsh-fork backend is enabled." + .to_string(), + ); + } + Ok(vec![ + zsh_fork_config.shell_zsh_path.to_string_lossy().to_string(), + if use_login_shell { "-lc" } else { "-c" }.to_string(), + args.cmd.clone(), + ]) + } } } diff --git a/codex-rs/core/src/tools/handlers/unified_exec_tests.rs b/codex-rs/core/src/tools/handlers/unified_exec_tests.rs index fbd2cb1081..73c6b32fbb 100644 --- a/codex-rs/core/src/tools/handlers/unified_exec_tests.rs +++ b/codex-rs/core/src/tools/handlers/unified_exec_tests.rs @@ -119,7 +119,7 @@ fn test_get_command_rejects_explicit_login_when_disallowed() -> anyhow::Result<( } #[test] -fn test_get_command_ignores_explicit_shell_in_zsh_fork_mode() -> anyhow::Result<()> { +fn test_get_command_rejects_explicit_shell_in_zsh_fork_mode() -> anyhow::Result<()> { let json = r#"{"cmd": "echo hello", "shell": "/bin/bash"}"#; let args: ExecCommandArgs = parse_arguments(json)?; let shell_zsh_path = AbsolutePathBuf::from_absolute_path(if cfg!(windows) { @@ -128,7 +128,7 @@ fn test_get_command_ignores_explicit_shell_in_zsh_fork_mode() -> anyhow::Result< "/opt/codex/zsh" })?; let shell_mode = UnifiedExecShellMode::ZshFork(ZshForkConfig { - shell_zsh_path: shell_zsh_path.clone(), + shell_zsh_path, main_execve_wrapper_exe: AbsolutePathBuf::from_absolute_path(if cfg!(windows) { r"C:\opt\codex\codex-execve-wrapper" } else { @@ -136,20 +136,14 @@ fn test_get_command_ignores_explicit_shell_in_zsh_fork_mode() -> anyhow::Result< })?, }); - let command = get_command(&args, Arc::new(default_user_shell()), &shell_mode, true) - .map_err(anyhow::Error::msg)?; - - assert_eq!( - command, - vec![ - shell_zsh_path.to_string_lossy().to_string(), - "-lc".to_string(), - "echo hello".to_string() - ] + let err = get_command(&args, Arc::new(default_user_shell()), &shell_mode, true) + .expect_err("shell override should be rejected for zsh-fork mode"); + assert!( + err.contains("shell override is not supported"), + "unexpected error: {err}" ); Ok(()) } - #[test] fn exec_command_args_resolve_relative_additional_permissions_against_workdir() -> anyhow::Result<()> { diff --git a/codex-rs/core/src/tools/runtimes/mod.rs b/codex-rs/core/src/tools/runtimes/mod.rs index 8003819a84..d1f740333a 100644 --- a/codex-rs/core/src/tools/runtimes/mod.rs +++ b/codex-rs/core/src/tools/runtimes/mod.rs @@ -71,29 +71,6 @@ pub(crate) fn maybe_wrap_shell_lc_with_snapshot( cwd: &Path, explicit_env_overrides: &HashMap, ) -> Vec { - if cfg!(windows) { - return command.to_vec(); - } - - let Some(snapshot) = session_shell.shell_snapshot() else { - return command.to_vec(); - }; - - if !snapshot.path.exists() { - return command.to_vec(); - } - - if if let (Ok(snapshot_cwd), Ok(command_cwd)) = ( - path_utils::normalize_for_path_comparison(snapshot.cwd.as_path()), - path_utils::normalize_for_path_comparison(cwd), - ) { - snapshot_cwd != command_cwd - } else { - snapshot.cwd != cwd - } { - return command.to_vec(); - } - if command.len() < 3 { return command.to_vec(); } @@ -103,29 +80,61 @@ pub(crate) fn maybe_wrap_shell_lc_with_snapshot( return command.to_vec(); } - let snapshot_path = snapshot.path.to_string_lossy(); let shell_path = session_shell.shell_path.to_string_lossy(); let original_shell = shell_single_quote(&command[0]); let original_script = shell_single_quote(&command[2]); - let snapshot_path = shell_single_quote(snapshot_path.as_ref()); let trailing_args = command[3..] .iter() .map(|arg| format!(" '{}'", shell_single_quote(arg))) .collect::(); - let (override_captures, override_exports) = build_override_exports(explicit_env_overrides); - let rewritten_script = if override_exports.is_empty() { - format!( - "if . '{snapshot_path}' >/dev/null 2>&1; then :; fi\n\nexec '{original_shell}' -c '{original_script}'{trailing_args}" - ) - } else { - format!( - "{override_captures}\n\nif . '{snapshot_path}' >/dev/null 2>&1; then :; fi\n\n{override_exports}\n\nexec '{original_shell}' -c '{original_script}'{trailing_args}" - ) + let Some(snapshot_restore_preamble) = + maybe_build_snapshot_restore_preamble(session_shell, cwd, explicit_env_overrides) + else { + return command.to_vec(); }; + let rewritten_script = format!( + "{snapshot_restore_preamble}\n\nexec '{original_shell}' -c '{original_script}'{trailing_args}" + ); vec![shell_path.to_string(), "-c".to_string(), rewritten_script] } +pub(crate) fn maybe_build_snapshot_restore_preamble( + session_shell: &Shell, + cwd: &Path, + explicit_env_overrides: &HashMap, +) -> Option { + if cfg!(windows) { + return None; + } + + let snapshot = session_shell.shell_snapshot()?; + if !snapshot.path.exists() { + return None; + } + + let snapshot_matches_cwd = if let (Ok(snapshot_cwd), Ok(command_cwd)) = ( + path_utils::normalize_for_path_comparison(snapshot.cwd.as_path()), + path_utils::normalize_for_path_comparison(cwd), + ) { + snapshot_cwd == command_cwd + } else { + snapshot.cwd == cwd + }; + if !snapshot_matches_cwd { + return None; + } + + let snapshot_path = shell_single_quote(snapshot.path.to_string_lossy().as_ref()); + let (override_captures, override_exports) = build_override_exports(explicit_env_overrides); + let source_snapshot = format!("if . '{snapshot_path}' >/dev/null 2>&1; then :; fi"); + Some(if override_exports.is_empty() { + source_snapshot + } else { + format!("{override_captures}\n\n{source_snapshot}\n\n{override_exports}") + }) +} + fn build_override_exports(explicit_env_overrides: &HashMap) -> (String, String) { let mut keys = explicit_env_overrides .keys() diff --git a/codex-rs/core/src/tools/runtimes/shell/unix_escalation.rs b/codex-rs/core/src/tools/runtimes/shell/unix_escalation.rs index 1ff4ef4fb6..64148d6c9b 100644 --- a/codex-rs/core/src/tools/runtimes/shell/unix_escalation.rs +++ b/codex-rs/core/src/tools/runtimes/shell/unix_escalation.rs @@ -15,6 +15,7 @@ use crate::shell::ShellType; use crate::skills::SkillMetadata; use crate::tools::runtimes::ExecveSessionApproval; use crate::tools::runtimes::build_command_spec; +use crate::tools::runtimes::maybe_build_snapshot_restore_preamble; use crate::tools::sandboxing::SandboxAttempt; use crate::tools::sandboxing::SandboxablePreference; use crate::tools::sandboxing::ToolCtx; @@ -36,6 +37,7 @@ use codex_protocol::protocol::ReviewDecision; use codex_protocol::protocol::SandboxPolicy; use codex_shell_command::bash::parse_shell_lc_plain_commands; use codex_shell_command::bash::parse_shell_lc_single_command_prefix; +use codex_shell_command::parse_command::shlex_join; use codex_shell_escalation::EscalateServer; use codex_shell_escalation::EscalationDecision; use codex_shell_escalation::EscalationExecution; @@ -50,10 +52,11 @@ use codex_shell_escalation::ShellCommandExecutor; use codex_shell_escalation::Stopwatch; use codex_utils_absolute_path::AbsolutePathBuf; use std::collections::HashMap; +use std::ffi::OsStr; +use std::path::Path; use std::path::PathBuf; use std::sync::Arc; use std::time::Duration; -use tokio::sync::RwLock; use tokio_util::sync::CancellationToken; use uuid::Uuid; @@ -62,6 +65,15 @@ pub(crate) struct PreparedUnifiedExecZshFork { pub(crate) escalation_session: EscalationSession, } +/// Rewrites a unified-exec shell launch so login-shell startup runs in an +/// outer shell, while the original script is executed by an exact matched +/// inner zsh-fork subprocess. +#[derive(Clone, Debug, Eq, PartialEq)] +struct ExactMatchZshForkReexec { + outer_command: Vec, + exact_intercepted_command: Vec, +} + const PROMPT_CONFLICT_REASON: &str = "approval required by policy, but AskForApproval is set to Never"; const REJECT_SANDBOX_APPROVAL_REASON: &str = @@ -87,11 +99,55 @@ fn approval_sandbox_permissions( } } +/// Builds the outer-shell wrapper used for unified-exec zsh-fork requests that +/// already received top-level escalation approval. +/// +/// When a matching shell snapshot exists for a login-shell request, the exact +/// inner `zsh -c ...` sources that snapshot and reapplies explicit env +/// overrides before running the original script, so the outer shell can stay +/// non-login and avoid re-running startup helpers under zsh-fork interception. +/// Otherwise the outer shell preserves the original `-c`/`-lc` startup +/// behavior and the inner command executes the original script directly. +fn build_exact_match_zsh_fork_reexec( + shell_command: &ParsedShellCommand, + session_shell: &crate::shell::Shell, + cwd: &Path, + explicit_env_overrides: &HashMap, + shell_zsh_path: &Path, +) -> ExactMatchZshForkReexec { + let snapshot_restore_preamble = shell_command + .login + .then(|| maybe_build_snapshot_restore_preamble(session_shell, cwd, explicit_env_overrides)) + .flatten(); + let exact_intercepted_script = snapshot_restore_preamble + .as_ref() + .map(|preamble| format!("{preamble}\n\n{}", shell_command.script)) + .unwrap_or_else(|| shell_command.script.clone()); + let exact_intercepted_command = vec![ + shell_zsh_path.to_string_lossy().into_owned(), + "-c".to_string(), + exact_intercepted_script, + ]; + let outer_command = vec![ + shell_command.program.clone(), + if shell_command.login && snapshot_restore_preamble.is_none() { + "-lc".to_string() + } else { + "-c".to_string() + }, + format!("exec {}", shlex_join(&exact_intercepted_command)), + ]; + ExactMatchZshForkReexec { + outer_command, + exact_intercepted_command, + } +} + pub(super) async fn try_run_zsh_fork( req: &ShellRequest, attempt: &SandboxAttempt<'_>, ctx: &ToolCtx, - command: &[String], + shell_command: &[String], ) -> Result, ToolError> { let Some(shell_zsh_path) = ctx.session.services.shell_zsh_path.as_ref() else { tracing::warn!("ZshFork backend specified, but shell_zsh_path is not configured."); @@ -106,8 +162,10 @@ pub(super) async fn try_run_zsh_fork( return Ok(None); } + let ParsedShellCommand { script, login, .. } = extract_shell_script(shell_command)?; + let spec = build_command_spec( - command, + shell_command, &req.cwd, &req.env, req.timeout_ms.into(), @@ -119,14 +177,14 @@ pub(super) async fn try_run_zsh_fork( .env_for(spec, req.network.as_ref()) .map_err(|err| ToolError::Codex(err.into()))?; let crate::sandboxing::ExecRequest { - command, + command: sandbox_command, cwd: sandbox_cwd, env: sandbox_env, network: sandbox_network, expiration: _sandbox_expiration, sandbox, windows_sandbox_level, - windows_sandbox_private_desktop: _windows_sandbox_private_desktop, + windows_sandbox_private_desktop, sandbox_permissions, sandbox_policy, file_system_sandbox_policy, @@ -134,16 +192,14 @@ pub(super) async fn try_run_zsh_fork( justification, arg0, } = sandbox_exec_request; - let ParsedShellCommand { script, login, .. } = extract_shell_script(&command)?; + let host_zsh_paths = + resolve_host_zsh_paths(sandbox_env.get("PATH").map(String::as_str), &sandbox_cwd); let effective_timeout = Duration::from_millis( req.timeout_ms .unwrap_or(crate::exec::DEFAULT_EXEC_COMMAND_TIMEOUT_MS), ); - let exec_policy = Arc::new(RwLock::new( - ctx.session.services.exec_policy.current().as_ref().clone(), - )); let command_executor = CoreShellCommandExecutor { - command, + command: sandbox_command, cwd: sandbox_cwd, sandbox_policy, file_system_sandbox_policy, @@ -152,6 +208,7 @@ pub(super) async fn try_run_zsh_fork( env: sandbox_env, network: sandbox_network, windows_sandbox_level, + windows_sandbox_private_desktop, sandbox_permissions, justification, arg0, @@ -164,6 +221,8 @@ pub(super) async fn try_run_zsh_fork( .clone(), codex_linux_sandbox_exe: ctx.turn.codex_linux_sandbox_exe.clone(), use_legacy_landlock: ctx.turn.features.use_legacy_landlock(), + shell_zsh_path: ctx.session.services.shell_zsh_path.clone(), + host_zsh_paths: host_zsh_paths.clone(), }; let main_execve_wrapper_exe = ctx .session @@ -192,7 +251,6 @@ pub(super) async fn try_run_zsh_fork( req.additional_permissions_preapproved, ); let escalation_policy = CoreShellActionProvider { - policy: Arc::clone(&exec_policy), session: Arc::clone(&ctx.session), turn: Arc::clone(&ctx.turn), call_id: ctx.call_id.clone(), @@ -201,10 +259,12 @@ pub(super) async fn try_run_zsh_fork( sandbox_policy: command_executor.sandbox_policy.clone(), file_system_sandbox_policy: command_executor.file_system_sandbox_policy.clone(), network_sandbox_policy: command_executor.network_sandbox_policy, - sandbox_permissions: req.sandbox_permissions, - approval_sandbox_permissions, + shell_request_sandbox_permissions: req.sandbox_permissions, + fallback_sandbox_permissions: approval_sandbox_permissions, + exact_match_reexec: None, prompt_permissions: req.additional_permissions.clone(), stopwatch: stopwatch.clone(), + host_zsh_paths, }; let escalate_server = EscalateServer::new( @@ -223,44 +283,92 @@ pub(super) async fn try_run_zsh_fork( pub(crate) async fn prepare_unified_exec_zsh_fork( req: &crate::tools::runtimes::unified_exec::UnifiedExecRequest, - _attempt: &SandboxAttempt<'_>, + attempt: &SandboxAttempt<'_>, ctx: &ToolCtx, + shell_command: &[String], exec_request: ExecRequest, shell_zsh_path: &std::path::Path, main_execve_wrapper_exe: &std::path::Path, ) -> Result, ToolError> { - let parsed = match extract_shell_script(&exec_request.command) { + let parsed_shell_command = match extract_shell_script(shell_command) { Ok(parsed) => parsed, Err(err) => { tracing::warn!("ZshFork unified exec fallback: {err:?}"); return Ok(None); } }; - if parsed.program != shell_zsh_path.to_string_lossy() { - tracing::warn!( - "ZshFork backend specified, but unified exec command targets `{}` instead of `{}`.", - parsed.program, - shell_zsh_path.display(), - ); - return Ok(None); - } - - let exec_policy = Arc::new(RwLock::new( - ctx.session.services.exec_policy.current().as_ref().clone(), - )); + // The outer shell may be the snapshot bootstrap shell instead of the + // configured zsh binary, so only validate the `[program, -c/-lc, script]` + // argv shape here. + let exact_match_reexec = if matches!( + req.sandbox_permissions, + SandboxPermissions::RequireEscalated + ) { + let mut explicit_env_overrides = req.env.clone(); + if let Some(network) = req.network.as_ref() { + network.apply_to_env(&mut explicit_env_overrides); + } + Some(build_exact_match_zsh_fork_reexec( + &parsed_shell_command, + &ctx.session.user_shell(), + &req.cwd, + &explicit_env_overrides, + shell_zsh_path, + )) + } else { + None + }; + let exec_request = if let Some(exact_match_reexec) = exact_match_reexec.as_ref() { + let mut env = req.env.clone(); + if let Some(network) = req.network.as_ref() { + network.apply_to_env(&mut env); + } + let spec = build_command_spec( + &exact_match_reexec.outer_command, + &req.cwd, + &env, + ExecExpiration::DefaultTimeout, + SandboxPermissions::UseDefault, + req.additional_permissions.clone(), + req.justification.clone(), + )?; + attempt + .env_for(spec, req.network.as_ref()) + .map_err(|err| ToolError::Codex(err.into()))? + } else { + exec_request + }; + let ExecRequest { + command, + cwd, + env, + network, + expiration: _expiration, + sandbox, + windows_sandbox_level, + windows_sandbox_private_desktop, + sandbox_permissions, + sandbox_policy, + file_system_sandbox_policy, + network_sandbox_policy, + justification, + arg0, + } = &exec_request; + let host_zsh_paths = resolve_host_zsh_paths(env.get("PATH").map(String::as_str), cwd); let command_executor = CoreShellCommandExecutor { - command: exec_request.command.clone(), - cwd: exec_request.cwd.clone(), - sandbox_policy: exec_request.sandbox_policy.clone(), - file_system_sandbox_policy: exec_request.file_system_sandbox_policy.clone(), - network_sandbox_policy: exec_request.network_sandbox_policy, - sandbox: exec_request.sandbox, - env: exec_request.env.clone(), - network: exec_request.network.clone(), - windows_sandbox_level: exec_request.windows_sandbox_level, - sandbox_permissions: exec_request.sandbox_permissions, - justification: exec_request.justification.clone(), - arg0: exec_request.arg0.clone(), + command: command.clone(), + cwd: cwd.clone(), + sandbox_policy: sandbox_policy.clone(), + file_system_sandbox_policy: file_system_sandbox_policy.clone(), + network_sandbox_policy: *network_sandbox_policy, + sandbox: *sandbox, + env: env.clone(), + network: network.clone(), + windows_sandbox_level: *windows_sandbox_level, + windows_sandbox_private_desktop: *windows_sandbox_private_desktop, + sandbox_permissions: *sandbox_permissions, + justification: justification.clone(), + arg0: arg0.clone(), sandbox_policy_cwd: ctx.turn.cwd.clone(), macos_seatbelt_profile_extensions: ctx .turn @@ -270,9 +378,19 @@ pub(crate) async fn prepare_unified_exec_zsh_fork( .clone(), codex_linux_sandbox_exe: ctx.turn.codex_linux_sandbox_exe.clone(), use_legacy_landlock: ctx.turn.features.use_legacy_landlock(), + shell_zsh_path: Some(shell_zsh_path.to_path_buf()), + host_zsh_paths: host_zsh_paths.clone(), }; + let fallback_sandbox_permissions = exact_match_reexec + .as_ref() + .map(|_| SandboxPermissions::UseDefault) + .unwrap_or_else(|| { + approval_sandbox_permissions( + req.sandbox_permissions, + req.additional_permissions_preapproved, + ) + }); let escalation_policy = CoreShellActionProvider { - policy: Arc::clone(&exec_policy), session: Arc::clone(&ctx.session), turn: Arc::clone(&ctx.turn), call_id: ctx.call_id.clone(), @@ -281,13 +399,12 @@ pub(crate) async fn prepare_unified_exec_zsh_fork( sandbox_policy: exec_request.sandbox_policy.clone(), file_system_sandbox_policy: exec_request.file_system_sandbox_policy.clone(), network_sandbox_policy: exec_request.network_sandbox_policy, - sandbox_permissions: req.sandbox_permissions, - approval_sandbox_permissions: approval_sandbox_permissions( - req.sandbox_permissions, - req.additional_permissions_preapproved, - ), + shell_request_sandbox_permissions: req.sandbox_permissions, + fallback_sandbox_permissions, + exact_match_reexec, prompt_permissions: req.additional_permissions.clone(), stopwatch: Stopwatch::unlimited(), + host_zsh_paths, }; let escalate_server = EscalateServer::new( @@ -307,7 +424,6 @@ pub(crate) async fn prepare_unified_exec_zsh_fork( } struct CoreShellActionProvider { - policy: Arc>, session: Arc, turn: Arc, call_id: String, @@ -316,10 +432,12 @@ struct CoreShellActionProvider { sandbox_policy: SandboxPolicy, file_system_sandbox_policy: FileSystemSandboxPolicy, network_sandbox_policy: NetworkSandboxPolicy, - sandbox_permissions: SandboxPermissions, - approval_sandbox_permissions: SandboxPermissions, + shell_request_sandbox_permissions: SandboxPermissions, + fallback_sandbox_permissions: SandboxPermissions, + exact_match_reexec: Option, prompt_permissions: Option, stopwatch: Stopwatch, + host_zsh_paths: Vec, } #[allow(clippy::large_enum_variant)] @@ -357,6 +475,119 @@ fn execve_prompt_is_rejected_by_policy( } } +fn paths_match(lhs: &Path, rhs: &Path) -> bool { + lhs == rhs + || match (lhs.canonicalize(), rhs.canonicalize()) { + (Ok(lhs), Ok(rhs)) => lhs == rhs, + _ => false, + } +} + +fn canonicalize_best_effort(path: PathBuf) -> PathBuf { + path.canonicalize().unwrap_or(path) +} + +fn is_executable_file(path: &Path) -> bool { + std::fs::metadata(path).is_ok_and(|metadata| { + metadata.is_file() && { + #[cfg(unix)] + { + use std::os::unix::fs::PermissionsExt; + metadata.permissions().mode() & 0o111 != 0 + } + #[cfg(not(unix))] + { + true + } + } + }) +} + +fn push_unique_path(paths: &mut Vec, candidate: PathBuf) { + if !paths.iter().any(|path| paths_match(path, &candidate)) { + paths.push(candidate); + } +} + +fn find_zsh_in_dirs(dirs: impl IntoIterator) -> Vec { + let mut paths = Vec::new(); + for dir in dirs { + let candidate = dir.join("zsh"); + if is_executable_file(&candidate) { + push_unique_path(&mut paths, canonicalize_best_effort(candidate)); + } + } + paths +} + +fn approved_host_zsh_paths() -> Vec { + let Ok(contents) = std::fs::read_to_string("/etc/shells") else { + return Vec::new(); + }; + contents + .lines() + .map(str::trim) + .filter(|line| !line.is_empty() && !line.starts_with('#')) + .map(PathBuf::from) + .filter(|path| path.file_name() == Some(OsStr::new("zsh")) && is_executable_file(path)) + .map(canonicalize_best_effort) + .collect() +} + +fn resolve_host_zsh_paths_from_path_env( + path_env: Option<&str>, + approved_host_zsh_paths: &[PathBuf], +) -> Vec { + let Some(path_env) = path_env else { + return Vec::new(); + }; + let mut paths = Vec::new(); + for dir in std::env::split_paths(path_env) { + let candidate = dir.join("zsh"); + if !is_executable_file(&candidate) { + continue; + } + let candidate = canonicalize_best_effort(candidate); + if approved_host_zsh_paths.contains(&candidate) { + push_unique_path(&mut paths, candidate); + } + } + paths +} + +fn resolve_host_zsh_paths(path_env: Option<&str>, _cwd: &Path) -> Vec { + let approved_host_zsh_paths = approved_host_zsh_paths(); + // Keep nested-zsh rewrites limited to host shell installations that are + // either registered in `/etc/shells` or found in standard system locations. + // Arbitrary PATH shims should not be treated as the host shell. + let mut paths = resolve_host_zsh_paths_from_path_env(path_env, &approved_host_zsh_paths); + for candidate in find_zsh_in_dirs( + ["/bin", "/usr/bin", "/usr/local/bin", "/opt/homebrew/bin"] + .into_iter() + .map(PathBuf::from), + ) { + push_unique_path(&mut paths, candidate); + } + paths +} + +fn is_unconfigured_zsh_exec( + program: &AbsolutePathBuf, + shell_zsh_path: Option<&Path>, + host_zsh_paths: &[PathBuf], +) -> bool { + let Some(shell_zsh_path) = shell_zsh_path else { + return false; + }; + if host_zsh_paths.is_empty() { + return false; + } + host_zsh_paths + .iter() + .any(|host_zsh_path| paths_match(program.as_path(), host_zsh_path)) + && !paths_match(program.as_path(), shell_zsh_path) +} + impl CoreShellActionProvider { fn decision_driven_by_policy(matched_rules: &[RuleMatch], decision: Decision) -> bool { matched_rules.iter().any(|rule_match| { @@ -469,6 +700,10 @@ impl CoreShellActionProvider { command, workdir, None, + // Intercepted exec prompts happen after the original tool call has + // started, so we do not attach an execpolicy amendment payload here. + // Amendments are currently surfaced only from the top-level tool + // request path. None, None, additional_permissions, @@ -630,6 +865,31 @@ impl EscalationPolicy for CoreShellActionProvider { "Determining escalation action for command {program:?} with args {argv:?} in {workdir:?}" ); + let command = join_program_and_argv(program, argv); + if self + .exact_match_reexec + .as_ref() + .is_some_and(|exact_match| exact_match.exact_intercepted_command == command) + { + tracing::debug!( + "Matched exact zsh-fork re-exec target {command:?}, inheriting shell request sandbox semantics" + ); + return Ok(EscalationDecision::escalate( + Self::shell_request_escalation_execution( + self.shell_request_sandbox_permissions, + &self.sandbox_policy, + &self.file_system_sandbox_policy, + self.network_sandbox_policy, + self.prompt_permissions.as_ref(), + self.turn + .config + .permissions + .macos_seatbelt_profile_extensions + .as_ref(), + ), + )); + } + // Check to see whether `program` has an existing entry in // `execve_session_approvals`. If so, we can skip policy checks and user // prompts and go straight to allowing execution. @@ -693,28 +953,38 @@ impl EscalationPolicy for CoreShellActionProvider { .await; } - let evaluation = { - let policy = self.policy.read().await; - evaluate_intercepted_exec_policy( - &policy, - program, - argv, - InterceptedExecPolicyContext { - approval_policy: self.approval_policy, - sandbox_policy: &self.sandbox_policy, - file_system_sandbox_policy: &self.file_system_sandbox_policy, - sandbox_permissions: self.approval_sandbox_permissions, - enable_shell_wrapper_parsing: - ENABLE_INTERCEPTED_EXEC_POLICY_SHELL_WRAPPER_PARSING, - }, - ) - }; + let policy = self.session.services.exec_policy.current(); + let evaluation = evaluate_intercepted_exec_policy( + policy.as_ref(), + program, + argv, + InterceptedExecPolicyContext { + approval_policy: self.approval_policy, + sandbox_policy: &self.sandbox_policy, + file_system_sandbox_policy: &self.file_system_sandbox_policy, + sandbox_permissions: self.fallback_sandbox_permissions, + enable_shell_wrapper_parsing: ENABLE_INTERCEPTED_EXEC_POLICY_SHELL_WRAPPER_PARSING, + }, + ); // When true, means the Evaluation was due to *.rules, not the // fallback function. let decision_driven_by_policy = Self::decision_driven_by_policy(&evaluation.matched_rules, evaluation.decision); - let needs_escalation = - self.sandbox_permissions.requires_escalated_permissions() || decision_driven_by_policy; + // Keep zsh-fork interception alive across nested shells: if an + // intercepted exec targets the known host `zsh` path instead of the + // configured zsh-fork binary, force it through escalation so the + // executor can rewrite the program path back to the configured shell. + let force_zsh_fork_reexec = is_unconfigured_zsh_exec( + program, + self.session.services.shell_zsh_path.as_deref(), + &self.host_zsh_paths, + ); + let needs_escalation = (self.exact_match_reexec.is_none() + && self + .shell_request_sandbox_permissions + .requires_escalated_permissions()) + || decision_driven_by_policy + || force_zsh_fork_reexec; let decision_source = if decision_driven_by_policy { DecisionSource::PrefixRule @@ -724,7 +994,7 @@ impl EscalationPolicy for CoreShellActionProvider { let escalation_execution = match decision_source { DecisionSource::PrefixRule => EscalationExecution::Unsandboxed, DecisionSource::UnmatchedCommandFallback => Self::shell_request_escalation_execution( - self.sandbox_permissions, + self.fallback_sandbox_permissions, &self.sandbox_policy, &self.file_system_sandbox_policy, self.network_sandbox_policy, @@ -855,6 +1125,7 @@ struct CoreShellCommandExecutor { env: HashMap, network: Option, windows_sandbox_level: WindowsSandboxLevel, + windows_sandbox_private_desktop: bool, sandbox_permissions: SandboxPermissions, justification: Option, arg0: Option, @@ -863,6 +1134,8 @@ struct CoreShellCommandExecutor { macos_seatbelt_profile_extensions: Option, codex_linux_sandbox_exe: Option, use_legacy_landlock: bool, + shell_zsh_path: Option, + host_zsh_paths: Vec, } struct PrepareSandboxedExecParams<'a> { @@ -905,7 +1178,7 @@ impl ShellCommandExecutor for CoreShellCommandExecutor { expiration: ExecExpiration::Cancellation(cancel_rx), sandbox: self.sandbox, windows_sandbox_level: self.windows_sandbox_level, - windows_sandbox_private_desktop: false, + windows_sandbox_private_desktop: self.windows_sandbox_private_desktop, sandbox_permissions: self.sandbox_permissions, sandbox_policy: self.sandbox_policy.clone(), file_system_sandbox_policy: self.file_system_sandbox_policy.clone(), @@ -936,7 +1209,8 @@ impl ShellCommandExecutor for CoreShellCommandExecutor { env: HashMap, execution: EscalationExecution, ) -> anyhow::Result { - let command = join_program_and_argv(program, argv); + let program = self.rewrite_intercepted_program_for_zsh_fork(program); + let command = join_program_and_argv(&program, argv); let Some(first_arg) = argv.first() else { return Err(anyhow::anyhow!( "intercepted exec request must contain argv[0]" @@ -1007,7 +1281,33 @@ impl ShellCommandExecutor for CoreShellCommandExecutor { } impl CoreShellCommandExecutor { - #[allow(clippy::too_many_arguments)] + fn rewrite_intercepted_program_for_zsh_fork( + &self, + program: &AbsolutePathBuf, + ) -> AbsolutePathBuf { + let Some(shell_zsh_path) = self.shell_zsh_path.as_ref() else { + return program.clone(); + }; + if !is_unconfigured_zsh_exec( + program, + Some(shell_zsh_path.as_path()), + &self.host_zsh_paths, + ) { + return program.clone(); + } + match AbsolutePathBuf::from_absolute_path(shell_zsh_path) { + Ok(rewritten) => rewritten, + Err(err) => { + tracing::warn!( + "failed to rewrite intercepted zsh path {} to configured shell {}: {err}", + program.display(), + shell_zsh_path.display(), + ); + program.clone() + } + } + } + fn prepare_sandboxed_exec( &self, params: PrepareSandboxedExecParams<'_>, @@ -1062,7 +1362,7 @@ impl CoreShellCommandExecutor { codex_linux_sandbox_exe: self.codex_linux_sandbox_exe.as_ref(), use_legacy_landlock: self.use_legacy_landlock, windows_sandbox_level: self.windows_sandbox_level, - windows_sandbox_private_desktop: false, + windows_sandbox_private_desktop: self.windows_sandbox_private_desktop, })?; if let Some(network) = exec_request.network.as_ref() { network.apply_to_env(&mut exec_request.env); @@ -1085,23 +1385,21 @@ struct ParsedShellCommand { } fn extract_shell_script(command: &[String]) -> Result { - // Commands reaching zsh-fork can be wrapped by environment/sandbox helpers, so - // we search for the first `-c`/`-lc` triple anywhere in the argv rather - // than assuming it is the first positional form. - if let Some((program, script, login)) = command.windows(3).find_map(|parts| match parts { - [program, flag, script] if flag == "-c" => { - Some((program.to_owned(), script.to_owned(), false)) + if let [program, flag, script, ..] = command { + if flag == "-c" { + return Ok(ParsedShellCommand { + program: program.to_owned(), + script: script.to_owned(), + login: false, + }); } - [program, flag, script] if flag == "-lc" => { - Some((program.to_owned(), script.to_owned(), true)) + if flag == "-lc" { + return Ok(ParsedShellCommand { + program: program.to_owned(), + script: script.to_owned(), + login: true, + }); } - _ => None, - }) { - return Ok(ParsedShellCommand { - program, - script, - login, - }); } Err(ToolError::Rejected( diff --git a/codex-rs/core/src/tools/runtimes/shell/unix_escalation_tests.rs b/codex-rs/core/src/tools/runtimes/shell/unix_escalation_tests.rs index 37c1b53a13..dffc751b73 100644 --- a/codex-rs/core/src/tools/runtimes/shell/unix_escalation_tests.rs +++ b/codex-rs/core/src/tools/runtimes/shell/unix_escalation_tests.rs @@ -6,8 +6,11 @@ use super::ParsedShellCommand; use super::commands_for_intercepted_exec_policy; use super::evaluate_intercepted_exec_policy; use super::extract_shell_script; +use super::is_unconfigured_zsh_exec; use super::join_program_and_argv; use super::map_exec_result; +use super::resolve_host_zsh_paths; +use super::resolve_host_zsh_paths_from_path_env; #[cfg(target_os = "macos")] use crate::config::Constrained; #[cfg(target_os = "macos")] @@ -22,6 +25,9 @@ use crate::protocol::SandboxPolicy; use crate::sandboxing::SandboxPermissions; #[cfg(target_os = "macos")] use crate::seatbelt::MACOS_PATH_TO_SEATBELT_EXECUTABLE; +use crate::shell::Shell; +use crate::shell::ShellType; +use crate::shell_snapshot::ShellSnapshot; use crate::skills::SkillMetadata; use codex_execpolicy::Decision; use codex_execpolicy::Evaluation; @@ -40,6 +46,7 @@ use codex_protocol::permissions::FileSystemSandboxPolicy; use codex_protocol::permissions::FileSystemSpecialPath; use codex_protocol::permissions::NetworkSandboxPolicy; use codex_protocol::protocol::SkillScope; +use codex_shell_command::parse_command::shlex_join; use codex_shell_escalation::EscalationExecution; use codex_shell_escalation::EscalationPermissions; use codex_shell_escalation::ExecResult; @@ -48,10 +55,13 @@ use codex_shell_escalation::Permissions as EscalatedPermissions; use codex_shell_escalation::ShellCommandExecutor; use codex_utils_absolute_path::AbsolutePathBuf; use pretty_assertions::assert_eq; -#[cfg(target_os = "macos")] use std::collections::HashMap; +use std::path::Path; use std::path::PathBuf; +use std::sync::Arc; use std::time::Duration; +use tempfile::tempdir; +use tokio::sync::watch; fn host_absolute_path(segments: &[&str]) -> String { let mut path = if cfg!(windows) { @@ -98,6 +108,23 @@ fn test_skill_metadata(permission_profile: Option) -> SkillMe } } +fn shell_with_snapshot( + shell_type: ShellType, + shell_path: &str, + snapshot_path: PathBuf, + snapshot_cwd: PathBuf, +) -> Shell { + let (_tx, shell_snapshot) = watch::channel(Some(Arc::new(ShellSnapshot { + path: snapshot_path, + cwd: snapshot_cwd, + }))); + Shell { + shell_type, + shell_path: PathBuf::from(shell_path), + shell_snapshot, + } +} + #[test] fn execve_prompt_rejection_uses_skill_approval_for_skill_scripts() { let decision_source = super::DecisionSource::SkillScript { @@ -182,6 +209,98 @@ fn approval_sandbox_permissions_only_downgrades_preapproved_additional_permissio ); } +#[test] +fn build_exact_match_zsh_fork_reexec_sources_snapshot_inside_inner_zsh_and_drops_outer_login() { + let dir = tempdir().expect("create temp dir"); + let snapshot_path = dir.path().join("snapshot.sh"); + std::fs::write( + &snapshot_path, + "# Snapshot file\nexport SNAPSHOT_LOGIN_ENV='from_snapshot'\n", + ) + .expect("write snapshot"); + let session_shell = shell_with_snapshot( + ShellType::Zsh, + "/bin/zsh", + snapshot_path.clone(), + dir.path().to_path_buf(), + ); + let shell_command = ParsedShellCommand { + program: "/bin/zsh".to_string(), + script: "python3 -c 'print(42)'".to_string(), + login: true, + }; + + let explicit_env_overrides = HashMap::from([("PATH".to_string(), "/worktree/bin".to_string())]); + let reexec = super::build_exact_match_zsh_fork_reexec( + &shell_command, + &session_shell, + dir.path(), + &explicit_env_overrides, + Path::new("/opt/codex/zsh"), + ); + + assert_eq!(reexec.exact_intercepted_command[0], "/opt/codex/zsh"); + assert_eq!(reexec.exact_intercepted_command[1], "-c"); + assert!(reexec.exact_intercepted_command[2].contains(&format!( + "if . '{}' >/dev/null 2>&1; then :; fi", + snapshot_path.display() + ))); + assert!( + reexec.exact_intercepted_command[2] + .contains("export PATH=\"${__CODEX_SNAPSHOT_OVERRIDE_0}\"") + ); + assert!( + reexec.exact_intercepted_command[2].ends_with("python3 -c 'print(42)'"), + "expected inner command to end with original script, got {:?}", + reexec.exact_intercepted_command[2] + ); + assert_eq!( + reexec.outer_command, + vec![ + "/bin/zsh".to_string(), + "-c".to_string(), + format!("exec {}", shlex_join(&reexec.exact_intercepted_command)), + ] + ); +} + +#[test] +fn build_exact_match_zsh_fork_reexec_preserves_outer_login_without_snapshot() { + let shell_command = ParsedShellCommand { + program: "/bin/zsh".to_string(), + script: "python3 -c 'print(42)'".to_string(), + login: true, + }; + let session_shell = Shell { + shell_type: ShellType::Zsh, + shell_path: PathBuf::from("/bin/zsh"), + shell_snapshot: crate::shell::empty_shell_snapshot_receiver(), + }; + + let reexec = super::build_exact_match_zsh_fork_reexec( + &shell_command, + &session_shell, + Path::new("/tmp"), + &HashMap::new(), + Path::new("/opt/codex/zsh"), + ); + let expected_inner = vec![ + "/opt/codex/zsh".to_string(), + "-c".to_string(), + "python3 -c 'print(42)'".to_string(), + ]; + + assert_eq!(reexec.exact_intercepted_command, expected_inner); + assert_eq!( + reexec.outer_command, + vec![ + "/bin/zsh".to_string(), + "-lc".to_string(), + format!("exec {}", shlex_join(&expected_inner)), + ] + ); +} + #[test] fn extract_shell_script_preserves_login_flag() { assert_eq!( @@ -203,39 +322,16 @@ fn extract_shell_script_preserves_login_flag() { } #[test] -fn extract_shell_script_supports_wrapped_command_prefixes() { - assert_eq!( - extract_shell_script(&[ - "/usr/bin/env".into(), - "CODEX_EXECVE_WRAPPER=1".into(), - "/bin/zsh".into(), - "-lc".into(), - "echo hello".into() - ]) - .unwrap(), - ParsedShellCommand { - program: "/bin/zsh".to_string(), - script: "echo hello".to_string(), - login: true, - } - ); - - assert_eq!( - extract_shell_script(&[ - "sandbox-exec".into(), - "-p".into(), - "sandbox_policy".into(), - "/bin/zsh".into(), - "-c".into(), - "pwd".into(), - ]) - .unwrap(), - ParsedShellCommand { - program: "/bin/zsh".to_string(), - script: "pwd".to_string(), - login: false, - } - ); +fn extract_shell_script_rejects_wrapped_command_prefixes() { + let err = extract_shell_script(&[ + "/usr/bin/env".into(), + "CODEX_EXECVE_WRAPPER=1".into(), + "/bin/zsh".into(), + "-lc".into(), + "echo hello".into(), + ]) + .unwrap_err(); + assert!(matches!(err, super::ToolError::Rejected(_))); } #[test] @@ -274,6 +370,136 @@ fn join_program_and_argv_replaces_original_argv_zero() { ); } +#[test] +fn is_unconfigured_zsh_exec_matches_non_configured_zsh_paths() { + let program = AbsolutePathBuf::try_from(host_absolute_path(&["bin", "zsh"])).unwrap(); + let host = vec![PathBuf::from(host_absolute_path(&["bin", "zsh"]))]; + let configured = PathBuf::from(host_absolute_path(&["tmp", "codex-zsh"])); + assert!(is_unconfigured_zsh_exec( + &program, + Some(configured.as_path()), + &host, + )); +} + +#[test] +fn is_unconfigured_zsh_exec_ignores_non_zsh_or_configured_paths() { + let configured = PathBuf::from(host_absolute_path(&["tmp", "codex-zsh"])); + let host = vec![PathBuf::from(host_absolute_path(&["bin", "zsh"]))]; + let configured_program = AbsolutePathBuf::try_from(configured.clone()).unwrap(); + assert!(!is_unconfigured_zsh_exec( + &configured_program, + Some(configured.as_path()), + &host, + )); + + let non_zsh = + AbsolutePathBuf::try_from(host_absolute_path(&["usr", "bin", "python3"])).unwrap(); + assert!(!is_unconfigured_zsh_exec( + &non_zsh, + Some(configured.as_path()), + &host, + )); + assert!(!is_unconfigured_zsh_exec(&non_zsh, None, &host,)); +} + +#[test] +fn is_unconfigured_zsh_exec_does_not_match_non_host_zsh_named_binaries() { + let program = AbsolutePathBuf::try_from(host_absolute_path(&["tmp", "repo", "zsh"])).unwrap(); + let configured = PathBuf::from(host_absolute_path(&["tmp", "codex-zsh"])); + let host = vec![PathBuf::from(host_absolute_path(&["bin", "zsh"]))]; + assert!(!is_unconfigured_zsh_exec( + &program, + Some(configured.as_path()), + &host, + )); +} + +#[test] +fn is_unconfigured_zsh_exec_matches_any_approved_host_zsh_path() { + let program = + AbsolutePathBuf::try_from(host_absolute_path(&["usr", "local", "bin", "zsh"])).unwrap(); + let configured = PathBuf::from(host_absolute_path(&["tmp", "codex-zsh"])); + let host_paths = vec![ + PathBuf::from(host_absolute_path(&["bin", "zsh"])), + PathBuf::from(host_absolute_path(&["usr", "local", "bin", "zsh"])), + ]; + assert!(is_unconfigured_zsh_exec( + &program, + Some(configured.as_path()), + &host_paths, + )); +} + +#[test] +fn resolve_host_zsh_paths_ignores_repo_local_path_shadowing() { + let shadow_dir = tempfile::tempdir().expect("create shadow dir"); + let cwd_dir = tempfile::tempdir().expect("create cwd dir"); + let fake_zsh = shadow_dir.path().join("zsh"); + std::fs::write(&fake_zsh, "#!/bin/sh\nexit 0\n").expect("write fake zsh"); + #[cfg(unix)] + { + use std::os::unix::fs::PermissionsExt; + let mut permissions = std::fs::metadata(&fake_zsh) + .expect("metadata for fake zsh") + .permissions(); + permissions.set_mode(0o755); + std::fs::set_permissions(&fake_zsh, permissions).expect("chmod fake zsh"); + } + + let path_env = + std::env::join_paths([shadow_dir.path(), Path::new("/usr/bin"), Path::new("/bin")]) + .expect("join PATH") + .into_string() + .expect("PATH should be UTF-8"); + let resolved = resolve_host_zsh_paths(Some(&path_env), cwd_dir.path()); + assert!( + resolved.iter().all(|path| path != fake_zsh.as_path()), + "repo-local shadow zsh should not be treated as a host shell: {resolved:?}" + ); +} + +#[test] +fn resolve_host_zsh_paths_accept_registered_nonstandard_path_entries() { + let host_dir = tempfile::tempdir().expect("create host dir"); + let second_host_dir = tempfile::tempdir().expect("create second host dir"); + let host_zsh = host_dir.path().join("zsh"); + let second_host_zsh = second_host_dir.path().join("zsh"); + std::fs::write(&host_zsh, "#!/bin/sh\nexit 0\n").expect("write host zsh"); + std::fs::write(&second_host_zsh, "#!/bin/sh\nexit 0\n").expect("write second host zsh"); + #[cfg(unix)] + { + use std::os::unix::fs::PermissionsExt; + for host_zsh in [&host_zsh, &second_host_zsh] { + let mut permissions = std::fs::metadata(host_zsh) + .expect("metadata for host zsh") + .permissions(); + permissions.set_mode(0o755); + std::fs::set_permissions(host_zsh, permissions).expect("chmod host zsh"); + } + } + + let path_env = std::env::join_paths([ + host_dir.path(), + second_host_dir.path(), + Path::new("/usr/bin"), + Path::new("/bin"), + ]) + .expect("join PATH") + .into_string() + .expect("PATH should be UTF-8"); + let expected = vec![ + host_zsh + .canonicalize() + .expect("host zsh path should canonicalize"), + second_host_zsh + .canonicalize() + .expect("second host zsh path should canonicalize"), + ]; + let resolved = resolve_host_zsh_paths_from_path_env(Some(&path_env), expected.as_slice()); + assert_eq!(resolved, expected); +} + #[test] fn commands_for_intercepted_exec_policy_parses_plain_shell_wrappers() { let program = AbsolutePathBuf::try_from(host_absolute_path(&["bin", "bash"])).unwrap(); @@ -660,6 +886,7 @@ async fn prepare_escalated_exec_turn_default_preserves_macos_seatbelt_extensions file_system_sandbox_policy: read_only_file_system_sandbox_policy(), network_sandbox_policy: NetworkSandboxPolicy::Restricted, windows_sandbox_level: WindowsSandboxLevel::Disabled, + windows_sandbox_private_desktop: false, sandbox_permissions: SandboxPermissions::UseDefault, justification: None, arg0: None, @@ -670,6 +897,8 @@ async fn prepare_escalated_exec_turn_default_preserves_macos_seatbelt_extensions }), codex_linux_sandbox_exe: None, use_legacy_landlock: false, + shell_zsh_path: None, + host_zsh_paths: Vec::new(), }; let prepared = executor @@ -712,6 +941,7 @@ async fn prepare_escalated_exec_permissions_preserve_macos_seatbelt_extensions() file_system_sandbox_policy: unrestricted_file_system_sandbox_policy(), network_sandbox_policy: NetworkSandboxPolicy::Enabled, windows_sandbox_level: WindowsSandboxLevel::Disabled, + windows_sandbox_private_desktop: false, sandbox_permissions: SandboxPermissions::UseDefault, justification: None, arg0: None, @@ -719,6 +949,8 @@ async fn prepare_escalated_exec_permissions_preserve_macos_seatbelt_extensions() macos_seatbelt_profile_extensions: None, codex_linux_sandbox_exe: None, use_legacy_landlock: false, + shell_zsh_path: None, + host_zsh_paths: Vec::new(), }; let permissions = Permissions { @@ -787,6 +1019,7 @@ async fn prepare_escalated_exec_permission_profile_unions_turn_and_requested_mac file_system_sandbox_policy: read_only_file_system_sandbox_policy(), network_sandbox_policy: NetworkSandboxPolicy::from(&sandbox_policy), windows_sandbox_level: WindowsSandboxLevel::Disabled, + windows_sandbox_private_desktop: false, sandbox_permissions: SandboxPermissions::UseDefault, justification: None, arg0: None, @@ -797,6 +1030,8 @@ async fn prepare_escalated_exec_permission_profile_unions_turn_and_requested_mac }), codex_linux_sandbox_exe: None, use_legacy_landlock: false, + shell_zsh_path: None, + host_zsh_paths: Vec::new(), }; let prepared = executor diff --git a/codex-rs/core/src/tools/runtimes/shell/zsh_fork_backend.rs b/codex-rs/core/src/tools/runtimes/shell/zsh_fork_backend.rs index 0ac9e08e0a..a4e24df762 100644 --- a/codex-rs/core/src/tools/runtimes/shell/zsh_fork_backend.rs +++ b/codex-rs/core/src/tools/runtimes/shell/zsh_fork_backend.rs @@ -37,10 +37,19 @@ pub(crate) async fn maybe_prepare_unified_exec( req: &UnifiedExecRequest, attempt: &SandboxAttempt<'_>, ctx: &ToolCtx, + shell_command: &[String], exec_request: ExecRequest, zsh_fork_config: &ZshForkConfig, ) -> Result, ToolError> { - imp::maybe_prepare_unified_exec(req, attempt, ctx, exec_request, zsh_fork_config).await + imp::maybe_prepare_unified_exec( + req, + attempt, + ctx, + shell_command, + exec_request, + zsh_fork_config, + ) + .await } #[cfg(unix)] @@ -53,21 +62,29 @@ mod imp { #[derive(Debug)] struct ZshForkSpawnLifecycle { - escalation_session: EscalationSession, + escalation_session: Option, } impl SpawnLifecycle for ZshForkSpawnLifecycle { fn inherited_fds(&self) -> Vec { self.escalation_session - .env() - .get(ESCALATE_SOCKET_ENV_VAR) + .as_ref() + .and_then(|escalation_session| { + escalation_session.env().get(ESCALATE_SOCKET_ENV_VAR) + }) .and_then(|fd| fd.parse().ok()) .into_iter() .collect() } fn after_spawn(&mut self) { - self.escalation_session.close_client_socket(); + if let Some(escalation_session) = self.escalation_session.as_ref() { + escalation_session.close_client_socket(); + } + } + + fn after_exit(&mut self) { + self.escalation_session = None; } } @@ -84,6 +101,7 @@ mod imp { req: &UnifiedExecRequest, attempt: &SandboxAttempt<'_>, ctx: &ToolCtx, + shell_command: &[String], exec_request: ExecRequest, zsh_fork_config: &ZshForkConfig, ) -> Result, ToolError> { @@ -91,6 +109,7 @@ mod imp { req, attempt, ctx, + shell_command, exec_request, zsh_fork_config.shell_zsh_path.as_path(), zsh_fork_config.main_execve_wrapper_exe.as_path(), @@ -103,7 +122,7 @@ mod imp { Ok(Some(PreparedUnifiedExecSpawn { exec_request: prepared.exec_request, spawn_lifecycle: Box::new(ZshForkSpawnLifecycle { - escalation_session: prepared.escalation_session, + escalation_session: Some(prepared.escalation_session), }), })) } @@ -127,10 +146,18 @@ mod imp { req: &UnifiedExecRequest, attempt: &SandboxAttempt<'_>, ctx: &ToolCtx, + shell_command: &[String], exec_request: ExecRequest, zsh_fork_config: &ZshForkConfig, ) -> Result, ToolError> { - let _ = (req, attempt, ctx, exec_request, zsh_fork_config); + let _ = ( + req, + attempt, + ctx, + shell_command, + exec_request, + zsh_fork_config, + ); Ok(None) } } diff --git a/codex-rs/core/src/tools/runtimes/unified_exec.rs b/codex-rs/core/src/tools/runtimes/unified_exec.rs index 7f0032c8d4..c438874056 100644 --- a/codex-rs/core/src/tools/runtimes/unified_exec.rs +++ b/codex-rs/core/src/tools/runtimes/unified_exec.rs @@ -230,6 +230,7 @@ impl<'a> ToolRuntime for UnifiedExecRunt req, attempt, ctx, + &command, exec_env, zsh_fork_config, ) diff --git a/codex-rs/core/src/tools/spec.rs b/codex-rs/core/src/tools/spec.rs index 331a55f0cb..bf3ca4ae2b 100644 --- a/codex-rs/core/src/tools/spec.rs +++ b/codex-rs/core/src/tools/spec.rs @@ -348,8 +348,6 @@ impl ToolsConfig { ); let shell_type = if !features.enabled(Feature::ShellTool) { ConfigShellToolType::Disabled - } else if features.enabled(Feature::ShellZshFork) { - ConfigShellToolType::ShellCommand } else if features.enabled(Feature::UnifiedExec) && unified_exec_allowed { // If ConPTY not supported (for old Windows versions), fallback on ShellCommand. if codex_utils_pty::conpty_supported() { @@ -357,7 +355,9 @@ impl ToolsConfig { } else { ConfigShellToolType::ShellCommand } - } else if model_info.shell_type == ConfigShellToolType::UnifiedExec && !unified_exec_allowed + } else if (model_info.shell_type == ConfigShellToolType::UnifiedExec + && !unified_exec_allowed) + || features.enabled(Feature::ShellZshFork) { ConfigShellToolType::ShellCommand } else { @@ -640,6 +640,7 @@ fn create_approval_parameters( fn create_exec_command_tool( allow_login_shell: bool, exec_permission_approvals_enabled: bool, + unified_exec_shell_mode: &UnifiedExecShellMode, ) -> ToolSpec { let mut properties = BTreeMap::from([ ( @@ -657,12 +658,6 @@ fn create_exec_command_tool( ), }, ), - ( - "shell".to_string(), - JsonSchema::String { - description: Some("Shell binary to launch. Defaults to the user's default shell.".to_string()), - }, - ), ( "tty".to_string(), JsonSchema::Boolean { @@ -690,6 +685,16 @@ fn create_exec_command_tool( }, ), ]); + if !matches!(unified_exec_shell_mode, UnifiedExecShellMode::ZshFork(_)) { + properties.insert( + "shell".to_string(), + JsonSchema::String { + description: Some( + "Shell binary to launch. Defaults to the user's default shell.".to_string(), + ), + }, + ); + } if allow_login_shell { properties.insert( "login".to_string(), @@ -2622,6 +2627,7 @@ pub(crate) fn build_specs_with_discoverable_tools( create_exec_command_tool( config.allow_login_shell, exec_permission_approvals_enabled, + &config.unified_exec_shell_mode, ), true, config.code_mode_enabled, diff --git a/codex-rs/core/src/tools/spec_tests.rs b/codex-rs/core/src/tools/spec_tests.rs index c3c228703a..1ee44114ef 100644 --- a/codex-rs/core/src/tools/spec_tests.rs +++ b/codex-rs/core/src/tools/spec_tests.rs @@ -450,7 +450,7 @@ fn test_full_toolset_specs_for_gpt5_codex_unified_exec_web_search() { // Build expected from the same helpers used by the builder. let mut expected: BTreeMap = BTreeMap::from([]); for spec in [ - create_exec_command_tool(true, false), + create_exec_command_tool(true, false, &UnifiedExecShellMode::Direct), create_write_stdin_tool(), PLAN_TOOL.clone(), create_request_user_input_tool(CollaborationModesConfig::default()), @@ -1418,7 +1418,7 @@ fn test_build_specs_default_shell_present() { } #[test] -fn shell_zsh_fork_prefers_shell_command_over_unified_exec() { +fn shell_zsh_fork_uses_unified_exec_when_enabled() { let config = test_config(); let model_info = ModelsManager::construct_model_info_offline_for_tests("o3", &config); let mut features = Features::with_defaults(); @@ -1441,7 +1441,7 @@ fn shell_zsh_fork_prefers_shell_command_over_unified_exec() { shell_snapshot: crate::shell::empty_shell_snapshot_receiver(), }; - assert_eq!(tools_config.shell_type, ConfigShellToolType::ShellCommand); + assert_eq!(tools_config.shell_type, ConfigShellToolType::UnifiedExec); assert_eq!( tools_config.shell_command_backend, ShellCommandBackendConfig::ZshFork @@ -1450,22 +1450,21 @@ fn shell_zsh_fork_prefers_shell_command_over_unified_exec() { tools_config.unified_exec_shell_mode, UnifiedExecShellMode::Direct ); + let tools_config = tools_config.with_unified_exec_shell_mode_for_session( + &user_shell, + Some(&PathBuf::from(if cfg!(windows) { + r"C:\opt\codex\zsh" + } else { + "/opt/codex/zsh" + })), + Some(&PathBuf::from(if cfg!(windows) { + r"C:\opt\codex\codex-execve-wrapper" + } else { + "/opt/codex/codex-execve-wrapper" + })), + ); assert_eq!( - tools_config - .with_unified_exec_shell_mode_for_session( - &user_shell, - Some(&PathBuf::from(if cfg!(windows) { - r"C:\opt\codex\zsh" - } else { - "/opt/codex/zsh" - })), - Some(&PathBuf::from(if cfg!(windows) { - r"C:\opt\codex\codex-execve-wrapper" - } else { - "/opt/codex/codex-execve-wrapper" - })), - ) - .unified_exec_shell_mode, + tools_config.unified_exec_shell_mode, if cfg!(unix) { UnifiedExecShellMode::ZshFork(ZshForkConfig { shell_zsh_path: AbsolutePathBuf::from_absolute_path("/opt/codex/zsh").unwrap(), @@ -1478,6 +1477,23 @@ fn shell_zsh_fork_prefers_shell_command_over_unified_exec() { UnifiedExecShellMode::Direct } ); + + let (tools, _) = build_specs(&tools_config, Some(HashMap::new()), None, &[]).build(); + let exec_spec = find_tool(&tools, "exec_command"); + let ToolSpec::Function(exec_tool) = &exec_spec.spec else { + panic!("exec_command should be a function tool spec"); + }; + let JsonSchema::Object { properties, .. } = &exec_tool.parameters else { + panic!("exec_command parameters should be an object schema"); + }; + assert_eq!( + properties.contains_key("shell"), + !matches!( + tools_config.unified_exec_shell_mode, + UnifiedExecShellMode::ZshFork(_) + ), + "exec_command should omit `shell` only when zsh-fork forces the configured shell", + ); } #[test] diff --git a/codex-rs/core/src/unified_exec/process.rs b/codex-rs/core/src/unified_exec/process.rs index 6da7c739ec..f0da51a795 100644 --- a/codex-rs/core/src/unified_exec/process.rs +++ b/codex-rs/core/src/unified_exec/process.rs @@ -1,6 +1,7 @@ #![allow(clippy::module_inception)] use std::sync::Arc; +use std::sync::Mutex as StdMutex; use std::sync::atomic::AtomicBool; use std::sync::atomic::Ordering; use tokio::sync::Mutex; @@ -36,6 +37,13 @@ pub(crate) trait SpawnLifecycle: std::fmt::Debug + Send + Sync { } fn after_spawn(&mut self) {} + + /// Releases resources that needed to stay alive until the child process was + /// fully launched or until the session was torn down. + /// + /// This hook must tolerate being called during normal process exit as well + /// as early termination paths, and it is guaranteed to run at most once. + fn after_exit(&mut self) {} } pub(crate) type SpawnLifecycleHandle = Box; @@ -66,7 +74,8 @@ pub(crate) struct UnifiedExecProcess { output_drained: Arc, output_task: JoinHandle<()>, sandbox_type: SandboxType, - _spawn_lifecycle: SpawnLifecycleHandle, + _spawn_lifecycle: Arc>, + spawn_lifecycle_released: Arc, } impl UnifiedExecProcess { @@ -74,7 +83,7 @@ impl UnifiedExecProcess { process_handle: ExecCommandSession, initial_output_rx: tokio::sync::broadcast::Receiver>, sandbox_type: SandboxType, - spawn_lifecycle: SpawnLifecycleHandle, + spawn_lifecycle: Arc>, ) -> Self { let output_buffer = Arc::new(Mutex::new(HeadTailBuffer::default())); let output_notify = Arc::new(Notify::new()); @@ -119,6 +128,19 @@ impl UnifiedExecProcess { output_task, sandbox_type, _spawn_lifecycle: spawn_lifecycle, + spawn_lifecycle_released: Arc::new(AtomicBool::new(false)), + } + } + + fn release_spawn_lifecycle(&self) { + if self + .spawn_lifecycle_released + .swap(true, std::sync::atomic::Ordering::AcqRel) + { + return; + } + if let Ok(mut lifecycle) = self._spawn_lifecycle.lock() { + lifecycle.after_exit(); } } @@ -157,6 +179,7 @@ impl UnifiedExecProcess { } pub(super) fn terminate(&self) { + self.release_spawn_lifecycle(); self.output_closed.store(true, Ordering::Release); self.output_closed_notify.notify_waiters(); self.process_handle.terminate(); @@ -232,12 +255,19 @@ impl UnifiedExecProcess { mut exit_rx, } = spawned; let output_rx = codex_utils_pty::combine_output_receivers(stdout_rx, stderr_rx); - let managed = Self::new(process_handle, output_rx, sandbox_type, spawn_lifecycle); + let spawn_lifecycle = Arc::new(StdMutex::new(spawn_lifecycle)); + let managed = Self::new( + process_handle, + output_rx, + sandbox_type, + Arc::clone(&spawn_lifecycle), + ); let exit_ready = matches!(exit_rx.try_recv(), Ok(_) | Err(TryRecvError::Closed)); if exit_ready { managed.signal_exit(); + managed.release_spawn_lifecycle(); managed.check_for_sandbox_denial().await?; return Ok(managed); } @@ -247,14 +277,22 @@ impl UnifiedExecProcess { .is_ok() { managed.signal_exit(); + managed.release_spawn_lifecycle(); managed.check_for_sandbox_denial().await?; return Ok(managed); } tokio::spawn({ let cancellation_token = managed.cancellation_token.clone(); + let spawn_lifecycle = Arc::clone(&spawn_lifecycle); + let spawn_lifecycle_released = Arc::clone(&managed.spawn_lifecycle_released); async move { let _ = exit_rx.await; + if !spawn_lifecycle_released.swap(true, Ordering::AcqRel) + && let Ok(mut lifecycle) = spawn_lifecycle.lock() + { + lifecycle.after_exit(); + } cancellation_token.cancel(); } }); diff --git a/codex-rs/core/tests/common/process.rs b/codex-rs/core/tests/common/process.rs index 82e42aac95..787b67be7e 100644 --- a/codex-rs/core/tests/common/process.rs +++ b/codex-rs/core/tests/common/process.rs @@ -1,6 +1,7 @@ use anyhow::Context; use std::fs; use std::path::Path; +use std::process::Stdio; use std::time::Duration; pub async fn wait_for_pid_file(path: &Path) -> anyhow::Result { @@ -24,6 +25,7 @@ pub async fn wait_for_pid_file(path: &Path) -> anyhow::Result { pub fn process_is_alive(pid: &str) -> anyhow::Result { let status = std::process::Command::new("kill") .args(["-0", pid]) + .stderr(Stdio::null()) .status() .context("failed to probe process liveness with kill -0")?; Ok(status.success()) diff --git a/codex-rs/core/tests/common/zsh_fork.rs b/codex-rs/core/tests/common/zsh_fork.rs index fbc46421d9..5fe5f8b91e 100644 --- a/codex-rs/core/tests/common/zsh_fork.rs +++ b/codex-rs/core/tests/common/zsh_fork.rs @@ -18,7 +18,11 @@ pub struct ZshForkRuntime { } impl ZshForkRuntime { - fn apply_to_config( + pub fn zsh_path(&self) -> &Path { + &self.zsh_path + } + + pub fn apply_to_config( &self, config: &mut Config, approval_policy: AskForApproval, @@ -73,6 +77,44 @@ pub fn zsh_fork_runtime(test_name: &str) -> Result> { })) } +pub fn find_host_zsh_path() -> Option { + let approved_host_zsh_paths = std::fs::read_to_string("/etc/shells") + .ok() + .map(|contents| { + contents + .lines() + .map(str::trim) + .filter(|line| !line.is_empty() && !line.starts_with('#')) + .map(PathBuf::from) + .filter(|path| path.file_name() == Some(std::ffi::OsStr::new("zsh"))) + .filter(|path| is_executable_file(path)) + .map(canonicalize_best_effort) + .collect::>() + }) + .unwrap_or_default(); + if let Some(path_env) = std::env::var_os("PATH") + && let Some(host_zsh_path) = std::env::split_paths(&path_env).find_map(|dir| { + let candidate = dir.join("zsh"); + if !is_executable_file(&candidate) { + return None; + } + let candidate = canonicalize_best_effort(candidate); + approved_host_zsh_paths + .contains(&candidate) + .then_some(candidate) + }) + { + return Some(host_zsh_path); + } + ["/bin", "/usr/bin", "/usr/local/bin", "/opt/homebrew/bin"] + .into_iter() + .map(PathBuf::from) + .find_map(|dir| { + let candidate = dir.join("zsh"); + is_executable_file(&candidate).then(|| canonicalize_best_effort(candidate)) + }) +} + pub async fn build_zsh_fork_test( server: &wiremock::MockServer, runtime: ZshForkRuntime, @@ -91,6 +133,29 @@ where builder.build(server).await } +pub async fn build_unified_exec_zsh_fork_test( + server: &wiremock::MockServer, + runtime: ZshForkRuntime, + approval_policy: AskForApproval, + sandbox_policy: SandboxPolicy, + pre_build_hook: F, +) -> Result +where + F: FnOnce(&Path) + Send + 'static, +{ + let mut builder = test_codex() + .with_pre_build_hook(pre_build_hook) + .with_config(move |config| { + runtime.apply_to_config(config, approval_policy, sandbox_policy); + config.use_experimental_unified_exec_tool = true; + config + .features + .enable(Feature::UnifiedExec) + .expect("test config should allow feature update"); + }); + builder.build(server).await +} + fn find_test_zsh_path() -> Result> { let repo_root = codex_utils_cargo_bin::repo_root()?; let dotslash_zsh = repo_root.join("codex-rs/app-server/tests/suite/zsh"); @@ -122,3 +187,23 @@ fn supports_exec_wrapper_intercept(zsh_path: &Path) -> bool { Err(_) => false, } } + +fn canonicalize_best_effort(path: PathBuf) -> PathBuf { + path.canonicalize().unwrap_or(path) +} + +fn is_executable_file(path: &Path) -> bool { + std::fs::metadata(path).is_ok_and(|metadata| { + metadata.is_file() && { + #[cfg(unix)] + { + use std::os::unix::fs::PermissionsExt; + metadata.permissions().mode() & 0o111 != 0 + } + #[cfg(not(unix))] + { + true + } + } + }) +} diff --git a/codex-rs/core/tests/suite/approvals.rs b/codex-rs/core/tests/suite/approvals.rs index 7a9dba038e..3446509c59 100644 --- a/codex-rs/core/tests/suite/approvals.rs +++ b/codex-rs/core/tests/suite/approvals.rs @@ -35,6 +35,7 @@ use core_test_support::test_codex::TestCodex; use core_test_support::test_codex::test_codex; use core_test_support::wait_for_event; use core_test_support::wait_for_event_with_timeout; +use core_test_support::zsh_fork::build_unified_exec_zsh_fork_test; use core_test_support::zsh_fork::build_zsh_fork_test; use core_test_support::zsh_fork::restrictive_workspace_write_policy; use core_test_support::zsh_fork::zsh_fork_runtime; @@ -1985,6 +1986,158 @@ async fn approving_execpolicy_amendment_persists_policy_and_skips_future_prompts Ok(()) } +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +#[cfg(unix)] +async fn unified_exec_zsh_fork_execpolicy_amendment_skips_later_subcommands() -> Result<()> { + skip_if_no_network!(Ok(())); + + let Some(runtime) = zsh_fork_runtime("unified exec zsh-fork execpolicy amendment test")? else { + return Ok(()); + }; + + let approval_policy = AskForApproval::UnlessTrusted; + let sandbox_policy = SandboxPolicy::new_read_only_policy(); + let server = start_mock_server().await; + let test = build_unified_exec_zsh_fork_test( + &server, + runtime, + approval_policy, + sandbox_policy.clone(), + |_| {}, + ) + .await?; + let allow_prefix_path = test.cwd.path().join("allow-prefix-zsh-fork.txt"); + let _ = fs::remove_file(&allow_prefix_path); + + let call_id = "allow-prefix-zsh-fork"; + let command = "touch allow-prefix-zsh-fork.txt && touch allow-prefix-zsh-fork.txt"; + let event = exec_command_event( + call_id, + command, + Some(1_000), + SandboxPermissions::UseDefault, + None, + )?; + let _ = mount_sse_once( + &server, + sse(vec![ + ev_response_created("resp-zsh-fork-allow-prefix-1"), + event, + ev_completed("resp-zsh-fork-allow-prefix-1"), + ]), + ) + .await; + let results = mount_sse_once( + &server, + sse(vec![ + ev_assistant_message("msg-zsh-fork-allow-prefix-1", "done"), + ev_completed("resp-zsh-fork-allow-prefix-2"), + ]), + ) + .await; + + submit_turn( + &test, + "allow-prefix-zsh-fork", + approval_policy, + sandbox_policy, + ) + .await?; + + let expected_execpolicy_amendment = ExecPolicyAmendment::new(vec![ + "touch".to_string(), + "allow-prefix-zsh-fork.txt".to_string(), + ]); + let mut saw_parent_approval = false; + let mut saw_subcommand_approval = false; + loop { + let event = wait_for_event(&test.codex, |event| { + matches!( + event, + EventMsg::ExecApprovalRequest(_) | EventMsg::TurnComplete(_) + ) + }) + .await; + + match event { + EventMsg::TurnComplete(_) => break, + EventMsg::ExecApprovalRequest(approval) => { + let command_parts = approval.command.clone(); + let last_arg = command_parts.last().map(String::as_str).unwrap_or_default(); + if last_arg == command { + assert!( + !saw_parent_approval, + "unexpected duplicate parent approval: {command_parts:?}" + ); + saw_parent_approval = true; + test.codex + .submit(Op::ExecApproval { + id: approval.effective_approval_id(), + turn_id: None, + decision: ReviewDecision::Approved, + }) + .await?; + continue; + } + + let is_touch_subcommand = command_parts + .iter() + .any(|part| part == "allow-prefix-zsh-fork.txt") + && command_parts + .first() + .is_some_and(|part| part.ends_with("/touch") || part == "touch"); + if is_touch_subcommand { + assert!( + !saw_subcommand_approval, + "execpolicy amendment should suppress later matching subcommand approvals: {command_parts:?}" + ); + saw_subcommand_approval = true; + assert_eq!( + approval.proposed_execpolicy_amendment, + Some(expected_execpolicy_amendment.clone()) + ); + test.codex + .submit(Op::ExecApproval { + id: approval.effective_approval_id(), + turn_id: None, + decision: ReviewDecision::ApprovedExecpolicyAmendment { + proposed_execpolicy_amendment: expected_execpolicy_amendment + .clone(), + }, + }) + .await?; + continue; + } + + test.codex + .submit(Op::ExecApproval { + id: approval.effective_approval_id(), + turn_id: None, + decision: ReviewDecision::Approved, + }) + .await?; + } + other => panic!("unexpected event: {other:?}"), + } + } + + assert!(saw_parent_approval, "expected parent unified-exec approval"); + assert!( + saw_subcommand_approval, + "expected at least one intercepted touch approval" + ); + + let result = parse_result(&results.single_request().function_call_output(call_id)); + assert_eq!(result.exit_code.unwrap_or(0), 0); + assert!( + allow_prefix_path.exists(), + "expected touch command to complete after approving the first intercepted subcommand; output: {}", + result.stdout + ); + + Ok(()) +} + #[tokio::test(flavor = "multi_thread", worker_threads = 2)] #[cfg(unix)] async fn matched_prefix_rule_runs_unsandboxed_under_zsh_fork() -> Result<()> { diff --git a/codex-rs/core/tests/suite/skill_approval.rs b/codex-rs/core/tests/suite/skill_approval.rs index b5fda12ae0..c04e956b1c 100644 --- a/codex-rs/core/tests/suite/skill_approval.rs +++ b/codex-rs/core/tests/suite/skill_approval.rs @@ -20,6 +20,7 @@ use core_test_support::skip_if_no_network; use core_test_support::test_codex::TestCodex; use core_test_support::wait_for_event; use core_test_support::wait_for_event_match; +use core_test_support::zsh_fork::build_unified_exec_zsh_fork_test; use core_test_support::zsh_fork::build_zsh_fork_test; use core_test_support::zsh_fork::restrictive_workspace_write_policy; use core_test_support::zsh_fork::zsh_fork_runtime; @@ -50,6 +51,13 @@ fn shell_command_arguments(command: &str) -> Result { }))?) } +fn exec_command_arguments(command: &str) -> Result { + Ok(serde_json::to_string(&json!({ + "cmd": command, + "yield_time_ms": 500, + }))?) +} + async fn submit_turn_with_policies( test: &TestCodex, prompt: &str, @@ -89,6 +97,38 @@ echo 'zsh-fork-stderr' >&2 ) } +#[cfg(unix)] +fn write_repo_skill_with_shell_script_contents( + repo_root: &Path, + name: &str, + script_name: &str, + script_contents: &str, +) -> Result { + use std::os::unix::fs::PermissionsExt; + + let skill_dir = repo_root.join(".agents").join("skills").join(name); + let scripts_dir = skill_dir.join("scripts"); + fs::create_dir_all(&scripts_dir)?; + fs::write(repo_root.join(".git"), "gitdir: here")?; + fs::write( + skill_dir.join("SKILL.md"), + format!( + r#"--- +name: {name} +description: {name} skill +--- +"# + ), + )?; + + let script_path = scripts_dir.join(script_name); + fs::write(&script_path, script_contents)?; + let mut permissions = fs::metadata(&script_path)?.permissions(); + permissions.set_mode(0o755); + fs::set_permissions(&script_path, permissions)?; + Ok(script_path) +} + #[cfg(unix)] fn write_skill_with_shell_script_contents( home: &Path, @@ -541,6 +581,168 @@ permissions: Ok(()) } +#[cfg(unix)] +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn unified_exec_zsh_fork_prompts_for_skill_script_execution() -> Result<()> { + skip_if_no_network!(Ok(())); + + let Some(runtime) = zsh_fork_runtime("unified exec zsh-fork skill prompt test")? else { + return Ok(()); + }; + + let server = start_mock_server().await; + let tool_call_id = "uexec-zsh-fork-skill-call"; + let test = build_unified_exec_zsh_fork_test( + &server, + runtime, + AskForApproval::OnRequest, + SandboxPolicy::new_workspace_write_policy(), + |home| { + write_skill_with_shell_script(home, "mbolin-test-skill", "hello-mbolin.sh").unwrap(); + write_skill_metadata( + home, + "mbolin-test-skill", + r#" +permissions: + file_system: + read: + - "./data" + write: + - "./output" +"#, + ) + .unwrap(); + }, + ) + .await?; + + let (script_path_str, command) = skill_script_command(&test, "hello-mbolin.sh")?; + let arguments = exec_command_arguments(&command)?; + let mocks = + mount_function_call_agent_response(&server, tool_call_id, &arguments, "exec_command").await; + + submit_turn_with_policies( + &test, + "use $mbolin-test-skill", + AskForApproval::OnRequest, + SandboxPolicy::new_workspace_write_policy(), + ) + .await?; + + let approval = wait_for_exec_approval_request(&test) + .await + .expect("expected exec approval request before completion"); + assert_eq!(approval.call_id, tool_call_id); + assert_eq!(approval.command, vec![script_path_str.clone()]); + assert_eq!( + approval.available_decisions, + Some(vec![ + ReviewDecision::Approved, + ReviewDecision::ApprovedForSession, + ReviewDecision::Abort, + ]) + ); + assert_eq!( + approval.additional_permissions, + Some(PermissionProfile { + file_system: Some(FileSystemPermissions { + read: Some(vec![absolute_path( + &test.codex_home_path().join("skills/mbolin-test-skill/data"), + )]), + write: Some(vec![absolute_path( + &test + .codex_home_path() + .join("skills/mbolin-test-skill/output"), + )]), + }), + ..Default::default() + }) + ); + + test.codex + .submit(Op::ExecApproval { + id: approval.effective_approval_id(), + turn_id: None, + decision: ReviewDecision::Denied, + }) + .await?; + + wait_for_turn_complete(&test).await; + + let call_output = mocks + .completion + .single_request() + .function_call_output(tool_call_id); + let output = call_output["output"].as_str().unwrap_or_default(); + assert!( + output.contains("Execution denied: User denied execution"), + "expected rejection marker in function_call_output: {output:?}" + ); + + Ok(()) +} + +#[cfg(unix)] +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn unified_exec_zsh_fork_keeps_skill_loading_pinned_to_turn_cwd() -> Result<()> { + skip_if_no_network!(Ok(())); + + let Some(runtime) = zsh_fork_runtime("unified exec zsh-fork turn cwd skill test")? else { + return Ok(()); + }; + + let server = start_mock_server().await; + let tool_call_id = "uexec-zsh-fork-repo-skill-call"; + let test = build_unified_exec_zsh_fork_test( + &server, + runtime, + AskForApproval::OnRequest, + SandboxPolicy::new_workspace_write_policy(), + |_| {}, + ) + .await?; + + let repo_root = test.cwd_path().join("repo"); + let script_path = write_repo_skill_with_shell_script_contents( + &repo_root, + "repo-skill", + "repo-skill.sh", + "#!/bin/sh\necho 'repo-skill-output'\n", + )?; + let script_path_quoted = shlex::try_join([script_path.to_string_lossy().as_ref()])?; + let repo_root_quoted = shlex::try_join([repo_root.to_string_lossy().as_ref()])?; + let command = format!("cd {repo_root_quoted} && {script_path_quoted}"); + let arguments = exec_command_arguments(&command)?; + let mocks = + mount_function_call_agent_response(&server, tool_call_id, &arguments, "exec_command").await; + + submit_turn_with_policies( + &test, + "run the repo skill after changing directories", + AskForApproval::OnRequest, + SandboxPolicy::new_workspace_write_policy(), + ) + .await?; + + let approval = wait_for_exec_approval_request(&test).await; + assert!( + approval.is_none(), + "changing directories inside unified exec should not load repo-local skills from the shell cwd", + ); + + let call_output = mocks + .completion + .single_request() + .function_call_output(tool_call_id); + let output = call_output["output"].as_str().unwrap_or_default(); + assert!( + output.contains("repo-skill-output"), + "expected repo skill script to run without skill-specific approval when only the shell cwd changes: {output:?}" + ); + + Ok(()) +} + /// Permissionless skills should inherit the turn sandbox without prompting. #[cfg(unix)] #[tokio::test(flavor = "multi_thread", worker_threads = 2)] diff --git a/codex-rs/core/tests/suite/unified_exec.rs b/codex-rs/core/tests/suite/unified_exec.rs index 848e777502..0396c95282 100644 --- a/codex-rs/core/tests/suite/unified_exec.rs +++ b/codex-rs/core/tests/suite/unified_exec.rs @@ -1,6 +1,8 @@ use std::collections::HashMap; use std::ffi::OsStr; use std::fs; +#[cfg(unix)] +use std::path::PathBuf; use anyhow::Context; use anyhow::Result; @@ -9,6 +11,7 @@ use codex_protocol::protocol::AskForApproval; use codex_protocol::protocol::EventMsg; use codex_protocol::protocol::ExecCommandSource; use codex_protocol::protocol::Op; +use codex_protocol::protocol::ReviewDecision; use codex_protocol::protocol::SandboxPolicy; use codex_protocol::user_input::UserInput; use core_test_support::assert_regex_match; @@ -31,7 +34,17 @@ use core_test_support::test_codex::test_codex; use core_test_support::wait_for_event; use core_test_support::wait_for_event_match; use core_test_support::wait_for_event_with_timeout; +#[cfg(unix)] +use core_test_support::zsh_fork::build_unified_exec_zsh_fork_test; +#[cfg(unix)] +use core_test_support::zsh_fork::find_host_zsh_path; +#[cfg(unix)] +use core_test_support::zsh_fork::restrictive_workspace_write_policy; +#[cfg(unix)] +use core_test_support::zsh_fork::zsh_fork_runtime; use pretty_assertions::assert_eq; +#[cfg(unix)] +use regex_lite::Regex; use serde_json::Value; use serde_json::json; use tokio::time::Duration; @@ -137,6 +150,172 @@ fn collect_tool_outputs(bodies: &[Value]) -> Result Result { + let output = std::process::Command::new("lsof") + .args(["-Fn", "-a", "-p", pid, "-d", "txt"]) + .output() + .with_context(|| format!("failed to inspect process {pid} executable mapping with lsof"))?; + if !output.status.success() { + return Err(anyhow::anyhow!( + "lsof failed for pid {pid} with status {:?}", + output.status.code() + )); + } + + let stdout = String::from_utf8(output.stdout).context("lsof output was not UTF-8")?; + let path = stdout + .lines() + .find_map(|line| line.strip_prefix('n')) + .ok_or_else(|| anyhow::anyhow!("lsof did not report a text binary path for pid {pid}"))?; + Ok(PathBuf::from(path)) +} + +#[cfg(unix)] +#[allow(clippy::expect_used)] +async fn run_unified_exec_zsh_fork_nested_zsh_rewrite_test( + enable_shell_snapshot: bool, +) -> Result<()> { + let test_name = if enable_shell_snapshot { + "unified exec zsh-fork nested zsh rewrite test with shell snapshot" + } else { + "unified exec zsh-fork nested zsh rewrite test" + }; + let Some(runtime) = zsh_fork_runtime(test_name)? else { + return Ok(()); + }; + let configured_zsh_path = + fs::canonicalize(runtime.zsh_path()).unwrap_or_else(|_| runtime.zsh_path().to_path_buf()); + let host_zsh = match find_host_zsh_path() { + Some(path) => path, + None => { + eprintln!("host zsh not found, skipping nested zsh rewrite test."); + return Ok(()); + } + }; + + let server = start_mock_server().await; + let test = if enable_shell_snapshot { + let mut builder = test_codex().with_config({ + let runtime = runtime.clone(); + move |config| { + runtime.apply_to_config( + config, + AskForApproval::Never, + SandboxPolicy::new_workspace_write_policy(), + ); + config.use_experimental_unified_exec_tool = true; + config + .features + .enable(Feature::UnifiedExec) + .expect("test config should allow feature update"); + config + .features + .enable(Feature::ShellSnapshot) + .expect("test config should allow feature update"); + } + }); + builder.build(&server).await? + } else { + build_unified_exec_zsh_fork_test( + &server, + runtime, + AskForApproval::Never, + SandboxPolicy::new_workspace_write_policy(), + |_| {}, + ) + .await? + }; + + let start_call_id = if enable_shell_snapshot { + "uexec-zsh-fork-nested-start-snapshot" + } else { + "uexec-zsh-fork-nested-start" + }; + let nested_command = format!( + "exec {} -lc 'echo CODEX_NESTED_ZSH_PID=$$; sleep 3; :'", + host_zsh.display(), + ); + let start_args = serde_json::json!({ + "cmd": nested_command, + "yield_time_ms": 500, + "tty": true, + }); + + let responses = vec![ + sse(vec![ + ev_response_created("resp-nested-1"), + ev_function_call( + start_call_id, + "exec_command", + &serde_json::to_string(&start_args)?, + ), + ev_completed("resp-nested-1"), + ]), + sse(vec![ + ev_assistant_message("msg-nested-1", "done"), + ev_completed("resp-nested-2"), + ]), + ]; + let request_log = mount_sse_sequence(&server, responses).await; + + test.codex + .submit(Op::UserTurn { + items: vec![UserInput::Text { + text: "test nested zsh rewrite behavior".into(), + text_elements: Vec::new(), + }], + final_output_json_schema: None, + cwd: test.cwd_path().to_path_buf(), + approval_policy: AskForApproval::Never, + sandbox_policy: SandboxPolicy::new_workspace_write_policy(), + model: test.session_configured.model.clone(), + effort: None, + summary: None, + service_tier: None, + collaboration_mode: None, + personality: None, + }) + .await?; + + wait_for_event(&test.codex, |event| { + matches!(event, EventMsg::TurnComplete(_)) + }) + .await; + + let requests = request_log.requests(); + let bodies = requests + .into_iter() + .map(|request| request.body_json()) + .collect::>(); + let outputs = collect_tool_outputs(&bodies)?; + + let start_output = outputs + .get(start_call_id) + .expect("missing start output for nested zsh exec_command"); + let normalized = start_output.output.replace("\r\n", "\n"); + let nested_zsh_pid = Regex::new(r"CODEX_NESTED_ZSH_PID=(\d+)") + .expect("valid nested zsh pid regex") + .captures(&normalized) + .and_then(|captures| captures.get(1)) + .map(|value| value.as_str().to_string()) + .with_context(|| format!("missing nested zsh pid marker in output {normalized:?}"))?; + assert!( + process_is_alive(&nested_zsh_pid)?, + "nested zsh process should be running before release, got output {normalized:?}" + ); + + let nested_text_binary = process_text_binary_path(&nested_zsh_pid)?; + let nested_text_binary = fs::canonicalize(&nested_text_binary).unwrap_or(nested_text_binary); + assert_eq!( + nested_text_binary, configured_zsh_path, + "nested zsh exec should be rewritten to configured zsh-fork binary, got {:?}", + nested_text_binary, + ); + + Ok(()) +} + #[tokio::test(flavor = "multi_thread", worker_threads = 2)] async fn unified_exec_intercepts_apply_patch_exec_command() -> Result<()> { skip_if_no_network!(Ok(())); @@ -1305,7 +1484,6 @@ async fn exec_command_reports_chunk_and_exit_metadata() -> Result<()> { .into_iter() .map(|request| request.body_json()) .collect::>(); - let outputs = collect_tool_outputs(&bodies)?; let metadata = outputs .get(call_id) @@ -1427,7 +1605,6 @@ async fn unified_exec_defaults_to_pipe() -> Result<()> { .into_iter() .map(|request| request.body_json()) .collect::>(); - let outputs = collect_tool_outputs(&bodies)?; let output = outputs .get(call_id) @@ -1521,7 +1698,6 @@ async fn unified_exec_can_enable_tty() -> Result<()> { .into_iter() .map(|request| request.body_json()) .collect::>(); - let outputs = collect_tool_outputs(&bodies)?; let output = outputs .get(call_id) @@ -1606,7 +1782,6 @@ async fn unified_exec_respects_early_exit_notifications() -> Result<()> { .into_iter() .map(|request| request.body_json()) .collect::>(); - let outputs = collect_tool_outputs(&bodies)?; let output = outputs .get(call_id) @@ -1805,6 +1980,471 @@ async fn write_stdin_returns_exit_metadata_and_clears_session() -> Result<()> { Ok(()) } +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +#[cfg(unix)] +async fn unified_exec_zsh_fork_keeps_python_repl_attached_to_zsh_session() -> Result<()> { + skip_if_no_network!(Ok(())); + + let Some(runtime) = zsh_fork_runtime("unified exec zsh-fork tty session test")? else { + return Ok(()); + }; + let configured_zsh_path = + fs::canonicalize(runtime.zsh_path()).unwrap_or_else(|_| runtime.zsh_path().to_path_buf()); + + let python = match which("python3") { + Ok(path) => path, + Err(_) => { + eprintln!("python3 not found in PATH, skipping zsh-fork python repl test."); + return Ok(()); + } + }; + + let server = start_mock_server().await; + let test = build_unified_exec_zsh_fork_test( + &server, + runtime, + AskForApproval::Never, + SandboxPolicy::new_workspace_write_policy(), + |_| {}, + ) + .await?; + + let start_call_id = "uexec-zsh-fork-python-start"; + let send_call_id = "uexec-zsh-fork-python-pid"; + let exit_call_id = "uexec-zsh-fork-python-exit"; + + let start_command = format!("{}; :", python.display()); + let start_args = serde_json::json!({ + "cmd": start_command, + "yield_time_ms": 500, + "tty": true, + }); + let send_args = serde_json::json!({ + "chars": "import os; print('CODEX_PY_PID=' + str(os.getpid()))\r\n", + "session_id": 1000, + "yield_time_ms": 500, + }); + let exit_args = serde_json::json!({ + "chars": "import sys; sys.exit(0)\r\n", + "session_id": 1000, + "yield_time_ms": 500, + }); + + let responses = vec![ + sse(vec![ + ev_response_created("resp-1"), + ev_function_call( + start_call_id, + "exec_command", + &serde_json::to_string(&start_args)?, + ), + ev_completed("resp-1"), + ]), + sse(vec![ + ev_response_created("resp-2"), + ev_function_call( + send_call_id, + "write_stdin", + &serde_json::to_string(&send_args)?, + ), + ev_completed("resp-2"), + ]), + sse(vec![ + ev_assistant_message("msg-1", "python is running"), + ev_completed("resp-3"), + ]), + sse(vec![ + ev_response_created("resp-4"), + ev_function_call( + exit_call_id, + "write_stdin", + &serde_json::to_string(&exit_args)?, + ), + ev_completed("resp-4"), + ]), + sse(vec![ + ev_assistant_message("msg-2", "all done"), + ev_completed("resp-5"), + ]), + ]; + let request_log = mount_sse_sequence(&server, responses).await; + + test.codex + .submit(Op::UserTurn { + items: vec![UserInput::Text { + text: "test unified exec zsh-fork tty behavior".into(), + text_elements: Vec::new(), + }], + final_output_json_schema: None, + cwd: test.cwd_path().to_path_buf(), + approval_policy: AskForApproval::Never, + sandbox_policy: SandboxPolicy::new_workspace_write_policy(), + model: test.session_configured.model.clone(), + effort: None, + summary: None, + service_tier: None, + collaboration_mode: None, + personality: None, + }) + .await?; + + wait_for_event(&test.codex, |event| { + matches!(event, EventMsg::TurnComplete(_)) + }) + .await; + + let requests = request_log.requests(); + assert!(!requests.is_empty(), "expected at least one POST request"); + let bodies = requests + .into_iter() + .map(|request| request.body_json()) + .collect::>(); + let outputs = collect_tool_outputs(&bodies)?; + + let start_output = outputs + .get(start_call_id) + .expect("missing start output for exec_command"); + let process_id = start_output + .process_id + .clone() + .expect("expected process id from exec_command"); + assert!( + start_output.exit_code.is_none(), + "initial exec_command should leave the PTY session running" + ); + + let send_output = outputs + .get(send_call_id) + .expect("missing write_stdin output"); + let normalized = send_output.output.replace("\r\n", "\n"); + let python_pid = Regex::new(r"CODEX_PY_PID=(\d+)") + .expect("valid python pid marker regex") + .captures(&normalized) + .and_then(|captures| captures.get(1)) + .map(|value| value.as_str().to_string()) + .with_context(|| format!("missing python pid in output {normalized:?}"))?; + assert!( + process_is_alive(&python_pid)?, + "python process should still be alive after printing its pid, got output {normalized:?}" + ); + assert_eq!(send_output.process_id.as_deref(), Some(process_id.as_str())); + assert!( + send_output.exit_code.is_none(), + "write_stdin should not report an exit code while the process is still running" + ); + + let zsh_pid = std::process::Command::new("ps") + .args(["-o", "ppid=", "-p", &python_pid]) + .output() + .context("failed to look up python parent pid")?; + let zsh_pid = String::from_utf8(zsh_pid.stdout) + .context("python parent pid output is not UTF-8")? + .trim() + .to_string(); + assert!( + !zsh_pid.is_empty(), + "expected python parent pid to identify the zsh session" + ); + assert!( + process_is_alive(&zsh_pid)?, + "expected zsh parent process {zsh_pid} to still be alive" + ); + + let zsh_command = std::process::Command::new("ps") + .args(["-o", "command=", "-p", &zsh_pid]) + .output() + .context("failed to look up zsh parent command")?; + let zsh_command = + String::from_utf8(zsh_command.stdout).context("zsh parent command output is not UTF-8")?; + assert!( + zsh_command.contains("zsh"), + "expected python parent command to be zsh, got {zsh_command:?}" + ); + let zsh_text_binary = process_text_binary_path(&zsh_pid)?; + let zsh_text_binary = fs::canonicalize(&zsh_text_binary).unwrap_or(zsh_text_binary); + assert_eq!( + zsh_text_binary, configured_zsh_path, + "python parent shell should run with configured zsh-fork binary, got {:?} ({zsh_command:?})", + zsh_text_binary, + ); + + test.codex + .submit(Op::UserTurn { + items: vec![UserInput::Text { + text: "shut down the python repl".into(), + text_elements: Vec::new(), + }], + final_output_json_schema: None, + cwd: test.cwd_path().to_path_buf(), + approval_policy: AskForApproval::Never, + sandbox_policy: SandboxPolicy::new_workspace_write_policy(), + model: test.session_configured.model.clone(), + effort: None, + summary: None, + service_tier: None, + collaboration_mode: None, + personality: None, + }) + .await?; + + wait_for_event(&test.codex, |event| { + matches!(event, EventMsg::TurnComplete(_)) + }) + .await; + + let requests = request_log.requests(); + assert!(!requests.is_empty(), "expected at least one POST request"); + let bodies = requests + .into_iter() + .map(|request| request.body_json()) + .collect::>(); + let outputs = collect_tool_outputs(&bodies)?; + let exit_output = outputs + .get(exit_call_id) + .expect("missing exit output after requesting python shutdown"); + assert!( + exit_output.exit_code.is_none() || exit_output.exit_code == Some(0), + "exit request should either leave cleanup to the background watcher or report success directly, got {exit_output:?}" + ); + wait_for_process_exit(&python_pid).await?; + + Ok(()) +} + +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +#[cfg(unix)] +async fn unified_exec_zsh_fork_write_stdin_reverts_to_default_sandbox_and_honors_prefix_rule() +-> Result<()> { + skip_if_no_network!(Ok(())); + + let Some(runtime) = zsh_fork_runtime("unified exec zsh-fork stdin sandbox test")? else { + return Ok(()); + }; + + let approval_policy = AskForApproval::OnRequest; + let sandbox_policy = restrictive_workspace_write_policy(); + let server = start_mock_server().await; + let rules = + r#"prefix_rule(pattern=["touch", "allow-prefix.txt"], decision="allow")"#.to_string(); + let test = build_unified_exec_zsh_fork_test( + &server, + runtime, + approval_policy, + sandbox_policy.clone(), + move |home| { + let rules_dir = home.join("rules"); + fs::create_dir_all(&rules_dir).unwrap(); + fs::write(rules_dir.join("default.rules"), &rules).unwrap(); + }, + ) + .await?; + let blocked_path = test.cwd.path().join("blocked-no-prefix.txt"); + let allowed_path = test.cwd.path().join("allow-prefix.txt"); + let _ = fs::remove_file(&blocked_path); + let _ = fs::remove_file(&allowed_path); + + let start_call_id = "uexec-zsh-fork-stdin-start"; + let blocked_call_id = "uexec-zsh-fork-stdin-blocked"; + let allowed_call_id = "uexec-zsh-fork-stdin-allowed"; + let exit_call_id = "uexec-zsh-fork-stdin-exit"; + + let start_args = serde_json::json!({ + "cmd": "/bin/sh", + "yield_time_ms": 500, + "tty": true, + "sandbox_permissions": "require_escalated", + "justification": "start privileged zsh-fork shell for stdin sandbox regression test", + }); + let blocked_args = serde_json::json!({ + "chars": "touch blocked-no-prefix.txt || printf 'BLOCKED\\n'\r\n", + "session_id": 1000, + "yield_time_ms": 500, + }); + let allowed_args = serde_json::json!({ + "chars": "touch allow-prefix.txt && printf 'ALLOW_OK\\n'\r\n", + "session_id": 1000, + "yield_time_ms": 500, + }); + let exit_args = serde_json::json!({ + "chars": "exit\r\n", + "session_id": 1000, + "yield_time_ms": 500, + }); + + let responses = vec![ + sse(vec![ + ev_response_created("resp-stdin-1"), + ev_function_call( + start_call_id, + "exec_command", + &serde_json::to_string(&start_args)?, + ), + ev_completed("resp-stdin-1"), + ]), + sse(vec![ + ev_response_created("resp-stdin-2"), + ev_function_call( + blocked_call_id, + "write_stdin", + &serde_json::to_string(&blocked_args)?, + ), + ev_completed("resp-stdin-2"), + ]), + sse(vec![ + ev_response_created("resp-stdin-3"), + ev_function_call( + allowed_call_id, + "write_stdin", + &serde_json::to_string(&allowed_args)?, + ), + ev_completed("resp-stdin-3"), + ]), + sse(vec![ + ev_response_created("resp-stdin-4"), + ev_function_call( + exit_call_id, + "write_stdin", + &serde_json::to_string(&exit_args)?, + ), + ev_completed("resp-stdin-4"), + ]), + sse(vec![ + ev_assistant_message("msg-stdin-1", "done"), + ev_completed("resp-stdin-5"), + ]), + ]; + let request_log = mount_sse_sequence(&server, responses).await; + + test.codex + .submit(Op::UserTurn { + items: vec![UserInput::Text { + text: "test unified exec zsh-fork stdin sandbox behavior".into(), + text_elements: Vec::new(), + }], + final_output_json_schema: None, + cwd: test.cwd_path().to_path_buf(), + approval_policy, + sandbox_policy, + model: test.session_configured.model.clone(), + effort: None, + summary: None, + service_tier: None, + collaboration_mode: None, + personality: None, + }) + .await?; + + let mut saw_parent_approval = false; + loop { + match wait_for_event(&test.codex, |event| { + matches!( + event, + EventMsg::ExecApprovalRequest(_) | EventMsg::TurnComplete(_) + ) + }) + .await + { + EventMsg::ExecApprovalRequest(approval) => { + assert!( + !saw_parent_approval, + "unexpected additional approval after the privileged parent shell: {:?}", + approval.command + ); + let last_arg = approval + .command + .last() + .map(String::as_str) + .unwrap_or_default(); + assert_eq!(last_arg, "/bin/sh"); + saw_parent_approval = true; + test.codex + .submit(Op::ExecApproval { + id: approval.effective_approval_id(), + turn_id: None, + decision: ReviewDecision::Approved, + }) + .await?; + } + EventMsg::TurnComplete(_) => break, + other => panic!("unexpected event: {other:?}"), + } + } + assert!( + saw_parent_approval, + "expected approval for the parent shell" + ); + + let requests = request_log.requests(); + let bodies = requests + .into_iter() + .map(|request| request.body_json()) + .collect::>(); + let outputs = collect_tool_outputs(&bodies)?; + + let start_output = outputs + .get(start_call_id) + .expect("missing start output for privileged parent shell"); + let process_id = start_output + .process_id + .clone() + .expect("expected process id from privileged parent shell"); + assert!( + start_output.exit_code.is_none(), + "parent shell should stay alive after exec_command" + ); + + let blocked_output = outputs + .get(blocked_call_id) + .expect("missing blocked write_stdin output"); + assert_eq!( + blocked_output.process_id.as_deref(), + Some(process_id.as_str()) + ); + assert!( + blocked_output.output.contains("BLOCKED"), + "expected blocked write_stdin to print BLOCKED, got {:?}", + blocked_output.output + ); + assert!( + !blocked_path.exists(), + "blocked write_stdin should stay sandboxed and not create {blocked_path:?}" + ); + + let allowed_output = outputs + .get(allowed_call_id) + .expect("missing allowed write_stdin output"); + assert_eq!( + allowed_output.process_id.as_deref(), + Some(process_id.as_str()) + ); + assert!( + allowed_output.output.contains("ALLOW_OK"), + "expected prefix-rule write_stdin to print ALLOW_OK, got {:?}", + allowed_output.output + ); + assert!( + allowed_path.exists(), + "prefix-rule write_stdin should create {allowed_path:?}" + ); + + Ok(()) +} + +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +#[cfg(unix)] +async fn unified_exec_zsh_fork_rewrites_nested_zsh_exec_to_configured_binary() -> Result<()> { + skip_if_no_network!(Ok(())); + run_unified_exec_zsh_fork_nested_zsh_rewrite_test(false).await +} + +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +#[cfg(unix)] +async fn unified_exec_zsh_fork_with_shell_snapshot_rewrites_nested_zsh_exec() -> Result<()> { + skip_if_no_network!(Ok(())); + + run_unified_exec_zsh_fork_nested_zsh_rewrite_test(true).await +} + #[tokio::test(flavor = "multi_thread", worker_threads = 2)] async fn unified_exec_emits_end_event_when_session_dies_via_stdin() -> Result<()> { skip_if_no_network!(Ok(()));