From 3ec56df101d894b13c19d06afacfc15e025cfb05 Mon Sep 17 00:00:00 2001 From: Sebastien Tardif Date: Wed, 2 Sep 2026 07:42:17 -0700 Subject: [PATCH 1/2] fix(github): reject empty or repeated review-thread cursors ListPullReviewThreads followed hasNextPage and assigned the next GraphQL cursor with no empty or repeat check. A GitHub or proxy page that kept hasNextPage true with a blank or stuck endCursor refetched forever during gitcrawl sync PR enrichment. Reject a missing endCursor when another page is claimed, and remember cursors already followed for both thread pages and nested comment pages. Sibling crawlers already fail closed on a non-advancing cursor. Signed-off-by: Sebastien Tardif --- CHANGELOG.md | 2 + internal/github/client_test.go | 169 ++++++++++++++++++++++++++++++ internal/github/review_threads.go | 16 ++- 3 files changed, 186 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 24ed6055..83458cd7 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,8 @@ ## 0.9.5 - Unreleased +- Stop review-thread GraphQL pagination when GitHub returns an empty or repeated endCursor. Thanks @SebTardif. + ## 0.9.4 - 2026-08-30 - Report the actual reset failure when portable-store initialization cannot recover from a dirty merge. Thanks @SebTardif. diff --git a/internal/github/client_test.go b/internal/github/client_test.go index fead75f3..ceb51a2b 100644 --- a/internal/github/client_test.go +++ b/internal/github/client_test.go @@ -439,6 +439,175 @@ func TestListPullReviewThreadsPaginatesReviewThreadComments(t *testing.T) { } } +func TestListPullReviewThreadsRejectsEmptyEndCursor(t *testing.T) { + var calls atomic.Int32 + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if calls.Add(1) > 8 { + http.Error(w, "stuck pagination", http.StatusInternalServerError) + return + } + _ = json.NewEncoder(w).Encode(map[string]any{"data": map[string]any{"repository": map[string]any{"pullRequest": map[string]any{ + "reviewThreads": map[string]any{ + "nodes": []map[string]any{{"id": "PRRT_1"}}, + "pageInfo": map[string]any{"hasNextPage": true, "endCursor": ""}, + }, + }}}}) + })) + defer server.Close() + + client := New(Options{BaseURL: server.URL, PageDelay: -1}) + ctx, cancel := context.WithTimeout(context.Background(), 2*time.Second) + defer cancel() + _, err := client.ListPullReviewThreads(ctx, "openclaw", "gitcrawl", 8, nil) + if err == nil { + t.Fatal("expected empty endCursor error") + } + if !strings.Contains(err.Error(), "missing endCursor") { + t.Fatalf("error = %v", err) + } + if got := calls.Load(); got != 1 { + t.Fatalf("calls = %d, want 1", got) + } +} + +func TestListPullReviewThreadsRejectsRepeatedEndCursor(t *testing.T) { + var calls atomic.Int32 + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if calls.Add(1) > 8 { + http.Error(w, "stuck pagination", http.StatusInternalServerError) + return + } + _ = json.NewEncoder(w).Encode(map[string]any{"data": map[string]any{"repository": map[string]any{"pullRequest": map[string]any{ + "reviewThreads": map[string]any{ + "nodes": []map[string]any{{"id": "PRRT_1"}}, + "pageInfo": map[string]any{"hasNextPage": true, "endCursor": "thread-cursor-1"}, + }, + }}}}) + })) + defer server.Close() + + client := New(Options{BaseURL: server.URL, PageDelay: -1}) + ctx, cancel := context.WithTimeout(context.Background(), 2*time.Second) + defer cancel() + _, err := client.ListPullReviewThreads(ctx, "openclaw", "gitcrawl", 8, nil) + if err == nil { + t.Fatal("expected repeated endCursor error") + } + if !strings.Contains(err.Error(), "repeated endCursor") { + t.Fatalf("error = %v", err) + } + if got := calls.Load(); got != 2 { + t.Fatalf("calls = %d, want 2", got) + } +} + +func TestListPullReviewThreadsPaginatesReviewThreads(t *testing.T) { + var calls int + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + calls++ + var body graphqlEnvelope + if err := json.NewDecoder(r.Body).Decode(&body); err != nil { + t.Fatalf("decode request: %v", err) + } + switch calls { + case 1: + if body.Variables["cursor"] != nil { + t.Fatalf("first request should omit cursor, variables=%+v", body.Variables) + } + _ = json.NewEncoder(w).Encode(map[string]any{"data": map[string]any{"repository": map[string]any{"pullRequest": map[string]any{ + "reviewThreads": map[string]any{ + "nodes": []map[string]any{{"id": "PRRT_1"}}, + "pageInfo": map[string]any{"hasNextPage": true, "endCursor": "thread-cursor-1"}, + }, + }}}}) + case 2: + if body.Variables["cursor"] != "thread-cursor-1" { + t.Fatalf("second request cursor = %+v", body.Variables) + } + _ = json.NewEncoder(w).Encode(map[string]any{"data": map[string]any{"repository": map[string]any{"pullRequest": map[string]any{ + "reviewThreads": map[string]any{ + "nodes": []map[string]any{{"id": "PRRT_2"}}, + "pageInfo": map[string]any{"hasNextPage": false, "endCursor": "thread-cursor-2"}, + }, + }}}}) + default: + t.Fatalf("unexpected graphql call %d", calls) + } + })) + defer server.Close() + + client := New(Options{BaseURL: server.URL, PageDelay: -1}) + rows, err := client.ListPullReviewThreads(context.Background(), "openclaw", "gitcrawl", 8, nil) + if err != nil { + t.Fatalf("list review threads: %v", err) + } + if calls != 2 { + t.Fatalf("calls = %d", calls) + } + if len(rows) != 2 || rows[0]["id"] != "PRRT_1" || rows[1]["id"] != "PRRT_2" { + t.Fatalf("rows = %#v", rows) + } +} + +func TestListPullReviewThreadsRejectsRepeatedCommentEndCursor(t *testing.T) { + var calls atomic.Int32 + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + call := calls.Add(1) + if call > 8 { + http.Error(w, "stuck pagination", http.StatusInternalServerError) + return + } + var body graphqlEnvelope + if err := json.NewDecoder(r.Body).Decode(&body); err != nil { + t.Fatalf("decode request: %v", err) + } + if call == 1 { + if body.Variables["threadID"] != nil { + t.Fatalf("first request should fetch review threads, variables=%+v", body.Variables) + } + _ = json.NewEncoder(w).Encode(map[string]any{"data": map[string]any{"repository": map[string]any{"pullRequest": map[string]any{ + "reviewThreads": map[string]any{ + "nodes": []map[string]any{{ + "id": "PRRT_1", + "comments": map[string]any{ + "nodes": []map[string]any{{"id": "PRRC_1"}}, + "pageInfo": map[string]any{"hasNextPage": true, "endCursor": "comment-cursor-1"}, + }, + }}, + "pageInfo": map[string]any{"hasNextPage": false, "endCursor": ""}, + }, + }}}}) + return + } + if body.Variables["threadID"] != "PRRT_1" || body.Variables["cursor"] != "comment-cursor-1" { + t.Fatalf("comment page variables = %+v", body.Variables) + } + _ = json.NewEncoder(w).Encode(map[string]any{"data": map[string]any{ + "node": map[string]any{ + "comments": map[string]any{ + "nodes": []map[string]any{{"id": "PRRC_2"}}, + "pageInfo": map[string]any{"hasNextPage": true, "endCursor": "comment-cursor-1"}, + }, + }, + }}) + })) + defer server.Close() + + client := New(Options{BaseURL: server.URL, PageDelay: -1}) + ctx, cancel := context.WithTimeout(context.Background(), 2*time.Second) + defer cancel() + _, err := client.ListPullReviewThreads(ctx, "openclaw", "gitcrawl", 8, nil) + if err == nil { + t.Fatal("expected repeated comment endCursor error") + } + if !strings.Contains(err.Error(), "repeated endCursor") { + t.Fatalf("error = %v", err) + } + if got := calls.Load(); got != 2 { + t.Fatalf("calls = %d, want 2", got) + } +} + func TestNextPageAndReporterBranches(t *testing.T) { header := `; rel="next", ; rel="last"` if got := nextPage(header, "https://api.github.test"); got != "/repos/o/r/issues?page=2&state=open" { diff --git a/internal/github/review_threads.go b/internal/github/review_threads.go index fb55ad83..cfb05ef3 100644 --- a/internal/github/review_threads.go +++ b/internal/github/review_threads.go @@ -122,6 +122,7 @@ type graphqlResponseEnvelope struct { func (c *Client) ListPullReviewThreads(ctx context.Context, owner, repo string, number int, reporter Reporter) ([]map[string]any, error) { var out []map[string]any var cursor string + seen := make(map[string]struct{}) for { vars := map[string]any{ "owner": owner, @@ -148,7 +149,15 @@ func (c *Client) ListPullReviewThreads(ctx context.Context, owner, repo string, if !page.PageInfo.HasNextPage { break } - cursor = page.PageInfo.EndCursor + next := page.PageInfo.EndCursor + if next == "" { + return nil, fmt.Errorf("review threads page missing endCursor") + } + if _, ok := seen[next]; ok { + return nil, fmt.Errorf("review threads page repeated endCursor %q", next) + } + seen[next] = struct{}{} + cursor = next } return out, nil } @@ -162,11 +171,16 @@ func (c *Client) completeReviewThreadComments(ctx context.Context, thread map[st if !comments.PageInfo.HasNextPage { return nil } + seen := make(map[string]struct{}) for comments.PageInfo.HasNextPage { cursor := comments.PageInfo.EndCursor if cursor == "" { return fmt.Errorf("review thread %s comments page missing endCursor", threadID) } + if _, ok := seen[cursor]; ok { + return fmt.Errorf("review thread %s comments page repeated endCursor %q", threadID, cursor) + } + seen[cursor] = struct{}{} vars := map[string]any{"threadID": threadID, "cursor": cursor} var resp pullReviewThreadCommentsResponse if err := c.doGraphQL(ctx, pullReviewThreadCommentsQuery, vars, reporter, &resp); err != nil { From 0916d4f91dd2028632a1e25a9b671f518f308931 Mon Sep 17 00:00:00 2001 From: Peter Steinberger Date: Fri, 4 Sep 2026 00:50:57 -0700 Subject: [PATCH 2/2] docs: explain review pagination failures --- docs/sync.md | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/docs/sync.md b/docs/sync.md index ed775779..2442068e 100644 --- a/docs/sync.md +++ b/docs/sync.md @@ -86,6 +86,12 @@ issue or pull request URLs. PR details land in `pr_files`, `pr_commits`, `pr_checks`, and `pr_runs` tables for local review, search, clustering, and TUI workflows. +Review-thread and nested-comment pagination fails if GitHub claims another page +but returns an empty or previously followed `endCursor`. Sync reports a +`missing endCursor` or `repeated endCursor` error instead of repeatedly fetching +the same page. Retry after the GitHub API or proxy returns advancing cursors; +the incomplete review-thread response is not saved as complete evidence. + Use `gitcrawl coverage [owner/repo] --json` to inspect archive completeness after a sync. It reports issue, PR, comment, and review counts alongside hydrated PR detail rows, missing PR details, known failed hydrations, and detail-table row counts per repository. The additive `enrichment` object exposes supported, eligible, covered, fresh, missing, stale, completeness, ratios, and latest timestamps for revisions, fingerprints, key summaries, clusters, and PR details. Use `--repos owner/a,owner/b` to compare selected repositories and `--min-missing-pr-details N` to focus backfill work on repositories with gaps. `gitcrawl sync-failures owner/repo --json` lists unresolved PR hydration failures with their operation, error class and message, timestamps, and retry count. Add `--include-resolved` to inspect failures cleared by a later successful hydration. This operational ledger stays local when `portable prune` runs unless the publisher explicitly passes `--include-sync-failures`, which retains the ledger only after replacing every error message with a redaction marker.