Conversation
Contributor
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b53e08a10f
ℹ️ 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".
4bb7d67 to
91c318e
Compare
bolinfest
added a commit
that referenced
this pull request
Jun 1, 2026
Migrate the exec-side Windows sandbox override plumbing to pass PermissionProfile plus the split filesystem/network policies instead of accepting a legacy SandboxPolicy. This keeps the remaining compatibility projection local to the writable-root comparison that still needs it, and removes ExecRequest::compatibility_sandbox_policy without touching the broader config, protocol, app-server, telemetry, or session surfaces from #25450.
bolinfest
added a commit
that referenced
this pull request
Jun 1, 2026
Migrate the exec-side Windows sandbox override plumbing to pass PermissionProfile plus the split filesystem/network policies instead of accepting a legacy SandboxPolicy. This keeps the remaining compatibility projection local to the writable-root comparison that still needs it, and removes ExecRequest::compatibility_sandbox_policy without touching the broader config, protocol, app-server, telemetry, or session surfaces from #25450.
bolinfest
added a commit
that referenced
this pull request
Jun 1, 2026
Migrate the exec-side Windows sandbox override plumbing to pass PermissionProfile plus the split filesystem/network policies instead of accepting a legacy SandboxPolicy. This keeps the remaining compatibility projection local to the writable-root comparison that still needs it, and removes ExecRequest::compatibility_sandbox_policy without touching the broader config, protocol, app-server, telemetry, or session surfaces from #25450.
bolinfest
added a commit
that referenced
this pull request
Jun 2, 2026
Migrate the exec-side Windows sandbox override plumbing to pass PermissionProfile plus the split filesystem/network policies instead of accepting a legacy SandboxPolicy. This keeps the remaining compatibility projection local to the writable-root comparison that still needs it, and removes ExecRequest::compatibility_sandbox_policy without touching the broader config, protocol, app-server, telemetry, or session surfaces from #25450.
bolinfest
added a commit
that referenced
this pull request
Jun 2, 2026
Migrate the exec-side Windows sandbox override plumbing to pass PermissionProfile plus the split filesystem/network policies instead of accepting a legacy SandboxPolicy. This keeps the remaining compatibility projection local to the writable-root comparison that still needs it, and removes ExecRequest::compatibility_sandbox_policy without touching the broader config, protocol, app-server, telemetry, or session surfaces from #25450.
bolinfest
added a commit
that referenced
this pull request
Jun 2, 2026
Migrate the exec-side Windows sandbox override plumbing to pass PermissionProfile plus the split filesystem/network policies instead of accepting a legacy SandboxPolicy. This keeps the remaining compatibility projection local to the writable-root comparison that still needs it, and removes ExecRequest::compatibility_sandbox_policy without touching the broader config, protocol, app-server, telemetry, or session surfaces from #25450.
bolinfest
added a commit
that referenced
this pull request
Jun 2, 2026
Migrate the exec-side Windows sandbox override plumbing to pass PermissionProfile plus the split filesystem/network policies instead of accepting a legacy SandboxPolicy. This keeps the remaining compatibility projection local to the writable-root comparison that still needs it, and removes ExecRequest::compatibility_sandbox_policy without touching the broader config, protocol, app-server, telemetry, or session surfaces from #25450.
wheakerd
pushed a commit
to wheakerd/codex
that referenced
this pull request
Jun 2, 2026
Migrate the exec-side Windows sandbox override plumbing to pass PermissionProfile plus the split filesystem/network policies instead of accepting a legacy SandboxPolicy. This keeps the remaining compatibility projection local to the writable-root comparison that still needs it, and removes ExecRequest::compatibility_sandbox_policy without touching the broader config, protocol, app-server, telemetry, or session surfaces from openai#25450.
bolinfest
added a commit
that referenced
this pull request
Jun 3, 2026
## Why #25450 attempts a broad `SandboxPolicy` removal across several unrelated surfaces, which makes it hard to review and still leaves new helper code moving legacy policies around. This PR is a narrower alternative: migrate only the exec-side Windows sandbox plumbing so the review can focus on one production path and one compatibility boundary. The goal is to stop threading `SandboxPolicy` through exec code without expanding the migration into app-server, protocol, telemetry, config, or session behavior. ## What changed - Removed `ExecRequest::compatibility_sandbox_policy()`. - Changed the Windows restricted-token and elevated filesystem override helpers to accept `PermissionProfile` plus the split filesystem/network policies instead of a `SandboxPolicy`. - Kept the remaining legacy projection local to the writable-root comparison that still needs to compare split policy behavior against the legacy Windows backend model. - Rejected restricted split filesystem policies that still grant full-disk writes before using the Windows restricted-token backend, preserving the previous clear-failure behavior for profiles that project to `ExternalSandbox`. - Updated the Windows sandbox override tests to exercise the new call shape and cover the full-write split-profile regression. ## Verification - `just test -p codex-core windows_restricted_token` - `just test -p codex-core windows_elevated`
dkropachev
pushed a commit
to dkropachev/codex
that referenced
this pull request
Jun 9, 2026
## Why openai#25450 attempts a broad `SandboxPolicy` removal across several unrelated surfaces, which makes it hard to review and still leaves new helper code moving legacy policies around. This PR is a narrower alternative: migrate only the exec-side Windows sandbox plumbing so the review can focus on one production path and one compatibility boundary. The goal is to stop threading `SandboxPolicy` through exec code without expanding the migration into app-server, protocol, telemetry, config, or session behavior. ## What changed - Removed `ExecRequest::compatibility_sandbox_policy()`. - Changed the Windows restricted-token and elevated filesystem override helpers to accept `PermissionProfile` plus the split filesystem/network policies instead of a `SandboxPolicy`. - Kept the remaining legacy projection local to the writable-root comparison that still needs to compare split policy behavior against the legacy Windows backend model. - Rejected restricted split filesystem policies that still grant full-disk writes before using the Windows restricted-token backend, preserving the previous clear-failure behavior for profiles that project to `ExternalSandbox`. - Updated the Windows sandbox override tests to exercise the new call shape and cover the full-write split-profile regression. ## Verification - `just test -p codex-core windows_restricted_token` - `just test -p codex-core windows_elevated`
dkropachev
pushed a commit
to dkropachev/codex
that referenced
this pull request
Jun 9, 2026
## Why openai#25450 attempts a broad `SandboxPolicy` removal across several unrelated surfaces, which makes it hard to review and still leaves new helper code moving legacy policies around. This PR is a narrower alternative: migrate only the exec-side Windows sandbox plumbing so the review can focus on one production path and one compatibility boundary. The goal is to stop threading `SandboxPolicy` through exec code without expanding the migration into app-server, protocol, telemetry, config, or session behavior. ## What changed - Removed `ExecRequest::compatibility_sandbox_policy()`. - Changed the Windows restricted-token and elevated filesystem override helpers to accept `PermissionProfile` plus the split filesystem/network policies instead of a `SandboxPolicy`. - Kept the remaining legacy projection local to the writable-root comparison that still needs to compare split policy behavior against the legacy Windows backend model. - Rejected restricted split filesystem policies that still grant full-disk writes before using the Windows restricted-token backend, preserving the previous clear-failure behavior for profiles that project to `ExternalSandbox`. - Updated the Windows sandbox override tests to exercise the new call shape and cover the full-write split-profile regression. ## Verification - `just test -p codex-core windows_restricted_token` - `just test -p codex-core windows_elevated`
dkropachev
pushed a commit
to dkropachev/codex
that referenced
this pull request
Jun 9, 2026
## Why openai#25450 attempts a broad `SandboxPolicy` removal across several unrelated surfaces, which makes it hard to review and still leaves new helper code moving legacy policies around. This PR is a narrower alternative: migrate only the exec-side Windows sandbox plumbing so the review can focus on one production path and one compatibility boundary. The goal is to stop threading `SandboxPolicy` through exec code without expanding the migration into app-server, protocol, telemetry, config, or session behavior. ## What changed - Removed `ExecRequest::compatibility_sandbox_policy()`. - Changed the Windows restricted-token and elevated filesystem override helpers to accept `PermissionProfile` plus the split filesystem/network policies instead of a `SandboxPolicy`. - Kept the remaining legacy projection local to the writable-root comparison that still needs to compare split policy behavior against the legacy Windows backend model. - Rejected restricted split filesystem policies that still grant full-disk writes before using the Windows restricted-token backend, preserving the previous clear-failure behavior for profiles that project to `ExternalSandbox`. - Updated the Windows sandbox override tests to exercise the new call shape and cover the full-write split-profile regression. ## Verification - `just test -p codex-core windows_restricted_token` - `just test -p codex-core windows_elevated`
dkropachev
pushed a commit
to dkropachev/codex
that referenced
this pull request
Jun 9, 2026
## Why openai#25450 attempts a broad `SandboxPolicy` removal across several unrelated surfaces, which makes it hard to review and still leaves new helper code moving legacy policies around. This PR is a narrower alternative: migrate only the exec-side Windows sandbox plumbing so the review can focus on one production path and one compatibility boundary. The goal is to stop threading `SandboxPolicy` through exec code without expanding the migration into app-server, protocol, telemetry, config, or session behavior. ## What changed - Removed `ExecRequest::compatibility_sandbox_policy()`. - Changed the Windows restricted-token and elevated filesystem override helpers to accept `PermissionProfile` plus the split filesystem/network policies instead of a `SandboxPolicy`. - Kept the remaining legacy projection local to the writable-root comparison that still needs to compare split policy behavior against the legacy Windows backend model. - Rejected restricted split filesystem policies that still grant full-disk writes before using the Windows restricted-token backend, preserving the previous clear-failure behavior for profiles that project to `ExternalSandbox`. - Updated the Windows sandbox override tests to exercise the new call shape and cover the full-write split-profile regression. ## Verification - `just test -p codex-core windows_restricted_token` - `just test -p codex-core windows_elevated`
dkropachev
pushed a commit
to dkropachev/codex
that referenced
this pull request
Jun 9, 2026
## Why openai#25450 attempts a broad `SandboxPolicy` removal across several unrelated surfaces, which makes it hard to review and still leaves new helper code moving legacy policies around. This PR is a narrower alternative: migrate only the exec-side Windows sandbox plumbing so the review can focus on one production path and one compatibility boundary. The goal is to stop threading `SandboxPolicy` through exec code without expanding the migration into app-server, protocol, telemetry, config, or session behavior. ## What changed - Removed `ExecRequest::compatibility_sandbox_policy()`. - Changed the Windows restricted-token and elevated filesystem override helpers to accept `PermissionProfile` plus the split filesystem/network policies instead of a `SandboxPolicy`. - Kept the remaining legacy projection local to the writable-root comparison that still needs to compare split policy behavior against the legacy Windows backend model. - Rejected restricted split filesystem policies that still grant full-disk writes before using the Windows restricted-token backend, preserving the previous clear-failure behavior for profiles that project to `ExternalSandbox`. - Updated the Windows sandbox override tests to exercise the new call shape and cover the full-write split-profile regression. ## Verification - `just test -p codex-core windows_restricted_token` - `just test -p codex-core windows_elevated`
dkropachev
pushed a commit
to dkropachev/codex
that referenced
this pull request
Jun 9, 2026
## Why openai#25450 attempts a broad `SandboxPolicy` removal across several unrelated surfaces, which makes it hard to review and still leaves new helper code moving legacy policies around. This PR is a narrower alternative: migrate only the exec-side Windows sandbox plumbing so the review can focus on one production path and one compatibility boundary. The goal is to stop threading `SandboxPolicy` through exec code without expanding the migration into app-server, protocol, telemetry, config, or session behavior. ## What changed - Removed `ExecRequest::compatibility_sandbox_policy()`. - Changed the Windows restricted-token and elevated filesystem override helpers to accept `PermissionProfile` plus the split filesystem/network policies instead of a `SandboxPolicy`. - Kept the remaining legacy projection local to the writable-root comparison that still needs to compare split policy behavior against the legacy Windows backend model. - Rejected restricted split filesystem policies that still grant full-disk writes before using the Windows restricted-token backend, preserving the previous clear-failure behavior for profiles that project to `ExternalSandbox`. - Updated the Windows sandbox override tests to exercise the new call shape and cover the full-write split-profile regression. ## Verification - `just test -p codex-core windows_restricted_token` - `just test -p codex-core windows_elevated`
dkropachev
pushed a commit
to dkropachev/codex
that referenced
this pull request
Jun 9, 2026
## Why openai#25450 attempts a broad `SandboxPolicy` removal across several unrelated surfaces, which makes it hard to review and still leaves new helper code moving legacy policies around. This PR is a narrower alternative: migrate only the exec-side Windows sandbox plumbing so the review can focus on one production path and one compatibility boundary. The goal is to stop threading `SandboxPolicy` through exec code without expanding the migration into app-server, protocol, telemetry, config, or session behavior. ## What changed - Removed `ExecRequest::compatibility_sandbox_policy()`. - Changed the Windows restricted-token and elevated filesystem override helpers to accept `PermissionProfile` plus the split filesystem/network policies instead of a `SandboxPolicy`. - Kept the remaining legacy projection local to the writable-root comparison that still needs to compare split policy behavior against the legacy Windows backend model. - Rejected restricted split filesystem policies that still grant full-disk writes before using the Windows restricted-token backend, preserving the previous clear-failure behavior for profiles that project to `ExternalSandbox`. - Updated the Windows sandbox override tests to exercise the new call shape and cover the full-write split-profile regression. ## Verification - `just test -p codex-core windows_restricted_token` - `just test -p codex-core windows_elevated`
dkropachev
pushed a commit
to dkropachev/codex
that referenced
this pull request
Jun 9, 2026
## Why openai#25450 attempts a broad `SandboxPolicy` removal across several unrelated surfaces, which makes it hard to review and still leaves new helper code moving legacy policies around. This PR is a narrower alternative: migrate only the exec-side Windows sandbox plumbing so the review can focus on one production path and one compatibility boundary. The goal is to stop threading `SandboxPolicy` through exec code without expanding the migration into app-server, protocol, telemetry, config, or session behavior. ## What changed - Removed `ExecRequest::compatibility_sandbox_policy()`. - Changed the Windows restricted-token and elevated filesystem override helpers to accept `PermissionProfile` plus the split filesystem/network policies instead of a `SandboxPolicy`. - Kept the remaining legacy projection local to the writable-root comparison that still needs to compare split policy behavior against the legacy Windows backend model. - Rejected restricted split filesystem policies that still grant full-disk writes before using the Windows restricted-token backend, preserving the previous clear-failure behavior for profiles that project to `ExternalSandbox`. - Updated the Windows sandbox override tests to exercise the new call shape and cover the full-write split-profile regression. ## Verification - `just test -p codex-core windows_restricted_token` - `just test -p codex-core windows_elevated`
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
codex-coreshould operate on the canonicalPermissionProfileplus split filesystem/network runtime policies instead of the legacySandboxPolicycompatibility shape. KeepingSandboxPolicyin core made it easy for new code to depend on the older model even though legacy sandbox modes should be translated at configuration/protocol/app-server boundaries.What Changed
SandboxPolicyimports and session/config APIs fromcodex-core.sandboxPolicyrequests intoPermissionProfileupdates at the protocol/app-server boundary while preserving existing deny-read entries.PermissionProfileand split runtime policies.PermissionProfileonly where older serialized formats still require them.codex-rs/core/README.mdto describe permission profiles rather than core-ownedSandboxPolicy.Verification
rg -n -g '!**/*test*' '\bSandboxPolicy\b' codex-rs/corereturns no matches.just test -p codex-protocoljust test -p codex-sandboxingjust test -p codex-mcpjust test -p codex-oteljust test -p codex-memories-writejust test -p codex-utils-sandbox-summaryjust fix -p codex-coreI also attempted
just test -p codex-coreandjust test -p codex-app-server. They did not complete green in this local sandbox: failures were from the outer macOS sandbox affectingsandbox-exec/sandbox environment expectations, plus app-server tests that depend on the local user skills set.