diff --git a/CHANGELOG.md b/CHANGELOG.md index dd1805e..a5de29c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -28,6 +28,12 @@ ### Bug Fixes +- **`directions_tool`: `geometries="geojson"` now always returns coordinates directly, regardless of trip length (#255).** Previously, every geojson request used `overview=full` (maximum-precision geometry) — fine for a short trip, but for a long one (reported: London to a point in Scotland) the response exceeded the 50KB context limit and the tool silently swapped to a completely different contract: geometry and legs stripped out, replaced by a `mapbox://temp/...` resource URI (a `mapbox://response/...` ref since #257) that has to be read through the MCP resources API rather than being usable directly — not even a fetchable HTTP URL. An agent that asked for coordinates because it needed to draw the route itself (e.g. in a generated app) had no way to know in advance which contract it would get, since the split was driven by trip length, not by anything the caller controlled. + - New optional `overview` input (`"full" | "simplified"`) now defaults to `"simplified"` whenever `geometries="geojson"` (and to `"full"`, unchanged, whenever `geometries="none"`, since no geometry is returned there either way). A simplified route is still a fully accurate, drawable line — confirmed live against the real Directions API, a real London→Edinburgh route (~420 miles) drops from 10,057 coordinate pairs / ~700KB down to 46 coordinate pairs / ~4.7KB, comfortably inside the response limit. `overview="full"` is still available for callers who explicitly need maximum-precision geometry, at the cost of the same large-response behavior as before (now a deliberate, documented opt-in rather than a surprise past a size cliff). + - Tradeoff: the Mapbox Directions API rejects the `congestion` annotation unless `overview=full` (`422: Overview option must be full for congestion`, confirmed live) — `distance`/`speed` annotations have no such restriction and stay accurate at any overview level. So `congestion_information` (the per-segment traffic breakdown) is only present when the effective `overview` is `"full"`; it's correctly **omitted** (not reported as a misleading all-zero breakdown, which is what the code did previously whenever congestion happened to be unavailable) when it wasn't requested. `average_speed_kph`, `distance`, `duration`, and turn-by-turn summaries are unaffected at any overview level. + - `render_map_tool`'s live map preview is unaffected either way — it already always fetches its own full-detail geometry directly from the Directions API client-side (`mapbox://selffetch/directions...`), independent of what `directions_tool`'s own response contains. + - Matches the `overview` parameter `optimization_tool` already exposes (also defaulting to `"simplified"`), which never needed a large-response fallback in the first place. + - `directionsRequestFromRef` (#276) now always asks for `overview=full`, so apps that draw a route from the ref still get every vertex when the original call took the new simplified default. - **`directions_tool`: turn-by-turn `instructions` now actually populate (#256).** The tool has always requested `steps=true` and documented its response as including "turn-by-turn instructions," but the extraction code read announcements from `step.voiceInstructions`, which the Directions API only returns when the separate `voice_instructions=true` parameter is set — never sent by this tool. So `instructions` was silently empty for every call, regardless of route length. Switched to `step.maneuver.instruction` (e.g. `"Bear right onto Great Portland Street/A4201. Continue on A4201."`), a human-readable instruction already present on every step whenever `steps=true` is set, no extra parameter required. Also raised the output cap from 10 to 200: the old cap was sized for the old (broken) source, which could emit several announcements per turn; `maneuver.instruction` is one entry per real turn, so a normal route — even a long one with dozens of turns — stays well under the new cap. Confirmed live against the real Directions API. - **`static_map_image_tool` and place photos: URLs with several trailing dots are now checked like the plain hostname.** The URL check stripped only one trailing root-label dot, so `https://localhost../x.png` was compared as `localhost.` and passed, as did `app.localhost..`, dotless hosts such as `intranet..`, and IP literals such as `127.0.0.1..` and `169.254.169.254..`. It now strips them all. This covers custom-marker URLs in `static_map_image_tool` and photo URLs in `PlacePhotoResource`. Public hosts with a trailing dot (`example.com.`) are still accepted. diff --git a/docs/importing-tools.md b/docs/importing-tools.md index 7e3c253..adbe6fc 100644 --- a/docs/importing-tools.md +++ b/docs/importing-tools.md @@ -391,7 +391,7 @@ if (request) { } ``` -The ref holds only the original call's inputs, so it resolves in any process with no server-side state. The route is fetched again, so with live traffic it can differ slightly from the original call. `resolveSelfFetchRef` decodes any of these refs (directions, isochrone, search and others) to the tool and parameters behind it. To build the request yourself, `buildDirectionsRequestUrl` is the URL builder `directions_tool` and `directionsRequestFromRef` both use, so your request matches the tool's. +The ref holds only the original call's inputs, so it resolves in any process with no server-side state. The route is fetched again, so with live traffic it can differ slightly from the original call. `resolveSelfFetchRef` decodes any of these refs (directions, isochrone, search and others) to the tool and parameters behind it. To build the request yourself, `buildDirectionsRequestUrl` is the URL builder `directions_tool` and `directionsRequestFromRef` both use, so your request matches the tool's. `directionsRequestFromRef` always asks for `geometries=geojson` and `overview=full`, whatever the original call used, so you get every vertex of the route. ## Example: Using in a Web Application diff --git a/src/tools/directions-tool/DirectionsTool.input.schema.ts b/src/tools/directions-tool/DirectionsTool.input.schema.ts index 35e43b9..240628b 100644 --- a/src/tools/directions-tool/DirectionsTool.input.schema.ts +++ b/src/tools/directions-tool/DirectionsTool.input.schema.ts @@ -146,9 +146,27 @@ export const DirectionsInputSchema = z.object({ '- none (default): no geometry object is returned in the response. The mapboxRender.ref ' + 'works with render_map_tool either way; use this whenever you do not need the raw ' + 'coordinates yourself.\n' + - '- geojson: as GeoJSON LineString (might be very long as there could be a lot of ' + - 'points). Only needed when you (the caller) require the coordinates directly — not ' + - 'required for render_map_tool.' + '- geojson: as a GeoJSON LineString, always returned directly in this response — the ' + + 'coordinate count depends on `overview` (see below), not on trip length, so this works ' + + 'the same way for a 2-mile trip and a 400-mile one. Use this whenever you (the caller) ' + + 'need the coordinates yourself, e.g. to draw the route in your own app; not required ' + + 'for render_map_tool.' + ), + overview: z + .enum(['full', 'simplified']) + .optional() + .describe( + 'Detail level of the geometry named above (has no effect when geometries="none", since no ' + + 'geometry is returned either way). Defaults to "simplified" when geometries="geojson" ' + + '(a route across an entire country is typically well under 100 coordinate pairs — ' + + 'enough to draw an accurate shape, not enough to bloat the response) and to "full" ' + + 'otherwise. Pass "full" explicitly for the maximum-precision geometry (every road ' + + 'segment vertex) if you need it, e.g. for a map people zoom in on; on a long trip this can be tens of ' + + 'thousands of coordinate pairs, at which point the response no longer fits in this ' + + 'reply and this tool returns a summary and a `mapbox://response/` ref instead (see the ' + + 'tool description). Note: the Directions API cannot return per-segment congestion data ' + + '(`congestion_information` in the response) together with anything other than ' + + '"full" — congestion is only included when the effective overview is "full".' ), max_height: z .number() diff --git a/src/tools/directions-tool/DirectionsTool.ts b/src/tools/directions-tool/DirectionsTool.ts index e0b40bd..2dcc182 100644 --- a/src/tools/directions-tool/DirectionsTool.ts +++ b/src/tools/directions-tool/DirectionsTool.ts @@ -29,8 +29,13 @@ export class DirectionsTool extends MapboxApiBasedTool< 'Returns a `mapboxRender.ref` in structuredContent regardless of the geometries value; if the ' + 'user wants to see the route on a map, pass it to render_map_tool. ' + 'Use geometries="none" (default) for compact text/data responses (distance, duration, ' + - 'turn-by-turn instructions). Use geometries="geojson" only when you need the raw route ' + - 'coordinates in the response yourself - render_map_tool works either way.'; + 'turn-by-turn instructions, congestion). Use geometries="geojson" when you need the raw route ' + + 'coordinates in the response yourself, e.g. to draw the route in your own app. It defaults ' + + "to a simplified geometry that keeps the route's shape when zoomed out but cuts corners up " + + 'close; pass overview="full" for every vertex when your map lets people zoom in on the ' + + 'route. render_map_tool works either way. Only overview="full" on a very long trip can exceed the response size ' + + 'limit; then this tool returns a summary and a `mapbox://response/...` ref you can read with ' + + 'the MCP resources API.'; annotations = { title: 'Directions Tool', readOnlyHint: true, diff --git a/src/tools/directions-tool/buildDirectionsRequestUrl.ts b/src/tools/directions-tool/buildDirectionsRequestUrl.ts index 191a80b..409bd8e 100644 --- a/src/tools/directions-tool/buildDirectionsRequestUrl.ts +++ b/src/tools/directions-tool/buildDirectionsRequestUrl.ts @@ -7,6 +7,7 @@ export interface DirectionsRequestInput { coordinates: { longitude: number; latitude: number }[]; routing_profile: string; geometries: 'none' | 'geojson'; + overview?: 'full' | 'simplified'; alternatives: boolean; exclude?: string; depart_at?: string; @@ -31,6 +32,15 @@ export function buildDirectionsRequestUrl(params: { }): string { const { input, accessToken, apiEndpoint, geometriesOverride } = params; const geometries = geometriesOverride ?? input.geometries; + // Deliberately keyed off input.geometries, not the possibly-overridden + // `geometries` above: geometriesOverride only exists so the self-fetch + // parity test can simulate the client's "always fetch geojson itself" + // behavior against this same function, and that self-fetch path always + // wants 'full' (see mapAppHtml.ts) regardless of what the original call + // requested. A real DirectionsTool call never sets geometriesOverride, so + // this only matters for that test. + const overview = + input.overview ?? (input.geometries === 'geojson' ? 'simplified' : 'full'); const joined = input.coordinates .map(({ longitude, latitude }) => `${longitude},${latitude}`) @@ -45,11 +55,19 @@ export function buildDirectionsRequestUrl(params: { queryParams.append('alternatives', input.alternatives.toString()); if (input.routing_profile === 'mapbox/driving-traffic') { - queryParams.append('annotations', 'distance,congestion,speed'); + // The Directions API rejects `congestion` unless overview=full + // (confirmed live: 422 "Overview option must be full for congestion") — + // distance/speed have no such restriction and stay accurate at any + // overview level, since per-segment annotations aren't affected by how + // much the returned geometry itself is simplified. + queryParams.append( + 'annotations', + overview === 'full' ? 'distance,congestion,speed' : 'distance,speed' + ); } else { queryParams.append('annotations', 'distance,speed'); } - queryParams.append('overview', 'full'); + queryParams.append('overview', overview); if (input.depart_at) { queryParams.append('depart_at', formatIsoDateTime(input.depart_at)); diff --git a/src/tools/directions-tool/cleanResponseData.ts b/src/tools/directions-tool/cleanResponseData.ts index 5613057..70448b6 100644 --- a/src/tools/directions-tool/cleanResponseData.ts +++ b/src/tools/directions-tool/cleanResponseData.ts @@ -213,6 +213,11 @@ export function cleanResponseData( moderate: 0, low: 0 }; + // The API only returns a congestion annotation at all when overview=full + // was requested (see buildDirectionsRequestUrl.ts) — tracked here so we + // can omit congestion_information entirely rather than reporting a + // misleading all-zero breakdown when it was never requested. + let sawCongestionAnnotation = false; if (route.legs) { route.legs.forEach((leg) => { @@ -229,6 +234,7 @@ export function cleanResponseData( } if (leg.annotation?.congestion && leg.annotation?.distance) { + sawCongestionAnnotation = true; // iterate every congestion string in leg.annotation.congestion // each string is one of `severe, heavy, moderate, low, unknown` // keep track of total distance by type of congestion @@ -324,13 +330,18 @@ export function cleanResponseData( cleanedRoute.num_legs = route.legs?.length || 0; - // Add congestion distance information to route - cleanedRoute.congestion_information = { - length_low: Math.round(congestionTypeToDistance.low), - length_moderate: Math.round(congestionTypeToDistance.moderate), - length_heavy: Math.round(congestionTypeToDistance.heavy), - length_severe: Math.round(congestionTypeToDistance.severe) - }; + // Add congestion distance information to route, but only when the API + // actually returned a congestion annotation -- omitted (rather than + // reported as all-zero) when it wasn't requested/available, so this + // can't be misread as "confirmed no traffic anywhere". + if (sawCongestionAnnotation) { + cleanedRoute.congestion_information = { + length_low: Math.round(congestionTypeToDistance.low), + length_moderate: Math.round(congestionTypeToDistance.moderate), + length_heavy: Math.round(congestionTypeToDistance.heavy), + length_severe: Math.round(congestionTypeToDistance.severe) + }; + } // Calculate and add average speed in km/h if (sumDistanceMeters > 0 && totalDistanceWeightedSpeed > 0) { diff --git a/src/utils/directionsRef.ts b/src/utils/directionsRef.ts index 1e1001b..df72937 100644 --- a/src/utils/directionsRef.ts +++ b/src/utils/directionsRef.ts @@ -55,6 +55,9 @@ export function directionsRequestFromRef( coordinates: coords, routing_profile: params.routing_profile ?? 'mapbox/driving-traffic', geometries: 'geojson', + // geojson now defaults to a simplified overview; whoever draws from + // this wants every vertex, like the map app's own self-fetch. + overview: 'full', alternatives: params.alternatives === true, exclude: params.exclude, depart_at: params.depart_at, diff --git a/test/tools/directions-tool/DirectionsTool.test.ts b/test/tools/directions-tool/DirectionsTool.test.ts index 3dacd70..89e7d8c 100644 --- a/test/tools/directions-tool/DirectionsTool.test.ts +++ b/test/tools/directions-tool/DirectionsTool.test.ts @@ -81,7 +81,10 @@ describe('DirectionsTool', () => { expect(calledUrl).toContain('geometries=geojson'); expect(calledUrl).toContain('alternatives=true'); expect(calledUrl).toContain('annotations=distance%2Cspeed'); - expect(calledUrl).toContain('overview=full'); + // Defaults to simplified for geometries=geojson -- see the + // "always returns coordinates directly, regardless of trip length" + // tests below for why. + expect(calledUrl).toContain('overview=simplified'); expect(calledUrl).toContain('exclude=ferry'); assertHeadersSent(mockHttpRequest); }); @@ -1311,6 +1314,141 @@ describe('DirectionsTool', () => { /^mapbox:\/\/selffetch\/directions\?data=/ ); }); + + describe('geometries="geojson" returns coordinates directly for both short and long trips', () => { + // A realistic *cleaned* shape for a long trip fetched with the new + // default overview=simplified: still just a few dozen coordinate + // pairs (confirmed live against the real Directions API for a real + // London->Edinburgh route: 46 points, ~4.7KB total), unlike overview + // =full's tens of thousands. cleanResponseData is identity-mocked in + // this file's beforeEach, so this fixture stands in for its own + // output directly. + function longRouteResponse() { + const geometry = { + type: 'LineString', + coordinates: Array.from({ length: 46 }, (_, i) => [ + -0.1278 + i * 0.065, + 51.5074 + i * 0.0967 + ]) + }; + return { + code: 'Ok', + routes: [ + { + distance: 671000, + duration: 26400, + geometry, + leg_summaries: ['A1, M1, A68'], + intersecting_admins: ['GBR'], + notifications_summary: [], + incidents_summary: [ + { + type: 'construction', + impact: 'major', + affected_road_names: ['A1'] + } + ], + num_legs: 1, + average_speed_kph: 91 + // No congestion_information -- overview=simplified can't + // request the congestion annotation (see + // buildDirectionsRequestUrl.ts). + } + ], + waypoints: [ + { location: [-0.1278, 51.5074], name: '' }, + { location: [-3.1883, 55.9533], name: '' } + ] + }; + } + + function shortRouteResponse() { + return { + code: 'Ok', + routes: [ + { + distance: 1500, + duration: 180, + geometry: { + type: 'LineString', + coordinates: [ + [-74.0, 40.7], + [-74.005, 40.705], + [-74.01, 40.71] + ] + }, + leg_summaries: ['Main St'], + num_legs: 1, + average_speed_kph: 30 + } + ], + waypoints: [ + { location: [-74.0, 40.7], name: '' }, + { location: [-74.01, 40.71], name: '' } + ] + }; + } + + it('returns geometry directly, with no mapbox://temp/ fallback, for a long trip', async () => { + const httpRequestFn = mockHttpRequestForGeometry(longRouteResponse()); + const result = await new DirectionsTool({ + httpRequest: httpRequestFn + }).run({ + coordinates: [ + { longitude: -0.1278, latitude: 51.5074 }, + { longitude: -3.1883, latitude: 55.9533 } + ], + geometries: 'geojson' + }); + + expect(result.isError).toBe(false); + const text = (result.content[0] as { text: string }).text; + expect(text).not.toContain('mapbox://temp/'); + expect(text).not.toContain('exceeds context limit'); + expect(Buffer.byteLength(text, 'utf8')).toBeLessThan(50 * 1024); + + const sc = result.structuredContent as { + routes?: Array<{ geometry?: { coordinates?: unknown[] } }>; + }; + expect(sc.routes?.[0]?.geometry?.coordinates?.length).toBe(46); + }); + + it('returns geometry directly, in the same response shape, for a short trip', async () => { + const httpRequestFn = mockHttpRequestForGeometry(shortRouteResponse()); + const result = await new DirectionsTool({ + httpRequest: httpRequestFn + }).run({ + coordinates: [ + { longitude: -74.0, latitude: 40.7 }, + { longitude: -74.01, latitude: 40.71 } + ], + geometries: 'geojson' + }); + + expect(result.isError).toBe(false); + const text = (result.content[0] as { text: string }).text; + expect(text).not.toContain('mapbox://temp/'); + + const sc = result.structuredContent as { + routes?: Array<{ geometry?: { coordinates?: unknown[] } }>; + }; + expect(sc.routes?.[0]?.geometry?.coordinates?.length).toBe(3); + }); + + it('requests overview=simplified by default for the long trip (the actual fix)', async () => { + const httpRequestFn = mockHttpRequestForGeometry(longRouteResponse()); + await new DirectionsTool({ httpRequest: httpRequestFn }).run({ + coordinates: [ + { longitude: -0.1278, latitude: 51.5074 }, + { longitude: -3.1883, latitude: 55.9533 } + ], + geometries: 'geojson' + }); + + const calledUrl = httpRequestFn.mock.calls[0][0] as string; + expect(calledUrl).toContain('overview=simplified'); + }); + }); }); describe('Route selection elicitation', () => { diff --git a/test/tools/directions-tool/buildDirectionsRequestUrl.test.ts b/test/tools/directions-tool/buildDirectionsRequestUrl.test.ts index 2372432..ead0493 100644 --- a/test/tools/directions-tool/buildDirectionsRequestUrl.test.ts +++ b/test/tools/directions-tool/buildDirectionsRequestUrl.test.ts @@ -78,6 +78,85 @@ describe('buildDirectionsRequestUrl', () => { expect(url).toContain('alternatives=true'); }); + it('defaults overview to simplified for geometries=geojson, dropping congestion for driving-traffic', () => { + const url = buildDirectionsRequestUrl({ + input: { + coordinates: [ + { longitude: -0.1278, latitude: 51.5074 }, + { longitude: -3.1883, latitude: 55.9533 } + ], + routing_profile: 'mapbox/driving-traffic', + geometries: 'geojson', + alternatives: false + }, + accessToken: 'pk.test-token', + apiEndpoint: 'https://api.mapbox.com/' + }); + + expect(url).toContain('overview=simplified'); + expect(url).toContain('annotations=distance%2Cspeed'); + expect(url).not.toContain('congestion'); + }); + + it('defaults overview to full for geometries=none, keeping congestion for driving-traffic', () => { + const url = buildDirectionsRequestUrl({ + input: { + coordinates: [ + { longitude: -0.1278, latitude: 51.5074 }, + { longitude: -3.1883, latitude: 55.9533 } + ], + routing_profile: 'mapbox/driving-traffic', + geometries: 'none', + alternatives: false + }, + accessToken: 'pk.test-token', + apiEndpoint: 'https://api.mapbox.com/' + }); + + expect(url).toContain('overview=full'); + expect(url).toContain('annotations=distance%2Ccongestion%2Cspeed'); + }); + + it('lets the caller opt into overview=full for geometries=geojson, restoring congestion', () => { + const url = buildDirectionsRequestUrl({ + input: { + coordinates: [ + { longitude: -0.1278, latitude: 51.5074 }, + { longitude: -3.1883, latitude: 55.9533 } + ], + routing_profile: 'mapbox/driving-traffic', + geometries: 'geojson', + overview: 'full', + alternatives: false + }, + accessToken: 'pk.test-token', + apiEndpoint: 'https://api.mapbox.com/' + }); + + expect(url).toContain('overview=full'); + expect(url).toContain('annotations=distance%2Ccongestion%2Cspeed'); + }); + + it('lets the caller opt into overview=simplified for geometries=none too', () => { + const url = buildDirectionsRequestUrl({ + input: { + coordinates: [ + { longitude: -0.1278, latitude: 51.5074 }, + { longitude: -3.1883, latitude: 55.9533 } + ], + routing_profile: 'mapbox/driving-traffic', + geometries: 'none', + overview: 'simplified', + alternatives: false + }, + accessToken: 'pk.test-token', + apiEndpoint: 'https://api.mapbox.com/' + }); + + expect(url).toContain('overview=simplified'); + expect(url).toContain('annotations=distance%2Cspeed'); + }); + it('percent-encodes exclude through URLSearchParams so it can never inject or duplicate another query parameter', () => { const url = buildDirectionsRequestUrl({ input: { diff --git a/test/tools/directions-tool/cleanResponseData.test.ts b/test/tools/directions-tool/cleanResponseData.test.ts index 2f9fbcb..3833b90 100644 --- a/test/tools/directions-tool/cleanResponseData.test.ts +++ b/test/tools/directions-tool/cleanResponseData.test.ts @@ -360,6 +360,33 @@ describe('cleanResponseData', () => { // Note: 'unknown' congestion type is skipped }); + it('omits congestion_information (rather than reporting all-zero) when no congestion annotation was requested', () => { + // Mirrors the real shape of a response fetched with overview=simplified + // -- distance/speed annotations are present, congestion is not (see + // buildDirectionsRequestUrl.ts). + const mockData = { + routes: [ + { + legs: [ + { + annotation: { + speed: [10, 20, 30], + distance: [100, 200, 300] + }, + summary: 'Leg with no congestion annotation' + } + ] + } + ] + }; + + const result = cleanResponseData(mockInput, mockData); + + expect(routesOf(result)[0].congestion_information).toBeUndefined(); + // Speed/distance-derived data is unaffected by the absence of congestion. + expect(routesOf(result)[0].average_speed_kph).toBe(84); + }); + it('should calculate average speed correctly', () => { const mockData = { routes: [ diff --git a/test/utils/directionsRef.test.ts b/test/utils/directionsRef.test.ts index 397c061..0d51bcb 100644 --- a/test/utils/directionsRef.test.ts +++ b/test/utils/directionsRef.test.ts @@ -55,7 +55,8 @@ describe('directionsRequestFromRef', () => { coordinates, routing_profile: 'mapbox/driving', exclude: 'toll', - geometries: 'geojson' + geometries: 'geojson', + overview: 'full' }; const { result, ref, url } = await runDirections(input); @@ -83,6 +84,16 @@ describe('directionsRequestFromRef', () => { expect(url.searchParams.get('access_token')).toBe('pk.test'); }); + it('asks for the full geometry when the original call took the simplified default', async () => { + const { ref, url } = await runDirections({ + coordinates, + geometries: 'geojson' + }); + expect(new URL(url).searchParams.get('overview')).toBe('simplified'); + const request = directionsRequestFromRef(ref, { accessToken: 'pk.test' }); + expect(new URL(request!.url).searchParams.get('overview')).toBe('full'); + }); + it('passes through the route the user picked among alternatives', () => { const ref = buildSelfFetchRef('directions', { coordinates,