Conversation
This was referenced May 14, 2026
bolinfest
force-pushed
the
pr22610
branch
4 times, most recently
from
28c51e6 to
797dbb4
Compare
bolinfest
force-pushed
the
pr22683
branch
2 times, most recently
from
396a7e8 to
cccf53e
Compare
bolinfest
force-pushed
the
pr22683
branch
3 times, most recently
from
033e494 to
bb493c2
Compare
bolinfest
added a commit
that referenced
this pull request
May 15, 2026
## Why This is the configuration/model half of the alternative permissions migration we discussed as a comparison point for [#22401](#22401) and [#22402](#22402). The old `workspace-write` model mixes three concerns that we want to keep separate: - reusable profile rules that should stay immutable once selected - user/runtime workspace roots from `cwd`, `--add-dir`, and legacy workspace-write config - internal Codex writable roots such as memories, which should not be shown as user workspace roots This PR gives permission profiles first-class `workspace_roots` so users can opt multiple repositories into the same `:workspace_roots` rules without using broad absolute-path write grants. It also starts separating the raw selected profile from the effective runtime profile by making `Permissions` expose explicit accessors instead of public mutable fields. A representative `config.toml` looks like this: ```toml default_permissions = "dev" [permissions.dev.workspace_roots] "~/code/openai" = true "~/code/developers-website" = true [permissions.dev.filesystem.":workspace_roots"] "." = "write" ".codex" = "read" ".git" = "read" ".vscode" = "read" ``` If Codex starts in `~/code/codex` with that profile selected, the effective workspace-root set becomes: - `~/code/codex` from the runtime `cwd` - `~/code/openai` from the profile - `~/code/developers-website` from the profile The `:workspace_roots` rules are materialized across each root, so `.git`, `.codex`, and `.vscode` stay scoped the same way everywhere. Runtime additions such as `--add-dir` can still layer on later stack entries without mutating the selected profile. ## Stack Shape This PR intentionally stops before the profile-identity cleanup in [#22683](#22683) so the base review stays focused on config loading, workspace-root materialization, and compatibility with legacy `workspace-write`. The representation in this PR is therefore transitional: `Permissions` carries enough state to distinguish the raw constrained profile from the effective runtime profile, and there are still call sites that must keep the active profile identity and constrained profile value in sync. The follow-up PR replaces that with a single resolved profile state (`ResolvedPermissionProfile` / `PermissionProfileState`) that keeps the profile id, immutable `PermissionProfile`, and profile-declared workspace roots together. That follow-up removes APIs such as `set_constrained_permission_profile_with_active_profile()` where separate arguments could drift out of sync. Downstream PRs then build on this base to switch app-server turn updates to profile ids plus runtime workspace roots and to finish the user-visible summary behavior. Reviewers should judge this PR as the workspace-roots foundation, not as the final in-memory shape of selected permission profiles. ## Review Guide Suggested review order: 1. Start with `codex-rs/core/src/config/mod.rs`. This is the main shape change in the base slice. `Permissions` now stores a private raw `Constrained<PermissionProfile>` plus runtime `workspace_roots`. Callers use `permission_profile()` when they need the raw constrained value and `effective_permission_profile()` when they need a materialized runtime profile. As noted above, [#22683](#22683) replaces this transitional shape with a resolved profile state that keeps identity and profile data together. 2. Review `codex-rs/config/src/permissions_toml.rs` and `codex-rs/core/src/config/permissions.rs`. These add `[permissions.<id>.workspace_roots]`, resolve enabled entries relative to the policy cwd, and keep `:workspace_roots` deny-read glob patterns symbolic until the actual roots are known. 3. Review `codex-rs/protocol/src/permissions.rs` and `codex-rs/protocol/src/models.rs`. These add the policy/profile materialization helpers that expand exact `:workspace_roots` entries and scoped deny-read globs over every workspace root. This is also where `ActivePermissionProfileModification` is removed from the core model. 4. Review the legacy bridge in `Config::load_from_base_config_with_overrides` and `Config::set_legacy_sandbox_policy`. This is where legacy `workspace-write` roots become runtime workspace roots, while Codex internal writable roots stay internal and do not appear as user-facing workspace roots. 5. Then skim downstream call sites. The interesting pattern is raw-vs-effective access: state/proxy/bwrap paths keep the raw constrained profile, while execution, summaries, and user-visible status use the effective profile and workspace-root list. ## What Changed - added `[permissions.<id>.workspace_roots]` to the config model and schema - added runtime `workspace_roots` state to `Config`/`Permissions` and `ConfigOverrides` - made `Permissions` profile fields private and replaced direct mutation with accessors/setters - added `PermissionProfile` and `FileSystemSandboxPolicy` helpers for materializing `:workspace_roots` exact paths and deny-read globs across all roots - moved legacy additional writable roots into runtime workspace-root state instead of active profile modifications - removed `ActivePermissionProfileModification` and its app-server protocol/schema export - updated sandbox/status summary paths so internal writable roots are not reported as user workspace roots ## Verification Strategy The targeted tests cover the behavior at the layers where regressions are most likely: - `codex-rs/core/src/config/config_tests.rs` verifies config loading, legacy workspace-root seeding, effective profile materialization, and memory-root handling. - `codex-rs/core/src/config/permissions_tests.rs` verifies profile `workspace_roots` parsing and `:workspace_roots` scoped/glob compilation. - `codex-rs/protocol/src/permissions.rs` unit tests verify exact and glob materialization over multiple workspace roots. - `codex-rs/tui/src/status/tests.rs` and `codex-rs/utils/sandbox-summary/src/sandbox_summary.rs` verify the user-facing summaries show effective workspace roots and hide internal writes. I also ran `cargo check --tests` locally after the latest stack refresh to catch cross-crate API breakage from the private-field/accessor changes. --- [//]: # (BEGIN SAPLING FOOTER) Stack created with [Sapling](https://sapling-scm.com). Best reviewed with [ReviewStack](https://reviewstack.dev/openai/codex/pull/22610). * #22612 * #22611 * #22683 * __->__ #22610
viyatb-oai
reviewed
May 15, 2026
| )?; | ||
| next_configuration | ||
| .permission_profile_state | ||
| .set_legacy_permission_profile( |
Collaborator
There was a problem hiding this comment.
[P2] Preserve the active profile on cwd-only rebinding.
This branch is the cwd-only rederivation path. Before this refactor it updated the constrained permission profile in place and left active_permission_profile intact. Routing it through set_legacy_permission_profile(...) now drops the selected profile identity (and any bound profile metadata) whenever a workspace-style session changes cwd, so later snapshots/status surfaces stop reporting the named or built-in profile even though the user did not change permissions.
Suggested change
| .set_legacy_permission_profile( | |
| next_configuration.permission_profile_state = next_configuration | |
| .permission_profile_state | |
| .clone_with_permission_profile( | |
| PermissionProfile::from_runtime_permissions_with_enforcement( | |
| SandboxEnforcement::from_legacy_sandbox_policy(¤t_sandbox_policy), | |
| &file_system_sandbox_policy, | |
| current_network_sandbox_policy, | |
| ), | |
| )?; |
viyatb-oai
approved these changes
May 15, 2026
Collaborator
viyatb-oai
left a comment
viyatb-oai
left a comment
There was a problem hiding this comment.
Approving to unblock. I left one P2 regression note inline: the cwd-only rebind path now clears the active permission-profile identity by converting the state to Legacy; preserving the resolved state while replacing only the rebound profile value should fix it.
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 subscribe to this conversation on GitHub.
Already have an account?
Sign in.
2 participants
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 layer that follows the workspace-roots base merged in #22610.
#22610 adds
[permissions.<id>.workspace_roots]and keeps runtime workspace roots separate from the raw permission profile, but its in-memory representation is intentionally transitional:Permissionsstill carries the selected profile identity next to a constrainedPermissionProfile. That makes APIs such asset_constrained_permission_profile_with_active_profile()fragile because the id and value only mean the right thing when every caller keeps them in sync.This PR introduces a single resolved profile state so profile identity,
extends, the profile value, and profile-declared workspace roots travel together. The next PR, #22611, builds on this by changing the app-server turn API to select permission profiles by id plus runtime workspace roots.Stack Context
workspace_roots, runtime workspace roots, and:workspace_rootsmaterialization.PermissionProfileState.Keeping this separate from #22611 is deliberate: reviewers can validate the internal state invariant before reviewing the app-server protocol migration.
What Changed
ResolvedPermissionProfile::{Legacy, BuiltIn, Named}andPermissionProfileState.BuiltInPermissionProfileId.Permissionsparallel profile fields with onepermission_profile_state.set_constrained_permission_profile_with_active_profile()from session sync paths.SessionConfiguredcompatibility through explicit session snapshot helpers.&PermissionProfiledirectly.Review Guide
Start with
codex-rs/core/src/config/resolved_permission_profile.rs; it is the new invariant boundary. Then reviewcodex-rs/core/src/config/mod.rsto see how config loading records active profile identity and profile workspace roots. The remaining call-site changes are mostly mechanical fallout fromPermissions::permission_profile()returning&PermissionProfileinstead of&Constrained<PermissionProfile>.Verification
The existing config/session coverage now constructs and asserts through
PermissionProfileState. The workspace-root config test also asserts that profile-declared roots are preserved in the resolved state, which is the behavior #22611 relies on when runtime roots become mutable through the app-server API.Stack created with Sapling. Best reviewed with ReviewStack.