don't include redundant write roots in apply_patch (#16030)

apply_patch sometimes provides additional parent dir as a writable root
when it is already writable. This is mostly a no-op on Mac/Linux but
causes actual ACL churn on Windows that is best avoided. We are also
seeing some actual failures with these ACLs in the wild, which I haven't
fully tracked down, but it's safe/best to avoid doing it altogether.

codex-rs/core/src/tools/handlers/apply_patch.rs

@@ -68,7 +68,11 @@ fn to_abs_path(cwd: &Path, path: &Path) -> Option<AbsolutePathBuf> {
     AbsolutePathBuf::resolve_path_against_base(path, cwd).ok()
 }
 
-fn write_permissions_for_paths(file_paths: &[AbsolutePathBuf]) -> Option<PermissionProfile> {
+fn write_permissions_for_paths(
+    file_paths: &[AbsolutePathBuf],
+    file_system_sandbox_policy: &codex_protocol::permissions::FileSystemSandboxPolicy,
+    cwd: &Path,
+) -> Option<PermissionProfile> {
     let write_paths = file_paths
         .iter()
         .map(|path| {
@@ -76,6 +80,7 @@ fn write_permissions_for_paths(file_paths: &[AbsolutePathBuf]) -> Option<Permiss
                 .unwrap_or_else(|| path.clone())
                 .into_path_buf()
         })
+        .filter(|path| !file_system_sandbox_policy.can_write_path_with_cwd(path.as_path(), cwd))
         .collect::<BTreeSet<_>>()
         .into_iter()
         .map(AbsolutePathBuf::from_absolute_path)
@@ -107,16 +112,16 @@ async fn effective_patch_permissions(
         session.granted_session_permissions().await.as_ref(),
         session.granted_turn_permissions().await.as_ref(),
     );
-    let effective_additional_permissions = apply_granted_turn_permissions(
-        session,
-        crate::sandboxing::SandboxPermissions::UseDefault,
-        write_permissions_for_paths(&file_paths),
-    )
-    .await;
     let file_system_sandbox_policy = effective_file_system_sandbox_policy(
         &turn.file_system_sandbox_policy,
         granted_permissions.as_ref(),
     );
+    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()),
+    )
+    .await;
 
     (
         file_paths,

codex-rs/core/src/tools/handlers/apply_patch_tests.rs

@@ -1,5 +1,7 @@
 use super::*;
 use codex_apply_patch::MaybeApplyPatchVerified;
+use codex_protocol::permissions::FileSystemSandboxPolicy;
+use codex_protocol::protocol::SandboxPolicy;
 use pretty_assertions::assert_eq;
 use tempfile::TempDir;
 
@@ -26,3 +28,53 @@ fn approval_keys_include_move_destination() {
     let keys = file_paths_for_action(&action);
     assert_eq!(keys.len(), 2);
 }
+
+#[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");
+    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");
+    let sandbox_policy = FileSystemSandboxPolicy::from(&SandboxPolicy::WorkspaceWrite {
+        writable_roots: vec![],
+        read_only_access: Default::default(),
+        network_access: false,
+        exclude_tmpdir_env_var: true,
+        exclude_slash_tmp: false,
+    });
+
+    let permissions = write_permissions_for_paths(&[file_path], &sandbox_policy, cwd);
+
+    assert_eq!(permissions, None);
+}
+
+#[test]
+fn write_permissions_for_paths_keep_dirs_outside_workspace_root() {
+    let tmp = TempDir::new().expect("tmp");
+    let cwd = tmp.path().join("workspace");
+    let outside = tmp.path().join("outside");
+    std::fs::create_dir_all(&cwd).expect("create cwd");
+    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 sandbox_policy = FileSystemSandboxPolicy::from(&SandboxPolicy::WorkspaceWrite {
+        writable_roots: vec![],
+        read_only_access: Default::default(),
+        network_access: false,
+        exclude_tmpdir_env_var: true,
+        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");
+
+    assert_eq!(
+        permissions.and_then(|profile| profile.file_system.and_then(|fs| fs.write)),
+        Some(vec![expected_outside])
+    );
+}