From a5ebedef67983b258bf82531ce6b95d59d9f9d05 Mon Sep 17 00:00:00 2001 From: maja-openai <163171781+maja-openai@users.noreply.github.com> Date: Thu, 30 Apr 2026 16:44:09 -0700 Subject: [PATCH] Bypass review for always-allow MCP tools in auto-review (#20069) ## Why When an MCP or app tool is configured with approval mode `approve` (always allow), users expect that decision to be authoritative. In guardian auto-review mode, ARC could still return `ask-user`, which then routed the approval question into guardian with the ARC reason as context. That meant a tool explicitly configured as always allowed still went through both safety monitors before running. This change keeps the existing ARC behavior for non-auto-review sessions, but avoids the ARC-to-guardian sequence when `approvals_reviewer = auto_review` and the tool approval mode is `approve`. ## What changed - Short-circuit MCP tool approval handling when `approval_mode == approve` and `approvals_reviewer == auto_review`. - Updated the MCP approval regression test so the auto-review case asserts neither ARC nor guardian is called. - Preserved existing tests that verify ARC can still block always-allow MCP tools outside guardian auto-review mode. ## Verification - `cargo test -p codex-core --lib mcp_tool_call` --- codex-rs/codex-mcp/src/elicitation.rs | 8 ++- codex-rs/codex-mcp/src/lib.rs | 1 + codex-rs/codex-mcp/src/mcp/mod.rs | 18 +++++++ codex-rs/codex-mcp/src/mcp/mod_tests.rs | 57 +++++++++++++++++++++ codex-rs/core/src/mcp_skill_dependencies.rs | 2 + codex-rs/core/src/mcp_tool_call.rs | 5 ++ codex-rs/core/src/mcp_tool_call_tests.rs | 34 ++++-------- 7 files changed, 98 insertions(+), 27 deletions(-) diff --git a/codex-rs/codex-mcp/src/elicitation.rs b/codex-rs/codex-mcp/src/elicitation.rs index 101bda412..def12a9d6 100644 --- a/codex-rs/codex-mcp/src/elicitation.rs +++ b/codex-rs/codex-mcp/src/elicitation.rs @@ -9,6 +9,7 @@ use std::collections::HashMap; use std::sync::Arc; use std::sync::Mutex as StdMutex; +use crate::mcp::McpPermissionPromptAutoApproveContext; use crate::mcp::mcp_permission_prompt_is_auto_approved; use anyhow::Context; use anyhow::Result; @@ -87,8 +88,11 @@ impl ElicitationRequestManager { .lock() .map(|profile| profile.clone()) .unwrap_or_default(); - if mcp_permission_prompt_is_auto_approved(approval_policy, &permission_profile) - && can_auto_accept_elicitation(&elicitation) + if mcp_permission_prompt_is_auto_approved( + approval_policy, + &permission_profile, + McpPermissionPromptAutoApproveContext::default(), + ) && can_auto_accept_elicitation(&elicitation) { return Ok(ElicitationResponse { action: ElicitationAction::Accept, diff --git a/codex-rs/codex-mcp/src/lib.rs b/codex-rs/codex-mcp/src/lib.rs index 1d3fd1761..9d4ee60e8 100644 --- a/codex-rs/codex-mcp/src/lib.rs +++ b/codex-rs/codex-mcp/src/lib.rs @@ -34,6 +34,7 @@ pub use mcp::resolve_oauth_scopes; pub use mcp::should_retry_without_scopes; pub use codex_apps::filter_non_codex_apps_mcp_tools_only; +pub use mcp::McpPermissionPromptAutoApproveContext; pub use mcp::mcp_permission_prompt_is_auto_approved; pub use mcp::qualified_mcp_tool_name_prefix; pub use tools::declared_openai_file_input_param_names; diff --git a/codex-rs/codex-mcp/src/mcp/mod.rs b/codex-rs/codex-mcp/src/mcp/mod.rs index e684bf6e5..3cfd4d01e 100644 --- a/codex-rs/codex-mcp/src/mcp/mod.rs +++ b/codex-rs/codex-mcp/src/mcp/mod.rs @@ -20,6 +20,8 @@ use async_channel::unbounded; use codex_config::Constrained; use codex_config::McpServerConfig; use codex_config::McpServerTransportConfig; +use codex_config::types::AppToolApproval; +use codex_config::types::ApprovalsReviewer; use codex_config::types::OAuthCredentialsStoreMode; use codex_login::CodexAuth; use codex_plugin::PluginCapabilitySummary; @@ -67,7 +69,17 @@ pub fn qualified_mcp_tool_name_prefix(server_name: &str) -> String { pub fn mcp_permission_prompt_is_auto_approved( approval_policy: AskForApproval, permission_profile: &PermissionProfile, + context: McpPermissionPromptAutoApproveContext, ) -> bool { + if matches!( + approval_policy, + AskForApproval::OnRequest | AskForApproval::Granular(_) + ) && context.approvals_reviewer == Some(ApprovalsReviewer::AutoReview) + && context.tool_approval_mode == Some(AppToolApproval::Approve) + { + return true; + } + if approval_policy != AskForApproval::Never { return false; } @@ -80,6 +92,12 @@ pub fn mcp_permission_prompt_is_auto_approved( } } +#[derive(Clone, Copy, Debug, Default, Eq, PartialEq)] +pub struct McpPermissionPromptAutoApproveContext { + pub approvals_reviewer: Option, + pub tool_approval_mode: Option, +} + /// MCP runtime settings derived from `codex_core::config::Config`. /// /// This struct should contain only long-lived configuration values that the diff --git a/codex-rs/codex-mcp/src/mcp/mod_tests.rs b/codex-rs/codex-mcp/src/mcp/mod_tests.rs index ef9fc9bfa..fa5cbf1f7 100644 --- a/codex-rs/codex-mcp/src/mcp/mod_tests.rs +++ b/codex-rs/codex-mcp/src/mcp/mod_tests.rs @@ -1,5 +1,7 @@ use super::*; use codex_config::Constrained; +use codex_config::types::AppToolApproval; +use codex_config::types::ApprovalsReviewer; use codex_login::CodexAuth; use codex_plugin::AppConnectorId; use codex_plugin::PluginCapabilitySummary; @@ -7,6 +9,7 @@ use codex_protocol::models::ManagedFileSystemPermissions; use codex_protocol::models::PermissionProfile; use codex_protocol::permissions::NetworkSandboxPolicy; use codex_protocol::protocol::AskForApproval; +use codex_protocol::protocol::GranularApprovalConfig; use pretty_assertions::assert_eq; use std::collections::HashMap; use std::path::PathBuf; @@ -45,6 +48,7 @@ fn mcp_prompt_auto_approval_honors_unrestricted_managed_profiles() { file_system: ManagedFileSystemPermissions::Unrestricted, network: NetworkSandboxPolicy::Enabled, }, + McpPermissionPromptAutoApproveContext::default(), )); assert!(mcp_permission_prompt_is_auto_approved( AskForApproval::Never, @@ -52,10 +56,12 @@ fn mcp_prompt_auto_approval_honors_unrestricted_managed_profiles() { file_system: ManagedFileSystemPermissions::Unrestricted, network: NetworkSandboxPolicy::Restricted, }, + McpPermissionPromptAutoApproveContext::default(), )); assert!(!mcp_permission_prompt_is_auto_approved( AskForApproval::Never, &PermissionProfile::read_only(), + McpPermissionPromptAutoApproveContext::default(), )); assert!(!mcp_permission_prompt_is_auto_approved( AskForApproval::OnRequest, @@ -63,6 +69,57 @@ fn mcp_prompt_auto_approval_honors_unrestricted_managed_profiles() { file_system: ManagedFileSystemPermissions::Unrestricted, network: NetworkSandboxPolicy::Enabled, }, + McpPermissionPromptAutoApproveContext::default(), + )); +} + +#[test] +fn mcp_prompt_auto_approval_honors_auto_review_approved_tools() { + assert!(mcp_permission_prompt_is_auto_approved( + AskForApproval::OnRequest, + &PermissionProfile::read_only(), + McpPermissionPromptAutoApproveContext { + approvals_reviewer: Some(ApprovalsReviewer::AutoReview), + tool_approval_mode: Some(AppToolApproval::Approve), + }, + )); + assert!(mcp_permission_prompt_is_auto_approved( + AskForApproval::Granular(GranularApprovalConfig { + sandbox_approval: true, + rules: true, + skill_approval: true, + request_permissions: true, + mcp_elicitations: true, + }), + &PermissionProfile::read_only(), + McpPermissionPromptAutoApproveContext { + approvals_reviewer: Some(ApprovalsReviewer::AutoReview), + tool_approval_mode: Some(AppToolApproval::Approve), + }, + )); + assert!(!mcp_permission_prompt_is_auto_approved( + AskForApproval::OnRequest, + &PermissionProfile::read_only(), + McpPermissionPromptAutoApproveContext { + approvals_reviewer: Some(ApprovalsReviewer::User), + tool_approval_mode: Some(AppToolApproval::Approve), + }, + )); + assert!(!mcp_permission_prompt_is_auto_approved( + AskForApproval::OnFailure, + &PermissionProfile::read_only(), + McpPermissionPromptAutoApproveContext { + approvals_reviewer: Some(ApprovalsReviewer::AutoReview), + tool_approval_mode: Some(AppToolApproval::Approve), + }, + )); + assert!(!mcp_permission_prompt_is_auto_approved( + AskForApproval::UnlessTrusted, + &PermissionProfile::read_only(), + McpPermissionPromptAutoApproveContext { + approvals_reviewer: Some(ApprovalsReviewer::AutoReview), + tool_approval_mode: Some(AppToolApproval::Approve), + }, )); } diff --git a/codex-rs/core/src/mcp_skill_dependencies.rs b/codex-rs/core/src/mcp_skill_dependencies.rs index a97424f13..882b09b9f 100644 --- a/codex-rs/core/src/mcp_skill_dependencies.rs +++ b/codex-rs/core/src/mcp_skill_dependencies.rs @@ -20,6 +20,7 @@ use crate::session::session::Session; use crate::session::turn_context::TurnContext; use crate::skills::model::SkillToolDependency; use codex_mcp::McpOAuthLoginSupport; +use codex_mcp::McpPermissionPromptAutoApproveContext; use codex_mcp::mcp_permission_prompt_is_auto_approved; use codex_mcp::oauth_login_support; use codex_mcp::resolve_oauth_scopes; @@ -222,6 +223,7 @@ async fn should_install_mcp_dependencies( if mcp_permission_prompt_is_auto_approved( turn_context.approval_policy.value(), &turn_context.permission_profile(), + McpPermissionPromptAutoApproveContext::default(), ) { return true; } diff --git a/codex-rs/core/src/mcp_tool_call.rs b/codex-rs/core/src/mcp_tool_call.rs index ed8451e35..85fc939ba 100644 --- a/codex-rs/core/src/mcp_tool_call.rs +++ b/codex-rs/core/src/mcp_tool_call.rs @@ -40,6 +40,7 @@ use codex_config::types::AppToolApproval; use codex_features::Feature; use codex_hooks::PermissionRequestDecision; use codex_mcp::CODEX_APPS_MCP_SERVER_NAME; +use codex_mcp::McpPermissionPromptAutoApproveContext; use codex_mcp::SandboxState; use codex_mcp::declared_openai_file_input_param_names; use codex_mcp::mcp_permission_prompt_is_auto_approved; @@ -956,6 +957,10 @@ async fn maybe_request_mcp_tool_approval( if mcp_permission_prompt_is_auto_approved( turn_context.approval_policy.value(), &turn_context.permission_profile(), + McpPermissionPromptAutoApproveContext { + approvals_reviewer: Some(turn_context.config.approvals_reviewer), + tool_approval_mode: Some(approval_mode), + }, ) { return None; } diff --git a/codex-rs/core/src/mcp_tool_call_tests.rs b/codex-rs/core/src/mcp_tool_call_tests.rs index 6a1412668..524138f01 100644 --- a/codex-rs/core/src/mcp_tool_call_tests.rs +++ b/codex-rs/core/src/mcp_tool_call_tests.rs @@ -2622,31 +2622,19 @@ async fn full_access_mode_skips_arc_monitor_for_all_approval_modes() { } #[tokio::test] -async fn approve_mode_routes_arc_ask_user_to_guardian_when_guardian_reviewer_is_enabled() { +async fn approve_mode_skips_arc_and_guardian_when_guardian_reviewer_is_enabled() { use wiremock::Mock; use wiremock::ResponseTemplate; use wiremock::matchers::method; use wiremock::matchers::path; let server = start_mock_server().await; - let guardian_request_log = mount_sse_once( - &server, - sse(vec![ - ev_response_created("resp-guardian"), - ev_assistant_message( - "msg-guardian", - &serde_json::json!({ - "risk_level": "low", - "user_authorization": "high", - "outcome": "allow", - "rationale": "The user already configured guardian to review escalated approvals for this session.", - }) - .to_string(), - ), - ev_completed("resp-guardian"), - ]), - ) - .await; + Mock::given(method("POST")) + .and(path("/v1/responses")) + .respond_with(ResponseTemplate::new(200)) + .expect(0) + .mount(&server) + .await; Mock::given(method("POST")) .and(path("/codex/safety/arc")) .respond_with(ResponseTemplate::new(200).set_body_json(serde_json::json!({ @@ -2660,7 +2648,7 @@ async fn approve_mode_routes_arc_ask_user_to_guardian_when_guardian_reviewer_is_ "why": "requires review", }], }))) - .expect(1) + .expect(0) .mount(&server) .await; @@ -2719,9 +2707,5 @@ async fn approve_mode_routes_arc_ask_user_to_guardian_when_guardian_reviewer_is_ ) .await; - assert_eq!(decision, Some(McpToolApprovalDecision::Accept)); - assert_eq!( - guardian_request_log.single_request().path(), - "/v1/responses" - ); + assert_eq!(decision, None); }