fix(tools): apply case-fold args normalization to render_plugin + add_dynamic_map_layer - #15
Merged
romer8 merged 1 commit intoMay 22, 2026
Conversation
…_dynamic_map_layer 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.
This was referenced May 22, 2026
Merged
romer8
deleted the
fix/render-plugin-and-dynamic-map-layer-args-case-normalize
branch
May 22, 2026 02:28
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
Post-/ce-debug audit identified the same LLM snake-case bug from PR #14 in two more tools that accept plugin `args` dicts: `render_plugin` and `add_dynamic_map_layer`. This PR wires the existing `_fetch_plugin_arg_names` + `_normalize_args_case` helpers into both, plus tightens their descriptions.
Why these two
Audit summary table:
Changes
`render_plugin` (`mcp_server.py:4304-4350`)
`add_dynamic_map_layer` (`mcp_server.py:4213-4285`)
Tests
8 new tests, 4 per tool, mirroring `TestPopupConfigArgsCaseNormalization` from PR #14:
Each class covers: lowercase→canonical rewrite, exact preserved, fetch-failure soft-fail pass-through, case-fold collision rejected with structured envelope.
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 preserved
When `TETHYSDASH_BASE_URL` is unset OR fetch fails OR source isn't in the registry, args pass through unchanged (matches pre-fix behavior). Only the case-fold collision is a hard-fail path.
Audit by-product (for record)
While verifying, also confirmed that `add_esri_feature_layer` is NOT affected by Class A (ESRI sublayer-name resolution). React's `getArcGISFeatureServiceLayerAttributes` keys by user-supplied layerName, consistent with server's `attr_key=name`. ESRI Feature URL convention makes the layer implicit; no display-name vs service-name split exists.
Flagged for later: `popup_options.aliases` on `add_esri_image_layer` may still have a Class A issue (React popup-table render path probably keys by ESRI sublayer name, not user's display name). Not observed yet; will surface when popup-table-rename feature is exercised.
Manual smoke after merge
Restart MCP server (with `TETHYSDASH_BASE_URL` set). Try a prompt like:
Expected: `render_plugin` called with `args = {"river_id": "12345"}`; server fetches `arg_names = ["river_ID"]`; persisted `args` contains `{"river_ID": "12345"}`. Plot fetches correctly.