fix: validate reminder dates without losing legacy spellings - #91
Conversation
|
🦞👀 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. |
clawsweeper
Bot
added
P2
|
Codex review: needs maintainer review before merge. Reviewed September 15, 2026, 4:02 AM ET / 08:02 UTC (Revision 2). ClawSweeper reviewWhat this changesValidate reminder dates before parsing, preserve supported legacy spellings and field order, and document the behavior with regression tests. Merge readiness✅ Ready for maintainer review The fix remains necessary on main. The latest commit addresses the previous compatibility concern with restored legacy spellings, regression coverage, and an explicit contributor explanation; no blocking patch defect remains. Priority: P2 Review scores
Verification
How this fits togetherRemindCore converts command-line due dates, alarm dates, and date filters into timestamps with all-day metadata. Commands use those results to select reminders or update Apple Reminders through EventKit. flowchart TD
A[Due and alarm arguments] --> C[Shared date parser]
B[Date filter arguments] --> C
C --> D[Relative date or absolute format]
D --> E[Calendar validation]
E --> F[Timestamp and all-day metadata]
E --> G[Invalid date error]
F --> H[Reminder filtering or EventKit update]
Before mergeNone. Agent review detailsSecurityNone. Review metrics
Technical reviewBest possible solution: Keep one shared parser that rejects malformed dates while preserving established spellings, explicit field order, time zones, and all-day intent. Do we have a high-confidence way to reproduce the issue? Yes: the captured report supplies concrete impossible-date, suffix, offset, and day-order examples, and the main parser follows the reported permissive path. Runtime reproduction was reported by the contributor, not independently executed during this read-only review. Is this the best way to solve the issue? Yes: selecting field order before strict parsing repairs the shared owner, and the follow-up preserves the legacy spellings identified by the earlier review without adding configuration or a competing parser. AGENTS.md: not found in the target repository. Codex review notes: model internal, reasoning medium; reviewed against 2ca6a4814793. LabelsLabel changes:
Label justifications:
EvidenceWhat I checked:
Likely related people:
Rating scale
Overall follows the weaker of proof and patch quality. Workflow
HistoryReview history (1 earlier review cycle)
|
steipete
changed the title
|
Addressed the compatibility concern with executable evidence rather than an upgrade-policy change. Compiling the original parser confirmed support for |
clawsweeper
Bot
removed
the
merge-risk: 🚨 compatibility
1 participant
Malformed dates could silently schedule a reminder for the wrong day:
2026-02-30T12:00:00Zbecame March 2,Zjunksuffixes and invalid offsets were accepted, and03-01-26was parsed as year 3. Select each absolute format by its field order before strict calendar validation, preserving local time zones, explicit offsets, fractions, relative dates, and all-day metadata.Preserve unambiguous spellings verified against the original compiled parser: unpadded month/day/time fields, extra spaces before times, and year-first slash/dot separators. Parameterized compatibility tests cover those inputs. Day-first hyphens and month-first slashes follow the existing declared formats. README, command docs, and Unreleased notes describe the corrected behavior.
Validation: the regressions failed before the fixes;
make checkpassed with strict lint, full tests, and the 90% coverage gate; docs and the credential-free release harness passed. Built-CLI probes reject impossible dates, trailing text, and invalid offsets with exit 1 before accessing EventKit. Independent P0–P2 autoreview covers the full candidate. All reminder fixtures are synthetic; no live reminder records were changed.