mirror of
https://github.com/openai/codex.git
synced 2026-11-08 20:40:33 +00:00
Preserve requested approval reviewers across policy overrides (#59272)
## Why A model requirement can force `auto_review` even when the requested reviewer is `user`. Persisting or sending that effective value back as an explicit choice loses the original selection, so removing the requirement cannot restore it on resume. ## What changed - Retain `requested_approvals_reviewer` separately from the effective reviewer in configuration and thread settings, with a fallback for older histories. - Restore the requested reviewer on resume while applying current model requirements and reviewer allowlists. - Let ordinary TUI turns, continuations, retries, and forks inherit the server's reviewer instead of echoing its effective value as an explicit request. - Preserve a worker's requested reviewer when reloading it rather than inheriting the sender's selection. ## Testing Extend resume coverage for explicit `auto_review`, removal of a model requirement, and a tightened reviewer allowlist. Update TUI assertions for omitted reviewer overrides and verify worker reload preserves its reviewer. GitOrigin-RevId: d8c9c3a7b89e55e63f0e8ae210d6a87cb5547933
This commit is contained in:
@@ -43,7 +43,11 @@ impl From<&ThreadSettingsSnapshot> for PersistedResumeSettings {
|
||||
fn from(settings: &ThreadSettingsSnapshot) -> Self {
|
||||
Self {
|
||||
approval_policy: settings.approval_policy,
|
||||
approvals_reviewer: Some(settings.approvals_reviewer),
|
||||
approvals_reviewer: Some(
|
||||
settings
|
||||
.requested_approvals_reviewer
|
||||
.unwrap_or(settings.approvals_reviewer),
|
||||
),
|
||||
active_permission_profile: settings.active_permission_profile.clone(),
|
||||
unnamed_permission_profile: settings
|
||||
.active_permission_profile
|
||||
@@ -78,9 +82,12 @@ fn latest_persisted_resume_settings(history: &[RolloutItem]) -> Option<Persisted
|
||||
approvals_reviewer: turn_context.approvals_reviewer.or_else(|| {
|
||||
history[..index].iter().rev().find_map(|item| match item {
|
||||
RolloutItem::TurnContext(turn_context) => turn_context.approvals_reviewer,
|
||||
RolloutItem::EventMsg(EventMsg::ThreadSettingsApplied(event)) => {
|
||||
Some(event.thread_settings.approvals_reviewer)
|
||||
}
|
||||
RolloutItem::EventMsg(EventMsg::ThreadSettingsApplied(event)) => Some(
|
||||
event
|
||||
.thread_settings
|
||||
.requested_approvals_reviewer
|
||||
.unwrap_or(event.thread_settings.approvals_reviewer),
|
||||
),
|
||||
_ => None,
|
||||
})
|
||||
}),
|
||||
@@ -93,7 +100,11 @@ fn latest_persisted_resume_settings(history: &[RolloutItem]) -> Option<Persisted
|
||||
let settings = &event.thread_settings;
|
||||
Some(PersistedResumeSettings {
|
||||
approval_policy: settings.approval_policy,
|
||||
approvals_reviewer: Some(settings.approvals_reviewer),
|
||||
approvals_reviewer: Some(
|
||||
settings
|
||||
.requested_approvals_reviewer
|
||||
.unwrap_or(settings.approvals_reviewer),
|
||||
),
|
||||
active_permission_profile: settings.active_permission_profile.clone(),
|
||||
unnamed_permission_profile: None,
|
||||
})
|
||||
|
||||
@@ -38,6 +38,7 @@ fn settings_item(
|
||||
service_tier: None,
|
||||
approval_policy,
|
||||
approvals_reviewer,
|
||||
requested_approvals_reviewer: None,
|
||||
permission_profile: PermissionProfile::read_only(),
|
||||
active_permission_profile,
|
||||
cwd: cwd(),
|
||||
|
||||
@@ -937,6 +937,7 @@ impl TurnRequestProcessor {
|
||||
runtime_workspace_roots: runtime_workspace_roots.clone(),
|
||||
approval_policy,
|
||||
approvals_reviewer,
|
||||
requested_approvals_reviewer: None,
|
||||
sandbox_policy: sandbox_policy.clone(),
|
||||
permission_profile: permission_profile.clone(),
|
||||
active_permission_profile: active_permission_profile.clone(),
|
||||
|
||||
Reference in New Issue
Block a user