mirror of
https://github.com/pchuan98/codex.git
synced 2026-07-01 00:31:56 +08:00
[codex-rs] auto-review model override (#23767)
## Why Guardian auto-review normally uses the provider-preferred review model when one is available. Some parent models need model-catalog metadata to select a different review model while keeping older `/models` payloads compatible when that metadata is absent. ## What changed - Added optional `ModelInfo::auto_review_model_override` metadata to the public model payload as a review-model slug. - Updated Guardian review model selection to prefer the catalog override when present, while preserving the existing provider preferred-model path and parent-model fallback when it is omitted. - Added focused Guardian coverage for override and no-override model selection. - Added an `auto_review` core integration suite test that loads override metadata from a remote model catalog path and asserts the strict auto-review `/responses` request uses the catalog-selected review model. - Updated existing `ModelInfo` fixtures and local catalog constructors for the new optional field. ## Validation - `cargo test -p codex-protocol model_info_defaults_availability_nux_to_none_when_omitted` - `cargo test -p codex-core guardian_review_uses_` - `cargo test -p codex-core remote_model_override_uses_catalog_model_for_strict_auto_review --test all` - `just fix -p codex-protocol` - `just fix -p codex-core` - `just fmt` - `git diff --check`
This commit is contained in:
committed by
GitHub
Unverified
parent
281b416c44
commit
f1609d9fb6
@@ -682,11 +682,13 @@ pub(super) async fn run_guardian_review_session(
|
||||
fallback
|
||||
}
|
||||
};
|
||||
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 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
|
||||
@@ -694,7 +696,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
|
||||
@@ -704,7 +706,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(),
|
||||
|
||||
@@ -1288,6 +1288,95 @@ 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<String>,
|
||||
) -> anyhow::Result<(String, String, String)> {
|
||||
let server = start_mock_server().await;
|
||||
let guardian_assessment = serde_json::json!({
|
||||
"outcome": "allow",
|
||||
})
|
||||
.to_string();
|
||||
let request_log = mount_sse_once(
|
||||
&server,
|
||||
sse(vec![
|
||||
ev_response_created("resp-guardian"),
|
||||
ev_assistant_message("msg-guardian", &guardian_assessment),
|
||||
ev_completed("resp-guardian"),
|
||||
]),
|
||||
)
|
||||
.await;
|
||||
|
||||
let (session, mut turn) = guardian_test_session_and_turn(&server).await;
|
||||
Arc::get_mut(&mut turn)
|
||||
.expect("turn should be unique")
|
||||
.model_info
|
||||
.auto_review_model_override = auto_review_model_override;
|
||||
let parent_model = turn.model_info.slug.clone();
|
||||
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(
|
||||
Arc::clone(&session),
|
||||
turn,
|
||||
GuardianApprovalRequest::Shell {
|
||||
id: "shell-1".to_string(),
|
||||
command: vec!["git".to_string(), "push".to_string()],
|
||||
cwd: test_path_buf("/repo/codex-rs/core").abs(),
|
||||
sandbox_permissions: crate::sandboxing::SandboxPermissions::UseDefault,
|
||||
additional_permissions: None,
|
||||
justification: None,
|
||||
},
|
||||
Some("Sandbox denied outbound git push to github.com.".to_string()),
|
||||
guardian_output_schema(),
|
||||
/*external_cancel*/ None,
|
||||
)
|
||||
.await;
|
||||
let (GuardianReviewOutcome::Completed(_), _) = outcome else {
|
||||
panic!("expected guardian assessment");
|
||||
};
|
||||
|
||||
let request_model = request_log
|
||||
.single_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))
|
||||
}
|
||||
|
||||
#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
|
||||
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(override_model.clone())).await?;
|
||||
|
||||
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_model_catalog_override()
|
||||
-> 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?;
|
||||
|
||||
assert_eq!(request_model, preferred_model);
|
||||
assert_ne!(request_model, parent_model);
|
||||
|
||||
Ok(())
|
||||
}
|
||||
|
||||
#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
|
||||
async fn guardian_review_request_layout_matches_model_visible_request_snapshot()
|
||||
-> anyhow::Result<()> {
|
||||
|
||||
Reference in New Issue
Block a user