From 4dc14e55200889e066bb8883fc1270c3ea58c2b5 Mon Sep 17 00:00:00 2001 From: Elizabeth Thompson Date: Thu, 8 Oct 2026 15:15:08 +0000 Subject: [PATCH 1/2] 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 --- superset/mcp_service/middleware.py | 36 ++-- .../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 339d44484bf8..13f01d5a2f55 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 @@ 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): + 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 @@ 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, " diff --git a/tests/unit_tests/mcp_service/test_middleware.py b/tests/unit_tests/mcp_service/test_middleware.py index d6decb56e53b..6a69d288b214 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 @@ 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.""" @@ -2882,6 +2903,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 From 14422343952ccf3596c051fa10530b472e8808d5 Mon Sep 17 00:00:00 2001 From: Elizabeth Thompson Date: Thu, 8 Oct 2026 15:33:02 +0000 Subject: [PATCH 2/2] fix(mcp): never let error classification raise from the last-resort catch (SC-125493) ToolResultCompatibilityMiddleware.on_call_tool's last-resort except is documented as "must never propagate", but the classification added to gate MCP_ERROR_HOOK inspects attributes of an arbitrary exception and can itself raise (e.g. an unhashable error_type makes the datasource membership check throw TypeError). Guard it and treat a classification failure as system-class, so the hook still fires and the client still gets an is_error ToolResult. Co-Authored-By: Claude Sonnet 5.5 --- superset/mcp_service/middleware.py | 11 +++++- .../unit_tests/mcp_service/test_middleware.py | 34 +++++++++++++++++++ 2 files changed, 44 insertions(+), 1 deletion(-) diff --git a/superset/mcp_service/middleware.py b/superset/mcp_service/middleware.py index 13f01d5a2f55..26c688b48c66 100644 --- a/superset/mcp_service/middleware.py +++ b/superset/mcp_service/middleware.py @@ -1129,7 +1129,16 @@ 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) and not _is_user_error_for_reporting(e): + # 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 diff --git a/tests/unit_tests/mcp_service/test_middleware.py b/tests/unit_tests/mcp_service/test_middleware.py index 6a69d288b214..c9bd10e2bf3a 100644 --- a/tests/unit_tests/mcp_service/test_middleware.py +++ b/tests/unit_tests/mcp_service/test_middleware.py @@ -2874,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