Skip to content

protocol: drop legacy sandbox from user turns#20446

Open
bolinfest wants to merge 1 commit intopr20441from
pr20446
Open

protocol: drop legacy sandbox from user turns#20446
bolinfest wants to merge 1 commit intopr20441from
pr20446

Conversation

@bolinfest
Copy link
Copy Markdown
Collaborator

@bolinfest bolinfest commented Apr 30, 2026

Why

Op::UserTurn now has a required permission_profile, so the legacy sandbox_policy field was only an ignored compatibility slot. Keeping both on the same operation makes new callsites look like they still need to reason about two permission models, even though core only honors the profile.

This removes that ambiguity for UserTurn while leaving the remaining legacy fields on UserInputWithTurnContext and OverrideTurnContext, where older serialized/request payloads still need compatibility handling.

What Changed

  • Removed sandbox_policy from the Op::UserTurn protocol variant.
  • Updated core session handling and TUI command conversion to construct and match UserTurn with permission_profile only.
  • Renamed the direct-constructor test helper from turn_permission_fields() to turn_permission_profile() and updated test callsites that previously threaded an always-None sandbox field.

Verification

  • cargo check -p codex-protocol -p codex-core -p codex-tui --tests
  • cargo test -p codex-protocol user_turn
  • cargo test -p codex-core user_turn_updates_approvals_reviewer
  • just fix -p codex-protocol
  • just fix -p codex-core
  • just fix -p codex-tui

Stack created with Sapling. Best reviewed with ReviewStack.

@bolinfest bolinfest requested a review from a team as a code owner April 30, 2026 15:35
@bolinfest bolinfest changed the base branch from main to pr20441 April 30, 2026 15:36
This was referenced Apr 30, 2026
@chatgpt-codex-connector
Copy link
Copy Markdown
Contributor

💡 Codex Review

// Current app-server responses include `permissionProfile`. If it is absent,
// keep the TUI on its configured profile rather than rebuilding a lossy
// profile from the legacy `sandbox` compatibility field.
config.permissions.permission_profile()

P1 Badge Restore remote fallback from legacy sandbox field

When permissionProfile is absent in thread/start|resume|fork responses (which can happen with older remote app-server builds), this now falls back to the local config profile instead of deriving from the response sandbox. In remote mode, subsequent turns always send a sandbox override (turn_permissions_overrides), so this mismatch can silently overwrite the server thread’s real sandbox/permissions on the next turn (for example, broadening a read-only thread to workspace-write). Please keep using the response sandbox as the compatibility fallback when permissionProfile is missing.

ℹ️ 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".

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant