Skip to content

fix: validate reminder indexes and honor parsing time zones - #87

Merged
steipete merged 1 commit into
mainfrom
fix/phase5-core-inputs
Sep 13, 2026
Merged

steipete merged 1 commit into
mainfrom
fix/phase5-core-inputs

Conversation

@steipete

Copy link
Copy Markdown
Collaborator

The minimum integer reminder index crashed because the resolver subtracted one before checking bounds. Validate the one-based range first, preserving the default-view/numericFrom contract. Local date parsing also ignored the caller's Calendar time zone; all local date formats now share a formatter that takes it explicitly, while ISO offsets retain precedence.

Regression tests cover the integer extremes, zero/negative/out-of-range indexes, opposing GMT+14/GMT-10 zones, local ISO and formatted dates, all-day metadata, and explicit UTC offsets. Existing date spellings and public signatures remain supported.

Proof: a standalone executable compiled from the actual RemindCore sources trapped with exit -5 for Int.min and failed a GMT+14 date assertion before the change; the rebuilt probe passes both index and dual-time-zone checks. The built, locally signed CLI now returns exit 1 and Invalid identifier: "-9223372036854775808". for info -- -9223372036854775808, instead of crashing. make check passes strict lint, the full Swift suite and coverage-gate tests, with 94.1% RemindCore coverage (950/1010); make build and make docs-site pass. Isolated Codex autoreview is scoped-clean through P2.

@clawsweeper

clawsweeper Bot commented Sep 13, 2026

Copy link
Copy Markdown

🦞👀
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. proof: sufficient Contributor real behavior proof is sufficient. 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

Codex review: needs maintainer review before merge. Reviewed September 13, 2026, 3:09 AM ET / 07:09 UTC.

ClawSweeper review

What this changes

The PR prevents crashes from invalid reminder indexes and makes local date parsing respect the caller’s time zone, with regression tests and documentation.

Merge readiness

Ready for maintainer review

The fixes remain necessary on current main and v0.3.6. The patch is focused, preserves existing caller contracts, and has no identified blocking defect.

Priority: P2
Reviewed head: 99eededaaac2df04f92228eed2f920f328311227

Review scores

Measure Result What it means
Overall readiness 🐚 platinum hermit (4/6) A focused repair with relevant reported runtime evidence, useful regression coverage, and no identified blocking finding.
Proof confidence 🐚 platinum hermit (4/6) Sufficient (terminal): The captured body reports actual-source before/after probes for both RemindCore changes and a built macOS CLI invocation returning the expected invalid-identifier error instead of crashing; the supplied execution results directly cover the changed behavior.
Patch quality 🐚 platinum hermit (4/6) No actionable review findings were identified.

Verification

Check Result Evidence
Real behavior Verified Sufficient (terminal): The captured body reports actual-source before/after probes for both RemindCore changes and a built macOS CLI invocation returning the expected invalid-identifier error instead of crashing; the supplied execution results directly cover the changed behavior.
Evidence reviewed 7 items Introduced patch and preserved indexing contract: The pinned main-to-head diff validates the one-based index before subtraction. Sorting, numericFrom selection, and ID-prefix handling remain unchanged; CommandHelpers still supplies the default today view.
Date parsing and compatibility: All local formatters now use calendar.timeZone. Explicit ISO-offset parsing still precedes local parsing, relative dates retain their existing calendar behavior, and default CLI callers continue using Calendar.current. Public signatures and persisted formats are unchanged.
Regression coverage: The added tests cover five invalid integer values, GMT+14 and GMT-10, three local timestamp formats, date-only midnight and metadata, and explicit UTC precedence. Existing filtered-view index coverage remains intact. Tests were inspected, not executed during this read-only review.
Findings None None.
Security None None.

How this fits together

RemindCore translates reminder identifiers and date strings for the macOS Reminders CLI. Its results determine which reminders commands select and how due dates and date filters are interpreted.

flowchart LR
  A[CLI input] --> B[RemindCore parsing]
  C[Reminder listing] --> D[Validate numeric index]
  B --> D
  B --> E[Interpret date and time zone]
  D --> F[Selected reminder or error]
  E --> G[Due date or date filter]
Loading

Before merge

None.

Agent review details

Security

None.

Review metrics

Metric Value Why it matters
Production and test delta Production +12/-16 (net -4); tests +28/-0 Two bounded fixes reduce production code while adding focused regression coverage.

Technical review

Best possible solution:

Retain the focused bounds check and shared local-date formatter while preserving default-view selection and explicit ISO-offset precedence.

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

