Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 16 additions & 1 deletion src/github/backfill.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -3851,7 +3858,10 @@ export async function fetchLiveReviewThreadBlockers(
const threads: Array<GitHubReviewThreadNode | null> = [];
let cursor: string | null = null;
const seenCursors = new Set<string>();
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)}) {
Expand All @@ -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
Expand Down
45 changes: 45 additions & 0 deletions test/unit/backfill-2.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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://github.com/ghapi/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://github.com/ghapi/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) => {
Expand Down