feat(mcp): chart type plugin registry for extensible generate_chart

[aminghadersohi]

SUMMARY

Introduces a ChartTypePlugin protocol and central registry that consolidates all chart-type-specific logic into a single plugin class per chart type. Previously, adding a new chart type required synchronised edits to four separate dispatch locations. With this change it requires one new file.

The problem: 4 dispatch points that had to stay in sync

Before this PR, generate_chart logic was split across:

Location	Responsibility
schema_validator.py	7 _pre_validate_* static methods, one per type
dataset_validator.py	isinstance branches in _extract_column_references and normalize_column_names
chart_utils.py	if/elif chain in map_config_to_form_data, generate_chart_name, _resolve_viz_type
runtime/__init__.py	isinstance(config, XYChartConfig) hardcoded branch

Result: column name normalization (case-insensitive matching) was only wired for XY and Table (2 of 7). The 5 remaining types (pie, pivot_table, mixed_timeseries, handlebars, big_number) skipped normalization — case mismatches surfaced only at query time. Column existence checks were already in place for all 7 types via the previous dispatch table.

---

Architecture

New file layout

superset/mcp_service/chart/
├── plugin.py          # ChartTypePlugin Protocol + BaseChartPlugin base class
├── registry.py        # Central registry with lazy bootstrap
└── plugins/
    ├── __init__.py    # Imports and registers all 7 built-in plugins
    ├── xy.py
    ├── table.py
    ├── pie.py
    ├── pivot_table.py
    ├── mixed_timeseries.py
    ├── handlebars.py
    └── big_number.py

Request flow (before → after)

flowchart LR
    subgraph BEFORE["Before: 4 separate dispatch points"]
        direction TB
        A1[schema_validator.py\n7 pre_validate methods]
        A2[dataset_validator.py\nisinstance branches]
        A3[chart_utils.py\nif/elif chains]
        A4[runtime/__init__.py\nXY hardcoding]
    end

    subgraph AFTER["After: single registry lookup"]
        direction TB
        B1[registry.get chart_type] --> B2[plugin.pre_validate]
        B1 --> B3[plugin.extract_column_refs]
        B1 --> B4[plugin.to_form_data]
        B1 --> B5[plugin.post_map_validate]
        B1 --> B6[plugin.normalize_column_refs]
        B1 --> B7[plugin.get_runtime_warnings]
        B1 --> B8[plugin.generate_name]
        B1 --> B9[plugin.resolve_viz_type]
    end

    BEFORE -.->|refactored to| AFTER

Loading

Plugin protocol

classDiagram
    class ChartTypePlugin {
        <<Protocol>>
        +str chart_type
        +str display_name
        +dict native_viz_types
        +pre_validate(config) ChartGenerationError|None
        +extract_column_refs(config) list[ColumnRef]
        +to_form_data(config, dataset_id) dict
        +post_map_validate(config, form_data, dataset_id) ChartGenerationError|None
        +normalize_column_refs(config, dataset_context) Any
        +get_runtime_warnings(config, dataset_id) list[str]
        +generate_name(config, dataset_name) str
        +resolve_viz_type(config) str
        +schema_error_hint() ChartGenerationError|None
    }

    class BaseChartPlugin {
        +pre_validate() None
        +extract_column_refs() []
        +to_form_data() NotImplementedError
        +post_map_validate() None
        +normalize_column_refs() config unchanged
        +get_runtime_warnings() []
        +generate_name() "Chart"
        +resolve_viz_type() "unknown"
        +schema_error_hint() None
        +_with_context(what, context)$ str
    }

    class XYChartPlugin
    class PieChartPlugin
    class TableChartPlugin
    class BigNumberChartPlugin
    class PivotTableChartPlugin
    class MixedTimeseriesChartPlugin
    class HandlebarsChartPlugin

    ChartTypePlugin <|.. BaseChartPlugin
    BaseChartPlugin <|-- XYChartPlugin
    BaseChartPlugin <|-- PieChartPlugin
    BaseChartPlugin <|-- TableChartPlugin
    BaseChartPlugin <|-- BigNumberChartPlugin
    BaseChartPlugin <|-- PivotTableChartPlugin
    BaseChartPlugin <|-- MixedTimeseriesChartPlugin
    BaseChartPlugin <|-- HandlebarsChartPlugin

