fix(mcp): restore typed ChartConfig in tool schemas for LLM visibility#39732
Merged
aminghadersohi merged 6 commits intoApr 28, 2026
Conversation
PR apache#39018 changed the config field from ChartConfig (discriminated union) to Dict[str, Any] in GenerateChartRequest, GenerateExploreLinkRequest, UpdateChartRequest, and UpdateChartPreviewRequest. This removed all chart config structure from the JSON Schema exposed to LLMs, causing them to fail with validation errors (wrong field names, missing required fields, invalid types). Also flips include_schemas default to True so search_tools results include full compacted inputSchema instead of the parameters_hint summary (which only shows "request" — the top-level wrapper name). Changes: - Revert config field type from Dict[str, Any] back to ChartConfig - Remove now-redundant parse_chart_config() calls in tool bodies (Pydantic validates the discriminated union at request parse time) - Remove dead code: _coerce_config_to_dict, _CHART_CONFIG_DESCRIPTION, _CHART_CONFIG_ADAPTER, parse_chart_config - Set include_schemas=True in MCP_TOOL_SEARCH_CONFIG default
Contributor
Code Review Agent Run #0d3f3eActionable 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 |
Remove parse_chart_config imports and update tests to validate ChartConfig through Pydantic request models instead of the removed parse_chart_config() function.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #39732 +/- ##
==========================================
+ Coverage 64.47% 64.49% +0.02%
==========================================
Files 2566 2566
Lines 134027 133960 -67
Branches 31121 31116 -5
==========================================
- Hits 86410 86394 -16
+ Misses 46122 46071 -51
Partials 1495 1495
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:
|
Convert dict-style config access (config["key"]) back to attribute access (config.key) in test files to match the restored ChartConfig typed field. Update include_schemas default assertion to True.
kgabryje
approved these changes
Apr 28, 2026
…error code expectations
- Replace request.config.get("chart_type", "unknown") with
request.config.chart_type in generate_chart and generate_explore_link
- Update test_pipeline_error_surface to expect VALIDATION_ERROR
(the error code produced by SchemaValidator when Pydantic catches
invalid config fields at parse time)
The error message from SchemaValidator is now generic; the field name appears in details instead of message.
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
generate_chart debug log called config.keys() expecting a dict, but config is now a typed Pydantic model.
Contributor
|
Bito Automatic Review Skipped – PR Already Merged |
bestlong
pushed a commit
to bestlong/superset
that referenced
this pull request
May 7, 2026
qfcwell
pushed a commit
to qfcwell/superset
that referenced
this pull request
May 12, 2026
9 tasks
aminghadersohi
added a commit
to aminghadersohi/superset
that referenced
this pull request
Jul 1, 2026
MCP_TOOL_SEARCH_CONFIG has defaulted include_schemas to True since apache#39732 (to keep discriminated-union chart configs visible to LLMs without a second round trip), but the comment above it still described include_schemas=False as the default.
9 tasks
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
2 participants
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
PR #39018 changed the
configfield fromChartConfig(typed discriminated union) toDict[str, Any]in chart request schemas to reduce token usage. This removed all chart config structure from the JSON Schema exposed to LLMs, causing validation errors:columninstead ofnamefor x-axis)x.name)y)Also flips
include_schemasdefault toTruesosearch_toolsresults include full compactedinputSchemainstead of theparameters_hintsummary which only shows"request"(the top-level wrapper name).Changes
configfield type fromDict[str, Any]back toChartConfiginGenerateChartRequest,GenerateExploreLinkRequest,UpdateChartRequest,UpdateChartPreviewRequestparse_chart_config()calls in tool bodies (Pydantic validates the discriminated union at request parse time)_coerce_config_to_dict,_CHART_CONFIG_DESCRIPTION,_CHART_CONFIG_ADAPTER,parse_chart_configinclude_schemas=TrueinMCP_TOOL_SEARCH_CONFIGdefaultToken cost
~12k tokens added back for chart tools. Justified: failed retries from broken schemas cost far more tokens than the larger schema.
Test plan
GenerateChartRequestincludes full discriminated union