Skip to content

fix(routes): validate numeric inputs and sample wrapped polylines - #54

Merged
steipete merged 1 commit into
mainfrom
fix/phase-five-geometry
Sep 13, 2026
Merged

steipete merged 1 commit into
mainfrom
fix/phase-five-geometry

Conversation

@steipete

Copy link
Copy Markdown
Collaborator

Numeric bounds checks using only < and > let NaN through, so route search could call Routes before discovering an invalid radius. Polyline decoding also accepted invalid characters and coordinates, longitude interpolation crossed the long way around the globe, and antipodal roundoff could produce a NaN distance and panic during sampling.

Share radius/coordinate validation across request types and reject non-finite ratings before HTTP. Decode both polyline dimensions through one bounded 32-bit decoder, validating characters and accumulated coordinates before sampling. Interpolate the short longitude crossing, treat ±180° as the same location, and clamp the haversine's roundoff at one. Malformed upstream data remains an API-processing error (exit 1); invalid user numbers now produce the existing validation class (exit 2). Update client and route documentation plus Unreleased notes.

Regression tests first failed for all these root cases. Go 1.26.8 race tests, lint, and 92.3% coverage now pass. Isolated Codex autoreview is scoped-clean at P0–P2. The commit was rebased onto the merged security fix with an identical reviewed source tree.

Built-CLI proof against a synthetic local Routes/Places server:

Case Before After
Route from longitude 179° to −179°, one waypoint Searches at longitude 0° Searches at 180°
Antipodal points (−58.77137, 17.12345) and (58.77137, −162.87655) Panics after computing NaN distance Finite midpoint; successful search
Invalid polyline characters Accepted and searched Exit 1; zero place searches
Out-of-range upstream coordinates Misreported as user validation failure Exit 1; zero place searches
NaN route radius Calls Routes, then fails encoding Exit 2; zero HTTP calls
NaN search/nearby/autocomplete/directions inputs Encoding failure, exit 1 Validation error, exit 2; zero HTTP calls

All ten regression scenarios pass. The normal 21-case built-CLI comparison across all eight commands remains byte-identical to baseline, SHA-256 043a9aa35c7e9a159a75b42afed4e4d56bf606646ee1352e3a22e8af3185eefe. No Google credentials or external API calls are used for this proof.

@clawsweeper

clawsweeper Bot commented Sep 13, 2026 •

Copy link
Copy Markdown
Contributor

🦞👀
ClawSweeper picked this up.

Pull request received. I will update this pull request when review starts.

ClawSweeper review complete

ClawSweeper finished reviewing this revision. The review result is being finalized.

View the workflow run.

@clawsweeper clawsweeper Bot added P2 Normal priority bug or improvement with limited blast radius. rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. labels Sep 13, 2026
@clawsweeper

clawsweeper Bot commented Sep 13, 2026

Copy link
Copy Markdown
Contributor

Codex review: needs maintainer review before merge. Reviewed September 12, 2026, 11:20 PM ET / September 13, 2026, 03:20 UTC.

ClawSweeper review

What this changes

The PR rejects non-finite request numbers, validates encoded route coordinates, fixes date-line waypoint sampling and antipodal distance calculations, and adds regression tests and documentation.

Merge readiness

✅ Ready for maintainer review

This remains a useful, focused fix: current main and v0.4.9 retain the reported defects. No introduced correctness or security defect was found, and collaborator-authored work is protected from automatic closure.

Priority: P2
Reviewed head: 9246684a9a7c9b6766bbf52544b82e4be087d695

Review scores

Measure Result What it means
Overall readiness 🐚 platinum hermit (4/6) A focused repair with useful regression coverage and reported CLI observations; no blocking patch defect was found.
Proof confidence 🌊 off-meta tidepool Not applicable: The ordinary proof gate is exempt for this collaborator-authored PR. Its supplied built-CLI observations exercise Route and shared validation through a local HTTP server, reporting corrected sampling and rejection before downstream calls; no reviewer execution or authenticated Google validation is claimed.
Patch quality 🐚 platinum hermit (4/6) No actionable review findings were identified.

Verification

Check Result Evidence
Real behavior Not applicable Not applicable: The ordinary proof gate is exempt for this collaborator-authored PR. Its supplied built-CLI observations exercise Route and shared validation through a local HTTP server, reporting corrected sampling and rejection before downstream calls; no reviewer execution or authenticated Google validation is claimed.
Evidence reviewed 8 items Current main still needs the geometry repair: Main interpolates longitude by direct subtraction, compares date-line endpoints without wrapping, and passes the unclamped haversine value to Asin. These independently support the reported date-line and antipodal defects.
Existing numeric validation misses NaN: Main's circle validation uses only ordered comparisons, which do not reject NaN. The PR adds explicit NaN checks while retaining existing finite bounds and route defaults.
Bounded decoding and request ordering: The decoder limits each delta to seven groups, rejects invalid characters and accumulated coordinates, and uses int64 accumulation. Route validates requests before Routes HTTP and decodes geometry before any Places search.
Findings None None.
Security None None.

How this fits together

goplaces exposes Google Places and Routes workflows through a Go library and CLI. Route searches validate user inputs, decode Google's route geometry, and search for places near sampled waypoints.

flowchart TD
 A[CLI or Go request] --> B[Numeric validation]
 B -->|Valid request| C[Google Routes response]
 C --> D[Decode and validate geometry]
 D --> E[Sample route waypoints]
 E --> F[Google Places searches]
 F --> G[Waypoint search results]
 B -->|Invalid input| H[Validation error]
Loading

Before merge

None.

Agent review details

Security

None.

Review metrics

Metric Value Why it matters
Production and test growth Production +67/-51 (net +16); tests +149/-0 The small net production increase consolidates numeric validation and bounds decoding, with focused regression coverage.

