Skip to content

rollout-trace: record permission profiles#20436

Closed
bolinfest wants to merge 1 commit into
pr20433from
pr20436
Closed

rollout-trace: record permission profiles#20436
bolinfest wants to merge 1 commit into
pr20433from
pr20436

Conversation

@bolinfest

@bolinfest bolinfest commented Apr 30, 2026 •
edited
Loading

Copy link
Copy Markdown
Collaborator

Why

Rollout trace thread metadata still recorded the legacy SandboxPolicy projection even though the session configuration is now driven by PermissionProfile. That kept trace metadata tied to a lossy compatibility shape and made new trace bundles look like they were still sourced from the old sandbox abstraction.

What Changed

  • Replaces ThreadStartedTraceMetadata.sandbox_policy with permission_profile.
  • Records the active session PermissionProfile when creating rollout thread traces.
  • Updates rollout trace tests and tool-dispatch trace fixtures to seed the new metadata field directly.

Verification

  • cd codex-rs && cargo check -p codex-rollout-trace -p codex-core --tests
  • cd codex-rs && just fix -p codex-rollout-trace
  • 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:42
@bolinfest bolinfest changed the base branch from main to pr20433 April 30, 2026 14:42
This was referenced Apr 30, 2026
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
This was referenced Apr 30, 2026
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Apr 30, 2026

Copy link
Copy Markdown
Contributor

💡 Codex Review

codex/codex-rs/tui/src/app_server_session.rs

Lines 1414 to 1417 in 976d8bc

// 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()

P2 Badge Restore legacy sandbox fallback for missing permissionProfile

When permission_profile is absent in thread start/resume/fork responses, this now always falls back to the local config profile, ignoring the server-provided legacy sandbox context. Because permissionProfile is optional in app-server responses (and older/remote flows can omit it), this can desynchronize the TUI’s session permissions from the actual remote thread (for example, resuming a danger-full-access thread while local config is read-only), leading to incorrect permission display and unintended sandbox overrides on later turns.

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

This was referenced Apr 30, 2026
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
@github-actions

github-actions Bot commented May 16, 2026

Copy link
Copy Markdown
Contributor

Closing this pull request because it has had no updates for more than 14 days. If you plan to continue working on it, feel free to reopen or open a new PR.

@github-actions github-actions Bot closed this May 16, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@bolinfest