fix(backup): reject repeated Groups Admin Keep page tokens

[SebTardif]

What Problem This Solves

gog backup push with Groups, Admin, or Keep walks Google page tokens in internal/cmd/backup_directory_keep.go. Five helpers copied nextPageToken into the next request with no seen-set: Cloud Identity group search, Cloud Identity memberships, Admin users, Admin groups, and Keep notes.

When Google repeats a continuation token, those loops never terminate. Backup collection keeps requesting the same page and never finishes the snapshot.

Admin group members in the same file already use loadPagedItems, which calls collectAllPages. Interactive Groups and Admin listing already use that helper too. Backup listing for Groups, Admin users/groups, and Keep was still on the unguarded loop.

The same hang class is already closed for Chat and Classroom backup (#1063), Drive sync push listing (#1065), Drive audit permission listing (#1066), calendar and Gmail listing (#1004), and People/Gmail-from-contact/contacts-export listing (#1044, #1045, #1046). Drive backup listing is the open sibling #1078 (different file). This change does not touch backup_drive.go.

The unguarded loops landed in efc3df2e (2026-04-27, feat(backup): expand google backup coverage).

Evidence

terminal output from the compiled internal/cmd listing binary after the patch. A stuck continuation token is rejected after two list calls, with no third request:

$ gogcli-backup-paging.exe -test.run TestFetchBackupCloudIdentityGroupsRejectsRepeatedPageToken -test.v
    backup_directory_keep_test.go:54: err = pagination loop: repeated page token "stuck" after 2 list calls

$ gogcli-backup-paging.exe -test.run TestFetchBackupAdminUsersRejectsRepeatedPageToken -test.v
    backup_directory_keep_test.go:140: err = pagination loop: repeated page token "stuck" after 2 list calls

$ gogcli-backup-paging.exe -test.run TestFetchBackupKeepNotesRejectsRepeatedPageToken -test.v
    backup_directory_keep_test.go:225: err = pagination loop: repeated page token "stuck" after 2 list calls

Before the patch, the same stuck token made a third list request and returned HTTP 400 from the safety cap (unexpected extra ... page request after 3 list calls) instead of stopping on the repeated token.

Real behavior proof

• Behavior or issue addressed: Groups, Admin, and Keep backup listing hung when Google repeated a page token. gog backup push with those services never finished those listings.
• Real environment tested: Windows, Go 1.27.1, branch fix/backup-directory-page-token at current HEAD, compiled internal/cmd listing binary.
• Exact steps or command run after this patch: Compiled the listing binary with go, then ran gogcli-backup-paging.exe with -test.v on the Groups, Admin users, Admin groups, Keep notes, and Cloud Identity membership hang-guard cases.
• Evidence after fix: terminal output from that binary shows pagination loop: repeated page token "stuck" after 2 list calls for Cloud Identity groups, Admin users, Admin groups, Keep notes, and Cloud Identity memberships.
• Observed result after fix: A repeated Groups, Admin, or Keep backup page token now stops with the shared paginator error after two requests. Distinct pages still concatenate. Admin users stay sorted by primary email. Keep notes stay sorted by name.
• What was not tested: Live Google Workspace Groups, Admin Directory, or Keep accounts. Drive backup listing is fix(backup): reject repeated Drive backup page tokens #1078, not this change.

[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, 2:28 AM ET / 06:28 UTC.

ClawSweeper review

What this changes

Routes five Groups, Admin, and Keep backup listings through the existing pagination guard and adds repeated-token and successful-pagination tests.

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. The supplied pagination proof is sufficient, but the previously reported membership-retention regression still blocks merge.

Priority: P2
Reviewed head: db946f5cbe9e89f9ea1ffbbd03a2e7267478d2a1

Review scores

Measure	Result	What it means
Overall readiness	🦐 gold shrimp (3/6)	Useful, adequately demonstrated guard reuse remains blocked by one concrete backup compatibility regression.
Proof confidence	🐚 platinum hermit (4/6)	Sufficient (terminal): The captured Windows terminal evidence exercises the actual backup collectors through generated Google HTTP clients and injected repeated-token responses, showing termination after two requests. This is sufficient internal reliability proof; ordinary later-page failure compatibility remains a separate code finding.
Patch quality	🦐 gold shrimp (3/6)	1 actionable review finding remain.

Verification

Check	Result	Evidence
Real behavior	Verified	Sufficient (terminal): The captured Windows terminal evidence exercises the actual backup collectors through generated Google HTTP clients and injected repeated-token responses, showing termination after two requests. This is sufficient internal reliability proof; ordinary later-page failure compatibility remains a separate code finding.
Evidence reviewed	11 items	Applicable repository policy: Read the complete root AGENTS.md. No nested AGENTS.md or maintainer-notes directory was found. Review-only handling and the Unreleased changelog requirement apply. Current main still needs the guard: All five affected collectors still advance directly to NextPageToken without cycle detection. Cloud Identity membership collection appends successful pages before recording a later provider error. Latest release has the same affected implementation: v0.39.0 and fetched main both reference blob 4103be3ed5b2ce66af22397b55204d68ed475efc for this file; the release therefore retains the same unguarded loops.
Findings	1 actionable finding	[P1] Preserve earlier membership pages on ordinary API failures
Security	None	None.

How this fits together

Backup service adapters fetch Google account data and turn it into rows for encrypted Git snapshots. Their pagination and error handling determine which records reach the completed backup.

flowchart TD
  A[Backup push] --> B[Groups Admin and Keep adapters]
  B --> C[Google API pages]
  C --> D[Pagination guard]
  D --> E[Collected records]
  D --> F[Collection errors]
  E --> G[Backup snapshot assembly]
  F --> G
  G --> H[Encrypted Git backup]

Loading

Before merge

• Preserve earlier membership pages on ordinary API failures (P1) - If page one returns members and page two fails, returning callErr makes collectAllPages discard page one's members. The caller emits only an error row, whereas main retains the fetched members alongside that error. buildGroupsBackupSnapshot still succeeds, including with --no-best-effort, so the completed backup omits previously retained data. This prior blocker remains unresolved: preserve successful pages for ordinary provider failures and add a populated-page-then-error regression.
• Resolve merge risk (P1) - Existing Groups backups can omit previously retained membership rows after a later provider failure while still completing successfully; the added tests do not cover that compatibility case.
• Complete next step (P2) - Preserve earlier Cloud Identity membership pages on ordinary API failures, add regression and snapshot compatibility coverage, and complete the backup documentation and Unreleased changelog follow-up.

Findings

• [P1] Preserve earlier membership pages on ordinary API failures — internal/cmd/backup_directory_keep.go:184-186

Agent review details

Security

None.

Review metrics

Metric	Value	Why it matters
Production and test LOC	Production +81/-91 (net -10); tests +298/-0	The guard reuses existing infrastructure and reduces production code while adding focused coverage.
Added pagination cases	5 repeated-token cases; 2 successful-pagination cases	The suite covers the intended guard but lacks the ordinary later-page failure case behind the remaining blocker.

Merge-risk options

Maintainer options:

1. Preserve membership partial results (recommended)
   Handle ordinary provider failures inside the membership callback so earlier pages and the error row survive, without weakening the shared paginator.

Copy recommended automerge instruction

@clawsweeper automerge

Special instructions:
Preserve Cloud Identity membership rows fetched before an ordinary later-page API failure, retaining the group's error record. Keep collectAllPages strict and retain repeated-token rejection. Add populated-page-then-error and snapshot compatibility coverage, document the actual error boundary in docs/backup.md, and add the required Unreleased changelog entry.

Technical review

Best possible solution:

Keep the shared pagination guard while preserving successful membership pages and per-group error records, with compatibility coverage through snapshot assembly.

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

Yes, source establishes the repeated-token loop on main and the PR's separate row-loss trigger: return members on page one, then a provider error on page two. This review did not execute either scenario.

Is this the best way to solve the issue?

No, not as written: reusing the shared guard is appropriate, but its strict error behavior needs adapting to the membership collector's existing partial-result contract.

Full review comments:

• [P1] Preserve earlier membership pages on ordinary API failures — internal/cmd/backup_directory_keep.go:184-186
  If page one returns members and page two fails, returning callErr makes collectAllPages discard page one's members. The caller emits only an error row, whereas main retains the fetched members alongside that error. buildGroupsBackupSnapshot still succeeds, including with --no-best-effort, so the completed backup omits previously retained data. This prior blocker remains unresolved: preserve successful pages for ordinary provider failures and add a populated-page-then-error regression.
  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 backup reliability repair for faulty provider pagination, with no demonstrated current widespread outage.
• merge-risk: 🚨 compatibility: The patch changes an existing Groups backup failure path from retaining fetched members to omitting them.
• 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 captured Windows terminal evidence exercises the actual backup collectors through generated Google HTTP clients and injected repeated-token responses, showing termination after two requests. This is sufficient internal reliability proof; ordinary later-page failure compatibility remains a separate code finding.
• proof: sufficient: Contributor real behavior proof is sufficient. The captured Windows terminal evidence exercises the actual backup collectors through generated Google HTTP clients and injected repeated-token responses, showing termination after two requests. This is sufficient internal reliability proof; ordinary later-page failure compatibility remains a separate code finding.

Evidence

Acceptance criteria:

• [P2] go test ./internal/cmd -run '^(TestFetchBackup(CloudIdentity|Admin|Keep)|TestBuildGroupsBackupSnapshot|TestCollectAllPages)' -count=1 -timeout=90s -v.
• [P2] go test -race ./internal/cmd -run '^(TestFetchBackup(CloudIdentity|Admin|Keep)|TestBuildGroupsBackupSnapshot|TestCollectAllPages)' -count=1 -timeout=120s.
• [P1] make ci.
• [P1] git diff --check.

What I checked:

• Applicable repository policy: Read the complete root AGENTS.md. No nested AGENTS.md or maintainer-notes directory was found. Review-only handling and the Unreleased changelog requirement apply. (AGENTS.md:37, db946f5cbe9e)
• Current main still needs the guard: All five affected collectors still advance directly to NextPageToken without cycle detection. Cloud Identity membership collection appends successful pages before recording a later provider error. (internal/cmd/backup_directory_keep.go:136, 25703c789da5)
• Latest release has the same affected implementation: v0.39.0 and fetched main both reference blob 4103be3ed5b2ce66af22397b55204d68ed475efc for this file; the release therefore retains the same unguarded loops. (internal/cmd/backup_directory_keep.go:136, b058dbbaeb44)
• Introduced membership-retention regression: The introduced callback returns ordinary provider failures to collectAllPages, which discards accumulated results. The caller consequently emits only an error row for that group instead of retaining successfully fetched members. (internal/cmd/backup_directory_keep.go:184, db946f5cbe9e)
• Strict collector contract: collectAllPages returns nil on fetch errors, repeated tokens, or the 10,000-page limit. Its existing tests explicitly require ordinary fetch errors to discard partial results. (internal/cmd/paging.go:30, db946f5cbe9e)
• Omitted members reach a successful snapshot: buildGroupsBackupSnapshot serializes the returned membership rows and returns no error. buildOptionalSnapshot only reacts to returned errors, so --no-best-effort cannot prevent this omission; BackupPushCmd then passes the snapshot to PushSnapshot. (internal/cmd/backup_directory_keep.go:42, db946f5cbe9e)

Likely related people:

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

• Retain successful membership pages plus the error record on later provider failures, with regression and snapshot coverage.
• Document the pagination/error boundary and add the required Unreleased changelog entry when landing.

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-05T06:17:31.599Z sha 519d166 :: blocked before merge. :: [P1] Preserve earlier membership pages on ordinary API failures

[steipete]

Superseded by #1087, which incorporates Groups, Admin, and Keep cycle protection and preserves fetched membership rows on ordinary later-page API failures. Thanks @SebTardif. The separate Cloud Identity query issue #1059 remains open. Superseded by #1087, which incorporates Drive collaboration cycle protection without adding a new page ceiling. Encrypted per-file error records and continued collection were verified. Thanks @SebTardif. Superseded by #1087, which incorporates Calendar, Contacts, and Tasks cycle protection while retaining fetched ACL rows on ordinary API errors. Backup failure and encrypted-row behavior were verified. Thanks @SebTardif.