From a2f62e88cf473fc89b14df9dd7f37002e29258a3 Mon Sep 17 00:00:00 2001 From: Eric Traut Date: Wed, 16 Sep 2026 19:44:16 +0000 Subject: [PATCH] Route permission shortcuts through the shared selection flow (#46013) ## Why Permission shortcuts updated local permission state as soon as the server accepted the settings request, before receiving the server's settings notification. ## What changed Use `select_permission_profile` for session-only permission shortcuts so local permissions follow the server's `ThreadSettingsUpdated` notification. Report the selection as requested while it is pending, and use the shared rejection messages. Include the full error chain when permission selection fails. ## Testing Update shortcut tests to verify that local permissions remain unchanged until the settings notification arrives, pending state clears afterward, and `config.toml` remains unchanged. Update rejection snapshots for unsupported servers and server errors. GitOrigin-RevId: 0ecd3d8b0698a039bf354d83b25b64b4c38fb45f --- codex-rs/tui/src/app/config_persistence.rs | 2 +- codex-rs/tui/src/app/event_dispatch.rs | 2 +- codex-rs/tui/src/app/permission_shortcuts.rs | 59 +----------------- .../app/tests/permission_shortcuts_tests.rs | 61 +++++++------------ ...sts__permission_shortcut_server_error.snap | 6 +- ...ests__permission_shortcut_unsupported.snap | 6 +- 6 files changed, 31 insertions(+), 105 deletions(-) diff --git a/codex-rs/tui/src/app/config_persistence.rs b/codex-rs/tui/src/app/config_persistence.rs index b5194666b3..8346bdd9e1 100644 --- a/codex-rs/tui/src/app/config_persistence.rs +++ b/codex-rs/tui/src/app/config_persistence.rs @@ -335,7 +335,7 @@ impl App { self.chat_widget .retain_input_after_failed_permission_selection(); self.chat_widget - .add_error_message(format!("Failed to select permissions: {error}")); + .add_error_message(format!("Failed to select permissions: {error:#}")); } } } diff --git a/codex-rs/tui/src/app/event_dispatch.rs b/codex-rs/tui/src/app/event_dispatch.rs index 30623b7f06..318e4b933f 100644 --- a/codex-rs/tui/src/app/event_dispatch.rs +++ b/codex-rs/tui/src/app/event_dispatch.rs @@ -1975,7 +1975,7 @@ impl App { ); } AppEvent::ApplyPermissionShortcut { thread_id, selection } => { - self.apply_permission_shortcut(app_server, tui, thread_id, selection).await; + self.apply_permission_shortcut(app_server, thread_id, selection).await; } AppEvent::OpenFeedbackNote { category, diff --git a/codex-rs/tui/src/app/permission_shortcuts.rs b/codex-rs/tui/src/app/permission_shortcuts.rs index e7c81e66ad..e5e511c141 100644 --- a/codex-rs/tui/src/app/permission_shortcuts.rs +++ b/codex-rs/tui/src/app/permission_shortcuts.rs @@ -1,13 +1,11 @@ -//! Confirm session-only permission changes before updating local state. +//! Route session-only permission shortcuts through the shared selection flow. use super::*; -use codex_app_server_protocol::ThreadSettingsUpdateParams; impl App { pub(super) async fn apply_permission_shortcut( &mut self, app_server: &mut AppServerSession, - tui: &mut tui::Tui, thread_id: ThreadId, selection: PermissionProfileSelection, ) { @@ -17,60 +15,7 @@ impl App { self.chat_widget.complete_permission_shortcut(thread_id); return; } - if self.reject_pending_permission_change() { - self.chat_widget.complete_permission_shortcut(thread_id); - return; - } - let result: Result<()> = async { - let active_profile = ActivePermissionProfile::new(selection.profile_id.clone()); - let profile = builtin_permission_profile_for_active_permission_profile(&active_profile) - .ok_or_else(|| color_eyre::eyre::eyre!("unknown built-in permission profile"))?; - let mut config = self.chat_widget.config_ref().clone(); - if let Some(policy) = selection.approval_policy { - config.permissions.approval_policy.set(policy.to_core())?; - } - if let Some(reviewer) = selection.approvals_reviewer { - config.config_layer_stack.requirements().approvals_reviewer.can_set(&reviewer)?; - config.approvals_reviewer = reviewer; - } - config.permissions.set_permission_profile_from_session_snapshot( - PermissionProfileSnapshot::active(profile, active_profile), - )?; - - if !app_server.thread_settings_update(ThreadSettingsUpdateParams { - thread_id: thread_id.to_string(), - permissions: Some(selection.profile_id.clone()), - approval_policy: selection.approval_policy, - approvals_reviewer: selection.approvals_reviewer.map(Into::into), - ..Default::default() - }).await? { - color_eyre::eyre::bail!("this app server does not support confirmed permission changes; use /permissions"); - } - - // This exact widget configuration was validated before the request. - self.chat_widget.set_permission_profile_with_active_profile( - config.permissions.permission_profile().clone(), - config.permissions.active_permission_profile(), - )?; - self.chat_widget.set_approval_policy(AskForApproval::from(config.permissions.approval_policy.value())); - self.config.permissions = config.permissions.clone(); - self.set_approvals_reviewer_in_app_and_widget(config.approvals_reviewer); - self.agents_overview.selected_permission_profiles.insert(thread_id, selection.profile_id); - self.runtime_approval_policy_override = selection.approval_policy.map(RuntimeApprovalPolicyOverride::Explicit); - self.runtime_permission_profile_override = Some(RuntimePermissionProfileOverride::from_config(&config)); - self.sync_active_thread_permission_settings_to_cached_session().await; - self.insert_history_cell(tui, Box::new(history_cell::new_info_event(format!("Permissions updated to {}", selection.display_label), /*hint*/ None))); - Ok(()) - }.await; + self.select_permission_profile(app_server, selection).await; self.chat_widget.complete_permission_shortcut(thread_id); - if let Err(err) = result { - let error = crate::config_update::format_config_error(&err); - self.insert_history_cell( - tui, - Box::new(history_cell::new_error_event(format!( - "Failed to update permissions: {error}" - ))), - ); - } } } diff --git a/codex-rs/tui/src/app/tests/permission_shortcuts_tests.rs b/codex-rs/tui/src/app/tests/permission_shortcuts_tests.rs index c69d579453..cc71c03840 100644 --- a/codex-rs/tui/src/app/tests/permission_shortcuts_tests.rs +++ b/codex-rs/tui/src/app/tests/permission_shortcuts_tests.rs @@ -15,7 +15,6 @@ fn read_only_selection() -> PermissionProfileSelection { async fn permission_shortcut_rejections_leave_state_unchanged() -> Result<()> { for experimental_api in [false, true] { let (mut app, mut events, _op_rx) = make_test_app_with_channels().await; - let mut tui = crate::tui::test_support::make_test_tui()?; let thread_id = ThreadId::new(); app.active_thread_id = Some(thread_id); app.chat_widget @@ -48,49 +47,26 @@ async fn permission_shortcut_rejections_leave_state_unchanged() -> Result<()> { ); while events.try_recv().is_ok() {} let transcript_len = app.transcript_cells.len(); - app.apply_permission_shortcut( - &mut app_server, - &mut tui, - ThreadId::new(), - read_only_selection(), - ) - .await; + app.apply_permission_shortcut(&mut app_server, ThreadId::new(), read_only_selection()) + .await; assert_eq!(app.transcript_cells.len(), transcript_len); assert!(events.try_recv().is_err()); - app.apply_permission_shortcut(&mut app_server, &mut tui, thread_id, read_only_selection()) + app.apply_permission_shortcut(&mut app_server, thread_id, read_only_selection()) .await; assert_eq!( RuntimePermissionProfileOverride::from_config(app.chat_widget.config_ref()), original ); assert_eq!(app.config.approvals_reviewer, original_reviewer); - let cell = app.transcript_cells.last().expect("rejection notice"); insta::assert_snapshot!( if experimental_api { "permission_shortcut_server_error" } else { "permission_shortcut_unsupported" }, - lines_to_single_string(&cell.display_lines(/*width*/ 120)) - .replace(&thread_id.to_string(), "") + next_history_message(&mut events).replace(&thread_id.to_string(), "") ); assert!(events.try_recv().is_err()); - if !experimental_api { - app.select_permission_profile( - &mut app_server, - PermissionProfileSelection { - profile_id: "server-only".into(), - approval_policy: None, - approvals_reviewer: None, - display_label: "server-only".into(), - }, - ) - .await; - insta::assert_snapshot!( - next_history_message(&mut events), - @"■ Permission selection requires a newer app server." - ); - } app_server.shutdown().await?; } Ok(()) @@ -99,7 +75,6 @@ async fn permission_shortcut_rejections_leave_state_unchanged() -> Result<()> { #[tokio::test] async fn permission_shortcut_confirms_without_persisting() -> Result<()> { let (mut app, mut events, _op_rx) = make_test_app_with_channels().await; - let mut tui = crate::tui::test_support::make_test_tui()?; let codex_home = tempdir()?; app.config.codex_home = codex_home.path().to_path_buf().abs(); app.config @@ -121,18 +96,28 @@ async fn permission_shortcut_confirms_without_persisting() -> Result<()> { let contents = std::fs::read_to_string(&config_path)?; while events.try_recv().is_ok() {} - app.apply_permission_shortcut(&mut app_server, &mut tui, thread_id, read_only_selection()) + let before = RuntimePermissionProfileOverride::from_config(app.chat_widget.config_ref()); + app.apply_permission_shortcut(&mut app_server, thread_id, read_only_selection()) .await; - let cell = app.transcript_cells.last().expect("confirmed notice"); + assert_eq!( + RuntimePermissionProfileOverride::from_config(app.chat_widget.config_ref()), + before + ); + assert!(app.pending_server_profiles.contains_key(&thread_id)); insta::assert_snapshot!( - lines_to_single_string(&cell.display_lines(/*width*/ 80)), - @"• Permissions updated to Read Only" + next_history_message(&mut events), + @"• Permission selection requested: Read Only" ); - let settings = next_thread_settings_updated(&mut app_server, thread_id) - .await - .thread_settings; + let notification = next_thread_settings_updated(&mut app_server, thread_id).await; + let settings = notification.thread_settings.clone(); + app.enqueue_thread_notification( + thread_id, + ServerNotification::ThreadSettingsUpdated(notification), + ) + .await?; + assert!(!app.pending_server_profiles.contains_key(&thread_id)); let profile = app .chat_widget .config_ref() @@ -145,10 +130,6 @@ async fn permission_shortcut_confirms_without_persisting() -> Result<()> { assert_eq!(profile, Some(ActivePermissionProfile::new(":read-only"))); assert_eq!(app.config.approvals_reviewer, ApprovalsReviewer::User); assert_eq!(std::fs::read_to_string(config_path)?, contents); - assert!( - events.try_recv().is_err(), - "must not queue another update or config write" - ); app_server.shutdown().await?; Ok(()) } diff --git a/codex-rs/tui/src/app/tests/snapshots/codex_tui__app__tests__permission_shortcuts_tests__permission_shortcut_server_error.snap b/codex-rs/tui/src/app/tests/snapshots/codex_tui__app__tests__permission_shortcuts_tests__permission_shortcut_server_error.snap index 9b439c6614..e1381e2ad9 100644 --- a/codex-rs/tui/src/app/tests/snapshots/codex_tui__app__tests__permission_shortcuts_tests__permission_shortcut_server_error.snap +++ b/codex-rs/tui/src/app/tests/snapshots/codex_tui__app__tests__permission_shortcuts_tests__permission_shortcut_server_error.snap @@ -1,6 +1,6 @@ --- source: tui/src/app/tests/permission_shortcuts_tests.rs -assertion_line: 63 -expression: "lines_to_single_string(&cell.display_lines(120)).replace(&missing_thread.to_string(),\n\"\")" +assertion_line: 65 +expression: "next_history_message(&mut\nevents).replace(&thread_id.to_string(), \"\")" --- -■ Failed to update permissions: thread/settings/update failed in TUI: thread/settings/update failed: thread not found: (code -32600) +■ Failed to select permissions: thread/settings/update failed in TUI: thread/settings/update failed: thread not found: (code -32600) diff --git a/codex-rs/tui/src/app/tests/snapshots/codex_tui__app__tests__permission_shortcuts_tests__permission_shortcut_unsupported.snap b/codex-rs/tui/src/app/tests/snapshots/codex_tui__app__tests__permission_shortcuts_tests__permission_shortcut_unsupported.snap index 310cad4785..5a822915a7 100644 --- a/codex-rs/tui/src/app/tests/snapshots/codex_tui__app__tests__permission_shortcuts_tests__permission_shortcut_unsupported.snap +++ b/codex-rs/tui/src/app/tests/snapshots/codex_tui__app__tests__permission_shortcuts_tests__permission_shortcut_unsupported.snap @@ -1,6 +1,6 @@ --- source: tui/src/app/tests/permission_shortcuts_tests.rs -assertion_line: 33 -expression: lines_to_single_string(&cell.display_lines(120)) +assertion_line: 66 +expression: "next_history_message(&mut\nevents).replace(&thread_id.to_string(), \"\")" --- -■ Failed to update permissions: this app server does not support confirmed permission changes; use /permissions +■ Permission selection requires a newer app server.