From 66225461695f0f1815333e71bbe354472f393963 Mon Sep 17 00:00:00 2001 From: Shijie Rao Date: Fri, 7 Aug 2026 03:31:55 +0000 Subject: [PATCH] Restore approval policy when resuming threads (#37368) ## What changed - On cold `thread/resume`, restore the latest approval policy from persisted thread settings instead of falling back to the current configured default. - Preserve the precedence of an explicit request override over the persisted policy. - Account for settings applied during a turn when the subsequent compacted turn context contains a stale policy. ## Testing - Add unit coverage for selecting the latest persisted policy. - Add app-server tests for legacy and paginated history, including explicit resume overrides. GitOrigin-RevId: 9127a3c53a90f16d67957ded9e7430e79180fc60 --- codex-rs/app-server/README.md | 2 +- .../request_processors/thread_processor.rs | 61 ++++++++ .../thread_processor_tests.rs | 130 ++++++++++++++++++ .../tests/suite/v2/thread_resume.rs | 128 +++++++++++++++++ 4 files changed, 320 insertions(+), 1 deletion(-) diff --git a/codex-rs/app-server/README.md b/codex-rs/app-server/README.md index d05cf4a5cf..5bd8d77099 100644 --- a/codex-rs/app-server/README.md +++ b/codex-rs/app-server/README.md @@ -347,7 +347,7 @@ Start a fresh thread when you need a new Codex conversation. Valid `personality` values are `"friendly"`, `"pragmatic"`, and `"none"`. When `"none"` is selected, the personality placeholder is replaced with an empty string. -To continue a stored session, call `thread/resume` with the `thread.id` you previously recorded. The response shape matches `thread/start`. When the stored session includes persisted token usage, the server emits `thread/tokenUsage/updated` immediately after the response so clients can render restored usage before the next turn starts. You can also pass the same configuration overrides supported by `thread/start`, including `approvalsReviewer`. +To continue a stored session, call `thread/resume` with the `thread.id` you previously recorded. The response shape matches `thread/start`. When the stored session includes persisted token usage, the server emits `thread/tokenUsage/updated` immediately after the response so clients can render restored usage before the next turn starts. You can also pass the same configuration overrides supported by `thread/start`, including `approvalsReviewer`. On cold resume, approval policy uses the first allowed value in this order: request override, latest persisted thread setting, current configured default. By default, `thread/resume` includes the reconstructed turn history in `thread.turns`. Experimental clients can pass `excludeTurns: true` to return only thread metadata and live resume state, then call `thread/turns/list` separately if they want to page the turn history over the network. A cold paginated resume can still replay persisted `thread/tokenUsage/updated` when it can identify the corresponding stored turn; resuming an already-loaded thread waits for the next live update. 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 07c9d1755f..a12874fea3 100644 --- a/codex-rs/app-server/src/request_processors/thread_processor.rs +++ b/codex-rs/app-server/src/request_processors/thread_processor.rs @@ -194,6 +194,42 @@ fn merge_persisted_approvals_reviewer( }); } +fn latest_persisted_approval_policy( + history: &[RolloutItem], +) -> Option { + history + .iter() + .enumerate() + .rev() + .find_map(|(index, item)| match item { + RolloutItem::TurnContext(turn_context) => { + let updated_policy = turn_context.turn_id.as_ref().and_then(|turn_id| { + let turn_start = history[..index].iter().rposition(|item| { + matches!( + item, + RolloutItem::EventMsg(EventMsg::TurnStarted(event)) + if &event.turn_id == turn_id + ) + })?; + history[turn_start + 1..index] + .iter() + .rev() + .find_map(|item| match item { + RolloutItem::EventMsg(EventMsg::ThreadSettingsApplied(event)) => { + Some(event.thread_settings.approval_policy) + } + _ => None, + }) + }); + Some(updated_policy.unwrap_or(turn_context.approval_policy)) + } + RolloutItem::EventMsg(EventMsg::ThreadSettingsApplied(event)) => { + Some(event.thread_settings.approval_policy) + } + _ => None, + }) +} + fn normalize_thread_list_cwd_filters( cwd: Option, ) -> Result>, JSONRPCErrorError> { @@ -3190,6 +3226,27 @@ impl ThreadRequestProcessor { developer_instructions, personality, ); + if typesafe_overrides.approval_policy.is_none() + && let Some(value) = request_overrides + .as_mut() + .and_then(|overrides| overrides.remove("approval_policy")) + { + let approval_policy = match serde_json::from_value(value) { + Ok(approval_policy) => approval_policy, + Err(err) => { + self.outgoing + .send_error( + request_id, + invalid_params(format!( + "invalid `approval_policy` config override: {err}" + )), + ) + .await; + return Ok(()); + } + }; + typesafe_overrides.approval_policy = Some(approval_policy); + } let has_explicit_model_resume_override = has_model_resume_override(request_overrides.as_ref(), &typesafe_overrides); let persisted_metadata = self @@ -3470,6 +3527,10 @@ impl ThreadRequestProcessor { request_overrides.as_ref(), typesafe_overrides, ); + if typesafe_overrides.approval_policy.is_none() { + typesafe_overrides.approval_policy = + latest_persisted_approval_policy(&resumed_history.history); + } let state_db_ctx = self.state_db.clone()?; let persisted_metadata = state_db_ctx .get_thread(resumed_history.conversation_id) diff --git a/codex-rs/app-server/src/request_processors/thread_processor_tests.rs b/codex-rs/app-server/src/request_processors/thread_processor_tests.rs index cf491c8f74..33cbd160f4 100644 --- a/codex-rs/app-server/src/request_processors/thread_processor_tests.rs +++ b/codex-rs/app-server/src/request_processors/thread_processor_tests.rs @@ -36,6 +36,136 @@ mod thread_list_cwd_filter_tests { } } +mod persisted_resume_approval_policy_tests { + use super::super::latest_persisted_approval_policy; + 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::RolloutItem; + use codex_protocol::protocol::SandboxPolicy; + use codex_protocol::protocol::ThreadSettingsAppliedEvent; + use codex_protocol::protocol::ThreadSettingsSnapshot; + use codex_protocol::protocol::TurnContextItem; + use codex_protocol::protocol::TurnStartedEvent; + 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(approval_policy: AskForApproval) -> 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, + approvals_reviewer: ApprovalsReviewer::User, + 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_started_item(turn_id: &str) -> RolloutItem { + RolloutItem::EventMsg(EventMsg::TurnStarted(TurnStartedEvent { + turn_id: turn_id.to_string(), + trace_id: None, + started_at: None, + model_context_window: None, + collaboration_mode_kind: ModeKind::Default, + })) + } + + fn turn_context_item(turn_id: &str, approval_policy: AskForApproval) -> RolloutItem { + RolloutItem::TurnContext(TurnContextItem { + turn_id: Some(turn_id.to_string()), + cwd: cwd(), + workspace_roots: None, + current_date: None, + timezone: None, + approval_policy, + approvals_reviewer: None, + sandbox_policy: SandboxPolicy::new_read_only_policy(), + 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(AskForApproval::Never), + settings_item(AskForApproval::OnRequest), + ]; + + assert_eq!( + latest_persisted_approval_policy(&history), + Some(AskForApproval::OnRequest) + ); + } + + #[test] + fn settings_applied_during_turn_wins_over_stale_compaction_context() { + let history = vec![ + turn_started_item("turn-1"), + settings_item(AskForApproval::Never), + turn_context_item("turn-1", AskForApproval::OnRequest), + ]; + + assert_eq!( + latest_persisted_approval_policy(&history), + Some(AskForApproval::Never) + ); + } + + #[test] + fn later_turn_context_wins_over_earlier_settings_update() { + let history = vec![ + turn_started_item("turn-1"), + settings_item(AskForApproval::Never), + turn_context_item("turn-1", AskForApproval::OnRequest), + turn_started_item("turn-2"), + turn_context_item("turn-2", AskForApproval::OnRequest), + ]; + + assert_eq!( + latest_persisted_approval_policy(&history), + Some(AskForApproval::OnRequest) + ); + } +} + mod background_terminal_pagination_tests { use super::super::paginate_background_terminals; use codex_app_server_protocol::ThreadBackgroundTerminal; diff --git a/codex-rs/app-server/tests/suite/v2/thread_resume.rs b/codex-rs/app-server/tests/suite/v2/thread_resume.rs index c6e70c86a4..e6a410fde0 100644 --- a/codex-rs/app-server/tests/suite/v2/thread_resume.rs +++ b/codex-rs/app-server/tests/suite/v2/thread_resume.rs @@ -716,6 +716,134 @@ async fn thread_resume_preserves_persisted_approvals_reviewer() -> Result<()> { Ok(()) } +#[tokio::test] +async fn thread_resume_restores_persisted_approval_policy() -> Result<()> { + assert_thread_resume_approval_policy( + ThreadHistoryMode::Legacy, + /*approval_policy*/ None, + AskForApproval::Never, + ) + .await +} + +#[tokio::test] +async fn paginated_thread_resume_restores_persisted_approval_policy() -> Result<()> { + assert_thread_resume_approval_policy( + ThreadHistoryMode::Paginated, + /*approval_policy*/ None, + AskForApproval::Never, + ) + .await +} + +#[tokio::test] +async fn thread_resume_approval_policy_override_wins_over_persisted_policy() -> Result<()> { + assert_thread_resume_approval_policy( + ThreadHistoryMode::Legacy, + Some(AskForApproval::OnRequest), + AskForApproval::OnRequest, + ) + .await +} + +async fn assert_thread_resume_approval_policy( + history_mode: ThreadHistoryMode, + approval_policy: Option, + expected_approval_policy: AskForApproval, +) -> Result<()> { + let server = create_mock_responses_server_repeating_assistant("Done").await; + let codex_home = TempDir::new()?; + let config_path = codex_home.path().join("config.toml"); + std::fs::write( + &config_path, + format!( + r#" +model = "gpt-5.4" +approval_policy = "never" +model_provider = "mock_provider" + +[model_providers.mock_provider] +name = "Mock provider for test" +base_url = "{}/v1" +wire_api = "responses" +request_max_retries = 0 +stream_max_retries = 0 +"#, + server.uri() + ), + )?; + + let thread_id = { + let mut mcp = TestAppServer::builder() + .with_codex_home(codex_home.path()) + .without_auto_env() + .build_initialized() + .await?; + + let start_id = mcp + .send_thread_start_request(ThreadStartParams { + model: Some("gpt-5.4".to_string()), + history_mode: Some(history_mode), + ..Default::default() + }) + .await?; + let ThreadStartResponse { thread, .. } = + timeout(DEFAULT_READ_TIMEOUT, mcp.read_response(start_id)).await??; + + let turn_id = mcp + .send_turn_start_request(TurnStartParams { + thread_id: thread.id.clone(), + client_user_message_id: None, + input: vec![UserInput::Text { + text: "materialize this thread".to_string(), + text_elements: Vec::new(), + }], + ..Default::default() + }) + .await?; + timeout( + DEFAULT_READ_TIMEOUT, + mcp.read_stream_until_response_message(RequestId::Integer(turn_id)), + ) + .await??; + timeout( + DEFAULT_READ_TIMEOUT, + mcp.read_stream_until_notification_message("turn/completed"), + ) + .await??; + + thread.id + }; + + let config = std::fs::read_to_string(&config_path)?; + std::fs::write( + config_path, + config.replace( + "approval_policy = \"never\"", + "approval_policy = \"on-request\"", + ), + )?; + + let mut mcp = TestAppServer::builder() + .with_codex_home(codex_home.path()) + .without_auto_env() + .build_initialized() + .await?; + let resume_id = mcp + .send_thread_resume_request(ThreadResumeParams { + thread_id, + approval_policy, + ..Default::default() + }) + .await?; + let ThreadResumeResponse { + approval_policy, .. + } = timeout(DEFAULT_READ_TIMEOUT, mcp.read_response(resume_id)).await??; + + assert_eq!(approval_policy, expected_approval_policy); + Ok(()) +} + #[tokio::test] async fn thread_resume_preserves_goal_first_and_fork_approvals_reviewer() -> Result<()> { let server = create_mock_responses_server_repeating_assistant("Done").await;