From c3125978c860465b044e9ece42958bcf36337c52 Mon Sep 17 00:00:00 2001 From: romer8 Date: Thu, 21 May 2026 17:28:09 -0600 Subject: [PATCH] fix(tools): name case-sensitive arg_names + 100-col grid in configure_popup_modal_layer popup_config description MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Debug session 2026-05-21 turn 2 with gemini-flash on the popup-modal test prompt. Two distinct LLM-routing bugs in the gridItems shape; one shared description-tightening fix. ## Bug A — arg-name case mismatch User prompt asked to "embed the GeoGLOWS Forecast Plot plugin with River ID = ${feature.comid}". list_intake_plugins (called in a prior turn) returned: {"source": "geoglows_forecast_plot", "arg_names": ["river_ID"]} ← capital ID LLM emitted: gridItems[0].args = {"river_id": "${feature.comid}"} ← lowercase The LLM normalized the user's natural-language "River ID" to snake_case and ignored the authoritative arg_names. Persisted args had the wrong key. At runtime the GeoGLOWS plugin looked up args["river_ID"] → undefined → no data fetched. Edit Visualization modal's "River ID" form input (bound to key "river_ID") rendered empty. User-visible: "Failed to retrieve data" in the popup + empty form field in the visualization config. ## Bug B — popup gridItem too small (12-col Bootstrap default) User prompt said "filling the popup grid." LLM emitted: gridItems[0] = {x: 0, y: 0, w: 12, h: 12} ← Bootstrap 12-col But the popup uses tethysdash's DashboardLayout with colCount=100 (DashboardLayout.js:30). React UI's PopupLayoutEditor.buildNewGridItem defaults to w=20, h=20 — already 20% of popup width. The LLM's w=12 rendered the tile at ~12% width — visibly too small (screenshot: geoglows_forecast_plot placeholder text wrapped vertically in a narrow column). ## Fix — description-only tightening (per /ce-debug option A) Both bugs are description-routing failures. The popup_config field description didn't anchor: (1) gridItems[].args keys to the authoritative arg_names from the discovery tools (list_intake_plugins / list_available_visualizations). (2) gridItems[].position to the popup's 100-column grid (and explicitly negate the Bootstrap 12-col default). This commit extends the field description with two CRITICAL clauses: "CRITICAL — gridItems[].args keys are case-sensitive and MUST match the exact arg_names returned by list_intake_plugins / list_available_visualizations verbatim. The arg_names field is the authoritative source; do NOT normalize to snake_case from the user's natural-language phrasing or label. If list_intake_plugins returns arg_names containing river_ID (capital ID), the key MUST be river_ID — NOT river_id, River ID, or riverId." "CRITICAL — gridItems[].position fields (x, y, w, h) are cells in the popup's 100-column react-grid-layout (the SAME grid system as the main tethysdash dashboard — NOT Bootstrap's 12-column grid)." Per feedback_no_examples_in_tool_descriptions.md the river_ID example is naming the canonical arg_name shape, not a copy-paste value the LLM should reuse blindly — anchoring text only. ## Tests 2 new in TestPopupConfigFieldDescription: - test_field_description_names_args_case_sensitivity: asserts "case-sensitive" + "arg_names" appear in the popup_config field description. - test_field_description_names_popup_grid_col_count: asserts the 100-column convention is named AND Bootstrap 12-col is explicitly negated. Full suite: 937 passed (935 baseline + 2 new). Description-only; zero behavior change; no new failure modes. ## Escalation note Per the /ce-debug analysis, if a subsequent LLM still gets either case or cols wrong after this tightening, the next step is server-side case-insensitive arg-name normalization (Option B from the same session) — fetch the source's arg_names from TETHYSDASH_BASE_URL and case-fold-match the keys server-side. That adds a network call per configure_popup_modal_layer invocation; it's deferred until we observe description-tightening fail. --- test_mcp/test_popup_modal_layer.py | 77 ++++++++++++++++++++++++++++++ tethysdash_mcp/mcp_server.py | 20 ++++++-- 2 files changed, 94 insertions(+), 3 deletions(-) diff --git a/test_mcp/test_popup_modal_layer.py b/test_mcp/test_popup_modal_layer.py index aed13de..049cb87 100644 --- a/test_mcp/test_popup_modal_layer.py +++ b/test_mcp/test_popup_modal_layer.py @@ -630,3 +630,80 @@ def test_description_points_at_source_discovery_tools(self, description): def test_description_names_full_overwrite_semantic(self, description): d = description.lower() assert "replaces" in d + + +class TestPopupConfigFieldDescription: + """Pin the popup_config field-description prose that guides the LLM. + + Debug session 2026-05-21 turn 2: gemini-flash emitted: + (1) gridItems[0].args = {"river_id": "${feature.comid}"} (lowercase id) + when the GeoGLOWS plugin's arg_names was ["river_ID"] (capital ID). + At runtime the plugin looked up "river_ID", got undefined, fetched + nothing. Edit Visualization modal's "River ID" form input read by + "river_ID" key — empty in the UI. + (2) gridItems[0] position w=12, h=12 (Bootstrap 12-col convention) + when the popup uses tethysdash's DashboardLayout with colCount=100. + Tile rendered as ~12% of popup width — visibly too small. + + Both bugs were description-routing failures: the field text didn't + name (1) the case-sensitive arg_names binding to the discovery-tool + output, and (2) the 100-col grid convention. These tests pin the + fix so future edits can't silently drop the guidance. + """ + + @pytest.fixture(scope="class") + def popup_config_field_description(self): + """Pull the popup_config field's description from the tool input schema.""" + import asyncio + + from tethysdash_mcp.mcp_server import mcp + + async def go(): + return await mcp._local_provider.list_tools() + + loop = asyncio.new_event_loop() + try: + tools = loop.run_until_complete(go()) + finally: + loop.close() + tool = next(t for t in tools if t.name == "configure_popup_modal_layer") + schema = tool.parameters + return schema["properties"]["popup_config"].get("description", "") + + def test_field_description_names_args_case_sensitivity( + self, popup_config_field_description + ): + """gridItems[].args keys MUST match arg_names verbatim (case-sensitive).""" + d = popup_config_field_description.lower() + # The case-sensitivity rule must be explicit so the LLM doesn't + # snake_case-normalize from the user's natural-language phrasing. + assert "case-sensitive" in d, ( + "popup_config description must name the case-sensitivity rule for " + "gridItems[].args keys. Without it, LLMs (gemini-flash specifically) " + "lowercase the user's label and produce keys like 'river_id' when the " + "plugin declared 'river_ID' — silent runtime fetch failure." + ) + # The description must point at the authoritative source for the names. + assert "arg_names" in popup_config_field_description, ( + "popup_config description must reference arg_names (from list_intake_plugins " + "/ list_available_visualizations) as the authoritative source for " + "gridItems[].args keys." + ) + + def test_field_description_names_popup_grid_col_count( + self, popup_config_field_description + ): + """gridItems[].position is a 100-column grid, NOT Bootstrap 12-col.""" + d = popup_config_field_description.lower() + # The 100-col convention must be explicit so the LLM doesn't default + # to Bootstrap's 12-col grid (which produces tiles ~12% of popup width). + assert "100-column" in d or "100 column" in d or "100 col" in d, ( + "popup_config description must name the popup's 100-column " + "react-grid-layout. Without it, LLMs default to Bootstrap's 12-col " + "convention (w=12) producing tiles that render at ~12% of popup width." + ) + # Anti-pattern call-out: explicitly say NOT Bootstrap's 12-col. + assert "bootstrap" in d or "12-column" in d or "not 12" in d, ( + "popup_config description must explicitly negate Bootstrap's " + "12-col convention so the LLM doesn't fall back to w=12." + ) diff --git a/tethysdash_mcp/mcp_server.py b/tethysdash_mcp/mcp_server.py index 27dd037..b004e4b 100644 --- a/tethysdash_mcp/mcp_server.py +++ b/tethysdash_mcp/mcp_server.py @@ -3536,9 +3536,23 @@ def configure_popup_modal_layer( "{leftPct, topPct, widthPct, heightPct} percentages 0-100 " "(widthPct/heightPct clamped >= 20), titleTemplate?: str (may " "embed ${feature.} tokens), gridItems: list of at least one " - "{source: str, args: dict (values may embed ${feature.} " - "tokens), x: int >=0, y: int >=0, w: int >=1, h: int >=1, " - "metadata?: dict}}. Accepts both Dict and JSON-string Dict." + "{source: str, args: dict, x: int >=0, y: int >=0, w: int >=1, " + "h: int >=1, metadata?: dict}}. Accepts both Dict and JSON-string Dict. " + "CRITICAL — gridItems[].args keys are case-sensitive and MUST match " + "the exact arg_names returned by list_intake_plugins / " + "list_available_visualizations verbatim. The arg_names field is the " + "authoritative source; do NOT normalize to snake_case from the " + "user's natural-language phrasing or label. If list_intake_plugins " + "returns arg_names containing river_ID (capital ID), the key MUST " + "be river_ID — NOT river_id, River ID, or riverId. String values " + "inside args may embed ${feature.} tokens. " + "CRITICAL — gridItems[].position fields (x, y, w, h) are cells in " + "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)." )), ], ) -> Dict[str, Any]: