aminghadersohi opened a new pull request, #44387:
URL: https://github.com/apache/superset/pull/44387
### SUMMARY
MCP clients cannot act on a failed tool call today, because three unrelated
failures reach the caller as one undifferentiated message.
**Where it comes from.** 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
(`FastMCP._call_tool`). `GlobalErrorHandlerMiddleware._handle_error` then
matches its very first branch:
```python
if isinstance(error, ToolError):
# Tool errors are already formatted for MCP
raise error
```
…and re-raises. Its entire per-type branch chain below that line —
`MCPPermissionDeniedError`, `PermissionError`, `SupersetSecurityException`,
`OperationalError`, `SupersetException`, the internal-error fallback — is
therefore **unreachable for anything a tool raises**. The caller gets whatever
`str(exc)` happened to say, prefixed by FastMCP's generic wrapper.
Argument errors are the one class that still worked: FastMCP raises those
from its argument parser *outside* the tool body, so they were the only
failures reaching the branch chain.
This also went unnoticed in tests because every existing case injects a
**raw** exception into `on_message`, exercising the branch chain directly — no
test drove a tool exception through FastMCP's wrapping first.
Two consequences beyond the message text:
- `_is_user_error()` was evaluated on the `ToolError` wrapper, which is in
`_USER_ERROR_TYPES`. Every tool failure — including genuine 500-class defects —
was logged at `WARNING` without `exc_info`, and `MCP_ERROR_HOOK` was never
invoked for them, so operators lost system-error capture.
- The client-facing text never passed through `_sanitize_error_for_logging`,
so raw driver output and server paths reached the caller verbatim.
**The fix** is in the shared handler, not in individual tools: classify on
the unwrapped `__cause__`. `LoggingMiddleware` already did exactly this for its
own metrics, so this extracts that into `_unwrap_tool_error()` and uses it in
both places. A `ToolError` raised deliberately by tool code is re-raised by
FastMCP untouched and carries no `__cause__`, so it still passes through
unchanged.
One new branch is added for **datasource/query failures**, which previously
had nowhere sensible to land and fell through to "Internal error" — wording
that implies a service defect rather than a dropped table. Only the enumerated
`SupersetErrorType` is echoed; raw driver output is not.
### BEFORE/AFTER
Driving the real middleware stack with an in-process FastMCP client:
| Failure | Before | After |
| --- | --- | --- |
| RBAC denial | `Error calling tool 'list_users': Permission denied: can_get
on User…` (clean only because `str()` happened to be) | `Permission denied:
can_get on User for user <u> (tool: list_users)` |
| Non-RBAC authz denial | `Error calling tool 'x': nope` | `Permission
denied for x: You don't have access to this resource.` |
| Dropped table | `Error calling tool 'get_chart_data': relation "orders"
does not exist (postgresql://admin:[email protected]/prod)` | `Datasource
error in get_chart_data: the query against the underlying datasource failed
(TABLE_DOES_NOT_EXIST_ERROR). The tool name and arguments were valid — the
datasource, table, or column it reads may be missing, renamed, or unreachable.`
|
| Wrong argument name | `Validation error in needs_id: id: Missing required
argument; identifier: Unexpected keyword argument` | unchanged — this advice
fits this class |
| Internal defect | `Error calling tool 'x': unexpected failure in
/srv/app/superset/…` | `Internal error in x: An unexpected error occurred.
Error ID: <id>.` |
Note rows 3 and 5: a connection string and a server path previously reached
the caller verbatim. Both are now withheld — this narrows disclosure, it does
not widen it. Permission denials continue to name the permission and the
resource, exactly as before.
### TESTING INSTRUCTIONS
```bash
pytest tests/unit_tests/mcp_service/test_error_classification.py
pytest tests/unit_tests/mcp_service/
```
`tests/unit_tests/mcp_service/test_error_classification.py` drives each
failure class end to end through a real `Client(mcp)` and the production
middleware, so it fails if FastMCP's wrapping is reintroduced into the
classification path. It asserts that:
- the RBAC case matches the `Permission denied: <permission> on <resource>`
shape and carries no schema guidance;
- the argument-error case keeps its validation guidance naming the offending
fields;
- the datasource case blames the query and states the call was valid;
- the four classes do not share a message;
- neither the planted connection string nor the planted server path appears
in any response.
A guard test pins the FastMCP wrapping behaviour the fix is built on, so a
future upgrade that changes it surfaces as a clear failure rather than silent
drift.
Manually: call any tool as a user lacking its permission and confirm the
denial names the permission and resource; call a tool with a misspelled
argument and confirm the validation guidance is unchanged; call
`get_chart_data` on a chart whose table has been dropped and confirm it reports
a datasource failure.
### ADDITIONAL INFORMATION
- [ ] Has associated issue:
- [ ] Required feature flags:
- [ ] Changes UI
- [ ] Includes DB Migration (follow approval process in
[SIP-59](https://github.com/apache/superset/issues/13351))
- [ ] Migration is atomic, supports rollback & is backwards-compatible
- [ ] Confirm DB migration upgrade and downgrade tested
- [ ] Runtime estimates and downtime expectations provided
- [ ] Introduces new feature or API
- [ ] Removes existing feature or API
--
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]
