Skip to content

fix: deduplicate reminder targets before delete and complete - #98

Merged
steipete merged 2 commits into
openclaw:mainfrom
SebTardif:fix/remindctl-f008
Oct 7, 2026
Merged

steipete merged 2 commits into
openclaw:mainfrom
SebTardif:fix/remindctl-f008

Conversation

@SebTardif

@SebTardif SebTardif commented Oct 4, 2026 •

Copy link
Copy Markdown
Contributor

Passing the same reminder more than once to delete previously counted it twice and then failed after the first removal. Resolve each reminder ID once, preserving the first input order, so delete and complete share consistent previews, counts, and mutation targets. Numeric indexes, full IDs, and prefixes that identify the same reminder are deduplicated after resolution; every input still must be valid and unambiguous.

Sebastien Tardif (@SebTardif) contributed the resolver fix and verified deletion of a disposable reminder through EventKit. This update retains that implementation and commit, extends regression coverage, and documents the behavior.

Validation:

  • Added synthetic regression cases failed on main with six duplicate-output assertion failures.
  • nice -n 19 swift test --jobs 2 --filter IDResolverTests passes all 10 tests with the fix, covering repeated indexes/full IDs, mixed prefixes and case, filtered numeric views, first-input order, and invalid subsequent inputs.
  • Swift format lint, strict SwiftLint for the changed Swift files, and git diff --check pass.
  • Independent Codex autoreview found no actionable P0–P2 findings.
  • Full macOS coverage, universal CLI smoke checks, documentation build, and CodeQL are checked by this PR's GitHub Actions.

delete 1 1 resolved the same reminder twice, deleted it, then failed the second lookup. Keep the first id so the prompt and the delete see one reminder.

Signed-off-by: Sebastien Tardif <sebtardif@ncf.ca>
@clawsweeper

clawsweeper Bot commented Oct 4, 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. rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. labels Oct 4, 2026
@clawsweeper

clawsweeper Bot commented Oct 4, 2026 •

Copy link
Copy Markdown

Codex review: needs maintainer review before merge. Reviewed October 7, 2026, 7:50 AM ET / 11:50 UTC (Revision 15).

ClawSweeper review

What this changes

The PR makes delete and complete process each resolved reminder once, preserves input order and validation, and adds regression tests and documentation.

Merge readiness

✅ Ready for maintainer review

The fix remains necessary: current main and v0.3.8 still resolve repeated targets multiple times. No blocking correctness finding or additional contributor action remains.

Priority: P2
Reviewed head: fa8ca21f84e0e23b63a5a320666e0bd15f7fb9cc

Review scores

Measure Result What it means
Overall readiness 🐚 platinum hermit (4/6) A focused repair with applicable real macOS proof, broader regression coverage, and no blocking findings.
Proof confidence 🦞 diamond lobster (5/6) Sufficient (terminal): Previously accepted macOS terminal proof exercises the unchanged resolver through repeated-ID preview, confirmation, real EventKit deletion, and an empty follow-up search. The latest commit adds tests and documentation; no stored-data contract changes.
Patch quality 🐚 platinum hermit (4/6) No actionable review findings were identified.

Verification

Check Result Evidence
Real behavior Verified Sufficient (terminal): Previously accepted macOS terminal proof exercises the unchanged resolver through repeated-ID preview, confirmation, real EventKit deletion, and an empty follow-up search. The latest commit adds tests and documentation; no stored-data contract changes.
Evidence reviewed 7 items Introduced implementation: Both numeric and prefix resolution append through an identity-based set while retaining the existing validation checks. The set changes only the returned target array; it introduces no persistent format or authorization change.
Current main still needs the repair: Main appends every resolved input, including repeated identities. CommandHelpers forwards this array to delete and complete; deletion looks up and commits removal separately for each supplied ID.
Latest release comparison: The resolver at the supplied latest release, v0.3.8, also appends repeated targets without deduplication.
Findings None None.
Security None None.

How this fits together

remindctl translates command-line indexes and ID prefixes into Apple Reminders targets. Delete and complete use those targets for previews and EventKit mutations.

flowchart TD
  A[Indexes and ID prefixes] --> B[Fetch reminders]
  B --> C[Resolve and validate targets]
  C --> D[Keep first occurrence]
  D --> E[Preview and confirmation]
  D --> F[EventKit mutation]
  F --> G[Command result]
Loading

Before merge

None.

Agent review details

Security

None.

Review metrics

Metric Value Why it matters
Production and test delta production +8/-2, tests +33/-0 The small production change directly repairs repeated mutation targets and has focused regression coverage.

Technical review

Best possible solution:

Use one ordered target per reminder identity throughout preview, confirmation, and mutation.

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

Yes: main resolves a repeated ID twice, and deletion commits removal for each occurrence, causing the second lookup to fail. This review established the mechanism from source without executing target code.

