mirror of
https://github.com/pchuan98/codex.git
synced 2026-07-01 00:31:56 +08:00
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
This commit is contained in:
Michael Bolin
committed by
GitHub
Unverified
GitHub
parent
4e05f3053c
commit
af95662a70
@@ -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,
|
||||
|
||||
@@ -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,
|
||||
|
||||
@@ -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"
|
||||
);
|
||||
}
|
||||
}
|
||||
|
||||
@@ -156,13 +156,13 @@ 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 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<PermissionProfile>,
|
||||
/// 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<AbsolutePathBuf>,
|
||||
pub(crate) reasoning_effort: Option<codex_protocol::openai_models::ReasoningEffort>,
|
||||
@@ -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<PermissionProfile>,
|
||||
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<RateLimitSnapshot> {
|
||||
@@ -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]);
|
||||
|
||||
@@ -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,
|
||||
|
||||
Reference in New Issue
Block a user