You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by "aminghadersohi (via GitHub)" <gi...@apache.org> on 2026/01/09 22:22:23 UTC
[PR] fix(mcp): resolve startup failures from circular DAO imports [superset]
aminghadersohi opened a new pull request, #37023:
URL: https://github.com/apache/superset/pull/37023
## Summary
Fixes MCP service startup failures caused by circular import issues and deprecation warnings.
**Root cause**: `get_schema.py` imported DAOs at module level, triggering SQLAlchemy model loading which requires Flask app initialization (for encrypted fields). This caused the import chain to crash before Flask was ready.
## Changes
- Move DAO imports from module-level to inside factory functions in `get_schema.py` (lazy imports)
- Add `initialize_core_mcp_dependencies()` call before tool/prompt imports in `app.py` to fix `@prompt` decorator `NotImplementedError`
- Move `stateless_http=True` from `create_mcp_app()` constructor to `run()` calls in `__main__.py` and `server.py` to fix FastMCP deprecation warning
- Add unit tests verifying initialization order and lazy import patterns
## Test plan
- [x] All 27 MCP unit tests pass
- [x] Pre-commit hooks pass
- [x] Manually tested all MCP tools via connected client
π€ Generated with [Claude Code](https://claude.com/claude-code)
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(mcp): resolve startup failures from circular DAO imports [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(mcp): resolve startup failures from circular DAO imports [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #37023:
URL: https://github.com/apache/superset/pull/37023#issuecomment-3730779517
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(mcp): resolve startup failures from circular DAO imports [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on PR #37023:
URL: https://github.com/apache/superset/pull/37023#issuecomment-3730779263
CodeAnt AI is reviewing your PR.
---
### Thanks for using CodeAnt! π
We're free for open-source projects. if you're enjoying it, help us grow by sharing.
[Share on X](https://twitter.com/intent/tweet?text=Just%20tried%20%40CodeAntAI%20for%20automated%20code%20review%20and%20I%27m%20impressed%21%20Free%20for%20open%20source%20with%20a%20free%20trial%20for%20private%20repos.%20Worth%20checking%20out%3A&url=https%3A//codeant.ai) Β·
[Reddit](https://www.reddit.com/submit?title=Check%20out%20CodeAnt%20for%20automated%20code%20review&text=Just%20tried%20CodeAnt%20for%20automated%20code%20review%20and%20I%27m%20impressed%21%20Free%20for%20open%20source%20with%20a%20free%20trial%20for%20private%20repos.%20Worth%20checking%20out%3A%20https%3A//codeant.ai) Β·
[LinkedIn](https://www.linkedin.com/sharing/share-offsite/?url=https%3A%2F%2Fcodeant.ai&mini=true&title=Check%20out%20CodeAnt%20for%20automated%20code%20review&summary=Just%20tried%20CodeAnt%20for%20automated%20code%20review%20and%20I%27m%20impressed%21%20Free%20for%20open%20source%20with%20a%20free%20trial%20for%20private%20repos)
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(mcp): resolve startup failures from circular DAO imports [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on PR #37023:
URL: https://github.com/apache/superset/pull/37023#issuecomment-3730787994
## Nitpicks π
<table>
<tr><td>π <strong>No security issues identified</strong></td></tr>
<tr><td>β‘ <strong>Recommended areas for review</strong><br><br>
- [ ] <a href='https://github.com/apache/superset/pull/37023/files#diff-9ebe980fc09c4554a65355be83dcd8cadac049bc64134a09aaab14831f0e6d51R121-R123'><strong>Behavior Change</strong></a><br>Passing `stateless_http=True` at `mcp_instance.run(...)` can change request/session semantics compared to the previous placement. Middleware or auth providers that rely on stateful HTTP (cookies, session-backed auth) may break or behave differently. Verify compatibility of all auth/middleware with stateless mode and add targeted tests.<br>
- [ ] <a href='https://github.com/apache/superset/pull/37023/files#diff-e408acc5e688745068830ee2c9a6c8d55bdbaff37663e2203e303ca91baa958fR125-R126'><strong>Environment parsing</strong></a><br>`port = int(os.environ.get("FASTMCP_PORT", "5008"))` will raise a ValueError if `FASTMCP_PORT` is set to a non-integer string, causing the process to crash on startup. This block lacks robust validation and a safe fallback path.<br>
- [ ] <a href='https://github.com/apache/superset/pull/37023/files#diff-e408acc5e688745068830ee2c9a6c8d55bdbaff37663e2203e303ca91baa958fR126-R126'><strong>Compatibility Risk</strong></a><br>The new call adds the `stateless_http=True` keyword to `mcp.run(...)`. If deployed with an older FastMCP/mcp implementation that does not accept this keyword, the call will raise a TypeError at startup and the service will fail to start. Consider a compatibility fallback or runtime feature detection.<br>
- [ ] <a href='https://github.com/apache/superset/pull/37023/files#diff-0b1268c08fa98c96adafc6abfc3d24dcf863c08e1959c49410c48bc78e74ae9dR288-R288'><strong>Dependency injection failure fallout</strong></a><br>The call to `initialize_core_mcp_dependencies()` runs at import time and will re-raise
non-ImportError exceptions from the injection routine. That can cause the entire module
import to fail and block application startup. Evaluate making the call resilient or
failing more gracefully so a non-fatal problem in injection doesn't break imports.<br>
- [ ] <a href='https://github.com/apache/superset/pull/37023/files#diff-9ebe980fc09c4554a65355be83dcd8cadac049bc64134a09aaab14831f0e6d51R111-R114'><strong>Run Guard</strong></a><br>There is no defensive check that `mcp_instance` is non-None and actually exposes a `run` method before calling it. If `init_fastmcp_server` failed or returned an unexpected object, this will raise. Add validation and ensure the environment marker is cleaned up on precondition failures.<br>
</td></tr>
</table>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(mcp): resolve startup failures from circular DAO imports [superset]
Posted by "sadpandajoe (via GitHub)" <gi...@apache.org>.
sadpandajoe merged PR #37023:
URL: https://github.com/apache/superset/pull/37023
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(mcp): resolve startup failures from circular DAO imports [superset]
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #37023:
URL: https://github.com/apache/superset/pull/37023#issuecomment-3730807765
## [Codecov](https://app.codecov.io/gh/apache/superset/pull/37023?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:x: Patch coverage is `0%` with `8 lines` in your changes missing coverage. Please review.
:white_check_mark: Project coverage is 48.29%. Comparing base ([`53dddf4`](https://app.codecov.io/gh/apache/superset/commit/53dddf4db26d32ec86673a6d42b009cc49fbd09d?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`9b96a19`](https://app.codecov.io/gh/apache/superset/commit/9b96a1926c7dcaebd1a320ddcc81d046af6bd5a4?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
| [Files with missing lines](https://app.codecov.io/gh/apache/superset/pull/37023?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [superset/mcp\_service/app.py](https://app.codecov.io/gh/apache/superset/pull/37023?src=pr&el=tree&filepath=superset%2Fmcp_service%2Fapp.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQvbWNwX3NlcnZpY2UvYXBwLnB5) | 0.00% | [3 Missing :warning: ](https://app.codecov.io/gh/apache/superset/pull/37023?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [superset/mcp\_service/system/tool/get\_schema.py](https://app.codecov.io/gh/apache/superset/pull/37023?src=pr&el=tree&filepath=superset%2Fmcp_service%2Fsystem%2Ftool%2Fget_schema.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQvbWNwX3NlcnZpY2Uvc3lzdGVtL3Rvb2wvZ2V0X3NjaGVtYS5weQ==) | 0.00% | [3 Missing :warning: ](https://app.codecov.io/gh/apache/superset/pull/37023?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [superset/mcp\_service/\_\_main\_\_.py](https://app.codecov.io/gh/apache/superset/pull/37023?src=pr&el=tree&filepath=superset%2Fmcp_service%2F__main__.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQvbWNwX3NlcnZpY2UvX19tYWluX18ucHk=) | 0.00% | [1 Missing :warning: ](https://app.codecov.io/gh/apache/superset/pull/37023?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [superset/mcp\_service/server.py](https://app.codecov.io/gh/apache/superset/pull/37023?src=pr&el=tree&filepath=superset%2Fmcp_service%2Fserver.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQvbWNwX3NlcnZpY2Uvc2VydmVyLnB5) | 0.00% | [1 Missing :warning: ](https://app.codecov.io/gh/apache/superset/pull/37023?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #37023 +/- ##
===========================================
+ Coverage 0 48.29% +48.29%
===========================================
Files 0 642 +642
Lines 0 47900 +47900
Branches 0 5227 +5227
===========================================
+ Hits 0 23133 +23133
- Misses 0 24111 +24111
- Partials 0 656 +656
```
| [Flag](https://app.codecov.io/gh/apache/superset/pull/37023/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Ξ | |
|---|---|---|
| [hive](https://app.codecov.io/gh/apache/superset/pull/37023/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `42.92% <0.00%> (?)` | |
| [presto](https://app.codecov.io/gh/apache/superset/pull/37023/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `46.50% <0.00%> (?)` | |
| [python](https://app.codecov.io/gh/apache/superset/pull/37023/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `48.29% <0.00%> (?)` | |
| [unit](https://app.codecov.io/gh/apache/superset/pull/37023/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `100.00% <ΓΈ> (?)` | |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
</details>
[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/apache/superset/pull/37023?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(mcp): resolve startup failures from circular DAO imports [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #37023:
URL: https://github.com/apache/superset/pull/37023#issuecomment-3730966988
**Bito Automatic Review Skipped β PR Already Merged**<br/><br/>Bito scheduled an automatic review for this pull request, but the review was skipped because this PR was merged before the review could be run.<br/>No action is needed if you didn't intend to review it. To get a review, you can type `/review` in a comment and save it
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(mcp): resolve startup failures from circular DAO imports [superset]
Posted by "netlify[bot] (via GitHub)" <gi...@apache.org>.
netlify[bot] commented on PR #37023:
URL: https://github.com/apache/superset/pull/37023#issuecomment-3730787399
### <span aria-hidden="true">β
</span> Deploy Preview for *superset-docs-preview* ready!
| Name | Link |
|:-:|------------------------|
|<span aria-hidden="true">π¨</span> Latest commit | d991f29c90780e4ac837f8bec96c7ce3a65e3245 |
|<span aria-hidden="true">π</span> Latest deploy log | https://app.netlify.com/projects/superset-docs-preview/deploys/69617fa0297b2b0008b1d35c |
|<span aria-hidden="true">π</span> Deploy Preview | [https://deploy-preview-37023--superset-docs-preview.netlify.app](https://deploy-preview-37023--superset-docs-preview.netlify.app) |
|<span aria-hidden="true">π±</span> Preview on mobile | <details><summary> Toggle QR Code... </summary><br /><br /><br /><br />_Use your smartphone camera to open QR code link._</details> |
---
<!-- [superset-docs-preview Preview](https://deploy-preview-37023--superset-docs-preview.netlify.app) -->
_To edit notification comments on pull requests, go to your [Netlify project configuration](https://app.netlify.com/projects/superset-docs-preview/configuration/notifications#deploy-notifications)._
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(mcp): resolve startup failures from circular DAO imports [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on PR #37023:
URL: https://github.com/apache/superset/pull/37023#issuecomment-3730790657
CodeAnt AI finished reviewing your PR.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(mcp): resolve startup failures from circular DAO imports [superset]
Posted by "aminghadersohi (via GitHub)" <gi...@apache.org>.
aminghadersohi commented on PR #37023:
URL: https://github.com/apache/superset/pull/37023#issuecomment-3730933313
> Thanks for fixing this
i broken it i think :D
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org