diff --git a/codex-rs/tui/src/chatwidget.rs b/codex-rs/tui/src/chatwidget.rs index 15dac9770c..ccaaa52657 100644 --- a/codex-rs/tui/src/chatwidget.rs +++ b/codex-rs/tui/src/chatwidget.rs @@ -537,6 +537,7 @@ pub(crate) struct ChatWidget { model_catalog: Arc, model_popup_request_id: Option, permission_popup_request_id: Option, + permission_discovery: Option, worktree_popup_request_id: Option, permission_profiles_menu_opened: bool, model_popup_model_ids: Vec, diff --git a/codex-rs/tui/src/chatwidget/constructor.rs b/codex-rs/tui/src/chatwidget/constructor.rs index cf78ed06f0..a7e19f0474 100644 --- a/codex-rs/tui/src/chatwidget/constructor.rs +++ b/codex-rs/tui/src/chatwidget/constructor.rs @@ -127,6 +127,7 @@ impl ChatWidget { model_catalog, model_popup_request_id: None, permission_popup_request_id: None, + permission_discovery: None, worktree_popup_request_id: None, permission_profiles_menu_opened: false, model_popup_model_ids: Vec::new(), diff --git a/codex-rs/tui/src/chatwidget/permission_discovery.rs b/codex-rs/tui/src/chatwidget/permission_discovery.rs index 42ee1b5fa1..9fa0b2a6a2 100644 --- a/codex-rs/tui/src/chatwidget/permission_discovery.rs +++ b/codex-rs/tui/src/chatwidget/permission_discovery.rs @@ -1,4 +1,4 @@ -//! Keep named-profile discovery responsive and discard replies after closing or changing scope. +//! Cache the last server catalog and discard discovery replies after changing scope. use super::*; use crate::permission_discovery::PermissionDiscovery; @@ -14,9 +14,12 @@ impl ChatWidget { } } pub(crate) fn request_permission_profiles(&mut self) { - self.invalidate_permission_discovery(); - let request_id = uuid::Uuid::new_v4(); - self.permission_popup_request_id = Some(request_id); + self.bottom_pane.dismiss_view_by_id(VIEW_ID); + if let Some(discovery) = self.permission_discovery.clone() { + self.permission_profiles_menu_opened = true; + self.open_permission_profiles_popup(discovery); + return; + } self.bottom_pane.show_selection_view(SelectionViewParams { view_id: Some(VIEW_ID), title: Some("Update Model Permissions".into()), @@ -27,6 +30,11 @@ impl ChatWidget { }], ..SelectionViewParams::picker() }); + if self.permission_popup_request_id.is_some() { + return; + } + let request_id = uuid::Uuid::new_v4(); + self.permission_popup_request_id = Some(request_id); self.app_event_tx.send(AppEvent::FetchPermissionProfiles { request_id, thread_cwd: self.thread_id.and(self.current_cwd.clone()), @@ -51,6 +59,9 @@ impl ChatWidget { return; } self.permission_popup_request_id = None; + if let Ok(discovery) = &result { + self.permission_discovery = Some(discovery.clone()); + } if !self.bottom_pane.dismiss_active_view_if_id(VIEW_ID) { self.bottom_pane.dismiss_view_by_id(VIEW_ID); return; diff --git a/codex-rs/tui/src/chatwidget/permission_shortcuts.rs b/codex-rs/tui/src/chatwidget/permission_shortcuts.rs index 73f2e3194f..444a11d8bb 100644 --- a/codex-rs/tui/src/chatwidget/permission_shortcuts.rs +++ b/codex-rs/tui/src/chatwidget/permission_shortcuts.rs @@ -1,4 +1,4 @@ -//! Session-scoped shortcuts for the ordinary built-in permission modes. +//! Cycle ordinary built-in modes using the last permission catalog received from the server. use super::*; @@ -29,6 +29,24 @@ impl ChatWidget { return true; }; + let Some(discovery) = self.permission_discovery.as_ref() else { + self.request_permission_profiles(); + return true; + }; + let server_requirements = codex_config::ConfigRequirements { + auto_review_required_models: discovery + .requirements + .as_ref() + .and_then(|requirements| requirements.auto_review.as_ref()) + .and_then(|auto_review| auto_review.required_on_models.as_ref()) + .map(|models| { + codex_config::Sourced::new( + models.iter().cloned().collect(), + codex_config::RequirementSource::Unknown, + ) + }), + ..Default::default() + }; let current_approval = AskForApproval::from(self.config.permissions.approval_policy.value()); let active_profile = self.config.permissions.active_permission_profile(); @@ -45,12 +63,14 @@ impl ChatWidget { continue; } let approval = AskForApproval::from(preset.approval); - let requirements = self.config.config_layer_stack.requirements(); - if self - .permission_mode_disabled_reason(&preset, approval) + if discovery + .disabled_reason( + &preset.active_permission_profile.id, + Some(approval), + Some(reviewer.into()), + ) .is_some() - || requirements.approvals_reviewer.can_set(&reviewer).is_err() - || (requirements.auto_review_required_for_model(self.current_model()) + || (server_requirements.auto_review_required_for_model(self.current_model()) && reviewer != ApprovalsReviewer::AutoReview) { continue; diff --git a/codex-rs/tui/src/chatwidget/permissions_menu.rs b/codex-rs/tui/src/chatwidget/permissions_menu.rs index 5ebd1dbf7c..4e2fcb6b38 100644 --- a/codex-rs/tui/src/chatwidget/permissions_menu.rs +++ b/codex-rs/tui/src/chatwidget/permissions_menu.rs @@ -49,31 +49,6 @@ pub(crate) fn cyber_model_approval_reviewer(config: &Config) -> Option Option { - self.config - .permissions - .approval_policy - .can_set(&approval_policy.to_core()) - .and_then(|()| { - self.config - .permissions - .can_set_permission_profile(&preset.permission_profile) - }) - .err() - .map(|err| err.to_string()) - .or_else(|| { - (!self.config.is_permission_profile_allowed( - &preset.active_permission_profile.id, - &preset.permission_profile, - )) - .then(|| "Disabled by requirements".to_string()) - }) - } - pub(super) fn open_permission_profiles_popup(&mut self, discovery: PermissionDiscovery) { let active_profile_id = self .config diff --git a/codex-rs/tui/src/chatwidget/reconnect.rs b/codex-rs/tui/src/chatwidget/reconnect.rs index 3caf832850..4cde2de18f 100644 --- a/codex-rs/tui/src/chatwidget/reconnect.rs +++ b/codex-rs/tui/src/chatwidget/reconnect.rs @@ -7,6 +7,8 @@ use crate::bottom_pane::RestrictedInputMode; impl ChatWidget { pub(crate) fn pause_for_disconnect(&mut self) { + self.permission_discovery = None; + self.invalidate_permission_discovery(); self.cancel_startup_submission(); self.cancel_image_submission(); // The app-server transport can fail while the separate WebRTC helper diff --git a/codex-rs/tui/src/chatwidget/session_flow.rs b/codex-rs/tui/src/chatwidget/session_flow.rs index 298c6645b5..a25db18fb3 100644 --- a/codex-rs/tui/src/chatwidget/session_flow.rs +++ b/codex-rs/tui/src/chatwidget/session_flow.rs @@ -57,6 +57,7 @@ impl ChatWidget { session.windows_sandbox_host }; self.invalidate_permission_discovery(); + self.permission_discovery = None; self.permission_profiles_menu_opened = false; self.transcript.reset_copy_history(); let history_metadata = session.message_history.unwrap_or_default(); diff --git a/codex-rs/tui/src/chatwidget/settings.rs b/codex-rs/tui/src/chatwidget/settings.rs index 4eef57e5c4..399b3f8bed 100644 --- a/codex-rs/tui/src/chatwidget/settings.rs +++ b/codex-rs/tui/src/chatwidget/settings.rs @@ -221,6 +221,7 @@ impl ChatWidget { // be identical across two accounts, so always invalidate account-scoped requests and data. self.model_popup_request_id = None; self.invalidate_permission_discovery(); + self.permission_discovery = None; self.invalidate_connector_scope(); self.clear_pending_rate_limit_reset_requests(); self.clear_backend_banner(); @@ -534,6 +535,9 @@ impl ChatWidget { fn apply_thread_settings_cwd(&mut self, cwd: AbsolutePathBuf) { let previous_cwd = std::mem::replace(&mut self.config.cwd, cwd.clone()); + if previous_cwd != cwd { + self.permission_discovery = None; + } self.current_cwd = Some(cwd.to_path_buf()); self.status_line_project_root_name_cache = None; diff --git a/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__permission_shortcut_loading.snap b/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__permission_shortcut_loading.snap new file mode 100644 index 0000000000..409bb583d0 --- /dev/null +++ b/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__permission_shortcut_loading.snap @@ -0,0 +1,11 @@ +--- +source: tui/src/chatwidget/tests/permission_shortcuts_tests.rs +assertion_line: 156 +expression: "render_bottom_popup(&chat, 80)" +--- + Update Model Permissions + + +› Loading permission profiles… + + enter select · esc back diff --git a/codex-rs/tui/src/chatwidget/tests/permission_shortcuts_tests.rs b/codex-rs/tui/src/chatwidget/tests/permission_shortcuts_tests.rs index 1e718ba7ab..7746dbeab4 100644 --- a/codex-rs/tui/src/chatwidget/tests/permission_shortcuts_tests.rs +++ b/codex-rs/tui/src/chatwidget/tests/permission_shortcuts_tests.rs @@ -16,6 +16,17 @@ async fn permission_shortcuts_cycle_builtin_modes() { { chat.set_windows_sandbox_mode(Some(WindowsSandboxSetupMode::Unelevated)); } + chat.permission_discovery = Some(crate::permission_discovery::PermissionDiscovery::local( + &chat.config, + )); + chat.config.config_layer_stack = requirements_stack( + serde_json::from_value(serde_json::json!({ + "allowed_approvals_reviewers": ["auto_review"], + "allowed_permission_profiles": {":workspace": false, ":read-only": false} + })) + .unwrap(), + ); + chat.turn_lifecycle.agent_turn_running = true; for (current, reviewer, key, expected, next_reviewer) in [ (":workspace", User, KeyCode::F(8), ":workspace", AutoReview), (":workspace", AutoReview, KeyCode::F(8), ":read-only", User), @@ -117,19 +128,13 @@ async fn permission_shortcuts_respect_managed_mode_requirements() { .expect("set active profile"); for requirements in [ - codex_config::ConfigRequirementsToml { - allowed_approvals_reviewers: Some(vec![AutoReview]), - ..Default::default() - }, - codex_config::ConfigRequirementsToml { - auto_review: Some(codex_config::AutoReviewRequirementsToml { - required_on_models: Some(vec![chat.current_model().to_string()]), - ..Default::default() - }), - ..Default::default() - }, + serde_json::json!({"allowedApprovalsReviewers": ["auto_review"]}), + serde_json::json!({"autoReview": {"requiredOnModels": [chat.current_model()]}}), + serde_json::json!({"allowedPermissionProfiles": {":workspace": false, ":read-only": false}}), ] { - chat.config.config_layer_stack = requirements_stack(requirements); + let mut discovery = crate::permission_discovery::PermissionDiscovery::local(&chat.config); + discovery.requirements = Some(serde_json::from_value(requirements).unwrap()); + chat.permission_discovery = Some(discovery); chat.handle_key_event(KeyEvent::from(KeyCode::F(8))); let AppEvent::InsertHistoryCell(cell) = rx.try_recv().expect("unavailable-mode notice") else { @@ -142,3 +147,47 @@ async fn permission_shortcuts_respect_managed_mode_requirements() { assert!(rx.try_recv().is_err(), "must not submit a forbidden mode"); } } + +#[tokio::test] +async fn permission_shortcuts_load_once_and_reuse_the_picker_catalog() { + let (mut chat, mut events, _ops) = make_chatwidget_manual(/*model_override*/ None).await; + let thread_id = ThreadId::new(); + chat.thread_id = Some(thread_id); + chat.chat_keymap.next_permission_mode = vec![crate::key_hint::plain(KeyCode::F(8))]; + chat.handle_key_event(KeyEvent::from(KeyCode::F(8))); + let request_id = assert_matches!(events.try_recv(), Ok(AppEvent::FetchPermissionProfiles { request_id, .. }) => request_id); + assert_chatwidget_snapshot!( + "permission_shortcut_loading", + render_bottom_popup(&chat, /*width*/ 80) + ); + chat.handle_key_event(KeyEvent::from(KeyCode::Esc)); + chat.handle_key_event(KeyEvent::from(KeyCode::F(8))); + assert!(events.try_recv().is_err(), "reuse pending fetch"); + chat.on_permission_profiles_loaded( + request_id, + Ok(crate::permission_discovery::PermissionDiscovery::local( + &chat.config, + )), + ); + chat.handle_key_event(KeyEvent::from(KeyCode::Esc)); + chat.open_permissions_popup(); + assert!(chat.bottom_pane.has_active_view()); + assert!( + events.try_recv().is_err(), + "reopening must use the cached catalog" + ); + chat.handle_key_event(KeyEvent::from(KeyCode::Esc)); + chat.handle_key_event(KeyEvent::from(KeyCode::F(8))); + assert_matches!( + events.try_recv(), + Ok(AppEvent::ApplyPermissionShortcut { .. }) + ); + chat.complete_permission_shortcut(thread_id); + chat.pause_for_disconnect(); + while events.try_recv().is_ok() {} + chat.handle_key_event(KeyEvent::from(KeyCode::F(8))); + assert_matches!( + events.try_recv(), + Ok(AppEvent::FetchPermissionProfiles { .. }) + ); +} diff --git a/codex-rs/tui/src/chatwidget/tests/permissions.rs b/codex-rs/tui/src/chatwidget/tests/permissions.rs index 47428157cd..4282cab86f 100644 --- a/codex-rs/tui/src/chatwidget/tests/permissions.rs +++ b/codex-rs/tui/src/chatwidget/tests/permissions.rs @@ -96,6 +96,7 @@ async fn permission_discovery_discards_stale_results_and_preserves_covering_moda chat.handle_key_event(KeyEvent::from(KeyCode::Esc)); chat.on_permission_profiles_loaded(first, Ok(Discovery::local(&chat.config))); assert!(!chat.bottom_pane.has_active_view()); + chat.permission_discovery = None; chat.open_permissions_popup(); let second = chat.permission_popup_request_id.unwrap(); chat.on_permission_profiles_loaded(first, Ok(Discovery::local(&chat.config))); @@ -107,6 +108,7 @@ async fn permission_discovery_discards_stale_results_and_preserves_covering_moda chat.on_permission_profiles_loaded(second, Ok(Discovery::local(&chat.config))); assert!(!chat.bottom_pane.has_active_view()); + chat.permission_discovery = None; chat.open_permissions_popup(); let request_id = chat.permission_popup_request_id.unwrap(); chat.bottom_pane.show_selection_view(SelectionViewParams { diff --git a/codex-rs/tui/src/permission_discovery.rs b/codex-rs/tui/src/permission_discovery.rs index 2579fc6e9b..effb98e7a4 100644 --- a/codex-rs/tui/src/permission_discovery.rs +++ b/codex-rs/tui/src/permission_discovery.rs @@ -1,4 +1,4 @@ -//! Bounded discovery for permission pickers; fixed request IDs bound abandoned RPCs. +//! Bounded permission catalog discovery with distinct IDs for concurrent requests. //! Discovery and availability come from the connected server; selection uses its settings API. use crate::app_event::AppEvent; @@ -22,7 +22,7 @@ use std::collections::HashSet; use std::time::Duration; use uuid::Uuid; -#[derive(Debug)] +#[derive(Clone, Debug)] pub(crate) struct PermissionDiscovery { pub(crate) profiles: Vec, pub(crate) requirements: Option, @@ -98,7 +98,9 @@ pub(crate) fn fetch( let request = async { let requirements: ConfigRequirementsReadResponse = handle .request_typed(ClientRequest::ConfigRequirementsRead { - request_id: RequestId::String("tui-permission-requirements".to_string()), + request_id: RequestId::String(format!( + "tui-permission-requirements-{request_id}" + )), params: None, }) .await @@ -106,10 +108,12 @@ pub(crate) fn fetch( let mut profiles = Vec::new(); let mut cursor = None; let mut cursors = HashSet::new(); - for _ in 0..10 { + for page in 0..10 { let response: PermissionProfileListResponse = handle .request_typed(ClientRequest::PermissionProfileList { - request_id: RequestId::String("tui-permission-profiles".to_string()), + request_id: RequestId::String(format!( + "tui-permission-profiles-{request_id}-{page}" + )), params: PermissionProfileListParams { cursor, limit: Some(100), diff --git a/codex-rs/tui/src/permission_discovery_tests.rs b/codex-rs/tui/src/permission_discovery_tests.rs index f40cbb2390..76187e3821 100644 --- a/codex-rs/tui/src/permission_discovery_tests.rs +++ b/codex-rs/tui/src/permission_discovery_tests.rs @@ -77,7 +77,7 @@ async fn discovery_preserves_config_scope_and_bounds_server_requests() { ], }); if case == "timeout" { - replies = vec![Value::Null]; + replies = vec![Value::Null; 4]; } let listener = tokio::net::TcpListener::bind("127.0.0.1:0").await.unwrap(); let websocket_url = format!("ws://{}", listener.local_addr().unwrap()); @@ -120,7 +120,7 @@ async fn discovery_preserves_config_scope_and_bounds_server_requests() { if case == "timeout" { tokio::time::pause(); } // Keep real time for the socket handshake. - for attempt in 0..if case == "timeout" { 4 } else { 1 } { + for _ in 0..if case == "timeout" { 4 } else { 1 } { fetch( &session, Uuid::new_v4(), @@ -135,8 +135,7 @@ async fn discovery_preserves_config_scope_and_bounds_server_requests() { "unsupported" => Some("Upgrade"), "config-error" => Some("Invalid project config"), "cycle" | "limit" => Some("pagination limit"), - "timeout" if attempt == 0 => Some("timed out"), - "timeout" => Some("duplicate"), + "timeout" => Some("timed out"), _ => None, }; if let Some(error) = error { @@ -157,7 +156,11 @@ async fn discovery_preserves_config_scope_and_bounds_server_requests() { session.shutdown().await.unwrap(); let requests = server.await.unwrap(); match case { - "timeout" => assert_eq!(requests.len(), 1), + "timeout" => { + assert_eq!(requests.len(), 4); + let ids: HashSet<_> = requests.iter().map(|request| &request["id"]).collect(); + assert_eq!(ids.len(), requests.len()); + } "local" | "session-only" | "remote" | "remote-default" | "thread" | "legacy" => { let pages: Vec<_> = requests .iter()