From fb054c588a85faa2af81d01759b7da09566b8e06 Mon Sep 17 00:00:00 2001 From: romer8 Date: Thu, 21 May 2026 17:56:52 -0600 Subject: [PATCH] fix(tools): server-side case-insensitive normalize of gridItems[].args keys MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Debug session 2026-05-21 third turn: gemini-flash STILL emitted args = {"river_id": "${feature.comid}"} for the geoglows_forecast_plot plugin despite PR #13's description-tightening that explicitly named "case-sensitive" and gave the river_ID counterexample. Two confirmed LLM failures on case normalization → description-only is insufficient for this class of bug (per feedback_proactive_over_reactive_llm_routing .md: "pattern-matching against open-vocabulary model output is a losing fight"). Escalation path agreed in /ce-debug Option B: server-side case-insensitive arg-name normalization. ## Implementation Two new helpers in mcp_server.py: 1. _fetch_plugin_arg_names(source) — hits the same TETHYSDASH_BASE_URL/visualizations/list/ endpoint as list_intake_plugins, returns the declared arg_names list for the given source. Returns: - list[str] when source found with declared args - [] when source found but has no declared args (Default registry types like Map/Text whose args are open-shape) - None when source not found OR fetch failed (network down, malformed response, etc.) 2. _normalize_args_case(args, arg_names) — case-fold-matches LLM- emitted args keys against arg_names. Rules: - arg_names is None → pass through unchanged (don't reject; the LLM may know something we don't) - arg_names empty → pass through unchanged - Exact match → preserved - Case-fold match → rewrite to canonical case - No match for a key → keep as-is (cheap path — runtime tile error boundary handles the bad arg downstream) - Two LLM keys collide on case-fold → return None to signal structured envelope error Wired into configure_popup_modal_layer's gridItem normalization loop: for each gridItem, fetch its source's arg_names, normalize args in-place before json.dumps. Soft-fails to pass-through behavior when TETHYSDASH_BASE_URL is unset or fetch fails — better to ship the LLM's verbatim args than to reject the whole flow when the registry is unreachable. ## Bonus: tighter h-hint in description Per user feedback "the height can be reduced a bit", refined the position field description's h guidance: - 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 The LLM had picked h=40 which was visually too tall; h=25-30 is the right default for a typical geoglows-forecast-plot use case. ## Tests 7 new in TestPopupConfigArgsCaseNormalization: - test_lowercase_key_rewritten_to_canonical_case (motivating case) - test_exact_match_preserved - test_unknown_source_args_pass_through - test_default_registry_source_no_declared_args - test_fetch_failure_soft_fails_to_pass_through - test_case_fold_collision_rejected_with_structured_error - test_normalization_runs_per_gridItem_independently Plus 5 direct unit cases on the _normalize_args_case helper verified during dev (exact, case-fold, None passthrough, empty passthrough, unknown-key passthrough, collision). Full suite: 944 passed (937 baseline including PR #13's 2 description tests + 7 new case-normalize tests). No regression to existing TestToolHappyPath / TestEnvelopeContract — TETHYSDASH_BASE_URL is unset in the test env so _fetch_plugin_arg_names returns None and passthrough preserves prior behavior. ## Trade-off acknowledged Adds one HTTP fetch per configure_popup_modal_layer invocation (per gridItem source). No caching in v1 — list_intake_plugins is the authoritative source and the fetch is already what list_intake_plugins does. Future optimization (per-request memoization or a TTL cache) can be added if observed latency justifies. Network failure mode is soft (warning logged, pass-through preserved), so an unreachable backend doesn't block the popup-config flow — just removes the safety net. --- test_mcp/test_popup_modal_layer.py | 196 +++++++++++++++++++++++++++++ tethysdash_mcp/mcp_server.py | 125 +++++++++++++++++- 2 files changed, 317 insertions(+), 4 deletions(-) diff --git a/test_mcp/test_popup_modal_layer.py b/test_mcp/test_popup_modal_layer.py index 049cb87..9c425cf 100644 --- a/test_mcp/test_popup_modal_layer.py +++ b/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": "

Hello

"}, + ) + ], + ), + ) + gridItem = result["patch_update"]["ops"][0]["value"]["gridItems"][0] + assert json.loads(gridItem["args_string"]) == {"text": "

Hello

"} + + 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"} diff --git a/tethysdash_mcp/mcp_server.py b/tethysdash_mcp/mcp_server.py index b004e4b..b419e79 100644 --- a/tethysdash_mcp/mcp_server.py +++ b/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,