test(mcp): add schema test for empty target_tab rejection

Verify that an empty string for target_tab raises a Pydantic ValidationError
(from min_length=1 on the Field) rather than reaching business logic and
returning a 'Tab not found' error.

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

superset/mcp_service/dashboard/schemas.py

@@ -480,7 +480,17 @@ class AddChartToDashboardRequest(BaseModel):
     )
     chart_id: int = Field(..., description="ID of the chart to add to the dashboard")
     target_tab: str | None = Field(
-        None, description="Target tab name (if dashboard has tabs)"
+        None,
+        min_length=1,
+        description=(
+            "Tab to place the chart in. Accepts a tab display name "
+            "(e.g. 'Sales') or a tab component ID (e.g. 'TAB-abc123'). "
+            "Display-name matching is case-insensitive and strips all emoji; "
+            "component ID matching is case-sensitive and exact. "
+            "When not found, the error response lists all available tab names. "
+            "When omitted on a tabbed dashboard the chart is placed in the "
+            "first tab."
+        ),
     )
 
 

superset/mcp_service/dashboard/tool/add_chart_to_existing_dashboard.py

@@ -149,29 +149,46 @@ def _first_tab_from_groups(
     return None
 
 
+def _collect_available_tab_names(layout: Dict[str, Any]) -> list[str]:
+    """Collect display names (or IDs) of all TAB components in the layout."""
+    names: list[str] = []
+    for tabs_children in _collect_tabs_groups(layout):
+        for tab_id in tabs_children:
+            tab = layout.get(tab_id)
+            if not tab or tab.get("type") != "TAB":
+                continue
+            text = (tab.get("meta") or {}).get("text", "")
+            names.append(text if text else tab_id)
+    return names
+
+
 def _find_tab_insert_target(
     layout: Dict[str, Any], target_tab: str | None = None
 ) -> str | None:
     """
     Detect if the dashboard uses tabs and return the appropriate tab's ID.
 
-    If *target_tab* is provided the function first tries to match it against
-    tab ``meta.text`` (display name) or the raw component ID.  When no match
-    is found (or *target_tab* is ``None``) the first ``TAB`` child is used as
-    a fallback so that new rows are still placed inside the tab structure
-    rather than directly under ``GRID_ID``.
+    When *target_tab* is ``None`` the function returns the first TAB child so
+    that new rows are placed inside the tab structure rather than directly
+    under ``GRID_ID``.
+
+    When *target_tab* is provided the function tries to match it against tab
+    ``meta.text`` (display name) or the raw component ID.  If no match is
+    found ``None`` is returned — the caller is responsible for surfacing an
+    error rather than silently placing the chart in the wrong tab.
 
     Returns:
-        The ID of the matched (or first) TAB component, or ``None`` if the
-        dashboard does not use top-level tabs.
+        The ID of the matched (or first) TAB component, or ``None``.
     """
     groups = _collect_tabs_groups(layout)
 
-    if target_tab:
+    if target_tab is not None:
         for tabs_children in groups:
             matched = _match_tab_in_children(layout, tabs_children, target_tab)
             if matched:
                 return matched
+        # target_tab specified but not found — signal mismatch to the caller.
+        return None
 
     return _first_tab_from_groups(layout, groups)
 
@@ -316,6 +333,45 @@ def _ensure_layout_structure(
         layout["DASHBOARD_VERSION_KEY"] = "v2"
 
 
+def _resolve_parent_container(
+    layout: Dict[str, Any],
+    dashboard_id: int,
+    target_tab: str | None,
+) -> tuple[str, None] | tuple[None, AddChartToDashboardResponse]:
+    """Return (parent_id, None) on success or (None, error_response) on mismatch.
+
+    When *target_tab* is specified and not found the caller receives a
+    descriptive error listing available tabs rather than a silent fallback.
+    """
+    tab_target = _find_tab_insert_target(layout, target_tab=target_tab)
+
+    if target_tab is not None and tab_target is None:
+        available = _collect_available_tab_names(layout)
+        if available:
+            tab_list = ", ".join(f"'{t}'" for t in available)
+            return None, AddChartToDashboardResponse(
+                dashboard=None,
+                dashboard_url=None,
+                position=None,
+                error=(
+                    f"Tab '{target_tab}' not found in dashboard {dashboard_id}. "
+                    f"Available tabs: {tab_list}."
+                ),
+            )
+        return None, AddChartToDashboardResponse(
+            dashboard=None,
+            dashboard_url=None,
+            position=None,
+            error=(
+                f"Dashboard {dashboard_id} has no tabs. "
+                "Remove the target_tab parameter to add the chart to "
+                "the default grid layout."
+            ),
+        )
+
+    return tab_target if tab_target else "GRID_ID", None
+
+
 def _find_and_authorize_dashboard(
     dashboard_id: int,
 ) -> tuple[Any, AddChartToDashboardResponse | None]:
@@ -369,7 +425,7 @@ def _find_and_authorize_dashboard(
         destructiveHint=False,
     ),
 )
-def add_chart_to_existing_dashboard(
+def add_chart_to_existing_dashboard(  # noqa: C901
     request: AddChartToDashboardRequest, ctx: Context
 ) -> AddChartToDashboardResponse:
     """
@@ -443,11 +499,16 @@ def add_chart_to_existing_dashboard(
             # Generate a unique ROW ID for the new row
             row_key = _find_next_row_position(current_layout)
 
-            # Detect tabbed dashboards and resolve target_tab by name or ID
-            tab_target = _find_tab_insert_target(
-                current_layout, target_tab=request.target_tab
+            # Detect tabbed dashboards and resolve target_tab by name or ID.
+            parent_id, tab_error = _resolve_parent_container(
+                current_layout, request.dashboard_id, request.target_tab
             )
-            parent_id = tab_target if tab_target else "GRID_ID"
+            if tab_error is not None:
+                return tab_error
+            if parent_id is None:
+                raise RuntimeError(
+                    "unreachable: tab_error is None implies parent_id is str"
+                )
 
             # Add chart, column, and row to layout
             chart_key, column_key, row_key = _add_chart_to_layout(

tests/unit_tests/mcp_service/dashboard/tool/test_add_chart_to_existing_dashboard.py

@@ -49,7 +49,7 @@ logger = logging.getLogger(__name__)
 # ---------------------------------------------------------------------------
 
 
-@pytest.fixture
+@pytest.fixture()
 def mcp_server() -> object:
     """Return the FastMCP app instance for use in MCP client tests."""
     return mcp
@@ -138,7 +138,7 @@ def _mock_dashboard(
 
 
 @patch("superset.daos.dashboard.DashboardDAO.find_by_id")
-@pytest.mark.asyncio
+@pytest.mark.asyncio()
 async def test_dashboard_not_found(mock_find_by_id: Mock, mcp_server: object) -> None:
     """Returns a clear error when the target dashboard does not exist."""
     mock_find_by_id.return_value = None
@@ -157,7 +157,7 @@ async def test_dashboard_not_found(mock_find_by_id: Mock, mcp_server: object) ->
 
 @patch("superset.security_manager.raise_for_ownership")
 @patch("superset.daos.dashboard.DashboardDAO.find_by_id")
-@pytest.mark.asyncio
+@pytest.mark.asyncio()
 async def test_permission_denied(
     mock_find_by_id: Mock, mock_raise_for_ownership: Mock, mcp_server: object
 ) -> None:
@@ -203,7 +203,7 @@ async def test_permission_denied(
 @patch("superset.db.session.get")
 @patch("superset.security_manager.raise_for_ownership")
 @patch("superset.daos.dashboard.DashboardDAO.find_by_id")
-@pytest.mark.asyncio
+@pytest.mark.asyncio()
 async def test_chart_not_found(
     mock_find_by_id: Mock,
     mock_raise_for_ownership: Mock,
@@ -231,7 +231,7 @@ async def test_chart_not_found(
 @patch("superset.db.session.get")
 @patch("superset.security_manager.raise_for_ownership")
 @patch("superset.daos.dashboard.DashboardDAO.find_by_id")
-@pytest.mark.asyncio
+@pytest.mark.asyncio()
 async def test_chart_already_in_dashboard(
     mock_find_by_id: Mock,
     mock_raise_for_ownership: Mock,
@@ -261,7 +261,7 @@ async def test_chart_already_in_dashboard(
 @patch("superset.db.session.get")
 @patch("superset.security_manager.raise_for_ownership")
 @patch("superset.daos.dashboard.DashboardDAO.find_by_id")
-@pytest.mark.asyncio
+@pytest.mark.asyncio()
 async def test_successful_add(
     mock_find_by_id: Mock,
     mock_raise_for_ownership: Mock,
@@ -295,3 +295,17 @@ async def test_successful_add(
     assert "/superset/dashboard/1/" in content["dashboard_url"]
     assert content["position"] is not None
     assert "chart_key" in content["position"]
+
+
+def test_empty_target_tab_rejected_by_schema() -> None:
+    """Empty string target_tab is rejected at schema layer, not as 'Tab not found'."""
+    from pydantic import ValidationError
+
+    from superset.mcp_service.dashboard.schemas import AddChartToDashboardRequest
+
+    with pytest.raises(ValidationError):
+        AddChartToDashboardRequest(dashboard_id=1, chart_id=10, target_tab="")
+
+    # None is valid (tab omitted)
+    req = AddChartToDashboardRequest(dashboard_id=1, chart_id=10, target_tab=None)
+    assert req.target_tab is None

tests/unit_tests/mcp_service/dashboard/tool/test_dashboard_generation.py

@@ -32,6 +32,7 @@ from superset.mcp_service.chart.chart_utils import DatasetValidationResult
 from superset.mcp_service.dashboard.constants import generate_id
 from superset.mcp_service.dashboard.tool.add_chart_to_existing_dashboard import (
     _add_chart_to_layout,
+    _collect_available_tab_names,
     _ensure_layout_structure,
     _find_next_row_position,
     _find_tab_insert_target,
@@ -1059,6 +1060,110 @@ class TestAddChartToExistingDashboard:
             assert "TAB-tab2" in chart_parents
             assert "TAB-tab1" not in chart_parents
 
+    @patch("superset.commands.dashboard.update.UpdateDashboardCommand")
+    @patch("superset.daos.dashboard.DashboardDAO.find_by_id")
+    @patch("superset.db.session")
+    @pytest.mark.asyncio
+    async def test_add_chart_target_tab_not_found(
+        self, mock_db_session, mock_find_dashboard, mock_update_command, mcp_server
+    ):
+        """target_tab specified but no matching tab → descriptive error listing
+        available tabs, not a silent fallback to the first tab."""
+        mock_dashboard = _mock_dashboard(id=3, title="Tabbed Dashboard")
+        mock_dashboard.slices = [_mock_chart(id=10)]
+        mock_dashboard.position_json = json.dumps(
+            {
+                "ROOT_ID": {"children": ["GRID_ID"], "id": "ROOT_ID", "type": "ROOT"},
+                "GRID_ID": {
+                    "children": ["TABS-abc123"],
+                    "id": "GRID_ID",
+                    "parents": ["ROOT_ID"],
+                    "type": "GRID",
+                },
+                "TABS-abc123": {
+                    "children": ["TAB-tab1", "TAB-tab2"],
+                    "id": "TABS-abc123",
+                    "parents": ["ROOT_ID", "GRID_ID"],
+                    "type": "TABS",
+                },
+                "TAB-tab1": {
+                    "children": [],
+                    "id": "TAB-tab1",
+                    "meta": {"text": "Overview"},
+                    "parents": ["ROOT_ID", "GRID_ID", "TABS-abc123"],
+                    "type": "TAB",
+                },
+                "TAB-tab2": {
+                    "children": [],
+                    "id": "TAB-tab2",
+                    "meta": {"text": "Details"},
+                    "parents": ["ROOT_ID", "GRID_ID", "TABS-abc123"],
+                    "type": "TAB",
+                },
+                "DASHBOARD_VERSION_KEY": "v2",
+            }
+        )
+        mock_chart = _mock_chart(id=30)
+        mock_db_session.get.return_value = mock_chart
+        mock_find_dashboard.return_value = mock_dashboard
+
+        request = {"dashboard_id": 3, "chart_id": 30, "target_tab": "Nonexistent Tab"}
+
+        async with Client(mcp_server) as client:
+            result = await client.call_tool(
+                "add_chart_to_existing_dashboard", {"request": request}
+            )
+
+            assert result.structured_content["error"] is not None
+            error = result.structured_content["error"]
+            assert "Nonexistent Tab" in error
+            assert "not found" in error
+            # Available tabs should be listed
+            assert "Overview" in error
+            assert "Details" in error
+            # No layout mutation should have been persisted
+            mock_update_command.assert_not_called()
+
+    @patch("superset.commands.dashboard.update.UpdateDashboardCommand")
+    @patch("superset.daos.dashboard.DashboardDAO.find_by_id")
+    @patch("superset.db.session")
+    @pytest.mark.asyncio
+    async def test_add_chart_target_tab_on_non_tabbed_dashboard(
+        self, mock_db_session, mock_find_dashboard, mock_update_command, mcp_server
+    ):
+        """target_tab on a dashboard with no tabs → descriptive error."""
+        mock_dashboard = _mock_dashboard(id=5, title="Flat Dashboard")
+        mock_dashboard.slices = []
+        mock_dashboard.position_json = json.dumps(
+            {
+                "ROOT_ID": {"children": ["GRID_ID"], "id": "ROOT_ID", "type": "ROOT"},
+                "GRID_ID": {
+                    "children": [],
+                    "id": "GRID_ID",
+                    "parents": ["ROOT_ID"],
+                    "type": "GRID",
+                },
+                "DASHBOARD_VERSION_KEY": "v2",
+            }
+        )
+        mock_chart = _mock_chart(id=99)
+        mock_db_session.get.return_value = mock_chart
+        mock_find_dashboard.return_value = mock_dashboard
+
+        request = {"dashboard_id": 5, "chart_id": 99, "target_tab": "Sales"}
+
+        async with Client(mcp_server) as client:
+            result = await client.call_tool(
+                "add_chart_to_existing_dashboard", {"request": request}
+            )
+
+            assert result.structured_content["error"] is not None
+            error = result.structured_content["error"]
+            assert "no tabs" in error.lower()
+            assert "target_tab" in error
+            # No layout mutation should have been persisted
+            mock_update_command.assert_not_called()
+
     @patch("superset.commands.dashboard.update.UpdateDashboardCommand")
     @patch("superset.daos.dashboard.DashboardDAO.find_by_id")
     @patch("superset.db.session")
@@ -1311,9 +1416,9 @@ class TestLayoutHelpers:
         }
         assert _find_tab_insert_target(layout, target_tab="TAB-second") == "TAB-second"
 
-    def test_find_tab_insert_target_unmatched_falls_back_to_first(self):
-        """Test _find_tab_insert_target falls back to first tab when target_tab
-        doesn't match any tab name or ID."""
+    def test_find_tab_insert_target_unmatched_returns_none(self):
+        """Test _find_tab_insert_target returns None when target_tab doesn't
+        match any tab name or ID, so the caller can return a descriptive error."""
         layout = {
             "GRID_ID": {"children": ["TABS-main"], "type": "GRID"},
             "TABS-main": {"children": ["TAB-first", "TAB-second"], "type": "TABS"},
@@ -1328,9 +1433,17 @@ class TestLayoutHelpers:
                 "meta": {"text": "Tab 2"},
             },
         }
-        assert (
-            _find_tab_insert_target(layout, target_tab="Nonexistent Tab") == "TAB-first"
-        )
+        assert _find_tab_insert_target(layout, target_tab="Nonexistent Tab") is None
+
+    def test_find_tab_insert_target_empty_string_returns_none(self):
+        """An empty-string target_tab is treated as specified-but-not-found,
+        not as 'no tab requested', so it returns None rather than first tab."""
+        layout = {
+            "GRID_ID": {"children": ["TABS-main"], "type": "GRID"},
+            "TABS-main": {"children": ["TAB-first"], "type": "TABS"},
+            "TAB-first": {"children": [], "type": "TAB", "meta": {"text": "Tab 1"}},
+        }
+        assert _find_tab_insert_target(layout, target_tab="") is None
 
     def test_find_tab_insert_target_tabs_under_root(self):
         """Test _find_tab_insert_target when TABS are under ROOT_ID (real layout)."""
@@ -1358,6 +1471,35 @@ class TestLayoutHelpers:
         """Test _find_tab_insert_target with missing GRID_ID."""
         assert _find_tab_insert_target({"ROOT_ID": {"type": "ROOT"}}) is None
 
+    def test_collect_available_tab_names_returns_display_names(self):
+        """_collect_available_tab_names returns each tab's display text."""
+        layout = {
+            "GRID_ID": {"children": ["TABS-x"], "type": "GRID"},
+            "TABS-x": {"children": ["TAB-a", "TAB-b"], "type": "TABS"},
+            "TAB-a": {"children": [], "type": "TAB", "meta": {"text": "Overview"}},
+            "TAB-b": {"children": [], "type": "TAB", "meta": {"text": "Details"}},
+        }
+        names = _collect_available_tab_names(layout)
+        assert names == ["Overview", "Details"]
+
+    def test_collect_available_tab_names_falls_back_to_id(self):
+        """_collect_available_tab_names uses component ID when text is empty."""
+        layout = {
+            "GRID_ID": {"children": ["TABS-x"], "type": "GRID"},
+            "TABS-x": {"children": ["TAB-a"], "type": "TABS"},
+            "TAB-a": {"children": [], "type": "TAB", "meta": {}},
+        }
+        names = _collect_available_tab_names(layout)
+        assert names == ["TAB-a"]
+
+    def test_collect_available_tab_names_no_tabs(self):
+        """_collect_available_tab_names returns empty list for non-tabbed dashboards."""
+        layout = {
+            "GRID_ID": {"children": ["ROW-1"], "type": "GRID"},
+            "ROW-1": {"children": [], "type": "ROW"},
+        }
+        assert _collect_available_tab_names(layout) == []
+
     def test_add_chart_to_layout_creates_column(self):
         """Test that _add_chart_to_layout creates ROW > COLUMN > CHART."""
         layout = {