Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
41 commits
Select commit Hold shift + click to select a range
d2bf221
feat(mcp): introduce chart type plugin registry for extensible chart …
aminghadersohi May 6, 2026
45d5532
feat(mcp): add display_name and native_viz_types to chart type plugins
aminghadersohi May 7, 2026
bb96501
refactor(mcp): eliminate dead code and complete plugin registry dispatch
aminghadersohi May 7, 2026
73928a8
refactor(mcp): complete plugin protocol — registry bootstrap, mypy fi…
aminghadersohi May 7, 2026
b2c70f7
fix(mcp): address reviewer comments — local import rationale, x-optio…
aminghadersohi May 7, 2026
1521a67
refactor(mcp): move all local imports to top level in chart type plugins
aminghadersohi May 7, 2026
5410a28
fix(mcp): resolve ruff E501 and formatting issues to pass pre-commit
aminghadersohi May 9, 2026
64dfe81
fix(mcp): resolve E402 and E501 in dataset_validator.py
aminghadersohi May 10, 2026
a08d586
fix(mcp): add threading lock to registry plugin loader
aminghadersohi May 13, 2026
29b15e9
fix(mcp): add full column validation to update_chart
aminghadersohi May 13, 2026
7f17d96
refactor(mcp): address Codex review — fix registry bug, DRY schema hi…
aminghadersohi May 13, 2026
8e8ec33
fix(mcp): fix E501 in update_chart.py and update_chart test mocks for…
aminghadersohi May 13, 2026
0248e09
feat(mcp): add runtime chart plugin enable/disable via _PluginFilterC…
aminghadersohi May 15, 2026
05934d8
fix(mcp): fix saved-metric name normalization across all chart plugins
aminghadersohi May 21, 2026
cd75287
fix(mcp): use (saved_metric, label) dedup key in XYChartConfig
aminghadersohi May 22, 2026
68fe635
fix(mcp): address Bito review — log bare exception in schemas, remove…
aminghadersohi May 22, 2026
94ab227
style: ruff-format auto-format fix
aminghadersohi May 26, 2026
f7817e8
perf(mcp): remove redundant DatasetValidator call in update_chart
aminghadersohi May 26, 2026
2097b2e
fix(mcp): lock register() writes and add circuit breaker to _ensure_p…
aminghadersohi May 27, 2026
b34640c
style: fix E501 in test_registry docstring (93 > 88 chars)
aminghadersohi May 27, 2026
f60d2b0
fix(mcp): address fitzee review — sanitize temporal_column, narrow BL…
aminghadersohi May 27, 2026
9af833b
fix(mcp): harden chart registry review fixes
aminghadersohi May 27, 2026
b663f90
fix(mcp): address bito review — pre_validate empty-list and chart_typ…
aminghadersohi May 28, 2026
4f67e21
fix(mcp): address copilot + bito review comments
aminghadersohi May 29, 2026
43fb904
fix(mcp): fix CI failures — sql_expression handling, cardinality guar…
aminghadersohi May 29, 2026
a756370
fix(mcp): extend pre_validate empty-list guard to xy, mixed_timeserie…
aminghadersohi May 30, 2026
650a6af
fix(mcp): fix import sort order in app.py (ruff I001)
aminghadersohi May 30, 2026
1ad3bef
fix(mcp): address self-review findings — comments, dedup, modern types
aminghadersohi May 30, 2026
7f51b9e
fix(mcp): restore master's API-key auth factory lost in rebase conflict
aminghadersohi Jun 10, 2026
742f09a
feat(mcp): warn on native_viz_types collision at plugin registration
aminghadersohi Jun 10, 2026
4c03bb1
chore(mcp): restore out-of-scope files to master state
aminghadersohi Jun 11, 2026
869291e
chore(mcp): drop spurious executable bits on three Python files
aminghadersohi Jun 11, 2026
96a9e43
fix(mcp): keep fastmcp optional — configure chart registry only in mc…
aminghadersohi Jun 11, 2026
975361b
fix(mcp): fix pre_validate aliases and sql_expression normalization
aminghadersohi Jun 26, 2026
8620204
fix(mcp): guard sql_expression in table/handlebars normalization; use…
aminghadersohi Jun 26, 2026
cc132ec
fix(mcp): guard pie dimension saved_metric; recompute display name on…
aminghadersohi Jun 26, 2026
f43ed34
refactor(mcp): consolidate DatasetContext builder; expose resolver me…
aminghadersohi Jun 28, 2026
b4e1f5d
fix(mcp): fix test_get_chart_info module import shadowed by __init__.…
aminghadersohi Jun 29, 2026
eec4967
fix(mcp): correct method count in ChartTypePlugin docstring (eight → …
aminghadersohi Jun 30, 2026
9e55ba0
fix(mcp): address reviewer action items from approval
aminghadersohi Jul 1, 2026
2984a3a
test(mcp): add required label to sql_expression metric in normalizati…
aminghadersohi Jul 1, 2026
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Empty file modified .pre-commit-config.yaml
100644 → 100755
Empty file.
11 changes: 7 additions & 4 deletions superset/mcp_service/app.py
Original file line number Diff line number Diff line change
Expand Up @@ -354,10 +354,12 @@ def get_default_instructions(
- PT1H (hourly), P1D (daily), P1W (weekly), P1M (monthly), P1Y (yearly)

Chart Types in Existing Charts (viewable via list_charts/get_chart_info):
- pie, big_number, big_number_total, funnel, gauge_chart
- echarts_timeseries_line, echarts_timeseries_bar, echarts_timeseries_area
- pivot_table_v2, heatmap_v2, sankey_v2, sunburst_v2, treemap_v2
- word_cloud, world_map, box_plot, bubble, mixed_timeseries
Each chart returned by list_charts / get_chart_info includes a
chart_type_display_name field with a human-readable name when available.
This field is populated only for the 7 chart types supported by generate_chart
(xy, pie, table, pivot_table, big_number, mixed_timeseries, handlebars).
For all other viz_types (Funnel, Gauge, Heatmap, etc.) it will be null —
use the raw viz_type field instead when referring to those chart types.

Query Examples:
- List all tables:
Expand Down Expand Up @@ -670,6 +672,7 @@ def create_mcp_app(
# NOTE: Always add new prompt/resource imports here when creating new prompts/resources.
# Prompts use @mcp.prompt decorators and resources use @mcp.resource decorators.
# They register automatically on import, similar to tools.
import superset.mcp_service.chart.plugins # noqa: F401, E402 — registers all chart type plugins

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion: This unconditional top-level import makes MCP startup hard-fail if any chart plugin import raises, which bypasses the registry's built-in lazy-load/error-containment path and can take down unrelated MCP tools. Defer plugin bootstrap through the registry loader (or guard this import with controlled fallback) so a plugin import issue does not crash app initialization. [logic error]

Severity Level: Critical 🚨
- ❌ MCP server startup fails if any chart plugin import errors.
- ⚠️ All MCP tools unavailable, even unrelated to chart generation.
Steps of Reproduction ✅
1. Start the MCP server via `superset/mcp_service/server.py:903`, which calls
`init_fastmcp_server()` and imports `superset.mcp_service.app` at `server.py:15`.

2. During import of `superset/mcp_service/app.py`, module-level code at `app.py:116–118`
executes `mcp = create_mcp_app()`, constructing the default FastMCP instance before tool
imports.

3. After instance creation, module-level import at `superset/mcp_service/app.py:54` runs
`import superset.mcp_service.chart.plugins`, which in turn imports all plugin modules and
registers them in the registry via `superset/mcp_service/chart/plugins/__init__.py:41–48`.

4. Introduce an import-time error in any plugin module (for example, add `raise
RuntimeError("broken plugin")` at the top of `superset/mcp_service/chart/plugins/xy.py`),
then restart the MCP server: the unguarded import at `app.py:54` propagates the exception,
causing `superset.mcp_service.app` import to fail and preventing `init_fastmcp_server()`
and all MCP tools from loading, even though the lazy loader `_ensure_plugins_loaded()` in
`superset/mcp_service/chart/registry.py:73–99` is designed to catch and contain plugin
import failures.

Fix in Cursor Fix in VSCode Claude

(Use Cmd/Ctrl + Click for best experience)

Prompt for AI Agent 🤖
This is a comment left during a code review.

**Path:** superset/mcp_service/app.py
**Line:** 673:673
**Comment:**
	*Logic Error: This unconditional top-level import makes MCP startup hard-fail if any chart plugin import raises, which bypasses the registry's built-in lazy-load/error-containment path and can take down unrelated MCP tools. Defer plugin bootstrap through the registry loader (or guard this import with controlled fallback) so a plugin import issue does not crash app initialization.

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.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
👍 | 👎

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This eager import is intentional — it runs the plugin-registration side effects that populate the chart type registry at startup, following the same pattern as every other tool module imported unconditionally in app.py. The registry itself uses a circuit-breaker sentinel (_plugins_load_failed) to handle per-plugin import failures gracefully. If this import raises, it means the chart plugin package itself is broken, which is a real startup error that should surface rather than be swallowed.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Customized review instruction saved!

Instruction:

Do not flag eager top-level imports in superset/mcp_service/app.py when they are intentionally used to trigger registration side effects at startup; if the imported plugin package is broken, the startup failure should surface rather than be swallowed.

Applied to:

  • superset/mcp_service/**

💡 To manage or update this instruction, visit: CodeAnt AI Settings

from superset.mcp_service.annotation_layer.tool import ( # noqa: F401, E402
get_annotation_layer_info,
get_layer_annotation_info,
Expand Down
146 changes: 53 additions & 93 deletions superset/mcp_service/chart/chart_utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -353,29 +353,44 @@ def map_config_to_form_data(
| BigNumberChartConfig,
dataset_id: int | str | None = None,
) -> Dict[str, Any]:
"""Map chart config to Superset form_data."""
if isinstance(config, TableChartConfig):
return map_table_config(config)
elif isinstance(config, XYChartConfig):
return map_xy_config(config, dataset_id=dataset_id)
elif isinstance(config, PieChartConfig):
return map_pie_config(config)
elif isinstance(config, PivotTableChartConfig):
return map_pivot_table_config(config)
elif isinstance(config, MixedTimeseriesChartConfig):
return map_mixed_timeseries_config(config, dataset_id=dataset_id)
elif isinstance(config, HandlebarsChartConfig):
return map_handlebars_config(config)
elif isinstance(config, BigNumberChartConfig):
if config.show_trendline and config.temporal_column:
if not is_column_truly_temporal(config.temporal_column, dataset_id):
raise ValueError(
f"Big Number trendline requires a temporal SQL column; "
f"'{config.temporal_column}' is not temporal."
)
return map_big_number_config(config)
else:
raise ValueError(f"Unsupported config type: {type(config)}")
"""Map chart config to Superset form_data via the plugin registry.

The previous if/elif chain across all 7 chart types has been replaced by a
single registry lookup. Cross-field constraints (e.g. BigNumber trendline
temporal check) are now owned by each plugin's post_map_validate() method
rather than being baked into this dispatcher.
"""
# Local import: plugins call map_*_config from their to_form_data() methods,
# so chart_utils is loaded before plugins finish registering. A top-level
# import of registry here would trigger plugin loading mid-import = cycle.
from superset.mcp_service.chart.registry import get_registry
Comment thread
aminghadersohi marked this conversation as resolved.

chart_type = getattr(config, "chart_type", None)
plugin = get_registry().get(chart_type) if chart_type else None
Comment thread
aminghadersohi marked this conversation as resolved.

if plugin is None:
if chart_type is None:
raise ValueError(f"Unsupported config type: {type(config)}")
raise ValueError(
f"Unsupported config type: {type(config)} (chart_type={chart_type!r})"
)
Comment thread
aminghadersohi marked this conversation as resolved.

form_data = plugin.to_form_data(config, dataset_id=dataset_id)
Comment thread
aminghadersohi marked this conversation as resolved.

# Run post-map validation (e.g. BigNumber trendline temporal type check).
# Raise ValueError to preserve backward-compatible error handling in callers.
# Include details and suggestions so callers logging str(e) surface actionable
# context (e.g. BigNumber trendline guidance) rather than just the headline.
error = plugin.post_map_validate(config, form_data, dataset_id=dataset_id)
if error is not None:
parts = [error.message]
if error.details:
parts.append(error.details)
if error.suggestions:
parts.append("Suggestions: " + "; ".join(error.suggestions))
raise ValueError(" ".join(parts))

return form_data


def _add_adhoc_filters(
Expand Down Expand Up @@ -1279,87 +1294,32 @@ def _big_number_chart_what(config: BigNumberChartConfig) -> str:


def generate_chart_name(
config: TableChartConfig
| XYChartConfig
| PieChartConfig
| PivotTableChartConfig
| MixedTimeseriesChartConfig
| HandlebarsChartConfig
| BigNumberChartConfig,
config: Any,
dataset_name: str | None = None,
) -> str:
"""Generate a descriptive chart name following a standard format.

Format conventions (by chart type):
Aggregated (bar/scatter with group_by): [Metric] by [Dimension]
Time-series (line/area, no group_by): [Metric] Over Time
Table (no aggregates): [Dataset] Records
Table (with aggregates): [Metric] Summary
Pie: [Dimension] by [Metric]
Pivot Table: Pivot Table – [Row1, Row2]
Mixed Timeseries: [Primary] + [Secondary]
An en-dash followed by context (filters / time grain) is appended
Delegates to each plugin's ``generate_name()`` method.
See each plugin's ``generate_name`` for chart-type-specific format conventions.
An en-dash followed by context (filters / time grain) is appended by the plugin
when such information is available.
"""
if isinstance(config, TableChartConfig):
what = _table_chart_what(config, dataset_name)
context = _summarize_filters(config.filters)
elif isinstance(config, XYChartConfig):
what = _xy_chart_what(config)
context = _xy_chart_context(config)
elif isinstance(config, PieChartConfig):
what = _pie_chart_what(config)
context = _summarize_filters(config.filters)
elif isinstance(config, PivotTableChartConfig):
what = _pivot_table_what(config)
context = _summarize_filters(config.filters)
elif isinstance(config, MixedTimeseriesChartConfig):
what = _mixed_timeseries_what(config)
context = _summarize_filters(config.filters)
elif isinstance(config, HandlebarsChartConfig):
what = _handlebars_chart_what(config)
context = _summarize_filters(getattr(config, "filters", None))
elif isinstance(config, BigNumberChartConfig):
what = _big_number_chart_what(config)
context = _summarize_filters(getattr(config, "filters", None))
else:
return "Chart"
from superset.mcp_service.chart.registry import get_registry

name = what
if context:
name = f"{what} \u2013 {context}"
return _truncate(name)
plugin = get_registry().get(getattr(config, "chart_type", ""))
Comment thread
aminghadersohi marked this conversation as resolved.
if plugin is None:
return "Chart"
return _truncate(plugin.generate_name(config, dataset_name))


def _resolve_viz_type(config: Any) -> str:
"""Resolve the Superset viz_type from a chart config object."""
chart_type = getattr(config, "chart_type", "unknown")
if chart_type == "xy":
kind = getattr(config, "kind", "line")
viz_type_map = {
"line": "echarts_timeseries_line",
"bar": "echarts_timeseries_bar",
"area": "echarts_area",
"scatter": "echarts_timeseries_scatter",
}
return viz_type_map.get(kind, "echarts_timeseries_line")
elif chart_type == "table":
return getattr(config, "viz_type", "table")
elif chart_type == "pie":
return "pie"
elif chart_type == "pivot_table":
return "pivot_table_v2"
elif chart_type == "mixed_timeseries":
return "mixed_timeseries"
elif chart_type == "handlebars":
return "handlebars"
elif chart_type == "big_number":
show_trendline = getattr(config, "show_trendline", False)
temporal_column = getattr(config, "temporal_column", None)
return (
"big_number" if show_trendline and temporal_column else "big_number_total"
)
return "unknown"
from superset.mcp_service.chart.registry import get_registry

plugin = get_registry().get(getattr(config, "chart_type", ""))
Comment thread
aminghadersohi marked this conversation as resolved.
if plugin is None:
return "unknown"
return plugin.resolve_viz_type(config)


TABLE_VIZ_TYPE_LABELS = {
Expand Down
51 changes: 4 additions & 47 deletions superset/mcp_service/chart/compile.py
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,10 @@

from superset.commands.exceptions import CommandException
from superset.errors import SupersetErrorType
from superset.mcp_service.chart.validation.dataset_validator import DatasetValidator
from superset.mcp_service.chart.validation.dataset_validator import (
build_dataset_context_from_orm,
DatasetValidator,
)
from superset.mcp_service.common.error_schemas import (
ChartGenerationError,
ColumnSuggestion,
Expand Down Expand Up @@ -94,52 +97,6 @@ class CompileResult:
row_count: int | None = None


def build_dataset_context_from_orm(dataset: Any) -> DatasetContext | None:
"""Construct a ``DatasetContext`` from an already-fetched ORM dataset.

Mirrors :py:meth:`DatasetValidator._get_dataset_context` but skips the
``DatasetDAO.find_by_id`` round trip. Callers that have already loaded
the dataset (for permission checks, etc.) should use this instead.
"""
if dataset is None:
return None

columns: List[Dict[str, Any]] = []
for col in getattr(dataset, "columns", []) or []:
columns.append(
{
"name": col.column_name,
"type": str(col.type) if col.type else "UNKNOWN",
"is_temporal": getattr(col, "is_temporal", False),
"is_numeric": getattr(col, "is_numeric", False),
}
)

metrics: List[Dict[str, Any]] = []
for metric in getattr(dataset, "metrics", []) or []:
metrics.append(
{
"name": metric.metric_name,
"expression": metric.expression,
"description": metric.description,
}
)

database = getattr(dataset, "database", None)
# ``DatasetContext.database_name`` is typed as required ``str``; default to
# an empty string when the relationship isn't loaded so we don't blow up
# Pydantic validation. The field is purely informational in error messages.
database_name = getattr(database, "database_name", None) or ""
return DatasetContext(
id=dataset.id,
table_name=dataset.table_name,
schema=dataset.schema,
database_name=database_name,
available_columns=columns,
available_metrics=metrics,
)


def _compile_chart(
form_data: Dict[str, Any],
dataset_id: int,
Expand Down
Loading
Loading