Skip to content

fix(paging): reject token cycles across CLI scans and backups - #1087

Merged
steipete merged 2 commits into
openclaw:mainfrom
SebTardif:fix/docs-comments-page-token
Sep 5, 2026
Merged

fix(paging): reject token cycles across CLI scans and backups#1087
steipete merged 2 commits into
openclaw:mainfrom
SebTardif:fix/docs-comments-page-token

Conversation

@SebTardif

@SebTardif SebTardif commented Sep 5, 2026

Copy link
Copy Markdown
Contributor

Repeated Google page tokens could leave backups, Drive reports, contact dedupe, Gmail bulk queries, recurring Calendar lookups, and open-comment scans running indefinitely. This consolidates #1078#1086 into this existing contributor PR and gives those paths one shared cycle guard.

The repair preserves each caller's behavior: existing unlimited scans stay unlimited, bounded scans and occurrence lookup stop as soon as satisfied, and ordinary later-page ACL/membership errors retain previously fetched rows. Cycles discard the affected incomplete collection and follow the existing command or backup error path. The existing bounded all-pages collector keeps its 10,000-page limit.

Validation includes generated-client regressions, the local CI gate, and 33 real built-CLI runs against a local HTTPS fault-injection peer. Those runs cover repeated-token errors, zero downstream writes after failed scans, early-result success, unchanged existing backup manifests/Git HEAD after fatal collection errors, and decryption of embedded error rows and retained ACL/membership records. All inputs, credentials, and resources in the fault fixture are synthetic; this does not claim that a healthy Google endpoint was forced to emit a bad cursor.

Credited Unreleased entries and command/backup documentation are included. The nine sibling PRs remain open for maintainer closeout after this consolidated repair lands. This does not resolve #1059's Cloud Identity query validation or change Gmail-web MIME behavior or download defaults.

Co-authored contribution: Sebastien Tardif (@SebTardif); preserved in the commit credit.

Captured examples from the stock-binary run (synthetic HTTPS peer):

{"case": "docs", "exit": 1, "target_requests": 2, "mutations": 0, "result": "cycle rejected; empty stdout"}
{"case": "dedupe-limit", "exit": 0, "target_requests": 1, "mutations": 0, "result": "stopped at existing result/limit"}
{"case": "calendar-match", "exit": 0, "target_requests": 1, "mutations": 1, "result": "stopped at existing result/limit"}
{"case": "drive-files", "exit": 1, "target_requests": 2, "mutations": 0, "result": "aborted; existing snapshot and Git HEAD unchanged"}
{"case": "acl-ordinary", "exit": 0, "target_requests": 2, "mutations": 0, "result": "verified encrypted error row; earlier row retained"}
{"case": "members-ordinary", "exit": 0, "target_requests": 2, "mutations": 0, "result": "verified encrypted error row; earlier row retained"}
PASS 33 stock CLI cases

Full request-count and backup-result trace.

Default gog docs comments list walks resolved-only pages by assigning
pageToken = nextPageToken with no seen-set. A repeated Google token
hangs until the process is killed.

Track seen tokens and return pagination loop: repeated page token,
matching collectAllPages. --all already used that helper; --locate
already had its own seen map.

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

clawsweeper Bot commented Sep 5, 2026

Copy link
Copy Markdown
Contributor

🦞👀
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. 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. labels Sep 5, 2026
@clawsweeper

clawsweeper Bot commented Sep 5, 2026

Copy link
Copy Markdown
Contributor

Codex review: needs maintainer review before merge. Reviewed September 5, 2026, 5:34 PM ET / 21:34 UTC.

ClawSweeper review

What this changes

Adds shared pagination-cycle detection across Google service scans and backups while preserving existing limits, early results, and ordinary API-error handling.

Merge readiness

Ready for maintainer review

Keep open: current main still needs this repair. The updated CLI traces resolve the prior proof blocker, the changelog finding is fixed, and no actionable correctness findings remain.

