fix(tools): widen ESRI Image attr_key resolution to fire for popup_options too

test_mcp/test_per_source_type_layer_tools.py

@@ -2235,7 +2235,6 @@ def test_consuming_types_still_accept_params(self):
         assert "layer_update" in result, result
 
 
-# ---------------------------------------------------------------------------
 # WMS attribute-variables / popup-options outer-key resolution
 # (debug audit 2026-05-21: Class A bug — same shape as ESRI Image pre-PR-#12)
 # ---------------------------------------------------------------------------
@@ -2315,3 +2314,149 @@ def test_no_attribute_variables_no_regression(self):
         # deletes the empty dicts).
         assert "attributeVariables" not in layer
         assert "attributeAliases" not in layer
+
+
+# ---------------------------------------------------------------------------
+# ESRI Image popup_options attr_key resolution (secondary Class A fix)
+# (debug audit 2026-05-21: attr_key resolution must also fire when
+# popup_options is provided — not only when attribute_variables is set)
+# ---------------------------------------------------------------------------
+
+
+class TestAddEsriImagePopupOptionsAttrKeyResolution:
+    """When popup_options is provided without attribute_variables, attr_key
+    must still resolve to the ESRI service's sublayer name. The React
+    popup-table render path (utilities.js getImageArcGISRestLayerAttributes)
+    keys by the service-side layer.name fetched from ?f=json — same as the
+    attribute_variables path. PR #11's outer-key normalization rewrites
+    popup_options.{aliases,omit} to attr_key, so attr_key must be the
+    resolved sublayer name regardless of which field triggered it.
+
+    Originally the resolver block fired only ``if attribute_variables:`` —
+    the popup_options-alone case fell through with the display-name
+    fallback, causing silent click-time misses in the popup table.
+    """
+
+    URL = "https://example.com/arcgis/rest/services/MyService/MapServer"
+
+    @patch(
+        "tethysdash_mcp.mcp_server._resolve_esri_layer_name",
+        return_value="Flow Forecast",
+    )
+    def test_popup_options_aliases_alone_uses_resolved_sublayer_name(
+        self, mock_resolve
+    ):
+        """popup_options.aliases without attribute_variables: outer key is
+        the resolved sublayer name, NOT the display name.
+
+        Real-world LLM emits the display name as the outer key (because the
+        tool description names the `name` arg) — PR #11's outer-key
+        normalization rewrites the single-entry outer key to attr_key. With
+        the fix, attr_key is now the resolved sublayer name when
+        popup_options is provided (previously fired only for
+        attribute_variables).
+        """
+        result = add_esri_image_layer(
+            map_uuid=MAP_UUID,
+            name="China Flowlines",
+            url=self.URL,
+            layer_id="0",
+            popup_options={
+                "aliases": {"China Flowlines": {"comid": "River ID"}},
+            },
+        )
+        layer = _get_layer_config(result)
+        # PR #11's single-entry outer-key normalize rewrote
+        # "China Flowlines" -> attr_key ("Flow Forecast").
+        assert "Flow Forecast" in layer["attributeAliases"]
+        assert "China Flowlines" not in layer["attributeAliases"]
+        assert layer["attributeAliases"]["Flow Forecast"] == {"comid": "River ID"}
+
+    @patch(
+        "tethysdash_mcp.mcp_server._resolve_esri_layer_name",
+        return_value="Flow Forecast",
+    )
+    def test_popup_options_omit_alone_uses_resolved_sublayer_name(
+        self, mock_resolve
+    ):
+        """popup_options.omit without attribute_variables: same resolution."""
+        result = add_esri_image_layer(
+            map_uuid=MAP_UUID,
+            name="China Flowlines",
+            url=self.URL,
+            layer_id="0",
+            popup_options={
+                "omit": {"China Flowlines": ["unused_field"]},
+            },
+        )
+        layer = _get_layer_config(result)
+        assert "Flow Forecast" in layer["omittedPopupAttributes"]
+        assert layer["omittedPopupAttributes"]["Flow Forecast"] == ["unused_field"]
+        assert "China Flowlines" not in layer["omittedPopupAttributes"]
+
+    @patch(
+        "tethysdash_mcp.mcp_server._resolve_esri_layer_name",
+        return_value="Flow Forecast",
+    )
+    def test_both_attribute_variables_and_popup_options_same_attr_key(
+        self, mock_resolve
+    ):
+        """Both fields keyed by the SAME resolved sublayer name."""
+        result = add_esri_image_layer(
+            map_uuid=MAP_UUID,
+            name="China Flowlines",
+            url=self.URL,
+            layer_id="0",
+            attribute_variables={"comid": "River ID"},
+            popup_options={
+                "aliases": {"China Flowlines": {"comid": "Comid Alias"}},
+            },
+        )
+        layer = _get_layer_config(result)
+        assert layer["attributeVariables"] == {"Flow Forecast": {"comid": "River ID"}}
+        assert layer["attributeAliases"] == {"Flow Forecast": {"comid": "Comid Alias"}}
+
+    @patch(
+        "tethysdash_mcp.mcp_server._resolve_esri_layer_name",
+        return_value=None,
+    )
+    def test_popup_options_alone_resolver_fails_falls_back_to_display_name(
+        self, mock_resolve
+    ):
+        """Soft-fail path: when resolver returns None, fallback to display
+        name (current behavior preserved when network down)."""
+        result = add_esri_image_layer(
+            map_uuid=MAP_UUID,
+            name="China Flowlines",
+            url=self.URL,
+            layer_id="0",
+            popup_options={
+                "aliases": {"China Flowlines": {"comid": "River ID"}},
+            },
+        )
+        layer = _get_layer_config(result)
+        assert "China Flowlines" in layer["attributeAliases"]
+        assert layer["attributeAliases"]["China Flowlines"] == {"comid": "River ID"}
+
+    def test_neither_attribute_variables_nor_popup_options_no_resolver_call(
+        self, mocker
+    ):
+        """Regression: when neither field is provided, the resolver block
+        doesn't fire (no wasted network call) and the layer builds normally."""
+        spy = mocker.patch(
+            "tethysdash_mcp.mcp_server._resolve_esri_layer_name",
+            return_value="Should Not Be Called",
+        )
+        result = add_esri_image_layer(
+            map_uuid=MAP_UUID,
+            name="China Flowlines",
+            url=self.URL,
+            layer_id="0",
+        )
+        assert "layer_update" in result
+        spy.assert_not_called()
+        layer = _get_layer_config(result)
+        # No attributeVariables / attributeAliases in the persisted layer
+        # (build() deletes the empty dicts).
+        assert "attributeVariables" not in layer
+        assert "attributeAliases" not in layer

