fix(mcp): fix CI failures — sql_expression handling, cardinality guard, test mock fixes

- big_number.py pre_validate: add sql_expression branch; return
  MISSING_SQL_METRIC_LABEL when label is absent/non-string, so the
  existing unit tests (and LLM callers) get a clear actionable error.
- xy.py normalize_column_refs: skip entries with sql_expression set
  (name is None for these metrics); previously crashed with
  AttributeError: 'NoneType'.lower() in _get_canonical_column_name.
- test_big_number_chart.py: replace three calls to deleted
  SchemaValidator._pre_validate_big_number_config with
  plugin.pre_validate() via get_registry().
- test_runtime_validator.py: replace call to deleted
  RuntimeValidator._validate_cardinality with XYChartPlugin.get_runtime_warnings;
  patch FormatTypeValidator to isolate cardinality guard.
- test_update_chart.py: set mock_create_preview.return_value to a
  3-tuple so the update_chart unpack doesn't crash; change RuntimeError
  to ValueError which is in NORMALIZATION_EXCEPTIONS.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

superset/mcp_service/chart/plugins/big_number.py

@@ -79,7 +79,25 @@ class BigNumberChartPlugin(BaseChartPlugin):
                 ],
                 error_code="INVALID_BIG_NUMBER_METRIC_TYPE",
             )