Loading

Registry bootstrap (lazy, no circular imports)

sequenceDiagram
    participant Caller as Caller (test / tool)
    participant Registry as registry.py
    participant Plugins as plugins/__init__.py

    Caller->>Registry: get("xy")
    Registry->>Registry: _ensure_plugins_loaded()
    alt first call
        Registry->>Plugins: import (triggers register() calls)
        Plugins-->>Registry: 7 plugins registered
    end
    Registry-->>Caller: XYChartPlugin instance

Loading

The registry self-bootstraps on first lookup — no dependency on app.py being imported first, so isolated unit tests work without the full Flask app.

Operator controls (runtime kill switch)

Two config knobs (defaults in mcp_config.py, overridable in superset_config.py) let operators filter chart plugins without code changes:

Config	Behavior
MCP_DISABLED_CHART_PLUGINS: frozenset[str]	Static deny-list — e.g. frozenset({"handlebars"}) as an emergency kill switch
MCP_CHART_PLUGIN_ENABLED_FUNC: Callable[[str], bool] | None	Dynamic per-call predicate (e.g. feature-flag backed); takes precedence over the deny-list and fails closed (plugin hidden) if it raises

registry.configure() is invoked solely from flask_singleton.py at two points: once after the base app config is loaded, and once after the standalone MCP service layers its own config overlay via get_mcp_config(). Both call sites are in flask_singleton.py — it is the registry's only configure site (noted in the file's inline comment). Filter state lives in an immutable dataclass swapped atomically, so concurrent readers never observe a torn (disabled, enabled_func) pair. A disabled chart type returns a structured DISABLED_CHART_TYPE error, distinct from INVALID_CHART_TYPE for unknown types.

Registry hardening

