codeant-ai-for-open-source[bot] commented on code in PR #40344: URL: https://github.com/apache/superset/pull/40344#discussion_r3305699734
########## tests/unit_tests/mcp_service/task/tool/test_task_tools.py: ########## @@ -0,0 +1,249 @@ +# 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. + +"""Unit tests for list_tasks and get_task_info MCP tools.""" + +import uuid +from datetime import datetime, timezone +from unittest.mock import MagicMock, patch + +import pytest +from fastmcp import Client +from pydantic import ValidationError + +from superset.mcp_service.app import mcp +from superset.mcp_service.task.schemas import ListTasksRequest, TaskColumnFilter +from superset.utils import json + +SAMPLE_UUID = str(uuid.uuid4()) + + +def create_mock_task( + task_id: int = 1, + task_uuid: str | None = None, + task_type: str = "sql_execution", + task_key: str = "default-key", + task_name: str | None = None, + status: str = "success", + scope: str = "private", + changed_on: datetime | None = None, + created_on: datetime | None = None, +) -> MagicMock: + task = MagicMock() + task.id = task_id + task.uuid = task_uuid or SAMPLE_UUID + task.task_type = task_type + task.task_key = task_key + task.task_name = task_name + task.status = status + task.scope = scope + task.changed_on = changed_on or datetime(2024, 1, 2, 10, 0, 0, tzinfo=timezone.utc) + task.created_on = created_on or datetime(2024, 1, 1, 9, 0, 0, tzinfo=timezone.utc) + return task + + [email protected] +def mcp_server(): + return mcp + + [email protected](autouse=True) +def mock_auth(): + from unittest.mock import Mock + + with patch("superset.mcp_service.auth.get_user_from_request") as mock_get_user: + mock_user = Mock() + mock_user.id = 1 + mock_user.username = "testuser" + mock_get_user.return_value = mock_user + yield mock_get_user + + +class TestTaskColumnFilterSchema: + def test_valid_filter_columns_accepted(self): + for col in ("task_type", "status", "scope"): + f = TaskColumnFilter(col=col, opr="eq", value="test") + assert f.col == col + + def test_invalid_filter_column_rejected(self): + with pytest.raises(ValidationError): + TaskColumnFilter(col="user_id", opr="eq", value=1) + + def test_uuid_column_rejected(self): + with pytest.raises(ValidationError): + TaskColumnFilter(col="uuid", opr="eq", value="some-uuid") + + +@patch("superset.daos.tasks.TaskDAO.list") [email protected] +async def test_list_tasks_basic(mock_list, mcp_server): + """Basic task listing returns structured response.""" + task = create_mock_task() + mock_list.return_value = ([task], 1) + + async with Client(mcp_server) as client: + result = await client.call_tool("list_tasks", {}) + + data = json.loads(result.content[0].text) + assert data["tasks"] is not None + assert len(data["tasks"]) == 1 + assert data["tasks"][0]["id"] == 1 + assert data["tasks"][0]["task_type"] == "sql_execution" + assert data["tasks"][0]["status"] == "success" + + +@patch("superset.daos.tasks.TaskDAO.list") [email protected] +async def test_list_tasks_with_status_filter(mock_list, mcp_server): + """Status filter is passed through to the DAO correctly.""" + task = create_mock_task(status="pending") + mock_list.return_value = ([task], 1) + + async with Client(mcp_server) as client: + request = ListTasksRequest( + filters=[{"col": "status", "opr": "eq", "value": "pending"}] + ) + result = await client.call_tool("list_tasks", {"request": request.model_dump()}) + + data = json.loads(result.content[0].text) + assert len(data["tasks"]) == 1 + assert data["tasks"][0]["status"] == "pending" Review Comment: **Suggestion:** This test does not verify that the `status` filter is actually forwarded to the DAO; it only checks the mocked response payload. If `list_tasks` silently drops `filters`, this test would still pass. Add an assertion on `mock_list.call_args` to confirm the expected filter operator/column/value was passed. [incomplete implementation] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx - ⚠️ `list_tasks` filter passthrough unverified by unit test. - ⚠️ MCP tool `list_tasks` may regress undetected. ``` </details> <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. Open `tests/unit_tests/mcp_service/task/tool/test_task_tools.py` and locate `test_list_tasks_with_status_filter` at lines 109–125, which patches `superset.daos.tasks.TaskDAO.list` and sets `mock_list.return_value = ([task], 1)` (lines 113–114). 2. Note that the test constructs a `ListTasksRequest` with a `status` filter at lines 116–119 and calls the MCP tool via `Client(mcp_server).call_tool("list_tasks", {"request": request.model_dump()})` at line 120, exercising `superset.mcp_service.task.tool.list_tasks.list_tasks` (lines 53–56 in `list_tasks.py`). 3. Follow the execution in `superset/mcp_service/task/tool/list_tasks.py`: `list_tasks` builds a `ModelListCore` and calls `list_tool.run_tool(filters=request.filters, ...)` at lines 106–115, which in turn passes `filters` into `ModelListCore.run_tool` (lines 229–38 of `mcp_core.py`), where they become `column_operators` passed to `TaskDAO.list` at lines 58–69. 4. Observe that the test assertions at lines 122–124 only verify `len(data["tasks"]) == 1` and `data["tasks"][0]["status"] == "pending"`—they never inspect `mock_list.call_args` to ensure `column_operators` contains the expected `status` filter. Any future regression in `list_tasks` or `ModelListCore` that drops or alters `filters` would still let this test pass as long as `mock_list.return_value` is set, confirming the suggested gap in filter-forwarding coverage. ``` </details> [Fix in Cursor](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=e212ff221c5744108192ceb0a1bbae90&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=e212ff221c5744108192ceb0a1bbae90&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/task/tool/test_task_tools.py **Line:** 120:124 **Comment:** *Incomplete Implementation: This test does not verify that the `status` filter is actually forwarded to the DAO; it only checks the mocked response payload. If `list_tasks` silently drops `filters`, this test would still pass. Add an assertion on `mock_list.call_args` to confirm the expected filter operator/column/value was passed. 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%2F40344&comment_hash=7aed7f43e68ce241ea0d88baa73dc9ad8cb748f66ef1c463210aa43e41760b7d&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40344&comment_hash=7aed7f43e68ce241ea0d88baa73dc9ad8cb748f66ef1c463210aa43e41760b7d&reaction=dislike'>👎</a> ########## tests/unit_tests/mcp_service/task/tool/test_task_tools.py: ########## @@ -0,0 +1,249 @@ +# 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. + +"""Unit tests for list_tasks and get_task_info MCP tools.""" + +import uuid +from datetime import datetime, timezone +from unittest.mock import MagicMock, patch + +import pytest +from fastmcp import Client +from pydantic import ValidationError + +from superset.mcp_service.app import mcp +from superset.mcp_service.task.schemas import ListTasksRequest, TaskColumnFilter +from superset.utils import json + +SAMPLE_UUID = str(uuid.uuid4()) + + +def create_mock_task( + task_id: int = 1, + task_uuid: str | None = None, + task_type: str = "sql_execution", + task_key: str = "default-key", + task_name: str | None = None, + status: str = "success", + scope: str = "private", + changed_on: datetime | None = None, + created_on: datetime | None = None, +) -> MagicMock: + task = MagicMock() + task.id = task_id + task.uuid = task_uuid or SAMPLE_UUID + task.task_type = task_type + task.task_key = task_key + task.task_name = task_name + task.status = status + task.scope = scope + task.changed_on = changed_on or datetime(2024, 1, 2, 10, 0, 0, tzinfo=timezone.utc) + task.created_on = created_on or datetime(2024, 1, 1, 9, 0, 0, tzinfo=timezone.utc) + return task + + [email protected] +def mcp_server(): + return mcp + + [email protected](autouse=True) +def mock_auth(): + from unittest.mock import Mock + + with patch("superset.mcp_service.auth.get_user_from_request") as mock_get_user: + mock_user = Mock() + mock_user.id = 1 + mock_user.username = "testuser" + mock_get_user.return_value = mock_user + yield mock_get_user + + +class TestTaskColumnFilterSchema: + def test_valid_filter_columns_accepted(self): + for col in ("task_type", "status", "scope"): + f = TaskColumnFilter(col=col, opr="eq", value="test") + assert f.col == col + + def test_invalid_filter_column_rejected(self): + with pytest.raises(ValidationError): + TaskColumnFilter(col="user_id", opr="eq", value=1) + + def test_uuid_column_rejected(self): + with pytest.raises(ValidationError): + TaskColumnFilter(col="uuid", opr="eq", value="some-uuid") + + +@patch("superset.daos.tasks.TaskDAO.list") [email protected] +async def test_list_tasks_basic(mock_list, mcp_server): + """Basic task listing returns structured response.""" + task = create_mock_task() + mock_list.return_value = ([task], 1) + + async with Client(mcp_server) as client: + result = await client.call_tool("list_tasks", {}) + + data = json.loads(result.content[0].text) + assert data["tasks"] is not None + assert len(data["tasks"]) == 1 + assert data["tasks"][0]["id"] == 1 + assert data["tasks"][0]["task_type"] == "sql_execution" + assert data["tasks"][0]["status"] == "success" + + +@patch("superset.daos.tasks.TaskDAO.list") [email protected] +async def test_list_tasks_with_status_filter(mock_list, mcp_server): + """Status filter is passed through to the DAO correctly.""" + task = create_mock_task(status="pending") + mock_list.return_value = ([task], 1) + + async with Client(mcp_server) as client: + request = ListTasksRequest( + filters=[{"col": "status", "opr": "eq", "value": "pending"}] + ) + result = await client.call_tool("list_tasks", {"request": request.model_dump()}) + + data = json.loads(result.content[0].text) + assert len(data["tasks"]) == 1 + assert data["tasks"][0]["status"] == "pending" + + +@patch("superset.daos.tasks.TaskDAO.list") [email protected] +async def test_list_tasks_taskfilter_scoping_is_applied(mock_list, mcp_server): + """TaskDAO.list is called with base_filter (TaskFilter) applied via DAO class.""" + mock_list.return_value = ([], 0) + + async with Client(mcp_server) as client: + await client.call_tool("list_tasks", {}) + + # Verify the DAO's list() is called — the TaskFilter scoping is enforced + # by TaskDAO.base_filter = TaskFilter which BaseDAO applies automatically. + assert mock_list.called Review Comment: **Suggestion:** This test claims to validate `TaskFilter` scoping, but patching `TaskDAO.list` replaces the method where base filtering is applied, so no scoping code runs. As written, it only proves the tool invoked the mocked DAO method. Use an integration-style DAO call (or assert the effective query inputs before patching lower-level query APIs) to actually exercise base-filter behavior. [incomplete implementation] <details> <summary><b>Severity Level:</b> Critical 🚨</summary> ```mdx - ❌ Task subscription scoping untested despite security importance. - ⚠️ `TaskFilter` access control regressions may pass CI. ``` </details> <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. Inspect `test_list_tasks_taskfilter_scoping_is_applied` in `tests/unit_tests/mcp_service/task/tool/test_task_tools.py` at lines 127–138; it uses `@patch("superset.daos.tasks.TaskDAO.list")` (line 127) and sets `mock_list.return_value = ([], 0)` (line 131). 2. Notice that the test calls `Client(mcp_server).call_tool("list_tasks", {})` at lines 133–134, which routes through `superset.mcp_service.task.tool.list_tasks.list_tasks` (lines 53–56 of `list_tasks.py`) and then `ModelListCore.run_tool` (lines 229–69 of `mcp_core.py`), where `TaskDAO.list` is invoked with `column_operators=filters`. 3. Examine the access-control implementation: `TaskDAO.base_filter = TaskFilter` at `superset/daos/tasks.py:18`, and `TaskFilter.apply` at `superset/tasks/filters.py:27–63`, which enforces subscription-based visibility by consulting `security_manager.is_admin()` and `TaskSubscriber` relations before filtering the SQLAlchemy query. 4. Because `TaskDAO.list` is fully patched in this test, none of the real `BaseDAO.list` / `TaskFilter.apply` logic ever runs; the test's only assertion at line 138 is `assert mock_list.called`. As a result, any future regression that removes `TaskFilter` from `TaskDAO.base_filter` or bypasses base-filter application in the real DAO would not cause this test to fail, validating the suggestion that the test does not genuinely exercise or guard the scoping behavior it claims to cover. ``` </details> [Fix in Cursor](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=e5f4064ddad64cc8b4477b3234b88143&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=e5f4064ddad64cc8b4477b3234b88143&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/task/tool/test_task_tools.py **Line:** 127:138 **Comment:** *Incomplete Implementation: This test claims to validate `TaskFilter` scoping, but patching `TaskDAO.list` replaces the method where base filtering is applied, so no scoping code runs. As written, it only proves the tool invoked the mocked DAO method. Use an integration-style DAO call (or assert the effective query inputs before patching lower-level query APIs) to actually exercise base-filter behavior. 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%2F40344&comment_hash=fbccaeac24d715cc1ad9f8cd631d3c2840833e31ee1f0b065eaaf0e6fb7f1417&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40344&comment_hash=fbccaeac24d715cc1ad9f8cd631d3c2840833e31ee1f0b065eaaf0e6fb7f1417&reaction=dislike'>👎</a> ########## tests/unit_tests/mcp_service/task/tool/test_task_tools.py: ########## @@ -0,0 +1,249 @@ +# 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. + +"""Unit tests for list_tasks and get_task_info MCP tools.""" + +import uuid +from datetime import datetime, timezone +from unittest.mock import MagicMock, patch + +import pytest +from fastmcp import Client +from pydantic import ValidationError + +from superset.mcp_service.app import mcp +from superset.mcp_service.task.schemas import ListTasksRequest, TaskColumnFilter +from superset.utils import json + +SAMPLE_UUID = str(uuid.uuid4()) + + +def create_mock_task( + task_id: int = 1, + task_uuid: str | None = None, + task_type: str = "sql_execution", + task_key: str = "default-key", + task_name: str | None = None, + status: str = "success", + scope: str = "private", + changed_on: datetime | None = None, + created_on: datetime | None = None, +) -> MagicMock: + task = MagicMock() + task.id = task_id + task.uuid = task_uuid or SAMPLE_UUID + task.task_type = task_type + task.task_key = task_key + task.task_name = task_name + task.status = status + task.scope = scope + task.changed_on = changed_on or datetime(2024, 1, 2, 10, 0, 0, tzinfo=timezone.utc) + task.created_on = created_on or datetime(2024, 1, 1, 9, 0, 0, tzinfo=timezone.utc) + return task + + [email protected] +def mcp_server(): + return mcp + + [email protected](autouse=True) +def mock_auth(): + from unittest.mock import Mock + + with patch("superset.mcp_service.auth.get_user_from_request") as mock_get_user: + mock_user = Mock() + mock_user.id = 1 + mock_user.username = "testuser" + mock_get_user.return_value = mock_user + yield mock_get_user + + +class TestTaskColumnFilterSchema: + def test_valid_filter_columns_accepted(self): + for col in ("task_type", "status", "scope"): + f = TaskColumnFilter(col=col, opr="eq", value="test") + assert f.col == col + + def test_invalid_filter_column_rejected(self): + with pytest.raises(ValidationError): + TaskColumnFilter(col="user_id", opr="eq", value=1) + + def test_uuid_column_rejected(self): + with pytest.raises(ValidationError): + TaskColumnFilter(col="uuid", opr="eq", value="some-uuid") + + +@patch("superset.daos.tasks.TaskDAO.list") [email protected] +async def test_list_tasks_basic(mock_list, mcp_server): + """Basic task listing returns structured response.""" + task = create_mock_task() + mock_list.return_value = ([task], 1) + + async with Client(mcp_server) as client: + result = await client.call_tool("list_tasks", {}) + + data = json.loads(result.content[0].text) + assert data["tasks"] is not None + assert len(data["tasks"]) == 1 + assert data["tasks"][0]["id"] == 1 + assert data["tasks"][0]["task_type"] == "sql_execution" + assert data["tasks"][0]["status"] == "success" + + +@patch("superset.daos.tasks.TaskDAO.list") [email protected] +async def test_list_tasks_with_status_filter(mock_list, mcp_server): + """Status filter is passed through to the DAO correctly.""" + task = create_mock_task(status="pending") + mock_list.return_value = ([task], 1) + + async with Client(mcp_server) as client: + request = ListTasksRequest( + filters=[{"col": "status", "opr": "eq", "value": "pending"}] + ) + result = await client.call_tool("list_tasks", {"request": request.model_dump()}) + + data = json.loads(result.content[0].text) + assert len(data["tasks"]) == 1 + assert data["tasks"][0]["status"] == "pending" + + +@patch("superset.daos.tasks.TaskDAO.list") [email protected] +async def test_list_tasks_taskfilter_scoping_is_applied(mock_list, mcp_server): + """TaskDAO.list is called with base_filter (TaskFilter) applied via DAO class.""" + mock_list.return_value = ([], 0) + + async with Client(mcp_server) as client: + await client.call_tool("list_tasks", {}) + + # Verify the DAO's list() is called — the TaskFilter scoping is enforced + # by TaskDAO.base_filter = TaskFilter which BaseDAO applies automatically. + assert mock_list.called + + +@patch("superset.daos.tasks.TaskDAO.list") [email protected] +async def test_list_tasks_pagination(mock_list, mcp_server): + """Pagination metadata is correct.""" + tasks = [create_mock_task(task_id=i) for i in range(1, 4)] + mock_list.return_value = (tasks, 30) + + async with Client(mcp_server) as client: + request = ListTasksRequest(page=2, page_size=3) + result = await client.call_tool("list_tasks", {"request": request.model_dump()}) + + data = json.loads(result.content[0].text) + assert data["count"] == 3 + assert data["total_count"] == 30 + assert data["page"] == 2 + assert data["page_size"] == 3 + assert data["has_previous"] is True + assert data["has_next"] is True + + +@patch("superset.daos.tasks.TaskDAO.list") [email protected] +async def test_list_tasks_uuid_in_response(mock_list, mcp_server): + """Task UUID is returned as a string in the response.""" + task_uuid = str(uuid.uuid4()) + task = create_mock_task(task_uuid=task_uuid) + mock_list.return_value = ([task], 1) + + async with Client(mcp_server) as client: + result = await client.call_tool("list_tasks", {}) + + data = json.loads(result.content[0].text) + assert data["tasks"][0]["uuid"] == task_uuid + + +@patch("superset.daos.tasks.TaskDAO.find_by_id") [email protected] +async def test_get_task_info_by_integer_id(mock_find, mcp_server): + """get_task_info resolves a task by integer ID.""" + task = create_mock_task(task_id=5, task_type="thumbnail", status="in_progress") + mock_find.return_value = task + + async with Client(mcp_server) as client: + result = await client.call_tool("get_task_info", {"request": {"identifier": 5}}) + + data = json.loads(result.content[0].text) + assert data["id"] == 5 + assert data["task_type"] == "thumbnail" + assert data["status"] == "in_progress" + + +@patch("superset.daos.tasks.TaskDAO.find_by_id") [email protected] +async def test_get_task_info_by_uuid(mock_find, mcp_server): + """get_task_info resolves a task by UUID string.""" + task_uuid = str(uuid.uuid4()) + task = create_mock_task(task_id=10, task_uuid=task_uuid, status="success") + mock_find.return_value = task + + async with Client(mcp_server) as client: + result = await client.call_tool( + "get_task_info", {"request": {"identifier": task_uuid}} + ) + + data = json.loads(result.content[0].text) + assert data["id"] == 10 + assert data["status"] == "success" + + +@patch("superset.daos.tasks.TaskDAO.find_by_id") [email protected] +async def test_get_task_info_not_found(mock_find, mcp_server): + """get_task_info returns error when task does not exist.""" + mock_find.return_value = None + + async with Client(mcp_server) as client: + result = await client.call_tool( + "get_task_info", {"request": {"identifier": 9999}} + ) + + data = json.loads(result.content[0].text) + assert data["error_type"] == "not_found" + + +@patch("superset.daos.tasks.TaskDAO.list") [email protected] +async def test_list_tasks_non_admin_sees_only_subscribed(mock_list, mcp_server): + """Non-admin users only receive tasks their subscriptions allow. + + The subscription scoping is enforced by TaskDAO.base_filter = TaskFilter, + which BaseDAO._apply_base_filter injects before each query. The MCP tool + itself adds no extra filtering — it just delegates to TaskDAO.list(), which + carries the filter class. This test confirms that: + + 1. list_tasks calls TaskDAO.list() (so the base_filter runs) + 2. Only items returned by that call appear in the response + """ + # Simulate DAO returning only the subscribed task + subscribed_task = create_mock_task(task_id=42, status="pending") + mock_list.return_value = ([subscribed_task], 1) + + async with Client(mcp_server) as client: + result = await client.call_tool("list_tasks", {}) + + data = json.loads(result.content[0].text) + assert len(data["tasks"]) == 1 + assert data["tasks"][0]["id"] == 42 + # TaskDAO.list was called exactly once — base_filter is applied inside + assert mock_list.call_count == 1 Review Comment: **Suggestion:** This non-admin visibility test is a false positive: it preloads the mocked DAO with already-filtered data and then only checks that same data is returned. It never verifies that non-admin scoping logic was applied by query/filter behavior, so regressions in access control filtering would go undetected. [incomplete implementation] <details> <summary><b>Severity Level:</b> Critical 🚨</summary> ```mdx - ❌ Non-admin task visibility scoping not truly validated. - ⚠️ Unauthorized task access regressions could escape detection. ``` </details> <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. Review `test_list_tasks_non_admin_sees_only_subscribed` at lines 225–249 in `tests/unit_tests/mcp_service/task/tool/test_task_tools.py`, which patches `superset.daos.tasks.TaskDAO.list` (line 225) and documents non-admin subscription scoping in its docstring at lines 228–237. 2. Observe that the test simulates scoping by constructing a single `subscribed_task = create_mock_task(task_id=42, status="pending")` and setting `mock_list.return_value = ([subscribed_task], 1)` at lines 238–240, then calling `Client(mcp_server).call_tool("list_tasks", {})` at lines 242–243. 3. The actual scoping implementation lives in `TaskFilter.apply` at `superset/tasks/filters.py:27–63` and is wired by `TaskDAO.base_filter = TaskFilter` at `superset/daos/tasks.py:18`, which is invoked inside `BaseDAO.list` (indirectly called from `ModelListCore.run_tool` at `mcp_core.py:58–69`). However, because `TaskDAO.list` is patched, none of this base-filter logic executes in this test. 4. The assertions at lines 245–249 only check `len(data["tasks"]) == 1`, `data["tasks"][0]["id"] == 42`, and `mock_list.call_count == 1`, effectively verifying that the tool echoes whatever the mocked DAO returns, not that non-admin users are actually filtered by `TaskFilter`. Thus, a regression that stops applying subscription-based filters in the real DAO would not break this test, confirming the suggestion that the non-admin visibility test is a false positive in terms of access-control coverage. ``` </details> [Fix in Cursor](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=b05f5a1de4564eec953ac1415cfb4e2d&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=b05f5a1de4564eec953ac1415cfb4e2d&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/task/tool/test_task_tools.py **Line:** 238:249 **Comment:** *Incomplete Implementation: This non-admin visibility test is a false positive: it preloads the mocked DAO with already-filtered data and then only checks that same data is returned. It never verifies that non-admin scoping logic was applied by query/filter behavior, so regressions in access control filtering would go undetected. 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%2F40344&comment_hash=2712c406ff91ff3488c007f1081b27725a3ae2315b39b4c18c42a5f388174a66&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40344&comment_hash=2712c406ff91ff3488c007f1081b27725a3ae2315b39b4c18c42a5f388174a66&reaction=dislike'>👎</a> ########## tests/unit_tests/mcp_service/task/tool/test_task_tools.py: ########## @@ -0,0 +1,249 @@ +# 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. + +"""Unit tests for list_tasks and get_task_info MCP tools.""" + +import uuid +from datetime import datetime, timezone +from unittest.mock import MagicMock, patch + +import pytest +from fastmcp import Client +from pydantic import ValidationError + +from superset.mcp_service.app import mcp +from superset.mcp_service.task.schemas import ListTasksRequest, TaskColumnFilter +from superset.utils import json + +SAMPLE_UUID = str(uuid.uuid4()) + + +def create_mock_task( + task_id: int = 1, + task_uuid: str | None = None, + task_type: str = "sql_execution", + task_key: str = "default-key", + task_name: str | None = None, + status: str = "success", + scope: str = "private", + changed_on: datetime | None = None, + created_on: datetime | None = None, +) -> MagicMock: + task = MagicMock() + task.id = task_id + task.uuid = task_uuid or SAMPLE_UUID + task.task_type = task_type + task.task_key = task_key + task.task_name = task_name + task.status = status + task.scope = scope + task.changed_on = changed_on or datetime(2024, 1, 2, 10, 0, 0, tzinfo=timezone.utc) + task.created_on = created_on or datetime(2024, 1, 1, 9, 0, 0, tzinfo=timezone.utc) + return task + + [email protected] +def mcp_server(): + return mcp + + [email protected](autouse=True) +def mock_auth(): + from unittest.mock import Mock + + with patch("superset.mcp_service.auth.get_user_from_request") as mock_get_user: + mock_user = Mock() + mock_user.id = 1 + mock_user.username = "testuser" + mock_get_user.return_value = mock_user + yield mock_get_user + + +class TestTaskColumnFilterSchema: + def test_valid_filter_columns_accepted(self): + for col in ("task_type", "status", "scope"): + f = TaskColumnFilter(col=col, opr="eq", value="test") + assert f.col == col + + def test_invalid_filter_column_rejected(self): + with pytest.raises(ValidationError): + TaskColumnFilter(col="user_id", opr="eq", value=1) + + def test_uuid_column_rejected(self): + with pytest.raises(ValidationError): + TaskColumnFilter(col="uuid", opr="eq", value="some-uuid") + + +@patch("superset.daos.tasks.TaskDAO.list") [email protected] +async def test_list_tasks_basic(mock_list, mcp_server): + """Basic task listing returns structured response.""" + task = create_mock_task() + mock_list.return_value = ([task], 1) + + async with Client(mcp_server) as client: + result = await client.call_tool("list_tasks", {}) + + data = json.loads(result.content[0].text) + assert data["tasks"] is not None + assert len(data["tasks"]) == 1 + assert data["tasks"][0]["id"] == 1 + assert data["tasks"][0]["task_type"] == "sql_execution" + assert data["tasks"][0]["status"] == "success" + + +@patch("superset.daos.tasks.TaskDAO.list") [email protected] +async def test_list_tasks_with_status_filter(mock_list, mcp_server): + """Status filter is passed through to the DAO correctly.""" + task = create_mock_task(status="pending") + mock_list.return_value = ([task], 1) + + async with Client(mcp_server) as client: + request = ListTasksRequest( + filters=[{"col": "status", "opr": "eq", "value": "pending"}] + ) + result = await client.call_tool("list_tasks", {"request": request.model_dump()}) + + data = json.loads(result.content[0].text) + assert len(data["tasks"]) == 1 + assert data["tasks"][0]["status"] == "pending" + + +@patch("superset.daos.tasks.TaskDAO.list") [email protected] +async def test_list_tasks_taskfilter_scoping_is_applied(mock_list, mcp_server): + """TaskDAO.list is called with base_filter (TaskFilter) applied via DAO class.""" + mock_list.return_value = ([], 0) + + async with Client(mcp_server) as client: + await client.call_tool("list_tasks", {}) + + # Verify the DAO's list() is called — the TaskFilter scoping is enforced + # by TaskDAO.base_filter = TaskFilter which BaseDAO applies automatically. + assert mock_list.called + + +@patch("superset.daos.tasks.TaskDAO.list") [email protected] +async def test_list_tasks_pagination(mock_list, mcp_server): + """Pagination metadata is correct.""" + tasks = [create_mock_task(task_id=i) for i in range(1, 4)] + mock_list.return_value = (tasks, 30) + + async with Client(mcp_server) as client: + request = ListTasksRequest(page=2, page_size=3) + result = await client.call_tool("list_tasks", {"request": request.model_dump()}) + + data = json.loads(result.content[0].text) + assert data["count"] == 3 + assert data["total_count"] == 30 + assert data["page"] == 2 + assert data["page_size"] == 3 + assert data["has_previous"] is True + assert data["has_next"] is True + + +@patch("superset.daos.tasks.TaskDAO.list") [email protected] +async def test_list_tasks_uuid_in_response(mock_list, mcp_server): + """Task UUID is returned as a string in the response.""" + task_uuid = str(uuid.uuid4()) + task = create_mock_task(task_uuid=task_uuid) + mock_list.return_value = ([task], 1) + + async with Client(mcp_server) as client: + result = await client.call_tool("list_tasks", {}) + + data = json.loads(result.content[0].text) + assert data["tasks"][0]["uuid"] == task_uuid + + +@patch("superset.daos.tasks.TaskDAO.find_by_id") [email protected] +async def test_get_task_info_by_integer_id(mock_find, mcp_server): + """get_task_info resolves a task by integer ID.""" + task = create_mock_task(task_id=5, task_type="thumbnail", status="in_progress") + mock_find.return_value = task + + async with Client(mcp_server) as client: + result = await client.call_tool("get_task_info", {"request": {"identifier": 5}}) + + data = json.loads(result.content[0].text) + assert data["id"] == 5 + assert data["task_type"] == "thumbnail" + assert data["status"] == "in_progress" + + +@patch("superset.daos.tasks.TaskDAO.find_by_id") [email protected] +async def test_get_task_info_by_uuid(mock_find, mcp_server): + """get_task_info resolves a task by UUID string.""" + task_uuid = str(uuid.uuid4()) + task = create_mock_task(task_id=10, task_uuid=task_uuid, status="success") + mock_find.return_value = task + + async with Client(mcp_server) as client: + result = await client.call_tool( + "get_task_info", {"request": {"identifier": task_uuid}} + ) + + data = json.loads(result.content[0].text) + assert data["id"] == 10 + assert data["status"] == "success" Review Comment: **Suggestion:** The UUID lookup test does not assert that lookup happened via UUID semantics (`id_column="uuid"` path). It only checks response fields from a mocked return object, so the test would still pass if UUID resolution regressed to the wrong DAO call path. Assert `mock_find` was called with the UUID identifier and UUID column selection arguments. [incomplete implementation] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx - ⚠️ UUID-based task lookups may regress unnoticed. - ⚠️ MCP tool `get_task_info` UUID path under-specified by tests. ``` </details> <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. Locate `test_get_task_info_by_uuid` in `tests/unit_tests/mcp_service/task/tool/test_task_tools.py` at lines 192–207; it patches `superset.daos.tasks.TaskDAO.find_by_id` (line 192), creates a `task` with a specific UUID at lines 196–197, and sets `mock_find.return_value = task` at line 198. 2. The test invokes the MCP tool via `Client(mcp_server).call_tool("get_task_info", {"request": {"identifier": task_uuid}})` at lines 200–203, which exercises `superset.mcp_service.task.tool.get_task_info.get_task_info` (lines 47–50 of `get_task_info.py`) and its `ModelGetInfoCore` wrapper. 3. Inspect `ModelGetInfoCore._find_object` in `superset/mcp_service/mcp_core.py` at lines 37–55: for a UUID string, it calls `self.dao_class.find_by_id(identifier, id_column="uuid", query_options=opts)` when `_is_uuid(identifier)` returns True, relying on the `id_column="uuid"` argument to distinguish UUID lookups from integer ID or slug lookups. 4. The test's assertions at lines 205–207 only check the deserialized response fields (`id` and `status`) and never assert how `mock_find` was called (e.g., with `id_column="uuid"`). As long as `get_task_info` returns `mock_find.return_value`, the test will pass even if `_find_object` regresses to call `TaskDAO.find_by_id` without `id_column="uuid"` or uses a different lookup path that still hits the patched method, demonstrating that the current test does not verify that the UUID-specific DAO lookup semantics are actually used. ``` </details> [Fix in Cursor](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=dbd68e74c8064e6a9ff21a4bc17ae1d4&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=dbd68e74c8064e6a9ff21a4bc17ae1d4&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/task/tool/test_task_tools.py **Line:** 192:207 **Comment:** *Incomplete Implementation: The UUID lookup test does not assert that lookup happened via UUID semantics (`id_column="uuid"` path). It only checks response fields from a mocked return object, so the test would still pass if UUID resolution regressed to the wrong DAO call path. Assert `mock_find` was called with the UUID identifier and UUID column selection arguments. 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%2F40344&comment_hash=02e3f8ef6b691c41bebd83a903653095139f13f7cb9620e4013f6f4eea746c0c&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40344&comment_hash=02e3f8ef6b691c41bebd83a903653095139f13f7cb9620e4013f6f4eea746c0c&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]