Priority: P2
Reviewed head: 12a6bdaef0a6413efe4b4207b4de7a8e0ca75f9a

Review scores

Measure Result What it means
Overall readiness 🐚 platinum hermit (4/6) A sound consolidated repair with strong production-path proof, substantial regression coverage, and no remaining actionable findings.
Proof confidence 🦞 diamond lobster (5/6) Sufficient (terminal): The exact-head stock CLI exercised the changed scan and backup owners through real HTTPS transport against an injected-fault peer; captured results show cycle rejection, preserved early success, prevented dependent actions, and preserved existing backup state, resolving the prior proof request.
Patch quality 🐚 platinum hermit (4/6) No actionable review findings were identified.

Verification

Check Result Evidence
Real behavior Verified Sufficient (terminal): The exact-head stock CLI exercised the changed scan and backup owners through real HTTPS transport against an injected-fault peer; captured results show cycle rejection, preserved early success, prevented dependent actions, and preserved existing backup state, resolving the prior proof request.
Evidence reviewed 9 items Applicable repository policy: Read the complete root AGENTS.md; no nested AGENTS.md or maintainer-note files were found. Applied read-only review, parseable-output, and credited Unreleased changelog guidance.
Verified introduced scope: Inspected the full local production delta and accompanying tests/docs between the pinned main and head. The supplied verified test merge has those exact main/head parents. Changes are confined to pagination, regression coverage, documentation, and release notes.
Still necessary on main and latest release: Pinned main still advances Gmail query tokens without a seen set; empty repeated pages leave the remaining count unchanged. The inspected v0.39.0 source also retains the unguarded default open-comment scan. Existing guarded all-pages paths do not repair these separate callers.
Findings None None.
Security None None.

How this fits together

gog’s scan helpers collect paginated Google API results for reports, message and contact operations, and encrypted backups. They must finish successfully before callers publish complete results or perform dependent actions.

flowchart TD
  A[CLI command] --> B[Google API page request]
  B --> C[Rows and continuation token]
  C --> D{Continue scanning?}
  D -->|Finished or limit reached| E[Report, action, or backup]
  D -->|More pages| F{Token already used?}
  F -->|No| B
  F -->|Yes| G[Existing error handling]
Loading

Before merge

None.

Agent review details

Security

None.

Review metrics

Metric Value Why it matters
Production versus test delta Production +204/−140 (net +64); tests +1,847/−0 Production growth supports shared cycle detection across existing scans, with most added lines devoted to regression coverage.
Recorded CLI proof 33 stock-binary fault-injection cases reported The supplied traces exercise termination, early success, and final backup or command effects through production entrypoints.

Root-cause cluster

Relationship: canonical
Canonical: #1087
Summary: Maintainer discussion selects this PR as the consolidated pagination repair; the supplied sibling PRs cover subsets, while earlier merged guards and query validation affect distinct paths.

Members:

Proposal only: this assessment does not dispatch repair, suppress jobs, mutate sibling items, close, or merge anything.

Technical review

Best possible solution:

Use the shared cycle guard while retaining each caller’s established completion, capacity, and error contracts.

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

Yes: current source exposes unguarded repeated-token loops, and the supplied main-control trace demonstrates repeated Docs requests. This read-only review did not execute the CLI.

Is this the best way to solve the issue?

Yes: sharing cycle detection while preserving bounded and unbounded scan contracts is a narrow repair, supported by caller-level regressions and stock-CLI fault traces.

AGENTS.md: found and applied where relevant.

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

Labels

Label justifications:

  • P2: Repairs bounded reliability defects triggered by repeated provider pagination tokens, without evidence of an urgent widespread outage.
  • 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 stock CLI exercised the changed scan and backup owners through real HTTPS transport against an injected-fault peer; captured results show cycle rejection, preserved early success, prevented dependent actions, and preserved existing backup state, resolving the prior proof request.
  • proof: sufficient: Contributor real behavior proof is sufficient. The exact-head stock CLI exercised the changed scan and backup owners through real HTTPS transport against an injected-fault peer; captured results show cycle rejection, preserved early success, prevented dependent actions, and preserved existing backup state, resolving the prior proof request.

