fix(calendar): reject repeated instance page tokens

[SebTardif]

What Problem This Solves

gog calendar delete and gog calendar update with --scope single or --scope future resolve the target instance through Events.Instances. That helper rebuilt each page request from NextPageToken and never recorded tokens it had already used. If Google returned the same token again, instance lookup never finished, so the command never deleted or patched the event.

The same unguarded rebuild has already been fixed for CalendarList (#1004) and other cmd list helpers. Recurring instance lookup was still on the old loop, introduced with calendar recurrence parity in #38 (1d595747, 2026-01-08).

Evidence

Public path: CalendarDeleteCmd.Run and the update/edit commands call resolveRecurringScopeResolution, which calls resolveRecurringInstanceID whenever --scope is single or future and --original-start is set.

A local Calendar Instances peer that always returns nextPageToken=stuck (and a non-matching item) shows the hang versus the fail-closed walk. The unguarded rebuild kept requesting until the peer refused after 40 calls. The seen-token walk stopped after two calls.

$ go run C:/Users/sebta/.grok/tmp/instance-page-token-proof.go
OLD: unguarded Events.Instances rebuild
old calls=41 err=googleapi: got HTTP response code 429 with body: stopped after 40 requests
NEW: collectAllPages seen-token walk
new calls=2 err=pagination loop: repeated page token "stuck"

The same peer through CalendarDeleteCmd with --scope single and --original-start 2025-01-02T10:00:00Z now returns that pagination error, makes two instance list calls, writes no JSON, and never sends DELETE.

Real behavior proof

• Behavior or issue addressed: Recurring instance lookup for calendar edit and delete hangs if Events.Instances repeats a page token.

• Real environment tested: Windows, Go 1.27.1, openclaw/gogcli at 25703c78 plus this patch, generated Calendar client against a local Instances peer.

• Exact steps or command run after this patch:

  go run C:/Users/sebta/.grok/tmp/instance-page-token-proof.go
  

• Evidence after fix: terminal output from the patched walk:

  OLD: unguarded Events.Instances rebuild
  old calls=41 err=googleapi: got HTTP response code 429 with body: stopped after 40 requests
  NEW: collectAllPages seen-token walk
  new calls=2 err=pagination loop: repeated page token "stuck"
  

• Observed result after fix: The unguarded rebuild issued 41 instance list requests. After the patch, the same peer stops after two requests with pagination loop: repeated page token "stuck". calendar delete --scope single does not emit a result or send DELETE once lookup fails.

• What was not tested: A live Google Calendar account returning a repeated Instances token.

Related: #1004, #38, #1066.

[clawsweeper]

🦞👀
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]

Codex review: blocked before merge. Reviewed September 5, 2026, 5:01 AM ET / 09:01 UTC.

ClawSweeper review

What this changes

The PR adds repeated-token protection to recurring Calendar instance lookup and tests loop rejection, later-page matches, and deletion suppression.

Regression provenance

Possible regression — suspected (reviewed change). No predecessor PR is attributed.

Merge readiness

⛔ Blocked before merge - 3 items remain

The fix remains necessary on main and v0.39.0, but the earlier first-match correctness blocker remains unresolved. The supplied fault-injection evidence supports the repeated-token guard.

Priority: P2
Reviewed head: 80c06e835bdfa69b38215f70ceef9d3c00a448eb

Review scores

Measure	Result	What it means
Overall readiness	🦐 gold shrimp (3/6)	Useful loop protection has adequate fault evidence but retains a concrete compatibility regression.
Proof confidence	🐚 platinum hermit (4/6)	Sufficient (terminal): The supplied Windows transcript exercises the generated Calendar client against a repeating-token HTTP peer, reducing requests from 41 to two; accompanying CalendarDeleteCmd observations report no DELETE or JSON after lookup failure. This establishes the internal fault-handling behavior, while first-match compatibility remains a separate correctness gap.
Patch quality	🦐 gold shrimp (3/6)	1 actionable review finding remain.

Verification

