From c24124b37d3b4f26ccab46d778610efac63d0edb Mon Sep 17 00:00:00 2001 From: starr-openai Date: Tue, 14 Apr 2026 12:49:02 -0700 Subject: [PATCH] Route apply_patch through the environment filesystem (#17674) ## Summary - route apply_patch runtime execution through the selected Environment filesystem instead of the local self-exec path - keep the standalone apply_patch command surface intact while restoring its launcher/test/docs contract - add focused apply_patch filesystem sandbox regression coverage ## Validation - remote devbox Bazel run in progress - passed: //codex-rs/apply-patch:apply-patch-unit-tests --test_filter=test_read_file_utf8_with_context_reports_invalid_utf8 - in progress / follow-up: focused core and exec Bazel test slices on dev ## Follow-up under review - remote pre-verification and approval/retry behavior still need explicit scrutiny for delete/update flows - runtime sandbox-denial classification may need a tighter assertion path than rendered stderr matching --------- Co-authored-by: Codex --- codex-rs/apply-patch/src/lib.rs | 92 ++++++---- codex-rs/arg0/src/lib.rs | 2 +- codex-rs/core/README.md | 4 +- codex-rs/core/src/apply_patch.rs | 15 +- .../core/src/tools/handlers/apply_patch.rs | 7 +- codex-rs/core/src/tools/handlers/shell.rs | 1 - .../core/src/tools/handlers/unified_exec.rs | 1 - .../core/src/tools/runtimes/apply_patch.rs | 173 ++++++++---------- .../src/tools/runtimes/apply_patch_tests.rs | 120 +++++++----- codex-rs/core/tests/suite/shell_snapshot.rs | 5 +- codex-rs/exec-server/tests/file_system.rs | 31 ++++ 11 files changed, 247 insertions(+), 204 deletions(-) diff --git a/codex-rs/apply-patch/src/lib.rs b/codex-rs/apply-patch/src/lib.rs index 8be5ef0f4..34fb4e95c 100644 --- a/codex-rs/apply-patch/src/lib.rs +++ b/codex-rs/apply-patch/src/lib.rs @@ -35,9 +35,9 @@ pub const APPLY_PATCH_TOOL_INSTRUCTIONS: &str = include_str!("../apply_patch_too /// internal `apply_patch` path. /// /// Although this constant lives in `codex-apply-patch` (to avoid forcing -/// `codex-arg0` to depend on `codex-core`), it is part of the "codex core" -/// process-invocation contract between the apply-patch runtime and the arg0 -/// dispatcher. +/// `codex-arg0` to depend on `codex-core`), it remains part of the "codex core" +/// process-invocation contract for the standalone `apply_patch` command +/// surface. pub const CODEX_CORE_APPLY_PATCH_ARG1: &str = "--codex-run-as-apply-patch"; #[derive(Debug, Error, PartialEq)] @@ -134,8 +134,8 @@ pub enum MaybeApplyPatchVerified { pub struct ApplyPatchAction { changes: HashMap, - /// The raw patch argument that can be used with `apply_patch` as an exec - /// call. i.e., if the original arg was parsed in "lenient" mode with a + /// The raw patch argument that can be used to apply the patch. i.e., if the + /// original arg was parsed in "lenient" mode with a /// heredoc, this should be the value without the heredoc wrapper. pub patch: String, @@ -274,23 +274,13 @@ async fn apply_hunks_to_files( let path_abs = hunk.resolve_path(cwd); match hunk { Hunk::AddFile { contents, .. } => { - if let Some(parent_abs) = path_abs.parent() { - fs.create_directory( - &parent_abs, - CreateDirectoryOptions { recursive: true }, - sandbox, - ) - .await - .with_context(|| { - format!( - "Failed to create parent directories for {}", - path_abs.display() - ) - })?; - } - fs.write_file(&path_abs, contents.clone().into_bytes(), sandbox) - .await - .with_context(|| format!("Failed to write file {}", path_abs.display()))?; + write_file_with_missing_parent_retry( + fs, + &path_abs, + contents.clone().into_bytes(), + sandbox, + ) + .await?; added.push(affected_path); } Hunk::DeleteFile { .. } => { @@ -323,23 +313,13 @@ async fn apply_hunks_to_files( derive_new_contents_from_chunks(&path_abs, chunks, fs, sandbox).await?; if let Some(dest) = move_path { let dest_abs = AbsolutePathBuf::resolve_path_against_base(dest, cwd); - if let Some(parent_abs) = dest_abs.parent() { - fs.create_directory( - &parent_abs, - CreateDirectoryOptions { recursive: true }, - sandbox, - ) - .await - .with_context(|| { - format!( - "Failed to create parent directories for {}", - dest_abs.display() - ) - })?; - } - fs.write_file(&dest_abs, new_contents.into_bytes(), sandbox) - .await - .with_context(|| format!("Failed to write file {}", dest_abs.display()))?; + write_file_with_missing_parent_retry( + fs, + &dest_abs, + new_contents.into_bytes(), + sandbox, + ) + .await?; let result: io::Result<()> = async { let metadata = fs.get_metadata(&path_abs, sandbox).await?; if metadata.is_directory { @@ -379,6 +359,40 @@ async fn apply_hunks_to_files( }) } +async fn write_file_with_missing_parent_retry( + fs: &dyn ExecutorFileSystem, + path_abs: &AbsolutePathBuf, + contents: Vec, + sandbox: Option<&FileSystemSandboxContext>, +) -> anyhow::Result<()> { + match fs.write_file(path_abs, contents.clone(), sandbox).await { + Ok(()) => Ok(()), + Err(err) if err.kind() == io::ErrorKind::NotFound => { + if let Some(parent_abs) = path_abs.parent() { + fs.create_directory( + &parent_abs, + CreateDirectoryOptions { recursive: true }, + sandbox, + ) + .await + .with_context(|| { + format!( + "Failed to create parent directories for {}", + path_abs.display() + ) + })?; + } + fs.write_file(path_abs, contents, sandbox) + .await + .with_context(|| format!("Failed to write file {}", path_abs.display()))?; + Ok(()) + } + Err(err) => { + Err(err).with_context(|| format!("Failed to write file {}", path_abs.display())) + } + } +} + struct AppliedPatch { original_contents: String, new_contents: String, diff --git a/codex-rs/arg0/src/lib.rs b/codex-rs/arg0/src/lib.rs index deb18fb99..38f88452a 100644 --- a/codex-rs/arg0/src/lib.rs +++ b/codex-rs/arg0/src/lib.rs @@ -254,7 +254,7 @@ where /// /// - UNIX: `apply_patch` symlink to the current executable /// - WINDOWS: `apply_patch.bat` batch script to invoke the current executable -/// with the "secret" --codex-run-as-apply-patch flag. +/// with the hidden `--codex-run-as-apply-patch` flag. /// /// This temporary directory is prepended to the PATH environment variable so /// that `apply_patch` can be on the PATH without requiring the user to diff --git a/codex-rs/core/README.md b/codex-rs/core/README.md index e1eab3493..2e311790d 100644 --- a/codex-rs/core/README.md +++ b/codex-rs/core/README.md @@ -84,4 +84,6 @@ instead of running with weaker enforcement. ### All Platforms -Expects the binary containing `codex-core` to simulate the virtual `apply_patch` CLI when `arg1` is `--codex-run-as-apply-patch`. See the `codex-arg0` crate for details. +Expects the binary containing `codex-core` to simulate the virtual +`apply_patch` CLI when `arg1` is `--codex-run-as-apply-patch`. See the +`codex-arg0` crate for details. diff --git a/codex-rs/core/src/apply_patch.rs b/codex-rs/core/src/apply_patch.rs index 0810d319b..1bce68a98 100644 --- a/codex-rs/core/src/apply_patch.rs +++ b/codex-rs/core/src/apply_patch.rs @@ -18,16 +18,13 @@ pub(crate) enum InternalApplyPatchInvocation { /// The `apply_patch` call was approved, either automatically because it /// appears that it should be allowed based on the user's sandbox policy - /// *or* because the user explicitly approved it. In either case, we use - /// exec with [`codex_apply_patch::CODEX_CORE_APPLY_PATCH_ARG1`] to realize - /// the `apply_patch` call, - /// but [`ApplyPatchExec::auto_approved`] is used to determine the sandbox - /// used with the `exec()`. - DelegateToExec(ApplyPatchExec), + /// *or* because the user explicitly approved it. The runtime realizes the + /// patch through the selected environment filesystem. + DelegateToRuntime(ApplyPatchRuntimeInvocation), } #[derive(Debug)] -pub(crate) struct ApplyPatchExec { +pub(crate) struct ApplyPatchRuntimeInvocation { pub(crate) action: ApplyPatchAction, pub(crate) auto_approved: bool, pub(crate) exec_approval_requirement: ExecApprovalRequirement, @@ -49,7 +46,7 @@ pub(crate) async fn apply_patch( SafetyCheck::AutoApprove { user_explicitly_approved, .. - } => InternalApplyPatchInvocation::DelegateToExec(ApplyPatchExec { + } => InternalApplyPatchInvocation::DelegateToRuntime(ApplyPatchRuntimeInvocation { action, auto_approved: !user_explicitly_approved, exec_approval_requirement: ExecApprovalRequirement::Skip { @@ -61,7 +58,7 @@ pub(crate) async fn apply_patch( // Delegate the approval prompt (including cached approvals) to the // tool runtime, consistent with how shell/unified_exec approvals // are orchestrator-driven. - InternalApplyPatchInvocation::DelegateToExec(ApplyPatchExec { + InternalApplyPatchInvocation::DelegateToRuntime(ApplyPatchRuntimeInvocation { action, auto_approved: false, exec_approval_requirement: ExecApprovalRequirement::NeedsApproval { diff --git a/codex-rs/core/src/tools/handlers/apply_patch.rs b/codex-rs/core/src/tools/handlers/apply_patch.rs index 3ab1c1454..8e932c3ad 100644 --- a/codex-rs/core/src/tools/handlers/apply_patch.rs +++ b/codex-rs/core/src/tools/handlers/apply_patch.rs @@ -197,7 +197,7 @@ impl ToolHandler for ApplyPatchHandler { let content = item?; Ok(ApplyPatchToolOutput::from_text(content)) } - InternalApplyPatchInvocation::DelegateToExec(apply) => { + InternalApplyPatchInvocation::DelegateToRuntime(apply) => { let changes = convert_apply_patch_to_protocol(&apply.action); let emitter = ToolEmitter::apply_patch(changes.clone(), apply.auto_approved); @@ -218,7 +218,6 @@ impl ToolHandler for ApplyPatchHandler { .additional_permissions, permissions_preapproved: effective_additional_permissions .permissions_preapproved, - timeout_ms: None, }; let mut orchestrator = ToolOrchestrator::new(); @@ -275,7 +274,6 @@ pub(crate) async fn intercept_apply_patch( command: &[String], cwd: &AbsolutePathBuf, fs: &dyn ExecutorFileSystem, - timeout_ms: Option, session: Arc, turn: Arc, tracker: Option<&SharedTurnDiffTracker>, @@ -308,7 +306,7 @@ pub(crate) async fn intercept_apply_patch( let content = item?; Ok(Some(FunctionToolOutput::from_text(content, Some(true)))) } - InternalApplyPatchInvocation::DelegateToExec(apply) => { + InternalApplyPatchInvocation::DelegateToRuntime(apply) => { let changes = convert_apply_patch_to_protocol(&apply.action); let emitter = ToolEmitter::apply_patch(changes.clone(), apply.auto_approved); let event_ctx = ToolEventCtx::new( @@ -328,7 +326,6 @@ pub(crate) async fn intercept_apply_patch( .additional_permissions, permissions_preapproved: effective_additional_permissions .permissions_preapproved, - timeout_ms, }; let mut orchestrator = ToolOrchestrator::new(); diff --git a/codex-rs/core/src/tools/handlers/shell.rs b/codex-rs/core/src/tools/handlers/shell.rs index 3ed21bd2e..aaa23645b 100644 --- a/codex-rs/core/src/tools/handlers/shell.rs +++ b/codex-rs/core/src/tools/handlers/shell.rs @@ -469,7 +469,6 @@ impl ShellHandler { &exec_params.command, &exec_params.cwd, fs.as_ref(), - exec_params.expiration.timeout_ms(), session.clone(), turn.clone(), Some(&tracker), diff --git a/codex-rs/core/src/tools/handlers/unified_exec.rs b/codex-rs/core/src/tools/handlers/unified_exec.rs index a604e9762..99c7e4a19 100644 --- a/codex-rs/core/src/tools/handlers/unified_exec.rs +++ b/codex-rs/core/src/tools/handlers/unified_exec.rs @@ -286,7 +286,6 @@ impl ToolHandler for UnifiedExecHandler { &command, &cwd, fs.as_ref(), - Some(yield_time_ms), context.session.clone(), context.turn.clone(), Some(&tracker), diff --git a/codex-rs/core/src/tools/runtimes/apply_patch.rs b/codex-rs/core/src/tools/runtimes/apply_patch.rs index 8324f365b..1afd21055 100644 --- a/codex-rs/core/src/tools/runtimes/apply_patch.rs +++ b/codex-rs/core/src/tools/runtimes/apply_patch.rs @@ -1,15 +1,11 @@ //! Apply Patch runtime: executes verified patches under the orchestrator. //! -//! Assumes `apply_patch` verification/approval happened upstream. Reuses that -//! decision to avoid re-prompting, applies through the remote filesystem when -//! the turn uses a remote environment, or builds the self-invocation command -//! for `codex --codex-run-as-apply-patch` and runs it under the current -//! `SandboxAttempt` with a minimal environment for local turns. -use crate::exec::ExecCapturePolicy; +//! Assumes `apply_patch` verification/approval happened upstream. Reuses the +//! selected turn environment filesystem for both local and remote turns, with +//! sandboxing enforced by the explicit filesystem sandbox context. +use crate::exec::is_likely_sandbox_denied; use crate::guardian::GuardianApprovalRequest; use crate::guardian::review_approval_request; -use crate::sandboxing::ExecOptions; -use crate::sandboxing::execute_env; use crate::tools::sandboxing::Approvable; use crate::tools::sandboxing::ApprovalCtx; use crate::tools::sandboxing::ExecApprovalRequirement; @@ -20,18 +16,23 @@ use crate::tools::sandboxing::ToolError; use crate::tools::sandboxing::ToolRuntime; use crate::tools::sandboxing::with_cached_approval; use codex_apply_patch::ApplyPatchAction; -use codex_apply_patch::CODEX_CORE_APPLY_PATCH_ARG1; +use codex_exec_server::FileSystemSandboxContext; +use codex_protocol::error::CodexErr; +use codex_protocol::error::SandboxErr; use codex_protocol::exec_output::ExecToolCallOutput; use codex_protocol::exec_output::StreamOutput; use codex_protocol::models::PermissionProfile; use codex_protocol::protocol::AskForApproval; +use codex_protocol::protocol::Event; +use codex_protocol::protocol::EventMsg; +use codex_protocol::protocol::ExecCommandOutputDeltaEvent; +use codex_protocol::protocol::ExecOutputStream; use codex_protocol::protocol::FileChange; use codex_protocol::protocol::ReviewDecision; -use codex_sandboxing::SandboxCommand; +use codex_sandboxing::SandboxType; use codex_sandboxing::SandboxablePreference; use codex_utils_absolute_path::AbsolutePathBuf; use futures::future::BoxFuture; -use std::collections::HashMap; use std::path::PathBuf; use std::time::Instant; @@ -43,7 +44,6 @@ pub struct ApplyPatchRequest { pub exec_approval_requirement: ExecApprovalRequirement, pub additional_permissions: Option, pub permissions_preapproved: bool, - pub timeout_ms: Option, } #[derive(Default)] @@ -66,56 +66,37 @@ impl ApplyPatchRuntime { } } - #[cfg(target_os = "windows")] - fn build_sandbox_command( + fn file_system_sandbox_context_for_attempt( req: &ApplyPatchRequest, - codex_home: &std::path::Path, - ) -> Result { - Ok(Self::build_sandbox_command_with_program( - req, - codex_windows_sandbox::resolve_current_exe_for_launch(codex_home, "codex.exe"), - )) - } - - #[cfg(not(target_os = "windows"))] - fn build_sandbox_command( - req: &ApplyPatchRequest, - codex_self_exe: Option<&PathBuf>, - ) -> Result { - let exe = Self::resolve_apply_patch_program(codex_self_exe)?; - Ok(Self::build_sandbox_command_with_program(req, exe)) - } - - #[cfg(not(target_os = "windows"))] - fn resolve_apply_patch_program(codex_self_exe: Option<&PathBuf>) -> Result { - if let Some(path) = codex_self_exe { - return Ok(path.clone()); + attempt: &SandboxAttempt<'_>, + ) -> Option { + if attempt.sandbox == SandboxType::None { + return None; } - std::env::current_exe() - .map_err(|e| ToolError::Rejected(format!("failed to determine codex exe: {e}"))) - } - - fn build_sandbox_command_with_program(req: &ApplyPatchRequest, exe: PathBuf) -> SandboxCommand { - SandboxCommand { - program: exe.into_os_string(), - args: vec![ - CODEX_CORE_APPLY_PATCH_ARG1.to_string(), - req.action.patch.clone(), - ], - cwd: req.action.cwd.clone(), - // Run apply_patch with a minimal environment for determinism and to avoid leaks. - env: HashMap::new(), + Some(FileSystemSandboxContext { + sandbox_policy: attempt.policy.clone(), + windows_sandbox_level: attempt.windows_sandbox_level, + windows_sandbox_private_desktop: attempt.windows_sandbox_private_desktop, + use_legacy_landlock: attempt.use_legacy_landlock, additional_permissions: req.additional_permissions.clone(), - } + }) } - fn stdout_stream(ctx: &ToolCtx) -> Option { - Some(crate::exec::StdoutStream { - sub_id: ctx.turn.sub_id.clone(), - call_id: ctx.call_id.clone(), - tx_event: ctx.session.get_tx_event(), - }) + async fn emit_output_delta(ctx: &ToolCtx, stream: ExecOutputStream, chunk: &[u8]) { + if chunk.is_empty() { + return; + } + + let event = Event { + id: ctx.turn.sub_id.clone(), + msg: EventMsg::ExecCommandOutputDelta(ExecCommandOutputDeltaEvent { + call_id: ctx.call_id.clone(), + stream, + chunk: chunk.to_vec(), + }), + }; + let _ = ctx.session.get_tx_event().send(event).await; } } @@ -215,51 +196,43 @@ impl ToolRuntime for ApplyPatchRuntime { attempt: &SandboxAttempt<'_>, ctx: &ToolCtx, ) -> Result { - if let Some(environment) = ctx.turn.environment.as_ref().filter(|env| env.is_remote()) { - let started_at = Instant::now(); - let fs = environment.get_filesystem(); - let sandbox = ctx - .turn - .file_system_sandbox_context(req.additional_permissions.clone()); - let mut stdout = Vec::new(); - let mut stderr = Vec::new(); - let result = codex_apply_patch::apply_patch( - &req.action.patch, - &req.action.cwd, - &mut stdout, - &mut stderr, - fs.as_ref(), - Some(&sandbox), - ) - .await; - let stdout = String::from_utf8_lossy(&stdout).into_owned(); - let stderr = String::from_utf8_lossy(&stderr).into_owned(); - let exit_code = if result.is_ok() { 0 } else { 1 }; - return Ok(ExecToolCallOutput { - exit_code, - stdout: StreamOutput::new(stdout.clone()), - stderr: StreamOutput::new(stderr.clone()), - aggregated_output: StreamOutput::new(format!("{stdout}{stderr}")), - duration: started_at.elapsed(), - timed_out: false, - }); - } - - #[cfg(target_os = "windows")] - let command = Self::build_sandbox_command(req, &ctx.turn.config.codex_home)?; - #[cfg(not(target_os = "windows"))] - let command = Self::build_sandbox_command(req, ctx.turn.codex_self_exe.as_ref())?; - let options = ExecOptions { - expiration: req.timeout_ms.into(), - capture_policy: ExecCapturePolicy::ShellTool, + let environment = ctx.turn.environment.as_ref().ok_or_else(|| { + ToolError::Rejected("apply_patch is unavailable in this session".to_string()) + })?; + let started_at = Instant::now(); + let fs = environment.get_filesystem(); + let sandbox = Self::file_system_sandbox_context_for_attempt(req, attempt); + let mut stdout = Vec::new(); + let mut stderr = Vec::new(); + let result = codex_apply_patch::apply_patch( + &req.action.patch, + &req.action.cwd, + &mut stdout, + &mut stderr, + fs.as_ref(), + sandbox.as_ref(), + ) + .await; + let stdout = String::from_utf8_lossy(&stdout).into_owned(); + let stderr = String::from_utf8_lossy(&stderr).into_owned(); + Self::emit_output_delta(ctx, ExecOutputStream::Stdout, stdout.as_bytes()).await; + Self::emit_output_delta(ctx, ExecOutputStream::Stderr, stderr.as_bytes()).await; + let exit_code = if result.is_ok() { 0 } else { 1 }; + let output = ExecToolCallOutput { + exit_code, + stdout: StreamOutput::new(stdout.clone()), + stderr: StreamOutput::new(stderr.clone()), + aggregated_output: StreamOutput::new(format!("{stdout}{stderr}")), + duration: started_at.elapsed(), + timed_out: false, }; - let env = attempt - .env_for(command, options, /*network*/ None) - .map_err(|err| ToolError::Codex(err.into()))?; - let out = execute_env(env, Self::stdout_stream(ctx)) - .await - .map_err(ToolError::Codex)?; - Ok(out) + if result.is_err() && is_likely_sandbox_denied(attempt.sandbox, &output) { + return Err(ToolError::Codex(CodexErr::Sandbox(SandboxErr::Denied { + output: Box::new(output), + network_policy_decision: None, + }))); + } + Ok(output) } } diff --git a/codex-rs/core/src/tools/runtimes/apply_patch_tests.rs b/codex-rs/core/src/tools/runtimes/apply_patch_tests.rs index e5831414c..740254cb8 100644 --- a/codex-rs/core/src/tools/runtimes/apply_patch_tests.rs +++ b/codex-rs/core/src/tools/runtimes/apply_patch_tests.rs @@ -1,10 +1,17 @@ use super::*; +use crate::tools::sandboxing::SandboxAttempt; +use codex_protocol::config_types::WindowsSandboxLevel; +use codex_protocol::models::FileSystemPermissions; +use codex_protocol::models::PermissionProfile; +use codex_protocol::permissions::FileSystemSandboxPolicy; +use codex_protocol::permissions::NetworkSandboxPolicy; use codex_protocol::protocol::GranularApprovalConfig; +use codex_protocol::protocol::SandboxPolicy; +use codex_sandboxing::SandboxManager; +use codex_sandboxing::SandboxType; use core_test_support::PathBufExt; use pretty_assertions::assert_eq; use std::collections::HashMap; -#[cfg(not(target_os = "windows"))] -use std::path::PathBuf; #[test] fn wants_no_sandbox_approval_granular_respects_sandbox_flag() { @@ -53,7 +60,6 @@ fn guardian_review_request_includes_patch_context() { }, additional_permissions: None, permissions_preapproved: false, - timeout_ms: None, }; let guardian_request = ApplyPatchRuntime::build_guardian_review_request(&request, "call-1"); @@ -69,70 +75,94 @@ fn guardian_review_request_includes_patch_context() { ); } -#[cfg(not(target_os = "windows"))] #[test] -fn build_sandbox_command_prefers_configured_codex_self_exe_for_apply_patch() { +fn file_system_sandbox_context_uses_active_attempt() { let path = std::env::temp_dir() - .join("apply-patch-current-exe-test.txt") + .join("apply-patch-runtime-attempt.txt") .abs(); - let action = ApplyPatchAction::new_add_for_test(&path, "hello".to_string()); - let request = ApplyPatchRequest { - action, + let additional_permissions = PermissionProfile { + network: None, + file_system: Some(FileSystemPermissions { + read: Some(vec![path.clone()]), + write: Some(Vec::new()), + }), + }; + let req = ApplyPatchRequest { + action: ApplyPatchAction::new_add_for_test(&path, "hello".to_string()), file_paths: vec![path.clone()], - changes: HashMap::from([( - path.to_path_buf(), - FileChange::Add { - content: "hello".to_string(), - }, - )]), - exec_approval_requirement: ExecApprovalRequirement::NeedsApproval { - reason: None, + changes: HashMap::new(), + exec_approval_requirement: ExecApprovalRequirement::Skip { + bypass_sandbox: false, proposed_execpolicy_amendment: None, }, - additional_permissions: None, + additional_permissions: Some(additional_permissions.clone()), permissions_preapproved: false, - timeout_ms: None, }; - let codex_self_exe = PathBuf::from("/tmp/codex"); + let sandbox_policy = SandboxPolicy::new_read_only_policy(); + let file_system_policy = FileSystemSandboxPolicy::from(&sandbox_policy); + let manager = SandboxManager::new(); + let attempt = SandboxAttempt { + sandbox: SandboxType::MacosSeatbelt, + policy: &sandbox_policy, + file_system_policy: &file_system_policy, + network_policy: NetworkSandboxPolicy::Restricted, + enforce_managed_network: false, + manager: &manager, + sandbox_cwd: path.as_path(), + codex_linux_sandbox_exe: None, + use_legacy_landlock: true, + windows_sandbox_level: WindowsSandboxLevel::RestrictedToken, + windows_sandbox_private_desktop: true, + }; - let command = ApplyPatchRuntime::build_sandbox_command(&request, Some(&codex_self_exe)) - .expect("build sandbox command"); + let sandbox = ApplyPatchRuntime::file_system_sandbox_context_for_attempt(&req, &attempt) + .expect("sandbox context"); - assert_eq!(command.program, codex_self_exe.into_os_string()); + assert_eq!(sandbox.sandbox_policy, sandbox_policy); + assert_eq!(sandbox.additional_permissions, Some(additional_permissions)); + assert_eq!( + sandbox.windows_sandbox_level, + WindowsSandboxLevel::RestrictedToken + ); + assert_eq!(sandbox.windows_sandbox_private_desktop, true); + assert_eq!(sandbox.use_legacy_landlock, true); } -#[cfg(not(target_os = "windows"))] #[test] -fn build_sandbox_command_falls_back_to_current_exe_for_apply_patch() { +fn no_sandbox_attempt_has_no_file_system_context() { let path = std::env::temp_dir() - .join("apply-patch-current-exe-test.txt") + .join("apply-patch-runtime-none.txt") .abs(); - let action = ApplyPatchAction::new_add_for_test(&path, "hello".to_string()); - let request = ApplyPatchRequest { - action, + let req = ApplyPatchRequest { + action: ApplyPatchAction::new_add_for_test(&path, "hello".to_string()), file_paths: vec![path.clone()], - changes: HashMap::from([( - path.to_path_buf(), - FileChange::Add { - content: "hello".to_string(), - }, - )]), - exec_approval_requirement: ExecApprovalRequirement::NeedsApproval { - reason: None, + changes: HashMap::new(), + exec_approval_requirement: ExecApprovalRequirement::Skip { + bypass_sandbox: false, proposed_execpolicy_amendment: None, }, additional_permissions: None, permissions_preapproved: false, - timeout_ms: None, }; - - let command = ApplyPatchRuntime::build_sandbox_command(&request, /*codex_self_exe*/ None) - .expect("build sandbox command"); + let sandbox_policy = SandboxPolicy::DangerFullAccess; + let file_system_policy = FileSystemSandboxPolicy::from(&sandbox_policy); + let manager = SandboxManager::new(); + let attempt = SandboxAttempt { + sandbox: SandboxType::None, + policy: &sandbox_policy, + file_system_policy: &file_system_policy, + network_policy: NetworkSandboxPolicy::Enabled, + enforce_managed_network: false, + manager: &manager, + sandbox_cwd: path.as_path(), + codex_linux_sandbox_exe: None, + use_legacy_landlock: false, + windows_sandbox_level: WindowsSandboxLevel::Disabled, + windows_sandbox_private_desktop: false, + }; assert_eq!( - command.program, - std::env::current_exe() - .expect("current exe") - .into_os_string() + ApplyPatchRuntime::file_system_sandbox_context_for_attempt(&req, &attempt), + None ); } diff --git a/codex-rs/core/tests/suite/shell_snapshot.rs b/codex-rs/core/tests/suite/shell_snapshot.rs index 0e044f392..516bc049f 100644 --- a/codex-rs/core/tests/suite/shell_snapshot.rs +++ b/codex-rs/core/tests/suite/shell_snapshot.rs @@ -531,8 +531,9 @@ async fn shell_command_snapshot_still_intercepts_apply_patch() -> Result<()> { let script = "apply_patch <<'EOF'\n*** Begin Patch\n*** Add File: snapshot-apply.txt\n+hello from snapshot\n*** End Patch\nEOF\n"; let args = json!({ "command": script, - // The intercepted apply_patch path self-invokes codex, which can take - // longer than a second in Bazel macOS test environments. + // Keep this above the default because intercepted apply_patch still + // performs filesystem work that can be slow in Bazel macOS test + // environments. "timeout_ms": 5_000, }); let call_id = "shell-snapshot-apply-patch"; diff --git a/codex-rs/exec-server/tests/file_system.rs b/codex-rs/exec-server/tests/file_system.rs index 6b4a05866..e24a2605f 100644 --- a/codex-rs/exec-server/tests/file_system.rs +++ b/codex-rs/exec-server/tests/file_system.rs @@ -290,6 +290,37 @@ async fn file_system_methods_cover_surface_area(use_remote: bool) -> Result<()> Ok(()) } +#[test_case(false ; "local")] +#[test_case(true ; "remote")] +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn file_system_write_file_reports_missing_parent(use_remote: bool) -> Result<()> { + let context = create_file_system_context(use_remote).await?; + let file_system = context.file_system; + + let tmp = TempDir::new()?; + let missing_parent_path = tmp.path().join("missing").join("note.txt"); + + let error = match file_system + .write_file( + &absolute_path(missing_parent_path.clone()), + b"hello from trait".to_vec(), + /*sandbox*/ None, + ) + .await + { + Ok(()) => anyhow::bail!("write should fail when parent directory is absent"), + Err(error) => error, + }; + assert_eq!( + error.kind(), + std::io::ErrorKind::NotFound, + "mode={use_remote}" + ); + assert!(!missing_parent_path.exists(), "mode={use_remote}"); + + Ok(()) +} + #[test_case(false ; "local")] #[test_case(true ; "remote")] #[tokio::test(flavor = "multi_thread", worker_threads = 2)]