diff --git a/codex-rs/tui/src/app/app_server_events.rs b/codex-rs/tui/src/app/app_server_events.rs index 2821d6d242..69ba044fb4 100644 --- a/codex-rs/tui/src/app/app_server_events.rs +++ b/codex-rs/tui/src/app/app_server_events.rs @@ -537,7 +537,8 @@ impl App { #[cfg(any(target_os = "windows", test))] if let ServerNotification::WindowsSandboxSetupCompleted(result) = notification { - let Some((mode, preset, profile_selection)) = self.windows_sandbox.pending_setup.take() + let Some((mode, preset, profile_selection)) = + self.windows_sandbox.pending_setup.clone() else { return; }; @@ -550,7 +551,6 @@ impl App { } }; if result.mode != expected_mode { - self.windows_sandbox.pending_setup = Some((mode, preset, profile_selection)); return; } if result.success { @@ -567,10 +567,18 @@ impl App { profile_selection, }); } else { + self.windows_sandbox.pending_setup = None; self.chat_widget.clear_windows_sandbox_setup_status(); self.windows_sandbox.setup_started_at = None; - self.chat_widget - .retain_input_after_failed_permission_selection(); + if self + .chat_widget + .recover_inline_permission_profile_selection() + { + self.app_event_tx.send(AppEvent::SettingsSelectionClosed); + } else { + self.chat_widget + .retain_input_after_failed_permission_selection(); + } self.chat_widget.add_error_message(format!( "Windows sandbox setup failed: {}", result.error.unwrap_or_else(|| "unknown error".to_string()) diff --git a/codex-rs/tui/src/app/config_persistence.rs b/codex-rs/tui/src/app/config_persistence.rs index 76d0111f4a..4b5d22f8f6 100644 --- a/codex-rs/tui/src/app/config_persistence.rs +++ b/codex-rs/tui/src/app/config_persistence.rs @@ -271,6 +271,8 @@ impl App { mut selection: PermissionProfileSelection, ) { if self.reject_pending_permission_change() { + self.chat_widget + .retain_input_after_failed_permission_selection(); return; } let Some(thread_id) = self.chat_widget.thread_id() else { @@ -282,22 +284,21 @@ impl App { return; }; let config = self.chat_widget.config_ref(); - if !self + let already_selected = !self .agents_overview .requested_permission_profiles .contains_key(&thread_id) - && selection.profile_id.starts_with(':') && config .permissions .active_permission_profile() .is_some_and(|profile| profile.id == selection.profile_id) - && selection - .approval_policy - .is_none_or(|policy| config.permissions.approval_policy.value() == policy.to_core()) + && selection.approval_policy.is_none_or(|policy| { + config.permissions.approval_policy.value() == policy.to_core() + }) && selection .approvals_reviewer - .is_none_or(|reviewer| config.approvals_reviewer == reviewer) - { + .is_none_or(|reviewer| config.approvals_reviewer == reviewer); + if already_selected && selection.profile_id.starts_with(':') { self.agents_overview .selected_permission_profiles .insert(thread_id, selection.profile_id); @@ -330,9 +331,13 @@ impl App { self.agents_overview .selected_permission_profiles .insert(thread_id, selection.profile_id.clone()); - self.agents_overview - .requested_permission_profiles - .insert(thread_id, selection.clone()); + // Custom profiles still go to the server so edited definitions are reloaded. + // An unchanged selection may not emit a settings notification. + if !already_selected { + self.agents_overview + .requested_permission_profiles + .insert(thread_id, selection.clone()); + } self.chat_widget.add_info_message( format!( "Permission selection requested: {}", diff --git a/codex-rs/tui/src/app/event_dispatch.rs b/codex-rs/tui/src/app/event_dispatch.rs index f1fad3d863..b5a5acf0db 100644 --- a/codex-rs/tui/src/app/event_dispatch.rs +++ b/codex-rs/tui/src/app/event_dispatch.rs @@ -109,6 +109,10 @@ impl App { AppEvent::OpenWindowsSandboxFallbackPrompt { .. } | AppEvent::EnableWindowsSandboxForAgentMode { .. } ) { + #[cfg(any(target_os = "windows", test))] + { + self.windows_sandbox.pending_setup = None; + } self.chat_widget.clear_windows_sandbox_setup_status(); } self.chat_widget.add_info_message( @@ -2186,14 +2190,52 @@ impl App { } } AppEvent::SettingsSelectionClosed => { + #[cfg(any(target_os = "windows", test))] + let windows_sandbox_setup_pending = + self.windows_sandbox.pending_setup.is_some(); + #[cfg(not(any(target_os = "windows", test)))] + let windows_sandbox_setup_pending = false; + if self.chat_widget.no_modal_or_popup_active() + && !windows_sandbox_setup_pending + { + self.chat_widget + .recover_inline_permission_profile_selection(); + } self.app_event_tx.send(AppEvent::SettingsSelectionSettled); } AppEvent::SettingsSelectionSettled => { if self.chat_widget.no_modal_or_popup_active() + && !self + .chat_widget + .inline_permission_profile_selection_pending() && !self .chat_widget .thread_id() - .is_some_and(|thread_id| self.pending_server_profiles.contains_key(&thread_id)) + .is_some_and(|thread_id| { + self.pending_server_profiles.contains_key(&thread_id) + || self + .agents_overview + .requested_permission_profiles + .get(&thread_id) + .is_some_and(|selection| { + let config = self.chat_widget.config_ref(); + config + .permissions + .active_permission_profile() + .is_none_or(|active| { + active.id != selection.profile_id + }) + || selection.approval_policy.is_some_and(|policy| { + config.permissions.approval_policy.value() + != policy.to_core() + }) + || selection + .approvals_reviewer + .is_some_and(|reviewer| { + config.approvals_reviewer != reviewer + }) + }) + }) { let config = self.chat_widget.config_ref(); let permissions_override = Self::turn_permissions_override_from_config( @@ -2381,6 +2423,10 @@ impl App { preset, profile_selection, } => { + #[cfg(any(target_os = "windows", test))] + { + self.windows_sandbox.pending_setup = None; + } self.session_telemetry.counter( "codex.windows_sandbox.fallback_prompt_shown", /*inc*/ 1, @@ -2432,6 +2478,10 @@ impl App { mode, profile_selection, } => { + #[cfg(any(target_os = "windows", test))] + { + self.windows_sandbox.pending_setup = None; + } #[cfg(target_os = "windows")] { self.chat_widget.clear_windows_sandbox_setup_status(); @@ -2456,7 +2506,11 @@ impl App { self.chat_widget.windows_sandbox_elevated_setup_complete = elevated_enabled; if let Some(selection) = profile_selection { + let inline = self.chat_widget.complete_inline_permission_profile_selection(); self.select_permission_profile(app_server, selection).await; + if inline { + self.app_event_tx.send(AppEvent::SettingsSelectionClosed); + } } else { self.app_event_tx.send(AppEvent::CodexOp( AppCommand::override_turn_context( @@ -2488,8 +2542,14 @@ impl App { .dark_gray(), ]), ]); + } else if self + .chat_widget + .recover_inline_permission_profile_selection() + { + self.app_event_tx.send(AppEvent::SettingsSelectionClosed); } else { - self.chat_widget.retain_input_after_failed_permission_selection(); + self.chat_widget + .retain_input_after_failed_permission_selection(); } } #[cfg(not(target_os = "windows"))] @@ -2704,7 +2764,13 @@ impl App { self.chat_widget.submit_initial_user_message_if_pending(); } AppEvent::SelectPermissionProfile(selection) => { + let inline_selection = self + .chat_widget + .complete_inline_permission_profile_selection(); self.select_permission_profile(app_server, selection).await; + if inline_selection { + self.app_event_tx.send(AppEvent::SettingsSelectionClosed); + } } AppEvent::UpdateApprovalsReviewer(policy) => { if self.reject_pending_permission_change() { @@ -3153,6 +3219,25 @@ impl App { self.chat_widget.add_error_message(err); } } + AppEvent::ResolvePermissionProfile { profile_id } => { + if self.reject_pending_permission_change() { + self.chat_widget + .retain_input_after_failed_permission_selection(); + self.app_event_tx.send(AppEvent::SettingsSelectionClosed); + return Ok(AppRunControl::Continue); + } + #[cfg(any(target_os = "windows", test))] + if self.chat_widget.windows_sandbox_local_server + && self.windows_sandbox_host() != WindowsSandboxHost::Remote + && self.chat_widget.windows_sandbox_config.requirements.is_none() + && !self.refresh_windows_sandbox_config(app_server).await + { + self.app_event_tx.send(AppEvent::SettingsSelectionClosed); + return Ok(AppRunControl::Continue); + } + self.chat_widget + .select_permission_profile_by_id(&profile_id); + } AppEvent::OpenPermissionsPopup | AppEvent::OpenApprovalsPopup => { if self.reject_pending_permission_change() { return Ok(AppRunControl::Continue); diff --git a/codex-rs/tui/src/app/tests/permission_selection_tests.rs b/codex-rs/tui/src/app/tests/permission_selection_tests.rs index d24946fa30..79bf0cf638 100644 --- a/codex-rs/tui/src/app/tests/permission_selection_tests.rs +++ b/codex-rs/tui/src/app/tests/permission_selection_tests.rs @@ -134,6 +134,91 @@ async fn matching_builtin_permission_selection_submits_pending_prompt() -> Resul Ok(()) } +#[tokio::test] +async fn inline_server_permission_selection_holds_queued_prompt_until_confirmation() -> Result<()> { + let (mut app, mut events, mut ops) = make_test_app_with_channels().await; + let home = tempdir()?; + std::fs::write( + home.path().join("config.toml"), + "default_permissions = \":workspace\"\n[permissions.server-only]\nextends = \":read-only\"\n", + )?; + let server_config = ConfigBuilder::default() + .codex_home(home.path().into()) + .build() + .await?; + let mut server = crate::start_embedded_app_server_for_picker(&server_config).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?; + while events.try_recv().is_ok() {} + while ops.try_recv().is_ok() {} + + app.chat_widget + .select_permission_profile_by_id("server-only"); + assert!(app.chat_widget.queue_user_message_with_options( + crate::chatwidget::UserMessage::from("use the selected profile"), + crate::bottom_pane::QueuedInputAction::Plain, + Vec::new(), + )); + let mut tui = crate::tui::test_support::make_test_tui()?; + let mut settings_settled = false; + for _ in 0..20 { + if app + .agents_overview + .requested_permission_profiles + .contains_key(&thread_id) + { + break; + } + let event = tokio::time::timeout(std::time::Duration::from_secs(/*secs*/ 2), events.recv()) + .await? + .expect("app event channel should remain open"); + app.handle_event(&mut tui, &mut server, event).await?; + } + assert!( + app.agents_overview + .requested_permission_profiles + .contains_key(&thread_id) + ); + while let Ok(event) = events.try_recv() { + app.handle_event(&mut tui, &mut server, event).await?; + } + assert!(ops.try_recv().is_err()); + + let settings = next_thread_settings_updated(&mut server, thread_id).await; + app.enqueue_thread_notification( + thread_id, + ServerNotification::ThreadSettingsUpdated(settings), + ) + .await?; + for _ in 0..20 { + let event = tokio::time::timeout(std::time::Duration::from_secs(/*secs*/ 2), events.recv()) + .await? + .expect("app event channel should remain open"); + let settled = matches!(event, AppEvent::SettingsSelectionSettled); + app.handle_event(&mut tui, &mut server, event).await?; + if settled { + settings_settled = true; + break; + } + } + assert!(settings_settled); + + let Op::UserTurn { items, .. } = next_user_turn_op(&mut ops) else { + panic!("expected queued user turn"); + }; + assert_eq!( + items, + vec![AppServerUserInput::Text { + text: "use the selected profile".to_string(), + text_elements: Vec::new(), + }] + ); + server.shutdown().await?; + Ok(()) +} + #[tokio::test] async fn custom_permission_selection_uses_server_definition_and_preserves_state_on_rejection() -> Result<()> { @@ -212,7 +297,7 @@ async fn custom_permission_selection_uses_server_definition_and_preserves_state_ ); std::fs::write( home.path().join("config.toml"), - "default_permissions = \":workspace\"\n[permissions.shared]\nextends = \":read-only\"\n", + "default_permissions = \":workspace\"\n[permissions.shared]\nextends = \":read-only\"\n[permissions.other]\nextends = \":read-only\"\n", )?; app.select_permission_profile( &mut server, @@ -240,6 +325,42 @@ async fn custom_permission_selection_uses_server_definition_and_preserves_state_ app.select_permission_profile(&mut server, selection.clone()) .await; assert!(!app.reject_pending_permission_change()); + assert!(app.agents_overview.requested_permission_profiles.is_empty()); + // Another client's policy change must not leave a no-op request's old policy behind. + server + .thread_settings_update(ThreadSettingsUpdateParams { + thread_id: thread_id.to_string(), + approval_policy: Some(AskForApproval::OnRequest), + ..Default::default() + }) + .await?; + let settings = next_thread_settings_updated(&mut server, thread_id).await; + app.enqueue_thread_notification( + thread_id, + ServerNotification::ThreadSettingsUpdated(settings), + ) + .await?; + app.select_permission_profile( + &mut server, + PermissionProfileSelection { + profile_id: "other".into(), + approval_policy: None, + approvals_reviewer: None, + display_label: "other".into(), + }, + ) + .await; + assert_eq!( + app.agents_overview.requested_permission_profiles[&thread_id].approval_policy, + Some(AskForApproval::OnRequest) + ); + let settings = next_thread_settings_updated(&mut server, thread_id).await; + app.enqueue_thread_notification( + thread_id, + ServerNotification::ThreadSettingsUpdated(settings), + ) + .await?; + assert!(app.agents_overview.requested_permission_profiles.is_empty()); let confirmed = RuntimePermissionProfileOverride::from_config(app.chat_widget.config_ref()); let confirmed_config = app.config.clone(); app.select_permission_profile( @@ -261,7 +382,7 @@ async fn custom_permission_selection_uses_server_definition_and_preserves_state_ .selected_permission_profiles .get(&thread_id) .map(String::as_str), - Some("shared") + Some("other") ); // Materialize the task so /fork exercises persisted history with the confirmed profile. server.thread_inject_items(thread_id, vec![serde_json::from_value(serde_json::json!({ @@ -269,10 +390,6 @@ async fn custom_permission_selection_uses_server_definition_and_preserves_state_ }))?]).await?; let mut tui = crate::tui::test_support::make_test_tui()?; while events.try_recv().is_ok() {} - app.change_working_directory(&mut tui, &mut server, client_home.path().abs()) - .await; - assert_eq!(app.chat_widget.thread_id(), Some(thread_id)); - insta::assert_snapshot!(next_history_message(&mut events), @"■ Changing directories with an unconfirmed named profile is not supported."); let side = server .fork_side_thread( &app.local_settings, @@ -288,10 +405,10 @@ async fn custom_permission_selection_uses_server_definition_and_preserves_state_ side.session.permission_profile, side.session.approval_policy ), - (PermissionProfile::read_only(), AskForApproval::Never) + (PermissionProfile::read_only(), AskForApproval::OnRequest) ); app.cli_kv_overrides.push(( - "permissions.shared.extends".into(), + "permissions.other.extends".into(), TomlValue::String(":danger-full-access".into()), )); // Both the accepted request and its confirmed descendant remain server-owned. diff --git a/codex-rs/tui/src/app/tests/startup.rs b/codex-rs/tui/src/app/tests/startup.rs index 196ff1ab16..a4c5a256a1 100644 --- a/codex-rs/tui/src/app/tests/startup.rs +++ b/codex-rs/tui/src/app/tests/startup.rs @@ -140,7 +140,7 @@ async fn windows_sandbox_setup_completion_requires_matching_pending_mode() -> Re let (mut app, mut events, _ops) = make_test_app_with_channels().await; app.chat_widget.windows_sandbox_local_server = true; while events.try_recv().is_ok() {} - let app_server = + let mut app_server = crate::start_embedded_app_server_for_picker(app.chat_widget.config_ref()).await?; let preset = builtin_approval_presets() .into_iter() @@ -148,6 +148,19 @@ async fn windows_sandbox_setup_completion_requires_matching_pending_mode() -> Re .expect("auto preset"); app.windows_sandbox.pending_setup = Some((WindowsSandboxEnableMode::Elevated, preset, None)); app.windows_sandbox.setup_started_at = Some(Instant::now()); + app.chat_widget + .select_permission_profile_by_id(":workspace"); + let AppEvent::FetchPermissionProfiles { request_id, .. } = events.try_recv()? else { + panic!("expected profile discovery"); + }; + app.chat_widget.on_permission_profiles_loaded( + request_id, + Ok(crate::permission_discovery::PermissionDiscovery::local( + app.chat_widget.config_ref(), + )), + ); + while events.try_recv().is_ok() {} + let mut tui = crate::tui::test_support::make_test_tui()?; for mode in [ WindowsSandboxSetupMode::Unelevated, @@ -166,10 +179,7 @@ async fn windows_sandbox_setup_completion_requires_matching_pending_mode() -> Re )), ) .await; - assert_eq!( - app.windows_sandbox.pending_setup.is_some(), - mode == WindowsSandboxSetupMode::Unelevated - ); + assert!(app.windows_sandbox.pending_setup.is_some()); } assert!(!app.chat_widget.windows_sandbox_elevated_setup_complete); assert!(app.windows_sandbox_blocks_thread_switch()); @@ -182,6 +192,14 @@ async fn windows_sandbox_setup_completion_requires_matching_pending_mode() -> Re )); assert!(events.try_recv().is_err()); + // Completion can overtake this close event; selection still belongs to the queued handler. + app.handle_event(&mut tui, &mut app_server, AppEvent::SettingsSelectionClosed) + .await?; + assert!( + app.chat_widget + .inline_permission_profile_selection_pending() + ); + app_server.shutdown().await?; Ok(()) } @@ -1932,9 +1950,21 @@ async fn windows_sandbox_config_refresh_uses_connected_server() -> Result<()> { let mut tui = crate::tui::test_support::make_test_tui()?; app.chat_widget.windows_sandbox_local_server = true; app.chat_widget.windows_sandbox_host = WindowsSandboxHost::Local; - Box::pin(app.handle_event(&mut tui, &mut recovered, AppEvent::OpenPermissionsPopup)).await?; + while events.try_recv().is_ok() {} + Box::pin(app.handle_event( + &mut tui, + &mut recovered, + AppEvent::ResolvePermissionProfile { + profile_id: codex_protocol::models::BUILT_IN_PERMISSION_PROFILE_READ_ONLY.to_string(), + }, + )) + .await?; assert_eq!(app.chat_widget.windows_sandbox_config, loaded_config); assert!(app.chat_widget.has_active_view()); + assert!(matches!( + events.try_recv(), + Ok(AppEvent::FetchPermissionProfiles { .. }) + )); recovered.shutdown().await?; server.shutdown().await?; Ok(()) diff --git a/codex-rs/tui/src/app/thread_routing.rs b/codex-rs/tui/src/app/thread_routing.rs index cb2d4e6387..2ace03d211 100644 --- a/codex-rs/tui/src/app/thread_routing.rs +++ b/codex-rs/tui/src/app/thread_routing.rs @@ -1144,12 +1144,25 @@ impl App { { self.chat_widget .on_thread_settings_updated(settings.clone()); + if confirmed_profile.is_some() + || settings + .thread_settings + .active_permission_profile + .as_ref() + .is_some_and(|profile| { + self.agents_overview + .selected_permission_profiles + .get(&thread_id) + == Some(&profile.id) + }) + { + // Also adopt changed custom definitions and settings from another client + // after a no-op re-selection that needed no pending request. + self.adopt_server_permissions(); + } notification = None; } if confirmed_profile.is_some() { - if self.chat_widget.thread_id() == Some(thread_id) { - self.adopt_server_permissions(); - } self.app_event_tx.send(AppEvent::SettingsSelectionSettled); } diff --git a/codex-rs/tui/src/app_event.rs b/codex-rs/tui/src/app_event.rs index c19c77f3c6..e2b86c9c1e 100644 --- a/codex-rs/tui/src/app_event.rs +++ b/codex-rs/tui/src/app_event.rs @@ -1560,6 +1560,11 @@ pub(crate) enum AppEvent { /// Re-open the permissions presets popup. OpenPermissionsPopup, + /// Resolve an inline permission profile after any app-owned platform preflight. + ResolvePermissionProfile { + profile_id: String, + }, + /// Open the branch picker option from the review popup. OpenReviewBranchPicker(PathBuf), diff --git a/codex-rs/tui/src/chatwidget.rs b/codex-rs/tui/src/chatwidget.rs index d28be65c8f..f6d4c16260 100644 --- a/codex-rs/tui/src/chatwidget.rs +++ b/codex-rs/tui/src/chatwidget.rs @@ -544,6 +544,8 @@ pub(crate) struct ChatWidget { model_popup_request_id: Option, permission_popup_request_id: Option, permission_discovery: Option, + inline_permission_profile_selection: + 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 185a10ae2a..b0a12a3d26 100644 --- a/codex-rs/tui/src/chatwidget/constructor.rs +++ b/codex-rs/tui/src/chatwidget/constructor.rs @@ -130,6 +130,7 @@ impl ChatWidget { model_popup_request_id: None, permission_popup_request_id: None, permission_discovery: None, + inline_permission_profile_selection: 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/input_flow.rs b/codex-rs/tui/src/chatwidget/input_flow.rs index 6a089a0039..ce5da8c93b 100644 --- a/codex-rs/tui/src/chatwidget/input_flow.rs +++ b/codex-rs/tui/src/chatwidget/input_flow.rs @@ -125,6 +125,12 @@ impl ChatWidget { } pub(super) fn on_modal_or_popup_closed(&mut self) { + if self.apply_pending_permission_profile() { + return; + } + if self.cancel_pending_permission_profile_selection() { + return; + } if self.input_queue.suppress_queue_autosend { self.app_event_tx.send(AppEvent::SettingsSelectionClosed); } else { diff --git a/codex-rs/tui/src/chatwidget/permission_discovery.rs b/codex-rs/tui/src/chatwidget/permission_discovery.rs index 9fa0b2a6a2..4d6f8c9e10 100644 --- a/codex-rs/tui/src/chatwidget/permission_discovery.rs +++ b/codex-rs/tui/src/chatwidget/permission_discovery.rs @@ -5,7 +5,22 @@ use crate::permission_discovery::PermissionDiscovery; pub(super) const VIEW_ID: &str = "permission-profiles"; +pub(super) enum InlinePermissionProfileSelection { + Discovering(String), + Applying, +} + impl ChatWidget { + pub(crate) fn select_permission_profile_by_id(&mut self, profile_id: &str) { + self.set_queue_autosend_suppressed(/*suppressed*/ true); + self.inline_permission_profile_selection = Some( + InlinePermissionProfileSelection::Discovering(profile_id.to_string()), + ); + if !self.apply_pending_permission_profile() { + self.request_permission_profiles(); + } + } + pub(crate) fn retain_input_after_failed_permission_selection(&mut self) { self.input_queue.recovered_queue |= self.input_queue.has_queued_follow_up_messages() || !self.input_queue.pending_steers.is_empty(); @@ -13,6 +28,54 @@ impl ChatWidget { self.restore_user_message_to_composer(message); } } + + pub(super) fn apply_pending_permission_profile(&mut self) -> bool { + let Some(discovery) = self.permission_discovery.clone() else { + return false; + }; + let Some(profile_id) = self.pending_permission_profile_id().map(str::to_owned) else { + return false; + }; + self.inline_permission_profile_selection = Some(InlinePermissionProfileSelection::Applying); + if !self.try_select_permission_profile_by_id(&discovery, &profile_id) { + self.recover_inline_permission_profile_selection(); + } + self.app_event_tx.send(AppEvent::SettingsSelectionClosed); + true + } + + pub(super) fn cancel_pending_permission_profile_selection(&mut self) -> bool { + if self.pending_permission_profile_id().is_none() { + return false; + } + self.recover_inline_permission_profile_selection(); + self.app_event_tx.send(AppEvent::SettingsSelectionClosed); + true + } + + pub(crate) fn complete_inline_permission_profile_selection(&mut self) -> bool { + self.inline_permission_profile_selection.take().is_some() + } + + pub(crate) fn recover_inline_permission_profile_selection(&mut self) -> bool { + if !self.complete_inline_permission_profile_selection() { + return false; + } + self.retain_input_after_failed_permission_selection(); + true + } + + pub(crate) fn inline_permission_profile_selection_pending(&self) -> bool { + self.inline_permission_profile_selection.is_some() + } + + fn pending_permission_profile_id(&self) -> Option<&str> { + match self.inline_permission_profile_selection.as_ref() { + Some(InlinePermissionProfileSelection::Discovering(profile_id)) => Some(profile_id), + Some(InlinePermissionProfileSelection::Applying) | None => None, + } + } + pub(crate) fn request_permission_profiles(&mut self) { self.bottom_pane.dismiss_view_by_id(VIEW_ID); if let Some(discovery) = self.permission_discovery.clone() { @@ -44,6 +107,9 @@ impl ChatWidget { pub(super) fn invalidate_permission_discovery(&mut self) { self.permission_popup_request_id = None; self.bottom_pane.dismiss_view_by_id(VIEW_ID); + if self.recover_inline_permission_profile_selection() { + self.app_event_tx.send(AppEvent::SettingsSelectionClosed); + } } pub(crate) fn permission_popup_request_is_current(&self, request_id: uuid::Uuid) -> bool { @@ -63,26 +129,46 @@ impl ChatWidget { 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); + let was_covered = self.bottom_pane.dismiss_view_by_id(VIEW_ID); + if was_covered && result.is_ok() { + return; + } + if let Err(message) = result + && self.pending_permission_profile_id().is_some() + { + self.add_error_message(message); + } + self.cancel_pending_permission_profile_selection(); return; } match result { Ok(discovery) => { - self.permission_profiles_menu_opened = true; - self.open_permission_profiles_popup(discovery); + if !self.apply_pending_permission_profile() { + self.permission_profiles_menu_opened = true; + self.open_permission_profiles_popup(discovery); + } + } + Err(message) => { + let retry_profile_id = self.pending_permission_profile_id().map(str::to_owned); + self.bottom_pane.show_selection_view(SelectionViewParams { + view_id: Some(VIEW_ID), + title: Some("Update Model Permissions".to_string()), + subtitle: Some(message), + items: vec![SelectionItem { + name: "Retry".to_string(), + actions: vec![Box::new(move |tx| { + if let Some(profile_id) = retry_profile_id.clone() { + tx.send(AppEvent::ResolvePermissionProfile { profile_id }); + } else { + tx.send(AppEvent::OpenPermissionsPopup); + } + })], + dismiss_on_select: false, + ..Default::default() + }], + ..SelectionViewParams::picker() + }); } - Err(message) => self.bottom_pane.show_selection_view(SelectionViewParams { - view_id: Some(VIEW_ID), - title: Some("Update Model Permissions".to_string()), - subtitle: Some(message), - items: vec![SelectionItem { - name: "Retry".to_string(), - actions: vec![Box::new(|tx| tx.send(AppEvent::OpenPermissionsPopup))], - dismiss_on_select: true, - ..Default::default() - }], - ..SelectionViewParams::picker() - }), } } } diff --git a/codex-rs/tui/src/chatwidget/permissions_menu.rs b/codex-rs/tui/src/chatwidget/permissions_menu.rs index 4e2fcb6b38..eac72ab9fa 100644 --- a/codex-rs/tui/src/chatwidget/permissions_menu.rs +++ b/codex-rs/tui/src/chatwidget/permissions_menu.rs @@ -3,6 +3,7 @@ use super::*; use crate::permission_discovery::PermissionDiscovery; use codex_protocol::models::BUILT_IN_PERMISSION_PROFILE_DANGER_FULL_ACCESS; +use codex_protocol::models::BUILT_IN_PERMISSION_PROFILE_WORKSPACE; pub(super) fn permission_preset_description(preset: &ApprovalPreset) -> &'static str { match preset.id { @@ -49,6 +50,89 @@ pub(crate) fn cyber_model_approval_reviewer(config: &Config) -> Option bool { + let reviewer = match profile_id { + ":approve-for-me" if !self.config.features.enabled(Feature::GuardianApproval) => { + self.add_error_message( + "Approve for me is unavailable. Run `/permissions` to see available profiles." + .into(), + ); + return false; + } + ":approve-for-me" => ApprovalsReviewer::AutoReview, + _ => ApprovalsReviewer::User, + }; + let profile_id = match profile_id { + ":ask-for-approval" | ":approve-for-me" => BUILT_IN_PERMISSION_PROFILE_WORKSPACE, + id => id, + }; + let Some(profile) = discovery + .profiles + .iter() + .find(|profile| profile.id == profile_id) + else { + self.add_error_message(format!( + "Unknown permission profile `{profile_id}`. Run `/permissions` to see available profiles." + )); + return false; + }; + let preset = builtin_approval_presets() + .into_iter() + .find(|preset| preset.active_permission_profile.id == profile_id); + let approval_policy = preset + .as_ref() + .map(|preset| AskForApproval::from(preset.approval)); + let approvals_reviewer = preset.as_ref().map(|_| reviewer); + if let Some(reason) = discovery.disabled_reason( + profile_id, + approval_policy, + approvals_reviewer.map(Into::into), + ) { + self.add_error_message(format!( + "Permission profile `{profile_id}` is unavailable: {reason}." + )); + return false; + } + let display_label = + preset + .as_ref() + .map_or(profile.id.as_str(), |preset| match (preset.id, reviewer) { + ("auto", ApprovalsReviewer::AutoReview) => APPROVE_FOR_ME_LABEL, + ("auto", ApprovalsReviewer::User) => ASK_FOR_APPROVAL_LABEL, + _ => preset.label, + }); + let selection = PermissionProfileSelection { + profile_id: profile_id.to_string(), + approval_policy, + approvals_reviewer, + display_label: display_label.to_string(), + }; + if let Some(preset) = preset { + let Some(action) = self + .permission_mode_actions( + &preset, + display_label.to_string(), + reviewer, + /*profile_selection*/ Some(selection), + /*return_to_permissions*/ true, + ) + .into_iter() + .next() + else { + return false; + }; + action(&self.app_event_tx); + return true; + } + self.app_event_tx + .send(AppEvent::SelectPermissionProfile(selection)); + true + } + 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/slash_dispatch.rs b/codex-rs/tui/src/chatwidget/slash_dispatch.rs index d6f9ee55e3..1ec8e08fa4 100644 --- a/codex-rs/tui/src/chatwidget/slash_dispatch.rs +++ b/codex-rs/tui/src/chatwidget/slash_dispatch.rs @@ -806,12 +806,26 @@ impl ChatWidget { source, } = prepared; let trimmed = args.trim(); + if trimmed.is_empty() { + self.dispatch_command_from_source(cmd, source); + return; + } match cmd { SlashCommand::Model if !trimmed.is_empty() => { self.handle_model_command_args(trimmed); self.app_event_tx.send(AppEvent::SettingsSelectionClosed); } - SlashCommand::Export if trimmed.is_empty() => self.show_transcript_export_popup(), + SlashCommand::Permissions + if self.windows_sandbox_local_server + && self.windows_sandbox_host != WindowsSandboxHost::Remote + && self.windows_sandbox_config.requirements.is_none() => + { + self.set_queue_autosend_suppressed(/*suppressed*/ true); + self.app_event_tx.send(AppEvent::ResolvePermissionProfile { + profile_id: trimmed.to_string(), + }); + } + SlashCommand::Permissions => self.select_permission_profile_by_id(trimmed), SlashCommand::Export => { self.set_queue_autosend_suppressed(/*suppressed*/ true); self.app_event_tx.send(AppEvent::ExportTranscript { diff --git a/codex-rs/tui/src/chatwidget/tests/permissions.rs b/codex-rs/tui/src/chatwidget/tests/permissions.rs index a08388f58d..221c58e3cb 100644 --- a/codex-rs/tui/src/chatwidget/tests/permissions.rs +++ b/codex-rs/tui/src/chatwidget/tests/permissions.rs @@ -3,6 +3,7 @@ use crate::legacy_core::config::PermissionProfileCatalogEntry; use crate::permission_discovery::PermissionDiscovery as Discovery; use codex_protocol::models::ActivePermissionProfile; use codex_protocol::models::BUILT_IN_PERMISSION_PROFILE_DANGER_FULL_ACCESS; +use codex_protocol::models::BUILT_IN_PERMISSION_PROFILE_READ_ONLY; use codex_protocol::models::ManagedFileSystemPermissions; use codex_protocol::permissions::FileSystemAccessMode; use codex_protocol::permissions::FileSystemPath; @@ -87,6 +88,208 @@ async fn permission_discovery_uses_server_catalog_for_remote_custom_selection() )); } +#[tokio::test] +async fn permissions_command_waits_for_covering_modal_before_selecting_profile() { + let (mut chat, mut events, _op_rx) = make_chatwidget_manual(/*model_override*/ None).await; + chat.thread_id = Some(ThreadId::new()); + chat.select_permission_profile_by_id(BUILT_IN_PERMISSION_PROFILE_READ_ONLY); + let request_id = assert_matches!( + events.try_recv(), + Ok(AppEvent::FetchPermissionProfiles { request_id, .. }) => request_id + ); + chat.bottom_pane.show_selection_view(SelectionViewParams { + view_id: Some("other-modal"), + title: Some("Other modal".to_string()), + ..Default::default() + }); + + chat.on_permission_profiles_loaded(request_id, Ok(Discovery::local(&chat.config))); + + assert_eq!(chat.bottom_pane.active_view_id(), Some("other-modal")); + assert!(events.try_recv().is_err()); + + chat.handle_key_event(KeyEvent::from(KeyCode::Esc)); + + assert_matches!( + events.try_recv(), + Ok(AppEvent::SelectPermissionProfile(PermissionProfileSelection { profile_id, .. })) + if profile_id == BUILT_IN_PERMISSION_PROFILE_READ_ONLY + ); + assert_matches!(events.try_recv(), Ok(AppEvent::SettingsSelectionClosed)); +} + +#[tokio::test] +async fn cancelled_permissions_command_does_not_apply_after_later_modal() { + let (mut chat, mut events, _op_rx) = make_chatwidget_manual(/*model_override*/ None).await; + chat.thread_id = Some(ThreadId::new()); + chat.input_queue + .queued_user_messages + .push_back(UserMessage::from("follow-up").into()); + chat.select_permission_profile_by_id(BUILT_IN_PERMISSION_PROFILE_READ_ONLY); + let request_id = assert_matches!( + events.try_recv(), + Ok(AppEvent::FetchPermissionProfiles { request_id, .. }) => request_id + ); + chat.handle_key_event(KeyEvent::from(KeyCode::Esc)); + assert_matches!(events.try_recv(), Ok(AppEvent::SettingsSelectionClosed)); + assert!(chat.input_queue.recovered_queue); + + chat.on_permission_profiles_loaded(request_id, Ok(Discovery::local(&chat.config))); + chat.bottom_pane.show_selection_view(SelectionViewParams { + view_id: Some("other-modal"), + title: Some("Other modal".to_string()), + ..Default::default() + }); + chat.handle_key_event(KeyEvent::from(KeyCode::Esc)); + + assert_matches!(events.try_recv(), Ok(AppEvent::SettingsSelectionClosed)); + assert!( + events.try_recv().is_err(), + "cancelled profile must not apply" + ); +} + +#[tokio::test] +async fn permissions_command_preserves_full_access_confirmation() { + let (mut chat, mut events, _op_rx) = make_chatwidget_manual(/*model_override*/ None).await; + chat.thread_id = Some(ThreadId::new()); + chat.permission_discovery = Some(Discovery::local(&chat.config)); + chat.input_queue + .queued_user_messages + .push_back(UserMessage::from("follow-up").into()); + + chat.dispatch_command_with_args( + SlashCommand::Permissions, + BUILT_IN_PERMISSION_PROFILE_DANGER_FULL_ACCESS.to_string(), + Vec::new(), + ); + + let (preset, return_to_permissions, profile_selection) = assert_matches!( + events.try_recv(), + Ok(AppEvent::OpenFullAccessConfirmation { + preset, + return_to_permissions, + profile_selection: Some(profile_selection), + }) if preset.id == "full-access" + && profile_selection.profile_id == BUILT_IN_PERMISSION_PROFILE_DANGER_FULL_ACCESS + => (preset, return_to_permissions, Some(profile_selection)) + ); + assert_matches!(events.try_recv(), Ok(AppEvent::SettingsSelectionClosed)); + chat.open_full_access_confirmation(preset, return_to_permissions, profile_selection); + + // Cancel returns to the picker; closing that picker must still recover the queue. + chat.handle_key_event(KeyEvent::from(KeyCode::Down)); + chat.handle_key_event(KeyEvent::from(KeyCode::Enter)); + assert_matches!(events.try_recv(), Ok(AppEvent::OpenPermissionsPopup)); + chat.open_permissions_popup(); + assert_matches!(events.try_recv(), Ok(AppEvent::SettingsSelectionClosed)); + assert!(chat.inline_permission_profile_selection_pending()); + chat.handle_key_event(KeyEvent::from(KeyCode::Esc)); + + assert_matches!(events.try_recv(), Ok(AppEvent::SettingsSelectionClosed)); + assert!(chat.inline_permission_profile_selection_pending()); + assert!(chat.recover_inline_permission_profile_selection()); + assert!(chat.input_queue.recovered_queue); + assert!(events.try_recv().is_err()); +} + +#[tokio::test] +async fn permissions_command_rejects_disabled_profile() { + let (mut chat, mut events, _op_rx) = make_chatwidget_manual(/*model_override*/ None).await; + let mut discovery = Discovery::local(&chat.config); + discovery.requirements = Some( + serde_json::from_value(serde_json::json!({ + "allowedPermissionProfiles": {BUILT_IN_PERMISSION_PROFILE_READ_ONLY: false} + })) + .unwrap(), + ); + chat.permission_discovery = Some(discovery); + chat.input_queue + .queued_user_messages + .push_back(UserMessage::from("follow-up").into()); + + chat.dispatch_command_with_args( + SlashCommand::Permissions, + BUILT_IN_PERMISSION_PROFILE_READ_ONLY.to_string(), + Vec::new(), + ); + + let cell = assert_matches!(events.try_recv(), Ok(AppEvent::InsertHistoryCell(cell)) => cell); + insta::assert_snapshot!( + "permissions_command_disabled_profile", + lines_to_single_string(&cell.display_lines(/*width*/ 80)) + ); + assert_matches!(events.try_recv(), Ok(AppEvent::SettingsSelectionClosed)); + assert!(chat.input_queue.recovered_queue); +} + +#[tokio::test] +async fn permissions_command_rejects_unknown_profile() { + let (mut chat, mut events, _op_rx) = make_chatwidget_manual(/*model_override*/ None).await; + chat.permission_discovery = Some(Discovery::local(&chat.config)); + + chat.dispatch_command_with_args( + SlashCommand::Permissions, + "unknown-profile".to_string(), + Vec::new(), + ); + + let cell = assert_matches!(events.try_recv(), Ok(AppEvent::InsertHistoryCell(cell)) => cell); + insta::assert_snapshot!( + "permissions_command_unknown_profile", + lines_to_single_string(&cell.display_lines(/*width*/ 80)) + ); + assert_matches!(events.try_recv(), Ok(AppEvent::SettingsSelectionClosed)); +} + +#[tokio::test] +async fn invalidating_inline_permission_discovery_recovers_queued_input() { + let (mut chat, mut events, _op_rx) = make_chatwidget_manual(/*model_override*/ None).await; + chat.thread_id = Some(ThreadId::new()); + chat.input_queue + .queued_user_messages + .push_back(UserMessage::from("follow-up").into()); + chat.select_permission_profile_by_id(BUILT_IN_PERMISSION_PROFILE_READ_ONLY); + let request_id = assert_matches!( + events.try_recv(), + Ok(AppEvent::FetchPermissionProfiles { request_id, .. }) => request_id + ); + + chat.invalidate_permission_discovery(); + + assert_matches!(events.try_recv(), Ok(AppEvent::SettingsSelectionClosed)); + assert!(chat.input_queue.recovered_queue); + chat.on_permission_profiles_loaded(request_id, Ok(Discovery::local(&chat.config))); + assert!(events.try_recv().is_err()); +} + +#[tokio::test] +async fn inline_permission_discovery_retry_preserves_profile_id() { + let (mut chat, mut events, _op_rx) = make_chatwidget_manual(/*model_override*/ None).await; + chat.thread_id = Some(ThreadId::new()); + chat.select_permission_profile_by_id("server-only"); + let request_id = assert_matches!( + events.try_recv(), + Ok(AppEvent::FetchPermissionProfiles { request_id, .. }) => request_id + ); + chat.on_permission_profiles_loaded( + request_id, + Err("Failed to load permissions: disconnected".to_string()), + ); + + chat.handle_key_event(KeyEvent::from(KeyCode::Enter)); + + assert_matches!( + events.try_recv(), + Ok(AppEvent::ResolvePermissionProfile { profile_id }) if profile_id == "server-only" + ); + assert_eq!( + chat.bottom_pane.active_view_id(), + Some("permission-profiles") + ); + assert!(events.try_recv().is_err()); +} + #[tokio::test] async fn permission_discovery_discards_stale_results_and_preserves_covering_modal() { let (mut chat, _rx, _op_rx) = make_chatwidget_manual(/*model_override*/ None).await; @@ -331,15 +534,26 @@ async fn profile_permissions_selection_emits_named_profile_event_only() { let events = std::iter::from_fn(|| rx.try_recv().ok()).collect::>(); assert_eq!(events.len(), 1); - assert!(matches!( - &events[0], - AppEvent::SelectPermissionProfile(PermissionProfileSelection { - profile_id, - approval_policy: Some(AskForApproval::OnRequest), - approvals_reviewer: Some(ApprovalsReviewer::User), - display_label, - }) if profile_id == ":workspace" && display_label == "Ask for approval" - )); + assert!( + chat.try_select_permission_profile_by_id( + &Discovery::local(&chat.config), + ":ask-for-approval", + ) + ); + for event in events + .into_iter() + .chain(std::iter::from_fn(|| rx.try_recv().ok())) + { + assert!(matches!( + event, + AppEvent::SelectPermissionProfile(PermissionProfileSelection { + profile_id, + approval_policy: Some(AskForApproval::OnRequest), + approvals_reviewer: Some(ApprovalsReviewer::User), + display_label, + }) if profile_id == ":workspace" && display_label == "Ask for approval" + )); + } } #[tokio::test] @@ -397,15 +611,26 @@ async fn profile_permissions_selection_emits_auto_review_mode_event() { let events = std::iter::from_fn(|| rx.try_recv().ok()).collect::>(); assert_eq!(events.len(), 1); - assert!(matches!( - &events[0], - AppEvent::SelectPermissionProfile(PermissionProfileSelection { - profile_id, - approval_policy: Some(AskForApproval::OnRequest), - approvals_reviewer: Some(ApprovalsReviewer::AutoReview), - display_label, - }) if profile_id == ":workspace" && display_label == "Approve for me" - )); + assert!( + chat.try_select_permission_profile_by_id( + &Discovery::local(&chat.config), + ":approve-for-me", + ) + ); + for event in events + .into_iter() + .chain(std::iter::from_fn(|| rx.try_recv().ok())) + { + assert!(matches!( + event, + AppEvent::SelectPermissionProfile(PermissionProfileSelection { + profile_id, + approval_policy: Some(AskForApproval::OnRequest), + approvals_reviewer: Some(ApprovalsReviewer::AutoReview), + display_label, + }) if profile_id == ":workspace" && display_label == "Approve for me" + )); + } } #[tokio::test] @@ -1199,7 +1424,7 @@ async fn permissions_selection_emits_history_cell_when_current_is_selected() { #[tokio::test] async fn permissions_selection_hides_auto_review_when_feature_disabled() { - let (mut chat, _rx, _op_rx) = make_chatwidget_manual(/*model_override*/ None).await; + let (mut chat, mut rx, _op_rx) = make_chatwidget_manual(/*model_override*/ None).await; #[cfg(target_os = "windows")] { chat.set_windows_sandbox_mode(Some(WindowsSandboxSetupMode::Unelevated)); @@ -1212,6 +1437,14 @@ async fn permissions_selection_hides_auto_review_when_feature_disabled() { !popup.contains("Approve for me"), "expected Approve for me to stay hidden until the feature is enabled: {popup}" ); + assert!( + !chat.try_select_permission_profile_by_id( + &Discovery::local(&chat.config), + ":approve-for-me", + ) + ); + let cell = assert_matches!(rx.try_recv(), Ok(AppEvent::InsertHistoryCell(cell)) => cell); + insta::assert_snapshot!(lines_to_single_string(&cell.display_lines(/*width*/ 120)), @"■ Approve for me is unavailable. Run `/permissions` to see available profiles."); } #[tokio::test] diff --git a/codex-rs/tui/src/chatwidget/tests/slash_commands.rs b/codex-rs/tui/src/chatwidget/tests/slash_commands.rs index 49601e3ccd..38716fdeb9 100644 --- a/codex-rs/tui/src/chatwidget/tests/slash_commands.rs +++ b/codex-rs/tui/src/chatwidget/tests/slash_commands.rs @@ -456,7 +456,7 @@ async fn queued_slash_menu_cancel_drains_next_input() { ) .await; assert_cancelled_queued_menu_drains_next_input( - "/permissions", + "/permissions ", "Update Model Permissions", KeyEvent::new(KeyCode::Char('c'), KeyModifiers::CONTROL), ) diff --git a/codex-rs/tui/src/chatwidget/tests/snapshots/codex_tui__chatwidget__tests__permissions__permissions_command_disabled_profile.snap b/codex-rs/tui/src/chatwidget/tests/snapshots/codex_tui__chatwidget__tests__permissions__permissions_command_disabled_profile.snap new file mode 100644 index 0000000000..22e4146b3a --- /dev/null +++ b/codex-rs/tui/src/chatwidget/tests/snapshots/codex_tui__chatwidget__tests__permissions__permissions_command_disabled_profile.snap @@ -0,0 +1,6 @@ +--- +source: tui/src/chatwidget/tests/permissions.rs +assertion_line: 168 +expression: lines_to_single_string(&cell.display_lines(80)) +--- +■ Permission profile `:read-only` is unavailable: Disabled by requirements. diff --git a/codex-rs/tui/src/chatwidget/tests/snapshots/codex_tui__chatwidget__tests__permissions__permissions_command_unknown_profile.snap b/codex-rs/tui/src/chatwidget/tests/snapshots/codex_tui__chatwidget__tests__permissions__permissions_command_unknown_profile.snap new file mode 100644 index 0000000000..1835b5d3e9 --- /dev/null +++ b/codex-rs/tui/src/chatwidget/tests/snapshots/codex_tui__chatwidget__tests__permissions__permissions_command_unknown_profile.snap @@ -0,0 +1,5 @@ +--- +source: tui/src/chatwidget/tests/permissions.rs +expression: lines_to_single_string(&cell.display_lines(80)) +--- +■ Unknown permission profile `unknown-profile`. Run `/permissions` to see available profiles. diff --git a/codex-rs/tui/src/slash_command.rs b/codex-rs/tui/src/slash_command.rs index d7b0e99d06..223bd3048b 100644 --- a/codex-rs/tui/src/slash_command.rs +++ b/codex-rs/tui/src/slash_command.rs @@ -174,6 +174,7 @@ impl SlashCommand { self, SlashCommand::Review | SlashCommand::Model + | SlashCommand::Permissions | SlashCommand::Rename | SlashCommand::New | SlashCommand::Clear