mirror of
https://github.com/openai/codex.git
synced 2026-09-17 12:23:33 +00:00
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
This commit is contained in:
@@ -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),
|
||||
|
||||
@@ -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<String>,
|
||||
model: String,
|
||||
reasoning_effort: Option<codex_protocol::openai_models::ReasoningEffort>,
|
||||
default_review_model_id: String,
|
||||
catalog_contains_auto_review: bool,
|
||||
model_overridden: bool,
|
||||
model_override: Option<String>,
|
||||
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,
|
||||
|
||||
@@ -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<String>,
|
||||
pub(crate) reasoning_effort: Option<ReasoningEffortConfig>,
|
||||
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<String>,
|
||||
pub(crate) reasoning_summary: ReasoningSummaryConfig,
|
||||
pub(crate) personality: Option<Personality>,
|
||||
pub(crate) external_cancel: Option<CancellationToken>,
|
||||
@@ -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(),
|
||||
|
||||
@@ -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,
|
||||
|
||||
Reference in New Issue
Block a user