-        if not metric.get("aggregate") and not metric.get("saved_metric"):
+        if metric.get("sql_expression"):
+            label = metric.get("label")
+            if not isinstance(label, str) or not label.strip():
+                return ChartGenerationError(
+                    error_type="missing_sql_metric_label",
+                    message="SQL expression metrics require a non-empty 'label'",
+                    details=(
+                        "When using a custom SQL expression as the Big Number metric, "
+                        "a human-readable 'label' string is required so Superset can "
+                        "display the metric name."
+                    ),
+                    suggestions=[
+                        "Add 'label': e.g. {'sql_expression': 'SUM(a)/SUM(b)', "
+                        "'label': 'Conversion Rate'}",
+                        "The label must be a non-empty string",
+                    ],
+                    error_code="MISSING_SQL_METRIC_LABEL",
+                )
+        elif not metric.get("aggregate") and not metric.get("saved_metric"):
             return ChartGenerationError(
                 error_type="missing_metric_aggregate",
                 message=(

superset/mcp_service/chart/plugins/xy.py

@@ -119,6 +119,8 @@ class XYChartPlugin(BaseChartPlugin):
                 config_dict["x"]["name"], dataset_context
             )
         for y_col in config_dict.get("y") or []:
+            if y_col.get("sql_expression"):
+                continue  # sql_expression metrics have no underlying column
             if y_col.get("saved_metric"):
                 y_col["name"] = get_canonical_metric(y_col["name"], dataset_context)
             else:

tests/unit_tests/mcp_service/chart/test_big_number_chart.py

@@ -100,23 +100,29 @@ class TestBigNumberChartConfig:
     def test_sql_expression_with_label_passes_pre_validation(self) -> None:
         """A custom SQL metric is a valid third option alongside aggregate and
         saved_metric in Tier-1 validation."""
+        from superset.mcp_service.chart.registry import get_registry
+
         data = {
             "chart_type": "big_number",
             "metric": {"sql_expression": "SUM(a)/SUM(b)", "label": "Ratio"},
         }
-        is_valid, error = SchemaValidator._pre_validate_big_number_config(data)
-        assert is_valid is True
+        plugin = get_registry().get("big_number")
+        assert plugin is not None
+        error = plugin.pre_validate(data)
         assert error is None
 
     def test_sql_expression_without_label_fails_pre_validation(self) -> None:
         """Tier-1 surfaces the label-required error with an LLM-actionable
         suggestion before the request reaches Pydantic's stricter error."""
+        from superset.mcp_service.chart.registry import get_registry
+
         data = {
             "chart_type": "big_number",
             "metric": {"sql_expression": "SUM(a)/SUM(b)"},
         }
-        is_valid, error = SchemaValidator._pre_validate_big_number_config(data)
-        assert is_valid is False
+        plugin = get_registry().get("big_number")
+        assert plugin is not None
+        error = plugin.pre_validate(data)
         assert error is not None
         assert error.error_code == "MISSING_SQL_METRIC_LABEL"
 
@@ -124,12 +130,15 @@ class TestBigNumberChartConfig:
         """Pre-validation runs on raw dict input before Pydantic coercion, so
         a non-string ``label`` (e.g. an int from a buggy client) must surface
         as a validation error, not an AttributeError from ``.strip()``."""
+        from superset.mcp_service.chart.registry import get_registry
+
         data = {
             "chart_type": "big_number",
             "metric": {"sql_expression": "SUM(a)/SUM(b)", "label": 123},
         }
-        is_valid, error = SchemaValidator._pre_validate_big_number_config(data)
-        assert is_valid is False
+        plugin = get_registry().get("big_number")
+        assert plugin is not None
+        error = plugin.pre_validate(data)
         assert error is not None
         assert error.error_code == "MISSING_SQL_METRIC_LABEL"
 

tests/unit_tests/mcp_service/chart/tool/test_update_chart.py

@@ -1358,6 +1358,7 @@ class TestUpdateChartColumnNormalization:
         mock_validate.return_value = CompileResult(
             success=True, error=None, error_code=None, tier="validation", error_obj=None
         )
+        mock_create_preview.return_value = ("http://example.com/explore", None, [])
 
         # normalize_column_names returns the config unchanged
         def _passthrough(config, dataset_id):
@@ -1418,7 +1419,8 @@ class TestUpdateChartColumnNormalization:
         mock_validate.return_value = CompileResult(
             success=True, error=None, error_code=None, tier="validation", error_obj=None
         )
-        mock_normalize.side_effect = RuntimeError("DB connection failed")
+        mock_create_preview.return_value = ("http://example.com/explore", None, [])
+        mock_normalize.side_effect = ValueError("DB connection failed")
 
         request = {
             "identifier": 1,

tests/unit_tests/mcp_service/chart/validation/test_runtime_validator.py

@@ -236,8 +236,13 @@ class TestRuntimeValidatorNonBlocking:
     def test_validate_cardinality_returns_cleanly_when_x_name_is_none(self) -> None:
         """The dimension-rejection guard on XYChartConfig normally forbids
         x.name=None, but a model_construct bypass (or a future code path)
-        could land us here. The defensive guard must return cleanly without
-        calling into CardinalityValidator (which assumes a real column)."""
+        could land us here. The defensive guard in XYChartPlugin.get_runtime_warnings
+        must skip cardinality without crashing."""
+        from superset.mcp_service.chart.plugins.xy import XYChartPlugin
+        from superset.mcp_service.chart.validation.runtime.format_validator import (
+            FormatTypeValidator,
+        )
+
         col = ColumnRef.model_construct(name=None)
         config = XYChartConfig.model_construct(
             chart_type="xy",
@@ -246,14 +251,19 @@ class TestRuntimeValidatorNonBlocking:
             kind="line",
         )
 
-        with patch(
-            "superset.mcp_service.chart.validation.runtime."
-            "cardinality_validator.CardinalityValidator.check_cardinality"
-        ) as mock_check:
-            warnings, suggestions = RuntimeValidator._validate_cardinality(
-                config, dataset_id=1
-            )
+        plugin = XYChartPlugin()
+        with (
+            patch.object(
+                FormatTypeValidator,
+                "validate_format_compatibility",
+                return_value=(True, []),
+            ),
+            patch(
+                "superset.mcp_service.chart.validation.runtime."
+                "cardinality_validator.CardinalityValidator.check_cardinality"
+            ) as mock_check,
+        ):
+            warnings = plugin.get_runtime_warnings(config, dataset_id=1)
 
         assert warnings == []
-        assert suggestions == []
         mock_check.assert_not_called()