From 97cfbf40011470c40b23cab25dab5e723b20c504 Mon Sep 17 00:00:00 2001 From: romer8 Date: Thu, 21 May 2026 15:01:56 -0600 Subject: [PATCH 1/2] feat(tools): configure_popup_modal_layer + matching @mcp.prompt MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adds an MCP surface for the custom popup-modal feature shipped on the tethysdash side 2026-05-13 (Aquaveo/tethysapp-tethys_dash#124). The new tool emits a single-op {patch_update} envelope with an RFC 6902 `add` op at `/args/layers//popupConfig`, re-using the existing apply_patch dispatch path in chatbox-core + DashboardLayout — no engine wiring change required. Surface: - New @mcp.tool `configure_popup_modal_layer(map_uuid, layer_index, popup_config)` - New @mcp.prompt `configure_popup_modal_layer` (slash-command counterpart) - 3 Pydantic models (_PopupConfigPayload, _PopupConfigPosition, _PopupConfigGridItemInput) with extra="forbid" to catch LLM old-shape leaks like `visualizationType` / `props` - `_popup_validation_error_envelope` converts Pydantic ValidationError to the structured {error, fix_hint, errors[]} envelope for one-cycle LLM repair Server-side normalization mirrors PopupLayoutEditor.js's canonical persisted shape — mint uuid4/i/id, stringify args→args_string and metadata→metadata_string — so round-trip parity with UI-authored popups is trivial (no React-side renormalization needed). ${feature.} and ${variable_name} template strings inside titleTemplate + gridItem args are preserved verbatim; substitution happens at render time inside the popup's FeatureScopedVariableInputs / VariableInputsContext scope. Tool description carries: - Mutual-exclusion clause (full-overwrite tool; partial edits go through patch_visualization per /args/layers/N/popupConfig/) per feedback_create_patch_mutual_exclusion.md - Same-turn race constraint (configure popups in a turn AFTER add_*_layer — dashboard_state snapshot is stale within the same turn) - ${feature.} syntax named abstractly (no concrete example values per feedback_no_examples_in_tool_descriptions.md) - Pointer to discovery tools (list_available_visualizations, list_intake_plugins, register_runtime_plugin) for valid gridItem source names across all three registries Tests: test_popup_modal_layer.py — 51 new tests across Pydantic shape validation, tool happy/error paths, envelope canonical-shape contract, whitelist coverage (proves /args/layers prefix already admits all deeper popupConfig sub-paths), slash-prompt scaffolding, and tool-description clause regression guards. Full suite: 890 tests pass. Tool catalog count: 25 → 26 (README + mcp_server.py prose updated; CHANGELOG entries for prior smoke-test runs left as historical record). Plan: docs/plans/2026-05-21-002-feat-tethysdash-mcps-popup-modal-surface-plan.md --- README.md | 2 +- test_mcp/test_popup_modal_layer.py | 612 +++++++++++++++++++++++++++++ tethysdash_mcp/mcp_server.py | 323 ++++++++++++++- 3 files changed, 932 insertions(+), 5 deletions(-) create mode 100644 test_mcp/test_popup_modal_layer.py diff --git a/README.md b/README.md index 79aab6e..eb9704c 100644 --- a/README.md +++ b/README.md @@ -137,7 +137,7 @@ With both servers running and the chatbox configured, work through these end-to- | Check | Prompt / Action | Expected | |---|---|---| -| Tool list | Open chatbox; observe tool count or slash-command popover | 25 tools surfaced (matches the standalone's `@mcp.tool` count) | +| Tool list | Open chatbox; observe tool count or slash-command popover | 26 tools surfaced (matches the standalone's `@mcp.tool` count) | | No-backend tool | `"create a card with title hello and description world showing the value 42"` | Card visualization renders on the dashboard | | Backend-touching tool | `"list available intake plugins"` | Returns the local tethysdash's intake plugins (proves `TETHYSDASH_BASE_URL` is reachable) | | Runtime plugin registration | Register a plugin via the chatbox sidebar's plugin-registration UI (NOT the `/register_runtime_plugin` slash command — that tool is feature-flagged off in standalone mode and returns a `registration_not_supported` envelope; plugins are registered through the browser-side UI which posts to tethysdash's `/runtime-plugins/sync/` endpoint with the user's session) | tethysdash's registry updates; the standalone reads the new plugin on its next `list_available_visualizations` call (no restart needed) | diff --git a/test_mcp/test_popup_modal_layer.py b/test_mcp/test_popup_modal_layer.py new file mode 100644 index 0000000..6a1552d --- /dev/null +++ b/test_mcp/test_popup_modal_layer.py @@ -0,0 +1,612 @@ +"""Contract tests for ``configure_popup_modal_layer`` MCP tool + prompt. + +Covers Units 1-4 of the popup-modal MCP surface plan: + +* Unit 1: Pydantic model shape validation (PopupConfigPayload + nested models) +* Unit 2: tool body — UUID validation, JSON-string coercion, normalization, + patch_update envelope construction +* Unit 3: @mcp.prompt counterpart shape +* Unit 4: end-to-end envelope contract — RFC 6902 add-op shape, canonical + persisted gridItem shape, whitelist prefix-match coverage for deep paths + +Layer 1 tests — no browser, no server, milliseconds per test. +""" + +import json +import uuid as uuid_mod + +import pytest + +from tethysdash_mcp.editable_schemas import is_path_allowed +from tethysdash_mcp.mcp_server import ( + _DEFAULT_GRID_ITEM_METADATA, + _DEFAULT_POPUP_POSITION, + _PopupConfigGridItemInput, + _PopupConfigPayload, + _PopupConfigPosition, + _prompt_configure_popup_modal_layer, + configure_popup_modal_layer, +) + + +# --------------------------------------------------------------------------- +# Helpers +# --------------------------------------------------------------------------- + + +def _fresh_uuid() -> str: + return str(uuid_mod.uuid4()) + + +def _minimal_gridItem(**overrides): + """Build a single gridItem with sensible defaults.""" + base = { + "source": "plotly", + "args": {"inlineData": {"data": [], "layout": {}}}, + "x": 0, + "y": 0, + "w": 12, + "h": 6, + } + base.update(overrides) + return base + + +def _minimal_payload(**overrides): + """Build a minimal valid popup_config payload (modal + one gridItem).""" + base = { + "mode": "modal", + "gridItems": [_minimal_gridItem()], + } + base.update(overrides) + return base + + +# --------------------------------------------------------------------------- +# Unit 1 — Pydantic shape validation +# --------------------------------------------------------------------------- + + +class TestPydanticModels: + """Direct shape validation against the Pydantic models.""" + + def test_minimal_payload_accepts(self): + m = _PopupConfigPayload(**_minimal_payload()) + assert m.mode == "modal" + assert m.position is None + assert m.titleTemplate == "" + assert len(m.gridItems) == 1 + + def test_full_payload_accepts(self): + m = _PopupConfigPayload( + mode="modal", + position={"leftPct": 10, "topPct": 10, "widthPct": 80, "heightPct": 80}, + titleTemplate="Site ${feature.station_name}", + gridItems=[ + _minimal_gridItem(), + _minimal_gridItem(x=12, y=0, source="Text", args={"text": "x"}), + ], + ) + assert isinstance(m.position, _PopupConfigPosition) + assert m.position.widthPct == 80 + assert len(m.gridItems) == 2 + + def test_position_below_size_min_rejected(self): + with pytest.raises(Exception): # pydantic.ValidationError + _PopupConfigPayload( + mode="modal", + position={"leftPct": 0, "topPct": 0, "widthPct": 10, "heightPct": 80}, + gridItems=[_minimal_gridItem()], + ) + + def test_position_above_100_rejected(self): + with pytest.raises(Exception): + _PopupConfigPayload( + mode="modal", + position={"leftPct": 150, "topPct": 0, "widthPct": 60, "heightPct": 60}, + gridItems=[_minimal_gridItem()], + ) + + def test_gridItem_negative_x_rejected(self): + with pytest.raises(Exception): + _PopupConfigPayload( + mode="modal", + gridItems=[_minimal_gridItem(x=-1)], + ) + + def test_gridItem_zero_w_rejected(self): + """w must be >= 1 (a zero-width gridItem can't render).""" + with pytest.raises(Exception): + _PopupConfigPayload( + mode="modal", + gridItems=[_minimal_gridItem(w=0)], + ) + + def test_gridItem_empty_source_rejected(self): + with pytest.raises(Exception): + _PopupConfigPayload( + mode="modal", + gridItems=[_minimal_gridItem(source="")], + ) + + def test_gridItem_non_dict_args_rejected(self): + with pytest.raises(Exception): + _PopupConfigPayload( + mode="modal", + gridItems=[_minimal_gridItem(args="not a dict")], + ) + + def test_empty_gridItems_rejected(self): + with pytest.raises(Exception): + _PopupConfigPayload(mode="modal", gridItems=[]) + + def test_mode_table_rejected(self): + """v1 supports only the modal mode; table mode lives on popup_options.aliases.""" + with pytest.raises(Exception): + _PopupConfigPayload(mode="table", gridItems=[_minimal_gridItem()]) + + def test_missing_mode_rejected(self): + with pytest.raises(Exception): + _PopupConfigPayload(gridItems=[_minimal_gridItem()]) + + def test_extra_field_on_gridItem_rejected(self): + """extra=forbid catches LLM-emitted unknown keys.""" + with pytest.raises(Exception): + _PopupConfigGridItemInput( + source="plotly", + args={}, + x=0, + y=0, + w=1, + h=1, + visualizationType="Plotly Chart", # LLM old-shape leak + ) + + def test_extra_field_on_payload_rejected(self): + with pytest.raises(Exception): + _PopupConfigPayload( + mode="modal", + gridItems=[_minimal_gridItem()], + extra_garbage_key="oops", + ) + + +# --------------------------------------------------------------------------- +# Unit 2 — tool body: happy paths +# --------------------------------------------------------------------------- + + +class TestToolHappyPath: + """Valid calls produce {patch_update: {uuid, source: 'Map', ops: [...]}}.""" + + def test_minimal_call_returns_patch_update(self): + result = configure_popup_modal_layer( + map_uuid=_fresh_uuid(), + layer_index=0, + popup_config=_minimal_payload(), + ) + assert "patch_update" in result + assert "error" not in result + + def test_envelope_source_is_Map(self): + result = configure_popup_modal_layer( + map_uuid=_fresh_uuid(), + layer_index=0, + popup_config=_minimal_payload(), + ) + assert result["patch_update"]["source"] == "Map" + + def test_single_op_only(self): + """v1 emits exactly one op — single-op atomicity (no partial-failure).""" + result = configure_popup_modal_layer( + map_uuid=_fresh_uuid(), + layer_index=0, + popup_config=_minimal_payload(), + ) + ops = result["patch_update"]["ops"] + assert len(ops) == 1 + assert ops[0]["op"] == "add" + + def test_path_uses_layer_index(self): + result = configure_popup_modal_layer( + map_uuid=_fresh_uuid(), + layer_index=3, + popup_config=_minimal_payload(), + ) + path = result["patch_update"]["ops"][0]["path"] + assert path == "/args/layers/3/popupConfig" + + def test_layer_index_zero_path(self): + result = configure_popup_modal_layer( + map_uuid=_fresh_uuid(), + layer_index=0, + popup_config=_minimal_payload(), + ) + assert result["patch_update"]["ops"][0]["path"] == "/args/layers/0/popupConfig" + + def test_two_gridItems_get_distinct_uuids_and_i(self): + result = configure_popup_modal_layer( + map_uuid=_fresh_uuid(), + layer_index=0, + popup_config=_minimal_payload( + gridItems=[ + _minimal_gridItem(), + _minimal_gridItem(x=12, source="Text", args={"text": "hi"}), + ] + ), + ) + gridItems = result["patch_update"]["ops"][0]["value"]["gridItems"] + assert len(gridItems) == 2 + assert gridItems[0]["uuid"] != gridItems[1]["uuid"] + assert gridItems[0]["i"] == "1" + assert gridItems[1]["i"] == "2" + + def test_json_string_popup_config_accepted(self): + """Some LLM providers stringify dict args — _coerce_json_strings handles it.""" + payload = _minimal_payload() + result = configure_popup_modal_layer( + map_uuid=_fresh_uuid(), + layer_index=0, + popup_config=json.dumps(payload), + ) + assert "patch_update" in result + assert "error" not in result + + def test_omitted_position_defaults_to_centered_60x60(self): + result = configure_popup_modal_layer( + map_uuid=_fresh_uuid(), + layer_index=0, + popup_config=_minimal_payload(), + ) + position = result["patch_update"]["ops"][0]["value"]["position"] + assert position == _DEFAULT_POPUP_POSITION + + def test_omitted_metadata_defaults_to_refresh_rate_zero(self): + result = configure_popup_modal_layer( + map_uuid=_fresh_uuid(), + layer_index=0, + popup_config=_minimal_payload(), + ) + gridItems = result["patch_update"]["ops"][0]["value"]["gridItems"] + metadata = json.loads(gridItems[0]["metadata_string"]) + assert metadata == _DEFAULT_GRID_ITEM_METADATA + + def test_omitted_titleTemplate_defaults_empty(self): + result = configure_popup_modal_layer( + map_uuid=_fresh_uuid(), + layer_index=0, + popup_config=_minimal_payload(), + ) + assert result["patch_update"]["ops"][0]["value"]["titleTemplate"] == "" + + def test_titleTemplate_preserves_feature_template_string(self): + """Template substitution happens at render time; the server preserves the string verbatim.""" + result = configure_popup_modal_layer( + map_uuid=_fresh_uuid(), + layer_index=0, + popup_config=_minimal_payload( + titleTemplate="Site ${feature.station_name}", + ), + ) + assert ( + result["patch_update"]["ops"][0]["value"]["titleTemplate"] + == "Site ${feature.station_name}" + ) + + def test_feature_template_inside_gridItem_args_preserved(self): + """`${feature.}` AND `${variable_name}` inside args survive json.dumps verbatim.""" + result = configure_popup_modal_layer( + map_uuid=_fresh_uuid(), + layer_index=0, + popup_config=_minimal_payload( + gridItems=[ + _minimal_gridItem( + source="GeoGLOWS Forecast Plot", + args={ + "River ID": "${feature.comid}", + "Dashboard Filter": "${siteName}", + }, + ) + ] + ), + ) + gridItems = result["patch_update"]["ops"][0]["value"]["gridItems"] + args = json.loads(gridItems[0]["args_string"]) + assert args["River ID"] == "${feature.comid}" + assert args["Dashboard Filter"] == "${siteName}" + + +# --------------------------------------------------------------------------- +# Unit 2 — tool body: error paths +# --------------------------------------------------------------------------- + + +class TestToolErrorPaths: + """Validation failures return {error, fix_hint} envelopes.""" + + def test_invalid_uuid_rejected(self): + result = configure_popup_modal_layer( + map_uuid="", # template placeholder + layer_index=0, + popup_config=_minimal_payload(), + ) + assert "error" in result + assert "patch_update" not in result + + def test_empty_gridItems_rejected_with_fix_hint(self): + result = configure_popup_modal_layer( + map_uuid=_fresh_uuid(), + layer_index=0, + popup_config={"mode": "modal", "gridItems": []}, + ) + assert "error" in result + assert "fix_hint" in result + assert "gridItem" in result["error"].lower() or "list" in result["error"].lower() + + def test_mode_carousel_rejected(self): + result = configure_popup_modal_layer( + map_uuid=_fresh_uuid(), + layer_index=0, + popup_config={"mode": "carousel", "gridItems": [_minimal_gridItem()]}, + ) + assert "error" in result + assert "fix_hint" in result + + def test_popup_config_none_rejected(self): + result = configure_popup_modal_layer( + map_uuid=_fresh_uuid(), + layer_index=0, + popup_config=None, + ) + assert "error" in result + assert "fix_hint" in result + + def test_popup_config_empty_string_rejected(self): + """Empty string passes _coerce_json_strings (json.loads('') raises JSONDecodeError).""" + result = configure_popup_modal_layer( + map_uuid=_fresh_uuid(), + layer_index=0, + popup_config="", + ) + assert "error" in result + + def test_popup_config_non_dict_rejected(self): + result = configure_popup_modal_layer( + map_uuid=_fresh_uuid(), + layer_index=0, + popup_config=42, + ) + assert "error" in result + assert "fix_hint" in result + + def test_negative_layer_index_rejected(self): + """Pydantic Field(ge=0) rejects negative layer_index via FastMCP validation + layer; we can't trigger this from the tool body directly because Annotated/ + Field validation runs before the function. Confirm the constraint exists. + """ + # Pydantic ConstrainedInt(ge=0) would raise if FastMCP passes -1 through; sanity- + # check via the model directly: + from typing_extensions import Annotated # noqa: F401 + from pydantic import Field, TypeAdapter + + adapter = TypeAdapter(Annotated[int, Field(ge=0)]) + with pytest.raises(Exception): + adapter.validate_python(-1) + + +# --------------------------------------------------------------------------- +# Unit 4 — Envelope contract round-trip +# --------------------------------------------------------------------------- + + +class TestEnvelopeContract: + """Server normalization produces the canonical persisted shape.""" + + def test_persisted_gridItem_has_canonical_fields(self): + result = configure_popup_modal_layer( + map_uuid=_fresh_uuid(), + layer_index=0, + popup_config=_minimal_payload(), + ) + gridItem = result["patch_update"]["ops"][0]["value"]["gridItems"][0] + expected_keys = { + "i", + "uuid", + "id", + "source", + "args_string", + "metadata_string", + "x", + "y", + "w", + "h", + } + assert set(gridItem.keys()) == expected_keys + + def test_persisted_gridItem_excludes_llm_input_shape_keys(self): + """No `args` (dict), `metadata` (dict), `visualizationType`, `props`, + or `position` at the gridItem level.""" + result = configure_popup_modal_layer( + map_uuid=_fresh_uuid(), + layer_index=0, + popup_config=_minimal_payload(), + ) + gridItem = result["patch_update"]["ops"][0]["value"]["gridItems"][0] + forbidden = {"args", "metadata", "visualizationType", "props", "position"} + assert forbidden.isdisjoint(gridItem.keys()) + + def test_args_string_is_json_encoded_dict(self): + result = configure_popup_modal_layer( + map_uuid=_fresh_uuid(), + layer_index=0, + popup_config=_minimal_payload( + gridItems=[_minimal_gridItem(args={"x": 1, "y": "two"})] + ), + ) + gridItem = result["patch_update"]["ops"][0]["value"]["gridItems"][0] + assert isinstance(gridItem["args_string"], str) + assert json.loads(gridItem["args_string"]) == {"x": 1, "y": "two"} + + def test_metadata_string_is_json_encoded_dict(self): + result = configure_popup_modal_layer( + map_uuid=_fresh_uuid(), + layer_index=0, + popup_config=_minimal_payload( + gridItems=[_minimal_gridItem(metadata={"refreshRate": 30})] + ), + ) + gridItem = result["patch_update"]["ops"][0]["value"]["gridItems"][0] + assert json.loads(gridItem["metadata_string"]) == {"refreshRate": 30} + + def test_persisted_uuid_is_string_uuid4(self): + result = configure_popup_modal_layer( + map_uuid=_fresh_uuid(), + layer_index=0, + popup_config=_minimal_payload(), + ) + gridItem = result["patch_update"]["ops"][0]["value"]["gridItems"][0] + # round-trips through uuid_mod.UUID — confirms valid uuid4 string + parsed = uuid_mod.UUID(gridItem["uuid"]) + assert parsed.version == 4 + + def test_persisted_id_is_None(self): + """Matches PopupLayoutEditor.js seed (id is the SQLAlchemy primary-key placeholder).""" + result = configure_popup_modal_layer( + map_uuid=_fresh_uuid(), + layer_index=0, + popup_config=_minimal_payload(), + ) + gridItem = result["patch_update"]["ops"][0]["value"]["gridItems"][0] + assert gridItem["id"] is None + + def test_recall_mints_fresh_uuids(self): + """Destructive replace: re-calling produces new gridItem UUIDs (KTD #8).""" + map_uuid = _fresh_uuid() + first = configure_popup_modal_layer( + map_uuid=map_uuid, + layer_index=0, + popup_config=_minimal_payload(), + ) + second = configure_popup_modal_layer( + map_uuid=map_uuid, + layer_index=0, + popup_config=_minimal_payload(), + ) + u1 = first["patch_update"]["ops"][0]["value"]["gridItems"][0]["uuid"] + u2 = second["patch_update"]["ops"][0]["value"]["gridItems"][0]["uuid"] + assert u1 != u2 + + +class TestWhitelistCoverage: + """The Map source whitelist's /args/layers prefix already covers deep paths.""" + + def test_path_to_popupConfig_allowed(self): + assert is_path_allowed("Map", "/args/layers/0/popupConfig") + + def test_path_to_popupConfig_title_allowed(self): + """patch_visualization on this sub-path must continue to work (R8).""" + assert is_path_allowed("Map", "/args/layers/0/popupConfig/titleTemplate") + + def test_path_to_gridItem_args_string_allowed(self): + """Deepest path the tool writes to — proves prefix-match covers it.""" + assert is_path_allowed( + "Map", "/args/layers/3/popupConfig/gridItems/0/args_string" + ) + + def test_path_to_position_widthPct_allowed(self): + assert is_path_allowed( + "Map", "/args/layers/0/popupConfig/position/widthPct" + ) + + def test_path_to_nonexistent_source_outside_layers_rejected(self): + """Sanity: a path under /args/legend (NOT in Map whitelist) is rejected.""" + assert not is_path_allowed("Map", "/args/legend/title") + + +# --------------------------------------------------------------------------- +# Unit 3 — Slash prompt +# --------------------------------------------------------------------------- + + +class TestSlashPrompt: + """Prompt scaffolds the tool call with provided args.""" + + def test_prompt_returns_string(self): + body = _prompt_configure_popup_modal_layer( + map_uuid=_fresh_uuid(), + layer_index="0", + popup_config=json.dumps(_minimal_payload()), + ) + assert isinstance(body, str) + assert "configure" in body.lower() or "popup" in body.lower() + + def test_prompt_includes_supplied_args(self): + """The scaffolded prompt references the user-supplied identifiers.""" + my_uuid = _fresh_uuid() + body = _prompt_configure_popup_modal_layer( + map_uuid=my_uuid, + layer_index="3", + popup_config="{}", + ) + assert my_uuid in body + assert "3" in body + + +# --------------------------------------------------------------------------- +# Tool description contract — keep documentation-shaping clauses in sync +# --------------------------------------------------------------------------- + + +class TestToolDescription: + """Description text carries the load-bearing prose clauses.""" + + @pytest.fixture(scope="class") + def description(self): + """Fetch the registered tool's description via FastMCP's local provider. + + Mirrors the pattern in test_tool_description_exclusivity.py to + bypass middleware/transforms and read the raw registered string. + """ + import asyncio + + from tethysdash_mcp.mcp_server import mcp + + async def go(): + return await mcp._local_provider.list_tools() + + loop = asyncio.new_event_loop() + try: + tools = loop.run_until_complete(go()) + finally: + loop.close() + by_name = {t.name: (t.description or "") for t in tools} + return by_name["configure_popup_modal_layer"] + + def test_description_names_mutual_exclusion(self, description): + """Per feedback_create_patch_mutual_exclusion.md.""" + d = description.lower() + assert "do not use this tool to edit" in d + assert "patch_visualization" in description + + def test_description_names_same_turn_race_constraint(self, description): + """KTD #6 / Unit 2 test scenario.""" + d = description.lower() + assert "after" in d + assert "add_" in description or "dashboard_state" in d + + def test_description_names_feature_template_abstractly(self, description): + """Names `${feature.}` syntax without concrete example values.""" + assert "${feature.}" in description + # Heuristic for "no concrete example value": no `${feature.station_name}` + # or other resolved-attribute names in the description. (Test scenarios + # use concrete values; the tool description itself must not.) + assert "${feature.station_name}" not in description + assert "${feature.comid}" not in description + + def test_description_points_at_source_discovery_tools(self, description): + assert "list_available_visualizations" in description + assert "list_intake_plugins" in description + + def test_description_names_full_overwrite_semantic(self, description): + d = description.lower() + assert "replaces" in d diff --git a/tethysdash_mcp/mcp_server.py b/tethysdash_mcp/mcp_server.py index 89544d7..3b7abf9 100644 --- a/tethysdash_mcp/mcp_server.py +++ b/tethysdash_mcp/mcp_server.py @@ -25,9 +25,9 @@ import json import re import uuid -from typing import Optional, Dict, Any, List, Union +from typing import Optional, Dict, Any, List, Literal, Union from typing_extensions import Annotated -from pydantic import Field +from pydantic import BaseModel, ConfigDict, Field, ValidationError from pydantic.functional_validators import BeforeValidator from fastmcp import FastMCP @@ -66,7 +66,7 @@ # + engine/embeddings.js) already runs per-prompt semantic-similarity # ranking using @huggingface/transformers on any server it classifies # as full-catalog with >= SMALL_CATALOG_THRESHOLD (8) tools. Tethysdash - # has 25 tools, well above that threshold, so the embedding ranker is + # has 26 tools, well above that threshold, so the embedding ranker is # the authoritative selection layer regardless of what BM25SearchTransform # would have done on the server side. # @@ -3346,6 +3346,268 @@ def _emit_rejection_telemetry( LOGGER.info("patch_rejection %s", payload) +# --------------------------------------------------------------------------- +# configure_popup_modal_layer — Pydantic models + tool +# --------------------------------------------------------------------------- +# +# Configures the custom-popup-modal feature shipped on the tethysdash side +# 2026-05-13. Each map layer's persisted config can carry a popupConfig +# object that drives a click-popup modal with an embedded grid of +# visualizations. The MCP tool emits a single-op {patch_update} envelope +# targeting /args/layers//popupConfig, re-using the existing +# apply_patch dispatch path in chatbox-core + DashboardLayout (no engine +# wiring change). +# +# Models below mirror the canonical persisted shape consumed by +# PopupConfigPane.js + DashboardLayout, so round-trip parity with a +# UI-configured popup is trivial. Server-side normalization fills in the +# fields the LLM shouldn't have to know about (uuid, i, id, stringified +# args/metadata). + + +class _PopupConfigPosition(BaseModel): + """Modal popup position as percentage of viewport (matches PopupConfigPane.js).""" + + model_config = ConfigDict(extra="forbid") + + leftPct: Annotated[float, Field(ge=0, le=100)] + topPct: Annotated[float, Field(ge=0, le=100)] + widthPct: Annotated[float, Field(ge=20, le=100)] + heightPct: Annotated[float, Field(ge=20, le=100)] + + +class _PopupConfigGridItemInput(BaseModel): + """One embedded visualization inside a popup modal (LLM-facing shape). + + The server transforms this into the canonical persisted shape + (``source``, ``args_string``, ``metadata_string``, ``uuid``, ``i``, + ``id``, ``x/y/w/h``) before writing into the layer config. Per-source- + type ``args`` validation is intentionally NOT enforced here — runtime + errors surface via the existing per-tile error boundary. + """ + + model_config = ConfigDict(extra="forbid") + + source: Annotated[str, Field(min_length=1)] + args: Dict[str, Any] + x: Annotated[int, Field(ge=0)] + y: Annotated[int, Field(ge=0)] + w: Annotated[int, Field(ge=1)] + h: Annotated[int, Field(ge=1)] + metadata: Optional[Dict[str, Any]] = None + + +class _PopupConfigPayload(BaseModel): + """Custom popup modal config for a single map layer. + + Mode is locked to ``"modal"`` in v1; the table-mode popup + configuration uses the existing ``popup_options.{aliases, omit}`` + path on the ``add_*_layer`` tools. + """ + + model_config = ConfigDict(extra="forbid") + + mode: Literal["modal"] + position: Optional[_PopupConfigPosition] = None + titleTemplate: str = "" + gridItems: Annotated[List[_PopupConfigGridItemInput], Field(min_length=1)] + + +# Centered 60x60 viewport rect — matches DEFAULT_POSITION at +# reactapp/components/modals/MapLayer/PopupConfigPane.js:24. +_DEFAULT_POPUP_POSITION: Dict[str, float] = { + "leftPct": 20.0, + "topPct": 20.0, + "widthPct": 60.0, + "heightPct": 60.0, +} + +# Matches the React UI default for newly-authored popup gridItems +# (PopupLayoutEditor.js:182). +_DEFAULT_GRID_ITEM_METADATA: Dict[str, Any] = {"refreshRate": 0} + + +def _popup_validation_error_envelope(ve: ValidationError) -> Dict[str, Any]: + """Convert a Pydantic ValidationError into a structured tool envelope. + + Names the failing field locations + the validator that fired so the + LLM can self-repair in one cycle (Plan SC3, R6). Caps the number of + enumerated errors to keep the envelope compact. + """ + errors = [] + for err in ve.errors()[:8]: + loc = ".".join(str(p) for p in err.get("loc", ())) + errors.append({ + "field": loc or "", + "type": err.get("type", "value_error"), + "msg": err.get("msg", "validation failed"), + }) + return { + "error": "invalid_popup_config: " + "; ".join( + f"{e['field']} {e['msg']}" for e in errors + ), + "fix_hint": ( + "popup_config must be {mode: 'modal', gridItems: [...non-empty...], " + "position?: {leftPct, topPct, widthPct, heightPct}, " + "titleTemplate?: str}. Each gridItem needs {source, args (dict), " + "x, y, w, h, metadata?}. See tool description for the full shape." + ), + "errors": errors, + } + + +@mcp.tool( + name="configure_popup_modal_layer", + description=( + "Configure a custom popup modal on an existing map layer. Use when " + "the user wants a click-popup that renders embedded visualizations " + "(plots, tables, cards, text) with feature-attribute-substituted " + "props, instead of (or alongside) the default attribute-table popup. " + "Required: map_uuid (from create_map_visualization or dashboard_state), " + "layer_index (0-based index into the map's layers, read from " + "dashboard_state), and popup_config. Returns a patch_update envelope " + "the chatbox engine applies via the existing apply_patch dispatch path. " + "Configure the popup in a turn AFTER the layer was added by an " + "add_*_layer tool — a same-turn add does NOT yet appear in " + "dashboard_state, so layer_index would be stale and the patch would " + "land on the wrong layer. " + "Template strings inside popup_config (titleTemplate and any string " + "value inside a gridItem's args dict) may embed ${feature.} " + "tokens that substitute against the clicked feature's attributes at " + "popup-render time; missing keys resolve to empty string. " + "Discover valid gridItem source names via list_available_visualizations " + "(Default registry — Map, Text, Card, etc.), list_intake_plugins " + "(intake-backed plugins), or register_runtime_plugin (runtime/MFE plugins). " + "DO NOT use this tool to edit fields on an existing popupConfig — use " + "patch_visualization on the specific sub-path (e.g., " + "/args/layers/N/popupConfig/titleTemplate) instead. Re-calling this " + "tool REPLACES the entire popupConfig including all gridItem UUIDs." + ), + tags=["map", "layer", "popup", "modal", "configure", "click", "feature"], +) +def configure_popup_modal_layer( + map_uuid: Annotated[ + str, + Field(description=( + "UUID of the target map visualization. Source: dashboard_state " + "or the return value of create_map_visualization." + )), + ], + layer_index: Annotated[ + int, + Field(ge=0, description=( + "0-based index into the map's layers array (read from " + "dashboard_state). Configure popups in a turn AFTER the layer " + "was added by add_*_layer — a same-turn add does not yet appear " + "in dashboard_state." + )), + ], + popup_config: Annotated[ + Union[Dict[str, Any], str], + Field(description=( + "Full popup modal payload. Shape: {mode: 'modal', position?: " + "{leftPct, topPct, widthPct, heightPct} percentages 0-100 " + "(widthPct/heightPct clamped >= 20), titleTemplate?: str (may " + "embed ${feature.} tokens), gridItems: list of at least one " + "{source: str, args: dict (values may embed ${feature.} " + "tokens), x: int >=0, y: int >=0, w: int >=1, h: int >=1, " + "metadata?: dict}}. Accepts both Dict and JSON-string Dict." + )), + ], +) -> Dict[str, Any]: + """Build a {patch_update} envelope writing popup_config into the layer. + + Server-side validation flow: + 1. UUID format check on map_uuid + 2. JSON-string -> dict coercion for popup_config + 3. Pydantic shape validation against _PopupConfigPayload + 4. Server-side normalization: mint uuid4/i/id, stringify args/metadata + 5. Construct single RFC 6902 add op at /args/layers//popupConfig + + Returns ``{patch_update: {uuid, source: 'Map', ops: [single_add_op]}}`` + on success or ``{error: "...", fix_hint: "..."}`` on validation failure. + """ + uuid_error = _validate_uuid_arg( + map_uuid, + "map_uuid", + "create_map_visualization (or dashboard_state)", + ) + if uuid_error: + return {"error": uuid_error} + + coercible = {"popup_config": popup_config} + coerce_err = _coerce_json_strings(coercible) + if coerce_err: + return coerce_err + popup_config_dict = coercible["popup_config"] + + if not isinstance(popup_config_dict, dict): + return { + "error": "invalid_popup_config: must be a dict (or JSON-string dict).", + "fix_hint": ( + "Pass popup_config as a JSON object with at minimum {mode: 'modal', " + "gridItems: [...]} plus optional position and titleTemplate." + ), + } + + try: + payload = _PopupConfigPayload(**popup_config_dict) + except ValidationError as ve: + return _popup_validation_error_envelope(ve) + + position_dict = ( + payload.position.model_dump() + if payload.position is not None + else dict(_DEFAULT_POPUP_POSITION) + ) + + gridItems_persisted: List[Dict[str, Any]] = [] + for idx, item in enumerate(payload.gridItems): + metadata_dict = ( + item.metadata if item.metadata is not None else dict(_DEFAULT_GRID_ITEM_METADATA) + ) + gridItems_persisted.append({ + "i": str(idx + 1), + "uuid": str(uuid.uuid4()), + "id": None, + "source": item.source, + "args_string": json.dumps(item.args), + "metadata_string": json.dumps(metadata_dict), + "x": item.x, + "y": item.y, + "w": item.w, + "h": item.h, + }) + + popup_config_persisted = { + "mode": payload.mode, + "position": position_dict, + "titleTemplate": payload.titleTemplate, + "gridItems": gridItems_persisted, + } + + LOGGER.info( + "configure_popup_modal_layer map_uuid=%s layer_index=%d gridItems=%d", + map_uuid, + layer_index, + len(gridItems_persisted), + ) + + return { + "patch_update": { + "uuid": map_uuid, + "source": "Map", + "ops": [ + { + "op": "add", + "path": f"/args/layers/{layer_index}/popupConfig", + "value": popup_config_persisted, + } + ], + } + } + + @mcp.tool( name="patch_visualization", description=( @@ -4630,6 +4892,59 @@ def _prompt_register_runtime_plugin( ) +@mcp.prompt(name="configure_popup_modal_layer") +def _prompt_configure_popup_modal_layer( + map_uuid: Annotated[ + str, + Field( + description=( + "UUID of the target map visualization (from dashboard_state " + "or the return of create_map_visualization)." + ), + ), + ], + layer_index: Annotated[ + str, + Field( + description=( + "0-based index into the map's layers array, as a string " + "(FastMCP prompt args are string-typed). Read the index from " + "dashboard_state." + ), + ), + ], + popup_config: Annotated[ + str, + Field( + description=( + "Popup modal payload as a JSON object string. Shape: " + "{mode: 'modal', position?: {leftPct, topPct, widthPct, " + "heightPct}, titleTemplate?: str, gridItems: [{source, " + "args, x, y, w, h, metadata?}, ...]}. The tool accepts " + "both Dict and JSON-string Dict; pass either." + ), + ), + ], +) -> str: + """Scaffold a configure_popup_modal_layer tool call. + + Drives the ``configure_popup_modal_layer`` tool. Use after the layer + was added by an ``add_*_layer`` tool — a same-turn add does not yet + appear in ``dashboard_state``, so ``layer_index`` would be stale. + + Template strings inside ``popup_config`` (``titleTemplate`` and any + string value inside a gridItem's ``args``) may embed + ``${feature.}`` tokens that substitute against the clicked + feature's attributes at popup-render time; missing keys resolve to + empty string. + """ + return ( + f"Configure a custom popup modal for the layer at index " + f"{layer_index} on map {map_uuid}. Apply the popup_config payload: " + f"{popup_config}." + ) + + @mcp.prompt(name="patch_visualization") def _prompt_patch_visualization( uuid: Annotated[ @@ -4691,7 +5006,7 @@ def _prompt_patch_visualization( # enforces this. # # Phase 3c probe (commit a739750) removed BM25SearchTransform, so all -# 25 tools are visible to chatbox-core's embedding ranker. No +# 26 tools are visible to chatbox-core's embedding ranker. No # always_visible pinning is needed for any of these layer prompts. # --------------------------------------------------------------------------- From bf372d179224bc874c4f9896092a49b6fad56c59 Mon Sep 17 00:00:00 2001 From: romer8 Date: Thu, 21 May 2026 16:13:05 -0600 Subject: [PATCH 2/2] fix(tools): lead configure_popup_modal_layer description with positive first-time-setup framing MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Debug session 2026-05-21 turn 2 with gemini-flash: asked to 'Enable the Custom Popup Modal on the China Flowlines layer...' (a first-time popup-modal setup), the LLM routed to patch_visualization instead of configure_popup_modal_layer, then fabricated: - wrong path: /args/layers/0/configuration/props/popup (canonical is /args/layers/0/popupConfig) - wrong value shape: {content, title, type} instead of {mode, position, titleTemplate, gridItems} Two reasons the LLM picked the wrong tool: 1. The system prompt's PRIORITY clause sends 'modify existing' → patch_visualization. 'Enable popup on existing layer' reads as modify-existing. (System-prompt fix is a sibling PR in tethysdash: add a SECOND exception to the PRIORITY clause for configure_popup_modal_layer.) 2. configure_popup_modal_layer's description LED with the exclusion clause ('DO NOT use this tool to edit fields on an existing popupConfig — use patch_visualization on the specific sub-path'), which the LLM appears to have read as 'don't use this tool for anything that touches an existing layer.' This commit fixes #2 by restructuring the description to lead with positive use: 'USE THIS TOOL for FIRST-TIME popup-modal setup on a map layer. It is THE correct tool whenever the user asks to enable, add, configure, create, or set up a Custom Popup Modal on an existing map layer — even though the map layer itself already exists. Adding a popupConfig to a layer for the first time is NOT modifying an existing visualization in the patch_visualization sense; it's a structured setup operation with its own tool.' Plus an explicit anti-pattern call-out: 'DO NOT use patch_visualization to add a popupConfig from scratch — the popupConfig shape is non-trivial (mode, position, titleTemplate, gridItems with source/args/x/y/w/h), the canonical path is /args/layers/N/popupConfig (not /args/layers/N/configuration/...), and this tool builds the persisted shape correctly.' The patch_visualization carve-out is preserved but scoped to its real case: partial edits to an already-existing popupConfig. Tests: split the prior test_description_names_mutual_exclusion into two assertions — one for positive-use leading (test_description_leads_with_ positive_use), one for the partial-edits-only carve-out (test_description_names_patch_visualization_carveout). Other 50 tests in TestPopupModalLayer / TestToolHappyPath / TestToolErrorPaths / TestEnvelopeContract / TestWhitelistCoverage / TestSlashPrompt / TestToolDescription still pass. Full suite: 891 passed. --- test_mcp/test_popup_modal_layer.py | 26 +++++++++++++++++++++++--- tethysdash_mcp/mcp_server.py | 30 +++++++++++++++++++++--------- 2 files changed, 44 insertions(+), 12 deletions(-) diff --git a/test_mcp/test_popup_modal_layer.py b/test_mcp/test_popup_modal_layer.py index 6a1552d..aed13de 100644 --- a/test_mcp/test_popup_modal_layer.py +++ b/test_mcp/test_popup_modal_layer.py @@ -582,11 +582,31 @@ async def go(): by_name = {t.name: (t.description or "") for t in tools} return by_name["configure_popup_modal_layer"] - def test_description_names_mutual_exclusion(self, description): - """Per feedback_create_patch_mutual_exclusion.md.""" + def test_description_leads_with_positive_use(self, description): + """Description must lead with FIRST-TIME setup framing. + + Debug session 2026-05-21 turn 2: gemini-flash read the prior + exclusion-first phrasing ("DO NOT use this tool to edit...") as + "don't use configure_popup_modal_layer for anything that touches + an existing layer" and routed to patch_visualization. Positive- + first framing fixes this. + """ + d = description.lower() + assert "use this tool for first-time popup-modal setup" in d, ( + "Description must lead with the positive use case so the LLM " + "picks this tool for first-time popup-modal setup." + ) + + def test_description_names_patch_visualization_carveout(self, description): + """Partial-edits-only clause for patch_visualization survives.""" d = description.lower() - assert "do not use this tool to edit" in d + # The patch_visualization carve-out must be present, but it must + # NOT be the leading framing (covered by test_description_leads_with_positive_use). assert "patch_visualization" in description + assert ( + "use patch_visualization only for partial edits" in d + or "only for partial edits to an already-existing" in d + ) def test_description_names_same_turn_race_constraint(self, description): """KTD #6 / Unit 2 test scenario.""" diff --git a/tethysdash_mcp/mcp_server.py b/tethysdash_mcp/mcp_server.py index 3b7abf9..ec41631 100644 --- a/tethysdash_mcp/mcp_server.py +++ b/tethysdash_mcp/mcp_server.py @@ -3459,10 +3459,26 @@ def _popup_validation_error_envelope(ve: ValidationError) -> Dict[str, Any]: @mcp.tool( name="configure_popup_modal_layer", description=( - "Configure a custom popup modal on an existing map layer. Use when " - "the user wants a click-popup that renders embedded visualizations " - "(plots, tables, cards, text) with feature-attribute-substituted " - "props, instead of (or alongside) the default attribute-table popup. " + "USE THIS TOOL for FIRST-TIME popup-modal setup on a map layer. " + "It is THE correct tool whenever the user asks to 'enable', 'add', " + "'configure', 'create', or 'set up' a Custom Popup Modal on an " + "existing map layer — even though the map layer itself already " + "exists. Adding a popupConfig to a layer for the first time is " + "NOT 'modifying an existing visualization' in the patch_visualization " + "sense; it's a structured setup operation with its own tool. " + "DO NOT use patch_visualization to add a popupConfig from scratch — " + "the popupConfig shape is non-trivial (mode, position, titleTemplate, " + "gridItems with source/args/x/y/w/h), the canonical path is " + "/args/layers/N/popupConfig (not /args/layers/N/configuration/...), " + "and this tool builds the persisted shape correctly. " + "Use patch_visualization ONLY for partial edits to an ALREADY-EXISTING " + "popupConfig (e.g., changing just the titleTemplate via " + "/args/layers/N/popupConfig/titleTemplate). Re-calling this tool " + "REPLACES the entire popupConfig including all gridItem UUIDs. " + "What it does: builds a custom click-popup that renders embedded " + "visualizations (plots, tables, cards, text) with feature-attribute-" + "substituted props, instead of (or alongside) the default attribute-" + "table popup. " "Required: map_uuid (from create_map_visualization or dashboard_state), " "layer_index (0-based index into the map's layers, read from " "dashboard_state), and popup_config. Returns a patch_update envelope " @@ -3477,11 +3493,7 @@ def _popup_validation_error_envelope(ve: ValidationError) -> Dict[str, Any]: "popup-render time; missing keys resolve to empty string. " "Discover valid gridItem source names via list_available_visualizations " "(Default registry — Map, Text, Card, etc.), list_intake_plugins " - "(intake-backed plugins), or register_runtime_plugin (runtime/MFE plugins). " - "DO NOT use this tool to edit fields on an existing popupConfig — use " - "patch_visualization on the specific sub-path (e.g., " - "/args/layers/N/popupConfig/titleTemplate) instead. Re-calling this " - "tool REPLACES the entire popupConfig including all gridItem UUIDs." + "(intake-backed plugins), or register_runtime_plugin (runtime/MFE plugins)." ), tags=["map", "layer", "popup", "modal", "configure", "click", "feature"], )