You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@superset.apache.org by el...@apache.org on 2026/10/08 15:15:26 UTC
(superset) 01/01: fix(mcp): don't page MCP_ERROR_HOOK for user-class errors in the last-resort catch (SC-125493)
This is an automated email from the ASF dual-hosted git repository.
eschutho pushed a commit to branch fix-mcp-error-hook-user-errors
in repository https://gitbox.apache.org/repos/asf/superset.git
commit 4dc14e55200889e066bb8883fc1270c3ea58c2b5
Author: Elizabeth Thompson <el...@preset.io>
AuthorDate: Thu Oct 8 15:15:08 2026 +0000
fix(mcp): don't page MCP_ERROR_HOOK for user-class errors in the last-resort catch (SC-125493)
ToolResultCompatibilityMiddleware.on_call_tool's last-resort catch invoked
MCP_ERROR_HOOK for every non-ToolError exception, regardless of whether it
was a user-class error. When the middleware is registered inside
GlobalErrorHandlerMiddleware (added after it), FastMCP argument validation
errors and other user-class failures reach this catch first, so the hook
fired for routine bad-argument calls that GlobalErrorHandlerMiddleware
deliberately keeps out of error trackers.
Gate the last-resort hook on the same classification
GlobalErrorHandlerMiddleware uses (_is_user_error refined by
_datasource_error_is_user_error), shared through a small helper. The
client-facing is_error ToolResult is unchanged, and system-class errors
are still reported.
Co-Authored-By: Claude Sonnet 5.5 <no...@anthropic.com>
---
superset/mcp_service/middleware.py | 36 ++++--
tests/unit_tests/mcp_service/test_middleware.py | 160 +++++++++++++++++++++++-
2 files changed, 181 insertions(+), 15 deletions(-)
diff --git a/superset/mcp_service/middleware.py b/superset/mcp_service/middleware.py
index 339d44484bf..13f01d5a2f5 100644
--- a/superset/mcp_service/middleware.py
+++ b/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,20 @@ class ToolResultCompatibilityMiddleware(Middleware):
except Exception: # noqa: BLE001
sanitized_message = type(e).__name__
error_text = f"Error: {sanitized_message}"
- if not isinstance(e, ToolError):
+ if not isinstance(e, ToolError) and not _is_user_error_for_reporting(e):
# 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 +1323,7 @@ class GlobalErrorHandlerMiddleware(Middleware):
# 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, "
diff --git a/tests/unit_tests/mcp_service/test_middleware.py b/tests/unit_tests/mcp_service/test_middleware.py
index d6decb56e53..6a69d288b21 100644
--- a/tests/unit_tests/mcp_service/test_middleware.py
+++ b/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 @@ from superset.commands.exceptions import (
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 @@ from superset.mcp_service.utils.response_size_utils import (
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 @@ class TestGlobalErrorHandlerErrorIdUsesCallId:
_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."""
@@ -2882,6 +2903,139 @@ class TestToolResultCompatibilityErrorHook:
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