• register() runs under the registry lock (an RLock, so plugins registering during the lazy import don't deadlock) and warns when a plugin claims native_viz_types already owned by another plugin (display-name lookups resolve to the earlier registration).
• A failed plugin import trips a circuit breaker (_plugins_load_failed) and rolls back partially-registered plugins — subsequent lookups return None instead of re-attempting the import on every call.

---

How to add a new chart type

Everything goes in one new file. Here is a minimal working example for a hypothetical funnel chart:

Step 1 — Add the Pydantic schema (chart/schemas.py):

class FunnelChartConfig(ChartConfig):
    chart_type: Literal["funnel"] = "funnel"
    metric: ColumnRef
    groupby: ColumnRef
    filters: list[FilterConfig] | None = None

Step 2 — Create the plugin (chart/plugins/funnel.py):

from superset.mcp_service.chart.plugin import BaseChartPlugin
from superset.mcp_service.chart.schemas import ColumnRef
from superset.mcp_service.common.error_schemas import ChartGenerationError


class FunnelChartPlugin(BaseChartPlugin):
    chart_type = "funnel"
    display_name = "Funnel Chart"
    native_viz_types = {"funnel": "Funnel Chart"}

    def pre_validate(self, config: dict) -> ChartGenerationError | None:
        if "metric" not in config or "groupby" not in config:
            return ChartGenerationError(
                error_type="missing_funnel_fields",
                message="Funnel chart requires 'metric' and 'groupby'",
                suggestions=["Add metric: {'name': 'col', 'aggregate': 'SUM'}"],
                error_code="MISSING_FUNNEL_FIELDS",
            )
        return None

    def extract_column_refs(self, config) -> list[ColumnRef]:
        from superset.mcp_service.chart.schemas import FunnelChartConfig
        if not isinstance(config, FunnelChartConfig):
            return []
        refs = [config.metric, config.groupby]
        if config.filters:
            refs.extend(ColumnRef(name=f.column) for f in config.filters)
        return refs

    def to_form_data(self, config, dataset_id=None) -> dict:
        return {
            "viz_type": "funnel",
            "metric": {"column": {"column_name": config.metric.name},
                       "aggregate": config.metric.aggregate},
            "groupby": [config.groupby.name],
        }

    def resolve_viz_type(self, config) -> str:
        return "funnel"

    def generate_name(self, config, dataset_name=None) -> str:
        return self._with_context(
            f"Funnel: {config.metric.aggregate}({config.metric.name})",
            dataset_name,
        )

    def normalize_column_refs(self, config, dataset_context):
        from superset.mcp_service.chart.schemas import FunnelChartConfig
        from superset.mcp_service.chart.validation.dataset_validator import DatasetValidator
        d = config.model_dump()
        get = DatasetValidator._get_canonical_column_name
        d["metric"]["name"] = get(d["metric"]["name"], dataset_context)
        d["groupby"]["name"] = get(d["groupby"]["name"], dataset_context)
        DatasetValidator._normalize_filters(d, dataset_context)
        return FunnelChartConfig.model_validate(d)

Step 3 — Register it (chart/plugins/__init__.py):

from superset.mcp_service.chart.plugins.funnel import FunnelChartPlugin
# ...existing imports...
register(FunnelChartPlugin())

Step 4 (optional) — Custom Pydantic error hint: override schema_error_hint() in the same plugin file. SchemaValidator resolves hints through the registry when Pydantic fails to parse the config union, so no validator edits are needed:

    def schema_error_hint(self) -> ChartGenerationError | None:
        return ChartGenerationError(
            error_type="funnel_validation_error",
            message="Funnel chart configuration validation failed",
            suggestions=["Add metric: {'name': 'col', 'aggregate': 'SUM'}"],
            error_code="FUNNEL_VALIDATION_ERROR",
        )

That is the complete change set — no edits to chart_utils.py, dataset_validator.py, schema_validator.py, or runtime/__init__.py dispatch logic.

---

Bug fixes included

• 5-type column normalization gap: column name normalization was previously only wired for XY and Table. The 5 remaining types (pie, pivot_table, mixed_timeseries, handlebars, big_number) now normalize column case too. Column existence checks were already in place for all 7 types before this PR.
• BigNumber trendline check: Moved out of the monolithic dispatcher into BigNumberChartPlugin.post_map_validate().
• Runtime validator decoupling: Removed isinstance(config, XYChartConfig) hardcoding from RuntimeValidator; XY-specific format/cardinality checks now live in XYChartPlugin.get_runtime_warnings().
• Stale docstring: generate_chart.py listed only 'xy' and 'table' as valid chart types; updated to list all 7.
• Pydantic error handling: Added missing pie, pivot_table, mixed_timeseries cases for discriminated-union parse failures; hints now come from each plugin's schema_error_hint() instead of a hardcoded lookup table in the validator.
• Registry bootstrap: Registry self-bootstraps on first access so unit tests running without app.py get a fully populated registry.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

N/A — backend-only change, no UI modifications.

TESTING INSTRUCTIONS

# Run the MCP unit tests for chart functionality
pytest tests/unit_tests/mcp_service/chart/ -x -q

# Spot-check column validation now works for previously-unvalidated types
pytest tests/unit_tests/mcp_service/chart/validation/ -x -q

Manual verification:

1. Start the MCP service: python -m superset.mcp_service.server
2. Create a pie chart with a non-existent dimension column → should get a column_not_found error with fuzzy suggestions (previously: silent pass, runtime failure)
3. Create a big_number chart with show_trendline=true but no temporal_column → should error at validation
4. Run generate_chart for all 7 chart types to confirm no regressions

ADDITIONAL INFORMATION

Behavioral change: generate_chart and update_chart now explicitly reject any
chart_type value that does not map to one of the 7 registered plugins (xy, table,
pie, pivot_table, mixed_timeseries, handlebars, big_number). Previously,
unknown types fell through various if/elif chains and could reach later stages silently.
This tightening is intentional — the registry is the authoritative list of supported
types, and callers should receive a clear unknown_chart_type error rather than
undefined behaviour.

Column-name sanitization and PR #39915: This branch pre-dates PR #39915
(the dedicated column-regex relaxation PR). The registry refactor carries forward the
same intended security boundary: ColumnRef.name, FilterConfig.column, and
BigNumberChartConfig.temporal_column no longer use the overly strict ASCII regex,
but each field is sanitized at the schema boundary with SQL-pattern checks enabled.
This preserves support for real-world column names such as 1Q_revenue,
order-date, and events.created-at while still rejecting SQL-injection patterns.

Behavioral change — 5-type column validation gap: There is no separate
Shortcut/GitHub issue for this bug; it was found during the registry refactor. The
previous behavior was inconsistent across chart types: xy and table validated
column references, while pie, pivot_table, mixed_timeseries, handlebars, and
big_number could accept non-existent columns and fail later at query/runtime. This
PR intentionally makes validation consistent for all supported MCP chart types.

Scope note: The branch has been rebased onto latest master and de-scoped — an
earlier revision carried ~25 files of unrelated formatting/typing churn (advanced data
type plugins, various api.py files, unrelated tests) which have been restored to
match master, so the diff is limited to superset/mcp_service/, its tests, and the
both wiring points are in flask_singleton.py.

New plugins can be added at any time by creating a file in
superset/mcp_service/chart/plugins/ and importing it in
superset/mcp_service/chart/plugins/__init__.py.

• Has associated issue:
• Required feature flags:
• Changes UI
• Includes DB Migration
• Introduces new feature or API
• Removes existing feature or API

[netlify]

✅ Deploy Preview for superset-docs-preview ready!

Name	Link
🔨 Latest commit	b4e1f5d
🔍 Latest deploy log	https://app.netlify.com/projects/superset-docs-preview/deploys/6a4419a4a042e20008ee892f
😎 Deploy Preview	https://deploy-preview-39922--superset-docs-preview.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.
🤖 Make changes	Run an agent on this branch

---

To edit notification comments on pull requests, go to your Netlify project configuration.

[bito-code-review]

The inline context is insufficient to determine what 'this' refers to in the question. No diff hunk or suggestion snippet is provided for the thread in superset/mcp_service/chart/chart_utils.py.

[Copilot]

Pull request overview

This PR introduces a chart-type plugin architecture for the MCP chart pipeline, centralizing per-chart-type behavior (pre-validation, column extraction/normalization, form_data mapping, runtime warnings, naming, viz_type resolution) into one plugin per supported chart_type. This reduces the previous need to update multiple dispatch sites when adding or modifying chart types.

Changes:

• Added a ChartTypePlugin protocol + BaseChartPlugin, a global plugin registry, and 7 built-in chart plugins (xy, table, pie, pivot_table, mixed_timeseries, handlebars, big_number).
• Refactored schema/dataset/runtime validation and form_data mapping to dispatch via the registry instead of isinstance / if/elif chains.
• Enhanced tool/docs output by expanding generate_chart docstring chart types and adding chart_type_display_name to ChartInfo.

Reviewed changes

Copilot reviewed 17 out of 17 changed files in this pull request and generated 9 comments.

Show a summary per file

File	Description
superset/mcp_service/chart/validation/schema_validator.py	Switch pre-validation dispatch to registry/plugins; refactor Pydantic error enhancement.
superset/mcp_service/chart/validation/runtime/init.py	Replace XY-only runtime checks with per-plugin runtime warning dispatch.
superset/mcp_service/chart/validation/dataset_validator.py	Delegate column extraction/normalization to plugins; apply aggregation validation for all chart types.
superset/mcp_service/chart/tool/generate_chart.py	Update tool docstring to list all supported chart_type values.
superset/mcp_service/chart/schemas.py	Add chart_type_display_name to ChartInfo; resolve display name via registry mapping.
superset/mcp_service/chart/registry.py	Introduce global registry for chart plugins + viz_type display-name lookup.
superset/mcp_service/chart/plugin.py	Add plugin protocol and base class defining the plugin responsibilities.
superset/mcp_service/chart/plugins/init.py	Register the built-in plugins on import.
superset/mcp_service/chart/plugins/xy.py	Implement XY plugin (pre-validate, refs, mapping, naming, viz_type, runtime warnings).
superset/mcp_service/chart/plugins/table.py	Implement Table plugin.
superset/mcp_service/chart/plugins/pie.py	Implement Pie plugin.
superset/mcp_service/chart/plugins/pivot_table.py	Implement Pivot Table plugin.
superset/mcp_service/chart/plugins/mixed_timeseries.py	Implement Mixed Timeseries plugin.
superset/mcp_service/chart/plugins/handlebars.py	Implement Handlebars plugin.
superset/mcp_service/chart/plugins/big_number.py	Implement Big Number plugin including post-map trendline temporal validation.
superset/mcp_service/chart/chart_utils.py	Replace mapping/name/viz_type dispatch with registry/plugin calls.
superset/mcp_service/app.py	Import plugins at MCP app startup; update default instructions about chart type naming.

[codecov]

Codecov Report

❌ Patch coverage is 32.16080% with 540 lines in your changes missing coverage. Please review.
✅ Project coverage is 59.16%. Comparing base (805c12e) to head (2984a3a).
⚠️ Report is 33 commits behind head on master.

Files with missing lines	Patch %	Lines
superset/mcp_service/chart/registry.py	38.67%	61 Missing and 4 partials ⚠️
superset/mcp_service/chart/plugins/xy.py	28.57%	60 Missing ⚠️
superset/mcp_service/chart/plugins/big_number.py	27.39%	53 Missing ⚠️
...rset/mcp_service/chart/plugins/mixed_timeseries.py	27.14%	51 Missing ⚠️
superset/mcp_service/chart/plugins/handlebars.py	29.68%	45 Missing ⚠️
superset/mcp_service/chart/plugins/pivot_table.py	30.64%	43 Missing ⚠️
.../mcp_service/chart/validation/dataset_validator.py	16.66%	35 Missing ⚠️
superset/mcp_service/chart/plugins/pie.py	35.84%	34 Missing ⚠️
superset/mcp_service/chart/schemas.py	18.91%	30 Missing ⚠️
superset/mcp_service/chart/plugins/table.py	39.58%	29 Missing ⚠️
... and 7 more

❗ There is a different number of reports uploaded between BASE (805c12e) and HEAD (2984a3a). Click for more details.

HEAD has 58 uploads less than BASE

Flag	BASE (805c12e)	HEAD (2984a3a)
hive	6	1
python	35	6
presto	6	1
sqlite	6	1
unit	6	1
postgres	6	1
mysql	5	1

Additional details and impacted files

@@            Coverage Diff             @@
##           master   #39922      +/-   ##
==========================================
- Coverage   64.54%   59.16%   -5.39%     
==========================================
  Files        2665      820    -1845     
  Lines      146641    70060   -76581     
  Branches    33880     8759   -25121     
==========================================
- Hits        94652    41450   -53202     
+ Misses      50271    26874   -23397     
- Partials     1718     1736      +18     

Flag	Coverage Δ
hive	39.19% <32.16%> (+0.02%)	⬆️
javascript	?
mysql	57.68% <32.16%> (-0.19%)	⬇️
postgres	57.74% <32.16%> (-0.19%)	⬇️
presto	40.73% <32.16%> (+<0.01%)	⬆️
python	59.14% <32.16%> (-0.22%)	⬇️
sqlite	57.33% <32.16%> (-0.24%)	⬇️
unit	100.00% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[aminghadersohi]

Internal architecture review — findings & resolutions

Ran an independent codex review of the plugin registry design. Summary of findings and their status:

Finding	Severity	Status
_ensure_plugins_loaded() flag not lock-protected — race condition on concurrent first-calls	MEDIUM	✅ Fixed in eface3b — added threading.Lock with double-checked locking
Duplicate register() call silently overwrites	MEDIUM	Already handled — registry.py lines 68-71 emit logger.warning on overwrite, no silent overwrite
extract_column_refs() returns [] for unrecognised configs	MEDIUM	Accepted — correct for the 7 known types (plugins always registered); unknown types have no column schema to validate against
BaseChartPlugin.extract_column_refs() no-op default	NIT	Accepted — base class provides a safe default; concrete plugins override for their types
Local imports in chart_utils.py / dataset_validator.py	NIT	Already addressed in previous commit with explanatory comments on why they must stay local (circular dependency prevention)

The thread-safety fix (eface3bf) is the only code change — the others are either already handled or accepted design decisions.

[bito-code-review]

Code Review Agent Run #8680f9

Actionable Suggestions - 7

• superset/mcp_service/chart/schemas.py - 1

  • CWE-390: Bare Exception Handler · Line 491-496
• superset/mcp_service/chart/plugin.py - 1

  • Invalid type annotation string · Line 175-175
• superset/mcp_service/chart/tool/update_chart.py - 2

  • Redundant DB query in validation · Line 203-204
  • Missing test coverage for normalization · Line 416-432
• superset/mcp_service/chart/plugins/handlebars.py - 1

  • Duplicate validation with schema · Line 44-124
• tests/unit_tests/mcp_service/chart/validation/test_runtime_validator.py - 1

  • Test coverage regression · Line 213-234
• superset/mcp_service/chart/validation/runtime/__init__.py - 1

  • Avoid catching blind Exception · Line 107-107

Additional Suggestions - 6

• superset/initialization/__init__.py - 1

  • Missing error handling for MCP init · Line 750-750
    Wrap the call to `configure_mcp_chart_registry()` in a try-except block to catch any exceptions from `registry.configure()` and log them so that `init_app()` can continue even if MCP chart registry configuration fails.
• superset/mcp_service/chart/validation/schema_validator.py - 2

  • Import inside function body · Line 151-152
    The `from superset.mcp_service.chart.registry import get_registry` import is placed inside the function body at lines 151-152. This causes the import to execute on every call to `_pre_validate_chart_type`. Move it to the module-level imports for better performance.
    Code suggestion

    --- superset/mcp_service/chart/validation/schema_validator.py
    +++ superset/mcp_service/chart/validation/schema_validator.py
     @@ -29,6 +29,7 @@ from superset.mcp_service.chart.schemas import (
      )
      from superset.mcp_service.common.error_schemas import ChartGenerationError
    +from superset.mcp_service.chart.registry import get_registry
     
      logger = logging.getLogger(__name__)
     
     @@ -148,9 +149,6 @@ class SchemaValidator:
              config: Dict[str, Any],
          ) -> Tuple[bool, ChartGenerationError | None]:
              """Validate chart type and dispatch to plugin pre-validation."""
    -        from superset.mcp_service.chart.registry import get_registry
    -
              registry = get_registry()

  • Misleading variable name · Line 176-176
    Variable `valid_types` on line 176 is misleading — `registry.all_types()` returns only enabled chart types (per registry.py:168), so the name suggests all registered types when it actually contains only enabled ones. Rename to `enabled_types` for clarity.
    Code suggestion

    --- superset/mcp_service/chart/validation/schema_validator.py (lines 175-182)
    +++ superset/mcp_service/chart/validation/schema_validator.py
     @@ -173,12 +173,12 @@ class SchemaValidator:
                  )
     
              if not registry.is_enabled(chart_type):
    -            valid_types = ", ".join(registry.all_types())
    +            enabled_types = ", ".join(registry.all_types())
                  return False, ChartGenerationError(
                      error_type="disabled_chart_type",
                      message=f"Chart type '{chart_type}' is not enabled on this instance",
                      details=f"Chart type '{chart_type}' is registered but has been "
                      f"disabled by the operator. "
    -                f"Enabled chart types: {valid_types}",
    +                f"Enabled chart types: {enabled_types}",
                      suggestions=[
    -                    f"Use one of the enabled chart types: {valid_types}",
    +                    f"Use one of the enabled chart types: {enabled_types}",
                          "Contact your administrator if you believe this is an error",
                      ],
                      error_code="DISABLED_CHART_TYPE",
                  )

