Use captured action settings for Guardian reviews (#44574)

## Why

Model changes during a turn and catalog refreshes can leave Guardian reviews using settings or metadata that do not match the action being reviewed.

## What changed

- Use the action's captured model, reasoning effort, reasoning summary, and personality when configuring reviews.
- Preserve captured model metadata when falling back to the parent model, while resolving a separate reviewer against the current catalog on each attempt.
- Evaluate live managed review requirements against the action's model and use its metadata for computer-use review behavior.
- Include personality in the review session reuse key so personality changes invalidate cached sessions.

## Testing

Add regression coverage for delayed and new actions across model changes and catalog refreshes, required-review routing, and session reuse when policy or personality changes. Extend integration tests to cover reviewer reuse across parent model changes and verify that interrupted reviews cannot execute commands after a delayed approval arrives.

GitOrigin-RevId: 5b434568e113e44bf7177780f2ae13192b55e942
This commit is contained in:
Abhinav authored and copyberry committed 2026-09-10 16:17:25 +00:00
1 parent 6baa076eb6
commit 6bb5be869f
10 files changed
+667 -161

No files matched your search

+3 -2
View File
@@ -59,9 +59,10 @@ pub(crate) async fn decide_approval(
let context = context.into(); let context = context.into();
let request = request.into(); let request = request.into();
let turn = context.turn(); let turn = context.turn();
let requirements = turn.config.config_layer_stack.requirements(); let live_config = session.get_config().await;
let requirements = live_config.config_layer_stack.requirements();
let model_requires_review = let model_requires_review =
requirements.auto_review_required_for_model(&turn.model_info().slug); requirements.auto_review_required_for_model(&context.model_info.slug);
let require_guardian = options.require_guardian let require_guardian = options.require_guardian
|| model_requires_review || model_requires_review
|| requirements || requirements
+5 -4
View File
@@ -23,6 +23,7 @@ mod runtime;
use std::sync::Arc; use std::sync::Arc;
use codex_protocol::config_types::ApprovalsReviewer; use codex_protocol::config_types::ApprovalsReviewer;
use codex_protocol::config_types::Personality;
use codex_protocol::config_types::ReasoningSummary; use codex_protocol::config_types::ReasoningSummary;
use codex_protocol::openai_models::ModelInfo; use codex_protocol::openai_models::ModelInfo;
use codex_protocol::openai_models::ReasoningEffort; use codex_protocol::openai_models::ReasoningEffort;
@@ -81,13 +82,10 @@ pub(crate) struct GuardianReviewContext {
pub(crate) parent_response_id: Option<String>, pub(crate) parent_response_id: Option<String>,
turn: Arc<TurnContext>, turn: Arc<TurnContext>,
environments: TurnEnvironmentSnapshot, environments: TurnEnvironmentSnapshot,
// Model and reasoning inputs are carried for the follow-up Guardian and V2 migrations.
#[expect(dead_code)]
pub(crate) model_info: Arc<ModelInfo>, pub(crate) model_info: Arc<ModelInfo>,
#[expect(dead_code)]
pub(crate) reasoning_effort: Option<ReasoningEffort>, pub(crate) reasoning_effort: Option<ReasoningEffort>,
#[expect(dead_code)]
pub(crate) reasoning_summary: ReasoningSummary, pub(crate) reasoning_summary: ReasoningSummary,
pub(crate) personality: Option<Personality>,
pub(crate) approval_policy: AskForApproval, pub(crate) approval_policy: AskForApproval,
pub(crate) approvals_reviewer: ApprovalsReviewer, pub(crate) approvals_reviewer: ApprovalsReviewer,
} }
@@ -106,6 +104,7 @@ impl GuardianReviewContext {
model_info: Arc::clone(&settings.model_info), model_info: Arc::clone(&settings.model_info),
reasoning_effort: settings.reasoning_effort().cloned(), reasoning_effort: settings.reasoning_effort().cloned(),
reasoning_summary: settings.reasoning_summary, reasoning_summary: settings.reasoning_summary,
personality: settings.personality(),
approval_policy: settings.approval_policy(), approval_policy: settings.approval_policy(),
approvals_reviewer: settings.approvals_reviewer(), approvals_reviewer: settings.approvals_reviewer(),
turn, turn,
@@ -134,6 +133,7 @@ impl From<&Arc<StepContext>> for GuardianReviewContext {
model_info: Arc::clone(&step.settings.model_info), model_info: Arc::clone(&step.settings.model_info),
reasoning_effort: step.settings.reasoning_effort().cloned(), reasoning_effort: step.settings.reasoning_effort().cloned(),
reasoning_summary: step.settings.reasoning_summary, reasoning_summary: step.settings.reasoning_summary,
personality: step.settings.personality(),
approval_policy: step.settings.approval_policy(), approval_policy: step.settings.approval_policy(),
approvals_reviewer: step.settings.approvals_reviewer(), approvals_reviewer: step.settings.approvals_reviewer(),
} }
@@ -151,6 +151,7 @@ impl From<Arc<TurnContext>> for GuardianReviewContext {
model_info: Arc::clone(turn.model_info()), model_info: Arc::clone(turn.model_info()),
reasoning_effort: turn.reasoning_effort().cloned(), reasoning_effort: turn.reasoning_effort().cloned(),
reasoning_summary: turn.reasoning_summary(), reasoning_summary: turn.reasoning_summary(),
personality: turn.personality(),
approval_policy: turn.approval_policy(), approval_policy: turn.approval_policy(),
approvals_reviewer: turn.config.approvals_reviewer, approvals_reviewer: turn.config.approvals_reviewer,
turn, turn,
+28 -18
View File
@@ -213,8 +213,9 @@ pub(super) struct GuardianReviewSessionConfig {
pub(super) async fn guardian_review_session_config( pub(super) async fn guardian_review_session_config(
session: &Session, session: &Session,
turn: &TurnContext, context: &GuardianReviewContext,
) -> anyhow::Result<GuardianReviewSessionConfig> { ) -> anyhow::Result<GuardianReviewSessionConfig> {
let turn = context.turn();
let network_proxy = session.services.network_proxy.load_full(); let network_proxy = session.services.network_proxy.load_full();
let live_network_config = match network_proxy.as_ref() { let live_network_config = match network_proxy.as_ref() {
Some(network_proxy) => Some(network_proxy.proxy().current_cfg().await?), Some(network_proxy) => Some(network_proxy.proxy().current_cfg().await?),
@@ -237,28 +238,39 @@ pub(super) async fn guardian_review_session_config(
model_overridden: guardian_review_model_overridden, model_overridden: guardian_review_model_overridden,
model_override: guardian_review_model_override, model_override: guardian_review_model_override,
} = codex_guardian_reviewer::select_review_model( } = codex_guardian_reviewer::select_review_model(
turn.model_info(), &context.model_info,
turn.reasoning_effort(), context.reasoning_effort.as_ref(),
default_review_model_id, default_review_model_id,
&available_models, &available_models,
); );
let guardian_model_info = session // Resolve a separate reviewer against the current catalog on every attempt.
.services // Parent fallback must retain the action's metadata even after a catalog refresh.
.models_manager let guardian_model_info =
.get_model_info( if !guardian_catalog_contains_auto_review && !guardian_review_model_overridden {
guardian_model.as_str(), Arc::clone(&context.model_info)
&turn.config.to_models_manager_config(), } else {
) Arc::new(
.await; session
.services
.models_manager
.get_model_info(
guardian_model.as_str(),
&turn.config.to_models_manager_config(),
)
.await,
)
};
let mut spawn_config = build_guardian_review_session_config( let mut spawn_config = build_guardian_review_session_config(
turn.config.as_ref(), turn.config.as_ref(),
live_network_config, live_network_config,
guardian_model.as_str(), guardian_model.as_str(),
guardian_reasoning_effort.clone(), guardian_reasoning_effort.clone(),
context.reasoning_summary,
context.personality,
guardian_model_info.model_messages.as_ref(), guardian_model_info.model_messages.as_ref(),
)?; )?;
if turn.model_info().computer_use_review_required() { if context.model_info.computer_use_review_required() {
spawn_config spawn_config
.features .features
.enable(Feature::RetainClientDeveloperMessages) .enable(Feature::RetainClientDeveloperMessages)
@@ -268,7 +280,7 @@ pub(super) async fn guardian_review_session_config(
) )
})?; })?;
} }
if guardian_model != turn.model_info().slug { if guardian_model != context.model_info.slug {
spawn_config.model_context_window = None; spawn_config.model_context_window = None;
spawn_config.model_auto_compact_token_limit = None; spawn_config.model_auto_compact_token_limit = None;
} }
@@ -310,9 +322,7 @@ async fn run_guardian_review_session_before_deadline(
external_cancel: Option<CancellationToken>, external_cancel: Option<CancellationToken>,
deadline: Instant, deadline: Instant,
) -> (GuardianReviewOutcome, GuardianReviewAnalyticsResult) { ) -> (GuardianReviewOutcome, GuardianReviewAnalyticsResult) {
let turn = context.turn(); let session_config = match guardian_review_session_config(session.as_ref(), &context).await {
let session_config = match guardian_review_session_config(session.as_ref(), turn.as_ref()).await
{
Ok(session_config) => session_config, Ok(session_config) => session_config,
Err(err) => { Err(err) => {
return ( return (
@@ -340,8 +350,8 @@ async fn run_guardian_review_session_before_deadline(
guardian_catalog_contains_auto_review: session_config.catalog_contains_auto_review, guardian_catalog_contains_auto_review: session_config.catalog_contains_auto_review,
guardian_review_model_overridden: session_config.model_overridden, guardian_review_model_overridden: session_config.model_overridden,
guardian_review_model_override: session_config.model_override, guardian_review_model_override: session_config.model_override,
reasoning_summary: turn.reasoning_summary(), reasoning_summary: context.reasoning_summary,
personality: turn.personality(), personality: context.personality,
external_cancel, external_cancel,
deadline, deadline,
}, },
@@ -216,6 +216,7 @@ pub struct GuardianReviewSessionReuseKey {
model_auto_compact_token_limit_scope: AutoCompactTokenLimitScope, model_auto_compact_token_limit_scope: AutoCompactTokenLimitScope,
model_reasoning_effort: Option<ReasoningEffortConfig>, model_reasoning_effort: Option<ReasoningEffortConfig>,
model_reasoning_summary: Option<ReasoningSummaryConfig>, model_reasoning_summary: Option<ReasoningSummaryConfig>,
personality: Option<Personality>,
permissions: Permissions, permissions: Permissions,
developer_instructions: Option<String>, developer_instructions: Option<String>,
base_instructions: Option<String>, base_instructions: Option<String>,
@@ -257,6 +258,7 @@ impl GuardianReviewSessionReuseKey {
model_auto_compact_token_limit_scope: spawn_config.model_auto_compact_token_limit_scope, model_auto_compact_token_limit_scope: spawn_config.model_auto_compact_token_limit_scope,
model_reasoning_effort: spawn_config.model_reasoning_effort.clone(), model_reasoning_effort: spawn_config.model_reasoning_effort.clone(),
model_reasoning_summary: spawn_config.model_reasoning_summary, model_reasoning_summary: spawn_config.model_reasoning_summary,
personality: spawn_config.personality,
permissions: spawn_config.permissions.clone(), permissions: spawn_config.permissions.clone(),
developer_instructions: spawn_config.developer_instructions.clone(), developer_instructions: spawn_config.developer_instructions.clone(),
base_instructions: spawn_config.base_instructions.clone(), base_instructions: spawn_config.base_instructions.clone(),
@@ -37,9 +37,7 @@ impl PreparedSession {
parent.guardian_context_mode, parent.guardian_context_mode,
) )
.with_environments(context.environments()) .with_environments(context.environments())
.with_node_repl_policy_eligibility( .with_node_repl_policy_eligibility(context.model_info.computer_use_review_required())
context.turn().model_info().computer_use_review_required(),
)
.with_node_repl_policy(node_repl_policy); .with_node_repl_policy(node_repl_policy);
key.root_authorization_version = root_authorization_version; key.root_authorization_version = root_authorization_version;
let host = parent let host = parent
@@ -230,11 +228,12 @@ pub(crate) fn prewarm_guardian_review_session(
) -> BoxFuture<'static, anyhow::Result<()>> { ) -> BoxFuture<'static, anyhow::Result<()>> {
// Keep the Session -> Guardian -> Session startup future on the heap. // Keep the Session -> Guardian -> Session startup future on the heap.
Box::pin(async move { Box::pin(async move {
let config = guardian_review_session_config(&parent, &turn).await?; let context = GuardianReviewContext::from(turn);
let config = guardian_review_session_config(&parent, &context).await?;
let history = parent.clone_history().await; let history = parent.clone_history().await;
let factory = PreparedSession::prepare( let factory = PreparedSession::prepare(
Arc::clone(&parent), Arc::clone(&parent),
GuardianReviewContext::from(turn), context,
config.spawn_config, config.spawn_config,
&history, &history,
&config.node_repl_policy, &config.node_repl_policy,
@@ -35,6 +35,8 @@ async fn run_review_preserves_evidence_during_parent_compaction() {
/*live_network_config*/ None, /*live_network_config*/ None,
&params.model, &params.model,
params.reasoning_effort.clone(), params.reasoning_effort.clone(),
params.reasoning_summary,
params.personality,
/*model_messages*/ None, /*model_messages*/ None,
) )
.unwrap(); .unwrap();
@@ -210,6 +212,8 @@ async fn test_review_params() -> GuardianReviewSessionParams {
/*live_network_config*/ None, /*live_network_config*/ None,
model.as_str(), model.as_str(),
reasoning_effort.clone(), reasoning_effort.clone(),
reasoning_summary,
personality,
/*model_messages*/ None, /*model_messages*/ None,
) )
.expect("guardian config"); .expect("guardian config");
@@ -279,6 +283,8 @@ async fn guardian_review_session_config_change_invalidates_cached_session() {
/*live_network_config*/ None, /*live_network_config*/ None,
"active-model", "active-model",
/*reasoning_effort*/ None, /*reasoning_effort*/ None,
ReasoningSummaryConfig::default(),
/*personality*/ None,
/*model_messages*/ None, /*model_messages*/ None,
) )
.expect("cached guardian config"); .expect("cached guardian config");
@@ -297,6 +303,8 @@ async fn guardian_review_session_config_change_invalidates_cached_session() {
/*live_network_config*/ None, /*live_network_config*/ None,
"active-model", "active-model",
/*reasoning_effort*/ None, /*reasoning_effort*/ None,
ReasoningSummaryConfig::default(),
/*personality*/ None,
/*model_messages*/ None, /*model_messages*/ None,
) )
.expect("next guardian config"); .expect("next guardian config");
@@ -474,6 +482,8 @@ async fn guardian_review_session_compact_scope_change_invalidates_cached_session
/*live_network_config*/ None, /*live_network_config*/ None,
"active-model", "active-model",
/*reasoning_effort*/ None, /*reasoning_effort*/ None,
ReasoningSummaryConfig::default(),
/*personality*/ None,
/*model_messages*/ None, /*model_messages*/ None,
) )
.expect("cached guardian config"); .expect("cached guardian config");
@@ -492,6 +502,8 @@ async fn guardian_review_session_compact_scope_change_invalidates_cached_session
/*live_network_config*/ None, /*live_network_config*/ None,
"active-model", "active-model",
/*reasoning_effort*/ None, /*reasoning_effort*/ None,
ReasoningSummaryConfig::default(),
/*personality*/ None,
/*model_messages*/ None, /*model_messages*/ None,
) )
.expect("next guardian config"); .expect("next guardian config");
@@ -518,6 +530,8 @@ async fn guardian_review_session_config_disables_hooks() {
/*live_network_config*/ None, /*live_network_config*/ None,
"active-model", "active-model",
/*reasoning_effort*/ None, /*reasoning_effort*/ None,
ReasoningSummaryConfig::default(),
/*personality*/ None,
/*model_messages*/ None, /*model_messages*/ None,
) )
.expect("guardian config"); .expect("guardian config");
@@ -535,6 +549,8 @@ async fn guardian_review_session_config_disables_skill_instructions() {
/*live_network_config*/ None, /*live_network_config*/ None,
"active-model", "active-model",
/*reasoning_effort*/ None, /*reasoning_effort*/ None,
ReasoningSummaryConfig::default(),
/*personality*/ None,
/*model_messages*/ None, /*model_messages*/ None,
) )
.expect("guardian config"); .expect("guardian config");
@@ -574,6 +590,8 @@ async fn guardian_review_session_config_prefers_managed_policy_and_uses_catalog_
/*live_network_config*/ None, /*live_network_config*/ None,
"active-model", "active-model",
/*reasoning_effort*/ None, /*reasoning_effort*/ None,
ReasoningSummaryConfig::default(),
/*personality*/ None,
Some(&model_messages), Some(&model_messages),
) )
.expect("guardian config"); .expect("guardian config");
@@ -616,6 +634,8 @@ async fn guardian_review_session_config_preserves_explicit_empty_catalog_policy(
/*live_network_config*/ None, /*live_network_config*/ None,
"active-model", "active-model",
/*reasoning_effort*/ None, /*reasoning_effort*/ None,
ReasoningSummaryConfig::default(),
/*personality*/ None,
Some(&model_messages), Some(&model_messages),
) )
.expect("guardian config"); .expect("guardian config");
@@ -666,6 +686,8 @@ async fn guardian_review_session_config_preserves_explicit_empty_catalog_templat
/*live_network_config*/ None, /*live_network_config*/ None,
"active-model", "active-model",
/*reasoning_effort*/ None, /*reasoning_effort*/ None,
ReasoningSummaryConfig::default(),
/*personality*/ None,
Some(&model_messages), Some(&model_messages),
) )
.expect("guardian config"); .expect("guardian config");
@@ -21,6 +21,8 @@ pub fn build_guardian_review_session_config(
live_network_config: Option<codex_network_proxy::NetworkProxyConfig>, live_network_config: Option<codex_network_proxy::NetworkProxyConfig>,
active_model: &str, active_model: &str,
reasoning_effort: Option<codex_protocol::openai_models::ReasoningEffort>, reasoning_effort: Option<codex_protocol::openai_models::ReasoningEffort>,
reasoning_summary: codex_protocol::config_types::ReasoningSummary,
personality: Option<codex_protocol::config_types::Personality>,
model_messages: Option<&ModelMessages>, model_messages: Option<&ModelMessages>,
) -> anyhow::Result<Config> { ) -> anyhow::Result<Config> {
let mut guardian_config = parent_config.clone(); let mut guardian_config = parent_config.clone();
@@ -31,6 +33,8 @@ pub fn build_guardian_review_session_config(
); );
guardian_config.model = Some(overrides.model); guardian_config.model = Some(overrides.model);
guardian_config.model_reasoning_effort = overrides.reasoning_effort; guardian_config.model_reasoning_effort = overrides.reasoning_effort;
guardian_config.model_reasoning_summary = Some(reasoning_summary);
guardian_config.personality = personality;
guardian_config.model_provider.request_max_retries = Some(overrides.request_max_retries); guardian_config.model_provider.request_max_retries = Some(overrides.request_max_retries);
guardian_config.model_provider.stream_max_retries = Some(overrides.stream_max_retries); guardian_config.model_provider.stream_max_retries = Some(overrides.stream_max_retries);
guardian_config.include_skill_instructions = overrides.include_skill_instructions; guardian_config.include_skill_instructions = overrides.include_skill_instructions;
+222 -23
View File
@@ -43,6 +43,7 @@ use codex_protocol::ThreadId;
use codex_protocol::approvals::GuardianAssessmentAction; use codex_protocol::approvals::GuardianAssessmentAction;
use codex_protocol::approvals::NetworkApprovalProtocol; use codex_protocol::approvals::NetworkApprovalProtocol;
use codex_protocol::config_types::ApprovalsReviewer; use codex_protocol::config_types::ApprovalsReviewer;
use codex_protocol::config_types::ReasoningSummary;
use codex_protocol::models::ContentItem; use codex_protocol::models::ContentItem;
use codex_protocol::models::FunctionCallOutputContentItem; use codex_protocol::models::FunctionCallOutputContentItem;
use codex_protocol::models::PermissionProfile; use codex_protocol::models::PermissionProfile;
@@ -1803,6 +1804,106 @@ enum GuardianTestCatalog {
ParentOnly, ParentOnly,
} }
#[test_case::test_case(None; "captured_policy")]
#[test_case::test_case(Some("configured policy wins"); "configured_policy")]
#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
async fn guardian_reuse_respects_effective_policy_and_personality(
configured_policy: Option<&str>,
) -> anyhow::Result<()> {
skip_if_no_network!(Ok(()));
let server = start_mock_server().await;
let responses = mount_sse_sequence(
&server,
(0..3)
.map(|index| {
sse(vec![
ev_response_created(&format!("review-{index}")),
ev_assistant_message("assessment", r#"{"outcome":"allow"}"#),
ev_completed(&format!("review-{index}")),
])
})
.collect(),
)
.await;
let (mut session, mut turn) = guardian_test_session_and_turn(&server).await;
Arc::make_mut(&mut Arc::get_mut(&mut turn).expect("unshared turn").config)
.guardian_policy_config = configured_policy.map(str::to_owned);
let mut captured = GuardianReviewContext::from(&turn);
let parent = Arc::make_mut(&mut captured.model_info);
parent.slug = "captured-parent".to_string();
parent.auto_review_model_override = None;
parent.model_messages = Some(serde_json::from_value(serde_json::json!({
"auto_review": {
"policy": "captured policy",
"policy_template": "captured template: {{ tenant_policy_config }}",
},
}))?);
captured.personality = Some(codex_protocol::config_types::Personality::Friendly);
Arc::get_mut(&mut session)
.expect("unshared session")
.services
.models_manager = Arc::new(StaticModelsManager::new(
/*auth_manager*/ None,
ModelsResponse { models: vec![] },
));
seed_guardian_parent_history(&session, &turn).await;
let mut changed_policy = captured.clone();
Arc::make_mut(&mut changed_policy.model_info)
.model_messages
.as_mut()
.unwrap()
.auto_review
.as_mut()
.unwrap()
.policy = Some("changed action policy".to_string());
let mut different_personality = changed_policy.clone();
different_personality.personality = Some(codex_protocol::config_types::Personality::Pragmatic);
for (index, context) in [captured, changed_policy, different_personality]
.into_iter()
.enumerate()
{
let (outcome, _) = run_guardian_review_session_for_test(
Arc::clone(&session),
context,
guardian_exec_command_request(&format!("action-{index}")),
ApprovalRequestReasons::default(),
guardian_output_schema(),
/*external_cancel*/ None,
/*max_attempts*/ 1,
)
.await;
assert!(matches!(outcome, GuardianReviewOutcome::Completed(_)));
}
let requests = responses.requests();
assert_eq!(requests.len(), 3);
let initial_policy = configured_policy.unwrap_or("captured policy");
assert_eq!(
requests[0].instructions_text(),
guardian_policy_prompt_with_config_and_template(
initial_policy,
"captured template: {{ tenant_policy_config }}"
)
);
let changed_policy = configured_policy.unwrap_or("changed action policy");
assert_eq!(
requests[1].instructions_text(),
guardian_policy_prompt_with_config_and_template(
changed_policy,
"captured template: {{ tenant_policy_config }}"
)
);
let thread_ids = requests
.iter()
.map(|request| request.body_json()["client_metadata"]["thread_id"].clone())
.collect::<Vec<_>>();
assert_eq!(thread_ids[0] == thread_ids[1], configured_policy.is_some());
assert_ne!(thread_ids[1], thread_ids[2]);
Ok(())
}
async fn guardian_request_model_for_auto_review( async fn guardian_request_model_for_auto_review(
auto_review_model_override: Option<String>, auto_review_model_override: Option<String>,
catalog: GuardianTestCatalog, catalog: GuardianTestCatalog,
@@ -3191,8 +3292,21 @@ async fn guardian_review_does_not_retry_valid_denial() -> anyhow::Result<()> {
Ok(()) Ok(())
} }
enum RequiredGuardianReview {
SandboxRetry,
EscalatedPermission,
LiveManagedModel,
LiveManagedModelWithGuardianV2,
}
#[test_case::test_case(RequiredGuardianReview::SandboxRetry; "sandbox_retry")]
#[test_case::test_case(RequiredGuardianReview::EscalatedPermission; "escalated_permission")]
#[test_case::test_case(RequiredGuardianReview::LiveManagedModel; "live_managed_action_model")]
#[test_case::test_case(RequiredGuardianReview::LiveManagedModelWithGuardianV2; "live_managed_action_model_with_guardian_v2")]
#[tokio::test(flavor = "multi_thread", worker_threads = 2)] #[tokio::test(flavor = "multi_thread", worker_threads = 2)]
async fn escalated_retry_bypasses_extension_approval_and_runs_guardian() -> anyhow::Result<()> { async fn guardian_review_routes_required_actions(
required_review: RequiredGuardianReview,
) -> anyhow::Result<()> {
skip_if_no_network!(Ok(())); skip_if_no_network!(Ok(()));
struct AutoApprovingReviewContributor; struct AutoApprovingReviewContributor;
@@ -3225,34 +3339,97 @@ async fn escalated_retry_bypasses_extension_approval_and_runs_guardian() -> anyh
) )
.await; .await;
let (mut session, turn) = guardian_test_session_and_turn(&server).await; let (mut session, mut turn) = guardian_test_session_and_turn(&server).await;
let mut extensions = codex_extension_api::ExtensionRegistryBuilder::<Config>::new(); let mut extensions = codex_extension_api::ExtensionRegistryBuilder::<Config>::new();
extensions.approval_review_contributor(Arc::new(AutoApprovingReviewContributor)); extensions.approval_review_contributor(Arc::new(AutoApprovingReviewContributor));
Arc::get_mut(&mut session) Arc::get_mut(&mut session)
.expect("session should be uniquely owned") .expect("session should be uniquely owned")
.services .services
.extensions = Arc::new(extensions.build()); .extensions = Arc::new(extensions.build());
let guardian_v2_enabled = matches!(
required_review,
RequiredGuardianReview::LiveManagedModelWithGuardianV2
);
let turn_mut = Arc::get_mut(&mut turn).expect("turn should be uniquely owned");
Arc::make_mut(&mut turn_mut.config)
.features
.set_enabled(Feature::GuardianV2, guardian_v2_enabled)?;
seed_guardian_parent_history(&session, &turn).await; seed_guardian_parent_history(&session, &turn).await;
let retry_reason = "The sandbox blocked the original command."; let retry_reason = "The sandbox blocked the original command.";
let mut context = GuardianReviewContext::from(&turn);
let (request, reasons) = match required_review {
RequiredGuardianReview::SandboxRetry => (
guardian_exec_command_request("shell-sandbox-retry"),
ApprovalRequestReasons {
approval: None,
retry: Some(retry_reason.to_string()),
},
),
RequiredGuardianReview::EscalatedPermission => (
GuardianApprovalRequest::ExecCommand {
id: "shell-escalated-permission".to_string(),
environment_id: codex_exec_server::LOCAL_ENVIRONMENT_ID.to_string(),
command: vec!["git".to_string(), "push".to_string()],
cwd: test_path_buf("/repo/codex-rs/core").abs().into(),
guardian_cwd: native_guardian_cwd("/repo/codex-rs/core"),
sandbox_permissions: crate::sandboxing::SandboxPermissions::RequireEscalated,
additional_permissions: None,
justification: Some("Need to push the reviewed docs fix.".to_string()),
tty: false,
},
ApprovalRequestReasons::default(),
),
RequiredGuardianReview::LiveManagedModel
| RequiredGuardianReview::LiveManagedModelWithGuardianV2 => {
Arc::make_mut(&mut context.model_info).slug = "required-action-model".to_string();
// The admitted turn has neither this model nor the new requirement.
let mut config = session.get_config().await.as_ref().clone();
let requirements = codex_config::ConfigRequirements {
auto_review_required_models: Some(Sourced::new(
std::collections::BTreeSet::from([context.model_info.slug.clone()]),
RequirementSource::Unknown,
)),
..Default::default()
};
config.config_layer_stack = ConfigLayerStack::new(
config
.config_layer_stack
.all_layers_low_to_high()
.cloned()
.collect(),
requirements,
config.config_layer_stack.requirements_toml().clone(),
)?;
session.refresh_mcp_config(config).await;
(
guardian_exec_command_request("shell-live-managed-model"),
ApprovalRequestReasons::default(),
)
}
};
let is_retry = reasons.retry.is_some();
let decision = review_approval_request( let decision = review_approval_request(
&session, &session,
&turn, context,
"review-escalated-retry".to_string(), "review-required-guardian".to_string(),
guardian_exec_command_request("shell-escalated-retry"), request,
ApprovalRequestReasons { reasons,
approval: None,
retry: Some(retry_reason.to_string()),
},
) )
.await; .await;
assert!(matches!(decision, ReviewDecision::Denied { .. })); if guardian_v2_enabled {
assert!( assert_eq!(decision, ReviewDecision::Approved);
request_log assert!(request_log.requests().is_empty());
.single_request() } else {
.body_contains_text(retry_reason) assert!(matches!(decision, ReviewDecision::Denied { .. }));
); assert_eq!(
request_log
.single_request()
.body_contains_text(retry_reason),
is_retry,
);
}
Ok(()) Ok(())
} }
@@ -3601,6 +3778,8 @@ async fn guardian_review_session_config_preserves_parent_network_proxy() {
/*live_network_config*/ None, /*live_network_config*/ None,
"parent-active-model", "parent-active-model",
Some(codex_protocol::openai_models::ReasoningEffort::Low), Some(codex_protocol::openai_models::ReasoningEffort::Low),
ReasoningSummary::default(),
/*personality*/ None,
/*model_messages*/ None, /*model_messages*/ None,
) )
.expect("guardian config"); .expect("guardian config");
@@ -3636,10 +3815,11 @@ async fn guardian_review_session_config_clears_context_overrides_for_distinct_ef
.expect("turn should be unique") .expect("turn should be unique")
.config = Arc::new(config); .config = Arc::new(config);
let guardian_config = guardian_review_session_config(session.as_ref(), turn.as_ref()) let guardian_config =
.await guardian_review_session_config(session.as_ref(), &GuardianReviewContext::from(&turn))
.expect("guardian config") .await
.spawn_config; .expect("guardian config")
.spawn_config;
assert_eq!( assert_eq!(
( (
@@ -3673,10 +3853,11 @@ async fn guardian_review_session_config_preserves_context_overrides_for_same_eff
.expect("turn should be unique") .expect("turn should be unique")
.config = Arc::new(config); .config = Arc::new(config);
let guardian_config = guardian_review_session_config(session.as_ref(), turn.as_ref()) let guardian_config =
.await guardian_review_session_config(session.as_ref(), &GuardianReviewContext::from(&turn))
.expect("guardian config") .await
.spawn_config; .expect("guardian config")
.spawn_config;
assert_eq!( assert_eq!(
( (
@@ -3698,6 +3879,8 @@ async fn guardian_review_session_config_clears_parent_developer_instructions() {
/*live_network_config*/ None, /*live_network_config*/ None,
"active-model", "active-model",
/*reasoning_effort*/ None, /*reasoning_effort*/ None,
ReasoningSummary::default(),
/*personality*/ None,
/*model_messages*/ None, /*model_messages*/ None,
) )
.expect("guardian config"); .expect("guardian config");
@@ -3725,6 +3908,8 @@ async fn guardian_review_session_config_clears_legacy_notify() {
/*live_network_config*/ None, /*live_network_config*/ None,
"active-model", "active-model",
/*reasoning_effort*/ None, /*reasoning_effort*/ None,
ReasoningSummary::default(),
/*personality*/ None,
/*model_messages*/ None, /*model_messages*/ None,
) )
.expect("guardian config"); .expect("guardian config");
@@ -3760,6 +3945,8 @@ async fn guardian_review_session_config_uses_live_network_proxy_state() {
Some(live_network.clone()), Some(live_network.clone()),
"active-model", "active-model",
/*reasoning_effort*/ None, /*reasoning_effort*/ None,
ReasoningSummary::default(),
/*personality*/ None,
/*model_messages*/ None, /*model_messages*/ None,
) )
.expect("guardian config"); .expect("guardian config");
@@ -3807,6 +3994,8 @@ async fn guardian_review_session_config_disables_mcp_apps_plugins_memories_and_g
/*live_network_config*/ None, /*live_network_config*/ None,
"active-model", "active-model",
/*reasoning_effort*/ None, /*reasoning_effort*/ None,
ReasoningSummary::default(),
/*personality*/ None,
/*model_messages*/ None, /*model_messages*/ None,
) )
.expect("guardian config"); .expect("guardian config");
@@ -3839,6 +4028,8 @@ async fn guardian_review_session_config_allows_pinned_disabled_feature() {
/*live_network_config*/ None, /*live_network_config*/ None,
"active-model", "active-model",
/*reasoning_effort*/ None, /*reasoning_effort*/ None,
ReasoningSummary::default(),
/*personality*/ None,
/*model_messages*/ None, /*model_messages*/ None,
) )
.expect("guardian config should continue when a disabled feature is pinned on"); .expect("guardian config should continue when a disabled feature is pinned on");
@@ -3858,6 +4049,8 @@ async fn guardian_review_session_config_uses_parent_active_model_instead_of_hard
/*live_network_config*/ None, /*live_network_config*/ None,
"active-model", "active-model",
/*reasoning_effort*/ None, /*reasoning_effort*/ None,
ReasoningSummary::default(),
/*personality*/ None,
/*model_messages*/ None, /*model_messages*/ None,
) )
.expect("guardian config"); .expect("guardian config");
@@ -3877,6 +4070,8 @@ async fn guardian_review_session_config_keeps_bedrock_provider_for_bedrock_gpt_5
/*live_network_config*/ None, /*live_network_config*/ None,
AMAZON_BEDROCK_GPT_5_4_MODEL_ID, AMAZON_BEDROCK_GPT_5_4_MODEL_ID,
Some(ReasoningEffort::Low), Some(ReasoningEffort::Low),
ReasoningSummary::default(),
/*personality*/ None,
/*model_messages*/ None, /*model_messages*/ None,
) )
.expect("guardian config"); .expect("guardian config");
@@ -3932,6 +4127,8 @@ async fn guardian_review_session_config_uses_requirements_guardian_policy_config
/*live_network_config*/ None, /*live_network_config*/ None,
"active-model", "active-model",
/*reasoning_effort*/ None, /*reasoning_effort*/ None,
ReasoningSummary::default(),
/*personality*/ None,
/*model_messages*/ None, /*model_messages*/ None,
) )
.expect("guardian config"); .expect("guardian config");
@@ -3972,6 +4169,8 @@ async fn guardian_review_session_config_uses_default_guardian_policy_without_req
/*live_network_config*/ None, /*live_network_config*/ None,
"active-model", "active-model",
/*reasoning_effort*/ None, /*reasoning_effort*/ None,
ReasoningSummary::default(),
/*personality*/ None,
/*model_messages*/ None, /*model_messages*/ None,
) )
.expect("guardian config"); .expect("guardian config");
+1 -1
View File
@@ -113,7 +113,7 @@ impl ResolvedStepSettings {
&self.selected.collaboration_mode &self.selected.collaboration_mode
} }
pub(super) fn personality(&self) -> Option<Personality> { pub(crate) fn personality(&self) -> Option<Personality> {
self.selected.personality self.selected.personality
} }
+376 -108
View File
@@ -23,6 +23,7 @@ use codex_features::CurrentTimeSource;
use codex_features::Feature; use codex_features::Feature;
use codex_history::RolloutItem; use codex_history::RolloutItem;
use codex_login::CodexAuth; use codex_login::CodexAuth;
use codex_models_manager::manager::RefreshStrategy;
use codex_protocol::ThreadId; use codex_protocol::ThreadId;
use codex_protocol::config_types::ApprovalsReviewer; use codex_protocol::config_types::ApprovalsReviewer;
use codex_protocol::config_types::WindowsSandboxLevel; use codex_protocol::config_types::WindowsSandboxLevel;
@@ -33,6 +34,7 @@ use codex_protocol::models::ResponseItem;
use codex_protocol::openai_models::AutoReviewMessages; use codex_protocol::openai_models::AutoReviewMessages;
use codex_protocol::openai_models::MODEL_SPECIALTY_CYBER; use codex_protocol::openai_models::MODEL_SPECIALTY_CYBER;
use codex_protocol::openai_models::ModelTokenBudgetConfig; use codex_protocol::openai_models::ModelTokenBudgetConfig;
use codex_protocol::openai_models::ModelVisibility;
use codex_protocol::openai_models::ModelsResponse; use codex_protocol::openai_models::ModelsResponse;
use codex_protocol::permissions::FileSystemAccessMode; use codex_protocol::permissions::FileSystemAccessMode;
use codex_protocol::permissions::FileSystemPath; use codex_protocol::permissions::FileSystemPath;
@@ -59,19 +61,20 @@ use core_test_support::responses::ev_completed_with_tokens;
use core_test_support::responses::ev_function_call; use core_test_support::responses::ev_function_call;
use core_test_support::responses::ev_function_call_with_namespace; use core_test_support::responses::ev_function_call_with_namespace;
use core_test_support::responses::ev_response_created; use core_test_support::responses::ev_response_created;
use core_test_support::responses::mount_response_once_match; use core_test_support::responses::mount_models_once;
use core_test_support::responses::mount_sse_once;
use core_test_support::responses::mount_sse_sequence; use core_test_support::responses::mount_sse_sequence;
use core_test_support::responses::sse; use core_test_support::responses::sse;
use core_test_support::responses::sse_response;
use core_test_support::responses::start_mock_server; use core_test_support::responses::start_mock_server;
use core_test_support::responses::start_websocket_server; use core_test_support::responses::start_websocket_server;
use core_test_support::skip_if_no_network; use core_test_support::skip_if_no_network;
use core_test_support::skip_if_sandbox; use core_test_support::skip_if_sandbox;
use core_test_support::skip_if_wine_exec; use core_test_support::skip_if_wine_exec;
use core_test_support::streaming_sse::StreamingSseChunk;
use core_test_support::streaming_sse::start_streaming_sse_server;
use core_test_support::test_codex::local_selections; use core_test_support::test_codex::local_selections;
use core_test_support::test_codex::test_codex; use core_test_support::test_codex::test_codex;
use core_test_support::wait_for_event; use core_test_support::wait_for_event;
use core_test_support::wait_for_event_match;
use core_test_support::wait_for_mcp_server; use core_test_support::wait_for_mcp_server;
use pretty_assertions::assert_eq; use pretty_assertions::assert_eq;
use serde_json::Value; use serde_json::Value;
@@ -948,6 +951,219 @@ async fn guardian_node_repl_policy_follows_production_approval_path(
Ok(()) Ok(())
} }
fn guardian_parent_catalog() -> ModelsResponse {
let template = codex_models_manager::bundled_models_response()
.expect("bundled model catalog")
.models
.into_iter()
.find(|model| model.slug == "gpt-5.4")
.expect("gpt-5.4 in bundled catalog");
// Keep safety settings compatible so active publication can switch A to B.
ModelsResponse {
models: ["guardian-parent-a", "guardian-parent-b"]
.into_iter()
.map(|slug| {
let mut model = template.clone();
model.slug = slug.to_string();
model.visibility = ModelVisibility::List;
model.auto_review_model_override = None;
model.supported_reasoning_levels.retain(|level| {
level.effort != codex_protocol::openai_models::ReasoningEffort::Low
});
model
.model_messages
.as_mut()
.expect("model messages")
.auto_review = Some(AutoReviewMessages {
policy: Some("captured policy".to_string()),
policy_template: Some(
"captured template: {{ tenant_policy_config }}".to_string(),
),
node_repl_policy: None,
rejection_instructions: None,
timeout_instructions: None,
});
model
})
.collect(),
}
}
#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
async fn guardian_reviews_delayed_and_new_actions_after_catalog_refresh() -> Result<()> {
skip_if_no_network!(Ok(()));
skip_if_sandbox!(Ok(()));
skip_if_wine_exec!(
Ok(()),
"Guardian approval actions require host-native paths"
);
#[derive(Default)]
struct PauseFirstAction {
started: tokio::sync::Notify,
resume: tokio::sync::Notify,
}
impl ToolLifecycleContributor for PauseFirstAction {
fn on_tool_start<'a>(&'a self, input: ToolStartInput<'a>) -> ToolLifecycleFuture<'a> {
Box::pin(async move {
if input.call_id == "captured-action" {
self.started.notify_one();
self.resume.notified().await;
}
})
}
}
let server = wiremock::MockServer::start().await;
let mut catalog = guardian_parent_catalog();
let mut preferred = catalog.models[0].clone();
preferred.slug = "codex-auto-review".to_string();
catalog.models.push(preferred);
mount_models_once(&server, catalog.clone()).await;
let pause = Arc::new(PauseFirstAction::default());
let mut extensions = ExtensionRegistryBuilder::<Config>::new();
extensions.tool_lifecycle_contributor(pause.clone());
let test = test_codex()
.with_auth(CodexAuth::create_dummy_chatgpt_auth_for_testing())
.with_model("guardian-parent-a")
.with_extensions(Arc::new(extensions.build()))
.with_config(|config| {
config.features.enable(Feature::StepModelSwitching).unwrap();
config.permissions.approval_policy = Constrained::allow_any(AskForApproval::OnRequest);
config.approvals_reviewer = ApprovalsReviewer::AutoReview;
config.model_reasoning_effort =
Some(codex_protocol::openai_models::ReasoningEffort::High);
config.model_reasoning_summary =
Some(codex_protocol::config_types::ReasoningSummary::Detailed);
})
.build_with_auto_env(&server)
.await?;
let models_manager = test.thread_manager.get_models_manager();
assert_eq!(models_manager.get_remote_models().await, catalog.models);
let mut events = Vec::new();
for (call_id, marker) in [("captured-action", "action-a"), ("new-action", "action-b")] {
events.push(sse(vec![
ev_response_created(call_id),
ev_function_call(
call_id,
"exec_command",
&json!({
"cmd": format!("printf {marker}"),
"sandbox_permissions": SandboxPermissions::RequireEscalated,
"justification": "Verify action-scoped Guardian fallback.",
})
.to_string(),
),
ev_completed(call_id),
]));
events.push(sse(vec![
ev_response_created("guardian"),
ev_assistant_message("assessment", r#"{"outcome":"allow"}"#),
ev_completed("guardian"),
]));
}
events.push(sse(vec![ev_response_created("done"), ev_completed("done")]));
let responses = mount_sse_sequence(&server, events).await;
test.codex
.start_or_steer_turn(TurnInputRequest::user_input(vec![UserInput::Text {
text: "run both protected commands".to_string(),
text_elements: Vec::new(),
}]))
.await?;
let turn_id = wait_for_event_match(&test.codex, |event| match event {
EventMsg::TurnStarted(event) => Some(event.turn_id.clone()),
_ => None,
})
.await;
tokio::time::timeout(Duration::from_secs(10), pause.started.notified()).await?;
let (reply, outcome) = tokio::sync::oneshot::channel();
test.codex
.submit(Op::TurnSettings {
turn_id,
update: codex_protocol::protocol::TurnSettingsUpdate {
model: Some("guardian-parent-b".to_string()),
effort: Some(Some(codex_protocol::openai_models::ReasoningEffort::Medium)),
summary: Some(codex_protocol::config_types::ReasoningSummary::Concise),
..Default::default()
},
reply,
})
.await?;
assert_eq!(
tokio::time::timeout(Duration::from_secs(10), outcome).await??,
codex_protocol::protocol::TurnSettingsUpdateOutcome::Applied
);
// A remains paused after the B settings request. Remove the preferred reviewer
// and replace A's metadata before review so fallback must use A's captured copy.
catalog
.models
.retain(|model| model.slug != "codex-auto-review");
catalog.models[0]
.model_messages
.as_mut()
.unwrap()
.auto_review = Some(AutoReviewMessages {
policy: Some("refreshed policy".to_string()),
policy_template: Some("refreshed template: {{ tenant_policy_config }}".to_string()),
node_repl_policy: None,
rejection_instructions: None,
timeout_instructions: None,
});
mount_models_once(&server, catalog.clone()).await;
assert_eq!(
models_manager
.raw_model_catalog(
RefreshStrategy::Online,
codex_core::test_support::default_http_client_factory(),
)
.await,
catalog
);
pause.resume.notify_one();
wait_for_event(&test.codex, |event| {
matches!(event, EventMsg::TurnComplete(_))
})
.await;
let requests = responses.requests();
let reviews = requests
.iter()
.filter(|request| request.body_json()["client_metadata"]["x-openai-subagent"] == "guardian")
.map(|request| {
let body = request.body_json();
assert!(
request
.instructions_text()
.starts_with("captured template: captured policy\n")
);
json!([
body["model"],
body["reasoning"]["effort"],
body["reasoning"]["summary"]
])
})
.collect::<Vec<_>>();
assert_eq!(
reviews,
vec![
json!(["guardian-parent-a", "high", "detailed"]),
json!(["guardian-parent-b", "medium", "concise"]),
]
);
assert!(
requests[2]
.function_call_output("captured-action")
.to_string()
.contains("action-a")
);
assert!(
requests[4]
.function_call_output("new-action")
.to_string()
.contains("action-b")
);
Ok(())
}
#[tokio::test(flavor = "multi_thread", worker_threads = 2)] #[tokio::test(flavor = "multi_thread", worker_threads = 2)]
async fn guardian_session_is_reused_for_consecutive_tool_reviews_without_prewarm() -> Result<()> { async fn guardian_session_is_reused_for_consecutive_tool_reviews_without_prewarm() -> Result<()> {
skip_if_no_network!(Ok(())); skip_if_no_network!(Ok(()));
@@ -964,6 +1180,7 @@ async fn guardian_session_is_reused_for_consecutive_tool_reviews_without_prewarm
let mut extensions = ExtensionRegistryBuilder::<Config>::new(); let mut extensions = ExtensionRegistryBuilder::<Config>::new();
extensions.tool_lifecycle_contributor(lifecycle_recorder.clone()); extensions.tool_lifecycle_contributor(lifecycle_recorder.clone());
let mut builder = test_codex() let mut builder = test_codex()
.with_model("gpt-5.4")
.with_extensions(Arc::new(extensions.build())) .with_extensions(Arc::new(extensions.build()))
.with_config(move |config| { .with_config(move |config| {
let secret_file = config.cwd.join("guardian-secret.txt"); let secret_file = config.cwd.join("guardian-secret.txt");
@@ -1141,9 +1358,25 @@ async fn guardian_session_is_reused_for_consecutive_tool_reviews_without_prewarm
matches!(event, EventMsg::TurnComplete(_)) matches!(event, EventMsg::TurnComplete(_))
}) })
.await; .await;
test.submit_text_turn("run the second command that requires Guardian review") test.codex
.start_or_steer_turn(
TurnInputRequest::user_input(vec![UserInput::Text {
text: "run the second command that requires Guardian review".into(),
text_elements: Vec::new(),
}])
.with_thread_settings(ThreadSettingsOverrides {
model: Some("gpt-5.5".to_string()),
..Default::default()
}),
)
.await?; .await?;
wait_for_event(&test.codex, |event| {
matches!(event, EventMsg::TurnComplete(_))
})
.await;
let requests = responses.requests(); let requests = responses.requests();
assert_eq!(requests[0].body_json()["model"], "gpt-5.4");
assert_eq!(requests[4].body_json()["model"], "gpt-5.5");
assert!( assert!(
requests requests
.iter() .iter()
@@ -1236,7 +1469,8 @@ async fn guardian_session_is_reused_for_consecutive_tool_reviews_without_prewarm
} }
#[tokio::test(flavor = "multi_thread", worker_threads = 2)] #[tokio::test(flavor = "multi_thread", worker_threads = 2)]
async fn interrupted_guardian_tool_review_aborts_without_executing_the_command() -> Result<()> { async fn interrupted_guardian_review_across_model_change_does_not_execute_the_command() -> Result<()>
{
skip_if_no_network!(Ok(())); skip_if_no_network!(Ok(()));
skip_if_sandbox!(Ok(())); skip_if_sandbox!(Ok(()));
skip_if_wine_exec!( skip_if_wine_exec!(
@@ -1252,137 +1486,171 @@ async fn interrupted_guardian_tool_review_aborts_without_executing_the_command()
exclude_tmpdir_env_var: true, exclude_tmpdir_env_var: true,
exclude_slash_tmp: true, exclude_slash_tmp: true,
}; };
let sandbox_policy_for_config = sandbox_policy.clone();
let mut builder = test_codex().with_config(move |config| {
config.permissions.approval_policy = Constrained::allow_any(approval_policy);
config.approvals_reviewer = ApprovalsReviewer::AutoReview;
config
.set_legacy_sandbox_policy(sandbox_policy_for_config)
.expect("set sandbox policy");
});
let test = builder.build_with_auto_env(&server).await?;
let output_file = test.cwd.path().join("guardian-interrupted.txt");
let command = format!("printf should-not-run > {}", output_file.display());
let tool_args = json!({ let tool_args = json!({
"cmd": command, "cmd": "printf should-not-run > guardian-interrupted.txt",
"yield_time_ms": 1_000_u64, "yield_time_ms": 1_000_u64,
"sandbox_permissions": SandboxPermissions::RequireEscalated, "sandbox_permissions": SandboxPermissions::RequireEscalated,
"justification": "Exercise interrupted Guardian approval.", "justification": "Exercise interrupted Guardian approval.",
}); });
mount_sse_once( let (release_review, review_gate) = tokio::sync::oneshot::channel();
&server, let (streaming, mut completions) = start_streaming_sse_server(vec![
sse(vec![ vec![StreamingSseChunk {
ev_response_created("resp-parent-interrupted-tool"), gate: None,
ev_function_call( body: sse(vec![
"exec-call-interrupted", ev_response_created("resp-parent-interrupted-tool"),
"exec_command", ev_function_call(
&serde_json::to_string(&tool_args)?, "exec-call-interrupted",
), "exec_command",
ev_completed("resp-parent-interrupted-tool"), &tool_args.to_string(),
]), ),
) ev_completed("resp-parent-interrupted-tool"),
]),
}],
vec![StreamingSseChunk {
gate: Some(review_gate),
body: sse(vec![
ev_response_created("resp-guardian-interrupted-review"),
ev_assistant_message("assessment", r#"{"outcome":"allow"}"#),
ev_completed("resp-guardian-interrupted-review"),
]),
}],
vec![StreamingSseChunk {
gate: None,
body: sse(vec![
ev_response_created("resp-parent-after-interrupted-review"),
ev_assistant_message("msg-parent-after-interrupted-review", "next turn completed"),
ev_completed("resp-parent-after-interrupted-review"),
]),
}],
])
.await; .await;
let pending_guardian = mount_response_once_match( let base_url = format!("{}/v1", streaming.uri());
&server, let test = test_codex()
|request: &wiremock::Request| { .with_model("guardian-parent-a")
serde_json::from_slice::<Value>(&request.body) .with_config(move |config| {
.ok() config.model_provider.base_url = Some(base_url);
.and_then(|body| { config.model_catalog = Some(guardian_parent_catalog());
body.pointer("/client_metadata/x-openai-subagent") config.features.enable(Feature::StepModelSwitching).unwrap();
.and_then(Value::as_str) config.permissions.approval_policy = Constrained::allow_any(approval_policy);
.map(str::to_owned) config.approvals_reviewer = ApprovalsReviewer::AutoReview;
}) config
.as_deref() .set_legacy_sandbox_policy(sandbox_policy)
== Some("guardian") .expect("set sandbox policy");
}, })
sse_response(sse(vec![ .build_with_auto_env(&server)
ev_response_created("resp-guardian-interrupted-review"),
ev_assistant_message(
"msg-guardian-interrupted-review",
&json!({
"risk_level": "low",
"user_authorization": "high",
"outcome": "allow",
"rationale": "This review should be interrupted before it completes.",
})
.to_string(),
),
ev_completed("resp-guardian-interrupted-review"),
]))
.set_delay(Duration::from_millis(200)),
)
.await;
test.codex
.start_or_steer_turn(
TurnInputRequest::user_input(vec![UserInput::Text {
text: "interrupt a Guardian-reviewed command".into(),
text_elements: Vec::new(),
}])
.with_thread_settings(ThreadSettingsOverrides {
environments: Some(local_selections(test.config.cwd.clone())),
approval_policy: Some(approval_policy),
approvals_reviewer: Some(ApprovalsReviewer::AutoReview),
sandbox_policy: Some(sandbox_policy),
..Default::default()
}),
)
.await?; .await?;
tokio::time::timeout(Duration::from_secs(5), async { test.codex
while pending_guardian.requests().is_empty() { .start_or_steer_turn(TurnInputRequest::user_input(vec![UserInput::Text {
tokio::task::yield_now().await; text: "interrupt a Guardian-reviewed command".into(),
} text_elements: Vec::new(),
}]))
.await?;
let turn_id = wait_for_event_match(&test.codex, |event| match event {
EventMsg::TurnStarted(event) => Some(event.turn_id.clone()),
_ => None,
}) })
.await;
tokio::time::timeout(
Duration::from_secs(5),
streaming.wait_for_request_count(/*count*/ 2),
)
.await .await
.context("timed out waiting for Guardian review request")?; .context("timed out waiting for Guardian review request")?;
let (reply, outcome) = tokio::sync::oneshot::channel();
test.codex
.submit(Op::TurnSettings {
turn_id,
update: codex_protocol::protocol::TurnSettingsUpdate {
model: Some("guardian-parent-b".to_string()),
..Default::default()
},
reply,
})
.await?;
assert_eq!(
tokio::time::timeout(Duration::from_secs(10), outcome).await??,
codex_protocol::protocol::TurnSettingsUpdateOutcome::Applied
);
test.codex.submit(Op::Interrupt).await?; test.codex.submit(Op::Interrupt).await?;
wait_for_event(&test.codex, |event| { wait_for_event(&test.codex, |event| {
matches!(event, EventMsg::TurnAborted(_)) matches!(event, EventMsg::TurnAborted(_))
}) })
.await; .await;
tokio::time::sleep(Duration::from_millis(350)).await; release_review
assert!( .send(())
!output_file.exists(), .expect("release interrupted review response");
"the interrupted Guardian-reviewed command executed after its delayed approval response" // The cancelled connection may close before the server finishes writing.
); let _ = tokio::time::timeout(Duration::from_secs(5), completions.remove(1)).await?;
let follow_up = mount_sse_once(
&server,
sse(vec![
ev_response_created("resp-parent-after-interrupted-review"),
ev_assistant_message("msg-parent-after-interrupted-review", "next turn completed"),
ev_completed("resp-parent-after-interrupted-review"),
]),
)
.await;
test.codex test.codex
.start_or_steer_turn(TurnInputRequest::user_input(vec![UserInput::Text { .start_or_steer_turn(
text: "verify Guardian cancellation left the next turn clean".into(), TurnInputRequest::user_input(vec![UserInput::Text {
text_elements: Vec::new(), text: "verify Guardian cancellation left the next turn clean".into(),
}])) text_elements: Vec::new(),
}])
.with_thread_settings(ThreadSettingsOverrides {
model: Some("guardian-parent-b".to_string()),
..Default::default()
}),
)
.await?; .await?;
wait_for_event(&test.codex, |event| { wait_for_event(&test.codex, |event| {
matches!(event, EventMsg::TurnComplete(_)) matches!(event, EventMsg::TurnComplete(_))
}) })
.await; .await;
let follow_up_request = follow_up.single_request(); let requests = streaming
assert!( .requests()
follow_up_request .await
.body_contains_text("verify Guardian cancellation left the next turn clean") .into_iter()
.map(|body| serde_json::from_slice::<Value>(&body))
.collect::<serde_json::Result<Vec<_>>>()?;
assert_eq!(
requests
.iter()
.map(|body| body["model"].clone())
.collect::<Vec<_>>(),
vec![
json!("guardian-parent-a"),
json!("guardian-parent-a"),
json!("guardian-parent-b")
]
); );
assert!(follow_up_request.has_function_call("exec-call-interrupted")); assert_eq!(
let interrupted_output = follow_up_request requests[1]["client_metadata"]["x-openai-subagent"],
.function_call_output_text("exec-call-interrupted") "guardian"
);
let follow_up_input = requests[2]["input"].as_array().expect("follow-up input");
assert!(follow_up_input.iter().any(|item| {
item["type"] == "function_call" && item["call_id"] == "exec-call-interrupted"
}));
let interrupted_output = follow_up_input
.iter()
.find(|item| {
item["type"] == "function_call_output" && item["call_id"] == "exec-call-interrupted"
})
.expect("next turn should contain the interrupted command's tool output"); .expect("next turn should contain the interrupted command's tool output");
assert!( assert!(
interrupted_output.contains("aborted"), interrupted_output["output"]
.as_str()
.expect("tool output text")
.contains("aborted"),
"unexpected interrupted tool output: {interrupted_output}" "unexpected interrupted tool output: {interrupted_output}"
); );
assert!(
matches!(
test.fs()
.get_metadata(
&test.workspace_path_uri("guardian-interrupted.txt")?,
Default::default(),
/*sandbox*/ None,
)
.await,
Err(error) if error.kind() == std::io::ErrorKind::NotFound
),
"the interrupted Guardian-reviewed command executed after its delayed approval response"
);
streaming.shutdown().await;
Ok(()) Ok(())
} }