From d1fb77d69274e81ece393728faee9e9bc44e70e3 Mon Sep 17 00:00:00 2001 From: jif Date: Tue, 4 Aug 2026 18:01:49 +0000 Subject: [PATCH] Use current session settings for review threads (#36941) ## Why Reviews may use a different model from their parent turn. They need current thread settings without inheriting defaults resolved for the parent model. ## What changed - Build review configuration from the parent turn context while preserving explicitly configured token-budget overrides. - Use the review model's token-budget defaults and select a supported reasoning effort when switching models. - Carry the session's resolved service tier and current environment, permissions, and approval settings into the review thread. ## Testing Expanded review coverage for updated thread settings, model-specific token-budget defaults, service tiers, and reasoning effort selection. GitOrigin-RevId: cefa0f22cd9b9e28047d94e1d2d49ff83eded43e --- codex-rs/core/src/session/mod.rs | 3 +- codex-rs/core/src/session/review.rs | 31 +++-- codex-rs/core/tests/suite/review.rs | 174 ++++++++++++++++++++++++---- 3 files changed, 177 insertions(+), 31 deletions(-) diff --git a/codex-rs/core/src/session/mod.rs b/codex-rs/core/src/session/mod.rs index def4a5f5b5..ad31042d3a 100644 --- a/codex-rs/core/src/session/mod.rs +++ b/codex-rs/core/src/session/mod.rs @@ -636,6 +636,7 @@ impl Session { let model_info = models_manager .get_model_info(model.as_str(), &config.to_models_manager_config()) .await; + let configured_config = Arc::clone(&config); if config.config_lock_export_dir.is_some() && config.config_lock_save_fields_resolved_from_model_catalog { @@ -767,7 +768,7 @@ impl Session { // This task will run until Op::Shutdown is received. let session_for_loop = Arc::clone(&session); let session_loop_handle = tokio::spawn(async move { - submission_loop(session_for_loop, config, rx_sub) + submission_loop(session_for_loop, configured_config, rx_sub) .instrument(info_span!("session_loop", thread_id = %thread_id)) .await; }); diff --git a/codex-rs/core/src/session/review.rs b/codex-rs/core/src/session/review.rs index afea0906c0..984b8e98bd 100644 --- a/codex-rs/core/src/session/review.rs +++ b/codex-rs/core/src/session/review.rs @@ -45,14 +45,25 @@ pub(super) async fn spawn_review_thread( let model_info = review_model_info.clone(); // Build per‑turn client with the requested model/family. - let mut per_turn_config = (*config).clone(); + let mut per_turn_config = (*parent_turn_context.config).clone(); + // Preserve configured overrides without carrying over the parent model's defaults. + per_turn_config.token_budget = config.token_budget.clone(); per_turn_config.model = Some(model.clone()); per_turn_config.features = review_features.clone(); - per_turn_config.permissions = parent_turn_context.config.permissions.clone(); - per_turn_config.approvals_reviewer = parent_turn_context.config.approvals_reviewer; - per_turn_config.codex_linux_sandbox_exe = - parent_turn_context.config.codex_linux_sandbox_exe.clone(); - per_turn_config.compact_prompt = parent_turn_context.config.compact_prompt.clone(); + if let Some(current_effort) = per_turn_config.model_reasoning_effort.as_ref() + && review_model_info.slug != parent_turn_context.model_info.slug + && !review_model_info.used_fallback_model_metadata + && !review_model_info + .supported_reasoning_levels + .iter() + .any(|preset| &preset.effort == current_effort) + { + let supported_reasoning_levels = &review_model_info.supported_reasoning_levels; + per_turn_config.model_reasoning_effort = supported_reasoning_levels + .get(supported_reasoning_levels.len().saturating_sub(1) / 2) + .map(|preset| preset.effort.clone()) + .or_else(|| review_model_info.default_reasoning_level.clone()); + } if let Err(err) = per_turn_config.web_search_mode.set(review_web_search_mode) { let fallback_value = per_turn_config.web_search_mode.value(); tracing::warn!( @@ -75,13 +86,19 @@ pub(super) async fn spawn_review_thread( .model_reasoning_summary .unwrap_or(model_info.default_reasoning_summary); let session_source = parent_turn_context.session_source.clone(); - let (forked_from_thread_id, thread_source) = { + let (forked_from_thread_id, thread_source, service_tier) = { let state = sess.state.lock().await; ( state.session_configuration.forked_from_thread_id, state.session_configuration.thread_source.clone(), + state + .session_configuration + .service_tier + .clone() + .or_else(|| config.service_tier.clone()), ) }; + per_turn_config.service_tier = service_tier; let per_turn_config = Arc::new(per_turn_config); let review_turn_id = sub_id.to_string(); diff --git a/codex-rs/core/tests/suite/review.rs b/codex-rs/core/tests/suite/review.rs index 3c52102659..7264bea8da 100644 --- a/codex-rs/core/tests/suite/review.rs +++ b/codex-rs/core/tests/suite/review.rs @@ -3,11 +3,18 @@ use codex_core::REVIEW_PROMPT; use codex_core::config::Config; use codex_core::config::Constrained; use codex_core::find_thread_path_by_id_str; +use codex_exec_server::CreateDirectoryOptions; +use codex_features::Feature; use codex_protocol::config_types::ApprovalsReviewer; +use codex_protocol::config_types::ServiceTier; use codex_protocol::items::TurnItem; use codex_protocol::models::ContentItem; use codex_protocol::models::PermissionProfile; use codex_protocol::models::ResponseItem; +use codex_protocol::openai_models::ModelServiceTier; +use codex_protocol::openai_models::ModelTokenBudgetConfig; +use codex_protocol::openai_models::ReasoningEffort; +use codex_protocol::openai_models::ReasoningEffortPreset; use codex_protocol::protocol::AskForApproval; use codex_protocol::protocol::ENVIRONMENT_CONTEXT_OPEN_TAG; use codex_protocol::protocol::EventMsg; @@ -22,6 +29,7 @@ use codex_protocol::protocol::ReviewTarget; use codex_protocol::protocol::RolloutItem; use codex_protocol::protocol::RolloutLine; use codex_protocol::protocol::ThreadSettingsOverrides; +use codex_protocol::protocol::TurnEnvironmentSelections; use codex_protocol::review_format::render_review_output_text; use codex_protocol::user_input::UserInput; use core_test_support::PathBufExt; @@ -555,30 +563,106 @@ async fn review_does_not_emit_agent_message_on_structured_output() { server.verify().await; } -/// Review requests must inherit permissions and reviewer settings updated after session startup. +/// Reviews inherit current session settings without inheriting another model's defaults. #[tokio::test(flavor = "multi_thread", worker_threads = 2)] async fn review_uses_updated_turn_permissions_and_approval_policy() { skip_if_no_network!(); + fn model_defaults(guidance_message: &str) -> ModelTokenBudgetConfig { + ModelTokenBudgetConfig { + reminder_threshold_tokens: 6_144, + reminder_message_template: "Reminder: {n_remaining} tokens remain.".to_string(), + guidance_message: guidance_message.to_string(), + auto_compact_fallback_prompt: "Preserve the important context.".to_string(), + auto_compact_fallback_buffer_tokens: 16_384, + } + } + let (server, request_log) = start_responses_server_with_sse(completed_sse(), /*expected_requests*/ 1).await; let codex_home = Arc::new(TempDir::new().unwrap()); - let codex = new_conversation_for_server(&server, codex_home.clone(), |config| { - config.approvals_reviewer = ApprovalsReviewer::AutoReview; - config.permissions.approval_policy = Constrained::allow_any(AskForApproval::OnRequest); - config - .permissions - .set_permission_profile(PermissionProfile::read_only()) - .expect("initial permission profile should be valid"); - }) - .await; + let test = test_codex() + .with_home(codex_home.clone()) + .with_model_info_override("gpt-5.2", |model_info| { + model_info.service_tiers.clear(); + model_info + .model_messages + .as_mut() + .expect("parent model should have model messages") + .token_budget = Some(model_defaults("PARENT MODEL ONLY")); + }) + .with_model_info_override("gpt-5.4", |model_info| { + model_info.service_tiers = vec![ModelServiceTier { + id: ServiceTier::Fast.request_value().to_string(), + name: "Fast".to_string(), + description: "Priority processing".to_string(), + }]; + model_info.supported_reasoning_levels = [ + ReasoningEffort::Low, + ReasoningEffort::Medium, + ReasoningEffort::High, + ] + .into_iter() + .map(|effort| ReasoningEffortPreset { + description: effort.to_string(), + effort, + }) + .collect(); + model_info.default_reasoning_level = Some(ReasoningEffort::High); + model_info + .model_messages + .as_mut() + .expect("review model should have model messages") + .token_budget = Some(model_defaults("REVIEW MODEL ONLY")); + }) + .with_model("gpt-5.2") + .with_config(|config| { + config.review_model = Some("gpt-5.4".to_string()); + config.model_context_window = Some(128_000); + config.config_lock_export_dir = Some(config.codex_home.join("review-config-locks")); + config.service_tier = Some(ServiceTier::Fast.request_value().to_string()); + config + .features + .enable(Feature::TokenBudget) + .expect("token budget should be available"); + config.approvals_reviewer = ApprovalsReviewer::AutoReview; + config.permissions.approval_policy = Constrained::allow_any(AskForApproval::OnRequest); + config + .permissions + .set_permission_profile(PermissionProfile::read_only()) + .expect("initial permission profile should be valid"); + }) + .build_with_auto_env(&server) + .await + .expect("review conversation should be created"); + let codex = Arc::clone(&test.codex); + let updated_cwd = test.config.cwd.join("updated-review-workspace"); + let mut selection = test.executor_environment().selection().clone(); + selection.cwd = selection + .cwd + .join("updated-review-workspace") + .expect("updated execution directory should be valid"); + selection.workspace_roots = vec![selection.cwd.clone()]; + test.fs() + .create_directory( + &selection.cwd, + CreateDirectoryOptions { recursive: true }, + /*sandbox*/ None, + ) + .await + .expect("updated review workspace should be created"); core_test_support::submit_thread_settings( &codex, ThreadSettingsOverrides { + environments: Some(TurnEnvironmentSelections::new( + updated_cwd.clone(), + vec![selection], + )), approval_policy: Some(AskForApproval::Never), approvals_reviewer: Some(ApprovalsReviewer::User), permission_profile: Some(PermissionProfile::Disabled), + effort: Some(Some(ReasoningEffort::XHigh)), ..Default::default() }, ) @@ -599,6 +683,11 @@ async fn review_uses_updated_turn_permissions_and_approval_policy() { wait_for_event(&codex, |event| matches!(event, EventMsg::TurnComplete(_))).await; let request = request_log.single_request(); + assert_eq!(request.body_json()["reasoning"]["effort"], "medium"); + assert_eq!( + request.body_json()["service_tier"], + ServiceTier::Fast.request_value() + ); assert!( request .message_input_texts("developer") @@ -606,6 +695,20 @@ async fn review_uses_updated_turn_permissions_and_approval_policy() { .any(|text| text.contains("Approval policy is currently never")), "review should use the updated approval policy" ); + assert!( + request + .message_input_texts("developer") + .iter() + .any(|text| text.contains("REVIEW MODEL ONLY")), + "review should use its own model's token-budget defaults" + ); + assert!( + !request + .message_input_texts("developer") + .iter() + .any(|text| text.contains("PARENT MODEL ONLY")), + "review should not inherit the parent model's token-budget defaults" + ); assert!( request .message_input_texts("user") @@ -627,6 +730,18 @@ async fn review_uses_updated_turn_permissions_and_approval_policy() { .expect("review thread should have a rollout"); let review_rollout = std::fs::read_to_string(review_rollout_path).expect("review rollout should be readable"); + let review_session_cwd = review_rollout + .lines() + .find_map(|line| { + let rollout_line: RolloutLine = + serde_json::from_str(line).expect("review rollout line should be valid"); + match rollout_line.item { + RolloutItem::SessionMeta(session_meta) => Some(session_meta.meta.cwd), + _ => None, + } + }) + .expect("review rollout should contain session metadata"); + assert_eq!(review_session_cwd, updated_cwd.as_path()); let review_approvals_reviewer = review_rollout .lines() .filter_map(|line| { @@ -653,12 +768,17 @@ async fn review_uses_custom_review_model_from_config() { let (server, request_log) = start_responses_server_with_sse(completed_sse(), /*expected_requests*/ 1).await; let codex_home = Arc::new(TempDir::new().unwrap()); - // Choose a review model different from the main model; ensure it is used. - let codex = new_conversation_for_server(&server, codex_home.clone(), |cfg| { - cfg.model = Some("gpt-4.1".to_string()); - cfg.review_model = Some("gpt-5.4".to_string()); - }) - .await; + let test = test_codex() + .with_home(Arc::clone(&codex_home)) + .with_config(|config| { + config.model = Some("gpt-4.1".to_string()); + config.review_model = Some("custom-review-model".to_string()); + config.model_reasoning_effort = Some(ReasoningEffort::Max); + }) + .build_with_auto_env(&server) + .await + .expect("custom review conversation should be created"); + let codex = Arc::clone(&test.codex); codex .submit(Op::Review { @@ -690,7 +810,8 @@ async fn review_uses_custom_review_model_from_config() { let request = request_log.single_request(); assert_eq!(request.path(), "/v1/responses"); let body = request.body_json(); - assert_eq!(body["model"].as_str().unwrap(), "gpt-5.4"); + assert_eq!(body["model"].as_str().unwrap(), "custom-review-model"); + assert_eq!(body["reasoning"]["effort"].as_str(), Some("max")); let _codex_home_guard = codex_home; server.verify().await; @@ -705,11 +826,17 @@ async fn review_uses_session_model_when_review_model_unset() { let (server, request_log) = start_responses_server_with_sse(completed_sse(), /*expected_requests*/ 1).await; let codex_home = Arc::new(TempDir::new().unwrap()); - let codex = new_conversation_for_server(&server, codex_home.clone(), |cfg| { - cfg.model = Some("gpt-4.1".to_string()); - cfg.review_model = None; - }) - .await; + let test = test_codex() + .with_home(Arc::clone(&codex_home)) + .with_config(|config| { + config.model = Some("gpt-5.4".to_string()); + config.review_model = None; + config.model_reasoning_effort = Some(ReasoningEffort::Max); + }) + .build_with_auto_env(&server) + .await + .expect("same-model review conversation should be created"); + let codex = Arc::clone(&test.codex); codex .submit(Op::Review { @@ -739,7 +866,8 @@ async fn review_uses_session_model_when_review_model_unset() { let request = request_log.single_request(); assert_eq!(request.path(), "/v1/responses"); let body = request.body_json(); - assert_eq!(body["model"].as_str().unwrap(), "gpt-4.1"); + assert_eq!(body["model"].as_str().unwrap(), "gpt-5.4"); + assert_eq!(body["reasoning"]["effort"].as_str(), Some("max")); let _codex_home_guard = codex_home; server.verify().await;