Conversation
💡 Codex Reviewcodex/codex-rs/tui/src/app_server_session.rs Lines 550 to 553 in 05359f1 In remote app-server mode, this fallback forces ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
|
[codex] Addressed in 0cf4684. Remote turn/start now uses a legacy-compatible workspace-write projection for profiles that cannot be represented directly, preserving non-legacy writable roots instead of falling back to read-only; added focused coverage for the compatibility projection. |
5f618ba to
afb15fb
Compare
## 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
7328f40 to
7de307d
Compare
## Summary - Move TUI permission state from legacy `SandboxPolicy` values to canonical `PermissionProfile` values across presets, app events, chat widget state, app commands, thread routing, and cached thread session state. - Keep app-server compatibility boundaries explicit: embedded sessions send `permissionProfile`, while remote sessions send only a legacy `sandbox` projection and fall back to read-only when a custom profile cannot be projected. - Update status/add-dir UI summaries and snapshots to render the active permission profile, including workspace profiles selected by the new built-in defaults. ## Verification - `rg '\bSandboxPolicy\b' codex-rs/tui -n` returns no matches. - `cargo test -p codex-tui` - `cargo check -p codex-tui --tests` - `cargo test -p codex-tui additional_dirs` - `just fmt` - `just fix -p codex-tui` --- [//]: # (BEGIN SAPLING FOOTER) Stack created with [Sapling](https://sapling-scm.com). Best reviewed with [ReviewStack](https://reviewstack.dev/openai/codex/pull/20008). * #20041 * #20040 * #20037 * #20035 * #20034 * #20033 * #20032 * #20030 * #20028 * #20027 * #20026 * #20024 * #20021 * #20018 * #20016 * #20015 * #20013 * #20011 * #20010 * __->__ #20008
etraut-openai
left a comment
There was a problem hiding this comment.
Overall, looks good. One potential hole in the translation logic and one nit for your consideration.
| .is_enabled() | ||
| .then_some(codex_app_server_protocol::SandboxMode::DangerFullAccess) | ||
| } else if file_system_policy.can_write_path_with_cwd(cwd, cwd) { | ||
| } else if !file_system_policy |
There was a problem hiding this comment.
This doesn't seem to be a precise translation. If the profile has any writable roots at all, we're translating this to "WorkspaceWrite", which has historically meant "all roots are writable". Is this intended?
There was a problem hiding this comment.
Addressed in 631451a. The remote legacy projection now only emits workspace-write when the active profile can write the configured cwd/project root. Profiles with only non-cwd writable roots fall back to read-only because the legacy remote field cannot represent arbitrary roots safely; added coverage for both cases.
| @@ -0,0 +1,102 @@ | |||
| use codex_protocol::models::PermissionProfile; | |||
There was a problem hiding this comment.
Nit: Add header comment indicating the purpose of this module?
There was a problem hiding this comment.
Addressed in 631451a by adding a module-level comment explaining that permission_compat contains compatibility projections from canonical permission profiles into legacy shapes still required by older or remote app-server APIs.
viyatb-oai
left a comment
There was a problem hiding this comment.
+1 to @etraut-openai's comment but otherwise lgtm
Summary
PermissionProfile-based turn submission helpers tocore_test_support, while keeping the legacySandboxPolicyhelper for tests that intentionally exercise legacy fallback behavior.TestCodex::submit_turn()path to send a realPermissionProfileplus the required legacy compatibility projection inOp::UserTurn.SandboxPolicy::{DangerFullAccess, ReadOnly}toPermissionProfile::{Disabled, read_only}.cwd.SandboxPolicyreferences incodex-rs/core/testsfrom 47 files to 41 files without changing production behavior.Testing
cargo check -p codex-core --testscargo test -p codex-tui compatibility_profile_preserves_unbridgeable_write_rootscargo test -p codex-tui sandbox_mode_preserves_non_cwd_write_roots_for_remote_sessionsjust fmtjust fix -p core_test_supportjust fix -p codex-core