codeant-ai-for-open-source[bot] commented on code in PR #40814: URL: https://github.com/apache/superset/pull/40814#discussion_r3391284272
########## tests/unit_tests/test_mcp_stdio_entrypoint.py: ########## @@ -0,0 +1,92 @@ +# 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. + +import sys +from importlib import util +from pathlib import Path +from types import ModuleType +from typing import Any, cast +from unittest.mock import MagicMock + +import click +import pytest + +_ENTRYPOINT_PATH = ( + Path(__file__).resolve().parents[2] / "superset/mcp_service/__main__.py" +) + + +def _install_entrypoint_stubs(monkeypatch: pytest.MonkeyPatch) -> None: + superset_module = cast(Any, ModuleType("superset")) + superset_module.__path__ = [] + + mcp_service_module = cast(Any, ModuleType("superset.mcp_service")) + mcp_service_module.__path__ = [] + + app_module = cast(Any, ModuleType("superset.mcp_service.app")) + app_module.init_fastmcp_server = MagicMock() + app_module.mcp = MagicMock() + + middleware_module = cast(Any, ModuleType("superset.mcp_service.middleware")) + middleware_module.create_response_size_guard_middleware = lambda: None + + server_module = cast(Any, ModuleType("superset.mcp_service.server")) + server_module.build_middleware_list = lambda: [] + + monkeypatch.setitem(sys.modules, "superset", superset_module) + monkeypatch.setitem(sys.modules, "superset.mcp_service", mcp_service_module) + monkeypatch.setitem(sys.modules, "superset.mcp_service.app", app_module) + monkeypatch.setitem( + sys.modules, + "superset.mcp_service.middleware", + middleware_module, + ) + monkeypatch.setitem(sys.modules, "superset.mcp_service.server", server_module) + + +def _load_entrypoint(monkeypatch: pytest.MonkeyPatch) -> None: + module_name = "superset.mcp_service.__main__" + spec = util.spec_from_file_location(module_name, _ENTRYPOINT_PATH) + assert spec is not None + assert spec.loader is not None + + module = util.module_from_spec(spec) + monkeypatch.setitem(sys.modules, module_name, module) + spec.loader.exec_module(module) + + +def test_stdio_click_output_is_redirected_to_stderr( + monkeypatch: pytest.MonkeyPatch, + capsys: pytest.CaptureFixture[str], +) -> None: + """click output should use the saved original functions in stdio mode.""" + original_echo = click.echo + original_secho = click.secho + + monkeypatch.setenv("FASTMCP_TRANSPORT", "stdio") + monkeypatch.setattr(click, "echo", original_echo) + monkeypatch.setattr(click, "secho", original_secho) + _install_entrypoint_stubs(monkeypatch) + _load_entrypoint(monkeypatch) + + click.echo("plain message") + click.secho("styled message") Review Comment: **Suggestion:** This test imports the entrypoint, which monkey-patches global `click.echo`/`click.secho`, but it never restores those globals afterward; that can leak state into later tests and create order-dependent failures. Restore the original Click functions in teardown (or use a context/finally block around the module load and assertions). [stale reference] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx - ⚠️ MCP stdio test leaves click.echo patched globally between tests. - ⚠️ CLI integration tests run with altered Click output streams. ``` </details> <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. The test `test_stdio_click_output_is_redirected_to_stderr` in `tests/unit_tests/test_mcp_stdio_entrypoint.py` (lines 72-92) saves the current `click.echo` and `click.secho` into `original_echo` and `original_secho` at lines 77-78. 2. The test sets `FASTMCP_TRANSPORT="stdio"` (line 80), restores `click.echo` and `click.secho` via `monkeypatch.setattr(click, "echo", original_echo)` and `monkeypatch.setattr(click, "secho", original_secho)` (lines 81-82), installs stub Superset/MCP modules, and then calls `_load_entrypoint(monkeypatch)` (line 84). 3. `_load_entrypoint` (lines 61-69) loads `superset/mcp_service/__main__.py`, whose top-level stdio block (lines 34-47) defines `echo_to_stderr` and `secho_to_stderr` and assigns them directly to `click.echo` and `click.secho` at lines 46-47; these assignments bypass `monkeypatch`, so they are not automatically reverted. 4. After the test asserts on `capsys.readouterr()` (lines 86-91) and returns, pytest only undoes the explicit `monkeypatch` changes; `click.echo` and `click.secho` remain globally patched to the stderr wrappers for the rest of the process, so later tests that import Click-based CLIs such as `superset.cli.importexport`, `superset.cli.thumbnails`, or `superset.cli.update` (see `tests/integration_tests/cli_tests.py` lines 29-31 and their use of those modules) will run with altered Click output behaviour, creating hidden cross-test coupling and order-dependent test outcomes. ``` </details> [Fix in Cursor](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=c0d73a7392cb49ebbb3fdc6e42b65419&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) | [Fix in VSCode Claude](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=c0d73a7392cb49ebbb3fdc6e42b65419&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/test_mcp_stdio_entrypoint.py **Line:** 83:87 **Comment:** *Stale Reference: This test imports the entrypoint, which monkey-patches global `click.echo`/`click.secho`, but it never restores those globals afterward; that can leak state into later tests and create order-dependent failures. Restore the original Click functions in teardown (or use a context/finally block around the module load and assertions). 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%2F40814&comment_hash=d169c3e81dd891ed4f5287b8fa53234ee8951d821b7f7b154eb4b2b8c52ba819&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40814&comment_hash=d169c3e81dd891ed4f5287b8fa53234ee8951d821b7f7b154eb4b2b8c52ba819&reaction=dislike'>👎</a> ########## superset/mcp_service/__main__.py: ########## @@ -32,14 +32,19 @@ # Monkey-patch click to redirect output to stderr in stdio mode if os.environ.get("FASTMCP_TRANSPORT", "stdio") == "stdio": + original_echo = click.echo original_secho = click.secho - def secho_to_stderr(*args: Any, **kwargs: Any) -> Any: + def echo_to_stderr(*args: Any, **kwargs: Any) -> None: kwargs["file"] = sys.stderr - return original_secho(*args, **kwargs) + original_echo(*args, **kwargs) + def secho_to_stderr(*args: Any, **kwargs: Any) -> None: + kwargs["file"] = sys.stderr + original_secho(*args, **kwargs) Review Comment: **Suggestion:** The wrappers force `file` via `kwargs` and then forward `*args`; if any caller passes the output stream as a positional argument (the second positional parameter in Click), this will raise `TypeError: got multiple values for argument 'file'`. Normalize arguments so `file` is set without creating a positional/keyword collision (for example, by handling positional `file` explicitly before forwarding). [api mismatch] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx - ❌ MCP stdio processes crash when click.echo called with file positional. - ⚠️ Any custom MCP tool using positional file may break. ``` </details> <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. In `superset/mcp_service/__main__.py`, the stdio guard at lines 34-47 defines `echo_to_stderr` (lines 38-41) and assigns it to `click.echo` at line 46, while preserving the original implementation as `original_echo` at line 35. 2. Start a Python process with `FASTMCP_TRANSPORT=stdio` and import `superset.mcp_service.__main__`; this executes the top-level block and globally patches `click.echo` to point at `echo_to_stderr` (line 46). 3. In that same process, call `click.echo("plain message", sys.stderr)` from any code path (for example, from an MCP tool handler running under this entrypoint); at call time, Python resolves `click.echo` to `echo_to_stderr` (line 38) with `args=("plain message", sys.stderr)` and an empty `kwargs` dict. 4. Inside `echo_to_stderr`, the wrapper sets `kwargs["file"] = sys.stderr` (line 39) and then calls `original_echo(*args, **kwargs)` (line 40); because `sys.stderr` is already being passed as the second positional argument corresponding to Click's `file` parameter, adding a `file` keyword argument causes `TypeError: echo() got multiple values for argument 'file'`, terminating the MCP stdio process. ``` </details> [Fix in Cursor](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=d5ddc712435e4728800ba709858be0a7&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) | [Fix in VSCode Claude](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=d5ddc712435e4728800ba709858be0a7&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:** superset/mcp_service/__main__.py **Line:** 38:44 **Comment:** *Api Mismatch: The wrappers force `file` via `kwargs` and then forward `*args`; if any caller passes the output stream as a positional argument (the second positional parameter in Click), this will raise `TypeError: got multiple values for argument 'file'`. Normalize arguments so `file` is set without creating a positional/keyword collision (for example, by handling positional `file` explicitly before forwarding). 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%2F40814&comment_hash=0423d4c1933af88e0ee8146e5f6c9b88a49742b6d4608975ea4bbc66cffd9444&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40814&comment_hash=0423d4c1933af88e0ee8146e5f6c9b88a49742b6d4608975ea4bbc66cffd9444&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]
