Skip to content

permissions: resolve profile identity with constraints#22683

Open
bolinfest wants to merge 1 commit into
pr22610from
pr22683
Open

permissions: resolve profile identity with constraints#22683
bolinfest wants to merge 1 commit into
pr22610from
pr22683

Conversation

@bolinfest
Copy link
Copy Markdown
Collaborator

@bolinfest bolinfest commented May 14, 2026

Why

This PR is the invariant-cleanup slice stacked directly on #22610.

#22610 adds profile-declared workspace_roots and separates raw permission profiles from effective runtime profiles, but its in-memory shape is intentionally transitional: Permissions still carries the selected active profile identity separately from the constrained PermissionProfile value. That makes APIs such as set_constrained_permission_profile_with_active_profile() awkward because callers must pass two values that are only correct when they stay in sync.

This PR introduces a single resolved profile state so the selected profile identity, immutable profile value, and profile-declared workspace roots travel together. That keeps the stack reviewable without leaving the next app-server migration layer to build on a representation that can drift.

Stack Context

This PR should be reviewed as the cleanup layer after #22610 and before the app-server protocol/API changes in #22611.

Keeping this as a separate PR is deliberate: it removes the most questionable temporary API from #22610 without folding app-server protocol migration into the base workspace-roots change.

Review Guide

Suggested review order:

  1. Start with codex-rs/core/src/config/resolved_permission_profile.rs.
    This new module defines ResolvedPermissionProfile::{Legacy, BuiltIn, Named} and wraps it in PermissionProfileState, preserving the existing Constrained<PermissionProfile> checks while keeping the profile id, extends, profile value, and profile workspace roots together.

  2. Review codex-rs/core/src/config/mod.rs.
    Permissions now stores PermissionProfileState instead of parallel constrained_permissions_profile and active_permission_profile fields. The old two-argument setter is replaced by session-snapshot helpers and state-level mutation methods.

  3. Review codex-rs/core/src/session/session.rs and codex-rs/core/src/session/mod.rs.
    Session configuration now stores PermissionProfileState directly. When the effective profile is materialized for a session, the selected profile identity and profile workspace roots are preserved instead of reconstructed as sidecar state.

  4. Skim the downstream call sites.
    Most of the remaining changes are mechanical fallout from Permissions::permission_profile() returning &PermissionProfile rather than &Constrained<PermissionProfile>, plus one MCP constructor helper for callers that already have the resolved profile value.

What Changed

  • added PermissionProfileState and ResolvedPermissionProfile to represent legacy, built-in, and named selected profiles
  • kept built-in profile ids typed via BuiltInPermissionProfileId
  • moved selected profile identity and profile-declared workspace roots into the resolved state
  • replaced Permissions parallel profile fields with one permission_profile_state
  • removed set_constrained_permission_profile_with_active_profile() from normal session sync paths
  • preserved session/replay compatibility with explicit session snapshot helpers for trusted core state
  • updated session configuration, MCP initialization, app-server, exec, TUI, and guardian call sites to use the resolved profile value directly

Verification Strategy

The meaningful coverage is in the existing config/session test paths that already exercise profile selection, session updates, workspace-root materialization, and legacy sandbox compatibility. This PR updates those tests to construct and assert through PermissionProfileState instead of independently mutating constrained profile and active-profile fields.


Stack created with Sapling. Best reviewed with ReviewStack.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant