codeant-ai-for-open-source[bot] commented on code in PR #41758:
URL: https://github.com/apache/superset/pull/41758#discussion_r3522734863
##########
tests/unit_tests/mcp_service/dashboard/tool/test_duplicate_dashboard.py:
##########
@@ -237,7 +237,7 @@ async def test_duplicate_with_duplicate_slices(
assert content["error"] is None
assert content["duplicated_slices"] is True
assert content["dashboard"]["id"] == 3
- assert "/superset/dashboard/3/" in content["dashboard_url"]
+ assert "/dashboard/3/" in content["dashboard_url"]
Review Comment:
**Suggestion:** This check has the same false-positive issue: a legacy
`"/superset/dashboard/3/"` URL still contains `"/dashboard/3/"`, so the test
can pass even when behavior is wrong after the redirect shim is removed. Make
the assertion strict on the full URL shape. [incorrect condition logic]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
⚠️ duplicate_dashboard duplicate_slices test may miss URL bugs.
⚠️ Canonical dashboard URL contract weakly enforced in tests.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. In `superset/mcp_service/dashboard/tool/duplicate_dashboard.py:115-149`,
confirm
`_serialize_new_dashboard()` constructs `dashboard_url` using
`f"{get_superset_base_url()}/dashboard/{dashboard.id}/"` and returns it as
part of the
`DuplicateDashboardResponse`.
2. In
`tests/unit_tests/mcp_service/dashboard/tool/test_duplicate_dashboard.py:60-91`,
`test_duplicate_with_duplicate_slices` calls the same tool and asserts
`"/dashboard/3/" in
content["dashboard_url"]` to validate the response URL.
3. Recognize that a legacy-style URL such as
`f"{get_superset_base_url()}/superset/dashboard/3/"` would still include the
substring
`"/dashboard/3/"`, so the assertion at line 240 would pass even if the tool
regressed to
emitting `/superset/dashboard/<id>/` instead of the canonical
`/dashboard/<id>/`.
4. Change the implementation at `duplicate_dashboard.py:119` to build
`dashboard_url =
f"{get_superset_base_url()}/superset/dashboard/{dashboard.id}/"`, run `pytest
tests/unit_tests/mcp_service/dashboard/tool/test_duplicate_dashboard.py`,
and observe
`test_duplicate_with_duplicate_slices` still succeeds, showing that the
current substring
assertion cannot detect a reversion to deprecated URL prefixes.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=56b43fb4948044ebbe550cefb2263cb0&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=56b43fb4948044ebbe550cefb2263cb0&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/dashboard/tool/test_duplicate_dashboard.py
**Line:** 240:240
**Comment:**
*Incorrect Condition Logic: This check has the same false-positive
issue: a legacy `"/superset/dashboard/3/"` URL still contains
`"/dashboard/3/"`, so the test can pass even when behavior is wrong after the
redirect shim is removed. Make the assertion strict on the full URL shape.
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%2F41758&comment_hash=c6255e96ff69a82006dd26cb112174ec524d231838f3fc473e1dfbf8da0fe7cc&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41758&comment_hash=c6255e96ff69a82006dd26cb112174ec524d231838f3fc473e1dfbf8da0fe7cc&reaction=dislike'>👎</a>
##########
tests/unit_tests/mcp_service/dashboard/tool/test_duplicate_dashboard.py:
##########
@@ -187,7 +187,7 @@ async def test_duplicate_referencing_same_charts(
# Response text is wrapped in LLM-context delimiters (prompt-injection
# defense), matching the standard dashboard serializers.
assert content["dashboard"]["dashboard_title"] == _wrapped("Staging Copy")
- assert "/superset/dashboard/2/" in content["dashboard_url"]
+ assert "/dashboard/2/" in content["dashboard_url"]
Review Comment:
**Suggestion:** This assertion is too weak: `"/dashboard/2/" in ...` will
still pass for legacy URLs like `"/superset/dashboard/2/"`, so the test won’t
catch a regression back to the deprecated prefix. Assert the full expected path
(or assert that `"/superset/"` is absent) to enforce the canonical URL
contract. [incorrect condition logic]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
⚠️ duplicate_dashboard happy-path test may miss URL regressions.
⚠️ MCP dashboard duplication URLs could break undetected.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Observe in
`superset/mcp_service/dashboard/tool/duplicate_dashboard.py:115-149` that
`_serialize_new_dashboard()` builds `dashboard_url =
f"{get_superset_base_url()}/dashboard/{dashboard.id}/"` and returns it in
the tool
response.
2. In
`tests/unit_tests/mcp_service/dashboard/tool/test_duplicate_dashboard.py:28-41`,
`test_duplicate_referencing_same_charts` calls the `duplicate_dashboard` MCP
tool via
`Client.call_tool("duplicate_dashboard", ...)` and asserts `"/dashboard/2/"
in
content["dashboard_url"]`.
3. Note that a legacy URL like
`f"{get_superset_base_url()}/superset/dashboard/2/"` (for
example if someone reintroduces the old prefix in
`_serialize_new_dashboard()` at line
119) still contains the substring `"/dashboard/2/"`, so this assertion would
pass even
though the URL path is non-canonical.
4. Modify `_serialize_new_dashboard()` at `duplicate_dashboard.py:119` to
return
`f"{get_superset_base_url()}/superset/dashboard/{dashboard.id}/"`, run
`pytest
tests/unit_tests/mcp_service/dashboard/tool/test_duplicate_dashboard.py`,
and observe
`test_duplicate_referencing_same_charts` still passes, demonstrating the
test cannot
detect regressions back to the deprecated `/superset/dashboard/<id>/` path.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=e466f5165dd34248857d5ac6b35d8f07&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=e466f5165dd34248857d5ac6b35d8f07&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/dashboard/tool/test_duplicate_dashboard.py
**Line:** 190:190
**Comment:**
*Incorrect Condition Logic: This assertion is too weak:
`"/dashboard/2/" in ...` will still pass for legacy URLs like
`"/superset/dashboard/2/"`, so the test won’t catch a regression back to the
deprecated prefix. Assert the full expected path (or assert that `"/superset/"`
is absent) to enforce the canonical URL contract.
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%2F41758&comment_hash=91307421a0bb2585325e802469c66abe1786416924cac8380c9ef5999fb20195&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41758&comment_hash=91307421a0bb2585325e802469c66abe1786416924cac8380c9ef5999fb20195&reaction=dislike'>👎</a>
##########
tests/unit_tests/mcp_service/dashboard/tool/test_remove_chart_from_dashboard.py:
##########
@@ -371,7 +371,7 @@ async def test_simple_grid_removal_prunes_empty_row(
assert content["error"] is None
assert content["permission_denied"] is False
assert content["dashboard_url"] is not None
- assert "/superset/dashboard/1/" in content["dashboard_url"]
+ assert "/dashboard/1/" in content["dashboard_url"]
Review Comment:
**Suggestion:** This URL assertion is similarly permissive and will pass for
legacy prefixed paths, so it does not actually verify that the tool emits the
new canonical route. Assert the exact expected URL pattern to prevent false
positives. [incorrect condition logic]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
⚠️ remove_chart_from_dashboard URL regressions may escape detection.
⚠️ MCP dashboard URL contract not strictly enforced by tests.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Review `remove_chart_from_dashboard()` in
`superset/mcp_service/dashboard/tool/remove_chart_from_dashboard.py:52-257`;
both the
SQLAlchemyError fallback at lines 161-175 and the main success path at lines
227-257 build
`dashboard_url =
f"{get_superset_base_url()}/dashboard/{updated_dashboard.id}/"` and
return it in `RemoveChartFromDashboardResponse`.
2. In
`tests/unit_tests/mcp_service/dashboard/tool/test_remove_chart_from_dashboard.py:29-56`,
`test_simple_grid_removal_prunes_empty_row` exercises the happy-path removal
and asserts
`assert "/dashboard/1/" in content["dashboard_url"]` after calling the
`_call_remove`
helper.
3. A legacy URL such as `f"{get_superset_base_url()}/superset/dashboard/1/"`
(for example
if someone reintroduces the `/superset` prefix into `dashboard_url`
construction at line
257) still contains the substring `"/dashboard/1/"`, so the assertion at
line 374 would
pass even though the MCP tool is emitting non-canonical, shim-dependent
routes.
4. Change `dashboard_url` builders in `remove_chart_from_dashboard.py` at
lines 162, 237,
and 257 to include `/superset/dashboard/` instead of `/dashboard/`, run
`pytest
tests/unit_tests/mcp_service/dashboard/tool/test_remove_chart_from_dashboard.py`,
and
observe `test_simple_grid_removal_prunes_empty_row` still passing,
confirming that the
substring assertion cannot detect regressions to the deprecated prefixed
paths.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=3421604565a647f993fcb6a74e67a071&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=3421604565a647f993fcb6a74e67a071&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/dashboard/tool/test_remove_chart_from_dashboard.py
**Line:** 374:374
**Comment:**
*Incorrect Condition Logic: This URL assertion is similarly permissive
and will pass for legacy prefixed paths, so it does not actually verify that
the tool emits the new canonical route. Assert the exact expected URL pattern
to prevent false positives.
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%2F41758&comment_hash=20cf01557024d18c83e39abb989a1bff53e3a9fc36ce8ce7b25d832c4e956e60&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41758&comment_hash=20cf01557024d18c83e39abb989a1bff53e3a9fc36ce8ce7b25d832c4e956e60&reaction=dislike'>👎</a>
##########
tests/unit_tests/mcp_service/dashboard/tool/test_duplicate_dashboard.py:
##########
@@ -449,7 +449,7 @@ async def
test_refetch_failure_rolls_back_and_returns_minimal_response(
assert content["error"] is None
assert content["dashboard"]["id"] == 7
assert content["dashboard"]["dashboard_title"] == _wrapped("Copy")
- assert "/superset/dashboard/7/" in content["dashboard_url"]
+ assert "/dashboard/7/" in content["dashboard_url"]
Review Comment:
**Suggestion:** This assertion also only checks containment, so
`"/superset/dashboard/7/"` would satisfy it and mask an incorrect response
format. Tighten the expectation to validate canonical dashboard URLs
explicitly. [incorrect condition logic]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
⚠️ Fallback duplicate_dashboard response URL regressions may pass tests.
⚠️ MCP error-path URL correctness weakly validated in suite.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Inspect `_refetch_and_serialize()` in
`superset/mcp_service/dashboard/tool/duplicate_dashboard.py:171-213`; in the
fallback
error-handling path it sets `dashboard_url =
f"{get_superset_base_url()}/dashboard/{new_dashboard.id}/"` and returns it
alongside a
minimal `DashboardInfo`.
2. In
`tests/unit_tests/mcp_service/dashboard/tool/test_duplicate_dashboard.py:269-303`,
`test_refetch_failure_rolls_back_and_returns_minimal_response` simulates a
re-fetch
`SQLAlchemyError`, triggering this fallback path, and then asserts
`"/dashboard/7/" in
content["dashboard_url"]` to validate the URL in the minimal response.
3. If `_refetch_and_serialize()` were to build a legacy URL like
`f"{get_superset_base_url()}/superset/dashboard/{new_dashboard.id}/"` (for
example by
accidentally reusing the deprecated prefix at line 205), the resulting string
`".../superset/dashboard/7/"` would still contain `"/dashboard/7/"`, so the
assertion at
line 452 would pass even though the tool is no longer emitting the canonical
route.
4. Apply that change at `duplicate_dashboard.py:205`, re-run `pytest
tests/unit_tests/mcp_service/dashboard/tool/test_duplicate_dashboard.py`,
and confirm that
`test_refetch_failure_rolls_back_and_returns_minimal_response` continues to
pass,
demonstrating that the test’s non-strict containment check cannot detect
regressions in
the fallback URL format.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=d2e0cfd4c2f9479f9eb7135d36b4bf6d&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=d2e0cfd4c2f9479f9eb7135d36b4bf6d&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/dashboard/tool/test_duplicate_dashboard.py
**Line:** 452:452
**Comment:**
*Incorrect Condition Logic: This assertion also only checks
containment, so `"/superset/dashboard/7/"` would satisfy it and mask an
incorrect response format. Tighten the expectation to validate canonical
dashboard URLs explicitly.
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%2F41758&comment_hash=d57867b98adc266b9521ab2726f44ff9f7c70798dc85b64f8f6c499da635d187&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41758&comment_hash=d57867b98adc266b9521ab2726f44ff9f7c70798dc85b64f8f6c499da635d187&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]
