fix(core): skip default approval for custom MCP tools without usable annotations

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<Session>,
     turn_context: &Arc<TurnContext>,
@@ -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,

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;