fix(tools): name aliases/omit keys in popup_options descriptions on all add_*_layer tools

test_mcp/test_per_source_type_layer_tools.py

@@ -1422,6 +1422,113 @@ def test_advanced_dicts_accepted_as_json_strings(self):
         }
 
 
+class TestPopupOptionsOuterKeyNormalization:
+    """Server-side LLM-tolerance for the popup_options outer-key shape.
+
+    Debug session 2026-05-21 (turn 1): gemini-flash emitted
+    ``popup_options.aliases = {"0": {"comid": "River ID"}}`` on an ESRI
+    Image Service layer. The "0" came from the prompt's
+    params.LAYERDEFS = "0:rivercountry = 'China'" — the LLM conflated the
+    sublayer index with the outer key. The React popup-render path
+    (`Map.js:641-650`) keys alias maps by layer NAME, so the alias
+    silently never fired at click time. These tests pin the server-side
+    auto-normalize that catches the wrong outer key and rewrites it to
+    the layer's `name` arg.
+    """
+
+    def test_single_entry_mismatched_outer_key_rewritten_to_name(self):
+        """The motivating debug case: outer key '0' rewritten to 'China Flowlines'."""
+        result = add_esri_image_layer(
+            map_uuid=MAP_UUID,
+            name="China Flowlines",
+            url="https://example.com/MapServer",
+            popup_options={"aliases": {"0": {"comid": "River ID"}}},
+        )
+        layer = result["layer_update"]["layer"]
+        assert layer["attributeAliases"] == {
+            "China Flowlines": {"comid": "River ID"}
+        }, "outer key should have been rewritten from '0' to the name arg"
+
+    def test_single_entry_matching_outer_key_preserved(self):
+        """When the LLM gets it right, no rewrite happens."""
+        result = add_esri_image_layer(
+            map_uuid=MAP_UUID,
+            name="China Flowlines",
+            url="https://example.com/MapServer",
+            popup_options={
+                "aliases": {"China Flowlines": {"comid": "River ID"}}
+            },
+        )
+        layer = result["layer_update"]["layer"]
+        assert layer["attributeAliases"] == {
+            "China Flowlines": {"comid": "River ID"}
+        }
+
+    def test_literal_placeholder_outer_key_rewritten(self):
+        """LLM copies the literal 'layer_name' placeholder from the description."""
+        result = add_esri_image_layer(
+            map_uuid=MAP_UUID,
+            name="China Flowlines",
+            url="https://example.com/MapServer",
+            popup_options={
+                "aliases": {"layer_name": {"comid": "River ID"}}
+            },
+        )
+        layer = result["layer_update"]["layer"]
+        assert layer["attributeAliases"] == {
+            "China Flowlines": {"comid": "River ID"}
+        }
+
+    def test_omit_single_entry_outer_key_also_normalized(self):
+        """popup_options.omit gets the same normalization as aliases."""
+        result = add_esri_image_layer(
+            map_uuid=MAP_UUID,
+            name="China Flowlines",
+            url="https://example.com/MapServer",
+            popup_options={"omit": {"0": ["sensitive_field"]}},
+        )
+        layer = result["layer_update"]["layer"]
+        assert layer["omittedPopupAttributes"] == {
+            "China Flowlines": ["sensitive_field"]
+        }
+
+    def test_multi_entry_outer_keys_preserved(self):
+        """Multi-entry case is unusual for add_*_layer but preserved verbatim.
+
+        Caller's apparent intent of authoring popups across multiple layers
+        in one call is respected — no rewrite.
+        """
+        result = add_esri_image_layer(
+            map_uuid=MAP_UUID,
+            name="China Flowlines",
+            url="https://example.com/MapServer",
+            popup_options={
+                "aliases": {
+                    "Layer A": {"a": "Alpha"},
+                    "Layer B": {"b": "Beta"},
+                }
+            },
+        )
+        layer = result["layer_update"]["layer"]
+        assert layer["attributeAliases"] == {
+            "Layer A": {"a": "Alpha"},
+            "Layer B": {"b": "Beta"},
+        }
+
+    def test_empty_popup_options_aliases_preserved(self):
+        """Empty sub-dict is a no-op."""
+        result = add_esri_image_layer(
+            map_uuid=MAP_UUID,
+            name="China Flowlines",
+            url="https://example.com/MapServer",
+            popup_options={"aliases": {}},
+        )
+        # Empty aliases doesn't produce an attributeAliases entry; layer
+        # still ends up with the queryable default block.
+        layer = result["layer_update"]["layer"]
+        assert layer.get("attributeAliases", {}) == {}
+
+
 # ---------------------------------------------------------------------------
 # source_props per-source-type allowlist
 # ---------------------------------------------------------------------------

test_mcp/test_popup_options_description_drift.py