Evidence

What I checked:

  • Applicable repository policy: Read the complete root AGENTS.md; no nested AGENTS.md or maintainer-note files were found. Applied read-only review, parseable-output, and credited Unreleased changelog guidance. (AGENTS.md:1, 12a6bdaef0a6)
  • Verified introduced scope: Inspected the full local production delta and accompanying tests/docs between the pinned main and head. The supplied verified test merge has those exact main/head parents. Changes are confined to pagination, regression coverage, documentation, and release notes. (12a6bdaef0a6)
  • Still necessary on main and latest release: Pinned main still advances Gmail query tokens without a seen set; empty repeated pages leave the remaining count unchanged. The inspected v0.39.0 source also retains the unguarded default open-comment scan. Existing guarded all-pages paths do not repair these separate callers. (internal/cmd/gmail_archive.go:255, 25703c789da5)
  • Capacity and early-return contracts: The existing collector retains its 10,000-page ceiling; the new unbounded collector shares cycle detection without that ceiling. Tests cover 10,001 distinct pages, multi-token cycles, early recurring-event matches, and contact limits. (internal/cmd/paging.go:19, 12a6bdaef0a6)
  • Backup error boundaries: Calendar ACL and Cloud Identity membership loops discard only the affected resource’s accumulated rows on cycles while retaining earlier rows on ordinary API errors. Fatal collection errors propagate before PushSnapshot; optional services retain their existing best-effort handling. (internal/cmd/backup_services.go:206, 12a6bdaef0a6)
  • Updated production-path proof: The captured body and fix(paging): reject token cycles across CLI scans and backups #1087 (comment) identify the exact head and an unmodified cmd/gog binary using real transport through a synthetic HTTPS peer. Supplied output records the main Docs loop, candidate rejection after two requests, early-result success, prevented downstream actions, unchanged existing manifests/Git HEAD, and retained encrypted ACL/membership rows. The comment reports 33 passing cases. Its final 1,444 characters were omitted from the supplied excerpt and could not be fetched; the available body and trace already establish the relevant outcomes. No embedded commands were executed. (12a6bdaef0a6)

Likely related people:

  • steipete: Suggested for follow-up; no historical authorship or introduction is verified. (role: unverified routing candidate; confidence: low)
  • salmonumbrella: Raw commit 3371e3f adds internal/cmd/paging.go:19 relative to its recorded parents. This identifies author metadata, not feature responsibility or a PR merger. (role: source-line author; confidence: high; commits: 3371e3f3ad6d; files: internal/cmd/paging.go)
  • SebTardif: 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 (2 earlier review cycles)
  • reviewed 2026-09-05T08:16:46.707Z sha 9015448 :: needs changes before merge. :: [P3] [P3] Record the hang fix in Unreleased
  • reviewed 2026-09-05T21:01:37.184Z sha 12a6bda :: needs real behavior proof before merge. :: none

Consolidate the repeated-token fixes from openclaw#1078 through openclaw#1087 using a shared cycle guard. Preserve unlimited distinct-page scans, early matches, and previously fetched ACL/member rows on ordinary API errors.

Add generated-client regression coverage, stock-binary HTTPS fault proof, and credited release notes.

Co-authored-by: Sebastien Tardif <SebTardif@ncf.ca>
@steipete steipete changed the title fix(docs): reject repeated comment scan page tokens fix(paging): reject token cycles across CLI scans and backups Sep 5, 2026
@steipete

steipete commented Sep 5, 2026

Copy link
Copy Markdown
Collaborator

Maintainer proof for 12a6bdaef0a6413efe4b4207b4de7a8e0ca75f9a:

  • make ci passed: format, lint, deadcode, full Go/Node tests, docs coverage, and generated skill checks.
  • autoreview --mode branch --base origin/main --max-priority P2 is scoped-clean.
  • Built the normal cmd/gog binary and ran 33 CLI cases against a local HTTPS peer using synthetic credentials and an isolated home. No production code or transport was replaced. The main binary reproduced the Docs loop; the candidate rejects the repeated token after two requests.
  • Docs, Drive tree/inventory/du, Contacts dedupe, Gmail archive, and Calendar occurrence lookup fail without success output or downstream writes. Open-comment discovery, dedupe limits, and a matching occurrence still return early.
  • Drive, Calendar, Contacts, Tasks, Groups, Admin, Keep, and Workspace backup faults preserve an existing manifest and Git HEAD when collection must abort. Collaboration/Form errors remain verifiable encrypted error records. Decrypted ACL and membership shards retain earlier rows on an ordinary later API error; cycles produce the affected resource's error record instead.
  • The collector regression also accepts 10,001 distinct pages; the pre-existing bounded collector retains its own limit.

The nine sibling pagination PRs are incorporated here with @SebTardif credited. They should be closed as superseded only after this PR lands. No merge or issue closure was performed in this preparation session.

Exact-head CI passed on Linux, Windows, and macOS, plus the tracking worker: https://github.com/openclaw/gogcli/actions/runs/33991525608

Captured stock-binary fault-injection output

The fixture launches the unmodified CLI with an isolated home, a synthetic direct token, and a local HTTPS proxy/CA. It records requests and downstream writes; backup cases initialize a disposable encrypted repository, seed it, inject the fault, then verify the manifest/Git HEAD or decrypt the affected shard.

Build and run entry points:

go build -o "$TMPDIR/gog-paging" ./cmd/gog
python3 paging-cli-proof.py "$TMPDIR/gog-paging" docs docs-open tree inventory du dedupe dedupe-limit gmail calendar calendar-match drive-drives drive-files calendar-list calendar-events calendar-settings contacts other-contacts contact-groups task-lists tasks groups admin-users admin-groups keep workspace forms permissions comments revisions acl acl-ordinary members members-ordinary

Main control (same synthetic peer, bounded by a fixture safety cap):

{
  "case": "docs",
  "expect": "old",
  "exit": 1,
  "requests": [
    {
      "method": "GET",
      "path": "/drive/v3/files/fixture-document/comments",
      "page": ""
    },
    {
      "method": "GET",
      "path": "/drive/v3/files/fixture-document/comments",
      "page": "stuck"
    },
    {
      "method": "GET",
      "path": "/drive/v3/files/fixture-document/comments",
      "page": "stuck"
    },
    {
      "method": "GET",
      "path": "/drive/v3/files/fixture-document/comments",
      "page": "stuck"
    }
  ],
  "writes": [],
  "stdout": "",
  "stderr": "Note: Using direct access token (expires in ~1 hour; no auto-refresh)\nGoogle API error (400): fixture stopped unguarded paging\n"
}

Candidate output:

