mirror of
https://github.com/openai/codex.git
synced 2026-09-20 12:47:38 +00:00
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
This commit is contained in:
@@ -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:#}"));
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
@@ -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,
|
||||
|
||||
@@ -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}"
|
||||
))),
|
||||
);
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
@@ -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(), "<THREAD_ID>")
|
||||
next_history_message(&mut events).replace(&thread_id.to_string(), "<THREAD_ID>")
|
||||
);
|
||||
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(())
|
||||
}
|
||||
|
||||
@@ -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\"<THREAD_ID>\")"
|
||||
assertion_line: 65
|
||||
expression: "next_history_message(&mut\nevents).replace(&thread_id.to_string(), \"<THREAD_ID>\")"
|
||||
---
|
||||
■ Failed to update permissions: thread/settings/update failed in TUI: thread/settings/update failed: thread not found: <THREAD_ID> (code -32600)
|
||||
■ Failed to select permissions: thread/settings/update failed in TUI: thread/settings/update failed: thread not found: <THREAD_ID> (code -32600)
|
||||
|
||||
@@ -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(), \"<THREAD_ID>\")"
|
||||
---
|
||||
■ Failed to update permissions: this app server does not support confirmed permission changes; use /permissions
|
||||
■ Permission selection requires a newer app server.
|
||||
|
||||
Reference in New Issue
Block a user