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
This commit is contained in:
olliem-oai
2026-08-31 22:08:44 +00:00
committed by copyberry
parent 746798b2f7
commit 865bbf9a69
11 changed files with 144 additions and 24 deletions
@@ -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()
}
}
+6
View File
@@ -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<codex_protocol::openai_models::ReasoningEffort>,
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,
+20 -7
View File
@@ -107,6 +107,7 @@ pub(crate) struct GuardianReviewSessionParams {
pub(crate) parent_session: Arc<Session>,
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<String>,
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<ResponseItem>
@@ -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(&params.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 = &params.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())
@@ -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(&params.node_repl_policy);
let manager = Arc::new(GuardianReviewSessionManager {
state: Arc::new(Mutex::new(GuardianReviewSessionState {
trunk: Some(Arc::new(review_session)),
@@ -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());
@@ -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,
@@ -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,
};
@@ -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,
});
+49 -12
View File
@@ -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::<Vec<_>>();
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<_>>(),
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),
});
@@ -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()),
};
+7
View File
@@ -623,6 +623,9 @@ pub struct CollaborationModeMessages {
pub struct AutoReviewMessages {
pub policy: Option<String>,
pub policy_template: Option<String>,
/// Extra developer policy for `node_repl` and `cua_repl` reviews.
#[serde(default, skip_serializing_if = "Option::is_none")]
pub node_repl_policy: Option<String>,
pub rejection_instructions: Option<String>,
pub timeout_instructions: Option<String>,
}
@@ -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()),
}),