From 2a0dcb5934e319b145e15e4e60054016b9e78c36 Mon Sep 17 00:00:00 2001 From: xfodev Date: Mon, 20 Jul 2026 13:56:44 +0200 Subject: [PATCH] fix(github): bound fetchLiveReviewThreadBlockers' review-thread GraphQL pagination The reviewThreads(first: 50, after: cursor) loop in fetchLiveReviewThreadBlockers was an unbounded for(;;) that only stopped on no-next-page / no-nodes / a repeated cursor, so a PR with a pathologically large number of review threads could drive an unbounded number of sequential GraphQL calls on every merge-readiness evaluation -- unlike every other paginated list-fetch in src/github/**. Add REVIEW_THREAD_MAX_PAGES=10 (mirroring PR_DETAIL_MAX_PAGES / MAX_WORKFLOW_RUN_LIST_PAGES) and bound the loop; reaching the cap returns the blockers gathered so far (fail-open), never throws. Also documents why the inner comments(first: 20) connection is intentionally not paginated (thread-level resolved/outdated flags gate blocking, and a missed authorizer past #20 only fails open). Adds regression tests for the multi-page walk and the page-cap bound. Closes #7454 --- src/github/backfill.ts | 17 +++++++++++++- test/unit/backfill-2.test.ts | 45 ++++++++++++++++++++++++++++++++++++ 2 files changed, 61 insertions(+), 1 deletion(-) diff --git a/src/github/backfill.ts b/src/github/backfill.ts index ce637c318e..9661084e43 100644 --- a/src/github/backfill.ts +++ b/src/github/backfill.ts @@ -3834,6 +3834,13 @@ type GitHubReviewThreadResponse = { }; }; +// Bound the reviewThreads GraphQL walk so a PR with a pathologically large number of review threads can't turn +// one merge-readiness evaluation into an unbounded sequence of sequential GraphQL calls -- mirrors this file's own +// PR_DETAIL_MAX_PAGES and app.ts's MAX_WORKFLOW_RUN_LIST_PAGES (both bounded to 10). 10 pages * 50 threads/page = +// 500 review threads, far beyond any real PR; hitting the cap returns the blockers derived from the threads +// gathered so far (fail-open, matching this function's own "GraphQL unavailable -> []" posture), never throws. +const REVIEW_THREAD_MAX_PAGES = 10; + /** Fetch unresolved GitHub review threads that should block merge readiness. GraphQL is required because REST * review comments do not expose thread resolution; if GraphQL is unavailable this fails open to [] rather than * guessing. Only maintainer/collaborator comments or known scanner-bot comments can create blockers, so @@ -3851,7 +3858,10 @@ export async function fetchLiveReviewThreadBlockers( const threads: Array = []; let cursor: string | null = null; const seenCursors = new Set(); - for (;;) { + // Bounded outer walk (REVIEW_THREAD_MAX_PAGES): the loop still stops early on no-next-page / no-nodes / a + // repeated cursor, but can never exceed the cap even if GitHub keeps reporting hasNextPage. Reaching the cap + // falls through to processing the threads gathered so far (fail-open), never throws. + for (let page = 0; page < REVIEW_THREAD_MAX_PAGES; page += 1) { const after: string = cursor ? `, after: ${JSON.stringify(cursor)}` : ""; const query: string = `query LoopOverPullRequestReviewThreads { repository(owner: ${JSON.stringify(owner)}, name: ${JSON.stringify(name)}) { @@ -3862,6 +3872,11 @@ export async function fetchLiveReviewThreadBlockers( isOutdated path line + # comments(first: 20) is intentionally NOT paginated (#7454): a thread blocks merge only while it is + # unresolved/non-outdated (a thread-level flag, unaffected by comment count), and the authorizing + # comment is the thread-opening review comment (index 0) or an early reply -- 20 covers that with + # wide margin. Missing an authorizing comment buried past #20 only fails OPEN (no blocker), matching + # this function's own fail-open posture, so full nested pagination isn't worth the extra round-trips. comments(first: 20) { nodes { body diff --git a/test/unit/backfill-2.test.ts b/test/unit/backfill-2.test.ts index 11ea8d20e9..1915c3b2ab 100644 --- a/test/unit/backfill-2.test.ts +++ b/test/unit/backfill-2.test.ts @@ -1467,6 +1467,51 @@ describe("GitHub backfill", () => { ]); }); + it("collects review threads across multiple pages and terminates when hasNextPage is false (#7454)", async () => { + const env = createTestEnv({ GITHUB_PUBLIC_TOKEN: "public-token" }); + const blockerThread = { + isResolved: false, + isOutdated: false, + path: "src/paginated.ts", + line: 5, + comments: { nodes: [{ body: "**P1:** blocker on the third page", url: "https://github.example/p3", author: { login: "superagent[bot]" }, authorAssociation: "NONE" }] }, + }; + const pages = [ + { nodes: [], hasNextPage: true, endCursor: "cursor-1" }, + { nodes: [], hasNextPage: true, endCursor: "cursor-2" }, + { nodes: [blockerThread], hasNextPage: false, endCursor: null }, + ]; + let call = 0; + const fetchMock = vi.fn(async (input: RequestInfo | URL) => { + if (input.toString() !== "https://api.github.com/graphql") return new Response("not found", { status: 404 }); + const page = pages[Math.min(call, pages.length - 1)]!; + call += 1; + return Response.json({ data: { repository: { pullRequest: { reviewThreads: { nodes: page.nodes, pageInfo: { hasNextPage: page.hasNextPage, endCursor: page.endCursor } } } } } }); + }); + vi.stubGlobal("fetch", fetchMock); + + const blockers = await fetchLiveReviewThreadBlockers(env, "JSONbored/loopover", 1, "public-token"); + expect(fetchMock).toHaveBeenCalledTimes(3); // walked all three pages, stopped on hasNextPage:false (well under the cap) + expect(blockers).toEqual([expect.objectContaining({ title: "blocker on the third page", priority: "P1", path: "src/paginated.ts" })]); + }); + + it("is bounded: a pathological always-hasNextPage response stops at REVIEW_THREAD_MAX_PAGES instead of looping unboundedly (#7454)", async () => { + const env = createTestEnv({ GITHUB_PUBLIC_TOKEN: "public-token" }); + let call = 0; + const fetchMock = vi.fn(async (input: RequestInfo | URL) => { + if (input.toString() !== "https://api.github.com/graphql") return new Response("not found", { status: 404 }); + call += 1; + // Always advertise a next page with a DISTINCT cursor (so the seen-cursor guard never trips first) — only + // the page cap can stop this. + return Response.json({ data: { repository: { pullRequest: { reviewThreads: { nodes: [], pageInfo: { hasNextPage: true, endCursor: `cursor-${call}` } } } } } }); + }); + vi.stubGlobal("fetch", fetchMock); + + const blockers = await fetchLiveReviewThreadBlockers(env, "JSONbored/loopover", 2, "public-token"); + expect(fetchMock).toHaveBeenCalledTimes(10); // REVIEW_THREAD_MAX_PAGES — the loop cannot exceed the cap + expect(blockers).toEqual([]); // fail-open: the capped pages yielded no blockers, and it never threw + }); + it("only trusts exact scanner bot logins for scanner-authored review thread blockers", async () => { const env = createTestEnv({ GITHUB_PUBLIC_TOKEN: "public-token" }); vi.stubGlobal("fetch", async (input: RequestInfo | URL) => {