diff --git a/codex-rs/core/src/mcp_tool_call.rs b/codex-rs/core/src/mcp_tool_call.rs index efa50b2b4e13..9c3a5d68b3a0 100644 --- a/codex-rs/core/src/mcp_tool_call.rs +++ b/codex-rs/core/src/mcp_tool_call.rs @@ -46,6 +46,7 @@ use codex_mcp::mcp_permission_prompt_is_auto_approved; use codex_otel::sanitize_metric_tag_value; use codex_protocol::mcp::CallToolResult; use codex_protocol::openai_models::InputModality; +use codex_protocol::protocol::AskForApproval; use codex_protocol::protocol::EventMsg; use codex_protocol::protocol::McpInvocation; use codex_protocol::protocol::McpToolCallBeginEvent; @@ -819,6 +820,24 @@ fn mcp_tool_approval_prompt_options( } } +/// Determines whether an MCP tool call requires user approval and, if so, +/// obtains a decision via the appropriate channel (interactive prompt, guardian, +/// or ARC safety monitor). +/// +/// Returns `None` when the call may proceed without any approval gate, or +/// `Some(decision)` when an approval path was triggered. The caller must +/// inspect the decision variant to decide whether to execute or skip the tool. +/// +/// The function's early-exit order matters: +/// 1. Full-access mode → skip everything. +/// 2. Custom MCP in `Auto` mode without annotations (or with empty annotations +/// where every hint is `None`) → skip (user opted in by configuring the +/// server; see `should_skip_default_custom_mcp_approval`). +/// 3. Annotation-based check → skip if annotations say the tool is safe and +/// the mode is not `Prompt`. +/// 4. ARC safety monitor (for `Approve` mode) → may block or escalate. +/// 5. Non-interactive guard → `Decline` rather than hang. +/// 6. Guardian / interactive prompt → obtain a user or guardian decision. async fn maybe_request_mcp_tool_approval( sess: &Arc, turn_context: &Arc, @@ -836,6 +855,9 @@ async fn maybe_request_mcp_tool_approval( } let annotations = metadata.and_then(|metadata| metadata.annotations.as_ref()); + if should_skip_default_custom_mcp_approval(invocation, approval_mode, annotations) { + return None; + } let approval_required = requires_mcp_tool_approval(annotations); if !approval_required && approval_mode != AppToolApproval::Prompt { return None; @@ -866,6 +888,14 @@ async fn maybe_request_mcp_tool_approval( } } + // In runs with `--ask-for-approval never` (for example `codex exec`), no + // user is available to answer the approval prompt. Decline rather than + // hanging forever. This check is intentionally placed *after* the ARC + // safety-monitor block so that `Approve`-mode tools are still + // safety-checked even when running non-interactively. + if matches!(turn_context.approval_policy.value(), AskForApproval::Never) { + return Some(McpToolApprovalDecision::Decline { message: None }); + } let session_approval_key = session_mcp_tool_approval_key(invocation, metadata, approval_mode); let persistent_approval_key = persistent_mcp_tool_approval_key(invocation, metadata, approval_mode); @@ -1018,6 +1048,42 @@ async fn maybe_request_mcp_tool_approval( Some(decision) } +/// Returns `true` when a custom (non-Codex-Apps) MCP tool call should bypass the +/// default annotation-driven approval path entirely. +/// +/// Codex Apps tools always carry `ToolAnnotations`, so the annotation-based logic in +/// `requires_mcp_tool_approval` is authoritative for them. Custom MCP servers, however, +/// may omit annotations entirely **or** supply a `ToolAnnotations` struct with every +/// hint field set to `None` (the "empty annotations" case — common for stdio-based +/// servers that echo the struct without populating it). Both cases signal the same +/// thing: the server has no opinion on the tool's risk profile. +/// +/// When a custom tool is in the default `Auto` approval mode and annotations are absent +/// or empty, the user has already opted in by configuring the server — prompting them +/// again would be a regression from the expected behavior (see #15824). +/// +/// This gate intentionally does **not** fire for `Prompt` or `Approve` modes: those +/// represent an explicit per-tool override that should always take effect regardless +/// of annotation presence. +/// +/// Callers that set any concrete hint value (e.g. `destructive_hint: Some(true)`) +/// will fall through to the normal `requires_mcp_tool_approval` path, ensuring +/// annotation-based approval is still enforced whenever the server actually provides +/// risk information. +fn should_skip_default_custom_mcp_approval( + invocation: &McpInvocation, + approval_mode: AppToolApproval, + annotations: Option<&ToolAnnotations>, +) -> bool { + invocation.server != CODEX_APPS_MCP_SERVER_NAME + && approval_mode == AppToolApproval::Auto + && annotations.is_none_or(|annotations| { + annotations.destructive_hint.is_none() + && annotations.read_only_hint.is_none() + && annotations.open_world_hint.is_none() + }) +} + async fn maybe_monitor_auto_approved_mcp_tool_call( sess: &Session, turn_context: &TurnContext, diff --git a/codex-rs/core/src/mcp_tool_call_tests.rs b/codex-rs/core/src/mcp_tool_call_tests.rs index 5ab2c6c4e89a..c505ff824472 100644 --- a/codex-rs/core/src/mcp_tool_call_tests.rs +++ b/codex-rs/core/src/mcp_tool_call_tests.rs @@ -200,6 +200,34 @@ fn openai_file_params_are_only_honored_for_codex_apps() { ); } +/// Builds an `McpInvocation` for a non-Codex-Apps server with no annotations, +/// representing the scenario that triggered the regression in #15824. +fn custom_mcp_invocation_without_annotations() -> McpInvocation { + McpInvocation { + server: "docs".to_string(), + tool: "search".to_string(), + arguments: Some(serde_json::json!({ "query": "approval regression" })), + } +} + +/// Builds metadata whose `annotations` field is `Some(...)` but every hint inside +/// is `None` — the "empty annotations" variant that stdio MCP servers commonly emit. +fn custom_mcp_metadata_with_empty_annotations() -> McpToolApprovalMetadata { + McpToolApprovalMetadata { + annotations: Some(annotations( + /*read_only*/ None, /*destructive*/ None, /*open_world*/ None, + )), + connector_id: None, + connector_name: None, + connector_description: None, + tool_title: Some("Search".to_string()), + tool_description: Some("Search docs".to_string()), + mcp_app_resource_uri: None, + codex_apps_meta: None, + openai_file_input_params: None, + } +} + #[test] fn approval_required_when_read_only_false_and_destructive() { let annotations = annotations(Some(false), Some(true), /*open_world*/ None); @@ -2073,6 +2101,167 @@ async fn custom_approve_mode_blocks_when_arc_returns_interrupt_for_model() { ); } +#[tokio::test] +async fn custom_auto_mode_skips_approval_when_annotations_are_missing_in_never_mode() { + let (session, mut turn_context) = make_session_and_context().await; + turn_context + .approval_policy + .set(AskForApproval::Never) + .expect("test setup should allow updating approval policy"); + let session = Arc::new(session); + let turn_context = Arc::new(turn_context); + let invocation = custom_mcp_invocation_without_annotations(); + + let decision = maybe_request_mcp_tool_approval( + &session, + &turn_context, + "call-custom-auto", + &invocation, + "mcp__docs__search", + /*metadata*/ None, + AppToolApproval::Auto, + ) + .await; + + assert_eq!(decision, None); +} + +#[tokio::test] +async fn custom_auto_mode_skips_approval_when_annotations_are_missing_in_on_request_mode() { + let (session, turn_context, _rx_event) = make_session_and_context_with_rx().await; + { + let mut active_turn = session.active_turn.lock().await; + *active_turn = Some(ActiveTurn::default()); + } + + let session = Arc::new(session); + let turn_context = Arc::new(turn_context); + let invocation = custom_mcp_invocation_without_annotations(); + + let mut approval_task = { + let session = Arc::clone(&session); + let turn_context = Arc::clone(&turn_context); + tokio::spawn(async move { + maybe_request_mcp_tool_approval( + &session, + &turn_context, + "call-custom-auto-on-request", + &invocation, + "mcp__docs__search", + /*metadata*/ None, + AppToolApproval::Auto, + ) + .await + }) + }; + + let decision = tokio::time::timeout(std::time::Duration::from_millis(200), &mut approval_task) + .await + .expect("custom MCP tools should not wait for approval when annotations are missing") + .expect("approval task should complete successfully"); + + assert_eq!(decision, None); +} + +#[tokio::test] +async fn custom_auto_mode_skips_approval_when_annotations_have_no_hints_in_on_request_mode() { + let (session, turn_context, _rx_event) = make_session_and_context_with_rx().await; + { + let mut active_turn = session.active_turn.lock().await; + *active_turn = Some(ActiveTurn::default()); + } + + let session = Arc::new(session); + let turn_context = Arc::new(turn_context); + let invocation = custom_mcp_invocation_without_annotations(); + let metadata = custom_mcp_metadata_with_empty_annotations(); + + let mut approval_task = { + let session = Arc::clone(&session); + let turn_context = Arc::clone(&turn_context); + tokio::spawn(async move { + maybe_request_mcp_tool_approval( + &session, + &turn_context, + "call-custom-auto-empty-annotations-on-request", + &invocation, + "mcp__docs__search", + Some(&metadata), + AppToolApproval::Auto, + ) + .await + }) + }; + + let decision = tokio::time::timeout(std::time::Duration::from_millis(200), &mut approval_task) + .await + .expect("custom MCP tools with empty annotations should not wait for approval") + .expect("approval task should complete successfully"); + + assert_eq!(decision, None); +} + +#[tokio::test] +async fn custom_approve_mode_without_metadata_still_uses_arc_monitor() { + use wiremock::Mock; + use wiremock::MockServer; + use wiremock::ResponseTemplate; + use wiremock::matchers::method; + use wiremock::matchers::path; + + let server = MockServer::start().await; + Mock::given(method("POST")) + .and(path("/codex/safety/arc")) + .respond_with(ResponseTemplate::new(200).set_body_json(serde_json::json!({ + "outcome": "steer-model", + "short_reason": "needs approval", + "rationale": "high-risk action", + "risk_score": 96, + "risk_level": "critical", + "evidence": [{ + "message": "search", + "why": "high-risk action", + }], + }))) + .expect(1) + .mount(&server) + .await; + + let (session, mut turn_context) = make_session_and_context().await; + turn_context.auth_manager = Some(crate::test_support::auth_manager_from_auth( + codex_login::CodexAuth::create_dummy_chatgpt_auth_for_testing(), + )); + let mut config = (*turn_context.config).clone(); + config.chatgpt_base_url = server.uri(); + turn_context.config = Arc::new(config); + + let session = Arc::new(session); + let turn_context = Arc::new(turn_context); + let invocation = McpInvocation { + server: "docs".to_string(), + tool: "search".to_string(), + arguments: Some(serde_json::json!({ "query": "approval regression" })), + }; + + let decision = maybe_request_mcp_tool_approval( + &session, + &turn_context, + "call-custom-approve-no-metadata", + &invocation, + "mcp__docs__search", + /*metadata*/ None, + AppToolApproval::Approve, + ) + .await; + + assert_eq!( + decision, + Some(McpToolApprovalDecision::BlockedBySafetyMonitor( + "Tool call was cancelled because of safety risks: high-risk action".to_string(), + )) + ); +} + #[tokio::test] async fn approve_mode_blocks_when_arc_returns_interrupt_without_annotations() { use wiremock::Mock;