[codex] Migrate apply_patch to executor filesystem (#17027)

- Migrate apply-patch verification and application internals to use the
async `ExecutorFileSystem` abstraction from `exec-server`.
- Convert apply-patch `cwd` handling to `AbsolutePathBuf` through the
verifier/parser/handler boundary.

Doesn't change how the tool itself works.
This commit is contained in:
pakrym-oai
2026-04-07 21:20:22 +00:00
committed by GitHub
parent d45513ce5a
commit e9702411ab
22 changed files with 817 additions and 334 deletions
+3 -2
View File
@@ -1,4 +1,5 @@
use super::*;
use core_test_support::PathBufExt;
use pretty_assertions::assert_eq;
use tempfile::tempdir;
@@ -6,14 +7,14 @@ use tempfile::tempdir;
#[test]
fn convert_apply_patch_maps_add_variant() {
let tmp = tempdir().expect("tmp");
let p = tmp.path().join("a.txt");
let p = tmp.path().join("a.txt").abs();
// Create an action with a single Add change
let action = ApplyPatchAction::new_add_for_test(&p, "hello".to_string());
let got = convert_apply_patch_to_protocol(&action);
assert_eq!(
got.get(&p),
got.get(p.as_path()),
Some(&FileChange::Add {
content: "hello".to_string()
})
+19 -12
View File
@@ -5,6 +5,7 @@ use codex_protocol::protocol::FileSystemSandboxEntry;
use codex_protocol::protocol::FileSystemSpecialPath;
use codex_protocol::protocol::GranularApprovalConfig;
use codex_utils_absolute_path::AbsolutePathBuf;
use core_test_support::PathBufExt;
use pretty_assertions::assert_eq;
use tempfile::TempDir;
@@ -17,7 +18,10 @@ fn test_writable_roots_constraint() {
let parent = cwd.parent().unwrap().to_path_buf();
// Helper to build a singleentry patch that adds a file at `p`.
let make_add_change = |p: PathBuf| ApplyPatchAction::new_add_for_test(&p, "".to_string());
let make_add_change = |p: PathBuf| {
let p = p.abs();
ApplyPatchAction::new_add_for_test(&p, "".to_string())
};
let add_inside = make_add_change(cwd.join("inner.txt"));
let add_outside = make_add_change(parent.join("outside.txt"));
@@ -64,7 +68,8 @@ fn test_writable_roots_constraint() {
fn external_sandbox_auto_approves_in_on_request() {
let tmp = TempDir::new().unwrap();
let cwd = tmp.path().to_path_buf();
let add_inside = ApplyPatchAction::new_add_for_test(&cwd.join("inner.txt"), "".to_string());
let add_inside_path = cwd.join("inner.txt").abs();
let add_inside = ApplyPatchAction::new_add_for_test(&add_inside_path, "".to_string());
let policy = SandboxPolicy::ExternalSandbox {
network_access: codex_protocol::protocol::NetworkAccess::Enabled,
@@ -91,8 +96,8 @@ fn granular_with_all_flags_true_matches_on_request_for_out_of_root_patch() {
let tmp = TempDir::new().unwrap();
let cwd = tmp.path().to_path_buf();
let parent = cwd.parent().unwrap().to_path_buf();
let add_outside =
ApplyPatchAction::new_add_for_test(&parent.join("outside.txt"), "".to_string());
let outside_path = parent.join("outside.txt").abs();
let add_outside = ApplyPatchAction::new_add_for_test(&outside_path, "".to_string());
let policy_workspace_only = SandboxPolicy::WorkspaceWrite {
writable_roots: vec![],
read_only_access: Default::default(),
@@ -136,8 +141,8 @@ fn granular_sandbox_approval_false_rejects_out_of_root_patch() {
let tmp = TempDir::new().unwrap();
let cwd = tmp.path().to_path_buf();
let parent = cwd.parent().unwrap().to_path_buf();
let add_outside =
ApplyPatchAction::new_add_for_test(&parent.join("outside.txt"), "".to_string());
let outside_path = parent.join("outside.txt").abs();
let add_outside = ApplyPatchAction::new_add_for_test(&outside_path, "".to_string());
let policy_workspace_only = SandboxPolicy::WorkspaceWrite {
writable_roots: vec![],
read_only_access: Default::default(),
@@ -171,7 +176,8 @@ fn granular_sandbox_approval_false_rejects_out_of_root_patch() {
fn read_only_policy_rejects_patch_with_read_only_reason() {
let tmp = TempDir::new().unwrap();
let cwd = tmp.path().to_path_buf();
let action = ApplyPatchAction::new_add_for_test(&cwd.join("inside.txt"), "".to_string());
let inside_path = cwd.join("inside.txt").abs();
let action = ApplyPatchAction::new_add_for_test(&inside_path, "".to_string());
let sandbox_policy = SandboxPolicy::new_read_only_policy();
let file_system_sandbox_policy =
FileSystemSandboxPolicy::from_legacy_sandbox_policy(&sandbox_policy, &cwd);
@@ -200,8 +206,8 @@ fn explicit_unreadable_paths_prevent_auto_approval_for_external_sandbox() {
let tmp = TempDir::new().unwrap();
let cwd = tmp.path().to_path_buf();
let blocked_path = cwd.join("blocked.txt");
let blocked_absolute = AbsolutePathBuf::from_absolute_path(blocked_path.clone()).unwrap();
let action = ApplyPatchAction::new_add_for_test(&blocked_path, "".to_string());
let blocked_absolute = blocked_path.abs();
let action = ApplyPatchAction::new_add_for_test(&blocked_absolute, "".to_string());
let sandbox_policy = SandboxPolicy::ExternalSandbox {
network_access: codex_protocol::protocol::NetworkAccess::Restricted,
};
@@ -243,8 +249,9 @@ fn explicit_read_only_subpaths_prevent_auto_approval_for_external_sandbox() {
let tmp = TempDir::new().unwrap();
let cwd = tmp.path().to_path_buf();
let blocked_path = cwd.join("docs").join("blocked.txt");
let blocked_absolute = blocked_path.abs();
let docs_absolute = AbsolutePathBuf::resolve_path_against_base("docs", &cwd);
let action = ApplyPatchAction::new_add_for_test(&blocked_path, "".to_string());
let action = ApplyPatchAction::new_add_for_test(&blocked_absolute, "".to_string());
let sandbox_policy = SandboxPolicy::ExternalSandbox {
network_access: codex_protocol::protocol::NetworkAccess::Restricted,
};
@@ -285,8 +292,8 @@ fn explicit_read_only_subpaths_prevent_auto_approval_for_external_sandbox() {
fn missing_project_dot_codex_config_requires_approval() {
let tmp = TempDir::new().unwrap();
let cwd = tmp.path().to_path_buf();
let action =
ApplyPatchAction::new_add_for_test(&cwd.join(".codex").join("config.toml"), "".to_string());
let config_path = cwd.join(".codex").join("config.toml").abs();
let action = ApplyPatchAction::new_add_for_test(&config_path, "".to_string());
let sandbox_policy = SandboxPolicy::WorkspaceWrite {
writable_roots: vec![],
read_only_access: Default::default(),
@@ -23,6 +23,7 @@ use crate::tools::runtimes::apply_patch::ApplyPatchRuntime;
use crate::tools::sandboxing::ToolCtx;
use codex_apply_patch::ApplyPatchAction;
use codex_apply_patch::ApplyPatchFileChange;
use codex_exec_server::ExecutorFileSystem;
use codex_protocol::models::FileSystemPermissions;
use codex_protocol::models::PermissionProfile;
use codex_sandboxing::policy_transforms::effective_file_system_sandbox_policy;
@@ -37,7 +38,7 @@ pub struct ApplyPatchHandler;
fn file_paths_for_action(action: &ApplyPatchAction) -> Vec<AbsolutePathBuf> {
let mut keys = Vec::new();
let cwd = action.cwd.as_path();
let cwd = &action.cwd;
for (path, change) in action.changes() {
if let Some(key) = to_abs_path(cwd, path) {
@@ -55,14 +56,14 @@ fn file_paths_for_action(action: &ApplyPatchAction) -> Vec<AbsolutePathBuf> {
keys
}
fn to_abs_path(cwd: &Path, path: &Path) -> Option<AbsolutePathBuf> {
fn to_abs_path(cwd: &AbsolutePathBuf, path: &Path) -> Option<AbsolutePathBuf> {
Some(AbsolutePathBuf::resolve_path_against_base(path, cwd))
}
fn write_permissions_for_paths(
file_paths: &[AbsolutePathBuf],
file_system_sandbox_policy: &codex_protocol::permissions::FileSystemSandboxPolicy,
cwd: &Path,
cwd: &AbsolutePathBuf,
) -> Option<PermissionProfile> {
let write_paths = file_paths
.iter()
@@ -71,7 +72,9 @@ fn write_permissions_for_paths(
.unwrap_or_else(|| path.clone())
.into_path_buf()
})
.filter(|path| !file_system_sandbox_policy.can_write_path_with_cwd(path.as_path(), cwd))
.filter(|path| {
!file_system_sandbox_policy.can_write_path_with_cwd(path.as_path(), cwd.as_path())
})
.collect::<BTreeSet<_>>()
.into_iter()
.map(AbsolutePathBuf::from_absolute_path)
@@ -110,7 +113,7 @@ async fn effective_patch_permissions(
let effective_additional_permissions = apply_granted_turn_permissions(
session,
crate::sandboxing::SandboxPermissions::UseDefault,
write_permissions_for_paths(&file_paths, &file_system_sandbox_policy, turn.cwd.as_path()),
write_permissions_for_paths(&file_paths, &file_system_sandbox_policy, &turn.cwd),
)
.await;
@@ -167,7 +170,14 @@ impl ToolHandler for ApplyPatchHandler {
// Avoid building temporary ExecParams/command vectors; derive directly from inputs.
let cwd = turn.cwd.clone();
let command = vec!["apply_patch".to_string(), patch_input.clone()];
match codex_apply_patch::maybe_parse_apply_patch_verified(&command, &cwd) {
let Some(environment) = turn.environment.as_ref() else {
return Err(FunctionCallError::RespondToModel(
"apply_patch is unavailable in this session".to_string(),
));
};
let fs = environment.get_filesystem();
match codex_apply_patch::maybe_parse_apply_patch_verified(&command, &cwd, fs.as_ref()).await
{
codex_apply_patch::MaybeApplyPatchVerified::Body(changes) => {
let (file_paths, effective_additional_permissions, file_system_sandbox_policy) =
effective_patch_permissions(session.as_ref(), turn.as_ref(), &changes).await;
@@ -254,7 +264,8 @@ impl ToolHandler for ApplyPatchHandler {
#[allow(clippy::too_many_arguments)]
pub(crate) async fn intercept_apply_patch(
command: &[String],
cwd: &Path,
cwd: &AbsolutePathBuf,
fs: &dyn ExecutorFileSystem,
timeout_ms: Option<u64>,
session: Arc<Session>,
turn: Arc<TurnContext>,
@@ -262,7 +273,7 @@ pub(crate) async fn intercept_apply_patch(
call_id: &str,
tool_name: &str,
) -> Result<Option<FunctionToolOutput>, FunctionCallError> {
match codex_apply_patch::maybe_parse_apply_patch_verified(command, cwd) {
match codex_apply_patch::maybe_parse_apply_patch_verified(command, cwd, fs).await {
codex_apply_patch::MaybeApplyPatchVerified::Body(changes) => {
session
.record_model_warning(
@@ -1,17 +1,21 @@
use super::*;
use codex_apply_patch::MaybeApplyPatchVerified;
use codex_exec_server::LOCAL_FS;
use codex_protocol::permissions::FileSystemSandboxPolicy;
use codex_protocol::protocol::SandboxPolicy;
use core_test_support::PathBufExt;
use core_test_support::PathExt;
use pretty_assertions::assert_eq;
use tempfile::TempDir;
#[test]
fn approval_keys_include_move_destination() {
#[tokio::test]
async fn approval_keys_include_move_destination() {
let tmp = TempDir::new().expect("tmp");
let cwd = tmp.path();
std::fs::create_dir_all(cwd.join("old")).expect("create old dir");
std::fs::create_dir_all(cwd.join("renamed/dir")).expect("create dest dir");
std::fs::write(cwd.join("old/name.txt"), "old content\n").expect("write old file");
let cwd_path = tmp.path();
let cwd = cwd_path.abs();
std::fs::create_dir_all(cwd_path.join("old")).expect("create old dir");
std::fs::create_dir_all(cwd_path.join("renamed/dir")).expect("create dest dir");
std::fs::write(cwd_path.join("old/name.txt"), "old content\n").expect("write old file");
let patch = r#"*** Begin Patch
*** Update File: old/name.txt
*** Move to: renamed/dir/name.txt
@@ -20,10 +24,13 @@ fn approval_keys_include_move_destination() {
+new content
*** End Patch"#;
let argv = vec!["apply_patch".to_string(), patch.to_string()];
let action = match codex_apply_patch::maybe_parse_apply_patch_verified(&argv, cwd) {
MaybeApplyPatchVerified::Body(action) => action,
other => panic!("expected patch body, got: {other:?}"),
};
let action =
match codex_apply_patch::maybe_parse_apply_patch_verified(&argv, &cwd, LOCAL_FS.as_ref())
.await
{
MaybeApplyPatchVerified::Body(action) => action,
other => panic!("expected patch body, got: {other:?}"),
};
let keys = file_paths_for_action(&action);
assert_eq!(keys.len(), 2);
@@ -32,8 +39,9 @@ fn approval_keys_include_move_destination() {
#[test]
fn write_permissions_for_paths_skip_dirs_already_writable_under_workspace_root() {
let tmp = TempDir::new().expect("tmp");
let cwd = tmp.path();
let nested = cwd.join("nested");
let cwd_path = tmp.path();
let cwd = cwd_path.abs();
let nested = cwd_path.join("nested");
std::fs::create_dir_all(&nested).expect("create nested dir");
let file_path = AbsolutePathBuf::try_from(nested.join("file.txt"))
.expect("nested file path should be absolute");
@@ -45,7 +53,7 @@ fn write_permissions_for_paths_skip_dirs_already_writable_under_workspace_root()
exclude_slash_tmp: false,
});
let permissions = write_permissions_for_paths(&[file_path], &sandbox_policy, cwd);
let permissions = write_permissions_for_paths(&[file_path], &sandbox_policy, &cwd);
assert_eq!(permissions, None);
}
@@ -59,6 +67,7 @@ fn write_permissions_for_paths_keep_dirs_outside_workspace_root() {
std::fs::create_dir_all(&outside).expect("create outside dir");
let file_path = AbsolutePathBuf::try_from(outside.join("file.txt"))
.expect("outside file path should be absolute");
let cwd_abs = cwd.abs();
let sandbox_policy = FileSystemSandboxPolicy::from(&SandboxPolicy::WorkspaceWrite {
writable_roots: vec![],
read_only_access: Default::default(),
@@ -67,11 +76,9 @@ fn write_permissions_for_paths_keep_dirs_outside_workspace_root() {
exclude_slash_tmp: true,
});
let permissions = write_permissions_for_paths(&[file_path], &sandbox_policy, &cwd);
let expected_outside = AbsolutePathBuf::from_absolute_path(dunce::simplified(
&outside.canonicalize().expect("canonicalize outside dir"),
))
.expect("outside dir should be absolute");
let permissions = write_permissions_for_paths(&[file_path], &sandbox_policy, &cwd_abs);
let expected_outside =
dunce::simplified(&outside.canonicalize().expect("canonicalize outside dir")).abs();
assert_eq!(
permissions.and_then(|profile| profile.file_system.and_then(|fs| fs.write)),
+16 -1
View File
@@ -39,6 +39,7 @@ use codex_protocol::models::PermissionProfile;
use codex_protocol::protocol::ExecCommandSource;
use codex_shell_command::is_safe_command::is_known_safe_command;
use codex_tools::ShellCommandBackendConfig;
use codex_utils_absolute_path::AbsolutePathBuf;
pub struct ShellHandler;
@@ -395,6 +396,13 @@ impl ShellHandler {
} = args;
let mut exec_params = exec_params;
let Some(environment) = turn.environment.as_ref() else {
return Err(FunctionCallError::RespondToModel(
"shell is unavailable in this session".to_string(),
));
};
let fs = environment.get_filesystem();
let dependency_env = session.dependency_env().await;
if !dependency_env.is_empty() {
exec_params.env.extend(dependency_env.clone());
@@ -458,9 +466,16 @@ impl ShellHandler {
}
// Intercept apply_patch if present.
let apply_patch_cwd =
AbsolutePathBuf::from_absolute_path(&exec_params.cwd).map_err(|err| {
FunctionCallError::RespondToModel(format!(
"apply_patch verification failed: failed to resolve cwd: {err}"
))
})?;
if let Some(output) = intercept_apply_patch(
&exec_params.command,
&exec_params.cwd,
&apply_patch_cwd,
fs.as_ref(),
exec_params.expiration.timeout_ms(),
session.clone(),
turn.clone(),
@@ -30,6 +30,7 @@ use codex_protocol::protocol::EventMsg;
use codex_protocol::protocol::TerminalInteractionEvent;
use codex_shell_command::is_safe_command::is_known_safe_command;
use codex_tools::UnifiedExecShellMode;
use codex_utils_absolute_path::AbsolutePathBuf;
use serde::Deserialize;
use std::path::PathBuf;
use std::sync::Arc;
@@ -176,6 +177,13 @@ impl ToolHandler for UnifiedExecHandler {
}
};
let Some(environment) = turn.environment.as_ref() else {
return Err(FunctionCallError::RespondToModel(
"unified exec is unavailable in this session".to_string(),
));
};
let fs = environment.get_filesystem();
let manager: &UnifiedExecProcessManager = &session.services.unified_exec_manager;
let context = UnifiedExecContext::new(session.clone(), turn.clone(), call_id.clone());
@@ -274,9 +282,19 @@ impl ToolHandler for UnifiedExecHandler {
}
};
let apply_patch_cwd = match AbsolutePathBuf::from_absolute_path(&cwd) {
Ok(cwd) => cwd,
Err(err) => {
manager.release_process_id(process_id).await;
return Err(FunctionCallError::RespondToModel(format!(
"apply_patch verification failed: failed to resolve cwd: {err}"
)));
}
};
if let Some(output) = intercept_apply_patch(
&command,
&cwd,
&apply_patch_cwd,
fs.as_ref(),
Some(yield_time_ms),
context.session.clone(),
context.turn.clone(),
@@ -58,7 +58,7 @@ impl ApplyPatchRuntime {
) -> GuardianApprovalRequest {
GuardianApprovalRequest::ApplyPatch {
id: call_id.to_string(),
cwd: req.action.cwd.clone(),
cwd: req.action.cwd.to_path_buf(),
files: req.file_paths.clone(),
patch: req.action.patch.clone(),
}
@@ -101,7 +101,7 @@ impl ApplyPatchRuntime {
CODEX_CORE_APPLY_PATCH_ARG1.to_string(),
req.action.patch.clone(),
],
cwd: req.action.cwd.clone(),
cwd: req.action.cwd.to_path_buf(),
// Run apply_patch with a minimal environment for determinism and to avoid leaks.
env: HashMap::new(),
additional_permissions: req.additional_permissions.clone(),
@@ -1,5 +1,6 @@
use super::*;
use codex_protocol::protocol::GranularApprovalConfig;
use core_test_support::PathBufExt;
use pretty_assertions::assert_eq;
use std::collections::HashMap;
#[cfg(not(target_os = "windows"))]
@@ -31,17 +32,17 @@ fn wants_no_sandbox_approval_granular_respects_sandbox_flag() {
#[test]
fn guardian_review_request_includes_patch_context() {
let path = std::env::temp_dir().join("guardian-apply-patch-test.txt");
let path = std::env::temp_dir()
.join("guardian-apply-patch-test.txt")
.abs();
let action = ApplyPatchAction::new_add_for_test(&path, "hello".to_string());
let expected_cwd = action.cwd.clone();
let expected_cwd = action.cwd.to_path_buf();
let expected_patch = action.patch.clone();
let request = ApplyPatchRequest {
action,
file_paths: vec![
AbsolutePathBuf::from_absolute_path(&path).expect("temp path should be absolute"),
],
file_paths: vec![path.clone()],
changes: HashMap::from([(
path,
path.to_path_buf(),
FileChange::Add {
content: "hello".to_string(),
},
@@ -71,15 +72,15 @@ 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 path = std::env::temp_dir()
.join("apply-patch-current-exe-test.txt")
.abs();
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"),
],
file_paths: vec![path.clone()],
changes: HashMap::from([(
path,
path.to_path_buf(),
FileChange::Add {
content: "hello".to_string(),
},
@@ -103,15 +104,15 @@ fn build_sandbox_command_prefers_configured_codex_self_exe_for_apply_patch() {
#[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 path = std::env::temp_dir()
.join("apply-patch-current-exe-test.txt")
.abs();
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"),
],
file_paths: vec![path.clone()],
changes: HashMap::from([(
path,
path.to_path_buf(),
FileChange::Add {
content: "hello".to_string(),
},