codeant-ai-for-open-source[bot] commented on code in PR #43374:
URL: https://github.com/apache/superset/pull/43374#discussion_r3827746679
##########
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:
**Suggestion:** This assertion was left at the end of the new test, but
`text` is local to the preceding `test_client_facing_text_is_sanitized` method
and is undefined here. The test therefore raises `NameError` after its intended
assertions pass; remove the misplaced assertion or move it back to the
sanitization test. [possible bug]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ MCP middleware regression test fails with NameError.
- ❌ CI cannot pass the affected unit-test module.
- ⚠️ The production middleware behavior remains unaffected by this assertion.
```
</details>
[](https://docs.codeant.ai/cli/resolve-pr-comments-skill)
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** tests/unit_tests/mcp_service/test_middleware.py
**Line:** 2180:2180
**Comment:**
*Possible Bug: This assertion was left at the end of the new test, but
`text` is local to the preceding `test_client_facing_text_is_sanitized` method
and is undefined here. The test therefore raises `NameError` after its intended
assertions pass; remove the misplaced assertion or move it back to the
sanitization test.
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%2F43374&comment_hash=dbbf0f5188ffecbfc0e9799bcf18a1bcf32fdf7e344a0459b9c3207a26aa83cf&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F43374&comment_hash=dbbf0f5188ffecbfc0e9799bcf18a1bcf32fdf7e344a0459b9c3207a26aa83cf&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]
