mirror of
https://github.com/openai/codex.git
synced 2026-11-13 06:45:07 +00:00
Allow updating the approval reviewer for active turns (#42121)
## What changed - Add `approvalsReviewer` to the experimental `turn/settings/update` API. - Apply reviewer changes to subsequently captured steps and new background approval requests while preserving pending approvals and future-thread defaults. - Allow reviewer-only updates without `step_model_switching`, while continuing to enforce managed reviewer restrictions and model-required auto review. - Use an explicit live reviewer update for MCP approvals while retaining refreshed thread defaults for clients that have not set one. ## Testing - Cover switching between user and automatic review during a live turn, future-turn isolation, and managed-authority rejection. GitOrigin-RevId: e1216b09c6a7972c2c9eaf4955d8d57d60604ca1
This commit is contained in:
1 parent
bc39b0ed59
commit
9112564114
14 files changed
+267
-28
No files matched your search
@@ -724,6 +724,7 @@ async fn review_guardian_mcp_elicitation(
|
||||
let Some(mcp_config) = session.services.mcp_runtime.current_config() else {
|
||||
return Ok(None);
|
||||
};
|
||||
let step_settings = turn_context.current_settings.load_full();
|
||||
|
||||
// The invocation identifies the tool event, but a nested elicitation can
|
||||
// review a different action and connector than the enclosing JavaScript.
|
||||
@@ -893,7 +894,9 @@ async fn review_guardian_mcp_elicitation(
|
||||
|
||||
let approvals_reviewer = crate::connectors::mcp_approvals_reviewer_from_layers(
|
||||
&mcp_config.config_layer_stack,
|
||||
mcp_config.approvals_reviewer,
|
||||
step_settings
|
||||
.mcp_approvals_reviewer_override
|
||||
.unwrap_or(mcp_config.approvals_reviewer),
|
||||
Some(turn_context.model_info().slug.as_str()),
|
||||
request.server_name.as_str(),
|
||||
elicitation_connector_id(&request.elicitation),
|
||||
|
||||
@@ -3479,11 +3479,14 @@ impl Session {
|
||||
if settings.selected().service_tier != root_service_tier {
|
||||
let mut selected = settings.selected().clone();
|
||||
selected.service_tier = root_service_tier;
|
||||
settings = Arc::new(ResolvedStepSettings::new(
|
||||
let mut inherited_settings = ResolvedStepSettings::new(
|
||||
Arc::new(selected),
|
||||
Arc::clone(&settings.model_info),
|
||||
self.features.enabled(Feature::FastMode),
|
||||
));
|
||||
);
|
||||
inherited_settings.mcp_approvals_reviewer_override =
|
||||
settings.mcp_approvals_reviewer_override;
|
||||
settings = Arc::new(inherited_settings);
|
||||
}
|
||||
}
|
||||
let token_budget = token_budget::resolve_token_budget(
|
||||
|
||||
@@ -48,18 +48,13 @@ fn check_legacy_turn_safety(
|
||||
|| ignored_models.is_some_and(|models| models.contains(&model.slug))
|
||||
};
|
||||
|
||||
// tools::approvals and guardian::review still route approvals using the
|
||||
// originating turn's policy, reviewer, and required-model classification.
|
||||
// Approval policy and required-model classification still have consumers
|
||||
// using the originating turn. The reviewer is captured separately.
|
||||
if destination.constrained_approval_policy() != current.constrained_approval_policy()
|
||||
|| destination.approval_policy() != turn_context.approval_policy()
|
||||
{
|
||||
return Err("the destination changes the admitted approval policy".to_string());
|
||||
}
|
||||
if destination.approvals_reviewer() != current.approvals_reviewer()
|
||||
|| destination.approvals_reviewer() != turn_context.config.approvals_reviewer
|
||||
{
|
||||
return Err("the destination changes the admitted approvals reviewer".to_string());
|
||||
}
|
||||
if required_review
|
||||
!= requirements
|
||||
.auto_review_required_for_model(current.selected_collaboration_mode().model())
|
||||
@@ -237,7 +232,12 @@ impl Session {
|
||||
turn_id: &str,
|
||||
update: TurnSettingsUpdate,
|
||||
) -> TurnSettingsUpdateOutcome {
|
||||
if !self.features.enabled(Feature::StepModelSwitching) {
|
||||
let reviewer_only = update.approvals_reviewer.is_some()
|
||||
&& update.model.is_none()
|
||||
&& update.effort.is_none()
|
||||
&& update.summary.is_none()
|
||||
&& update.service_tier.is_none();
|
||||
if !reviewer_only && !self.features.enabled(Feature::StepModelSwitching) {
|
||||
return TurnSettingsUpdateOutcome::Rejected {
|
||||
reason: "turn settings updates require the step_model_switching feature"
|
||||
.to_string(),
|
||||
@@ -265,12 +265,14 @@ impl Session {
|
||||
return TurnSettingsUpdateOutcome::TargetUnavailable;
|
||||
};
|
||||
let TurnSettingsUpdate {
|
||||
approvals_reviewer,
|
||||
model,
|
||||
effort,
|
||||
summary,
|
||||
service_tier,
|
||||
} = update;
|
||||
let update = StepSettingsUpdate {
|
||||
approvals_reviewer,
|
||||
model,
|
||||
effort,
|
||||
reasoning_summary: summary,
|
||||
@@ -313,6 +315,11 @@ impl Session {
|
||||
)
|
||||
.map_err(|error| error.to_string())
|
||||
.and_then(|()| {
|
||||
// A reviewer-only patch cannot change any model-owned authority.
|
||||
// Managed reviewer restrictions were checked above.
|
||||
if reviewer_only {
|
||||
return Ok(());
|
||||
}
|
||||
check_legacy_turn_safety(
|
||||
&turn_context,
|
||||
¤t,
|
||||
|
||||
@@ -607,6 +607,60 @@ enum ManagedAuthorizationChange {
|
||||
ApprovalsReviewer,
|
||||
}
|
||||
|
||||
#[test_case(false; "reviewer allow-list")]
|
||||
#[test_case(true; "model-required review")]
|
||||
#[tokio::test]
|
||||
async fn reviewer_only_activation_enforces_managed_authority(required_review: bool) {
|
||||
let ActivationFixture { session, turn, .. } = activation_fixture(activation_models()).await;
|
||||
let original = turn.current_settings.load_full();
|
||||
let desired = desired_step_settings(&session).await;
|
||||
let source = RequirementSource::Unknown;
|
||||
let mut sourced = ConfigRequirementsWithSources::default();
|
||||
let reviewer = if required_review {
|
||||
sourced.auto_review = Some(Sourced::new(
|
||||
AutoReviewRequirementsToml {
|
||||
required_on_models: Some(vec![turn.model_info().slug.clone()]),
|
||||
..Default::default()
|
||||
},
|
||||
source,
|
||||
));
|
||||
ApprovalsReviewer::User
|
||||
} else {
|
||||
sourced.allowed_approvals_reviewers =
|
||||
Some(Sourced::new(vec![ApprovalsReviewer::User], source));
|
||||
ApprovalsReviewer::AutoReview
|
||||
};
|
||||
{
|
||||
let mut state = session.state.lock().await;
|
||||
let config = Arc::make_mut(&mut state.session_configuration.original_config_do_not_use);
|
||||
config.config_layer_stack = ConfigLayerStack::new(
|
||||
config
|
||||
.config_layer_stack
|
||||
.all_layers_low_to_high()
|
||||
.cloned()
|
||||
.collect(),
|
||||
ConfigRequirements::try_from(sourced.clone()).expect("managed requirements"),
|
||||
sourced.into_toml(),
|
||||
)
|
||||
.expect("managed config stack");
|
||||
}
|
||||
assert!(matches!(
|
||||
session
|
||||
.apply_turn_settings(
|
||||
&turn.sub_id,
|
||||
TurnSettingsUpdate {
|
||||
approvals_reviewer: Some(reviewer),
|
||||
..Default::default()
|
||||
}
|
||||
)
|
||||
.await,
|
||||
TurnSettingsUpdateOutcome::Rejected { .. }
|
||||
));
|
||||
assert!(Arc::ptr_eq(&turn.current_settings.load_full(), &original));
|
||||
assert_eq!(desired_step_settings(&session).await, desired);
|
||||
session.abort_all_tasks(TurnAbortReason::Replaced).await;
|
||||
}
|
||||
|
||||
#[test_case(ManagedAuthorizationChange::ApprovalPolicy; "approval policy refreshed during lookup")]
|
||||
#[test_case(ManagedAuthorizationChange::ApprovalsReviewer; "approvals reviewer refreshed during lookup")]
|
||||
#[tokio::test]
|
||||
|
||||
@@ -47,6 +47,9 @@ pub(crate) struct ResolvedStepSettings {
|
||||
pub(crate) reasoning_summary: ReasoningSummary,
|
||||
/// Tier sent on requests, filtered by feature and model support.
|
||||
pub(crate) service_tier: Option<String>,
|
||||
/// MCP historically follows refreshed thread defaults. Preserve that behavior
|
||||
/// for older clients until this turn receives an explicit live reviewer update.
|
||||
pub(crate) mcp_approvals_reviewer_override: Option<ApprovalsReviewer>,
|
||||
}
|
||||
|
||||
impl ResolvedStepSettings {
|
||||
@@ -69,6 +72,7 @@ impl ResolvedStepSettings {
|
||||
model_info,
|
||||
reasoning_summary,
|
||||
service_tier,
|
||||
mcp_approvals_reviewer_override: None,
|
||||
}
|
||||
}
|
||||
|
||||
@@ -135,7 +139,11 @@ impl ResolvedStepSettings {
|
||||
.await,
|
||||
)
|
||||
};
|
||||
Ok(Self::new(Arc::new(selected), model_info, fast_mode_enabled))
|
||||
let mut next = Self::new(Arc::new(selected), model_info, fast_mode_enabled);
|
||||
next.mcp_approvals_reviewer_override = update
|
||||
.approvals_reviewer
|
||||
.or(self.mcp_approvals_reviewer_override);
|
||||
Ok(next)
|
||||
}
|
||||
|
||||
/// Rechecks the retained selection against current managed constraints.
|
||||
|
||||
@@ -9141,8 +9141,12 @@ async fn refreshed_mcp_binding_captures_current_approval_authority() {
|
||||
);
|
||||
}
|
||||
|
||||
#[test_case(false; "legacy thread reviewer")]
|
||||
#[test_case(true; "explicit live reviewer")]
|
||||
#[tokio::test]
|
||||
async fn mcp_elicitation_reviewer_uses_latest_runtime_authority() {
|
||||
async fn mcp_elicitation_reviewer_uses_active_reviewer_and_latest_runtime_policy(
|
||||
live_update: bool,
|
||||
) {
|
||||
let guardian_server = start_mock_server().await;
|
||||
mount_sse_once(
|
||||
&guardian_server,
|
||||
@@ -9182,17 +9186,34 @@ async fn mcp_elicitation_reviewer_uses_latest_runtime_authority() {
|
||||
)
|
||||
.await;
|
||||
|
||||
session
|
||||
.update_settings(SessionSettingsUpdate {
|
||||
step_settings: StepSettingsUpdate {
|
||||
approvals_reviewer: Some(ApprovalsReviewer::AutoReview),
|
||||
..Default::default()
|
||||
},
|
||||
..Default::default()
|
||||
})
|
||||
.await
|
||||
.expect("reviewer settings should update");
|
||||
session.mark_mcp_runtime_dirty();
|
||||
session.refresh_mcp_if_dirty().await;
|
||||
if live_update {
|
||||
assert_eq!(
|
||||
session
|
||||
.apply_turn_settings(
|
||||
&old_turn.sub_id,
|
||||
codex_protocol::protocol::TurnSettingsUpdate {
|
||||
approvals_reviewer: Some(ApprovalsReviewer::AutoReview),
|
||||
..Default::default()
|
||||
},
|
||||
)
|
||||
.await,
|
||||
codex_protocol::protocol::TurnSettingsUpdateOutcome::Applied
|
||||
);
|
||||
} else {
|
||||
session
|
||||
.update_settings(SessionSettingsUpdate {
|
||||
step_settings: StepSettingsUpdate {
|
||||
approvals_reviewer: Some(ApprovalsReviewer::AutoReview),
|
||||
..Default::default()
|
||||
},
|
||||
..Default::default()
|
||||
})
|
||||
.await
|
||||
.expect("reviewer settings should update");
|
||||
session.refresh_mcp_if_dirty().await;
|
||||
}
|
||||
|
||||
let request = codex_mcp::ElicitationReviewRequest {
|
||||
server_name: "browser-use".to_string(),
|
||||
|
||||
Reference in new issue
Block a user