From ad2eb4ca5a548202697e5f075aeec0f0fe7e1f27 Mon Sep 17 00:00:00 2001 From: Qiyao Qin Date: Sat, 14 Mar 2026 17:44:09 -0700 Subject: [PATCH] fix run out of sandbox issue --- codex-rs/core/src/tools/events.rs | 13 +- .../core/src/tools/handlers/shell_tests.rs | 164 ++++++++++++++++-- codex-rs/core/src/tools/mod.rs | 25 +++ codex-rs/core/src/tools/orchestrator.rs | 32 +++- 4 files changed, 212 insertions(+), 22 deletions(-) diff --git a/codex-rs/core/src/tools/events.rs b/codex-rs/core/src/tools/events.rs index 5c8384baaf..13e1c49547 100644 --- a/codex-rs/core/src/tools/events.rs +++ b/codex-rs/core/src/tools/events.rs @@ -293,9 +293,16 @@ impl ToolEmitter { ctx: ToolEventCtx<'_>, ) -> String { match self { - Self::Shell { freeform: true, .. } => { - super::format_exec_output_for_model_freeform(output, ctx.turn.truncation_policy) - } + Self::Shell { + freeform: true, + command, + is_command_overridden, + .. + } => super::format_exec_output_for_model_freeform( + output, + ctx.turn.truncation_policy, + is_command_overridden.then_some(command.as_slice()), + ), Self::Shell { command, is_command_overridden, diff --git a/codex-rs/core/src/tools/handlers/shell_tests.rs b/codex-rs/core/src/tools/handlers/shell_tests.rs index d0d3d7d278..1e5e5da172 100644 --- a/codex-rs/core/src/tools/handlers/shell_tests.rs +++ b/codex-rs/core/src/tools/handlers/shell_tests.rs @@ -361,14 +361,8 @@ async fn shell_handler_retry_override_keeps_single_begin_and_reports_override() "echo override-after-retry".to_string(), ] } else { - vec![ - "/bin/sh".to_string(), - "-c".to_string(), - "printf 'override-after-retry\\n'".to_string(), - ] + vec!["/bin/echo".to_string(), "override-after-retry".to_string()] }; - let expected_stdout = "override-after-retry\n".to_string(); - let tracker = Arc::new(tokio::sync::Mutex::new(TurnDiffTracker::new())); let call_id = "call-retry-override".to_string(); let handler = ShellHandler; @@ -465,21 +459,24 @@ async fn shell_handler_retry_override_keeps_single_begin_and_reports_override() let response = timeout(Duration::from_secs(5), handle) .await .expect("timed out waiting for shell handler") - .expect("shell handler join error") - .expect("shell handler should succeed"); + .expect("shell handler join error"); let duration_seconds = (end_event.duration.as_secs_f32() * 10.0).round() / 10.0; let expected_body = serde_json::json!({ - "output": expected_stdout, + "output": end_event.aggregated_output, "metadata": { - "exit_code": 0, + "exit_code": end_event.exit_code, "duration_seconds": duration_seconds, }, "executed_command": override_command, }); + let tool_output = match response { + Ok(output) => tool_output_text(&output), + Err(crate::function_tool::FunctionCallError::RespondToModel(message)) => message, + Err(err) => panic!("unexpected shell handler error: {err:?}"), + }; assert_eq!(begin_count, 1); - assert_eq!(end_event.status, ExecCommandStatus::Completed); assert_eq!( end_event.command, expected_body["executed_command"] @@ -489,10 +486,147 @@ async fn shell_handler_retry_override_keeps_single_begin_and_reports_override() .map(|value| value.as_str().expect("string").to_string()) .collect::>() ); - assert_eq!(end_event.stdout, expected_stdout); assert_eq!( - serde_json::from_str::(&tool_output_text(&response)) - .expect("valid json"), + serde_json::from_str::(&tool_output).expect("valid json"), expected_body ); } + +#[tokio::test] +async fn shell_handler_retry_override_reapplies_sandbox_selection() { + let (session, mut turn_context, rx) = make_session_and_context_with_rx().await; + *session.active_turn.lock().await = Some(ActiveTurn::default()); + Arc::get_mut(&mut turn_context) + .expect("single turn context ref") + .approval_policy + .set(AskForApproval::OnFailure) + .expect("test setup should allow updating approval policy"); + + let temp_dir = tempfile::tempdir_in( + turn_context + .cwd + .parent() + .expect("workspace cwd should have a parent"), + ) + .expect("create temp dir outside workspace"); + let requested_path = temp_dir.path().join("requested-outside-sandbox.txt"); + let override_path = temp_dir.path().join("override-outside-sandbox.txt"); + std::fs::write(&requested_path, b"requested should stay sandboxed\n") + .expect("write requested file"); + std::fs::write(&override_path, b"override should stay sandboxed\n") + .expect("write override file"); + + let requested_command = if cfg!(windows) { + vec![ + "cmd.exe".to_string(), + "/C".to_string(), + format!("type \"{}\"", requested_path.display()), + ] + } else { + vec!["/bin/cat".to_string(), requested_path.display().to_string()] + }; + let override_command = if cfg!(windows) { + vec![ + "cmd.exe".to_string(), + "/C".to_string(), + format!("type \"{}\"", override_path.display()), + ] + } else { + vec!["/bin/cat".to_string(), override_path.display().to_string()] + }; + + let tracker = Arc::new(tokio::sync::Mutex::new(TurnDiffTracker::new())); + let call_id = "call-retry-override-sandboxed".to_string(); + let handler = ShellHandler; + let handle: tokio::task::JoinHandle< + Result, + > = tokio::spawn({ + let session = Arc::clone(&session); + let turn_context = Arc::clone(&turn_context); + let tracker = Arc::clone(&tracker); + let requested_command = requested_command.clone(); + let call_id = call_id.clone(); + async move { + handler + .handle(ToolInvocation { + session, + turn: turn_context.clone(), + tracker, + call_id: call_id.clone(), + tool_name: "shell".to_string(), + tool_namespace: None, + payload: ToolPayload::Function { + arguments: serde_json::json!({ + "command": requested_command, + "workdir": Some(turn_context.cwd.to_string_lossy().to_string()), + "timeout_ms": 1_000u64, + "sandbox_permissions": SandboxPermissions::UseDefault, + }) + .to_string(), + }, + }) + .await + } + }); + + let begin_event = loop { + let event = timeout(Duration::from_secs(5), rx.recv()) + .await + .expect("timed out waiting for begin event") + .expect("begin event missing"); + if let EventMsg::ExecCommandBegin(begin) = event.msg + && begin.call_id == call_id + { + break begin; + } + }; + + let approval_event = loop { + let event = timeout(Duration::from_secs(5), rx.recv()) + .await + .expect("timed out waiting for retry approval request") + .expect("retry approval request missing"); + if let EventMsg::ExecApprovalRequest(request) = event.msg + && request.call_id == call_id + { + break request; + } + }; + + assert_eq!(begin_event.command, requested_command); + assert_eq!(approval_event.command, requested_command); + + session + .notify_approval( + &call_id, + ReviewDecision::ApprovedOverrideCommand { + command: override_command.clone(), + }, + ) + .await; + + let end_event = loop { + let event = timeout(Duration::from_secs(5), rx.recv()) + .await + .expect("timed out waiting for exec end event") + .expect("exec end event missing"); + if let EventMsg::ExecCommandEnd(end) = event.msg + && end.call_id == call_id + { + break end; + } + }; + + let response = timeout(Duration::from_secs(5), handle) + .await + .expect("timed out waiting for shell handler") + .expect("shell handler join error"); + + assert!( + response.is_err(), + "shell handler should keep override command sandboxed" + ); + + assert_eq!(end_event.status, ExecCommandStatus::Failed); + assert_eq!(end_event.command, override_command); +} diff --git a/codex-rs/core/src/tools/mod.rs b/codex-rs/core/src/tools/mod.rs index b1aae60749..f773e19691 100644 --- a/codex-rs/core/src/tools/mod.rs +++ b/codex-rs/core/src/tools/mod.rs @@ -18,6 +18,7 @@ use crate::exec::ExecToolCallOutput; use crate::truncate::TruncationPolicy; use crate::truncate::formatted_truncate_text; use crate::truncate::truncate_text; +use codex_shell_command::parse_command::shlex_join; pub use router::ToolRouter; use serde::Serialize; @@ -75,6 +76,7 @@ pub fn format_exec_output_for_model_structured( pub fn format_exec_output_for_model_freeform( exec_output: &ExecToolCallOutput, truncation_policy: TruncationPolicy, + executed_command: Option<&[String]>, ) -> String { // round to 1 decimal place let duration_seconds = ((exec_output.duration.as_secs_f32()) * 10.0).round() / 10.0; @@ -87,6 +89,9 @@ pub fn format_exec_output_for_model_freeform( let mut sections = Vec::new(); + if let Some(command) = executed_command { + sections.push(format!("Executed command: {}", shlex_join(command))); + } sections.push(format!("Exit code: {}", exec_output.exit_code)); sections.push(format!("Wall time: {duration_seconds} seconds")); if total_lines != formatted_output.lines().count() { @@ -146,4 +151,24 @@ mod tests { r#"{"output":"override-ok\n","metadata":{"exit_code":0,"duration_seconds":0.1},"executed_command":["/bin/echo","override-ok"]}"# ); } + + #[test] + fn format_exec_output_for_model_freeform_includes_executed_command_when_present() { + let exec_output = ExecToolCallOutput { + aggregated_output: crate::exec::StreamOutput::new("override-ok\n".to_string()), + duration: Duration::from_millis(100), + ..ExecToolCallOutput::default() + }; + + let formatted = format_exec_output_for_model_freeform( + &exec_output, + TruncationPolicy::Bytes(1024), + Some(&["/bin/echo".to_string(), "override-ok".to_string()]), + ); + + assert_eq!( + formatted, + "Executed command: /bin/echo override-ok\nExit code: 0\nWall time: 0.1 seconds\nOutput:\noverride-ok\n" + ); + } } diff --git a/codex-rs/core/src/tools/orchestrator.rs b/codex-rs/core/src/tools/orchestrator.rs index 840eeb1991..da5b1ecf1e 100644 --- a/codex-rs/core/src/tools/orchestrator.rs +++ b/codex-rs/core/src/tools/orchestrator.rs @@ -286,6 +286,7 @@ impl ToolOrchestrator { let bypass_retry_approval = tool .should_bypass_approval(approval_policy, already_approved) && network_approval_context.is_none(); + let mut command_overridden_on_retry = false; if !bypass_retry_approval { let approval_ctx = ApprovalCtx { session: &effective_tool_ctx.session, @@ -299,6 +300,7 @@ impl ToolOrchestrator { let decision = tool.start_approval_async(req, approval_ctx).await; otel.tool_decision(otel_tn, otel_ci, &decision, otel_user); if let Some(command) = decision.override_command() { + command_overridden_on_retry = true; effective_tool_ctx.command_override = Some(command.to_vec()); if let Some(effective_command) = &effective_tool_ctx.effective_command { *effective_command.lock().await = command.to_vec(); @@ -329,15 +331,37 @@ impl ToolOrchestrator { } } - let escalated_attempt = SandboxAttempt { - sandbox: crate::exec::SandboxType::None, + let retry_sandbox = if command_overridden_on_retry { + match tool.sandbox_mode_for_first_attempt(req) { + SandboxOverride::BypassSandboxFirstAttempt => { + crate::exec::SandboxType::None + } + SandboxOverride::NoOverride => self.sandbox.select_initial( + &turn_ctx.file_system_sandbox_policy, + turn_ctx.network_sandbox_policy, + tool.sandbox_preference(), + turn_ctx.windows_sandbox_level, + has_managed_network_requirements, + ), + } + } else { + crate::exec::SandboxType::None + }; + let retry_codex_linux_sandbox_exe = if command_overridden_on_retry { + turn_ctx.codex_linux_sandbox_exe.as_ref() + } else { + None + }; + + let retry_attempt = SandboxAttempt { + sandbox: retry_sandbox, policy: &turn_ctx.sandbox_policy, file_system_policy: &turn_ctx.file_system_sandbox_policy, network_policy: turn_ctx.network_sandbox_policy, enforce_managed_network: has_managed_network_requirements, manager: &self.sandbox, sandbox_cwd: &turn_ctx.cwd, - codex_linux_sandbox_exe: None, + codex_linux_sandbox_exe: retry_codex_linux_sandbox_exe, use_legacy_landlock, windows_sandbox_level: turn_ctx.windows_sandbox_level, windows_sandbox_private_desktop: turn_ctx @@ -351,7 +375,7 @@ impl ToolOrchestrator { tool, req, &effective_tool_ctx, - &escalated_attempt, + &retry_attempt, has_managed_network_requirements, ) .await;