refactor(mcp): one plugin lifecycle contract and compact chart config schemas

[aminghadersohi]

SUMMARY

The MCP chart tools branch on viz_type in several shared modules: query construction (chart_helpers), result normalization (query_result), the compile check (compile), saved-chart previews (get_chart_preview), form-data previews (preview_utils), update merges (chart_utils.merge_chart_form_data) and the update tools' rebind rules. The saved and unsaved preview paths each keep their own copy of the per-chart dispatch. Every new chart type adds another branch to each of these, and the open chart-type PRs each add their own.

This PR puts that per-chart behavior behind one plugin lifecycle contract:

• Hooks on BaseChartPlugin, each with a default that keeps the shared behavior:
  • query construction: resolve_query_fields, build_query_dicts
  • result normalization, applied by compile, previews and (when normalize_data_results is set) get_chart_data and its CSV/XLSX exports: normalize_query_result
  • row limits: compile_row_limit, preview_row_limit
  • previews: ascii_preview, vega_lite_preview
  • update and rebind: resolve_update_config, merge_update_form_data, validate_merged_form_data
• Contract flags that replace the hard-coded viz-type lists: requires_compile_check, requires_config_for_dataset_rebind, unbound_form_data_is_rebind, normalize_data_results, allows_empty_result, resizes_saved_preview, supports_column_append, preview_note, invalid_result_error_code / invalid_result_message, and additional_viz_types for saved legacy or sibling viz types (bubble, pop_kpi).
• registry.plugin_for_viz_type() resolves the plugin that owns a saved chart's viz_type. It ignores runtime enablement, so disabling a type only stops new charts of that type from being created.
• One dispatcher per concern. Each shared tool calls that dispatcher instead of branching on viz type. Treemap, Gauge, Gantt, Big Number, Bubble, Mixed Timeseries, Table and Handlebars behavior moves into their plugins as it is.

Bug fix found by the contract suite: Histogram charts queried through the shared builder (get_chart_data, previews, compile) selected no columns and no metrics. The Histogram plugin now mirrors the frontend buildQuery and histogramOperator: [...groupby, column], a COUNT(*) metric when a HAVING filter is present, and histogram post-processing.

Other visible differences:

• Saved-chart Vega-Lite previews honor the plugin’s allows_empty_result flag before rendering. Saved Bubble and Histogram charts with zero rows return the same empty specs that their unsaved previews return, where they used to return NoDataError.
• The rebind error for Gauge now uses the plugin display name: "Gauge Chart dataset rebind requires a complete Gauge Chart config."
• resolve_metrics(form_data, viz_type) returns Big Number's singular metric, matching resolve_metrics_and_groupby and the executed query.

Registry-wide regression suite (test_chart_plugin_contract.py). It is parametrized over the live registry, so it covers every chart type registered now and any added later. Each registered type must:

• implement every hook and declare typed flags;
• publish at least one schema example;
• own its viz types uniquely, and keep owning them when disabled;
• round-trip the viz type it maps back to itself;
• build non-empty, well-formed queries, and agree with its own build_query_dicts;
• handle malformed and failed result envelopes without raising or mutating them;
• return a typed preview or error from every preview format;
• render saved and unsaved Vega-Lite previews from the same renderer;
• keep newly mapped query roles in a same-viz update;
• drop stale roles and filters on a dataset rebind;
• drop every saved control when the viz type changes.

An AST guard also fails if a shared dispatcher compares a viz or chart type against a registered chart type, so new chart types have to use hooks.

Compact chart config in tool schemas. generate_chart, update_chart, update_chart_preview and generate_explore_link used to inline every chart type's JSON Schema, which added several kB per chart type and left generate_chart at 49,531 of its 50,000-byte budget. config is now advertised as a compact discriminated reference: an object whose chart_type enum is derived from the ChartConfig union, with a description pointing to get_chart_type_schema(chart_type), which already serves each type's schema and examples. Server-side validation still uses the full discriminated union.

tool	before (bytes)	after (bytes)
generate_chart	49,531	2,389
update_chart	53,395	4,076
update_chart_preview	52,409	2,003
generate_explore_link	49,230	1,836

The inventory budgets follow the small-tool snapshot rule and are measured without the registry-derived chart_type enum. Adding a chart type therefore needs no budget change, while any inlined per-type schema still fails the budget. A test also asserts that no *ChartConfig schema is inlined into these tools, and that every advertised chart_type is served by get_chart_type_schema. UPDATING.md documents the change for MCP clients.

Gateway search page. MCP gateways deliver a tool-search page of up to max_results (5) entries and cap it at 100 KB. With chart config schemas inlined, a page holding update_chart (53,395 B) and generate_chart (49,214 B) already exceeded the cap, and on master the five largest entries total about 210 KB. With the compact schema, the largest possible page is 25,350 B. test_worst_case_search_page_fits_gateway_limit fails on master and passes here.

