Conversation
This was referenced May 15, 2026
bolinfest
force-pushed
the
pr22792
branch
2 times, most recently
from
6dcb834 to
b734970
Compare
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8e7e4c06bf
ℹ️ 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
force-pushed
the
pr22792
branch
2 times, most recently
from
e293ae4 to
64f38b4
Compare
viyatb-oai
reviewed
May 15, 2026
Collaborator
viyatb-oai
left a comment
viyatb-oai
left a comment
There was a problem hiding this comment.
Flagging one resume-path regression that looks worth fixing before merge.
bolinfest
added a commit
that referenced
this pull request
May 15, 2026
## Why `SandboxPolicy` is being pushed back toward legacy config loading and compatibility boundaries. Guardian review sessions already want the built-in read-only permission behavior; carrying that as an active `PermissionProfile` makes the review sandbox follow the new permissions path instead of configuring the child session through the legacy policy API. ## What Changed - Configure the guardian review session with `PermissionProfile::read_only()`. - Send the read-only profile through the guardian child `Op::UserTurn`. - Keep the legacy `sandbox_policy` field populated with `SandboxPolicy::new_read_only_policy()` declared next to the profile so the two remain visibly in sync until the compatibility field goes away. ## How To Review Start in `codex-rs/core/src/guardian/review_session.rs`. The important check is that both the guardian config and the child turn now use the read-only permission profile, while the remaining `SandboxPolicy::ReadOnly` assignment is only the compatibility field required by the current turn protocol. ## Verification - `cargo test -p codex-core guardian_review_session_config_clears_parent_developer_instructions` --- [//]: # (BEGIN SAPLING FOOTER) Stack created with [Sapling](https://sapling-scm.com). Best reviewed with [ReviewStack](https://reviewstack.dev/openai/codex/pull/22789). * #22795 * #22792 * #22791 * #22790 * __->__ #22789
bolinfest
added a commit
that referenced
this pull request
May 15, 2026
## Why The permissions instruction builder should consume the new permissions model directly. Keeping a `SandboxPolicy` conversion helper in this path encourages new code to route through legacy sandbox policy values even when the caller already has a `PermissionProfile`. ## What Changed - Removed `PermissionsInstructions::from_policy`. - Removed the test that exercised that legacy helper. - Left the existing profile-based instruction coverage in place. ## How To Review Review `codex-rs/core/src/context/permissions_instructions.rs` first. This PR is intentionally narrow: the production behavior should be unchanged for profile callers, and the deleted surface was only a convenience adapter from `SandboxPolicy`. ## Verification - `cargo test -p codex-core builds_permissions_from_profile` --- [//]: # (BEGIN SAPLING FOOTER) Stack created with [Sapling](https://sapling-scm.com). Best reviewed with [ReviewStack](https://reviewstack.dev/openai/codex/pull/22790). * #22795 * #22792 * #22791 * __->__ #22790
bolinfest
added a commit
that referenced
this pull request
May 15, 2026
## Why Sandbox telemetry tags should be derived from the active permission profile, not from a legacy `SandboxPolicy`, so the tagging code stays aligned with the permissions migration and does not preserve a policy-shaped production helper only for tests. ## What Changed - Removed the production `sandbox_tag(&SandboxPolicy, ...)` helper. - Updated sandbox tag tests to construct the relevant `PermissionProfile` values directly. - Kept the platform-specific sandbox tag behavior under the existing `permission_profile_sandbox_tag` path. ## How To Review The production change is in `codex-rs/core/src/sandbox_tags.rs`. Most of the diff is test cleanup that replaces legacy policy setup with permission profiles, so review the expected tag assertions rather than the old helper mechanics. ## Verification - `cargo test -p codex-core sandbox_tag` --- [//]: # (BEGIN SAPLING FOOTER) Stack created with [Sapling](https://sapling-scm.com). Best reviewed with [ReviewStack](https://reviewstack.dev/openai/codex/pull/22791). * #22795 * #22792 * __->__ #22791
bolinfest
force-pushed
the
pr22792
branch
2 times, most recently
from
bcf35da to
6db97e9
Compare
viyatb-oai
approved these changes
May 15, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
The app-server thread lifecycle API should no longer expose the full
PermissionProfilevalue. After the permissions-profile migration, clients should round-trip only the active profile identity throughactivePermissionProfileandpermissionswhen that identity is known.The full profile is server-side config. Treating a response-derived legacy sandbox projection as a new local profile can lose named-profile restrictions and accidentally widen permissions on the next turn. The legacy
sandboxresponse field remains only as the compatibility/display fallback.What Changed
permissionProfilefromThreadStartResponse,ThreadResumeResponse, andThreadForkResponse.permissionsonly when anactivePermissionProfileid is known, and otherwise sending no sandbox override unless the user selected a local override.thread/resumehydration server-authored whenactivePermissionProfileis absent, which matches the live-thread attach path where the server ignores requested overrides.permissionProfilereference. The remainingpermissionProfileREADME references are request-side permission overrides.large_enum_variant, matching the existing payload enum exemption after the lifecycle response variants shrank.How To Review
Start with
codex-rs/app-server-protocol/src/protocol/v2/thread.rsto confirm the response shape, then check the response construction incodex-rs/app-server/src/request_processors. The generated schema and TypeScript fixture changes are mechanical follow-through from the protocol removal.The TUI behavior is the delicate part: review
codex-rs/tui/src/app_server_session.rsfor response hydration and turn-start override projection, thencodex-rs/tui/src/app/thread_routing.rsfor the decision about whether the next turn should preserve the server snapshot, send an active profile id, or send a legacy sandbox for an explicit local override.Verification
just write-app-server-schemacargo test -p codex-app-server-protocol thread_lifecycle_responses_default_missing_optional_fieldscargo test -p codex-exec session_configured_from_thread_response_uses_permission_profile_from_configcargo test -p codex-tui --lib thread_responsecargo test -p codex-tui turn_permissions_cargo test -p codex-tui resume_response_restores_turns_from_thread_itemscargo test -p codex-analytics track_response_only_enqueues_analytics_relevant_responsesjust fix -p codex-analyticsjust fix -p codex-app-server-protocoljust fix -p codex-tuijust argument-comment-lintStack created with Sapling. Best reviewed with ReviewStack.