Clarify MCP pre-tool-use approval precedence

codex-rs/core/src/mcp_tool_call.rs

@@ -974,10 +974,9 @@ async fn maybe_request_mcp_tool_approval(
     );
 
     let annotations = metadata.and_then(|metadata| metadata.annotations.as_ref());
-    let approval_required = requires_mcp_tool_approval(annotations);
     let approval_required = !auto_approved_by_permissions
-        && (approval_required || approval_mode == AppToolApproval::Prompt);
-    if !pre_tool_use_allows && !approval_required && !pre_tool_use_asks_user {
+        && (requires_mcp_tool_approval(annotations) || approval_mode == AppToolApproval::Prompt);
+    if !approval_required && !pre_tool_use_asks_user {
         return None;
     }
 
@@ -1006,21 +1005,27 @@ async fn maybe_request_mcp_tool_approval(
             }
         }
     }
-    if pre_tool_use_allows {
+    let must_prompt_user = monitor_reason.is_some() || pre_tool_use_asks_user;
+
+    if auto_approved_by_policy && approval_required && !must_prompt_user {
+        return None;
+    }
+    if pre_tool_use_allows && !must_prompt_user {
         return 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);
-    if pre_tool_use_permission_decision.is_none()
+    if !must_prompt_user
+        && pre_tool_use_permission_decision.is_none()
         && let Some(key) = session_approval_key.as_ref()
         && mcp_tool_approval_is_remembered(sess, key).await
     {
         return Some(McpToolApprovalDecision::Accept);
     }
 
-    if pre_tool_use_permission_decision.is_none() {
+    if !must_prompt_user && pre_tool_use_permission_decision.is_none() {
         match run_permission_request_hooks(
             sess,
             turn_context,
@@ -1052,7 +1057,7 @@ async fn maybe_request_mcp_tool_approval(
         .features
         .enabled(Feature::ToolCallMcpElicitation);
 
-    if routes_approval_to_guardian(turn_context) && !pre_tool_use_asks_user {
+    if routes_approval_to_guardian(turn_context) && !must_prompt_user {
         let review_id = new_guardian_review_id();
         let decision = review_approval_request(
             sess,
@@ -1074,9 +1079,14 @@ async fn maybe_request_mcp_tool_approval(
         return Some(decision);
     }
 
+    let (prompt_session_approval_key, prompt_persistent_approval_key) = if must_prompt_user {
+        (None, None)
+    } else {
+        (session_approval_key, persistent_approval_key)
+    };
     let prompt_options = mcp_tool_approval_prompt_options(
-        session_approval_key.as_ref(),
-        persistent_approval_key.as_ref(),
+        prompt_session_approval_key.as_ref(),
+        prompt_persistent_approval_key.as_ref(),
         tool_call_mcp_elicitation_enabled,
     );
     let question_id = format!("{MCP_TOOL_APPROVAL_QUESTION_ID_PREFIX}_{call_id}");
@@ -1101,12 +1111,11 @@ async fn maybe_request_mcp_tool_approval(
             .as_ref()
             .map(|rendered_template| rendered_template.question.as_str()),
     );
-    let prompt_reason = match &pre_tool_use_permission_decision {
-        Some(PreToolUsePermissionDecision::Ask { reason }) => {
-            reason.as_deref().or(monitor_reason.as_deref())
-        }
-        Some(PreToolUsePermissionDecision::Allow { .. }) | None => monitor_reason.as_deref(),
+    let pre_tool_use_reason = match &pre_tool_use_permission_decision {
+        Some(PreToolUsePermissionDecision::Ask { reason }) => reason.as_deref(),
+        Some(PreToolUsePermissionDecision::Allow { .. }) | None => None,
     };
+    let prompt_reason = monitor_reason.as_deref().or(pre_tool_use_reason);
     question.question = mcp_tool_approval_question_text(question.question, prompt_reason);
     if tool_call_mcp_elicitation_enabled {
         let request_id = rmcp::model::RequestId::String(
@@ -1142,8 +1151,8 @@ async fn maybe_request_mcp_tool_approval(
             sess,
             turn_context,
             &decision,
-            session_approval_key,
-            persistent_approval_key,
+            prompt_session_approval_key,
+            prompt_persistent_approval_key,
         )
         .await;
         return Some(decision);
@@ -1163,8 +1172,8 @@ async fn maybe_request_mcp_tool_approval(
         sess,
         turn_context,
         &decision,
-        session_approval_key,
-        persistent_approval_key,
+        prompt_session_approval_key,
+        prompt_persistent_approval_key,
     )
     .await;
     Some(decision)

codex-rs/core/src/mcp_tool_call_tests.rs

@@ -2399,6 +2399,243 @@ async fn approve_mode_blocks_when_arc_returns_interrupt_for_model() {
     );
 }
 
+#[tokio::test]
+async fn pre_tool_use_allow_runs_mcp_after_arc_ok() {
+    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": "ok",
+            "short_reason": "",
+            "rationale": "",
+            "risk_score": 1,
+            "risk_level": "low",
+            "evidence": [],
+        })))
+        .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);
+    turn_context.record_pre_tool_use_permission_decision(
+        "call-pretooluse-allow-arc-ok".to_string(),
+        codex_hooks::PreToolUsePermissionDecision::Allow {
+            reason: Some("approved by hook".to_string()),
+        },
+    );
+
+    let session = Arc::new(session);
+    let turn_context = Arc::new(turn_context);
+    let invocation = McpInvocation {
+        server: CODEX_APPS_MCP_SERVER_NAME.to_string(),
+        tool: "dangerous_tool".to_string(),
+        arguments: Some(serde_json::json!({ "id": 1 })),
+    };
+    let metadata = McpToolApprovalMetadata {
+        annotations: Some(annotations(Some(false), Some(true), Some(true))),
+        connector_id: Some("calendar".to_string()),
+        connector_name: Some("Calendar".to_string()),
+        connector_description: Some("Manage events".to_string()),
+        tool_title: Some("Dangerous Tool".to_string()),
+        tool_description: Some("Performs a risky action.".to_string()),
+        mcp_app_resource_uri: None,
+        codex_apps_meta: None,
+        openai_file_input_params: None,
+    };
+
+    let decision = maybe_request_mcp_tool_approval(
+        &session,
+        &turn_context,
+        "call-pretooluse-allow-arc-ok",
+        &invocation,
+        "mcp__test__tool",
+        Some(&metadata),
+        AppToolApproval::Approve,
+    )
+    .await;
+
+    assert_eq!(decision, None);
+}
+
+#[tokio::test]
+async fn pre_tool_use_allow_still_prompts_when_arc_asks_user() {
+    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": "ask-user",
+            "short_reason": "needs confirmation",
+            "rationale": "ARC wants a second review",
+            "risk_score": 65,
+            "risk_level": "medium",
+            "evidence": [{
+                "message": "dangerous_tool",
+                "why": "requires review",
+            }],
+        })))
+        .expect(1)
+        .mount(&server)
+        .await;
+
+    let (session, mut 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 turn_context = Arc::get_mut(&mut turn_context).expect("single turn context ref");
+        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);
+        turn_context.record_pre_tool_use_permission_decision(
+            "call-pretooluse-allow-arc-ask".to_string(),
+            codex_hooks::PreToolUsePermissionDecision::Allow {
+                reason: Some("approved by hook".to_string()),
+            },
+        );
+    }
+    let invocation = McpInvocation {
+        server: CODEX_APPS_MCP_SERVER_NAME.to_string(),
+        tool: "dangerous_tool".to_string(),
+        arguments: Some(serde_json::json!({ "id": 1 })),
+    };
+    let metadata = McpToolApprovalMetadata {
+        annotations: Some(annotations(Some(false), Some(true), Some(true))),
+        connector_id: Some("calendar".to_string()),
+        connector_name: Some("Calendar".to_string()),
+        connector_description: Some("Manage events".to_string()),
+        tool_title: Some("Dangerous Tool".to_string()),
+        tool_description: Some("Performs a risky action.".to_string()),
+        mcp_app_resource_uri: None,
+        codex_apps_meta: None,
+        openai_file_input_params: None,
+    };
+
+    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-pretooluse-allow-arc-ask",
+                &invocation,
+                "mcp__test__tool",
+                Some(&metadata),
+                AppToolApproval::Approve,
+            )
+            .await
+        })
+    };
+
+    assert!(
+        tokio::time::timeout(std::time::Duration::from_millis(200), &mut approval_task)
+            .await
+            .is_err(),
+        "ARC ask-user should still wait for user approval after pre-tool-use allow"
+    );
+    approval_task.abort();
+}
+
+#[tokio::test]
+async fn pre_tool_use_allow_still_blocks_when_arc_steers_model() {
+    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": "dangerous_tool",
+                "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);
+    turn_context.record_pre_tool_use_permission_decision(
+        "call-pretooluse-allow-arc-steer".to_string(),
+        codex_hooks::PreToolUsePermissionDecision::Allow {
+            reason: Some("approved by hook".to_string()),
+        },
+    );
+
+    let session = Arc::new(session);
+    let turn_context = Arc::new(turn_context);
+    let invocation = McpInvocation {
+        server: CODEX_APPS_MCP_SERVER_NAME.to_string(),
+        tool: "dangerous_tool".to_string(),
+        arguments: Some(serde_json::json!({ "id": 1 })),
+    };
+    let metadata = McpToolApprovalMetadata {
+        annotations: Some(annotations(Some(false), Some(true), Some(true))),
+        connector_id: Some("calendar".to_string()),
+        connector_name: Some("Calendar".to_string()),
+        connector_description: Some("Manage events".to_string()),
+        tool_title: Some("Dangerous Tool".to_string()),
+        tool_description: Some("Performs a risky action.".to_string()),
+        mcp_app_resource_uri: None,
+        codex_apps_meta: None,
+        openai_file_input_params: None,
+    };
+
+    let decision = maybe_request_mcp_tool_approval(
+        &session,
+        &turn_context,
+        "call-pretooluse-allow-arc-steer",
+        &invocation,
+        "mcp__test__tool",
+        Some(&metadata),
+        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 custom_approve_mode_blocks_when_arc_returns_interrupt_for_model() {
     use wiremock::Mock;