From a0dcfe2ada3f5bbd5059a34c0fc6fac244741a67 Mon Sep 17 00:00:00 2001 From: jif Date: Wed, 2 Sep 2026 10:23:09 +0000 Subject: [PATCH] Skip Guardian scoring in User approval mode (#42256) ## What changed - Skip Guardian prewarming and asynchronous scoring when `approvalsReviewer` is `"user"`, including when the reviewer changes during an active turn. - Automatically accept ordinary `node_repl.js` execution confirmations in this mode while continuing to surface sensitive-action checks and requests for user input. - Cover transitions between User approval, automatic review, and Full Access in the app-server integration tests. GitOrigin-RevId: 599c8ebb672728b9bf3d64c34377ddec230325f8 --- codex-rs/app-server/README.md | 5 + .../app-server/tests/suite/v2/guardian_v2.rs | 125 ++++++++++++++---- .../app-server/tests/suite/v2/mcp_tool.rs | 5 +- codex-rs/core/src/codex_thread.rs | 14 ++ codex-rs/core/src/session/mcp.rs | 39 ++++-- .../guardian-v2/src/async_scorer/extension.rs | 30 +++-- .../src/async_scorer/extension_tests.rs | 18 ++- 7 files changed, 182 insertions(+), 54 deletions(-) diff --git a/codex-rs/app-server/README.md b/codex-rs/app-server/README.md index dc084fdfbf..a0aa92b7fc 100644 --- a/codex-rs/app-server/README.md +++ b/codex-rs/app-server/README.md @@ -1923,6 +1923,11 @@ persisted rollout errors, so unavailable details after a restart remain a termin ## Approvals +In User approval mode (`approvalsReviewer: "user"`), async Guardian scoring and +prewarming are skipped, and ordinary `node_repl.js` execution confirmations are +accepted automatically. Separate sensitive-action checks and requests for user +input keep their existing behavior. Approve for me and Full Access are unchanged. + Full Access (`approvalPolicy: "never"` with unrestricted selected environments) skips Guardian, including background scoring. Confirmation-only MCP approvals, including strict or sensitive CUA requests, are accepted. Strict responses retain 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 c409e96967..bb217a9f62 100644 --- a/codex-rs/app-server/tests/suite/v2/guardian_v2.rs +++ b/codex-rs/app-server/tests/suite/v2/guardian_v2.rs @@ -23,8 +23,10 @@ use axum::routing::get; use axum::routing::post; use codex_app_server_protocol::ApprovalsReviewer; use codex_app_server_protocol::AskForApproval; +use codex_app_server_protocol::ClientRequest; use codex_app_server_protocol::ItemCompletedNotification; use codex_app_server_protocol::ItemGuardianApprovalReviewStartedNotification; +use codex_app_server_protocol::McpServerElicitationRequest; use codex_app_server_protocol::McpToolCallStatus; use codex_app_server_protocol::SandboxMode; use codex_app_server_protocol::ServerRequest; @@ -39,6 +41,9 @@ use codex_app_server_protocol::ThreadRollbackParams; use codex_app_server_protocol::ThreadRollbackResponse; use codex_app_server_protocol::ThreadStartParams; use codex_app_server_protocol::TurnCompletedNotification; +use codex_app_server_protocol::TurnSettingsUpdateParams; +use codex_app_server_protocol::TurnSettingsUpdateResponse; +use codex_app_server_protocol::TurnSettingsUpdateStatus; use codex_app_server_protocol::TurnStartParams; use codex_app_server_protocol::TurnStartResponse; use codex_app_server_protocol::TurnStatus; @@ -161,6 +166,7 @@ struct MockResponsesState { transcript_content: TranscriptContent, mcp_server_name: Option<&'static str>, mcp_tool_sequence: Option<&'static [&'static str]>, + mcp_messages: Mutex>, root_worker: bool, root_user_restriction: bool, root_user_input_restriction: bool, @@ -448,7 +454,13 @@ async fn parent_response( || (state.root_worker || state.root_user_restriction) && request_number == 3 { let call_id = format!("guardian-action-{request_number}"); - let mut message = format!("guardian-{request_number}"); + let mut message = state + .mcp_messages + .lock() + .expect("MCP messages lock") + .get(request_number) + .map(|message| (*message).to_string()) + .unwrap_or_else(|| format!("guardian-{request_number}")); if request_number == 0 && matches!(state.review_outcome, ReviewOutcome::Deny) { message.push_str(&"x".repeat(2_000)); } @@ -1817,11 +1829,11 @@ async fn first_cua_review_does_not_wait_for_initial_score( } #[tokio::test(flavor = "multi_thread", worker_threads = 2)] -async fn full_access_skips_guardian_until_review_is_enabled() -> Result<()> { +async fn user_approval_skips_async_guardian_without_changing_other_modes() -> Result<()> { skip_if_no_network!(Ok(())); let state = Arc::new(MockResponsesState { mcp_server_name: Some("node_repl"), - mcp_tool_sequence: Some(&["js", "js"]), + mcp_tool_sequence: Some(&["js", "js", "js"]), ..Default::default() }); let listener = TcpListener::bind("127.0.0.1:0").await?; @@ -1832,15 +1844,15 @@ async fn full_access_skips_guardian_until_review_is_enabled() -> Result<()> { let responses_server = tokio::spawn(async move { let _ = axum::serve(listener, router).await; }); - // Exercise strict, sensitive CUA confirmations using the existing MCP server. + // Mix ordinary execution, a user-input form, and a sensitive check. let (mcp_url, mcp_server) = - start_mcp_server_with_tools(&["js"], /*sensitive_action*/ Some(true)).await?; + 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("never") - .with_root_config("approvals_reviewer = \"auto_review\"") + .with_approval_policy("on-request") + .with_root_config("approvals_reviewer = \"user\"") .with_extra_config(&format!( "[mcp_servers.node_repl]\nurl = \"{mcp_url}/mcp\"\ndefault_tools_approval_mode = \"auto\"\n\n[features.guardianv2]\nenabled = true" )) @@ -1856,7 +1868,8 @@ async fn full_access_skips_guardian_until_review_is_enabled() -> Result<()> { .await?; let thread = app_server .start_thread(ThreadStartParams { - approval_policy: Some(AskForApproval::Never), + approval_policy: Some(AskForApproval::OnRequest), + approvals_reviewer: Some(ApprovalsReviewer::User), sandbox: Some(SandboxMode::DangerFullAccess), ..Default::default() }) @@ -1865,26 +1878,82 @@ async fn full_access_skips_guardian_until_review_is_enabled() -> Result<()> { // Leave async scoring pending so reviewed mode must use synchronous Guardian. state.allow_guardian_review.notify_one(); - for approval_policy in [AskForApproval::Never, AskForApproval::OnRequest] { + use ApprovalsReviewer::AutoReview; + use ApprovalsReviewer::User; + use AskForApproval::Never; + use AskForApproval::OnRequest; + for (approval_policy, reviewer, live_reviewer, expected_reviews) in [ + (OnRequest, User, None, 1), + (Never, User, None, 0), + (Never, AutoReview, None, 0), + (OnRequest, User, Some(AutoReview), 2), + (OnRequest, AutoReview, Some(User), 2), + (OnRequest, AutoReview, None, 3), + (OnRequest, User, None, 1), + ] { + let reviews_before = state.guardian_reviews.load(Ordering::SeqCst); + *state.mcp_messages.lock().expect("MCP messages lock") = vec![ + if approval_policy == OnRequest { + "confirm" + } else { + "guardian-0" + }, + "guardian-1", + "sensitive", + ]; state.parent_requests.store(0, Ordering::SeqCst); app_server.clear_message_buffer(); - let completed = timeout( - TIMEOUT, - app_server.start_turn_and_wait_for_completion(TurnStartParams { + let request_id = app_server + .send_turn_start_request(TurnStartParams { thread_id: thread.id.clone(), input: vec![UserInput::Text { text: USER_CONTEXT.to_owned(), text_elements: Vec::new(), }], approval_policy: Some(approval_policy), + approvals_reviewer: Some(reviewer), ..Default::default() - }), - ) - .await??; + }) + .await?; + let started: TurnStartResponse = + timeout(TIMEOUT, app_server.read_response(request_id)).await??; + if approval_policy == OnRequest { + let request = + timeout(TIMEOUT, app_server.read_stream_until_request_message()).await??; + let ServerRequest::McpServerElicitationRequest { request_id, params } = request else { + panic!("expected user input, got {request:?}"); + }; + assert!( + matches!(params.request, McpServerElicitationRequest::Form { message, .. } + if message == "Allow this request?") + ); + if let Some(reviewer) = live_reviewer { + let updated: TurnSettingsUpdateResponse = app_server + .request(|request_id| ClientRequest::TurnSettingsUpdate { + request_id, + params: TurnSettingsUpdateParams { + thread_id: thread.id.clone(), + turn_id: started.turn.id.clone(), + approvals_reviewer: Some(reviewer), + ..Default::default() + }, + }) + .await?; + assert_eq!(updated.status, TurnSettingsUpdateStatus::Applied); + } + app_server + .send_response( + request_id, + json!({ "action": "accept", "content": { "confirmed": true } }), + ) + .await?; + } + let completed: TurnCompletedNotification = + timeout(TIMEOUT, app_server.read_notification("turn/completed")).await??; assert_eq!(completed.turn.status, TurnStatus::Completed); let mut tool_results = Vec::new(); - while tool_results.len() < 2 { + while tool_results.len() < 3 { let completed: ItemCompletedNotification = timeout(TIMEOUT, app_server.read_notification("item/completed")).await??; if let ThreadItem::McpToolCall { @@ -1899,22 +1968,28 @@ async fn full_access_skips_guardian_until_review_is_enabled() -> Result<()> { } assert_eq!( tool_results, - (0..2) + (0..3) .map(|index| ( McpToolCallStatus::Completed, - Some(vec![ - json!({ "type": "text", "text": format!("echo: guardian-{index}") }) - ]), + Some(vec![json!({ "type": "text", "text": match index { + 0 if approval_policy == OnRequest => "accepted".to_string(), + 2 => "echo: sensitive".to_string(), + _ => format!("echo: guardian-{index}"), + } })]), None, )) .collect::>() ); - if approval_policy == AskForApproval::Never { - assert_eq!(state.guardian_reviews.load(Ordering::SeqCst), 0); - assert_eq!(state.luna_connections.load(Ordering::SeqCst), 0); - } else { + assert_eq!( + state.guardian_reviews.load(Ordering::SeqCst), + reviews_before + expected_reviews + ); + if approval_policy == OnRequest + && (reviewer == AutoReview || live_reviewer == Some(AutoReview)) + { wait_for_luna_request(&state, /*index*/ 0).await?; - assert_eq!(state.guardian_reviews.load(Ordering::SeqCst), 2); + } else if reviews_before <= 1 { + assert_eq!(state.luna_connections.load(Ordering::SeqCst), 0); } } app_server.shutdown_gracefully().await?; 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 210ce82351..58bab01e2a 100644 --- a/codex-rs/app-server/tests/suite/v2/mcp_tool.rs +++ b/codex-rs/app-server/tests/suite/v2/mcp_tool.rs @@ -1369,7 +1369,10 @@ impl ServerHandler for ToolAppsMcpServer { // Match the execution approval emitted by the real Node REPL server. approval_meta["connector_id"] = json!("node_repl"); } - if let Some(sensitive_action) = self.sensitive_action { + if let Some(sensitive_action) = self + .sensitive_action + .or((message == "sensitive").then_some(true)) + { approval_meta["codex_sensitive_action"] = json!(sensitive_action); } let result = context diff --git a/codex-rs/core/src/codex_thread.rs b/codex-rs/core/src/codex_thread.rs index 3fa1aac54c..172726f98b 100644 --- a/codex-rs/core/src/codex_thread.rs +++ b/codex-rs/core/src/codex_thread.rs @@ -733,6 +733,20 @@ impl CodexThread { self.session.thread_config_snapshot().await } + /// Returns the active turn's reviewer, including live updates, or the thread default. + pub async fn approvals_reviewer_for_turn(&self, turn_id: &str) -> ApprovalsReviewer { + if let Some((turn, settings, _)) = self + .session + .active_turn_context_and_strict_auto_review() + .await + && turn.sub_id == turn_id + { + settings.approvals_reviewer() + } else { + self.config_snapshot().await.approvals_reviewer + } + } + /// Returns thread-owned settings suitable for rollout persistence and resume. pub async fn thread_settings_snapshot(&self) -> ThreadSettingsSnapshot { self.session.thread_settings_snapshot().await diff --git a/codex-rs/core/src/session/mcp.rs b/codex-rs/core/src/session/mcp.rs index b656980491..46af2d336e 100644 --- a/codex-rs/core/src/session/mcp.rs +++ b/codex-rs/core/src/session/mcp.rs @@ -726,19 +726,34 @@ async fn review_guardian_mcp_elicitation( }; let step_settings = turn_context.current_settings.load_full(); + // User approval skips ordinary CUA checks, not separate sensitive requests. + let user_cua_execution = step_settings.approvals_reviewer() == ApprovalsReviewer::User + && is_node_repl_backed_server(&request.server_name) + && request.elicitation.meta().is_some_and(|meta| { + metadata_str(meta, MCP_ELICITATION_TOOL_NAME_KEY) == Some("js") + && metadata_str(meta, MCP_ELICITATION_CONNECTOR_ID_KEY) == Some("node_repl") + && metadata_str(meta, MCP_ELICITATION_APPROVAL_KIND_KEY) + == Some(MCP_ELICITATION_APPROVAL_KIND_MCP_TOOL_CALL) + && meta.get(MCP_ELICITATION_SENSITIVE_ACTION_KEY) != Some(&Value::Bool(true)) + && meta.get("codex_requires_user_input") != Some(&Value::Bool(true)) + }); + // Full Access skips inference, not the active-turn and cancellation checks. - if turn_context.environments.has_full_access( - turn_context.approval_policy(), - &turn_context - .config - .permissions - .effective_permission_profile(), - ) && matches!( - &request.elicitation, - Elicitation::Mcp(rmcp::model::ElicitRequestParams::FormElicitationParams { - requested_schema, .. - }) if requested_schema.properties.is_empty() - ) { + if (user_cua_execution + || turn_context.environments.has_full_access( + turn_context.approval_policy(), + &turn_context + .config + .permissions + .effective_permission_profile(), + )) + && matches!( + &request.elicitation, + Elicitation::Mcp(rmcp::model::ElicitRequestParams::FormElicitationParams { + requested_schema, .. + }) if requested_schema.properties.is_empty() + ) + { let decision = if cancellation_token.is_cancelled() { ReviewDecision::Abort } else { 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 6f87b7014f..8558577f5d 100644 --- a/codex-rs/ext/guardian-v2/src/async_scorer/extension.rs +++ b/codex-rs/ext/guardian-v2/src/async_scorer/extension.rs @@ -40,6 +40,7 @@ use codex_login::AgentIdentityAuthPolicy; use codex_login::AuthManager; use codex_model_provider::create_model_provider; use codex_protocol::ThreadId; +use codex_protocol::config_types::ApprovalsReviewer; use codex_protocol::mcp::is_node_repl_backed_server; use codex_protocol::mcp::is_node_repl_backed_tool; use codex_protocol::openai_models::ModelInfo; @@ -214,16 +215,18 @@ impl ThreadLifecycleContributor for GuardianV2Extension { .insert(TrustedSkillRoots::from_config(input.config)); input.thread_store.insert(guardian_v2_enabled); - // Keep the sampler available if a later turn leaves Full Access, but do - // not open Guardian connections while Full Access is selected. - if !has_full_access( - input.config.permissions.approval_policy.value(), - &input.config.permissions.effective_permission_profile(), - input - .environments - .iter() - .map(|environment| &environment.config), - ) { + // Keep the sampler available for later automatic review, but do not + // prewarm while User approval mode or Full Access is selected. + if input.config.approvals_reviewer == ApprovalsReviewer::AutoReview + && !has_full_access( + input.config.permissions.approval_policy.value(), + &input.config.permissions.effective_permission_profile(), + input + .environments + .iter() + .map(|environment| &environment.config), + ) + { tokio::spawn(async move { sampler.prewarm().await; }); @@ -493,10 +496,11 @@ impl GuardianV2Extension { return; } }; - // Read current permissions, not the startup config: existing threads can - // enter Full Access after the sampler has already been initialized. + // Use the live reviewer, not the startup config or per-app reviewer overrides. let snapshot = thread.config_snapshot().await; - if snapshot.full_access { + if snapshot.full_access + || thread.approvals_reviewer_for_turn(input.turn_id).await == ApprovalsReviewer::User + { // A skipped call invalidates older scores, including ones still in flight. score_progress .latest_failed_tool_call 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 ae13d85a01..a19c1558a5 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 @@ -34,6 +34,7 @@ use codex_login::ExternalAuthRefreshContext; use codex_model_provider_info::ModelProviderInfo; use codex_protocol::ResponseItemId; use codex_protocol::ThreadId; +use codex_protocol::config_types::ApprovalsReviewer; use codex_protocol::models::ContentItem; use codex_protocol::models::FunctionCallOutputContentItem; use codex_protocol::models::FunctionCallOutputPayload; @@ -115,7 +116,10 @@ async fn installed_extension_warms_connections_without_blocking_thread_start() - skip_if_no_network!(Ok(())); let thread_server = responses::start_mock_server().await; - let test = test_codex().build_with_auto_env(&thread_server).await?; + let test = test_codex() + .with_config(|config| config.approvals_reviewer = ApprovalsReviewer::AutoReview) + .build_with_auto_env(&thread_server) + .await?; let mut connections = vec![ WebSocketConnectionConfig { requests: Vec::new(), @@ -172,7 +176,10 @@ async fn installed_extension_reconnects_after_auth_refresh() -> Result<()> { skip_if_no_network!(Ok(())); let thread_server = responses::start_mock_server().await; - let test = test_codex().build_with_auto_env(&thread_server).await?; + let test = test_codex() + .with_config(|config| config.approvals_reviewer = ApprovalsReviewer::AutoReview) + .build_with_auto_env(&thread_server) + .await?; let events = vec![ ev_assistant_message("sample", "low"), ev_completed("response-1"), @@ -903,7 +910,10 @@ async fn sample_configured_conversation_history_with_source( .policy = Some(TEST_CATALOG_GUARDIAN_POLICY.to_owned()); }) .with_model("gpt-5.5") - .with_config(move |config| config.guardian_policy_config = guardian_policy) + .with_config(move |config| { + config.approvals_reviewer = ApprovalsReviewer::AutoReview; + config.guardian_policy_config = guardian_policy; + }) .with_pre_build_hook(move |home| { std::fs::write(home.join("config.toml"), guardian_config) .expect("Guardian v2 configuration should be written"); @@ -2171,6 +2181,7 @@ async fn contributor_skips_required_models_in_standard_scope() -> Result<()> { let test = test_codex() .with_home(Arc::clone(&initial.home)) .with_config(move |config| { + config.approvals_reviewer = ApprovalsReviewer::AutoReview; config.config_layer_stack = config_layer_stack; config .features @@ -2806,6 +2817,7 @@ async fn contributor_reuses_the_latest_compatible_parent_compaction() -> Result< let thread_server = responses::start_mock_server().await; let test = test_codex() + .with_config(|config| config.approvals_reviewer = ApprovalsReviewer::AutoReview) .with_pre_build_hook(|home| { std::fs::write( home.join("config.toml"),