AurimasNav commented on code in PR #43374:
URL: https://github.com/apache/superset/pull/43374#discussion_r3841097406
##########
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._
Addressed in `c1575b3` — this comment (and Bito's matching one) flagged the
first push, where an insertion split `test_client_facing_text_is_sanitized` and
orphaned its `[REDACTED]` assertion. The amended commit restores the assertion
to that test; the new test class ends at `assert result.is_error is True`.
Since the fix: pre-commit (ruff + mypy) passes, Bito's re-run reports zero
actionable suggestions, and all four new tests execute and pass in CI (verified
in the `junit-results-current` artifact).
--
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]