• superset/mcp_service/chart/plugins/xy.py - 3

  • Private member access in DatasetValidator · Line 114-114
    Accessing private method `_get_canonical_column_name`. Consider using a public API or adding a public wrapper method.
  • Private member access in DatasetValidator · Line 115-115
    Accessing private method `_get_canonical_metric_name`. Consider using a public API or adding a public wrapper method.
  • Private member access in DatasetValidator · Line 129-129
    Accessing private method `_normalize_filters`. Consider using a public API or adding a public wrapper method.

Filtered by Review Rules

Bito filtered these suggestions based on rules created automatically for your feedback. Manage rules.

• superset/mcp_service/chart/registry.py - 1

  • Broad exception catch · Line 87-87

Review Details

• Files reviewed - 27 · Commit Range: d3af275..f90f1cd

  • superset/initialization/__init__.py
  • superset/mcp_service/app.py
  • superset/mcp_service/chart/chart_utils.py
  • superset/mcp_service/chart/plugin.py
  • superset/mcp_service/chart/plugins/__init__.py
  • superset/mcp_service/chart/plugins/big_number.py
  • superset/mcp_service/chart/plugins/handlebars.py
  • superset/mcp_service/chart/plugins/mixed_timeseries.py
  • superset/mcp_service/chart/plugins/pie.py
  • superset/mcp_service/chart/plugins/pivot_table.py
  • superset/mcp_service/chart/plugins/table.py
  • superset/mcp_service/chart/plugins/xy.py
  • superset/mcp_service/chart/registry.py
  • superset/mcp_service/chart/schemas.py
  • superset/mcp_service/chart/tool/generate_chart.py
  • superset/mcp_service/chart/tool/update_chart.py
  • superset/mcp_service/chart/validation/dataset_validator.py
  • superset/mcp_service/chart/validation/runtime/__init__.py
  • superset/mcp_service/chart/validation/schema_validator.py
  • superset/mcp_service/flask_singleton.py
  • superset/mcp_service/mcp_config.py
  • tests/unit_tests/mcp_service/chart/test_big_number_chart.py
  • tests/unit_tests/mcp_service/chart/test_registry.py
  • tests/unit_tests/mcp_service/chart/test_registry_filters.py
  • tests/unit_tests/mcp_service/chart/tool/test_update_chart.py
  • tests/unit_tests/mcp_service/chart/validation/test_column_name_normalization.py
  • tests/unit_tests/mcp_service/chart/validation/test_runtime_validator.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

