fix(paging): stop calendar and Gmail listing on a repeated page token - #1004
Merged
Conversation
listCalendarList now uses collectAllPages. Gmail backup ID listing rejects a repeated NextPageToken the same way Photos Picker already does, so a stuck Google token cannot hang --all or backup. Signed-off-by: Sebastien Tardif <sebtardif@ncf.ca>
Contributor
|
🦞👀 Pull request received. I will update this pull request when review starts. |
clawsweeper
Bot
added
P2
Contributor
|
Codex review: needs real behavior proof before merge. Reviewed August 24, 2026, 9:16 PM ET / August 25, 2026, 01:16 UTC. ClawSweeper reviewWhat this changesThe PR detects repeated continuation tokens in calendar-list and Gmail backup message-ID pagination, with regression coverage for both paths. Regression provenancePossible regression — probable (reviewed change; reproduction). No predecessor PR is attributed. Merge readiness⛔ Blocked until stronger real behavior proof is added - 3 items remain This focused reliability fix is still needed: current main retains both unguarded loops, and the patch has no code-level or security finding. Before merge, the Gmail half needs production-transport proof rather than only a fake Source implementation. Priority: P2 Review scores
Verification
Live VerificationCommand: Result: FAIL (failed) — execution before step 1 Assertions:
How this fits togetherCalendar commands enumerate calendars before selecting and listing events, while Gmail backup enumerates message IDs before fetching messages. Both paths consume Google API continuation tokens to request subsequent pages. flowchart LR
A[Google list response] --> B[Calendar or Gmail paginator]
B --> C[Read continuation token]
C --> D{Previously seen?}
D -->|No| E[Request next page]
E --> A
D -->|Yes| F[Return pagination error]
F --> G[CLI command or backup workflow]
Before merge
Agent review detailsSecurityNone. Review metrics
Merge-risk optionsMaintainer options:
Technical reviewBest possible solution: Keep the narrow guards and tests, then add redacted terminal proof using Gmail ServiceSource with a controlled repeated-token HTTP response before merging. Do we have a high-confidence way to reproduce the issue? Yes. Current main directly reuses returned continuation tokens in both paths, and the branch provides deterministic repeated-token harnesses that exercise the resulting error path. Is this the best way to solve the issue? Yes. Reusing the existing guarded collector for Calendar and keeping a small package-local seen-token set for Gmail is the narrowest maintainable repair. AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against c4952a2241c4. LabelsLabel changes:
Label justifications:
EvidenceWhat I checked:
Likely related people:
Rank-up movesOptional improvements that raise the rating; they are not merge blockers.
Rating scale
Overall follows the weaker of proof and patch quality. Workflow
HistoryReview history (1 earlier review cycle)
|
clawsweeper
Bot
added
rating: 🦐 gold shrimp
Co-authored-by: Sebastien Tardif <sebtardif@ncf.ca>
Contributor
|
ClawSweeper status: review started. I am starting a fresh review of this pull request: fix(paging): stop calendar and Gmail listing on a repeated page token This is item 1/1 in the current shard. Shard 0/1. This placeholder means the worker is alive and reading the current context. I will edit this same comment with the actual review when the claws are done clicking. Crustacean status: shell secured, claws on keyboard, evidence pebbles being sorted. |
Collaborator
|
Landed in 0996b6a after strengthening the original fix and independently verifying the real Google client path.
Thanks @SebTardif! |
chrischall
added a commit
to chrischall/gogcli-mcp
that referenced
this pull request
Aug 25, 2026
… and calendar reminders (#286) Wraps the new surface in [gogcli 0.38.0](https://github.com/openclaw/gogcli/releases/tag/v0.38.0). Two commits, so the PR title is what release-please parses. ## What | Area | Tools | Notes | |---|---|---| | **Sheets** — Connected Sheets writes | `datasource_add` / `_update` / `_refresh` / `_delete` | Completes the read-only half wrapped for 0.37.0 | | **Slides** — visibility | `skip_slide` / `unskip_slide` | Keeps the slide in the deck; omits it when presenting | | **Calendar** — reminders | params on `create` + `update` | `--reminder` was never wrapped either; `--no-reminders` is the 0.38.0 addition | | **Chat** (new service) | 12 tools | spaces, threads, messages, DMs, reactions, `run` | | **Apps Script** (new service) | 8 tools | metadata, source, pull, deployments, versions, function execution, `run` | Chat and Apps Script land as **base service modules with no sub-package**, following the `tasks` precedent. Base is now 112 tools across 13 services. ### Decisions worth reviewing - **`datasource_delete` carries `--force`.** gog gates it and the runner always injects `--no-input`. Confirmed live rather than assumed — without it gog answers `refusing to delete Connected Sheets data source … without --force (non-interactive)`. The mocked suite cannot catch this, per the Gotchas section in CLAUDE.md. - **`datasource_add/update/refresh` say they cost money.** Each starts a BigQuery execution billed to `billingProject`, and returns once the execution is *requested*, not finished — the descriptions tell the caller to poll `datasource_describe` for `SUCCEEDED`/`FAILED`. - **Calendar reminders are three states in one param pair.** A list sets overrides; `noReminders` disables them; an **empty list** is gog's "restore the calendar's defaults" (an empty `--reminder`), which differs from omitting the flag. All three verified with `--dry-run`. - **`appscript_run_function` is annotated destructive** and says so loudly: it executes someone's script under this account's authority, and the wrapper cannot tell a read from a write. - **Naming:** `gog_appscript_run` stays the escape hatch (fleet-consistent across 13 services); function execution is `gog_appscript_run_function`. Both descriptions point at each other. ### Version pins `MIN_GOG_VERSION` → **0.38.0**, with `fly-gog-runner/Dockerfile` and the nine `mint.yaml` pins from #284 moved alongside it. The second commit also extends `check-runner-gog-version.mjs` to cover `mint.yaml`, since an mcp-host `--npm` registration installs gog from that pin — a third path nothing was checking. Verified the guard *fails* on a stale pin, not merely that it passes. ## Test plan - `npm test` — 878 base + sub-package suites, 100% coverage gate held (three branches the first pass missed are now covered). - `npm run typecheck` and `npm run build` clean. (A green vitest run is not a green `tsc`.) - **Live against a real gog 0.38.0**, beyond the mocked suite: every new argv shape dry-run; the delete gate refused without `--force` and proceeded with it; all three reminder states produced the expected `reminders` patch. ## Known limits, deliberately shipped Neither new service is reachable on a consumer account, so both facts are in the **tool descriptions** where a model reads them, not just in comments: - **Chat is Workspace-only.** gog refuses every call on `@gmail.com` regardless of scopes — re-authorizing cannot fix it, and a model that reads "permission error" as "missing scope" would otherwise waste a re-auth round trip. - **The Apps Script API is off by default** on a Cloud project; gog's error names the exact console URL. Also a project setting, not a scope. `docs/auth-scopes.md` now says both, since it previously claimed these scopes had no tools here. ## Follow-ups (not in this PR) - **The Fly runner still runs 0.37.0** — the Dockerfile pin is bumped, but the hosted connector needs a redeploy before it actually has the binary. - **`fetchGmailPages` has no repeated-page-token guard** (`gmail-results.ts:190`). gog 0.38.0 fixed this on its own side (openclaw/gogcli#1004), but our wrapper does its own multi-page walk over single-page gog calls: if Google repeats a token we re-fetch and merge duplicates until `maxPages`. Wrong results rather than an error. - Whether Chat belongs in the kitchen-sink base package at all, given it can never work for a consumer user, and given base is now 112 tools. Also corrects the base README/SKILL service tables, which advertised "52 tools across 8 services" while the package shipped 112 across 13 — Slides, Classroom and the API escape hatch were missing entirely, predating this change. 🤖 Generated with [Claude Code](https://claude.com/claude-code) https://claude.ai/code/session_01119smk3joNhptNrSQfLK1e --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
chrischall
added a commit
to chrischall/gogcli-mcp
that referenced
this pull request
Aug 25, 2026
#288) Found while reviewing the gog 0.38.0 surface for #286; independent of that PR and branched off `main`, so the two can land in either order. ## The bug `fetchGmailPages` (`packages/gogcli-mcp/src/gmail-results.ts`) walks up to `maxPages` by making **N separate single-page gog calls**, following whatever `nextPageToken` each returns. Google can hand back a cursor it has already issued. Following it re-fetches a page that was already merged, so every remaining iteration appends the same threads again. Two harms, the first much worse than the second: 1. **Duplicates presented as more results.** The payload looks perfectly well-formed — nothing about it says "these twelve threads are the same three, four times over". A caller counting or de-duplicating downstream gets a wrong answer with no error to notice. 2. **A wasted gog spawn per duplicate**, up to the full `maxPages` budget. gog 0.38.0 fixed this class on its own side (openclaw/gogcli#1004 — "stop safely when Google repeats a pagination token"), but that fix covers gog's `--all`. This walk is ours: separate `--page=<token>` invocations, so the repeated cursor arrives here untouched. Reachable today through `gog_gmail_search` and the gmail sub-package's search whenever `maxPages > 1`. ## The fix The walk remembers every cursor it has fetched with and stops on a repeat, **before** requesting the duplicate page — so the duplicate is never fetched and never merged. - **Seeded with the caller's own cursor**, since a response echoing that straight back would re-fetch the page just merged. - **Detects any earlier cursor**, not just the immediately-previous one — a `T1 → T2 → T1` cycle stalls exactly the same way. - **Keeps the cursor in the result** rather than dropping it. The walk never established an end, so the set stays truncated; dropping it would claim a completeness we never had, and a set that reads complete is precisely how mail that exists gets reported as missing. Handing the cursor back also lets a caller retry later. `fetchGmailPages` is the only multi-page walk in the repo — I checked the rest of `nextPageToken`'s uses, and they are single-page pass-through or descriptive text, so there is no second instance of this class to fix. ## Test plan - Four new cases in `tests/gmail-results.test.ts`, written first and confirmed failing against the old code — the first failed by running off the end of the mocked responses, which is the stall itself. - stops rather than re-fetching, and does not burn the remaining page budget - the result still reads as truncated afterwards - a cursor repeating the *caller-supplied* one is caught - a cursor repeating an *earlier* token in the walk is caught - `npm test` green (834 base + sub-package suites) with the 100% coverage gate held; `npm run typecheck` and `npm run build` clean. 🤖 Generated with [Claude Code](https://claude.com/claude-code) https://claude.ai/code/session_01119smk3joNhptNrSQfLK1e Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What Problem This Solves
gog calendar events --all(and any other path that lists every calendar) walksCalendarList.Listwith no memory of page tokens it has already seen. Gmail backup ID listing does the same onusers.messages.list. If Google repeatsnextPageToken, both loops never finish.Why
The repo already has this guard.
collectAllPageserrors withpagination loop: repeated page token. Photos Picker and Drive changes do the same with a local seen-token map.listCalendarListis in packagecmd, so it now callscollectAllPagesinstead of growing a second helper. Gmail backup lives underinternal/backup/gmailand cannot importcmd, so it adds the same localseenTokensmap Photos Picker already uses.User Impact
A stuck Google page token now fails with
repeated page tokeninstead of hanginggog calendar events --allor a Gmail backup list until the process is killed.Evidence
terminal output from the unpatched loops versus this patch, using a Calendar
httptestserver and a Gmail source that always returnnextPageToken=stuck.Unpatched (300ms context; the HTTP peer answers immediately, so the deadline is the only stop):
Patched (same commands, same stuck token, returns immediately):
go test ./internal/cmd/ ./internal/backup/gmail/ -count=1also completed on this tree.Real behavior proof
nextPageTokenvalues could hang calendar listing (--all) and Gmail backup ID listing./tmp/pr-gogcli-pagesonfix/repeated-page-tokensfromupstream/main./tmp/pr-gogcli-pages, ran the samego testcommands shown above against a Calendarhttptestthat always returnsnextPageToken=stuck, then rango run proof_gmail_pages.gowhich calls exportedListMessageIDswith a source that always returnsNextPageToken "stuck".pagination loop: repeated page token "stuck"in 0.00s. GmailListMessageIDsreturnslist Gmail backup messages: repeated page token "stuck"withelapsed=0s.Related
collectAllPagesininternal/cmd/paging.go, Photos Picker (errPhotosPickerRepeatedPage), and Drive changes.9977c0be, 2026-02-15, 184 days ago).12461f5d(2026-06-13, 66 days ago).