Conversation
This was referenced May 14, 2026
f92f2c3 to
28c51e6
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
This PR is the invariant-cleanup slice stacked directly on #22610.
#22610 adds profile-declared
workspace_rootsand separates raw permission profiles from effective runtime profiles, but its in-memory shape is intentionally transitional:Permissionsstill carries the selected active profile identity separately from the constrainedPermissionProfilevalue. That makes APIs such asset_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.
[permissions.<id>.workspace_roots], runtime workspace roots, and:workspace_rootsmaterialization.PermissionProfileState.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:
Start with
codex-rs/core/src/config/resolved_permission_profile.rs.This new module defines
ResolvedPermissionProfile::{Legacy, BuiltIn, Named}and wraps it inPermissionProfileState, preserving the existingConstrained<PermissionProfile>checks while keeping the profile id,extends, profile value, and profile workspace roots together.Review
codex-rs/core/src/config/mod.rs.Permissionsnow storesPermissionProfileStateinstead of parallelconstrained_permissions_profileandactive_permission_profilefields. The old two-argument setter is replaced by session-snapshot helpers and state-level mutation methods.Review
codex-rs/core/src/session/session.rsandcodex-rs/core/src/session/mod.rs.Session configuration now stores
PermissionProfileStatedirectly. 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.Skim the downstream call sites.
Most of the remaining changes are mechanical fallout from
Permissions::permission_profile()returning&PermissionProfilerather than&Constrained<PermissionProfile>, plus one MCP constructor helper for callers that already have the resolved profile value.What Changed
PermissionProfileStateandResolvedPermissionProfileto represent legacy, built-in, and named selected profilesBuiltInPermissionProfileIdPermissionsparallel profile fields with onepermission_profile_stateset_constrained_permission_profile_with_active_profile()from normal session sync pathsVerification 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
PermissionProfileStateinstead of independently mutating constrained profile and active-profile fields.Stack created with Sapling. Best reviewed with ReviewStack.