diff --git a/CHANGELOG.md b/CHANGELOG.md index ca8c812..c75b0db 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,13 @@ ## Unreleased +### Fixed + +- **`directions_tool`: `geometries="geojson"` now always returns coordinates directly, regardless of trip length.** 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 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. + ### New Features - **The server now identifies which MCP client connected to it.** `server.server.getClientVersion()` (populated from the `clientInfo` sent in the client's `initialize` request) is now logged on connect, e.g. `Client identified as: claude-ai v1.0.0` — useful for support/debugging when behavior differs across Claude Desktop, Cursor, VS Code, etc. Reading it required moving the read to a `server.server.oninitialized` callback rather than right after `server.connect()`, since `getClientVersion()`/`getClientCapabilities()` are only populated once the client's `initialize` request has actually been processed, which is not guaranteed by the time `connect()`'s promise resolves (it only waits for the transport to start). Confirmed live that reading capabilities immediately after `connect()` reliably returned `undefined` even for a client that declared them; moving both reads into `oninitialized` fixed the same-shaped bug in the existing capability-gated tool registration (currently dormant, since no tool is registered through that path yet, but was silently broken for whenever one is added). The client name/version is also recorded as `mcp.client.name`/`mcp.client.version` on every subsequent tool-execution trace span, so OTel-backed traces can be filtered or grouped by client. diff --git a/src/tools/directions-tool/DirectionsTool.input.schema.ts b/src/tools/directions-tool/DirectionsTool.input.schema.ts index 6ae3e00..aed7971 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 map preview UI ' + 'still renders the route on its own; 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 the map preview to work.' + '- 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 the map preview, which fetches its own full-detail geometry separately.' + ), + 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 specifically need that; 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 stores it as a `mapbox://temp/` resource 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 8adf183..99b4a63 100644 --- a/src/tools/directions-tool/DirectionsTool.ts +++ b/src/tools/directions-tool/DirectionsTool.ts @@ -29,10 +29,16 @@ export class DirectionsTool extends MapboxApiBasedTool< description = 'Fetches directions from Mapbox API based on provided coordinates and direction method. ' + 'Returns a `mapboxRender.ref` in structuredContent regardless of the geometries value - pass ' + - 'it to render_map_tool to display the route on a live Mapbox GL JS map. ' + + 'it to render_map_tool to display the route on a live Mapbox GL JS map (this always works, ' + + 'for any trip length, independent of what you pass for geometries/overview below). ' + '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 - the map preview works either way.'; + 'turn-by-turn instructions, congestion). Use geometries="geojson" whenever YOU need the ' + + 'route coordinates yourself, e.g. to draw the route in your own app - this always returns ' + + 'coordinates directly in this response, for a 2-mile trip and a 400-mile one alike, because ' + + 'it defaults to a simplified-but-fully-drawable geometry (see the `overview` parameter). ' + + 'Only if you explicitly request overview="full" (maximum-precision geometry) can a very ' + + "long trip still exceed this response's size limit; in that rare case the full geometry is " + + 'stored as a `mapbox://temp/...` resource instead, readable via 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 9078724..c4fa100 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 @@ -320,13 +326,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/test/tools/directions-tool/DirectionsTool.test.ts b/test/tools/directions-tool/DirectionsTool.test.ts index 58529e0..0efaf34 100644 --- a/test/tools/directions-tool/DirectionsTool.test.ts +++ b/test/tools/directions-tool/DirectionsTool.test.ts @@ -79,7 +79,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); }); @@ -1309,6 +1312,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 a257bc9..747d251 100644 --- a/test/tools/directions-tool/cleanResponseData.test.ts +++ b/test/tools/directions-tool/cleanResponseData.test.ts @@ -304,6 +304,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(result.routes[0].congestion_information).toBeUndefined(); + // Speed/distance-derived data is unaffected by the absence of congestion. + expect(result.routes[0].average_speed_kph).toBe(84); + }); + it('should calculate average speed correctly', () => { const mockData = { routes: [