Skip to content

core: set permission profiles from snapshots#22920

Merged
bolinfest merged 1 commit into
mainfrom
pr22920
May 16, 2026
Merged

core: set permission profiles from snapshots#22920
bolinfest merged 1 commit into
mainfrom
pr22920

Conversation

@bolinfest
Copy link
Copy Markdown
Collaborator

@bolinfest bolinfest commented May 15, 2026

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

@bolinfest bolinfest requested a review from a team as a code owner May 15, 2026 22:57
@chatgpt-codex-connector
Copy link
Copy Markdown
Contributor

💡 Codex Review

let active_permission_profile = self.config.permissions.active_permission_profile();
let op = AppCommand::user_turn(
items,
self.config.cwd.to_path_buf(),
AskForApproval::from(self.config.permissions.approval_policy.value()),
active_permission_profile,

P2 Badge Add integration coverage for turn permission snapshots

This changes submitted user turns from carrying the concrete PermissionProfile to carrying only the active profile id, which changes whether app-server turn/start receives Preserve, ActiveProfile, or LegacySandbox permission overrides. The added coverage is TUI/unit-level, but the repo guidance requires integration coverage for agent/session logic changes; please add an integration test under codex-rs/core/tests/suite that exercises a real turn start or resume snapshot path so permission routing regressions are caught end-to-end.

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Copy link
Copy Markdown
Collaborator

@viyatb-oai viyatb-oai left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Small API-shape suggestion below. Not blocking, but it seems aligned with the goal of this refactor.

@@ -343,25 +327,11 @@ impl Permissions {
pub fn replace_permission_profile_from_session_snapshot(
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This still leaves the replacement path with two sources of truth: callers pass both a constrained PermissionProfile and a PermissionProfileSnapshot. Since the only current caller builds Constrained::allow_only(session.permission_profile.clone()), could this API take only the snapshot and derive that constraint internally? That would make the “snapshot is the coherent unit” story complete here too.

Suggested change
pub fn replace_permission_profile_from_session_snapshot(
pub fn replace_permission_profile_from_session_snapshot(
&mut self,
snapshot: PermissionProfileSnapshot,
) -> ConstraintResult<()> {
let permission_profile =
Constrained::allow_only(snapshot.permission_profile().clone());
self.permission_profile_state = PermissionProfileState::from_constrained_resolved(
permission_profile,
snapshot.into_resolved_permission_profile(),
)?;
Ok(())
}

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed in the latest revision: replace_permission_profile_from_session_snapshot() now takes only PermissionProfileSnapshot and derives Constrained::allow_only(snapshot.permission_profile().clone()) internally, so the replacement path uses the snapshot as the single coherent source.

@bolinfest bolinfest merged commit 108234b into main May 16, 2026
47 of 62 checks passed
@bolinfest bolinfest deleted the pr22920 branch May 16, 2026 14:26
@github-actions github-actions Bot locked and limited conversation to collaborators May 16, 2026
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants