fix(tools): server-side case-insensitive normalize of gridItems[].args keys

test_mcp/test_popup_modal_layer.py

@@ -707,3 +707,199 @@ def test_field_description_names_popup_grid_col_count(
             "popup_config description must explicitly negate Bootstrap's "
             "12-col convention so the LLM doesn't fall back to w=12."
         )
+
+
+# ---------------------------------------------------------------------------
+# Server-side case-insensitive args normalization (debug session 2026-05-21)
+# ---------------------------------------------------------------------------
+
+
+class TestPopupConfigArgsCaseNormalization:
+    """Server-side case-fold matching of gridItems[].args keys against
+    the plugin's declared arg_names (fetched from list_intake_plugins).
+
+    Debug session 2026-05-21 (third turn): gemini-flash emitted
+    ``gridItems[0].args = {"river_id": "${feature.comid}"}`` for the
+    geoglows_forecast_plot plugin whose declared arg_names is
+    ``["river_ID"]`` (capital ID). At runtime the plugin looked up
+    ``river_ID``, got undefined, fetched nothing. Description tightening
+    (PR #13) did NOT fix this — LLM case-normalization is too strong a
+    prior to override via prose alone. Per
+    ``feedback_proactive_over_reactive_llm_routing.md``, the escalation
+    is server-side normalization.
+    """
+
+    def test_lowercase_key_rewritten_to_canonical_case(self, mocker):
+        """The motivating case: river_id -> river_ID."""
+        mocker.patch(
+            "tethysdash_mcp.mcp_server._fetch_plugin_arg_names",
+            return_value=["river_ID"],
+        )
+        result = configure_popup_modal_layer(
+            map_uuid=_fresh_uuid(),
+            layer_index=0,
+            popup_config=_minimal_payload(
+                gridItems=[
+                    _minimal_gridItem(
+                        source="geoglows_forecast_plot",
+                        args={"river_id": "${feature.comid}"},
+                    )
+                ],
+            ),
+        )
+        gridItem = result["patch_update"]["ops"][0]["value"]["gridItems"][0]
+        persisted_args = json.loads(gridItem["args_string"])
+        assert persisted_args == {"river_ID": "${feature.comid}"}, (
+            "Server should have case-fold-rewritten 'river_id' to 'river_ID' "
+            "to match the plugin's declared arg_names."
+        )
+
+    def test_exact_match_preserved(self, mocker):
+        """When LLM gets the case right, the key is preserved verbatim."""
+        mocker.patch(
+            "tethysdash_mcp.mcp_server._fetch_plugin_arg_names",
+            return_value=["river_ID"],
+        )
+        result = configure_popup_modal_layer(
+            map_uuid=_fresh_uuid(),
+            layer_index=0,
+            popup_config=_minimal_payload(
+                gridItems=[
+                    _minimal_gridItem(
+                        source="geoglows_forecast_plot",
+                        args={"river_ID": "${feature.comid}"},
+                    )
+                ],
+            ),
+        )
+        gridItem = result["patch_update"]["ops"][0]["value"]["gridItems"][0]
+        assert json.loads(gridItem["args_string"]) == {"river_ID": "${feature.comid}"}
+
+    def test_unknown_source_args_pass_through(self, mocker):
+        """When source isn't in the registry, args pass through unchanged."""
+        mocker.patch(
+            "tethysdash_mcp.mcp_server._fetch_plugin_arg_names",
+            return_value=None,
+        )
+        result = configure_popup_modal_layer(
+            map_uuid=_fresh_uuid(),
+            layer_index=0,
+            popup_config=_minimal_payload(
+                gridItems=[
+                    _minimal_gridItem(
+                        source="some_unknown_plugin",
+                        args={"weird_key": "value"},
+                    )
+                ],
+            ),
+        )
+        gridItem = result["patch_update"]["ops"][0]["value"]["gridItems"][0]
+        # Pass-through: no normalization happens for unknown sources.
+        assert json.loads(gridItem["args_string"]) == {"weird_key": "value"}
+
+    def test_default_registry_source_no_declared_args(self, mocker):
+        """Default registry types (Map, Text) have no arg_names; args pass through."""
+        mocker.patch(
+            "tethysdash_mcp.mcp_server._fetch_plugin_arg_names",
+            return_value=[],
+        )
+        result = configure_popup_modal_layer(
+            map_uuid=_fresh_uuid(),
+            layer_index=0,
+            popup_config=_minimal_payload(
+                gridItems=[
+                    _minimal_gridItem(
+                        source="Text",
+                        args={"text": "<h1>Hello</h1>"},
+                    )
+                ],
+            ),
+        )
+        gridItem = result["patch_update"]["ops"][0]["value"]["gridItems"][0]
+        assert json.loads(gridItem["args_string"]) == {"text": "<h1>Hello</h1>"}
+
+    def test_fetch_failure_soft_fails_to_pass_through(self, mocker):
+        """When _fetch_plugin_arg_names returns None (fetch failed), args pass through."""
+        mocker.patch(
+            "tethysdash_mcp.mcp_server._fetch_plugin_arg_names",
+            return_value=None,  # simulates network failure
+        )
+        result = configure_popup_modal_layer(
+            map_uuid=_fresh_uuid(),
+            layer_index=0,
+            popup_config=_minimal_payload(
+                gridItems=[
+                    _minimal_gridItem(
+                        source="geoglows_forecast_plot",
+                        args={"river_id": "${feature.comid}"},
+                    )
+                ],
+            ),
+        )
+        gridItem = result["patch_update"]["ops"][0]["value"]["gridItems"][0]
+        # Soft-fail: the original (wrong-case) key is preserved when we can't
+        # fetch arg_names. Better to ship a broken-at-runtime call than to
+        # reject the whole flow when the registry is unreachable.
+        assert json.loads(gridItem["args_string"]) == {"river_id": "${feature.comid}"}
+        assert "error" not in result
+
+    def test_case_fold_collision_rejected_with_structured_error(self, mocker):
+        """Two LLM-emitted keys colliding on case-fold -> structured envelope."""
+        mocker.patch(
+            "tethysdash_mcp.mcp_server._fetch_plugin_arg_names",
+            return_value=["river_ID"],
+        )
+        result = configure_popup_modal_layer(
+            map_uuid=_fresh_uuid(),
+            layer_index=0,
+            popup_config=_minimal_payload(
+                gridItems=[
+                    _minimal_gridItem(
+                        source="geoglows_forecast_plot",
+                        # Both keys map to canonical "river_ID" under case-fold.
+                        args={"river_id": "${feature.comid}", "RIVER_ID": "X"},
+                    )
+                ],
+            ),
+        )
+        assert "error" in result
+        assert "fix_hint" in result
+        assert "collide" in result["error"].lower() or "collide" in result["fix_hint"].lower()
+        assert "patch_update" not in result
+
+    def test_normalization_runs_per_gridItem_independently(self, mocker):
+        """Each gridItem's args are normalized against its own source's arg_names."""
+        def fake_fetch(source):
+            if source == "geoglows_forecast_plot":
+                return ["river_ID"]
+            if source == "Text":
+                return []
+            return None
+
+        mocker.patch(
+            "tethysdash_mcp.mcp_server._fetch_plugin_arg_names",
+            side_effect=fake_fetch,
+        )
+        result = configure_popup_modal_layer(
+            map_uuid=_fresh_uuid(),
+            layer_index=0,
+            popup_config=_minimal_payload(
+                gridItems=[
+                    _minimal_gridItem(
+                        source="geoglows_forecast_plot",
+                        args={"river_id": "X"},
+                        x=0, y=0, w=50, h=40,
+                    ),
+                    _minimal_gridItem(
+                        source="Text",
+                        args={"text": "Y"},
+                        x=50, y=0, w=50, h=40,
+                    ),
+                ],
+            ),
+        )
+        gridItems = result["patch_update"]["ops"][0]["value"]["gridItems"]
+        # Plugin item: normalized to canonical case.
+        assert json.loads(gridItems[0]["args_string"]) == {"river_ID": "X"}
+        # Text item: no normalization (empty arg_names), passes through.
+        assert json.loads(gridItems[1]["args_string"]) == {"text": "Y"}

