This is an automated email from the ASF dual-hosted git repository.
aminghadersohi pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/superset.git
The following commit(s) were added to refs/heads/master by this push:
new c0c688d3e4b fix(mcp): return a specific error per failure class on
tool calls (#44387)
c0c688d3e4b is described below
commit c0c688d3e4b2696bf481757f5364a5dcc4184fbe
Author: Amin Ghadersohi <[email protected]>
AuthorDate: Thu Sep 24 03:24:45 2026 +1000
fix(mcp): return a specific error per failure class on tool calls (#44387)
Co-authored-by: Claude Opus 5 <[email protected]>
---
docs/admin_docs/configuration/mcp-server.mdx | 16 +
superset/mcp_service/chart/compile.py | 29 +-
superset/mcp_service/constants.py | 28 +
superset/mcp_service/middleware.py | 287 ++++++++-
.../mcp_service/test_error_classification.py | 679 +++++++++++++++++++++
5 files changed, 995 insertions(+), 44 deletions(-)
diff --git a/docs/admin_docs/configuration/mcp-server.mdx
b/docs/admin_docs/configuration/mcp-server.mdx
index 086dce6b9a4..a8bb4d6a92c 100644
--- a/docs/admin_docs/configuration/mcp-server.mdx
+++ b/docs/admin_docs/configuration/mcp-server.mdx
@@ -1175,6 +1175,22 @@ answer. Dashboard access checks and row-level security
remain independent of thi
display-context projection. Integrations must ensure the lookup actually
happens
and the assistant uses the result; tool guidance alone cannot guarantee this.
+## Tool-call errors
+
+For exceptions raised by tools, the shared error handler distinguishes
permission
+denials, invalid arguments, datasource failures, invalid SQL, and internal
errors.
+Datasource responses include an enumerated reason rather than raw driver
output;
+internal errors return an error ID rather than implementation details. Tools
such
+as `get_chart_data` that return structured error envelopes instead of raising
do
+not pass through this handler.
+
+Missing tables, columns, and schemas log at `WARNING` without invoking
+`MCP_ERROR_HOOK`, even when the exception inherits status 500. Connection
failures
+with status 500 or higher remain `ERROR` and invoke the hook. An explicitly
+sub-500 exception remains a warning even if it carries a connection error
reason:
+the generic engine reason can also describe a caller's malformed column, and a
+connection access denial is still a permission failure.
+
## Result serialization
Data-bearing tool responses sanitize row values, column samples, and statistics
diff --git a/superset/mcp_service/chart/compile.py
b/superset/mcp_service/chart/compile.py
index 4befcb8f5be..b97ab8ec2ce 100644
--- a/superset/mcp_service/chart/compile.py
+++ b/superset/mcp_service/chart/compile.py
@@ -40,7 +40,6 @@ from typing import Any, Dict, List, Literal
from sqlalchemy.exc import SQLAlchemyError
from superset.commands.exceptions import CommandException
-from superset.errors import SupersetErrorType
from superset.mcp_service.chart.query_result import (
normalize_chart_query_result,
query_result_failure,
@@ -57,34 +56,10 @@ from superset.mcp_service.common.error_schemas import (
ColumnSuggestion,
DatasetContext,
)
+from superset.mcp_service.constants import CONNECTION_ERROR_TYPES
logger = logging.getLogger(__name__)
-# Error types from db_engine_spec.extract_errors() that indicate a database
-# connectivity or authentication issue rather than a query/config problem.
-#
-# GENERIC_DB_ENGINE_ERROR is included because many engines (BigQuery,
-# Snowflake, Athena, Databricks, Trino) lack specific CONNECTION_* regex
-# patterns in their engine specs — all their connection failures fall back
-# to this generic type. This is safe here because _compile_chart only runs
-# after Tier 1 schema validation has already verified columns, metrics, and
-# filters against the dataset. At that point the SQL is auto-generated by
-# Superset's query builder, so genuine SQL/config errors are very unlikely.
-_CONNECTION_ERROR_TYPES = {
- SupersetErrorType.CONNECTION_INVALID_USERNAME_ERROR,
- SupersetErrorType.CONNECTION_INVALID_PASSWORD_ERROR,
- SupersetErrorType.CONNECTION_INVALID_HOSTNAME_ERROR,
- SupersetErrorType.CONNECTION_PORT_CLOSED_ERROR,
- SupersetErrorType.CONNECTION_INVALID_PORT_ERROR,
- SupersetErrorType.CONNECTION_HOST_DOWN_ERROR,
- SupersetErrorType.CONNECTION_ACCESS_DENIED_ERROR,
- SupersetErrorType.CONNECTION_UNKNOWN_DATABASE_ERROR,
- SupersetErrorType.CONNECTION_DATABASE_PERMISSIONS_ERROR,
- SupersetErrorType.CONNECTION_MISSING_PARAMETERS_ERROR,
- SupersetErrorType.CONNECTION_DATABASE_TIMEOUT,
- SupersetErrorType.GENERIC_DB_ENGINE_ERROR,
-}
-
@dataclass
class CompileResult:
@@ -354,7 +329,7 @@ def _classify_as_database_error(exc: BaseException,
dataset_id: int) -> bool:
dataset = DatasetDAO.find_by_id(dataset_id)
if dataset and dataset.database and isinstance(exc, Exception):
errors = dataset.database.db_engine_spec.extract_errors(exc)
- return any(e.error_type in _CONNECTION_ERROR_TYPES for e in errors)
+ return any(e.error_type in CONNECTION_ERROR_TYPES for e in errors)
except Exception: # pylint: disable=broad-except
logger.debug(
"Failed to classify error via engine spec for dataset %s: %s",
diff --git a/superset/mcp_service/constants.py
b/superset/mcp_service/constants.py
index 4e6b8a9c240..0b435dba815 100644
--- a/superset/mcp_service/constants.py
+++ b/superset/mcp_service/constants.py
@@ -18,6 +18,8 @@
from typing import Literal
+from superset.errors import SupersetErrorType
+
# Supported model types for schema discovery and MCP tools
ModelType = Literal["chart", "dataset", "dashboard", "database", "report"]
@@ -30,3 +32,29 @@ DEFAULT_MAX_RESPONSE_BYTES = 50_000 # ~50KB preserves the
former 25K-token guar
DEFAULT_WARN_THRESHOLD_PCT = 80 # Log warnings above 80% of limit
# Phase 2 list-field truncation cap; matches MAX_PAGE_SIZE
DEFAULT_MAX_LIST_ITEMS = 100
+
+# Error types that mean the connection to the analytics database failed, as
+# opposed to the query being malformed. Shared by chart compilation (which
+# retries/reports connection trouble separately) and the MCP error handler
+# (which must not blame the caller for an unreachable datasource).
+#
+# GENERIC_DB_ENGINE_ERROR is included because many engines (BigQuery,
+# Snowflake, Athena, Databricks, Trino) lack specific CONNECTION_* regex
+# patterns in their engine specs — all their connection failures fall back
+# to this generic type.
+CONNECTION_ERROR_TYPES = frozenset(
+ {
+ SupersetErrorType.CONNECTION_ACCESS_DENIED_ERROR,
+ SupersetErrorType.CONNECTION_DATABASE_PERMISSIONS_ERROR,
+ SupersetErrorType.CONNECTION_DATABASE_TIMEOUT,
+ SupersetErrorType.CONNECTION_HOST_DOWN_ERROR,
+ SupersetErrorType.CONNECTION_INVALID_HOSTNAME_ERROR,
+ SupersetErrorType.CONNECTION_INVALID_PASSWORD_ERROR,
+ SupersetErrorType.CONNECTION_INVALID_PORT_ERROR,
+ SupersetErrorType.CONNECTION_INVALID_USERNAME_ERROR,
+ SupersetErrorType.CONNECTION_MISSING_PARAMETERS_ERROR,
+ SupersetErrorType.CONNECTION_PORT_CLOSED_ERROR,
+ SupersetErrorType.CONNECTION_UNKNOWN_DATABASE_ERROR,
+ SupersetErrorType.GENERIC_DB_ENGINE_ERROR,
+ }
+)
diff --git a/superset/mcp_service/middleware.py
b/superset/mcp_service/middleware.py
index 59893fd9fdf..35c4cce2535 100644
--- a/superset/mcp_service/middleware.py
+++ b/superset/mcp_service/middleware.py
@@ -38,7 +38,15 @@ from superset.commands.exceptions import (
ForbiddenError,
ObjectNotFoundError,
)
-from superset.exceptions import SupersetException, SupersetSecurityException
+from superset.errors import SupersetErrorType
+from superset.exceptions import (
+ ColumnNotFoundException,
+ DatabaseNotFound,
+ SupersetErrorsException,
+ SupersetException,
+ SupersetGenericDBErrorException,
+ SupersetSecurityException,
+)
from superset.extensions import event_logger, stats_logger_manager
from superset.mcp_service.auth import (
_get_app_context_manager,
@@ -49,6 +57,7 @@ from superset.mcp_service.auth import (
MCPPermissionDeniedError,
)
from superset.mcp_service.constants import (
+ CONNECTION_ERROR_TYPES,
DEFAULT_MAX_LIST_ITEMS,
DEFAULT_MAX_RESPONSE_BYTES,
DEFAULT_WARN_THRESHOLD_PCT,
@@ -209,6 +218,204 @@ def _invoke_error_hook(error: Exception, hook_context:
dict[str, Any]) -> None:
logger.warning("MCP_ERROR_HOOK raised an exception: %s", hook_error)
+# The prefix FastMCP puts on every ToolError it wraps a tool exception in.
+# Pinned by a guard test so an upstream change surfaces as a clear failure
+# rather than silently reinstating the undifferentiated-error bug.
+_FASTMCP_WRAPPED_ERROR_PREFIX = "Error calling tool "
+
+
+def _unwrap_tool_error(error: Exception) -> Exception:
+ """Return the exception a ``ToolError`` was raised from, if any.
+
+ A tool failure is wrapped twice on its way out: FastMCP wraps whatever the
+ tool body raised, and :class:`GlobalErrorHandlerMiddleware` re-raises its
+ classified message as a fresh ``ToolError`` chained off the same cause.
+ Either way the real failure is the ``__cause__``.
+
+ This is for *classification only* — log severity, metrics, and
+ error-tracker capture — where attributing a failure to the wrapper rather
+ than to ``OperationalError`` or ``MCPPermissionDeniedError`` loses the
+ distinction that matters. It does not decide client-facing text; use
+ :func:`_unwrap_fastmcp_wrapped_error` for that.
+ """
+ if isinstance(error, ToolError) and isinstance(error.__cause__, Exception):
+ return error.__cause__
+ return error
+
+
+def _unwrap_fastmcp_wrapped_error(error: Exception) -> Exception:
+ """Return the original exception behind *FastMCP's* ``ToolError`` wrapper.
+
+ FastMCP catches every non-``FastMCPError`` raised inside a tool body and
+ re-raises it as ``ToolError(f"Error calling tool {name!r}: {e}") from e``
+ *before* any middleware error hook runs (see ``FastMCP._call_tool``). By
+ the time :class:`GlobalErrorHandlerMiddleware` sees a tool failure, the
+ concrete type — ``MCPPermissionDeniedError``, ``SupersetException``,
+ ``OperationalError`` — is no longer the exception itself, only its
+ ``__cause__``. Classifying the wrapper instead of the cause collapses
+ every distinct failure into one undifferentiated message.
+
+ A ``ToolError`` raised deliberately by tool code is already formatted for
+ MCP and is returned as-is. Tool code may legitimately chain one off
+ another exception (``raise ToolError(...) from exc``), so ``__cause__``
+ alone does not identify FastMCP's wrapper — the message prefix is
+ required as well, otherwise a tool-authored message would be discarded
+ and replaced by cause-based handling.
+ """
+ if (
+ isinstance(error, ToolError)
+ and isinstance(error.__cause__, Exception)
+ and str(error).startswith(_FASTMCP_WRAPPED_ERROR_PREFIX)
+ ):
+ return error.__cause__
+ return error
+
+
+# Exception classes that mean "the query behind this tool failed", not "the
+# caller used the tool wrong". The tool name and arguments were valid; the
+# datasource, table, column, or connection it reads is broken or gone.
+#
+# Deliberately narrow: only classes that are datasource-scoped *by definition*
+# and that do not reliably carry a recognisable ``SupersetErrorType``. Broader
+# classes are matched by error type instead (see _DATASOURCE_ERROR_TYPES),
+# because the class alone does not establish that the datasource is at fault:
+#
+# - ``SQLAlchemyError`` also covers metadata-database IntegrityError /
+# ProgrammingError raised while a tool reads Superset's own metastore.
+# - ``SupersetTimeoutException`` is the generic timeout class; SigalrmTimeout
+# and TimerTimeout raise it with BACKEND_TIMEOUT_ERROR.
+# - ``QueryObjectValidationError`` is raised for missing/invalid query fields
+# and invalid result types, which are caller or configuration problems.
+#
+# Misclassifying any of those would tell the caller their arguments were valid
+# and blame a datasource that is in fact healthy.
+_DATASOURCE_ERROR_EXCEPTIONS = (
+ ColumnNotFoundException,
+ DatabaseNotFound,
+ SupersetGenericDBErrorException,
+)
+
+# ``SupersetError.error_type`` values in the DB-engine, viz, and SQL Lab
+# families. Superset raises a bare ``SupersetErrorException`` for many of
+# these, so the exception class alone is not enough to classify them. The
+# connection half comes from the shared CONNECTION_ERROR_TYPES set so this
+# cannot drift out of sync with chart compilation's view of the same thing.
+_DATASOURCE_ERROR_TYPES = (
+ frozenset(
+ {
+ SupersetErrorType.COLUMN_DOES_NOT_EXIST_ERROR,
+ SupersetErrorType.DATABASE_NOT_FOUND_ERROR,
+ SupersetErrorType.FAILED_FETCHING_DATASOURCE_INFO_ERROR,
+ SupersetErrorType.INVALID_SQL_ERROR,
+ SupersetErrorType.OBJECT_DOES_NOT_EXIST_ERROR,
+ SupersetErrorType.RESULTS_BACKEND_ERROR,
+ SupersetErrorType.SCHEMA_DOES_NOT_EXIST_ERROR,
+ SupersetErrorType.SQLLAB_TIMEOUT_ERROR,
+ SupersetErrorType.SYNTAX_ERROR,
+ SupersetErrorType.TABLE_DOES_NOT_EXIST_ERROR,
+ SupersetErrorType.TABLE_NOT_FOUND_ERROR,
+ SupersetErrorType.UNKNOWN_DATASOURCE_TYPE_ERROR,
+ SupersetErrorType.VIZ_GET_DF_ERROR,
+ }
+ )
+ | CONNECTION_ERROR_TYPES
+)
+
+# Surfaced when a datasource failure carries no recognised SupersetErrorType.
+# A fixed sentinel, never a Python class name: the reason is part of the
+# client-facing message and must stay a closed, non-sensitive vocabulary.
+_GENERIC_DATASOURCE_REASON = "DATASOURCE_QUERY_FAILED"
+
+# Reasons where the *query* is at fault, not the datasource. For a
+# SQL-authoring tool the query text is itself an argument, so telling the
+# caller "your arguments were valid" would steer an agent away from fixing
+# its own malformed SQL.
+_QUERY_SYNTAX_REASONS = frozenset(
+ {
+ SupersetErrorType.INVALID_SQL_ERROR.value,
+ SupersetErrorType.SYNTAX_ERROR.value,
+ }
+)
+
+# Reasons that mean the connection to the analytics database is unhealthy —
+# an operational problem worth paging on, unlike a missing table.
+_CONNECTION_REASONS = frozenset(t.value for t in CONNECTION_ERROR_TYPES)
+
+
+def _datasource_error_reason(error: Exception) -> str | None:
+ """Return the enumerated reason for a datasource failure, if any.
+
+ Only ``SupersetErrorType`` members are returned — they are a closed,
+ non-sensitive vocabulary. Raw driver output (which can carry SQL, table
+ contents, or connection strings) is never surfaced from here.
+ """
+ errors = getattr(error, "errors", None)
+ single_error = getattr(error, "error", None)
+ if isinstance(error, SupersetErrorsException) and errors:
+ # SupersetErrorsException carries a list of SupersetError. Scan all of
+ # them, not just the first: a datasource failure reported alongside
+ # other errors would otherwise lose its specific reason.
+ candidates = [getattr(err, "error_type", None) for err in errors]
+ elif single_error is not None:
+ # SupersetErrorException carries a single SupersetError.
+ candidates = [getattr(single_error, "error_type", None)]
+ else:
+ # Plain SupersetException exposes error_type directly.
+ candidates = [getattr(error, "error_type", None)]
+ for error_type in candidates:
+ if error_type in _DATASOURCE_ERROR_TYPES:
+ return str(getattr(error_type, "value", error_type))
+ return None
+
+
+def _is_datasource_error(error: Exception) -> bool:
+ """Classify a failure as coming from the datasource behind the tool.
+
+ ``SupersetSecurityException`` subclasses ``SupersetErrorException``, so
+ callers must check for permission failures *before* calling this.
+ """
+ return (
+ isinstance(error, _DATASOURCE_ERROR_EXCEPTIONS)
+ or _datasource_error_reason(error) is not None
+ )
+
+
+def _datasource_error_is_user_error(error: Exception) -> bool | None:
+ """Severity for a datasource failure; ``None`` if it is not one.
+
+ :func:`_is_user_error` keys on ``SupersetException.status``, but Superset
+ raises a *bare* ``SupersetErrorException`` for most datasource failures,
+ and that does not override ``SupersetException.status = 500``. A dropped
+ table therefore logs at ERROR with a traceback and fires
+ ``MCP_ERROR_HOOK``, paging on what is routine MCP traffic — an agent
+ pointing at a chart whose table was renamed.
+
+ Classify by what actually failed instead of by an inherited default: an
+ unreachable or misconfigured *connection* is an operational problem worth
+ paging on; a missing table, column, or schema is not.
+
+ This only ever *de-escalates*. A sub-500 status is a deliberate judgement
+ by the exception class that the caller is at fault, and is never
+ overridden — otherwise the connection half of the allow-list would page
+ on exactly the errors this function exists to stop paging on. The
+ catch-all ``GENERIC_DB_ENGINE_ERROR`` makes that concrete: engines
+ without specific ``CONNECTION_*`` regexes (BigQuery, Snowflake, Athena,
+ Databricks, Trino) report a malformed adhoc column through it, carried by
+ a status-400 ``SupersetGenericDBErrorException``. Genuine connection
+ failures arrive as a bare status-500 ``SupersetErrorException`` and still
+ page.
+
+ Returns ``None`` when no recognised reason is available, leaving the
+ existing status-based judgement in place.
+ """
+ reason = _datasource_error_reason(error)
+ if reason is None:
+ return None
+ if getattr(error, "status", 500) < 500:
+ return True
+ return reason not in _CONNECTION_REASONS
+
+
# Errors caused by the LLM/user — expected in normal MCP operation.
# Agents send bad params, try tools they lack access to, request nonexistent
# resources. These are 400-class errors and should be logged at WARNING.
@@ -669,15 +876,11 @@ class LoggingMiddleware(Middleware):
)
return result
except Exception as exc:
- # GlobalErrorHandlerMiddleware (inner) wraps tool exceptions in
- # ToolError with the original attached as __cause__; unwrap it so
- # error_type and the user/system classification reflect the real
- # failure rather than the ToolError wrapper.
- original = (
- exc.__cause__
- if isinstance(exc, ToolError) and exc.__cause__ is not None
- else exc
- )
+ # Tool exceptions arrive wrapped in ToolError with the original
+ # attached as __cause__; unwrap it so error_type and the
+ # user/system classification reflect the real failure rather than
+ # the ToolError wrapper.
+ original = _unwrap_tool_error(exc)
error_type = type(original).__name__
raised_is_user_error = _is_user_error(original)
success = False
@@ -998,12 +1201,24 @@ class GlobalErrorHandlerMiddleware(Middleware):
async def _handle_error( # noqa: C901
self,
- error: Exception,
+ wrapped_error: Exception,
context: MiddlewareContext,
tool_name: str,
duration_ms: int,
) -> None:
- """Handle different types of errors with appropriate responses"""
+ """Handle different types of errors with appropriate responses.
+
+ ``wrapped_error`` is what reached the middleware; ``error`` is the
+ real failure. FastMCP re-raises everything a tool body throws as
+ ``ToolError(...) from e`` before any middleware runs, so classifying
+ the exception as received would funnel an RBAC denial, a dead table,
+ and an internal bug into the same message. Every decision below —
+ log severity, error-tracker capture, and the client-facing text — is
+ therefore made on the unwrapped cause. See
+ :func:`_unwrap_fastmcp_wrapped_error`.
+ """
+ error = _unwrap_fastmcp_wrapped_error(wrapped_error)
+
# Extract user context for logging
user_id = None
try:
@@ -1015,6 +1230,12 @@ class GlobalErrorHandlerMiddleware(Middleware):
# 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
log_fn = logger.warning if is_user else logger.error
log_fn(
"MCP tool call failed: tool=%s, user_id=%s, "
@@ -1071,7 +1292,9 @@ class GlobalErrorHandlerMiddleware(Middleware):
# Handle specific error types with appropriate responses
if isinstance(error, ToolError):
- # Tool errors are already formatted for MCP
+ # A ToolError that survived _unwrap_fastmcp_wrapped_error was
+ # raised deliberately by tool code (it carries no cause, or no
+ # FastMCP wrapper prefix) and is already formatted for MCP.
raise error
elif isinstance(error, ValidationError):
# Pydantic validation errors
@@ -1100,12 +1323,19 @@ class GlobalErrorHandlerMiddleware(Middleware):
f"Service error in {tool_name}:
{_sanitize_error_for_logging(error)}"
) from error
elif isinstance(error, MCPPermissionDeniedError):
- # MCP RBAC permission denied — convert to structured ToolError.
- # Must come before the generic PermissionError branch because
- # MCPPermissionDeniedError inherits from PermissionError.
+ # MCP RBAC permission denied. Rendered from the exception's own
+ # structured fields ("Permission denied: <permission> on
+ # <resource>") rather than through _sanitize_error_for_logging,
+ # which flattens every PermissionError to "Access denied" and
+ # would throw away the two facts that make the denial
+ # actionable. Must come before the generic PermissionError
+ # branch because MCPPermissionDeniedError inherits from it.
raise ToolError(str(error)) from error
elif isinstance(error, PermissionError):
- # Permission/authorization errors
+ # Authorization failures that are not raised by the MCP RBAC
+ # decorator still get the "Permission denied" shape, so callers
+ # can tell a denial from a malformed call no matter which layer
+ # refused them.
raise ToolError(
f"Permission denied for {tool_name}: "
f"You don't have access to this resource."
@@ -1127,6 +1357,29 @@ class GlobalErrorHandlerMiddleware(Middleware):
f"Permission denied for {tool_name}: "
f"{_sanitize_error_for_logging(error)}"
) from error
+ elif _is_datasource_error(error):
+ # The query behind the tool failed. The caller must NOT be told to
+ # re-check the tool schema — that advice fits an argument error and
+ # sends them down the wrong path for a dropped table or a dead
+ # connection. Only the enumerated SupersetErrorType is echoed; raw
+ # driver output could carry SQL or connection details.
+ reason = _datasource_error_reason(error) or
_GENERIC_DATASOURCE_REASON
+ if reason in _QUERY_SYNTAX_REASONS:
+ # The datasource is fine; the query is malformed. For a
+ # SQL-authoring tool that query is the caller's own argument,
+ # so blaming the datasource would steer an agent away from
+ # the one thing it can actually fix.
+ raise ToolError(
+ f"Query error in {tool_name}: the datasource rejected the "
+ f"query as invalid ({reason}). Fix the query itself — the "
+ f"datasource is reachable."
+ ) from error
+ raise ToolError(
+ f"Datasource error in {tool_name}: the query against the "
+ f"underlying datasource failed ({reason}). The tool name and "
+ f"arguments were valid — the datasource, table, or column it "
+ f"reads may be missing, renamed, or unreachable."
+ ) from error
elif isinstance(error, SupersetException):
# Other Superset errors — .status determines severity (already
# classified by _is_user_error above for log level)
diff --git a/tests/unit_tests/mcp_service/test_error_classification.py
b/tests/unit_tests/mcp_service/test_error_classification.py
new file mode 100644
index 00000000000..bde63de30be
--- /dev/null
+++ b/tests/unit_tests/mcp_service/test_error_classification.py
@@ -0,0 +1,679 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+"""Per-failure-class error classification for MCP tool calls.
+
+Three unrelated failures used to reach MCP clients as one undifferentiated
+message, because FastMCP re-raises everything a tool body throws as
+``ToolError(f"Error calling tool {name!r}: {e}") from e`` *before* any
+middleware error hook runs. ``GlobalErrorHandlerMiddleware`` then matched its
+first branch, ``isinstance(error, ToolError)``, and re-raised — so its entire
+per-type branch chain was unreachable for tool failures, and the client got
+whatever ``str(exc)`` happened to say.
+
+The rest of the suite exercises that chain by injecting *raw* exceptions into
+``on_message``, which is why the gap went unnoticed: no test drove a tool
+exception through FastMCP's wrapping first. These tests do, end to end.
+"""
+
+from typing import Any
+from unittest.mock import patch
+
+import pytest
+from fastmcp import FastMCP
+from fastmcp.client import Client
+from fastmcp.exceptions import ToolError
+from sqlalchemy.exc import IntegrityError, OperationalError
+
+from superset.commands.exceptions import ForbiddenError
+from superset.errors import ErrorLevel, SupersetError, SupersetErrorType
+from superset.exceptions import (
+ DatabaseNotFound,
+ QueryObjectValidationError,
+ SupersetErrorException,
+ SupersetErrorsException,
+ SupersetException,
+ SupersetGenericDBErrorException,
+ SupersetSecurityException,
+ SupersetTimeoutException,
+)
+from superset.mcp_service.auth import MCPPermissionDeniedError
+from superset.mcp_service.constants import CONNECTION_ERROR_TYPES
+from superset.mcp_service.middleware import (
+ _datasource_error_is_user_error,
+ _datasource_error_reason,
+ _FASTMCP_WRAPPED_ERROR_PREFIX,
+ _GENERIC_DATASOURCE_REASON,
+ _is_datasource_error,
+ _is_user_error,
+ _unwrap_fastmcp_wrapped_error,
+ _unwrap_tool_error,
+ GlobalErrorHandlerMiddleware,
+ ToolResultCompatibilityMiddleware,
+)
+
+# A connection string and an internal path, planted in the exception messages
+# below to prove neither reaches the caller.
+LEAKED_DSN = "postgresql://admin:[email protected]/prod"
+LEAKED_PATH = "/srv/app/superset/mcp_service/chart/tool/get_chart_data.py"
+
+
+def _dead_table_error() -> SupersetErrorException:
+ """The upstream failure a chart over a dropped table produces."""
+ return SupersetErrorException(
+ SupersetError(
+ message=f'relation "orders" does not exist ({LEAKED_DSN})',
+ error_type=SupersetErrorType.TABLE_DOES_NOT_EXIST_ERROR,
+ level=ErrorLevel.ERROR,
+ )
+ )
+
+
+def connection_denied(unused: int = 1) -> str:
+ """A connection access denial remains a permission failure."""
+ raise SupersetSecurityException(
+ SupersetError(
+ message="denied",
+ error_type=SupersetErrorType.CONNECTION_ACCESS_DENIED_ERROR,
+ level=ErrorLevel.ERROR,
+ )
+ )
+
+
+def _build_server() -> FastMCP:
+ """A FastMCP server with one tool per failure class.
+
+ ``LoggingMiddleware`` is deliberately omitted: it needs a Flask app
+ context, and it does not participate in classifying the response.
+ """
+ mcp: FastMCP = FastMCP("error-classification")
+
+ @mcp.tool
+ def rbac_denied(unused: int = 1) -> str:
+ """RBAC denial raised by the @tool permission decorator."""
+ raise MCPPermissionDeniedError(
+ permission_name="can_get",
+ view_name="User",
+ tool_name="rbac_denied",
+ )
+
+ @mcp.tool
+ def forbidden(unused: int = 1) -> str:
+ """Authorization refused below the MCP layer."""
+ raise ForbiddenError()
+
+ @mcp.tool
+ def dead_table(unused: int = 1) -> str:
+ """Well-formed call whose datasource is gone."""
+ raise _dead_table_error()
+
+ @mcp.tool
+ def untyped_datasource(unused: int = 1) -> str:
+ """Datasource failure carrying no recognised SupersetErrorType."""
+ raise DatabaseNotFound("Database backing this chart is gone")
+
+ @mcp.tool
+ def unreachable_datasource(unused: int = 1) -> str:
+ """The analytics database cannot be reached."""
+ raise SupersetErrorException(
+ SupersetError(
+ message="could not connect to host",
+ error_type=SupersetErrorType.CONNECTION_HOST_DOWN_ERROR,
+ level=ErrorLevel.ERROR,
+ )
+ )
+
+ @mcp.tool
+ def malformed_column(unused: int = 1) -> str:
+ """Engine reports a bad adhoc column via the GENERIC_DB_ENGINE
catch-all."""
+ raise SupersetGenericDBErrorException("bad adhoc column expression")
+
+ mcp.tool(connection_denied)
+
+ @mcp.tool
+ def bad_sql(unused: int = 1) -> str:
+ """The caller's own SQL is malformed."""
+ raise SupersetErrorException(
+ SupersetError(
+ message="syntax error at or near SELEC",
+ error_type=SupersetErrorType.SYNTAX_ERROR,
+ level=ErrorLevel.ERROR,
+ )
+ )
+
+ @mcp.tool
+ def needs_id(id: int) -> str: # noqa: A002
+ """Schema mismatch: callers guess ``identifier`` for ``id``."""
+ return "ok"
+
+ @mcp.tool
+ def internal_bug(unused: int = 1) -> str:
+ """A genuine server-side defect."""
+ raise RuntimeError(f"unexpected failure in {LEAKED_PATH}")
+
+ mcp.add_middleware(
+ ToolResultCompatibilityMiddleware(structured_output_enabled=False)
+ )
+ mcp.add_middleware(GlobalErrorHandlerMiddleware())
+ return mcp
+
+
+async def _call(tool: str, arguments: dict[str, Any] | None = None) -> str:
+ """Return the client-facing text for a failing tool call."""
+ async with Client(_build_server()) as client:
+ result = await client.call_tool(tool, arguments or {},
raise_on_error=False)
+ assert result.is_error, f"{tool} was expected to fail"
+ return result.content[0].text
+
+
+class TestFastMCPWrappingPrecondition:
+ """Guard the assumption the classification fix is built on."""
+
+ @pytest.mark.asyncio
+ async def test_tool_exceptions_arrive_wrapped_in_tool_error(self) -> None:
+ """FastMCP wraps tool exceptions before middleware sees them.
+
+ If a FastMCP upgrade stops doing this, ``_unwrap_tool_error`` becomes
+ a no-op and the branch chain keeps working — but this test failing is
+ the signal that the comments explaining *why* it exists are stale.
+ """
+ seen: list[Exception] = []
+
+ mcp: FastMCP = FastMCP("precondition")
+
+ @mcp.tool
+ def boom(unused: int = 1) -> str:
+ raise PermissionError("denied")
+
+ class Capture(ToolResultCompatibilityMiddleware):
+ async def on_call_tool(self, context: Any, call_next: Any) -> Any:
+ try:
+ return await call_next(context)
+ except Exception as exc: # noqa: BLE001
+ seen.append(exc)
+ raise
+
+ mcp.add_middleware(Capture(structured_output_enabled=False))
+
+ async with Client(mcp) as client:
+ await client.call_tool("boom", {}, raise_on_error=False)
+
+ assert len(seen) == 1
+ assert isinstance(seen[0], ToolError)
+ assert isinstance(seen[0].__cause__, PermissionError)
+ # _unwrap_fastmcp_wrapped_error uses this prefix to identify the
wrapper
+ # apart from a ToolError tool code chained off another exception.
+ assert str(seen[0]).startswith(_FASTMCP_WRAPPED_ERROR_PREFIX)
+
+
+class TestUnwrapToolError:
+ """``_unwrap_tool_error`` classifies by the underlying failure.
+
+ Used for log severity, metrics, and error-tracker capture only — never to
+ pick client-facing text — so it unwraps any chained ToolError, including
+ GlobalErrorHandlerMiddleware's own re-raise, which LoggingMiddleware (the
+ outer middleware) is what actually sees.
+ """
+
+ def test_returns_cause_of_fastmcp_wrapped_error(self) -> None:
+ cause = MCPPermissionDeniedError(permission_name="can_read",
view_name="Chart")
+ wrapped = ToolError("Error calling tool 'list_charts': denied")
+ wrapped.__cause__ = cause
+
+ assert _unwrap_tool_error(wrapped) is cause
+
+ def test_returns_cause_of_our_own_handlers_reraise(self) -> None:
+ """GlobalErrorHandlerMiddleware re-raises a classified message chained
+ off the same cause, with no FastMCP prefix. LoggingMiddleware must
+ still attribute the failure to OperationalError, not to ToolError."""
+ cause = OperationalError("db error", {}, Exception())
+ reraised = ToolError("Database error in execute_sql")
+ reraised.__cause__ = cause
+
+ assert _unwrap_tool_error(reraised) is cause
+
+ def test_passes_through_unchained_tool_error(self) -> None:
+ deliberate = ToolError("'search_tools' cannot be called via the proxy")
+
+ assert _unwrap_tool_error(deliberate) is deliberate
+
+ def test_passes_through_unwrapped_exception(self) -> None:
+ error = ValueError("page must be positive")
+
+ assert _unwrap_tool_error(error) is error
+
+
+class TestUnwrapFastMCPWrappedError:
+ """``_unwrap_fastmcp_wrapped_error`` decides client-facing text, so it
+ must recover FastMCP's cause without eating a tool-authored message."""
+
+ def test_returns_cause_of_fastmcp_wrapped_error(self) -> None:
+ cause = MCPPermissionDeniedError(permission_name="can_read",
view_name="Chart")
+ wrapped = ToolError("Error calling tool 'list_charts': denied")
+ wrapped.__cause__ = cause
+
+ assert _unwrap_fastmcp_wrapped_error(wrapped) is cause
+
+ def test_passes_through_deliberate_tool_error(self) -> None:
+ """A ToolError raised by tool code is already formatted for MCP, so
+ it must survive untouched."""
+ deliberate = ToolError("'search_tools' cannot be called via the proxy")
+
+ assert _unwrap_fastmcp_wrapped_error(deliberate) is deliberate
+
+ def test_passes_through_deliberate_tool_error_chained_off_a_cause(
+ self,
+ ) -> None:
+ """``raise ToolError(...) from exc`` in tool code is legitimate. Its
+ author-written message must not be discarded in favour of cause-based
+ handling just because a cause is attached."""
+ deliberate = ToolError("dataset is missing a time column")
+ deliberate.__cause__ = ValueError("no granularity")
+
+ assert _unwrap_fastmcp_wrapped_error(deliberate) is deliberate
+
+ def test_passes_through_unwrapped_exception(self) -> None:
+ error = ValueError("page must be positive")
+
+ assert _unwrap_fastmcp_wrapped_error(error) is error
+
+
+class TestDatasourceErrorClassification:
+ """Only genuine query/datasource failures take the datasource branch."""
+
+ def test_classifies_missing_table(self) -> None:
+ assert _is_datasource_error(_dead_table_error()) is True
+
+ def test_reason_is_the_enumerated_error_type(self) -> None:
+ assert (
+ _datasource_error_reason(_dead_table_error())
+ == SupersetErrorType.TABLE_DOES_NOT_EXIST_ERROR.value
+ )
+
+ def test_security_exception_is_not_a_datasource_error(self) -> None:
+ """SupersetSecurityException subclasses SupersetErrorException, so the
+ permission branch must win — a denied table is not a broken table."""
+ error = SupersetSecurityException(
+ SupersetError(
+ message="denied",
+ error_type=SupersetErrorType.TABLE_SECURITY_ACCESS_ERROR,
+ level=ErrorLevel.ERROR,
+ )
+ )
+
+ assert _is_datasource_error(error) is False
+
+ def test_unrelated_superset_exception_is_not_a_datasource_error(self) ->
None:
+ assert _is_datasource_error(SupersetException("something else")) is
False
+
+ def test_connection_failures_are_datasource_errors(self) -> None:
+ """Every canonical connection error type classifies, so an
+ unreachable database is never reported as an internal error."""
+ for error_type in CONNECTION_ERROR_TYPES:
+ error = SupersetErrorException(
+ SupersetError(
+ message="cannot connect",
+ error_type=error_type,
+ level=ErrorLevel.ERROR,
+ )
+ )
+
+ assert _is_datasource_error(error) is True, error_type
+ assert _datasource_error_reason(error) == error_type.value
+
+ def test_metastore_failure_is_not_a_datasource_error(self) -> None:
+ """SQLAlchemyError also covers Superset's own metadata database. A
+ metastore failure must not be reported as a failed datasource query
+ with the caller told their arguments were valid."""
+ error = IntegrityError("INSERT INTO logs", {}, Exception("duplicate
key"))
+
+ assert _is_datasource_error(error) is False
+
+ def test_generic_backend_timeout_is_not_a_datasource_error(self) -> None:
+ """SigalrmTimeout/TimerTimeout raise SupersetTimeoutException with
+ BACKEND_TIMEOUT_ERROR; that is not a datasource outage."""
+ error = SupersetTimeoutException(
+ error_type=SupersetErrorType.BACKEND_TIMEOUT_ERROR,
+ message="Process timed out",
+ level=ErrorLevel.ERROR,
+ )
+
+ assert _is_datasource_error(error) is False
+
+ def test_query_object_validation_error_is_not_a_datasource_error(self) ->
None:
+ """Raised for missing/invalid query fields and invalid result types —
+ a caller or configuration problem, not a broken datasource."""
+ error = QueryObjectValidationError("Invalid result type: bogus")
+
+ assert _is_datasource_error(error) is False
+
+ def test_reason_scans_every_error_in_a_multi_error_exception(self) -> None:
+ """A datasource error reported alongside others keeps its specific
+ reason instead of falling back to the generic sentinel."""
+ error = SupersetErrorsException(
+ [
+ SupersetError(
+ message="first",
+ error_type=SupersetErrorType.GENERIC_COMMAND_ERROR,
+ level=ErrorLevel.ERROR,
+ ),
+ SupersetError(
+ message="second",
+ error_type=SupersetErrorType.TABLE_DOES_NOT_EXIST_ERROR,
+ level=ErrorLevel.ERROR,
+ ),
+ ]
+ )
+
+ assert (
+ _datasource_error_reason(error)
+ == SupersetErrorType.TABLE_DOES_NOT_EXIST_ERROR.value
+ )
+
+ def test_reason_is_none_for_unlisted_error_type(self) -> None:
+ error = SupersetErrorException(
+ SupersetError(
+ message="nope",
+ error_type=SupersetErrorType.GENERIC_COMMAND_ERROR,
+ level=ErrorLevel.ERROR,
+ )
+ )
+
+ assert _datasource_error_reason(error) is None
+
+
+class TestPerClassClientFacingErrors:
+ """Each failure class reaches the caller as its own honest message."""
+
+ @pytest.mark.asyncio
+ async def test_rbac_denial_uses_permission_denied_shape(self) -> None:
+ """An RBAC denial names the permission and the resource, and never
+ tells the caller to go re-read the tool schema."""
+ message = await _call("rbac_denied")
+
+ assert "Permission denied: can_get on User" in message
+ assert "Validation error" not in message
+ assert "inputSchema" not in message
+ assert "Internal error" not in message
+ # Anchor on the *fixed* shape. A bare `in` check also passes on the
+ # unfixed code, where FastMCP's wrapper message merely contains the
+ # denial text -- the assertion above would give false confidence.
+ assert _FASTMCP_WRAPPED_ERROR_PREFIX not in message
+
+ @pytest.mark.asyncio
+ async def test_non_rbac_authorization_failure_also_says_permission_denied(
+ self,
+ ) -> None:
+ """Denials raised below the MCP RBAC decorator get the same shape, so
+ a caller can recognise a denial regardless of which layer refused."""
+ message = await _call("forbidden")
+
+ assert "Permission denied" in message
+
+ @pytest.mark.asyncio
+ async def test_schema_mismatch_keeps_validation_guidance(self) -> None:
+ """Wrong argument names still get the advice that actually fits:
+ the offending fields, so the caller can match the inputSchema."""
+ message = await _call("needs_id", {"identifier": 5})
+
+ assert "Validation error in needs_id" in message
+ assert "id" in message
+ assert "identifier" in message
+ assert "Permission denied" not in message
+
+ @pytest.mark.asyncio
+ async def test_datasource_failure_blames_the_query_not_the_caller(self) ->
None:
+ """A dropped table says the datasource failed and states the call was
+ valid — the previous message implied a bad tool name or arguments."""
+ message = await _call("dead_table")
+
+ assert "Datasource error in dead_table" in message
+ assert SupersetErrorType.TABLE_DOES_NOT_EXIST_ERROR.value in message
+ assert "arguments were valid" in message
+ assert "Permission denied" not in message
+ assert "Validation error" not in message
+
+ @pytest.mark.asyncio
+ async def test_untyped_datasource_failure_uses_the_generic_sentinel(self)
-> None:
+ """The reason is part of the client-facing message, so it stays a
+ closed vocabulary — never the Python exception class name."""
+ message = await _call("untyped_datasource")
+
+ assert "Datasource error in untyped_datasource" in message
+ assert _GENERIC_DATASOURCE_REASON in message
+ assert "DatabaseNotFound" not in message
+ assert "Exception" not in message
+
+ @pytest.mark.asyncio
+ async def test_internal_error_is_opaque_and_carries_an_error_id(self) ->
None:
+ message = await _call("internal_bug")
+
+ assert "Internal error in internal_bug" in message
+ assert "Error ID:" in message
+
+ @pytest.mark.asyncio
+ async def test_the_four_classes_do_not_share_a_message(self) -> None:
+ """The regression in one assertion: conflating these is the bug."""
+ messages = [
+ await _call("rbac_denied"),
+ await _call("needs_id", {"identifier": 5}),
+ await _call("dead_table"),
+ await _call("internal_bug"),
+ ]
+
+ assert len(set(messages)) == len(messages)
+ # Distinctness alone is not the property under test: the unfixed code
+ # also produced four distinct strings, just four *unclassified* ones.
+ # None may still carry FastMCP's wrapper.
+ for message in messages:
+ assert _FASTMCP_WRAPPED_ERROR_PREFIX not in message
+
+
+class TestNoNewDisclosure:
+ """Classification must not widen what a failure discloses."""
+
+ @pytest.mark.asyncio
+ async def test_datasource_error_does_not_echo_raw_driver_output(self) ->
None:
+ """Only the enumerated SupersetErrorType is surfaced; the driver
+ message (which carried a DSN) is not."""
+ message = await _call("dead_table")
+
+ assert LEAKED_DSN not in message
+ assert "hunter2" not in message
+ assert "db.internal" not in message
+ assert "orders" not in message
+
+ @pytest.mark.asyncio
+ async def test_internal_error_does_not_echo_server_paths(self) -> None:
+ message = await _call("internal_bug")
+
+ assert LEAKED_PATH not in message
+ assert "mcp_service" not in message
+
+
+class TestDatasourceErrorSeverity:
+ """A datasource failure's severity must follow what actually broke.
+
+ ``_is_user_error`` keys on ``SupersetException.status``, and a *bare*
+ ``SupersetErrorException`` — the shape Superset raises for most datasource
+ failures — inherits ``status = 500``. Without an override, a dropped table
+ logs at ERROR with a traceback and pages through ``MCP_ERROR_HOOK``.
+ """
+
+ @pytest.mark.parametrize(
+ "error_type",
+ [
+ SupersetErrorType.TABLE_DOES_NOT_EXIST_ERROR,
+ SupersetErrorType.COLUMN_DOES_NOT_EXIST_ERROR,
+ SupersetErrorType.SCHEMA_DOES_NOT_EXIST_ERROR,
+ ],
+ )
+ def test_missing_object_is_a_user_error_despite_status_500(
+ self, error_type: SupersetErrorType
+ ) -> None:
+ error = SupersetErrorException(
+ SupersetError(message="gone", error_type=error_type,
level=ErrorLevel.ERROR)
+ )
+
+ # The status this would otherwise be judged by.
+ assert error.status == 500
+ assert _is_user_error(error) is False
+ # ...but a renamed table is routine MCP traffic, not a page.
+ assert _datasource_error_is_user_error(error) is True
+
+ def test_connection_failure_stays_a_system_error(self) -> None:
+ """An unreachable database is an operational problem worth paging on.
+
+ Genuine connection failures arrive as a *bare*
``SupersetErrorException``,
+ which inherits ``status = 500`` — the shape the escalation applies to.
+ """
+ for error_type in CONNECTION_ERROR_TYPES:
+ error = SupersetErrorException(
+ SupersetError(
+ message="unreachable",
+ error_type=error_type,
+ level=ErrorLevel.ERROR,
+ )
+ )
+
+ assert error.status == 500
+ assert _datasource_error_is_user_error(error) is False, error_type
+
+ def test_sub_500_exceptions_are_never_escalated(self) -> None:
+ """The override only de-escalates.
+
+ A sub-500 status is a deliberate judgement by the exception class that
+ the caller is at fault. Escalating it would re-create the very
+ false-paging this function exists to remove — just for a different
+ error type.
+ """
+ # GENERIC_DB_ENGINE_ERROR is in CONNECTION_ERROR_TYPES (the catch-all
+ # for engines without specific CONNECTION_* regexes), but engines also
+ # report a malformed adhoc column through it, carried by a status-400
+ # exception. Reachable via generate_chart -> _compile_chart.
+ malformed_column = SupersetGenericDBErrorException("bad adhoc column")
+
+ assert malformed_column.status == 400
+ assert _is_user_error(malformed_column) is True
+ assert _datasource_error_is_user_error(malformed_column) is True
+
+ def test_security_exception_with_connection_reason_is_not_escalated(
+ self,
+ ) -> None:
+ """A 403 denial must not page just because its reason happens to be a
+ connection error type; the client is correctly told access was
denied."""
+ denial = SupersetSecurityException(
+ SupersetError(
+ message="denied",
+ error_type=SupersetErrorType.CONNECTION_ACCESS_DENIED_ERROR,
+ level=ErrorLevel.ERROR,
+ )
+ )
+
+ assert denial.status == 403
+ assert _datasource_error_is_user_error(denial) is True
+
+ def test_non_datasource_error_keeps_status_based_judgement(self) -> None:
+ assert _datasource_error_is_user_error(ValueError("bad page")) is None
+ assert _datasource_error_is_user_error(SupersetException("other")) is
None
+
+ @pytest.mark.asyncio
+ async def test_missing_table_logs_warning_and_does_not_fire_error_hook(
+ self,
+ ) -> None:
+ """End to end: the dropped-table shape must not page on-call."""
+ hook_calls: list[Any] = []
+
+ with (
+ patch(
+ "superset.mcp_service.middleware._invoke_error_hook",
+ side_effect=lambda *a, **k: hook_calls.append(a),
+ ),
+ patch("superset.mcp_service.middleware.logger") as mock_logger,
+ ):
+ await _call("dead_table")
+
+ assert hook_calls == []
+ mock_logger.warning.assert_called()
+ mock_logger.error.assert_not_called()
+
+ @pytest.mark.asyncio
+ async def test_connection_failure_logs_error_and_fires_error_hook(self) ->
None:
+ """End to end: an unreachable datasource still reaches the tracker."""
+ hook_calls: list[Any] = []
+
+ with (
+ patch(
+ "superset.mcp_service.middleware._invoke_error_hook",
+ side_effect=lambda *a, **k: hook_calls.append(a),
+ ),
+ patch("superset.mcp_service.middleware.logger") as mock_logger,
+ ):
+ await _call("unreachable_datasource")
+
+ assert len(hook_calls) == 1
+ mock_logger.error.assert_called()
+
+ @pytest.mark.asyncio
+ @pytest.mark.parametrize(
+ ("tool_name", "message_prefix"),
+ [
+ ("malformed_column", "Datasource error in malformed_column:"),
+ ("connection_denied", "Permission denied"),
+ ],
+ )
+ async def test_sub_500_connection_reason_logs_warning_without_error_hook(
+ self, tool_name: str, message_prefix: str
+ ) -> None:
+ """Keep 400 engine errors and 403 denials non-paging through
FastMCP."""
+ hook_calls: list[Any] = []
+
+ with (
+ patch(
+ "superset.mcp_service.middleware._invoke_error_hook",
+ side_effect=lambda *a, **k: hook_calls.append(a),
+ ),
+ patch("superset.mcp_service.middleware.logger") as mock_logger,
+ ):
+ message = await _call(tool_name)
+
+ assert message.removeprefix("Error: ").startswith(message_prefix)
+ assert hook_calls == []
+ failure_logs = [
+ call
+ for call in mock_logger.warning.call_args_list
+ if call.args[0].startswith("MCP tool call failed:")
+ ]
+ assert len(failure_logs) == 1
+ assert failure_logs[0].kwargs["exc_info"] is False
+ mock_logger.error.assert_not_called()
+
+
+class TestQuerySyntaxErrors:
+ """A malformed query is the caller's problem, not the datasource's."""
+
+ @pytest.mark.asyncio
+ async def test_syntax_error_blames_the_query_not_the_datasource(self) ->
None:
+ """For a SQL-authoring tool the query text *is* an argument, so
+ "arguments were valid" would steer an agent away from the one thing
+ it can fix."""
+ message = await _call("bad_sql")
+
+ assert "Query error in bad_sql" in message
+ assert SupersetErrorType.SYNTAX_ERROR.value in message
+ assert "Fix the query itself" in message
+ assert "arguments were valid" not in message
