diff --git a/codex-rs/analytics/src/analytics_client_tests.rs b/codex-rs/analytics/src/analytics_client_tests.rs index c0983a3db..7c634ce37 100644 --- a/codex-rs/analytics/src/analytics_client_tests.rs +++ b/codex-rs/analytics/src/analytics_client_tests.rs @@ -1987,6 +1987,11 @@ async fn guardian_review_event_ingests_custom_fact_with_optional_target_item() { guardian_session_kind: None, guardian_model: None, guardian_reasoning_effort: None, + guardian_default_review_model_id: Some("codex-auto-review".to_string()), + guardian_catalog_contains_auto_review: Some(false), + guardian_review_model_overridden: Some(false), + guardian_review_model_override: None, + guardian_model_provider_id: Some("openai".to_string()), had_prior_review_context: None, review_timeout_ms: 90_000, tool_call_count: None, @@ -2053,6 +2058,26 @@ async fn guardian_review_event_ingests_custom_fact_with_optional_target_item() { assert_eq!(payload[0]["event_params"]["failure_reason"], "timeout"); assert_eq!(payload[0]["event_params"]["attempt_count"], 1); assert_eq!(payload[0]["event_params"]["review_timeout_ms"], 90_000); + assert_eq!( + payload[0]["event_params"]["guardian_default_review_model_id"], + "codex-auto-review" + ); + assert_eq!( + payload[0]["event_params"]["guardian_catalog_contains_auto_review"], + false + ); + assert_eq!( + payload[0]["event_params"]["guardian_review_model_overridden"], + false + ); + assert_eq!( + payload[0]["event_params"]["guardian_review_model_override"], + json!(null) + ); + assert_eq!( + payload[0]["event_params"]["guardian_model_provider_id"], + "openai" + ); } #[tokio::test] diff --git a/codex-rs/analytics/src/events.rs b/codex-rs/analytics/src/events.rs index 2df113a43..eed972852 100644 --- a/codex-rs/analytics/src/events.rs +++ b/codex-rs/analytics/src/events.rs @@ -274,6 +274,11 @@ pub struct GuardianReviewEventParams { pub guardian_session_kind: Option, pub guardian_model: Option, pub guardian_reasoning_effort: Option, + pub guardian_default_review_model_id: Option, + pub guardian_catalog_contains_auto_review: Option, + pub guardian_review_model_overridden: Option, + pub guardian_review_model_override: Option, + pub guardian_model_provider_id: Option, pub had_prior_review_context: Option, pub review_timeout_ms: u64, pub tool_call_count: Option, @@ -347,6 +352,11 @@ impl GuardianReviewTrackContext { guardian_session_kind: result.guardian_session_kind, guardian_model: result.guardian_model, guardian_reasoning_effort: result.guardian_reasoning_effort, + guardian_default_review_model_id: result.guardian_default_review_model_id, + guardian_catalog_contains_auto_review: result.guardian_catalog_contains_auto_review, + guardian_review_model_overridden: result.guardian_review_model_overridden, + guardian_review_model_override: result.guardian_review_model_override, + guardian_model_provider_id: result.guardian_model_provider_id, had_prior_review_context: result.had_prior_review_context, review_timeout_ms: self.review_timeout_ms, // TODO(rhan-oai): plumb nested Guardian review session tool-call counts. @@ -383,6 +393,11 @@ pub struct GuardianReviewAnalyticsResult { pub guardian_session_kind: Option, pub guardian_model: Option, pub guardian_reasoning_effort: Option, + pub guardian_default_review_model_id: Option, + pub guardian_catalog_contains_auto_review: Option, + pub guardian_review_model_overridden: Option, + pub guardian_review_model_override: Option, + pub guardian_model_provider_id: Option, pub had_prior_review_context: Option, pub reviewed_action_truncated: bool, pub token_usage: Option, @@ -403,6 +418,11 @@ impl GuardianReviewAnalyticsResult { guardian_session_kind: None, guardian_model: None, guardian_reasoning_effort: None, + guardian_default_review_model_id: None, + guardian_catalog_contains_auto_review: None, + guardian_review_model_overridden: None, + guardian_review_model_override: None, + guardian_model_provider_id: None, had_prior_review_context: None, reviewed_action_truncated: false, token_usage: None, @@ -410,24 +430,38 @@ impl GuardianReviewAnalyticsResult { } } - pub fn from_session( - guardian_thread_id: String, - guardian_session_kind: GuardianReviewSessionKind, - guardian_model: String, - guardian_reasoning_effort: Option, - had_prior_review_context: bool, - ) -> Self { + pub fn from_session(params: GuardianReviewSessionAnalyticsParams) -> Self { Self { - guardian_thread_id: Some(guardian_thread_id), - guardian_session_kind: Some(guardian_session_kind), - guardian_model: Some(guardian_model), - guardian_reasoning_effort, - had_prior_review_context: Some(had_prior_review_context), + guardian_thread_id: Some(params.guardian_thread_id), + guardian_session_kind: Some(params.guardian_session_kind), + guardian_model: Some(params.guardian_model), + guardian_reasoning_effort: params.guardian_reasoning_effort, + guardian_default_review_model_id: Some(params.guardian_default_review_model_id), + guardian_catalog_contains_auto_review: Some( + params.guardian_catalog_contains_auto_review, + ), + guardian_review_model_overridden: Some(params.guardian_review_model_overridden), + guardian_review_model_override: params.guardian_review_model_override, + guardian_model_provider_id: Some(params.guardian_model_provider_id), + had_prior_review_context: Some(params.had_prior_review_context), ..Self::without_session() } } } +pub struct GuardianReviewSessionAnalyticsParams { + pub guardian_thread_id: String, + pub guardian_session_kind: GuardianReviewSessionKind, + pub guardian_model: String, + pub guardian_reasoning_effort: Option, + pub guardian_default_review_model_id: String, + pub guardian_catalog_contains_auto_review: bool, + pub guardian_review_model_overridden: bool, + pub guardian_review_model_override: Option, + pub guardian_model_provider_id: String, + pub had_prior_review_context: bool, +} + #[derive(Serialize)] pub(crate) struct GuardianReviewEventPayload { pub(crate) session_id: String, diff --git a/codex-rs/analytics/src/lib.rs b/codex-rs/analytics/src/lib.rs index fd236cbce..6e4723275 100644 --- a/codex-rs/analytics/src/lib.rs +++ b/codex-rs/analytics/src/lib.rs @@ -16,6 +16,7 @@ pub use events::GuardianReviewAnalyticsResult; pub use events::GuardianReviewDecision; pub use events::GuardianReviewEventParams; pub use events::GuardianReviewFailureReason; +pub use events::GuardianReviewSessionAnalyticsParams; pub use events::GuardianReviewSessionKind; pub use events::GuardianReviewTerminalStatus; pub use events::GuardianReviewTrackContext; diff --git a/codex-rs/core/src/guardian/review.rs b/codex-rs/core/src/guardian/review.rs index 99f9c5d99..92223083d 100644 --- a/codex-rs/core/src/guardian/review.rs +++ b/codex-rs/core/src/guardian/review.rs @@ -706,6 +706,7 @@ async fn run_guardian_review_session_before_deadline( .models_manager .list_models(codex_models_manager::manager::RefreshStrategy::Offline) .await; + let default_review_model_id = turn.provider.approval_review_preferred_model(); let preferred_reasoning_effort = |supports_low: bool, fallback| { if supports_low { Some(codex_protocol::openai_models::ReasoningEffort::Low) @@ -714,11 +715,15 @@ async fn run_guardian_review_session_before_deadline( } }; 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_id = model_override.unwrap_or(default_review_model_id); let review_model = available_models .iter() .find(|preset| preset.model == review_model_id); + let guardian_catalog_contains_auto_review = available_models + .iter() + .any(|preset| preset.model == default_review_model_id); + let guardian_review_model_overridden = model_override.is_some(); + let guardian_review_model_override = model_override.map(str::to_string); let (guardian_model, guardian_reasoning_effort) = if let Some(preset) = review_model { let reasoning_effort = preferred_reasoning_effort( preset @@ -773,6 +778,10 @@ async fn run_guardian_review_session_before_deadline( schema, model: guardian_model, reasoning_effort: guardian_reasoning_effort, + guardian_default_review_model_id: default_review_model_id.to_string(), + guardian_catalog_contains_auto_review, + guardian_review_model_overridden, + guardian_review_model_override, reasoning_summary: turn.reasoning_summary, personality: turn.personality, external_cancel, diff --git a/codex-rs/core/src/guardian/review_session.rs b/codex-rs/core/src/guardian/review_session.rs index f8eb88aaf..2226a1b98 100644 --- a/codex-rs/core/src/guardian/review_session.rs +++ b/codex-rs/core/src/guardian/review_session.rs @@ -6,6 +6,7 @@ use std::time::Duration; use anyhow::anyhow; use codex_analytics::GuardianReviewAnalyticsResult; +use codex_analytics::GuardianReviewSessionAnalyticsParams; use codex_analytics::GuardianReviewSessionKind; use codex_extension_api::UserInstructions; use codex_protocol::ThreadId; @@ -78,6 +79,10 @@ pub(crate) struct GuardianReviewSessionParams { pub(crate) schema: Value, pub(crate) model: String, 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, @@ -680,13 +685,19 @@ async fn run_review_on_session( } else { None }; - let mut analytics_result = GuardianReviewAnalyticsResult::from_session( - review_session.codex.session.thread_id.to_string(), - guardian_session_kind, - params.model.clone(), - guardian_reasoning_effort.map(|effort| effort.to_string()), - had_prior_review_context(&prompt_mode), - ); + let mut analytics_result = + GuardianReviewAnalyticsResult::from_session(GuardianReviewSessionAnalyticsParams { + guardian_thread_id: review_session.codex.session.thread_id.to_string(), + guardian_session_kind, + guardian_model: params.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_model_provider_id: params.spawn_config.model_provider_id.clone(), + had_prior_review_context: had_prior_review_context(&prompt_mode), + }); if send_followup_reminder { append_guardian_followup_reminder(review_session).await; } @@ -1172,6 +1183,10 @@ mod tests { schema: super::super::prompt::guardian_output_schema(), model, 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, diff --git a/codex-rs/core/src/guardian/tests.rs b/codex-rs/core/src/guardian/tests.rs index 1698ea0c6..7b269bd33 100644 --- a/codex-rs/core/src/guardian/tests.rs +++ b/codex-rs/core/src/guardian/tests.rs @@ -25,6 +25,8 @@ use codex_model_provider::create_model_provider; use codex_model_provider_info::AMAZON_BEDROCK_GPT_5_4_MODEL_ID; use codex_model_provider_info::AMAZON_BEDROCK_PROVIDER_ID; use codex_model_provider_info::ModelProviderInfo; +use codex_model_provider_info::OPENAI_PROVIDER_ID; +use codex_models_manager::manager::StaticModelsManager; use codex_network_proxy::NetworkProxyConfig; use codex_protocol::ThreadId; use codex_protocol::approvals::NetworkApprovalProtocol; @@ -32,6 +34,7 @@ use codex_protocol::config_types::ApprovalsReviewer; use codex_protocol::models::ContentItem; use codex_protocol::models::PermissionProfile; use codex_protocol::models::ResponseItem; +use codex_protocol::openai_models::ModelsResponse; use codex_protocol::openai_models::ReasoningEffort; use codex_protocol::permissions::FileSystemAccessMode; use codex_protocol::permissions::FileSystemPath; @@ -1346,9 +1349,20 @@ fn guardian_output_schema_requires_only_outcome_and_allows_optional_details() { ); } -async fn guardian_request_model_for_auto_review_override( +enum GuardianTestCatalog { + Bundled, + ParentOnly, +} + +async fn guardian_request_model_for_auto_review( auto_review_model_override: Option, -) -> anyhow::Result<(String, String, String)> { + catalog: GuardianTestCatalog, +) -> anyhow::Result<( + String, + String, + String, + codex_analytics::GuardianReviewAnalyticsResult, +)> { let server = start_mock_server().await; let guardian_assessment = serde_json::json!({ "outcome": "allow", @@ -1364,7 +1378,24 @@ async fn guardian_request_model_for_auto_review_override( ) .await; - let (session, mut turn) = guardian_test_session_and_turn(&server).await; + let (mut session, mut turn) = guardian_test_session_and_turn(&server).await; + match catalog { + GuardianTestCatalog::Bundled => {} + GuardianTestCatalog::ParentOnly => { + let parent_model = turn.model_info.clone(); + let auth_manager = Arc::clone(&session.services.auth_manager); + let models_manager = StaticModelsManager::new( + Some(auth_manager), + ModelsResponse { + models: vec![parent_model], + }, + ); + Arc::get_mut(&mut session) + .expect("session should be unique") + .services + .models_manager = Arc::new(models_manager); + } + } Arc::get_mut(&mut turn) .expect("turn should be unique") .model_info @@ -1373,7 +1404,7 @@ async fn guardian_request_model_for_auto_review_override( let preferred_model = turn.provider.approval_review_preferred_model().to_string(); seed_guardian_parent_history(&session, &turn).await; - let outcome = run_guardian_review_session_for_test( + let (outcome, analytics_result) = run_guardian_review_session_for_test( Arc::clone(&session), turn, GuardianApprovalRequest::Shell { @@ -1390,19 +1421,24 @@ async fn guardian_request_model_for_auto_review_override( /*max_attempts*/ 1, ) .await; - let (GuardianReviewOutcome::Completed(_), _) = outcome else { + let GuardianReviewOutcome::Completed(_) = outcome else { panic!("expected guardian assessment"); }; - let request_model = request_log - .single_request() + let request = request_log.single_request(); + let request_model = request .body_json() .get("model") .and_then(|value| value.as_str()) .expect("guardian request should include a model") .to_string(); - Ok((request_model, parent_model, preferred_model)) + Ok(( + request_model, + parent_model, + preferred_model, + analytics_result, + )) } #[tokio::test(flavor = "multi_thread", worker_threads = 2)] @@ -1411,12 +1447,36 @@ async fn guardian_review_uses_model_catalog_override_when_preferred_review_model 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(override_model.clone())).await?; + let (request_model, parent_model, preferred_model, analytics_result) = + guardian_request_model_for_auto_review( + Some(override_model.clone()), + GuardianTestCatalog::Bundled, + ) + .await?; assert_eq!(request_model, override_model); assert_ne!(request_model, parent_model); assert_ne!(request_model, preferred_model); + assert_eq!( + analytics_result.guardian_catalog_contains_auto_review, + Some(true) + ); + assert_eq!( + analytics_result.guardian_default_review_model_id.as_deref(), + Some(preferred_model.as_str()) + ); + assert_eq!( + analytics_result.guardian_review_model_overridden, + Some(true) + ); + assert_eq!( + analytics_result.guardian_review_model_override.as_deref(), + Some(override_model.as_str()) + ); + assert_eq!( + analytics_result.guardian_model_provider_id.as_deref(), + Some(OPENAI_PROVIDER_ID) + ); Ok(()) } @@ -1426,12 +1486,73 @@ async fn guardian_review_uses_preferred_review_model_without_model_catalog_overr -> anyhow::Result<()> { skip_if_no_network!(Ok(())); - let (request_model, parent_model, preferred_model) = - guardian_request_model_for_auto_review_override(/*auto_review_model_override*/ None) - .await?; + let (request_model, parent_model, preferred_model, analytics_result) = + guardian_request_model_for_auto_review( + /*auto_review_model_override*/ None, + GuardianTestCatalog::Bundled, + ) + .await?; assert_eq!(request_model, preferred_model); assert_ne!(request_model, parent_model); + assert_eq!( + analytics_result.guardian_catalog_contains_auto_review, + Some(true) + ); + assert_eq!( + analytics_result.guardian_default_review_model_id.as_deref(), + Some(preferred_model.as_str()) + ); + assert_eq!( + analytics_result.guardian_review_model_overridden, + Some(false) + ); + assert_eq!( + analytics_result.guardian_review_model_override.as_deref(), + None + ); + assert_eq!( + analytics_result.guardian_model_provider_id.as_deref(), + Some(OPENAI_PROVIDER_ID) + ); + + Ok(()) +} + +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn guardian_review_records_missing_auto_review_model_in_analytics_metadata() +-> anyhow::Result<()> { + skip_if_no_network!(Ok(())); + + let (request_model, parent_model, preferred_model, analytics_result) = + guardian_request_model_for_auto_review( + /*auto_review_model_override*/ None, + GuardianTestCatalog::ParentOnly, + ) + .await?; + + assert_eq!(request_model, parent_model); + assert_ne!(request_model, preferred_model); + assert_eq!( + analytics_result.guardian_catalog_contains_auto_review, + Some(false) + ); + assert_eq!( + analytics_result.guardian_default_review_model_id.as_deref(), + Some(preferred_model.as_str()) + ); + assert_eq!( + analytics_result.guardian_review_model_overridden, + Some(false) + ); + assert_eq!( + analytics_result.guardian_review_model_override.as_deref(), + None + ); + assert_eq!( + analytics_result.guardian_model_provider_id.as_deref(), + Some(OPENAI_PROVIDER_ID) + ); Ok(()) }