fix(mcp): surface actionable error when adhoc_filters passed to generate_chart#39507
Draft
sadpandajoe wants to merge 1 commit intomasterfrom
Draft
fix(mcp): surface actionable error when adhoc_filters passed to generate_chart#39507sadpandajoe wants to merge 1 commit intomasterfrom
sadpandajoe wants to merge 1 commit intomasterfrom
Conversation
…ate_chart
The validation pipeline caught unknown-field errors from parse_chart_config
but used a template_key ("validation_error") that didn't exist in
ChartErrorBuilder.TEMPLATES, causing the error to fall through to the
default "An error occurred" message with empty details and suggestions.
Additionally, Pydantic's discriminated union error strings start with a
very long type name listing all variants, which caused the 200-char
security truncation to cut off the actual helpful message before it
reached the user.
Fixes:
- Add "validation_error" template to ChartErrorBuilder.TEMPLATES
- Extract meaningful error messages from Pydantic's __cause__ chain
before truncation (only accesses err["msg"], never err["input"])
- Catch config validation errors specifically in the pipeline with a
targeted error_type/error_code instead of the generic catch-all
- Skip generic-message replacement for config errors so field-specific
"did you mean?" suggestions are preserved
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #39507 +/- ##
==========================================
- Coverage 64.47% 64.36% -0.11%
==========================================
Files 2558 2560 +2
Lines 133243 133570 +327
Branches 30956 31012 +56
==========================================
+ Hits 85902 85972 +70
- Misses 45850 46105 +255
- Partials 1491 1493 +2
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:
|
Contributor
There was a problem hiding this comment.
Pull request overview
Improves MCP chart validation error surfacing so LLM clients get actionable, field-specific feedback (not opaque “An error occurred”) when invalid chart configs (e.g., adhoc_filters) are provided.
Changes:
- Add a
validation_errortemplate toChartErrorBuilderso validation failures don’t fall back to the generic default. - Extract concise, meaningful Pydantic validation messages (from the
__cause__ValidationError) before security truncation, with an option to skip generic message replacement. - Add regression tests ensuring unknown config fields yield actionable messages/suggestions and the
CONFIG_VALIDATION_ERRORcode.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
superset/mcp_service/utils/error_builder.py |
Adds a missing validation_error template used by the validation pipeline to produce structured error responses. |
superset/mcp_service/chart/validation/pipeline.py |
Improves sanitization/extraction of config parsing validation errors and returns a specific config-validation error response. |
tests/unit_tests/mcp_service/chart/validation/test_pipeline_unknown_fields.py |
Adds regression coverage for unknown config fields (including adhoc_filters) to ensure actionable errors/suggestions. |
Comment on lines
+178
to
+185
| "validation_error": { | ||
| "message": "Configuration validation failed: {reason}", | ||
| "details": "{reason}", | ||
| "suggestions": [ | ||
| "Check field names in your chart configuration", | ||
| "Read the chart://configs resource for valid fields and examples", | ||
| ], | ||
| }, |
Comment on lines
+18
to
+25
| """ | ||
| Regression tests for validation pipeline error surfacing. | ||
|
|
||
| Ensures that unknown fields (like adhoc_filters) produce actionable error | ||
| messages instead of opaque "An error occurred" responses. | ||
|
|
||
| See: sc-103356 | ||
| """ |
Comment on lines
+88
to
+101
| if not isinstance(cause, PydanticValidationError): | ||
| return None | ||
|
|
||
| # Extract just the message parts from Pydantic's structured errors | ||
| messages = [] | ||
| for err in cause.errors()[:3]: | ||
| msg = err.get("msg", "") | ||
| # Pydantic prefixes model_validator errors with "Value error, " | ||
| if msg.startswith("Value error, "): | ||
| msg = msg[len("Value error, ") :] | ||
| if msg: | ||
| messages.append(msg) | ||
|
|
||
| return " | ".join(messages) if messages else None |
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
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.
SUMMARY
When LLMs pass
adhoc_filters(Superset's internal format) to thegenerate_chartMCP tool, the validation pipeline returns an opaquevalidation_system_errorwith message "An error occurred", empty details, and no suggestions — making it impossible for the LLM to self-correct.Root cause:
ChartErrorBuilder.TEMPLATEShad no"validation_error"key, sobuild_error()fell through to defaults. Additionally, Pydantic's discriminated union error strings list all variant type names, blowing past the 200-char security truncation before the actual "did you mean 'filters'?" message.Fix:
"validation_error"template toChartErrorBuilder.TEMPLATES__cause__chain before truncation (only accesseserr["msg"], nevererr["input"])error_type/error_codeinstead of the generic catch-allskip_genericflag to prevent keyword-based replacement from masking field-specific messagesBEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
Before:
adhoc_filtersin config returns:{"success": false, "error_type": "validation_system_error", "message": "An error occurred", "details": "", "suggestions": [], "error_code": "VALIDATION_PIPELINE_ERROR"}After:
{"success": false, "error_type": "config_validation_error", "message": "Configuration validation failed: Unknown field 'adhoc_filters' — did you mean 'filters'?", "details": "Unknown field 'adhoc_filters' — did you mean 'filters'?", "suggestions": ["Check field names in your chart configuration", "Read the chart://configs resource for valid fields and examples", "Use the 'filters' field for structured filters (column/op/value) — do NOT use 'adhoc_filters'"], "error_code": "CONFIG_VALIDATION_ERROR"}TESTING INSTRUCTIONS
generate_chartwithadhoc_filtersin the config — should now return error mentioning "did you mean 'filters'?" withCONFIG_VALIDATION_ERRORcodepytest tests/unit_tests/mcp_service/chart/validation/test_pipeline_unknown_fields.py -vpytest tests/unit_tests/mcp_service/ -v(1237 tests pass)ADDITIONAL INFORMATION
🤖 Generated with Claude Code