fix(mcp): resolve dashboard permalinks - #43482
Conversation
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>
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.
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
| if self.identifier is None and self.permalink_key is None: | ||
| raise ValueError("Provide identifier or permalink_key") |
There was a problem hiding this comment.
Suggestion: These validators only reject None, so empty strings such as {"identifier": ""} or {"permalink_key": ""} pass validation even though they represent no usable reference. The downstream resolver treats them as ordinary identifiers or falsey permalink references, producing misleading lookup/permalink errors instead of the intended “Provide identifier or permalink_key” validation error. Reject blank strings after trimming in both request validators. [incorrect condition logic]
Severity Level: Minor 🧹
- ⚠️ Blank info requests reach dashboard lookup unnecessarily.
- ⚠️ Blank layout requests return misleading permalink errors.
- ⚠️ Clients receive inconsistent validation guidance.
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset/mcp_service/dashboard/schemas.py
**Line:** 302:303
**Comment:**
*Incorrect Condition Logic: These validators only reject `None`, so empty strings such as `{"identifier": ""}` or `{"permalink_key": ""}` pass validation even though they represent no usable reference. The downstream resolver treats them as ordinary identifiers or falsey permalink references, producing misleading lookup/permalink errors instead of the intended “Provide identifier or permalink_key” validation error. Reject blank strings after trimming in both request validators.
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
| permalink_state = get_matching_dashboard_permalink_state( | ||
| lookup_result, result.id, result.uuid | ||
| ) |
There was a problem hiding this comment.
Suggestion: Legacy permalinks can store the dashboard slug in dashboardId, but this call verifies the permalink using only result.id and result.uuid. Because DashboardLayout does not expose the slug, a valid slug-based permalink is treated as belonging to a different dashboard and its active-tab/filter state is discarded, unlike get_dashboard_info. Include the dashboard slug in the layout lookup/serialization or otherwise pass it to the matching check. [api mismatch]
Severity Level: Major ⚠️
- ⚠️ Layout permalink state is discarded for legacy slug references.
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset/mcp_service/dashboard/tool/get_dashboard_layout.py
**Line:** 115:117
**Comment:**
*Api Mismatch: Legacy permalinks can store the dashboard slug in `dashboardId`, but this call verifies the permalink using only `result.id` and `result.uuid`. Because `DashboardLayout` does not expose the slug, a valid slug-based permalink is treated as belonging to a different dashboard and its active-tab/filter state is discarded, unlike `get_dashboard_info`. Include the dashboard slug in the layout lookup/serialization or otherwise pass it to the matching check.
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
|
The flagged issue is correct. The current Here is the corrected implementation for @model_validator(mode="after")
def _require_identifier_or_permalink(self) -> "GetDashboardInfoRequest":
if (self.identifier is None or (isinstance(self.identifier, str) and not self.identifier.strip())) and \
(self.permalink_key is None or not self.permalink_key.strip()):
raise ValueError("Provide identifier or permalink_key")
return self(Apply this same logic to 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/schemas.py |
![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 #af015b
Actionable Suggestions - 1
-
superset/mcp_service/dashboard/permalink.py - 1
- Missing test for privacy redaction · Line 182-182
Filtered by Review Rules
Bito filtered these suggestions based on rules created automatically for your feedback. Manage rules.
-
superset/mcp_service/dashboard/schemas.py - 2
- CWE-20: Inverted validator condition · Line 302-302
- Duplicated validator with inverted logic · Line 335-335
Review Details
-
Files reviewed - 9 · Commit Range:
1f6f618..8da8807- 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 an incremental AI Review. -
/review full- 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
|
|
||
| raw_state = value.get("state") | ||
| state: dict[str, object] = dict(raw_state) if isinstance(raw_state, dict) else {} | ||
| if not user_can_view_data_model_metadata(): |
There was a problem hiding this comment.
The privacy-gated redaction on line 182 is not exercised by existing tests. test_get_matching_dashboard_permalink_state_skips_check_when_permalink_resolved passes resolved_from_permalink=True and hits the early-return before line 182, while the other get_matching_dashboard_permalink_state tests never call user_can_view_data_model_metadata or redact_filter_state_data_model_metadata. Add coverage to prevent silent regressions if the redaction logic is ever refactored.
Code Review Run #af015b
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #43482 +/- ##
==========================================
+ Coverage 78.92% 78.94% +0.01%
==========================================
Files 2878 2879 +1
Lines 165088 165176 +88
Branches 38137 38153 +16
==========================================
+ Hits 130296 130390 +94
+ Misses 32342 32336 -6
Partials 2450 2450
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:
|
gabotorresruiz
left a comment
gabotorresruiz
left a comment
There was a problem hiding this comment.
Thanks Amin for carefully reconstructing this after the fork mishap, and for the thorough carried-over summary in the description.
I verified the recreation rather than assuming it: all five commits from the approved #42659 are patch-identical here (git range-diff shows them equal), and the source files are byte-identical to the head I approved there, so the live verification from that review carries over: a permalink created through the real dashboard permalink API round-trips its uuid dashboardId and full filter state through both MCP tools over HTTP, and a mistyped identifier reports its own not-found error. The only new content is the test-only commit adding the nine direct lookup_dashboard_reference tests, which read well and cover each resolution branch. Locally the dashboard suite passes (411 tests) and the full mcp_service suite passes (3579 tests) at this head, and CI is green.
The bot note about empty-string identifiers is cosmetic: I checked that {"identifier": ""} returns a clean not_found naming the identifier without touching the permalink store, so tightening the validator is optional. Same for the layout slug note, which we already agreed was a non-blocking follow-up.
LGTM.
2 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.
Carried-over review history from #42659
#42659 was reviewed by @gabotorresruiz, who raised two issues and subsequently approved. Both fixes are included here:
dashboardId—CreateDashboardPermalinkCommandstoresstr(dashboard.uuid), so the originalint(...)comparison failed for virtually every real permalink, dropping shared state with a spurious "belongs to a different dashboard" warning.get_matching_dashboard_permalink_statenow compares the reference against id, uuid and slug (slug covers pre-3.1 permalinks), andDashboardLookupResult.resolved_from_permalinkmarks the permalink-only path so it skips re-verification entirely.result is not None, reserving the permalink wording for permalink-only requests.A bot review also flagged that
lookup_dashboard_referencehad no direct tests despite four branching paths; nine direct tests were added covering each resolution path.This branch was additionally rebased across #43202 ("preserve user-authored result values"), which removed
sanitize_for_llm_contextservice-wide and rewrote_apply_permalink_stateto usemodel_copy. Both were reconciled rather than resolved in this branch's favour: the removed sanitization call was dropped (the privacy redaction viauser_can_view_data_model_metadatais retained, since that is access control rather than value rewriting) and master'smodel_copyform was kept.BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
Not applicable; this changes MCP tool behavior and schemas only.
TESTING INSTRUCTIONS
The dashboard MCP unit suite passes (411 tests). Changed-file pre-commit passes, including mypy, ruff and pylint.
Coverage worth noting:
dashboardIdassertingfilter_stateis present, for both tools (test_dashboard_tools.py,test_get_dashboard_layout.py), plus a slug case for pre-3.1 permalinks.lookup_dashboard_referencetests. These were mutation-checked: removing the identifier-precedence guard, forcingresolved_from_permalink=False, and disabling permalink-URL extraction each cause failures, so the tests pin behaviour rather than merely passing.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.
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.
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.