mirror of
https://github.com/openai/codex.git
synced 2026-10-18 17:18:52 +00:00
Route built-in permission selections through the app server (#46008)
## Why Built-in permission selections in connected TUI threads previously applied local settings directly. Use server-confirmed settings so the TUI adopts the authoritative permissions and refreshes cached network configuration. ## What changed - Send built-in permission selections through the existing app-server settings API, including selections after Windows sandbox setup. Allow built-in selections during a running turn. - Block task creation and switching while permission updates are pending, restoring submitted text to the composer when necessary. - Refresh local permissions, approval settings, runtime overrides, and network configuration when the server confirms a built-in selection. - Submit a pending initial prompt when the selected permissions already match the current settings. ## Testing Add regression tests for server-confirmed built-in selection during a running turn, blocked task switches, stale proxy removal, and pending prompt submission when permissions already match. Update background-task tests to wait for settings confirmation. GitOrigin-RevId: 76e71a8c278c37c0a7ce7c0246da7d185e12ee7a
This commit is contained in:
@@ -335,6 +335,9 @@ impl App {
|
||||
self.chat_widget.pre_draw_tick();
|
||||
return Ok(AppRunControl::Continue);
|
||||
}
|
||||
if self.reject_pending_permission_root_switch() {
|
||||
return Ok(AppRunControl::Continue);
|
||||
}
|
||||
loading::draw(tui)?;
|
||||
if self.primary_thread_id != Some(root_thread_id) {
|
||||
let previous_displayed_thread_id = self.current_displayed_thread_id();
|
||||
|
||||
@@ -252,7 +252,7 @@ impl App {
|
||||
if self.reject_pending_permission_change() {
|
||||
return;
|
||||
}
|
||||
if selection.profile_id.starts_with(':')
|
||||
if (self.chat_widget.thread_id().is_none() && selection.profile_id.starts_with(':'))
|
||||
|| (app_server.thread_params_mode()
|
||||
== crate::app_server_session::ThreadParamsMode::Embedded
|
||||
&& self
|
||||
@@ -274,7 +274,9 @@ impl App {
|
||||
);
|
||||
return;
|
||||
};
|
||||
if self.chat_widget.is_user_turn_pending_or_running() {
|
||||
if !selection.profile_id.starts_with(':')
|
||||
&& self.chat_widget.is_user_turn_pending_or_running()
|
||||
{
|
||||
self.chat_widget
|
||||
.retain_input_after_failed_permission_selection();
|
||||
self.chat_widget.add_error_message(
|
||||
@@ -297,6 +299,7 @@ impl App {
|
||||
self.agents_overview
|
||||
.selected_permission_profiles
|
||||
.insert(thread_id, selection.profile_id);
|
||||
self.chat_widget.submit_initial_user_message_if_pending();
|
||||
return;
|
||||
}
|
||||
let params = ThreadSettingsUpdateParams {
|
||||
@@ -326,7 +329,7 @@ impl App {
|
||||
self.chat_widget
|
||||
.retain_input_after_failed_permission_selection();
|
||||
self.chat_widget
|
||||
.add_error_message("Named profiles require a newer app server.".into());
|
||||
.add_error_message("Permission selection requires a newer app server.".into());
|
||||
}
|
||||
Err(error) => {
|
||||
self.chat_widget
|
||||
@@ -351,6 +354,19 @@ impl App {
|
||||
false
|
||||
}
|
||||
|
||||
pub(super) fn reject_pending_permission_root_switch(&mut self) -> bool {
|
||||
if !self
|
||||
.pending_server_profiles
|
||||
.keys()
|
||||
.any(|thread_id| !self.thread_unavailable(*thread_id))
|
||||
{
|
||||
return false;
|
||||
}
|
||||
self.chat_widget
|
||||
.add_error_message("Wait for permissions to update before switching tasks.".into());
|
||||
true
|
||||
}
|
||||
|
||||
pub(super) fn confirmed_server_profile(
|
||||
&self,
|
||||
thread_id: ThreadId,
|
||||
|
||||
@@ -326,6 +326,9 @@ impl App {
|
||||
self.chat_widget.copy_selection(text, label, format);
|
||||
}
|
||||
AppEvent::ClearUi { name } => {
|
||||
if self.reject_pending_permission_root_switch() {
|
||||
return Ok(AppRunControl::Continue);
|
||||
}
|
||||
self.clear_terminal_ui(tui, /*redraw_header*/ false)?;
|
||||
self.reset_app_ui_state_after_clear();
|
||||
|
||||
@@ -342,6 +345,10 @@ impl App {
|
||||
self.apply_raw_output_mode(tui, enabled, /*notify*/ false);
|
||||
}
|
||||
AppEvent::ClearUiAndSubmitUserMessage { text } => {
|
||||
if self.reject_pending_permission_root_switch() {
|
||||
self.chat_widget.restore_user_message_to_composer(text.into());
|
||||
return Ok(AppRunControl::Continue);
|
||||
}
|
||||
self.clear_terminal_ui(tui, /*redraw_header*/ false)?;
|
||||
self.reset_app_ui_state_after_clear();
|
||||
|
||||
@@ -2093,9 +2100,7 @@ impl App {
|
||||
self.chat_widget.windows_sandbox_elevated_setup_complete =
|
||||
elevated_enabled;
|
||||
if let Some(selection) = profile_selection {
|
||||
if self.apply_permission_profile_selection(selection).await {
|
||||
self.chat_widget.submit_initial_user_message_if_pending();
|
||||
}
|
||||
self.select_permission_profile(app_server, selection).await;
|
||||
} else {
|
||||
self.app_event_tx.send(AppEvent::CodexOp(
|
||||
AppCommand::override_turn_context(
|
||||
|
||||
@@ -542,6 +542,9 @@ impl App {
|
||||
if self.active_thread_id == Some(thread_id) && !self.thread_unavailable(thread_id) {
|
||||
return Ok(());
|
||||
}
|
||||
if self.reject_pending_permission_root_switch() {
|
||||
return Ok(());
|
||||
}
|
||||
if self.windows_sandbox_blocks_thread_switch() {
|
||||
self.chat_widget.add_info_message(
|
||||
"Finish Windows sandbox setup before switching threads.".to_string(),
|
||||
@@ -920,6 +923,12 @@ impl App {
|
||||
initial_user_message: Option<crate::chatwidget::UserMessage>,
|
||||
new_thread_name: Option<String>,
|
||||
) {
|
||||
if self.reject_pending_permission_root_switch() {
|
||||
if let Some(message) = initial_user_message {
|
||||
self.chat_widget.restore_user_message_to_composer(message);
|
||||
}
|
||||
return;
|
||||
}
|
||||
// Start a fresh in-memory session while preserving resumability via persisted rollout
|
||||
// history. If an initial message is provided, `enqueue_primary_thread_session` suppresses it
|
||||
// until the new session is configured and any replayed turns have been rendered.
|
||||
@@ -1218,6 +1227,9 @@ impl App {
|
||||
return Ok(AppRunControl::Continue);
|
||||
}
|
||||
|
||||
if self.reject_pending_permission_root_switch() {
|
||||
return Ok(AppRunControl::Continue);
|
||||
}
|
||||
let (mut resume_config, local_settings) = match self
|
||||
.resume_config_for_target(tui, app_server, &target_session)
|
||||
.await
|
||||
|
||||
@@ -40,6 +40,9 @@ mod model_defaults;
|
||||
mod pagination_completion_tests;
|
||||
#[path = "tests/patch_approval_tests.rs"]
|
||||
mod patch_approval_tests;
|
||||
#[path = "tests/permission_selection_tests.rs"]
|
||||
mod permission_selection_tests;
|
||||
|
||||
#[path = "tests/permission_shortcuts_tests.rs"]
|
||||
mod permission_shortcuts_tests;
|
||||
mod plugin_catalog;
|
||||
|
||||
@@ -11,6 +11,25 @@ use crate::tui::test_support::make_test_tui;
|
||||
use codex_state::SqliteConfig;
|
||||
use pretty_assertions::assert_eq;
|
||||
|
||||
async fn confirm_permission_selection(
|
||||
app: &mut App,
|
||||
server: &mut AppServerSession,
|
||||
thread_id: ThreadId,
|
||||
) -> Result<()> {
|
||||
for _ in 0..20 {
|
||||
let settings = next_thread_settings_updated(server, thread_id).await;
|
||||
app.enqueue_thread_notification(
|
||||
thread_id,
|
||||
ServerNotification::ThreadSettingsUpdated(settings),
|
||||
)
|
||||
.await?;
|
||||
if !app.pending_server_profiles.contains_key(&thread_id) {
|
||||
return Ok(());
|
||||
}
|
||||
}
|
||||
color_eyre::eyre::bail!("permission update was not confirmed");
|
||||
}
|
||||
|
||||
fn trust_launch_folder(app: &mut App) {
|
||||
let projects = serde_json::json!({
|
||||
app.config.cwd.display().to_string(): {"trust_level": "trusted"},
|
||||
@@ -676,6 +695,7 @@ async fn command_center_new_restores_blank_drafts_and_builtin_permissions() -> R
|
||||
Box::pin(app.handle_event(&mut tui, &mut server, event)).await?;
|
||||
}
|
||||
}
|
||||
confirm_permission_selection(&mut app, &mut server, first).await?;
|
||||
// Test both immediate creation and creation after A -> B -> A.
|
||||
for switch in [false, true] {
|
||||
if switch {
|
||||
@@ -864,6 +884,7 @@ async fn command_center_new_checkout_and_worktree_preserve_source_and_default_br
|
||||
while let Ok(event) = events.try_recv() {
|
||||
Box::pin(app.handle_event(&mut tui, &mut server, event)).await?;
|
||||
}
|
||||
confirm_permission_selection(&mut app, &mut server, first).await?;
|
||||
assert_eq!(
|
||||
app.chat_widget.config_ref().approvals_reviewer,
|
||||
ApprovalsReviewer::User
|
||||
|
||||
@@ -0,0 +1,140 @@
|
||||
//! Permission selection through the existing app-server settings API.
|
||||
|
||||
use super::*;
|
||||
use pretty_assertions::assert_eq;
|
||||
|
||||
#[tokio::test]
|
||||
async fn builtin_permission_selection_adopts_server_settings() -> Result<()> {
|
||||
let (mut app, mut events, _ops) = make_test_app_with_channels().await;
|
||||
let mut server = start_config_write_test_app_server(&app).await?;
|
||||
let started = server.start_thread(&app.config).await?;
|
||||
let thread_id = started.session.thread_id;
|
||||
app.enqueue_primary_thread_session(started.session, started.turns)
|
||||
.await?;
|
||||
let other = server.start_thread(&app.config).await?.session.thread_id;
|
||||
// A previously selected custom profile may have left a profile-specific proxy cached.
|
||||
let home = tempdir()?;
|
||||
std::fs::write(
|
||||
home.path().join("config.toml"),
|
||||
r#"default_permissions = "proxied"
|
||||
[features]
|
||||
network_proxy = true
|
||||
[permissions.proxied]
|
||||
extends = ":workspace"
|
||||
[permissions.proxied.network]
|
||||
enabled = true
|
||||
proxy_url = "http://127.0.0.1:43128"
|
||||
"#,
|
||||
)?;
|
||||
let proxied = ConfigBuilder::default()
|
||||
.codex_home(home.path().to_path_buf())
|
||||
.loader_overrides(codex_config::LoaderOverrides::without_managed_config_for_tests())
|
||||
.build()
|
||||
.await?;
|
||||
assert!(proxied.permissions.network.is_some());
|
||||
app.chat_widget
|
||||
.set_permission_network(proxied.permissions.network);
|
||||
app.chat_widget.handle_server_notification(
|
||||
turn_started_notification(thread_id, "turn-1"),
|
||||
/*replay_kind*/ None,
|
||||
);
|
||||
assert!(app.chat_widget.is_user_turn_pending_or_running());
|
||||
let before = RuntimePermissionProfileOverride::from_config(app.chat_widget.config_ref());
|
||||
let profile_id = if before
|
||||
.active_permission_profile
|
||||
.as_ref()
|
||||
.is_some_and(|profile| profile.id == ":workspace")
|
||||
{
|
||||
":read-only"
|
||||
} else {
|
||||
":workspace"
|
||||
};
|
||||
while events.try_recv().is_ok() {}
|
||||
app.select_permission_profile(
|
||||
&mut server,
|
||||
PermissionProfileSelection {
|
||||
profile_id: profile_id.into(),
|
||||
approval_policy: Some(AskForApproval::OnRequest),
|
||||
approvals_reviewer: Some(ApprovalsReviewer::User),
|
||||
display_label: profile_id.into(),
|
||||
},
|
||||
)
|
||||
.await;
|
||||
assert_eq!(
|
||||
RuntimePermissionProfileOverride::from_config(app.chat_widget.config_ref()),
|
||||
before,
|
||||
);
|
||||
assert!(app.pending_server_profiles.contains_key(&thread_id));
|
||||
insta::assert_snapshot!(next_history_message(&mut events).replace(profile_id, "<PROFILE>"), @"• Permission selection requested: <PROFILE>");
|
||||
while events.try_recv().is_ok() {}
|
||||
let mut tui = crate::tui::test_support::make_test_tui()?;
|
||||
app.handle_event(&mut tui, &mut server, AppEvent::NewSession { name: None })
|
||||
.await?;
|
||||
assert_eq!(app.chat_widget.thread_id(), Some(thread_id));
|
||||
insta::assert_snapshot!(next_history_message(&mut events), @"■ Wait for permissions to update before switching tasks.");
|
||||
app.select_agents_overview_thread(&mut tui, &mut server, other)
|
||||
.await?;
|
||||
assert_eq!(app.chat_widget.thread_id(), Some(thread_id));
|
||||
let settings = next_thread_settings_updated(&mut server, thread_id).await;
|
||||
app.enqueue_thread_notification(
|
||||
thread_id,
|
||||
ServerNotification::ThreadSettingsUpdated(settings),
|
||||
)
|
||||
.await?;
|
||||
assert!(!app.pending_server_profiles.contains_key(&thread_id));
|
||||
assert_eq!(
|
||||
(
|
||||
app.chat_widget
|
||||
.config_ref()
|
||||
.permissions
|
||||
.active_permission_profile(),
|
||||
app.config.approvals_reviewer
|
||||
),
|
||||
(
|
||||
Some(ActivePermissionProfile::new(profile_id)),
|
||||
ApprovalsReviewer::User
|
||||
),
|
||||
);
|
||||
assert_eq!(app.chat_widget.config_ref().permissions.network, None);
|
||||
assert_eq!(app.config.permissions.network, None);
|
||||
assert_eq!(
|
||||
app.runtime_permission_profile_override,
|
||||
Some(RuntimePermissionProfileOverride::from_config(&app.config))
|
||||
);
|
||||
server.shutdown().await?;
|
||||
Ok(())
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn matching_builtin_permission_selection_submits_pending_prompt() -> Result<()> {
|
||||
let (mut app, mut events, mut ops) = make_test_app_with_channels().await;
|
||||
let mut server = start_config_write_test_app_server(&app).await?;
|
||||
let started = server.start_thread(&app.config).await?;
|
||||
app.enqueue_primary_thread_session(started.session, started.turns)
|
||||
.await?;
|
||||
app.chat_widget.initial_user_message =
|
||||
create_initial_user_message(Some("review this".to_string()), Vec::new(), Vec::new());
|
||||
let config = app.chat_widget.config_ref();
|
||||
let selection = PermissionProfileSelection {
|
||||
profile_id: config.permissions.active_permission_profile().unwrap().id,
|
||||
approval_policy: Some(config.permissions.approval_policy.value().into()),
|
||||
approvals_reviewer: Some(config.approvals_reviewer),
|
||||
display_label: "Current permissions".to_string(),
|
||||
};
|
||||
while events.try_recv().is_ok() {}
|
||||
app.select_permission_profile(&mut server, selection).await;
|
||||
assert!(app.chat_widget.initial_user_message.is_none());
|
||||
assert!(app.pending_server_profiles.is_empty());
|
||||
let Op::UserTurn { items, .. } = next_user_turn_op(&mut ops) else {
|
||||
panic!("expected initial user turn");
|
||||
};
|
||||
assert_eq!(
|
||||
items,
|
||||
vec![AppServerUserInput::Text {
|
||||
text: "review this".to_string(),
|
||||
text_elements: Vec::new(),
|
||||
}]
|
||||
);
|
||||
server.shutdown().await?;
|
||||
Ok(())
|
||||
}
|
||||
@@ -88,7 +88,7 @@ async fn permission_shortcut_rejections_leave_state_unchanged() -> Result<()> {
|
||||
.await;
|
||||
insta::assert_snapshot!(
|
||||
next_history_message(&mut events),
|
||||
@"■ Named profiles require a newer app server."
|
||||
@"■ Permission selection requires a newer app server."
|
||||
);
|
||||
}
|
||||
app_server.shutdown().await?;
|
||||
|
||||
@@ -265,7 +265,7 @@ pub(super) async fn start_recording_app_server_with_realtime_speech(
|
||||
let state_db =
|
||||
crate::init_state_db_for_app_server_target(config, &crate::AppServerTarget::Embedded)
|
||||
.await?;
|
||||
let embedded = crate::start_embedded_app_server(
|
||||
let mut embedded = crate::start_embedded_app_server(
|
||||
codex_arg0::Arg0DispatchPaths::default(),
|
||||
config.clone(),
|
||||
Vec::new(),
|
||||
@@ -289,7 +289,29 @@ pub(super) async fn start_recording_app_server_with_realtime_speech(
|
||||
let mut inventories = usize::from(failed_thread_name == Some("background"));
|
||||
let mut reject_detach = false;
|
||||
let mut reject_thread_list = history_capabilities == HistoryCapabilities::ThreadListFails;
|
||||
while let Some(frame) = websocket.next().await {
|
||||
loop {
|
||||
let frame = tokio::select! {
|
||||
frame = websocket.next() => frame,
|
||||
event = embedded.next_event() => {
|
||||
let Some(event) = event else { break };
|
||||
if let codex_app_server_client::InProcessServerEvent::ServerNotification(notification) = event
|
||||
&& matches!(*notification, ServerNotification::ThreadSettingsUpdated(_))
|
||||
{
|
||||
websocket.send(Message::Text(serde_json::to_string(¬ification)?.into())).await?;
|
||||
}
|
||||
continue;
|
||||
}
|
||||
};
|
||||
let Some(frame) = frame else { break };
|
||||
// The client can close with an unread settings notification during shutdown.
|
||||
match &frame {
|
||||
Err(tokio_tungstenite::tungstenite::Error::Protocol(
|
||||
tokio_tungstenite::tungstenite::error::ProtocolError::ResetWithoutClosingHandshake,
|
||||
)) => break,
|
||||
Err(tokio_tungstenite::tungstenite::Error::Io(error))
|
||||
if matches!(error.kind(), std::io::ErrorKind::BrokenPipe | std::io::ErrorKind::ConnectionReset) => break,
|
||||
_ => {}
|
||||
}
|
||||
let Message::Text(text) = frame? else {
|
||||
continue;
|
||||
};
|
||||
|
||||
@@ -1282,6 +1282,34 @@ impl App {
|
||||
notification = None;
|
||||
}
|
||||
if permission_change_confirmed {
|
||||
if self.chat_widget.thread_id() == Some(thread_id)
|
||||
&& let Some(profile) = self
|
||||
.chat_widget
|
||||
.config_ref()
|
||||
.permissions
|
||||
.active_permission_profile()
|
||||
&& profile.id.starts_with(':')
|
||||
{
|
||||
let config = self.chat_widget.config_ref();
|
||||
let network = config
|
||||
.network_proxy_spec_for_active_permission_profile(
|
||||
&profile,
|
||||
config.permissions.permission_profile(),
|
||||
)
|
||||
.unwrap_or_else(|err| {
|
||||
tracing::warn!(%err, "failed to refresh local permission network settings");
|
||||
None
|
||||
});
|
||||
self.chat_widget.set_permission_network(network);
|
||||
self.config.permissions = self.chat_widget.config_ref().permissions.clone();
|
||||
self.config.approvals_reviewer = self.chat_widget.config_ref().approvals_reviewer;
|
||||
self.runtime_approval_policy_override =
|
||||
Some(RuntimeApprovalPolicyOverride::Explicit(
|
||||
self.config.permissions.approval_policy.value().into(),
|
||||
));
|
||||
self.runtime_permission_profile_override =
|
||||
Some(RuntimePermissionProfileOverride::from_config(&self.config));
|
||||
}
|
||||
self.app_event_tx.send(AppEvent::SettingsSelectionSettled);
|
||||
}
|
||||
|
||||
|
||||
@@ -311,6 +311,14 @@ impl ChatWidget {
|
||||
profile_selection: Option<PermissionProfileSelection>,
|
||||
return_to_permissions: bool,
|
||||
) -> Vec<SelectionAction> {
|
||||
let profile_selection = profile_selection.or_else(|| {
|
||||
self.thread_id.map(|_| PermissionProfileSelection {
|
||||
profile_id: preset.active_permission_profile.id.clone(),
|
||||
approval_policy: Some(AskForApproval::from(preset.approval)),
|
||||
approvals_reviewer: Some(approvals_reviewer),
|
||||
display_label: label.clone(),
|
||||
})
|
||||
});
|
||||
let apply_actions = || {
|
||||
profile_selection.clone().map_or_else(
|
||||
|| {
|
||||
|
||||
Reference in New Issue
Block a user