Yes: current-main source subtracts one from Int.min before validation and ignores a supplied calendar’s time zone for local strings. The PR reports matching before/after execution; this review did not run code.

Is this the best way to solve the issue?

Yes: validating before arithmetic and passing the existing calendar time zone into local formatters directly repair the defects without adding configuration, changing public signatures, or rewriting stored data.

AGENTS.md: not found in the target repository.

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

Labels

Label changes:

  • add P2: An extreme numeric input can crash one CLI invocation, and explicit-calendar callers can parse incorrect local dates; both are bounded existing-behavior defects.
  • add proof: sufficient: Contributor real behavior proof is sufficient. The captured body reports actual-source before/after probes for both RemindCore changes and a built macOS CLI invocation returning the expected invalid-identifier error instead of crashing; the supplied execution results directly cover the changed behavior.
  • add rating: 🐚 platinum hermit: Overall readiness is 🐚 platinum hermit; proof is 🐚 platinum hermit and patch quality is 🐚 platinum hermit.
  • add status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Sufficient (terminal): The captured body reports actual-source before/after probes for both RemindCore changes and a built macOS CLI invocation returning the expected invalid-identifier error instead of crashing; the supplied execution results directly cover the changed behavior.

Label justifications:

  • P2: An extreme numeric input can crash one CLI invocation, and explicit-calendar callers can parse incorrect local dates; both are bounded existing-behavior defects.
  • rating: 🐚 platinum hermit: Overall readiness is 🐚 platinum hermit; proof is 🐚 platinum hermit and patch quality is 🐚 platinum hermit.
  • status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Sufficient (terminal): The captured body reports actual-source before/after probes for both RemindCore changes and a built macOS CLI invocation returning the expected invalid-identifier error instead of crashing; the supplied execution results directly cover the changed behavior.
  • proof: sufficient: Contributor real behavior proof is sufficient. The captured body reports actual-source before/after probes for both RemindCore changes and a built macOS CLI invocation returning the expected invalid-identifier error instead of crashing; the supplied execution results directly cover the changed behavior.

Evidence

What I checked:

  • Introduced patch and preserved indexing contract: The pinned main-to-head diff validates the one-based index before subtraction. Sorting, numericFrom selection, and ID-prefix handling remain unchanged; CommandHelpers still supplies the default today view. (Sources/RemindCore/IDResolver.swift:17, 99eededaaac2)
  • Date parsing and compatibility: All local formatters now use calendar.timeZone. Explicit ISO-offset parsing still precedes local parsing, relative dates retain their existing calendar behavior, and default CLI callers continue using Calendar.current. Public signatures and persisted formats are unchanged. (Sources/RemindCore/DateParsing.swift:94, 99eededaaac2)
  • Regression coverage: The added tests cover five invalid integer values, GMT+14 and GMT-10, three local timestamp formats, date-only midnight and metadata, and explicit UTC precedence. Existing filtered-view index coverage remains intact. Tests were inspected, not executed during this read-only review. (Tests/RemindCoreTests/DateParsingTests.swift:41, 99eededaaac2)
  • Captured real behavior evidence: The supplied complete PR body reports before/after probes compiled from actual RemindCore sources: Int.min previously trapped with exit -5, a GMT+14 assertion failed, and rebuilt probes passed index and dual-zone checks. It also supplies the built CLI result for info -- -9223372036854775808: exit 1 with an invalid-identifier error. This is contributor-reported execution evidence, not reviewer execution. Captured context sourceRevision: 7fd260a49bb0150e7596ef8d75189bc3a91154f00546c97416a4777afd5577cc. (99eededaaac2)
  • Current main and released behavior: GitHub confirms current main remains d272180. Inspection of both main and v0.3.6 shows subtraction before bounds validation and local formatters using TimeZone.current. Neither fix is already implemented there; no local release tag contains the PR head. (Sources/RemindCore/IDResolver.swift:17, 972357da9e14)
  • Feature-history routing: Main-branch file history identifies prior default-view indexing work at 883ae25 and date-intent work at 3d8d1a9. GitHub commit metadata identifies steipete for both. Local follow-history and blame encountered unavailable historical blobs; GitHub commit patches supplied the relevant prior changes. (Sources/RemindCore/IDResolver.swift:12, 883ae257911e)

Likely related people:

  • unknown: The claimed source-line change could not be verified from bounded local history. (role: source history unknown; confidence: low)

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 310c186 into main Sep 13, 2026
11 checks passed
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. proof: sufficient Contributor real behavior proof is sufficient. 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