codeant-ai-for-open-source[bot] commented on code in PR #41924:
URL: https://github.com/apache/superset/pull/41924#discussion_r3573728503
##########
tests/unit_tests/mcp_service/semantic_layer/tool/test_list_metrics.py:
##########
@@ -169,22 +208,12 @@ async def test_list_metrics_privacy_check(mcp_server:
FastMCP) -> None:
assert data["error_type"] == "DataModelMetadataRestricted"
[email protected]
[email protected]()
async def test_list_metrics_search_filter(mcp_server: FastMCP) -> None:
"""list_metrics filters metrics by search term."""
mock_ds: MagicMock = _make_dataset(1)
Review Comment:
**Suggestion:** Type-annotate this newly added local mock variable
(consistent with nearby tests that already use typed locals). [custom_rule]
**Severity Level:** Minor ๐งน
<details>
<summary><b>Why it matters? โญ </b></summary>
This is another newly introduced local variable in the modified test file
and it is not annotated, so the typing rule applies here as well.
</details>
<details>
<summary><b>Rule source ๐ </b></summary>
.cursor/rules/dev-standard.mdc (line 28)
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=55b30a7484f94396875b492635103759&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=55b30a7484f94396875b492635103759&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/semantic_layer/tool/test_list_metrics.py
**Line:** 214:214
**Comment:**
*Custom Rule: Type-annotate this newly added local mock variable
(consistent with nearby tests that already use typed locals).
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=ed29c6afe4f9fca50fff5e4505f939a9f173a90c9642819e4e5882f94f1cd6fa&reaction=like'>๐</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41924&comment_hash=ed29c6afe4f9fca50fff5e4505f939a9f173a90c9642819e4e5882f94f1cd6fa&reaction=dislike'>๐</a>
##########
tests/unit_tests/mcp_service/semantic_layer/tool/test_list_metrics.py:
##########
@@ -112,18 +115,54 @@ def _access_denied_exc(message: str = "Access denied") ->
SupersetSecurityExcept
)
[email protected]
-async def test_list_metrics_builtin_happy_path(mcp_server: FastMCP) -> None:
- """list_metrics returns builtin metrics when only datasets exist."""
- mock_ds = _make_dataset(42)
[email protected]
+def _patched_dataset_lookup(
+ dataset: MagicMock | None,
+) -> Generator[tuple[MagicMock, MagicMock], None, None]:
+ """Patch the ``dataset_id``-driven lookup path.
+
+ Covers ``DatasetDAO.find_by_id`` (direct lookup by id) plus the
+ ``SemanticViewDAO.find_accessible`` call every ``list_metrics`` request
+ makes regardless of scope.
+ """
+ with (
+ patch.object(list_metrics_module, "DatasetDAO") as mock_dao,
+ patch.object(list_metrics_module, "SemanticViewDAO") as mock_view_dao,
+ ):
+ mock_dao.find_by_id.return_value = dataset
+ mock_view_dao.find_accessible.return_value = []
+ yield mock_dao, mock_view_dao
+
[email protected]
+def _patched_dataset_search(
+ datasets: list[MagicMock],
+) -> Generator[tuple[MagicMock, MagicMock, MagicMock], None, None]:
+ """Patch the search-driven lookup path.
+
+ Covers the unscoped/searched query path: ``DatasetDAO._apply_base_filter``
+ applied to a ``db.session.query(...).options(...)`` chain, as used when no
+ ``dataset_id``/``view_id`` narrows the request.
+ """
with (
patch.object(list_metrics_module, "DatasetDAO") as mock_dao,
patch.object(list_metrics_module, "SemanticViewDAO") as mock_view_dao,
+ patch.object(list_metrics_module, "db") as mock_db,
):
- mock_dao.find_by_id.return_value = mock_ds
mock_view_dao.find_accessible.return_value = []
+ mock_query: MagicMock = MagicMock()
+ mock_db.session.query.return_value.options.return_value = mock_query
+ mock_dao._apply_base_filter.return_value = mock_query
+ mock_query.all.return_value = datasets
+ yield mock_dao, mock_view_dao, mock_db
+
[email protected]()
+async def test_list_metrics_builtin_happy_path(mcp_server: FastMCP) -> None:
+ """list_metrics returns builtin metrics when only datasets exist."""
+ mock_ds = _make_dataset(42)
Review Comment:
**Suggestion:** Add an explicit type annotation to this newly introduced
local variable to satisfy the typing requirement for relevant variables.
[custom_rule]
**Severity Level:** Minor ๐งน
<details>
<summary><b>Why it matters? โญ </b></summary>
This is a newly introduced Python local variable that can be annotated, but
it is assigned without any type hint. That matches the stated typing rule
violation.
</details>
<details>
<summary><b>Rule source ๐ </b></summary>
.cursor/rules/dev-standard.mdc (line 28)
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=c0008dddca494bcc967166bc9baea7f7&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=c0008dddca494bcc967166bc9baea7f7&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/semantic_layer/tool/test_list_metrics.py
**Line:** 163:163
**Comment:**
*Custom Rule: Add an explicit type annotation to this newly introduced
local variable to satisfy the typing requirement for relevant variables.
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=5ee8725d630f74242d16f35403320b1093ffc44c85f0e378a8c0429271b35b2c&reaction=like'>๐</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41924&comment_hash=5ee8725d630f74242d16f35403320b1093ffc44c85f0e378a8c0429271b35b2c&reaction=dislike'>๐</a>
##########
tests/unit_tests/mcp_service/sql_lab/tool/test_open_sql_lab_with_context.py:
##########
@@ -337,3 +337,108 @@ def
test_sanitizes_error_and_keeps_empty_url_for_missing_database(self) -> None:
)
finally:
_restore_modules(saved_modules)
+
+ def test_returns_error_for_invalid_nonexistent_database_id(self) -> None:
+ """DatabaseDAO.find_by_id returning None for an unknown ID must produce
+ a structured not-found error rather than a raw crash."""
+ mod, saved_modules = _get_tool_module()
+ try:
+ request = OpenSqlLabRequest(database_id=999999999, sql="SELECT 1")
+
+ with (
+ patch(
+ "superset.daos.database.DatabaseDAO.find_by_id",
+ return_value=None,
+ ) as mock_find_by_id,
+ patch.object(
+ mod.event_logger, "log_context", return_value=nullcontext()
+ ),
+ ):
+ response = mod.open_sql_lab_with_context(request,
_make_mock_ctx())
+
+ mock_find_by_id.assert_called_once_with(999999999)
+ assert response.url == ""
+ assert response.database_id == 999999999
+ assert response.error == sanitize_for_llm_context(
+ "Database with ID 999999999 not found."
+ " Use list_databases to get valid database IDs.",
+ field_path=("error",),
+ )
+ finally:
+ _restore_modules(saved_modules)
+
+ def test_returns_generic_not_found_error_when_database_access_denied(
+ self,
+ ) -> None:
+ """The tool has no dedicated permission-denied branch: it relies solely
+ on ``DatabaseDAO.find_by_id``, whose base filter (``DatabaseFilter`` in
+ ``superset/databases/filters.py``) scopes query results to databases
the
+ requesting user can access. A database that exists but that the current
+ user lacks access to is filtered out of the query and ``find_by_id``
+ returns ``None`` -- indistinguishable, from this tool's perspective,
+ from a genuinely nonexistent ID. This test locks in that fail-closed,
+ non-leaking behavior: no distinct "access denied" message is emitted
+ that would reveal the database's existence to an unauthorized caller.
+ """
+ mod, saved_modules = _get_tool_module()
+ try:
+ request = OpenSqlLabRequest(
+ database_id=42,
+ schema="restricted_schema",
+ title="Query I cannot access",
+ )
Review Comment:
**Suggestion:** Add an explicit type annotation for the newly added local
`request` variable in this test method. [custom_rule]
**Severity Level:** Minor ๐งน
<details>
<summary><b>Why it matters? โญ </b></summary>
This added local variable is an obvious candidate for annotation and the
snippet shows no type hint. The suggestion accurately identifies a real
omission under the Python type-hint rule.
</details>
<details>
<summary><b>Rule source ๐ </b></summary>
.cursor/rules/dev-standard.mdc (line 28)
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=7a8572672dda4561930ac6530eb43bce&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=7a8572672dda4561930ac6530eb43bce&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/sql_lab/tool/test_open_sql_lab_with_context.py
**Line:** 385:389
**Comment:**
*Custom Rule: Add an explicit type annotation for the newly added local
`request` variable in this test method.
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=85dc07ebcbb043ca79c32fcd85f54b419be1d4558355a953004f8b6c0d85304b&reaction=like'>๐</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41924&comment_hash=85dc07ebcbb043ca79c32fcd85f54b419be1d4558355a953004f8b6c0d85304b&reaction=dislike'>๐</a>
##########
tests/unit_tests/mcp_service/sql_lab/tool/test_open_sql_lab_with_context.py:
##########
@@ -337,3 +337,108 @@ def
test_sanitizes_error_and_keeps_empty_url_for_missing_database(self) -> None:
)
finally:
_restore_modules(saved_modules)
+
+ def test_returns_error_for_invalid_nonexistent_database_id(self) -> None:
+ """DatabaseDAO.find_by_id returning None for an unknown ID must produce
+ a structured not-found error rather than a raw crash."""
+ mod, saved_modules = _get_tool_module()
+ try:
+ request = OpenSqlLabRequest(database_id=999999999, sql="SELECT 1")
Review Comment:
**Suggestion:** Add an explicit type annotation for the new local `request`
variable to satisfy the type-hint requirement for relevant variables.
[custom_rule]
**Severity Level:** Minor ๐งน
<details>
<summary><b>Why it matters? โญ </b></summary>
This is a newly added Python local variable in modified code, and it is
assignable a clear type from construction but has no annotation. That matches
the type-hint rule for relevant variables.
</details>
<details>
<summary><b>Rule source ๐ </b></summary>
.cursor/rules/dev-standard.mdc (line 28)
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=df0f3616655a43e3ad2b927b6fc99a8a&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=df0f3616655a43e3ad2b927b6fc99a8a&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/sql_lab/tool/test_open_sql_lab_with_context.py
**Line:** 346:346
**Comment:**
*Custom Rule: Add an explicit type annotation for the new local
`request` variable to satisfy the type-hint requirement for relevant variables.
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=3a58ddf4e60ba4e75e758cd2a34c00531d963fee66cf5291d4476978c7076c6b&reaction=like'>๐</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41924&comment_hash=3a58ddf4e60ba4e75e758cd2a34c00531d963fee66cf5291d4476978c7076c6b&reaction=dislike'>๐</a>
##########
tests/unit_tests/mcp_service/sql_lab/tool/test_open_sql_lab_with_context.py:
##########
@@ -337,3 +337,108 @@ def
test_sanitizes_error_and_keeps_empty_url_for_missing_database(self) -> None:
)
finally:
_restore_modules(saved_modules)
+
+ def test_returns_error_for_invalid_nonexistent_database_id(self) -> None:
+ """DatabaseDAO.find_by_id returning None for an unknown ID must produce
+ a structured not-found error rather than a raw crash."""
+ mod, saved_modules = _get_tool_module()
+ try:
+ request = OpenSqlLabRequest(database_id=999999999, sql="SELECT 1")
+
+ with (
+ patch(
+ "superset.daos.database.DatabaseDAO.find_by_id",
+ return_value=None,
+ ) as mock_find_by_id,
+ patch.object(
+ mod.event_logger, "log_context", return_value=nullcontext()
+ ),
+ ):
+ response = mod.open_sql_lab_with_context(request,
_make_mock_ctx())
+
+ mock_find_by_id.assert_called_once_with(999999999)
+ assert response.url == ""
+ assert response.database_id == 999999999
+ assert response.error == sanitize_for_llm_context(
+ "Database with ID 999999999 not found."
+ " Use list_databases to get valid database IDs.",
+ field_path=("error",),
+ )
+ finally:
+ _restore_modules(saved_modules)
+
+ def test_returns_generic_not_found_error_when_database_access_denied(
+ self,
+ ) -> None:
+ """The tool has no dedicated permission-denied branch: it relies solely
+ on ``DatabaseDAO.find_by_id``, whose base filter (``DatabaseFilter`` in
+ ``superset/databases/filters.py``) scopes query results to databases
the
+ requesting user can access. A database that exists but that the current
+ user lacks access to is filtered out of the query and ``find_by_id``
+ returns ``None`` -- indistinguishable, from this tool's perspective,
+ from a genuinely nonexistent ID. This test locks in that fail-closed,
+ non-leaking behavior: no distinct "access denied" message is emitted
+ that would reveal the database's existence to an unauthorized caller.
+ """
+ mod, saved_modules = _get_tool_module()
+ try:
+ request = OpenSqlLabRequest(
+ database_id=42,
+ schema="restricted_schema",
+ title="Query I cannot access",
+ )
+
+ with (
+ patch(
+ "superset.daos.database.DatabaseDAO.find_by_id",
+ return_value=None,
+ ) as mock_find_by_id,
+ patch.object(
+ mod.event_logger, "log_context", return_value=nullcontext()
+ ),
+ ):
+ response = mod.open_sql_lab_with_context(request,
_make_mock_ctx())
+
+ mock_find_by_id.assert_called_once_with(42)
+ assert response.url == ""
+ assert response.database_id == 42
+ assert response.schema_name == "restricted_schema"
+ assert response.error == sanitize_for_llm_context(
+ "Database with ID 42 not found."
+ " Use list_databases to get valid database IDs.",
+ field_path=("error",),
+ )
+ finally:
+ _restore_modules(saved_modules)
+
+ def test_returns_error_and_rolls_back_session_on_unexpected_exception(
+ self,
+ ) -> None:
+ """Any unexpected error during DB validation (e.g. a broken connection)
+ must be caught by the outermost handler, roll back the session, and
+ surface a structured error instead of propagating a raw exception."""
+ mod, saved_modules = _get_tool_module()
+ try:
+ request = OpenSqlLabRequest(database_id=7, sql="SELECT 1")
Review Comment:
**Suggestion:** Add an explicit type annotation to the new `request` local
variable in this test to comply with required typing coverage. [custom_rule]
**Severity Level:** Minor ๐งน
<details>
<summary><b>Why it matters? โญ </b></summary>
This is another newly introduced local variable in the modified test code
and it lacks an explicit type annotation even though its type is clear from the
constructor call.
</details>
<details>
<summary><b>Rule source ๐ </b></summary>
.cursor/rules/dev-standard.mdc (line 28)
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=4ff245a0ba13460e9246b036f3f74fae&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=4ff245a0ba13460e9246b036f3f74fae&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/sql_lab/tool/test_open_sql_lab_with_context.py
**Line:** 422:422
**Comment:**
*Custom Rule: Add an explicit type annotation to the new `request`
local variable in this test to comply with required typing coverage.
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=d164da31221313859f7816ccc4528c77540bc4fb7e689956a7c2c79beb9b7102&reaction=like'>๐</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41924&comment_hash=d164da31221313859f7816ccc4528c77540bc4fb7e689956a7c2c79beb9b7102&reaction=dislike'>๐</a>
##########
tests/unit_tests/mcp_service/system/tool/test_health_check.py:
##########
@@ -17,7 +17,40 @@
"""Tests for health_check MCP tool."""
+import importlib
+from collections.abc import Iterator
+from unittest.mock import MagicMock, Mock, patch
+
+import pytest
+from fastmcp import Client, FastMCP
+from flask import Flask
+
+from superset.mcp_service.app import mcp
from superset.mcp_service.system.schemas import HealthCheckResponse
+from superset.utils import json
+
+# Import the submodule directly so ``patch.object`` targets the module (not the
+# ``health_check`` function that ``tool/__init__.py`` re-exports onto the
+# package).
+health_check_module = importlib.import_module(
+ "superset.mcp_service.system.tool.health_check"
+)
Review Comment:
**Suggestion:** Add an explicit type annotation to the module-level imported
module variable to satisfy the type-hint requirement for newly introduced
relevant variables. [custom_rule]
**Severity Level:** Minor ๐งน
<details>
<summary><b>Why it matters? โญ </b></summary>
This is a newly introduced module-level variable and it has no type
annotation. The custom rule requires type hints on relevant newly added
variables that can be annotated, so the suggestion identifies a real violation.
</details>
<details>
<summary><b>Rule source ๐ </b></summary>
.cursor/rules/dev-standard.mdc (line 28)
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=744c89881b3c4f3f90f7c5d112473dcf&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=744c89881b3c4f3f90f7c5d112473dcf&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent ๐ค </b></summary>
```mdx
This is a comment left during a code review.
**Path:** tests/unit_tests/mcp_service/system/tool/test_health_check.py
**Line:** 35:37
**Comment:**
*Custom Rule: Add an explicit type annotation to the module-level
imported module variable to satisfy the type-hint requirement for newly
introduced relevant variables.
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=0afb45a37f7b70537b330d29d278c917ac9c848a98dddc6aad2c2ae3c984921d&reaction=like'>๐</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41924&comment_hash=0afb45a37f7b70537b330d29d278c917ac9c848a98dddc6aad2c2ae3c984921d&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]
