From 08b5e96678331cb75a124d552d6e6c2bc5813a3a Mon Sep 17 00:00:00 2001 From: Eric Traut Date: Wed, 22 Apr 2026 22:40:35 -0700 Subject: [PATCH] TUI: preserve permission state after side conversations (#18924) Addresses #18854 ## Why The `/permissions` selector updates the active TUI session state, but the cached session snapshot used when replaying a thread could still contain the old approval or sandbox settings. After opening and leaving `/side`, the main thread replay could restore those stale settings into the `ChatWidget`, so the UI and the next submitted turn could fall back to the old permission mode. ## What - Sync the active thread's cached `ThreadSessionState` whenever approval policy, sandbox policy, or approval reviewer changes. ## Verification Confirmed bug prior to fix and correct behavior after fix. --- codex-rs/tui/src/app/config_persistence.rs | 2 + codex-rs/tui/src/app/event_dispatch.rs | 6 + codex-rs/tui/src/app/thread_session_state.rs | 143 +++++++++++++++++++ 3 files changed, 151 insertions(+) diff --git a/codex-rs/tui/src/app/config_persistence.rs b/codex-rs/tui/src/app/config_persistence.rs index 1ea89d884..7ed3b2976 100644 --- a/codex-rs/tui/src/app/config_persistence.rs +++ b/codex-rs/tui/src/app/config_persistence.rs @@ -311,6 +311,8 @@ impl App { || approvals_reviewer_override.is_some() || sandbox_policy_override.is_some() { + self.sync_active_thread_permission_settings_to_cached_session() + .await; // This uses `OverrideTurnContext` intentionally: toggling the // experiment should update the active thread's effective approval // settings immediately, just like a `/approvals` selection. Without diff --git a/codex-rs/tui/src/app/event_dispatch.rs b/codex-rs/tui/src/app/event_dispatch.rs index 40f440814..2b9e3c91b 100644 --- a/codex-rs/tui/src/app/event_dispatch.rs +++ b/codex-rs/tui/src/app/event_dispatch.rs @@ -1134,6 +1134,8 @@ impl App { Some(self.config.permissions.approval_policy.value()); self.chat_widget .set_approval_policy(self.config.permissions.approval_policy.value()); + self.sync_active_thread_permission_settings_to_cached_session() + .await; } AppEvent::UpdateSandboxPolicy(policy) => { #[cfg(target_os = "windows")] @@ -1162,6 +1164,8 @@ impl App { } self.runtime_sandbox_policy_override = Some(self.config.permissions.sandbox_policy.get().clone()); + self.sync_active_thread_permission_settings_to_cached_session() + .await; // If sandbox policy becomes workspace-write or read-only, run the Windows world-writable scan. #[cfg(target_os = "windows")] @@ -1196,6 +1200,8 @@ impl App { AppEvent::UpdateApprovalsReviewer(policy) => { self.config.approvals_reviewer = policy; self.chat_widget.set_approvals_reviewer(policy); + self.sync_active_thread_permission_settings_to_cached_session() + .await; let profile = self.active_profile.as_deref(); let segments = if let Some(profile) = profile { vec![ diff --git a/codex-rs/tui/src/app/thread_session_state.rs b/codex-rs/tui/src/app/thread_session_state.rs index 49a715aff..cbf40d0d7 100644 --- a/codex-rs/tui/src/app/thread_session_state.rs +++ b/codex-rs/tui/src/app/thread_session_state.rs @@ -5,6 +5,34 @@ use codex_app_server_protocol::Thread; use codex_protocol::ThreadId; impl App { + pub(super) async fn sync_active_thread_permission_settings_to_cached_session(&mut self) { + let Some(active_thread_id) = self.active_thread_id else { + return; + }; + + let approval_policy = self.config.permissions.approval_policy.value(); + let approvals_reviewer = self.config.approvals_reviewer; + let sandbox_policy = self.config.permissions.sandbox_policy.get().clone(); + let update_session = |session: &mut ThreadSessionState| { + session.approval_policy = approval_policy; + session.approvals_reviewer = approvals_reviewer; + session.sandbox_policy = sandbox_policy.clone(); + }; + + if self.primary_thread_id == Some(active_thread_id) + && let Some(session) = self.primary_session_configured.as_mut() + { + update_session(session); + } + + if let Some(channel) = self.thread_event_channels.get(&active_thread_id) { + let mut store = channel.store.lock().await; + if let Some(session) = store.session.as_mut() { + update_session(session); + } + } + } + pub(super) async fn session_state_for_thread_read( &self, thread_id: ThreadId, @@ -53,3 +81,118 @@ impl App { session } } + +#[cfg(test)] +mod tests { + use super::*; + use crate::app::side::SideThreadState; + use crate::app::test_support::make_test_app; + use crate::app::thread_events::ThreadEventChannel; + use crate::test_support::PathBufExt; + use crate::test_support::test_path_buf; + use codex_config::types::ApprovalsReviewer; + use codex_protocol::protocol::AskForApproval; + use codex_protocol::protocol::SandboxPolicy; + use pretty_assertions::assert_eq; + use std::path::PathBuf; + + fn test_thread_session(thread_id: ThreadId, cwd: PathBuf) -> ThreadSessionState { + ThreadSessionState { + thread_id, + forked_from_id: None, + fork_parent_title: None, + thread_name: None, + model: "gpt-test".to_string(), + model_provider_id: "test-provider".to_string(), + service_tier: None, + approval_policy: AskForApproval::Never, + approvals_reviewer: ApprovalsReviewer::User, + sandbox_policy: SandboxPolicy::new_read_only_policy(), + cwd: cwd.abs(), + instruction_source_paths: Vec::new(), + reasoning_effort: None, + history_log_id: 0, + history_entry_count: 0, + network_proxy: None, + rollout_path: Some(PathBuf::new()), + } + } + + #[tokio::test] + async fn permission_settings_sync_updates_active_snapshot_without_rewriting_side_thread() { + let mut app = make_test_app().await; + let main_thread_id = + ThreadId::from_string("00000000-0000-0000-0000-000000000401").expect("valid thread"); + let side_thread_id = + ThreadId::from_string("00000000-0000-0000-0000-000000000402").expect("valid thread"); + let main_session = test_thread_session(main_thread_id, test_path_buf("/tmp/main")); + let side_session = ThreadSessionState { + approval_policy: AskForApproval::OnRequest, + sandbox_policy: SandboxPolicy::new_workspace_write_policy(), + ..test_thread_session(side_thread_id, test_path_buf("/tmp/side")) + }; + + app.primary_thread_id = Some(main_thread_id); + app.active_thread_id = Some(main_thread_id); + app.primary_session_configured = Some(main_session.clone()); + app.thread_event_channels.insert( + main_thread_id, + ThreadEventChannel::new_with_session( + /*capacity*/ 4, + main_session.clone(), + Vec::new(), + ), + ); + app.thread_event_channels.insert( + side_thread_id, + ThreadEventChannel::new_with_session( + /*capacity*/ 4, + side_session.clone(), + Vec::new(), + ), + ); + app.side_threads + .insert(side_thread_id, SideThreadState::new(main_thread_id)); + app.config.permissions.approval_policy = + codex_config::Constrained::allow_any(AskForApproval::OnRequest); + app.config.approvals_reviewer = ApprovalsReviewer::GuardianSubagent; + app.config.permissions.sandbox_policy = + codex_config::Constrained::allow_any(SandboxPolicy::new_workspace_write_policy()); + + app.sync_active_thread_permission_settings_to_cached_session() + .await; + + let expected_main_session = ThreadSessionState { + approval_policy: AskForApproval::OnRequest, + approvals_reviewer: ApprovalsReviewer::GuardianSubagent, + sandbox_policy: SandboxPolicy::new_workspace_write_policy(), + ..main_session + }; + assert_eq!( + app.primary_session_configured, + Some(expected_main_session.clone()) + ); + + let main_store_session = app + .thread_event_channels + .get(&main_thread_id) + .expect("main thread channel") + .store + .lock() + .await + .session + .clone(); + assert_eq!(main_store_session, Some(expected_main_session)); + + let side_store_session = app + .thread_event_channels + .get(&side_thread_id) + .expect("side thread channel") + .store + .lock() + .await + .session + .clone(); + assert_eq!(side_store_session, Some(side_session)); + } +}