From daa3eaf10fda93ad8949b926c059dd8cc399f76a Mon Sep 17 00:00:00 2001 From: jif Date: Wed, 26 Aug 2026 19:56:13 +0000 Subject: [PATCH] Allow Guardian scoring for required computer-use models (#40967) ## Why Models that require automatic review previously skipped Guardian v2 risk scoring entirely, even when Guardian was limited to computer-use tools. That scope cannot use a low-risk result to approve unrelated tools, so the blanket skip is unnecessary. ## What changed - Retain Guardian v2 risk scoring for required-review models in `ComputerUseOnly` scope while continuing to skip it in standard scope. - Preserve strict automatic approval review for Node REPL-backed tools, including when classification fails. ## Testing Added coverage for low-risk, high-risk, and invalid classifications across the Node REPL and computer-use REPL paths, plus an unrelated MCP tool. GitOrigin-RevId: ee38fabf82e196df0ae13b4216b5892be9b799cf --- .../app-server/tests/suite/v2/guardian_v2.rs | 89 +++++++++++++++---- .../app-server/tests/suite/v2/mcp_tool.rs | 42 +++++++++ .../guardian-v2/src/async_scorer/extension.rs | 15 ++-- .../src/async_scorer/extension_tests.rs | 12 ++- 4 files changed, 133 insertions(+), 25 deletions(-) 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 3d27bc98bb..458079c6fa 100644 --- a/codex-rs/app-server/tests/suite/v2/guardian_v2.rs +++ b/codex-rs/app-server/tests/suite/v2/guardian_v2.rs @@ -10,6 +10,7 @@ use app_test_support::MockResponsesConfig; use app_test_support::TestAppServer; use app_test_support::create_fake_rollout; use app_test_support::rollout_path; +use app_test_support::write_models_cache_with_models; use axum::Json; use axum::Router; use axum::extract::State; @@ -39,6 +40,7 @@ use codex_app_server_protocol::UserInput; use codex_features::Feature; use codex_state::StateRuntime; use codex_utils_absolute_path::test_support::PathExt; +use core_test_support::load_default_config_for_test; use core_test_support::responses; use core_test_support::skip_if_no_network; use pretty_assertions::assert_eq; @@ -79,6 +81,7 @@ struct MockResponsesState { classification_completed: Notify, truncation_recorded: Notify, luna_score: f64, + invalid_classification: bool, review_outcome: ReviewOutcome, transcript_content: TranscriptContent, mcp_server_name: Option<&'static str>, @@ -108,6 +111,7 @@ enum GuardianRisk { Low, Threshold, High, + InvalidResponse, } #[derive(Clone, Copy)] @@ -404,7 +408,9 @@ async fn luna_websocket( .push(request); state.allow_luna.notified().await; } - let classification = if state.luna_score < 0.5 { + let classification = if state.invalid_classification { + "invalid" + } else if state.luna_score < 0.5 { "low" } else { "high" @@ -457,14 +463,19 @@ async fn guardian_v2_routes_scoped_tool_approvals( GuardianToolScope::AllTools => TEST_SERVER_NAME, GuardianToolScope::ComputerUseOnly { server_name } => server_name, }; - let classifier_in_scope = !matches!(scope, GuardianToolScope::ComputerUseOnly { .. }) - || codex_protocol::mcp::is_node_repl_backed_server(server_name); - let (luna_score, expected_guardian_reviews) = match (requirement, risk) { - (ModelReviewRequirement::Required, _) => (0.25, 2), - (ModelReviewRequirement::Optional, _) if !classifier_in_scope => (0.25, 2), - (ModelReviewRequirement::Optional, GuardianRisk::Low) => (0.25, 1), - (ModelReviewRequirement::Optional, GuardianRisk::Threshold) => (0.5, 2), - (ModelReviewRequirement::Optional, GuardianRisk::High) => (0.95, 2), + let classifier_in_scope = match scope { + GuardianToolScope::AllTools => matches!(requirement, ModelReviewRequirement::Optional), + GuardianToolScope::ComputerUseOnly { .. } => { + codex_protocol::mcp::is_node_repl_backed_server(server_name) + } + }; + let node_repl_review_required = matches!(requirement, ModelReviewRequirement::Required) + && codex_protocol::mcp::is_node_repl_backed_server(server_name); + let (luna_score, expected_guardian_reviews) = match risk { + GuardianRisk::Low if classifier_in_scope => (0.25, 1), + GuardianRisk::Low | GuardianRisk::InvalidResponse => (0.25, 2), + GuardianRisk::Threshold => (0.5, 2), + GuardianRisk::High => (0.95, 2), }; let expected_guardian_reviews = expected_guardian_reviews * if matches!(review_outcome, ReviewOutcome::Malformed) { @@ -474,6 +485,7 @@ async fn guardian_v2_routes_scoped_tool_approvals( }; let responses_state = Arc::new(MockResponsesState { luna_score, + invalid_classification: matches!(risk, GuardianRisk::InvalidResponse), review_outcome, transcript_content, mcp_server_name: Some(server_name), @@ -544,13 +556,18 @@ async fn guardian_v2_routes_scoped_tool_approvals( } GuardianToolScope::ComputerUseOnly { .. } => "\n\n[features.guardianv2]\nenabled = true", }; + let tool_approval_mode = if node_repl_review_required { + "auto" + } else { + "prompt" + }; let mut mock_config = MockResponsesConfig::new(&responses_url) .with_model(MODEL) .with_provider_config("supports_websockets = false") .with_approval_policy("on-request") .with_root_config(reviewer_config) .with_extra_config(&format!( - "[mcp_servers.{server_name}]\nurl = \"{mcp_server_url}/mcp\"\ndefault_tools_approval_mode = \"prompt\"\n\n[analytics]\nenabled = true\n\n[otel]\nmetrics_exporter = {{ otlp-http = {{ endpoint = \"{responses_url}/metrics\", protocol = \"json\" }} }}{guardian_scope_config}" + "[mcp_servers.{server_name}]\nurl = \"{mcp_server_url}/mcp\"\ndefault_tools_approval_mode = \"{tool_approval_mode}\"\n\n[analytics]\nenabled = true\n\n[otel]\nmetrics_exporter = {{ otlp-http = {{ endpoint = \"{responses_url}/metrics\", protocol = \"json\" }} }}{guardian_scope_config}" )) .enable_feature(Feature::GuardianApproval); if lifecycle.has_user_input() || lifecycle.has_root_user_input() { @@ -562,6 +579,12 @@ async fn guardian_v2_routes_scoped_tool_approvals( .enable_feature(Feature::MultiAgentV2); } mock_config.write(codex_home.path())?; + if node_repl_review_required { + let config = load_default_config_for_test(&codex_home).await; + let mut model_info = codex_core::test_support::construct_model_info_offline(MODEL, &config); + model_info.node_repl_auto_review_required = true; + write_models_cache_with_models(codex_home.path(), vec![model_info])?; + } let original_thread_id = match lifecycle { ThreadLifecycle::New | ThreadLifecycle::UserInputRestriction @@ -696,7 +719,7 @@ async fn guardian_v2_routes_scoped_tool_approvals( assert_eq!(reviewed_thread_id, thread_id); } - if matches!(requirement, ModelReviewRequirement::Optional) && classifier_in_scope { + if classifier_in_scope { let luna_request = wait_for_luna_request(responses_state.as_ref(), /*index*/ 0).await?; assert_eq!( luna_request["prompt_cache_key"], @@ -791,7 +814,11 @@ async fn guardian_v2_routes_scoped_tool_approvals( 2 ); } - assert!(reviews[0].contains("guardian-action-0")); + if node_repl_review_required { + assert!(reviews[0].contains(&format!("mcp_elicitation:{server_name}:"))); + } else { + assert!(reviews[0].contains("guardian-action-0")); + } assert!(reviews[0].contains("guardian-0")); assert!(!reviews[0].contains("guardian-action-1")); assert!(reviews[0].contains(match review_outcome { @@ -857,19 +884,21 @@ async fn guardian_v2_routes_scoped_tool_approvals( "the synchronous Guardian reviewer must also see the genuine user answer" ); } - if matches!(requirement, ModelReviewRequirement::Required) || !classifier_in_scope { + if !classifier_in_scope { assert!( responses_state .luna_requests .lock() .expect("Luna request lock should not be poisoned") .is_empty(), - "out-of-scope tools and protected models must not receive Guardian v2 risk scoring" + "tools outside the permitted classification scope must not receive risk scoring" ); } - let requires_strict_review = matches!(requirement, ModelReviewRequirement::Optional) - && classifier_in_scope - && matches!(risk, GuardianRisk::Threshold | GuardianRisk::High); + let requires_strict_review = classifier_in_scope + && matches!( + risk, + GuardianRisk::Threshold | GuardianRisk::High | GuardianRisk::InvalidResponse + ); let strict_review_count = app_server .pending_notification_methods() .into_iter() @@ -897,7 +926,7 @@ async fn guardian_v2_routes_scoped_tool_approvals( ); } - if matches!(requirement, ModelReviewRequirement::Optional) && classifier_in_scope { + if classifier_in_scope && !matches!(risk, GuardianRisk::InvalidResponse) { let state_db = StateRuntime::init( codex_state::SqliteConfig::new_for_testing(codex_home.path().abs()), "mock_provider".to_owned(), @@ -1057,6 +1086,30 @@ async fn guardian_v2_computer_use_only_scopes_classification_and_fast_reviews( .await } +#[test_case("node_repl", GuardianRisk::Low; "browser low risk")] +#[test_case("cua_repl", GuardianRisk::Low; "computer use low risk")] +#[test_case("node_repl", GuardianRisk::High; "browser high risk")] +#[test_case("cua_repl", GuardianRisk::High; "computer use high risk")] +#[test_case("node_repl", GuardianRisk::InvalidResponse; "browser classifier failure")] +#[test_case("cua_repl", GuardianRisk::InvalidResponse; "computer use classifier failure")] +#[test_case(TEST_SERVER_NAME, GuardianRisk::Low; "other tools retain full review")] +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn guardian_v2_required_model_computer_use_preserves_strict_approval( + server_name: &'static str, + risk: GuardianRisk, +) -> Result<()> { + skip_if_no_network!(Ok(())); + guardian_v2_routes_scoped_tool_approvals( + risk, + ThreadLifecycle::New, + ModelReviewRequirement::Required, + ReviewOutcome::Allow, + TranscriptContent::Normal, + GuardianToolScope::ComputerUseOnly { server_name }, + ) + .await +} + #[test_case(ReviewOutcome::Allow; "approved_evidence")] #[test_case(ReviewOutcome::Deny; "denied_evidence")] #[tokio::test(flavor = "multi_thread", worker_threads = 2)] 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 2e40008067..6aa309c4f6 100644 --- a/codex-rs/app-server/tests/suite/v2/mcp_tool.rs +++ b/codex-rs/app-server/tests/suite/v2/mcp_tool.rs @@ -63,6 +63,7 @@ use rmcp::model::JsonObject; use rmcp::model::ListToolsResult; use rmcp::model::MetaObject; use rmcp::model::PrimitiveSchemaDefinition; +use rmcp::model::RequestMetaObject; use rmcp::model::ServerCapabilities; use rmcp::model::ServerInfo; use rmcp::model::Tool; @@ -1265,6 +1266,47 @@ impl ServerHandler for ToolAppsMcpServer { let mut meta = MetaObject::new(); meta.0.insert("calledBy".to_string(), json!("mcp-app")); + // Node REPL requests strict review inside tools/call, despite its read-only annotation. + let turn_metadata = context.meta.0.0.get("x-codex-turn-metadata"); + if turn_metadata + .and_then(|metadata| metadata.get("node_repl_auto_review_required")) + .and_then(serde_json::Value::as_bool) + == Some(true) + { + let approval_meta = json!({ + "codex_request_type": "approval_request", + "codex_approval_kind": "mcp_tool_call", + "codex_strict_auto_review": true, + "tool_name": TEST_TOOL_NAME, + "tool_params": request.arguments, + "x-codex-turn-metadata": turn_metadata, + }); + let result = context + .peer + .create_elicitation(ElicitRequestParams::FormElicitationParams { + meta: Some(RequestMetaObject::from( + approval_meta + .as_object() + .expect("approval metadata") + .clone(), + )), + message: "Review tool execution".to_string(), + requested_schema: ElicitationSchema::new(Default::default()), + }) + .await + .map_err(|err| { + rmcp::ErrorData::internal_error(err.to_string(), /*data*/ None) + })?; + assert_eq!( + serde_json::to_value(result).expect("elicitation response"), + json!({ + "action": "accept", + "content": {}, + "_meta": { "approvals_reviewer": "auto_review" }, + }) + ); + } + if message == LARGE_RESPONSE_MESSAGE { let large_text = "large-mcp-content-".repeat(DEFAULT_OUTPUT_BYTES_CAP / 8); let mut result = CallToolResult::structured(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 e1cf0efb3a..d3d33c5483 100644 --- a/codex-rs/ext/guardian-v2/src/async_scorer/extension.rs +++ b/codex-rs/ext/guardian-v2/src/async_scorer/extension.rs @@ -525,12 +525,15 @@ impl GuardianV2Extension { } }; let parent_model = input.thread_store.get::(); - if parent_model.as_ref().is_some_and(|model| { - config - .config_layer_stack - .requirements() - .auto_review_required_for_model(&model.slug) - }) { + // 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| { + config + .config_layer_stack + .requirements() + .auto_review_required_for_model(&model.slug) + }) + { input.thread_store.remove::(); return; } 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 f8fb1ed449..50080ed702 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 @@ -1901,7 +1901,7 @@ async fn contributor_persists_nested_code_mode_action_with_score() -> Result<()> } #[tokio::test(flavor = "multi_thread", worker_threads = 2)] -async fn contributor_skips_models_requiring_managed_guardian_review() -> Result<()> { +async fn contributor_skips_required_models_in_standard_scope() -> Result<()> { skip_if_no_network!(Ok(())); let thread_server = responses::start_mock_server().await; @@ -1961,6 +1961,16 @@ async fn contributor_skips_models_requiring_managed_guardian_review() -> Result< }) .await; + let mut guardian_config = thread_store + .get::() + .expect("Guardian v2 should have initialized") + .as_ref() + .clone(); + guardian_config.review_scope = GuardianV2ReviewScope::Standard { + sandboxed_exec_commands: false, + }; + thread_store.insert(guardian_config); + let mut model_info = test .thread_manager .get_models_manager()