mirror of
https://github.com/pchuan98/codex.git
synced 2026-07-01 00:31:56 +08:00
permissions: store thread sessions as profiles (#19776)
## Why After thread sessions have a required `PermissionProfile`, the TUI no longer needs to cache a separate legacy `SandboxPolicy` in `ThreadSessionState`. Keeping the legacy field would reintroduce two permission authorities in the session cache and make later replay/switching logic easier to get wrong. This PR keeps legacy app-server compatibility at the ingestion boundary: old `sandbox` response values are still accepted, but they are immediately converted to a cwd-anchored profile. ## What Changed - Removes `ThreadSessionState.sandbox_policy`. - Updates active-session permission syncing to write only the current `PermissionProfile`. - Updates thread-read/replay/test fixtures to use profiles as the cached session permission source. - Leaves legacy `sandbox` fields in app-server request/response protocol paths unchanged; those are compatibility boundaries and are converted before entering cached TUI state. ## Verification - `cargo test -p codex-tui thread_session_state::tests --lib` - `cargo test -p codex-tui inactive_thread_started_notification_initializes_replay_session --lib` - `cargo test -p codex-tui thread_events --lib` - `just fix -p codex-tui` --- [//]: # (BEGIN SAPLING FOOTER) Stack created with [Sapling](https://sapling-scm.com). Best reviewed with [ReviewStack](https://reviewstack.dev/openai/codex/pull/19776). * #19900 * #19899 * __->__ #19776
This commit is contained in:
Michael Bolin
committed by
GitHub
Unverified
GitHub
parent
92fb848065
commit
341550c275
@@ -2211,7 +2211,6 @@ async fn inactive_thread_approval_bubbles_into_active_view() -> Result<()> {
|
||||
/*capacity*/ 1,
|
||||
ThreadSessionState {
|
||||
approval_policy: AskForApproval::OnRequest,
|
||||
sandbox_policy: SandboxPolicy::new_workspace_write_policy(),
|
||||
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"))
|
||||
@@ -2371,7 +2370,6 @@ async fn side_defers_subagent_approval_overlay_until_side_exits() -> Result<()>
|
||||
/*capacity*/ 4,
|
||||
ThreadSessionState {
|
||||
approval_policy: AskForApproval::OnRequest,
|
||||
sandbox_policy: SandboxPolicy::new_workspace_write_policy(),
|
||||
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"))
|
||||
@@ -2663,7 +2661,6 @@ async fn inactive_thread_approval_badge_clears_after_turn_completion_notificatio
|
||||
/*capacity*/ 4,
|
||||
ThreadSessionState {
|
||||
approval_policy: AskForApproval::OnRequest,
|
||||
sandbox_policy: SandboxPolicy::new_workspace_write_policy(),
|
||||
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"))
|
||||
@@ -2717,7 +2714,6 @@ async fn inactive_thread_started_notification_initializes_replay_session() -> Re
|
||||
ThreadId::from_string("00000000-0000-0000-0000-000000000202").expect("valid thread");
|
||||
let primary_session = ThreadSessionState {
|
||||
approval_policy: AskForApproval::OnRequest,
|
||||
sandbox_policy: SandboxPolicy::new_workspace_write_policy(),
|
||||
permission_profile: PermissionProfile::workspace_write(),
|
||||
..test_thread_session(main_thread_id, test_path_buf("/tmp/main"))
|
||||
};
|
||||
@@ -2742,7 +2738,7 @@ async fn inactive_thread_started_notification_initializes_replay_session() -> Re
|
||||
current_date: None,
|
||||
timezone: None,
|
||||
approval_policy: primary_session.approval_policy,
|
||||
sandbox_policy: primary_session.sandbox_policy.clone(),
|
||||
sandbox_policy: SandboxPolicy::new_workspace_write_policy(),
|
||||
permission_profile: None,
|
||||
network: None,
|
||||
file_system_sandbox_policy: None,
|
||||
@@ -2830,7 +2826,6 @@ async fn inactive_thread_started_notification_preserves_primary_model_when_path_
|
||||
ThreadId::from_string("00000000-0000-0000-0000-000000000302").expect("valid thread");
|
||||
let primary_session = ThreadSessionState {
|
||||
approval_policy: AskForApproval::OnRequest,
|
||||
sandbox_policy: SandboxPolicy::new_workspace_write_policy(),
|
||||
permission_profile: PermissionProfile::workspace_write(),
|
||||
..test_thread_session(main_thread_id, test_path_buf("/tmp/main"))
|
||||
};
|
||||
@@ -2899,7 +2894,6 @@ async fn thread_read_session_state_does_not_reuse_primary_permission_profile() {
|
||||
ThreadId::from_string("00000000-0000-0000-0000-000000000402").expect("valid thread");
|
||||
let primary_session = ThreadSessionState {
|
||||
approval_policy: AskForApproval::OnRequest,
|
||||
sandbox_policy: SandboxPolicy::new_workspace_write_policy(),
|
||||
permission_profile: PermissionProfile::workspace_write(),
|
||||
..test_thread_session(main_thread_id, test_path_buf("/tmp/main"))
|
||||
};
|
||||
@@ -3803,7 +3797,6 @@ fn test_thread_session(thread_id: ThreadId, cwd: PathBuf) -> ThreadSessionState
|
||||
service_tier: None,
|
||||
approval_policy: AskForApproval::Never,
|
||||
approvals_reviewer: ApprovalsReviewer::User,
|
||||
sandbox_policy: SandboxPolicy::new_read_only_policy(),
|
||||
permission_profile: PermissionProfile::read_only(),
|
||||
cwd: cwd.abs(),
|
||||
instruction_source_paths: Vec::new(),
|
||||
|
||||
@@ -335,7 +335,6 @@ mod tests {
|
||||
use codex_config::types::ApprovalsReviewer;
|
||||
use codex_protocol::models::PermissionProfile;
|
||||
use codex_protocol::protocol::AskForApproval;
|
||||
use codex_protocol::protocol::SandboxPolicy;
|
||||
use pretty_assertions::assert_eq;
|
||||
use std::path::PathBuf;
|
||||
|
||||
@@ -350,7 +349,6 @@ mod tests {
|
||||
service_tier: None,
|
||||
approval_policy: AskForApproval::Never,
|
||||
approvals_reviewer: ApprovalsReviewer::User,
|
||||
sandbox_policy: SandboxPolicy::new_read_only_policy(),
|
||||
permission_profile: PermissionProfile::read_only(),
|
||||
cwd: cwd.abs(),
|
||||
instruction_source_paths: Vec::new(),
|
||||
|
||||
@@ -4,7 +4,6 @@ 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) {
|
||||
@@ -14,10 +13,6 @@ impl App {
|
||||
|
||||
let approval_policy = self.config.permissions.approval_policy.value();
|
||||
let approvals_reviewer = self.config.approvals_reviewer;
|
||||
let sandbox_policy = self
|
||||
.config
|
||||
.permissions
|
||||
.legacy_sandbox_policy(self.config.cwd.as_path());
|
||||
let permission_profile = self
|
||||
.chat_widget
|
||||
.config_ref()
|
||||
@@ -26,7 +21,6 @@ impl App {
|
||||
let update_session = |session: &mut ThreadSessionState| {
|
||||
session.approval_policy = approval_policy;
|
||||
session.approvals_reviewer = approvals_reviewer;
|
||||
session.sandbox_policy = sandbox_policy.clone();
|
||||
session.permission_profile = permission_profile.clone();
|
||||
};
|
||||
|
||||
@@ -49,7 +43,6 @@ impl App {
|
||||
thread_id: ThreadId,
|
||||
thread: &Thread,
|
||||
) -> ThreadSessionState {
|
||||
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
|
||||
@@ -64,7 +57,6 @@ 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: sandbox_policy.clone(),
|
||||
permission_profile: permission_profile.clone(),
|
||||
cwd: thread.cwd.clone(),
|
||||
instruction_source_paths: Vec::new(),
|
||||
@@ -78,7 +70,6 @@ impl App {
|
||||
session.thread_name = thread.name.clone();
|
||||
session.model_provider_id = thread.model_provider.clone();
|
||||
session.cwd = thread.cwd.clone();
|
||||
session.sandbox_policy = sandbox_policy;
|
||||
session.permission_profile = permission_profile;
|
||||
session.instruction_source_paths = Vec::new();
|
||||
session.rollout_path = thread.path.clone();
|
||||
@@ -100,13 +91,6 @@ impl App {
|
||||
.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)]
|
||||
@@ -141,7 +125,6 @@ mod tests {
|
||||
service_tier: None,
|
||||
approval_policy: AskForApproval::Never,
|
||||
approvals_reviewer: ApprovalsReviewer::User,
|
||||
sandbox_policy: SandboxPolicy::new_read_only_policy(),
|
||||
permission_profile: PermissionProfile::read_only(),
|
||||
cwd: cwd.abs(),
|
||||
instruction_source_paths: Vec::new(),
|
||||
@@ -163,7 +146,7 @@ mod tests {
|
||||
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(),
|
||||
permission_profile: PermissionProfile::workspace_write(),
|
||||
..test_thread_session(side_thread_id, test_path_buf("/tmp/side"))
|
||||
};
|
||||
|
||||
@@ -215,7 +198,6 @@ mod tests {
|
||||
let expected_main_session = ThreadSessionState {
|
||||
approval_policy: AskForApproval::OnRequest,
|
||||
approvals_reviewer: ApprovalsReviewer::AutoReview,
|
||||
sandbox_policy: expected_sandbox_policy,
|
||||
permission_profile: expected_permission_profile,
|
||||
..main_session
|
||||
};
|
||||
@@ -348,17 +330,11 @@ mod tests {
|
||||
.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,
|
||||
|
||||
@@ -156,9 +156,6 @@ pub(crate) struct ThreadSessionState {
|
||||
pub(crate) service_tier: Option<codex_protocol::config_types::ServiceTier>,
|
||||
pub(crate) approval_policy: AskForApproval,
|
||||
pub(crate) approvals_reviewer: codex_protocol::config_types::ApprovalsReviewer,
|
||||
/// Legacy sandbox projection kept only for compatibility fields that have
|
||||
/// not migrated to `PermissionProfile` yet.
|
||||
pub(crate) sandbox_policy: SandboxPolicy,
|
||||
/// 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.
|
||||
@@ -1420,7 +1417,6 @@ async fn thread_session_state_from_thread_response(
|
||||
service_tier,
|
||||
approval_policy,
|
||||
approvals_reviewer,
|
||||
sandbox_policy,
|
||||
permission_profile,
|
||||
cwd,
|
||||
instruction_source_paths,
|
||||
|
||||
Reference in New Issue
Block a user