From 9f06cf1a0907f650887ca26af5cee13b4f9a13ea Mon Sep 17 00:00:00 2001 From: jif Date: Mon, 22 Jun 2026 18:33:28 +0100 Subject: [PATCH] Report remote sandbox denials semantically (#29424) ## Why #29113 moved remote sandbox setup and enforcement to the exec server. That gives the executor ownership of the platform-specific work: a Linux executor chooses and runs a Linux sandbox even when the Codex orchestrator is running on macOS or Windows. It also means the orchestrator no longer knows which concrete sandbox the executor selected. When that sandbox blocks a remote command, the orchestrator currently sees only a failed process and can treat the denial as an ordinary command failure. The existing sandbox approval and retry path is then skipped. This PR lets the executor report one portable fact: > This command probably failed because the executor sandbox blocked it. The executor keeps its concrete sandbox type private. The protocol sends only the semantic result. ## Example Suppose a local macOS Codex session asks a Linux devbox to write outside the allowed workspace. Before this PR: ```text Linux sandbox blocks the write -> remote process exits with "Permission denied" -> local orchestrator sees an ordinary command failure -> the normal sandbox approval and retry path can be skipped ``` With this PR: ```text Linux sandbox blocks the write -> executor reports sandboxDenied: true -> unified exec returns UnifiedExecError::SandboxDenied -> the existing approval prompt is shown -> an approved retry runs through the existing unsandboxed retry path ``` ## What changes ### The executor remembers its selected sandbox The prepared remote process now retains the executor-selected `SandboxType`. This value never crosses the executor boundary. Commands started without a sandbox retain `SandboxType::None` and are never reported as sandbox denials. ### The executor uses the existing denial heuristic The existing local denial heuristic moves from `codex-core` into the shared `codex-sandboxing` crate. When a sandboxed remote process exits, the executor: 1. waits the same short output grace period used by local unified exec; 2. reads the output currently available in the existing retained output buffer; 3. runs the existing heuristic using the exit code and common denial messages; 4. stores the yes/no result before publishing the process exit. This deliberately matches the old local unified-exec behavior. It does not add a new streaming classifier, another output buffer, or stronger output-retention guarantees. ### The protocol reports a portable boolean `process/read` gains `sandboxDenied`: ```json { "exited": true, "exitCode": 1, "closed": false, "sandboxDenied": true } ``` The field defaults to `false` when an older executor omits it. The response does not expose the executor sandbox implementation or executor-native paths. ### Unified exec uses the existing error path The exec-server client carries `sandboxDenied` into the unified process state. If it is true, unified exec returns the existing `SandboxDenied` error instead of trying to classify remote output using an orchestrator-side sandbox type. Remote process exit remains visible as soon as the process exits. This PR does not wait for stdout or stderr to close and does not change the existing process lifecycle. ## Scope This PR is intentionally limited to matching the existing local unified-exec behavior for the initial command execution path. It does not add: - incremental denial tracking across the full output stream; - new denial handling for commands completed later through `write_stdin`; - new guarantees for preserving the semantic flag during the narrow reconnect-recovery race. Those can be considered separately if the same behavior is added for local execution. ## Test coverage One remote end-to-end integration test covers the complete intended flow: ```text remote read-only sandbox -> denied write -> executor reports the denial -> Codex requests approval -> user approves -> retry succeeds on the remote executor ``` Existing lifecycle coverage continues to verify that remote process exit is reported before late output streams close. --- codex-rs/core/src/exec.rs | 63 +------- codex-rs/core/src/unified_exec/mod_tests.rs | 20 ++- codex-rs/core/src/unified_exec/process.rs | 19 ++- .../core/src/unified_exec/process_manager.rs | 15 +- .../src/unified_exec/process_manager_tests.rs | 19 ++- .../core/src/unified_exec/process_state.rs | 3 + .../core/src/unified_exec/process_tests.rs | 7 +- codex-rs/core/tests/suite/remote_env.rs | 152 +++++++++++++++++- codex-rs/exec-server/src/client.rs | 2 + codex-rs/exec-server/src/client_recovery.rs | 1 + codex-rs/exec-server/src/local_process.rs | 46 ++++++ codex-rs/exec-server/src/process_sandbox.rs | 3 + codex-rs/exec-server/src/protocol.rs | 3 + codex-rs/exec-server/tests/exec_process.rs | 18 +-- codex-rs/exec-server/tests/process.rs | 4 +- codex-rs/exec-server/tests/relay.rs | 2 +- codex-rs/sandboxing/src/denial.rs | 57 +++++++ codex-rs/sandboxing/src/lib.rs | 2 + 18 files changed, 330 insertions(+), 106 deletions(-) create mode 100644 codex-rs/sandboxing/src/denial.rs diff --git a/codex-rs/core/src/exec.rs b/codex-rs/core/src/exec.rs index 99c87cdd9..647203db2 100644 --- a/codex-rs/core/src/exec.rs +++ b/codex-rs/core/src/exec.rs @@ -42,6 +42,7 @@ use codex_sandboxing::SandboxTransformRequest; use codex_sandboxing::SandboxType; use codex_sandboxing::SandboxablePreference; use codex_sandboxing::WindowsSandboxFilesystemOverrides; +pub(crate) use codex_sandboxing::is_likely_sandbox_denied; #[cfg(test)] use codex_sandboxing::permission_profile_supports_windows_restricted_token_sandbox; use codex_sandboxing::resolve_windows_elevated_filesystem_overrides; @@ -819,68 +820,6 @@ fn finalize_exec_result( } } -/// We don't have a fully deterministic way to tell if our command failed -/// because of the sandbox - a command in the user's zshrc file might hit an -/// error, but the command itself might fail or succeed for other reasons. -/// For now, we conservatively check for well known command failure exit codes and -/// also look for common sandbox denial keywords in the command output. -pub(crate) fn is_likely_sandbox_denied( - sandbox_type: SandboxType, - exec_output: &ExecToolCallOutput, -) -> bool { - if sandbox_type == SandboxType::None || exec_output.exit_code == 0 { - return false; - } - - // Quick rejects: well-known non-sandbox shell exit codes - // 2: misuse of shell builtins - // 126: permission denied - // 127: command not found - const SANDBOX_DENIED_KEYWORDS: [&str; 7] = [ - "operation not permitted", - "permission denied", - "read-only file system", - "seccomp", - "sandbox", - "landlock", - "failed to write file", - ]; - - let has_sandbox_keyword = [ - &exec_output.stderr.text, - &exec_output.stdout.text, - &exec_output.aggregated_output.text, - ] - .into_iter() - .any(|section| { - let lower = section.to_lowercase(); - SANDBOX_DENIED_KEYWORDS - .iter() - .any(|needle| lower.contains(needle)) - }); - - if has_sandbox_keyword { - return true; - } - - const QUICK_REJECT_EXIT_CODES: [i32; 3] = [2, 126, 127]; - if QUICK_REJECT_EXIT_CODES.contains(&exec_output.exit_code) { - return false; - } - - #[cfg(unix)] - { - const SIGSYS_CODE: i32 = libc::SIGSYS; - if sandbox_type == SandboxType::LinuxSeccomp - && exec_output.exit_code == EXIT_CODE_SIGNAL_BASE + SIGSYS_CODE - { - return true; - } - } - - false -} - #[derive(Debug)] struct RawExecToolCallOutput { pub exit_status: ExitStatus, diff --git a/codex-rs/core/src/unified_exec/mod_tests.rs b/codex-rs/core/src/unified_exec/mod_tests.rs index d5b813cb4..2dc0afd26 100644 --- a/codex-rs/core/src/unified_exec/mod_tests.rs +++ b/codex-rs/core/src/unified_exec/mod_tests.rs @@ -228,6 +228,7 @@ impl BlockingTerminateExecProcess { exit_code: None, closed: false, failure: None, + sandbox_denied: false, }) } @@ -286,17 +287,14 @@ async fn blocking_terminate_unified_process( ) -> anyhow::Result> { let (wake_tx, _wake_rx) = watch::channel(0); Ok(Arc::new( - UnifiedExecProcess::from_exec_server_started( - StartedExecProcess { - process: Arc::new(BlockingTerminateExecProcess { - process_id: process_id.to_string().into(), - terminate_started, - allow_terminate, - wake_tx, - }), - }, - SandboxType::None, - ) + UnifiedExecProcess::from_exec_server_started(StartedExecProcess { + process: Arc::new(BlockingTerminateExecProcess { + process_id: process_id.to_string().into(), + terminate_started, + allow_terminate, + wake_tx, + }), + }) .await?, )) } diff --git a/codex-rs/core/src/unified_exec/process.rs b/codex-rs/core/src/unified_exec/process.rs index 725be9eed..5f785adfa 100644 --- a/codex-rs/core/src/unified_exec/process.rs +++ b/codex-rs/core/src/unified_exec/process.rs @@ -285,8 +285,9 @@ impl UnifiedExecProcess { &self, text: &str, ) -> Result<(), UnifiedExecError> { + let executor_reported_denial = self.state_rx.borrow().sandbox_denied; let sandbox_type = self.sandbox_type(); - if sandbox_type == SandboxType::None || !self.has_exited() { + if !self.has_exited() || (!executor_reported_denial && sandbox_type == SandboxType::None) { return Ok(()); } @@ -297,7 +298,7 @@ impl UnifiedExecProcess { aggregated_output: StreamOutput::new(text.to_string()), ..Default::default() }; - if is_likely_sandbox_denied(sandbox_type, &exec_output) { + if executor_reported_denial || is_likely_sandbox_denied(sandbox_type, &exec_output) { let snippet = formatted_truncate_text( text, TruncationPolicy::Tokens(UNIFIED_EXEC_OUTPUT_MAX_TOKENS), @@ -374,10 +375,13 @@ impl UnifiedExecProcess { pub(super) async fn from_exec_server_started( started: StartedExecProcess, - sandbox_type: SandboxType, ) -> Result { let process_handle = ProcessHandle::ExecServer(Arc::clone(&started.process)); - let mut managed = Self::new(process_handle, sandbox_type, /*spawn_lifecycle*/ None); + let mut managed = Self::new( + process_handle, + SandboxType::None, + /*spawn_lifecycle*/ None, + ); let output_handles = managed.output_handles(); managed.output_task = Some(Self::spawn_exec_server_output_task( started, @@ -437,6 +441,7 @@ impl UnifiedExecProcess { exit_code, closed, failure, + sandbox_denied, } = response; for chunk in chunks { @@ -457,6 +462,12 @@ impl UnifiedExecProcess { break; } + if sandbox_denied { + let mut state = state_tx.borrow().clone(); + state.sandbox_denied = true; + let _ = state_tx.send_replace(state); + } + if exited { let state = state_tx.borrow().clone(); let _ = state_tx.send_replace(state.exited(exit_code)); diff --git a/codex-rs/core/src/unified_exec/process_manager.rs b/codex-rs/core/src/unified_exec/process_manager.rs index a3b4bcf9c..2cb62a6d4 100644 --- a/codex-rs/core/src/unified_exec/process_manager.rs +++ b/codex-rs/core/src/unified_exec/process_manager.rs @@ -10,6 +10,7 @@ use tokio::sync::watch; use tokio::time::Duration; use tokio::time::Instant; use tokio_util::sync::CancellationToken; +use uuid::Uuid; use crate::codex_thread::BackgroundTerminalInfo; use crate::exec_env::CODEX_THREAD_ID_ENV_VAR; @@ -157,8 +158,14 @@ fn exec_server_params_for_request( tty: bool, ) -> codex_exec_server::ExecParams { let (env_policy, env) = exec_server_env_for_request(request); + // Sandbox retries reuse the unified-exec ID but start a distinct executor process. + let exec_server_process_id = if request.exec_server_sandbox.is_some() { + format!("{process_id}-{}", Uuid::new_v4()) + } else { + process_id.to_string() + }; codex_exec_server::ExecParams { - process_id: exec_server_process_id(process_id).into(), + process_id: exec_server_process_id.into(), argv: request.command.clone(), cwd: request.cwd.clone(), env_policy, @@ -198,10 +205,6 @@ impl Drop for InitialExecCommandGuard { } } -fn exec_server_process_id(process_id: i32) -> String { - process_id.to_string() -} - async fn unregister_network_approval_for_entry(entry: &ProcessEntry) { if let Some(network_approval) = entry.network_approval.as_ref() && let Some(session) = entry.session.upgrade() @@ -1041,7 +1044,7 @@ impl UnifiedExecProcessManager { .await .map_err(|err| UnifiedExecError::create_process(err.to_string()))?; spawn_lifecycle.after_spawn(); - return UnifiedExecProcess::from_exec_server_started(started, request.sandbox).await; + return UnifiedExecProcess::from_exec_server_started(started).await; } // TODO(anp): Keep PathUri through the local PTY/process launch boundary. diff --git a/codex-rs/core/src/unified_exec/process_manager_tests.rs b/codex-rs/core/src/unified_exec/process_manager_tests.rs index b6afdc85b..cd48d01c7 100644 --- a/codex-rs/core/src/unified_exec/process_manager_tests.rs +++ b/codex-rs/core/src/unified_exec/process_manager_tests.rs @@ -76,7 +76,7 @@ fn exec_server_params_use_path_uri_and_env_policy_overlay_contract() { codex_protocol::permissions::FileSystemSandboxPolicy::unrestricted(); let network_sandbox_policy = codex_protocol::permissions::NetworkSandboxPolicy::Restricted; let permission_profile = codex_protocol::models::PermissionProfile::Disabled; - let request = ExecRequest { + let mut request = ExecRequest { command: vec!["bash".to_string(), "-lc".to_string(), "true".to_string()], cwd: cwd.clone().into(), env: HashMap::from([ @@ -106,7 +106,7 @@ fn exec_server_params_use_path_uri_and_env_policy_overlay_contract() { windows_sandbox_workspace_roots: vec![cwd], windows_sandbox_level: codex_protocol::config_types::WindowsSandboxLevel::Disabled, windows_sandbox_private_desktop: false, - permission_profile, + permission_profile: permission_profile.clone(), file_system_sandbox_policy, network_sandbox_policy, windows_sandbox_filesystem_overrides: None, @@ -128,11 +128,16 @@ fn exec_server_params_use_path_uri_and_env_policy_overlay_contract() { ("CODEX_THREAD_ID".to_string(), "thread-1".to_string()), ]) ); -} - -#[test] -fn exec_server_process_id_matches_unified_exec_process_id() { - assert_eq!(exec_server_process_id(/*process_id*/ 4321), "4321"); + request.exec_server_sandbox = Some( + codex_exec_server::FileSystemSandboxContext::from_permission_profile(permission_profile), + ); + let first = + exec_server_params_for_request(/*process_id*/ 123, &request, /*tty*/ true); + let second = + exec_server_params_for_request(/*process_id*/ 123, &request, /*tty*/ true); + assert!(first.process_id.as_str().starts_with("123-")); + assert!(second.process_id.as_str().starts_with("123-")); + assert_ne!(first.process_id, second.process_id); } #[cfg(windows)] diff --git a/codex-rs/core/src/unified_exec/process_state.rs b/codex-rs/core/src/unified_exec/process_state.rs index 267406da2..65e11b6f3 100644 --- a/codex-rs/core/src/unified_exec/process_state.rs +++ b/codex-rs/core/src/unified_exec/process_state.rs @@ -3,6 +3,7 @@ pub(crate) struct ProcessState { pub(crate) has_exited: bool, pub(crate) exit_code: Option, pub(crate) failure_message: Option, + pub(crate) sandbox_denied: bool, } impl ProcessState { @@ -11,6 +12,7 @@ impl ProcessState { has_exited: true, exit_code, failure_message: self.failure_message.clone(), + sandbox_denied: self.sandbox_denied, } } @@ -19,6 +21,7 @@ impl ProcessState { has_exited: true, exit_code: self.exit_code, failure_message: Some(message), + sandbox_denied: self.sandbox_denied, } } } diff --git a/codex-rs/core/src/unified_exec/process_tests.rs b/codex-rs/core/src/unified_exec/process_tests.rs index 37ec5d858..db64d461e 100644 --- a/codex-rs/core/src/unified_exec/process_tests.rs +++ b/codex-rs/core/src/unified_exec/process_tests.rs @@ -10,7 +10,6 @@ use codex_exec_server::ReadResponse; use codex_exec_server::StartedExecProcess; use codex_exec_server::WriteResponse; use codex_exec_server::WriteStatus; -use codex_sandboxing::SandboxType; use pretty_assertions::assert_eq; use std::collections::VecDeque; use std::sync::Arc; @@ -40,6 +39,7 @@ impl MockExecProcess { exit_code: None, closed: false, failure: None, + sandbox_denied: false, })) } @@ -103,7 +103,7 @@ async fn remote_process( }), }; - UnifiedExecProcess::from_exec_server_started(started, SandboxType::None) + UnifiedExecProcess::from_exec_server_started(started) .await .expect("remote process should start") } @@ -190,6 +190,7 @@ async fn remote_process_waits_for_early_exit_event() { exit_code: Some(17), closed: true, failure: None, + sandbox_denied: false, }])), terminate_error: None, wake_tx: wake_tx.clone(), @@ -201,7 +202,7 @@ async fn remote_process_waits_for_early_exit_event() { let _ = wake_tx.send(1); }); - let process = UnifiedExecProcess::from_exec_server_started(started, SandboxType::None) + let process = UnifiedExecProcess::from_exec_server_started(started) .await .expect("remote process should observe early exit"); diff --git a/codex-rs/core/tests/suite/remote_env.rs b/codex-rs/core/tests/suite/remote_env.rs index 337ca8d40..fa805fa44 100644 --- a/codex-rs/core/tests/suite/remote_env.rs +++ b/codex-rs/core/tests/suite/remote_env.rs @@ -87,6 +87,7 @@ async fn submit_turn_with_approval_and_environments( test: &TestCodex, prompt: &str, environments: Vec, + approval_policy: AskForApproval, ) -> Result<()> { let turn_environment_selections = codex_protocol::protocol::TurnEnvironmentSelections::new( test.config.cwd.clone(), @@ -103,7 +104,7 @@ async fn submit_turn_with_approval_and_environments( additional_context: Default::default(), thread_settings: codex_protocol::protocol::ThreadSettingsOverrides { environments: Some(turn_environment_selections), - approval_policy: Some(AskForApproval::OnRequest), + approval_policy: Some(approval_policy), approvals_reviewer: Some(ApprovalsReviewer::User), sandbox_policy: Some(SandboxPolicy::new_read_only_policy()), collaboration_mode: Some(codex_protocol::config_types::CollaborationMode { @@ -312,6 +313,151 @@ async fn explicit_remote_shell_runs_in_remote_cwd() -> Result<()> { Ok(()) } +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn remote_sandbox_denial_requests_approval_and_retries() -> Result<()> { + skip_if_no_network!(Ok(())); + skip_if_wine_exec!(Ok(()), "requires the Docker-backed POSIX executor"); + let Some(_remote_env) = get_remote_test_env() else { + return Ok(()); + }; + + const CALL_ID: &str = "remote-sandbox-denial"; + const CONTENTS: &str = "remote sandbox retry succeeded"; + + let server = start_mock_server().await; + let test = unified_exec_test(&server).await?; + let nonce = SystemTime::now().duration_since(UNIX_EPOCH)?.as_millis(); + let remote_cwd = PathBuf::from(format!("/tmp/codex-remote-denial-cwd-{nonce}")).abs(); + let target_path = PathBuf::from(format!("/tmp/codex-remote-denial-target-{nonce}")).abs(); + let remote_cwd_uri = PathUri::from_path(&remote_cwd)?; + let target_uri = PathUri::from_path(&target_path)?; + test.fs() + .create_directory( + &remote_cwd_uri, + CreateDirectoryOptions { recursive: true }, + /*sandbox*/ None, + ) + .await?; + test.fs() + .remove( + &target_uri, + RemoveOptions { + recursive: false, + force: true, + }, + /*sandbox*/ None, + ) + .await?; + + let command = format!("printf {CONTENTS:?} > {target_path:?} && cat {target_path:?}"); + let response_mock = mount_sse_sequence( + &server, + vec![ + sse(vec![ + ev_response_created("resp-remote-denial-1"), + ev_function_call( + CALL_ID, + "exec_command", + &json!({ + "shell": "/bin/sh", + "cmd": command, + "login": false, + "yield_time_ms": 5_000, + "environment_id": REMOTE_ENVIRONMENT_ID, + }) + .to_string(), + ), + ev_completed("resp-remote-denial-1"), + ]), + sse(vec![ + ev_response_created("resp-remote-denial-2"), + ev_assistant_message("msg-remote-denial", "done"), + ev_completed("resp-remote-denial-2"), + ]), + ], + ) + .await; + + submit_turn_with_approval_and_environments( + &test, + "retry a sandbox-denied command in the remote environment", + vec![TurnEnvironmentSelection { + environment_id: REMOTE_ENVIRONMENT_ID.to_string(), + cwd: PathUri::from_abs_path(&remote_cwd), + }], + AskForApproval::OnFailure, + ) + .await?; + + let event = wait_for_event(&test.codex, |event| { + matches!( + event, + EventMsg::ExecApprovalRequest(_) | EventMsg::TurnComplete(_) + ) + }) + .await; + let EventMsg::ExecApprovalRequest(approval) = event else { + panic!("expected remote sandbox approval before completion: {event:?}"); + }; + assert_eq!(approval.call_id, CALL_ID); + assert_eq!( + approval.environment_id.as_deref(), + Some(REMOTE_ENVIRONMENT_ID) + ); + assert_eq!( + approval.reason.as_deref(), + Some("command failed; retry without sandbox?") + ); + + test.codex + .submit(Op::ExecApproval { + id: approval.effective_approval_id(), + turn_id: None, + decision: ReviewDecision::Approved, + }) + .await?; + wait_for_event(&test.codex, |event| { + matches!(event, EventMsg::TurnComplete(_)) + }) + .await; + + assert!( + response_mock + .function_call_output_text(CALL_ID) + .is_some_and(|output| output.contains(CONTENTS)), + "approved retry should return the remote command output" + ); + assert_eq!( + test.fs() + .read_file_text(&target_uri, /*sandbox*/ None) + .await?, + CONTENTS + ); + + test.fs() + .remove( + &target_uri, + RemoveOptions { + recursive: false, + force: true, + }, + /*sandbox*/ None, + ) + .await?; + test.fs() + .remove( + &remote_cwd_uri, + RemoveOptions { + recursive: true, + force: true, + }, + /*sandbox*/ None, + ) + .await?; + + Ok(()) +} + #[tokio::test(flavor = "multi_thread", worker_threads = 2)] async fn deferred_executor_does_not_duplicate_initial_environment_context() -> Result<()> { let server = start_mock_server().await; @@ -834,6 +980,7 @@ async fn remote_request_permissions_grant_unblocks_later_remote_exec() -> Result cwd: PathUri::from_abs_path(&remote_cwd), }, ], + AskForApproval::OnRequest, ) .await?; @@ -1115,6 +1262,7 @@ async fn apply_patch_approvals_are_remembered_per_environment() -> Result<()> { &test, "apply patch in local environment", environments.clone(), + AskForApproval::OnRequest, ) .await?; let approval = expect_patch_approval(&test, "call-local").await; @@ -1134,6 +1282,7 @@ async fn apply_patch_approvals_are_remembered_per_environment() -> Result<()> { &test, "apply patch in remote environment", environments.clone(), + AskForApproval::OnRequest, ) .await?; let approval = expect_patch_approval(&test, "call-remote").await; @@ -1158,6 +1307,7 @@ async fn apply_patch_approvals_are_remembered_per_environment() -> Result<()> { &test, "apply patch again in remote environment", environments, + AskForApproval::OnRequest, ) .await?; wait_for_completion_without_patch_approval(&test).await; diff --git a/codex-rs/exec-server/src/client.rs b/codex-rs/exec-server/src/client.rs index 88c0c8034..25d1b54e8 100644 --- a/codex-rs/exec-server/src/client.rs +++ b/codex-rs/exec-server/src/client.rs @@ -926,6 +926,7 @@ impl SessionState { exit_code: None, closed: true, failure: Some(message), + sandbox_denied: false, } } @@ -1869,6 +1870,7 @@ mod tests { exit_code: None, closed: false, failure: None, + sandbox_denied: false, }) .expect("read response should serialize"), }), diff --git a/codex-rs/exec-server/src/client_recovery.rs b/codex-rs/exec-server/src/client_recovery.rs index 77feb0a4b..f58e36572 100644 --- a/codex-rs/exec-server/src/client_recovery.rs +++ b/codex-rs/exec-server/src/client_recovery.rs @@ -58,6 +58,7 @@ impl SessionState { exit_code, closed, failure, + sandbox_denied: _, } = response; if let Some(message) = failure { return Err(ExecServerError::Protocol(format!( diff --git a/codex-rs/exec-server/src/local_process.rs b/codex-rs/exec-server/src/local_process.rs index 802c8e69c..c7e99de53 100644 --- a/codex-rs/exec-server/src/local_process.rs +++ b/codex-rs/exec-server/src/local_process.rs @@ -10,7 +10,11 @@ use std::time::Duration; use codex_app_server_protocol::JSONRPCErrorError; use codex_protocol::config_types::EnvironmentVariablePattern; use codex_protocol::config_types::ShellEnvironmentPolicy; +use codex_protocol::exec_output::ExecToolCallOutput; +use codex_protocol::exec_output::StreamOutput; use codex_protocol::shell_environment; +use codex_sandboxing::SandboxType; +use codex_sandboxing::is_likely_sandbox_denied; use codex_utils_pty::ExecCommandSession; use codex_utils_pty::ProcessSignal as PtyProcessSignal; use codex_utils_pty::TerminalSize; @@ -87,6 +91,8 @@ struct RunningProcess { output_notify: Arc, open_streams: usize, closed: bool, + sandbox: SandboxType, + sandbox_denied: bool, } /// Bounded cache of stdin write ids that have already been accepted for one process. @@ -313,6 +319,8 @@ impl LocalProcess { output_notify: Arc::clone(&output_notify), open_streams: 2, closed: false, + sandbox: prepared.sandbox, + sandbox_denied: false, })), ); } @@ -407,6 +415,7 @@ impl LocalProcess { exit_code: process.exit_code, closed: process.closed, failure: None, + sandbox_denied: process.sandbox_denied, }, Arc::clone(&process.output_notify), ) @@ -797,12 +806,46 @@ async fn watch_exit( output_notify: Arc, ) { let exit_code = exit_rx.await.unwrap_or(-1); + let sandboxed = { + let processes = inner.processes.lock().await; + matches!( + processes.get(&process_id), + Some(ProcessEntry::Running(process)) if process.sandbox != SandboxType::None + ) + }; + if sandboxed { + let _ = tokio::time::timeout(Duration::from_millis(20), output_notify.notified()).await; + } let notification = { let mut processes = inner.processes.lock().await; if let Some(ProcessEntry::Running(process)) = processes.get_mut(&process_id) { let seq = process.next_seq; process.next_seq += 1; process.exit_code = Some(exit_code); + if process.sandbox != SandboxType::None { + let mut stdout = Vec::new(); + let mut stderr = Vec::new(); + let mut aggregated = Vec::new(); + for chunk in &process.output { + match chunk.stream { + ExecOutputStream::Stdout | ExecOutputStream::Pty => { + stdout.extend_from_slice(&chunk.chunk); + } + ExecOutputStream::Stderr => stderr.extend_from_slice(&chunk.chunk), + } + aggregated.extend_from_slice(&chunk.chunk); + } + let exec_output = ExecToolCallOutput { + exit_code, + stdout: StreamOutput::new(String::from_utf8_lossy(&stdout).into_owned()), + stderr: StreamOutput::new(String::from_utf8_lossy(&stderr).into_owned()), + aggregated_output: StreamOutput::new( + String::from_utf8_lossy(&aggregated).into_owned(), + ), + ..Default::default() + }; + process.sandbox_denied = is_likely_sandbox_denied(process.sandbox, &exec_output); + } let _ = process.wake_tx.send(seq); process .events @@ -1000,6 +1043,7 @@ mod tests { exit_code: Some(0), closed: false, failure: None, + sandbox_denied: false, } ); @@ -1124,6 +1168,8 @@ mod tests { output_notify: Arc::clone(&output_notify), open_streams: 2, closed: false, + sandbox: SandboxType::None, + sandbox_denied: false, })), ); assert!(previous.is_none()); diff --git a/codex-rs/exec-server/src/process_sandbox.rs b/codex-rs/exec-server/src/process_sandbox.rs index be95b92d3..d157f4c6d 100644 --- a/codex-rs/exec-server/src/process_sandbox.rs +++ b/codex-rs/exec-server/src/process_sandbox.rs @@ -20,6 +20,7 @@ pub(crate) struct PreparedExecRequest { pub(crate) cwd: AbsolutePathBuf, pub(crate) env: HashMap, pub(crate) arg0: Option, + pub(crate) sandbox: SandboxType, } pub(crate) fn prepare_exec_request( @@ -33,6 +34,7 @@ pub(crate) fn prepare_exec_request( cwd: native_path(¶ms.cwd, "cwd")?, env, arg0: params.arg0.clone(), + sandbox: SandboxType::None, }); }; let runtime_paths = runtime_paths @@ -116,6 +118,7 @@ pub(crate) fn prepare_exec_request( cwd: native_path(&request.cwd, "cwd")?, env: request.env, arg0: request.arg0, + sandbox: request.sandbox, }) } diff --git a/codex-rs/exec-server/src/protocol.rs b/codex-rs/exec-server/src/protocol.rs index e05595f27..99ee3431a 100644 --- a/codex-rs/exec-server/src/protocol.rs +++ b/codex-rs/exec-server/src/protocol.rs @@ -153,6 +153,9 @@ pub struct ReadResponse { pub exit_code: Option, pub closed: bool, pub failure: Option, + /// Whether the executor classified the process failure as a sandbox denial. + #[serde(default)] + pub sandbox_denied: bool, } #[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] diff --git a/codex-rs/exec-server/tests/exec_process.rs b/codex-rs/exec-server/tests/exec_process.rs index ac38f50ea..f35923815 100644 --- a/codex-rs/exec-server/tests/exec_process.rs +++ b/codex-rs/exec-server/tests/exec_process.rs @@ -230,7 +230,7 @@ async fn assert_exec_process_streams_output(use_remote: bool) -> Result<()> { .await?; assert_eq!(session.process.process_id().as_str(), process_id); - let StartedExecProcess { process } = session; + let StartedExecProcess { process, .. } = session; let wake_rx = process.subscribe_wake(); let (output, exit_code, closed) = collect_process_output_from_reads(process, wake_rx).await?; assert_eq!(output, "session output\n"); @@ -263,7 +263,7 @@ async fn assert_exec_process_pushes_events(use_remote: bool) -> Result<()> { .await?; assert_eq!(session.process.process_id().as_str(), process_id); - let StartedExecProcess { process } = session; + let StartedExecProcess { process, .. } = session; let actual = collect_process_event_snapshots(process).await?; assert_eq!( actual, @@ -312,7 +312,7 @@ async fn assert_exec_process_replays_events_after_close(use_remote: bool) -> Res .await?; assert_eq!(session.process.process_id().as_str(), process_id); - let StartedExecProcess { process } = session; + let StartedExecProcess { process, .. } = session; let wake_rx = process.subscribe_wake(); let read_result = collect_process_output_from_reads(Arc::clone(&process), wake_rx).await?; assert_eq!( @@ -362,7 +362,7 @@ async fn assert_exec_process_retains_output_after_exit_until_streams_close( .await?; assert_eq!(session.process.process_id().as_str(), process_id); - let StartedExecProcess { process } = session; + let StartedExecProcess { process, .. } = session; let exit_response = timeout( Duration::from_secs(2), @@ -439,7 +439,7 @@ async fn assert_exec_process_write_then_read(use_remote: bool) -> Result<()> { tokio::time::sleep(Duration::from_millis(200)).await; session.process.write(b"hello\n".to_vec()).await?; - let StartedExecProcess { process } = session; + let StartedExecProcess { process, .. } = session; let wake_rx = process.subscribe_wake(); let (output, exit_code, closed) = collect_process_output_from_reads(process, wake_rx).await?; @@ -479,7 +479,7 @@ async fn assert_exec_process_write_then_read_without_tty(use_remote: bool) -> Re tokio::time::sleep(Duration::from_millis(200)).await; let write_response = session.process.write(b"hello\n".to_vec()).await?; assert_eq!(write_response.status, WriteStatus::Accepted); - let StartedExecProcess { process } = session; + let StartedExecProcess { process, .. } = session; let wake_rx = process.subscribe_wake(); let actual = collect_process_output_from_reads(process, wake_rx).await?; @@ -513,7 +513,7 @@ async fn assert_exec_process_rejects_write_without_pipe_stdin(use_remote: bool) let write_response = session.process.write(b"ignored\n".to_vec()).await?; assert_eq!(write_response.status, WriteStatus::StdinClosed); - let StartedExecProcess { process } = session; + let StartedExecProcess { process, .. } = session; let wake_rx = process.subscribe_wake(); let (output, exit_code, closed) = collect_process_output_from_reads(process, wake_rx).await?; @@ -547,7 +547,7 @@ async fn assert_exec_process_signal_interrupts_process(use_remote: bool) -> Resu .await?; assert_eq!(session.process.process_id().as_str(), process_id); - let StartedExecProcess { process } = session; + let StartedExecProcess { process, .. } = session; let mut wake_rx = process.subscribe_wake(); let mut ready_output = String::new(); let mut after_seq = None; @@ -645,7 +645,7 @@ async fn assert_exec_process_preserves_queued_events_before_subscribe( tokio::time::sleep(Duration::from_millis(200)).await; - let StartedExecProcess { process } = session; + let StartedExecProcess { process, .. } = session; let wake_rx = process.subscribe_wake(); let (output, exit_code, closed) = collect_process_output_from_reads(process, wake_rx).await?; assert_eq!(output, "queued output\n"); diff --git a/codex-rs/exec-server/tests/process.rs b/codex-rs/exec-server/tests/process.rs index 571fd427f..ba7f64a61 100644 --- a/codex-rs/exec-server/tests/process.rs +++ b/codex-rs/exec-server/tests/process.rs @@ -70,7 +70,7 @@ async fn exec_server_starts_process_over_websocket() -> anyhow::Result<()> { assert_eq!( process_start_response, ExecResponse { - process_id: ProcessId::from("proc-1") + process_id: ProcessId::from("proc-1"), } ); @@ -135,7 +135,7 @@ async fn exec_server_defaults_omitted_pipe_stdin_to_closed_stdin() -> anyhow::Re assert_eq!( process_start_response, ExecResponse { - process_id: ProcessId::from("proc-default-stdin") + process_id: ProcessId::from("proc-default-stdin"), } ); diff --git a/codex-rs/exec-server/tests/relay.rs b/codex-rs/exec-server/tests/relay.rs index 5f49655e3..c9f873c15 100644 --- a/codex-rs/exec-server/tests/relay.rs +++ b/codex-rs/exec-server/tests/relay.rs @@ -157,7 +157,7 @@ async fn remote_environment_routes_encrypted_exec_server_rpc() -> Result<()> { assert_eq!( response, ExecResponse { - process_id: ProcessId::from("proc-1") + process_id: ProcessId::from("proc-1"), } ); diff --git a/codex-rs/sandboxing/src/denial.rs b/codex-rs/sandboxing/src/denial.rs new file mode 100644 index 000000000..f355fa1c0 --- /dev/null +++ b/codex-rs/sandboxing/src/denial.rs @@ -0,0 +1,57 @@ +use codex_protocol::exec_output::ExecToolCallOutput; + +use crate::SandboxType; + +/// Returns whether a failed command was likely denied by the selected sandbox. +pub fn is_likely_sandbox_denied( + sandbox_type: SandboxType, + exec_output: &ExecToolCallOutput, +) -> bool { + if sandbox_type == SandboxType::None || exec_output.exit_code == 0 { + return false; + } + + const SANDBOX_DENIED_KEYWORDS: [&str; 7] = [ + "operation not permitted", + "permission denied", + "read-only file system", + "seccomp", + "sandbox", + "landlock", + "failed to write file", + ]; + + let has_sandbox_keyword = [ + &exec_output.stderr.text, + &exec_output.stdout.text, + &exec_output.aggregated_output.text, + ] + .into_iter() + .any(|section| { + let lower = section.to_lowercase(); + SANDBOX_DENIED_KEYWORDS + .iter() + .any(|needle| lower.contains(needle)) + }); + + if has_sandbox_keyword { + return true; + } + + const QUICK_REJECT_EXIT_CODES: [i32; 3] = [2, 126, 127]; + if QUICK_REJECT_EXIT_CODES.contains(&exec_output.exit_code) { + return false; + } + + #[cfg(unix)] + { + const EXIT_CODE_SIGNAL_BASE: i32 = 128; + if sandbox_type == SandboxType::LinuxSeccomp + && exec_output.exit_code == EXIT_CODE_SIGNAL_BASE + libc::SIGSYS + { + return true; + } + } + + false +} diff --git a/codex-rs/sandboxing/src/lib.rs b/codex-rs/sandboxing/src/lib.rs index 22c4984c5..0688d0f92 100644 --- a/codex-rs/sandboxing/src/lib.rs +++ b/codex-rs/sandboxing/src/lib.rs @@ -1,5 +1,6 @@ #[cfg(target_os = "linux")] mod bwrap; +mod denial; pub mod landlock; mod manager; pub mod policy_transforms; @@ -11,6 +12,7 @@ mod windows; pub use bwrap::find_system_bwrap_in_path; #[cfg(target_os = "linux")] pub use bwrap::system_bwrap_warning; +pub use denial::is_likely_sandbox_denied; pub use manager::SandboxCommand; pub use manager::SandboxDirectSpawnTransformRequest; pub use manager::SandboxExecRequest;