• Customize agent settings
• Review rules
• General documentation
• FAQ

AI Code Review powered by

[codeant-ai-for-open-source]

Suggestion: Add an explicit type annotation to this module-level proxy instance to comply with the variable type-hint rule. [custom_rule]

Severity Level: Minor ⚠️

Why it matters? 🤔

This is a module-level variable that can be annotated, and it is currently assigned without an explicit type hint. That is a valid type-hint rule violation.

(Use Cmd/Ctrl + Click for best experience)

Prompt for AI Agent 🤖

This is a comment left during a code review.

**Path:** superset/mcp_service/chart/registry.py
**Line:** 274:274
**Comment:**
	*Custom Rule: Add an explicit type annotation to this module-level proxy instance to comply with the variable type-hint rule.

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

👍 | 👎

[aminghadersohi]

Declining — Minor ⚠️ type annotation suggestion from automated bot review. Code passes mypy and pre-commit without these annotations; inferred types are unambiguous.

[fitzee]

Duplicate review — posted 3x due to a client-side retry bug on my end (each POST actually succeeded but the read-after-write check I ran looked empty, so I retried). Full review is at #pullrequestreview-4611453533. Apologies for the noise.

[fitzee]

Duplicate review — posted 3x due to a client-side retry bug on my end (each POST actually succeeded but the read-after-write check I ran looked empty, so I retried). Full review is at #pullrequestreview-4611453533. Apologies for the noise.

