Clarify sandbox permission override helper semantics - #13703
Conversation
Co-authored-by: Codex <noreply@openai.com>
|
@codex review this |
|
Codex Review: Didn't find any major issues. Keep them coming! ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
Co-authored-by: Codex <noreply@openai.com>
|
@codex review this |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bd55d3c640
ℹ️ 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".
dylan-hurd-oai
left a comment
There was a problem hiding this comment.
naming is hard - good suggestion!
Summary
Today
SandboxPermissions::requires_additional_permissions()does not actually mean "isWithAdditionalPermissions". It returnstruefor any non-default sandbox override, includingRequireEscalated. That broad behavior is relied on in multiplemaincallsites.The naming is security-sensitive because
SandboxPermissionsis used on shell-like tool calls to tell the executor how a single command should relate to the turn sandbox:UseDefault: run with the turn sandbox unchangedRequireEscalated: request execution outside the sandboxWithAdditionalPermissions: stay sandboxed but widen permissions for that command onlyProblem
The old helper name reads as if it only applies to the
WithAdditionalPermissionsvariant. In practice it means "this command requested any explicit sandbox override."That ambiguity made it easy to read production checks incorrectly and made the guardian change look like a standalone
mainfix when it is not.On
maintoday:shellandunified_execintentionally reject any explicitsandbox_permissionsrequest unless approval policy isOnRequestexec_policyintentionally treats any explicit sandbox override as prompt-worthy in restricted sandboxesRequireEscalatedandWithAdditionalPermissionsas explicit sandbox override requestsSo changing those callsites from the broad helper to a narrow
WithAdditionalPermissionscheck would be a behavior change, not a pure cleanup.What This PR Does
SandboxPermissionsas a per-command sandbox override, not a generic permissions bagrequests_sandbox_override()for the broad meaning: anything exceptUseDefaultuses_additional_permissions()for the narrow meaning: onlyWithAdditionalPermissionsrequires_additional_permissions()as a compatibility alias to the broad meaning for nowWhat This PR Does Not Do
This PR does not change runtime behavior. That is intentional.