Conversation
This was referenced Apr 17, 2026
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ad29119e25
ℹ️ 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".
Comment on lines
+930
to
+933
| let permission_profile = permission_profile_override_from_config(config); | ||
| let sandbox = permission_profile | ||
| .is_none() | ||
| .then(|| sandbox_mode_from_policy(config.permissions.sandbox_policy.get().clone())) |
Contributor
There was a problem hiding this comment.
These lines always derive permission_profile from the local Config and then suppress sandbox, even when ThreadParamsMode::Remote is in use. In remote mode we already avoid forwarding local-only values like cwd/model_provider, but config.permissions.permission_profile() is runtime-expanded with local filesystem entries, so sending it to a remote app-server can apply client-local absolute paths and miss the server-side workspace carveouts it would normally derive. This affects thread/start, thread/resume, and thread/fork because they all share this override path.
Useful? React with 👍 / 👎.
bolinfest
force-pushed
the
pr18280
branch
2 times, most recently
from
0164ac6 to
853bd3b
Compare
bolinfest
force-pushed
the
pr18279
branch
2 times, most recently
from
536d5c4 to
c100e8e
Compare
bolinfest
force-pushed
the
pr18280
branch
2 times, most recently
from
ee0a15a to
a56ac61
Compare
bolinfest
force-pushed
the
pr18279
branch
2 times, most recently
from
929878c to
69ab9eb
Compare
bolinfest
added a commit
that referenced
this pull request
Apr 21, 2026
## Why #18274 made `PermissionProfile` the canonical file-system permissions shape, but the round-trip from `FileSystemSandboxPolicy` to `PermissionProfile` still dropped one piece of policy metadata: `glob_scan_max_depth`. That field is security-relevant for deny-read globs such as `**/*.env`. On Linux, bubblewrap sandbox construction uses it to bound unreadable glob expansion. If a profile copied from active runtime permissions loses this value and is submitted back as an override, the resulting `FileSystemSandboxPolicy` can behave differently even though the visible permission entries look equivalent. ## What changed - Add `glob_scan_max_depth` to protocol `FileSystemPermissions` and preserve it when converting to/from `FileSystemSandboxPolicy`. - Keep legacy `read`/`write` JSON for simple path-only permissions, but force canonical JSON when glob scan depth is present so the metadata is not silently dropped. - Carry `globScanMaxDepth` through app-server `AdditionalFileSystemPermissions`, generated JSON/TypeScript schemas, and app-server/TUI conversion call sites. - Preserve the metadata through sandboxing permission normalization, merging, and intersection. - Carry the merged scan depth into the effective `FileSystemSandboxPolicy` used for command execution, so bounded deny-read globs reach Linux bubblewrap materialization. ## Verification - `cargo test -p codex-sandboxing glob_scan -- --nocapture` - `cargo test -p codex-sandboxing policy_transforms -- --nocapture` - `just fix -p codex-sandboxing` --- [//]: # (BEGIN SAPLING FOOTER) Stack created with [Sapling](https://sapling-scm.com). Best reviewed with [ReviewStack](https://reviewstack.dev/openai/codex/pull/18713). * #18288 * #18287 * #18286 * #18285 * #18284 * #18283 * #18282 * #18281 * #18280 * #18279 * #18278 * #18277 * #18276 * #18275 * __->__ #18713
bolinfest
force-pushed
the
pr18279
branch
2 times, most recently
from
3c66e19 to
e99a496
Compare
bolinfest
force-pushed
the
pr18280
branch
2 times, most recently
from
44c5e27 to
ea8eb64
Compare
bolinfest
force-pushed
the
pr18279
branch
2 times, most recently
from
1a9b5dc to
9d354ba
Compare
bolinfest
force-pushed
the
pr18280
branch
2 times, most recently
from
ab0e270 to
c013a96
Compare
bolinfest
force-pushed
the
pr18279
branch
2 times, most recently
from
0d7a36c to
c897233
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
After app-server can accept
PermissionProfile, first-party clients should stop preferring legacy sandbox fields when they have canonical permission information available. This keeps the migration moving without removing legacy compatibility yet.What changed
This updates the exec and TUI app-server clients to send
permissionProfileoverrides derived from config, while preserving legacy sandbox fallback behavior when a profile is not available.Verification
cargo test -p codex-tui permissions -- --nocapturecargo test -p codex-core --test all permissions_messages -- --nocaptureStack created with Sapling. Best reviewed with ReviewStack.