mirror of
https://github.com/pchuan98/codex.git
synced 2026-07-01 00:31:56 +08:00
Add Guardian catalog diagnostics metadata (#27109)
## Why We need request-level evidence for Guardian cases where `codex-auto-review` is missing from the client-side model catalog and the review falls back to the parent model. ## What changed - Add `guardian_catalog_contains_auto_review` to Guardian Responses API client metadata. - Add `guardian_model_provider_id` to Guardian Responses API client metadata. - Keep review-session metadata optional so callers without metadata preserve the existing `None` path. - Add tests for override, normal preferred-model, and missing-auto-review-catalog behavior. ## Validation - `just test -p codex-core guardian_review_records_missing_auto_review_model_in_request_metadata` - `just test -p codex-core guardian_review_uses_model_catalog_override_when_preferred_review_model_exists` - `just test -p codex-core guardian_review_uses_preferred_review_model_without_model_catalog_override` - `git diff --check origin/main`
This commit is contained in:
committed by
GitHub
Unverified
parent
c0f1ec5afd
commit
0605f9c14f
@@ -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]
|
||||
|
||||
@@ -274,6 +274,11 @@ pub struct GuardianReviewEventParams {
|
||||
pub guardian_session_kind: Option<GuardianReviewSessionKind>,
|
||||
pub guardian_model: Option<String>,
|
||||
pub guardian_reasoning_effort: Option<String>,
|
||||
pub guardian_default_review_model_id: Option<String>,
|
||||
pub guardian_catalog_contains_auto_review: Option<bool>,
|
||||
pub guardian_review_model_overridden: Option<bool>,
|
||||
pub guardian_review_model_override: Option<String>,
|
||||
pub guardian_model_provider_id: Option<String>,
|
||||
pub had_prior_review_context: Option<bool>,
|
||||
pub review_timeout_ms: u64,
|
||||
pub tool_call_count: Option<u64>,
|
||||
@@ -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<GuardianReviewSessionKind>,
|
||||
pub guardian_model: Option<String>,
|
||||
pub guardian_reasoning_effort: Option<String>,
|
||||
pub guardian_default_review_model_id: Option<String>,
|
||||
pub guardian_catalog_contains_auto_review: Option<bool>,
|
||||
pub guardian_review_model_overridden: Option<bool>,
|
||||
pub guardian_review_model_override: Option<String>,
|
||||
pub guardian_model_provider_id: Option<String>,
|
||||
pub had_prior_review_context: Option<bool>,
|
||||
pub reviewed_action_truncated: bool,
|
||||
pub token_usage: Option<TokenUsage>,
|
||||
@@ -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<String>,
|
||||
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<String>,
|
||||
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<String>,
|
||||
pub guardian_model_provider_id: String,
|
||||
pub had_prior_review_context: bool,
|
||||
}
|
||||
|
||||
#[derive(Serialize)]
|
||||
pub(crate) struct GuardianReviewEventPayload {
|
||||
pub(crate) session_id: String,
|
||||
|
||||
@@ -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;
|
||||
|
||||
@@ -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,
|
||||
|
||||
@@ -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<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>,
|
||||
@@ -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,
|
||||
|
||||
@@ -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<String>,
|
||||
) -> 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(())
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user