diff --git a/codex-rs/core/src/guardian/review.rs b/codex-rs/core/src/guardian/review.rs index 502773f157..d9966e38f4 100644 --- a/codex-rs/core/src/guardian/review.rs +++ b/codex-rs/core/src/guardian/review.rs @@ -659,14 +659,13 @@ pub(super) async fn run_guardian_review_session( fallback } }; - let use_parent_model_for_auto_review = turn.model_info.auto_review_model_override == Some(true); - let preferred_model_id = turn.provider.approval_review_preferred_model(); - let preferred_model = available_models + let model_override = turn.model_info.auto_review_model_override.as_deref(); + let review_model_id = + model_override.unwrap_or_else(|| turn.provider.approval_review_preferred_model()); + let review_model = available_models .iter() - .find(|preset| preset.model == preferred_model_id); - let (guardian_model, guardian_reasoning_effort) = if !use_parent_model_for_auto_review - && let Some(preset) = preferred_model - { + .find(|preset| preset.model == review_model_id); + let (guardian_model, guardian_reasoning_effort) = if let Some(preset) = review_model { let reasoning_effort = preferred_reasoning_effort( preset .supported_reasoning_efforts @@ -674,7 +673,7 @@ pub(super) async fn run_guardian_review_session( .any(|effort| effort.effort == codex_protocol::openai_models::ReasoningEffort::Low), Some(preset.default_reasoning_effort), ); - (preferred_model_id.to_string(), reasoning_effort) + (review_model_id.to_string(), reasoning_effort) } else { let reasoning_effort = preferred_reasoning_effort( turn.model_info @@ -684,7 +683,12 @@ pub(super) async fn run_guardian_review_session( turn.reasoning_effort .or(turn.model_info.default_reasoning_level), ); - (turn.model_info.slug.clone(), reasoning_effort) + ( + model_override + .unwrap_or(turn.model_info.slug.as_str()) + .to_string(), + reasoning_effort, + ) }; let guardian_config = build_guardian_review_session_config( turn.config.as_ref(), diff --git a/codex-rs/core/src/guardian/tests.rs b/codex-rs/core/src/guardian/tests.rs index 1fdace5ddc..aefc7632b9 100644 --- a/codex-rs/core/src/guardian/tests.rs +++ b/codex-rs/core/src/guardian/tests.rs @@ -1226,7 +1226,7 @@ fn guardian_output_schema_requires_only_outcome_and_allows_optional_details() { } async fn guardian_request_model_for_auto_review_override( - auto_review_model_override: Option, + auto_review_model_override: Option, ) -> anyhow::Result<(String, String, String)> { let server = start_mock_server().await; let guardian_assessment = serde_json::json!({ @@ -1284,31 +1284,32 @@ async fn guardian_request_model_for_auto_review_override( } #[tokio::test(flavor = "multi_thread", worker_threads = 2)] -async fn guardian_review_uses_parent_model_when_model_catalog_override_is_true() +async fn guardian_review_uses_model_catalog_override_when_preferred_review_model_exists() -> anyhow::Result<()> { skip_if_no_network!(Ok(())); + let override_model = "guardian-review-model-override".to_string(); let (request_model, parent_model, preferred_model) = - guardian_request_model_for_auto_review_override(Some(true)).await?; + guardian_request_model_for_auto_review_override(Some(override_model.clone())).await?; - assert_eq!(request_model, parent_model); + assert_eq!(request_model, override_model); + assert_ne!(request_model, parent_model); assert_ne!(request_model, preferred_model); Ok(()) } #[tokio::test(flavor = "multi_thread", worker_threads = 2)] -async fn guardian_review_uses_preferred_review_model_without_parent_override() -> anyhow::Result<()> -{ +async fn guardian_review_uses_preferred_review_model_without_model_catalog_override() +-> anyhow::Result<()> { skip_if_no_network!(Ok(())); - for auto_review_model_override in [None, Some(false)] { - let (request_model, parent_model, preferred_model) = - guardian_request_model_for_auto_review_override(auto_review_model_override).await?; + let (request_model, parent_model, preferred_model) = + guardian_request_model_for_auto_review_override(/*auto_review_model_override*/ None) + .await?; - assert_eq!(request_model, preferred_model); - assert_ne!(request_model, parent_model); - } + assert_eq!(request_model, preferred_model); + assert_ne!(request_model, parent_model); Ok(()) } diff --git a/codex-rs/core/tests/suite/auto_review.rs b/codex-rs/core/tests/suite/auto_review.rs index 45e525e33c..d27526dda5 100644 --- a/codex-rs/core/tests/suite/auto_review.rs +++ b/codex-rs/core/tests/suite/auto_review.rs @@ -40,16 +40,17 @@ use serde_json::json; use wiremock::MockServer; #[tokio::test(flavor = "multi_thread", worker_threads = 2)] -async fn remote_model_override_uses_parent_model_for_strict_auto_review() -> Result<()> { +async fn remote_model_override_uses_catalog_model_for_strict_auto_review() -> Result<()> { skip_if_no_network!(Ok(())); skip_if_sandbox!(Ok(())); let server = MockServer::start().await; let model = "remote-auto-review-parent"; + let review_model = "remote-auto-review-reviewer"; mount_models_once( &server, ModelsResponse { - models: vec![remote_model_with_auto_review_override(model)], + models: vec![remote_model_with_auto_review_override(model, review_model)], }, ) .await; @@ -129,7 +130,10 @@ async fn remote_model_override_uses_parent_model_for_strict_auto_review() -> Res let model_info = models_manager .get_model_info(model, &config.to_models_manager_config()) .await; - assert_eq!(model_info.auto_review_model_override, Some(true)); + assert_eq!( + model_info.auto_review_model_override, + Some(review_model.to_string()) + ); core_test_support::submit_thread_settings( &codex, @@ -191,12 +195,15 @@ async fn remote_model_override_uses_parent_model_for_strict_auto_review() -> Res .into_iter() .find(|request| request.body_contains_text("auto-review-model-override.txt")) .expect("expected Guardian request for strict apply_patch"); - assert_eq!(guardian_request.body_json()["model"].as_str(), Some(model)); + assert_eq!( + guardian_request.body_json()["model"].as_str(), + Some(review_model) + ); Ok(()) } -fn remote_model_with_auto_review_override(slug: &str) -> ModelInfo { +fn remote_model_with_auto_review_override(slug: &str, review_model: &str) -> ModelInfo { ModelInfo { slug: slug.to_string(), display_name: format!("{slug} display"), @@ -212,7 +219,7 @@ fn remote_model_with_auto_review_override(slug: &str) -> ModelInfo { input_modalities: default_input_modalities(), used_fallback_model_metadata: false, supports_search_tool: false, - auto_review_model_override: Some(true), + auto_review_model_override: Some(review_model.to_string()), priority: 1, additional_speed_tiers: Vec::new(), service_tiers: Vec::new(), diff --git a/codex-rs/protocol/src/openai_models.rs b/codex-rs/protocol/src/openai_models.rs index fe727db138..83db2eb1f3 100644 --- a/codex-rs/protocol/src/openai_models.rs +++ b/codex-rs/protocol/src/openai_models.rs @@ -313,7 +313,7 @@ pub struct ModelInfo { #[serde(default)] pub supports_search_tool: bool, #[serde(default, skip_serializing_if = "Option::is_none")] - pub auto_review_model_override: Option, + pub auto_review_model_override: Option, } impl ModelInfo {