Skip to content

session: tag turns with permission profiles#20432

Open
bolinfest wants to merge 1 commit intopr20431from
pr20432
Open

session: tag turns with permission profiles#20432
bolinfest wants to merge 1 commit intopr20431from
pr20432

Conversation

@bolinfest
Copy link
Copy Markdown
Collaborator

@bolinfest bolinfest commented Apr 30, 2026

Why

TurnContext already carries the canonical PermissionProfile for a turn. Keeping a TurnContext::sandbox_policy() convenience method made it easy for production code to re-project that profile into the legacy SandboxPolicy shape even when the caller did not actually need a legacy boundary payload.

This change keeps legacy projection explicit: new turn telemetry records the canonical profile, while tests that intentionally construct old rollout records build the compatibility sandbox_policy locally as a fixture detail.

What Changed

  • Removed TurnContext::sandbox_policy() from the core session turn context.
  • Changed sampling feedback tags from sandbox_policy to permission_profile so feedback captures the canonical permission model instead of a lossy compatibility projection.
  • Updated legacy rollout reconstruction fixtures to call a test-local helper when they need a TurnContextItem with permission_profile: None and a legacy sandbox_policy.

Verification

  • cd codex-rs && cargo check -p codex-core --tests
  • cd codex-rs && just fix -p codex-core

Stack created with Sapling. Best reviewed with ReviewStack.

@bolinfest bolinfest requested a review from a team as a code owner April 30, 2026 14:27
@bolinfest bolinfest changed the base branch from main to pr20431 April 30, 2026 14:27
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 Preserve remote thread permissions when profile is omitted

When permissionProfile is absent in a thread/start|resume|fork response (e.g., older remote app-server versions that still provide only legacy sandbox), this fallback now uses the local config profile instead of the server thread’s sandbox-derived permissions. That desynchronizes client/server permission state, and subsequent turn_start calls build overrides from the wrong profile (turn_permissions_overrides), which can silently broaden or restrict sandboxing for future turns on that thread. The previous remote-mode fallback reconstructed from the response sandbox, so this is a behavior regression for mixed-version remote deployments.

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