feat(amazon-location-service): Updated Amazon Location Service plugin to v1.1.0 - #250
Conversation
laithalsaadoon
left a comment
laithalsaadoon
left a comment
There was a problem hiding this comment.
Review: content verified against the published service models, AWS docs, and third-party sources
We checked every Amazon Location operation, field, and enum in this PR against the machine-readable service models in aws/api-models-aws (geo-places / geo-routes / geo-maps / location), the AWS API Reference & Developer Guide, and the relevant third-party sources (Google Maps SDK docs, MapLibre docs/source, CocoaPods trunk, Maven Central, npm).
The core change is solid. The address-verification rewrite around the Jobs API (StartJob + ValidateAddress) verified clean end-to-end: operations, job statuses, Parquet limits, the geo.amazonaws.com trust policy, S3 permissions + bucket versioning, output column names, enums, and country coverage all match the model and docs. Same for the SKILL.md restructure and the Geocode-vs-Jobs framing. Nice work on that.
The three new Google-migration guides are where the problems are. The Amazon-side concepts are right, but the installation, auth, imports, and "Before" Google code don't match the current SDKs, and several samples won't compile, won't authenticate, or return wrong data. Details are in the inline comments; the headline items:
- iOS: CocoaPods instructions reference pods that don't exist (SPM is the only distribution channel for the Swift SDK and auth SDK);
import Mapboxfails on the pinned MapLibre 6.x;GMSGeocoder.geocodeAddressString/GMSDirectionsService/GMSDistanceMatrixServicedon't exist in Google's iOS SDK; every multi-argument Swift init passes labels in the wrong order (generated inits are alphabetical); two nonexistent type names. - Android: API key passed as a SigV4 access key (fails auth — the supported path is
AuthHelper.withApiKeyfrom software.amazon.location:auth); Mapbox's private Maven repo instructed where Maven Central suffices; missing annotation-plugin dependency and missing mandatoryMapLibre.getInstance(); a dependency-removal instruction the file's own later code contradicts. - Both mobile guides: a hand-rolled Google-Polyline5 decoder presented for route geometry that Amazon Location returns as FlexiblePolyline by default — aws-geospatial/polyline is the official codec and should replace it.
- Web:
MaxDistance→QueryRadius;Walking→Pedestrian; TRANSIT is supported by the Migration SDK's DirectionsService;MigrationEncoding/MigrationPolyaren't importable from the published package; "Simple (the default)" — FlexiblePolyline is the observed default. - Smaller files: category filter applied to SearchText where the API only supports it on SearchNearby; a response-structure example with
TravelStepsat the wrong nesting level and a v1-onlyRouteBBoxfield; an EventBridge pattern thatPutRulerejects; aVerifyDevicePositionsample missing two required fields; a samples link that 404s.
Items flagged as author's-discretion / not fully verified
LegGeometryFormatdefault: the current API Reference doesn't print an explicit "Default value" line for CalculateRoutes (it does for the isoline and snap-to-roads equivalents, both FlexiblePolyline). Our evidence is behavioral: the developer guide's own example omits the parameter and gets FlexiblePolyline back. If you have confirmation that Simple is the default anywhere, we'd take the pointer — but the docs as published point the other way, and this PR's calculate-routes.md defaults table agrees with us.- Category ID casing in the iOS
includeCategories: ["Restaurant"]sample — docs list lowercase IDs; not runtime-verified. GMSPlacesClient.findAutocompletePredictions→ Autocomplete mapping: AmazonSuggestis arguably the closer equivalent for POI/query predictions (Autocomplete is address-oriented). Judgment call, not flagged inline.- dynamic-map.md's MapLibre
main-branch line-number links: behavior verified correct, anchors already drifted; pinning to a tag is a style choice. - plugin.json vs marketplace.json version sync: two other plugins also mismatch, so possibly intentional — flagged, not asserted.
- Pre-existing, not introduced here (fix optional): address-input.md L137 reads
item.Place?.PlaceId, but Autocomplete result items carryPlaceIdat the top level, so the suggestion click handler storesundefined.
Happy to re-review quickly once the migration guides are updated — the fixes are mechanical (correct install/auth/import blocks from the aws-geospatial READMEs plus the one-line API corrections above), not a rewrite.
…on and reference guides (part 2)
scottschreckengaust
left a comment
scottschreckengaust
left a comment
There was a problem hiding this comment.
Request changes
Thanks for the substantial expansion here — the three new Google-migration guides, the LocationClient sections, and the FlexiblePolyline guidance are genuinely useful additions, and the v1.1.0 bump is applied consistently across the manifests. The reference docs are also unusually well-sourced; most of what follows is in the sample code rather than the prose.
That said, several code samples in these references won't compile, won't parse, or are silently wrong — and because this plugin's whole purpose is to be copied verbatim by an agent into a user's project, a broken sample is a shipped bug rather than a typo. I've left 15 inline comments; the ones I'd consider blocking:
Won't compile / won't parse
google-migration-web.md:496— three JS blocks each declare the sameconsttwice (encoded,contains,distanceMeters). All three geometry examples areSyntaxErroron load.google-migration-android.md:177—Credentialsis used with no import (it'saws.smithy.kotlin.runtime.auth.awscredentials.Credentials).google-migration-ios.md:552—AutocompleteResultItemneeds theGeoPlacesClientTypes.prefix.
Silently wrong at runtime
address-verification.md:309— falsy-zero onOutput_Position_Longitudediscards the geocode for every address on the prime meridian.google-migration-web.md:511— the geometry mapping table equates meters with kilometers forcomputeDistanceBetween/computeOffset, so a "within 500m" check silently becomes 500km.google-migration-android.md:642—routePointsis undefined and the lat/lng order is inverted relative to whatCalculateRoutesreturns.device-tracking.md:195— the live-tracking poll drops all but the first page of devices, contradicting line 138.
Security
device-tracking.md:181—Popup.setHTML()with a client-suppliedDeviceIdis HTML injection in the canonical fleet-dashboard sample.
Android lifecycle
google-migration-android.md:381— nomapView.onCreate()and no lifecycle forwarding, a regression versus theMapFragmentcode it replaces.
Self-contradictions within a single file
zone-alerts.md:189saysBatchEvaluateGeofencesreturns an empty body; line 296 says always check itsErrorsarray.address-verification.md:351hard-rejectsUnconfirmedaddress numbers; line 325 says route those to review.google-migration-ios.md:812says to check CocoaPods config; line 65 says the packages are SPM-only.
Rendering / tooling
calculate-routes.md:305— converting##### 1.…##### 6.headings to list items without indenting the code fences makes all 11 criteria render as "1.".plugins/amazon-location-service/.codex-plugin/plugin.json:4— generated manifest is stale; the PR drops a trailing period from.claude-plugin/plugin.jsononly, leaving a three-way description mismatch.
Verification
I checked that none of this is caught by CI, so nothing here will surface on its own: mise run lint:md (0 errors) and mise run fmt:check both pass on 8049da4. python3 tools/generate_codex_manifests.py does rewrite the codex manifest on this branch, confirming the drift, and tools/validate-cross-refs.cjs never compares descriptions.
Non-blocking nits
google-migration-android.md:64-70— Gradle dynamic versions (1.8.+) on five AWS SDK artifacts make builds non-reproducible; pin them.google-migration-ios.md:747— theCLLocationCoordinate2Dextension is duplicated from line 509, so a reader following both sections hits "invalid redeclaration". Cross-reference the first copy instead.- Table-of-contents omissions in all three new guides: Android misses Best Practices / Troubleshooting (674, 716); iOS misses 721, 790; web misses Common Migration Patterns / Best Practices / Troubleshooting (627, 778, 813).
dynamic-map.md:266describesmap.once("idle", cb)as re-firing "whenever everything is loaded and stable" —oncefires exactly once.dynamic-map.md:271warns about custom-element marker drag "in MapLibre v5" while the links at 245 and 266 pin v6.4.1.device-tracking.md:153usesEndTimeExclusive: new Date("2024-01-15T23:59:59Z")as an end-of-day bound, which drops positions in the final second. UseT00:00:00Zof the next day.SKILL.md:229introduces@aws-sdk/credential-providersin the newLocationClientsection without adding it to the package list near line 160.
Happy to re-review as soon as the sample code is fixed — the structure and coverage of these guides is the right shape.
Reviewed with Claude Code
|
Follow-up to scope my review: on a second pass I'm narrowing the blocking set to six items Blocking (sample code an agent copies verbatim that fails or is silently wrong):
Everything else in my inline comments is non-blocking; I've marked those threads |
|
Pushed c2ca646 to this branch (edits-from-maintainers is on) so this can land. Applies the fixes from the open review threads: scottschreckengaust's six, the four remaining from mine, and QueryRadius at places-search.md:218 (MaxDistance is not a SearchNearby member; JS drops it silently). Doc-only, 9 files, +44/-41. dprint, markdownlint, manifest schemas clean locally. Revert anything you disagree with. |
c2ca646
Head branch was pushed to by a user without write access
|
Thread cleanup at c2ca646. Approval stands. Resolved, fixed in c3168cd: ios L317/L386/L586, android L371, web L483/L474, places-search L163, device-tracking L302. Resolved, fixed in c2ca646: android L45 (play-services-location kept, FusedLocationProviderClient needs it), calculate-routes L857/L876/L1153, web-javascript L321/L330/L331, zone-alerts L223-224, places-search L218 (MaxDistance -> QueryRadius, no thread). Released as non-blocking, resolved: ios L817, ios L824 (withdrawn, link redirects fine), zone-alerts L189/L271, device-tracking L161. Optional one-liners left in each thread. |
|
Thank you @theagenticguy for the commit, looks good to me. We usually make changes in our repo(https://github.com/aws-geospatial/amazon-location-agent-context) first, I will match your change on our end later
|
cgalvan
left a comment
cgalvan
left a comment
There was a problem hiding this comment.
Thanks for the feedback @theagenticguy and @scottschreckengaust . I've reviewed all the changes and am aligned :)
…d align plugin.json version
31fabbb
Head branch was pushed to by a user without write access
5 participants
feat: Update amazon-location-service plugin to v1.1.0
Expands the Amazon Location Service plugin with new reference guides and corrects API/SDK inaccuracies across the existing content.
What changed
New reference guides
PositionPropertiesmetadata, and tracking workflows.Address validation rewrite
StartJobwith ActionValidateAddress, plusGetJob/ListJobs/CancelJob) instead of Geocode: bulk Parquet S3 I/O,ExecutionRoleArn, and an IAM trust policy forgeo.amazonaws.com.MatchScoresfor single-address ad-hoc checks.API/SDK corrections
AuthHelper.withApiKeyinstead of static credentials.GMSServices.provideAPIKeysetup fromAppDelegate.TRANSITis a supported travel mode (onlyBICYCLINGis unsupported); useQueryRadiusforSearchNearby; corrected supported travel mode names.SimplevsFlexiblePolylinegeometry format trade-offs.Version bump
amazon-location-servicebumped to1.1.0in.claude-plugin/plugin.json,.codex-plugin/plugin.json, and.claude-plugin/marketplace.json.Testing
Ran
mise run buildlocally — all lint, format, and security checks passed.By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of the project license.