eudaimos opened a new issue, #42580:
URL: https://github.com/apache/superset/issues/42580

   ### Bug
   
   `LoggingMiddleware._is_error_response` in 
`superset/mcp_service/middleware.py` decides whether a tool call succeeded by 
substring-matching the serialized response:
   
   ```python
   def _is_error_response(self, result: ToolResult) -> bool:
       """Check if a tool result contains an error schema response.
   
       MCP tools return error schemas (ChartError, DashboardError, etc.)
       instead of raising exceptions. These serialize to JSON containing
       an "error_type" field.
       """
       try:
           return '"error_type"' in result.content[0].text
       except (AttributeError, IndexError):
           return False
   ```
   
   This is used to set the `success` field on the `mcp_tool_call` audit event 
and log line:
   
   ```python
   result = await call_next(context)
   success = not self._is_error_response(result)
   ```
   
   The check tests for the *presence of the key*, not for a non-null value. 
`ExecuteSqlResponse` includes `error` and `error_type` as nullable fields that 
are always serialized, so a fully successful `execute_sql` response contains 
`"error_type":null` — and is therefore logged as a failure.
   
   ### Reproduction
   
   Four tool calls in a single authenticated session against Superset 6.1.0, 
all of which returned correct results to the client:
   
   | Tool | Response contains `"error_type"` | Logged `success=` |
   |---|---|---|
   | `health_check` | no | `True` |
   | `get_instance_info` | no | `True` |
   | `list_databases` | no | `True` |
   | `execute_sql` | yes (`"error_type":null`) | **`False`** |
   
   The `execute_sql` call returned real data:
   
   ```json
   {"success":true,"rows":[{"database":"bronze_layer","tables":161}, ...],
    "row_count":12,"error":null,"error_type":null}
   ```
   
   while the server logged:
   
   ```
   INFO:superset.mcp_service.middleware:MCP tool call: tool=execute_sql, 
user_id=2, method=tools/call, duration_ms=195, success=False
   ```
   
   Note the tool's own payload says `"success":true` and `"error_type":null` in 
the same response the middleware classifies as an error.
   
   It also contradicts the tool's own progress logging in the same request:
   
   ```
   DEBUG Sending INFO to client: SQL execution completed successfully: 
rows_returned=1, execution_time=7.04
   INFO:superset.mcp_service.middleware:MCP tool call: tool=execute_sql, ..., 
duration_ms=7098, success=False
   ```
   
   ### Impact
   
   `success` is wrong for every tool whose response schema declares a nullable 
`error_type`, which includes `execute_sql` — likely the single most-used tool 
in the service. Every successful SQL execution is recorded as a failure in the 
`logs` table and the Action Log UI.
   
   Combined with #42579 (error events never written at all), MCP observability 
is inverted end to end: real failures are absent, and successes are recorded as 
failures. Any dashboard or alert built on `mcp_tool_call.success` reports a 
~100% failure rate for `execute_sql`.
   
   This actively misleads operators triaging a report. While diagnosing an 
unrelated user issue we used `success=` to distinguish working calls from 
broken ones, and it disagreed with both the tool's own response payload and its 
progress log.
   
   ### Suggested fix
   
   Inspect the parsed value rather than substring-matching the key:
   
   ```python
   def _is_error_response(self, result: ToolResult) -> bool:
       try:
           payload = json.loads(result.content[0].text)
       except (AttributeError, IndexError, ValueError):
           return False
       if not isinstance(payload, dict):
           return False
       return payload.get("error_type") is not None
   ```
   
   A more robust option is to have the tools signal errors structurally — e.g. 
check `isinstance` against the error schema types before serialization, or set 
`ToolResult.meta` — rather than inferring status from response text at all.
   
   ### Environment
   
   - Superset 6.1.0 (`apache/superset:6.1.0`)
   - fastmcp 3.4.3
   - Python 3.10.20, Linux
   - Transport: streamable-http
   


-- 
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]

Reply via email to