codeant-ai-for-open-source[bot] commented on code in PR #37023:
URL: https://github.com/apache/superset/pull/37023#discussion_r2677790864
##########
superset/mcp_service/__main__.py:
##########
@@ -123,7 +123,7 @@ def main() -> None:
if transport == "streamable-http":
host = os.environ.get("FASTMCP_HOST", "127.0.0.1")
port = int(os.environ.get("FASTMCP_PORT", "5008"))
Review Comment:
**Suggestion:** Parsing `FASTMCP_PORT` with `int(os.environ.get(...))` will
raise a ValueError if the environment variable is set to a non-integer string;
validate or fallback safely when conversion fails. [type error]
**Severity Level:** Minor ⚠️
```suggestion
port_env = os.environ.get("FASTMCP_PORT", "5008")
try:
port = int(port_env)
except (TypeError, ValueError):
# Fallback to default port when env var is invalid
port = 5008
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
Calling int() directly on an environment value can raise ValueError for
malformed input; the suggested try/except fallback makes startup more robust
and avoids crashing the process due to a bad env var.
</details>
<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:** 125:125
**Comment:**
*Type Error: Parsing `FASTMCP_PORT` with `int(os.environ.get(...))`
will raise a ValueError if the environment variable is set to a non-integer
string; validate or fallback safely when conversion fails.
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.
```
</details>
##########
tests/unit_tests/mcp_service/test_mcp_tool_registration.py:
##########
@@ -49,3 +49,120 @@ def test_tool_import_works():
# Should return a decorator function
decorator = tool(name="test", description="test")
assert callable(decorator)
+
+
+def test_prompt_import_works():
+ """Test that prompt can be imported from superset_core.mcp after
+ initialization."""
+ from superset_core.mcp import prompt
+
+ # Should be callable
+ assert callable(prompt)
+
+ # Should return a decorator function
+ decorator = prompt(name="test", description="test")
+ assert callable(decorator)
+
+
+def _find_mcp_creation_line(tree):
+ """Find line number of mcp = create_mcp_app() assignment."""
+ import ast
+
+ for node in tree.body:
+ if not isinstance(node, ast.Assign):
+ continue
+ for target in node.targets:
+ if not isinstance(target, ast.Name) or target.id != "mcp":
+ continue
+ if not isinstance(node.value, ast.Call):
+ continue
+ if (
+ hasattr(node.value.func, "id")
+ and node.value.func.id == "create_mcp_app"
+ ):
+ return node.lineno
+ return None
+
+
+def _find_injection_call_line(tree):
+ """Find line number of initialize_core_mcp_dependencies() call."""
+ import ast
+
+ for node in tree.body:
+ if not isinstance(node, ast.Expr):
+ continue
+ if not isinstance(node.value, ast.Call):
+ continue
+ func = node.value.func
+ if hasattr(func, "id") and func.id ==
"initialize_core_mcp_dependencies":
Review Comment:
**Suggestion:** The helper `_find_injection_call_line` only checks for
`ast.Call.func` having an `.id` (i.e., a bare name), so it will fail to detect
calls like `module.initialize_core_mcp_dependencies()` where `func` is an
`ast.Attribute`; update to handle both `ast.Name` and `ast.Attribute` so the
initialize call is reliably detected. [logic error]
**Severity Level:** Minor ⚠️
```suggestion
func_name = None
if isinstance(func, ast.Name):
func_name = func.id
elif isinstance(func, ast.Attribute):
func_name = func.attr
if func_name == "initialize_core_mcp_dependencies":
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
Same problem as the previous helper: it only checks for ast.Name via .id and
will miss module.initialize_core_mcp_dependencies() calls. The improved code
correctly normalizes ast.Name and ast.Attribute to a func_name before
comparison, resolving a real false-negative detection scenario for the test.
</details>
<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_tool_registration.py
**Line:** 97:97
**Comment:**
*Logic Error: The helper `_find_injection_call_line` only checks for
`ast.Call.func` having an `.id` (i.e., a bare name), so it will fail to detect
calls like `module.initialize_core_mcp_dependencies()` where `func` is an
`ast.Attribute`; update to handle both `ast.Name` and `ast.Attribute` so the
initialize call is reliably detected.
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.
```
</details>
##########
tests/unit_tests/mcp_service/system/tool/test_get_schema.py:
##########
@@ -328,6 +328,71 @@ async def test_get_schema_default_sort_values(self,
mock_filters, mcp_server):
assert info["default_sort_direction"] == "desc"
+class TestGetSchemaLazyImports:
+ """Test lazy imports for DAOs to avoid circular dependencies."""
+
+ def test_no_module_level_dao_imports(self):
+ """Verify get_schema.py has no module-level DAO imports.
+
+ This is critical because module-level DAO imports trigger SQLAlchemy
model
+ loading, which requires Flask app to be initialized (for encrypted
fields).
+ DAOs must be imported inside functions (lazy imports) to avoid this
issue.
+ """
+ import ast
+
+ with open("superset/mcp_service/system/tool/get_schema.py", "r") as f:
+ source = f.read()
+
+ tree = ast.parse(source)
+
+ # Check only top-level imports (direct children of Module)
+ dao_imports = []
+ for node in tree.body:
+ if isinstance(node, ast.ImportFrom):
+ module_name = node.module or ""
+ if "dao" in module_name.lower():
+ dao_imports.append(module_name)
Review Comment:
**Suggestion:** Logic bug: the module-level import check only inspects
ImportFrom nodes and misses top-level "import ..." statements (ast.Import).
Module-level imports like "import superset.daos.chart" will not be detected,
producing false negatives; update the check to also inspect ast.Import nodes
and alias names. [logic error]
**Severity Level:** Minor ⚠️
```suggestion
# Handle "from x import y" forms and check both module and alias
names
if isinstance(node, ast.ImportFrom):
module_name = node.module or ""
if "dao" in module_name.lower() or any("dao" in
alias.name.lower() for alias in node.names):
dao_imports.append(module_name or ", ".join(alias.name
for alias in node.names))
# Handle plain "import x.y" forms
elif isinstance(node, ast.Import):
for alias in node.names:
if "dao" in alias.name.lower():
dao_imports.append(alias.name)
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
This suggestion correctly identifies a real gap: the test only inspects
ast.ImportFrom nodes and will miss top-level "import x.y" statements that could
reference DAOs. The proposed improved_code handles ast.Import and checks alias
names, fixing false negatives. It's a targeted test improvement that resolves
an actual detection bug.
</details>
<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_get_schema.py
**Line:** 351:354
**Comment:**
*Logic Error: Logic bug: the module-level import check only inspects
ImportFrom nodes and misses top-level "import ..." statements (ast.Import).
Module-level imports like "import superset.daos.chart" will not be detected,
producing false negatives; update the check to also inspect ast.Import nodes
and alias names.
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.
```
</details>
##########
tests/unit_tests/mcp_service/test_mcp_tool_registration.py:
##########
@@ -49,3 +49,120 @@ def test_tool_import_works():
# Should return a decorator function
decorator = tool(name="test", description="test")
assert callable(decorator)
+
+
+def test_prompt_import_works():
+ """Test that prompt can be imported from superset_core.mcp after
+ initialization."""
+ from superset_core.mcp import prompt
+
+ # Should be callable
+ assert callable(prompt)
+
+ # Should return a decorator function
+ decorator = prompt(name="test", description="test")
+ assert callable(decorator)
+
+
+def _find_mcp_creation_line(tree):
+ """Find line number of mcp = create_mcp_app() assignment."""
+ import ast
+
+ for node in tree.body:
+ if not isinstance(node, ast.Assign):
+ continue
+ for target in node.targets:
+ if not isinstance(target, ast.Name) or target.id != "mcp":
+ continue
+ if not isinstance(node.value, ast.Call):
+ continue
+ if (
+ hasattr(node.value.func, "id")
+ and node.value.func.id == "create_mcp_app"
+ ):
Review Comment:
**Suggestion:** The helper `_find_mcp_creation_line` only checks for a
function name when the call's `func` is an `ast.Name` (has `.id`), but it
ignores `ast.Attribute` (e.g., `module.create_mcp_app()`), so it will miss
valid calls and return None incorrectly; update the function to handle both
`ast.Name` and `ast.Attribute` by extracting the callable name in both cases.
[logic error]
**Severity Level:** Minor ⚠️
```suggestion
func = node.value.func
func_name = None
if isinstance(func, ast.Name):
func_name = func.id
elif isinstance(func, ast.Attribute):
func_name = func.attr
if func_name == "create_mcp_app":
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
The existing helper only detects plain Name calls (create_mcp_app(...)) and
will miss attribute calls like module.create_mcp_app(...). The proposed change
extracts the callable name for both ast.Name and ast.Attribute, which fixes a
real detection gap in the AST traversal and is syntactically correct and
appropriate for this test helper.
</details>
<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_tool_registration.py
**Line:** 79:82
**Comment:**
*Logic Error: The helper `_find_mcp_creation_line` only checks for a
function name when the call's `func` is an `ast.Name` (has `.id`), but it
ignores `ast.Attribute` (e.g., `module.create_mcp_app()`), so it will miss
valid calls and return None incorrectly; update the function to handle both
`ast.Name` and `ast.Attribute` by extracting the callable name in both cases.
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.
```
</details>
##########
superset/mcp_service/app.py:
##########
@@ -275,7 +275,17 @@ def create_mcp_app(
# Create default MCP instance for backward compatibility
-mcp = create_mcp_app(stateless_http=True)
+# Note: stateless_http should be provided when calling run(), not at creation
time
+mcp = create_mcp_app()
+
+# Initialize MCP dependency injection BEFORE importing tools/prompts
+# This replaces the abstract @tool and @prompt decorators in superset_core.mcp
+# with concrete implementations that can register with the mcp instance
+from superset.core.mcp.core_mcp_injection import ( # noqa: E402
+ initialize_core_mcp_dependencies,
+)
+
+initialize_core_mcp_dependencies()
Review Comment:
**Suggestion:** Importing and executing `initialize_core_mcp_dependencies()`
at module import time can raise exceptions (it re-raises non-ImportError
exceptions) and break imports; guard the import and call, handle ImportError
separately, and log other failures rather than letting them crash the process.
[possible bug]
**Severity Level:** Critical 🚨
```suggestion
try:
from superset.core.mcp.core_mcp_injection import ( # noqa: E402
initialize_core_mcp_dependencies,
)
initialize_core_mcp_dependencies()
except ImportError as e:
logger.warning("superset_core not available, skipping MCP injection:
%s", e)
except Exception as e:
logger.error("Failed to initialize MCP dependency injection (non-import
error): %s", e)
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
The imported initializer will re-raise non-ImportError exceptions (see
core_mcp_injection). Allowing those exceptions to propagate at module
import time can crash the process. Wrapping the import and call with
explicit handling for ImportError and logging other failures makes the
module import more robust and gives clearer diagnostics. This is a
targeted, sensible hardening change that doesn't silently hide failures
(it still logs errors).
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/mcp_service/app.py
**Line:** 284:288
**Comment:**
*Possible Bug: Importing and executing
`initialize_core_mcp_dependencies()` at module import time can raise exceptions
(it re-raises non-ImportError exceptions) and break imports; guard the import
and call, handle ImportError separately, and log other failures rather than
letting them crash the process.
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.
```
</details>
##########
tests/unit_tests/mcp_service/system/tool/test_get_schema.py:
##########
@@ -328,6 +328,71 @@ async def test_get_schema_default_sort_values(self,
mock_filters, mcp_server):
assert info["default_sort_direction"] == "desc"
+class TestGetSchemaLazyImports:
+ """Test lazy imports for DAOs to avoid circular dependencies."""
+
+ def test_no_module_level_dao_imports(self):
+ """Verify get_schema.py has no module-level DAO imports.
+
+ This is critical because module-level DAO imports trigger SQLAlchemy
model
+ loading, which requires Flask app to be initialized (for encrypted
fields).
+ DAOs must be imported inside functions (lazy imports) to avoid this
issue.
+ """
+ import ast
+
+ with open("superset/mcp_service/system/tool/get_schema.py", "r") as f:
+ source = f.read()
+
+ tree = ast.parse(source)
+
+ # Check only top-level imports (direct children of Module)
+ dao_imports = []
+ for node in tree.body:
+ if isinstance(node, ast.ImportFrom):
+ module_name = node.module or ""
+ if "dao" in module_name.lower():
+ dao_imports.append(module_name)
+
+ # Should be empty - no module-level DAO imports
+ assert dao_imports == [], (
+ f"Found module-level DAO imports: {dao_imports}. "
+ "DAOs must be imported inside functions to avoid circular
dependencies."
+ )
+
+ def test_dao_imports_inside_factory_functions(self):
+ """Verify DAOs are imported inside factory functions."""
+ import ast
+
+ with open("superset/mcp_service/system/tool/get_schema.py", "r") as f:
+ source = f.read()
+
+ tree = ast.parse(source)
+
+ # Find DAO imports inside functions
+ function_dao_imports = []
+ for node in ast.walk(tree):
+ if isinstance(node, ast.FunctionDef):
Review Comment:
**Suggestion:** Logic bug: the code only checks ast.FunctionDef nodes when
searching for imports inside functions and will miss async function definitions
(ast.AsyncFunctionDef), causing false negatives if any factory function is
async; change the type check to accept both FunctionDef and AsyncFunctionDef.
[logic error]
**Severity Level:** Minor ⚠️
```suggestion
if isinstance(node, (ast.FunctionDef, ast.AsyncFunctionDef)):
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
The current AST walk only matches ast.FunctionDef and will miss async defs
(ast.AsyncFunctionDef). Accepting both types is a harmless, correct improvement
to avoid false negatives if any factory functions are async. The suggested
change is minimal and fixes a legitimate edge case in the test.
</details>
<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_get_schema.py
**Line:** 374:374
**Comment:**
*Logic Error: Logic bug: the code only checks ast.FunctionDef nodes
when searching for imports inside functions and will miss async function
definitions (ast.AsyncFunctionDef), causing false negatives if any factory
function is async; change the type check to accept both FunctionDef and
AsyncFunctionDef.
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.
```
</details>
##########
tests/unit_tests/mcp_service/test_mcp_tool_registration.py:
##########
@@ -49,3 +49,120 @@ def test_tool_import_works():
# Should return a decorator function
decorator = tool(name="test", description="test")
assert callable(decorator)
+
+
+def test_prompt_import_works():
+ """Test that prompt can be imported from superset_core.mcp after
+ initialization."""
+ from superset_core.mcp import prompt
+
+ # Should be callable
+ assert callable(prompt)
+
+ # Should return a decorator function
+ decorator = prompt(name="test", description="test")
+ assert callable(decorator)
+
+
+def _find_mcp_creation_line(tree):
+ """Find line number of mcp = create_mcp_app() assignment."""
+ import ast
+
+ for node in tree.body:
+ if not isinstance(node, ast.Assign):
+ continue
+ for target in node.targets:
+ if not isinstance(target, ast.Name) or target.id != "mcp":
+ continue
+ if not isinstance(node.value, ast.Call):
+ continue
+ if (
+ hasattr(node.value.func, "id")
+ and node.value.func.id == "create_mcp_app"
+ ):
+ return node.lineno
+ return None
+
+
+def _find_injection_call_line(tree):
+ """Find line number of initialize_core_mcp_dependencies() call."""
+ import ast
+
+ for node in tree.body:
+ if not isinstance(node, ast.Expr):
+ continue
+ if not isinstance(node.value, ast.Call):
+ continue
+ func = node.value.func
+ if hasattr(func, "id") and func.id ==
"initialize_core_mcp_dependencies":
+ return node.lineno
+ return None
+
+
+def _find_first_tool_import_line(tree):
+ """Find line number of first tool/prompt import."""
+ import ast
+
+ for node in tree.body:
+ if not isinstance(node, ast.ImportFrom):
+ continue
+ if node.module and "superset.mcp_service.chart" in node.module:
+ return node.lineno
+ return None
+
+
+def test_mcp_app_initialization_order():
+ """Test that app.py initializes dependencies before importing tools."""
+ import ast
+
+ with open("superset/mcp_service/app.py", "r") as f:
Review Comment:
**Suggestion:** The test opens "superset/mcp_service/app.py" by a hardcoded
relative path which can fail if the test runner's current working directory is
different; resolve the module path via importlib (finding the module spec
origin) and open the located file to robustly read the source. [possible bug]
**Severity Level:** Critical 🚨
```suggestion
import importlib.util
spec = importlib.util.find_spec("superset.mcp_service.app")
if spec is None or spec.origin is None:
raise FileNotFoundError("Could not locate superset.mcp_service.app
source file")
with open(spec.origin, "r") as f:
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
Relying on a hardcoded relative path can break when pytest's CWD isn't the
repository root. Using importlib.util.find_spec to locate the module and
opening spec.origin is more robust and makes the test resilient to different
invocation contexts. The suggested code is valid and addresses a plausible test
flakiness scenario.
</details>
<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_tool_registration.py
**Line:** 118:118
**Comment:**
*Possible Bug: The test opens "superset/mcp_service/app.py" by a
hardcoded relative path which can fail if the test runner's current working
directory is different; resolve the module path via importlib (finding the
module spec origin) and open the located file to robustly read the source.
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.
```
</details>
##########
superset/mcp_service/server.py:
##########
@@ -118,7 +118,9 @@ def run_server(
os.environ[env_key] = "1"
try:
logging.info("Starting FastMCP on %s:%s", host, port)
- mcp_instance.run(transport="streamable-http", host=host, port=port)
+ mcp_instance.run(
+ transport="streamable-http", host=host, port=port,
stateless_http=True
+ )
Review Comment:
**Suggestion:** The environment flag `FASTMCP_RUNNING_{port}` is only
cleared in the `except Exception` block, so if `mcp_instance.run` raises a
BaseException (e.g. KeyboardInterrupt or SystemExit) or otherwise exits
normally, the env var can remain set and incorrectly block future starts in the
same process; ensure the env var is always cleared by using a finally block
that runs for all exit paths. [resource leak]
**Severity Level:** Minor ⚠️
```suggestion
try:
mcp_instance.run(
transport="streamable-http", host=host, port=port,
stateless_http=True
)
finally:
# Ensure the environment flag is always cleared, even for
BaseException
os.environ.pop(env_key, None)
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
The current except Exception block doesn't catch BaseException
(KeyboardInterrupt/SystemExit),
so the env flag can remain set if the process exits via those. Wrapping the
run call with a
try/finally and popping the env key in finally ensures the flag is always
cleared. This fixes a
real resource/state leak in the current code.
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/mcp_service/server.py
**Line:** 121:123
**Comment:**
*Resource Leak: The environment flag `FASTMCP_RUNNING_{port}` is only
cleared in the `except Exception` block, so if `mcp_instance.run` raises a
BaseException (e.g. KeyboardInterrupt or SystemExit) or otherwise exits
normally, the env var can remain set and incorrectly block future starts in the
same process; ensure the env var is always cleared by using a finally block
that runs for all exit paths.
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.
```
</details>
--
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]
