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: 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".
| workspace_roots: Some(runtime_workspace_roots_request.clone().unwrap_or_else( | ||
| || { | ||
| snapshot | ||
| .workspace_roots | ||
| .iter() | ||
| .map(AbsolutePathBuf::to_path_buf) | ||
| .collect() | ||
| }, | ||
| )), |
There was a problem hiding this comment.
When a turn selects permissions, these overrides force the config loader to materialize the selected profile against the current/requested runtime roots, and the code below then stores config.permissions.permission_profile() as the session's new profile. That accessor is already concrete, so symbolic :workspace_roots entries are lost; a later turn/start that only replaces runtimeWorkspaceRoots updates the snapshot roots but leaves the effective sandbox permissions pointed at the old roots. This makes command/file access stale after the sequence turn/start { permissions, runtimeWorkspaceRoots: A } followed by turn/start { runtimeWorkspaceRoots: B }.
Useful? React with 👍 / 👎.
| if let Some(workspace_roots) = updates.workspace_roots.clone() { | ||
| next_configuration.workspace_roots = workspace_roots; |
There was a problem hiding this comment.
When a turn updates only cwd, this leaves workspace_roots bound to the previous cwd. Because file_system_sandbox_policy() now uses the materialized profile, the later cwd-rebind branch no longer sees the symbolic :workspace_roots write and does not rederive permissions for the new cwd; callers that submit a cwd-only turn context therefore keep write access to the old workspace instead of the new one.
Useful? React with 👍 / 👎.
#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
2 times, most recently
from
99a5efa to
559afb6
Compare
bolinfest
force-pushed
the
pr22611
branch
2 times, most recently
from
5b6acbb to
cd12cc2
Compare
bolinfest
force-pushed
the
pr22611
branch
2 times, most recently
from
572a5c3 to
8429c89
Compare
bolinfest
force-pushed
the
pr22610
branch
2 times, most recently
from
9ccddb3 to
2fbcb85
Compare
bolinfest
force-pushed
the
pr22611
branch
2 times, most recently
from
f42586d to
fefc579
Compare
1 participant
Why
This PR builds on #22610 and is the app-server side of the alternative migration path we discussed as a comparison point for #22401 / #22402.
Once a named permission profile can carry its own immutable
workspace_roots, app-server no longer needs to mutate the selectedPermissionProfilejust to represent thread-specific filesystem context. The mutable part can instead live on the thread as explicit runtime workspace roots.What Changed
Option<String>) forthread/start,thread/resume,thread/fork, andturn/startPermissionProfileSelectionParams,PermissionProfileModificationParams,ActivePermissionProfileModification)runtimeWorkspaceRootsfields to the thread lifecycle and turn-start APIsVerification
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.rsThe key regression checks exercise that:
runtimeWorkspaceRootsresolve against the effective cwd on thread startthread/resumeStack created with Sapling. Best reviewed with ReviewStack.