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: d92aa0c7b3
ℹ️ 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".
| let Ok(sandbox_policy) = permission_profile.to_legacy_sandbox_policy(cwd.as_path()) | ||
| else { | ||
| tx.send(AppEvent::OpenWorldWritableWarningConfirmation { | ||
| preset: None, | ||
| sample_paths: Vec::new(), | ||
| extra_count: 0usize, | ||
| failed_scan: true, | ||
| }); | ||
| return; | ||
| }; |
There was a problem hiding this comment.
Hmm, I need to look into this...
There was a problem hiding this comment.
Updating so that we can avoid the spawn_blocking() altogether in the event of an early return.
## Why The migration away from `SandboxPolicy` needs new configs to start from permissions profiles instead of deriving profiles from legacy sandbox modes. Existing users can have empty `config.toml` files, and we should not rewrite user-owned config files that may live in shared repositories. This PR introduces built-in profile names so an empty config can resolve to a canonical `PermissionProfile`, while explicit named `[permissions]` profiles still behave predictably. ## What changed - Adds built-in `default_permissions` profile names: - `:read-only` maps to `PermissionProfile::read_only()`. - `:workspace` maps to the workspace-write profile, including project-root metadata carveouts. - `:danger-no-sandbox` maps to `PermissionProfile::Disabled`, preserving the distinction between no sandbox and a broad managed sandbox. - Reserves the `:` prefix for built-in profiles so user-defined `[permissions]` profiles cannot collide with future built-ins. - Allows `default_permissions` to reference a built-in profile without requiring a `[permissions]` table. - Makes an otherwise empty config choose a built-in profile by trust/platform context: trusted or untrusted project roots use `:workspace` when the platform supports that sandbox, while roots without a trust decision use `:read-only`. - Keeps legacy `sandbox_mode` configs on the legacy path, and still rejects user-defined `[permissions]` profiles that omit `default_permissions` so we do not silently guess among custom profiles. - Preserves compatibility behavior for implicit defaults: bare `network.enabled = true` allows runtime network without starting the managed proxy, explicit profile proxy policy still starts the proxy, and implicit workspace/add-dir roots keep legacy metadata carveouts. ## Verification - `cargo test -p codex-core builtin --lib` - `cargo test -p codex-core profile_network_proxy_config` - `cargo test -p codex-core implicit_builtin_workspace_profile_preserves_add_dir_metadata_carveouts` - `cargo test -p codex-core permissions_profiles_network_enabled_allows_runtime_network_without_proxy` - `cargo test -p codex-core permissions_profiles_proxy_policy_starts_managed_network_proxy` ## Documentation Public Codex config docs should mention these built-in names when the `[permissions]` config format is ready to document as stable. --- [//]: # (BEGIN SAPLING FOOTER) Stack created with [Sapling](https://sapling-scm.com). Best reviewed with [ReviewStack](https://reviewstack.dev/openai/codex/pull/19900). * #20041 * #20040 * #20037 * #20035 * #20034 * #20033 * #20032 * #20030 * #20028 * #20027 * #20026 * #20024 * #20021 * #20018 * #20016 * #20015 * #20013 * #20011 * #20010 * #20008 * __->__ #19900
There was a problem hiding this comment.
I reviewed the code and performed some sanity checks on the areas touched by this PR:
-
/statusreports Default for the normal workspace-write preset -
Switching
/permissionsfrom Default to Full Access updates the history line and/statustoPermissions: Full Access. -
Starting with
--sandbox read-only --add-dir /tmp/codex-pr20008-extrarejects the extra writable root. -
Starting with
--sandbox workspace-write --add-dir ...and--sandbox danger-full-access --add-dir ...starts successfully without the add-dir error. -
After switching to Full Access, the next submitted turn is actually sent with
permission_profile: disabledandsandbox-policy: danger-full-access, so the runtime permission change is taking effect. Verified it directly on the rollout file.
One unrelated behavior I noticed while checking this: on main as well as this branch, starting with a fresh CODEX_HOME and running the first /status shows:
Permissions: Custom (workspace-write, on-request)
If I switch to another permission level and then back to Default, /status then reports Default as expected. I am not familiar enough with the current implementation to tell whether that is intentional.
I am approving it but I also left two review comments on potential issues I noticed in the diff. I will leave it to you to decide whether either is worth addressing here.
| if !file_system_policy | ||
| .get_writable_roots_with_cwd(cwd) | ||
| .is_empty() | ||
| { |
There was a problem hiding this comment.
Should we use file_system_policy.can_write_path_with_cwd(cwd, cwd) here instead like we do in mod.rs?
There was a problem hiding this comment.
Addressed in d6fe246. --add-dir warnings now check whether the active filesystem policy can write the current workspace (can_write_path_with_cwd(cwd, cwd)) instead of treating any writable root as sufficient, and I added a regression test for a profile that can write elsewhere but not cwd.
| _ => false, | ||
| match preset.id { | ||
| "full-access" => matches!(current_permission_profile, PermissionProfile::Disabled), | ||
| "read-only" => { |
There was a problem hiding this comment.
This one was identified by Codex but here's my understanding of it:
It seems like this classifies any managed profile that can't write cwd as Read Only. Isn't it broader than the actual read-only access?
A custom profile that can write /tmp/foo while not writing the repo still satisfies this check, so /permissions would show Read Only even though the session is actually writable elsewhere?
Could this required a real read-only policy, rather than only !can_write_path_with_cwd(cwd, cwd)?
There was a problem hiding this comment.
Addressed in d6fe246. The Read Only preset now requires a managed profile with no full-disk write access and no writable roots, rather than only checking that cwd is not writable. I also added coverage for a managed profile that can write /tmp/writable but not the workspace so it is no longer classified as Read Only.
bolinfest
force-pushed
the
pr20008
branch
4 times, most recently
from
f116112 to
bad0fd2
Compare
2 participants
Summary
SandboxPolicyvalues to canonicalPermissionProfilevalues across presets, app events, chat widget state, app commands, thread routing, and cached thread session state.permissionProfile, while remote sessions send only a legacysandboxprojection and fall back to read-only when a custom profile cannot be projected.Verification
rg '\bSandboxPolicy\b' codex-rs/tui -nreturns no matches.cargo test -p codex-tuicargo check -p codex-tui --testscargo test -p codex-tui additional_dirsjust fmtjust fix -p codex-tuiStack created with Sapling. Best reviewed with ReviewStack.