Is this the best way to solve the issue?

Yes: deduplicating resolved identities in the shared resolver aligns previews and mutations while preserving order and validating every input.

AGENTS.md: not found in the target repository.

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

Labels

Label changes:

No label changes.

Label justifications:

  • P2: This repairs a bounded deletion failure triggered by repeated reminder identifiers.
  • rating: 🐚 platinum hermit: Overall readiness is 🐚 platinum hermit; proof is 🦞 diamond lobster and patch quality is 🐚 platinum hermit.
  • status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Sufficient (terminal): Previously accepted macOS terminal proof exercises the unchanged resolver through repeated-ID preview, confirmation, real EventKit deletion, and an empty follow-up search. The latest commit adds tests and documentation; no stored-data contract changes.
  • proof: sufficient: Contributor real behavior proof is sufficient. Previously accepted macOS terminal proof exercises the unchanged resolver through repeated-ID preview, confirmation, real EventKit deletion, and an empty follow-up search. The latest commit adds tests and documentation; no stored-data contract changes.

Evidence

What I checked:

  • Introduced implementation: Both numeric and prefix resolution append through an identity-based set while retaining the existing validation checks. The set changes only the returned target array; it introduces no persistent format or authorization change. (Sources/RemindCore/IDResolver.swift:14, fa8ca21f84e0)
  • Current main still needs the repair: Main appends every resolved input, including repeated identities. CommandHelpers forwards this array to delete and complete; deletion looks up and commits removal separately for each supplied ID. (Sources/RemindCore/IDResolver.swift:21, cdd7a06b8527)
  • Latest release comparison: The resolver at the supplied latest release, v0.3.8, also appends repeated targets without deduplication. (Sources/RemindCore/IDResolver.swift:21, ea2fb1098a2d)
  • Regression coverage: The added cases cover repeated indexes and IDs, mixed prefixes and case, first-occurrence order, filtered numeric views, and invalid subsequent inputs. The captured PR body reports six failing assertions on main and all 10 resolver tests passing after the fix; this read-only review did not execute tests. (Tests/RemindCoreTests/IDResolverTests.swift:41, fa8ca21f84e0)
  • Previously accepted runtime proof remains applicable: The prior completed review at fix: deduplicate reminder targets before delete and complete #98 (comment) records a macOS transcript at 5c18880: create a disposable reminder, preview its repeated full ID, confirm one target, delete once through EventKit, and observe an empty subsequent search. The resolver is unchanged between that reviewed revision and the current head. The current captured body retains the EventKit verification claim. The original transcript is summarized in the prior review rather than reproduced in the current body. (Sources/RemindCore/IDResolver.swift:14, 5c1888016409)
  • Feature-history routing: Available main history records Peter Steinberger's initial CLI, show-view resolution, and numeric-index validation work. GitHub commit metadata identifies steipete for the validation commit. Some older parent blobs were unavailable, so exact source-line introduction is not asserted. (Sources/RemindCore/IDResolver.swift, 310c1860d73d)

Likely related people:

  • steipete: Suggested for follow-up; no historical authorship or introduction is verified. (role: unverified routing candidate; 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.

History

Review history (14 earlier review cycles; latest 8 shown)
  • reviewed 2026-10-06T04:34:22.466Z sha 5c18880 :: needs real behavior proof before merge. :: none
  • reviewed 2026-10-06T09:53:46.609Z sha 5c18880 :: needs real behavior proof before merge. :: none
  • reviewed 2026-10-06T12:40:54.483Z sha 5c18880 :: needs real behavior proof before merge. :: none
  • reviewed 2026-10-06T18:00:35.048Z sha 5c18880 :: needs real behavior proof before merge. :: none
  • reviewed 2026-10-06T18:27:37.463Z sha 5c18880 :: needs maintainer review before merge. :: none
  • reviewed 2026-10-06T20:49:36.780Z sha 5c18880 :: needs maintainer review before merge. :: none
  • reviewed 2026-10-07T05:50:27.610Z sha 5c18880 :: needs maintainer review before merge. :: none
  • reviewed 2026-10-07T10:49:18.215Z sha 5c18880 :: needs maintainer review before merge. :: none

@clawsweeper clawsweeper Bot added 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. and removed status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. labels Oct 6, 2026
Cover repeated IDs and indexes, mixed prefixes, filtered numeric views, first-input order, and validation after duplicates. Document unique delete and complete targets.

Co-authored-by: Sebastien Tardif <sebtardif@ncf.ca>
@steipete steipete changed the title fix: dedupe reminder ids before delete fix: deduplicate reminder targets before delete and complete Oct 7, 2026
@steipete
steipete merged commit dc9fbe4 into openclaw:main Oct 7, 2026
8 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.

2 participants