From 865bbf9a69467e2321daa2252c775b0336e9d454 Mon Sep 17 00:00:00 2001 From: olliem-oai Date: Mon, 31 Aug 2026 21:32:10 +0000 Subject: [PATCH] Source Guardian REPL policy from model metadata (#41919) ## What changed - Add `auto_review.node_repl_policy` to model messages for `node_repl` and `cua_repl` reviews. - Use the reviewer model's configured policy, falling back to the bundled policy when the field is absent and skipping injection when it is explicitly empty. - Include the effective policy in Guardian session reuse checks and reject unsafe parent-model fallback transitions that would change it. ## Testing - Cover catalog, bundled, and explicitly empty policies across Node and CUA review paths. - Verify that policy changes invalidate cached reviewer sessions and incompatible fallback transitions. GitOrigin-RevId: 51c0bdfed9b464091b6eb47d91ca5a0990d7a315 --- .../src/context/guardian_node_repl_policy.rs | 21 ++++++- codex-rs/core/src/guardian/review.rs | 6 ++ codex-rs/core/src/guardian/review_session.rs | 27 +++++--- .../core/src/guardian/review_session_tests.rs | 24 +++++++- codex-rs/core/src/session/step_activation.rs | 12 ++++ .../core/src/session/step_activation_tests.rs | 7 +++ .../core/src/session/step_settings_tests.rs | 1 + .../tests/suite/guardian_mcp_elicitation.rs | 1 + codex-rs/core/tests/suite/guardian_review.rs | 61 +++++++++++++++---- .../models-manager/src/model_info_tests.rs | 1 + codex-rs/protocol/src/openai_models.rs | 7 +++ 11 files changed, 144 insertions(+), 24 deletions(-) diff --git a/codex-rs/core/src/context/guardian_node_repl_policy.rs b/codex-rs/core/src/context/guardian_node_repl_policy.rs index 7e465b6758..c05ed242f1 100644 --- a/codex-rs/core/src/context/guardian_node_repl_policy.rs +++ b/codex-rs/core/src/context/guardian_node_repl_policy.rs @@ -1,8 +1,23 @@ use super::ContextualUserFragment; use codex_protocol::models::ContentItemKind; +use codex_protocol::openai_models::ModelMessages; -#[derive(Debug, Clone, Copy, PartialEq, Eq)] -pub(crate) struct GuardianNodeReplPolicy; +#[derive(Debug, Clone, PartialEq, Eq)] +pub(crate) struct GuardianNodeReplPolicy { + policy: String, +} + +impl GuardianNodeReplPolicy { + pub(crate) fn from_model_messages(messages: Option<&ModelMessages>) -> Self { + let policy = messages + .and_then(|messages| messages.auto_review.as_ref()) + .and_then(|messages| messages.node_repl_policy.as_deref()) + .unwrap_or(include_str!("../../assets/guardian/node_repl_policy.md")); + Self { + policy: policy.to_string(), + } + } +} impl ContextualUserFragment for GuardianNodeReplPolicy { fn content_kind(&self) -> ContentItemKind { @@ -22,6 +37,6 @@ impl ContextualUserFragment for GuardianNodeReplPolicy { } fn body(&self) -> String { - include_str!("../../assets/guardian/node_repl_policy.md").to_string() + self.policy.clone() } } diff --git a/codex-rs/core/src/guardian/review.rs b/codex-rs/core/src/guardian/review.rs index 2882cb799f..6f299c62a9 100644 --- a/codex-rs/core/src/guardian/review.rs +++ b/codex-rs/core/src/guardian/review.rs @@ -33,6 +33,7 @@ use tokio::time::Instant; use tokio::time::sleep_until; use tokio_util::sync::CancellationToken; +use crate::context::GuardianNodeReplPolicy; use crate::context::GuardianReviewEvidence; use crate::session::session::Session; use crate::session::turn_context::TurnContext; @@ -822,6 +823,7 @@ pub(crate) fn spawn_approval_request_review( pub(super) struct GuardianReviewSessionConfig { pub(super) spawn_config: crate::config::Config, + pub(super) node_repl_policy: GuardianNodeReplPolicy, model: String, reasoning_effort: Option, default_review_model_id: String, @@ -923,6 +925,9 @@ pub(super) async fn guardian_review_session_config( } Ok(GuardianReviewSessionConfig { spawn_config, + node_repl_policy: GuardianNodeReplPolicy::from_model_messages( + guardian_model_info.model_messages.as_ref(), + ), model: guardian_model, reasoning_effort: guardian_reasoning_effort, default_review_model_id: default_review_model_id.to_string(), @@ -973,6 +978,7 @@ async fn run_guardian_review_session_before_deadline( parent_session: Arc::clone(&session), parent_context: context.clone(), spawn_config: session_config.spawn_config, + node_repl_policy: session_config.node_repl_policy, request, reasons, schema, diff --git a/codex-rs/core/src/guardian/review_session.rs b/codex-rs/core/src/guardian/review_session.rs index 8cd0095ecf..330941c0d5 100644 --- a/codex-rs/core/src/guardian/review_session.rs +++ b/codex-rs/core/src/guardian/review_session.rs @@ -107,6 +107,7 @@ pub(crate) struct GuardianReviewSessionParams { pub(crate) parent_session: Arc, pub(crate) parent_context: GuardianReviewContext, pub(crate) spawn_config: Config, + pub(crate) node_repl_policy: GuardianNodeReplPolicy, pub(crate) request: GuardianApprovalRequest, pub(crate) reasons: ApprovalRequestReasons, pub(crate) schema: Value, @@ -193,6 +194,7 @@ struct GuardianReviewSessionReuseKey { // history rewrites that invalidate existing reviewer context. parent_history_version: u64, node_repl_auto_review_required: bool, + node_repl_policy: String, model: Option, model_provider_id: String, model_provider: ModelProviderInfo, @@ -231,6 +233,7 @@ impl GuardianReviewSessionReuseKey { 0 }, node_repl_auto_review_required: false, + node_repl_policy: String::new(), model: spawn_config.model.clone(), model_provider_id: spawn_config.model_provider_id.clone(), model_provider: spawn_config.model_provider.clone(), @@ -267,6 +270,11 @@ impl GuardianReviewSessionReuseKey { self.node_repl_auto_review_required = required; self } + + fn with_node_repl_policy(mut self, policy: &GuardianNodeReplPolicy) -> Self { + self.node_repl_policy = policy.body(); + self + } } fn encrypted_parent_compaction<'a, I>(items: I) -> Option @@ -420,9 +428,9 @@ impl GuardianReviewSessionManager { ) -> BoxFuture<'_, anyhow::Result<()>> { // Boxing breaks the Session::new -> Guardian -> Session::new future recursion. Box::pin(async move { - let spawn_config = guardian_review_session_config(&parent_session, &parent_turn) - .await? - .spawn_config; + let session_config = + guardian_review_session_config(&parent_session, &parent_turn).await?; + let spawn_config = session_config.spawn_config; let parent_history = parent_session.clone_history().await; let parent_compaction = spawn_config .features @@ -441,7 +449,8 @@ impl GuardianReviewSessionManager { .turn() .model_info() .node_repl_auto_review_required, - ); + ) + .with_node_repl_policy(&session_config.node_repl_policy); let spawn_cancel_token = self.cancellation_token.child_token(); let spawn_cancel_guard = spawn_cancel_token.clone().drop_guard(); let review_session = spawn_guardian_review_session( @@ -531,7 +540,8 @@ impl GuardianReviewSessionManager { .turn() .model_info() .node_repl_auto_review_required, - ); + ) + .with_node_repl_policy(¶ms.node_repl_policy); let mut spawned_trunk = false; let trunk_candidate = match run_before_review_deadline( deadline, @@ -1328,8 +1338,11 @@ async fn ensure_guardian_node_repl_policy( return Ok(()); } - let policy = GuardianNodeReplPolicy; + let policy = ¶ms.node_repl_policy; let policy_body = policy.body(); + if policy_body.is_empty() { + return Ok(()); + } let already_injected = review_session .session .clone_history() @@ -1367,7 +1380,7 @@ async fn ensure_guardian_node_repl_policy( initialize_context.await?; } - let item: ResponseItem = ContextualUserFragment::into(policy); + let item: ResponseItem = ContextualUserFragment::into(policy.clone()); review_session .session .inject_client_response_items(vec![item], turn_context.as_ref()) diff --git a/codex-rs/core/src/guardian/review_session_tests.rs b/codex-rs/core/src/guardian/review_session_tests.rs index 7399adc136..0b0c6b6f40 100644 --- a/codex-rs/core/src/guardian/review_session_tests.rs +++ b/codex-rs/core/src/guardian/review_session_tests.rs @@ -100,6 +100,7 @@ async fn test_review_params() -> GuardianReviewSessionParams { parent_session: Arc::new(session), parent_context: GuardianReviewContext::from(Arc::new(turn)), spawn_config, + node_repl_policy: GuardianNodeReplPolicy::from_model_messages(/*messages*/ None), request: GuardianApprovalRequest::ExecCommand { id: "shell-1".to_string(), command: vec!["git".to_string(), "status".to_string()], @@ -212,9 +213,24 @@ async fn guardian_review_session_config_change_invalidates_cached_session() { cached_reuse_key .clone() .with_node_repl_policy_eligibility(/*required*/ false), - cached_reuse_key.with_node_repl_policy_eligibility(/*required*/ true), + cached_reuse_key + .clone() + .with_node_repl_policy_eligibility(/*required*/ true), "switching parent-model Node REPL eligibility must invalidate reviewer history" ); + let messages = serde_json::from_value(serde_json::json!({ + "auto_review": { "node_repl_policy": "Catalog REPL policy." } + })) + .expect("catalog model messages"); + assert_ne!( + cached_reuse_key.clone().with_node_repl_policy( + &GuardianNodeReplPolicy::from_model_messages(/*messages*/ None), + ), + cached_reuse_key.with_node_repl_policy(&GuardianNodeReplPolicy::from_model_messages(Some( + &messages + )),), + "changing the effective Node REPL policy must invalidate reviewer history" + ); let mut compaction_enabled_config = cached_spawn_config; compaction_enabled_config @@ -385,6 +401,7 @@ async fn guardian_review_session_config_prefers_managed_policy_and_uses_catalog_ auto_review: Some(AutoReviewMessages { policy: Some("Use the catalog Guardian policy.".to_string()), policy_template: Some(catalog_template.to_string()), + node_repl_policy: None, rejection_instructions: None, timeout_instructions: None, }), @@ -426,6 +443,7 @@ async fn guardian_review_session_config_preserves_explicit_empty_catalog_policy( auto_review: Some(AutoReviewMessages { policy: Some(String::new()), policy_template: None, + node_repl_policy: None, rejection_instructions: None, timeout_instructions: None, }), @@ -475,6 +493,7 @@ async fn guardian_review_session_config_preserves_explicit_empty_catalog_templat auto_review: Some(AutoReviewMessages { policy: Some(catalog_policy.to_string()), policy_template: Some(String::new()), + node_repl_policy: None, rejection_instructions: None, timeout_instructions: None, }), @@ -721,7 +740,8 @@ async fn run_review_removes_trunk_when_event_stream_is_broken() { .await .history_version(), ) - .with_environments(params.parent_context.environments()); + .with_environments(params.parent_context.environments()) + .with_node_repl_policy(¶ms.node_repl_policy); let manager = Arc::new(GuardianReviewSessionManager { state: Arc::new(Mutex::new(GuardianReviewSessionState { trunk: Some(Arc::new(review_session)), diff --git a/codex-rs/core/src/session/step_activation.rs b/codex-rs/core/src/session/step_activation.rs index ffbe0ebd19..6bd3a5c530 100644 --- a/codex-rs/core/src/session/step_activation.rs +++ b/codex-rs/core/src/session/step_activation.rs @@ -8,6 +8,7 @@ use super::step_settings::StepSettingsUpdate; use super::turn_context::TurnContext; use crate::config::Config; use crate::config::ConstraintResult; +use crate::context::GuardianNodeReplPolicy; use crate::exec_policy::AllowPrefixRules; use crate::guardian::BUNDLED_GUARDIAN_POLICY_TEMPLATE; use codex_features::Feature; @@ -173,6 +174,17 @@ fn check_legacy_model_safety( // catalog refresh. V1 uses the admitted config; V2 can use the live config. // An unchanged explicit reviewer override prevents both fallback paths. if destination.auto_review_model_override.is_none() { + let destination_node_repl_policy = + GuardianNodeReplPolicy::from_model_messages(destination.model_messages.as_ref()); + for model in retained_models { + let policy = GuardianNodeReplPolicy::from_model_messages(model.model_messages.as_ref()); + if policy != destination_node_repl_policy { + return Err( + "the destination changes the Guardian parent-fallback node REPL policy" + .to_string(), + ); + } + } for config in [admitted_config, live_config] { let destination_policy = config.resolve_guardian_policy(destination.model_messages.as_ref()); diff --git a/codex-rs/core/src/session/step_activation_tests.rs b/codex-rs/core/src/session/step_activation_tests.rs index c3ca3b572a..cd34233e98 100644 --- a/codex-rs/core/src/session/step_activation_tests.rs +++ b/codex-rs/core/src/session/step_activation_tests.rs @@ -861,6 +861,7 @@ fn parent_review_messages(model: &mut ModelInfo) -> &mut AutoReviewMessages { .get_or_insert(AutoReviewMessages { policy: None, policy_template: None, + node_repl_policy: None, rejection_instructions: None, timeout_instructions: None, }) @@ -996,6 +997,11 @@ async fn parent_fallback_preserves_explicit_empty_and_bundled_defaults() { ); parent_review_messages(&mut destination).policy = None; assert_eq!(check(&destination), Ok(())); + parent_review_messages(&mut destination).node_repl_policy = Some(String::new()); + assert_eq!( + check(&destination), + Err("the destination changes the Guardian parent-fallback node REPL policy".to_string()) + ); } #[tokio::test] @@ -1008,6 +1014,7 @@ async fn unchanged_explicit_reviewer_does_not_use_parent_policy() { parent_review_messages(&mut admitted).policy = Some("catalog policy A".to_string()); parent_review_messages(&mut destination).policy = Some("catalog policy B".to_string()); parent_review_messages(&mut destination).policy_template = Some(String::new()); + parent_review_messages(&mut destination).node_repl_policy = Some(String::new()); assert_eq!( check_legacy_model_safety( &admitted, diff --git a/codex-rs/core/src/session/step_settings_tests.rs b/codex-rs/core/src/session/step_settings_tests.rs index eb75b3c730..ba04038947 100644 --- a/codex-rs/core/src/session/step_settings_tests.rs +++ b/codex-rs/core/src/session/step_settings_tests.rs @@ -411,6 +411,7 @@ async fn model_resolution_preserves_startup_overrides_and_instruction_provenance let auto_review = AutoReviewMessages { policy: Some("catalog review policy".to_string()), policy_template: Some("catalog review template".to_string()), + node_repl_policy: None, rejection_instructions: None, timeout_instructions: None, }; diff --git a/codex-rs/core/tests/suite/guardian_mcp_elicitation.rs b/codex-rs/core/tests/suite/guardian_mcp_elicitation.rs index 5ab710bd59..daa5b54ad3 100644 --- a/codex-rs/core/tests/suite/guardian_mcp_elicitation.rs +++ b/codex-rs/core/tests/suite/guardian_mcp_elicitation.rs @@ -243,6 +243,7 @@ async fn server_initiated_mcp_elicitation_can_require_synchronous_auto_review( .auto_review = Some(AutoReviewMessages { policy: None, policy_template: None, + node_repl_policy: None, rejection_instructions: Some(REJECTION_INSTRUCTIONS.to_string()), timeout_instructions: None, }); diff --git a/codex-rs/core/tests/suite/guardian_review.rs b/codex-rs/core/tests/suite/guardian_review.rs index 49f52c014c..b1940bf262 100644 --- a/codex-rs/core/tests/suite/guardian_review.rs +++ b/codex-rs/core/tests/suite/guardian_review.rs @@ -655,15 +655,19 @@ async fn guardian_session_prewarms_and_is_reused_for_first_review( } #[tokio::test(flavor = "multi_thread", worker_threads = 2)] -#[test_case("first_node", "node_repl"; "injects_policy_for_first_node_action")] -#[test_case("shell_then_nodes", "node_repl"; "reuses_shell_reviewer_and_injects_policy_once")] -#[test_case("ineligible_node", "node_repl"; "omits_policy_for_ineligible_parent_model")] -#[test_case("first_node", "cua_repl"; "injects_policy_for_first_cua_action")] -#[test_case("shell_then_nodes", "cua_repl"; "reuses_shell_reviewer_and_injects_cua_policy_once")] -#[test_case("ineligible_node", "cua_repl"; "omits_cua_policy_for_ineligible_parent_model")] +#[test_case("first_node", "node_repl", None; "injects_policy_for_first_node_action")] +#[test_case("shell_then_nodes", "node_repl", None; "reuses_shell_reviewer_and_injects_policy_once")] +#[test_case("ineligible_node", "node_repl", None; "omits_policy_for_ineligible_parent_model")] +#[test_case("first_node", "cua_repl", None; "injects_policy_for_first_cua_action")] +#[test_case("shell_then_nodes", "cua_repl", None; "reuses_shell_reviewer_and_injects_cua_policy_once")] +#[test_case("ineligible_node", "cua_repl", None; "omits_cua_policy_for_ineligible_parent_model")] +#[test_case("shell_then_nodes", "node_repl", Some("Catalog REPL policy."); "catalog_node_policy")] +#[test_case("shell_then_nodes", "cua_repl", Some("Catalog REPL policy."); "catalog_cua_policy")] +#[test_case("first_node", "node_repl", Some(""); "empty_catalog_policy")] async fn guardian_node_repl_policy_follows_production_approval_path( scenario: &str, repl_server: &'static str, + node_repl_policy: Option<&'static str>, ) -> Result<()> { skip_if_no_network!(Ok(())); skip_if_sandbox!(Ok(())); @@ -676,8 +680,33 @@ async fn guardian_node_repl_policy_follows_production_approval_path( let mcp_server_bin = remote_aware_stdio_server_bin()?; let node_repl_auto_review_required = scenario != "ineligible_node"; let mut builder = test_codex() + .with_model_info_override("gpt-5.6-luna", move |model| { + model + .model_messages + .as_mut() + .expect("reviewer model messages") + .auto_review = Some(AutoReviewMessages { + policy: None, + policy_template: None, + node_repl_policy: node_repl_policy.map(str::to_string), + rejection_instructions: None, + timeout_instructions: None, + }); + }) .with_model_info_override("gpt-5.4", move |model| { model.node_repl_auto_review_required = node_repl_auto_review_required; + model.auto_review_model_override = Some("gpt-5.6-luna".to_string()); + model + .model_messages + .as_mut() + .expect("acting model messages") + .auto_review = Some(AutoReviewMessages { + policy: None, + policy_template: None, + node_repl_policy: Some("Acting-model REPL policy.".to_string()), + rejection_instructions: None, + timeout_instructions: None, + }); }) .with_config(move |config| { config.permissions.approval_policy = Constrained::allow_any(AskForApproval::OnRequest); @@ -754,7 +783,8 @@ async fn guardian_node_repl_policy_follows_production_approval_path( .collect::>(); assert_eq!(guardian_requests.len(), actions.len()); - let policy = include_str!("../../assets/guardian/node_repl_policy.md"); + let bundled_policy = include_str!("../../assets/guardian/node_repl_policy.md"); + let policy = node_repl_policy.unwrap_or(bundled_policy); let first_guardian_thread = guardian_requests[0].body_json()["client_metadata"]["thread_id"] .as_str() .expect("Guardian reviewer thread id") @@ -766,15 +796,19 @@ async fn guardian_node_repl_policy_follows_production_approval_path( "shell and Node approvals must reuse the same Guardian reviewer" ); let expected = usize::from( - node_repl_auto_review_required && !(scenario == "shell_then_nodes" && index == 0), + node_repl_auto_review_required + && !policy.is_empty() + && !(scenario == "shell_then_nodes" && index == 0), ); assert_eq!( request .message_input_texts("developer") - .iter() - .filter(|text| text.as_str() == policy) - .count(), - expected, + .into_iter() + .filter(|text| { + [bundled_policy, "Acting-model REPL policy.", policy].contains(&text.as_str()) + }) + .collect::>(), + vec![policy.to_string(); expected], "Node REPL policy must appear exactly once on eligible Node reviews" ); if expected == 1 && (index == 0 || scenario == "shell_then_nodes" && index == 1) { @@ -1243,6 +1277,7 @@ async fn guardian_denial_rejects_tool_call_with_rationale( .auto_review = Some(AutoReviewMessages { policy: None, policy_template: None, + node_repl_policy: None, rejection_instructions: Some("Reviewer-only rejection instructions.".to_string()), timeout_instructions: None, }); @@ -1256,6 +1291,7 @@ async fn guardian_denial_rejects_tool_call_with_rationale( .auto_review = Some(AutoReviewMessages { policy: None, policy_template: None, + node_repl_policy: None, rejection_instructions: rejection_instructions.map(str::to_string), timeout_instructions: None, }); @@ -1404,6 +1440,7 @@ async fn guardian_timeout_rejects_tool_call_with_acting_model_instructions( .auto_review = Some(AutoReviewMessages { policy: None, policy_template: None, + node_repl_policy: None, rejection_instructions: None, timeout_instructions: timeout_instructions.map(str::to_string), }); diff --git a/codex-rs/models-manager/src/model_info_tests.rs b/codex-rs/models-manager/src/model_info_tests.rs index 921dd58189..e57a5ab70d 100644 --- a/codex-rs/models-manager/src/model_info_tests.rs +++ b/codex-rs/models-manager/src/model_info_tests.rs @@ -42,6 +42,7 @@ fn base_instruction_override_is_literal_and_preserves_catalog_messages() { let auto_review = AutoReviewMessages { policy: Some("review policy".to_string()), policy_template: Some("review policy template".to_string()), + node_repl_policy: None, rejection_instructions: Some("rejection instructions".to_string()), timeout_instructions: Some(String::new()), }; diff --git a/codex-rs/protocol/src/openai_models.rs b/codex-rs/protocol/src/openai_models.rs index 2e3104e864..c942db4d9a 100644 --- a/codex-rs/protocol/src/openai_models.rs +++ b/codex-rs/protocol/src/openai_models.rs @@ -623,6 +623,9 @@ pub struct CollaborationModeMessages { pub struct AutoReviewMessages { pub policy: Option, pub policy_template: Option, + /// Extra developer policy for `node_repl` and `cua_repl` reviews. + #[serde(default, skip_serializing_if = "Option::is_none")] + pub node_repl_policy: Option, pub rejection_instructions: Option, pub timeout_instructions: Option, } @@ -1139,6 +1142,7 @@ mod tests { "auto_review": { "policy": "policy", "policy_template": "", + "node_repl_policy": "", "rejection_instructions": "", "timeout_instructions": "" } @@ -1151,6 +1155,7 @@ mod tests { Some(AutoReviewMessages { policy: Some("policy".to_string()), policy_template: None, + node_repl_policy: None, rejection_instructions: None, timeout_instructions: None, }) @@ -1160,6 +1165,7 @@ mod tests { Some(AutoReviewMessages { policy: Some("policy".to_string()), policy_template: Some(String::new()), + node_repl_policy: Some(String::new()), rejection_instructions: Some(String::new()), timeout_instructions: Some(String::new()), }) @@ -1559,6 +1565,7 @@ mod tests { auto_review: Some(AutoReviewMessages { policy: Some("policy".to_string()), policy_template: None, + node_repl_policy: None, rejection_instructions: Some("rejection instructions".to_string()), timeout_instructions: Some("timeout instructions".to_string()), }),