Check	Result	Evidence
Real behavior	Verified	Sufficient (terminal): The supplied Windows transcript exercises the generated Calendar client against a repeating-token HTTP peer, reducing requests from 41 to two; accompanying CalendarDeleteCmd observations report no DELETE or JSON after lookup failure. This establishes the internal fault-handling behavior, while first-match compatibility remains a separate correctness gap.
Evidence reviewed	11 items	Current main still needs the guard: The resolver follows NextPageToken without recording previously used tokens, while returning immediately when an occurrence matches. Latest release retains the unguarded lookup: The v0.39.0 resolver contains the same unguarded instance loop; the related listing fixes did not cover this path. Introduced first-match regression: The callback forwards every next token, and matching happens only after collection succeeds. An unnecessary later request can therefore invalidate an occurrence already returned by Google.
Findings	1 actionable finding	[P1] Preserve the early return when the requested instance is found
Security	None	None.

How this fits together

Calendar update and delete commands resolve a recurring occurrence from its original start time before modifying Google Calendar. This lookup consumes paginated Instances responses and returns either an event ID or an error.

flowchart TD
  A[Calendar update or delete] --> B[Resolve recurrence scope]
  B --> C[Fetch Google instance pages]
  C --> D[Pagination guard]
  D --> E[Match original start]
  D --> F[Return lookup error]
  E --> G[Modify selected occurrence]

Loading

Before merge

• Preserve the early return when the requested instance is found (P1) - If an Instances response contains the requested occurrence and a nonempty nextPageToken, this forwards that token before checking the match. A later API error or token cycle makes collectAllPages discard the already-found occurrence, so existing --scope single and --scope future updates/deletes fail unnecessarily. This prior blocker remains: inspect each page for a match before continuing and cover a matching page that must not cause another request.
• Resolve merge risk (P1) - Existing single/future updates and deletes can start failing after upgrade when a matching page advertises another page and that unnecessary request errors or repeats a token.
• Complete next step (P2) - Restore first-match success with a no-extra-request regression and add the credited fix to the existing Unreleased changelog section.

Findings

• [P1] Preserve the early return when the requested instance is found — internal/cmd/calendar_recurrence.go:80-84

Agent review details

Security

None.

Review metrics

Metric	Value	Why it matters
Production and test delta	production +18/-20; tests +173/-0; 3 tests added	The implementation stays small and adds focused fault coverage, but no added test covers a match on a nonterminal page.

Merge-risk options

Maintainer options:

1. Preserve successful instance lookup (recommended)
   Stop collection when a page contains the requested occurrence, while retaining cycle rejection for unresolved searches and adding compatibility coverage.

Copy recommended automerge instruction

@clawsweeper automerge

Special instructions:
Check each Instances response for the requested occurrence before forwarding its next token to collectAllPages; terminate collection on a match without changing the shared collector's full-list error semantics. Add regressions proving no further request after a matching page, retain repeated-token and deletion-suppression coverage, and add a credited Calendar fix to the existing Unreleased changelog section.

Technical review

Best possible solution:

Instance lookup should stop at the first matching occurrence and reject token cycles only while the search remains unresolved, preserving existing scoped mutations.

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

Yes, from source: repeating a nonempty Instances token with no matching occurrence keeps main paging; a matching page followed by an error exposes the PR's compatibility regression. Neither scenario was executed during this read-only review.

Is this the best way to solve the issue?

No, collecting all pages changes successful lookup behavior unnecessarily; the existing collector can still be reused if the callback terminates collection immediately upon finding the occurrence.

Full review comments:

• [P1] Preserve the early return when the requested instance is found — internal/cmd/calendar_recurrence.go:80-84
  If an Instances response contains the requested occurrence and a nonempty nextPageToken, this forwards that token before checking the match. A later API error or token cycle makes collectAllPages discard the already-found occurrence, so existing --scope single and --scope future updates/deletes fail unnecessarily. This prior blocker remains: inspect each page for a match before continuing and cover a matching page that must not cause another request.
  Confidence: 0.99

Overall correctness: patch is incorrect
Overall confidence: 0.98

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning high; reviewed against 25703c789da5.

Labels

Label justifications:

