Repository navigation
Conversation
Allow add to use a verified point without address geocoding. Validate paired finite coordinates and bounds before opening a store, and retain location text as a display label. Exercise detached alarm construction with a failing geocoder to prove the bypass.
|
🦞👀 Pull request received. I will update this pull request when review starts. ClawSweeper review completeClawSweeper finished reviewing this revision. The review result is being finalized. |
|
Codex review: blocked before merge. What this changesThis PR adds paired latitude and longitude options for exact geofences, with an optional location label. Example: Create a reminder at latitude 12.5, longitude -34.25 with a 200-meter radius.
Review scores
ProductKind: Feature · Worth it: Needs a maintainer decision · Fix scope: Complete Merge readiness⛔ Blocked before merge - 1 item remains Keep this PR open: it provides a distinct, working coordinate-input capability absent from main, with no actionable code defect found; the public interface still needs an owner decision. Priority: P2 Decision needed
Before merge
FindingsNone. Tests
Agent review detailsHow this fits togetherThe add command validates location arguments, constructs the existing LocationTrigger model, and passes it to RemindersStore, which saves an EventKit geofence and returns its persisted metadata. flowchart TD
A[CLI location arguments] --> B[Add command validation]
B --> C[LocationTrigger]
C --> D[EventKit alarm construction]
D --> E[Coordinate point or address geocoder]
E --> F[EventKit reminder storage]
F --> G[JSON readback]
Technical reviewBest possible solution: Approve or decline the paired coordinate interface, retaining the existing alarm owner, address behavior, timeout contract, and persisted format. Do we have a high-confidence way to reproduce the issue? Main source establishes that CLI callers cannot supply coordinates despite existing model support; the contributor's native output demonstrates the added route, while the original landmark geocoder response was not independently reproduced. Is this the best way to solve the issue? This is a focused implementation of an explicit remedy offered in the linked report, using existing owners without changing automatic geocoder selection; choosing that remedy remains a product decision. AGENTS.md: not found in the target repository. Codex review notes: model internal, reasoning medium; reviewed against 136b79b86355. Provenance checked
TestingProof path: shipped entry point. SecurityNone. EvidenceWhat I checked:
LabelsLabel changes:
Label justifications:
Rating scale6/6 🦀 challenger crab · 5/6 🦞 diamond lobster · 4/6 🐚 platinum hermit · 3/6 🦐 gold shrimp · 2/6 🦪 silver shellfish · 1/6 🧂 unranked krab. Overall follows the weaker of proof and patch quality; ✨ marks media proof (a screenshot, video, or linked artifact) that directly shows the changed behavior. WorkflowClawSweeper edits this one comment on every review. Comment HistoryReview history (1 earlier review cycle)
Reviewed October 28, 2026, 12:19 AM ET / 04:19 UTC (Revision 2). |
|
Added inspectable synthetic create/readback/delete output and regression evidence to the PR body for unchanged head @clawsweeper re-review |
|
🦞👀 Re-review progress:
|
addaccepts--latitudeand--longitudetogether to create a geofence at an exact point without geocoding.--locationbecomes an optional display label when coordinates are supplied; address-only triggers and the existing radius and departure options retain their behavior. Coordinates must be paired, finite and within their latitude/longitude bounds.Closes #112.
Native CLI evidence
Recorded 2026-10-28 UTC on macOS, head
c432e9e0688e7ff119b107d369511dab8ea6e4a5. Locally built release-configuration binary, ad-hoc signed. SHA-256:282182c8140b20dd69be222f871a526b82e490c47c8aa055c689ddd2860f759e; signature verification passed. Each command below starts a fresh candidate process. Coordinates and reminder titles are synthetic. Output shows selected JSON fields; the actual list ID and list name are replaced consistently with<redacted-list-id>and<redacted-list>. The runner verified both against the designated list before retaining these responses.Create, fresh-process readback, exact-ID deletion and absence
Both exact returned IDs were read back with their saved geometry, deleted individually, then confirmed absent in fresh processes. Known IDs: 2; confirmed absent: 2; unverified IDs: 0; no pending write.
Regression and offline evidence
The full offline gate was rerun 2026-10-28 UTC on the submitted head. The complete upstream offline gate ran without filtering suites; the upstream 90% coverage threshold and exclusions are unchanged.
Behavior removal fails; the submitted head passes
Earlier behavior-removal checks applied both changes together: restore the base AddCommand behavior with unused optional latitude/longitude parameters retained so the tests compile, and force the alarm factory through the geocoder branch. Both filtered suites ran against that modified tree. Their source commit was not recorded. The excerpts show the coordinate failures alongside passing compatibility assertions. Fresh passing output below was captured on the submitted head.
Maintainer direction
The maintainer decision remains open: should
addexpose paired--latitudeand--longitude, with--locationas an optional display label? This PR proposes that interface as the coordinate-input option described in #112. The evidence above establishes behavior; it does not establish maintainer approval.