codeant-ai-for-open-source[bot] commented on code in PR #44387:
URL: https://github.com/apache/superset/pull/44387#discussion_r4036350280
##########
superset/mcp_service/middleware.py:
##########
@@ -165,6 +177,105 @@ def _invoke_error_hook(error: Exception, hook_context:
dict[str, Any]) -> None:
logger.warning("MCP_ERROR_HOOK raised an exception: %s", hook_error)
+def _unwrap_tool_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 re-raised by FastMCP
+ untouched and therefore carries no ``__cause__``; it is already formatted
+ for MCP and is returned as-is.
+ """
+ if isinstance(error, ToolError) and error.__cause__ is not None:
+ cause = error.__cause__
+ if isinstance(cause, Exception):
+ return cause
Review Comment:
**Suggestion:** A deliberately raised `ToolError` chained from another
exception is mistaken for FastMCP's wrapper, so its tool-authored client
message is replaced by cause-based handling.
**Assessment:** ๐ `Major` ยท ๐ `Occurrence: Rarely` ยท ๐ท๏ธ `Api mismatch`
[](https://docs.codeant.ai/cli/resolve-pr-comments-skill)
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=ee17a1fe0c7b4b3584d74529a2108c03&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=ee17a1fe0c7b4b3584d74529a2108c03&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
<details>
<summary><b>Prompt for AI Agent ๐ค </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/mcp_service/middleware.py
**Line:** 196:199
**Comment:**
*Api Mismatch: A deliberately raised `ToolError` chained from another
exception is mistaken for FastMCP's wrapper, so its tool-authored client
message is replaced by cause-based handling.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask
user if the user wants to fix the rest of the comments as well. if said yes,
then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F44387&comment_hash=c40ffa17d6160e3a199693d71fe0a7b16d0afcb99d872ec1f9807fc91b036a50&reaction=like'>๐</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F44387&comment_hash=c40ffa17d6160e3a199693d71fe0a7b16d0afcb99d872ec1f9807fc91b036a50&reaction=dislike'>๐</a>
##########
superset/mcp_service/middleware.py:
##########
@@ -165,6 +177,105 @@ def _invoke_error_hook(error: Exception, hook_context:
dict[str, Any]) -> None:
logger.warning("MCP_ERROR_HOOK raised an exception: %s", hook_error)
+def _unwrap_tool_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 re-raised by FastMCP
+ untouched and therefore carries no ``__cause__``; it is already formatted
+ for MCP and is returned as-is.
+ """
+ if isinstance(error, ToolError) and error.__cause__ is not None:
+ cause = error.__cause__
+ if isinstance(cause, Exception):
+ return 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.
+_DATASOURCE_ERROR_EXCEPTIONS = (
+ ColumnNotFoundException,
+ DatabaseNotFound,
+ QueryObjectValidationError,
+ SupersetGenericDBErrorException,
+ SupersetParseError,
+ SupersetTimeoutException,
+ SupersetVizException,
+ SQLAlchemyError,
Review Comment:
**Suggestion:** `SupersetTimeoutException` and
`SupersetGenericDBErrorException` have 4xx statuses, so `_is_user_error` logs
these datasource failures only as warnings and skips `MCP_ERROR_HOOK` capture.
**Assessment:** ๐ `Major` ยท ๐ `Occurrence: Sometimes` ยท ๐ท๏ธ `Incorrect
condition logic`
[](https://docs.codeant.ai/cli/resolve-pr-comments-skill)
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=a0221704089f49f7b5aa1bc7cdf2ff14&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=a0221704089f49f7b5aa1bc7cdf2ff14&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
<details>
<summary><b>Prompt for AI Agent ๐ค </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/mcp_service/middleware.py
**Line:** 210:214
**Comment:**
*Incorrect Condition Logic: `SupersetTimeoutException` and
`SupersetGenericDBErrorException` have 4xx statuses, so `_is_user_error` logs
these datasource failures only as warnings and skips `MCP_ERROR_HOOK` capture.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask
user if the user wants to fix the rest of the comments as well. if said yes,
then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F44387&comment_hash=01c6408ecb711f66b25f74d2fbb525363a8098647e6a77fcd1aaf0de57ea9e60&reaction=like'>๐</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F44387&comment_hash=01c6408ecb711f66b25f74d2fbb525363a8098647e6a77fcd1aaf0de57ea9e60&reaction=dislike'>๐</a>
##########
superset/mcp_service/middleware.py:
##########
@@ -165,6 +177,105 @@ def _invoke_error_hook(error: Exception, hook_context:
dict[str, Any]) -> None:
logger.warning("MCP_ERROR_HOOK raised an exception: %s", hook_error)
+def _unwrap_tool_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 re-raised by FastMCP
+ untouched and therefore carries no ``__cause__``; it is already formatted
+ for MCP and is returned as-is.
+ """
+ if isinstance(error, ToolError) and error.__cause__ is not None:
+ cause = error.__cause__
+ if isinstance(cause, Exception):
+ return 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.
+_DATASOURCE_ERROR_EXCEPTIONS = (
+ ColumnNotFoundException,
+ DatabaseNotFound,
+ QueryObjectValidationError,
+ SupersetGenericDBErrorException,
+ SupersetParseError,
+ SupersetTimeoutException,
+ SupersetVizException,
+ SQLAlchemyError,
+)
+
+# ``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.
+_DATASOURCE_ERROR_TYPES = frozenset(
+ {
+ SupersetErrorType.COLUMN_DOES_NOT_EXIST_ERROR,
+ SupersetErrorType.CONNECTION_DATABASE_TIMEOUT,
+ SupersetErrorType.CONNECTION_HOST_DOWN_ERROR,
+ SupersetErrorType.CONNECTION_PORT_CLOSED_ERROR,
+ SupersetErrorType.CONNECTION_UNKNOWN_DATABASE_ERROR,
+ SupersetErrorType.DATABASE_NOT_FOUND_ERROR,
+ SupersetErrorType.FAILED_FETCHING_DATASOURCE_INFO_ERROR,
+ SupersetErrorType.GENERIC_DB_ENGINE_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,
+ }
+)
+
+
+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.
+ error_type = getattr(errors[0], "error_type", None)
Review Comment:
**Suggestion:** Only `errors[0]` is examined, so a multi-error exception
with a later datasource error is not classified as a datasource failure and
loses its specific reason.
**Assessment:** ๐ `Major` ยท ๐ `Occurrence: Sometimes` ยท ๐ท๏ธ `Logic error`
[](https://docs.codeant.ai/cli/resolve-pr-comments-skill)
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=16f5c425d8ba434794fc7ca2f3b1056b&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=16f5c425d8ba434794fc7ca2f3b1056b&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
<details>
<summary><b>Prompt for AI Agent ๐ค </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/mcp_service/middleware.py
**Line:** 253:255
**Comment:**
*Logic Error: Only `errors[0]` is examined, so a multi-error exception
with a later datasource error is not classified as a datasource failure and
loses its specific reason.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask
user if the user wants to fix the rest of the comments as well. if said yes,
then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F44387&comment_hash=1a594d2b0b7dc1f3c3baa6dc803b475e73ed6a4ea221300c24d53db620dcf170&reaction=like'>๐</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F44387&comment_hash=1a594d2b0b7dc1f3c3baa6dc803b475e73ed6a4ea221300c24d53db620dcf170&reaction=dislike'>๐</a>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
