From 38ba8cdceb536aa55af7db132d6bc830da8c0129 Mon Sep 17 00:00:00 2001 From: jif Date: Thu, 3 Sep 2026 00:40:31 +0000 Subject: [PATCH] Honor model requirements in Guardian computer-use scoring (#42422) ## Why Computer-use-only Guardian scoring should follow the active model's REPL auto-review requirement, including when the model changes within a live thread. ## What changed - Run computer-use scoring and fast approval decisions only when the active model sets `node_repl_auto_review_required`. - Invalidate prior or in-flight scores when a model switch skips scoring, so switching back to a reviewed model cannot revive a stale decision. ## Testing - Cover model switches for both `node_repl` and `cua_repl` MCP servers. - Verify skipped scoring and stale-score rejection across requirement changes. GitOrigin-RevId: 46aec4d017bea8f135b435bcd769b87369b8ce95 --- .../app-server/tests/suite/v2/guardian_v2.rs | 6 +- .../tests/suite/v2/guardian_v2_model_tests.rs | 137 ++++++++++++++++++ .../app-server/tests/suite/v2/mcp_tool.rs | 6 + .../guardian-v2/src/async_scorer/extension.rs | 12 +- .../src/async_scorer/extension_tests.rs | 51 +++++++ 5 files changed, 209 insertions(+), 3 deletions(-) create mode 100644 codex-rs/app-server/tests/suite/v2/guardian_v2_model_tests.rs diff --git a/codex-rs/app-server/tests/suite/v2/guardian_v2.rs b/codex-rs/app-server/tests/suite/v2/guardian_v2.rs index bb217a9f62..6558213dec 100644 --- a/codex-rs/app-server/tests/suite/v2/guardian_v2.rs +++ b/codex-rs/app-server/tests/suite/v2/guardian_v2.rs @@ -77,6 +77,9 @@ use super::mcp_tool::start_mcp_server_with_tools; #[path = "guardian_v2_history_tests.rs"] mod history; +#[path = "guardian_v2_model_tests.rs"] +mod model_tests; + const TIMEOUT: Duration = Duration::from_secs(30); const MODEL: &str = "mock-model"; const REQUIRED_MODEL: &str = "protected-model"; @@ -591,7 +594,8 @@ async fn guardian_v2_routes_scoped_tool_approvals( codex_protocol::mcp::is_node_repl_backed_server(server_name) } }; - let node_repl_review_required = matches!(requirement, ModelReviewRequirement::Required) + let node_repl_review_required = (matches!(requirement, ModelReviewRequirement::Required) + || matches!(scope, GuardianToolScope::ComputerUseOnly { .. })) && codex_protocol::mcp::is_node_repl_backed_server(server_name); let late_root_restriction = matches!( lifecycle, diff --git a/codex-rs/app-server/tests/suite/v2/guardian_v2_model_tests.rs b/codex-rs/app-server/tests/suite/v2/guardian_v2_model_tests.rs new file mode 100644 index 0000000000..e2fb38d0ff --- /dev/null +++ b/codex-rs/app-server/tests/suite/v2/guardian_v2_model_tests.rs @@ -0,0 +1,137 @@ +//! Verifies model-required CUA scoring across model changes in a live thread. + +use std::sync::Arc; +use std::sync::Mutex; +use std::sync::atomic::Ordering; + +use anyhow::Result; +use app_test_support::MockResponsesConfig; +use app_test_support::TestAppServer; +use app_test_support::write_models_cache_with_models; +use axum::Router; +use axum::routing::get; +use codex_app_server_protocol::ApprovalsReviewer; +use codex_app_server_protocol::ThreadStartParams; +use codex_app_server_protocol::ThreadStartResponse; +use codex_app_server_protocol::TurnCompletedNotification; +use codex_app_server_protocol::TurnStartParams; +use codex_app_server_protocol::TurnStartResponse; +use codex_app_server_protocol::TurnStatus; +use codex_app_server_protocol::UserInput; +use codex_features::Feature; +use core_test_support::load_default_config_for_test; +use core_test_support::skip_if_no_network; +use pretty_assertions::assert_eq; +use tempfile::TempDir; +use test_case::test_case; +use tokio::net::TcpListener; +use tokio::time::timeout; + +use super::MODEL; +use super::MockResponsesState; +use super::TIMEOUT; +use super::USER_CONTEXT; +use super::luna_websocket; +use super::parent_response; +use super::start_mcp_server_with_tools; +use super::wait_for_luna_request; + +#[test_case("node_repl"; "browser")] +#[test_case("cua_repl"; "computer use")] +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn computer_use_scoring_follows_model_review_requirement( + server_name: &'static str, +) -> Result<()> { + skip_if_no_network!(Ok(())); + + const REVIEWED_MODEL: &str = "reviewed-model"; + let state = Arc::new(MockResponsesState { + mcp_server_name: Some(server_name), + mcp_tool_sequence: Some(&["js"]), + mcp_messages: Mutex::new(vec!["hello"]), + ..Default::default() + }); + let listener = TcpListener::bind("127.0.0.1:0").await?; + let responses_url = format!("http://{}", listener.local_addr()?); + let router = Router::new() + .route("/v1/responses", get(luna_websocket).post(parent_response)) + .with_state(Arc::clone(&state)); + let responses_server = tokio::spawn(async move { + let _ = axum::serve(listener, router).await; + }); + let (mcp_url, mcp_server) = + start_mcp_server_with_tools(&["js"], /*sensitive_action*/ None).await?; + let codex_home = TempDir::new()?; + MockResponsesConfig::new(&responses_url) + .with_model(MODEL) + .with_provider_config("supports_websockets = false") + .with_approval_policy("on-request") + .with_root_config("approvals_reviewer = \"auto_review\"") + .with_extra_config(&format!( + "[mcp_servers.{server_name}]\nurl = \"{mcp_url}/mcp\"\ndefault_tools_approval_mode = \"auto\"\n\n[features.guardianv2]\nenabled = true" + )) + .enable_feature(Feature::GuardianApproval) + .write(codex_home.path())?; + let config = load_default_config_for_test(&codex_home).await; + let ordinary_model = codex_core::test_support::construct_model_info_offline(MODEL, &config); + let mut reviewed_model = + codex_core::test_support::construct_model_info_offline(REVIEWED_MODEL, &config); + reviewed_model.node_repl_auto_review_required = true; + write_models_cache_with_models(codex_home.path(), vec![ordinary_model, reviewed_model])?; + let mut app_server = TestAppServer::builder() + .with_codex_home(codex_home.path()) + .build_initialized_with_timeout(TIMEOUT) + .await?; + let request_id = app_server + .send_thread_start_request_with_auto_env(ThreadStartParams { + approvals_reviewer: Some(ApprovalsReviewer::AutoReview), + ..Default::default() + }) + .await?; + let thread: ThreadStartResponse = + timeout(TIMEOUT, app_server.read_response(request_id)).await??; + + state.allow_guardian_review.notify_one(); + for (model, expected_samples) in [ + (MODEL, 0), + (REVIEWED_MODEL, 1), + (MODEL, 1), + (REVIEWED_MODEL, 2), + ] { + state.parent_requests.store(0, Ordering::SeqCst); + app_server.clear_message_buffer(); + state.allow_luna.notify_one(); + let request_id = app_server + .send_turn_start_request(TurnStartParams { + thread_id: thread.thread.id.clone(), + model: Some(model.to_owned()), + input: vec![UserInput::Text { + text: USER_CONTEXT.to_owned(), + text_elements: Vec::new(), + }], + ..Default::default() + }) + .await?; + let _: TurnStartResponse = timeout(TIMEOUT, app_server.read_response(request_id)).await??; + let completed: TurnCompletedNotification = + timeout(TIMEOUT, app_server.read_notification("turn/completed")).await??; + assert_eq!(completed.turn.status, TurnStatus::Completed); + if model == REVIEWED_MODEL { + wait_for_luna_request(&state, expected_samples - 1).await?; + } + assert_eq!( + state + .luna_requests + .lock() + .expect("luna requests lock") + .len(), + expected_samples, + "only models requiring REPL review should send classifier requests" + ); + } + + app_server.shutdown_gracefully().await?; + mcp_server.abort(); + responses_server.abort(); + Ok(()) +} diff --git a/codex-rs/app-server/tests/suite/v2/mcp_tool.rs b/codex-rs/app-server/tests/suite/v2/mcp_tool.rs index 58bab01e2a..4270a17199 100644 --- a/codex-rs/app-server/tests/suite/v2/mcp_tool.rs +++ b/codex-rs/app-server/tests/suite/v2/mcp_tool.rs @@ -1391,6 +1391,12 @@ impl ServerHandler for ToolAppsMcpServer { .map_err(|err| { rmcp::ErrorData::internal_error(err.to_string(), /*data*/ None) })?; + if matches!(result.action, ElicitationAction::Decline) { + return Ok(CallToolResult::error(vec![ContentBlock::text( + "Tool execution was declined by Guardian.", + )]) + .into()); + } assert_eq!( serde_json::to_value(result).expect("elicitation response"), json!({ diff --git a/codex-rs/ext/guardian-v2/src/async_scorer/extension.rs b/codex-rs/ext/guardian-v2/src/async_scorer/extension.rs index 8558577f5d..c7e7295566 100644 --- a/codex-rs/ext/guardian-v2/src/async_scorer/extension.rs +++ b/codex-rs/ext/guardian-v2/src/async_scorer/extension.rs @@ -280,6 +280,9 @@ impl ApprovalReviewContributor for GuardianV2Extension { .get("server") .and_then(serde_json::Value::as_str) .is_some_and(is_node_repl_backed_server) + || !thread_store + .get::() + .is_some_and(|model| model.node_repl_auto_review_required) { record_fast_decision(extension_metrics.as_deref(), "deferred", "out_of_scope"); return None; @@ -498,16 +501,21 @@ impl GuardianV2Extension { }; // Use the live reviewer, not the startup config or per-app reviewer overrides. let snapshot = thread.config_snapshot().await; + let parent_model = input.thread_store.get::(); if snapshot.full_access || thread.approvals_reviewer_for_turn(input.turn_id).await == ApprovalsReviewer::User + || (guardian_config.review_scope == GuardianV2ReviewScope::ComputerUseOnly + && !parent_model + .as_ref() + .is_some_and(|model| model.node_repl_auto_review_required)) { - // A skipped call invalidates older scores, including ones still in flight. + // A skipped call invalidates older scores, including ones still in flight + // when switching to a model that does not require REPL review. score_progress .latest_failed_tool_call .fetch_max(tool_call_index, Ordering::Release); return; } - let parent_model = input.thread_store.get::(); // Computer-use-only scores cannot approve other tools for required models. if guardian_config.review_scope != GuardianV2ReviewScope::ComputerUseOnly && parent_model.as_ref().is_some_and(|model| { diff --git a/codex-rs/ext/guardian-v2/src/async_scorer/extension_tests.rs b/codex-rs/ext/guardian-v2/src/async_scorer/extension_tests.rs index a19c1558a5..9a7011f879 100644 --- a/codex-rs/ext/guardian-v2/src/async_scorer/extension_tests.rs +++ b/codex-rs/ext/guardian-v2/src/async_scorer/extension_tests.rs @@ -47,6 +47,7 @@ use codex_protocol::models::MessagePhase; use codex_protocol::models::ReasoningItemReasoningSummary; use codex_protocol::openai_models::GuardianV2ModelConfig; use codex_protocol::openai_models::GuardianV2TranscriptModelConfig; +use codex_protocol::openai_models::ModelInfo; use codex_protocol::openai_models::ReasoningEffort; use codex_protocol::protocol::EventMsg; use codex_protocol::protocol::Op; @@ -229,6 +230,13 @@ async fn installed_extension_reconnects_after_auth_refresh() -> Result<()> { let progress = thread_store .get::() .expect("Guardian v2 should initialize"); + let mut model = test + .thread_manager + .get_models_manager() + .get_model_info("gpt-5.5", &config.to_models_manager_config()) + .await; + model.node_repl_auto_review_required = true; + thread_store.insert(model); let turn_store = ExtensionData::new("turn-1"); let tool_name = ToolName::namespaced("mcp__node_repl__", "js"); let payload = ToolPayload::Function { @@ -550,6 +558,14 @@ async fn computer_use_only_scores_cannot_approve_other_actions() -> Result<()> { let fixture = GuardianFailureFixture::new().await?; let thread_store = fixture.test.codex.thread_extension_data(); + let mut model = fixture + .test + .thread_manager + .get_models_manager() + .get_model_info("gpt-5.5", &fixture.test.config.to_models_manager_config()) + .await; + model.node_repl_auto_review_required = true; + thread_store.insert(model); let mut config = thread_store .get::() .expect("Guardian v2 should have initialized") @@ -687,6 +703,41 @@ async fn computer_use_only_scores_cannot_approve_other_actions() -> Result<()> { ] ); + let mut model = thread_store.get::().unwrap().as_ref().clone(); + model.node_repl_auto_review_required = false; + thread_store.insert(model.clone()); + fixture + .score_tool(ToolName::namespaced("mcp__node_repl__", "js")) + .await; + assert!( + progress.latest_failed_tool_call.load(Ordering::Acquire) + > progress.latest_scored_tool_call.load(Ordering::Acquire) + ); + for required in [false, true] { + model.node_repl_auto_review_required = required; + thread_store.insert(model.clone()); + // Also reject a low score published by an older, in-flight classifier. + thread_store.insert(SecurityRiskScore { + scores: BTreeMap::from([("action_risk".to_owned(), 0.0)]), + call_id: None, + action: None, + sampled_at: None, + }); + assert_eq!( + fixture + .registry + .fast_approval_decision( + &fixture.session_store, + thread_store, + r#"{"tool":"mcp_tool_call","server":"node_repl","tool_name":"js"}"#, + /*extension_metrics*/ None, + ) + .await, + None, + "switching back to a reviewed model must not revive a skipped score" + ); + } + Ok(()) }