diff --git a/codex-rs/core/src/guardian/decision.rs b/codex-rs/core/src/guardian/decision.rs index 22ba7dc8d3..6709fb2e67 100644 --- a/codex-rs/core/src/guardian/decision.rs +++ b/codex-rs/core/src/guardian/decision.rs @@ -59,9 +59,10 @@ pub(crate) async fn decide_approval( let context = context.into(); let request = request.into(); 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 = - 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 || model_requires_review || requirements diff --git a/codex-rs/core/src/guardian/mod.rs b/codex-rs/core/src/guardian/mod.rs index d405d982a7..13e2ba9d24 100644 --- a/codex-rs/core/src/guardian/mod.rs +++ b/codex-rs/core/src/guardian/mod.rs @@ -23,6 +23,7 @@ mod runtime; use std::sync::Arc; use codex_protocol::config_types::ApprovalsReviewer; +use codex_protocol::config_types::Personality; use codex_protocol::config_types::ReasoningSummary; use codex_protocol::openai_models::ModelInfo; use codex_protocol::openai_models::ReasoningEffort; @@ -81,13 +82,10 @@ pub(crate) struct GuardianReviewContext { pub(crate) parent_response_id: Option, turn: Arc, environments: TurnEnvironmentSnapshot, - // Model and reasoning inputs are carried for the follow-up Guardian and V2 migrations. - #[expect(dead_code)] pub(crate) model_info: Arc, - #[expect(dead_code)] pub(crate) reasoning_effort: Option, - #[expect(dead_code)] pub(crate) reasoning_summary: ReasoningSummary, + pub(crate) personality: Option, pub(crate) approval_policy: AskForApproval, pub(crate) approvals_reviewer: ApprovalsReviewer, } @@ -106,6 +104,7 @@ impl GuardianReviewContext { model_info: Arc::clone(&settings.model_info), reasoning_effort: settings.reasoning_effort().cloned(), reasoning_summary: settings.reasoning_summary, + personality: settings.personality(), approval_policy: settings.approval_policy(), approvals_reviewer: settings.approvals_reviewer(), turn, @@ -134,6 +133,7 @@ impl From<&Arc> for GuardianReviewContext { model_info: Arc::clone(&step.settings.model_info), reasoning_effort: step.settings.reasoning_effort().cloned(), reasoning_summary: step.settings.reasoning_summary, + personality: step.settings.personality(), approval_policy: step.settings.approval_policy(), approvals_reviewer: step.settings.approvals_reviewer(), } @@ -151,6 +151,7 @@ impl From> for GuardianReviewContext { model_info: Arc::clone(turn.model_info()), reasoning_effort: turn.reasoning_effort().cloned(), reasoning_summary: turn.reasoning_summary(), + personality: turn.personality(), approval_policy: turn.approval_policy(), approvals_reviewer: turn.config.approvals_reviewer, turn, diff --git a/codex-rs/core/src/guardian/review.rs b/codex-rs/core/src/guardian/review.rs index f88c987219..641b6cc175 100644 --- a/codex-rs/core/src/guardian/review.rs +++ b/codex-rs/core/src/guardian/review.rs @@ -213,8 +213,9 @@ pub(super) struct GuardianReviewSessionConfig { pub(super) async fn guardian_review_session_config( session: &Session, - turn: &TurnContext, + context: &GuardianReviewContext, ) -> anyhow::Result { + let turn = context.turn(); let network_proxy = session.services.network_proxy.load_full(); let live_network_config = match network_proxy.as_ref() { 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_override: guardian_review_model_override, } = codex_guardian_reviewer::select_review_model( - turn.model_info(), - turn.reasoning_effort(), + &context.model_info, + context.reasoning_effort.as_ref(), default_review_model_id, &available_models, ); - let guardian_model_info = session - .services - .models_manager - .get_model_info( - guardian_model.as_str(), - &turn.config.to_models_manager_config(), - ) - .await; + // Resolve a separate reviewer against the current catalog on every attempt. + // Parent fallback must retain the action's metadata even after a catalog refresh. + let guardian_model_info = + if !guardian_catalog_contains_auto_review && !guardian_review_model_overridden { + Arc::clone(&context.model_info) + } else { + Arc::new( + 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( turn.config.as_ref(), live_network_config, guardian_model.as_str(), guardian_reasoning_effort.clone(), + context.reasoning_summary, + context.personality, 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 .features .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_auto_compact_token_limit = None; } @@ -310,9 +322,7 @@ async fn run_guardian_review_session_before_deadline( external_cancel: Option, deadline: Instant, ) -> (GuardianReviewOutcome, GuardianReviewAnalyticsResult) { - let turn = context.turn(); - let session_config = match guardian_review_session_config(session.as_ref(), turn.as_ref()).await - { + let session_config = match guardian_review_session_config(session.as_ref(), &context).await { Ok(session_config) => session_config, Err(err) => { 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_review_model_overridden: session_config.model_overridden, guardian_review_model_override: session_config.model_override, - reasoning_summary: turn.reasoning_summary(), - personality: turn.personality(), + reasoning_summary: context.reasoning_summary, + personality: context.personality, external_cancel, deadline, }, diff --git a/codex-rs/core/src/guardian/review_session.rs b/codex-rs/core/src/guardian/review_session.rs index 75495b185b..7e0dd0c5e2 100644 --- a/codex-rs/core/src/guardian/review_session.rs +++ b/codex-rs/core/src/guardian/review_session.rs @@ -216,6 +216,7 @@ pub struct GuardianReviewSessionReuseKey { model_auto_compact_token_limit_scope: AutoCompactTokenLimitScope, model_reasoning_effort: Option, model_reasoning_summary: Option, + personality: Option, permissions: Permissions, developer_instructions: Option, base_instructions: Option, @@ -257,6 +258,7 @@ impl GuardianReviewSessionReuseKey { 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_summary: spawn_config.model_reasoning_summary, + personality: spawn_config.personality, permissions: spawn_config.permissions.clone(), developer_instructions: spawn_config.developer_instructions.clone(), base_instructions: spawn_config.base_instructions.clone(), diff --git a/codex-rs/core/src/guardian/review_session_factory.rs b/codex-rs/core/src/guardian/review_session_factory.rs index aa4ff2dd23..b70ee4ff8a 100644 --- a/codex-rs/core/src/guardian/review_session_factory.rs +++ b/codex-rs/core/src/guardian/review_session_factory.rs @@ -37,9 +37,7 @@ impl PreparedSession { parent.guardian_context_mode, ) .with_environments(context.environments()) - .with_node_repl_policy_eligibility( - context.turn().model_info().computer_use_review_required(), - ) + .with_node_repl_policy_eligibility(context.model_info.computer_use_review_required()) .with_node_repl_policy(node_repl_policy); key.root_authorization_version = root_authorization_version; let host = parent @@ -230,11 +228,12 @@ pub(crate) fn prewarm_guardian_review_session( ) -> BoxFuture<'static, anyhow::Result<()>> { // Keep the Session -> Guardian -> Session startup future on the heap. 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 factory = PreparedSession::prepare( Arc::clone(&parent), - GuardianReviewContext::from(turn), + context, config.spawn_config, &history, &config.node_repl_policy, diff --git a/codex-rs/core/src/guardian/review_session_tests.rs b/codex-rs/core/src/guardian/review_session_tests.rs index d78c04654b..176faac1ad 100644 --- a/codex-rs/core/src/guardian/review_session_tests.rs +++ b/codex-rs/core/src/guardian/review_session_tests.rs @@ -35,6 +35,8 @@ async fn run_review_preserves_evidence_during_parent_compaction() { /*live_network_config*/ None, ¶ms.model, params.reasoning_effort.clone(), + params.reasoning_summary, + params.personality, /*model_messages*/ None, ) .unwrap(); @@ -210,6 +212,8 @@ async fn test_review_params() -> GuardianReviewSessionParams { /*live_network_config*/ None, model.as_str(), reasoning_effort.clone(), + reasoning_summary, + personality, /*model_messages*/ None, ) .expect("guardian config"); @@ -279,6 +283,8 @@ async fn guardian_review_session_config_change_invalidates_cached_session() { /*live_network_config*/ None, "active-model", /*reasoning_effort*/ None, + ReasoningSummaryConfig::default(), + /*personality*/ None, /*model_messages*/ None, ) .expect("cached guardian config"); @@ -297,6 +303,8 @@ async fn guardian_review_session_config_change_invalidates_cached_session() { /*live_network_config*/ None, "active-model", /*reasoning_effort*/ None, + ReasoningSummaryConfig::default(), + /*personality*/ None, /*model_messages*/ None, ) .expect("next guardian config"); @@ -474,6 +482,8 @@ async fn guardian_review_session_compact_scope_change_invalidates_cached_session /*live_network_config*/ None, "active-model", /*reasoning_effort*/ None, + ReasoningSummaryConfig::default(), + /*personality*/ None, /*model_messages*/ None, ) .expect("cached guardian config"); @@ -492,6 +502,8 @@ async fn guardian_review_session_compact_scope_change_invalidates_cached_session /*live_network_config*/ None, "active-model", /*reasoning_effort*/ None, + ReasoningSummaryConfig::default(), + /*personality*/ None, /*model_messages*/ None, ) .expect("next guardian config"); @@ -518,6 +530,8 @@ async fn guardian_review_session_config_disables_hooks() { /*live_network_config*/ None, "active-model", /*reasoning_effort*/ None, + ReasoningSummaryConfig::default(), + /*personality*/ None, /*model_messages*/ None, ) .expect("guardian config"); @@ -535,6 +549,8 @@ async fn guardian_review_session_config_disables_skill_instructions() { /*live_network_config*/ None, "active-model", /*reasoning_effort*/ None, + ReasoningSummaryConfig::default(), + /*personality*/ None, /*model_messages*/ None, ) .expect("guardian config"); @@ -574,6 +590,8 @@ async fn guardian_review_session_config_prefers_managed_policy_and_uses_catalog_ /*live_network_config*/ None, "active-model", /*reasoning_effort*/ None, + ReasoningSummaryConfig::default(), + /*personality*/ None, Some(&model_messages), ) .expect("guardian config"); @@ -616,6 +634,8 @@ async fn guardian_review_session_config_preserves_explicit_empty_catalog_policy( /*live_network_config*/ None, "active-model", /*reasoning_effort*/ None, + ReasoningSummaryConfig::default(), + /*personality*/ None, Some(&model_messages), ) .expect("guardian config"); @@ -666,6 +686,8 @@ async fn guardian_review_session_config_preserves_explicit_empty_catalog_templat /*live_network_config*/ None, "active-model", /*reasoning_effort*/ None, + ReasoningSummaryConfig::default(), + /*personality*/ None, Some(&model_messages), ) .expect("guardian config"); diff --git a/codex-rs/core/src/guardian/reviewer_config.rs b/codex-rs/core/src/guardian/reviewer_config.rs index f0b90f5a1b..a79ef12bc9 100644 --- a/codex-rs/core/src/guardian/reviewer_config.rs +++ b/codex-rs/core/src/guardian/reviewer_config.rs @@ -21,6 +21,8 @@ pub fn build_guardian_review_session_config( live_network_config: Option, active_model: &str, reasoning_effort: Option, + reasoning_summary: codex_protocol::config_types::ReasoningSummary, + personality: Option, model_messages: Option<&ModelMessages>, ) -> anyhow::Result { 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_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.stream_max_retries = Some(overrides.stream_max_retries); guardian_config.include_skill_instructions = overrides.include_skill_instructions; diff --git a/codex-rs/core/src/guardian/tests.rs b/codex-rs/core/src/guardian/tests.rs index c47c2c299c..8deb900959 100644 --- a/codex-rs/core/src/guardian/tests.rs +++ b/codex-rs/core/src/guardian/tests.rs @@ -43,6 +43,7 @@ use codex_protocol::ThreadId; use codex_protocol::approvals::GuardianAssessmentAction; use codex_protocol::approvals::NetworkApprovalProtocol; use codex_protocol::config_types::ApprovalsReviewer; +use codex_protocol::config_types::ReasoningSummary; use codex_protocol::models::ContentItem; use codex_protocol::models::FunctionCallOutputContentItem; use codex_protocol::models::PermissionProfile; @@ -1803,6 +1804,106 @@ enum GuardianTestCatalog { 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::>(); + 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( auto_review_model_override: Option, catalog: GuardianTestCatalog, @@ -3191,8 +3292,21 @@ async fn guardian_review_does_not_retry_valid_denial() -> anyhow::Result<()> { 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)] -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(())); struct AutoApprovingReviewContributor; @@ -3225,34 +3339,97 @@ async fn escalated_retry_bypasses_extension_approval_and_runs_guardian() -> anyh ) .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::::new(); extensions.approval_review_contributor(Arc::new(AutoApprovingReviewContributor)); Arc::get_mut(&mut session) .expect("session should be uniquely owned") .services .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; 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( &session, - &turn, - "review-escalated-retry".to_string(), - guardian_exec_command_request("shell-escalated-retry"), - ApprovalRequestReasons { - approval: None, - retry: Some(retry_reason.to_string()), - }, + context, + "review-required-guardian".to_string(), + request, + reasons, ) .await; - assert!(matches!(decision, ReviewDecision::Denied { .. })); - assert!( - request_log - .single_request() - .body_contains_text(retry_reason) - ); + if guardian_v2_enabled { + assert_eq!(decision, ReviewDecision::Approved); + assert!(request_log.requests().is_empty()); + } else { + assert!(matches!(decision, ReviewDecision::Denied { .. })); + assert_eq!( + request_log + .single_request() + .body_contains_text(retry_reason), + is_retry, + ); + } Ok(()) } @@ -3601,6 +3778,8 @@ async fn guardian_review_session_config_preserves_parent_network_proxy() { /*live_network_config*/ None, "parent-active-model", Some(codex_protocol::openai_models::ReasoningEffort::Low), + ReasoningSummary::default(), + /*personality*/ None, /*model_messages*/ None, ) .expect("guardian config"); @@ -3636,10 +3815,11 @@ async fn guardian_review_session_config_clears_context_overrides_for_distinct_ef .expect("turn should be unique") .config = Arc::new(config); - let guardian_config = guardian_review_session_config(session.as_ref(), turn.as_ref()) - .await - .expect("guardian config") - .spawn_config; + let guardian_config = + guardian_review_session_config(session.as_ref(), &GuardianReviewContext::from(&turn)) + .await + .expect("guardian config") + .spawn_config; assert_eq!( ( @@ -3673,10 +3853,11 @@ async fn guardian_review_session_config_preserves_context_overrides_for_same_eff .expect("turn should be unique") .config = Arc::new(config); - let guardian_config = guardian_review_session_config(session.as_ref(), turn.as_ref()) - .await - .expect("guardian config") - .spawn_config; + let guardian_config = + guardian_review_session_config(session.as_ref(), &GuardianReviewContext::from(&turn)) + .await + .expect("guardian config") + .spawn_config; assert_eq!( ( @@ -3698,6 +3879,8 @@ async fn guardian_review_session_config_clears_parent_developer_instructions() { /*live_network_config*/ None, "active-model", /*reasoning_effort*/ None, + ReasoningSummary::default(), + /*personality*/ None, /*model_messages*/ None, ) .expect("guardian config"); @@ -3725,6 +3908,8 @@ async fn guardian_review_session_config_clears_legacy_notify() { /*live_network_config*/ None, "active-model", /*reasoning_effort*/ None, + ReasoningSummary::default(), + /*personality*/ None, /*model_messages*/ None, ) .expect("guardian config"); @@ -3760,6 +3945,8 @@ async fn guardian_review_session_config_uses_live_network_proxy_state() { Some(live_network.clone()), "active-model", /*reasoning_effort*/ None, + ReasoningSummary::default(), + /*personality*/ None, /*model_messages*/ None, ) .expect("guardian config"); @@ -3807,6 +3994,8 @@ async fn guardian_review_session_config_disables_mcp_apps_plugins_memories_and_g /*live_network_config*/ None, "active-model", /*reasoning_effort*/ None, + ReasoningSummary::default(), + /*personality*/ None, /*model_messages*/ None, ) .expect("guardian config"); @@ -3839,6 +4028,8 @@ async fn guardian_review_session_config_allows_pinned_disabled_feature() { /*live_network_config*/ None, "active-model", /*reasoning_effort*/ None, + ReasoningSummary::default(), + /*personality*/ None, /*model_messages*/ None, ) .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, "active-model", /*reasoning_effort*/ None, + ReasoningSummary::default(), + /*personality*/ None, /*model_messages*/ None, ) .expect("guardian config"); @@ -3877,6 +4070,8 @@ async fn guardian_review_session_config_keeps_bedrock_provider_for_bedrock_gpt_5 /*live_network_config*/ None, AMAZON_BEDROCK_GPT_5_4_MODEL_ID, Some(ReasoningEffort::Low), + ReasoningSummary::default(), + /*personality*/ None, /*model_messages*/ None, ) .expect("guardian config"); @@ -3932,6 +4127,8 @@ async fn guardian_review_session_config_uses_requirements_guardian_policy_config /*live_network_config*/ None, "active-model", /*reasoning_effort*/ None, + ReasoningSummary::default(), + /*personality*/ None, /*model_messages*/ None, ) .expect("guardian config"); @@ -3972,6 +4169,8 @@ async fn guardian_review_session_config_uses_default_guardian_policy_without_req /*live_network_config*/ None, "active-model", /*reasoning_effort*/ None, + ReasoningSummary::default(), + /*personality*/ None, /*model_messages*/ None, ) .expect("guardian config"); diff --git a/codex-rs/core/src/session/step_settings.rs b/codex-rs/core/src/session/step_settings.rs index d9d99b9b14..87d8d1d10e 100644 --- a/codex-rs/core/src/session/step_settings.rs +++ b/codex-rs/core/src/session/step_settings.rs @@ -113,7 +113,7 @@ impl ResolvedStepSettings { &self.selected.collaboration_mode } - pub(super) fn personality(&self) -> Option { + pub(crate) fn personality(&self) -> Option { self.selected.personality } diff --git a/codex-rs/core/tests/suite/guardian_review.rs b/codex-rs/core/tests/suite/guardian_review.rs index 0593841618..994fbb5f3b 100644 --- a/codex-rs/core/tests/suite/guardian_review.rs +++ b/codex-rs/core/tests/suite/guardian_review.rs @@ -23,6 +23,7 @@ use codex_features::CurrentTimeSource; use codex_features::Feature; use codex_history::RolloutItem; use codex_login::CodexAuth; +use codex_models_manager::manager::RefreshStrategy; use codex_protocol::ThreadId; use codex_protocol::config_types::ApprovalsReviewer; 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::MODEL_SPECIALTY_CYBER; use codex_protocol::openai_models::ModelTokenBudgetConfig; +use codex_protocol::openai_models::ModelVisibility; use codex_protocol::openai_models::ModelsResponse; use codex_protocol::permissions::FileSystemAccessMode; 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_with_namespace; use core_test_support::responses::ev_response_created; -use core_test_support::responses::mount_response_once_match; -use core_test_support::responses::mount_sse_once; +use core_test_support::responses::mount_models_once; use core_test_support::responses::mount_sse_sequence; 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_websocket_server; use core_test_support::skip_if_no_network; use core_test_support::skip_if_sandbox; 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::test_codex; use core_test_support::wait_for_event; +use core_test_support::wait_for_event_match; use core_test_support::wait_for_mcp_server; use pretty_assertions::assert_eq; use serde_json::Value; @@ -948,6 +951,219 @@ async fn guardian_node_repl_policy_follows_production_approval_path( 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::::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::>(); + 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)] async fn guardian_session_is_reused_for_consecutive_tool_reviews_without_prewarm() -> Result<()> { 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::::new(); extensions.tool_lifecycle_contributor(lifecycle_recorder.clone()); let mut builder = test_codex() + .with_model("gpt-5.4") .with_extensions(Arc::new(extensions.build())) .with_config(move |config| { 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(_)) }) .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?; + wait_for_event(&test.codex, |event| { + matches!(event, EventMsg::TurnComplete(_)) + }) + .await; 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!( requests .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)] -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_sandbox!(Ok(())); 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_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!({ - "cmd": command, + "cmd": "printf should-not-run > guardian-interrupted.txt", "yield_time_ms": 1_000_u64, "sandbox_permissions": SandboxPermissions::RequireEscalated, "justification": "Exercise interrupted Guardian approval.", }); - mount_sse_once( - &server, - sse(vec![ - ev_response_created("resp-parent-interrupted-tool"), - ev_function_call( - "exec-call-interrupted", - "exec_command", - &serde_json::to_string(&tool_args)?, - ), - ev_completed("resp-parent-interrupted-tool"), - ]), - ) + let (release_review, review_gate) = tokio::sync::oneshot::channel(); + let (streaming, mut completions) = start_streaming_sse_server(vec![ + vec![StreamingSseChunk { + gate: None, + body: sse(vec![ + ev_response_created("resp-parent-interrupted-tool"), + ev_function_call( + "exec-call-interrupted", + "exec_command", + &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; - let pending_guardian = mount_response_once_match( - &server, - |request: &wiremock::Request| { - serde_json::from_slice::(&request.body) - .ok() - .and_then(|body| { - body.pointer("/client_metadata/x-openai-subagent") - .and_then(Value::as_str) - .map(str::to_owned) - }) - .as_deref() - == Some("guardian") - }, - sse_response(sse(vec![ - 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() - }), - ) + let base_url = format!("{}/v1", streaming.uri()); + let test = test_codex() + .with_model("guardian-parent-a") + .with_config(move |config| { + config.model_provider.base_url = Some(base_url); + config.model_catalog = Some(guardian_parent_catalog()); + config.features.enable(Feature::StepModelSwitching).unwrap(); + config.permissions.approval_policy = Constrained::allow_any(approval_policy); + config.approvals_reviewer = ApprovalsReviewer::AutoReview; + config + .set_legacy_sandbox_policy(sandbox_policy) + .expect("set sandbox policy"); + }) + .build_with_auto_env(&server) .await?; - tokio::time::timeout(Duration::from_secs(5), async { - while pending_guardian.requests().is_empty() { - tokio::task::yield_now().await; - } + test.codex + .start_or_steer_turn(TurnInputRequest::user_input(vec![UserInput::Text { + 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 .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?; wait_for_event(&test.codex, |event| { matches!(event, EventMsg::TurnAborted(_)) }) .await; - tokio::time::sleep(Duration::from_millis(350)).await; - assert!( - !output_file.exists(), - "the interrupted Guardian-reviewed command executed after its delayed approval response" - ); - - 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; + release_review + .send(()) + .expect("release interrupted review response"); + // The cancelled connection may close before the server finishes writing. + let _ = tokio::time::timeout(Duration::from_secs(5), completions.remove(1)).await?; test.codex - .start_or_steer_turn(TurnInputRequest::user_input(vec![UserInput::Text { - text: "verify Guardian cancellation left the next turn clean".into(), - text_elements: Vec::new(), - }])) + .start_or_steer_turn( + TurnInputRequest::user_input(vec![UserInput::Text { + 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?; wait_for_event(&test.codex, |event| { matches!(event, EventMsg::TurnComplete(_)) }) .await; - let follow_up_request = follow_up.single_request(); - assert!( - follow_up_request - .body_contains_text("verify Guardian cancellation left the next turn clean") + let requests = streaming + .requests() + .await + .into_iter() + .map(|body| serde_json::from_slice::(&body)) + .collect::>>()?; + assert_eq!( + requests + .iter() + .map(|body| body["model"].clone()) + .collect::>(), + vec![ + json!("guardian-parent-a"), + json!("guardian-parent-a"), + json!("guardian-parent-b") + ] ); - assert!(follow_up_request.has_function_call("exec-call-interrupted")); - let interrupted_output = follow_up_request - .function_call_output_text("exec-call-interrupted") + assert_eq!( + requests[1]["client_metadata"]["x-openai-subagent"], + "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"); assert!( - interrupted_output.contains("aborted"), + interrupted_output["output"] + .as_str() + .expect("tool output text") + .contains("aborted"), "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(()) }