From e501980cb2f0d5bafbab5783e598edf03ded713c Mon Sep 17 00:00:00 2001 From: romer8 Date: Thu, 21 May 2026 19:56:59 -0600 Subject: [PATCH] fix(tools): apply case-fold args normalization to render_plugin + add_dynamic_map_layer MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Post-/ce-debug audit 2026-05-21: PR #14 fixed Bug B (LLM snake-case-normalization habit) in configure_popup_modal_layer but the same class of bug exists in render_plugin and add_dynamic_map_layer. Both accept an args dict for an intake plugin; both passed it through verbatim with zero normalization. LLM emits e.g. {"river_id": ...} when the plugin declares river_ID → plugin runtime lookup fails silently. This commit lifts the same normalization pattern (already implemented in mcp_server.py via _fetch_plugin_arg_names + _normalize_args_case) and wires it into both tools. No new helpers — direct re-use of PR #14's shared infrastructure. ## render_plugin - Insert _fetch_plugin_arg_names(source) + _normalize_args_case(args, ...) before building the visualization spec. - Collision case (None return) → structured {error, fix_hint} envelope. - Description tightening: replace the prior concrete example that read {"gauge_id": "${my_gauge}"} (which reinforced snake_case) with case-sensitivity prose that names the river_ID counterexample (an intentionally mixed-case canonical shape, NOT a value the LLM should copy verbatim per feedback_no_examples_in_tool_descriptions.md). ## add_dynamic_map_layer - Same insertion: helpers called between _resolve_dynamic_map_layer_plugin and builder.set_plugin_source. - Description tightening on the args field: case-sensitivity prose matching render_plugin. ## Audit findings (for record) Class B (args case-fold) confirmed in render_plugin + add_dynamic_map_layer. Class A (ESRI sublayer-name resolution) verified NOT applicable to add_esri_feature_layer — React's getArcGISFeatureServiceLayerAttributes keys by user-supplied layerName, consistent with server's attr_key=name. ESRI Feature URL convention (.../FeatureServer/) makes the layer implicit; no display-name vs service-name split. Hidden secondary concern flagged for later: popup_options.aliases on add_esri_image_layer may still have Class A issue (React popup-table render path probably keys by ESRI sublayer name, not user's display name). Not yet observed; will surface when popup-table-rename feature is exercised next. ## Tests 8 new tests across 2 files, mirroring TestPopupConfigArgsCaseNormalization pattern from PR #14: - test_visualization_contracts.py::TestRenderPluginArgsCaseNormalization: - test_lowercase_key_rewritten_to_canonical_case - test_exact_match_preserved - test_fetch_failure_soft_fails_to_pass_through - test_case_fold_collision_rejected_with_structured_error - test_layer_contracts.py::TestAddDynamicMapLayerArgsCaseNormalization: - same 4, parameterized for add_dynamic_map_layer Full suite: 952 passed (944 baseline + 8 new). Zero regression — existing TestRenderPlugin + TestAddDynamicMapLayer tests pass because TETHYSDASH_BASE_URL is unset in the test env so _fetch_plugin_arg_names returns None and the helper passes through. ## Soft-fail behavior preserved When TETHYSDASH_BASE_URL is unset OR the fetch fails OR the source isn't in the registry, args pass through unchanged (current pre-fix behavior). Better to ship the LLM's verbatim args than to reject the whole flow when the registry is briefly unreachable. The collision case (two LLM keys map to the same canonical arg_name) is the only hard-fail path — it indicates a genuine LLM mistake that the user should see. --- test_mcp/test_layer_contracts.py | 108 +++++++++++++++++++++++ test_mcp/test_visualization_contracts.py | 67 ++++++++++++++ tethysdash_mcp/mcp_server.py | 75 ++++++++++++++-- 3 files changed, 243 insertions(+), 7 deletions(-) diff --git a/test_mcp/test_layer_contracts.py b/test_mcp/test_layer_contracts.py index dee7ac9..41b1333 100644 --- a/test_mcp/test_layer_contracts.py +++ b/test_mcp/test_layer_contracts.py @@ -648,3 +648,111 @@ def test_valid_uuid_passes_validation(self): "UUID validator must accept a real UUID; the error came from " "elsewhere in the tool body. " + result["error"] ) + + + +# --------------------------------------------------------------------------- +# add_dynamic_map_layer server-side args case-fold normalization +# (debug audit 2026-05-21, same root cause as PR #14) +# --------------------------------------------------------------------------- + + +class TestAddDynamicMapLayerArgsCaseNormalization: + """LLM snake-case-normalization habit applies to add_dynamic_map_layer too. + + Per the 2026-05-21 audit, add_dynamic_map_layer had the same Bug B + class as pre-PR-#14 configure_popup_modal_layer: the LLM emits args + keys in snake_case regardless of the plugin's declared arg_names. + Server now case-fold-normalizes via the shared _fetch_plugin_arg_names + + _normalize_args_case helpers — same pattern as render_plugin. + """ + + PLUGIN = { + "source": "geoglows_map_layer", + "type": "map_layer", + "dynamic_map_layer": True, + } + + def _stub_resolver(self, source): + def _stub(s): + if s == source: + return {"plugin": self.PLUGIN} + return {"error": f"Unknown plugin source: {s!r}"} + return _stub + + def test_lowercase_key_rewritten_to_canonical_case(self, mocker): + mocker.patch( + "tethysdash_mcp.mcp_server._fetch_plugin_arg_names", + return_value=["region_ID"], + ) + mocker.patch( + "tethysdash_mcp.mcp_server._resolve_dynamic_map_layer_plugin", + side_effect=self._stub_resolver("geoglows_map_layer"), + ) + result = add_dynamic_map_layer( + map_uuid=MAP_UUID, + source="geoglows_map_layer", + name="Forecast", + args={"region_id": "south_america"}, + ) + assert "layer_update" in result + plugin_block = result["layer_update"]["layer"]["configuration"]["props"]["pluginSource"] + assert plugin_block["args"] == {"region_ID": "south_america"} + + def test_exact_match_preserved(self, mocker): + mocker.patch( + "tethysdash_mcp.mcp_server._fetch_plugin_arg_names", + return_value=["region_ID"], + ) + mocker.patch( + "tethysdash_mcp.mcp_server._resolve_dynamic_map_layer_plugin", + side_effect=self._stub_resolver("geoglows_map_layer"), + ) + result = add_dynamic_map_layer( + map_uuid=MAP_UUID, + source="geoglows_map_layer", + name="Forecast", + args={"region_ID": "south_america"}, + ) + plugin_block = result["layer_update"]["layer"]["configuration"]["props"]["pluginSource"] + assert plugin_block["args"] == {"region_ID": "south_america"} + + def test_fetch_failure_soft_fails_to_pass_through(self, mocker): + mocker.patch( + "tethysdash_mcp.mcp_server._fetch_plugin_arg_names", + return_value=None, + ) + mocker.patch( + "tethysdash_mcp.mcp_server._resolve_dynamic_map_layer_plugin", + side_effect=self._stub_resolver("geoglows_map_layer"), + ) + result = add_dynamic_map_layer( + map_uuid=MAP_UUID, + source="geoglows_map_layer", + name="Forecast", + args={"region_id": "south_america"}, + ) + # Soft-fail: original key preserved when fetch fails. + plugin_block = result["layer_update"]["layer"]["configuration"]["props"]["pluginSource"] + assert plugin_block["args"] == {"region_id": "south_america"} + assert "error" not in result + + def test_case_fold_collision_rejected_with_structured_error(self, mocker): + mocker.patch( + "tethysdash_mcp.mcp_server._fetch_plugin_arg_names", + return_value=["region_ID"], + ) + mocker.patch( + "tethysdash_mcp.mcp_server._resolve_dynamic_map_layer_plugin", + side_effect=self._stub_resolver("geoglows_map_layer"), + ) + result = add_dynamic_map_layer( + map_uuid=MAP_UUID, + source="geoglows_map_layer", + name="Forecast", + args={"region_id": "A", "REGION_ID": "B"}, + ) + assert "error" in result + assert "fix_hint" in result + assert "collide" in result["error"].lower() + assert "layer_update" not in result diff --git a/test_mcp/test_visualization_contracts.py b/test_mcp/test_visualization_contracts.py index 76f6bc1..b8b3e27 100644 --- a/test_mcp/test_visualization_contracts.py +++ b/test_mcp/test_visualization_contracts.py @@ -734,3 +734,70 @@ def test_r3a_uuids_are_unique(self, mock_plugins): r1 = render_custom_visualization(source="RuntimePanel") r2 = render_custom_visualization(source="RuntimePanel") assert r1["visualization"]["uuid"] != r2["visualization"]["uuid"] + + +# --------------------------------------------------------------------------- +# render_plugin server-side args case-fold normalization +# (debug audit 2026-05-21, same root cause as PR #14) +# --------------------------------------------------------------------------- + + +class TestRenderPluginArgsCaseNormalization: + """LLM snake-case-normalization habit applies to render_plugin too. + + Per the 2026-05-21 audit, render_plugin had the same Bug B class as + pre-PR-#14 configure_popup_modal_layer: the LLM emits args keys in + snake_case regardless of the plugin's declared arg_names. Server now + case-fold-normalizes via the shared _fetch_plugin_arg_names + + _normalize_args_case helpers. + """ + + def test_lowercase_key_rewritten_to_canonical_case(self, mocker): + mocker.patch( + "tethysdash_mcp.mcp_server._fetch_plugin_arg_names", + return_value=["river_ID"], + ) + result = render_plugin( + source="geoglows_forecast_plot", + args={"river_id": "${feature.comid}"}, + ) + assert result["visualization"]["args"] == {"river_ID": "${feature.comid}"} + + def test_exact_match_preserved(self, mocker): + mocker.patch( + "tethysdash_mcp.mcp_server._fetch_plugin_arg_names", + return_value=["river_ID"], + ) + result = render_plugin( + source="geoglows_forecast_plot", + args={"river_ID": "12345"}, + ) + assert result["visualization"]["args"] == {"river_ID": "12345"} + + def test_fetch_failure_soft_fails_to_pass_through(self, mocker): + """When TETHYSDASH_BASE_URL is unset or fetch fails, args pass through.""" + mocker.patch( + "tethysdash_mcp.mcp_server._fetch_plugin_arg_names", + return_value=None, + ) + result = render_plugin( + source="geoglows_forecast_plot", + args={"river_id": "12345"}, + ) + # Soft-fail: prefer ship-broken-at-runtime over reject-the-whole-flow. + assert result["visualization"]["args"] == {"river_id": "12345"} + assert "error" not in result + + def test_case_fold_collision_rejected_with_structured_error(self, mocker): + mocker.patch( + "tethysdash_mcp.mcp_server._fetch_plugin_arg_names", + return_value=["river_ID"], + ) + result = render_plugin( + source="geoglows_forecast_plot", + args={"river_id": "X", "RIVER_ID": "Y"}, + ) + assert "error" in result + assert "fix_hint" in result + assert "collide" in result["error"].lower() + assert "visualization" not in result diff --git a/tethysdash_mcp/mcp_server.py b/tethysdash_mcp/mcp_server.py index b419e79..0d04e51 100644 --- a/tethysdash_mcp/mcp_server.py +++ b/tethysdash_mcp/mcp_server.py @@ -4219,10 +4219,16 @@ def add_dynamic_map_layer( ))], name: Annotated[str, Field(description="Display name for the layer in the layer control")], args: Annotated[Optional[Union[Dict[str, Any], str]], Field(description=( - "Plugin args dict passed to fetch_features at render time. Supports " - "${variable_name} syntax for dashboard variable references — these " - "are preserved verbatim at persist time. Pass None or omit when the " - "plugin takes no args." + "Plugin args dict passed to fetch_features at render time. Keys MUST " + "match the exact arg_names returned by list_intake_plugins for this " + "source — case-sensitive. Do NOT normalize to snake_case from the " + "user's natural-language phrasing. If list_intake_plugins returns " + "arg_names containing region_ID (capital ID), the key MUST be " + "region_ID — NOT region_id. The server auto-corrects case-fold-" + "matching keys when TETHYSDASH_BASE_URL is configured. " + "Supports ${variable_name} syntax in arg VALUES for dashboard " + "variable references — these are preserved verbatim at persist time. " + "Pass None or omit when the plugin takes no args." ))] = None, ) -> Dict[str, Any]: """Add a runtime plugin-backed map layer. @@ -4270,9 +4276,30 @@ def add_dynamic_map_layer( if "error" in resolution: return resolution + # Server-side case-insensitive normalization for args keys against the + # source's declared arg_names. Catches the LLM's snake-case- + # normalization habit (same root cause as configure_popup_modal_layer + # and render_plugin). Soft-fails to pass-through when the source isn't + # found or TETHYSDASH_BASE_URL is unset. + plugin_arg_names = _fetch_plugin_arg_names(source) + normalized_args = _normalize_args_case(args, plugin_arg_names) + if normalized_args is None: + return { + "error": ( + f"invalid_args: keys collide case-insensitively after " + f"matching against the plugin's declared arg_names. " + f"Source: {source!r}." + ), + "fix_hint": ( + "Two of your args keys map to the same canonical arg_name " + "under case-fold. Keep only one entry per declared arg_name. " + "Use list_intake_plugins to confirm the exact arg_names." + ), + } + try: builder = LayerConfigurationBuilder(name, "GeoJSON") - builder.set_plugin_source(source, args) + builder.set_plugin_source(source, normalized_args) layer_config = builder.build() except ValueError as err: return {"error": str(err)} @@ -4303,7 +4330,17 @@ def add_dynamic_map_layer( ) def render_plugin( source: Annotated[str, Field(description="Intake driver name from the 'source' field in list_intake_plugins results. Always call list_intake_plugins first to get the exact source name. Do NOT guess or invent source names — using a wrong name causes a 'not installed' error.")], - args: Annotated[Dict[str, Any], Field(description="Plugin arguments. Use ${variable_name} syntax to reference dashboard variable inputs. Example: {\"gauge_id\": \"${my_gauge}\"}")], + args: Annotated[Dict[str, Any], Field(description=( + "Plugin arguments. Keys MUST match the exact arg_names returned by " + "list_intake_plugins for this source — case-sensitive. 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. The server auto-corrects " + "case-fold-matching keys when TETHYSDASH_BASE_URL is configured, but " + "the LLM should still emit the correct shape. Use ${variable_name} " + "syntax in arg VALUES to reference dashboard variable inputs." + ))], w: Annotated[ int, Field( @@ -4338,12 +4375,36 @@ def render_plugin( """ LOGGER.info("render_plugin: source=%s, args=%s", source, args) + # 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 isn't found in the + # registry or TETHYSDASH_BASE_URL is unset, args pass through + # unchanged. See _normalize_args_case for the full rule table. + plugin_arg_names = _fetch_plugin_arg_names(source) + normalized_args = _normalize_args_case(args, plugin_arg_names) + if normalized_args is None: + return { + "error": ( + f"invalid_args: keys collide case-insensitively after " + f"matching against the plugin's declared arg_names. " + f"Source: {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." + ), + } + return { "visualization": { "source": source, "vizType": "intake_plugin", "uuid": str(uuid.uuid4()), - "args": args, + "args": normalized_args, "w": w, "h": h, }