[fitzee]

Review (dual pass: Claude + Codex)

Re-reviewed from scratch given the PR has grown substantially since an earlier draft-stage pass (39 commits, now non-draft). All four earlier blocking concerns (registry write-lock, column-name sanitization vs #39915, the 5-type validation claim, and plugin-load retry storm) were re-verified against the current diff and live source rather than taken from the PR description at face value.

Verdict: Ship it with nits — two-way door (code-only, no schema/data migration), no CRITICAL/HIGH findings.

Resolved from the earlier pass

• register() now writes under _plugins_lock (RLock), covering the collision-check loop too. Well tested.
• The regex removal on ColumnRef.name / FilterConfig.column / BigNumberChartConfig.temporal_column is backed by sanitize_user_input(check_sql_keywords=True) on all three — confirmed by reading schemas.py directly. Security boundary intact.
• Plugin-load circuit breaker (_plugins_load_failed) added, plus rollback of partially-registered plugins on a failed import. Well tested.

One correction worth making to the PR description

The "5 of 7 chart types silently skipped column validation" framing doesn't match master: dataset_validator.py::_extract_column_references() and schema_validator.py's dispatch dict already had explicit branches for all 7 types before this PR — the pre-PR docstring even says so directly ("Covers every supported ChartConfig variant ... not just XY and table"). Column-existence validation for all 7 types was already live.

What's actually new (and a legitimate fix): normalize_column_names() previously only corrected column-name casing for 2 of 7 types (XY, Table); this PR extends that to all 7 via plugin.normalize_column_refs(). That's backward-compatible — normalization only fixes casing, it can't newly reject a valid column — so the risk here is much lower than "breaking behavioral change" implies. Worth updating the description so future readers aren't misled, but not a merge blocker.

Remaining nits

• MEDIUM — pivot_table.py's normalize_column_refs() (rows/columns/metrics) has no test coverage. It's the newly-normalized type with the most fields; worth a test before merge.
• LOW — XYChartPlugin.get_runtime_warnings() merges cardinality suggestions into the warnings list instead of keeping them separate (plugins/xy.py). On master, _validate_cardinality() returned warnings and suggestions as distinct lists that landed in separate response fields. The new ChartTypePlugin.get_runtime_warnings() -> list[str] protocol signature has no channel for suggestions at all, so every plugin is structurally stuck conflating the two. Worth widening the protocol return type (e.g. a small dataclass or (warnings, suggestions) tuple) before other plugins grow suggestion logic.
• LOW — the "Scope note" says ~25 unrelated formatting-churn files were restored to match master; 14 still remain in the diff (advanced_data_type/plugins/*, datasets/api.py, datasource/api.py, utils/excel.py, utils/mock_data.py, views/log/api.py, and a handful of unrelated test files). Small (a few lines each) but worth dropping to keep the diff scoped to the stated intent.
• LOW — the description says registry.configure() is wired from superset/initialization/__init__.py and flask_singleton.py; both call sites are actually in flask_singleton.py (whose own comment says it's "the registry's only configure site"). The wiring itself is correct and complete — verified all three branches of the app-boot logic — just the location claim is off.

What's sound

The plugin consolidation is the right call: four previously out-of-sync dispatch tables collapse into one ChartTypePlugin Protocol + BaseChartPlugin, the registry self-bootstraps lazily with no circular-import issues, and the operator kill-switch (MCP_DISABLED_CHART_PLUGINS / MCP_CHART_PLUGIN_ENABLED_FUNC) swaps its filter config atomically. The locking, sanitization, and circuit-breaker fixes since the last pass show real engineering, not minimal patches.

None of the above blocks merge. Recommend closing the pivot_table test gap and the warnings/suggestions split before merge, and tightening the description in a follow-up.

[gabotorresruiz]

LGTM!

[aminghadersohi]

Thanks @fitzee — all four items addressed in 9e55ba0:

MEDIUM — pivot_table.py test coverage:
Added TestNormalizePivotTableColumnRefs to test_column_name_normalization.py with 6 cases covering rows, columns, metrics, filters, saved_metric canonical rename, and sql_expression passthrough (no crash on name=None).

LOW — get_runtime_warnings() suggestions/warnings conflation:
Widened the ChartTypePlugin Protocol return type from list[str] to tuple[list[str], list[str]]. Updated BaseChartPlugin default, XYChartPlugin implementation (cardinality suggestions now go to the suggestions list, not warnings), and RuntimeValidator._validate_plugin_runtime caller to unpack the tuple. Updated the test mocks accordingly.

LOW — 14 formatting-churn files:
Restored all 7 remaining out-of-scope files to master state via git checkout origin/master: advanced_data_type/plugins/internet_address.py, internet_port.py, datasets/api.py, datasource/api.py, utils/excel.py, utils/mock_data.py, views/log/api.py.

LOW — PR description corrections:

• "silently skipped column validation" → "skipped normalization — case mismatches surfaced only at query time. Column existence checks were already in place for all 7 types"
• "5-type column validation gap" → "5-type column normalization gap"
• Removed initialization/__init__.py from the registry.configure() wiring claim; corrected to flask_singleton.py only