fix(mcp): reject unsafe dashboard layout replacements - #43367
fix(mcp): reject unsafe dashboard layout replacements#43367aminghadersohi wants to merge 3 commits into
Conversation
dosubot
Bot
added
change:backend
Code Review Agent Run #90c7afActionable 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 |
d184f9a to
0255d06
Compare
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #43367 +/- ##
===========================================
+ Coverage 66.81% 78.85% +12.04%
===========================================
Files 2876 2878 +2
Lines 164454 168756 +4302
Branches 37960 38866 +906
===========================================
+ Hits 109873 133076 +23203
+ Misses 52388 33110 -19278
- Partials 2193 2570 +377
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:
|
![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 #2ded6f
Actionable Suggestions - 1
-
tests/unit_tests/mcp_service/dashboard/test_layout_validation.py - 1
- Test expects wrong error message · Line 150-156
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_layout_validation.py - 1
- Test assertion mismatch with actual behavior · Line 141-147
Review Details
-
Files reviewed - 5 · Commit Range:
0255d06..b36ba19- superset/mcp_service/dashboard/layout_validation.py
- superset/mcp_service/dashboard/schemas.py
- superset/mcp_service/dashboard/tool/update_dashboard.py
- tests/unit_tests/mcp_service/dashboard/test_layout_validation.py
- tests/unit_tests/mcp_service/dashboard/tool/test_update_dashboard.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
Code Review Agent Run #2d7a94Actionable 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 |
gabotorresruiz
left a comment
gabotorresruiz
left a comment
There was a problem hiding this comment.
Thanks for this fix @aminghadersohi, the failure mode is real and the validator is a solid piece of work. I dug in empirically: verified the hydration mechanism from the description (buildDashboardLayout indexes every CHART node, which suppresses the auto-place fallback for unreachable charts), ran the full mcp_service unit suite on this branch (3530 passed), confirmed the new regression test fails without the fix, and exercised update_dashboard live over MCP HTTP. The shape, graph, cycle, and reachability protections all behave as described, and invalid updates leave the stored layout untouched.
One thing is blocking for me: the parents consistency check rejects 5 of the 9 example dashboards shipped in this repo, and live it rejects a verbatim round-trip of a dashboard's own saved layout. Details inline, and happy to dig in with you on the normalization approach if useful.
116e3e1 to
037c7ea
Compare
Code Review Agent Run #d59f57Actionable Suggestions - 0Additional Suggestions - 1
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 |
2 participants
SUMMARY
Reject unsafe full
position_jsonreplacements in the MCPupdate_dashboardtool before they overwrite a valid saved layout.Superset renders only layout nodes reachable from
ROOT_ID, but dashboard hydration indexes everyCHARTnode inposition_json. An unreachable chart therefore prevents the normal missing-chart fallback while remaining invisible, which can leave a dashboard blank even though the chart nodes and slice associations still exist.This change validates renderer-required component shape, the v2 marker, graph references, parent/child compatibility, cycles, component IDs, parent paths, reachability, and root/top-level-tab shape. Traversal is iterative, imported decimal-string chart IDs are normalized through a helper shared with chart removal, and
DYNAMICnodes are rejected because the backend cannot validate the frontend registry. Reachable chart IDs must exactly match the charts associated with the dashboard.Invalid updates return
InvalidDashboardLayoutbefore any field mutation or commit, preserving the previous layout. The validator handles Superset's two reserved exceptions: detachedHEADER_ID, and the empty detachedGRID_IDretained by top-level tab layouts.The MCP schema now explicitly states that
get_dashboard_layoutis a summary rather than a round-trippable raw tree, so callers are not directed into unsafe incremental full replacements.Tracking: SC-117987
[sc-117987]Related: #43133 contains generate-only fallback validation as part of native AI authoring. This PR provides the stronger shared validator for that path to reuse after rebase while keeping update rejection separate from generate fallback behavior.
TESTING INSTRUCTIONS
pre-commit run --files superset/mcp_service/dashboard/layout_validation.py superset/mcp_service/dashboard/schemas.py superset/mcp_service/dashboard/tool/update_dashboard.py superset/mcp_service/dashboard/tool/remove_chart_from_dashboard.py tests/unit_tests/mcp_service/dashboard/test_layout_validation.py tests/unit_tests/mcp_service/dashboard/tool/test_update_dashboard.pyruff checkandruff format --checkpass for all changed Python files.git diff --checkADDITIONAL INFORMATION