diff --git a/codex-rs/app-server/src/permission_presets.rs b/codex-rs/app-server/src/permission_presets.rs index 359dcc3797..f9f61581a8 100644 --- a/codex-rs/app-server/src/permission_presets.rs +++ b/codex-rs/app-server/src/permission_presets.rs @@ -26,16 +26,26 @@ pub(crate) async fn permission_presets( .load_for_cwd(None, ConfigOverrides::default(), cwd) .await?; let selectable_profiles = config - .selectable_permission_profiles() - .await? + .selectable_permission_profiles()? .into_iter() .map(|profile| (profile.id.clone(), profile)) .collect::>(); + let active_profile_id = config + .permissions + .active_permission_profile() + .map(|profile| profile.id); let mut candidates = Vec::new(); for preset in builtin_approval_presets() { - let Some(profile) = selectable_profiles.get(&preset.active_permission_profile.id) else { + if !selectable_profiles.contains_key(&preset.active_permission_profile.id) { continue; + } + let profile_is_default = match active_profile_id.as_deref() { + Some(id) => id == preset.active_permission_profile.id, + None => preset.matches_permission_profile( + config.permissions.permission_profile(), + config.cwd.as_path(), + ), }; candidates.push(PermissionPresetCandidate { id: preset.id.to_string(), @@ -43,7 +53,7 @@ pub(crate) async fn permission_presets( description: None, approval_policy: preset.approval, approvals_reviewer: ApprovalsReviewer::User, - profile_is_default: profile.is_default, + profile_is_default, }); if preset.id == "auto" { candidates.push(PermissionPresetCandidate { @@ -58,7 +68,7 @@ pub(crate) async fn permission_presets( mcp_elicitations: true, }), approvals_reviewer: ApprovalsReviewer::User, - profile_is_default: profile.is_default, + profile_is_default, }); if config.features.enabled(Feature::GuardianApproval) { candidates.push(PermissionPresetCandidate { @@ -67,7 +77,7 @@ pub(crate) async fn permission_presets( description: None, approval_policy: AskForApproval::OnRequest, approvals_reviewer: ApprovalsReviewer::AutoReview, - profile_is_default: profile.is_default, + profile_is_default, }); } } @@ -78,18 +88,17 @@ pub(crate) async fn permission_presets( .filter(|profile| !profile.id.starts_with(':')) .collect::>(); custom_profiles.sort_by(|left, right| left.id.cmp(&right.id)); - candidates.extend( - custom_profiles - .into_iter() - .map(|profile| PermissionPresetCandidate { - id: format!("permission-profile:{}", profile.id), - permission_profile_id: profile.id, - description: profile.description, - approval_policy: config.permissions.approval_policy.value(), - approvals_reviewer: config.approvals_reviewer, - profile_is_default: profile.is_default, - }), - ); + candidates.extend(custom_profiles.into_iter().map(|profile| { + let profile_is_default = active_profile_id.as_deref() == Some(profile.id.as_str()); + PermissionPresetCandidate { + id: format!("permission-profile:{}", profile.id), + permission_profile_id: profile.id, + description: profile.description, + approval_policy: config.permissions.approval_policy.value(), + approvals_reviewer: config.approvals_reviewer, + profile_is_default, + } + })); let requirements = config.config_layer_stack.requirements(); let mut presets = Vec::new(); diff --git a/codex-rs/app-server/tests/suite/v2/permission_preset_list.rs b/codex-rs/app-server/tests/suite/v2/permission_preset_list.rs index faad8dc3ff..3f4c7c6e69 100644 --- a/codex-rs/app-server/tests/suite/v2/permission_preset_list.rs +++ b/codex-rs/app-server/tests/suite/v2/permission_preset_list.rs @@ -172,128 +172,82 @@ description = "Inspect without writes." } #[tokio::test] -async fn permission_preset_list_matches_legacy_read_only_as_default() -> Result<()> { - let codex_home = TempDir::new()?; - std::fs::write( - codex_home.path().join("config.toml"), - r#" +async fn permission_preset_list_matches_legacy_defaults() -> Result<()> { + for (config, requirements, expected) in [ + ( + r#" sandbox_mode = "read-only" approval_policy = "on-request" "#, - )?; - - let mut mcp = TestAppServer::new(codex_home.path()).await?; - timeout(DEFAULT_TIMEOUT, mcp.initialize()).await??; - - let request_id = mcp - .send_permission_preset_list_request(PermissionPresetListParams { - cursor: None, - limit: None, - cwd: None, - }) - .await?; - let actual = read_response::(&mut mcp, request_id).await?; - - assert_eq!( - actual - .data - .into_iter() - .filter(|preset| preset.is_default) - .collect::>(), - vec![preset( - "read-only", - BUILT_IN_PERMISSION_PROFILE_READ_ONLY, - AskForApproval::OnRequest, - ApprovalsReviewer::User, - true, - )] - ); - Ok(()) -} - -#[tokio::test] -async fn permission_preset_list_marks_managed_legacy_fallback_as_default() -> Result<()> { - let codex_home = TempDir::new()?; - std::fs::write( - codex_home.path().join("config.toml"), - r#" + None, + preset( + "read-only", + BUILT_IN_PERMISSION_PROFILE_READ_ONLY, + AskForApproval::OnRequest, + ApprovalsReviewer::User, + true, + ), + ), + ( + r#" sandbox_mode = "workspace-write" approval_policy = "on-request" "#, - )?; - std::fs::write( - codex_home.path().join("requirements.toml"), - r#" + Some( + r#" allowed_sandbox_modes = ["read-only"] "#, - )?; - - let mut mcp = TestAppServer::new(codex_home.path()).await?; - timeout(DEFAULT_TIMEOUT, mcp.initialize()).await??; - - let request_id = mcp - .send_permission_preset_list_request(PermissionPresetListParams { - cursor: None, - limit: None, - cwd: None, - }) - .await?; - let actual = read_response::(&mut mcp, request_id).await?; - - assert_eq!( - actual - .data - .into_iter() - .filter(|preset| preset.is_default) - .collect::>(), - vec![preset( - "read-only", - BUILT_IN_PERMISSION_PROFILE_READ_ONLY, - AskForApproval::OnRequest, - ApprovalsReviewer::User, - true, - )] - ); - Ok(()) -} - -#[tokio::test] -async fn permission_preset_list_matches_legacy_full_access_as_default() -> Result<()> { - let codex_home = TempDir::new()?; - std::fs::write( - codex_home.path().join("config.toml"), - r#" + ), + preset( + "read-only", + BUILT_IN_PERMISSION_PROFILE_READ_ONLY, + AskForApproval::OnRequest, + ApprovalsReviewer::User, + true, + ), + ), + ( + r#" sandbox_mode = "danger-full-access" approval_policy = "never" "#, - )?; + None, + preset( + "full-access", + BUILT_IN_PERMISSION_PROFILE_DANGER_FULL_ACCESS, + AskForApproval::Never, + ApprovalsReviewer::User, + true, + ), + ), + ] { + let codex_home = TempDir::new()?; + std::fs::write(codex_home.path().join("config.toml"), config)?; + if let Some(requirements) = requirements { + std::fs::write(codex_home.path().join("requirements.toml"), requirements)?; + } - let mut mcp = TestAppServer::new(codex_home.path()).await?; - timeout(DEFAULT_TIMEOUT, mcp.initialize()).await??; + let mut mcp = TestAppServer::new(codex_home.path()).await?; + timeout(DEFAULT_TIMEOUT, mcp.initialize()).await??; - let request_id = mcp - .send_permission_preset_list_request(PermissionPresetListParams { - cursor: None, - limit: None, - cwd: None, - }) - .await?; - let actual = read_response::(&mut mcp, request_id).await?; + let request_id = mcp + .send_permission_preset_list_request(PermissionPresetListParams { + cursor: None, + limit: None, + cwd: None, + }) + .await?; + let actual = read_response::(&mut mcp, request_id).await?; - assert_eq!( - actual - .data - .into_iter() - .filter(|preset| preset.is_default) - .collect::>(), - vec![preset( - "full-access", - BUILT_IN_PERMISSION_PROFILE_DANGER_FULL_ACCESS, - AskForApproval::Never, - ApprovalsReviewer::User, - true, - )] - ); + assert_eq!( + actual + .data + .into_iter() + .filter(|preset| preset.is_default) + .collect::>(), + vec![expected] + ); + } Ok(()) } diff --git a/codex-rs/core/src/config/config_tests.rs b/codex-rs/core/src/config/config_tests.rs index 9b4b6e93f8..f2fd9428b0 100644 --- a/codex-rs/core/src/config/config_tests.rs +++ b/codex-rs/core/src/config/config_tests.rs @@ -2417,7 +2417,6 @@ async fn default_permissions_can_select_builtin_profile_without_permissions_tabl .await?; assert!(config.explicit_permission_profile_mode); - assert!(config.custom_permission_profiles.is_empty()); let policy = config.permissions.file_system_sandbox_policy(); assert_eq!( config diff --git a/codex-rs/core/src/config/mod.rs b/codex-rs/core/src/config/mod.rs index c7394cbb3b..585f6882ff 100644 --- a/codex-rs/core/src/config/mod.rs +++ b/codex-rs/core/src/config/mod.rs @@ -3773,7 +3773,6 @@ impl Config { windows_sandbox_private_desktop, }, explicit_permission_profile_mode, - custom_permission_profiles, approvals_reviewer: constrained_approvals_reviewer.value(), enforce_residency: enforce_residency.value, notify: cfg.notify, diff --git a/codex-rs/core/src/config/selectable_permission_profiles.rs b/codex-rs/core/src/config/selectable_permission_profiles.rs index d14b811a88..04f103d6fe 100644 --- a/codex-rs/core/src/config/selectable_permission_profiles.rs +++ b/codex-rs/core/src/config/selectable_permission_profiles.rs @@ -5,9 +5,7 @@ use super::permissions::BUILT_IN_READ_ONLY_PROFILE; use super::permissions::BUILT_IN_WORKSPACE_PROFILE; use super::permissions::builtin_permission_profile; use super::permissions::compile_permission_profile_selection; -use crate::windows_sandbox::WindowsSandboxLevelExt; use codex_config::config_toml::ConfigToml; -use codex_protocol::config_types::WindowsSandboxLevel; use codex_protocol::models::PermissionProfile; use std::io::ErrorKind; @@ -15,12 +13,11 @@ use std::io::ErrorKind; pub struct SelectablePermissionProfile { pub id: String, pub description: Option, - pub is_default: bool, } impl Config { - /// Returns named permission profiles that can be selected under the current requirements. - pub async fn selectable_permission_profiles( + /// Returns built-in and configured permission profiles allowed by current requirements. + pub fn selectable_permission_profiles( &self, ) -> std::io::Result> { let config_toml: ConfigToml = self @@ -31,26 +28,6 @@ impl Config { let requirements_toml = self.config_layer_stack.requirements_toml(); let permissions = merge_managed_permission_profiles(config_toml.permissions.as_ref(), requirements_toml)?; - let active_profile_id = self - .permissions - .active_permission_profile() - .map(|profile| profile.id); - let requirements = self.config_layer_stack.requirements(); - let default_permission_profile = if active_profile_id.is_none() { - let mut permission_profile = requirements.permission_profile.clone(); - let configured_profile = config_toml - .derive_permission_profile( - /*sandbox_mode_override*/ None, - WindowsSandboxLevel::from_config(self), - Some(&self.active_project), - Some(&requirements.permission_profile), - ) - .await; - let _ = permission_profile.set(configured_profile); - Some(permission_profile.get().clone()) - } else { - None - }; let mut profiles = Vec::new(); for id in [ @@ -62,8 +39,6 @@ impl Config { permissions.as_ref(), id, /*description*/ None, - active_profile_id.as_deref(), - default_permission_profile.as_ref(), ) { profiles.push(profile); } @@ -74,8 +49,6 @@ impl Config { Some(permissions), id, profile.description.clone(), - active_profile_id.as_deref(), - default_permission_profile.as_ref(), ) { profiles.push(profile); } @@ -90,8 +63,6 @@ impl Config { permissions: Option<&codex_config::permissions_toml::PermissionsToml>, id: &str, description: Option, - active_profile_id: Option<&str>, - default_permission_profile: Option<&PermissionProfile>, ) -> Option { if self .config_layer_stack @@ -120,21 +91,16 @@ impl Config { self.permissions .can_set_permission_profile(&permission_profile) .ok()?; - let mut constrained_permission_profile = self - .config_layer_stack + self.config_layer_stack .requirements() .permission_profile - .clone(); - constrained_permission_profile + .clone() .set(permission_profile) .ok()?; Some(SelectablePermissionProfile { id: id.to_string(), description, - is_default: active_profile_id == Some(id) - || active_profile_id.is_none() - && default_permission_profile == Some(constrained_permission_profile.get()), }) } } diff --git a/codex-rs/thread-manager-sample/src/main.rs b/codex-rs/thread-manager-sample/src/main.rs index 3d8561a176..487b8c1519 100644 --- a/codex-rs/thread-manager-sample/src/main.rs +++ b/codex-rs/thread-manager-sample/src/main.rs @@ -185,7 +185,6 @@ fn new_config(model: Option, arg0_paths: Arg0DispatchPaths) -> anyhow::R Constrained::allow_any(PermissionProfile::read_only()), )?, explicit_permission_profile_mode: false, - custom_permission_profiles: Vec::new(), approvals_reviewer: ApprovalsReviewer::User, enforce_residency: Constrained::allow_any(/*initial_value*/ None), hide_agent_reasoning: false, diff --git a/codex-rs/tui/src/chatwidget/permission_popups.rs b/codex-rs/tui/src/chatwidget/permission_popups.rs index 1283a9e490..dca84ee3b9 100644 --- a/codex-rs/tui/src/chatwidget/permission_popups.rs +++ b/codex-rs/tui/src/chatwidget/permission_popups.rs @@ -25,6 +25,13 @@ impl ChatWidget { let current_permission_profile = self.config.permissions.permission_profile().clone(); let guardian_approval_enabled = self.config.features.enabled(Feature::GuardianApproval); let current_review_policy = self.config.approvals_reviewer; + let preset_is_current = |preset: &ApprovalPreset| { + current_approval == AskForApproval::from(preset.approval) + && preset.matches_permission_profile( + ¤t_permission_profile, + self.config.cwd.as_path(), + ) + }; let mut items: Vec = Vec::new(); let presets: Vec = builtin_approval_presets(); @@ -86,12 +93,7 @@ impl ChatWidget { name: base_name.clone(), description: base_description.clone(), is_current: current_review_policy == ApprovalsReviewer::User - && Self::preset_matches_current( - current_approval, - ¤t_permission_profile, - self.config.cwd.as_path(), - &preset, - ), + && preset_is_current(&preset), actions: default_actions, dismiss_on_select: true, disabled_reason: default_disabled_reason, @@ -103,12 +105,7 @@ impl ChatWidget { name: APPROVE_FOR_ME_LABEL.to_string(), description: Some(AUTO_REVIEW_DESCRIPTION.to_string()), is_current: current_review_policy == ApprovalsReviewer::AutoReview - && Self::preset_matches_current( - current_approval, - ¤t_permission_profile, - self.config.cwd.as_path(), - &preset, - ), + && preset_is_current(&preset), actions: self.permission_mode_actions( &preset, APPROVE_FOR_ME_LABEL.to_string(), @@ -126,12 +123,7 @@ impl ChatWidget { items.push(SelectionItem { name: base_name, description: base_description, - is_current: Self::preset_matches_current( - current_approval, - ¤t_permission_profile, - self.config.cwd.as_path(), - &preset, - ), + is_current: preset_is_current(&preset), actions: default_actions, dismiss_on_select: true, disabled_reason: default_disabled_reason, @@ -365,45 +357,6 @@ impl ChatWidget { apply_actions() } - pub(super) fn preset_matches_current( - current_approval: AskForApproval, - current_permission_profile: &PermissionProfile, - cwd: &std::path::Path, - preset: &ApprovalPreset, - ) -> bool { - let preset_approval = AskForApproval::from(preset.approval); - if current_approval != preset_approval { - return false; - } - - match preset.id { - "full-access" => matches!(current_permission_profile, PermissionProfile::Disabled), - "read-only" => { - let file_system_policy = current_permission_profile.file_system_sandbox_policy(); - matches!( - current_permission_profile, - PermissionProfile::Managed { .. } - ) && !file_system_policy.has_full_disk_write_access() - && file_system_policy - .get_writable_roots_with_cwd(cwd) - .is_empty() - && current_permission_profile.network_sandbox_policy() - == preset.permission_profile.network_sandbox_policy() - } - "auto" => { - let file_system_policy = current_permission_profile.file_system_sandbox_policy(); - matches!( - current_permission_profile, - PermissionProfile::Managed { .. } - ) && file_system_policy.can_write_path_with_cwd(cwd, cwd) - && !file_system_policy.has_full_disk_write_access() - && current_permission_profile.network_sandbox_policy() - == preset.permission_profile.network_sandbox_policy() - } - _ => current_permission_profile == &preset.permission_profile, - } - } - pub(crate) fn open_full_access_confirmation( &mut self, preset: ApprovalPreset, diff --git a/codex-rs/tui/src/chatwidget/permissions_menu.rs b/codex-rs/tui/src/chatwidget/permissions_menu.rs index c97721819e..fb11e38362 100644 --- a/codex-rs/tui/src/chatwidget/permissions_menu.rs +++ b/codex-rs/tui/src/chatwidget/permissions_menu.rs @@ -27,17 +27,15 @@ impl ChatWidget { ); return; }; - let mut items = vec![ - self.builtin_permission_mode_selection_item( - default, - ":workspace", - default - .description - .replace(" (Identical to Agent mode)", ""), - AskForApproval::from(default.approval), - ApprovalsReviewer::User, - ), - ]; + let mut items = vec![self.builtin_permission_mode_selection_item( + default, + ":workspace", + default + .description + .replace(" (Identical to Agent mode)", ""), + AskForApproval::from(default.approval), + ApprovalsReviewer::User, + )]; if self.config.features.enabled(Feature::GuardianApproval) { items.push(self.builtin_permission_mode_selection_item( default, diff --git a/codex-rs/tui/src/chatwidget/tests/permissions.rs b/codex-rs/tui/src/chatwidget/tests/permissions.rs index ebac595671..5d66e8499e 100644 --- a/codex-rs/tui/src/chatwidget/tests/permissions.rs +++ b/codex-rs/tui/src/chatwidget/tests/permissions.rs @@ -326,23 +326,9 @@ async fn preset_matching_accepts_workspace_write_with_extra_roots() { let cwd = test_path_buf("/tmp/project").abs(); assert!( - ChatWidget::preset_matches_current( - AskForApproval::OnRequest, - ¤t_profile, - cwd.as_path(), - &preset - ), + preset.matches_permission_profile(¤t_profile, cwd.as_path()), "WorkspaceWrite with extra roots should still match the Ask for approval preset" ); - assert!( - !ChatWidget::preset_matches_current( - AskForApproval::Never, - ¤t_profile, - cwd.as_path(), - &preset - ), - "approval mismatch should prevent matching the preset" - ); } #[tokio::test] @@ -374,12 +360,7 @@ async fn preset_matching_does_not_treat_non_cwd_writable_profile_as_read_only() let cwd = test_path_buf("/tmp/project").abs(); assert!( - !ChatWidget::preset_matches_current( - AskForApproval::OnRequest, - ¤t_profile, - cwd.as_path(), - &preset - ), + !preset.matches_permission_profile(¤t_profile, cwd.as_path()), "profiles with any writable root should not be classified as Read Only" ); } diff --git a/codex-rs/utils/approval-presets/src/lib.rs b/codex-rs/utils/approval-presets/src/lib.rs index c7ce9e578b..d7c3efcd2f 100644 --- a/codex-rs/utils/approval-presets/src/lib.rs +++ b/codex-rs/utils/approval-presets/src/lib.rs @@ -4,6 +4,7 @@ use codex_protocol::models::BUILT_IN_PERMISSION_PROFILE_READ_ONLY; use codex_protocol::models::BUILT_IN_PERMISSION_PROFILE_WORKSPACE; use codex_protocol::models::PermissionProfile; use codex_protocol::protocol::AskForApproval; +use std::path::Path; /// A simple preset pairing an approval policy with a permission profile. #[derive(Debug, Clone)] @@ -22,6 +23,44 @@ pub struct ApprovalPreset { pub permission_profile: PermissionProfile, } +impl ApprovalPreset { + /// Whether the current permission profile has the same user-visible access as this preset. + pub fn matches_permission_profile( + &self, + current_permission_profile: &PermissionProfile, + cwd: &Path, + ) -> bool { + match self.active_permission_profile.id.as_str() { + BUILT_IN_PERMISSION_PROFILE_DANGER_FULL_ACCESS => { + matches!(current_permission_profile, PermissionProfile::Disabled) + } + BUILT_IN_PERMISSION_PROFILE_READ_ONLY => { + let file_system_policy = current_permission_profile.file_system_sandbox_policy(); + matches!( + current_permission_profile, + PermissionProfile::Managed { .. } + ) && !file_system_policy.has_full_disk_write_access() + && file_system_policy + .get_writable_roots_with_cwd(cwd) + .is_empty() + && current_permission_profile.network_sandbox_policy() + == self.permission_profile.network_sandbox_policy() + } + BUILT_IN_PERMISSION_PROFILE_WORKSPACE => { + let file_system_policy = current_permission_profile.file_system_sandbox_policy(); + matches!( + current_permission_profile, + PermissionProfile::Managed { .. } + ) && file_system_policy.can_write_path_with_cwd(cwd, cwd) + && !file_system_policy.has_full_disk_write_access() + && current_permission_profile.network_sandbox_policy() + == self.permission_profile.network_sandbox_policy() + } + _ => current_permission_profile == &self.permission_profile, + } + } +} + /// Built-in list of approval presets that pair approval and permissions. /// /// Keep this UI-agnostic so it can be reused by both TUI and MCP server.