github/codex
4
0
Fork 0
mirror of https://github.com/openai/codex.git synced 2026-10-19 16:17:49 +00:00
Code Issues Packages Projects Releases Wiki Activity

Use the server permission catalog for TUI permission shortcuts (#50140)

Browse Source 
## Why

Permission shortcuts checked local configuration while the picker used the connected server's catalog and requirements. Shortcuts should respect the same server restrictions, including model-specific auto-review requirements.

## What changed

- Share a cached server catalog between the permission picker and shortcuts. Show the loading picker when no catalog is available and reuse an outstanding fetch.
- Clear the cache on session, account, working-directory, or connection changes.
- Give discovery RPCs distinct request IDs for each fetch and profile page so repeated requests after timeouts do not collide.

## Testing

Add coverage for loading and catalog reuse, managed permission restrictions, and refetching after disconnect. Update timeout coverage to verify that repeated discovery attempts use distinct IDs and each reports a timeout.

GitOrigin-RevId: 7d79874294136bfe3a654bb306eb7caf36d4df1e
This commit is contained in:
Eric Traut
2026-10-02 01:34:23 +00:00
committed by copyberry
parent 8d44977aa2
commit cb6da58876
13 changed files with 141 additions and 57 deletions
Download Patch File Download Diff File Expand all files Collapse all files
codex-rs/tui/src/chatwidget.rs
+1
View File
@@ -537,6 +537,7 @@ pub(crate) struct ChatWidget {
model_catalog: Arc<ModelCatalog>,
model_popup_request_id: Option<uuid::Uuid>,
permission_popup_request_id: Option<uuid::Uuid>,
permission_discovery: Option<crate::permission_discovery::PermissionDiscovery>,
worktree_popup_request_id: Option<uuid::Uuid>,
permission_profiles_menu_opened: bool,
model_popup_model_ids: Vec<String>,
codex-rs/tui/src/chatwidget/constructor.rs
+1
View File
@@ -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(),
codex-rs/tui/src/chatwidget/permission_discovery.rs
+15 -4
View File
@@ -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;
codex-rs/tui/src/chatwidget/permission_shortcuts.rs
+26 -6
View File
@@ -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;
codex-rs/tui/src/chatwidget/permissions_menu.rs
-25
View File
@@ -49,31 +49,6 @@ pub(crate) fn cyber_model_approval_reviewer(config: &Config) -> Option<Approvals
}
impl ChatWidget {
pub(super) fn permission_mode_disabled_reason(
&self,
preset: &ApprovalPreset,
approval_policy: AskForApproval,
) -> Option<String> {
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
codex-rs/tui/src/chatwidget/reconnect.rs
+2
View File
@@ -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
codex-rs/tui/src/chatwidget/session_flow.rs
+1
View File
@@ -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();
codex-rs/tui/src/chatwidget/settings.rs
+4
View File
@@ -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;
codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__permission_shortcut_loading.snap
+11
View File
@@ -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
codex-rs/tui/src/chatwidget/tests/permission_shortcuts_tests.rs
+61 -12
View File
@@ -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 { .. })
);
}
codex-rs/tui/src/chatwidget/tests/permissions.rs
+2
View File
@@ -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 {
codex-rs/tui/src/permission_discovery.rs
+9 -5
View File
@@ -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<PermissionProfileSummary>,
pub(crate) requirements: Option<ConfigRequirements>,
@@ -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),
codex-rs/tui/src/permission_discovery_tests.rs
+8 -5
View File
@@ -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()