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: af1dbc447b
ℹ️ 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".
| if sandbox.is_some() && permission_profile.is_some() { | ||
| self.send_invalid_request_error( | ||
| request_id, | ||
| "`permissionProfile` cannot be combined with `sandbox`".to_string(), | ||
| ) |
There was a problem hiding this comment.
This validation runs only after resume_running_thread(...) returns, so for already-loaded threads the method can return early with a successful resume response and never hit this check. That means thread/resume accepts requests that include both sandbox and permissionProfile when the thread is running, but rejects the same payload when it is not running. This state-dependent behavior violates the new mutual-exclusion contract and can hide client bugs.
Useful? React with 👍 / 👎.
| sandbox_mode: sandbox.map(SandboxMode::to_core), | ||
| permission_profile: permission_profile.map(Into::into), | ||
| codex_linux_sandbox_exe: self.arg0_paths.codex_linux_sandbox_exe.clone(), |
There was a problem hiding this comment.
When a request sets permissionProfile (and no sandbox), this builder leaves sandbox_mode unset, but thread_start_task’s trust decision for downgraded requests relies on typesafe_overrides.sandbox_mode (requested_sandbox_trusts_project). In constrained environments, a full-access permissionProfile can be downgraded to read-only and then fail to mark the cwd as trusted, unlike the equivalent sandbox request path. This makes the two override APIs behave inconsistently for the same user intent.
Useful? React with 👍 / 👎.
bolinfest
force-pushed
the
pr18279
branch
2 times, most recently
from
78e68a1 to
844c1fa
Compare
bolinfest
force-pushed
the
pr18278
branch
2 times, most recently
from
1d58b0b to
56b3b8a
Compare
bolinfest
force-pushed
the
pr18279
branch
2 times, most recently
from
36b39b0 to
536d5c4
Compare
## Why #18275 anchors session-scoped `:cwd` and `:project_roots` grants to the request cwd before recording them for reuse. Relative deny glob entries need the same treatment. Without anchoring, a stored session permission can keep a pattern such as `**/*.env` relative, then reinterpret that deny against a later turn cwd. That makes the persisted profile depend on the cwd at reuse time instead of the cwd that was reviewed and approved. ## What changed `intersect_permission_profiles` now materializes retained `FileSystemPath::GlobPattern` entries against the request cwd, matching the existing materialization for cwd-sensitive special paths. Materialized accepted grants are now deduplicated before deny retention runs. This keeps the sticky-grant preapproval shape stable when a repeated request is merged with the stored grant and both `:cwd = write` and the materialized absolute cwd write are present. The preapproval check compares against the same materialized form, so a later request for the same cwd-relative deny glob still matches the stored anchored grant instead of re-prompting or rejecting. Tests cover both the storage path and the preapproval path: a session-scoped `:cwd = write` grant with `**/*.env = none` is stored with both the cwd write and deny glob anchored to the original request cwd, cannot be reused from a later cwd, and remains preapproved when re-requested from the original cwd after merging with the stored grant. ## Verification - `cargo test -p codex-sandboxing policy_transforms` - `cargo test -p codex-core --lib relative_deny_glob_grants_remain_preapproved_after_materialization` - `cargo clippy -p codex-sandboxing --tests -- -D clippy::redundant_clone` - `cargo clippy -p codex-core --lib -- -D clippy::redundant_clone` --- [//]: # (BEGIN SAPLING FOOTER) Stack created with [Sapling](https://sapling-scm.com). Best reviewed with [ReviewStack](https://reviewstack.dev/openai/codex/pull/18867). * #18288 * #18287 * #18286 * #18285 * #18284 * #18283 * #18282 * #18281 * #18280 * #18279 * #18278 * #18277 * #18276 * __->__ #18867
bolinfest
force-pushed
the
pr18278
branch
2 times, most recently
from
68e82a9 to
fcfb087
Compare
## Why `Permissions` should not store a separate `PermissionProfile` that can drift from the constrained `SandboxPolicy` and network settings. The active profile needs to be derived from the same constrained values that already honor `requirements.toml`. ## What changed This adds derivation of the active `PermissionProfile` from the constrained runtime permission settings and exposes that derived value through config snapshots and thread state. The app-server can then report the active profile without introducing a second source of truth. ## Verification - `cargo test -p codex-core --test all permissions_messages -- --nocapture` - `cargo test -p codex-core --test all request_permissions -- --nocapture` --- [//]: # (BEGIN SAPLING FOOTER) Stack created with [Sapling](https://sapling-scm.com). Best reviewed with [ReviewStack](https://reviewstack.dev/openai/codex/pull/18277). * #18288 * #18287 * #18286 * #18285 * #18284 * #18283 * #18282 * #18281 * #18280 * #18279 * #18278 * __->__ #18277
bolinfest
force-pushed
the
pr18278
branch
3 times, most recently
from
1065304 to
e0369a0
Compare
## Why The `PermissionProfile` migration needs app-server clients to see the same constrained permission model that core is using at runtime. Before this PR, thread lifecycle responses only exposed the legacy `SandboxPolicy` shape, so clients still had to infer active permissions from sandbox fields. That makes downstream resume, fork, and override flows harder to make `PermissionProfile`-first. External sandbox policies are intentionally excluded from this canonical view. External enforcement cannot be round-tripped as a `PermissionProfile`, and exposing a lossy root-write profile would let clients accidentally change sandbox semantics if they echo the profile back later. ## What changed - Adds the app-server v2 `PermissionProfile` wire shape, including filesystem permissions and glob scan depth metadata. - Adds `PermissionProfileNetworkPermissions` so the profile response does not expose active network state through the older additional-permissions naming. - Returns `permissionProfile` from thread start, resume, and fork responses when the active sandbox can be represented as a `PermissionProfile`. - Keeps legacy `sandbox` in those responses for compatibility and documents `permissionProfile` as canonical when present. - Makes lifecycle `permissionProfile` nullable and returns `null` for `ExternalSandbox` to avoid exposing a lossy profile. - Regenerates the app-server JSON schema and TypeScript fixtures. ## Verification - `cargo test -p codex-app-server-protocol` - `cargo test -p codex-app-server thread_response_permission_profile_omits_external_sandbox -- --nocapture` - `cargo check --tests -p codex-analytics -p codex-exec -p codex-tui` - `just fix -p codex-app-server-protocol -p codex-app-server -p codex-analytics -p codex-exec -p codex-tui` --- [//]: # (BEGIN SAPLING FOOTER) Stack created with [Sapling](https://sapling-scm.com). Best reviewed with [ReviewStack](https://reviewstack.dev/openai/codex/pull/18278). * #18279 * __->__ #18278
bolinfest
force-pushed
the
pr18279
branch
3 times, most recently
from
89a0761 to
d29fd8d
Compare
| .approvals_reviewer | ||
| .map(codex_app_server_protocol::ApprovalsReviewer::to_core), | ||
| sandbox_policy: params.sandbox_policy.map(|p| p.to_core()), | ||
| permission_profile: params.permission_profile.map(Into::into), |
There was a problem hiding this comment.
[P2] Do not start a turn after a rejected permissionProfile override
submit_core_op only enqueues the OverrideTurnContext; if SessionConfiguration::apply later rejects the new permissionProfile, the failure is emitted as a core error event by the submission loop, but this handler has already queued the UserInput and will still return a turn/start response. That is especially easy to hit with the new full-profile field because profiles can fail conversion when they request unsupported write roots or violate sandbox constraints. For example, a client trying to narrow a currently broad thread with an invalid permissionProfile can still get the turn started under the previous broader permissions. The override needs to be validated/applied synchronously before accepting the user input, or the request should fail instead of continuing with stale permissions.
| network_sandbox_policy, | ||
| ) = if profiles_are_active { | ||
| ) = if let Some(permission_profile) = permission_profile { | ||
| let configured_network_proxy_config = NetworkProxyConfig::default(); |
There was a problem hiding this comment.
[P1] Preserve network proxy config for permissionProfile overrides
The permission-profile override path builds filesystem and network sandbox permissions from the supplied profile, but it resets the network proxy policy to NetworkProxyConfig::default(). The named-profile path just below preserves the profile network settings with network_proxy_config_from_profile_network(profile.network.as_ref()), including proxy URLs, SOCKS settings, domain and socket rules, and local-binding policy. In #18280 the clients send config.permissions.permission_profile() as this override, so a config that locally runs with a managed proxy or allowlist can be restarted through app-server with the same apparent permissionProfile but no proxy policy, broadening or de-auditing network access. This path needs to carry the configured profile network proxy state, or the API should not treat permissionProfile as a full config override until that policy is representable.
bolinfest
force-pushed
the
pr18279
branch
3 times, most recently
from
b6c82bf to
99901f5
Compare
2 participants
Why
PermissionProfileis becoming the canonical permissions shape shared by core and app-server. After app-server responses expose the active profile, clients need to be able to send that same shape back when starting, resuming, forking, or overriding a turn instead of translating through the legacysandbox/sandboxPolicyshorthands.This still needs to preserve the existing requirements/platform enforcement model. A profile-shaped request can be downgraded or rejected by constraints, but the server should keep the user's elevated-access intent for project trust decisions. Turn-level profile overrides also need to retain existing read protections, including deny-read entries and bounded glob-scan metadata, so a permission override cannot accidentally drop configured protections such as
**/*.env = deny.What changed
permissionProfilerequest fields tothread/start,thread/resume,thread/fork, andturn/start.permissionProfileand the legacysandbox/sandboxPolicyfields, including running-thread resume requests.globScanMaxDepthwhen applying turn-levelpermissionProfileoverrides.Verification
cargo test -p codex-app-server-protocol schema_fixturescargo test -p codex-core session_configuration_apply_permission_profile_preserves_existing_deny_read_entriesStack created with Sapling. Best reviewed with ReviewStack.