{"case": "docs", "exit": 1, "target_requests": 2, "mutations": 0, "result": "cycle rejected; empty stdout"}
{"case": "docs-open", "exit": 0, "target_requests": 2, "mutations": 0, "result": "stopped at existing result/limit"}
{"case": "tree", "exit": 1, "target_requests": 2, "mutations": 0, "result": "cycle rejected; empty stdout"}
{"case": "inventory", "exit": 1, "target_requests": 2, "mutations": 0, "result": "cycle rejected; empty stdout"}
{"case": "du", "exit": 1, "target_requests": 2, "mutations": 0, "result": "cycle rejected; empty stdout"}
{"case": "dedupe", "exit": 1, "target_requests": 2, "mutations": 0, "result": "cycle rejected; empty stdout"}
{"case": "dedupe-limit", "exit": 0, "target_requests": 1, "mutations": 0, "result": "stopped at existing result/limit"}
{"case": "gmail", "exit": 1, "target_requests": 2, "mutations": 0, "result": "cycle rejected; empty stdout"}
{"case": "calendar", "exit": 1, "target_requests": 2, "mutations": 0, "result": "cycle rejected; empty stdout"}
{"case": "calendar-match", "exit": 0, "target_requests": 1, "mutations": 1, "result": "stopped at existing result/limit"}
{"case": "drive-drives", "exit": 1, "target_requests": 2, "mutations": 0, "result": "aborted; existing snapshot and Git HEAD unchanged"}
{"case": "drive-files", "exit": 1, "target_requests": 2, "mutations": 0, "result": "aborted; existing snapshot and Git HEAD unchanged"}
{"case": "calendar-list", "exit": 1, "target_requests": 2, "mutations": 0, "result": "aborted; existing snapshot and Git HEAD unchanged"}
{"case": "calendar-events", "exit": 1, "target_requests": 2, "mutations": 0, "result": "aborted; existing snapshot and Git HEAD unchanged"}
{"case": "calendar-settings", "exit": 1, "target_requests": 2, "mutations": 0, "result": "aborted; existing snapshot and Git HEAD unchanged"}
{"case": "contacts", "exit": 1, "target_requests": 2, "mutations": 0, "result": "aborted; existing snapshot and Git HEAD unchanged"}
{"case": "other-contacts", "exit": 1, "target_requests": 2, "mutations": 0, "result": "aborted; existing snapshot and Git HEAD unchanged"}
{"case": "contact-groups", "exit": 1, "target_requests": 2, "mutations": 0, "result": "aborted; existing snapshot and Git HEAD unchanged"}
{"case": "task-lists", "exit": 1, "target_requests": 2, "mutations": 0, "result": "aborted; existing snapshot and Git HEAD unchanged"}
{"case": "tasks", "exit": 1, "target_requests": 2, "mutations": 0, "result": "aborted; existing snapshot and Git HEAD unchanged"}
{"case": "groups", "exit": 1, "target_requests": 2, "mutations": 0, "result": "aborted; existing snapshot and Git HEAD unchanged"}
{"case": "admin-users", "exit": 1, "target_requests": 2, "mutations": 0, "result": "aborted; existing snapshot and Git HEAD unchanged"}
{"case": "admin-groups", "exit": 1, "target_requests": 2, "mutations": 0, "result": "aborted; existing snapshot and Git HEAD unchanged"}
{"case": "keep", "exit": 1, "target_requests": 2, "mutations": 0, "result": "aborted; existing snapshot and Git HEAD unchanged"}
{"case": "workspace", "exit": 1, "target_requests": 2, "mutations": 0, "result": "aborted; existing snapshot and Git HEAD unchanged"}
{"case": "forms", "exit": 0, "target_requests": 2, "mutations": 0, "result": "verified encrypted error row"}
{"case": "permissions", "exit": 0, "target_requests": 2, "mutations": 0, "result": "verified encrypted error row"}
{"case": "comments", "exit": 0, "target_requests": 2, "mutations": 0, "result": "verified encrypted error row"}
{"case": "revisions", "exit": 0, "target_requests": 2, "mutations": 0, "result": "verified encrypted error row"}
{"case": "acl", "exit": 0, "target_requests": 2, "mutations": 0, "result": "verified encrypted error row"}
{"case": "acl-ordinary", "exit": 0, "target_requests": 2, "mutations": 0, "result": "verified encrypted error row; earlier row retained"}
{"case": "members", "exit": 0, "target_requests": 2, "mutations": 0, "result": "verified encrypted error row"}
{"case": "members-ordinary", "exit": 0, "target_requests": 2, "mutations": 0, "result": "verified encrypted error row; earlier row retained"}
PASS 33 stock CLI cases

@clawsweeper clawsweeper Bot added rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. 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 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. rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. labels Sep 5, 2026
@steipete
steipete merged commit 91b66c9 into openclaw:main Sep 5, 2026
11 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.

groups list 400 INVALID_ARGUMENT: searchTransitiveGroups rejects (A || B) OR label filter

2 participants