@@ -0,0 +1,123 @@
+"""Drift guard for popup_options field descriptions across add_*_layer tools.
+
+Debug session 2026-05-21: gemini-flash misrouted a "alias the comid
+attribute to River ID" prompt to `attribute_variables` instead of
+`popup_options.aliases` on `add_esri_image_layer`. Root cause was
+asymmetric description weight — `attribute_variables` had a 35-word
+description with "attribute" repeated 3 times; `popup_options` had a
+3-word stub ("Click-popup options") with no mention of aliases or
+omit. The LLM picked the field whose description carried the relevant
+domain idiom.
+
+`add_wms_layer` had the full description ("aliases: {layer_name: {field:
+alias}}", "omit: {layer_name: [field, ...]}"); the 10 sibling layer-add
+tools all got the stub. This test pins the wording so future edits
+can't silently drop it again.
+
+Tools that need the clause: every add_*_layer tool that accepts a
+`popup_options` parameter. (add_dynamic_map_layer does not — it uses
+the plugin's own popup mechanism — so it is intentionally excluded.)
+"""
+
+import asyncio
+
+import pytest
+
+from tethysdash_mcp.mcp_server import mcp
+
+
+# Every add_*_layer tool that accepts a popup_options parameter. The
+# description on each must name both the `aliases` and `omit` keys so
+# the LLM can route "alias the X attribute to Y" or "hide field X" prompts
+# to popup_options instead of an adjacent field like attribute_variables.
+TOOLS_WITH_POPUP_OPTIONS = (
+    "add_wms_layer",
+    "add_esri_image_layer",
+    "add_esri_feature_layer",
+    "add_geojson_layer",
+    "add_kml_layer",
+    "add_image_tile_layer",
+    "add_vector_tile_layer",
+    "add_pmtiles_vector_layer",
+    "add_pmtiles_raster_layer",
+    "add_geotiff_layer",
+    "add_static_image_layer",
+)
+
+
+def _run(coro):
+    loop = asyncio.new_event_loop()
+    try:
+        return loop.run_until_complete(coro)
+    finally:
+        loop.close()
+
+
+@pytest.fixture(scope="module")
+def tool_input_schemas():
+    """Map of tool name -> JSON schema for tool inputs (from MCP catalog)."""
+    async def go():
+        return await mcp._local_provider.list_tools()
+
+    tools = _run(go())
+    return {t.name: t.parameters for t in tools}
+
+
+@pytest.mark.parametrize("tool_name", TOOLS_WITH_POPUP_OPTIONS)
+def test_popup_options_description_names_aliases_keyword(
+    tool_input_schemas: dict, tool_name: str
+):
+    """popup_options description must name `aliases` so LLMs route table-column
+    renames here, not to attribute_variables or another adjacent field.
+    """
+    schema = tool_input_schemas.get(tool_name)
+    assert schema is not None, f"tool {tool_name!r} not in catalog"
+    props = schema.get("properties", {})
+    assert "popup_options" in props, (
+        f"tool {tool_name!r} missing popup_options field — update "
+        f"TOOLS_WITH_POPUP_OPTIONS if popup_options was intentionally dropped"
+    )
+    description = props["popup_options"].get("description", "")
+    assert "aliases" in description, (
+        f"tool {tool_name!r} popup_options description doesn't name 'aliases'. "
+        f"Current: {description!r}. Add the aliases keyword so LLMs (especially "
+        f"small models like gemini-flash) can route 'alias attribute X to Y' "
+        f"prompts here instead of misrouting to attribute_variables."
+    )
+
+
+@pytest.mark.parametrize("tool_name", TOOLS_WITH_POPUP_OPTIONS)
+def test_popup_options_description_names_omit_keyword(
+    tool_input_schemas: dict, tool_name: str
+):
+    """popup_options description must also name `omit` for symmetric routing."""
+    schema = tool_input_schemas[tool_name]
+    description = schema["properties"]["popup_options"].get("description", "")
+    assert "omit" in description, (
+        f"tool {tool_name!r} popup_options description doesn't name 'omit'. "
+        f"Current: {description!r}."
+    )
+
+
+@pytest.mark.parametrize("tool_name", TOOLS_WITH_POPUP_OPTIONS)
+def test_popup_options_description_anchors_outer_key_to_name_arg(
+    tool_input_schemas: dict, tool_name: str
+):
+    """The outer-key/`name`-arg binding must be explicit.
+
+    Debug session 2026-05-21 turn 2: gemini-flash emitted
+    ``popup_options.aliases = {"0": {"comid": "River ID"}}`` on an
+    ESRI Image layer call, because it conflated "layer ID" (the
+    sublayer index from params.LAYERDEFS) with the outer-key shape's
+    ``layer_name`` slot. The React popup-render path keys by layer
+    name, so the alias silently never fired. This test pins the
+    anchoring text that tells the LLM to use the `name` arg.
+    """
+    schema = tool_input_schemas[tool_name]
+    description = schema["properties"]["popup_options"].get("description", "")
+    assert "`name` arg" in description, (
+        f"tool {tool_name!r} popup_options description doesn't anchor the "
+        f"outer layer_name key to the `name` arg. Current: {description!r}. "
+        f"Add language naming the binding so LLMs don't fall back to "
+        f"sublayer IDs."
+    )