fix(github): reject empty or repeated review-thread cursors

[SebTardif]

When GitHub or a proxy returns hasNextPage: true with a missing or repeated endCursor, gitcrawl sync --with pr-details can refetch review-thread pages indefinitely. Nested comment pages already rejected missing cursors but could also loop on repeated cursors.

This change validates cursors in the GitHub pagination client before following another page. Both loops reject previously followed cursors, and the thread loop also rejects empty cursors. Errors propagate through PR enrichment without saving incomplete review-thread evidence. Normal pagination remains unchanged. The sync guide documents the error and retry behavior.

Thanks @SebTardif for the fix, regression tests, and original reproduction. The original contributor commit is preserved, with a maintainer documentation follow-up.

Validation:

• Focused review-thread HTTP regression tests pass with Go 1.26.7.
• A separately built CLI was exercised through sync synthetic/cursor-proof --numbers 8 --include-comments --with pr-details --json against a local synthetic HTTP API and real temporary SQLite archive. Main exceeded a three-second deadline with thousands of GraphQL requests for empty, repeated, and nested repeated cursors. The fixed CLI exits after one or two requests, reports the cursor error, and preserves previously stored review rows.
• Autoreview at P2: no accepted/actionable findings across the full candidate diff and documentation.

The synthetic fixture proves the production CLI/HTTP/storage path; no malformed response was induced on github.com.

Maintainer closeout for 0916d4f91dd2028632a1e25a9b671f518f308931:

• make check passes using Go 1.26.7 and its matching formatter: tidy, format, vet, govulncheck, deadcode, full suite (85.1% coverage), CLI smoke, release-script tests, and all six snapshot targets.
• Linux/macOS CI, Docker, and secret scanning pass on this head.

[clawsweeper]

🦞👀
ClawSweeper picked this up.

Pull request received. I will update this pull request when review starts.

[clawsweeper]

Codex review: needs maintainer review before merge. Reviewed September 4, 2026, 4:03 AM ET / 08:03 UTC.

ClawSweeper review

What this changes

Gitcrawl rejects empty or repeated review-pagination cursors, adds regression coverage, and documents the resulting sync errors.

Merge readiness

✅ Ready for maintainer review

This remains a useful, well-supported fix: main and v0.9.4 still lack the guards. No blocking correctness or security findings remain.

Priority: P2
Reviewed head: 0916d4f91dd2028632a1e25a9b671f518f308931

Review scores

Measure	Result	What it means
Overall readiness	🦞 diamond lobster (5/6)	A focused repair with production-path before/after evidence, storage-preservation checks, regression coverage, and operator documentation.
Proof confidence	🦞 diamond lobster (5/6)	Sufficient (terminal): The captured CLI run exercises the changed GraphQL client through real HTTP and SQLite with three malformed-cursor scenarios, reporting bounded error exits and preservation of stored reviews after the fix.
Patch quality	🦞 diamond lobster (5/6)	No actionable review findings were identified.

Verification

Check	Result	Evidence
Real behavior	Verified	Sufficient (terminal): The captured CLI run exercises the changed GraphQL client through real HTTP and SQLite with three malformed-cursor scenarios, reporting bounded error exits and preservation of stored reviews after the fix.
Evidence reviewed	10 items	Verified introduced change: The local head matches the pinned PR head. The introduced delta contains four files; production code adds cursor tracking and validation in both review pagination loops. The verified test merge has the pinned main and PR head as its ordered parents and the same tree as the PR head. Main still contains the defect: The outer loop assigns endCursor without checking emptiness or repetition; nested comments reject emptiness but do not detect repetition. Neither the shared HTTP transport nor repository-list limits provide these GraphQL guards. Latest-release comparison: The supplied v0.9.4 release commit and fetched main resolve this file to the identical blob fb55ad83ac119cd57a529911bb66c17bf76750cf, establishing that the latest release also lacks the guards.
Findings	None	None.
Security	None	None.

How this fits together

Gitcrawl fetches GitHub review threads and their comments during pull-request synchronization. Complete responses become local SQLite evidence used by review and search workflows.

flowchart TD
  A[Sync with PR details] --> B[GitHub GraphQL client]
  B --> C[Review threads and comments]
  C --> D{Pagination complete?}
  D -->|Yes| E[Store complete review evidence]
  D -->|No| F{Cursor advances?}
  F -->|Yes| B
  F -->|No| G[Report error and retain prior evidence]

Loading

Before merge

None.

Agent review details

Security

None.

Review metrics

Metric	Value	Why it matters
Production and test delta	Production +15/-1 lines; tests +169 lines	The small production increase implements two cursor guards with four focused regression and compatibility tests.

Technical review

Best possible solution:

Reject non-advancing pagination through the existing sync error path while retaining prior archive evidence and normal multi-page results.

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

Yes. Returning hasNextPage=true with an empty or cycling cursor makes current main repeat GraphQL requests; source establishes the loop, and the supplied CLI run reports reproducing it. This review did not execute the scenario.

Is this the best way to solve the issue?

Yes. Per-fetch cursor tracking repairs the owning loops without changing pagination APIs, stored formats, or valid-response behavior.

AGENTS.md: not found in the target repository.

Codex review notes: model internal, reasoning high; reviewed against 488e87da8d37.

Labels

Label justifications:

• P2: Malformed pagination can stall PR-detail synchronization; the repair is narrow and no widespread active outage is established.
• rating: 🦞 diamond lobster: Overall readiness is 🦞 diamond lobster; proof is 🦞 diamond lobster and patch quality is 🦞 diamond lobster.
• status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Sufficient (terminal): The captured CLI run exercises the changed GraphQL client through real HTTP and SQLite with three malformed-cursor scenarios, reporting bounded error exits and preservation of stored reviews after the fix.
• proof: sufficient: Contributor real behavior proof is sufficient. The captured CLI run exercises the changed GraphQL client through real HTTP and SQLite with three malformed-cursor scenarios, reporting bounded error exits and preservation of stored reviews after the fix.

Evidence

What I checked:

• Verified introduced change: The local head matches the pinned PR head. The introduced delta contains four files; production code adds cursor tracking and validation in both review pagination loops. The verified test merge has the pinned main and PR head as its ordered parents and the same tree as the PR head. (internal/github/review_threads.go:125, 0916d4f91dd2)
• Main still contains the defect: The outer loop assigns endCursor without checking emptiness or repetition; nested comments reject emptiness but do not detect repetition. Neither the shared HTTP transport nor repository-list limits provide these GraphQL guards. (internal/github/review_threads.go:151, 488e87da8d37)
• Latest-release comparison: The supplied v0.9.4 release commit and fetched main resolve this file to the identical blob fb55ad83ac119cd57a529911bb66c17bf76750cf, establishing that the latest release also lacks the guards. (internal/github/review_threads.go:151, 274881750869)
• Errors precede review persistence: Review-fetch errors are recorded and returned before the payload reaches review-row persistence. Failure recording updates repository/thread metadata and the failure ledger, without replacing review rows or marking review hydration complete. (internal/syncer/syncer.go:193, 0916d4f91dd2)
• Regression and compatibility coverage: Four added HTTP tests cover empty outer cursors, repeated outer cursors, repeated nested-comment cursors, and successful outer pagination. Existing tests cover successful nested pagination and failed hydration followed by recovery. Tests were inspected, not executed during this review. (internal/github/client_test.go:442, 0916d4f91dd2)
• Captured production-path proof: The supplied PR body reports a separately built CLI running sync against a synthetic HTTP API and real temporary SQLite archive: main exceeded three seconds with thousands of GraphQL requests; the fixed executable returned cursor errors after one or two requests and preserved existing review rows. This exercises the changed client through the production CLI and storage boundary. Captured context sourceRevision: d46dd7250741822b07f3ea3562afba7c621ebd4955ec1b8bd22837c2102ce16a. No github.com fault injection is claimed. (internal/github/review_threads.go:122, 0916d4f91dd2)

Likely related people:

• steipete: Suggested for follow-up; no historical authorship or introduction is verified. (role: unverified routing candidate; confidence: low)
• Andy Ye: 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 (8 earlier review cycles)

• reviewed 2026-09-02T14:45:28.844Z sha 3ec56df :: needs maintainer review before merge. :: none
• reviewed 2026-09-02T21:58:56.517Z sha 3ec56df :: needs maintainer review before merge. :: none
• reviewed 2026-09-02T23:35:15.861Z sha 3ec56df :: needs maintainer review before merge. :: none
• reviewed 2026-09-03T03:59:16.718Z sha 3ec56df :: needs maintainer review before merge. :: none
• reviewed 2026-09-03T08:54:09.049Z sha 3ec56df :: needs maintainer review before merge. :: none
• reviewed 2026-09-03T15:54:56.042Z sha 3ec56df :: needs maintainer review before merge. :: none
• reviewed 2026-09-03T20:44:07.781Z sha 3ec56df :: needs maintainer review before merge. :: none
• reviewed 2026-09-04T07:56:48.966Z sha 0916d4f :: needs maintainer review before merge. :: none