AurimasNav commented on code in PR #43374:
URL: https://github.com/apache/superset/pull/43374#discussion_r3860460422
##########
tests/unit_tests/mcp_service/test_middleware.py:
##########
@@ -2088,4 +2088,93 @@ async def test_client_facing_text_is_sanitized(self) ->
None:
assert text.startswith("Error:")
assert "s3cret" not in text
assert "db.internal" not in text
+
+
+class TestStructuredContentStripperIsErrorFlag:
+ """Failures caught by StructuredContentStripperMiddleware must still be
+ reported as errors on the wire — a client that only inspects isError
+ would otherwise read a denial or a crash as a successful call."""
+
+ @pytest.mark.asyncio
+ async def test_tool_error_is_flagged_as_error(self) -> None:
+ """A permission denial surfaces as ToolError; it must not come back
+ looking like a successful tool call."""
+ middleware = StructuredContentStripperMiddleware()
+ context = MagicMock()
+ context.message.name = "save_sql_query"
+ call_next = AsyncMock(
+ side_effect=ToolError("Permission denied: can_write on SavedQuery")
+ )
+ mock_flask_app = MagicMock()
+ mock_flask_app.config.get.return_value = None
+
+ 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:")
+
+ @pytest.mark.asyncio
+ async def test_unexpected_exception_is_flagged_as_error(self) -> None:
+ """The same holds for exceptions that bypass
+ GlobalErrorHandlerMiddleware and reach the last-resort catch."""
+ middleware = StructuredContentStripperMiddleware()
+ context = MagicMock()
+ context.message.name = "list_charts"
+ call_next = AsyncMock(side_effect=RuntimeError("boom"))
+ mock_flask_app = MagicMock()
+ mock_flask_app.config.get.return_value = None
+
+ 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
+
+ @pytest.mark.asyncio
+ async def test_successful_result_is_not_flagged(self) -> None:
+ """The success path must stay untouched."""
+ from fastmcp.tools.tool import ToolResult
+ from mcp.types import TextContent
+
+ middleware = StructuredContentStripperMiddleware()
+ context = MagicMock()
+ context.message.name = "list_charts"
+ call_next = AsyncMock(
+ return_value=ToolResult(content=[TextContent(type="text",
text="ok")])
+ )
+
+ result = await middleware.on_call_tool(context, call_next)
+
+ assert result.is_error is False
+ assert result.content[0].text == "ok"
+
+ @pytest.mark.asyncio
+ async def test_is_error_survives_structured_content_stripping(self) ->
None:
+ """Rebuilding the result to drop structured_content must not clear
+ the flag, or a failure reported alongside structured output would
+ read as a success."""
+ from fastmcp.tools.tool import ToolResult
+ from mcp.types import TextContent
+
+ middleware = StructuredContentStripperMiddleware()
+ context = MagicMock()
+ context.message.name = "list_charts"
+ call_next = AsyncMock(
+ return_value=ToolResult(
+ content=[TextContent(type="text", text="failed")],
+ structured_content={"detail": "nope"},
+ is_error=True,
+ )
+ )
+
+ result = await middleware.on_call_tool(context, call_next)
+
+ assert result.structured_content is None
+ assert result.is_error is True
assert "[REDACTED]" in text
Review Comment:
> _Drafted with AI assistance._
Follow-up for anyone reading this thread now: two details above are out of
date after later review changes. The
`test_is_error_survives_structured_content_stripping` test was removed in
dbb60b7 (it exercised an unreachable path — see the thread on `middleware.py`),
so the class now ends at `test_successful_result_is_not_flagged`. The original
fix to this file — restoring the `[REDACTED]` assertion to
`test_client_facing_text_is_sanitized` — is unchanged. Coverage is now three
isolated tests here plus one end-to-end test in `test_mcp_e2e_smoke.py`
(dcac9fe), all passing in CI.
##########
superset/mcp_service/middleware.py:
##########
@@ -815,12 +815,27 @@ async def on_call_tool(
"duration_ms": None,
},
)
+ # Flag the failure so clients can distinguish it from a
+ # successful call. Returning a ToolResult here (rather than
+ # letting the exception reach the SDK) is what avoids the
+ # bridge encoding failure described above; is_error rides
+ # along in the serialized result as a plain boolean, so the
+ # protocol stays conformant without reintroducing the
+ # unencodable error response.
return ToolResult(
content=[mt.TextContent(type="text", text=error_text)],
meta={"mcp_call_id": mcp_call_id} if mcp_call_id else None,
+ is_error=True,
)
if isinstance(result, ToolResult) and result.structured_content is not
None:
- result = ToolResult(content=result.content, meta=result.meta)
+ # Rebuilding to drop structured_content must preserve is_error,
+ # or a tool that reported failure alongside structured output
+ # would come back looking successful.
+ result = ToolResult(
+ content=result.content,
+ meta=result.meta,
+ is_error=result.is_error,
+ )
Review Comment:
> _Drafted with AI assistance._
One addition: the reachability argument above was by construction only.
dcac9fe adds `test_tools_call_failure_sets_is_error_over_real_asgi_transport`
in `test_mcp_e2e_smoke.py`, which drives a failing `tools/call` through
`build_middleware_list()` and the streamable-HTTP JSON-RPC wire and asserts
`is_error` where the client reads it — so the catch-all path is now proven end
to end rather than assumed. It passes in CI (JUnit artifact, 0.139s).
--
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]
