mirror of
https://github.com/openai/codex.git
synced 2026-11-02 16:20:35 +00:00
core: set permission profiles from snapshots (#22920)
## Why #22891 moved the TUI turn-command path to pass `ActivePermissionProfile` instead of the full `PermissionProfile`, but the remaining config/session bridge still accepted the concrete `PermissionProfile` and active profile id as separate arguments. That shape made it too easy for future callers to update the concrete profile and active profile id out of sync. This PR makes the trusted session snapshot path pass one coherent value into `Permissions`, while keeping `requirements.toml` enforcement owned by the existing constrained permission state. ## What Changed - Added `PermissionProfileSnapshot` as the public snapshot value for trusted session/config synchronization. - Changed `Permissions::set_permission_profile_from_session_snapshot()` and `replace_permission_profile_from_session_snapshot()` to take a `PermissionProfileSnapshot`. - Updated the replacement path to derive its constrained `PermissionProfile` from the snapshot, so callers cannot pass a separate profile that disagrees with the snapshot. - Removed the internal tuple-style `PermissionProfileState::set_active_permission_profile()` mutation path. - Updated core session projection and TUI call sites to construct explicit legacy or active snapshots. - Documented the snapshot constructors so legacy use and id/profile mismatch hazards are called out at the API boundary. - Added a focused config test that verifies snapshot updates still respect existing permission constraints. ## How To Review 1. Start with `codex-rs/core/src/config/resolved_permission_profile.rs`; `PermissionProfileSnapshot` is the public wrapper, while `ResolvedPermissionProfile` stays internal. 2. Check `codex-rs/core/src/config/mod.rs` to confirm both session-snapshot setters validate through `PermissionProfileState` and no longer accept loose profile/id pairs. 3. Skim `codex-rs/core/src/session/session.rs` for the session projection path; it now builds the snapshot before installing it. 4. Skim the TUI changes as call-site migration from loose argument pairs to explicit snapshot construction. ## Verification - `cargo test -p codex-core permission_snapshot_setter_preserves_permission_constraints` - `cargo test -p codex-tui status_permissions_` - `cargo test -p codex-tui session_configured_preserves_profile_workspace_roots` - `just fix -p codex-core -p codex-tui`
This commit is contained in:
1 parent
de9c5c0226
commit
108234b5eb
15 files changed
+225
-132
No files matched your search
@@ -47,6 +47,7 @@ use crate::keymap::RuntimeKeymap;
|
||||
use crate::legacy_core::config::Config;
|
||||
use crate::legacy_core::config::ConfigBuilder;
|
||||
use crate::legacy_core::config::ConfigOverrides;
|
||||
use crate::legacy_core::config::PermissionProfileSnapshot;
|
||||
use crate::legacy_core::config::edit::ConfigEdit;
|
||||
use crate::legacy_core::config::edit::ConfigEditsBuilder;
|
||||
#[cfg(target_os = "windows")]
|
||||
|
||||
@@ -128,10 +128,10 @@ impl App {
|
||||
|
||||
if let Err(err) = config
|
||||
.permissions
|
||||
.set_permission_profile_from_session_snapshot(
|
||||
.set_permission_profile_from_session_snapshot(PermissionProfileSnapshot::active(
|
||||
permission_profile.clone(),
|
||||
Some(active_permission_profile),
|
||||
)
|
||||
active_permission_profile,
|
||||
))
|
||||
{
|
||||
tracing::warn!(error = %err, "{log_message}");
|
||||
self.chat_widget
|
||||
@@ -328,8 +328,10 @@ impl App {
|
||||
&& let Err(err) = self
|
||||
.chat_widget
|
||||
.set_permission_profile_from_session_snapshot(
|
||||
permission_profile.clone(),
|
||||
active_permission_profile_override.clone(),
|
||||
PermissionProfileSnapshot::from_session_snapshot(
|
||||
permission_profile.clone(),
|
||||
active_permission_profile_override.clone(),
|
||||
),
|
||||
)
|
||||
{
|
||||
tracing::error!(
|
||||
|
||||
@@ -1422,8 +1422,10 @@ impl App {
|
||||
if let Err(err) = self
|
||||
.chat_widget
|
||||
.set_permission_profile_from_session_snapshot(
|
||||
permission_profile_for_chat,
|
||||
Some(active_permission_profile),
|
||||
PermissionProfileSnapshot::active(
|
||||
permission_profile_for_chat,
|
||||
active_permission_profile,
|
||||
),
|
||||
)
|
||||
{
|
||||
tracing::warn!(%err, "failed to set permission profile on chat config");
|
||||
|
||||
@@ -28,6 +28,7 @@ use crate::app_command::AppCommand as Op;
|
||||
use crate::diff_model::FileChange;
|
||||
use crate::legacy_core::config::ConfigBuilder;
|
||||
use crate::legacy_core::config::ConfigOverrides;
|
||||
use crate::legacy_core::config::PermissionProfileSnapshot;
|
||||
use crate::legacy_core::config::TerminalResizeReflowMaxRows;
|
||||
use codex_app_server_protocol::AdditionalFileSystemPermissions;
|
||||
use codex_app_server_protocol::AdditionalNetworkPermissions;
|
||||
@@ -1730,10 +1731,9 @@ async fn update_feature_flags_disabling_guardian_clears_review_policy_and_restor
|
||||
app.chat_widget
|
||||
.set_approval_policy(AskForApproval::OnRequest);
|
||||
app.chat_widget
|
||||
.set_permission_profile_from_session_snapshot(
|
||||
.set_permission_profile_from_session_snapshot(PermissionProfileSnapshot::legacy(
|
||||
PermissionProfile::workspace_write(),
|
||||
/*active_profile*/ None,
|
||||
)?;
|
||||
))?;
|
||||
|
||||
app.update_feature_flags(vec![(Feature::GuardianApproval, false)])
|
||||
.await;
|
||||
@@ -3153,10 +3153,9 @@ async fn side_fork_config_inherits_parent_thread_runtime_settings() {
|
||||
app.chat_widget
|
||||
.set_approval_policy(AskForApproval::OnRequest);
|
||||
app.chat_widget
|
||||
.set_permission_profile_from_session_snapshot(
|
||||
.set_permission_profile_from_session_snapshot(PermissionProfileSnapshot::legacy(
|
||||
parent_permission_profile.clone(),
|
||||
/*active_profile*/ None,
|
||||
)
|
||||
))
|
||||
.expect("test permission profile should be accepted");
|
||||
app.chat_widget
|
||||
.set_approvals_reviewer(ApprovalsReviewer::AutoReview);
|
||||
|
||||
@@ -120,6 +120,7 @@ mod tests {
|
||||
use crate::app::side::SideThreadState;
|
||||
use crate::app::test_support::make_test_app;
|
||||
use crate::app::thread_events::ThreadEventChannel;
|
||||
use crate::legacy_core::config::PermissionProfileSnapshot;
|
||||
use crate::test_support::PathBufExt;
|
||||
use crate::test_support::test_path_buf;
|
||||
use codex_app_server_protocol::AskForApproval;
|
||||
@@ -201,10 +202,10 @@ mod tests {
|
||||
ActivePermissionProfile::new(BUILT_IN_PERMISSION_PROFILE_WORKSPACE);
|
||||
app.chat_widget.handle_thread_session(main_session.clone());
|
||||
app.chat_widget
|
||||
.set_permission_profile_from_session_snapshot(
|
||||
.set_permission_profile_from_session_snapshot(PermissionProfileSnapshot::active(
|
||||
expected_permission_profile.clone(),
|
||||
Some(expected_active_permission_profile.clone()),
|
||||
)
|
||||
expected_active_permission_profile.clone(),
|
||||
))
|
||||
.expect("set widget permission profile");
|
||||
app.config
|
||||
.permissions
|
||||
|
||||
@@ -61,6 +61,7 @@ use crate::legacy_core::DEFAULT_AGENTS_MD_FILENAME;
|
||||
use crate::legacy_core::config::Config;
|
||||
use crate::legacy_core::config::Constrained;
|
||||
use crate::legacy_core::config::ConstraintResult;
|
||||
use crate::legacy_core::config::PermissionProfileSnapshot;
|
||||
#[cfg(target_os = "windows")]
|
||||
use crate::legacy_core::windows_sandbox::WindowsSandboxLevelExt;
|
||||
use crate::mention_codec::LinkedMention;
|
||||
|
||||
@@ -49,22 +49,20 @@ impl ChatWidget {
|
||||
self.config.permissions.approval_policy =
|
||||
Constrained::allow_only(session.approval_policy.to_core());
|
||||
}
|
||||
let permission_snapshot = PermissionProfileSnapshot::from_session_snapshot(
|
||||
session.permission_profile.clone(),
|
||||
session.active_permission_profile.clone(),
|
||||
);
|
||||
let permission_sync = self
|
||||
.config
|
||||
.permissions
|
||||
.set_permission_profile_from_session_snapshot(
|
||||
session.permission_profile.clone(),
|
||||
session.active_permission_profile.clone(),
|
||||
);
|
||||
.set_permission_profile_from_session_snapshot(permission_snapshot.clone());
|
||||
if let Err(err) = permission_sync {
|
||||
tracing::warn!(%err, "failed to sync permissions from SessionConfigured");
|
||||
if let Err(replace_err) = self
|
||||
.config
|
||||
.permissions
|
||||
.replace_permission_profile_from_session_snapshot(
|
||||
Constrained::allow_only(session.permission_profile.clone()),
|
||||
session.active_permission_profile.clone(),
|
||||
)
|
||||
.replace_permission_profile_from_session_snapshot(permission_snapshot)
|
||||
{
|
||||
tracing::error!(
|
||||
%replace_err,
|
||||
|
||||
@@ -20,12 +20,11 @@ impl ChatWidget {
|
||||
#[cfg_attr(not(target_os = "windows"), allow(dead_code))]
|
||||
pub(crate) fn set_permission_profile_from_session_snapshot(
|
||||
&mut self,
|
||||
profile: PermissionProfile,
|
||||
active_profile: Option<ActivePermissionProfile>,
|
||||
snapshot: PermissionProfileSnapshot,
|
||||
) -> ConstraintResult<()> {
|
||||
self.config
|
||||
.permissions
|
||||
.set_permission_profile_from_session_snapshot(profile, active_profile)?;
|
||||
.set_permission_profile_from_session_snapshot(snapshot)?;
|
||||
self.refresh_status_surfaces();
|
||||
Ok(())
|
||||
}
|
||||
|
||||
@@ -292,10 +292,9 @@ async fn session_configured_syncs_widget_config_permissions_and_cwd() {
|
||||
assert_eq!(&chat.config_ref().cwd, &expected_cwd);
|
||||
|
||||
let updated_profile = PermissionProfile::workspace_write();
|
||||
chat.set_permission_profile_from_session_snapshot(
|
||||
chat.set_permission_profile_from_session_snapshot(PermissionProfileSnapshot::legacy(
|
||||
updated_profile.clone(),
|
||||
/*active_profile*/ None,
|
||||
)
|
||||
))
|
||||
.expect("set permission profile");
|
||||
assert_eq!(
|
||||
chat.config_ref().permissions.permission_profile(),
|
||||
|
||||
@@ -5,6 +5,7 @@ use super::rate_limit_snapshot_display;
|
||||
use crate::history_cell::HistoryCell;
|
||||
use crate::legacy_core::config::Config;
|
||||
use crate::legacy_core::config::ConfigBuilder;
|
||||
use crate::legacy_core::config::PermissionProfileSnapshot;
|
||||
use crate::status::StatusAccountDisplay;
|
||||
use crate::test_support::PathBufExt;
|
||||
use crate::test_support::test_path_buf;
|
||||
@@ -304,10 +305,10 @@ async fn status_permissions_named_read_only_profile_shows_builtin_label() {
|
||||
.expect("set approval policy");
|
||||
config
|
||||
.permissions
|
||||
.set_permission_profile_from_session_snapshot(
|
||||
.set_permission_profile_from_session_snapshot(PermissionProfileSnapshot::active(
|
||||
PermissionProfile::read_only(),
|
||||
Some(ActivePermissionProfile::read_only()),
|
||||
)
|
||||
ActivePermissionProfile::read_only(),
|
||||
))
|
||||
.expect("set permission profile");
|
||||
|
||||
assert_eq!(
|
||||
@@ -331,13 +332,13 @@ async fn status_permissions_read_only_profile_shows_additional_writable_roots()
|
||||
.with_additional_writable_roots(config.cwd.as_path(), std::slice::from_ref(&extra_root));
|
||||
config
|
||||
.permissions
|
||||
.set_permission_profile_from_session_snapshot(
|
||||
.set_permission_profile_from_session_snapshot(PermissionProfileSnapshot::active(
|
||||
PermissionProfile::from_runtime_permissions(
|
||||
&file_system_policy,
|
||||
NetworkSandboxPolicy::Restricted,
|
||||
),
|
||||
Some(ActivePermissionProfile::read_only()),
|
||||
)
|
||||
ActivePermissionProfile::read_only(),
|
||||
))
|
||||
.expect("set permission profile");
|
||||
|
||||
assert_eq!(
|
||||
@@ -357,12 +358,10 @@ async fn status_permissions_named_workspace_profile_shows_builtin_label() {
|
||||
.expect("set approval policy");
|
||||
config
|
||||
.permissions
|
||||
.set_permission_profile_from_session_snapshot(
|
||||
.set_permission_profile_from_session_snapshot(PermissionProfileSnapshot::active(
|
||||
PermissionProfile::workspace_write(),
|
||||
Some(ActivePermissionProfile::new(
|
||||
BUILT_IN_PERMISSION_PROFILE_WORKSPACE,
|
||||
)),
|
||||
)
|
||||
ActivePermissionProfile::new(BUILT_IN_PERMISSION_PROFILE_WORKSPACE),
|
||||
))
|
||||
.expect("set permission profile");
|
||||
|
||||
assert_eq!(
|
||||
@@ -383,12 +382,10 @@ async fn status_permissions_workspace_auto_review_shows_reviewer_label() {
|
||||
.expect("set approval policy");
|
||||
config
|
||||
.permissions
|
||||
.set_permission_profile_from_session_snapshot(
|
||||
.set_permission_profile_from_session_snapshot(PermissionProfileSnapshot::active(
|
||||
PermissionProfile::workspace_write(),
|
||||
Some(ActivePermissionProfile::new(
|
||||
BUILT_IN_PERMISSION_PROFILE_WORKSPACE,
|
||||
)),
|
||||
)
|
||||
ActivePermissionProfile::new(BUILT_IN_PERMISSION_PROFILE_WORKSPACE),
|
||||
))
|
||||
.expect("set permission profile");
|
||||
|
||||
assert_eq!(
|
||||
@@ -409,17 +406,15 @@ async fn status_permissions_named_profile_shows_additional_writable_roots() {
|
||||
let extra_root = test_path_buf("/workspace/extra").abs();
|
||||
config
|
||||
.permissions
|
||||
.set_permission_profile_from_session_snapshot(
|
||||
.set_permission_profile_from_session_snapshot(PermissionProfileSnapshot::active(
|
||||
PermissionProfile::workspace_write_with(
|
||||
std::slice::from_ref(&extra_root),
|
||||
NetworkSandboxPolicy::Restricted,
|
||||
/*exclude_tmpdir_env_var*/ false,
|
||||
/*exclude_slash_tmp*/ false,
|
||||
),
|
||||
Some(ActivePermissionProfile::new(
|
||||
BUILT_IN_PERMISSION_PROFILE_WORKSPACE,
|
||||
)),
|
||||
)
|
||||
ActivePermissionProfile::new(BUILT_IN_PERMISSION_PROFILE_WORKSPACE),
|
||||
))
|
||||
.expect("set permission profile");
|
||||
|
||||
assert_eq!(
|
||||
@@ -445,10 +440,10 @@ async fn status_permissions_workspace_roots_show_additional_directories() {
|
||||
.set_workspace_roots(config.workspace_roots.clone());
|
||||
config
|
||||
.permissions
|
||||
.set_permission_profile_from_session_snapshot(
|
||||
.set_permission_profile_from_session_snapshot(PermissionProfileSnapshot::active(
|
||||
PermissionProfile::workspace_write(),
|
||||
Some(ActivePermissionProfile::new(":workspace")),
|
||||
)
|
||||
ActivePermissionProfile::new(":workspace"),
|
||||
))
|
||||
.expect("set permission profile");
|
||||
|
||||
assert_eq!(
|
||||
@@ -470,15 +465,17 @@ async fn status_permissions_workspace_roots_include_profile_defined_directories(
|
||||
let profile_root = test_path_buf("/workspace/shared").abs();
|
||||
config
|
||||
.permissions
|
||||
.set_permission_profile_from_session_snapshot_with_profile_workspace_roots(
|
||||
PermissionProfile::workspace_write_with(
|
||||
std::slice::from_ref(&profile_root),
|
||||
NetworkSandboxPolicy::Restricted,
|
||||
/*exclude_tmpdir_env_var*/ false,
|
||||
/*exclude_slash_tmp*/ false,
|
||||
.set_permission_profile_from_session_snapshot(
|
||||
PermissionProfileSnapshot::active_with_profile_workspace_roots(
|
||||
PermissionProfile::workspace_write_with(
|
||||
std::slice::from_ref(&profile_root),
|
||||
NetworkSandboxPolicy::Restricted,
|
||||
/*exclude_tmpdir_env_var*/ false,
|
||||
/*exclude_slash_tmp*/ false,
|
||||
),
|
||||
ActivePermissionProfile::new(":workspace"),
|
||||
vec![profile_root.clone()],
|
||||
),
|
||||
Some(ActivePermissionProfile::new(":workspace")),
|
||||
vec![profile_root.clone()],
|
||||
)
|
||||
.expect("set permission profile");
|
||||
|
||||
@@ -502,17 +499,15 @@ async fn status_permissions_broadened_workspace_profile_shows_builtin_label() {
|
||||
.expect("set approval policy");
|
||||
config
|
||||
.permissions
|
||||
.set_permission_profile_from_session_snapshot(
|
||||
.set_permission_profile_from_session_snapshot(PermissionProfileSnapshot::active(
|
||||
PermissionProfile::workspace_write_with(
|
||||
&[],
|
||||
NetworkSandboxPolicy::Enabled,
|
||||
/*exclude_tmpdir_env_var*/ false,
|
||||
/*exclude_slash_tmp*/ false,
|
||||
),
|
||||
Some(ActivePermissionProfile::new(
|
||||
BUILT_IN_PERMISSION_PROFILE_WORKSPACE,
|
||||
)),
|
||||
)
|
||||
ActivePermissionProfile::new(BUILT_IN_PERMISSION_PROFILE_WORKSPACE),
|
||||
))
|
||||
.expect("set permission profile");
|
||||
|
||||
assert_eq!(
|
||||
@@ -527,10 +522,10 @@ async fn status_permissions_user_defined_profile_shows_name() {
|
||||
let mut config = test_config(&temp_home).await;
|
||||
config
|
||||
.permissions
|
||||
.set_permission_profile_from_session_snapshot(
|
||||
.set_permission_profile_from_session_snapshot(PermissionProfileSnapshot::active(
|
||||
PermissionProfile::read_only(),
|
||||
Some(ActivePermissionProfile::new("locked")),
|
||||
)
|
||||
ActivePermissionProfile::new("locked"),
|
||||
))
|
||||
.expect("set permission profile");
|
||||
|
||||
assert_eq!(
|
||||
@@ -547,10 +542,10 @@ async fn status_snapshot_shows_active_user_defined_profile() {
|
||||
set_workspace_cwd(&mut config, test_path_buf("/workspace/tests").abs());
|
||||
config
|
||||
.permissions
|
||||
.set_permission_profile_from_session_snapshot(
|
||||
.set_permission_profile_from_session_snapshot(PermissionProfileSnapshot::active(
|
||||
PermissionProfile::read_only(),
|
||||
Some(ActivePermissionProfile::new("locked")),
|
||||
)
|
||||
ActivePermissionProfile::new("locked"),
|
||||
))
|
||||
.expect("set permission profile");
|
||||
|
||||
let usage = TokenUsage::default();
|
||||
@@ -645,12 +640,10 @@ async fn status_snapshot_shows_auto_review_permissions() {
|
||||
config.approvals_reviewer = ApprovalsReviewer::AutoReview;
|
||||
config
|
||||
.permissions
|
||||
.set_permission_profile_from_session_snapshot(
|
||||
.set_permission_profile_from_session_snapshot(PermissionProfileSnapshot::active(
|
||||
PermissionProfile::workspace_write(),
|
||||
Some(ActivePermissionProfile::new(
|
||||
BUILT_IN_PERMISSION_PROFILE_WORKSPACE,
|
||||
)),
|
||||
)
|
||||
ActivePermissionProfile::new(BUILT_IN_PERMISSION_PROFILE_WORKSPACE),
|
||||
))
|
||||
.expect("set permission profile");
|
||||
|
||||
let usage = TokenUsage::default();
|
||||
|
||||
Reference in new issue
Block a user