Technical review

Best possible solution:

Keep validation in the shared client helpers and geometry corrections in the route sampler, preserving valid-input defaults and distinguishing upstream errors from user validation failures.

Do we have a high-confidence way to reproduce the issue?

Yes, from source: NaN bypasses main's ordered comparisons, and interpolation between longitudes 179 and −179 selects longitude 0. The supplied body reports corresponding CLI failures; this review did not execute them.

Is this the best way to solve the issue?

Yes. The patch repairs existing validation and geometry owners without introducing competing APIs, settings, or dependencies, and covers the relevant rejection and sampling paths.

AGENTS.md: not found in the target repository.

Codex review notes: model internal, reasoning medium; reviewed against 84fff29e4fd4.

Labels

Label changes:

  • add P2: This fixes bounded numeric-input and route-geometry edge cases without evidence of a widespread urgent regression.
  • add rating: 🐚 platinum hermit: Overall readiness is 🐚 platinum hermit; proof is 🌊 off-meta tidepool and patch quality is 🐚 platinum hermit.
  • add status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Not applicable: The ordinary proof gate is exempt for this collaborator-authored PR. Its supplied built-CLI observations exercise Route and shared validation through a local HTTP server, reporting corrected sampling and rejection before downstream calls; no reviewer execution or authenticated Google validation is claimed.

Label justifications:

  • P2: This fixes bounded numeric-input and route-geometry edge cases without evidence of a widespread urgent regression.
  • rating: 🐚 platinum hermit: Overall readiness is 🐚 platinum hermit; proof is 🌊 off-meta tidepool and patch quality is 🐚 platinum hermit.
  • status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Not applicable: The ordinary proof gate is exempt for this collaborator-authored PR. Its supplied built-CLI observations exercise Route and shared validation through a local HTTP server, reporting corrected sampling and rejection before downstream calls; no reviewer execution or authenticated Google validation is claimed.

Evidence

What I checked:

  • Current main still needs the geometry repair: Main interpolates longitude by direct subtraction, compares date-line endpoints without wrapping, and passes the unclamped haversine value to Asin. These independently support the reported date-line and antipodal defects. (internal/places/route_geometry.go:144, 84fff29e4fd4)
  • Existing numeric validation misses NaN: Main's circle validation uses only ordered comparisons, which do not reject NaN. The PR adds explicit NaN checks while retaining existing finite bounds and route defaults. (internal/places/validation.go:25, 84fff29e4fd4)
  • Bounded decoding and request ordering: The decoder limits each delta to seven groups, rejects invalid characters and accumulated coordinates, and uses int64 accumulation. Route validates requests before Routes HTTP and decodes geometry before any Places search. (internal/places/route_geometry.go:47, 9246684a9a7c)
  • Regression coverage and error classification: New tests cover malformed geometry, both date-line directions, antipodal distance, and nine request-field scenarios with NaN and both infinities. HTTP counters assert rejection before downstream calls; existing CLI handling maps ValidationError to exit 2 and other errors to exit 1. Tests were inspected, not executed during this read-only review. (internal/places/validation_test.go:14, 9246684a9a7c)
  • Latest release retains the old behavior: GitHub identifies v0.4.9 as the latest release. Its route.go still has unchecked radius comparisons, direct longitude interpolation, and the unclamped haversine calculation. (internal/places/route.go:403, 424df65ba142)
  • Relevant merged history: GitHub history associates steipete with the merged geometry extraction, route-search parallelization, and earlier request-contract hardening. Blame locates the geometry constants in the recent extraction commit; this is routing evidence, not proof of semantic feature introduction. (internal/places/route_geometry.go:11, b9ba1ef76399)

Likely related people:

  • Peter Steinberger: Raw commit b9ba1ef adds internal/places/route_geometry.go:11 relative to its recorded parents. This identifies author metadata, not feature responsibility or a PR merger. (role: source-line author; confidence: high; commits: b9ba1ef76399; files: internal/places/route_geometry.go)

Rating scale

Score Internal tier Crab rank Meaning
6/6 S 🦀 challenger crab Exceptional readiness
5/6 A 🦞 diamond lobster Very strong readiness
4/6 B 🐚 platinum hermit Good normal PR; ordinary maintainer review
3/6 C 🦐 gold shrimp Useful, but confidence is limited
2/6 D 🦪 silver shellfish Proof or implementation needs work
1/6 F 🧂 unranked krab Not merge-ready
N/A NA 🌊 off-meta tidepool Rating does not apply

Overall follows the weaker of proof and patch quality.
Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics.

Workflow

  • ClawSweeper keeps one durable marker-backed review comment per issue or PR.
  • Re-runs edit this comment so the latest verdict, findings, and automation markers stay together instead of adding duplicate bot comments.
  • A fresh review can be triggered by eligible @clawsweeper re-review comments, exact-item GitHub events, scheduled/background review runs, or manual workflow dispatch.
  • PR/issue authors and users with repository write access can comment @clawsweeper re-review or @clawsweeper re-run on an open PR or issue to request a fresh review only.
  • Maintainers can also comment @clawsweeper review to request a fresh review only.
  • Fresh-review commands do not start repair, autofix, rebase, CI repair, or automerge.
  • Maintainer-only repair and merge flows require explicit commands such as @clawsweeper autofix, @clawsweeper automerge, @clawsweeper fix ci, or @clawsweeper address review.
  • Maintainers can comment @clawsweeper explain to ask for more context, or @clawsweeper stop to stop active automation.

@steipete
steipete merged commit eb85c73 into main Sep 13, 2026
11 checks passed
@vincentkoc
vincentkoc deleted the fix/phase-five-geometry branch September 25, 2026 11:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

P2 Normal priority bug or improvement with limited blast radius. rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant