codex: simplify apply_patch sandbox FS path

This commit is contained in:
starr-openai
2026-04-06 16:40:53 -07:00
parent 1121fd8490
commit 23d45e5224
5 changed files with 264 additions and 195 deletions
+45 -23
View File
@@ -9,9 +9,11 @@ use codex_apply_patch::ApplyPatchFileChange;
use codex_apply_patch::ApplyPatchFileSystem;
use codex_exec_server::CreateDirectoryOptions;
use codex_exec_server::ExecutorFileSystem;
use codex_exec_server::FileSystemOperationOptions;
use codex_exec_server::RemoveOptions;
use codex_protocol::protocol::FileChange;
use codex_protocol::protocol::FileSystemSandboxPolicy;
use codex_protocol::protocol::SandboxPolicy;
use codex_utils_absolute_path::AbsolutePathBuf;
use std::collections::HashMap;
use std::future::Future;
@@ -28,16 +30,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 tool runtime realizes
/// the verified patch through the environment filesystem.
DelegateToRuntime(ApprovedApplyPatch),
}
#[derive(Debug)]
pub(crate) struct ApplyPatchExec {
pub(crate) struct ApprovedApplyPatch {
pub(crate) action: ApplyPatchAction,
pub(crate) auto_approved: bool,
pub(crate) exec_approval_requirement: ExecApprovalRequirement,
@@ -45,11 +44,27 @@ pub(crate) struct ApplyPatchExec {
pub(crate) struct EnvironmentApplyPatchFileSystem {
file_system: Arc<dyn ExecutorFileSystem>,
operation_options: FileSystemOperationOptions,
}
impl EnvironmentApplyPatchFileSystem {
pub(crate) fn new(file_system: Arc<dyn ExecutorFileSystem>) -> Self {
Self { file_system }
pub(crate) fn for_verification(file_system: Arc<dyn ExecutorFileSystem>) -> Self {
Self {
file_system,
operation_options: FileSystemOperationOptions::default(),
}
}
pub(crate) fn for_apply(
file_system: Arc<dyn ExecutorFileSystem>,
sandbox_policy: SandboxPolicy,
) -> Self {
Self {
file_system,
operation_options: FileSystemOperationOptions {
sandbox_policy: Some(sandbox_policy),
},
}
}
}
@@ -61,9 +76,13 @@ impl ApplyPatchFileSystem for EnvironmentApplyPatchFileSystem {
{
Box::pin(async move {
let path = absolute_path(path)?;
let bytes = self.file_system.read_file(&path).await.map_err(|source| {
ApplyPatchError::io_error(format!("Failed to read {}", path.display()), source)
})?;
let bytes = self
.file_system
.read_file_with_options(&path, &self.operation_options)
.await
.map_err(|source| {
ApplyPatchError::io_error(format!("Failed to read {}", path.display()), source)
})?;
String::from_utf8(bytes).map_err(|source| {
ApplyPatchError::io_error(
format!("Failed to decode UTF-8 for {}", path.display()),
@@ -80,8 +99,9 @@ impl ApplyPatchFileSystem for EnvironmentApplyPatchFileSystem {
) -> Pin<Box<dyn Future<Output = std::result::Result<(), ApplyPatchError>> + Send + 'a>> {
Box::pin(async move {
let path = absolute_path(path)?;
let contents = contents.into_bytes();
self.file_system
.write_file(&path, contents.into_bytes())
.write_file_with_options(&path, contents, &self.operation_options)
.await
.map_err(|source| {
ApplyPatchError::io_error(
@@ -99,7 +119,11 @@ impl ApplyPatchFileSystem for EnvironmentApplyPatchFileSystem {
Box::pin(async move {
let path = absolute_path(path)?;
self.file_system
.create_directory(&path, CreateDirectoryOptions { recursive: true })
.create_directory_with_options(
&path,
CreateDirectoryOptions { recursive: true },
&self.operation_options,
)
.await
.map_err(|source| {
ApplyPatchError::io_error(
@@ -116,14 +140,12 @@ impl ApplyPatchFileSystem for EnvironmentApplyPatchFileSystem {
) -> Pin<Box<dyn Future<Output = std::result::Result<(), ApplyPatchError>> + Send + 'a>> {
Box::pin(async move {
let path = absolute_path(path)?;
let remove_options = RemoveOptions {
recursive: false,
force: false,
};
self.file_system
.remove(
&path,
RemoveOptions {
recursive: false,
force: false,
},
)
.remove_with_options(&path, remove_options, &self.operation_options)
.await
.map_err(|source| {
ApplyPatchError::io_error(
@@ -160,7 +182,7 @@ pub(crate) async fn apply_patch(
SafetyCheck::AutoApprove {
user_explicitly_approved,
..
} => InternalApplyPatchInvocation::DelegateToExec(ApplyPatchExec {
} => InternalApplyPatchInvocation::DelegateToRuntime(ApprovedApplyPatch {
action,
auto_approved: !user_explicitly_approved,
exec_approval_requirement: ExecApprovalRequirement::Skip {
@@ -172,7 +194,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(ApprovedApplyPatch {
action,
auto_approved: false,
exec_approval_requirement: ExecApprovalRequirement::NeedsApproval {
+187
View File
@@ -1,7 +1,15 @@
use super::*;
use async_trait::async_trait;
use codex_exec_server::FileSystemOperationOptions;
use codex_protocol::protocol::SandboxPolicy;
use pretty_assertions::assert_eq;
use std::path::Path;
use std::path::PathBuf;
use std::sync::Arc;
use std::sync::Mutex;
use tempfile::tempdir;
use tokio::io;
#[test]
fn convert_apply_patch_maps_add_variant() {
@@ -19,3 +27,182 @@ fn convert_apply_patch_maps_add_variant() {
})
);
}
#[derive(Default)]
struct RecordingExecutorFileSystem {
raw_reads: Mutex<Vec<PathBuf>>,
option_reads: Mutex<Vec<Option<SandboxPolicy>>>,
raw_writes: Mutex<Vec<PathBuf>>,
option_writes: Mutex<Vec<Option<SandboxPolicy>>>,
raw_creates: Mutex<Vec<PathBuf>>,
option_creates: Mutex<Vec<Option<SandboxPolicy>>>,
raw_removes: Mutex<Vec<PathBuf>>,
option_removes: Mutex<Vec<Option<SandboxPolicy>>>,
}
#[async_trait]
impl ExecutorFileSystem for RecordingExecutorFileSystem {
async fn read_file(&self, path: &AbsolutePathBuf) -> io::Result<Vec<u8>> {
self.raw_reads
.lock()
.expect("raw_reads lock")
.push(path.as_path().to_path_buf());
Ok(b"before\n".to_vec())
}
async fn read_file_with_options(
&self,
path: &AbsolutePathBuf,
options: &FileSystemOperationOptions,
) -> io::Result<Vec<u8>> {
self.option_reads
.lock()
.expect("option_reads lock")
.push(options.sandbox_policy.clone());
self.read_file(path).await
}
async fn write_file(&self, path: &AbsolutePathBuf, _contents: Vec<u8>) -> io::Result<()> {
self.raw_writes
.lock()
.expect("raw_writes lock")
.push(path.as_path().to_path_buf());
Ok(())
}
async fn write_file_with_options(
&self,
path: &AbsolutePathBuf,
contents: Vec<u8>,
options: &FileSystemOperationOptions,
) -> io::Result<()> {
self.option_writes
.lock()
.expect("option_writes lock")
.push(options.sandbox_policy.clone());
self.write_file(path, contents).await
}
async fn create_directory(
&self,
path: &AbsolutePathBuf,
_options: CreateDirectoryOptions,
) -> io::Result<()> {
self.raw_creates
.lock()
.expect("raw_creates lock")
.push(path.as_path().to_path_buf());
Ok(())
}
async fn create_directory_with_options(
&self,
path: &AbsolutePathBuf,
options: CreateDirectoryOptions,
fs_options: &FileSystemOperationOptions,
) -> io::Result<()> {
self.option_creates
.lock()
.expect("option_creates lock")
.push(fs_options.sandbox_policy.clone());
self.create_directory(path, options).await
}
async fn get_metadata(&self, _path: &AbsolutePathBuf) -> io::Result<FileMetadata> {
Err(io::Error::other("unused"))
}
async fn read_directory(&self, _path: &AbsolutePathBuf) -> io::Result<Vec<ReadDirectoryEntry>> {
Err(io::Error::other("unused"))
}
async fn remove(&self, path: &AbsolutePathBuf, _options: RemoveOptions) -> io::Result<()> {
self.raw_removes
.lock()
.expect("raw_removes lock")
.push(path.as_path().to_path_buf());
Ok(())
}
async fn remove_with_options(
&self,
path: &AbsolutePathBuf,
options: RemoveOptions,
fs_options: &FileSystemOperationOptions,
) -> io::Result<()> {
self.option_removes
.lock()
.expect("option_removes lock")
.push(fs_options.sandbox_policy.clone());
self.remove(path, options).await
}
async fn copy(
&self,
_source_path: &AbsolutePathBuf,
_destination_path: &AbsolutePathBuf,
_options: CopyOptions,
) -> io::Result<()> {
Err(io::Error::other("unused"))
}
}
#[tokio::test]
async fn verification_filesystem_uses_default_operation_options() {
let file_system = Arc::new(RecordingExecutorFileSystem::default());
let adapter = EnvironmentApplyPatchFileSystem::for_verification(file_system.clone());
let content = adapter
.read_text(Path::new("/tmp/apply-patch-verification.txt"))
.await
.expect("read through adapter");
assert_eq!(content, "before\n");
assert_eq!(
file_system
.option_reads
.lock()
.expect("option_reads lock")
.as_slice(),
[None]
);
assert_eq!(
file_system
.raw_reads
.lock()
.expect("raw_reads lock")
.as_slice(),
[PathBuf::from("/tmp/apply-patch-verification.txt")]
);
}
#[tokio::test]
async fn apply_filesystem_uses_sandbox_options() {
let file_system = Arc::new(RecordingExecutorFileSystem::default());
let sandbox_policy = SandboxPolicy::new_workspace_write_policy();
let path = Path::new("/tmp/apply-patch-sandboxed/new.txt");
let action = ApplyPatchAction::new_add_for_test(path, "hello".to_string());
let adapter =
EnvironmentApplyPatchFileSystem::for_apply(file_system.clone(), sandbox_policy.clone());
codex_apply_patch::apply_action_with_fs(&action, &adapter)
.await
.expect("apply patch through adapter");
assert_eq!(
file_system
.option_creates
.lock()
.expect("option_creates lock")
.as_slice(),
[Some(sandbox_policy.clone())]
);
assert_eq!(
file_system
.option_writes
.lock()
.expect("option_writes lock")
.as_slice(),
[Some(sandbox_policy)]
);
}
+20 -15
View File
@@ -26,6 +26,7 @@ use codex_apply_patch::ApplyPatchAction;
use codex_apply_patch::ApplyPatchFileChange;
use codex_protocol::models::FileSystemPermissions;
use codex_protocol::models::PermissionProfile;
use codex_sandboxing::policy_transforms::EffectiveSandboxPermissions;
use codex_sandboxing::policy_transforms::effective_file_system_sandbox_policy;
use codex_sandboxing::policy_transforms::merge_permission_profiles;
use codex_sandboxing::policy_transforms::normalize_additional_permissions;
@@ -179,7 +180,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);
@@ -195,12 +196,16 @@ impl ToolHandler for ApplyPatchHandler {
action: apply.action,
file_paths,
changes,
sandbox_policy: EffectiveSandboxPermissions::new(
&turn.sandbox_policy.get(),
effective_additional_permissions
.additional_permissions
.as_ref(),
)
.sandbox_policy,
exec_approval_requirement: apply.exec_approval_requirement,
additional_permissions: effective_additional_permissions
.additional_permissions,
permissions_preapproved: effective_additional_permissions
.permissions_preapproved,
timeout_ms: None,
};
let mut orchestrator = ToolOrchestrator::new();
@@ -256,7 +261,7 @@ impl ToolHandler for ApplyPatchHandler {
pub(crate) async fn intercept_apply_patch(
command: &[String],
cwd: &Path,
timeout_ms: Option<u64>,
_timeout_ms: Option<u64>,
session: Arc<Session>,
turn: Arc<TurnContext>,
tracker: Option<&SharedTurnDiffTracker>,
@@ -282,7 +287,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(
@@ -297,12 +302,16 @@ pub(crate) async fn intercept_apply_patch(
action: apply.action,
file_paths: approval_keys,
changes,
sandbox_policy: EffectiveSandboxPermissions::new(
&turn.sandbox_policy.get(),
effective_additional_permissions
.additional_permissions
.as_ref(),
)
.sandbox_policy,
exec_approval_requirement: apply.exec_approval_requirement,
additional_permissions: effective_additional_permissions
.additional_permissions,
permissions_preapproved: effective_additional_permissions
.permissions_preapproved,
timeout_ms,
};
let mut orchestrator = ToolOrchestrator::new();
@@ -352,12 +361,8 @@ async fn parse_apply_patch_verified(
command: &[String],
cwd: &Path,
) -> codex_apply_patch::MaybeApplyPatchVerified {
if turn.environment.exec_server_url().is_some() {
let fs = EnvironmentApplyPatchFileSystem::new(turn.environment.get_filesystem());
codex_apply_patch::maybe_parse_apply_patch_verified_with_fs(command, cwd, &fs).await
} else {
codex_apply_patch::maybe_parse_apply_patch_verified(command, cwd)
}
let fs = EnvironmentApplyPatchFileSystem::for_verification(turn.environment.get_filesystem());
codex_apply_patch::maybe_parse_apply_patch_verified_with_fs(command, cwd, &fs).await
}
#[cfg(test)]
+10 -85
View File
@@ -1,16 +1,12 @@
//! Apply Patch runtime: executes verified patches under the orchestrator.
//!
//! Assumes `apply_patch` verification/approval happened upstream. Reuses that
//! decision to avoid re-prompting, builds the self-invocation command for
//! `codex --codex-run-as-apply-patch`, and runs under the current
//! `SandboxAttempt` with a minimal environment.
//! decision to avoid re-prompting, then applies the verified action directly
//! through the turn environment's filesystem with the effective sandbox policy.
use crate::apply_patch::EnvironmentApplyPatchFileSystem;
use crate::exec::ExecCapturePolicy;
use crate::guardian::GuardianApprovalRequest;
use crate::guardian::review_approval_request;
use crate::guardian::routes_approval_to_guardian;
use crate::sandboxing::ExecOptions;
use crate::sandboxing::execute_env;
use crate::tools::sandboxing::Approvable;
use crate::tools::sandboxing::ApprovalCtx;
use crate::tools::sandboxing::ExecApprovalRequirement;
@@ -21,18 +17,15 @@ 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_protocol::exec_output::ExecToolCallOutput;
use codex_protocol::exec_output::StreamOutput;
use codex_protocol::models::PermissionProfile;
use codex_protocol::protocol::AskForApproval;
use codex_protocol::protocol::FileChange;
use codex_protocol::protocol::ReviewDecision;
use codex_sandboxing::SandboxCommand;
use codex_protocol::protocol::SandboxPolicy;
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::Duration;
@@ -41,10 +34,9 @@ pub struct ApplyPatchRequest {
pub action: ApplyPatchAction,
pub file_paths: Vec<AbsolutePathBuf>,
pub changes: std::collections::HashMap<PathBuf, FileChange>,
pub sandbox_policy: SandboxPolicy,
pub exec_approval_requirement: ExecApprovalRequirement,
pub additional_permissions: Option<PermissionProfile>,
pub permissions_preapproved: bool,
pub timeout_ms: Option<u64>,
}
#[derive(Default)]
@@ -67,58 +59,6 @@ impl ApplyPatchRuntime {
}
}
#[cfg(target_os = "windows")]
fn build_sandbox_command(
req: &ApplyPatchRequest,
codex_home: &std::path::Path,
) -> Result<SandboxCommand, ToolError> {
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<SandboxCommand, ToolError> {
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<PathBuf, ToolError> {
if let Some(path) = codex_self_exe {
return Ok(path.clone());
}
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(),
additional_permissions: req.additional_permissions.clone(),
}
}
fn stdout_stream(ctx: &ToolCtx) -> Option<crate::exec::StdoutStream> {
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 run_with_environment_fs(
req: &ApplyPatchRequest,
fs: EnvironmentApplyPatchFileSystem,
@@ -234,29 +174,14 @@ impl ToolRuntime<ApplyPatchRequest, ExecToolCallOutput> for ApplyPatchRuntime {
async fn run(
&mut self,
req: &ApplyPatchRequest,
attempt: &SandboxAttempt<'_>,
_attempt: &SandboxAttempt<'_>,
ctx: &ToolCtx,
) -> Result<ExecToolCallOutput, ToolError> {
if ctx.turn.environment.exec_server_url().is_some() {
let fs = EnvironmentApplyPatchFileSystem::new(ctx.turn.environment.get_filesystem());
return Self::run_with_environment_fs(req, fs).await;
}
#[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 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)
let fs = EnvironmentApplyPatchFileSystem::for_apply(
ctx.turn.environment.get_filesystem(),
req.sandbox_policy.clone(),
);
Self::run_with_environment_fs(req, fs).await
}
}
@@ -1,9 +1,8 @@
use super::*;
use codex_protocol::protocol::GranularApprovalConfig;
use codex_protocol::protocol::SandboxPolicy;
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() {
@@ -46,13 +45,12 @@ fn guardian_review_request_includes_patch_context() {
content: "hello".to_string(),
},
)]),
sandbox_policy: SandboxPolicy::DangerFullAccess,
exec_approval_requirement: ExecApprovalRequirement::NeedsApproval {
reason: None,
proposed_execpolicy_amendment: None,
},
additional_permissions: None,
permissions_preapproved: false,
timeout_ms: None,
};
let guardian_request = ApplyPatchRuntime::build_guardian_review_request(&request, "call-1");
@@ -67,71 +65,3 @@ 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() {
let path = std::env::temp_dir().join("apply-patch-current-exe-test.txt");
let action = ApplyPatchAction::new_add_for_test(&path, "hello".to_string());
let request = ApplyPatchRequest {
action,
file_paths: vec![
AbsolutePathBuf::from_absolute_path(&path).expect("temp path should be absolute"),
],
changes: HashMap::from([(
path,
FileChange::Add {
content: "hello".to_string(),
},
)]),
exec_approval_requirement: ExecApprovalRequirement::NeedsApproval {
reason: None,
proposed_execpolicy_amendment: None,
},
additional_permissions: None,
permissions_preapproved: false,
timeout_ms: None,
};
let codex_self_exe = PathBuf::from("/tmp/codex");
let command = ApplyPatchRuntime::build_sandbox_command(&request, Some(&codex_self_exe))
.expect("build sandbox command");
assert_eq!(command.program, codex_self_exe.into_os_string());
}
#[cfg(not(target_os = "windows"))]
#[test]
fn build_sandbox_command_falls_back_to_current_exe_for_apply_patch() {
let path = std::env::temp_dir().join("apply-patch-current-exe-test.txt");
let action = ApplyPatchAction::new_add_for_test(&path, "hello".to_string());
let request = ApplyPatchRequest {
action,
file_paths: vec![
AbsolutePathBuf::from_absolute_path(&path).expect("temp path should be absolute"),
],
changes: HashMap::from([(
path,
FileChange::Add {
content: "hello".to_string(),
},
)]),
exec_approval_requirement: ExecApprovalRequirement::NeedsApproval {
reason: None,
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");
assert_eq!(
command.program,
std::env::current_exe()
.expect("current exe")
.into_os_string()
);
}