mirror of
https://github.com/pchuan98/codex.git
synced 2026-07-01 00:31:56 +08:00
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`
This commit is contained in:
@@ -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,
|
||||
|
||||
@@ -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;
|
||||
|
||||
@@ -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<ApprovalsReviewer>,
|
||||
pub tool_approval_mode: Option<AppToolApproval>,
|
||||
}
|
||||
|
||||
/// MCP runtime settings derived from `codex_core::config::Config`.
|
||||
///
|
||||
/// This struct should contain only long-lived configuration values that the
|
||||
|
||||
@@ -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),
|
||||
},
|
||||
));
|
||||
}
|
||||
|
||||
|
||||
Reference in New Issue
Block a user