fix(mcp): improve chart save behavior and error messaging in generate_chart#38712
Closed
aminghadersohi wants to merge 1 commit into
Closed
fix(mcp): improve chart save behavior and error messaging in generate_chart#38712aminghadersohi wants to merge 1 commit into
aminghadersohi wants to merge 1 commit into
Conversation
…_chart When save_chart=True and the compile check fails, keep the chart instead of deleting it. The user explicitly asked to save, so we should preserve the chart and return a warning about the compile issue. The chart can still be edited and fixed in the Explore view. Also improve error messaging: - When dataset_id looks like a name (not numeric/UUID), return a helpful error explaining to use list_datasets to find the numeric ID - Include the actual database error in compile failure messages so the chatbot frontend can display actionable information instead of a generic "network error"
Contributor
Code Review Agent Run #10a5f7Actionable 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 |
codeant-ai-for-open-source
Bot
added
the
size:M
Contributor
Sequence DiagramThis PR changes generate_chart so compile failures no longer delete charts when save mode is enabled, and improves user-facing error messages for dataset lookup and preview compile failures. The flow now preserves saved work with warnings and returns more actionable error details. sequenceDiagram
participant User
participant GenerateChart
participant DatasetService
participant ChartCompiler
User->>GenerateChart: Request generate chart with dataset_id and save_chart
GenerateChart->>DatasetService: Resolve dataset_id
DatasetService-->>GenerateChart: Dataset found or not found
alt Dataset not found and id looks like name
GenerateChart-->>User: Return dataset not found with use list_datasets guidance
else Dataset found
GenerateChart->>ChartCompiler: Run compile check
ChartCompiler-->>GenerateChart: Compile success or failure
alt Compile fails and save_chart is true
GenerateChart-->>User: Return saved chart plus compile warning
else Compile fails in preview mode
GenerateChart-->>User: Return compile error with database message
end
end
Generated by CodeAnt AI |
Codecov Report❌ Patch coverage is
❌ Your project status has failed because the head coverage (99.92%) is below the target coverage (100.00%). You can increase the head coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## master #38712 +/- ##
==========================================
- Coverage 65.12% 64.32% -0.81%
==========================================
Files 1819 2532 +713
Lines 72690 129734 +57044
Branches 23235 29995 +6760
==========================================
+ Hits 47339 83447 +36108
- Misses 25351 44835 +19484
- Partials 0 1452 +1452
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:
|
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
Comment on lines
+568
to
+579
| compile_error_detail = str(compile_result.error) or "" | ||
| error = ChartGenerationError( | ||
| error_type="compile_error", | ||
| message=( | ||
| "Chart query failed to execute. " | ||
| "The chart configuration is invalid." | ||
| "Chart query failed to execute: %s" | ||
| % ( | ||
| compile_error_detail[:200] | ||
| if compile_error_detail | ||
| else "unknown error" | ||
| ) | ||
| ), | ||
| details=str(compile_result.error) or "", | ||
| details=compile_error_detail, |
Contributor
There was a problem hiding this comment.
Suggestion: The compile error is truncated in message but the full raw database error is still returned in details, which can leak internal SQL/engine information and produce oversized error payloads. Use the same sanitized/truncated value for externally returned details. [security]
Severity Level: Major ⚠️
- ❌ Raw SQL/engine internals exposed in MCP error payload.
- ⚠️ Oversized error details can bloat client responses.
Suggested change
| compile_error_detail = str(compile_result.error) or "" | |
| error = ChartGenerationError( | |
| error_type="compile_error", | |
| message=( | |
| "Chart query failed to execute. " | |
| "The chart configuration is invalid." | |
| "Chart query failed to execute: %s" | |
| % ( | |
| compile_error_detail[:200] | |
| if compile_error_detail | |
| else "unknown error" | |
| ) | |
| ), | |
| details=str(compile_result.error) or "", | |
| details=compile_error_detail, | |
| compile_error_detail = str(compile_result.error or "") | |
| compile_error_safe = ( | |
| compile_error_detail[:200] if compile_error_detail else "unknown error" | |
| ) | |
| error = ChartGenerationError( | |
| error_type="compile_error", | |
| message=("Chart query failed to execute: %s" % (compile_error_safe)), | |
| details=compile_error_safe, |
Steps of Reproduction ✅
1. Invoke `generate_chart` through MCP (tool import/registration in
`superset/mcp_service/app.py:395-397`), using default preview flow (`save_chart=False`
default verified in
`tests/unit_tests/mcp_service/chart/tool/test_generate_chart.py:191-199`) with a config
that causes query compilation failure.
2. Preview compile path runs `_compile_chart` (`generate_chart.py:275-280`), and
`_compile_chart` propagates raw backend exception text via `str(exc)`
(`generate_chart.py:116-119`).
3. On failure, response message is truncated, but `details` is set to full raw error
(`generate_chart.py:568-579`) and returned to client through `error.model_dump()`
(`generate_chart.py:589-593`).
4. Because `ChartGenerationError.details` is a direct response field
(`superset/mcp_service/common/error_schemas.py:77`) and `GenerateChartResponse.error` is
returned to clients (`superset/mcp_service/chart/schemas.py:1172-1175`), full SQL/engine
internals can be exposed externally.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset/mcp_service/chart/tool/generate_chart.py
**Line:** 568:579
**Comment:**
*Security: The compile error is truncated in `message` but the full raw database error is still returned in `details`, which can leak internal SQL/engine information and produce oversized error payloads. Use the same sanitized/truncated value for externally returned details.
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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
User description
SUMMARY
Two related bug fixes in the MCP
generate_charttool:1. Chart deletion on compile failure was too aggressive when
save_chart=TrueWhen a user explicitly requests
save_chart=True, the chart was being deleted if the post-creation compile check (test query) failed. This is incorrect -- the user asked to save the chart, so we should keep it and return a warning instead. The chart can still be viewed and fixed in the Explore view.save_chart=True= chart deleted, error returned, user loses worksave_chart=True= chart kept, warning added towarningslist, chart URL and ID returned normally. Preview-only mode (save_chart=False) still returns compile errors as before since there is no saved chart to keep.2. Poor error messaging for dataset lookup and compile failures
When chart creation fails due to dataset issues, the error propagated to the chatbot frontend was too generic ("network error"). Two improvements:
dataset_id(user passed a dataset name instead of ID), the error message explicitly tells the user to uselist_datasetsto find the numeric IDmessagefield (truncated to 200 chars) so the chatbot can display actionable informationBEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
N/A - backend-only changes
TESTING INSTRUCTIONS
pytest tests/unit_tests/mcp_service/chart/ -x -q(333 tests pass)generate_chartwithsave_chart=Trueand a dataset that has a broken column -- chart should be saved with a warning instead of deletedgenerate_chartwithdataset_id="my_dataset_name"-- error should suggest usinglist_datasetsgenerate_chartin preview mode with invalid columns -- error message should include the actual DB errorADDITIONAL INFORMATION
CodeAnt-AI Description
Keep saved charts when compile check fails and show clearer dataset/compile errors
What Changed
Impact
✅ Fewer lost charts when save_chart=True✅ Clearer dataset ID guidance for users who passed a name✅ Clearer compile error messages for previewed charts💡 Usage Guide
Checking Your Pull Request
Every time you make a pull request, our system automatically looks through it. We check for security issues, mistakes in how you're setting up your infrastructure, and common code problems. We do this to make sure your changes are solid and won't cause any trouble later.
Talking to CodeAnt AI
Got a question or need a hand with something in your pull request? You can easily get in touch with CodeAnt AI right here. Just type the following in a comment on your pull request, and replace "Your question here" with whatever you want to ask:
This lets you have a chat with CodeAnt AI about your pull request, making it easier to understand and improve your code.
Example
Preserve Org Learnings with CodeAnt
You can record team preferences so CodeAnt AI applies them in future reviews. Reply directly to the specific CodeAnt AI suggestion (in the same thread) and replace "Your feedback here" with your input:
This helps CodeAnt AI learn and adapt to your team's coding style and standards.
Example
Retrigger review
Ask CodeAnt AI to review the PR again, by typing:
Check Your Repository Health
To analyze the health of your code repository, visit our dashboard at https://app.codeant.ai. This tool helps you identify potential issues and areas for improvement in your codebase, ensuring your repository maintains high standards of code health.