codeant-ai-for-open-source[bot] commented on code in PR #41924: URL: https://github.com/apache/superset/pull/41924#discussion_r3573750544
########## tests/unit_tests/mcp_service/test_mcp_e2e_smoke.py: ########## @@ -0,0 +1,223 @@ +# 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] = [] + shutdown_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_failure.append(message.get("message", "shutdown failed")) + shutdown_complete.set() + + async with anyio.create_task_group() as task_group: + task_group.start_soon( + app, {"type": "lifespan", "asgi": {"version": "3.0"}}, receive, send + ) + await send_stream.send({"type": "lifespan.startup"}) + await startup_complete.wait() + if startup_failure: + raise RuntimeError(f"ASGI app failed to start: {startup_failure[0]}") + try: + yield + finally: + await send_stream.send({"type": "lifespan.shutdown"}) + await shutdown_complete.wait() + if shutdown_failure: + raise RuntimeError( + f"ASGI app failed to shut down: {shutdown_failure[0]}" + ) + + [email protected] +async def _real_asgi_client() -> AsyncIterator[Client]: + """A FastMCP ``Client`` wired to the real ASGI app over real HTTP semantics. + + Builds the app the way ``run_server()`` does for the multi-pod/http_app + path (``server.py:938``): FastMCP-level middleware from + ``build_middleware_list()`` attached to the shared ``mcp`` instance, then + ``mcp.http_app(transport="streamable-http", stateless_http=True)``. + + The request/response cycle is driven over ``httpx.ASGITransport`` (no + real socket) using FastMCP's own ``StreamableHttpTransport`` so the + client speaks genuine MCP streamable-HTTP JSON-RPC -- initialize + handshake, session headers, and message envelopes -- rather than the + in-process ``FastMCPTransport`` every other test in this package uses. + + Deliberately a plain ``@asynccontextmanager`` used directly by each test + (``async with _real_asgi_client() as client:``) rather than a + yield-based pytest fixture: the anyio task group started inside + ``_run_asgi_lifespan`` enforces that its cancel scope is entered and + exited from the *same* asyncio Task, and pytest-asyncio does not + guarantee that a fixture's pre-yield and post-yield halves run in the + same Task. Keeping setup and teardown inside the test function's own + task sidesteps that entirely. + """ + original_middleware = list(mcp.middleware) + for middleware in build_middleware_list(): + mcp.add_middleware(middleware) + + try: + asgi_app = mcp.http_app(transport="streamable-http", stateless_http=True) + + def httpx_client_factory(**kwargs: Any) -> httpx.AsyncClient: + return httpx.AsyncClient( + transport=httpx.ASGITransport(app=asgi_app), + base_url="http://testserver";, + **kwargs, + ) + + transport = StreamableHttpTransport( + "http://testserver/mcp";, httpx_client_factory=httpx_client_factory + ) + + async with _run_asgi_lifespan(asgi_app): + async with Client(transport) as client: + yield client + finally: + # Restore the shared FastMCP singleton's middleware list in place + # (not by reassignment) so this file cannot leak state into other + # mcp_service tests, even if something else holds a reference to + # the original list object. + mcp.middleware[:] = original_middleware + + [email protected]() +async def test_tools_list_over_real_asgi_transport() -> None: + """``tools/list`` round-trips over the real ASGI app + JSON-RPC wire protocol. + + This alone proves the full stack boots: the Starlette app built by + ``http_app()``, the FastMCP-level middleware chain (Logging, + GlobalErrorHandler, StructuredContentStripper, RBAC visibility), the + streamable-HTTP session manager, and real JSON-RPC (de)serialization -- + none of which the in-process ``Client(mcp)`` tests elsewhere in this + package exercise. + """ + async with _real_asgi_client() as client: + tools = await client.list_tools() + + assert len(tools) > 0 Review Comment: **Suggestion:** This smoke test does not mock authentication for `tools/list`, so it depends on ambient auth configuration and can fail-closed (empty tool list) in environments where credential resolution is enabled or misconfigured. Make auth deterministic in this test (like the health-check test) so the assertion does not become environment-dependent/flaky. [logic error] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx - ❌ tools/list smoke test may fail under misconfigured auth. - ⚠️ MCP ASGI e2e coverage becomes environment-dependent and flaky. ``` </details> <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. The RBAC visibility middleware in `superset/mcp_service/middleware.py:8-47` implements `on_list_tools`, calling `get_user_from_request()` to derive the current user and then filtering tools by `is_tool_visible_to_current_user`, returning an empty list when authentication fails (ValueError or PermissionError) and failing open only when no auth source is configured (`MCPNoAuthSourceError`) or when there is no Flask app context. 2. The auth resolver `get_user_from_request()` in `superset/mcp_service/auth.py:14-37` selects a user based on JWT context, API key, `MCP_DEV_USERNAME`, or `g.user`, and raises `ValueError` when these sources do not yield a valid user (for example, a misconfigured `MCP_DEV_USERNAME` pointing to a non-existent database user). 3. The smoke test `test_tools_list_over_real_asgi_transport` in `tests/unit_tests/mcp_service/test_mcp_e2e_smoke.py:181-197` builds the real ASGI app via `_real_asgi_client()`, then calls `client.list_tools()` without patching `superset.mcp_service.auth.get_user_from_request`, and finally asserts `len(tools) > 0` and that `"health_check"` is present in the returned tool names. 4. In an environment where `get_user_from_request()` fails (e.g., `MCP_DEV_USERNAME` is set but the user does not exist, or an invalid API key is present), `on_list_tools()` logs a credential failure and returns `[]`, causing the assertion `len(tools) > 0` in `test_tools_list_over_real_asgi_transport` to fail; this demonstrates that the test outcome is dependent on ambient authentication configuration rather than just the ASGI/JSON-RPC stack, making the test realistically flaky across different deployments. ``` </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=06463836b810444990cb16b7e850a0b5&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=06463836b810444990cb16b7e850a0b5&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:** 192:195 **Comment:** *Logic Error: This smoke test does not mock authentication for `tools/list`, so it depends on ambient auth configuration and can fail-closed (empty tool list) in environments where credential resolution is enabled or misconfigured. Make auth deterministic in this test (like the health-check test) so the assertion does not become environment-dependent/flaky. 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=a02e760471cfdd5560d5e867d08153d74d8cc0943bafadb8afb49ed82018f684&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41924&comment_hash=a02e760471cfdd5560d5e867d08153d74d8cc0943bafadb8afb49ed82018f684&reaction=dislike'>👎</a> ########## tests/unit_tests/mcp_service/test_mcp_e2e_smoke.py: ########## @@ -0,0 +1,223 @@ +# 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] = [] + shutdown_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_failure.append(message.get("message", "shutdown failed")) + shutdown_complete.set() + + async with anyio.create_task_group() as task_group: + task_group.start_soon( + app, {"type": "lifespan", "asgi": {"version": "3.0"}}, receive, send + ) + await send_stream.send({"type": "lifespan.startup"}) + await startup_complete.wait() + if startup_failure: + raise RuntimeError(f"ASGI app failed to start: {startup_failure[0]}") + try: + yield + finally: + await send_stream.send({"type": "lifespan.shutdown"}) + await shutdown_complete.wait() + if shutdown_failure: + raise RuntimeError( + f"ASGI app failed to shut down: {shutdown_failure[0]}" + ) + + [email protected] +async def _real_asgi_client() -> AsyncIterator[Client]: + """A FastMCP ``Client`` wired to the real ASGI app over real HTTP semantics. + + Builds the app the way ``run_server()`` does for the multi-pod/http_app + path (``server.py:938``): FastMCP-level middleware from + ``build_middleware_list()`` attached to the shared ``mcp`` instance, then + ``mcp.http_app(transport="streamable-http", stateless_http=True)``. + + The request/response cycle is driven over ``httpx.ASGITransport`` (no + real socket) using FastMCP's own ``StreamableHttpTransport`` so the + client speaks genuine MCP streamable-HTTP JSON-RPC -- initialize + handshake, session headers, and message envelopes -- rather than the + in-process ``FastMCPTransport`` every other test in this package uses. + + Deliberately a plain ``@asynccontextmanager`` used directly by each test + (``async with _real_asgi_client() as client:``) rather than a + yield-based pytest fixture: the anyio task group started inside + ``_run_asgi_lifespan`` enforces that its cancel scope is entered and + exited from the *same* asyncio Task, and pytest-asyncio does not + guarantee that a fixture's pre-yield and post-yield halves run in the + same Task. Keeping setup and teardown inside the test function's own + task sidesteps that entirely. + """ + original_middleware = list(mcp.middleware) + for middleware in build_middleware_list(): + mcp.add_middleware(middleware) + + try: + asgi_app = mcp.http_app(transport="streamable-http", stateless_http=True) + + def httpx_client_factory(**kwargs: Any) -> httpx.AsyncClient: + return httpx.AsyncClient( + transport=httpx.ASGITransport(app=asgi_app), + base_url="http://testserver";, + **kwargs, + ) + + transport = StreamableHttpTransport( + "http://testserver/mcp";, httpx_client_factory=httpx_client_factory + ) + + async with _run_asgi_lifespan(asgi_app): + async with Client(transport) as client: + yield client + finally: + # Restore the shared FastMCP singleton's middleware list in place + # (not by reassignment) so this file cannot leak state into other + # mcp_service tests, even if something else holds a reference to + # the original list object. + mcp.middleware[:] = original_middleware + + [email protected]() +async def test_tools_list_over_real_asgi_transport() -> None: + """``tools/list`` round-trips over the real ASGI app + JSON-RPC wire protocol. + + This alone proves the full stack boots: the Starlette app built by + ``http_app()``, the FastMCP-level middleware chain (Logging, + GlobalErrorHandler, StructuredContentStripper, RBAC visibility), the + streamable-HTTP session manager, and real JSON-RPC (de)serialization -- + none of which the in-process ``Client(mcp)`` tests elsewhere in this + package exercise. + """ + async with _real_asgi_client() as client: + tools = await client.list_tools() + + assert len(tools) > 0 + tool_names = {tool.name for tool in tools} + assert "health_check" in tool_names + + [email protected]() +async def test_tools_call_health_check_over_real_asgi_transport() -> None: + """A real ``tools/call`` for ``health_check`` over the real ASGI transport. + + ``health_check`` is ``@tool(protect=True)`` by default (auth wraps every + tool unless ``protect=False`` is explicit), so authentication is mocked + the same way every other MCP tool test does it: patching + ``get_user_from_request``. That function is called directly from + ``_setup_user_context()`` regardless of how the request arrived (in + -process client vs. real HTTP transport), so the same mock works Review Comment: **Suggestion:** There is an obvious typo in the docstring (`in -process`) that should be corrected to `in-process` to avoid misleading wording in test documentation. [typo] <details> <summary><b>Severity Level:</b> Minor 🧹</summary> ```mdx - ⚠️ Health-check smoke test docstring contains malformed phrase. - ⚠️ Cosmetic typo only; functionality and assertions unchanged. ``` </details> <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. Open `tests/unit_tests/mcp_service/test_mcp_e2e_smoke.py` and inspect the docstring for `test_tools_call_health_check_over_real_asgi_transport` around lines 200-211 in the current PR version. 2. At `tests/unit_tests/mcp_service/test_mcp_e2e_smoke.py:208-209`, the explanatory sentence reads: ``_setup_user_context()`` regardless of how the request arrived (in<newline> -process client vs. real HTTP transport), splitting the word "in-process" across lines as "in" and "-process". 3. Run `pytest tests/unit_tests/mcp_service/test_mcp_e2e_smoke.py` and observe that the tests pass; the typo does not affect execution but remains visible in IDE tooltips and any generated documentation or code browsing. 4. Correcting the phrase to "in-process client vs. real HTTP transport" in this docstring would remove the awkward line break and hyphenation, improving documentation clarity while leaving test behavior unchanged. ``` </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=82fc50794dac4917ab0b0056bb1822cd&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=82fc50794dac4917ab0b0056bb1822cd&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:** 208:209 **Comment:** *Typo: There is an obvious typo in the docstring (`in -process`) that should be corrected to `in-process` to avoid misleading wording in test documentation. 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=f66cfa7ed3ecea297710692e6f48d8ffeda8ccb43389bc362a8b87c07271c54f&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41924&comment_hash=f66cfa7ed3ecea297710692e6f48d8ffeda8ccb43389bc362a8b87c07271c54f&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]
