mirror of
https://github.com/pchuan98/codex.git
synced 2026-07-01 00:31:56 +08:00
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.
This commit is contained in:
committed by
GitHub
Unverified
parent
23afa173f4
commit
08b5e96678
@@ -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
|
||||
|
||||
@@ -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![
|
||||
|
||||
@@ -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));
|
||||
}
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user