From af95662a70bf665fd5629588a9f3a4f0f1ba7123 Mon Sep 17 00:00:00 2001 From: Michael Bolin Date: Mon, 27 Apr 2026 20:39:06 -0700 Subject: [PATCH] permissions: require profiles in TUI thread state (#19773) ## Why `ThreadSessionState` is the TUI's cached view of an app-server session. To make `PermissionProfile` the canonical runtime permissions model, cached thread sessions need to always have a profile instead of treating the profile as an optional supplement to a legacy `sandbox` response field. The main compatibility concern is older app-server v2 lifecycle responses that only include `sandbox` and omit `permissionProfile`: - `thread/start` -> `ThreadStartResponse.sandbox` - `thread/resume` -> `ThreadResumeResponse.sandbox` - `thread/fork` -> `ThreadForkResponse.sandbox` Those responses must still hydrate correctly when the TUI is pointed at an older app-server. This PR converts the legacy `sandbox` value into a `PermissionProfile` immediately at response ingestion time, using the response `cwd`, so cached sessions do not carry an optional profile that can later reinterpret cwd-bound grants against a different thread cwd. This fallback is intentionally boundary compatibility. The follow-up PRs in this stack continue the cleanup by making `SessionConfiguredEvent` profile-only, deriving sandbox projections from snapshots only when an API still needs them, and then removing `sandbox_policy` from `ThreadSessionState`. ## What Changed - Makes `ThreadSessionState.permission_profile` required. - Converts legacy app-server response `sandbox` values into a `PermissionProfile` at ingestion time using the response cwd. - Ensures `thread/read` hydration does not reuse a primary session profile that may be anchored to a different cwd; it uses the active widget permission settings for the read thread fallback instead of reusing cached primary-session permissions. - Keeps the app-server request path unchanged: embedded sessions send profiles, while remote sessions continue using legacy sandbox overrides for compatibility. ## Verification - `cargo test -p codex-tui thread_read --lib` - `cargo test -p codex-tui permission_settings_sync_preserves_active_profile_only_rules --lib` - `cargo test -p codex-tui resume_response_restores_turns_from_thread_items --lib` - `cargo test -p codex-tui thread_session_state::tests --lib` --- [//]: # (BEGIN SAPLING FOOTER) Stack created with [Sapling](https://sapling-scm.com). Best reviewed with [ReviewStack](https://reviewstack.dev/openai/codex/pull/19773). * #19900 * #19899 * #19776 * #19775 * #19774 * __->__ #19773 --- codex-rs/tui/src/app/tests.rs | 31 +++--- codex-rs/tui/src/app/thread_events.rs | 2 +- codex-rs/tui/src/app/thread_session_state.rs | 106 ++++++++++++++++--- codex-rs/tui/src/app_server_session.rs | 30 ++++-- codex-rs/tui/src/chatwidget.rs | 2 +- 5 files changed, 132 insertions(+), 39 deletions(-) diff --git a/codex-rs/tui/src/app/tests.rs b/codex-rs/tui/src/app/tests.rs index d9d3b0c02..8a08f8bf1 100644 --- a/codex-rs/tui/src/app/tests.rs +++ b/codex-rs/tui/src/app/tests.rs @@ -2212,7 +2212,7 @@ async fn inactive_thread_approval_bubbles_into_active_view() -> Result<()> { ThreadSessionState { approval_policy: AskForApproval::OnRequest, sandbox_policy: SandboxPolicy::new_workspace_write_policy(), - permission_profile: Some(PermissionProfile::workspace_write()), + permission_profile: PermissionProfile::workspace_write(), rollout_path: Some(test_path_buf("/tmp/agent-rollout.jsonl")), ..test_thread_session(agent_thread_id, test_path_buf("/tmp/agent")) }, @@ -2372,7 +2372,7 @@ async fn side_defers_subagent_approval_overlay_until_side_exits() -> Result<()> ThreadSessionState { approval_policy: AskForApproval::OnRequest, sandbox_policy: SandboxPolicy::new_workspace_write_policy(), - permission_profile: Some(PermissionProfile::workspace_write()), + permission_profile: PermissionProfile::workspace_write(), rollout_path: Some(test_path_buf("/tmp/agent-rollout.jsonl")), ..test_thread_session(agent_thread_id, test_path_buf("/tmp/agent")) }, @@ -2664,7 +2664,7 @@ async fn inactive_thread_approval_badge_clears_after_turn_completion_notificatio ThreadSessionState { approval_policy: AskForApproval::OnRequest, sandbox_policy: SandboxPolicy::new_workspace_write_policy(), - permission_profile: Some(PermissionProfile::workspace_write()), + permission_profile: PermissionProfile::workspace_write(), rollout_path: Some(test_path_buf("/tmp/agent-rollout.jsonl")), ..test_thread_session(agent_thread_id, test_path_buf("/tmp/agent")) }, @@ -2718,7 +2718,7 @@ async fn inactive_thread_started_notification_initializes_replay_session() -> Re let primary_session = ThreadSessionState { approval_policy: AskForApproval::OnRequest, sandbox_policy: SandboxPolicy::new_workspace_write_policy(), - permission_profile: Some(PermissionProfile::workspace_write()), + permission_profile: PermissionProfile::workspace_write(), ..test_thread_session(main_thread_id, test_path_buf("/tmp/main")) }; @@ -2831,7 +2831,7 @@ async fn inactive_thread_started_notification_preserves_primary_model_when_path_ let primary_session = ThreadSessionState { approval_policy: AskForApproval::OnRequest, sandbox_policy: SandboxPolicy::new_workspace_write_policy(), - permission_profile: Some(PermissionProfile::workspace_write()), + permission_profile: PermissionProfile::workspace_write(), ..test_thread_session(main_thread_id, test_path_buf("/tmp/main")) }; @@ -2887,9 +2887,9 @@ async fn inactive_thread_started_notification_preserves_primary_model_when_path_ Ok(()) } -/// `thread/read` is metadata/replay hydration and does not return an -/// authoritative runtime `PermissionProfile`, so it must not reuse the active -/// primary session profile after swapping in the read thread's cwd. +/// `thread/read` is metadata/replay hydration and does not return a fresh +/// server-authored `PermissionProfile`, so it must not reuse the cached primary +/// session profile after swapping in the read thread's cwd. #[tokio::test] async fn thread_read_session_state_does_not_reuse_primary_permission_profile() { let mut app = make_test_app().await; @@ -2900,7 +2900,7 @@ async fn thread_read_session_state_does_not_reuse_primary_permission_profile() { let primary_session = ThreadSessionState { approval_policy: AskForApproval::OnRequest, sandbox_policy: SandboxPolicy::new_workspace_write_policy(), - permission_profile: Some(PermissionProfile::workspace_write()), + permission_profile: PermissionProfile::workspace_write(), ..test_thread_session(main_thread_id, test_path_buf("/tmp/main")) }; app.primary_session_configured = Some(primary_session); @@ -2931,10 +2931,15 @@ async fn thread_read_session_state_does_not_reuse_primary_permission_profile() { assert_eq!(session.thread_id, read_thread_id); assert_eq!(session.cwd.as_path(), test_path_buf("/tmp/read").as_path()); + let expected_permission_profile = app + .chat_widget + .config_ref() + .permissions + .permission_profile(); assert_eq!( - session.permission_profile, None, - "thread/read does not return an authoritative permission profile; reusing the primary \ - session profile would reinterpret cwd-bound entries against the read thread cwd" + session.permission_profile, expected_permission_profile, + "thread/read does not return fresh server permissions; the fallback profile must use the \ + active widget permissions rather than reusing the cached primary session profile" ); } @@ -3800,7 +3805,7 @@ fn test_thread_session(thread_id: ThreadId, cwd: PathBuf) -> ThreadSessionState approval_policy: AskForApproval::Never, approvals_reviewer: ApprovalsReviewer::User, sandbox_policy: SandboxPolicy::new_read_only_policy(), - permission_profile: Some(PermissionProfile::read_only()), + permission_profile: PermissionProfile::read_only(), cwd: cwd.abs(), instruction_source_paths: Vec::new(), reasoning_effort: None, diff --git a/codex-rs/tui/src/app/thread_events.rs b/codex-rs/tui/src/app/thread_events.rs index 9bbb41fbd..d73c39dfe 100644 --- a/codex-rs/tui/src/app/thread_events.rs +++ b/codex-rs/tui/src/app/thread_events.rs @@ -351,7 +351,7 @@ mod tests { approval_policy: AskForApproval::Never, approvals_reviewer: ApprovalsReviewer::User, sandbox_policy: SandboxPolicy::new_read_only_policy(), - permission_profile: Some(PermissionProfile::read_only()), + permission_profile: PermissionProfile::read_only(), cwd: cwd.abs(), instruction_source_paths: Vec::new(), reasoning_effort: None, diff --git a/codex-rs/tui/src/app/thread_session_state.rs b/codex-rs/tui/src/app/thread_session_state.rs index b4d0fb268..a0ba5bdc3 100644 --- a/codex-rs/tui/src/app/thread_session_state.rs +++ b/codex-rs/tui/src/app/thread_session_state.rs @@ -3,6 +3,8 @@ use crate::app_server_session::ThreadSessionState; use crate::read_session_model; use codex_app_server_protocol::Thread; use codex_protocol::ThreadId; +use codex_protocol::models::PermissionProfile; +use codex_protocol::protocol::SandboxPolicy; impl App { pub(super) async fn sync_active_thread_permission_settings_to_cached_session(&mut self) { @@ -16,12 +18,11 @@ impl App { .config .permissions .legacy_sandbox_policy(self.config.cwd.as_path()); - let permission_profile = Some( - self.chat_widget - .config_ref() - .permissions - .permission_profile(), - ); + let permission_profile = self + .chat_widget + .config_ref() + .permissions + .permission_profile(); let update_session = |session: &mut ThreadSessionState| { session.approval_policy = approval_policy; session.approvals_reviewer = approvals_reviewer; @@ -48,10 +49,8 @@ impl App { thread_id: ThreadId, thread: &Thread, ) -> ThreadSessionState { - let sandbox_policy = self - .config - .permissions - .legacy_sandbox_policy(self.config.cwd.as_path()); + let sandbox_policy = self.active_legacy_sandbox_policy_for_cwd(thread.cwd.as_path()); + let permission_profile = self.active_permission_profile(); let mut session = self .primary_session_configured .clone() @@ -65,8 +64,8 @@ impl App { service_tier: self.chat_widget.current_service_tier(), approval_policy: self.config.permissions.approval_policy.value(), approvals_reviewer: self.config.approvals_reviewer, - sandbox_policy, - permission_profile: None, + sandbox_policy: sandbox_policy.clone(), + permission_profile: permission_profile.clone(), cwd: thread.cwd.clone(), instruction_source_paths: Vec::new(), reasoning_effort: self.chat_widget.current_reasoning_effort(), @@ -79,7 +78,8 @@ impl App { session.thread_name = thread.name.clone(); session.model_provider_id = thread.model_provider.clone(); session.cwd = thread.cwd.clone(); - session.permission_profile = None; + session.sandbox_policy = sandbox_policy; + session.permission_profile = permission_profile; session.instruction_source_paths = Vec::new(); session.rollout_path = thread.path.clone(); if let Some(model) = @@ -93,6 +93,20 @@ impl App { session.history_entry_count = 0; session } + + fn active_permission_profile(&self) -> PermissionProfile { + self.chat_widget + .config_ref() + .permissions + .permission_profile() + } + + fn active_legacy_sandbox_policy_for_cwd(&self, cwd: &std::path::Path) -> SandboxPolicy { + self.chat_widget + .config_ref() + .permissions + .legacy_sandbox_policy(cwd) + } } #[cfg(test)] @@ -128,7 +142,7 @@ mod tests { approval_policy: AskForApproval::Never, approvals_reviewer: ApprovalsReviewer::User, sandbox_policy: SandboxPolicy::new_read_only_policy(), - permission_profile: None, + permission_profile: PermissionProfile::read_only(), cwd: cwd.abs(), instruction_source_paths: Vec::new(), reasoning_effort: None, @@ -202,7 +216,7 @@ mod tests { approval_policy: AskForApproval::OnRequest, approvals_reviewer: ApprovalsReviewer::AutoReview, sandbox_policy: expected_sandbox_policy, - permission_profile: Some(expected_permission_profile), + permission_profile: expected_permission_profile, ..main_session }; assert_eq!( @@ -256,7 +270,7 @@ mod tests { NetworkSandboxPolicy::Restricted, ); let session = ThreadSessionState { - permission_profile: Some(profile.clone()), + permission_profile: profile.clone(), ..test_thread_session(thread_id, test_path_buf("/tmp/main")) }; @@ -276,7 +290,7 @@ mod tests { let expected_session = ThreadSessionState { approval_policy: AskForApproval::OnRequest, - permission_profile: Some(profile), + permission_profile: profile, ..session }; assert_eq!( @@ -295,4 +309,62 @@ mod tests { .clone(); assert_eq!(store_session, Some(expected_session)); } + + #[tokio::test] + async fn thread_read_fallback_uses_active_permission_settings() { + let mut app = make_test_app().await; + let primary_thread_id = + ThreadId::from_string("00000000-0000-0000-0000-000000000404").expect("valid thread"); + let read_thread_id = + ThreadId::from_string("00000000-0000-0000-0000-000000000405").expect("valid thread"); + let primary_session = ThreadSessionState { + permission_profile: PermissionProfile::workspace_write(), + ..test_thread_session(primary_thread_id, test_path_buf("/tmp/primary")) + }; + let read_thread = Thread { + id: read_thread_id.to_string(), + forked_from_id: None, + preview: "read thread".to_string(), + ephemeral: false, + model_provider: "read-provider".to_string(), + created_at: 1, + updated_at: 2, + status: codex_app_server_protocol::ThreadStatus::Idle, + path: None, + cwd: test_path_buf("/tmp/read").abs(), + cli_version: "0.0.0".to_string(), + source: codex_app_server_protocol::SessionSource::Unknown, + agent_nickname: None, + agent_role: None, + git_info: None, + name: Some("read thread".to_string()), + turns: Vec::new(), + }; + + app.primary_session_configured = Some(primary_session.clone()); + app.chat_widget.handle_thread_session(primary_session); + + let session = app + .session_state_for_thread_read(read_thread_id, &read_thread) + .await; + + let expected_sandbox_policy = app + .chat_widget + .config_ref() + .permissions + .legacy_sandbox_policy(read_thread.cwd.as_path()); + let expected_permission_profile = app + .chat_widget + .config_ref() + .permissions + .permission_profile(); + assert_eq!(session.sandbox_policy, expected_sandbox_policy); + assert_eq!(session.permission_profile, expected_permission_profile); + assert_ne!( + session.permission_profile, + app.config.permissions.permission_profile(), + "thread/read fallback must use the active widget permissions rather than stale app \ + config defaults" + ); + } } diff --git a/codex-rs/tui/src/app_server_session.rs b/codex-rs/tui/src/app_server_session.rs index 11779c204..5a33896dd 100644 --- a/codex-rs/tui/src/app_server_session.rs +++ b/codex-rs/tui/src/app_server_session.rs @@ -156,13 +156,13 @@ pub(crate) struct ThreadSessionState { pub(crate) service_tier: Option, pub(crate) approval_policy: AskForApproval, pub(crate) approvals_reviewer: codex_protocol::config_types::ApprovalsReviewer, - /// Legacy sandbox projection kept for compatibility. Use this only when - /// `permission_profile` is `None`. + /// Legacy sandbox projection kept only for compatibility fields that have + /// not migrated to `PermissionProfile` yet. pub(crate) sandbox_policy: SandboxPolicy, - /// Canonical active permissions when available. Consumers should prefer - /// this over `sandbox_policy`; `None` means the session only has a legacy - /// sandbox projection. - pub(crate) permission_profile: Option, + /// Canonical active permissions for this session. Legacy app-server + /// responses are converted to a profile at ingestion time using the + /// response cwd so cached sessions do not reinterpret cwd-bound grants. + pub(crate) permission_profile: PermissionProfile, pub(crate) cwd: AbsolutePathBuf, pub(crate) instruction_source_paths: Vec, pub(crate) reasoning_effort: Option, @@ -1407,6 +1407,8 @@ async fn thread_session_state_from_thread_response( .map_err(|err| format!("forked_from_id is invalid: {err}"))?; let (history_log_id, history_entry_count) = message_history_metadata(config).await; let history_entry_count = u64::try_from(history_entry_count).unwrap_or(u64::MAX); + let permission_profile = + permission_profile_from_response_permissions(&sandbox_policy, permission_profile, &cwd); Ok(ThreadSessionState { thread_id, @@ -1430,6 +1432,16 @@ async fn thread_session_state_from_thread_response( }) } +fn permission_profile_from_response_permissions( + sandbox_policy: &SandboxPolicy, + permission_profile: Option, + cwd: &AbsolutePathBuf, +) -> PermissionProfile { + permission_profile.unwrap_or_else(|| { + PermissionProfile::from_legacy_sandbox_policy_for_cwd(sandbox_policy, cwd.as_path()) + }) +} + pub(crate) fn app_server_rate_limit_snapshots_to_core( response: GetAccountRateLimitsResponse, ) -> Vec { @@ -1770,7 +1782,11 @@ mod tests { ); assert_eq!( started.session.permission_profile, - response.permission_profile.clone().map(Into::into) + response + .permission_profile + .clone() + .map(Into::into) + .expect("response includes profile") ); assert_eq!(started.turns.len(), 1); assert_eq!(started.turns[0], response.thread.turns[0]); diff --git a/codex-rs/tui/src/chatwidget.rs b/codex-rs/tui/src/chatwidget.rs index 8fbb2fe83..f69eea4af 100644 --- a/codex-rs/tui/src/chatwidget.rs +++ b/codex-rs/tui/src/chatwidget.rs @@ -1621,7 +1621,7 @@ fn thread_session_state_to_legacy_event( approval_policy: session.approval_policy, approvals_reviewer: session.approvals_reviewer, sandbox_policy: session.sandbox_policy, - permission_profile: session.permission_profile, + permission_profile: Some(session.permission_profile), cwd: session.cwd, reasoning_effort: session.reasoning_effort, history_log_id: session.history_log_id,