From bc7a4870398ac8f7ac90aaab1dc10ee0766f7fe1 Mon Sep 17 00:00:00 2001 From: Shijie Rao Date: Tue, 18 Aug 2026 05:50:51 +0000 Subject: [PATCH] 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 --- codex-rs/app-server/src/request_processors.rs | 1 + .../persisted_resume_settings.rs | 40 ++++++ .../persisted_resume_settings_tests.rs | 120 ++++++++++++++++++ .../request_processors/thread_processor.rs | 54 ++++---- 4 files changed, 183 insertions(+), 32 deletions(-) create mode 100644 codex-rs/app-server/src/request_processors/persisted_resume_settings.rs create mode 100644 codex-rs/app-server/src/request_processors/persisted_resume_settings_tests.rs diff --git a/codex-rs/app-server/src/request_processors.rs b/codex-rs/app-server/src/request_processors.rs index f2da39044a..975d016297 100644 --- a/codex-rs/app-server/src/request_processors.rs +++ b/codex-rs/app-server/src/request_processors.rs @@ -544,6 +544,7 @@ mod git_processor; mod initialize_processor; mod marketplace_processor; mod mcp_processor; +mod persisted_resume_settings; mod plugins; mod process_exec_processor; mod projects; diff --git a/codex-rs/app-server/src/request_processors/persisted_resume_settings.rs b/codex-rs/app-server/src/request_processors/persisted_resume_settings.rs new file mode 100644 index 0000000000..55e28953fb --- /dev/null +++ b/codex-rs/app-server/src/request_processors/persisted_resume_settings.rs @@ -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, +} + +pub(super) fn latest_persisted_resume_settings( + history: &[RolloutItem], +) -> Option { + 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; diff --git a/codex-rs/app-server/src/request_processors/persisted_resume_settings_tests.rs b/codex-rs/app-server/src/request_processors/persisted_resume_settings_tests.rs new file mode 100644 index 0000000000..48fc65cd0c --- /dev/null +++ b/codex-rs/app-server/src/request_processors/persisted_resume_settings_tests.rs @@ -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) -> 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), + }) + ); +} diff --git a/codex-rs/app-server/src/request_processors/thread_processor.rs b/codex-rs/app-server/src/request_processors/thread_processor.rs index 2a17787f93..4fda4b08a3 100644 --- a/codex-rs/app-server/src/request_processors/thread_processor.rs +++ b/codex-rs/app-server/src/request_processors/thread_processor.rs @@ -1,3 +1,4 @@ +use super::persisted_resume_settings::latest_persisted_resume_settings; use super::thread_enrichment::enrich_loaded_threads; use super::thread_fork_goal::inherit_thread_goal_snapshot; 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>, - 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( history: &[RolloutItem], ) -> Option { @@ -3950,11 +3931,15 @@ impl ThreadRequestProcessor { let InitialHistory::Resumed(resumed_history) = thread_history else { return None; }; - merge_persisted_approvals_reviewer( - &resumed_history.history, - request_overrides.as_ref(), - typesafe_overrides, - ); + 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(&resumed_history.history) + .and_then(|settings| settings.approvals_reviewer); + } if typesafe_overrides.approval_policy.is_none() { typesafe_overrides.approval_policy = latest_persisted_approval_policy(&resumed_history.history); @@ -4711,13 +4696,18 @@ impl ThreadRequestProcessor { } else { None }; - merge_persisted_approvals_reviewer( - latest_context - .as_deref() - .unwrap_or_else(|| source_history_items.as_ref()), - request_overrides.as_ref(), - &mut typesafe_overrides, - ); + 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 + .as_deref() + .unwrap_or_else(|| source_history_items.as_ref()), + ) + .and_then(|settings| settings.approvals_reviewer); + } // Derive a Config using the same logic as new conversation, honoring overrides if provided. let config = self .config_manager