fix: directions_tool map preview fetches its own route from the Directions API

[zmofei]

Summary

• directions_tool always attaches a map preview UI, but until now it could only draw a route when the tool's own response happened to carry geometries="geojson" — with the default geometries="none" the map had no route data and showed an error, so correctness depended on the calling model guessing the right parameter.
• This PR started as a prompt-only mitigation (strengthening the tool/geometries descriptions) and now includes the more robust follow-up: the map preview decouples entirely from geometries by fetching its own route directly from the Mapbox Directions API using the tool call's input parameters (coordinates, routing profile, exclude, depart_at/arrive_by, vehicle dimensions), always forcing geometries=geojson for itself. The tool's own response can stay compact with geometries="none" by default without ever breaking the preview.
• Covers both UI ingress paths:
  • MCP Apps (ui://mapbox/directions-app/index.html): listens for ui/notifications/tool-input and self-fetches as soon as it arrives, in parallel with the real tool call executing server-side. ui/notifications/tool-result remains as a fallback for hosts that don't send tool-input, guarded against double-rendering.
  • Legacy MCP-UI (inline rawHtml): bakes the call's input params (not response geometry) into initialData and attaches the inline UI unconditionally, including for the large-response (>50KB) branch, which previously never attached a UI resource at all.
• Extracts buildDirectionsRequestUrl() as the single server-side source of truth for the Directions request URL, and hand-ports an equivalent in the iframe's vanilla JS (no bundler for the iframe HTML), kept in sync by a parity test that runs the iframe script in a Node vm sandbox and asserts byte-identical output against the server function.
• Fixes a race condition where a premature "could not find route data" error could show — and stick on screen, since nothing cleared it — if tool-result arrived before the self-fetch resolved.
• Simplifies the tool and geometries parameter descriptions now that the map no longer depends on which geometries value was requested.

Test plan

• npm test — 757/757 passing
• npm run build — clean
• npx eslint / npx prettier --check — clean on all touched files
• Manual/live check: confirm the baked-in public (pk.*) token is authorized to call the Directions API directly from the browser (standard Mapbox usage, not yet confirmed against a real account)
• Confirm target MCP Apps hosts reliably send ui/notifications/tool-input; hosts that don't fall back to the pre-existing tool-result-based behavior (no regression, but self-fetch's latency benefit d

oesn't apply)

[mattpodwysocki]

Summary

Reviewed this end-to-end (diff read line-by-line + 8 independent finder passes + 1-vote verification per candidate). This is a real fix for a real bug (map preview breaking on the default geometries="none"), and the self-fetch approach + parity test is a reasonable way to decouple the two. A few things surfaced that are worth addressing before merge, most importantly #1 and #2 below.

1. routeRendered is set once and never reset — permanently blocks all renders after the first (CONFIRMED)

src/resources/ui-apps/directionsAppHtml.ts: routeRendered is declared false, set to true inside renderRoute(), and never reset anywhere in the file (checked the full file, not just the diff hunks — grepped all 7 occurrences). Every subsequent tool-input/tool-result delivery to the same iframe instance now silently no-ops via the if (routeRendered) return; guards.

This directly contradicts the pre-existing comment two lines above the new flag: "Track markers across re-renders so we can remove them before drawing the next route — otherwise a second tool-result delivery stacks pins" — which documents that a second route delivery to the same iframe (e.g. a host reusing one UI resource across two sequential directions_tool calls in a conversation) was expected to work. Post-PR, that second delivery is dropped entirely: no update, no error, nothing.

Suggest replacing the two independent booleans (routeRendered, selfFetchStarted) with a single state variable that resets to 'idle' whenever a new tool-input/initial-data delivery starts (not just once globally) — otherwise reusing the iframe for a follow-up route (e.g. "avoid tolls this time") will silently show the first route forever.

2. No event.origin check on the postMessage listener, combined with unvalidated client-side input (CONFIRMED)

src/resources/ui-apps/directionsAppHtml.ts: the window.addEventListener('message', ...) handler never checks event.origin, so any frame holding a reference to this iframe can post a fake ui/notifications/tool-input with arbitrary arguments. Those flow unvalidated into buildDirectionsApiUrl:

• routing_profile is spliced directly into the URL path with no allow-list/encoding (server enforces a 4-value zod enum for the same field)
• coordinate elements aren't shape-checked before .map(c => c.longitude + ',' + c.latitude) — a malformed element throws an unguarded TypeError inside the listener
• formatIsoDateTimeClient passes through anything that doesn't match its regex unchanged, unlike the server path where depart_at/arrive_by already passed zod validation

Impact is bounded (requests still only reach Mapbox's own API using the already-client-visible public token — no XSS, no arbitrary host redirect), but this is a real gap versus the server's validation, and the missing origin check is worth fixing regardless of severity since it's a standard postMessage hardening step.

3. The Directions API is now called twice per invocation (CONFIRMED)

The server still fetches the route for the tool's own response, but the map preview never reuses that result — the iframe's self-fetch (unconditional for legacy MCP-UI via consumeInitialData, triggered by tool-input for MCP Apps) always re-fetches the same route from the browser with geometries=geojson forced, with no "server's response already has geojson, skip the client fetch" check. Every legacy-MCP-UI-enabled call now doubles Directions API cost and adds a full extra network round-trip to render latency.

4. MCP Apps pathway's fix depends on ui/notifications/tool-input, and host support for it is unconfirmed (PLAUSIBLE — already flagged in your own test plan)

It's a real notification from @modelcontextprotocol/ext-apps (not invented for this PR), but your test plan itself leaves this unchecked: "Confirm target MCP Apps hosts reliably send ui/notifications/tool-input..." If a host doesn't send it, selfFetchStarted stays false, and a tool-result with the default geometries="none" still falls through to showError('Could not find route data in tool result.') — the exact bug this PR fixes, silently persisting for the MCP Apps pathway on any host that only sends tool-result. Worth confirming against a real host before relying on this as the primary fix mechanism for that pathway.

5. The parity test never exercises the actual production code path (CONFIRMED)

All three cases in directionsUrlParity.test.ts call the server builder with geometriesOverride: 'geojson', but DirectionsTool.ts's real production call never passes geometriesOverride — it uses input.geometries, which defaults to 'none' (server omits the geometries param entirely in that case). The client always hardcodes geometries=geojson unconditionally. So the "byte-identical" guarantee this test provides never actually covers the geometries !== 'none' branch that production exercises — only the geojson-forced branch, which no real caller uses. Worth adding a case with no geometriesOverride to actually close this gap.

6. Inline UI resource is now attached unconditionally, including for legitimate "no route found" responses (PLAUSIBLE)

The old hasGeojson || hasPolyline gate (skip attaching the map UI when there's nothing to draw) is removed; tryRenderInlineUiHtml now runs regardless. For a real "no route between these coordinates" Directions API response, the map UI still attaches, its self-fetch hits the same no-route condition, and now shows a visible error box where previously no map-related UI appeared at all. Probably fine/intentional, but worth a quick sanity check that this is the desired UX.

---

Separate coordination note, not a code issue: this PR's entire diff lives in DirectionsAppUIResource.ts/directionsAppHtml.ts, both of which are deleted wholesale by #199 (feat/generic-map-app), which replaces all per-tool map UI with a generic render_map_tool. Whichever of #199/#227 merges second will need to reconcile or redo this fix in the new architecture — worth syncing with that PR's author before either merges.

[zmofei]

Thanks for the thorough review — point-by-point:

1. routeRendered never reset — fixed in 301dfef. Replaced both one-shot booleans with a generation counter: every delivery (tool-input or baked initial-data) bumps generation and opens a new render cycle; a tool-result not paired with the current cycle's self-fetch opens its own cycle (covers hosts that only send tool-result); async callbacks capture their generation and drop stale results. This restores iframe reuse (a follow-up call replaces the route), drops a slow call-1 fetch that resolves after call 2 started, and keeps the race suppression scoped to a single call.

2. postMessage hardening — fixed in 301dfef, two layers. (a) Sender pinning: if (event.source !== window.parent) return; — an event.origin allowlist isn't feasible for a portable MCP App (the host's origin is unknowable at HTML-generation time: claude.ai, VS Code webviews, sandbox-proxy opaque origins…), while source pinning blocks sibling-frame injection on any host. (b) isSafeDirectionsParams() validates everything interpolated into the URL before fetching — finite numeric coordinates, routing_profile against /^mapbox\/[a-z-]+$/ (kills path splicing), exclude against a strict charset (kills & query injection) — mirroring the zod guarantees the server path already has. Verified with a vm-sandbox harness running the real generated script: hostile-sibling messages dropped, ../../styles/v1/… profiles and &-injection excludes rejected, legitimate inputs produce byte-identical URLs as before.

3. Double fetch — intentional; it's the core trade-off of this PR. The goal is that the LLM-facing response can stay geometries="none" so route geometry never enters the model's context window (that's the token cost this design removes), while the map still always gets full geometry. Reusing the server's response only works when the model happens to request geojson — which is exactly the coupling that caused the incident. The self-fetch also starts on tool-input, before the server call completes, so the map renders earlier, not later. One extra Directions call per render is the documented, accepted price.

4. Host tool-input support — agreed, and it's already tracked in the test plan. Worst case on a host that never sends tool-input: that pathway degrades to exactly the pre-PR status quo (the tool-result fallback is unchanged), so it can't get worse — and the legacy MCP-UI path is fully fixed regardless of host support. We'll verify against a real host before relying on it as the primary mechanism.

5. Respectfully, I think this one misreads the parity test's purpose. The client's only production behavior is forced geometries=geojson — the self-fetch iframe is a real production caller of that branch on every render. The parity test exists solely to keep the hand-ported client builder byte-identical with the server builder for what the client actually does. A no-override case can't assert equality: with geometries='none' the server intentionally omits the param while the client always sends it — the URLs differ by design. The no-override server branches are covered where they belong: buildDirectionsRequestUrl.test.ts case 1 asserts the param is omitted for 'none' (the production default), case 3 exercises input.geometries='geojson' with no override, and the DirectionsTool.test.ts URL-construction tests exercise the real execute() path end-to-end with both values.

6. Unconditional UI attach — intentional. For a genuine "no route between these coordinates" response, a map panel with an explicit "Directions API returned no route." tells the user why, versus the pre-PR behavior of silently attaching nothing. If product prefers suppressing the panel for no-route responses we can gate it later, but consistent UI presence was the deliberate choice here.

Re: the #199 coordination note — agreed these need syncing, but on ordering: this PR is an incident fix for currently-broken user-facing behavior (the map preview errors on every default geometries="none" call today), while #199 is a feature refactor. The fix should land first, and #199 should rebase and carry it into the generic render_map_tool architecture — the self-fetch + generation-cycle logic is self-contained in the iframe script and ports over cleanly. Since #199 is yours, happy to help with that port.

[mattpodwysocki]

Verified 301dfef — both blocking findings are solidly fixed, not just patched over:

• #1 (generation counter): traced through the interleavings by hand — repeat calls reusing the iframe correctly open a new cycle and re-render (fixes the regression), a slow stale fetch from an earlier call is correctly dropped once a newer call has started (gen !== generation), and handleToolResult always calls renderRoute on authoritative geometry regardless of cycle state, so the "real data always wins" behavior is preserved. This is actually more correct than the pre-PR code, which had no staleness protection at all for the now-possible "two in-flight fetches" case.
• #2 (postMessage hardening): event.source !== window.parent is the right call over an origin allowlist given the real constraint (host origin is unknowable at render time across Claude Desktop/VS Code/sandbox-proxy contexts) — it blocks the sibling-frame injection vector without needing to know the host in advance. isSafeDirectionsParams() closes exactly the paths that mattered: routing_profile regex blocks path-splicing, the exclude charset blocks &/= query injection, coordinates are shape/finite-checked before hitting .longitude/.latitude. (depart_at/arrive_by/dimensions still pass through unvalidated, but those go through URLSearchParams.append, which encodes the value safely regardless of content — no injection risk there, just a possible 4xx from Mapbox on garbage input. Not blocking.)

On the pushback for #3 and #5 — both are fair, I'm conceding them:

• #3: the self-fetch starting on tool-input (before the server call even resolves) reframes this from "wasteful duplicate" to "the mechanism that keeps geometry out of the model's context while still rendering early." Agreed this is the intended trade-off, not an oversight.
• #5: you're right that I conflated parity-testing with coverage of the server's own none path. The client only ever produces a geometries=geojson URL in production, so that's correctly the only thing the parity test needs to assert equality on — and buildDirectionsRequestUrl.test.ts case 1 already covers the server's none-omits-the-param behavior on its own. No actual gap here, my mistake.

#4 and #6 — agreed these are accepted/tracked as-is, nothing further needed from my side.

On #199 coordination: agreed on ordering — this is a live incident fix and should land first, #199 is a refactor and should carry this forward. I'll port the generation-cycle + postMessage-hardening logic into the generic render_map_tool iframe when I rebase #199 on this. Appreciate the offer to help — will reach out when I get there.

Updating my review to approve given both blocking items are verified fixed.