mirror of
https://github.com/openai/codex.git
synced 2026-11-10 00:42:33 +00:00
Centralize persisted resume settings lookup (#39147)
## What changed - Add a shared helper for recovering the latest persisted approvals reviewer from turn context and thread settings history. - Use the helper when resuming and forking threads while continuing to honor explicit request overrides. - Fall back to an older persisted reviewer when the latest turn context omits the field. ## Testing - Add unit coverage for settings snapshot precedence, turn context precedence, and fallback to an older reviewer. GitOrigin-RevId: dfc0332b0f1410a4c9a550236eb32624f5133baa
This commit is contained in:
1 parent
230791fd1f
commit
bc7a487039
4 files changed
+180
-29
No files matched your search
@@ -544,6 +544,7 @@ mod git_processor;
|
|||||||
mod initialize_processor;
|
mod initialize_processor;
|
||||||
mod marketplace_processor;
|
mod marketplace_processor;
|
||||||
mod mcp_processor;
|
mod mcp_processor;
|
||||||
|
mod persisted_resume_settings;
|
||||||
mod plugins;
|
mod plugins;
|
||||||
mod process_exec_processor;
|
mod process_exec_processor;
|
||||||
mod projects;
|
mod projects;
|
||||||
|
|||||||
@@ -0,0 +1,40 @@
|
|||||||
|
use codex_protocol::config_types::ApprovalsReviewer;
|
||||||
|
use codex_protocol::protocol::EventMsg;
|
||||||
|
use codex_rollout::RolloutItem;
|
||||||
|
|
||||||
|
#[derive(Debug, PartialEq, Eq)]
|
||||||
|
pub(super) struct PersistedResumeSettings {
|
||||||
|
pub(super) approvals_reviewer: Option<ApprovalsReviewer>,
|
||||||
|
}
|
||||||
|
|
||||||
|
pub(super) fn latest_persisted_resume_settings(
|
||||||
|
history: &[RolloutItem],
|
||||||
|
) -> Option<PersistedResumeSettings> {
|
||||||
|
history
|
||||||
|
.iter()
|
||||||
|
.enumerate()
|
||||||
|
.rev()
|
||||||
|
.find_map(|(index, item)| match item {
|
||||||
|
RolloutItem::TurnContext(turn_context) => Some(PersistedResumeSettings {
|
||||||
|
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)
|
||||||
|
}
|
||||||
|
_ => None,
|
||||||
|
})
|
||||||
|
}),
|
||||||
|
}),
|
||||||
|
RolloutItem::EventMsg(EventMsg::ThreadSettingsApplied(event)) => {
|
||||||
|
Some(PersistedResumeSettings {
|
||||||
|
approvals_reviewer: Some(event.thread_settings.approvals_reviewer),
|
||||||
|
})
|
||||||
|
}
|
||||||
|
_ => None,
|
||||||
|
})
|
||||||
|
}
|
||||||
|
|
||||||
|
#[cfg(test)]
|
||||||
|
#[path = "persisted_resume_settings_tests.rs"]
|
||||||
|
mod tests;
|
||||||
@@ -0,0 +1,120 @@
|
|||||||
|
use super::PersistedResumeSettings;
|
||||||
|
use super::latest_persisted_resume_settings;
|
||||||
|
use codex_protocol::config_types::ApprovalsReviewer;
|
||||||
|
use codex_protocol::config_types::CollaborationMode;
|
||||||
|
use codex_protocol::config_types::ModeKind;
|
||||||
|
use codex_protocol::config_types::Settings;
|
||||||
|
use codex_protocol::models::PermissionProfile;
|
||||||
|
use codex_protocol::protocol::AskForApproval;
|
||||||
|
use codex_protocol::protocol::EventMsg;
|
||||||
|
use codex_protocol::protocol::SandboxPolicy;
|
||||||
|
use codex_protocol::protocol::ThreadSettingsAppliedEvent;
|
||||||
|
use codex_protocol::protocol::ThreadSettingsSnapshot;
|
||||||
|
use codex_protocol::protocol::TurnContextItem;
|
||||||
|
use codex_rollout::RolloutItem;
|
||||||
|
use codex_utils_absolute_path::AbsolutePathBuf;
|
||||||
|
use pretty_assertions::assert_eq;
|
||||||
|
|
||||||
|
fn cwd() -> AbsolutePathBuf {
|
||||||
|
AbsolutePathBuf::try_from(std::env::current_dir().expect("current directory"))
|
||||||
|
.expect("absolute current directory")
|
||||||
|
}
|
||||||
|
|
||||||
|
fn settings_item(approvals_reviewer: ApprovalsReviewer) -> RolloutItem {
|
||||||
|
RolloutItem::EventMsg(EventMsg::ThreadSettingsApplied(
|
||||||
|
ThreadSettingsAppliedEvent {
|
||||||
|
thread_settings: ThreadSettingsSnapshot {
|
||||||
|
model: "gpt-5".to_string(),
|
||||||
|
model_provider_id: "openai".to_string(),
|
||||||
|
service_tier: None,
|
||||||
|
approval_policy: AskForApproval::OnRequest,
|
||||||
|
approvals_reviewer,
|
||||||
|
permission_profile: PermissionProfile::read_only(),
|
||||||
|
active_permission_profile: None,
|
||||||
|
cwd: cwd(),
|
||||||
|
reasoning_effort: None,
|
||||||
|
reasoning_summary: None,
|
||||||
|
personality: None,
|
||||||
|
collaboration_mode: CollaborationMode {
|
||||||
|
mode: ModeKind::Default,
|
||||||
|
settings: Settings {
|
||||||
|
model: "gpt-5".to_string(),
|
||||||
|
reasoning_effort: None,
|
||||||
|
developer_instructions: None,
|
||||||
|
},
|
||||||
|
},
|
||||||
|
},
|
||||||
|
},
|
||||||
|
))
|
||||||
|
}
|
||||||
|
|
||||||
|
fn turn_context_item(turn_id: &str, approvals_reviewer: Option<ApprovalsReviewer>) -> RolloutItem {
|
||||||
|
RolloutItem::TurnContext(TurnContextItem {
|
||||||
|
turn_id: Some(turn_id.to_string()),
|
||||||
|
cwd: cwd(),
|
||||||
|
workspace_roots: None,
|
||||||
|
current_date: None,
|
||||||
|
timezone: None,
|
||||||
|
approval_policy: AskForApproval::OnRequest,
|
||||||
|
approvals_reviewer,
|
||||||
|
sandbox_policy: SandboxPolicy::new_read_only_policy(),
|
||||||
|
permission_profile: None,
|
||||||
|
active_permission_profile: None,
|
||||||
|
network: None,
|
||||||
|
file_system_sandbox_policy: None,
|
||||||
|
model: "gpt-5".to_string(),
|
||||||
|
comp_hash: None,
|
||||||
|
personality: None,
|
||||||
|
collaboration_mode: None,
|
||||||
|
multi_agent_version: None,
|
||||||
|
multi_agent_mode: None,
|
||||||
|
realtime_active: None,
|
||||||
|
effort: None,
|
||||||
|
summary: codex_protocol::config_types::ReasoningSummary::Auto,
|
||||||
|
})
|
||||||
|
}
|
||||||
|
|
||||||
|
#[test]
|
||||||
|
fn latest_settings_snapshot_wins() {
|
||||||
|
let history = vec![
|
||||||
|
settings_item(ApprovalsReviewer::User),
|
||||||
|
settings_item(ApprovalsReviewer::AutoReview),
|
||||||
|
];
|
||||||
|
|
||||||
|
assert_eq!(
|
||||||
|
latest_persisted_resume_settings(&history),
|
||||||
|
Some(PersistedResumeSettings {
|
||||||
|
approvals_reviewer: Some(ApprovalsReviewer::AutoReview),
|
||||||
|
})
|
||||||
|
);
|
||||||
|
}
|
||||||
|
|
||||||
|
#[test]
|
||||||
|
fn latest_turn_context_wins_over_earlier_settings_update() {
|
||||||
|
let history = vec![
|
||||||
|
settings_item(ApprovalsReviewer::AutoReview),
|
||||||
|
turn_context_item("turn-2", Some(ApprovalsReviewer::User)),
|
||||||
|
];
|
||||||
|
|
||||||
|
assert_eq!(
|
||||||
|
latest_persisted_resume_settings(&history),
|
||||||
|
Some(PersistedResumeSettings {
|
||||||
|
approvals_reviewer: Some(ApprovalsReviewer::User),
|
||||||
|
})
|
||||||
|
);
|
||||||
|
}
|
||||||
|
|
||||||
|
#[test]
|
||||||
|
fn older_reviewer_is_used_when_latest_turn_context_omits_it() {
|
||||||
|
let history = vec![
|
||||||
|
turn_context_item("turn-1", Some(ApprovalsReviewer::AutoReview)),
|
||||||
|
turn_context_item("turn-2", /*approvals_reviewer*/ None),
|
||||||
|
];
|
||||||
|
|
||||||
|
assert_eq!(
|
||||||
|
latest_persisted_resume_settings(&history),
|
||||||
|
Some(PersistedResumeSettings {
|
||||||
|
approvals_reviewer: Some(ApprovalsReviewer::AutoReview),
|
||||||
|
})
|
||||||
|
);
|
||||||
|
}
|
||||||
@@ -1,3 +1,4 @@
|
|||||||
|
use super::persisted_resume_settings::latest_persisted_resume_settings;
|
||||||
use super::thread_enrichment::enrich_loaded_threads;
|
use super::thread_enrichment::enrich_loaded_threads;
|
||||||
use super::thread_fork_goal::inherit_thread_goal_snapshot;
|
use super::thread_fork_goal::inherit_thread_goal_snapshot;
|
||||||
use super::turn_processor::can_accept_direct_input;
|
use super::turn_processor::can_accept_direct_input;
|
||||||
@@ -225,26 +226,6 @@ fn merge_persisted_resume_metadata(
|
|||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
fn merge_persisted_approvals_reviewer(
|
|
||||||
history: &[RolloutItem],
|
|
||||||
request_overrides: Option<&HashMap<String, serde_json::Value>>,
|
|
||||||
typesafe_overrides: &mut ConfigOverrides,
|
|
||||||
) {
|
|
||||||
if typesafe_overrides.approvals_reviewer.is_some()
|
|
||||||
|| request_overrides.is_some_and(|overrides| overrides.contains_key("approvals_reviewer"))
|
|
||||||
{
|
|
||||||
return;
|
|
||||||
}
|
|
||||||
|
|
||||||
typesafe_overrides.approvals_reviewer = history.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)
|
|
||||||
}
|
|
||||||
_ => None,
|
|
||||||
});
|
|
||||||
}
|
|
||||||
|
|
||||||
fn latest_persisted_approval_policy(
|
fn latest_persisted_approval_policy(
|
||||||
history: &[RolloutItem],
|
history: &[RolloutItem],
|
||||||
) -> Option<codex_protocol::protocol::AskForApproval> {
|
) -> Option<codex_protocol::protocol::AskForApproval> {
|
||||||
@@ -3950,11 +3931,15 @@ impl ThreadRequestProcessor {
|
|||||||
let InitialHistory::Resumed(resumed_history) = thread_history else {
|
let InitialHistory::Resumed(resumed_history) = thread_history else {
|
||||||
return None;
|
return None;
|
||||||
};
|
};
|
||||||
merge_persisted_approvals_reviewer(
|
if typesafe_overrides.approvals_reviewer.is_none()
|
||||||
&resumed_history.history,
|
&& !request_overrides
|
||||||
request_overrides.as_ref(),
|
.as_ref()
|
||||||
typesafe_overrides,
|
.is_some_and(|overrides| overrides.contains_key("approvals_reviewer"))
|
||||||
);
|
{
|
||||||
|
typesafe_overrides.approvals_reviewer =
|
||||||
|
latest_persisted_resume_settings(&resumed_history.history)
|
||||||
|
.and_then(|settings| settings.approvals_reviewer);
|
||||||
|
}
|
||||||
if typesafe_overrides.approval_policy.is_none() {
|
if typesafe_overrides.approval_policy.is_none() {
|
||||||
typesafe_overrides.approval_policy =
|
typesafe_overrides.approval_policy =
|
||||||
latest_persisted_approval_policy(&resumed_history.history);
|
latest_persisted_approval_policy(&resumed_history.history);
|
||||||
@@ -4711,13 +4696,18 @@ impl ThreadRequestProcessor {
|
|||||||
} else {
|
} else {
|
||||||
None
|
None
|
||||||
};
|
};
|
||||||
merge_persisted_approvals_reviewer(
|
if typesafe_overrides.approvals_reviewer.is_none()
|
||||||
|
&& !request_overrides
|
||||||
|
.as_ref()
|
||||||
|
.is_some_and(|overrides| overrides.contains_key("approvals_reviewer"))
|
||||||
|
{
|
||||||
|
typesafe_overrides.approvals_reviewer = latest_persisted_resume_settings(
|
||||||
latest_context
|
latest_context
|
||||||
.as_deref()
|
.as_deref()
|
||||||
.unwrap_or_else(|| source_history_items.as_ref()),
|
.unwrap_or_else(|| source_history_items.as_ref()),
|
||||||
request_overrides.as_ref(),
|
)
|
||||||
&mut typesafe_overrides,
|
.and_then(|settings| settings.approvals_reviewer);
|
||||||
);
|
}
|
||||||
// Derive a Config using the same logic as new conversation, honoring overrides if provided.
|
// Derive a Config using the same logic as new conversation, honoring overrides if provided.
|
||||||
let config = self
|
let config = self
|
||||||
.config_manager
|
.config_manager
|
||||||
|
|||||||
Reference in new issue
Block a user