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: 393a21dcd6
ℹ️ 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 profiles_are_active = matches!( | ||
| permission_config_syntax, | ||
| Some(PermissionConfigSyntax::Profiles) | ||
| ) || (permission_config_syntax.is_none() | ||
| && has_permission_profiles); | ||
| ) || permission_config_syntax.is_none(); |
There was a problem hiding this comment.
Treating permission_config_syntax.is_none() as profiles_are_active routes configs with no explicit permission syntax through built-in profiles, which bypasses ConfigToml::derive_permission_profile and therefore ignores [sandbox_workspace_write] knobs. A trusted/untrusted project that previously relied on implicit workspace-write plus sandbox_workspace_write (for network_access or extra writable_roots) will silently lose those settings after this change, changing runtime permissions for existing configs.
Useful? React with 👍 / 👎.
## Why `ThreadSessionState` is the TUI's cached view of an app-server session. To make `PermissionProfile` the canonical runtime permissions model, cached thread sessions need to always have a profile instead of treating the profile as an optional supplement to a legacy `sandbox` response field. The main compatibility concern is older app-server v2 lifecycle responses that only include `sandbox` and omit `permissionProfile`: - `thread/start` -> `ThreadStartResponse.sandbox` - `thread/resume` -> `ThreadResumeResponse.sandbox` - `thread/fork` -> `ThreadForkResponse.sandbox` Those responses must still hydrate correctly when the TUI is pointed at an older app-server. This PR converts the legacy `sandbox` value into a `PermissionProfile` immediately at response ingestion time, using the response `cwd`, so cached sessions do not carry an optional profile that can later reinterpret cwd-bound grants against a different thread cwd. This fallback is intentionally boundary compatibility. The follow-up PRs in this stack continue the cleanup by making `SessionConfiguredEvent` profile-only, deriving sandbox projections from snapshots only when an API still needs them, and then removing `sandbox_policy` from `ThreadSessionState`. ## What Changed - Makes `ThreadSessionState.permission_profile` required. - Converts legacy app-server response `sandbox` values into a `PermissionProfile` at ingestion time using the response cwd. - Ensures `thread/read` hydration does not reuse a primary session profile that may be anchored to a different cwd; it uses the active widget permission settings for the read thread fallback instead of reusing cached primary-session permissions. - Keeps the app-server request path unchanged: embedded sessions send profiles, while remote sessions continue using legacy sandbox overrides for compatibility. ## Verification - `cargo test -p codex-tui thread_read --lib` - `cargo test -p codex-tui permission_settings_sync_preserves_active_profile_only_rules --lib` - `cargo test -p codex-tui resume_response_restores_turns_from_thread_items --lib` - `cargo test -p codex-tui thread_session_state::tests --lib` --- [//]: # (BEGIN SAPLING FOOTER) Stack created with [Sapling](https://sapling-scm.com). Best reviewed with [ReviewStack](https://reviewstack.dev/openai/codex/pull/19773). * #19900 * #19899 * #19776 * #19775 * #19774 * __->__ #19773
bolinfest
force-pushed
the
pr19899
branch
2 times, most recently
from
28113b6 to
669323a
Compare
## Why `SessionConfiguredEvent` is the internal event that tells clients what permissions are active for a session. Emitting both `sandbox_policy` and `permission_profile` leaves two possible authorities and forces every consumer to decide which one to honor. At this point in the migration, the profile is expressive enough to represent managed, disabled, and external sandbox enforcement, so the internal event can be profile-only. The wire compatibility concern is older serialized events or rollout data that only contain `sandbox_policy`; those still need to deserialize. ## What Changed - Removes `sandbox_policy` from `SessionConfiguredEvent` and makes `permission_profile` required. - Adds custom deserialization so old payloads with only `sandbox_policy` are upgraded to a cwd-anchored `PermissionProfile`. - Updates core event emission and TUI session handling to sync permissions from the profile directly. - Updates app-server response construction to derive the legacy `sandbox` response field from the active thread snapshot instead of from `SessionConfiguredEvent`. - Updates yolo-mode display logic to treat both `PermissionProfile::Disabled` and managed unrestricted filesystem plus enabled network as full-access, while still preserving the distinction between no sandbox and external sandboxing. ## Verification - `cargo test -p codex-protocol session_configured_event --lib` - `cargo test -p codex-protocol serialize_event --lib` - `cargo test -p codex-exec session_configured --lib` - `cargo test -p codex-app-server thread_response_permission_profile_preserves_enforcement --lib` - `cargo test -p codex-core session_configured_reports_permission_profile_for_external_sandbox --lib` - `cargo test -p codex-tui session_configured --lib` - `cargo test -p codex-tui yolo_mode_includes_managed_full_access_profiles --lib` --- [//]: # (BEGIN SAPLING FOOTER) Stack created with [Sapling](https://sapling-scm.com). Best reviewed with [ReviewStack](https://reviewstack.dev/openai/codex/pull/19774). * #19900 * #19899 * #19776 * #19775 * __->__ #19774
## Why `ThreadConfigSnapshot` is used by app-server and thread metadata code as a stable view of active runtime settings. Keeping both `sandbox_policy` and `permission_profile` in the snapshot duplicates permission state and makes it possible for the legacy projection to drift from the canonical profile. The legacy `sandbox` value is still needed at app-server compatibility boundaries, so this PR derives it on demand from the snapshot profile and cwd instead of storing it. ## What Changed - Removes `ThreadConfigSnapshot.sandbox_policy`. - Adds `ThreadConfigSnapshot::sandbox_policy()` as a compatibility projection from `permission_profile` plus `cwd`. - Updates app-server response/metadata code and tests to call the projection only where legacy fields still exist. - Keeps snapshot construction profile-only so split filesystem rules, disabled enforcement, and external enforcement remain represented by the canonical profile. ## Verification - `cargo test -p codex-app-server thread_response_permission_profile_preserves_enforcement --lib` - `cargo test -p codex-core dispatch_reclaims_stale_global_lock_and_starts_consolidation --lib` --- [//]: # (BEGIN SAPLING FOOTER) Stack created with [Sapling](https://sapling-scm.com). Best reviewed with [ReviewStack](https://reviewstack.dev/openai/codex/pull/19775). * #19900 * #19899 * #19776 * __->__ #19775
| let configured_network_proxy_config = network_proxy_config_for_profile_selection( | ||
| cfg.permissions.as_ref(), | ||
| default_permissions, | ||
| )?; |
There was a problem hiding this comment.
[P2] Keep profile network enablement separate from proxy startup
One network-side detail to keep separate from the legacy sandbox mapping: [permissions.<profile>.network].enabled = true should map to the sandbox network bit (NetworkSandboxPolicy::Enabled, and legacy network_access = true when projected), but it should not by itself start the managed network proxy. Users coming from legacy network_access = true expect either direct/all network allowed or no network, not an implicit proxy path.
Right now this selection helper builds configured_network_proxy_config from the selected profile's NetworkToml, and NetworkToml::apply_to_network_proxy_config() copies network.enabled into NetworkProxyConfig.network.enabled. Later config loading keeps Config.permissions.network = Some(...) whenever that proxy config is enabled, so a profile that only wants sandbox network enabled can also activate the proxy.
Suggested fix: treat NetworkToml.enabled as input to compile_network_sandbox_policy() only, and do not let it flip NetworkProxyConfig.network.enabled in network_proxy_config_for_profile_selection() / network_proxy_config_from_profile_network(). Keep proxy startup behind a separate explicit knob such as enable_network_proxy (or managed network requirements), while still allowing the other profile network fields (domains, sockets, proxy URL, local binding) to overlay proxy policy when that separate proxy path is enabled.
There was a problem hiding this comment.
[codex] Addressed in 346a2a1. Profile network.enabled now maps to the runtime network sandbox bit without starting the managed proxy; managed network requirements remain the separate proxy-start path and the affected integration tests now inject those requirements during config construction. Added config/helper regression coverage for this.
2 participants
Why
The migration away from
SandboxPolicyneeds new configs to start from permissions profiles instead of deriving profiles from legacy sandbox modes. Existing users can have emptyconfig.tomlfiles, 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
default_permissionsprofile names::read-onlymaps toPermissionProfile::read_only().:workspacemaps to the workspace-write profile, including project-root metadata carveouts.:danger-no-sandboxmaps toPermissionProfile::Disabled, preserving the distinction between no sandbox and a broad managed sandbox.:prefix for built-in profiles so user-defined[permissions]profiles cannot collide with future built-ins.default_permissionsto reference a built-in profile without requiring a[permissions]table.:workspacewhen the platform supports that sandbox, while roots without a trust decision use:read-only.sandbox_modeconfigs on the legacy path, and still rejects user-defined[permissions]profiles that omitdefault_permissionsso we do not silently guess among custom profiles.network.enabled = trueallows 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 --libcargo test -p codex-core profile_network_proxy_configcargo test -p codex-core implicit_builtin_workspace_profile_preserves_add_dir_metadata_carveoutscargo test -p codex-core permissions_profiles_network_enabled_allows_runtime_network_without_proxycargo test -p codex-core permissions_profiles_proxy_policy_starts_managed_network_proxyDocumentation
Public Codex config docs should mention these built-in names when the
[permissions]config format is ready to document as stable.Stack created with Sapling. Best reviewed with ReviewStack.