diff --git a/codex-rs/core/src/guardian/review.rs b/codex-rs/core/src/guardian/review.rs index e606c01d09..0ab61056b0 100644 --- a/codex-rs/core/src/guardian/review.rs +++ b/codex-rs/core/src/guardian/review.rs @@ -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, diff --git a/codex-rs/core/src/guardian/review_session.rs b/codex-rs/core/src/guardian/review_session.rs index de56de41a6..02136f2e2e 100644 --- a/codex-rs/core/src/guardian/review_session.rs +++ b/codex-rs/core/src/guardian/review_session.rs @@ -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, active_model: &str, reasoning_effort: Option, + model_messages: Option<&ModelMessages>, ) -> anyhow::Result { 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( diff --git a/codex-rs/core/src/guardian/tests.rs b/codex-rs/core/src/guardian/tests.rs index 7dc4d8544e..349a4f59ed 100644 --- a/codex-rs/core/src/guardian/tests.rs +++ b/codex-rs/core/src/guardian/tests.rs @@ -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"); diff --git a/codex-rs/core/tests/suite/guardian_review.rs b/codex-rs/core/tests/suite/guardian_review.rs index 383954b816..b8d79a2cf0 100644 --- a/codex-rs/core/tests/suite/guardian_review.rs +++ b/codex-rs/core/tests/suite/guardian_review.rs @@ -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"); diff --git a/codex-rs/core/tests/suite/permissions_messages.rs b/codex-rs/core/tests/suite/permissions_messages.rs index 7f42ba893c..6503c0407f 100644 --- a/codex-rs/core/tests/suite/permissions_messages.rs +++ b/codex-rs/core/tests/suite/permissions_messages.rs @@ -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 } diff --git a/codex-rs/core/tests/suite/personality.rs b/codex-rs/core/tests/suite/personality.rs index a493a8be07..a979f7de41 100644 --- a/codex-rs/core/tests/suite/personality.rs +++ b/codex-rs/core/tests/suite/personality.rs @@ -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, diff --git a/codex-rs/models-manager/src/model_info.rs b/codex-rs/models-manager/src/model_info.rs index 75d333efe3..2e48eff34f 100644 --- a/codex-rs/models-manager/src/model_info.rs +++ b/codex-rs/models-manager/src/model_info.rs @@ -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 { personality_pragmatic: Some(LOCAL_PRAGMATIC_TEMPLATE.to_string()), }), approvals: None, + auto_review: None, }), _ => None, } diff --git a/codex-rs/models-manager/src/model_info_tests.rs b/codex-rs/models-manager/src/model_info_tests.rs index 2240f0a391..4576ef2363 100644 --- a/codex-rs/models-manager/src/model_info_tests.rs +++ b/codex-rs/models-manager/src/model_info_tests.rs @@ -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), }) ); } diff --git a/codex-rs/protocol/src/openai_models.rs b/codex-rs/protocol/src/openai_models.rs index 7e7c637aa0..f54e328b86 100644 --- a/codex-rs/protocol/src/openai_models.rs +++ b/codex-rs/protocol/src/openai_models.rs @@ -486,6 +486,7 @@ pub struct ModelMessages { pub instructions_template: Option, pub instructions_variables: Option, pub approvals: Option, + pub auto_review: Option, } #[derive(Debug, Serialize, Deserialize, Clone, PartialEq, Eq, TS, JsonSchema)] @@ -494,6 +495,11 @@ pub struct ApprovalMessages { pub on_request_auto_review: Option, } +#[derive(Debug, Serialize, Deserialize, Clone, PartialEq, Eq, TS, JsonSchema)] +pub struct AutoReviewMessages { + pub policy: Option, +} + 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));