From f619c552e2ea852e8788c98298db3374f03feb97 Mon Sep 17 00:00:00 2001 From: galuis116 Date: Mon, 20 Jul 2026 08:53:25 -0400 Subject: [PATCH] fix(github): paginate GraphQL PR-detail fallback past 100 nodes fetchPullRequestDetailsFromGraphQl's files/reviews connections requested only first: 100 with no pageInfo, so a PR with more than 100 changed files or more than 100 reviews silently truncated when the REST detail fetch fell back to GraphQL -- with no way to even detect it. Add pageInfo { hasNextPage endCursor } to both connections and walk each independently with cursor pagination, bounded by a new PR_DETAIL_GRAPHQL_MAX_PAGES constant mirroring the REST path's PR_DETAIL_MAX_PAGES. A later-page failure keeps the items already fetched instead of discarding them, matching githubPaginatedList's partial-result semantics; a page-1 failure still surfaces the same way to callers as before. Closes #7453 --- src/github/backfill.ts | 127 ++++++++++++++++++++++++++----------- test/unit/backfill.test.ts | 105 ++++++++++++++++++++++++++++++ 2 files changed, 196 insertions(+), 36 deletions(-) diff --git a/src/github/backfill.ts b/src/github/backfill.ts index 9661084e43..a6dfb1d4a6 100644 --- a/src/github/backfill.ts +++ b/src/github/backfill.ts @@ -257,11 +257,14 @@ type GitHubOpenPullRequestsResponse = { errors?: Array<{ message?: string }>; }; +type GitHubGraphQlPageInfo = { hasNextPage?: boolean | null; endCursor?: string | null } | null; + type GitHubPullRequestDetailsResponse = { data?: { repository?: { pullRequest?: { files?: { + pageInfo?: GitHubGraphQlPageInfo; nodes?: Array<{ path?: string | null; additions?: number | null; @@ -270,6 +273,7 @@ type GitHubPullRequestDetailsResponse = { } | null> | null; } | null; reviews?: { + pageInfo?: GitHubGraphQlPageInfo; nodes?: Array<{ databaseId?: number | null; author?: { login?: string | null } | null; @@ -4137,6 +4141,12 @@ export async function fetchLinkedIssueFacts( }; } +// GraphQL's `files`/`reviews` connections default to first: 100 with no follow-up, so a PR with more than +// 100 changed files or more than 100 reviews silently truncates here too -- mirror PR_DETAIL_MAX_PAGES's +// REST bound with the same 10-page-of-100 (1,000 item) ceiling, walked independently per connection via +// `pageInfo { hasNextPage endCursor }` instead of a Link header. +const PR_DETAIL_GRAPHQL_MAX_PAGES = 10; + async function fetchPullRequestDetailsFromGraphQl( env: Env, repoFullName: string, @@ -4146,48 +4156,93 @@ async function fetchPullRequestDetailsFromGraphQl( ): Promise<{ files: GitHubFilePayload[]; reviews: GitHubReviewPayload[] }> { /* v8 ignore start -- GitHub detail GraphQL sparse-node fallbacks are exercised through PR detail hydration tests. */ const { owner, name } = repoParts(repoFullName); - const query = `query LoopOverPullRequestDetails { - repository(owner: ${JSON.stringify(owner)}, name: ${JSON.stringify(name)}) { - pullRequest(number: ${pullNumber}) { - files(first: 100) { + const files: GitHubFilePayload[] = []; + const reviews: GitHubReviewPayload[] = []; + let filesCursor: string | undefined; + let reviewsCursor: string | undefined; + let filesHasNextPage = true; + let reviewsHasNextPage = true; + + for (let page = 1; page <= PR_DETAIL_GRAPHQL_MAX_PAGES && (filesHasNextPage || reviewsHasNextPage); page += 1) { + // Only request a connection that still has more pages -- once one connection finishes, the other keeps + // paginating alone rather than re-fetching data already fully collected. + const filesSelection = filesHasNextPage + ? `files(first: 100${filesCursor ? `, after: ${JSON.stringify(filesCursor)}` : ""}) { + pageInfo { hasNextPage endCursor } nodes { path additions deletions changeType } - } - reviews(first: 100) { + }` + : ""; + const reviewsSelection = reviewsHasNextPage + ? `reviews(first: 100${reviewsCursor ? `, after: ${JSON.stringify(reviewsCursor)}` : ""}) { + pageInfo { hasNextPage endCursor } nodes { databaseId author { login } state authorAssociation submittedAt } - } + }` + : ""; + const query = `query LoopOverPullRequestDetails { + repository(owner: ${JSON.stringify(owner)}, name: ${JSON.stringify(name)}) { + pullRequest(number: ${pullNumber}) { + ${filesSelection} + ${reviewsSelection} } } rateLimit { remaining resetAt } }`; - const response = await githubGraphQl(env, query, token, admissionKey); - const pullRequest = response.data?.repository?.pullRequest; - if (!pullRequest) throw new GitHubApiError(`GitHub GraphQL failed for ${repoFullName} pull request #${pullNumber}: pull request not found`, 404, null, null, null, ""); - const files: GitHubFilePayload[] = (pullRequest.files?.nodes ?? []).flatMap((file) => { - if (!file?.path) return []; - const additions = Number(file.additions ?? 0); - const deletions = Number(file.deletions ?? 0); - return [ - { - filename: file.path, - status: String(file.changeType ?? "modified").toLowerCase(), - additions, - deletions, - changes: additions + deletions, - }, - ]; - }); - const reviews: GitHubReviewPayload[] = (pullRequest.reviews?.nodes ?? []).flatMap((review) => { - if (!review?.databaseId) return []; - return [ - { - id: review.databaseId, - ...(review.author?.login ? { user: { login: review.author.login } } : {}), - ...(review.state ? { state: review.state } : {}), - ...(review.authorAssociation ? { author_association: review.authorAssociation } : {}), - ...(review.submittedAt === undefined ? {} : { submitted_at: review.submittedAt }), - }, - ]; - }); + // A page-1 failure surfaces the same way the un-paginated call always did (the caller's `.catch(() => + // undefined)` falls through to the REST+GraphQL-failed warning). A LATER page failing must not discard + // the pages already collected -- same "keep a successful partial result" semantics as githubPaginatedList. + let response: GitHubPullRequestDetailsResponse | undefined; + try { + response = await githubGraphQl(env, query, token, admissionKey); + } catch (error) { + if (page === 1) throw error; + break; + } + const pullRequest = response.data?.repository?.pullRequest; + if (!pullRequest) { + if (page === 1) throw new GitHubApiError(`GitHub GraphQL failed for ${repoFullName} pull request #${pullNumber}: pull request not found`, 404, null, null, null, ""); + break; + } + if (filesHasNextPage) { + files.push( + ...(pullRequest.files?.nodes ?? []).flatMap((file) => { + if (!file?.path) return []; + const additions = Number(file.additions ?? 0); + const deletions = Number(file.deletions ?? 0); + return [ + { + filename: file.path, + status: String(file.changeType ?? "modified").toLowerCase(), + additions, + deletions, + changes: additions + deletions, + }, + ]; + }), + ); + const pageInfo = pullRequest.files?.pageInfo; + if (pageInfo?.hasNextPage && pageInfo.endCursor) filesCursor = pageInfo.endCursor; + else filesHasNextPage = false; + } + if (reviewsHasNextPage) { + reviews.push( + ...(pullRequest.reviews?.nodes ?? []).flatMap((review) => { + if (!review?.databaseId) return []; + return [ + { + id: review.databaseId, + ...(review.author?.login ? { user: { login: review.author.login } } : {}), + ...(review.state ? { state: review.state } : {}), + ...(review.authorAssociation ? { author_association: review.authorAssociation } : {}), + ...(review.submittedAt === undefined ? {} : { submitted_at: review.submittedAt }), + }, + ]; + }), + ); + const pageInfo = pullRequest.reviews?.pageInfo; + if (pageInfo?.hasNextPage && pageInfo.endCursor) reviewsCursor = pageInfo.endCursor; + else reviewsHasNextPage = false; + } + } return { files, reviews }; /* v8 ignore stop */ } diff --git a/test/unit/backfill.test.ts b/test/unit/backfill.test.ts index 2d4b85e6a7..cbfe52dfd4 100644 --- a/test/unit/backfill.test.ts +++ b/test/unit/backfill.test.ts @@ -3663,6 +3663,111 @@ describe("GitHub backfill", () => { ); }); + it("paginates the GraphQL PR-detail fallback past 100 files instead of truncating at the first page (#7453)", async () => { + const env = createTestEnv({ GITHUB_PUBLIC_TOKEN: "public-token" }); + await seedRegisteredRepo(env); + await upsertPullRequestFromGitHub(env, "JSONbored/gittensory", { + number: 30, + title: "PR with more than 100 changed files", + state: "open", + user: { login: "oktofeesh1" }, + head: { sha: "hugesha" }, + labels: [], + body: "", + }); + const firstPageFiles = Array.from({ length: 100 }, (_, index) => ({ path: `src/file-${index}.ts`, additions: 1, deletions: 0, changeType: "MODIFIED" })); + vi.stubGlobal("fetch", async (input: RequestInfo | URL, init?: RequestInit) => { + const url = input.toString(); + if (url === "https://api.github.com/graphql") { + const query = JSON.parse(String(init?.body ?? "{}")).query as string; + if (query.includes("LoopOverPullRequestDetails")) { + // A second-page request carries the `after` cursor the first page's pageInfo.endCursor handed back. + // The un-paginated fallback never sends this, so this branch only fires against the fixed code. + if (query.includes('after: "files-page-2"')) { + return Response.json({ + data: { + repository: { + pullRequest: { + files: { pageInfo: { hasNextPage: false, endCursor: null }, nodes: [{ path: "src/file-100.ts", additions: 1, deletions: 0, changeType: "MODIFIED" }] }, + }, + }, + rateLimit: { remaining: 4998, resetAt: "2026-05-25T16:00:00Z" }, + }, + }); + } + return Response.json({ + data: { + repository: { + pullRequest: { + files: { pageInfo: { hasNextPage: true, endCursor: "files-page-2" }, nodes: firstPageFiles }, + reviews: { pageInfo: { hasNextPage: false, endCursor: null }, nodes: [{ databaseId: 1, author: { login: "rev1" }, state: "APPROVED", authorAssociation: "MEMBER", submittedAt: "2026-05-25T00:00:00Z" }] }, + }, + }, + rateLimit: { remaining: 4999, resetAt: "2026-05-25T16:00:00Z" }, + }, + }); + } + } + if (url.includes("/pulls/30/files") || url.includes("/pulls/30/reviews")) return new Response("", { status: 404 }); + if (url.includes("/commits/hugesha/check-runs")) return Response.json({}); + return Response.json([]); + }); + + const result = await backfillOpenPullRequestDetails(env, { repoFullName: "JSONbored/gittensory", mode: "full", cursor: 0 }); + + expect(result).toMatchObject({ status: "complete", processed: 1, warnings: [] }); + // 101 files total (100 on page 1 + 1 on page 2) must all be persisted -- truncating at page 1 would leave 100. + expect(await listPullRequestFiles(env, "JSONbored/gittensory", 30)).toHaveLength(101); + expect(await listPullRequestReviews(env, "JSONbored/gittensory", 30)).toEqual([expect.objectContaining({ id: "JSONbored/gittensory#30#1", reviewerLogin: "rev1" })]); + }); + + it("does not send a second GraphQL PR-detail page when the fallback's files/reviews already fit on page one", async () => { + const env = createTestEnv({ GITHUB_PUBLIC_TOKEN: "public-token" }); + await seedRegisteredRepo(env); + await upsertPullRequestFromGitHub(env, "JSONbored/gittensory", { + number: 31, + title: "Single-page PR", + state: "open", + user: { login: "oktofeesh1" }, + head: { sha: "singlesha" }, + labels: [], + body: "", + }); + let graphQlCalls = 0; + vi.stubGlobal("fetch", async (input: RequestInfo | URL, init?: RequestInit) => { + const url = input.toString(); + if (url === "https://api.github.com/graphql") { + const query = JSON.parse(String(init?.body ?? "{}")).query as string; + if (query.includes("LoopOverPullRequestDetails")) { + graphQlCalls += 1; + return Response.json({ + data: { + repository: { + pullRequest: { + files: { pageInfo: { hasNextPage: false, endCursor: null }, nodes: [{ path: "src/only.ts", additions: 2, deletions: 1, changeType: "MODIFIED" }] }, + reviews: { pageInfo: { hasNextPage: false, endCursor: null }, nodes: [{ databaseId: 60, author: { login: "solo-reviewer" }, state: "APPROVED", authorAssociation: "MEMBER", submittedAt: "2026-05-25T00:00:00Z" }] }, + }, + }, + rateLimit: { remaining: 4999, resetAt: "2026-05-25T16:00:00Z" }, + }, + }); + } + } + if (url.includes("/pulls/31/files") || url.includes("/pulls/31/reviews")) return new Response("", { status: 404 }); + if (url.includes("/commits/singlesha/check-runs")) return Response.json({}); + return Response.json([]); + }); + + const result = await backfillOpenPullRequestDetails(env, { repoFullName: "JSONbored/gittensory", mode: "full", cursor: 0 }); + + expect(result).toMatchObject({ status: "complete", processed: 1, warnings: [] }); + expect(await listPullRequestFiles(env, "JSONbored/gittensory", 31)).toEqual([expect.objectContaining({ path: "src/only.ts", changes: 3 })]); + expect(await listPullRequestReviews(env, "JSONbored/gittensory", 31)).toEqual([expect.objectContaining({ id: "JSONbored/gittensory#31#60", reviewerLogin: "solo-reviewer" })]); + // fetchPullRequestFiles and fetchPullRequestReviews each independently fall back to GraphQL (one call + // apiece), but neither should send a page-2 request when pageInfo.hasNextPage is false on page one. + expect(graphQlCalls).toBe(2); + }); + it("refreshes one pull request's files before gate evaluation and drops stale cached paths", async () => { const env = createTestEnv({ GITHUB_PUBLIC_TOKEN: "public-token" }); await seedRegisteredRepo(env);