feat(mcp): add compile check to validate chart queries before returning#38392
feat(mcp): add compile check to validate chart queries before returning#38392aminghadersohi wants to merge 2 commits intoapache:masterfrom
Conversation
After generating a chart, execute a lightweight test query via QueryContextFactory + ChartDataCommand to verify the chart renders without errors. If the query fails, return a structured error instead of a link to a broken chart. - Add _compile_chart() helper that runs the chart query with row_limit=2 - For saved charts (save_chart=True): delete the chart on compile failure - For preview-only mode: return compile error without saving - Add 4 unit tests covering success, query errors, command exceptions, and bad config scenarios
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #38392 +/- ##
==========================================
+ Coverage 64.30% 65.09% +0.78%
==========================================
Files 1811 2484 +673
Lines 71557 124205 +52648
Branches 22810 28886 +6076
==========================================
+ Hits 46015 80850 +34835
- Misses 25542 41939 +16397
- Partials 0 1416 +1416
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 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 #146e02
Actionable Suggestions - 1
-
superset/mcp_service/chart/tool/generate_chart.py - 1
- Security: Missing permission check in preview compile · Line 519-524
Review Details
-
Files reviewed - 2 · Commit Range:
a570d28..a570d28- superset/mcp_service/chart/tool/generate_chart.py
- tests/unit_tests/mcp_service/chart/tool/test_generate_chart.py
-
Files skipped - 0
-
Tools
- Whispers (Secret Scanner) - ✔︎ Successful
- Detect-secrets (Secret Scanner) - ✔︎ Successful
- MyPy (Static Code Analysis) - ✔︎ Successful
- Astral Ruff (Static Code Analysis) - ✔︎ 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
| else: | ||
| from superset.daos.dataset import DatasetDAO | ||
|
|
||
| ds = DatasetDAO.find_by_id(request.dataset_id, id_column="uuid") | ||
| if ds: | ||
| numeric_dataset_id = ds.id |
There was a problem hiding this comment.
The preview compile check fetches datasets without permission validation, unlike the save_chart path. This could allow users to probe compilation of inaccessible datasets via UUID. Add has_dataset_access checks after dataset lookup to prevent unauthorized information disclosure.
Citations
- Rule Violated: CLAUDE.md:202
Code Review Run #146e02
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
There was a problem hiding this comment.
Good catch — added has_dataset_access(ds) check in the preview compile path UUID lookup (b69ec08). The save path already had this validation; the preview path was missing it.
There was a problem hiding this comment.
Yes, adding the has_dataset_access(ds) check in the preview compile path UUID lookup addresses the security issue by preventing unauthorized access to dataset compilation, matching the validation already in the save path.
…pile path Add has_dataset_access() check when resolving dataset by UUID in the preview-only compile check path. The save_chart path already validated permissions, but the preview path allowed UUID-based dataset probing without access verification.
Code Review Agent Run #67e2d8Actionable 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 |
|
Reopening with a clean branch name. |
1 participant
SUMMARY
When generating a chart via MCP, the tool previously returned a chart URL without verifying
that the underlying query actually executes successfully. This meant users could receive
links to broken charts that fail to render.
This PR adds a compile check step that runs a lightweight test query (row_limit=2) via
QueryContextFactory+ChartDataCommandafter chart generation. If the query fails,a structured error response is returned instead of a broken chart link.
Behavior:
save_chart=True): If compile check fails, the chart is deleted fromthe database and an error is returned — no broken charts are left behind
save_chart=False): If compile check fails, an error is returnedwith details about what went wrong
The
_compile_chart()helper catchesChartDataQueryFailedError,ChartDataCacheLoadError,CommandException,ValueError, andKeyError— returning aCompileResultdataclasswith success status, error message, warnings, and row count.
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
N/A — backend-only change, no UI modifications.
TESTING INSTRUCTIONS
TestCompileChartclass covers:ChartDataQueryFailedErrorexceptionsValueErrorfrom invalid configurationADDITIONAL INFORMATION