Conversation
bolinfest
changed the title
![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: 9df4851671
ℹ️ 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".
bolinfest
force-pushed
the
pr15914
branch
4 times, most recently
from
4698886 to
82f8680
Compare
bolinfest
force-pushed
the
pr15914
branch
6 times, most recently
from
b577927 to
4a15695
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 604c14cfb3
ℹ️ 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".
| match value.entries { | ||
| Some(entries) => CoreFileSystemPermissions { entries }, | ||
| None => CoreFileSystemPermissions::from_read_write_roots(value.read, value.write), |
There was a problem hiding this comment.
Treating any non-None entries value as authoritative drops valid grants when a client sends entries: [] alongside populated legacy read/write fields. In that case this conversion returns an empty CoreFileSystemPermissions, so an approval response that should grant filesystem access is silently downgraded to no filesystem permissions. Because entries is nullable/array in the v2 wire shape, clients can legitimately emit an empty array as a default; this path should either treat empty entries as absent or merge with read/write.
Useful? React with 👍 / 👎.
bolinfest
force-pushed
the
pr15914
branch
2 times, most recently
from
f88522c to
cece4f1
Compare
| intersect_permission_paths(requested_file_system.write, granted_file_system.write); | ||
| FileSystemPermissions { read, write } | ||
| let granted_entries = granted.file_system.unwrap_or_default().entries; | ||
| let entries = requested_file_system |
There was a problem hiding this comment.
[P1] Preserve narrowing entries when clamping approval grants
This exact-entry intersection is no longer a safe cap now that PermissionProfile can contain semantic narrowing entries. If the request is :root=write plus /tmp/secret.txt=none, and an app-server client responds with only :root=write, the returned profile keeps the root write entry and drops the deny. Because request_permissions_response_from_client_result relies on this helper to clamp client responses to the original request, a partial or buggy client response can grant more filesystem access than Codex requested. The intersection needs to be semantic, or broad grants must be rejected unless the requested carveouts they dominate are preserved.
| } | ||
| _ => false, | ||
| }; | ||
| let session_cwd = session.cwd().await; |
There was a problem hiding this comment.
[P2] Do not let preapproved requests narrow sticky grants
The new semantic preapproval check can decide that existing sticky grants cover a fresh request, but the effective permissions passed to the runtime still contain both the fresh request and the sticky grant. With a session grant like /workspace=write and a command request like /workspace/docs=read, preapproval succeeds, but the effective profile still contains the more-specific read entry; the runtime policy resolves the child path as read-only, so the command loses write access that the session grant already provided. Sticky grants should continue to widen command permissions, not be revoked by a narrower explicit request unless that is a deliberate deny operation.
| Self { | ||
| read: value.read, | ||
| write: value.write, | ||
| match value.entries { |
There was a problem hiding this comment.
[P2] Do not let empty entries fall back to stale legacy grants
The server can emit mixed permission profiles with both canonical entries and legacy read/write populated. On the response path, though, entries: [] is treated the same as absent entries and falls back to read/write. If a client edits or clears the canonical list but leaves the duplicate legacy fields in place, Codex can grant paths the client no longer meant to grant. Treat a present entries field as authoritative even when empty, or reject/normalize responses where entries and read/write disagree.
bolinfest
force-pushed
the
pr15914
branch
3 times, most recently
from
86a473e to
6231222
Compare
2 participants
Why
SandboxPolicyis still acting as the thread-level source of truth for permissions, but it collapses richer filesystem and network state back into a lossy legacy enum. That makes it hard to preserve special filesystem entries and per-turn runtime overrides as we move towardPermissionProfilebeing the primary permission abstraction throughout the codebase.This PR starts that migration by teaching
PermissionProfileto represent the current runtime permissions, while keeping the existing constrained runtime policies as the authoritative state incore. That avoids introducing duplicated or ambiguous permission fields beforePermissionProfileis lossless enough to fully replace the legacy runtime model.What changed
FileSystemPermissions { read, write }with a canonicalentries: Vec<FileSystemSandboxEntry>representation inprotocol, while preserving legacyread/writeserde compatibility for existing wire shapesPermissionProfilefrom runtime filesystem and network sandbox policies, plus compatibility helpers to project back to legacySandboxPolicywhen neededcoreruntime state authoritative on the constrained split permission fields (sandbox_policy, filesystem, network, and macOS seatbelt extensions) rather than storing a secondpermission_profilefield alongside thementrieswhen special paths or deny rules cannot be represented losslessly as legacyread/writeliststui_app_serverapproval overlays to render special filesystem grants and deny entries instead of silently omitting themTesting
cargo test -p codex-core default_permissions_profile_populates_runtime_sandbox_policy -- --nocapturecargo test -p codex-core session_configuration_apply_preserves_split_file_system_policy_on_cwd_only_update -- --nocapturecargo test -p codex-app-server-protocolcargo test -p codex-tui -p codex-tui-app-server approval_overlay -- --nocapturejust write-app-server-schemajust fix -p codex-app-server-protocol -p codex-tui -p codex-tui-app-serverjust argument-comment-lint