tethysdash_mcp/mcp_server.py

@@ -3442,6 +3442,98 @@ class _PopupConfigPayload(BaseModel):
 _DEFAULT_GRID_ITEM_METADATA: Dict[str, Any] = {"refreshRate": 0}
 
 
+def _fetch_plugin_arg_names(source: str) -> Optional[List[str]]:
+    """Fetch the declared ``arg_names`` for a registered viz source.
+
+    Hits the same backend endpoint as ``list_intake_plugins`` but returns
+    just the arg-name list for the given source. Used by
+    ``configure_popup_modal_layer`` to case-normalize LLM-emitted
+    ``gridItems[].args`` keys against the plugin's authoritative
+    declared arg names.
+
+    Returns:
+        list[str] of arg_names when the source is found and has declared args.
+        Empty list when the source is found but has no declared args (e.g.,
+        Default registry types like Map/Text whose args are open-shape).
+        ``None`` when the source isn't in the registry OR the fetch failed.
+        Callers should treat ``None`` as "skip normalization" (don't reject
+        the call — the LLM may be using a source name we don't know about).
+    """
+    if not TETHYSDASH_BASE_URL:
+        return None
+    try:
+        response = http_requests.get(
+            f"{TETHYSDASH_BASE_URL}/visualizations/list/",
+            timeout=10,
+        )
+        response.raise_for_status()
+        data = response.json()
+        groups = (
+            data.get("visualizations", []) if isinstance(data, dict)
+            else data if isinstance(data, list)
+            else []
+        )
+        for group in groups:
+            options = group.get("options", []) if isinstance(group, dict) else []
+            for opt in options:
+                if opt.get("source") != source:
+                    continue
+                args = opt.get("args", {})
+                if not isinstance(args, dict):
+                    return []
+                return list(args.keys())
+        return None
+    except Exception as exc:
+        LOGGER.warning(
+            "Failed to fetch arg_names for source %r from %s: %s",
+            source, TETHYSDASH_BASE_URL, exc,
+        )
+        return None
+
+
+def _normalize_args_case(
+    args: Dict[str, Any], arg_names: Optional[List[str]]
+) -> Optional[Dict[str, Any]]:
+    """Case-fold-match LLM-emitted ``args`` keys against the source's ``arg_names``.
+
+    LLMs (especially small models like gemini-flash) aggressively
+    snake-case identifiers regardless of explicit "case-sensitive"
+    instructions — e.g., the user prompt "River ID" becomes
+    ``{"river_id": ...}`` even when the plugin declared ``river_ID``.
+    This helper rewrites mismatched keys to the canonical case.
+
+    Rules:
+    * ``arg_names`` is ``None`` (source not found / fetch failed) -> return
+      ``args`` unchanged. Don't reject — the LLM may know something we don't.
+    * ``arg_names`` is empty (source has no declared args, e.g., Default
+      registry types) -> return ``args`` unchanged.
+    * Exact match exists -> keep the key as-is.
+    * Case-fold match exists and the canonical name is different -> rewrite.
+    * No match for a key -> keep as-is (don't reject; surface as runtime
+      error per the existing "cheap path" KTD #2 if the plugin rejects it).
+    * Two LLM-emitted keys collide under case-fold (e.g., both
+      ``river_id`` and ``RIVER_ID`` map to the same canonical
+      ``river_ID``) -> return ``None`` to signal a structured error.
+
+    Returns:
+        The (possibly rewritten) args dict, OR ``None`` when two
+        LLM-emitted keys collide case-insensitively. Caller converts
+        ``None`` into a structured ``{error, fix_hint}`` envelope.
+    """
+    if arg_names is None or not arg_names:
+        return args
+    canonical_by_lower = {n.lower(): n for n in arg_names}
+    seen_lower: Dict[str, str] = {}
+    rewritten: Dict[str, Any] = {}
+    for k, v in args.items():
+        canonical = canonical_by_lower.get(k.lower(), k)
+        if canonical in seen_lower.values():
+            return None  # collision under case-fold
+        seen_lower[k] = canonical
+        rewritten[canonical] = v
+    return rewritten
+
+
 def _popup_validation_error_envelope(ve: ValidationError) -> Dict[str, Any]:
     """Convert a Pydantic ValidationError into a structured tool envelope.
 
@@ -3550,9 +3642,11 @@ def configure_popup_modal_layer(
             "the popup's 100-column react-grid-layout (the SAME grid system "
             "as the main tethysdash dashboard — NOT Bootstrap's 12-column "
             "grid). Valid range: x, y >= 0; w, h >= 1; w <= 100. To make a "
-            "single gridItem fill the popup width use w=100; for a typical "
-            "single-visualization popup that fills the popup, use w=100 with "
-            "h sized to the popup height (e.g., h=40-60 for a tall plot)."
+            "single gridItem fill the popup width use w=100. Pick h based on "
+            "expected content: h~25-30 for a single time-series plot "
+            "(common case), h~35-40 for a tall plot or one with thick "
+            "legends, h~15-20 for a card or short text block. Avoid h>50 "
+            "unless the popup will scroll."
         )),
     ],
 ) -> Dict[str, Any]:
@@ -3607,12 +3701,35 @@ def configure_popup_modal_layer(
         metadata_dict = (
             item.metadata if item.metadata is not None else dict(_DEFAULT_GRID_ITEM_METADATA)
         )
+        # Server-side case-insensitive normalization for args keys against
+        # the source's declared arg_names. Catches the LLM's near-universal
+        # snake-case-normalization habit (e.g., emitting "river_id" when
+        # the plugin declared "river_ID"). When the source has no declared
+        # arg_names or isn't found in the registry, args pass through
+        # unchanged. See _normalize_args_case for the full rule table.
+        plugin_arg_names = _fetch_plugin_arg_names(item.source)
+        normalized_args = _normalize_args_case(item.args, plugin_arg_names)
+        if normalized_args is None:
+            return {
+                "error": (
+                    f"invalid_popup_config: gridItems[{idx}].args contains keys "
+                    f"that collide case-insensitively after matching against the "
+                    f"plugin's declared arg_names. Source: {item.source!r}."
+                ),
+                "fix_hint": (
+                    "Two of your args keys map to the same canonical arg_name "
+                    "under case-fold (e.g., 'river_id' AND 'River_ID' both "
+                    "match 'river_ID'). Keep only one entry per declared "
+                    "arg_name. Use list_intake_plugins to confirm the exact "
+                    "arg_names for this source."
+                ),
+            }
         gridItems_persisted.append({
             "i": str(idx + 1),
             "uuid": str(uuid.uuid4()),
             "id": None,
             "source": item.source,
-            "args_string": json.dumps(item.args),
+            "args_string": json.dumps(normalized_args),
             "metadata_string": json.dumps(metadata_dict),
             "x": item.x,
             "y": item.y,