Skip to content

permissions: centralize legacy sandbox projection#19734

Merged
bolinfest merged 1 commit intomainfrom
pr19734
Apr 27, 2026
Merged

permissions: centralize legacy sandbox projection#19734
bolinfest merged 1 commit intomainfrom
pr19734

Conversation

@bolinfest
Copy link
Copy Markdown
Collaborator

@bolinfest bolinfest commented Apr 27, 2026

Why

The remaining migration work still needs SandboxPolicy at a few compatibility boundaries, but those projections should come from one canonical path. Keeping ad hoc legacy projections scattered through app-server, CLI, and config code makes it easy for behavior to drift as PermissionProfile gains fidelity that the legacy enum cannot represent.

What Changed

  • Adds Permissions::legacy_sandbox_policy(cwd) and Config::legacy_sandbox_policy() as the compatibility projection from the canonical PermissionProfile.
  • Adds Permissions::can_set_legacy_sandbox_policy() so legacy inputs are checked after they are converted into profile semantics.
  • Updates app-server command handling, Windows sandbox setup, session configuration, and sandbox summaries to use the centralized projection helper.
  • Leaves SandboxPolicy in place only for boundary inputs/outputs that still speak the legacy abstraction.

Verification

  • cargo check -p codex-config -p codex-core -p codex-sandboxing -p codex-app-server -p codex-cli -p codex-tui
  • cargo test -p codex-tui permissions_selection_history_snapshot_full_access_to_default -- --nocapture
  • cargo test -p codex-tui permissions_selection_sends_approvals_reviewer_in_override_turn_context -- --nocapture
  • bazel test //codex-rs/tui:tui-unit-tests-bin --test_arg=permissions_selection_history_snapshot_full_access_to_default --test_output=errors
  • bazel test //codex-rs/tui:tui-unit-tests-bin --test_arg=permissions_selection_sends_approvals_reviewer_in_override_turn_context --test_output=errors

Stack created with Sapling. Best reviewed with ReviewStack.

@bolinfest bolinfest requested a review from a team as a code owner April 27, 2026 00:40
@bolinfest bolinfest changed the base branch from main to pr19395 April 27, 2026 00:40
bolinfest added a commit that referenced this pull request Apr 27, 2026
## Why

Several execution paths still converted profile-backed permissions into
`SandboxPolicy` and then rebuilt runtime permissions from that legacy
shape. Those round trips are unnecessary after the preceding PRs and can
lose split filesystem semantics. Core approval and escalation should
carry the resolved profile directly.

## What Changed

- Removes `sandbox_policy` from `ResolvedPermissionProfile`; the
resolved permission object now carries the canonical `PermissionProfile`
directly.
- Updates exec-policy fallback, shell/unified-exec interception,
escalation reruns, and related tests to pass profiles instead of legacy
policies.
- Removes legacy additional-permission merge helpers that built an
effective `SandboxPolicy` before rebuilding runtime permissions.
- Keeps legacy projections only at compatibility boundaries that still
require `SandboxPolicy`, not in core permission computation.

## Verification

- `cargo test -p codex-core direct_write_roots`
- `cargo test -p codex-core runtime_roots_to_legacy_projection`
- `cargo test -p codex-app-server
requested_permissions_trust_project_uses_permission_profile_intent`







































































---
[//]: # (BEGIN SAPLING FOOTER)
Stack created with [Sapling](https://sapling-scm.com). Best reviewed
with [ReviewStack](https://reviewstack.dev/openai/codex/pull/19394).
* #19737
* #19736
* #19735
* #19734
* #19395
* __->__ #19394
@chatgpt-codex-connector
Copy link
Copy Markdown
Contributor

💡 Codex Review

if file_system_policy.has_full_disk_write_access() {
"full_access"

P2 Badge Preserve network-restricted mode in managed sandbox metrics

This branch classifies every managed profile with full-disk write access as "full_access", but managed profiles can still have restricted networking (e.g., unrestricted filesystem + restricted network). Before this change, those cases projected to legacy ExternalSandbox and were emitted as "external_sandbox"; now they are counted as full access, which will skew sandbox-mode analytics and any downstream experiment/reporting that depends on this field.

ℹ️ 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
Copy link
Copy Markdown
Collaborator Author

[codex] Addressed by preserving the external_sandbox analytics bucket when a managed profile has full filesystem access but restricted network, with managed_full_disk_with_restricted_network_reports_external_sandbox coverage. The fix is in the lower #19395 commit so it applies throughout this stack.

@bolinfest bolinfest merged commit 0d8cdc0 into main Apr 27, 2026
45 of 58 checks passed
@bolinfest bolinfest deleted the pr19734 branch April 27, 2026 03:31
@github-actions github-actions Bot locked and limited conversation to collaborators Apr 27, 2026
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants