Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6c3262458d
ℹ️ 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".
| approval_policy: constrained_approval_policy.value, | ||
| permission_profile: constrained_permission_profile.value, | ||
| active_permission_profile, | ||
| workspace_roots, |
There was a problem hiding this comment.
When a caller supplies runtime workspace roots (--add-dir / ConfigOverrides.workspace_roots), they are only stored in this new Permissions.workspace_roots side field. The session path later copies config.permissions.permission_profile.clone() into SessionConfiguration and does not carry this field, so actual turn sandboxes materialize :project_roots only against the cwd and drop the extra roots; for example codex --add-dir ../backend can show the backend root in config/status while shell/apply_patch turns cannot write it, and profile deny globs for that runtime root are not enforced. Materialize these roots into the stored profile before session creation or pass the workspace-root list through the session/turn configuration.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in the current head. Session creation now stores the effective permission profile after materializing runtime workspace roots, so --add-dir / runtime roots are carried into turn sandboxes instead of only appearing in config/status. The focused history-replay coverage for this path passes on the current PR head.
| /// inheritance. This is always `None` until that config feature exists. | ||
| #[serde(default, skip_serializing_if = "Option::is_none")] | ||
| #[ts(optional)] | ||
| pub extends: Option<String>, |
There was a problem hiding this comment.
Removing the modifications member here leaves existing callers broken: codex-rs/app-server-protocol/src/protocol/v2/permissions.rs still converts CoreActivePermissionProfileModification, and both codex-rs/exec/src/lib.rs and codex-rs/tui/src/app_server_session.rs still read active.modifications when round-tripping additional writable roots. Any build that includes those crates now fails, and even after fixing imports this would drop additionalWritableRoot selections from the active-profile API unless all conversions are updated together.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in the current head. The app-server protocol conversions and embedded client call sites were updated together: core no longer carries the old active-profile modification type, and the embedded protocol still emits bounded AdditionalWritableRoot modifications where that compatibility path needs them.
| "codex-otel", | ||
| "codex-protocol", | ||
| "codex-state", | ||
| "codex-utils-absolute-path", |
There was a problem hiding this comment.
This lockfile entry now claims codex-rollout depends on codex-utils-absolute-path, but codex-rs/rollout/Cargo.toml was not changed and does not declare that dependency; the same unrelated addition appears for codex-thread-store at line 3711. With the lockfile no longer matching the workspace manifests, Cargo --locked/CI will reject or rewrite it, so regenerate Cargo.lock from the actual manifest changes and drop these unrelated dependency entries.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in the current head. The spurious codex-utils-absolute-path entries are no longer present for codex-rollout or codex-thread-store in Cargo.lock, and the current PR head is passing CI.
#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`
bolinfest
force-pushed
the
pr22610
branch
8 times, most recently
from
6589c91 to
5e3f04d
Compare
| fn permissions_selection_from_active_profile( | ||
| active: ActivePermissionProfile, | ||
| ) -> PermissionProfileSelectionParams { | ||
| let modifications = active | ||
| .modifications | ||
| .into_iter() | ||
| .map(|modification| match modification { | ||
| ActivePermissionProfileModification::AdditionalWritableRoot { path } => { | ||
| PermissionProfileModificationParams::AdditionalWritableRoot { path } | ||
| } | ||
| }) | ||
| .collect::<Vec<_>>(); | ||
| PermissionProfileSelectionParams::Profile { | ||
| id: active.id, | ||
| modifications: (!modifications.is_empty()).then_some(modifications), | ||
| modifications: None, | ||
| } | ||
| } |
There was a problem hiding this comment.
This still needs the caller sites to pass cwd plus the runtime root slice, and to restore the removed PermissionProfileModificationParams import, but the profile handoff should keep emitting bounded writable-root modifications instead of dropping them.
| fn permissions_selection_from_active_profile( | |
| active: ActivePermissionProfile, | |
| ) -> PermissionProfileSelectionParams { | |
| let modifications = active | |
| .modifications | |
| .into_iter() | |
| .map(|modification| match modification { | |
| ActivePermissionProfileModification::AdditionalWritableRoot { path } => { | |
| PermissionProfileModificationParams::AdditionalWritableRoot { path } | |
| } | |
| }) | |
| .collect::<Vec<_>>(); | |
| PermissionProfileSelectionParams::Profile { | |
| id: active.id, | |
| modifications: (!modifications.is_empty()).then_some(modifications), | |
| modifications: None, | |
| } | |
| } | |
| fn permissions_selection_from_active_profile( | |
| active: ActivePermissionProfile, | |
| cwd: &std::path::Path, | |
| workspace_roots: &[AbsolutePathBuf], | |
| ) -> PermissionProfileSelectionParams { | |
| let modifications = workspace_roots | |
| .iter() | |
| .filter(|root| root.as_path() != cwd) | |
| .cloned() | |
| .map(|path| PermissionProfileModificationParams::AdditionalWritableRoot { path }) | |
| .collect::<Vec<_>>(); | |
| PermissionProfileSelectionParams::Profile { | |
| id: active.id, | |
| modifications: (!modifications.is_empty()).then_some(modifications), | |
| } | |
| } |
There was a problem hiding this comment.
Addressed in the current head. permissions_selection_from_active_profile now takes cwd and the runtime workspace-root slice, emits AdditionalWritableRoot only for non-cwd roots, and there is embedded-turn coverage for extra workspace roots.
| let approval_policy = AskForApproval::from(config.permissions.approval_policy.value()); | ||
| let permission_profile = config.permissions.permission_profile(); | ||
| let permission_profile = config.permissions.effective_permission_profile(); | ||
| let workspace_roots = config.permissions.workspace_roots(); | ||
| let mut config_entries = vec
2 participants
Why
This is the configuration/model half of the alternative permissions migration we discussed as a comparison point for #22401 and #22402.
The old
workspace-writemodel mixes three concerns that we want to keep separate:cwd,--add-dir, and legacy workspace-write configThis PR gives permission profiles first-class
workspace_rootsso users can opt multiple repositories into the same:workspace_rootsrules without using broad absolute-path write grants. It also starts separating the raw selected profile from the effective runtime profile by makingPermissionsexpose explicit accessors instead of public mutable fields.A representative
config.tomllooks like this:If Codex starts in
~/code/codexwith that profile selected, the effective workspace-root set becomes:~/code/codexfrom the runtimecwd~/code/openaifrom the profile~/code/developers-websitefrom the profileThe
:workspace_rootsrules are materialized across each root, so.git,.codex, and.vscodestay scoped the same way everywhere. Runtime additions such as--add-dircan still layer on later stack entries without mutating the selected profile.Stack Shape
This PR intentionally stops before the profile-identity cleanup in #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:
Permissionscarries 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, immutablePermissionProfile, and profile-declared workspace roots together. That follow-up removes APIs such asset_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:
Start with
codex-rs/core/src/config/mod.rs.This is the main shape change in the base slice.
Permissionsnow stores a private rawConstrained<PermissionProfile>plus runtimeworkspace_roots. Callers usepermission_profile()when they need the raw constrained value andeffective_permission_profile()when they need a materialized runtime profile. As noted above, #22683 replaces this transitional shape with a resolved profile state that keeps identity and profile data together.Review
codex-rs/config/src/permissions_toml.rsandcodex-rs/core/src/config/permissions.rs.These add
[permissions.<id>.workspace_roots], resolve enabled entries relative to the policy cwd, and keep:workspace_rootsdeny-read glob patterns symbolic until the actual roots are known.Review
codex-rs/protocol/src/permissions.rsandcodex-rs/protocol/src/models.rs.These add the policy/profile materialization helpers that expand exact
:workspace_rootsentries and scoped deny-read globs over every workspace root. This is also whereActivePermissionProfileModificationis removed from the core model.Review the legacy bridge in
Config::load_from_base_config_with_overridesandConfig::set_legacy_sandbox_policy.This is where legacy
workspace-writeroots become runtime workspace roots, while Codex internal writable roots stay internal and do not appear as user-facing workspace roots.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
[permissions.<id>.workspace_roots]to the config model and schemaworkspace_rootsstate toConfig/PermissionsandConfigOverridesPermissionsprofile fields private and replaced direct mutation with accessors/settersPermissionProfileandFileSystemSandboxPolicyhelpers for materializing:workspace_rootsexact paths and deny-read globs across all rootsActivePermissionProfileModificationand its app-server protocol/schema exportVerification Strategy
The targeted tests cover the behavior at the layers where regressions are most likely:
codex-rs/core/src/config/config_tests.rsverifies config loading, legacy workspace-root seeding, effective profile materialization, and memory-root handling.codex-rs/core/src/config/permissions_tests.rsverifies profileworkspace_rootsparsing and:workspace_rootsscoped/glob compilation.codex-rs/protocol/src/permissions.rsunit tests verify exact and glob materialization over multiple workspace roots.codex-rs/tui/src/status/tests.rsandcodex-rs/utils/sandbox-summary/src/sandbox_summary.rsverify the user-facing summaries show effective workspace roots and hide internal writes.I also ran
cargo check --testslocally after the latest stack refresh to catch cross-crate API breakage from the private-field/accessor changes.Stack created with Sapling. Best reviewed with ReviewStack.