fix(mcp): don't page MCP_ERROR_HOOK for user-class errors in the last-resort catch (SC-125493)

superset/mcp_service/middleware.py

@@ -478,6 +478,19 @@ def _is_user_error(error: Exception) -> bool:
     return False
 
 
+def _is_user_error_for_reporting(error: Exception) -> bool:
+    """:func:`_is_user_error`, refined by :func:`_datasource_error_is_user_error`.
+
+    The single classification every ``MCP_ERROR_HOOK`` call site gates on, so
+    a failure is paged (or not) the same way whichever handler catches it.
+    """
+    # A datasource failure's severity follows what actually broke, not the
+    # 500 status a bare SupersetErrorException inherits.
+    if (datasource_is_user := _datasource_error_is_user_error(error)) is not None:
+        return datasource_is_user
+    return _is_user_error(error)
+
+
 _SENSITIVE_PARAM_KEYS = frozenset(
     {
         "password",
@@ -1116,15 +1129,29 @@ async def on_call_tool(
             except Exception:  # noqa: BLE001
                 sanitized_message = type(e).__name__
             error_text = f"Error: {sanitized_message}"
-            if not isinstance(e, ToolError):
+            # Classification inspects attributes of an arbitrary exception
+            # (e.g. an unhashable ``error_type`` or a non-int ``status``) and
+            # can itself raise. Treat a classification failure as
+            # system-class so the hook still fires and this catch never
+            # propagates.
+            try:
+                is_user_error = _is_user_error_for_reporting(e)
+            except Exception:  # noqa: BLE001
+                is_user_error = False
+            if not isinstance(e, ToolError) and not is_user_error:
                 # GlobalErrorHandlerMiddleware converts every exception it
                 # sees into ToolError (and already invokes MCP_ERROR_HOOK
                 # for system-class errors there). A non-ToolError reaching
                 # this final catch means it slipped past that handler
-                # entirely — invoke the hook here as the true last-resort
-                # capture point. All contract keys are populated so hooks
-                # can index them unconditionally; user_id and duration_ms
-                # are unknown at this layer and passed as None.
+                # entirely — e.g. when this middleware is registered inside
+                # it — so invoke the hook here as the true last-resort
+                # capture point. Only system-class errors are reported,
+                # matching GlobalErrorHandlerMiddleware: user errors (bad
+                # arguments, permission denials) are expected MCP traffic
+                # and would otherwise flood an error tracker. All contract
+                # keys are populated so hooks can index them
+                # unconditionally; user_id and duration_ms are unknown at
+                # this layer and passed as None.
                 await _invoke_error_hook_off_loop(
                     e,
                     {
@@ -1305,13 +1332,7 @@ async def _handle_error(  # noqa: C901
         # Log with appropriate level: user errors (expected) → WARNING,
         # system errors (unexpected) → ERROR
         sanitized_error = _sanitize_error_for_logging(error)
-        is_user = _is_user_error(error)
-        # A datasource failure's severity follows what actually broke, not the
-        # 500 status a bare SupersetErrorException inherits. See
-        # _datasource_error_is_user_error.
-        datasource_is_user = _datasource_error_is_user_error(error)
-        if datasource_is_user is not None:
-            is_user = datasource_is_user
+        is_user = _is_user_error_for_reporting(error)
         log_fn = logger.warning if is_user else logger.error
         log_fn(
             "MCP tool call failed: tool=%s, user_id=%s, "

tests/unit_tests/mcp_service/test_middleware.py

@@ -19,12 +19,14 @@
 Unit tests for MCP service middleware.
 """
 
-from typing import Any
+from typing import Any, Callable
 from unittest.mock import AsyncMock, MagicMock, patch
 
 import pytest
+from fastmcp import FastMCP
+from fastmcp.client import Client
 from fastmcp.exceptions import ToolError, ValidationError as FastMCPValidationError
-from pydantic import ValidationError
+from pydantic import BaseModel, ValidationError
 from sqlalchemy.exc import OperationalError
 
 from superset.commands.exceptions import (
@@ -33,12 +35,17 @@
     ObjectNotFoundError,
 )
 from superset.errors import ErrorLevel, SupersetError, SupersetErrorType
-from superset.exceptions import SupersetException, SupersetSecurityException
+from superset.exceptions import (
+    SupersetErrorException,
+    SupersetException,
+    SupersetSecurityException,
+)
 from superset.mcp_service.auth import MCPNoAuthSourceError, MCPPermissionDeniedError
 from superset.mcp_service.constants import DEFAULT_MAX_LIST_ITEMS
 from superset.mcp_service.mcp_config import MCP_RESPONSE_SIZE_CONFIG
 from superset.mcp_service.middleware import (
     _is_user_error,
+    _sanitize_error_for_logging,
     _sanitize_params,
     create_response_size_guard_middleware,
     GlobalErrorHandlerMiddleware,
@@ -51,6 +58,7 @@
     get_response_size_bytes,
     UNMEASURABLE_RESPONSE_BYTES,
 )
+from superset.mcp_service.utils.validation import validation_message
 from superset.utils import json as utils_json
 from superset.utils.log import DBEventLogger
 
@@ -2763,6 +2771,19 @@ async def test_falls_back_to_generated_id_when_no_call_id_set(self) -> None:
             _mcp_call_id_var.reset(token)
 
 
+def _pydantic_validation_error() -> ValidationError:
+    """A real pydantic ValidationError for a missing required field."""
+
+    class Request(BaseModel):
+        dataset_id: int
+
+    try:
+        Request.model_validate({})
+    except ValidationError as exc:
+        return exc
+    raise AssertionError("expected a ValidationError")
+
+
 class TestToolResultCompatibilityErrorHook:
     """Test the last-resort MCP_ERROR_HOOK capture point in
     ToolResultCompatibilityMiddleware.on_call_tool's except block."""
@@ -2853,6 +2874,40 @@ def __str__(self) -> str:
 
         assert result.content[0].text == "Error: HostileStrError"
 
+    @pytest.mark.asyncio
+    async def test_classification_failure_does_not_escape_last_resort_handler(
+        self,
+    ) -> None:
+        """Classifying the error for reporting inspects attributes of an
+        arbitrary exception and can itself raise (an unhashable
+        ``error_type`` makes the datasource membership check throw
+        TypeError). The last-resort handler must still return an is_error
+        result rather than propagate, and treat the error as system-class
+        so the hook fires."""
+
+        class UnhashableErrorTypeError(Exception):
+            error_type: dict[str, str] = {}
+
+        middleware = ToolResultCompatibilityMiddleware()
+        context = MagicMock()
+        context.message.name = "list_charts"
+        error = UnhashableErrorTypeError("boom")
+        call_next = AsyncMock(side_effect=error)
+        mock_hook = MagicMock()
+        mock_flask_app = MagicMock()
+        mock_flask_app.config.get.return_value = mock_hook
+
+        with patch(
+            "superset.mcp_service.flask_singleton.get_flask_app",
+            return_value=mock_flask_app,
+        ):
+            result = await middleware.on_call_tool(context, call_next)
+
+        assert result.is_error is True
+        assert result.content[0].text.startswith("Error:")
+        mock_hook.assert_called_once()
+        assert mock_hook.call_args[0][0] is error
+
     @pytest.mark.asyncio
     async def test_client_facing_text_is_sanitized(self) -> None:
         """An exception bypassing GlobalErrorHandlerMiddleware must not
@@ -2882,6 +2937,139 @@ async def test_client_facing_text_is_sanitized(self) -> None:
         assert "db.internal" not in text
         assert "[REDACTED]" in text
 
+    @pytest.mark.asyncio
+    @pytest.mark.parametrize(
+        "make_error",
+        [
+            pytest.param(
+                _pydantic_validation_error,
+                id="pydantic-ValidationError",
+            ),
+            pytest.param(
+                lambda: FastMCPValidationError("request: Field required"),
+                id="fastmcp-ValidationError",
+            ),
+            pytest.param(lambda: ValueError("page must be >= 1"), id="ValueError"),
+            pytest.param(lambda: PermissionError("denied"), id="PermissionError"),
+            pytest.param(
+                lambda: SupersetErrorException(
+                    SupersetError(
+                        message="gone",
+                        error_type=SupersetErrorType.TABLE_DOES_NOT_EXIST_ERROR,
+                        level=ErrorLevel.ERROR,
+                    )
+                ),
+                id="missing-table-datasource-error",
+            ),
+        ],
+    )
+    async def test_does_not_invoke_hook_for_user_error(
+        self, make_error: Callable[[], Exception]
+    ) -> None:
+        """User-class errors (bad arguments, denials, missing objects) are
+        expected MCP traffic. GlobalErrorHandlerMiddleware deliberately keeps
+        them out of MCP_ERROR_HOOK; the last-resort catch must apply the same
+        classification instead of paging on every one that reaches it — which
+        is every one when this middleware is registered inside that handler.
+        The client-facing response is unchanged."""
+        error = make_error()
+        middleware = ToolResultCompatibilityMiddleware()
+        context = MagicMock()
+        context.message.name = "get_dataset_info"
+        context.fastmcp_context = None
+        call_next = AsyncMock(side_effect=error)
+        mock_hook = MagicMock()
+        mock_flask_app = MagicMock()
+        mock_flask_app.config.get.return_value = mock_hook
+
+        with patch(
+            "superset.mcp_service.flask_singleton.get_flask_app",
+            return_value=mock_flask_app,
+        ):
+            result = await middleware.on_call_tool(context, call_next)
+
+        mock_hook.assert_not_called()
+        assert result.is_error is True
+        if isinstance(error, (ValidationError, FastMCPValidationError)):
+            expected = await validation_message(error, context)
+        else:
+            expected = _sanitize_error_for_logging(error)
+        assert result.content[0].text == f"Error: {expected}"
+
+    @pytest.mark.asyncio
+    async def test_invokes_hook_for_datasource_connection_failure(self) -> None:
+        """A datasource failure is classified as GlobalErrorHandlerMiddleware
+        classifies it: an unreachable database is still paged."""
+        error = SupersetErrorException(
+            SupersetError(
+                message="could not connect to host",
+                error_type=SupersetErrorType.CONNECTION_HOST_DOWN_ERROR,
+                level=ErrorLevel.ERROR,
+            )
+        )
+        middleware = ToolResultCompatibilityMiddleware()
+        context = MagicMock()
+        context.message.name = "get_chart_data"
+        call_next = AsyncMock(side_effect=error)
+        mock_hook = MagicMock()
+        mock_flask_app = MagicMock()
+        mock_flask_app.config.get.return_value = mock_hook
+
+        with patch(
+            "superset.mcp_service.flask_singleton.get_flask_app",
+            return_value=mock_flask_app,
+        ):
+            result = await middleware.on_call_tool(context, call_next)
+
+        assert result.is_error is True
+        mock_hook.assert_called_once()
+        assert mock_hook.call_args[0][0] is error
+
+    @pytest.mark.asyncio
+    async def test_invalid_arguments_do_not_page_when_registered_inside_handler(
+        self,
+    ) -> None:
+        """End to end, with the compatibility middleware registered *after*
+        (i.e. inside) GlobalErrorHandlerMiddleware: FastMCP's argument
+        validation error reaches the last-resort catch first. It must come
+        back as an is_error result without invoking MCP_ERROR_HOOK."""
+        mcp: FastMCP = FastMCP("compat-innermost")
+
+        class Request(BaseModel):
+            dataset_id: int
+
+        @mcp.tool
+        def get_dataset_info(request: Request) -> dict[str, int]:
+            return {"id": request.dataset_id}
+
+        mcp.add_middleware(GlobalErrorHandlerMiddleware())
+        mcp.add_middleware(ToolResultCompatibilityMiddleware())
+
+        mock_hook = MagicMock()
+        mock_flask_app = MagicMock()
+        mock_flask_app.config.get.side_effect = lambda key, default=None: (
+            mock_hook if key == "MCP_ERROR_HOOK" else default
+        )
+
+        with (
+            patch(
+                "superset.mcp_service.flask_singleton.get_flask_app",
+                return_value=mock_flask_app,
+            ),
+            patch("superset.mcp_service.middleware.get_user_id", return_value=None),
+            patch("superset.mcp_service.middleware.event_logger"),
+        ):
+            async with Client(mcp) as client:
+                result = await client.call_tool(
+                    "get_dataset_info", {}, raise_on_error=False
+                )
+
+        assert result.is_error is True
+        assert result.content[0].text == (
+            "Error: Validation error in get_dataset_info: request: Field required"
+        )
+        mock_hook.assert_not_called()
+
 
 class TestToolResultCompatibilityIsErrorFlag:
     """Failures caught by ToolResultCompatibilityMiddleware must still be