Acceptance fixes in hook form (same behavior as #44744).

• Waterfall queries select the x-axis category plus breakdown, ordered by those columns.
• Histogram Vega-Lite previews plot bin counts instead of re-binning them.
• Funnel renders a stage-bar preview; Sankey and Radar return an explicit unsupported-geometry error, through a shared fallback used by both preview paths.

#44744's own tests pass unchanged against this implementation.

Out of scope: Jinja g.form_data seeding in the compile, preview and form-data get_chart_data paths (the gap reported for #40570) is handled by #43176. That PR also carries the approach from #43711 by @Abdulrehman-PIAIC80387. This PR does not touch those call sites, so the two merge independently.

TESTING INSTRUCTIONS

pytest tests/unit_tests/mcp_service tests/unit_tests/charts \
  tests/unit_tests/common/test_form_data_query_context.py \
  tests/unit_tests/common/test_query_context_factory.py \
  tests/unit_tests/common/test_query_context_processor.py \
  tests/unit_tests/common/test_query_context_processor_timing.py

Locally: 6460 passed, 4 skipped, 2 xfailed. The new Histogram test fails on master (the query selects only ['region']) and passes with this change.

Manual check: call get_chart_data on a saved Histogram chart. It returns binned rows instead of failing on an empty query. Treemap, Gauge and Gantt previews, update_chart and update_chart_preview behave as before.

ADDITIONAL INFORMATION

• Has associated issue:
• Required feature flags:
• Changes UI
• Includes DB Migration (follow approval process in SIP-59)
  • Migration is atomic, supports rollback & is backwards-compatible
  • Confirm DB migration upgrade and downgrade tested
  • Runtime estimates and downtime expectations provided
• Introduces new feature or API
• Removes existing feature or API

[netlify]

✅ Deploy Preview for superset-docs-preview ready!

Name	Link
🔨 Latest commit	56f5165
🔍 Latest deploy log	https://app.netlify.com/projects/superset-docs-preview/deploys/6abcf01342c72a000805afd3
😎 Deploy Preview	https://deploy-preview-44746--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 issue is that int() is called on row_limit without validation, which can cause unhandled exceptions when the value is malformed or negative. To resolve this, you should add a check to ensure the value is a positive integer before conversion.

# Example fix for gauge.py
row_limit_raw = form_data.get("row_limit")
row_limit = int(row_limit_raw) if str(row_limit_raw).isdigit() and int(row_limit_raw) > 0 else DEFAULT_LIMIT

I have checked the available PR context, and there are no other comments in the provided pr_comments.csv file to address. Would you like me to help with anything else?

superset/mcp_service/chart/plugins/gauge.py

row_limit_raw = form_data.get("row_limit")
row_limit = int(row_limit_raw) if str(row_limit_raw).isdigit() and int(row_limit_raw) > 0 else DEFAULT_LIMIT

[codecov]

Codecov Report

❌ Patch coverage is 93.14516% with 34 lines in your changes missing coverage. Please review.
✅ Project coverage is 81.65%. Comparing base (3e8cec5) to head (56f5165).
⚠️ Report is 1 commits behind head on master.

Files with missing lines	Patch %	Lines
superset/mcp_service/chart/plugin.py	86.66%	10 Missing ⚠️
superset/mcp_service/chart/preview_utils.py	88.88%	3 Missing and 3 partials ⚠️
superset/mcp_service/chart/plugins/treemap.py	91.48%	3 Missing and 1 partial ⚠️
superset/mcp_service/chart/plugins/histogram.py	88.88%	2 Missing and 1 partial ⚠️
superset/mcp_service/chart/plugins/gantt.py	92.59%	1 Missing and 1 partial ⚠️
superset/mcp_service/chart/plugins/gauge.py	92.59%	1 Missing and 1 partial ⚠️
superset/mcp_service/chart/tool/get_chart_data.py	84.61%	2 Missing ⚠️
superset/mcp_service/chart/chart_utils.py	96.96%	0 Missing and 1 partial ⚠️
superset/mcp_service/chart/registry.py	95.45%	1 Missing ⚠️
superset/mcp_service/chart/schemas.py	95.83%	0 Missing and 1 partial ⚠️
... and 2 more

Additional details and impacted files

@@            Coverage Diff             @@
##           master   #44746      +/-   ##
==========================================
+ Coverage   81.58%   81.65%   +0.06%     
==========================================
  Files        2977     2977              
  Lines      180831   181146     +315     
  Branches    41850    41858       +8     
==========================================
+ Hits       147534   147906     +372     
+ Misses      30574    30514      -60     
- Partials     2723     2726       +3     

Flag	Coverage Δ
hive	36.68% <34.07%> (+0.01%)	⬆️
mysql	55.86% <34.07%> (-0.06%)	⬇️
postgres	55.87% <34.07%> (-0.06%)	⬇️
presto	38.59% <34.07%> (+<0.01%)	⬆️
python	85.83% <93.14%> (+0.10%)	⬆️
sqlite	55.60% <34.07%> (-0.05%)	⬇️
unit	78.64% <93.14%> (+0.12%)	⬆️

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.

[bito-code-review]

Code Review Agent Run #d830d4

Actionable Suggestions - 3

• superset/mcp_service/chart/tool/update_chart.py - 1

  • CWE-470: Disabled plugin skips config completion · Line 872-876
• superset/mcp_service/chart/plugins/gantt.py - 1

  • Uncaught ValueError · Line 260-268
• superset/mcp_service/chart/plugins/treemap.py - 1

  • CWE-20: Unguarded int() row limit · Line 216-217

Additional Suggestions - 16

• superset/mcp_service/chart/plugins/mixed_timeseries.py - 1

  • Missing series_columns · Line 193-208
    The frontend `buildQuery.ts` sets `series_columns: fd.groupby` on both queries, but these dicts omit it. `QueryObject._init_series_columns` then defaults `series_columns` to `[]` (is_timeseries unset), so the pivot operator falls back to `columns` (which includes the x-axis) instead of groupby — diverging from the frontend series grouping. Consider setting `series_columns` on each query.
• superset/mcp_service/chart/chart_utils.py - 2

  • Undocumented inline import · Line 985-985
    The inline `from superset.mcp_service.chart.registry import plugin_for_viz_type` has no comment justifying why it is function-scoped. The same file documents the reason for the identical import at `map_config_to_form_data` (chart_utils.py:370-371): a module-level registry import triggers plugin loading mid-import and cycles. Per the repo's inline-import rule, add the same explanatory comment here.
  • Unreachable rebind fallback · Line 994-997
    For Gantt/Gauge/Treemap configs the fallback is unreachable: line 983 guarantees same viz_type, so `plugin_for_viz_type` resolves the owning plugin, and `GanttPlugin`/`GaugePlugin`/`TreemapPlugin.merge_update_form_data` return non-None whenever `isinstance(config, ...)` matches (plugins/gantt.py:285-292, plugins/gauge.py:246-251, plugins/treemap.py:269-274). The base-class None return (plugin.py:449) only applies to non-Gantt/Gauge/Treemap configs, so `return dict(new_form_data)` on dataset rebind can never run for these types.
• superset/mcp_service/chart/plugins/treemap.py - 2

  • Undocumented local imports · Line 185-188
    Function-local imports in `build_query_dicts`, `normalize_query_result`, `ascii_preview`, `vega_lite_preview`, `resolve_update_config`, and `merge_update_form_data` carry no explanatory comment, which BITO rule 12745 requires when a local import avoids a cycle (`chart_helpers.py:773` shows the documented pattern). Add the comment or hoist the imports if no cycle exists.
  • Missing docstrings on hooks · Line 165-174
    The nine new hook overrides (`resolve_query_fields`, `build_query_dicts`, `normalize_query_result`, `compile_row_limit`, `preview_row_limit`, `ascii_preview`, `vega_lite_preview`, `resolve_update_config`, `merge_update_form_data`) have no docstrings, unlike the base hooks they override in `plugin.py` (e.g. plugin.py:233-237). BITO rule 12147 requires docstrings on all new Python functions; add a one-liner per method.
• superset/mcp_service/chart/plugins/histogram.py - 2

  • duplicated column resolution · Line 235-245
    Column/groupby resolution is split across the two new methods: `resolve_query_fields` builds `columns` from `form_data`, then `build_query_dicts` re-derives the column label via `column_result_label(form_data.get("column"))` and the groupby labels from `columns[:-1]`. This duplicates the extraction logic and couples `build_query_dicts` to the invariant that the binned column is always last in `columns`. If that ordering or the column shape changes, the two can diverge.
  • magic default bin count · Line 238-240
    The default bin count `5` is hardcoded twice here (lines 238 and 240), duplicating the schema default `bins: int = Field(5, ...)` in `schemas.py`. If the default ever changes, these two spots and the schema can drift. Extract a module-level `DEFAULT_HISTOGRAM_BINS` constant and use it in both the `get` default and the `except` fallback.
• superset/mcp_service/chart/tool/update_chart.py - 1

  • Generic hook, Gantt-specific catch · Line 591-596
    The try body now dispatches to any plugin's `validate_merged_form_data`, but the following `except` (line 597, unchanged) catches only `GanttSemanticNormalizationError` with a Gantt-specific message, while the protocol documents plain `ValueError` (plugin.py). Today only the Gantt plugin overrides the hook, so this is future-proofing: a second overriding plugin's ValueError would surface via the generic outer handler instead.
• superset/mcp_service/chart/registry.py - 3

  • Duplicated registry scan · Line 208-213
    The two loops over `list(_REGISTRY.values())` (lines 208-210 and 211-213) duplicate the same scan-and-return shape, differing only in the membership test. One loop checking `native_viz_types` then `additional_viz_types` per plugin keeps the documented native-over-additional priority while removing the duplicated traversal.
  • Redundant getattr fallback · Line 212-212
    The base class declares `additional_viz_types: ClassVar[frozenset[str]] = frozenset()` (plugin.py:327), so the `getattr(..., ())` fallback can never fire for any registered `ChartTypePlugin` and wrongly suggests the attribute may be absent. Access it directly like `native_viz_types` on line 209 so mypy validates it.
  • Missing proxy docstring · Line 311-314
    `_RegistryProxy.all_plugins` is the only method on the proxy without a docstring, while its siblings (`get`, `all_types`, `is_registered`, `display_name_for_viz_type`, `plugin_for_viz_type`) and the module-level functions document their contracts. Add the one-line docstring to keep the proxy self-documenting and satisfy the repo docstring rule.
• superset/mcp_service/chart/compile.py - 1

  • Dead fallback branches · Line 138-146
    The `else BaseChartPlugin.invalid_result_error_code` / `else BaseChartPlugin.invalid_result_message` fallbacks are unreachable. When `plugin` is None, `normalize_chart_query_result` returns `result` unchanged (query_result.py:281-284) and `ChartDataCommand.run()` returns a dict, so `isinstance(result, ChartError)` can never be true with plugin None. Use `plugin.invalid_result_error_code` / `plugin.invalid_result_message` directly.
• superset/mcp_service/chart/plugins/gantt.py - 1

  • Dead plugin flag · Line 53-53
    `allows_empty_result = True` has no consumer anywhere in the repo — only the base-class declaration (plugin.py:215/332) and a contract test assert it exists. Unlike `resizes_saved_preview` (read at get_chart_preview.py:414), nothing reads this flag, so it silently does nothing. Either delete it or add the consumer it was meant to enable.
• superset/mcp_service/chart/plugins/big_number.py - 2

  • ignored param, empty columns · Line 253-262
    `resolve_query_fields` never reads `viz_type` and always returns `([metric], [])`. For the trendline viz the real columns come from `resolve_big_number_columns` in `build_query_dicts`, so the `(metrics, columns)` contract here is misleading for `big_number`. Currently unreachable on a live path (shared builder only handles `big_number_total`), but a latent trap for future callers.
  • duplicate viz-type literal · Line 46-46
    `TRENDLINE_VIZ_TYPE` (line 46) is defined but `resolve_viz_type` (line 208) still returns the literal `"big_number"`. Two sources of truth for the same viz type will silently diverge if the constant changes. Reference `self.TRENDLINE_VIZ_TYPE` in `resolve_viz_type`.
• superset/mcp_service/chart/plugin.py - 1

  • Disallow dynamic typing (Any) in public API · Line 251-251
    `typing.Any` is disallowed in public method signatures. This occurs in multiple methods (lines 251, 277, 281, 289, 301, 415, 436, 440, 447, 458). Consider using more specific types or add `# noqa: ANN401` if the use is intentional.

Review Details

• Files reviewed - 25 · Commit Range: 8d95326..8d95326

  • docs/admin_docs/configuration/mcp-server.mdx
  • superset/mcp_service/chart/chart_helpers.py
  • superset/mcp_service/chart/chart_utils.py
  • superset/mcp_service/chart/compile.py
  • superset/mcp_service/chart/plugin.py
  • superset/mcp_service/chart/plugins/big_number.py
  • superset/mcp_service/chart/plugins/bubble.py
  • superset/mcp_service/chart/plugins/gantt.py
  • superset/mcp_service/chart/plugins/gauge.py
  • superset/mcp_service/chart/plugins/handlebars.py
  • superset/mcp_service/chart/plugins/histogram.py
  • superset/mcp_service/chart/plugins/mixed_timeseries.py
  • superset/mcp_service/chart/plugins/table.py
  • superset/mcp_service/chart/plugins/treemap.py
  • superset/mcp_service/chart/preview_utils.py
  • superset/mcp_service/chart/query_result.py
  • superset/mcp_service/chart/registry.py
  • superset/mcp_service/chart/tool/get_chart_data.py
  • superset/mcp_service/chart/tool/get_chart_preview.py
  • superset/mcp_service/chart/tool/update_chart.py
  • superset/mcp_service/chart/tool/update_chart_preview.py
  • tests/unit_tests/mcp_service/chart/test_chart_plugin_contract.py
  • tests/unit_tests/mcp_service/chart/test_histogram_boxplot_charts.py
  • tests/unit_tests/mcp_service/chart/test_treemap_completeness.py
  • tests/unit_tests/mcp_service/chart/tool/test_update_chart.py
• Files skipped - 0
• Tools

  • MyPy (Static Code Analysis) - ✔︎ Successful
  • Astral Ruff (Static Code Analysis) - ✔︎ Successful
  • Whispers (Secret Scanner) - ✔︎ Successful
  • Detect-secrets (Secret Scanner) - ✔︎ Successful

---

Bito Usage Guide

Commands

Type the following command in the pull request comment and save the comment.

• /review - Manually triggers an incremental AI Review.

• /review full - 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

[bito-code-review]

Code Review Agent Run #bd88ea

Actionable Suggestions - 4

• superset/mcp_service/chart/plugins/treemap.py - 1

  • Magic numbers encode limit policy · Line 219-222
• tests/unit_tests/mcp_service/chart/tool/test_get_chart_data.py - 2

  • Missing tooltip_metrics case · Line 3496-3502
  • Inconsistent mock pattern · Line 3509-3512
• superset/mcp_service/chart/preview_utils.py - 1

  • Unguarded ValueError from get_column_name · Line 1309-1309

Additional Suggestions - 11

• superset/mcp_service/chart/tool/update_chart.py - 1

  • Inconsistent disabled-plugin policy · Line 372-372
    This change lets saved-chart config updates use disabled plugins (matching `registry.get`'s 'saved-chart updates may include disabled plugins' contract and `update_chart_preview.py`), but the sibling `map_config_to_form_data(TableChartConfig(...))` call in `_append_table_columns` (line 188) still omits `include_disabled=True`. With `MCP_DISABLED_CHART_PLUGINS={'table'}`, `add_columns` on a saved table chart raises ValueError and surfaces as 'Chart update failed'. Pass include_disabled=True there too.
• superset/mcp_service/chart/tool/get_chart_preview.py - 1

  • Missing tests for fallback dispatch · Line 509-512
    The new `fallback_vega_lite_preview` dispatch in `VegaLitePreviewGenerator.generate` has no test coverage: no test under `tests/unit_tests/mcp_service/chart/` references `fallback_vega_lite_preview`, the funnel renderer, or the sankey/radar UnsupportedFormat error (verified by grep). Add saved-chart tests for funnel, unsupported geometries, and generic-spec fall-through per the repo testing rule. ([BITO.md])
• superset/mcp_service/chart/preview_utils.py - 1

  • Inline import violates rule · Line 1301-1301
    Both new functions import `get_column_name` inline (lines 1301, 1335). `superset.utils.core` is already imported at module scope elsewhere in this service with no circularity, and repo rule 12745 requires module-level imports unless a documented circular dependency exists. Hoist the import to the top-level import block and delete the two inline copies.
• superset/mcp_service/chart/registry.py - 1

  • Cryptic docstring on proxy get · Line 299-299
    The `_RegistryProxy.get` docstring "optionally retaining disabled update contracts" is garbled — it omits what is retained and obscures that `include_disabled=True` bypasses the `_is_plugin_enabled` filter (including `is_available()` and the operator `enabled_func`). The module-level `get` docstring (line 218) states this clearly; align the proxy wording with it so callers of the shared `_PROXY` don't misuse the filter bypass.
• tests/unit_tests/mcp_service/test_chart_tool_inventory.py - 1

  • Missing local type annotations · Line 163-164
    New locals `text` and `grown` in `test_budget_is_independent_of_chart_type_count` lack explicit type annotations. BITO adaptive rule 13153 requires annotations on all test-file locals even when inferable; adding `: str` keeps the new test aligned with the repo typing standard and mypy-friendly.
• superset/mcp_service/chart/schemas.py - 3

  • Fragile type fallback · Line 3713-3713
    The `or (config_type,)` fallback cannot rescue the inputs it appears to handle: for a bare model class, `get_args(config_type)[0]` raises IndexError first, and a bare union dies on `model_fields` (both verified empirically). The fallback only serves Annotated-wrapped single models, which the `Any`-typed signature does not suggest. Consider guarding the unwrap or documenting that only Annotated unions are supported.
  • Shared mutable schema dict · Line 3731-3731
    `CHART_CONFIG_REFERENCE_SCHEMA` (and its `CHART_TYPE_VALUES` list) is a module-level mutable object embedded by reference into every tool schema via `WithJsonSchema` at lines 3742-3743. If any schema post-processing mutates the dict, all four chart tools are corrupted together. Returning a per-call deep copy would isolate them; no in-repo mutator was found, so this is precautionary.
  • Discriminator name coupling · Line 3713-3713
    `model_fields["chart_type"]` hardcodes the discriminator name that `ChartConfig` declares separately via `Field(discriminator="chart_type")` (line 3697). If either side changes, the module fails at import with an unhelpful KeyError. A shared constant for the discriminator field name would tie the introspection to the declaration.
• superset/mcp_service/chart/tool/get_chart_data.py - 2

  • Silent ValidationError path · Line 1069-1070
    The new `QueryObjectValidationError` handler in `execute_chart_data` returns silently, while every sibling terminal handler logs (`ctx.error` + `logger.error` at 1072-1080) and even the client-caused OAuth path logs at info/warning (1091). Add a `logger.warning` with `chart_id` so validation failures are diagnosable; same gap in `_query_from_form_data`.
  • Silent ValidationError path · Line 1325-1326
    Same gap in `_query_from_form_data`: the new `QueryObjectValidationError` handler returns a `ChartError` with no log, while the sibling tuple handler logs via `logger.error` (1327-1328). Since this path returns rather than raises, middleware never sees it. Add a `logger.warning` so unsaved-chart validation failures leave a trace; mirrors the handler at 1069-1070.
• superset/mcp_service/chart/plugins/gauge.py - 1

  • Bool row_limit bypasses fallback · Line 216-216
    `int()` accepts `bool` (bool subclasses int), so a persisted `row_limit: true` becomes 1 instead of falling back to 10 like the other malformed values this method normalizes (see `test_compile_row_limit_handles_persisted_values`, which feeds lists/dicts/strings). Exclude bools explicitly before converting.

Filtered by Review Rules

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

• tests/unit_tests/mcp_service/chart/test_treemap_completeness.py - 1

  • Duplicated plugin-filter monkeypatch · Line 712-720
• superset/mcp_service/chart/plugins/treemap.py - 1

  • Duplicated row-limit logic · Line 218-222
• superset/mcp_service/chart/plugins/gauge.py - 1

  • Duplicated row-limit clamp · Line 215-219

Review Details

• Files reviewed - 22 · Commit Range: 8d95326..49b2e0a

  • superset/mcp_service/chart/plugin.py
  • superset/mcp_service/chart/plugins/gantt.py
  • superset/mcp_service/chart/plugins/gauge.py
  • superset/mcp_service/chart/plugins/treemap.py
  • superset/mcp_service/chart/tool/update_chart.py
  • tests/unit_tests/mcp_service/chart/test_chart_plugin_contract.py
  • superset/mcp_service/chart/schemas.py
  • tests/unit_tests/mcp_service/test_chart_tool_inventory.py
  • tests/unit_tests/mcp_service/test_tool_inventory.py
  • superset/mcp_service/chart/plugins/histogram.py
  • superset/mcp_service/chart/plugins/waterfall.py
  • superset/mcp_service/chart/preview_utils.py
  • superset/mcp_service/chart/tool/get_chart_preview.py
  • docs/admin_docs/configuration/mcp-server.mdx
  • superset/mcp_service/chart/chart_utils.py
  • superset/mcp_service/chart/registry.py
  • superset/mcp_service/chart/tool/get_chart_data.py
  • superset/mcp_service/chart/tool/update_chart_preview.py
  • superset/mcp_service/chart/validation/dataset_validator.py
  • tests/unit_tests/mcp_service/chart/test_registry_filters.py
  • tests/unit_tests/mcp_service/chart/test_treemap_completeness.py
  • tests/unit_tests/mcp_service/chart/tool/test_get_chart_data.py
• Files skipped - 1

  • UPDATING.md - Reason: Filter setting
• Tools

  • MyPy (Static Code Analysis) - ✔︎ Successful
  • Astral Ruff (Static Code Analysis) - ✔︎ Successful
  • Whispers (Secret Scanner) - ✔︎ Successful
  • Detect-secrets (Secret Scanner) - ✔︎ Successful

---

Bito Usage Guide

Commands

Type the following command in the pull request comment and save the comment.

• /review - Manually triggers an incremental AI Review.

• /review full - 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

[bito-code-review]

Code Review Agent Run #b9da27

Actionable Suggestions - 0

Additional Suggestions - 1

• tests/unit_tests/mcp_service/chart/test_preview_utils.py - 1

  • Inline imports in tests · Line 281-281
    The new funnel tests import `ChartError` (line 281) and `VegaLitePreview` (line 296) inside the function bodies, but this module already imports `TablePreview` from the same `schemas` module at line 30, so there is no circular-dependency justification. Hoist both to the module-level import block for consistency.

Review Details

• Files reviewed - 6 · Commit Range: 49b2e0a..21f20f0

  • superset/mcp_service/chart/registry.py
  • tests/unit_tests/mcp_service/chart/test_registry.py
  • docs/admin_docs/configuration/mcp-server.mdx
  • superset/mcp_service/chart/preview_utils.py
  • tests/unit_tests/mcp_service/chart/test_preview_utils.py
  • tests/unit_tests/mcp_service/chart/tool/test_get_chart_data.py
• Files skipped - 0
• Tools

  • MyPy (Static Code Analysis) - ✔︎ Successful
  • Astral Ruff (Static Code Analysis) - ✔︎ Successful
  • Whispers (Secret Scanner) - ✔︎ Successful
  • Detect-secrets (Secret Scanner) - ✔︎ Successful

---

Bito Usage Guide

Commands

Type the following command in the pull request comment and save the comment.

• /review - Manually triggers an incremental AI Review.

• /review full - 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

[aminghadersohi]

Commit eb08fb8 ports the #44148 fix (19c44fe) for the same saved-params scalar-groupby finding, including shared Gauge/Treemap compile limits. Regression tests reproduce three scalar failures without the fix and pass with it. MCP chart unit tests: 2619 passed, 3 skipped; pre-commit passed.

[rebenitez1802]

Request changes (minor): strong, well-tested refactor — security model intact, and the Histogram query fix is faithful to the frontend — but one brand-new contract flag ships as dead code, and the PR is currently in merge conflict with master.

I reviewed all 35 files, diffing PR-vs-master per concern, with a focus on the areas this refactor puts at risk.

Verified clean:

• Dataset rebind does not leak query roles/columns across datasets — requires_config_for_dataset_rebind / strict_dataset_rebind / unbound_form_data_is_rebind correctly stop stale roles from crossing, and the actor must already hold update access to the chart plus access to the target dataset.
• Histogram query fix matches the frontend buildQuery / histogramOperator exactly: columns = [...groupby, column], a COUNT(*) metric only when a HAVING filter is present, and histogram post-processing.
• get_chart_data normalization across JSON/CSV/XLSX and the compact-schema / full-union-validation split hold up.

---

🟡 Medium — allows_empty_result is a dead contract flag

In get_chart_preview.py (VegaLitePreviewStrategy.generate(), ~494–508) the plugin short-circuit

if (plugin_preview := self._create_plugin_preview(chart_data, form_data)) is not None:
    return plugin_preview

runs before the if not chart_data: return NoDataError guard. So the empty-vs-error decision is decided implicitly by whether a plugin overrides vega_lite_preview — not by the new allows_empty_result flag, which no production code reads (plugin.py:229/:347 declare it, gantt.py:53 sets it True, and only the contract test references it; the sibling resizes_saved_preview is consumed at get_chart_preview.py:415).

The flag is already self-contradictory: bubble/treemap/gauge/histogram all render empty specs while inheriting allows_empty_result=False, and a future plugin that sets allows_empty_result=True but renders via the generic spec path would still wrongly get NoDataError. Since a declarative lifecycle contract is the whole point of this PR, I'd fix this here rather than ship the flag dead.

Fix: make the flag load-bearing — gate the short-circuit, e.g.

if not chart_data and not (plugin and plugin.allows_empty_result):
    return ChartError(error="No data available for Vega-Lite visualization", error_type="NoDataError")

and set allows_empty_result = True on the plugins that intentionally render empty (bubble, treemap, gauge, histogram, in addition to gantt) — or remove the flag and document that overriding vega_lite_preview implies empty-rendering.

🟢 Low — Histogram's empty→spec change is undocumented (Bubble's is)

The summary explicitly calls out the saved empty-Bubble NoDataError → empty spec change, but the identical new behavior for saved empty Histogram previews isn't mentioned. It's consistent with the unsaved-preview path so it's fine behaviorally — just worth a line in the PR description / UPDATING.md. Folds into the Medium fix above once the flag is wired up.

🟢 Low — AST anti-branching guard has evasion gaps

test_chart_plugin_contract.py (test_dispatchers_do_not_branch_on_registered_chart_types, ~389–464) is what enforces the PR's central "no per-viz_type branching" invariant, but it (1) scans only the functions enumerated in _DISPATCHERS for the 5 library modules — untouched helpers in the same module (e.g. in preview_utils) are never parsed — and (2) flags only ast.Compare nodes whose ast.unparse text literally contains "viz_type"/"chart_type", so a renamed local (vt = fd.get("viz_type"); if vt == "waterfall":) or a dict-keyed dispatch slips through. Fix: whole-file-scan the library modules like the tool modules, and match Compare / Dict-key / .get() operands structurally against registered type names.

🟢 Low — Histogram resolve_query_fields overstates frontend parity

histogram.py:199–203 injects the synthetic COUNT(*) metric on bool(form_data.get("having")) or an adhoc HAVING clause, under a comment "Matches Histogram buildQuery." The frontend buildQuery only inspects adhoc_filters for a HAVING clause. The top-level-having disjunct is practically unreachable and is consistent with MCP honoring top-level having elsewhere, so this is cosmetic — either drop the disjunct or soften the comment.

---

Nothing here is a functional bug in a normal flow; the Medium is a defect in the new abstraction itself, which is why I'd address it in this PR. The merge conflict with master is the only hard merge blocker. Nice work overall — the contract suite and the AST guard are a good direction.

[bito-code-review]

Code Review Agent Run #ea5aba

Actionable Suggestions - 0

Additional Suggestions - 2

• superset/mcp_service/chart/chart_helpers.py - 2

  • Missing local type annotation · Line 516-516
    Per BITO.md rule 13598, annotatable locals should carry explicit type hints; the deleted code annotated its local (`groupby: list[Any]`), while the new `raw_groupby` on line 516 is unannotated. Adding `raw_groupby: Any` keeps the new helper aligned with the organization typing standard.
  • Inconsistent form_data typing · Line 514-514
    `normalize_groupby` (line 514) accepts `Mapping[str, Any]`, but `resolve_groupby`, which delegates to it at line 526, still declares `dict[str, Any]`, while sibling `resolve_shared_metrics` takes `Mapping`. Plugin callers declare `form_data: Mapping[str, Any]` (e.g. `resolve_query_fields` in `waterfall.py`), so they can call the helper but not `resolve_groupby` without wrapping in `dict()`. Consider widening `resolve_groupby` to `Mapping` for consistency.

Review Details

• Files reviewed - 8 · Commit Range: 21f20f0..4d952ec

  • docs/admin_docs/configuration/mcp-server.mdx
  • superset/mcp_service/chart/chart_helpers.py
  • superset/mcp_service/chart/plugin.py
  • superset/mcp_service/chart/plugins/gauge.py
  • superset/mcp_service/chart/plugins/treemap.py
  • superset/mcp_service/chart/plugins/waterfall.py
  • superset/mcp_service/chart/preview_utils.py
  • tests/unit_tests/mcp_service/chart/test_chart_plugin_contract.py
• Files skipped - 0
• Tools

  • MyPy (Static Code Analysis) - ✔︎ Successful
  • Astral Ruff (Static Code Analysis) - ✔︎ Successful
  • Whispers (Secret Scanner) - ✔︎ Successful
  • Detect-secrets (Secret Scanner) - ✔︎ Successful

---

Bito Usage Guide

Commands

Type the following command in the pull request comment and save the comment.

• /review - Manually triggers an incremental AI Review.

• /review full - 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

[aminghadersohi]

Addressed all four items in 5039119:

1. Empty-result flag: gates saved previews before plugin dispatch, including the generic renderer; Bubble, Treemap, Gauge and Histogram opt in alongside Gantt. Regression run before the fix: 7 failed, 2 passed; all pass after.
2. Histogram documentation: added empty→spec behavior to the PR description, MCP docs and UPDATING.md.
3. AST guard: scans whole library/tool modules and structurally checks comparisons, dictionary keys and lookups, including aliases. Exact pre-existing helper/rendering/metadata expressions are pinned as a counted baseline, not function exemptions; added evasion regressions.
4. HAVING comment: distinguishes frontend adhoc HAVING parity from MCP's top-level having support.

Chart MCP tests: 2,648 passed, 3 skipped; final contract suite: 464 passed. Pre-commit passed across all 35 PR-changed files, including mypy. The master conflict was already cleared: verified MERGEABLE at 4d952ec; no merge needed.

[bito-code-review]

Code Review Agent Run #471b63

Actionable Suggestions - 0

Additional Suggestions - 2

• superset/mcp_service/chart/plugins/treemap.py - 1

  • Empty-result flag ineffective · Line 45-45
    Setting `allows_empty_result = True` here has no effect: `get_chart_preview.py:494` skips its NoDataError guard, but control reaches `vega_lite_preview` → `treemap_vega_lite` (`treemap_preview.py:106-107`), which returns `NoDataError` for empty data. Empty treemap results still error, contradicting the flag's contract. Either drop the flag or make `treemap_vega_lite` render an empty layout.
• superset/mcp_service/chart/tool/get_chart_preview.py - 1

  • Duplicate plugin lookup · Line 493-493
    Line 493 repeats `plugin_for_viz_type(form_data.get("viz_type"))`, already evaluated inside `_create_plugin_preview` (line 412, invoked at 502) and again in `plugin_vega_lite_preview` (preview_utils.py:160) within the same `generate()` flow — the third copy of this expression in the file. Fetching the plugin once and threading it through keeps the lookup single-sourced.

Review Details

• Files reviewed - 7 · Commit Range: 4d952ec..ce599ca

  • docs/admin_docs/configuration/mcp-server.mdx
  • superset/mcp_service/chart/plugins/bubble.py
  • superset/mcp_service/chart/plugins/gauge.py
  • superset/mcp_service/chart/plugins/histogram.py
  • superset/mcp_service/chart/plugins/treemap.py
  • superset/mcp_service/chart/tool/get_chart_preview.py
  • tests/unit_tests/mcp_service/chart/test_chart_plugin_contract.py
• Files skipped - 1

  • UPDATING.md - Reason: Filter setting
• Tools

  • MyPy (Static Code Analysis) - ✔︎ Successful
  • Astral Ruff (Static Code Analysis) - ✔︎ Successful
  • Whispers (Secret Scanner) - ✔︎ Successful
  • Detect-secrets (Secret Scanner) - ✔︎ Successful

---

Bito Usage Guide

Commands

Type the following command in the pull request comment and save the comment.

• /review - Manually triggers an incremental AI Review.

• /review full - 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

[aminghadersohi]

@rebenitez1802 thanks for the thorough review. Every point is addressed, and master is merged in with a merge commit, so the conflict is cleared.

🟡 Medium: allows_empty_result was a dead flag. Fixed in 5039119. The flag now makes the empty-vs-error decision:

• VegaLitePreviewStrategy.generate() (get_chart_preview.py:494) returns NoDataError for empty rows unless the owning plugin sets allows_empty_result, before either the plugin renderer or the generic spec runs.
• The plugins that intentionally render empty results now opt in: bubble, treemap, gauge and histogram, alongside gantt.
• test_saved_empty_preview_obeys_plugin_contract covers all four flag × renderer combinations, so a flagged plugin that renders through the generic spec path no longer gets NoDataError.
• test_empty_rendering_plugins_opt_in pins the opt-ins.

🟢 Low: the empty saved Histogram preview change was undocumented. Documented in UPDATING.md ("Empty MCP chart previews") and in docs/admin_docs/configuration/mcp-server.mdx, next to the Bubble change.

🟢 Low: the AST guard had evasion gaps. Fixed in 5039119:

• test_dispatchers_do_not_branch_on_registered_chart_types now parses the five library modules whole, as well as the tool modules, instead of a per-function list.
• _branches_on_registered_type matches structurally, independent of variable names: Compare operands (including tuple/list/set literals), Dict keys, the key argument of .get(), and subscripts, plus isinstance(..., *ChartConfig).
• The chart-specific expressions that already exist are pinned in an exact-multiset baseline (_LEGACY_TYPE_BRANCHES), so adding or duplicating a branch fails, and so does leaving a stale baseline entry.
• test_dispatch_guard_detects_structural_branches covers your examples: a renamed local (vt == "waterfall"), membership in a tuple, dict-keyed dispatch, .get("waterfall"), subscripts, and a previously unlisted helper. test_dispatch_guard_ignores_get_default keeps fd.get("viz_type", "table") from being flagged.

🟢 Low: the Histogram parity comment overstated things. I softened the comment rather than dropping the disjunct (plugins/histogram.py:200). It now says the frontend adds the aggregate for adhoc HAVING filters and that MCP also honors the top-level having expression. I kept that path because MCP accepts top-level having elsewhere, and #44744's tests exercise it.

Also in this round: a live retest found that MCP gateways cap a tool-search page at 100 KB, and a page holding both update_chart (53 KB) and generate_chart (49 KB) exceeded it.

• The compact config schema in this PR already fixes that; the worst-case five-tool page is now 25,350 B.
• 3426b5f adds test_worst_case_search_page_fits_gateway_limit. It fails on master and passes here.

Checks: tests/unit_tests/mcp_service, tests/unit_tests/charts and the query-context tests pass locally: 6645 passed, 4 skipped, 2 xfailed. Pre-commit is clean.

[bito-code-review]

Code Review Agent Run #06cf99

Actionable Suggestions - 0

Additional Suggestions - 5

• tests/unit_tests/mcp_service/test_tool_inventory.py - 5

  • Serializer rerun per sort key · Line 197-204
    `entry_bytes` re-runs the full `_create_search_result_serializer` pipeline (`_serialize_tools_without_output_schema`) once per tool inside `sorted`'s key, so ~77 tools are JSON-serialized ~77 times where one pass suffices. Serialize once into a name->bytes dict and sort on that.
  • Magic 100KB limit untraceable · Line 188-188
    `SEARCH_PAGE_BYTE_LIMIT = 100_000` encodes the comment's 'gateway cap at 100 KB', but the only size limit defined in this repo is `DEFAULT_MAX_RESPONSE_BYTES = 50_000` (constants.py), and `search_tools` is excluded from that guard (server.py). Name the real constraint or reference the guard constant so the test tracks the limit that actually applies.
  • Untyped locals in new test · Line 197-205
    `text`, `limit`, `largest`, `page` (and `chart_tools`/`chart_page` below) are untyped locals in a test file; repo standard requires explicit annotations on all test-file locals even when inferable. Annotate them.
  • Helper missing docstring · Line 197-197
    Nested helper `entry_bytes` has no docstring; the repo rule requires docstrings on all new Python functions, including test helpers. Add a one-liner stating what is measured.
  • Chart tool names duplicated · Line 210-221
    This inline set re-lists the four chart tools already enumerated as `CHART_TOOLS` in `test_chart_tool_inventory.py` (which imports from this module). A new chart tool must be added in two places or this page check silently skips it. Share one constant via a helper module; importing back would cycle.

Review Details

• Files reviewed - 1 · Commit Range: ce599ca..3426b5f

  • tests/unit_tests/mcp_service/test_tool_inventory.py
• Files skipped - 0
• Tools

  • MyPy (Static Code Analysis) - ✔︎ Successful
  • Astral Ruff (Static Code Analysis) - ✔︎ Successful
  • Whispers (Secret Scanner) - ✔︎ Successful
  • Detect-secrets (Secret Scanner) - ✔︎ Successful

---

Bito Usage Guide

Commands

Type the following command in the pull request comment and save the comment.

• /review - Manually triggers an incremental AI Review.

• /review full - 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

[aminghadersohi]

Retest #2: all three reported oversized chart tools are already covered by the shared compact chart-config annotation at the PR head; no additional production compaction is needed.

Measured complete definitions as UTF-8 compact JSON (not just inputSchema, and without subtracting the chart-type enum). Native search definitions are normalized through MCP Tool validation and the gateway's metadata alias / exclude_unset serialization; direct tools/list retains output schemas and full descriptions:

Tool	Reporter-observed before	PR head native search	PR head direct catalog
update_chart_preview	112,956 B	2,004 B	3,551 B
generate_explore_link	106,800 B	1,837 B	8,157 B
update_chart	114,991 B	4,077 B	26,769 B

The before values are the reporter's measurements, not a reproduction against a customer workspace. A single-item gateway page adds two bytes for brackets. Added regression tests pinning the entire page below 100,000 bytes for both catalog paths and all four chart tools. Expanded FastMCP invalid-per-type-field validation coverage to all four request models; no runtime validation was weakened.

Rechecked @rebenitez1802's change-request review and threads: the empty-result flag gates both plugin and generic rendering; all five intentional empty renderers opt in; Histogram's behavior is documented in UPDATING.md and the MCP docs; the AST guard scans whole modules and structurally checks comparisons/dictionary keys/get operands with mutation regressions; Histogram's comment explicitly distinguishes MCP top-level having support. The earlier master conflict is resolved and the PR is mergeable, so no additional merge is needed.

Validation: 507 focused inventory/contract tests passed; branch-scoped pre-commit (including mypy and pylint) passed apart from a formatting auto-fix, followed by a clean staged pre-commit run.

[rebenitez1802]

Approve — all of my earlier feedback is resolved, and the fixes went beyond what I asked for. Nice work.

Re-reviewed the delta since my last pass (the lifecycle-contract fix + the two gateway-cap test commits, on top of a clean master merge):

• allows_empty_result is now load-bearing — get_chart_preview.py gates the empty case on if not chart_data and not (plugin and plugin.allows_empty_result): NoDataError, and bubble/gauge/histogram/treemap declare the flag (gantt already did). Verified the 5 plugins overriding vega_lite_preview are exactly the 5 that opt in — so no type wrongly errors on empty rows, and the funnel fallback path is unchanged.
• The empty-result contract is now tested both ways — test_saved_empty_preview_obeys_plugin_contract (flag on/off × plugin/generic renderer) and test_empty_rendering_plugins_opt_in.
• The AST anti-branching guard is substantially hardened — whole-file scanning of the library dispatchers, structural matching (Compare/Dict keys/.get() key-vs-default/Subscript/renamed locals), and a _LEGACY_TYPE_BRANCHES multiset baseline that rejects added or duplicated branches and flags stale entries, with dedicated evasion tests. This closes the gaps I flagged.
• Histogram parity comment corrected to reflect that the top-level having disjunct is an intentional MCP superset of the frontend behavior.
• Merge conflict resolved (MERGEABLE), and CI is fully green.

The Histogram query fix remains faithful to the frontend buildQuery/histogramOperator, and the dataset-rebind path holds up against cross-dataset exposure. 🚀

[bito-code-review]

Code Review Agent Run #61f853

Actionable Suggestions - 0

Additional Suggestions - 1

• tests/unit_tests/mcp_service/test_chart_tool_inventory.py - 1

  • Duplicated key selection · Line 328-328
    This `"identifier" if name == "update_chart" else "dataset_id"` conditional duplicates the identical expression at line 232 in `test_chart_tool_inventory_preserves_complete_schema`. If another tool ever needs `identifier` (or the key rule changes), both sites must be updated in lockstep. Consider a shared `_request_key(name)` helper to keep the schema-key rule single-sourced.

Review Details

• Files reviewed - 1 · Commit Range: 3426b5f..500a043

  • tests/unit_tests/mcp_service/test_chart_tool_inventory.py
• Files skipped - 0
• Tools

  • MyPy (Static Code Analysis) - ✔︎ Successful
  • Astral Ruff (Static Code Analysis) - ✔︎ Successful
  • Whispers (Secret Scanner) - ✔︎ Successful
  • Detect-secrets (Secret Scanner) - ✔︎ Successful

---

Bito Usage Guide

Commands

Type the following command in the pull request comment and save the comment.

• /review - Manually triggers an incremental AI Review.

• /review full - 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

[aminghadersohi]

Review follow-up:

• rebenitez1802's approved review (5356562885): the complete body confirms the earlier feedback is resolved; no further change requests.
• Bito's duplicated request-key suggestion (5897718375): addressed in 09179a3. Both inventory tests use _request_key(name), with a parameterized regression test covering all four chart tools.

Merged Apache master in 7645c1c without rewriting history. The sole conflict was UPDATING.md; both the MCP upgrade notes and master's upgrade notes were retained. #44744 remains open, so no alternative histogram/waterfall implementation required adaptation.

Validation: MCP unit suite 6,206 passed, 4 skipped, using the repository-required FastMCP 3.4.7 and local superset-core/src on PYTHONPATH. The initial run with outdated FastMCP 3.4.2 had three catalog middleware signature failures; all pass with 3.4.7. Pre-commit passed on all 35 PR-changed files, including mypy and pylint.

[aminghadersohi]

Checked both September 28 Bito review bodies against the head.

Closed in 56f5165:

• Duplicated registry scan; Redundant getattr fallback: one scan with direct attribute access, preserving global native-over-additional priority and insertion-order ties; regression tests cover both registration orders.
• Missing series_columns: each mixed-timeseries query carries its own grouping.
• Undocumented inline import; Undocumented local imports; Missing docstrings on hooks; Missing proxy docstring; Inline import violates rule; Cryptic docstring on proxy get: documented deferred imports/hooks, hoisted get_column_name, clarified proxy wording.
• duplicated column resolution; magic default bin count: consume the resolved histogram column and share the schema's bin default.
• Generic hook, Gantt-specific catch: handle the protocol's ValueError while preserving the Gantt-specific response.
• Dead fallback branches: consolidate default result metadata selection without changing responses.
• ignored param, empty columns; duplicate viz-type literal: resolve trendline columns in the fields hook and reuse TRENDLINE_VIZ_TYPE.
• Inconsistent disabled-plugin policy: saved table metric appends retain disabled-plugin contracts.
• Missing tests for fallback dispatch: saved funnel, sankey/radar rejection, and generic fall-through tests added.
• Missing local type annotations: annotate text/grown.
• Fragile type fallback; Shared mutable schema dict; Discriminator name coupling: document Annotated inputs, correctly unwrap single models, deep-copy annotations, and share the discriminator constant.
• Silent ValidationError path (both handlers): add diagnostic warnings.
• Bool row_limit bypasses fallback: reject booleans in the shared compile-limit helper.

Other body-only items:

• Dead plugin flag: already fixed by 5039119; preview generation consumes allows_empty_result, with contract tests.
• Unreachable rebind fallback: not dead globally; plugins inheriting the base merge hook return None and need the generic dataset-rebind fallback. Retained.
• Disallow dynamic typing (Any) in public API: heterogeneous Python configuration/form-data/query-result payloads intentionally use Any; the frontend TypeScript prohibition does not apply. Retained.

Previously resolved inline items remain closed: disabled-plugin config completion, Gantt ValueError, and unguarded Treemap int() in 49b2e0a; tooltip_metrics coverage and funnel stage ValueError in 21f20f0. “Magic numbers encode limit policy” and “Inconsistent mock pattern” retain their documented dispositions in the resolved threads.

Filtered body suggestions: Treemap/Gauge compile-limit duplication was already removed by eb08fb8; per-test plugin-filter monkeypatches intentionally isolate test state.

Validation: chart unit suite plus chart tool inventory: 2708 passed, 3 skipped. Pre-commit on all touched files passed, including mypy and pylint.

[bito-code-review]

Code Review Agent Run #8b971a

Actionable Suggestions - 0

Additional Suggestions - 7

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

  • Unguarded attribute access · Line 213-215
    The rewrite drops the previous `getattr(plugin, 'additional_viz_types', ())` guard in favor of direct attribute access. Every current plugin extends `BaseChartPlugin` (which defaults `additional_viz_types` to `frozenset()`), and the `ChartTypePlugin` Protocol declares the attribute, so this holds today. But `register()` validates only `chart_type`, so a future duck-typed plugin lacking the attribute would raise `AttributeError` in every viz lookup instead of being skipped.
• tests/unit_tests/mcp_service/chart/test_chart_plugin_contract.py - 5

  • Inline import without justification · Line 739-739
    `capped_compile_row_limit` is imported inside the test body, but `superset.mcp_service.chart.plugin` is already imported at module level (line 46), so no circular dependency justifies the inline import. Per repo rule 12745, move it into the existing module-level import alongside `BaseChartPlugin`.
  • Magic cap literal in test · Line 741-741
    The literal `10` restates `capped_compile_row_limit`'s `cap` default (plugin.py:37) without naming it. If the production cap changes, this test must be edited in lockstep, and a reader cannot tell whether 10 is the cap or a coincidental fallback. Prefer asserting against a named constant so the fallback assertion tracks the cap explicitly.
  • Inline import block unjustified · Line 747-750
    Same rule as the sibling test: `CHART_CONFIG_REFERENCE_SCHEMA` and `chart_config_reference_schema` are imported inside the test body while `superset.mcp_service.chart.schemas` is already imported at module level (line 54). Move both names into the top-level import; add a comment only if a circular dependency actually exists.
  • Unannotated test locals · Line 752-753
    BITO rule 13153 requires explicit annotations for test-file locals even when inferable. Annotate `first`/`second` as `WithJsonSchema` (import from pydantic at module level), consistent with annotated locals elsewhere in this file such as `names: set[str]` and `violations: list[str]`.
  • Unannotated ternary local · Line 756-756
    `schema` also lacks an explicit annotation (BITO rule 13153). Annotate as `dict[str, Any]` — both ternary arms yield the reference-schema dict — keeping this file consistent with its annotated locals (`operands: list[ast.expr | None]`, `violations: list[str]`).
• superset/mcp_service/chart/schemas.py - 1

  • Docstring overstates union support · Line 3719-3722
    `_chart_type_values` now indexes `get_args(config_type)[0]`, which is only valid for `Annotated` types (verified on Python 3.11.2: a plain union's `get_args` has no metadata element, so `[0]` yields the first member and member models then crash on `.model_fields`). The only caller (line 3732) passes the Annotated `ChartConfig`, so this is currently unreachable, but the reworded docstring promises union support the code does not implement.

Filtered by Review Rules

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

• superset/mcp_service/chart/plugins/mixed_timeseries.py - 1

  • Duplicated groupby_b normalization · Line 210-215

Review Details

• Files reviewed - 19 · Commit Range: 500a043..56f5165

  • tests/unit_tests/mcp_service/test_chart_tool_inventory.py
  • docs/admin_docs/configuration/mcp-server.mdx
  • superset/mcp_service/chart/chart_utils.py
  • superset/mcp_service/chart/compile.py
  • superset/mcp_service/chart/plugin.py
  • superset/mcp_service/chart/plugins/big_number.py
  • superset/mcp_service/chart/plugins/histogram.py
  • superset/mcp_service/chart/plugins/mixed_timeseries.py
  • superset/mcp_service/chart/plugins/treemap.py
  • superset/mcp_service/chart/preview_utils.py
  • superset/mcp_service/chart/registry.py
  • superset/mcp_service/chart/schemas.py
  • superset/mcp_service/chart/tool/get_chart_data.py
  • superset/mcp_service/chart/tool/update_chart.py
  • tests/unit_tests/mcp_service/chart/test_chart_helpers.py
  • tests/unit_tests/mcp_service/chart/test_chart_plugin_contract.py
  • tests/unit_tests/mcp_service/chart/test_registry.py
  • tests/unit_tests/mcp_service/chart/tool/test_get_chart_preview.py
  • tests/unit_tests/mcp_service/chart/tool/test_update_chart.py
• Files skipped - 0
• Tools

  • MyPy (Static Code Analysis) - ✔︎ Successful
  • Astral Ruff (Static Code Analysis) - ✔︎ Successful
  • Whispers (Secret Scanner) - ✔︎ Successful
  • Detect-secrets (Secret Scanner) - ✔︎ Successful

---

Bito Usage Guide

Commands

Type the following command in the pull request comment and save the comment.

• /review - Manually triggers an incremental AI Review.

• /review full - 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