tethysdash_mcp/mcp_server.py

@@ -2087,18 +2087,28 @@ def add_esri_image_layer(
         flat_source_props["params"] = esri_params
 
     attr_key = name
-    if attribute_variables:
+    # Resolve attr_key to the ESRI service's sublayer name whenever either
+    # attribute_variables OR popup_options is provided. Both code paths feed
+    # the same React popup-render machinery, which keys alias maps by the
+    # ESRI service's actual sublayer name (fetched from ?f=json), NOT by the
+    # user-supplied display name. PR #11's outer-key normalization rewrites
+    # popup_options.{aliases,omit} to attr_key, so attr_key must be the
+    # resolved sublayer name for popup_options to land on the right key.
+    # Originally this block fired only `if attribute_variables:` — that
+    # missed the popup_options.aliases-alone case (2026-05-21 audit).
+    if attribute_variables or popup_options:
         effective_layer_id = flat_source_props.get("params", {}).get("LAYERS")
         if effective_layer_id is None:
             # LAYERDEFS encodes the sublayer ID as the prefix before the
             # first colon: "<layer_id>:<where_clause>" (e.g.,
             # "0:rivercountry = 'China'"). When the LLM sets LAYERDEFS but
             # not LAYERS or the layer_id arg, derive the layer index from
             # LAYERDEFS so the ?f=json lookup can resolve the actual ESRI
-            # sublayer name. Without this fallback, attributeVariables ends
-            # up keyed by the user-facing display name (the `name` arg),
-            # while the React popup-render path queries by the ESRI
-            # service's sublayer name — silent click-time lookup failure.
+            # sublayer name. Without this fallback, attributeVariables /
+            # popup_options end up keyed by the user-facing display name
+            # (the `name` arg), while the React popup-render path queries
+            # by the ESRI service's sublayer name — silent click-time
+            # lookup failure.
             layerdefs = flat_source_props.get("params", {}).get("LAYERDEFS")
             if isinstance(layerdefs, str) and ":" in layerdefs:
                 candidate = layerdefs.split(":", 1)[0].strip()