Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4554fcc19e
ℹ️ 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".
#22624) ## Why This is a small precursor to the larger permissions-migration work. Both the comparison stack in [#22401](#22401) / [#22402](#22402) and the alternate stack in [#22610](#22610) / [#22611](#22611) / [#22612](#22612) are easier to review if the terminology is already settled underneath them. Because `:project_roots` and `:danger-no-sandbox` have not shipped as stable user-facing surface area, carrying them forward as aliases would just add more migration logic to the later stacks. This PR removes that ambiguity now so the follow-on work can rely on one spelling for each built-in concept. ## What Changed - renamed the config-facing special filesystem key from `:project_roots` to `:workspace_roots` - dropped unpublished `:project_roots` parsing support in `core/src/config/permissions.rs`, so new config only recognizes `:workspace_roots` - renamed the built-in full-access permission profile id from `:danger-no-sandbox` to `:danger-full-access` - dropped unpublished `:danger-no-sandbox` support entirely, including the old active-profile canonicalization path, and added explicit rejection coverage for the legacy id - introduced shared built-in permission-profile id constants in `codex-rs/protocol/src/models.rs` - updated `core`, `app-server`, and `tui` call sites that special-case built-in profiles to use the shared constants and canonical ids - updated tests and the Linux sandbox README to use `:workspace_roots` / `:danger-full-access` ## Verification I focused verification on the three places this rename can regress: config parsing, active-profile identity surfaced back out of `core`, and user/server call sites that special-case built-in profiles. Targeted checks: - `config::tests::default_permissions_can_select_builtin_profile_without_permissions_table` - `config::tests::default_permissions_read_only_applies_additional_writable_roots_as_modifications` - `config::tests::default_permissions_can_select_builtin_full_access_profile` - `config::tests::legacy_danger_no_sandbox_is_rejected` - `workspace_root` filtered `codex-core` tests - `request_processors::thread_processor::thread_processor_tests::thread_processor_behavior_tests::requested_permissions_trust_project_uses_permission_profile_intent` - `suite::v2::turn_start::turn_start_rejects_invalid_permission_selection_before_starting_turn` - `status::tests::status_snapshot_shows_auto_review_permissions` - `status::tests::status_permissions_full_disk_managed_with_network_is_danger_full_access` - `app_server_session::tests::embedded_turn_permissions_use_active_profile_selection`
99a5efa to
559afb6
Compare
5b6acbb to
cd12cc2
Compare
572a5c3 to
8429c89
Compare
9ccddb3 to
2fbcb85
Compare
f42586d to
fefc579
Compare
## Why This PR is the invariant-cleanup layer that follows the workspace-roots base merged in [#22610](#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: `Permissions` still carries the selected profile identity next to a constrained `PermissionProfile`. That makes APIs such as `set_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](#22611), builds on this by changing the app-server turn API to select permission profiles by id plus runtime workspace roots. ## Stack Context - #22610, now merged: adds profile-declared `workspace_roots`, runtime workspace roots, and `:workspace_roots` materialization. - This PR: replaces the parallel active-profile/profile-value fields with `PermissionProfileState`. - #22611: switches app-server turn updates toward profile ids plus runtime workspace roots. - #22612: updates TUI/exec summaries to show the effective workspace roots. Keeping this separate from #22611 is deliberate: reviewers can validate the internal state invariant before reviewing the app-server protocol migration. ## What Changed - Added `ResolvedPermissionProfile::{Legacy, BuiltIn, Named}` and `PermissionProfileState`. - Typed built-in profile ids with `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 session sync paths. - Kept trusted session replay/`SessionConfigured` compatibility through explicit session snapshot helpers. - Updated session configuration, MCP initialization, app-server, exec, TUI, and guardian call sites to consume `&PermissionProfile` directly. ## Review Guide Start with `codex-rs/core/src/config/resolved_permission_profile.rs`; it is the new invariant boundary. Then review `codex-rs/core/src/config/mod.rs` to see how config loading records active profile identity and profile workspace roots. The remaining call-site changes are mostly mechanical fallout from `Permissions::permission_profile()` returning `&PermissionProfile` instead 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. --- [//]: # (BEGIN SAPLING FOOTER) Stack created with [Sapling](https://sapling-scm.com). Best reviewed with [ReviewStack](https://reviewstack.dev/openai/codex/pull/22683). * #22612 * #22611 * __->__ #22683
viyatb-oai
left a comment
There was a problem hiding this comment.
Approving to unblock. I left one P2 regression suggestion inline: keep session permission state symbolic so turn-level runtimeWorkspaceRoots-only updates actually rebind :workspace_roots instead of silently continuing to use the old concrete roots.
viyatb-oai
left a comment
There was a problem hiding this comment.
I found a P1 compile blocker on a second pass, so I am withdrawing the unblock approval for now: codex-rs/tui/src/chatwidget/tests/history_replay.rs still has one ThreadSessionState initializer in session_configured_preserves_profile_workspace_roots without the new required runtime_workspace_roots field. The neighboring initializers were updated, but this one was missed, so the TUI test target will not compile until it is added (the natural value there is session_workspace_roots.clone()).
The earlier P2 suggestion about keeping session permission state symbolic still stands.
d525c4b to
1d60544
Compare
viyatb-oai
left a comment
There was a problem hiding this comment.
Re-reviewed current head bc34cfa2c8. The two issues I raised are fixed: session permission state now stays symbolic for runtimeWorkspaceRoots rebinding, and the missing runtime_workspace_roots test field was added. Approving.
2391b44 to
e4b24d9
Compare
Why
This PR builds on #22610 and is the app-server side of the migration from mutable per-turn
SandboxPolicyreplacement toward selecting immutable permission profiles by id plus mutable runtime workspace roots.Once permission profiles can carry their own immutable
workspace_roots, app-server no longer needs to mutate the selectedPermissionProfilejust to represent thread-specific filesystem context. The mutable part now lives on the thread as explicitruntimeWorkspaceRoots, while:workspace_rootsremains symbolic until the sandbox is realized for a turn.What Changed
thread/start,thread/resume,thread/fork, andturn/start.PermissionProfileSelectionParams,PermissionProfileModificationParams,ActivePermissionProfileModification).runtimeWorkspaceRootsfields to the thread lifecycle and turn-start APIs.:workspace_rootscorrectly.Verification
Targeted coverage for this layer lives in:
codex-rs/app-server-protocol/src/protocol/v2/tests.rscodex-rs/app-server/tests/suite/v2/thread_start.rscodex-rs/app-server/tests/suite/v2/thread_resume.rscodex-rs/app-server/tests/suite/v2/turn_start.rscodex-rs/core/src/session/tests.rsThe key regression checks exercise that:
runtimeWorkspaceRootsresolve against the effective cwd on thread start.thread/resume.Stack created with Sapling. Best reviewed with ReviewStack.