From 508a006d7aaa485ac0367c9e45c69ebb948af518 Mon Sep 17 00:00:00 2001 From: felixxia-oai Date: Tue, 15 Sep 2026 12:06:39 +0000 Subject: [PATCH] Pass `ReviewModel` through guardian review sessions (#45684) ## What changed Replace duplicated model-selection fields in `GuardianReviewSessionConfig` and `GuardianReviewSessionParams` with the existing `ReviewModel` struct. Read model settings and selection metadata from that struct for session execution, analytics, and failed-review records, and update the existing test fixtures accordingly. GitOrigin-RevId: 1da2001e68168ec40a3d726694eb5fd0445aa3d7 --- codex-rs/core/src/guardian/feedback.rs | 2 +- codex-rs/core/src/guardian/review.rs | 41 +++++-------------- codex-rs/core/src/guardian/review_session.rs | 27 ++++++------ .../core/src/guardian/review_session_tests.rs | 18 ++++---- 4 files changed, 33 insertions(+), 55 deletions(-) diff --git a/codex-rs/core/src/guardian/feedback.rs b/codex-rs/core/src/guardian/feedback.rs index b227af8a5f..efff7dc840 100644 --- a/codex-rs/core/src/guardian/feedback.rs +++ b/codex-rs/core/src/guardian/feedback.rs @@ -35,7 +35,7 @@ pub(super) async fn record_failed_review( ), target_item_id: guardian_request_target_item_id(¶ms.request), reviewer_thread_id: reviewer.thread_id(), - model: ¶ms.model, + model: ¶ms.review_model.model, action: &action, action_truncated: false, instructions: Some(&instructions.text), diff --git a/codex-rs/core/src/guardian/review.rs b/codex-rs/core/src/guardian/review.rs index e8e7cae725..2f73c631ec 100644 --- a/codex-rs/core/src/guardian/review.rs +++ b/codex-rs/core/src/guardian/review.rs @@ -13,6 +13,7 @@ use codex_guardian_reviewer::GuardianReviewError; use codex_guardian_reviewer::GuardianReviewOutcome; #[cfg(test)] use codex_guardian_reviewer::GuardianReviewSessionLimits; +use codex_guardian_reviewer::ReviewModel; use codex_protocol::config_types::ApprovalsReviewer; use codex_protocol::protocol::AskForApproval; use codex_protocol::protocol::EventMsg; @@ -144,12 +145,7 @@ pub(super) struct GuardianReviewSessionConfig { pub(super) spawn_config: crate::config::Config, pub(super) node_repl_policy: GuardianNodeReplPolicy, pub(super) compaction_model_hash: Option, - model: String, - reasoning_effort: Option, - default_review_model_id: String, - catalog_contains_auto_review: bool, - model_overridden: bool, - model_override: Option, + review_model: ReviewModel, } pub(super) async fn guardian_review_session_config( @@ -171,14 +167,7 @@ pub(super) async fn guardian_review_session_config( ) .await; let default_review_model_id = turn.provider.approval_review_preferred_model(); - let codex_guardian_reviewer::ReviewModel { - model: guardian_model, - reasoning_effort: guardian_reasoning_effort, - default_review_model_id, - catalog_contains_auto_review: guardian_catalog_contains_auto_review, - model_overridden: guardian_review_model_overridden, - model_override: guardian_review_model_override, - } = codex_guardian_reviewer::select_review_model( + let review_model = codex_guardian_reviewer::select_review_model( &context.model_info, context.reasoning_effort.as_ref(), default_review_model_id, @@ -188,7 +177,7 @@ pub(super) async fn guardian_review_session_config( // Resolve a separate reviewer against the current catalog on every attempt. // Parent fallback must retain the action's metadata even after a catalog refresh. let guardian_model_info = - if !guardian_catalog_contains_auto_review && !guardian_review_model_overridden { + if !review_model.catalog_contains_auto_review && !review_model.model_overridden { Arc::clone(&context.model_info) } else { Arc::new( @@ -196,7 +185,7 @@ pub(super) async fn guardian_review_session_config( .services .models_manager .get_model_info( - guardian_model.as_str(), + review_model.model.as_str(), &turn.config.to_models_manager_config(), ) .await, @@ -205,8 +194,8 @@ pub(super) async fn guardian_review_session_config( let mut spawn_config = build_guardian_review_session_config( turn.config.as_ref(), live_network_config, - guardian_model.as_str(), - guardian_reasoning_effort.clone(), + review_model.model.as_str(), + review_model.reasoning_effort.clone(), context.reasoning_summary, context.personality, guardian_model_info.model_messages.as_ref(), @@ -221,7 +210,7 @@ pub(super) async fn guardian_review_session_config( ) })?; } - if guardian_model != context.model_info.slug { + if review_model.model != context.model_info.slug { spawn_config.model_context_window = None; spawn_config.model_auto_compact_token_limit = None; } @@ -231,12 +220,7 @@ pub(super) async fn guardian_review_session_config( node_repl_policy: GuardianNodeReplPolicy::from_model_messages( guardian_model_info.model_messages.as_ref(), ), - model: guardian_model, - reasoning_effort: guardian_reasoning_effort, - default_review_model_id, - catalog_contains_auto_review: guardian_catalog_contains_auto_review, - model_overridden: guardian_review_model_overridden, - model_override: guardian_review_model_override, + review_model, }) } @@ -292,13 +276,8 @@ async fn run_guardian_review_session_before_deadline( request, reasons, schema, - model: session_config.model, + review_model: session_config.review_model, compaction_model_hash: session_config.compaction_model_hash, - reasoning_effort: session_config.reasoning_effort, - guardian_default_review_model_id: session_config.default_review_model_id, - guardian_catalog_contains_auto_review: session_config.catalog_contains_auto_review, - guardian_review_model_overridden: session_config.model_overridden, - guardian_review_model_override: session_config.model_override, reasoning_summary: context.reasoning_summary, personality: context.personality, external_cancel, diff --git a/codex-rs/core/src/guardian/review_session.rs b/codex-rs/core/src/guardian/review_session.rs index 6c0ef314ff..8c1a9146ed 100644 --- a/codex-rs/core/src/guardian/review_session.rs +++ b/codex-rs/core/src/guardian/review_session.rs @@ -25,6 +25,7 @@ use codex_analytics::GuardianReviewSessionKind; use codex_extension_api::Instructions; use codex_guardian_reviewer::ConversationCheckpoint; use codex_guardian_reviewer::ConversationState; +use codex_guardian_reviewer::ReviewModel; use codex_history::InitialHistory; use codex_history::RolloutItem; use codex_protocol::ThreadId; @@ -112,13 +113,8 @@ pub(crate) struct GuardianReviewSessionParams { pub(crate) request: GuardianApprovalRequest, pub(crate) reasons: ApprovalRequestReasons, pub(crate) schema: Value, - pub(crate) model: String, + pub(crate) review_model: ReviewModel, pub(crate) compaction_model_hash: Option, - pub(crate) reasoning_effort: Option, - pub(crate) guardian_default_review_model_id: String, - pub(crate) guardian_catalog_contains_auto_review: bool, - pub(crate) guardian_review_model_overridden: bool, - pub(crate) guardian_review_model_override: Option, pub(crate) reasoning_summary: ReasoningSummaryConfig, pub(crate) personality: Option, pub(crate) external_cancel: Option, @@ -324,16 +320,17 @@ async fn run_review_on_session( bool, GuardianReviewAnalyticsResult, ) { + let review_model = ¶ms.review_model; let model_info = params .parent_session .services .models_manager .get_model_info( - params.model.as_str(), + review_model.model.as_str(), ¶ms.spawn_config.to_models_manager_config(), ) .await; - let guardian_reasoning_effort = params + let guardian_reasoning_effort = review_model .reasoning_effort .clone() .or_else(|| model_info.default_reasoning_level.clone()); @@ -348,12 +345,12 @@ async fn run_review_on_session( GuardianReviewAnalyticsResult::from_session(GuardianReviewSessionAnalyticsParams { guardian_thread_id: review_session.session.thread_id().to_string(), guardian_session_kind, - guardian_model: params.model.clone(), + guardian_model: review_model.model.clone(), guardian_reasoning_effort: guardian_reasoning_effort.map(|effort| effort.to_string()), - guardian_default_review_model_id: params.guardian_default_review_model_id.clone(), - guardian_catalog_contains_auto_review: params.guardian_catalog_contains_auto_review, - guardian_review_model_overridden: params.guardian_review_model_overridden, - guardian_review_model_override: params.guardian_review_model_override.clone(), + guardian_default_review_model_id: review_model.default_review_model_id.clone(), + guardian_catalog_contains_auto_review: review_model.catalog_contains_auto_review, + guardian_review_model_overridden: review_model.model_overridden, + guardian_review_model_override: review_model.model_override.clone(), guardian_model_provider_id: params.spawn_config.model_provider_id.clone(), had_prior_review_context: had_prior_context, }); @@ -640,8 +637,8 @@ async fn run_review_on_session( permission_profile: params.spawn_config.permissions.permission_profile().clone(), reasoning_summary: params.reasoning_summary, personality: params.personality, - model: params.model.clone(), - reasoning_effort: params.reasoning_effort.clone(), + model: review_model.model.clone(), + reasoning_effort: review_model.reasoning_effort.clone(), parent_response_id: params.parent_context.parent_response_id.clone(), schema: params.schema.clone(), parent_turn_id: parent_turn.sub_id.clone(), diff --git a/codex-rs/core/src/guardian/review_session_tests.rs b/codex-rs/core/src/guardian/review_session_tests.rs index 34ff8fd968..d69e37918f 100644 --- a/codex-rs/core/src/guardian/review_session_tests.rs +++ b/codex-rs/core/src/guardian/review_session_tests.rs @@ -35,8 +35,8 @@ async fn run_review_preserves_evidence_during_parent_compaction() { params.spawn_config = build_guardian_review_session_config( turn.config.as_ref(), /*live_network_config*/ None, - ¶ms.model, - params.reasoning_effort.clone(), + ¶ms.review_model.model, + params.review_model.reasoning_effort.clone(), params.reasoning_summary, params.personality, /*model_messages*/ None, @@ -236,13 +236,15 @@ async fn test_review_params() -> GuardianReviewSessionParams { }, reasons: ApprovalRequestReasons::default(), schema: super::super::guardian_output_schema(), - model, + review_model: ReviewModel { + model, + reasoning_effort, + default_review_model_id: "codex-auto-review".to_string(), + catalog_contains_auto_review: true, + model_overridden: false, + model_override: None, + }, compaction_model_hash: None, - reasoning_effort, - guardian_default_review_model_id: "codex-auto-review".to_string(), - guardian_catalog_contains_auto_review: true, - guardian_review_model_overridden: false, - guardian_review_model_override: None, reasoning_summary, personality, external_cancel: None,