From f242b49c8adda8f7bb1e5ff225df22295ac8e07b Mon Sep 17 00:00:00 2001 From: ioanbudea-OAI Date: Thu, 12 Nov 2026 21:16:36 +0000 Subject: [PATCH] Remove redundant legacy permission helpers (#61194) ## What changed Remove `PermissionProfile::from_legacy_sandbox_policy` and the filesystem helpers for materializing workspace roots and adding legacy writable roots. Keep legacy template conversion self-contained and update round-trip tests to use the conversion that accepts a working directory. ## Testing Update preset tests to compare converted legacy policies, and move extra-root metadata protection coverage to `FileSystemSandboxPolicy::workspace_write`. GitOrigin-RevId: d545b329c13b2f09609fd2cbf42f0adbbda5386a --- codex-rs/protocol/src/models.rs | 37 +++++++------- codex-rs/protocol/src/permissions.rs | 51 ------------------- .../protocol/src/permissions/target_tests.rs | 3 -- codex-rs/protocol/src/permissions_tests.rs | 47 +++++++++++++---- 4 files changed, 54 insertions(+), 84 deletions(-) diff --git a/codex-rs/protocol/src/models.rs b/codex-rs/protocol/src/models.rs index be6288a1bc..1973c2693f 100644 --- a/codex-rs/protocol/src/models.rs +++ b/codex-rs/protocol/src/models.rs @@ -655,14 +655,6 @@ impl PermissionProfile { } } - pub fn from_legacy_sandbox_policy(sandbox_policy: &SandboxPolicy) -> Self { - Self::from_runtime_permissions_with_enforcement( - SandboxEnforcement::from_legacy_sandbox_policy(sandbox_policy), - &FileSystemSandboxPolicy::from(sandbox_policy), - NetworkSandboxPolicy::from(sandbox_policy), - ) - } - /// Decodes a portable legacy template without probing workspace metadata. pub fn from_legacy_sandbox_policy_template(sandbox_policy: &SandboxPolicy) -> Self { if let SandboxPolicy::WorkspaceWrite { @@ -681,7 +673,11 @@ impl PermissionProfile { NetworkSandboxPolicy::from(sandbox_policy), ) } else { - Self::from_legacy_sandbox_policy(sandbox_policy) + Self::from_runtime_permissions_with_enforcement( + SandboxEnforcement::from_legacy_sandbox_policy(sandbox_policy), + &FileSystemSandboxPolicy::from(sandbox_policy), + NetworkSandboxPolicy::from(sandbox_policy), + ) } } @@ -3138,24 +3134,26 @@ mod tests { } #[test] - fn permission_profile_presets_match_legacy_defaults() { + fn permission_profile_presets_match_legacy_defaults() -> Result<()> { + let cwd = tempdir()?; assert_eq!( - PermissionProfile::read_only(), - PermissionProfile::from_legacy_sandbox_policy(&SandboxPolicy::new_read_only_policy()) + PermissionProfile::read_only().to_legacy_sandbox_policy(cwd.path())?, + SandboxPolicy::new_read_only_policy() ); assert_eq!( - PermissionProfile::workspace_write(), - PermissionProfile::from_legacy_sandbox_policy( - &SandboxPolicy::new_workspace_write_policy() - ) + PermissionProfile::workspace_write().to_legacy_sandbox_policy(cwd.path())?, + SandboxPolicy::new_workspace_write_policy() ); + Ok(()) } #[test] fn permission_profile_round_trip_preserves_disabled_sandbox() -> Result<()> { let cwd = tempdir()?; - let permission_profile = - PermissionProfile::from_legacy_sandbox_policy(&SandboxPolicy::DangerFullAccess); + let permission_profile = PermissionProfile::from_legacy_sandbox_policy_for_cwd( + &SandboxPolicy::DangerFullAccess, + cwd.path(), + ); assert_eq!(permission_profile, PermissionProfile::Disabled); assert_eq!( @@ -3237,7 +3235,8 @@ mod tests { let sandbox_policy = SandboxPolicy::ExternalSandbox { network_access: crate::protocol::NetworkAccess::Restricted, }; - let permission_profile = PermissionProfile::from_legacy_sandbox_policy(&sandbox_policy); + let permission_profile = + PermissionProfile::from_legacy_sandbox_policy_for_cwd(&sandbox_policy, cwd.path()); assert_eq!( permission_profile, diff --git a/codex-rs/protocol/src/permissions.rs b/codex-rs/protocol/src/permissions.rs index 05999db7f7..50650627e9 100644 --- a/codex-rs/protocol/src/permissions.rs +++ b/codex-rs/protocol/src/permissions.rs @@ -1339,19 +1339,6 @@ impl FileSystemSandboxPolicy { Some(self) } - /// Preserves symbolic `:workspace_roots` entries while also adding concrete - /// entries for each provided workspace root. - pub fn with_materialized_project_roots_for_workspace_roots( - self, - workspace_roots: &[AbsolutePathBuf], - ) -> Self { - let roots = workspace_roots - .iter() - .map(PathUri::from_abs_path) - .collect::>(); - self.with_materialized_project_roots_for_path_uris(&roots) - } - pub fn with_additional_readable_roots( mut self, cwd: &Path, @@ -1394,44 +1381,6 @@ impl FileSystemSandboxPolicy { self } - /// Add roots using legacy `WorkspaceWrite` behavior. - /// - /// Unlike [`Self::with_additional_writable_roots`], this mirrors legacy - /// writable-roots semantics by adding exact roots even when they are - /// already writable through `:workspace_roots`, and by adding the default - /// read-only protected subpaths for each new root. - pub fn with_additional_legacy_workspace_writable_roots( - mut self, - additional_writable_roots: &[AbsolutePathBuf], - ) -> Self { - if !matches!(self.kind, FileSystemSandboxKind::Restricted) { - return self; - } - - for path in additional_writable_roots { - if !self.entries.iter().any(|entry| { - entry.access.can_write() - && matches!(&entry.path, FileSystemPath::Path { path: existing } if existing == &PathUri::from_abs_path(path)) - }) { - self.entries.push(FileSystemSandboxEntry::new( - path.clone().into(), - FileSystemAccessMode::Write, - )); - } - - for protected_path in default_read_only_subpaths_for_writable_root( - path, /*protect_missing_dot_codex*/ false, - ) { - append_default_read_only_path_if_no_explicit_rule( - &mut self.entries, - protected_path, - ); - } - } - - self - } - pub fn needs_direct_runtime_enforcement( &self, network_policy: NetworkSandboxPolicy, diff --git a/codex-rs/protocol/src/permissions/target_tests.rs b/codex-rs/protocol/src/permissions/target_tests.rs index 229705f702..01cf15d0fe 100644 --- a/codex-rs/protocol/src/permissions/target_tests.rs +++ b/codex-rs/protocol/src/permissions/target_tests.rs @@ -437,9 +437,6 @@ fn materializing_legacy_home_relative_workspace_denials_removes_all_grants() { &native_root, )), ); - materialized.push(policy.with_materialized_project_roots_for_workspace_roots( - std::slice::from_ref(&native_root), - )); } for policy in materialized { assert_eq!(policy, FileSystemSandboxPolicy::restricted(Vec::new())); diff --git a/codex-rs/protocol/src/permissions_tests.rs b/codex-rs/protocol/src/permissions_tests.rs index 95930032cb..e0e1078e9c 100644 --- a/codex-rs/protocol/src/permissions_tests.rs +++ b/codex-rs/protocol/src/permissions_tests.rs @@ -1748,25 +1748,26 @@ fn materialize_project_roots_with_workspace_roots_expands_exact_and_glob_entries } #[test] -fn with_additional_legacy_workspace_writable_roots_protects_metadata() { +fn workspace_write_with_extra_root_protects_metadata() { let temp_dir = TempDir::new().expect("tempdir"); let extra = AbsolutePathBuf::from_absolute_path(temp_dir.path().join("extra")) .expect("resolve extra root"); std::fs::create_dir_all(extra.join(".git")).expect("create .git dir"); - let policy = FileSystemSandboxPolicy::restricted(vec![FileSystemSandboxEntry { - path: FileSystemPath::Special { - value: FileSystemSpecialPath::project_roots(/*subpath*/ None), - }, - access: FileSystemAccessMode::Write, - missing_path_behavior: None, - }]); - - let actual = - policy.with_additional_legacy_workspace_writable_roots(std::slice::from_ref(&extra)); + let actual = FileSystemSandboxPolicy::workspace_write( + std::slice::from_ref(&extra), + /*exclude_tmpdir_env_var*/ true, + /*exclude_slash_tmp*/ true, + ); assert_eq!( actual, FileSystemSandboxPolicy::restricted(vec![ + FileSystemSandboxEntry::new( + FileSystemPath::Special { + value: FileSystemSpecialPath::Root + }, + FileSystemAccessMode::Read, + ), FileSystemSandboxEntry { path: FileSystemPath::Special { value: FileSystemSpecialPath::project_roots(/*subpath*/ None), @@ -1781,6 +1782,30 @@ fn with_additional_legacy_workspace_writable_roots_protects_metadata() { access: FileSystemAccessMode::Write, missing_path_behavior: None, }, + FileSystemSandboxEntry::skip_missing_path( + FileSystemPath::Special { + value: FileSystemSpecialPath::project_roots(Some(".git".to_string())), + }, + FileSystemAccessMode::Read, + ), + FileSystemSandboxEntry::skip_missing_path( + FileSystemPath::Special { + value: FileSystemSpecialPath::project_roots(Some(".agents".to_string())), + }, + FileSystemAccessMode::Read, + ), + FileSystemSandboxEntry::skip_missing_path( + FileSystemPath::Special { + value: FileSystemSpecialPath::project_roots(Some(".codex".to_string())), + }, + FileSystemAccessMode::Read, + ), + FileSystemSandboxEntry::skip_missing_path( + FileSystemPath::Special { + value: FileSystemSpecialPath::project_roots(Some(".aws".to_string())), + }, + FileSystemAccessMode::Read, + ), FileSystemSandboxEntry::skip_missing_path( FileSystemPath::Path { path: extra.join(".git").into()