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: afb44ee779
ℹ️ 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".
| pub sandbox: SandboxPolicy, | ||
| pub permission_profile: PermissionProfile, | ||
| pub reasoning_effort: Option<ReasoningEffort>, |
There was a problem hiding this comment.
Adding permission_profile as a required field makes ThreadStartResponse/ThreadResumeResponse/ThreadForkResponse fail to deserialize when that field is absent, which breaks mixed-version clients and any stored payloads from older servers that predate this field. Existing lifecycle parsing paths intentionally tolerate some missing response fields, so this strict requirement is a wire-compatibility regression unless permission_profile is given a default (or made optional) during deserialization.
Useful? React with 👍 / 👎.
bolinfest
force-pushed
the
pr18278
branch
2 times, most recently
from
4874afe to
25420fc
Compare
bolinfest
force-pushed
the
pr18277
branch
2 times, most recently
from
5a57cef to
f02bf74
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
## 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
| approval_policy: config_snapshot.approval_policy.into(), | ||
| approvals_reviewer: config_snapshot.approvals_reviewer.into(), | ||
| sandbox: config_snapshot.sandbox_policy.into(), | ||
| permission_profile: config_snapshot.permission_profile.into(), |
There was a problem hiding this comment.
[P2] Do not expose lossy profiles for ExternalSandbox
This now publishes permissionProfile for every thread response, including threads whose sandbox is externalSandbox. PermissionProfile::from_runtime_permissions() cannot represent FileSystemSandboxKind::ExternalSandbox: it serializes that filesystem policy as root write, and converting the profile back yields a restricted root-write filesystem policy instead of external enforcement. This is not fixed above; later stack PRs start accepting and carrying permissionProfile, so non-TUI clients can round-trip the advertised value and change sandbox semantics. Omit or mark the profile for ExternalSandbox, or extend the profile model to carry the enforcement kind before exposing it as canonical thread state.
2 participants
Why
The
PermissionProfilemigration 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 legacySandboxPolicyshape, so clients still had to infer active permissions from sandbox fields. That makes downstream resume, fork, and override flows harder to makePermissionProfile-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
PermissionProfilewire shape, including filesystem permissions and glob scan depth metadata.PermissionProfileNetworkPermissionsso the profile response does not expose active network state through the older additional-permissions naming.permissionProfilefrom thread start, resume, and fork responses when the active sandbox can be represented as aPermissionProfile.sandboxin those responses for compatibility and documentspermissionProfileas canonical when present.permissionProfilenullable and returnsnullforExternalSandboxto avoid exposing a lossy profile.Verification
cargo test -p codex-app-server-protocolcargo test -p codex-app-server thread_response_permission_profile_omits_external_sandbox -- --nocapturecargo check --tests -p codex-analytics -p codex-exec -p codex-tuijust fix -p codex-app-server-protocol -p codex-app-server -p codex-analytics -p codex-exec -p codex-tuiStack created with Sapling. Best reviewed with ReviewStack.