openai/codex
1
0
Fork 0
mirror of https://github.com/openai/codex.git synced 2026-10-19 17:28:46 +00:00
Code Issues Packages Projects Releases Wiki Activity

Preserve approval policies for auto-reviewed models (#38439)

Browse Source 
## What changed

- Keep the configured `approvalPolicy` when a model in
  `auto_review.required_on_models` selects the `auto_review` reviewer.
- Continue downgrading full-access sessions to workspace-write and rejecting
  incompatible reviewer overrides.

## Testing

- Cover supported approval policies across thread start, settings updates,
  turn-time model switches, resume, and fork flows.

GitOrigin-RevId: 2bd3e2dcd46f3cadeb2fad20222cf8917c267208
This commit is contained in:
viyatb-oai
2026-08-13 21:40:42 +00:00
committed by copyberry
parent b87327f4e5
commit 1992f8c018
4 changed files with 103 additions and 33 deletions
Download Patch File Download Diff File Expand all files Collapse all files
codex-rs/app-server/README.md
+1 -1
View File
@@ -876,7 +876,7 @@ required_on_models = ["protected-model"]
ignore_rules = ["protected-model"]
```
Models in `required_on_models` always start with `approvalPolicy: "on-request"` and `approvalsReviewer: "auto_review"`, even when clients provide incompatible startup values. Full Access is automatically downgraded to workspace-write access. Incompatible runtime overrides or disabled Guardian automatic review are rejected. Models in `ignore_rules` ignore saved command-prefix approvals.
Models in `required_on_models` use `approvalsReviewer: "auto_review"` while preserving any valid configured `approvalPolicy`. Full Access is downgraded to workspace-write access. Incompatible runtime overrides or disabled Guardian automatic review are rejected. Models in `ignore_rules` ignore saved command-prefix approvals.
```json
{ "method": "turn/start", "id": 30, "params": {
codex-rs/app-server/tests/suite/v2/model_auto_review.rs
+93 -11
View File
@@ -8,6 +8,7 @@ use codex_app_server_protocol::ApprovalsReviewer::User;
use codex_app_server_protocol::AskForApproval;
use codex_app_server_protocol::AskForApproval::Never;
use codex_app_server_protocol::AskForApproval::OnRequest;
use codex_app_server_protocol::AskForApproval::UnlessTrusted;
use codex_app_server_protocol::JSONRPCError;
use codex_app_server_protocol::RequestId;
use codex_app_server_protocol::SandboxMode;
@@ -32,8 +33,19 @@ use tokio::time::timeout;
const TIMEOUT: Duration = Duration::from_secs(10);
const MODEL: &str = "protected-model";
const REQUIREMENTS: &str = "[auto_review]\nrequired_on_models = [\"protected-model\"]\n";
const UNSAFE: [(Option<AskForApproval>, Option<ApprovalsReviewer>); 2] =
[(Some(Never), None), (None, Some(User))];
const APPROVAL_POLICIES: [AskForApproval; 4] = [
UnlessTrusted,
OnRequest,
AskForApproval::Granular {
sandbox_approval: true,
rules: false,
skill_approval: false,
request_permissions: true,
mcp_elicitations: false,
},
Never,
];
const UNSAFE: [(Option<AskForApproval>, Option<ApprovalsReviewer>); 1] = [(None, Some(User))];
macro_rules! params {
($ty:ident, $($field:ident $(= $value:expr)?),* $(,)?) => {
@@ -74,20 +86,36 @@ async fn assert_error(server: &mut TestAppServer, request_id: i64, message: &str
Ok(())
}
async fn assert_protected_update(server: &mut TestAppServer) -> Result<()> {
async fn assert_protected_update(
server: &mut TestAppServer,
expected_policy: AskForApproval,
) -> Result<()> {
let updated: SettingsUpdated =
timeout(TIMEOUT, server.read_notification("thread/settings/updated")).await??;
let settings = updated.thread_settings;
assert_protected(
assert_protected_with_policy(
&settings.model,
settings.approval_policy,
settings.approvals_reviewer,
expected_policy,
);
Ok(())
}
fn assert_protected(model: &str, policy: AskForApproval, reviewer: ApprovalsReviewer) {
assert_eq!((model, policy, reviewer), (MODEL, OnRequest, AutoReview));
assert_protected_with_policy(model, policy, reviewer, OnRequest);
}
fn assert_protected_with_policy(
model: &str,
policy: AskForApproval,
reviewer: ApprovalsReviewer,
expected_policy: AskForApproval,
) {
assert_eq!(
(model, policy, reviewer),
(MODEL, expected_policy, AutoReview)
);
}
#[tokio::test]
@@ -101,6 +129,21 @@ async fn thread_start_enforces_protected_model_auto_review() -> Result<()> {
started.approval_policy,
started.approvals_reviewer,
);
for approval_policy in APPROVAL_POLICIES {
let started = server
.start_thread(params!(
StartParams,
model = Some(MODEL.to_string()),
approval_policy = Some(approval_policy),
))
.await?;
assert_protected_with_policy(
&started.model,
started.approval_policy,
started.approvals_reviewer,
approval_policy,
);
}
for (approval_policy, approvals_reviewer) in UNSAFE {
let started = server
.start_thread(params!(
@@ -125,10 +168,11 @@ async fn thread_start_enforces_protected_model_auto_review() -> Result<()> {
sandbox = Some(SandboxMode::DangerFullAccess),
))
.await?;
assert_protected(
assert_protected_with_policy(
&started.model,
started.approval_policy,
started.approvals_reviewer,
Never,
);
assert!(matches!(
started.sandbox,
@@ -162,7 +206,7 @@ async fn thread_and_turn_settings_enforce_protected_model_auto_review() -> Resul
))
.await?;
let _: UpdateResponse = timeout(TIMEOUT, server.read_response(id)).await??;
assert_protected_update(&mut server).await?;
assert_protected_update(&mut server, OnRequest).await?;
for (approval_policy, approvals_reviewer) in UNSAFE {
let id = server
.send_thread_settings_update_request(params!(
@@ -174,6 +218,17 @@ async fn thread_and_turn_settings_enforce_protected_model_auto_review() -> Resul
.await?;
assert_error(&mut server, id, "you need to use auto review").await?;
}
for approval_policy in APPROVAL_POLICIES {
let id = server
.send_thread_settings_update_request(params!(
UpdateParams,
thread_id = thread.id.clone(),
approval_policy = Some(approval_policy),
))
.await?;
let _: UpdateResponse = timeout(TIMEOUT, server.read_response(id)).await??;
assert_protected_update(&mut server, approval_policy).await?;
}
let id = server
.send_thread_settings_update_request(params!(
UpdateParams,
@@ -209,7 +264,27 @@ async fn thread_and_turn_settings_enforce_protected_model_auto_review() -> Resul
))
.await?;
let _: TurnResponse = timeout(TIMEOUT, server.read_response(id)).await??;
assert_protected_update(&mut server).await
assert_protected_update(&mut server, OnRequest).await?;
for approval_policy in APPROVAL_POLICIES {
let policy_turn_thread = server
.start_thread(params!(
StartParams,
approval_policy = Some(approval_policy)
))
.await?
.thread;
let id = server
.send_turn_start_request(params!(
TurnParams,
thread_id = policy_turn_thread.id,
model = Some(MODEL.to_string()),
))
.await?;
let _: TurnResponse = timeout(TIMEOUT, server.read_response(id)).await??;
assert_protected_update(&mut server, approval_policy).await?;
}
Ok(())
}
#[tokio::test]
@@ -257,7 +332,12 @@ async fn thread_resume_and_fork_upgrade_legacy_protected_model_settings() -> Res
))
.await?;
let fork: ForkResponse = timeout(TIMEOUT, server.read_response(id)).await??;
assert_protected(&fork.model, fork.approval_policy, fork.approvals_reviewer);
assert_protected_with_policy(
&fork.model,
fork.approval_policy,
fork.approvals_reviewer,
Never,
);
let id = server
.send_thread_fork_request(params!(
ForkParams,
@@ -276,19 +356,21 @@ async fn thread_resume_and_fork_upgrade_legacy_protected_model_settings() -> Res
))
.await?;
let resumed: ResumeResponse = timeout(TIMEOUT, server.read_response(id)).await??;
assert_protected(
assert_protected_with_policy(
&resumed.model,
resumed.approval_policy,
resumed.approvals_reviewer,
Never,
);
let id = server
.send_thread_resume_request(params!(ResumeParams, thread_id))
.await?;
let resumed: ResumeResponse = timeout(TIMEOUT, server.read_response(id)).await??;
assert_protected(
assert_protected_with_policy(
&resumed.model,
resumed.approval_policy,
resumed.approvals_reviewer,
Never,
);
Ok(())
}
codex-rs/core/src/session/mod.rs
-5
View File
@@ -612,11 +612,6 @@ impl Session {
);
}
}
config
.permissions
.approval_policy
.set(AskForApproval::OnRequest)
.map_err(|err| CodexErr::InvalidRequest(err.to_string()))?;
config
.config_layer_stack
.requirements()
codex-rs/core/src/session/session.rs
+9 -16
View File
@@ -260,8 +260,7 @@ impl SessionConfiguration {
return Ok(());
}
if self.approval_policy.value() == AskForApproval::OnRequest
&& self.approvals_reviewer == ApprovalsReviewer::AutoReview
if self.approvals_reviewer == ApprovalsReviewer::AutoReview
&& !self
.file_system_sandbox_policy(environments)
.has_full_disk_write_access()
@@ -350,21 +349,15 @@ impl SessionConfiguration {
.config_layer_stack
.requirements()
.auto_review_required_for_model(next_configuration.collaboration_mode.model())
&& updates.approvals_reviewer.is_none()
{
if updates.approval_policy.is_none() {
next_configuration
.approval_policy
.set(AskForApproval::OnRequest)?;
}
if updates.approvals_reviewer.is_none() {
next_configuration
.original_config_do_not_use
.config_layer_stack
.requirements()
.approvals_reviewer
.can_set(&ApprovalsReviewer::AutoReview)?;
next_configuration.approvals_reviewer = ApprovalsReviewer::AutoReview;
}
next_configuration
.original_config_do_not_use
.config_layer_stack
.requirements()
.approvals_reviewer
.can_set(&ApprovalsReviewer::AutoReview)?;
next_configuration.approvals_reviewer = ApprovalsReviewer::AutoReview;
}
if let Some(windows_sandbox_level) = updates.windows_sandbox_level {
next_configuration.windows_sandbox_level = windows_sandbox_level;