codeant-ai-for-open-source[bot] commented on code in PR #41924:
URL: https://github.com/apache/superset/pull/41924#discussion_r3561967831
##########
tests/unit_tests/mcp_service/system/tool/test_health_check.py:
##########
@@ -53,3 +84,94 @@ def test_health_check_response_with_uptime():
)
assert response.uptime_seconds == 123.45
+
+
+# ---------------------------------------------------------------------------
+# Tool-level tests: health_check via MCP Client
+# ---------------------------------------------------------------------------
+
+
[email protected]
+async def test_health_check_success_via_client(mcp_server):
+ """Happy path: tool returns a healthy status with real system info."""
+ with patch.object(
+ health_check_module,
+ "get_version_metadata",
+ return_value={"version_string": "4.1.0"},
+ ):
+ async with Client(mcp_server) as client:
+ result = await client.call_tool("health_check", {})
+
+ data = json.loads(result.content[0].text)
+ assert data["status"] == "healthy"
+ assert data["service"] == "Superset MCP Service"
+ assert data["version"] == "4.1.0"
+ assert data["timestamp"] is not None
+ assert data["uptime_seconds"] is not None
+ assert data["uptime_seconds"] >= 0
+
+
[email protected]
+async def test_health_check_uses_configured_app_name(mcp_server, app):
+ """service name is derived from the APP_NAME config, not hardcoded."""
+ app.config["APP_NAME"] = "Acme Analytics"
+ try:
+ with patch.object(
+ health_check_module,
+ "get_version_metadata",
+ return_value={"version_string": "4.1.0"},
+ ):
+ async with Client(mcp_server) as client:
+ result = await client.call_tool("health_check", {})
+ finally:
+ app.config.pop("APP_NAME", None)
Review Comment:
**Suggestion:** This test mutates global app config but does not restore the
previous `APP_NAME` value; it unconditionally removes the key. If `APP_NAME`
was set before the test, subsequent tests in the module will run with altered
configuration, creating order-dependent behavior. Save the original value and
restore it exactly in `finally`. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Global Flask APP_NAME config key removed after test.
- ⚠️ Subsequent tests can observe mutated APP_NAME configuration.
- ⚠️ Order-dependent behavior risk in MCP-related test suite.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. The global Superset Flask app used in tests is created in
`tests/integration_tests/test_app.py:28-32` via `create_app(...)`, which
loads
configuration from `superset.config.APP_NAME = "Superset"` (see
`superset/config.py:432`),
so `app.config["APP_NAME"]` is initially set.
2. Pytest provides the `app` fixture from `tests/conftest.py` to MCP tests;
`test_health_check_uses_configured_app_name` in
`tests/unit_tests/mcp_service/system/tool/test_health_check.py:115-131` uses
this shared
`app` instance.
3. Inside that test, line 117 sets `app.config["APP_NAME"] = "Acme
Analytics"`, and the
`finally` block at lines 126-127 calls `app.config.pop("APP_NAME", None)`,
removing the
key entirely instead of restoring the previous value `"Superset"` from the
global
configuration.
4. After the test completes, inspecting `app.config` shows that `APP_NAME`
is no longer
present even though it was set before the test, meaning this test leaves the
global Flask
config mutated; any code or later tests that rely on
`current_app.config["APP_NAME"]`
rather than `.get(..., "Superset")` would now see a missing key or fallback,
creating
order-dependent behavior.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=4ec330ae30264164a9a34ad83de9d9c1&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=4ec330ae30264164a9a34ad83de9d9c1&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<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/system/tool/test_health_check.py
**Line:** 117:127
**Comment:**
*Logic Error: This test mutates global app config but does not restore
the previous `APP_NAME` value; it unconditionally removes the key. If
`APP_NAME` was set before the test, subsequent tests in the module will run
with altered configuration, creating order-dependent behavior. Save the
original value and restore it exactly in `finally`.
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%2F41924&comment_hash=08c1837ea68e8bac99c25e94df313e2cd6bef9445e4b69c271811111b2c7b28c&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41924&comment_hash=08c1837ea68e8bac99c25e94df313e2cd6bef9445e4b69c271811111b2c7b28c&reaction=dislike'>👎</a>
##########
tests/unit_tests/mcp_service/test_mcp_e2e_smoke.py:
##########
@@ -0,0 +1,214 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+"""End-to-end smoke test for the real MCP ASGI/HTTP stack.
+
+Every other test under ``tests/unit_tests/mcp_service/`` drives tools via
+FastMCP's in-process ``Client(mcp)``, which talks to the bare ``FastMCP``
+object through an in-memory transport (``FastMCPTransport``). That path
+never serializes a JSON-RPC message over HTTP and never runs through the
+Starlette ASGI app that ``superset.mcp_service.server.run_server()`` builds
+via ``mcp_instance.http_app(...)`` -- so the FastMCP-level middleware stack
+(``LoggingMiddleware``, ``GlobalErrorHandlerMiddleware``,
+``StructuredContentStripperMiddleware``, etc., see
+``build_middleware_list()`` in ``server.py``) is never actually exercised in
+CI.
+
+This module closes that gap with a single smoke test file that:
+
+1. Builds the *real* ASGI app the way ``run_server()`` does --
+ ``mcp.http_app(transport="streamable-http", stateless_http=True)`` --
+ with the production FastMCP-level middleware list attached.
+2. Serves it in-process over real MCP streamable-HTTP JSON-RPC using
+ ``httpx.ASGITransport`` (no real TCP socket, no real network).
+3. Drives it with FastMCP's own high-level ``Client``, proving the full
+ request/response wire protocol (headers, session negotiation, JSON-RPC
+ envelopes) round-trips correctly through the real transport.
+
+Deliberately out of scope: the Starlette-level ``BrowserHelloMiddleware``
+(added via ``_build_starlette_middleware()`` in ``server.py``) only
+intercepts ``GET``/``HEAD`` requests carrying a browser ``Accept`` header
+(see ``BrowserHelloMiddleware.dispatch`` in ``jwt_verifier.py``); it never
+touches the ``POST /mcp`` JSON-RPC path this test exercises, and wiring it
+up would require standing up real auth/config machinery for no additional
+coverage here.
+
+Global state note: the FastMCP middleware list lives on the *shared*
+``superset.mcp_service.app.mcp`` singleton that every other test file also
+imports (``mcp.middleware`` is a plain list mutated in place by
+``add_middleware()``). The ``_real_asgi_client`` helper below snapshots and
+restores that list so this file cannot leak middleware into other tests.
+"""
+
+import contextlib
+from collections.abc import AsyncIterator, Mapping
+from typing import Any
+from unittest.mock import Mock, patch
+
+import anyio
+import httpx
+import pytest
+from fastmcp import Client
+from fastmcp.client.transports import StreamableHttpTransport
+from starlette.applications import Starlette
+
+from superset.mcp_service.app import mcp
+from superset.mcp_service.server import build_middleware_list
+from superset.utils import json
+
+ASGIMessage = Mapping[str, Any]
+
+
[email protected]
+async def _run_asgi_lifespan(app: Starlette) -> AsyncIterator[None]:
+ """Manually drive the ASGI lifespan protocol for ``app``.
+
+ FastMCP's streamable-HTTP app only creates its
+ ``StreamableHTTPSessionManager`` (and starts its task group) inside the
+ app's ``lifespan`` context manager -- see ``create_streamable_http_app``
+ in ``fastmcp.server.http``. ``httpx.ASGITransport`` drives the ASGI
+ ``http`` scope but never sends ``lifespan`` events, so without this
+ helper every request would fail because the session manager was never
+ started. This is the same protocol libraries like ``asgi-lifespan``
+ implement; it's inlined here to avoid a new test-only dependency.
+ """
+ send_stream, receive_stream =
anyio.create_memory_object_stream[ASGIMessage](4)
+ startup_complete = anyio.Event()
+ shutdown_complete = anyio.Event()
+ startup_failure: list[str] = []
+
+ async def receive() -> ASGIMessage:
+ return await receive_stream.receive()
+
+ async def send(message: ASGIMessage) -> None:
+ if message["type"] == "lifespan.startup.complete":
+ startup_complete.set()
+ elif message["type"] == "lifespan.startup.failed":
+ startup_failure.append(message.get("message", "startup failed"))
+ startup_complete.set()
+ elif message["type"] == "lifespan.shutdown.complete":
+ shutdown_complete.set()
+ elif message["type"] == "lifespan.shutdown.failed":
+ shutdown_complete.set()
Review Comment:
**Suggestion:** The lifespan helper swallows `lifespan.shutdown.failed` and
continues as if teardown succeeded. That masks real shutdown failures (and can
hide leaked tasks/resources), making the smoke test report green even when the
ASGI app cannot shut down cleanly. Capture the failure message and raise after
`shutdown_complete` just like startup failures are handled. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ ASGI shutdown failures hidden in MCP e2e smoke test.
- ⚠️ Real MCP ASGI teardown errors can go undetected.
- ⚠️ Resource leaks may persist despite passing smoke tests.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. The ASGI lifespan helper `_run_asgi_lifespan` is defined in
`tests/unit_tests/mcp_service/test_mcp_e2e_smoke.py:75-119` and is used by
`_real_asgi_client` at lines 121-165 to drive the real MCP ASGI app for
tests like
`test_tools_list_over_real_asgi_transport` (lines 172-188) and
`test_tools_call_health_check_over_real_asgi_transport` (lines 191-214).
2. Within `_run_asgi_lifespan`, the `send` coroutine at lines 96-105 handles
ASGI lifespan
messages; startup failures are recorded in `startup_failure` and cause a
`RuntimeError`
after `startup_complete.wait()` (lines 88-113), but shutdown messages are
only signaled
via `shutdown_complete`.
3. If the ASGI app’s lifespan implementation (e.g., FastMCP’s
`create_streamable_http_app`
session manager) encounters an error during shutdown and sends a `{"type":
"lifespan.shutdown.failed", "message": ...}` event, the `send` function
executes the
branch at lines 104-105, which only calls `shutdown_complete.set()` and
discards the
failure message.
4. The outer context in `_run_asgi_lifespan` then waits for
`shutdown_complete` at lines
117-118 and exits without ever checking for a shutdown failure, so
`_real_asgi_client` and
the e2e smoke tests will report success even when the ASGI app cannot shut
down cleanly,
hiding teardown errors or resource leaks in the real MCP ASGI/HTTP stack.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=6fe82bfc0e0d49d2981bc1f86d965661&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=6fe82bfc0e0d49d2981bc1f86d965661&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<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_mcp_e2e_smoke.py
**Line:** 104:105
**Comment:**
*Logic Error: The lifespan helper swallows `lifespan.shutdown.failed`
and continues as if teardown succeeded. That masks real shutdown failures (and
can hide leaked tasks/resources), making the smoke test report green even when
the ASGI app cannot shut down cleanly. Capture the failure message and raise
after `shutdown_complete` just like startup failures are handled.
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%2F41924&comment_hash=ad2f80c7eb06b08a1d1f3f93cd8e09a8648b94a56f69d5038da3b51829b60863&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41924&comment_hash=ad2f80c7eb06b08a1d1f3f93cd8e09a8648b94a56f69d5038da3b51829b60863&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]
