fix(tools): name aliases/omit keys in popup_options descriptions on all add_*_layer tools - #11
Merged
Merged
Conversation
Comments in mcp_server.py and plugin_helpers.py referenced 'Plan 2026-XX-YYY' documents that no longer add value to a reader of the code. Per the project's "no comments referencing the current task or PR" convention, replace them with comments that explain the WHY of the surrounding code without mentioning the plan that introduced it. No behavior change. Comment-only edits.
…ll add_*_layer tools
Debug session 2026-05-21: gemini-flash misrouted "alias the comid
attribute to River ID" to attribute_variables on add_esri_image_layer
instead of popup_options.aliases. Root cause: 10 of 11 add_*_layer
tools shipped with a 3-word stub description for popup_options
("Click-popup options") that named neither the aliases nor omit keys,
while attribute_variables sat next to it with a 35-word description
mentioning "feature attribute names" 3 times — strong semantic match
for "alias attribute". The LLM picked the field whose description
carried the relevant domain idiom.
add_wms_layer was the lone outlier with the full description naming
both sub-keys. This commit lockstep-replaces the stub across the
remaining 10 tools (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)
with the same wording add_wms_layer already has, so all 11 popup-
capable layer tools now describe popup_options identically.
Adds test_mcp/test_popup_options_description_drift.py — 22
parametrized assertions (11 tools × 2 keywords) that pin the wording
so future edits can't silently drop the aliases or omit keywords. The
test reads descriptions via the FastMCP catalog (mcp._local_provider.
list_tools()), mirroring test_tool_description_exclusivity.py's
pattern.
No behavior change. Full suite: 861 tests pass (839 prior + 22 new).
…ion binding
Debug session 2026-05-21 turn 1 with gemini-flash: the LLM emitted
`popup_options.aliases = {"0": {"comid": "River ID"}}` on an ESRI
Image layer call. The "0" came from the user prompt's
`params.LAYERDEFS = "0:rivercountry = 'China'"` — the model conflated
the sublayer index with the outer-key shape's `layer_name` slot. The
server accepted the call and stored the alias under key "0", but the
React popup-render path (`Map.js:641-650`) keys alias maps by layer
NAME, so the alias silently never fired at click time. User-visible
bug: popup table showed "comid" instead of "River ID".
Two fixes layered onto PR #11's description-drift work:
1. **Server-side tolerance** — `_normalize_popup_outer_keys` in
`_apply_common_layer_options`: when `popup_options.aliases` or
`.omit` is a single-entry dict whose outer key doesn't match the
layer's `name` arg, rewrite the key to `name`. Catches the
sublayer-ID case AND the literal "layer_name" placeholder case.
Multi-entry dicts pass through unchanged (caller's apparent intent
of cross-layer authoring is respected). Logs the rewrite at DEBUG
so it's grep-able if it ever surprises a caller.
2. **Description anchoring** — extend the description PR #11 lockstep-
updated on all 11 `add_*_layer` tools' `popup_options` field: name
the binding explicitly ("outer layer_name key MUST be the same
string passed as this tool's `name` arg — NOT a sublayer ID from
params, params.LAYERDEFS, layer_id, or any other identifier"). Per
`feedback_input_output_name_alignment.md` — description text is
the LLM's primary signal.
Tests:
- 6 new tests in TestPopupOptionsOuterKeyNormalization
(test_per_source_type_layer_tools.py): single-entry rewrite,
matching-key preserved, literal-placeholder rewrite, omit
normalization symmetry, multi-entry preserved, empty no-op.
- 11 new parametrized assertions in
test_popup_options_description_drift.py asserting the
`\`name\` arg` anchor text appears on every add_*_layer's
popup_options description.
Full suite: 878 passed (zero regressions).
This was referenced May 21, 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
10 of the 11 `add_*_layer` tools shipped with a 3-word stub description for `popup_options` ("Click-popup options") that named neither the `aliases` nor `omit` sub-keys. `add_wms_layer` was the lone outlier with the full description. This PR brings the other 10 into lockstep.
Root cause (debug session 2026-05-21)
Manual chatbox test of the popup-modal MCP surface (PR #10) with gemini-flash routed "alias the comid attribute to River ID" to `attribute_variables` on `add_esri_image_layer` instead of `popup_options.aliases`. The user's intent (popup-table column rename) was silently dropped.
Side-by-side on `add_esri_image_layer`:
A small model facing "alias the X attribute" has no signal pointing at `popup_options` — but `attribute_variables` reads like a perfect match. The misroute was predictable.
This is the same description-asymmetry pattern documented in `feedback_input_output_name_alignment.md` (memory).
Fix
Lockstep replace the stub on the 10 affected tools with the same wording `add_wms_layer` already uses:
```python
popup_options: Annotated[Optional[Union[Dict[str, Any], str]], Field(description=(
"Click-popup options. Accepts {'aliases': {layer_name: {field: alias}}} "
"and {'omit': {layer_name: [field, ...]}} sub-dicts."
))] = None,
```
Tools updated (was → is, all to the same canonical wording):
`add_dynamic_map_layer` doesn't accept `popup_options` (plugin-backed layers use the plugin's own popup mechanism) and is intentionally excluded.
Regression test
Adds `test_mcp/test_popup_options_description_drift.py` — 22 parametrized assertions (11 tools × 2 keywords) pinning the wording. For every `add_*_layer` tool that accepts `popup_options`, asserts the registered description contains both `aliases` AND `omit` keywords.
Reads descriptions via `mcp._local_provider.list_tools()` — same pattern as the existing `test_tool_description_exclusivity.py`.
Test plan
Companion PR
PR #10 (`feat/configure-popup-modal-layer`) adds the new `configure_popup_modal_layer` tool for modal-mode popups. This fix is for the table-mode popup config (the existing `popup_options.aliases` path); the two are independent but tightly related. Either can land first.