Skip to content

fix(tools): server-side case-insensitive normalize of gridItems[].args keys - #14

Merged
romer8 merged 1 commit into
mainfrom
fix/popup-config-args-case-normalize
May 22, 2026
Merged

fix(tools): server-side case-insensitive normalize of gridItems[].args keys#14
romer8 merged 1 commit into
mainfrom
fix/popup-config-args-case-normalize

Conversation

@romer8

@romer8 romer8 commented May 21, 2026

Copy link
Copy Markdown
Collaborator

Summary

Description-tightening (PR #13) didn't stop gemini-flash from emitting `args = {"river_id": ...}` when the plugin declared `river_ID`. Per the escalation path agreed in /ce-debug Option B, this PR adds server-side case-insensitive normalization: fetch the source's declared `arg_names` from `TETHYSDASH_BASE_URL` and rewrite mismatched gridItem args keys to canonical case before persisting.

Root cause (confirmed in two LLM runs)

User prompt: "embed the GeoGLOWS Forecast Plot plugin with River ID = ${feature.comid}". The plugin declares `arg_names: ["river_ID"]`. The LLM emitted `args_string: "{\"river_id\": ...}"` (lowercase). Plugin's `run()` looked up `river_ID` → undefined → no data fetched. Edit Visualization "River ID" form input bound to key `river_ID` → empty.

PR #13 added "case-sensitive" + the `river_ID` counterexample to the description. gemini-flash still emitted lowercase. Per workspace memory:

`feedback_proactive_over_reactive_llm_routing.md`: "Pattern-matching against open-vocabulary model output is a losing fight."

Description tightening worked for the cols bug (Bug B from prior /ce-debug; LLM correctly emitted `w=100` after PR #13). It didn't work for case-sensitivity because LLM snake-case normalization is a much stronger prior.

Fix

Two new helpers in `mcp_server.py` near `configure_popup_modal_layer`:

`_fetch_plugin_arg_names(source)`

Hits `${TETHYSDASH_BASE_URL}/visualizations/list/` (same endpoint as `list_intake_plugins`). Returns:

  • `list[str]` when source found with declared args
  • `[]` when source found but has no declared args (Default registry types like Map/Text whose args are open-shape)
  • `None` when source not found OR fetch failed

`_normalize_args_case(args, arg_names)`

Case-fold matches LLM-emitted args keys against `arg_names`. Rewrites mismatched keys to canonical case. Soft rules:

  • `arg_names` is `None` / empty → pass through unchanged
  • Exact match → preserved
  • Case-fold match → rewrite (e.g., `river_id` → `river_ID`)
  • No match for a key → keep as-is (cheap path; runtime tile error boundary handles bad arg downstream)
  • Two LLM keys collide on case-fold → return `None` → structured envelope error

Wiring

In `configure_popup_modal_layer`'s gridItem normalization loop: for each gridItem, fetch its source's arg_names and normalize args in-place before `json.dumps` into `args_string`.

Soft-fails to pass-through when `TETHYSDASH_BASE_URL` is unset or fetch fails — better to ship the LLM's verbatim args than to reject the whole flow when the registry is unreachable.

Bonus: tighter h-hint per user feedback

User noted "the height can be reduced a bit" — LLM picked `h=40` which was visually too tall. Refined description's h guidance:

  • `h~25-30` for a single time-series plot (common case)
  • `h~35-40` for a tall plot or one with thick legends
  • `h~15-20` for a card or short text block
  • avoid `h>50` unless the popup will scroll

Tests

7 new in `TestPopupConfigArgsCaseNormalization`:

  • `test_lowercase_key_rewritten_to_canonical_case` — motivating real-user case
  • `test_exact_match_preserved`
  • `test_unknown_source_args_pass_through`
  • `test_default_registry_source_no_declared_args`
  • `test_fetch_failure_soft_fails_to_pass_through`
  • `test_case_fold_collision_rejected_with_structured_error`
  • `test_normalization_runs_per_gridItem_independently`

Full suite: 944 passed (937 baseline post-#13 + 7 new). No regression — existing tests pass because `TETHYSDASH_BASE_URL` is unset in the test env so `_fetch_plugin_arg_names` returns `None` and the helper passes through.

Trade-offs

  • Network call per `configure_popup_modal_layer` invocation (per gridItem source). No caching in v1; can add per-request memoization or TTL cache if observed latency justifies.
  • Soft network-failure behavior — when fetch fails, args pass through verbatim. The LLM's wrong-case key still gets persisted (current pre-fix behavior). Better than blocking the flow when the backend is briefly unreachable.
  • Doesn't generalize to the `attribute_variables` arg on `add_*_layer` tools (different bug class but same pattern). If users hit the same case-mismatch issue there, the helper is reusable.

Manual smoke after merge

Restart MCP server. Re-run turn-2 prompt. Expected:

  • args_string contains `"river_ID"` (capital ID) regardless of what the LLM emitted
  • Popup opens at runtime with data fetched
  • Edit Visualization modal "River ID" field populated with `${feature.comid}`
  • gridItem h closer to 25-30 (per the tighter description hint)

…s keys

Debug session 2026-05-21 third turn: gemini-flash STILL emitted
args = {"river_id": "${feature.comid}"} for the geoglows_forecast_plot
plugin despite PR #13's description-tightening that explicitly named
"case-sensitive" and gave the river_ID counterexample. Two confirmed
LLM failures on case normalization → description-only is insufficient
for this class of bug (per feedback_proactive_over_reactive_llm_routing
.md: "pattern-matching against open-vocabulary model output is a losing
fight"). Escalation path agreed in /ce-debug Option B: server-side
case-insensitive arg-name normalization.

## Implementation

Two new helpers in mcp_server.py:

1. _fetch_plugin_arg_names(source) — hits the same
   TETHYSDASH_BASE_URL/visualizations/list/ endpoint as
   list_intake_plugins, returns the declared arg_names list for the
   given source. Returns:
   - list[str] when source found with declared args
   - [] when source found but has no declared args (Default registry
     types like Map/Text whose args are open-shape)
   - None when source not found OR fetch failed (network down,
     malformed response, etc.)

2. _normalize_args_case(args, arg_names) — case-fold-matches LLM-
   emitted args keys against arg_names. Rules:
   - arg_names is None → pass through unchanged (don't reject; the
     LLM may know something we don't)
   - arg_names empty → pass through unchanged
   - Exact match → preserved
   - Case-fold match → rewrite to canonical case
   - No match for a key → keep as-is (cheap path — runtime tile error
     boundary handles the bad arg downstream)
   - Two LLM keys collide on case-fold → return None to signal
     structured envelope error

Wired into configure_popup_modal_layer's gridItem normalization loop:
for each gridItem, fetch its source's arg_names, normalize args
in-place before json.dumps. Soft-fails to pass-through behavior when
TETHYSDASH_BASE_URL is unset or fetch fails — better to ship the
LLM's verbatim args than to reject the whole flow when the registry
is unreachable.

## Bonus: tighter h-hint in description

Per user feedback "the height can be reduced a bit", refined the
position field description's h guidance:
- h~25-30 for a single time-series plot (common case)
- h~35-40 for a tall plot or one with thick legends
- h~15-20 for a card or short text block
- avoid h>50 unless the popup will scroll

The LLM had picked h=40 which was visually too tall; h=25-30 is the
right default for a typical geoglows-forecast-plot use case.

## Tests

7 new in TestPopupConfigArgsCaseNormalization:
- test_lowercase_key_rewritten_to_canonical_case (motivating case)
- test_exact_match_preserved
- test_unknown_source_args_pass_through
- test_default_registry_source_no_declared_args
- test_fetch_failure_soft_fails_to_pass_through
- test_case_fold_collision_rejected_with_structured_error
- test_normalization_runs_per_gridItem_independently

Plus 5 direct unit cases on the _normalize_args_case helper verified
during dev (exact, case-fold, None passthrough, empty passthrough,
unknown-key passthrough, collision).

Full suite: 944 passed (937 baseline including PR #13's 2 description
tests + 7 new case-normalize tests). No regression to existing
TestToolHappyPath / TestEnvelopeContract — TETHYSDASH_BASE_URL is
unset in the test env so _fetch_plugin_arg_names returns None and
passthrough preserves prior behavior.

## Trade-off acknowledged

Adds one HTTP fetch per configure_popup_modal_layer invocation (per
gridItem source). No caching in v1 — list_intake_plugins is the
authoritative source and the fetch is already what list_intake_plugins
does. Future optimization (per-request memoization or a TTL cache)
can be added if observed latency justifies.

Network failure mode is soft (warning logged, pass-through preserved),
so an unreachable backend doesn't block the popup-config flow — just
removes the safety net.
@romer8
romer8 merged commit 0de7a5f into main May 22, 2026
2 checks passed
@romer8
romer8 deleted the fix/popup-config-args-case-normalize branch May 22, 2026 01:12
romer8 added a commit that referenced this pull request May 22, 2026
…_dynamic_map_layer (#15)

Post-/ce-debug audit 2026-05-21: PR #14 fixed Bug B (LLM
snake-case-normalization habit) in configure_popup_modal_layer but
the same class of bug exists in render_plugin and add_dynamic_map_layer.
Both accept an args dict for an intake plugin; both passed it through
verbatim with zero normalization. LLM emits e.g. {"river_id": ...} when
the plugin declares river_ID → plugin runtime lookup fails silently.

This commit lifts the same normalization pattern (already implemented
in mcp_server.py via _fetch_plugin_arg_names + _normalize_args_case) and
wires it into both tools. No new helpers — direct re-use of PR #14's
shared infrastructure.

## render_plugin

- Insert _fetch_plugin_arg_names(source) + _normalize_args_case(args, ...)
  before building the visualization spec.
- Collision case (None return) → structured {error, fix_hint} envelope.
- Description tightening: replace the prior concrete example that read
  {"gauge_id": "${my_gauge}"} (which reinforced snake_case) with
  case-sensitivity prose that names the river_ID counterexample (an
  intentionally mixed-case canonical shape, NOT a value the LLM should
  copy verbatim per feedback_no_examples_in_tool_descriptions.md).

## add_dynamic_map_layer

- Same insertion: helpers called between _resolve_dynamic_map_layer_plugin
  and builder.set_plugin_source.
- Description tightening on the args field: case-sensitivity prose
  matching render_plugin.

## Audit findings (for record)

Class B (args case-fold) confirmed in render_plugin + add_dynamic_map_layer.
Class A (ESRI sublayer-name resolution) verified NOT applicable to
add_esri_feature_layer — React's getArcGISFeatureServiceLayerAttributes
keys by user-supplied layerName, consistent with server's attr_key=name.
ESRI Feature URL convention (.../FeatureServer/<layer_id>) makes the
layer implicit; no display-name vs service-name split.

Hidden secondary concern flagged for later: popup_options.aliases on
add_esri_image_layer may still have Class A issue (React popup-table
render path probably keys by ESRI sublayer name, not user's display
name). Not yet observed; will surface when popup-table-rename feature
is exercised next.

## Tests

8 new tests across 2 files, mirroring TestPopupConfigArgsCaseNormalization
pattern from PR #14:
- test_visualization_contracts.py::TestRenderPluginArgsCaseNormalization:
  - test_lowercase_key_rewritten_to_canonical_case
  - test_exact_match_preserved
  - test_fetch_failure_soft_fails_to_pass_through
  - test_case_fold_collision_rejected_with_structured_error
- test_layer_contracts.py::TestAddDynamicMapLayerArgsCaseNormalization:
  - same 4, parameterized for add_dynamic_map_layer

Full suite: 952 passed (944 baseline + 8 new). Zero regression — existing
TestRenderPlugin + TestAddDynamicMapLayer tests pass because
TETHYSDASH_BASE_URL is unset in the test env so _fetch_plugin_arg_names
returns None and the helper passes through.

## Soft-fail behavior preserved

When TETHYSDASH_BASE_URL is unset OR the fetch fails OR the source isn't
in the registry, args pass through unchanged (current pre-fix behavior).
Better to ship the LLM's verbatim args than to reject the whole flow
when the registry is briefly unreachable. The collision case (two LLM
keys map to the same canonical arg_name) is the only hard-fail path —
it indicates a genuine LLM mistake that the user should see.
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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant