mirror of
https://github.com/openai/codex.git
synced 2026-11-03 08:22:30 +00:00
feat: route guardian review model selection through providers (#22258)
## Why Guardian review selection was hard-coded in `core`, which worked for the default OpenAI path but did not give provider implementations a way to choose backend-specific reviewer model IDs. That matters for Amazon Bedrock: guardian review should run through the Bedrock/Mantle provider using Bedrock's `openai.gpt-5.4` model ID, instead of accidentally selecting a reviewer model that implies the OpenAI backend. ## What Changed - Added provider-owned approval review model selection via `ModelProvider::approval_review_model_selection`. - Moved the existing default selection policy into the provider abstraction: prefer the requested reviewer model when it is available, otherwise fall back to the active turn model, preferring `Low` reasoning when supported. - Added an Amazon Bedrock override that pins guardian review to `openai.gpt-5.4` with `Low` reasoning.
This commit is contained in:
1 parent
51bfb5f3b1
commit
e2eb7c30fe
7 files changed
+82
-10
No files matched your search
@@ -40,7 +40,6 @@ pub(crate) use review::routes_approval_to_guardian;
|
|||||||
pub(crate) use review::spawn_approval_request_review;
|
pub(crate) use review::spawn_approval_request_review;
|
||||||
pub(crate) use review_session::GuardianReviewSessionManager;
|
pub(crate) use review_session::GuardianReviewSessionManager;
|
||||||
|
|
||||||
const GUARDIAN_PREFERRED_MODEL: &str = "codex-auto-review";
|
|
||||||
pub(crate) const GUARDIAN_REVIEW_TIMEOUT: Duration = Duration::from_secs(90);
|
pub(crate) const GUARDIAN_REVIEW_TIMEOUT: Duration = Duration::from_secs(90);
|
||||||
pub(crate) const GUARDIAN_REVIEWER_NAME: &str = "guardian";
|
pub(crate) const GUARDIAN_REVIEWER_NAME: &str = "guardian";
|
||||||
pub(crate) const MAX_CONSECUTIVE_GUARDIAN_DENIALS_PER_TURN: u32 = 3;
|
pub(crate) const MAX_CONSECUTIVE_GUARDIAN_DENIALS_PER_TURN: u32 = 3;
|
||||||
|
|||||||
@@ -659,9 +659,10 @@ pub(super) async fn run_guardian_review_session(
|
|||||||
fallback
|
fallback
|
||||||
}
|
}
|
||||||
};
|
};
|
||||||
|
let preferred_model_id = turn.provider.approval_review_preferred_model();
|
||||||
let preferred_model = available_models
|
let preferred_model = available_models
|
||||||
.iter()
|
.iter()
|
||||||
.find(|preset| preset.model == super::GUARDIAN_PREFERRED_MODEL);
|
.find(|preset| preset.model == preferred_model_id);
|
||||||
let (guardian_model, guardian_reasoning_effort) = if let Some(preset) = preferred_model {
|
let (guardian_model, guardian_reasoning_effort) = if let Some(preset) = preferred_model {
|
||||||
let reasoning_effort = preferred_reasoning_effort(
|
let reasoning_effort = preferred_reasoning_effort(
|
||||||
preset
|
preset
|
||||||
@@ -670,10 +671,7 @@ pub(super) async fn run_guardian_review_session(
|
|||||||
.any(|effort| effort.effort == codex_protocol::openai_models::ReasoningEffort::Low),
|
.any(|effort| effort.effort == codex_protocol::openai_models::ReasoningEffort::Low),
|
||||||
Some(preset.default_reasoning_effort),
|
Some(preset.default_reasoning_effort),
|
||||||
);
|
);
|
||||||
(
|
(preferred_model_id.to_string(), reasoning_effort)
|
||||||
super::GUARDIAN_PREFERRED_MODEL.to_string(),
|
|
||||||
reasoning_effort,
|
|
||||||
)
|
|
||||||
} else {
|
} else {
|
||||||
let reasoning_effort = preferred_reasoning_effort(
|
let reasoning_effort = preferred_reasoning_effort(
|
||||||
turn.model_info
|
turn.model_info
|
||||||
|
|||||||
@@ -22,6 +22,9 @@ use codex_config::types::McpServerConfig;
|
|||||||
use codex_exec_server::LOCAL_FS;
|
use codex_exec_server::LOCAL_FS;
|
||||||
use codex_features::Feature;
|
use codex_features::Feature;
|
||||||
use codex_model_provider::create_model_provider;
|
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_network_proxy::NetworkProxyConfig;
|
use codex_network_proxy::NetworkProxyConfig;
|
||||||
use codex_protocol::ThreadId;
|
use codex_protocol::ThreadId;
|
||||||
use codex_protocol::approvals::NetworkApprovalProtocol;
|
use codex_protocol::approvals::NetworkApprovalProtocol;
|
||||||
@@ -29,6 +32,7 @@ use codex_protocol::config_types::ApprovalsReviewer;
|
|||||||
use codex_protocol::models::ContentItem;
|
use codex_protocol::models::ContentItem;
|
||||||
use codex_protocol::models::PermissionProfile;
|
use codex_protocol::models::PermissionProfile;
|
||||||
use codex_protocol::models::ResponseItem;
|
use codex_protocol::models::ResponseItem;
|
||||||
|
use codex_protocol::openai_models::ReasoningEffort;
|
||||||
use codex_protocol::protocol::AskForApproval;
|
use codex_protocol::protocol::AskForApproval;
|
||||||
use codex_protocol::protocol::Event;
|
use codex_protocol::protocol::Event;
|
||||||
use codex_protocol::protocol::EventMsg;
|
use codex_protocol::protocol::EventMsg;
|
||||||
@@ -2336,6 +2340,35 @@ async fn guardian_review_session_config_uses_parent_active_model_instead_of_hard
|
|||||||
assert_eq!(guardian_config.model, Some("active-model".to_string()));
|
assert_eq!(guardian_config.model, Some("active-model".to_string()));
|
||||||
}
|
}
|
||||||
|
|
||||||
|
#[tokio::test]
|
||||||
|
async fn guardian_review_session_config_keeps_bedrock_provider_for_bedrock_gpt_5_4() {
|
||||||
|
let mut parent_config = test_config().await;
|
||||||
|
parent_config.model_provider_id = AMAZON_BEDROCK_PROVIDER_ID.to_string();
|
||||||
|
parent_config.model_provider =
|
||||||
|
ModelProviderInfo::create_amazon_bedrock_provider(/*aws*/ None);
|
||||||
|
|
||||||
|
let guardian_config = build_guardian_review_session_config_for_test(
|
||||||
|
&parent_config,
|
||||||
|
/*live_network_config*/ None,
|
||||||
|
AMAZON_BEDROCK_GPT_5_4_MODEL_ID,
|
||||||
|
Some(ReasoningEffort::Low),
|
||||||
|
)
|
||||||
|
.expect("guardian config");
|
||||||
|
|
||||||
|
assert_eq!(
|
||||||
|
(
|
||||||
|
guardian_config.model,
|
||||||
|
guardian_config.model_provider_id,
|
||||||
|
guardian_config.model_provider,
|
||||||
|
),
|
||||||
|
(
|
||||||
|
Some(AMAZON_BEDROCK_GPT_5_4_MODEL_ID.to_string()),
|
||||||
|
AMAZON_BEDROCK_PROVIDER_ID.to_string(),
|
||||||
|
ModelProviderInfo::create_amazon_bedrock_provider(/*aws*/ None),
|
||||||
|
)
|
||||||
|
);
|
||||||
|
}
|
||||||
|
|
||||||
#[tokio::test]
|
#[tokio::test]
|
||||||
async fn guardian_review_session_config_uses_requirements_guardian_policy_config() {
|
async fn guardian_review_session_config_uses_requirements_guardian_policy_config() {
|
||||||
let codex_home = tempfile::tempdir().expect("create temp dir");
|
let codex_home = tempfile::tempdir().expect("create temp dir");
|
||||||
|
|||||||
@@ -37,6 +37,7 @@ pub const OPENAI_PROVIDER_ID: &str = "openai";
|
|||||||
pub const CHATGPT_CODEX_BASE_URL: &str = "https://chatgpt.com/backend-api/codex";
|
pub const CHATGPT_CODEX_BASE_URL: &str = "https://chatgpt.com/backend-api/codex";
|
||||||
const AMAZON_BEDROCK_PROVIDER_NAME: &str = "Amazon Bedrock";
|
const AMAZON_BEDROCK_PROVIDER_NAME: &str = "Amazon Bedrock";
|
||||||
pub const AMAZON_BEDROCK_PROVIDER_ID: &str = "amazon-bedrock";
|
pub const AMAZON_BEDROCK_PROVIDER_ID: &str = "amazon-bedrock";
|
||||||
|
pub const AMAZON_BEDROCK_GPT_5_4_MODEL_ID: &str = "openai.gpt-5.4";
|
||||||
pub const AMAZON_BEDROCK_DEFAULT_BASE_URL: &str =
|
pub const AMAZON_BEDROCK_DEFAULT_BASE_URL: &str =
|
||||||
"https://bedrock-mantle.us-east-1.api.aws/openai/v1";
|
"https://bedrock-mantle.us-east-1.api.aws/openai/v1";
|
||||||
const AMAZON_BEDROCK_MANTLE_CLIENT_AGENT_HEADER: &str = "x-amzn-mantle-client-agent";
|
const AMAZON_BEDROCK_MANTLE_CLIENT_AGENT_HEADER: &str = "x-amzn-mantle-client-agent";
|
||||||
|
|||||||
@@ -1,3 +1,4 @@
|
|||||||
|
use codex_model_provider_info::AMAZON_BEDROCK_GPT_5_4_MODEL_ID;
|
||||||
use codex_models_manager::model_info::BASE_INSTRUCTIONS;
|
use codex_models_manager::model_info::BASE_INSTRUCTIONS;
|
||||||
use codex_protocol::config_types::ReasoningSummary;
|
use codex_protocol::config_types::ReasoningSummary;
|
||||||
use codex_protocol::config_types::ServiceTier;
|
use codex_protocol::config_types::ServiceTier;
|
||||||
@@ -18,7 +19,6 @@ use codex_protocol::openai_models::WebSearchToolType;
|
|||||||
const GPT_OSS_CONTEXT_WINDOW: i64 = 128_000;
|
const GPT_OSS_CONTEXT_WINDOW: i64 = 128_000;
|
||||||
const GPT_5_4_CONTEXT_WINDOW: i64 = 272_000;
|
const GPT_5_4_CONTEXT_WINDOW: i64 = 272_000;
|
||||||
const GPT_5_4_MAX_CONTEXT_WINDOW: i64 = 1_000_000;
|
const GPT_5_4_MAX_CONTEXT_WINDOW: i64 = 1_000_000;
|
||||||
const GPT_5_4_CMB_MODEL_ID: &str = "openai.gpt-5.4";
|
|
||||||
|
|
||||||
pub(crate) fn static_model_catalog() -> ModelsResponse {
|
pub(crate) fn static_model_catalog() -> ModelsResponse {
|
||||||
ModelsResponse {
|
ModelsResponse {
|
||||||
@@ -40,7 +40,7 @@ pub(crate) fn static_model_catalog() -> ModelsResponse {
|
|||||||
|
|
||||||
fn gpt_5_4_cmb_bedrock_model(priority: i32) -> ModelInfo {
|
fn gpt_5_4_cmb_bedrock_model(priority: i32) -> ModelInfo {
|
||||||
ModelInfo {
|
ModelInfo {
|
||||||
slug: GPT_5_4_CMB_MODEL_ID.to_string(),
|
slug: AMAZON_BEDROCK_GPT_5_4_MODEL_ID.to_string(),
|
||||||
display_name: "gpt-5.4".to_string(),
|
display_name: "gpt-5.4".to_string(),
|
||||||
description: Some("Strong model for everyday coding.".to_string()),
|
description: Some("Strong model for everyday coding.".to_string()),
|
||||||
default_reasoning_level: Some(ReasoningEffort::Medium),
|
default_reasoning_level: Some(ReasoningEffort::Medium),
|
||||||
@@ -155,7 +155,7 @@ mod tests {
|
|||||||
let catalog = static_model_catalog();
|
let catalog = static_model_catalog();
|
||||||
|
|
||||||
assert_eq!(catalog.models.len(), 3);
|
assert_eq!(catalog.models.len(), 3);
|
||||||
assert_eq!(catalog.models[0].slug, GPT_5_4_CMB_MODEL_ID);
|
assert_eq!(catalog.models[0].slug, AMAZON_BEDROCK_GPT_5_4_MODEL_ID);
|
||||||
assert_eq!(catalog.models[1].slug, "openai.gpt-oss-120b");
|
assert_eq!(catalog.models[1].slug, "openai.gpt-oss-120b");
|
||||||
assert_eq!(catalog.models[2].slug, "openai.gpt-oss-20b");
|
assert_eq!(catalog.models[2].slug, "openai.gpt-oss-20b");
|
||||||
}
|
}
|
||||||
@@ -166,7 +166,7 @@ mod tests {
|
|||||||
let cmb_model = catalog
|
let cmb_model = catalog
|
||||||
.models
|
.models
|
||||||
.iter()
|
.iter()
|
||||||
.find(|model| model.slug == GPT_5_4_CMB_MODEL_ID)
|
.find(|model| model.slug == AMAZON_BEDROCK_GPT_5_4_MODEL_ID)
|
||||||
.expect("Bedrock catalog should include GPT-5.4 CMB");
|
.expect("Bedrock catalog should include GPT-5.4 CMB");
|
||||||
|
|
||||||
assert_eq!(
|
assert_eq!(
|
||||||
|
|||||||
@@ -9,6 +9,7 @@ use codex_api::Provider;
|
|||||||
use codex_api::SharedAuthProvider;
|
use codex_api::SharedAuthProvider;
|
||||||
use codex_login::AuthManager;
|
use codex_login::AuthManager;
|
||||||
use codex_login::CodexAuth;
|
use codex_login::CodexAuth;
|
||||||
|
use codex_model_provider_info::AMAZON_BEDROCK_GPT_5_4_MODEL_ID;
|
||||||
use codex_model_provider_info::ModelProviderAwsAuthInfo;
|
use codex_model_provider_info::ModelProviderAwsAuthInfo;
|
||||||
use codex_model_provider_info::ModelProviderInfo;
|
use codex_model_provider_info::ModelProviderInfo;
|
||||||
use codex_models_manager::manager::SharedModelsManager;
|
use codex_models_manager::manager::SharedModelsManager;
|
||||||
@@ -62,6 +63,10 @@ impl ModelProvider for AmazonBedrockModelProvider {
|
|||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
|
fn approval_review_preferred_model(&self) -> &'static str {
|
||||||
|
AMAZON_BEDROCK_GPT_5_4_MODEL_ID
|
||||||
|
}
|
||||||
|
|
||||||
fn auth_manager(&self) -> Option<Arc<AuthManager>> {
|
fn auth_manager(&self) -> Option<Arc<AuthManager>> {
|
||||||
None
|
None
|
||||||
}
|
}
|
||||||
@@ -140,4 +145,16 @@ mod tests {
|
|||||||
}
|
}
|
||||||
);
|
);
|
||||||
}
|
}
|
||||||
|
|
||||||
|
#[test]
|
||||||
|
fn approval_review_preferred_model_uses_bedrock_gpt_5_4() {
|
||||||
|
let provider = AmazonBedrockModelProvider::new(
|
||||||
|
ModelProviderInfo::create_amazon_bedrock_provider(/*aws*/ None),
|
||||||
|
);
|
||||||
|
|
||||||
|
assert_eq!(
|
||||||
|
provider.approval_review_preferred_model(),
|
||||||
|
AMAZON_BEDROCK_GPT_5_4_MODEL_ID
|
||||||
|
);
|
||||||
|
}
|
||||||
}
|
}
|
||||||
@@ -70,6 +70,10 @@ impl std::error::Error for ProviderAccountError {}
|
|||||||
|
|
||||||
pub type ProviderAccountResult = std::result::Result<ProviderAccountState, ProviderAccountError>;
|
pub type ProviderAccountResult = std::result::Result<ProviderAccountState, ProviderAccountError>;
|
||||||
|
|
||||||
|
/// Default model used for automatic approval review when a provider does not
|
||||||
|
/// require a backend-specific model ID.
|
||||||
|
pub const DEFAULT_APPROVAL_REVIEW_PREFERRED_MODEL: &str = "codex-auto-review";
|
||||||
|
|
||||||
/// Runtime provider abstraction used by model execution.
|
/// Runtime provider abstraction used by model execution.
|
||||||
///
|
///
|
||||||
/// Implementations own provider-specific behavior for a model backend. The
|
/// Implementations own provider-specific behavior for a model backend. The
|
||||||
@@ -85,6 +89,13 @@ pub trait ModelProvider: fmt::Debug + Send + Sync {
|
|||||||
ProviderCapabilities::default()
|
ProviderCapabilities::default()
|
||||||
}
|
}
|
||||||
|
|
||||||
|
/// Returns the preferred model used for automatic approval review.
|
||||||
|
///
|
||||||
|
/// Providers that require backend-specific model IDs should override this.
|
||||||
|
fn approval_review_preferred_model(&self) -> &'static str {
|
||||||
|
DEFAULT_APPROVAL_REVIEW_PREFERRED_MODEL
|
||||||
|
}
|
||||||
|
|
||||||
/// Returns whether requests made through this provider should include attestation.
|
/// Returns whether requests made through this provider should include attestation.
|
||||||
fn supports_attestation(&self) -> bool {
|
fn supports_attestation(&self) -> bool {
|
||||||
false
|
false
|
||||||
@@ -350,6 +361,19 @@ mod tests {
|
|||||||
assert_eq!(provider.capabilities(), ProviderCapabilities::default());
|
assert_eq!(provider.capabilities(), ProviderCapabilities::default());
|
||||||
}
|
}
|
||||||
|
|
||||||
|
#[test]
|
||||||
|
fn configured_provider_uses_default_approval_review_preferred_model() {
|
||||||
|
let provider = create_model_provider(
|
||||||
|
ModelProviderInfo::create_openai_provider(/*base_url*/ None),
|
||||||
|
/*auth_manager*/ None,
|
||||||
|
);
|
||||||
|
|
||||||
|
assert_eq!(
|
||||||
|
provider.approval_review_preferred_model(),
|
||||||
|
DEFAULT_APPROVAL_REVIEW_PREFERRED_MODEL
|
||||||
|
);
|
||||||
|
}
|
||||||
|
|
||||||
#[tokio::test]
|
#[tokio::test]
|
||||||
async fn configured_provider_runtime_base_url_uses_configured_base_url() {
|
async fn configured_provider_runtime_base_url_uses_configured_base_url() {
|
||||||
let provider = create_model_provider(
|
let provider = create_model_provider(
|
||||||
|
|||||||
Reference in new issue
Block a user