mirror of
https://github.com/openai/codex.git
synced 2026-11-05 20:10:33 +00:00
Use model catalog policies for Guardian auto review (#32875)
- Add an `auto_review.policy` field to model catalog messages. - Use the selected Guardian model's catalog policy for review-session instructions, while preserving the precedence of `guardian_policy_config` and falling back to the built-in policy when neither is present. - Preserve auto-review messages when model instruction overrides remove catalog instruction templates. - Cover configured-policy precedence, explicit empty catalog policies, catalog-message preservation, and propagation of the catalog policy into a prewarmed Guardian session. GitOrigin-RevId: 26b61ae2958ea8325a64834dcf91f47e140d74b3
This commit is contained in:
@@ -737,11 +737,20 @@ pub(super) async fn guardian_review_session_config(
|
||||
)
|
||||
};
|
||||
|
||||
let guardian_model_info = session
|
||||
.services
|
||||
.models_manager
|
||||
.get_model_info(
|
||||
guardian_model.as_str(),
|
||||
&turn.config.to_models_manager_config(),
|
||||
)
|
||||
.await;
|
||||
let spawn_config = build_guardian_review_session_config(
|
||||
turn.config.as_ref(),
|
||||
live_network_config,
|
||||
guardian_model.as_str(),
|
||||
guardian_reasoning_effort.clone(),
|
||||
guardian_model_info.model_messages.as_ref(),
|
||||
)?;
|
||||
Ok(GuardianReviewSessionConfig {
|
||||
spawn_config,
|
||||
|
||||
@@ -15,6 +15,7 @@ use codex_protocol::config_types::Personality;
|
||||
use codex_protocol::config_types::ReasoningSummary as ReasoningSummaryConfig;
|
||||
use codex_protocol::models::PermissionProfile;
|
||||
use codex_protocol::models::ResponseItem;
|
||||
use codex_protocol::openai_models::ModelMessages;
|
||||
use codex_protocol::openai_models::ReasoningEffort as ReasoningEffortConfig;
|
||||
use codex_protocol::protocol::AskForApproval;
|
||||
use codex_protocol::protocol::CodexErrorInfo;
|
||||
@@ -995,6 +996,7 @@ pub(crate) fn build_guardian_review_session_config(
|
||||
live_network_config: Option<codex_network_proxy::NetworkProxyConfig>,
|
||||
active_model: &str,
|
||||
reasoning_effort: Option<codex_protocol::openai_models::ReasoningEffort>,
|
||||
model_messages: Option<&ModelMessages>,
|
||||
) -> anyhow::Result<Config> {
|
||||
let mut guardian_config = parent_config.clone();
|
||||
guardian_config.model = Some(active_model.to_string());
|
||||
@@ -1004,10 +1006,14 @@ pub(crate) fn build_guardian_review_session_config(
|
||||
guardian_config.include_skill_instructions = false;
|
||||
guardian_config.memories.use_memories = false;
|
||||
guardian_config.memories.dedicated_tools = false;
|
||||
let catalog_policy = model_messages
|
||||
.and_then(|messages| messages.auto_review.as_ref())
|
||||
.and_then(|messages| messages.policy.as_deref());
|
||||
guardian_config.base_instructions = Some(
|
||||
parent_config
|
||||
.guardian_policy_config
|
||||
.as_deref()
|
||||
.or(catalog_policy)
|
||||
.map(guardian_policy_prompt_with_config)
|
||||
.unwrap_or_else(guardian_policy_prompt),
|
||||
);
|
||||
@@ -1124,6 +1130,7 @@ async fn interrupt_and_drain_turn(codex: &Codex, expected_turn_id: &str) -> anyh
|
||||
#[cfg(test)]
|
||||
mod tests {
|
||||
use super::*;
|
||||
use codex_protocol::openai_models::AutoReviewMessages;
|
||||
use codex_protocol::protocol::AgentStatus;
|
||||
use codex_protocol::protocol::ErrorEvent;
|
||||
use codex_protocol::protocol::Submission;
|
||||
@@ -1211,6 +1218,7 @@ mod tests {
|
||||
/*live_network_config*/ None,
|
||||
model.as_str(),
|
||||
reasoning_effort.clone(),
|
||||
/*model_messages*/ None,
|
||||
)
|
||||
.expect("guardian config");
|
||||
|
||||
@@ -1249,6 +1257,7 @@ mod tests {
|
||||
/*live_network_config*/ None,
|
||||
"active-model",
|
||||
/*reasoning_effort*/ None,
|
||||
/*model_messages*/ None,
|
||||
)
|
||||
.expect("cached guardian config");
|
||||
let cached_reuse_key = GuardianReviewSessionReuseKey::from_spawn_config(
|
||||
@@ -1264,6 +1273,7 @@ mod tests {
|
||||
/*live_network_config*/ None,
|
||||
"active-model",
|
||||
/*reasoning_effort*/ None,
|
||||
/*model_messages*/ None,
|
||||
)
|
||||
.expect("next guardian config");
|
||||
let next_reuse_key = GuardianReviewSessionReuseKey::from_spawn_config(
|
||||
@@ -1329,6 +1339,7 @@ mod tests {
|
||||
/*live_network_config*/ None,
|
||||
"active-model",
|
||||
/*reasoning_effort*/ None,
|
||||
/*model_messages*/ None,
|
||||
)
|
||||
.expect("cached guardian config");
|
||||
let cached_reuse_key = GuardianReviewSessionReuseKey::from_spawn_config(
|
||||
@@ -1344,6 +1355,7 @@ mod tests {
|
||||
/*live_network_config*/ None,
|
||||
"active-model",
|
||||
/*reasoning_effort*/ None,
|
||||
/*model_messages*/ None,
|
||||
)
|
||||
.expect("next guardian config");
|
||||
let next_reuse_key = GuardianReviewSessionReuseKey::from_spawn_config(
|
||||
@@ -1367,6 +1379,7 @@ mod tests {
|
||||
/*live_network_config*/ None,
|
||||
"active-model",
|
||||
/*reasoning_effort*/ None,
|
||||
/*model_messages*/ None,
|
||||
)
|
||||
.expect("guardian config");
|
||||
|
||||
@@ -1383,12 +1396,73 @@ mod tests {
|
||||
/*live_network_config*/ None,
|
||||
"active-model",
|
||||
/*reasoning_effort*/ None,
|
||||
/*model_messages*/ None,
|
||||
)
|
||||
.expect("guardian config");
|
||||
|
||||
assert!(!guardian_config.include_skill_instructions);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn guardian_review_session_config_prefers_managed_policy_over_catalog_policy() {
|
||||
let mut parent_config = crate::config::test_config().await;
|
||||
let managed_policy = "Use the managed Guardian policy.";
|
||||
parent_config.guardian_policy_config = Some(managed_policy.to_string());
|
||||
let model_messages = ModelMessages {
|
||||
instructions_template: None,
|
||||
instructions_variables: None,
|
||||
approvals: None,
|
||||
auto_review: Some(AutoReviewMessages {
|
||||
policy: Some("Use the catalog Guardian policy.".to_string()),
|
||||
}),
|
||||
};
|
||||
|
||||
let guardian_config = build_guardian_review_session_config(
|
||||
&parent_config,
|
||||
/*live_network_config*/ None,
|
||||
"active-model",
|
||||
/*reasoning_effort*/ None,
|
||||
Some(&model_messages),
|
||||
)
|
||||
.expect("guardian config");
|
||||
|
||||
assert_eq!(
|
||||
guardian_config.base_instructions,
|
||||
Some(guardian_policy_prompt_with_config(managed_policy))
|
||||
);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn guardian_review_session_config_preserves_explicit_empty_catalog_policy() {
|
||||
let parent_config = crate::config::test_config().await;
|
||||
let model_messages = ModelMessages {
|
||||
instructions_template: None,
|
||||
instructions_variables: None,
|
||||
approvals: None,
|
||||
auto_review: Some(AutoReviewMessages {
|
||||
policy: Some(String::new()),
|
||||
}),
|
||||
};
|
||||
|
||||
let guardian_config = build_guardian_review_session_config(
|
||||
&parent_config,
|
||||
/*live_network_config*/ None,
|
||||
"active-model",
|
||||
/*reasoning_effort*/ None,
|
||||
Some(&model_messages),
|
||||
)
|
||||
.expect("guardian config");
|
||||
|
||||
assert_eq!(
|
||||
guardian_config.base_instructions,
|
||||
Some(guardian_policy_prompt_with_config(""))
|
||||
);
|
||||
assert_ne!(
|
||||
guardian_config.base_instructions,
|
||||
Some(guardian_policy_prompt())
|
||||
);
|
||||
}
|
||||
|
||||
#[tokio::test(flavor = "current_thread")]
|
||||
async fn run_before_review_deadline_times_out_before_future_completes() {
|
||||
let outcome = run_before_review_deadline(
|
||||
|
||||
@@ -2896,6 +2896,7 @@ async fn guardian_review_session_config_preserves_parent_network_proxy() {
|
||||
/*live_network_config*/ None,
|
||||
"parent-active-model",
|
||||
Some(codex_protocol::openai_models::ReasoningEffort::Low),
|
||||
/*model_messages*/ None,
|
||||
)
|
||||
.expect("guardian config");
|
||||
|
||||
@@ -2929,6 +2930,7 @@ async fn guardian_review_session_config_clears_parent_developer_instructions() {
|
||||
/*live_network_config*/ None,
|
||||
"active-model",
|
||||
/*reasoning_effort*/ None,
|
||||
/*model_messages*/ None,
|
||||
)
|
||||
.expect("guardian config");
|
||||
|
||||
@@ -2952,6 +2954,7 @@ async fn guardian_review_session_config_clears_legacy_notify() {
|
||||
/*live_network_config*/ None,
|
||||
"active-model",
|
||||
/*reasoning_effort*/ None,
|
||||
/*model_messages*/ None,
|
||||
)
|
||||
.expect("guardian config");
|
||||
|
||||
@@ -2986,6 +2989,7 @@ async fn guardian_review_session_config_uses_live_network_proxy_state() {
|
||||
Some(live_network.clone()),
|
||||
"active-model",
|
||||
/*reasoning_effort*/ None,
|
||||
/*model_messages*/ None,
|
||||
)
|
||||
.expect("guardian config");
|
||||
|
||||
@@ -3028,6 +3032,7 @@ async fn guardian_review_session_config_disables_mcp_apps_plugins_and_memories()
|
||||
/*live_network_config*/ None,
|
||||
"active-model",
|
||||
/*reasoning_effort*/ None,
|
||||
/*model_messages*/ None,
|
||||
)
|
||||
.expect("guardian config");
|
||||
|
||||
@@ -3058,6 +3063,7 @@ async fn guardian_review_session_config_allows_pinned_disabled_feature() {
|
||||
/*live_network_config*/ None,
|
||||
"active-model",
|
||||
/*reasoning_effort*/ None,
|
||||
/*model_messages*/ None,
|
||||
)
|
||||
.expect("guardian config should continue when a disabled feature is pinned on");
|
||||
|
||||
@@ -3076,6 +3082,7 @@ async fn guardian_review_session_config_uses_parent_active_model_instead_of_hard
|
||||
/*live_network_config*/ None,
|
||||
"active-model",
|
||||
/*reasoning_effort*/ None,
|
||||
/*model_messages*/ None,
|
||||
)
|
||||
.expect("guardian config");
|
||||
|
||||
@@ -3094,6 +3101,7 @@ async fn guardian_review_session_config_keeps_bedrock_provider_for_bedrock_gpt_5
|
||||
/*live_network_config*/ None,
|
||||
AMAZON_BEDROCK_GPT_5_4_MODEL_ID,
|
||||
Some(ReasoningEffort::Low),
|
||||
/*model_messages*/ None,
|
||||
)
|
||||
.expect("guardian config");
|
||||
|
||||
@@ -3148,6 +3156,7 @@ async fn guardian_review_session_config_uses_requirements_guardian_policy_config
|
||||
/*live_network_config*/ None,
|
||||
"active-model",
|
||||
/*reasoning_effort*/ None,
|
||||
/*model_messages*/ None,
|
||||
)
|
||||
.expect("guardian config");
|
||||
|
||||
@@ -3186,6 +3195,7 @@ async fn guardian_review_session_config_uses_default_guardian_policy_without_req
|
||||
/*live_network_config*/ None,
|
||||
"active-model",
|
||||
/*reasoning_effort*/ None,
|
||||
/*model_messages*/ None,
|
||||
)
|
||||
.expect("guardian config");
|
||||
|
||||
|
||||
@@ -4,6 +4,8 @@ use anyhow::Result;
|
||||
use codex_core::config::Constrained;
|
||||
use codex_core::sandboxing::SandboxPermissions;
|
||||
use codex_protocol::config_types::ApprovalsReviewer;
|
||||
use codex_protocol::openai_models::AutoReviewMessages;
|
||||
use codex_protocol::openai_models::ModelsResponse;
|
||||
use codex_protocol::protocol::AskForApproval;
|
||||
use codex_protocol::protocol::EventMsg;
|
||||
use codex_protocol::protocol::Op;
|
||||
@@ -36,6 +38,20 @@ use tempfile::TempDir;
|
||||
async fn guardian_session_prewarms_and_is_reused_for_first_review() -> Result<()> {
|
||||
skip_if_no_network!(Ok(()));
|
||||
|
||||
let catalog_policy = "Use the catalog-provided Guardian policy.";
|
||||
let mut review_model = codex_models_manager::bundled_models_response()?
|
||||
.models
|
||||
.into_iter()
|
||||
.find(|model| model.slug == "codex-auto-review")
|
||||
.expect("bundled auto-review model");
|
||||
let model_messages = review_model
|
||||
.model_messages
|
||||
.as_mut()
|
||||
.expect("auto-review model messages");
|
||||
model_messages.auto_review = Some(AutoReviewMessages {
|
||||
policy: Some(catalog_policy.to_string()),
|
||||
});
|
||||
|
||||
let tool_args = json!({
|
||||
"cmd": "true",
|
||||
"sandbox_permissions": SandboxPermissions::RequireEscalated,
|
||||
@@ -56,7 +72,10 @@ async fn guardian_session_prewarms_and_is_reused_for_first_review() -> Result<()
|
||||
]],
|
||||
])
|
||||
.await;
|
||||
let mut builder = test_codex().with_config(|config| {
|
||||
let mut builder = test_codex().with_config(move |config| {
|
||||
config.model_catalog = Some(ModelsResponse {
|
||||
models: vec![review_model],
|
||||
});
|
||||
config.permissions.approval_policy = Constrained::allow_any(AskForApproval::OnRequest);
|
||||
config.approvals_reviewer = ApprovalsReviewer::AutoReview;
|
||||
});
|
||||
@@ -77,6 +96,11 @@ async fn guardian_session_prewarms_and_is_reused_for_first_review() -> Result<()
|
||||
})
|
||||
.expect("guardian startup prewarm request");
|
||||
assert_eq!(guardian_prewarm["generate"].as_bool(), Some(false));
|
||||
assert!(
|
||||
guardian_prewarm["instructions"]
|
||||
.as_str()
|
||||
.is_some_and(|instructions| instructions.contains(catalog_policy))
|
||||
);
|
||||
let guardian_thread_id = guardian_prewarm["client_metadata"]["thread_id"]
|
||||
.as_str()
|
||||
.expect("guardian thread id");
|
||||
|
||||
@@ -51,6 +51,7 @@ fn model_with_approval_messages(
|
||||
on_request: Some(on_request.to_string()),
|
||||
on_request_auto_review: Some(on_request_auto_review.to_string()),
|
||||
}),
|
||||
auto_review: None,
|
||||
});
|
||||
model
|
||||
}
|
||||
|
||||
@@ -564,6 +564,7 @@ async fn remote_model_friendly_personality_instructions_with_feature() -> anyhow
|
||||
personality_pragmatic: Some("Pragmatic variant".to_string()),
|
||||
}),
|
||||
approvals: None,
|
||||
auto_review: None,
|
||||
}),
|
||||
include_skills_usage_instructions: false,
|
||||
supports_reasoning_summaries: false,
|
||||
@@ -681,6 +682,7 @@ async fn user_turn_personality_remote_model_template_includes_update_message() -
|
||||
personality_pragmatic: Some(remote_pragmatic_message.to_string()),
|
||||
}),
|
||||
approvals: None,
|
||||
auto_review: None,
|
||||
}),
|
||||
include_skills_usage_instructions: false,
|
||||
supports_reasoning_summaries: false,
|
||||
|
||||
@@ -66,7 +66,7 @@ fn clear_instruction_messages(model: &mut ModelInfo) {
|
||||
if let Some(model_messages) = &mut model.model_messages {
|
||||
model_messages.instructions_template = None;
|
||||
model_messages.instructions_variables = None;
|
||||
if model_messages.approvals.is_none() {
|
||||
if model_messages.approvals.is_none() && model_messages.auto_review.is_none() {
|
||||
model.model_messages = None;
|
||||
}
|
||||
}
|
||||
@@ -130,6 +130,7 @@ fn local_personality_messages_for_slug(slug: &str) -> Option<ModelMessages> {
|
||||
personality_pragmatic: Some(LOCAL_PRAGMATIC_TEMPLATE.to_string()),
|
||||
}),
|
||||
approvals: None,
|
||||
auto_review: None,
|
||||
}),
|
||||
_ => None,
|
||||
}
|
||||
|
||||
@@ -1,6 +1,7 @@
|
||||
use super::*;
|
||||
use crate::ModelsManagerConfig;
|
||||
use codex_protocol::openai_models::ApprovalMessages;
|
||||
use codex_protocol::openai_models::AutoReviewMessages;
|
||||
use pretty_assertions::assert_eq;
|
||||
|
||||
#[test]
|
||||
@@ -60,6 +61,7 @@ fn base_instruction_override_preserves_catalog_approval_messages() {
|
||||
personality_pragmatic: Some("pragmatic".to_string()),
|
||||
}),
|
||||
approvals: Some(approvals.clone()),
|
||||
auto_review: None,
|
||||
});
|
||||
let config = ModelsManagerConfig {
|
||||
base_instructions: Some("override".to_string()),
|
||||
@@ -74,6 +76,7 @@ fn base_instruction_override_preserves_catalog_approval_messages() {
|
||||
instructions_template: None,
|
||||
instructions_variables: None,
|
||||
approvals: Some(approvals),
|
||||
auto_review: None,
|
||||
})
|
||||
);
|
||||
}
|
||||
@@ -89,6 +92,7 @@ fn disabled_personality_preserves_catalog_approval_messages() {
|
||||
instructions_template: Some("template".to_string()),
|
||||
instructions_variables: None,
|
||||
approvals: Some(approvals.clone()),
|
||||
auto_review: None,
|
||||
});
|
||||
let config = ModelsManagerConfig {
|
||||
personality_enabled: false,
|
||||
@@ -103,6 +107,37 @@ fn disabled_personality_preserves_catalog_approval_messages() {
|
||||
instructions_template: None,
|
||||
instructions_variables: None,
|
||||
approvals: Some(approvals),
|
||||
auto_review: None,
|
||||
})
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn base_instruction_override_preserves_catalog_auto_review_messages() {
|
||||
let mut model = model_info_from_slug("unknown-model");
|
||||
let auto_review = AutoReviewMessages {
|
||||
policy: Some("review policy".to_string()),
|
||||
};
|
||||
model.model_messages = Some(ModelMessages {
|
||||
instructions_template: Some("template".to_string()),
|
||||
instructions_variables: None,
|
||||
approvals: None,
|
||||
auto_review: Some(auto_review.clone()),
|
||||
});
|
||||
let config = ModelsManagerConfig {
|
||||
base_instructions: Some("override".to_string()),
|
||||
..Default::default()
|
||||
};
|
||||
|
||||
let updated = with_config_overrides(model, &config);
|
||||
|
||||
assert_eq!(
|
||||
updated.model_messages,
|
||||
Some(ModelMessages {
|
||||
instructions_template: None,
|
||||
instructions_variables: None,
|
||||
approvals: None,
|
||||
auto_review: Some(auto_review),
|
||||
})
|
||||
);
|
||||
}
|
||||
|
||||
@@ -486,6 +486,7 @@ pub struct ModelMessages {
|
||||
pub instructions_template: Option<String>,
|
||||
pub instructions_variables: Option<ModelInstructionsVariables>,
|
||||
pub approvals: Option<ApprovalMessages>,
|
||||
pub auto_review: Option<AutoReviewMessages>,
|
||||
}
|
||||
|
||||
#[derive(Debug, Serialize, Deserialize, Clone, PartialEq, Eq, TS, JsonSchema)]
|
||||
@@ -494,6 +495,11 @@ pub struct ApprovalMessages {
|
||||
pub on_request_auto_review: Option<String>,
|
||||
}
|
||||
|
||||
#[derive(Debug, Serialize, Deserialize, Clone, PartialEq, Eq, TS, JsonSchema)]
|
||||
pub struct AutoReviewMessages {
|
||||
pub policy: Option<String>,
|
||||
}
|
||||
|
||||
impl ModelMessages {
|
||||
fn has_personality_placeholder(&self) -> bool {
|
||||
self.instructions_template
|
||||
@@ -814,6 +820,7 @@ mod tests {
|
||||
instructions_template: Some("Hello {{ personality }}".to_string()),
|
||||
instructions_variables: Some(personality_variables()),
|
||||
approvals: None,
|
||||
auto_review: None,
|
||||
}));
|
||||
|
||||
let instructions = model.get_model_instructions(Some(Personality::Friendly));
|
||||
@@ -831,6 +838,7 @@ mod tests {
|
||||
personality_pragmatic: None,
|
||||
}),
|
||||
approvals: None,
|
||||
auto_review: None,
|
||||
}));
|
||||
assert_eq!(
|
||||
model.get_model_instructions(Some(Personality::Friendly)),
|
||||
@@ -857,6 +865,7 @@ mod tests {
|
||||
personality_pragmatic: None,
|
||||
}),
|
||||
approvals: None,
|
||||
auto_review: None,
|
||||
}));
|
||||
assert_eq!(
|
||||
model_no_personality.get_model_instructions(Some(Personality::Friendly)),
|
||||
@@ -886,6 +895,7 @@ mod tests {
|
||||
personality_pragmatic: None,
|
||||
}),
|
||||
approvals: None,
|
||||
auto_review: None,
|
||||
}));
|
||||
|
||||
let instructions = model.get_model_instructions(Some(Personality::Friendly));
|
||||
|
||||
Reference in New Issue
Block a user