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, }