• P2: This is a bounded Calendar reliability fix for faulty pagination, without evidence of a widespread active outage.
• merge-risk: 🚨 compatibility: Collecting every page before matching can turn previously successful scoped Calendar mutations into errors.
• rating: 🦐 gold shrimp: Overall readiness is 🦐 gold shrimp; proof is 🐚 platinum hermit and patch quality is 🦐 gold shrimp.
• status: ⏳ waiting on author: ClawSweeper has contributor-facing work open and is waiting for author action. Sufficient (terminal): The supplied Windows transcript exercises the generated Calendar client against a repeating-token HTTP peer, reducing requests from 41 to two; accompanying CalendarDeleteCmd observations report no DELETE or JSON after lookup failure. This establishes the internal fault-handling behavior, while first-match compatibility remains a separate correctness gap.
• proof: sufficient: Contributor real behavior proof is sufficient. The supplied Windows transcript exercises the generated Calendar client against a repeating-token HTTP peer, reducing requests from 41 to two; accompanying CalendarDeleteCmd observations report no DELETE or JSON after lookup failure. This establishes the internal fault-handling behavior, while first-match compatibility remains a separate correctness gap.

Evidence

Acceptance criteria:

• [P1] go test ./internal/cmd -run '^(TestResolveRecurring|TestCalendarDeleteCmd|TestCalendarUpdateCmd|TestCollectAllPages)' -count=1.
• [P1] make ci.

What I checked:

• Current main still needs the guard: The resolver follows NextPageToken without recording previously used tokens, while returning immediately when an occurrence matches. (internal/cmd/calendar_recurrence.go:73, 25703c789da5)
• Latest release retains the unguarded lookup: The v0.39.0 resolver contains the same unguarded instance loop; the related listing fixes did not cover this path. (internal/cmd/calendar_recurrence.go:73, b058dbbaeb44)
• Introduced first-match regression: The callback forwards every next token, and matching happens only after collection succeeds. An unnecessary later request can therefore invalidate an occurrence already returned by Google. (internal/cmd/calendar_recurrence.go:80, 80c06e835bdf)
• Collector deliberately discards partial results: The shared collector returns nil on provider errors or repeated tokens. Its tests explicitly enforce that contract, so the resolver must stop collection when its own search succeeds. (internal/cmd/paging.go:25, 80c06e835bdf)
• Verified merge retains the defect: The test merge has the exact supplied main and head parents, and its comparison against main contains the same collect-before-match change. (internal/cmd/calendar_recurrence.go:80, 318707df45c6)
• Prior review continuity: The completed review at fix(calendar): reject repeated instance page tokens #1084 (comment) requested first-match success and a credited Unreleased entry. Both remain outstanding in the current source; the supplied latest commit description addresses error-variable shadowing. (bad68b64b1c2)

Likely related people:

• Peter Steinberger: Raw commit f1d355b adds internal/cmd/calendar_recurrence_scope.go:44 relative to its recorded parents. This identifies author metadata, not feature responsibility or a PR merger. (role: source-line author; confidence: high; commits: f1d355bfa7e4; files: internal/cmd/calendar_recurrence_scope.go)
• SebTardif: Suggested for follow-up; no historical authorship or introduction is verified. (role: unverified routing candidate; confidence: low)

Rank-up moves

Optional improvements that raise the rating; they are not merge blockers.

• Preserve first-match success and add coverage proving that a matching page cannot trigger another request.
• Add the Calendar fix to the existing Unreleased changelog section with contributor credit.

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 (1 earlier review cycle)

• reviewed 2026-09-05T07:47:37.416Z sha bad68b6 :: blocked before merge. :: [P1] Preserve the early return when the requested instance is found

[steipete]

Superseded by #1087, which incorporates recurring-instance cycle protection while preserving immediate success when the requested occurrence is found. Failed lookup performs no event deletion. Thanks @SebTardif. Superseded by #1087, which incorporates cycle protection for Drive tree, inventory, and disk-usage scans. All three stock CLI commands were verified to fail without partial success output. Thanks @SebTardif. Superseded by #1087, which incorporates Gmail query-cycle protection, including repeated empty pages, while retaining existing result limits. Failure occurs before downstream message changes. Thanks @SebTardif.