diff --git a/CHANGELOG.md b/CHANGELOG.md index 1559560..7a46085 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -23,6 +23,7 @@ ### Bug Fixes +- **`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. ### Maintenance diff --git a/src/tools/directions-tool/cleanResponseData.ts b/src/tools/directions-tool/cleanResponseData.ts index 9078724..5613057 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,14 @@ export function cleanResponseData( }); } - // Process steps if they exist to collect voice instructions + // One instruction per step, e.g. "Bear right onto Great Portland + // Street/A4201". `maneuver.instruction` comes back whenever + // steps=true; `step.voiceInstructions` would need + // voice_instructions=true, which this tool doesn't send. 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 +311,15 @@ 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 maneuver, so even a long route with dozens of + // turns fits. The cap only guards against extreme cases, like many + // waypoints each with a complex urban leg. + 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 8a30c91..2f9fbcb 100644 --- a/test/tools/directions-tool/cleanResponseData.test.ts +++ b/test/tools/directions-tool/cleanResponseData.test.ts @@ -228,26 +228,49 @@ describe('cleanResponseData', () => { ); }); - 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 turn-by-turn instructions' + } + ] + } + ] + }; + + const result = cleanResponseData(mockInput, mockData); + + expect(routesOf(result)[0].instructions).toEqual([ + 'Drive northeast on Main St.', + 'Bear right onto Elm St. Continue on Elm St.', + 'Arrive at your destination.' + ]); + }); + + it('ignores steps with no maneuver.instruction', () => { + const mockData = { + routes: [ + { + legs: [ + { + steps: [ + { maneuver: { instruction: 'Drive north on Main St.' } }, + { maneuver: {} }, + {} ], - summary: 'Leg with voice instructions' + summary: 'Leg with a step missing an instruction' } ] } @@ -257,27 +280,47 @@ describe('cleanResponseData', () => { const result = cleanResponseData(mockInput, mockData); expect(routesOf(result)[0].instructions).toEqual([ - 'Turn right in 100 meters', - 'Turn right now', - 'Continue straight for 500 meters' + 'Drive north on Main St.' ]); }); - it('should not include instructions when there are too many', () => { + 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(routesOf(result)[0].instructions).toHaveLength(40); + expect(routesOf(result)[0].instructions?.[0]).toBe('Turn 1'); + expect(routesOf(result)[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' } ] } @@ -286,8 +329,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(routesOf(result)[0].instructions).toBeUndefined(); });