fix(mcp): resolve dashboard permalinks - #42659
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #42659 +/- ##
==========================================
- Coverage 66.83% 66.79% -0.05%
==========================================
Files 2876 2877 +1
Lines 164061 164310 +249
Branches 37860 37922 +62
==========================================
+ Hits 109653 109748 +95
- Misses 52233 52377 +144
- Partials 2175 2185 +10
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
dosubot
Bot
added
api
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
|
The flagged issue is correct. In the current implementation of To resolve this, you should validate that the # ... inside _lookup_dashboard ...
lookup_identifier = (
permalink_value.get("dashboardId")
if permalink_value is not None
else request.identifier
)
# Validate identifier match if both are provided
if permalink_value and request.identifier and str(lookup_identifier) != str(request.identifier):
# Handle mismatch: raise error or prioritize one
return (
DashboardError.create("Identifier and permalink refer to different dashboards.", "mismatch"),
permalink_key,
None,
)
result = tool.run_tool(lookup_identifier) # type: ignore[arg-type]
# ...Would you like me to fetch all other comments on this PR to validate and implement fixes for them as well? superset/mcp_service/dashboard/tool/get_dashboard_info.py |
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
Addressed the identifier/permalink precedence feedback in |
dosubot
Bot
added
api
8a8e109 to
a721e5e
Compare
|
CI follow-up: I rebased this branch onto current |
786fef6 to
52453c3
Compare
|
Fixed the remaining Python CI failure in |
Code Review Agent Run #0bb033Actionable Suggestions - 0Additional Suggestions - 4
Filtered by Review RulesBito filtered these suggestions based on rules created automatically for your feedback. Manage rules.
Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
6b9ed48 to
e038e93
Compare
|
Addressed all four suggestions from the latest Bito review in
The affected MCP tests pass (52 passed), along with Ruff and targeted mypy. |
Code Review Agent Run #a2c76bActionable Suggestions - 0Additional Suggestions - 3
Filtered by Review RulesBito filtered these suggestions based on rules created automatically for your feedback. Manage rules.
Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
gabotorresruiz
left a comment
gabotorresruiz
left a comment
There was a problem hiding this comment.
Thanks Amin, transparent permalink resolution in the lookup tools is the right shape for this, and the identifier-precedence contract you settled with the earlier bot comment reads well. I found one blocking issue though: the dashboardId match doesn't survive contact with real permalinks, so the shared state gets dropped in practice. Details inline, happy to dig in with you.
|
Pushed
On the remaining bot suggestion (
|
b36d56e to
b6cb9c9
Compare
Code Review Agent Run #973badActionable Suggestions - 0Filtered by Review RulesBito filtered these suggestions based on rules created automatically for your feedback. Manage rules.
Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
|
Hey Amin, I went to re-review and hit a surprise: |
Hi Gabriel, thanks for the review. not sure what happened but fixing now. |
|
Thanks for the careful re-check — and good news: the commit is on the branch, it just has a new SHA. I rebased onto master, which rewrote From the PR timeline:
Current head is
I diffed the current PR head against my local branch across all eight files in the PR diff and they are byte-identical: So the state your harness validated with On CI: 45 checks pass, one fails —
Sorry for the confusion the force-push caused — happy to re-run anything you want against the current head. |
CreateDashboardPermalinkCommand stores dashboardId as str(dashboard.uuid), so the int() comparison dropped shared state for virtually every real permalink. The permalink-only path now records that it selected the dashboard itself and skips re-verification, while the explicit identifier + permalink_key combination compares the reference against the dashboard id, uuid, and slug. A plain identifier that simply does not exist also no longer reports the permalink wording; its own not-found error is preserved and the permalink message is reserved for requests that had to resolve through a permalink. Co-Authored-By: Claude <noreply@anthropic.com>
2778555 to
daf1838
Compare
|
Correction and update to my previous comment — that comment was accurate about the SHA, but incomplete, and I should flag what I missed. What I got right: the fix was on the branch; the rebase had rewritten What I missed: the PR was in a
I have rebased onto
New head is Verification: 399 dashboard MCP tests pass; full Worth re-running your harness against |
![bito-code-review[bot]](https://avatars.githubusercontent.com/in/1061978?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review Agent Run #c9ea69
Actionable Suggestions - 1
-
superset/mcp_service/dashboard/permalink.py - 1
- Untested public function · Line 103-149
Filtered by Review Rules
Bito filtered these suggestions based on rules created automatically for your feedback. Manage rules.
-
tests/unit_tests/mcp_service/dashboard/test_permalink.py - 1
- CWE-483: Mock target path incorrect · Line 85-85
Review Details
-
Files reviewed - 9 · Commit Range:
0a77b35..daf1838- superset/mcp_service/app.py
- superset/mcp_service/dashboard/permalink.py
- superset/mcp_service/dashboard/schemas.py
- superset/mcp_service/dashboard/tool/get_dashboard_info.py
- superset/mcp_service/dashboard/tool/get_dashboard_layout.py
- tests/unit_tests/mcp_service/dashboard/test_dashboard_schemas.py
- tests/unit_tests/mcp_service/dashboard/test_permalink.py
- tests/unit_tests/mcp_service/dashboard/tool/test_dashboard_tools.py
- tests/unit_tests/mcp_service/dashboard/tool/test_get_dashboard_layout.py
-
Files skipped - 0
-
Tools
- MyPy (Static Code Analysis) - ✔︎ Successful
- Astral Ruff (Static Code Analysis) - ✔︎ Successful
- Whispers (Secret Scanner) - ✔︎ Successful
- Detect-secrets (Secret Scanner) - ✔︎ Successful
Bito Usage Guide
Commands
Type the following command in the pull request comment and save the comment.
-
/review- Manually triggers a full AI review. -
/pause- Pauses automatic reviews on this pull request. -
/resume- Resumes automatic reviews. -
/resolve- Marks all Bito-posted review comments as resolved. -
/abort- Cancels all in-progress reviews.
Refer to the documentation for additional commands.
Configuration
This repository uses Superset You can customize the agent settings here or contact your Bito workspace admin at evan@preset.io.
Documentation & Help
AI Code Review powered by
| def lookup_dashboard_reference( | ||
| *, | ||
| identifier: int | str | None, | ||
| permalink_key: str | None, | ||
| lookup: Callable[[int | str], LookupResultT], | ||
| is_found: Callable[[LookupResultT], bool], | ||
| ) -> DashboardLookupResult[LookupResultT]: | ||
| """Look up a dashboard while preserving identifier precedence. | ||
|
|
||
| A supplied identifier selects the dashboard and an explicit permalink only | ||
| contributes state. Shared permalink URLs and permalink-only requests select | ||
| the dashboard embedded in the permalink. Ambiguous bare strings use normal | ||
| identifier lookup first, then fall back to permalink resolution. | ||
| """ | ||
| key = permalink_key | ||
| identifier_is_permalink_url = False | ||
| if isinstance(identifier, str): | ||
| extracted_key = extract_dashboard_permalink_key(identifier) | ||
| identifier_is_permalink_url = extracted_key != identifier | ||
| if identifier_is_permalink_url: | ||
| key = extracted_key | ||
|
|
||
| if identifier is not None and not identifier_is_permalink_url: | ||
| result = lookup(identifier) | ||
| if is_found(result): | ||
| resolved = get_dashboard_permalink(key) if key else None | ||
| return DashboardLookupResult( | ||
| result=result, | ||
| permalink_key=resolved[0] if resolved else key, | ||
| permalink_value=resolved[1] if resolved else None, | ||
| ) | ||
| if permalink_key is not None or not isinstance(identifier, str): | ||
| return DashboardLookupResult(result=result, permalink_key=key) | ||
| else: | ||
| result = None | ||
|
|
||
| reference = key or (identifier if isinstance(identifier, str) else None) | ||
| resolved = get_dashboard_permalink(reference) if reference else None | ||
| if resolved is None: | ||
| return DashboardLookupResult(result=result, permalink_key=reference) | ||
| key, value = resolved | ||
| return DashboardLookupResult( | ||
| result=lookup(value["dashboardId"]), | ||
| permalink_key=key, | ||
| permalink_value=value, | ||
| resolved_from_permalink=True, | ||
| ) |
There was a problem hiding this comment.
The lookup_dashboard_reference function handles four distinct code paths with complex branching logic (identifier type checks, permalink URL detection, fallback resolution). It has zero test coverage despite being a public utility used by dashboard tools. Missing tests increase the risk of undetected regressions as the MCP service evolves.
Code Review Run #c9ea69
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
There was a problem hiding this comment.
Fair catch — confirmed. lookup_dashboard_reference was exercised indirectly through the get_dashboard_info / get_dashboard_layout tool tests, but it had no direct tests, and its branching is the part of this PR most likely to regress quietly.
Added nine direct tests in tests/unit_tests/mcp_service/dashboard/test_permalink.py, one per resolution path:
- identifier only, no permalink involved
- identifier wins, permalink contributes state only (
resolved_from_permalink is False) - dashboard kept when the permalink is expired/unresolvable
- numeric identifier not found — never falls back to permalink resolution
- string identifier not found with an explicit
permalink_key— keeps the identifier's own not-found result - shared
/dashboard/p/<key>/URL passed asidentifier - permalink-only request
- ambiguous bare string, identifier lookup first then permalink fallback
- fully unresolvable reference
To check these actually pin behaviour rather than just passing, I ran three mutations against the source and confirmed each is caught:
| Mutation | Result |
|---|---|
| Remove the identifier-precedence guard | test_..._identifier_not_found_with_explicit_permalink fails |
Force resolved_from_permalink=False |
3 tests fail |
Ignore permalink-URL extraction in identifier |
test_..._shared_url_identifier fails |
get_dashboard_permalink is patched at the module boundary so these stay unit tests with no DB or request context. Full dashboard suite is 408 passed; pre-commit (mypy, ruff, pylint) clean.
There was a problem hiding this comment.
The addition of nine direct unit tests in tests/unit_tests/mcp_service/dashboard/test_permalink.py effectively addresses the concern regarding the lack of test coverage for lookup_dashboard_reference. By covering each resolution path and verifying behavior through mutation testing, the implementation is now robustly validated against regressions.
Add direct unit tests for the identifier/permalink resolution branches: identifier-only, identifier plus permalink state, unresolvable permalink, numeric and string not-found precedence, shared URL identifiers, permalink-only requests, bare-string fallback, and fully unresolvable references.
|
@gabotorresruiz thanks for the approval, and for the two catches that got it here — both were real and both were reproducible exactly as you described. Recapping how each landed, since the branch was rebased after your first review and the line numbers have moved: 1. UUID 2. Swallowed not-found error ( Heads-up: I pushed one commit after your approval. Also worth noting for the record: this branch was rebased onto master after |
Code Review Agent Run #62d793Actionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
|
Superseded by #43482. This PR could not be reopened: the source fork repository was deleted, and GitHub permanently severs the link in that case ( #43482 contains the same work rebased onto current @gabotorresruiz apologies for the churn — this needs a fresh approval on #43482 since the original one could not carry over. |
3 participants
SUMMARY
Dashboard lookup tools can resolve shared
/dashboard/p/<key>/links and bare permalink keys, returning the dashboard identifier together with the permalink's active-tab and filter state. This extends the existing tools rather than adding a resolver round trip: their response models already carry dashboard metadata and state, so transparent resolution keeps the workflow discoverable without losing context.Invalid or expired permalinks return an actionable error asking for a fresh shared link. Numeric IDs, UUIDs, and slugs retain their existing lookup path; ambiguous bare strings are attempted as permalinks only after ordinary identifier lookup fails.
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
Not applicable; this changes MCP tool behavior and schemas only.
TESTING INSTRUCTIONS
The dashboard MCP unit suite passes (362 tests). Changed-file pre-commit passes. The repository-wide pre-commit run was also attempted; unrelated existing type/lint failures and missing frontend dependencies prevent that full-tree gate from completing.
ADDITIONAL INFORMATION
Blast radius
Apache Superset's optional MCP service only. No migrations, feature flags, authentication changes, workspace isolation changes, or database query behavior changes.
Risk and rollback
The main risk is interpreting an unknown slug-like identifier as a permalink after normal lookup fails. Existing identifiers are resolved first, and reverting this commit restores the previous behavior.
Eval evidence
The deployment-backed agent eval suite was not run for this draft because no deployed build is available. The deterministic MCP unit coverage exercises permalink resolution, active-tab/filter context, invalid links, and existing identifier forms.
Cost and latency delta
No model, prompt-routing, or token changes. A bare permalink key can add one permalink lookup after an unsuccessful ordinary identifier lookup; explicit permalink inputs and shared URLs resolve directly. Deployment-backed latency measurements were not available for this draft.
Prompt / non-determinism
Tool descriptions were updated to identify
/dashboard/p/<key>/links and direct agents to the lookup tools. The resolution behavior is deterministic; no model prompt or routing behavior changed.Review guidance
Start with
dashboard/permalink.pyand the request/response schema changes, then review how each lookup tool reuses the resolved dashboard ID and safely exposes permalink state. The most important behavior is the ordinary-identifier-first fallback for bare strings.