Skip to content

fix: dedupe reminder ids before delete - #98

Open
SebTardif wants to merge 1 commit into
openclaw:mainfrom
SebTardif:fix/remindctl-f008
Open

SebTardif wants to merge 1 commit into
openclaw:mainfrom
SebTardif:fix/remindctl-f008

Conversation

@SebTardif

@SebTardif SebTardif commented Oct 4, 2026 •

Copy link
Copy Markdown
Contributor

What Problem This Solves

remindctl delete with the same reminder id twice resolved that reminder twice. It removed the reminder, then failed the second lookup. The command now keeps the first id, so the confirmation count and the removal both see one reminder.

Why This Change Was Made

Identifier resolution keeps the first occurrence of each reminder id, in input order.

  • Sources/RemindCore/IDResolver.swift
  • Tests/RemindCoreTests/IDResolverTests.swift

User Impact

Passing the same id twice asks to delete one reminder and removes that one reminder.

Evidence

Patched commit 5c1888016409351ddf1fa233fe9a158b727e181d on fix/remindctl-f008, built on macOS with full Reminders access. One disposable reminder was created in the Reminders list, then deleted by passing its id twice.

$ remindctl add remindctl98-proof-disposable --json
{
  "id" : "03E206FE-92C9-49E0-AE61-88112D175A3F",
  "listName" : "Reminders",
  "title" : "remindctl98-proof-disposable"
}

$ remindctl delete 03E206FE-92C9-49E0-AE61-88112D175A3F 03E206FE-92C9-49E0-AE61-88112D175A3F --dry-run --json
[
  {
    "id" : "03E206FE-92C9-49E0-AE61-88112D175A3F",
    "title" : "remindctl98-proof-disposable"
  }
]

$ remindctl delete 03E206FE-92C9-49E0-AE61-88112D175A3F 03E206FE-92C9-49E0-AE61-88112D175A3F
Delete 1 reminder(s)? [y/N] y
Deleted 1 reminder(s)

$ remindctl search remindctl98-proof-disposable --json
[
]

Real behavior proof

  • Behavior or issue addressed: Duplicate reminder ids were resolved twice, so delete confirmed two targets and then failed the second removal. The same id is now kept once.
  • Real environment tested: macOS, Reminders access full, remindctl built from commit 5c1888016409351ddf1fa233fe9a158b727e181d.
  • Exact steps or command run after this patch: Created one disposable reminder in the Reminders list, previewed delete with that id twice, then ran delete with the same id twice and answered the confirmation. Searched for the title afterward.
  • Evidence after fix: terminal output from the patched delete command:
$ remindctl delete 03E206FE-92C9-49E0-AE61-88112D175A3F 03E206FE-92C9-49E0-AE61-88112D175A3F --dry-run --json
[
  {
    "id" : "03E206FE-92C9-49E0-AE61-88112D175A3F",
    "title" : "remindctl98-proof-disposable"
  }
]

$ remindctl delete 03E206FE-92C9-49E0-AE61-88112D175A3F 03E206FE-92C9-49E0-AE61-88112D175A3F
Delete 1 reminder(s)? [y/N] y
Deleted 1 reminder(s)

$ remindctl search remindctl98-proof-disposable --json
[
]
  • Observed result after fix: The duplicate id resolved to one reminder. The prompt said Delete 1 reminder(s). The command printed Deleted 1 reminder(s). A later search for that title returned no reminders.
  • What was not tested: hosted runners, and lists other than the default Reminders list.

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, 6:49 AM ET / 10:49 UTC (Revision 14).

ClawSweeper review

What this changes

The PR resolves each reminder once while preserving input order, so repeated indexes or ID prefixes produce accurate delete previews and confirmation counts.

Merge readiness

✅ Ready for maintainer review

This PR remains useful: main and v0.3.8 still resolve duplicate targets repeatedly. The focused patch and real macOS delete transcript support readiness, with no blocking findings or additional required action.

Priority: P2
Reviewed head: 5c1888016409351ddf1fa233fe9a158b727e181d

Review scores

