diff --git a/CHANGELOG.md b/CHANGELOG.md index ca8c812..fb7f0db 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,9 @@ ## Unreleased +### Fixed + +- **`directions_tool`: turn-by-turn `instructions` now actually populate.** 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. + ### 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/cleanResponseData.ts b/src/tools/directions-tool/cleanResponseData.ts index 9078724..585f887 100644 --- a/src/tools/directions-tool/cleanResponseData.ts +++ b/src/tools/directions-tool/cleanResponseData.ts @@ -42,13 +42,13 @@ interface RawIncident { [key: string]: unknown; } -interface RawVoiceInstruction { - announcement?: string; +interface RawManeuver { + instruction?: string; [key: string]: unknown; } interface RawStep { - voiceInstructions?: RawVoiceInstruction[]; + maneuver?: RawManeuver; [key: string]: unknown; } @@ -200,8 +200,8 @@ export function cleanResponseData( length?: number; }> = []; - // Collect voice instruction announcements from all steps - const routeAnnouncements: string[] = []; + // Collect turn-by-turn instructions from all steps + const routeInstructions: string[] = []; let totalDistanceWeightedSpeed = 0; // Sum of (speed × distance) for each segment let sumDistanceMeters = 0; @@ -284,15 +284,17 @@ export function cleanResponseData( }); } - // Process steps if they exist to collect voice instructions + // Process steps if they exist to collect turn-by-turn instructions. + // `maneuver.instruction` (e.g. "Bear right onto Great Portland + // Street/A4201") is present on every step whenever steps=true is + // requested, unlike `step.voiceInstructions`, which requires the + // separate voice_instructions=true parameter -- never sent by this + // tool, so that field was always empty and `instructions` never + // actually populated. if (leg.steps) { leg.steps.forEach((step) => { - if (step.voiceInstructions) { - step.voiceInstructions.forEach((instruction) => { - if (instruction.announcement) { - routeAnnouncements.push(instruction.announcement); - } - }); + if (step.maneuver?.instruction) { + routeInstructions.push(step.maneuver.instruction); } }); } @@ -312,10 +314,18 @@ export function cleanResponseData( // Add all incidents with the specified fields as a new property on the route cleanedRoute.incidents_summary = routeIncidents; - // Add voice instruction announcements only if there are 1 to 10 of them - // If there are more than 10, it's just too many, and if there is 0 then we don't have them. - if (routeAnnouncements.length >= 1 && routeAnnouncements.length <= 10) { - cleanedRoute.instructions = routeAnnouncements; + // One instruction per real maneuver (unlike the old voice-announcement + // source, which could repeat several announcements per turn), so a + // normal route -- even a long one with dozens of turns -- stays well + // within a generous cap. The cap here is just a backstop against a + // pathological case (e.g. many waypoints each with a complex urban leg), + // not a limit expected to bite in practice. + const MAX_INSTRUCTIONS = 200; + if ( + routeInstructions.length >= 1 && + routeInstructions.length <= MAX_INSTRUCTIONS + ) { + cleanedRoute.instructions = routeInstructions; } cleanedRoute.num_legs = route.legs?.length || 0; diff --git a/test/tools/directions-tool/cleanResponseData.test.ts b/test/tools/directions-tool/cleanResponseData.test.ts index a257bc9..c100afc 100644 --- a/test/tools/directions-tool/cleanResponseData.test.ts +++ b/test/tools/directions-tool/cleanResponseData.test.ts @@ -213,26 +213,22 @@ describe('cleanResponseData', () => { expect(result.routes[0].incidents_summary[0].extra_field).toBeUndefined(); }); - it('should collect voice instructions when within limits', () => { + it('should collect turn-by-turn instructions from step.maneuver.instruction', () => { const mockData = { routes: [ { legs: [ { steps: [ + { maneuver: { instruction: 'Drive northeast on Main St.' } }, { - voiceInstructions: [ - { announcement: 'Turn right in 100 meters' }, - { announcement: 'Turn right now' } - ] + maneuver: { + instruction: 'Bear right onto Elm St. Continue on Elm St.' + } }, - { - voiceInstructions: [ - { announcement: 'Continue straight for 500 meters' } - ] - } + { maneuver: { instruction: 'Arrive at your destination.' } } ], - summary: 'Leg with voice instructions' + summary: 'Leg with turn-by-turn instructions' } ] } @@ -242,27 +238,72 @@ describe('cleanResponseData', () => { const result = cleanResponseData(mockInput, mockData); expect(result.routes[0].instructions).toEqual([ - 'Turn right in 100 meters', - 'Turn right now', - 'Continue straight for 500 meters' + 'Drive northeast on Main St.', + 'Bear right onto Elm St. Continue on Elm St.', + 'Arrive at your destination.' ]); }); - it('should not include instructions when there are too many', () => { + it('ignores steps with no maneuver.instruction', () => { + const mockData = { + routes: [ + { + legs: [ + { + steps: [ + { maneuver: { instruction: 'Drive north on Main St.' } }, + { maneuver: {} }, + {} + ], + summary: 'Leg with a step missing an instruction' + } + ] + } + ] + }; + + const result = cleanResponseData(mockInput, mockData); + + expect(result.routes[0].instructions).toEqual(['Drive north on Main St.']); + }); + + it('still includes turn-by-turn instructions for a long route with dozens of real turns', () => { + // Mirrors a real long driving route (confirmed live: London->Edinburgh + // has ~40 steps in a single leg) -- well within the new cap, unlike the + // old 10-instruction cap (calibrated for a different, always-empty + // voice-announcement source) which would have excluded this outright. + const mockData = { + routes: [ + { + legs: [ + { + steps: Array.from({ length: 40 }, (_, i) => ({ + maneuver: { instruction: `Turn ${i + 1}` } + })), + summary: 'Leg with many real turns' + } + ] + } + ] + }; + + const result = cleanResponseData(mockInput, mockData); + + expect(result.routes[0].instructions).toHaveLength(40); + expect(result.routes[0].instructions?.[0]).toBe('Turn 1'); + expect(result.routes[0].instructions?.[39]).toBe('Turn 40'); + }); + + it('excludes instructions entirely past the pathological-case cap', () => { const mockData = { routes: [ { legs: [ { - steps: Array(6) - .fill(0) - .map(() => ({ - voiceInstructions: [ - { announcement: 'Instruction 1' }, - { announcement: 'Instruction 2' } - ] - })), - summary: 'Leg with many instructions' + steps: Array.from({ length: 201 }, (_, i) => ({ + maneuver: { instruction: `Turn ${i + 1}` } + })), + summary: 'Leg with an unreasonable number of turns' } ] } @@ -271,8 +312,6 @@ describe('cleanResponseData', () => { const result = cleanResponseData(mockInput, mockData); - // With 6 steps and 2 instructions each, we'd have 12 instructions total - // The function should exclude them since it's > 10 expect(result.routes[0].instructions).toBeUndefined(); });