Skip to content

core: stop passing legacy SandboxPolicy to guardian reviews#25911

Merged
bolinfest merged 1 commit into
mainfrom
pr25911
Jun 2, 2026
Merged

core: stop passing legacy SandboxPolicy to guardian reviews#25911
bolinfest merged 1 commit into
mainfrom
pr25911

Conversation

@bolinfest
Copy link
Copy Markdown
Collaborator

@bolinfest bolinfest commented Jun 2, 2026

Why

Guardian review turns already submit a read-only PermissionProfile, which is the permissions model the runtime should honor. Passing the equivalent legacy SandboxPolicy through ThreadSettingsOverrides keeps two representations of the same read-only constraint alive on this path and makes the guardian flow depend on compatibility plumbing that is being phased out.

What Changed

  • Set sandbox_policy to None when the guardian review session submits its child Op::UserInput.
  • Keep permission_profile: Some(PermissionProfile::read_only()) and approval_policy: Some(AskForApproval::Never), so the guardian review remains read-only and cannot request approvals.
  • Remove the now-unused SandboxPolicy import and redundant comment from codex-rs/core/src/guardian/review_session.rs.

Verification

Not run locally; this is a narrow cleanup of redundant thread-settings override state.

@bolinfest bolinfest requested a review from a team as a code owner June 2, 2026 18:14
@bolinfest bolinfest changed the base branch from main to pr25700 June 2, 2026 18:14
@bolinfest bolinfest changed the base branch from pr25700 to main June 2, 2026 19:08
@bolinfest bolinfest changed the title chore: stop passing SandboxPolicy for ThreadSettingsOverrides with guardian core: stop passing legacy SandboxPolicy to guardian reviews Jun 2, 2026
@bolinfest bolinfest requested a review from viyatb-oai June 2, 2026 19:16
Copy link
Copy Markdown
Collaborator

@viyatb-oai viyatb-oai left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. permission_profile is authoritative in SessionConfiguration::apply, so the legacy SandboxPolicy override is redundant.

Optional follow-up: is it beneficial to add an integration test where the guardian reviewer attempts a write and verify that the read-only PermissionProfile blocks it?

@bolinfest bolinfest merged commit f6d64bd into main Jun 2, 2026
46 of 61 checks passed
@bolinfest bolinfest deleted the pr25911 branch June 2, 2026 21:16
@github-actions github-actions Bot locked and limited conversation to collaborators Jun 2, 2026
@bolinfest
Copy link
Copy Markdown
Collaborator Author

Optional follow-up: is it beneficial to add an integration test where the guardian reviewer attempts a write and verify that the read-only PermissionProfile blocks it?

Will investigate in a subsequent PR.

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