Measure Result What it means
Overall readiness 🐚 platinum hermit (4/6) A focused, correct repair with regression coverage and convincing real macOS behavior proof.
Proof confidence 🦞 diamond lobster (5/6) Sufficient (terminal): The exact-head macOS terminal transcript exercises ID resolution through delete preview, interactive confirmation, EventKit removal, and a subsequent empty search, demonstrating successful deletion of one repeated target. 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): The exact-head macOS terminal transcript exercises ID resolution through delete preview, interactive confirmation, EventKit removal, and a subsequent empty search, demonstrating successful deletion of one repeated target. No stored-data contract changes.
Evidence reviewed 6 items Verified introduced patch: The pinned base-to-head delta adds identity-based deduplication to both resolution branches and regression coverage for repeated indexes, mixed identifiers, and distinct-target order.
Current main still needs the fix: The main resolver appends every resolved input without deduplication; the shared command helper forwards its results to delete and complete.
Release comparison: The resolver in the supplied latest release, v0.3.8, likewise appends duplicate targets; this change is not already implemented there.
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 commands consume those targets through EventKit.

flowchart TD
  A[Indexes and ID prefixes] --> B[Fetch Apple reminders]
  B --> C[Resolve reminder targets]
  C --> D[Keep first occurrence of each reminder]
  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 +10/-0 The small production change directly addresses repeated mutation targets and has focused regression coverage.

Technical review

Best possible solution:

Keep one ordered target per reminder identity before previewing or mutating reminders.

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

Yes: main resolves a repeated ID twice and deletion performs a committed removal for each occurrence. This review established the mechanism from source without executing target code.

Is this the best way to solve the issue?

Yes: deduplicating resolved identities aligns preview and mutation while preserving order, existing validation, and distinct targets.

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 fixes a bounded delete 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): The exact-head macOS terminal transcript exercises ID resolution through delete preview, interactive confirmation, EventKit removal, and a subsequent empty search, demonstrating successful deletion of one repeated target. No stored-data contract changes.
  • proof: sufficient: Contributor real behavior proof is sufficient. The exact-head macOS terminal transcript exercises ID resolution through delete preview, interactive confirmation, EventKit removal, and a subsequent empty search, demonstrating successful deletion of one repeated target. No stored-data contract changes.

Evidence

What I checked:

  • Verified introduced patch: The pinned base-to-head delta adds identity-based deduplication to both resolution branches and regression coverage for repeated indexes, mixed identifiers, and distinct-target order. (Sources/RemindCore/IDResolver.swift:14, 5c1888016409)
  • Current main still needs the fix: The main resolver appends every resolved input without deduplication; the shared command helper forwards its results to delete and complete. (Sources/RemindCore/IDResolver.swift:21, cdd7a06b8527)
  • Release comparison: The resolver in the supplied latest release, v0.3.8, likewise appends duplicate targets; this change is not already implemented there. (Sources/RemindCore/IDResolver.swift:21, ea2fb1098a2d)
  • Production deletion boundary: Delete uses the resolved array for preview, confirmation count, and mutation. The EventKit store looks up and commits removal separately for every supplied ID, explaining why duplicate targets can fail after the first removal. (Sources/RemindCore/EventKitStore.swift:193, 5c1888016409)
  • After-fix real behavior proof: The complete supplied PR body identifies the exact patched commit and macOS with full Reminders access. Its terminal transcript creates a disposable reminder, previews deletion with the same full ID twice, confirms one target, reports one deletion, and shows an empty subsequent search. This exercises the resolver through the real CLI and EventKit deletion path. (5c1888016409)
  • Area history and routing: Available history records Peter Steinberger's work on initial command behavior, show-based identifier resolution, and index validation. GitHub commit metadata verifies the steipete login for the validation commit. Older blame traversal encountered unavailable promisor objects, so no exact introduction attribution is asserted. Canonical GitHub list searches were denied by the read-only endpoint policy; the supplied context establishes no replacement PR. (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 (13 earlier review cycles; latest 8 shown)
  • reviewed 2026-10-06T01:05:11.311Z sha 5c18880 :: needs real behavior proof before merge. :: none
  • 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

@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

This branch has not been deployed

No deployments
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