fix(tools): derive ESRI sublayer ID from LAYERDEFS when LAYERS is absent - #12
Merged
Conversation
Debug session 2026-05-21 turn 1 with gemini-flash, full user prompt:
"Add the China Flowlines ESRI Image and Map Service layer ... make it
queryable, set params.LAYERDEFS to 0: rivercountry = 'China'. Alias the
comid attribute to River ID."
The LLM correctly translated "alias the comid attribute to River ID" to
attribute_variables = {"comid": "River ID"} on add_esri_image_layer. But
the user's params spec used LAYERDEFS to encode both the sublayer ID
(0) and the WHERE clause — no LAYERS field, no layer_id arg.
Server-side, the ESRI-sublayer-name resolver only looked at
params.LAYERS:
effective_layer_id = flat_source_props.get("params", {}).get("LAYERS")
resolved = _resolve_esri_layer_name(url, effective_layer_id)
With LAYERS absent, effective_layer_id was None. _resolve_esri_layer_name
returned None immediately (it requires a non-None layer_id). The server
fell back to keying attributeVariables by the display name ("Bolivia
Flowlines" / "China Flowlines") instead of the ESRI service's actual
sublayer name ("Flow Forecast (m³/sec)").
The React popup-render path (AttributesPane.js:322-356) keys lookups by
the ESRI sublayer name fetched fresh from ?f=json. Key mismatch ->
"Variable Input Name" column rendered empty at runtime even though the
data was persisted. CLAUDE.md MCP Data Contract Rule 5 already calls
this out: "ESRI attributeVariables key: Use the ESRI service's actual
layer name (fetched from {url}?f=json), not the client display name."
Fix: when LAYERS is absent, fall back to extracting the layer index
from LAYERDEFS. LAYERDEFS encodes "<layer_id>:<where_clause>" — split
on the first colon, validate the prefix is all digits, pass to
_resolve_esri_layer_name. LAYERS still wins when both are set (existing
precedence preserved). Non-digit LAYERDEFS prefixes (raw filter strings
without a layer-id prefix) are correctly NOT extracted, so the resolver
gets None and the fallback-to-display-name behavior is unchanged for
that case.
Tests: 5 new in TestAddEsriImageLayer:
- test_layerdefs_only_resolves_attribute_variables_key: motivating case
- test_layerdefs_with_space_after_colon: real user prompt had "0: "
- test_layerdefs_only_resolver_fails_falls_back_to_display_name:
preserves fallback path
- test_layers_and_layerdefs_both_present_layers_wins: LAYERS precedence
- test_layerdefs_non_digit_prefix_no_extraction: raw LAYERDEFS string
not misparsed as a layer-id prefix
Full suite: 935 passed (930 baseline + 5 new). One change at one site;
no regression to the existing LAYERS-based code path.
This was referenced May 22, 2026
Merged
romer8
added a commit
that referenced
this pull request
May 22, 2026
…s value, not display name (#16) Post-/ce-debug audit 2026-05-21: same Class A bug class as ESRI Image pre-PR-#12, just on a different identifier. add_wms_layer was keying attributeVariables and popup_options.aliases by the user's `name` arg (display name), but React's getImageWMSLayerAttributes (in reactapp/components/map/utilities.js) keys alias maps by the WMS LAYERS param value (e.g., "topp:states") — fetched from DescribeFeatureType?typename=<LAYERS>. Mismatch caused silent click-time lookup failures whenever the user's display name differed from the WMS LAYERS value (the common case — display names are friendly labels like "US States"; LAYERS values are workspace-prefixed like "topp:states"). This commit: - Adds `_resolve_wms_attr_key(wms_layers, fallback_name)` helper near `_resolve_esri_layer_name`. Picks the first comma-separated entry of `wms_layers` (stripped), falling back to `fallback_name` if `wms_layers` is empty/missing. Simpler than the ESRI Image case — no network call needed; the LAYERS value is already in the `wms_layers` arg. - Wires into add_wms_layer's `_apply_common_layer_options` call: `attr_key=_resolve_wms_attr_key(wms_layers, name)` instead of `attr_key=name`. Affects both attribute_variables and the popup_options outer-key normalization PR #11 added. ## Multi-layer WMS For comma-separated `wms_layers` (e.g., "topp:states,topp:counties"), the helper picks the first entry as `attr_key`. Multi-layer WMS calls are rare in practice and the React attribute-config UI is per-layer anyway; the first layer is the conservative pick. Cross-layer attribute-variable authoring should use one add_wms_layer call per layer. ## Tests 4 new in TestAddWmsLayerAttrKeyFromWmsLayers: - test_attribute_variables_outer_key_is_wms_layers_value: motivating case — outer key is "topp:states", not display name "US States" - test_comma_separated_wms_layers_picks_first: multi-layer picks first - test_popup_options_aliases_also_uses_wms_layers_value: PR #11's outer-key normalization fires against the new attr_key - test_no_attribute_variables_no_regression: when neither attribute_variables nor popup_options is provided, the regular WMS layer-add flow is unchanged (no attributeVariables / attributeAliases in the persisted layer) Plus updated 1 pre-existing test (TestAdvancedMetadata::test_advanced_dicts_accepted_as_json_strings) to assert the new correct outer-key behavior — it was previously asserting the pre-fix display-name key. Full suite: 948 passed (944 baseline + 4 new). One assertion update on an existing test reflects the new correct behavior; zero true regression. ## Audit by-product context This is the 7th PR in the 2026-05-21 debug arc. Class A surface now covered on ESRI Image (PR #12) + WMS (this PR). Other layer types verified NOT affected: ESRI Feature (caller-supplied layerName matches server-side), GeoJSON (same), KML (same), tile types (not queryable). PMTiles Vector remains low-risk / flagged for if-when-used.
romer8
added a commit
that referenced
this pull request
May 22, 2026
…tions too Debug audit 2026-05-21 secondary finding: add_esri_image_layer's attr_key resolution (PR #12) was gated on `if attribute_variables:`. The popup_options-alone case fell through with the display-name fallback, causing silent click-time misses in the popup-table render path. Server side (mcp_server.py:2061): `if attribute_variables:` triggered the _resolve_esri_layer_name call. popup_options provided WITHOUT attribute_variables left attr_key = name (display name). React side (utilities.js getImageArcGISRestLayerAttributes): keys alias maps by the service-side layer.name fetched from ?f=json — same as the attribute_variables path. So popup_options.{aliases,omit} keyed by display name would silently miss at lookup time. PR #11's outer-key normalization rewrites popup_options.{aliases,omit} to attr_key. With attr_key=display_name, that rewrote to the wrong key. Widen the guard from `if attribute_variables:` to `if attribute_variables or popup_options:`. The resolver is a soft-fail (returns None on network failure → fallback to display name preserved). Inline comment expanded to explain both code paths and the React popup-render contract. No new helper or behavior change for the attribute_variables case — just makes the existing resolution machinery fire for one more condition. 5 new in TestAddEsriImagePopupOptionsAttrKeyResolution: - test_popup_options_aliases_alone_uses_resolved_sublayer_name: the motivating case — popup_options.aliases alone, outer key rewritten to resolved sublayer name, not display name - test_popup_options_omit_alone_uses_resolved_sublayer_name: same for the omit field - test_both_attribute_variables_and_popup_options_same_attr_key: defensive — both fields keyed by the same resolved sublayer name - test_popup_options_alone_resolver_fails_falls_back_to_display_name: soft-fail path preserved - test_neither_attribute_variables_nor_popup_options_no_resolver_call: regression — when neither field is provided, resolver doesn't fire (no wasted network call) Full suite: 949 passed (944 baseline + 5 new). This is the 8th PR in the 2026-05-21 debug arc. Class A surface for ESRI Image now fully covered — the original PR #12 fix for attribute_variables + this PR for popup_options. Combined with PR #16 (WMS attr_key from wms_layers), all known Class A bugs are addressed. ESRI Feature / GeoJSON / KML / tile types / GeoTIFF / static image remain not-affected (caller-supplied layerName matches across server + React, or are not queryable at the attribute level).
romer8
added a commit
that referenced
this pull request
May 22, 2026
…tions too (#17) Debug audit 2026-05-21 secondary finding: add_esri_image_layer's attr_key resolution (PR #12) was gated on `if attribute_variables:`. The popup_options-alone case fell through with the display-name fallback, causing silent click-time misses in the popup-table render path. Server side (mcp_server.py:2061): `if attribute_variables:` triggered the _resolve_esri_layer_name call. popup_options provided WITHOUT attribute_variables left attr_key = name (display name). React side (utilities.js getImageArcGISRestLayerAttributes): keys alias maps by the service-side layer.name fetched from ?f=json — same as the attribute_variables path. So popup_options.{aliases,omit} keyed by display name would silently miss at lookup time. PR #11's outer-key normalization rewrites popup_options.{aliases,omit} to attr_key. With attr_key=display_name, that rewrote to the wrong key. Widen the guard from `if attribute_variables:` to `if attribute_variables or popup_options:`. The resolver is a soft-fail (returns None on network failure → fallback to display name preserved). Inline comment expanded to explain both code paths and the React popup-render contract. No new helper or behavior change for the attribute_variables case — just makes the existing resolution machinery fire for one more condition. 5 new in TestAddEsriImagePopupOptionsAttrKeyResolution: - test_popup_options_aliases_alone_uses_resolved_sublayer_name: the motivating case — popup_options.aliases alone, outer key rewritten to resolved sublayer name, not display name - test_popup_options_omit_alone_uses_resolved_sublayer_name: same for the omit field - test_both_attribute_variables_and_popup_options_same_attr_key: defensive — both fields keyed by the same resolved sublayer name - test_popup_options_alone_resolver_fails_falls_back_to_display_name: soft-fail path preserved - test_neither_attribute_variables_nor_popup_options_no_resolver_call: regression — when neither field is provided, resolver doesn't fire (no wasted network call) Full suite: 949 passed (944 baseline + 5 new). This is the 8th PR in the 2026-05-21 debug arc. Class A surface for ESRI Image now fully covered — the original PR #12 fix for attribute_variables + this PR for popup_options. Combined with PR #16 (WMS attr_key from wms_layers), all known Class A bugs are addressed. ESRI Feature / GeoJSON / KML / tile types / GeoTIFF / static image remain not-affected (caller-supplied layerName matches across server + React, or are not queryable at the attribute level).
romer8
added a commit
that referenced
this pull request
May 22, 2026
Eight PRs from the 2026-05-21 popup-modal arc, grouped into Added / Changed / Fixed / Tests / Documentation per Keep a Changelog. New tool + slash-prompt for configure_popup_modal_layer (PR #10), case-fold args normalization for popup gridItems + render_plugin + add_dynamic_map_layer (PRs #14 + #15), ESRI sublayer-ID extraction from LAYERDEFS + ESRI Image guard widening (PRs #12 + #17), WMS attr_key from wms_layers (PR #16), popup_options outer-key normalization across all 11 add_*_layer tools (PR #11), tool description tightening for case-sensitive arg_names + 100-col grid (PR #13). Suite grew 930 → 961.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Real user-prompt failure surfaced during turn-1 testing of the popup-modal MCP surface. `add_esri_image_layer`'s ESRI-sublayer-name resolver only checked `params.LAYERS`; when the LLM specified the sublayer ID via `params.LAYERDEFS` instead (no LAYERS, no `layer_id` arg), the resolver got `None` and the server fell back to keying `attributeVariables` by the display name — but the React popup-render path keys lookups by the ESRI service's actual sublayer name fetched from `?f=json`. Silent mismatch → "Variable Input Name" column empty at runtime.
Root cause (full causal chain — see /ce-debug 2026-05-21)
User prompt:
Per `CLAUDE.md` MCP Data Contract Rule 5: "ESRI attributeVariables key: Use the ESRI service's actual layer name (fetched from {url}?f=json), not the client display name." The rule was correct; the LAYERDEFS-only input shape just bypassed it.
Fix
In `mcp_server.py` around line 2062, fall back to extracting the layer index from `LAYERDEFS` when `LAYERS` is absent. LAYERDEFS encodes `"<layer_id>:<where_clause>"` — split on the first colon, validate the prefix is all digits, pass to `_resolve_esri_layer_name`.
`LAYERS` still wins when both are set (precedence preserved). Non-digit `LAYERDEFS` prefixes (raw filter strings without a layer-id prefix) are correctly NOT extracted — the resolver gets `None` and the existing fallback-to-display-name behavior is unchanged for that path.
Tests
5 new in `TestAddEsriImageLayer`:
Full suite: 935 passed (930 baseline + 5 new). One change at one site; no regression to the existing LAYERS-based code path.
Manual smoke after merge
Re-run the original turn 1 prompt against the live MCP server (after restart). Server should log a resolved name (e.g., "Flow Forecast") instead of the "falling back to display name" warning, and clicking a feature at zoom 12+ should publish the comid value to a dashboard variable input named "River ID".
Context: why PR #11's fix didn't help
PR #11 / A1+A2 (popup_options.aliases outer-key normalization + description anchoring) addressed a parallel mechanism — `attributeAliases` (popup-table column rename). The user's actual intent was `attributeVariables` (variable-input publishing), which the LLM correctly identified. PR #11's drift fix is still valid for its own scope; it just didn't touch this code path.