diff --git a/src/db/repositories.ts b/src/db/repositories.ts index 7771512266..7bcf60859b 100644 --- a/src/db/repositories.ts +++ b/src/db/repositories.ts @@ -313,12 +313,12 @@ export async function upsertPullRequestFromGitHub( .from(pullRequests) .where(and(eq(pullRequests.repoFullName, repoFullName), eq(pullRequests.number, pr.number))) .limit(1); - const linkedIssueClaimedAt = - record.linkedIssues.length === 0 - ? null - : existingClaimRows[0]?.linkedIssuesJson === linkedIssuesJson - ? (existingClaimRows[0].linkedIssueClaimedAt ?? observedLinkedIssueClaimedAt) - : observedLinkedIssueClaimedAt; + const linkedIssueClaimedAt = resolveLinkedIssueClaimedAt( + record.linkedIssues, + linkedIssuesJson, + existingClaimRows[0], + observedLinkedIssueClaimedAt, + ); await db .insert(pullRequests) .values({ @@ -355,10 +355,7 @@ export async function upsertPullRequestFromGitHub( htmlUrl: pr.html_url, labelsJson: jsonString(record.labels), linkedIssuesJson, - linkedIssueClaimedAt: - record.linkedIssues.length === 0 - ? null - : sql`CASE WHEN ${pullRequests.linkedIssuesJson} = ${linkedIssuesJson} THEN COALESCE(${pullRequests.linkedIssueClaimedAt}, ${observedLinkedIssueClaimedAt}) ELSE ${observedLinkedIssueClaimedAt} END`, + linkedIssueClaimedAt, lastSeenOpenAt, payloadJson: jsonString(compactGitHubPayload(pr)), updatedAt: syncedAt, @@ -367,6 +364,31 @@ export async function upsertPullRequestFromGitHub( return { ...record, linkedIssueClaimedAt }; } +function resolveLinkedIssueClaimedAt( + linkedIssues: number[], + linkedIssuesJson: string, + existing: + | { + linkedIssuesJson: string; + linkedIssueClaimedAt: string | null; + } + | undefined, + observedLinkedIssueClaimedAt: string | null, +): string | null { + if (linkedIssues.length === 0) return null; + if (!existing) return observedLinkedIssueClaimedAt; + if (existing.linkedIssuesJson === linkedIssuesJson) return existing.linkedIssueClaimedAt ?? observedLinkedIssueClaimedAt; + if (existing.linkedIssueClaimedAt && linkedIssuesOverlap(parseJson(existing.linkedIssuesJson, []), linkedIssues)) { + return existing.linkedIssueClaimedAt; + } + return observedLinkedIssueClaimedAt; +} + +function linkedIssuesOverlap(left: number[], right: number[]): boolean { + const rightIssues = new Set(right); + return left.some((issue) => rightIssues.has(issue)); +} + export async function upsertIssueFromGitHub(env: Env, repoFullName: string, issue: GitHubIssuePayload, options: { seenOpenAt?: string } = {}): Promise { const record = toIssueRecord(repoFullName, issue); const db = getDb(env.DB); diff --git a/src/github/backfill.ts b/src/github/backfill.ts index 00f76b2003..88c7bf34b4 100644 --- a/src/github/backfill.ts +++ b/src/github/backfill.ts @@ -2223,25 +2223,33 @@ export async function fetchLivePullRequestReviewDecision(env: Env, repoFullName: return result?.data?.repository?.pullRequest?.reviewDecision ?? undefined; } +type GitHubReviewThreadNode = { + isResolved?: boolean | null; + isOutdated?: boolean | null; + path?: string | null; + line?: number | null; + comments?: { + nodes?: Array<{ + body?: string | null; + url?: string | null; + author?: { login?: string | null } | null; + } | null> | null; + } | null; +}; + +type GitHubReviewThreadConnection = { + nodes?: Array | null; + pageInfo?: { + hasNextPage?: boolean | null; + endCursor?: string | null; + } | null; +}; + type GitHubReviewThreadResponse = { data?: { repository?: { pullRequest?: { - reviewThreads?: { - nodes?: Array<{ - isResolved?: boolean | null; - isOutdated?: boolean | null; - path?: string | null; - line?: number | null; - comments?: { - nodes?: Array<{ - body?: string | null; - url?: string | null; - author?: { login?: string | null } | null; - } | null> | null; - } | null; - } | null> | null; - } | null; + reviewThreads?: GitHubReviewThreadConnection | null; } | null; } | null; }; @@ -2254,30 +2262,49 @@ export async function fetchLiveReviewThreadBlockers(env: Env, repoFullName: stri if (!token) return []; const [owner, name] = repoFullName.split("/"); if (!owner || !name) return []; - const query = `query GittensoryPullRequestReviewThreads { - repository(owner: ${JSON.stringify(owner)}, name: ${JSON.stringify(name)}) { - pullRequest(number: ${prNumber}) { - reviewThreads(first: 50) { - nodes { - isResolved - isOutdated - path - line - comments(first: 20) { - nodes { - body - url - author { login } + const threads: Array = []; + let cursor: string | null = null; + const seenCursors = new Set(); + for (;;) { + const after: string = cursor ? `, after: ${JSON.stringify(cursor)}` : ""; + const query: string = `query GittensoryPullRequestReviewThreads { + repository(owner: ${JSON.stringify(owner)}, name: ${JSON.stringify(name)}) { + pullRequest(number: ${prNumber}) { + reviewThreads(first: 50${after}) { + nodes { + isResolved + isOutdated + path + line + comments(first: 20) { + nodes { + body + url + author { login } + } } } + pageInfo { + hasNextPage + endCursor + } } } } + }`; + const result: GitHubReviewThreadResponse | undefined = await githubGraphQl(env, query, token).catch(() => undefined); + const connection: GitHubReviewThreadConnection | null | undefined = result?.data?.repository?.pullRequest?.reviewThreads; + if (!connection?.nodes) { + if (threads.length === 0) return []; + break; } - }`; - const result = await githubGraphQl(env, query, token).catch(() => undefined); - const threads = result?.data?.repository?.pullRequest?.reviewThreads?.nodes; - if (!threads) return []; + threads.push(...connection.nodes); + if (connection.pageInfo?.hasNextPage !== true) break; + const nextCursor: string | null | undefined = connection.pageInfo.endCursor; + if (!nextCursor || seenCursors.has(nextCursor)) break; + seenCursors.add(nextCursor); + cursor = nextCursor; + } const blockers: ReviewThreadBlocker[] = []; for (const thread of threads) { if (!thread || thread.isResolved !== false || thread.isOutdated === true) continue; diff --git a/test/unit/backfill.test.ts b/test/unit/backfill.test.ts index 3820a2fb99..b93530b5cc 100644 --- a/test/unit/backfill.test.ts +++ b/test/unit/backfill.test.ts @@ -3338,6 +3338,199 @@ describe("GitHub backfill", () => { ]); }); + it("paginates review threads so blockers beyond the first page cannot hide", async () => { + const env = createTestEnv({ GITHUB_PUBLIC_TOKEN: "public-token" }); + const queries: string[] = []; + const fetchSpy = vi.fn(async (input: RequestInfo | URL, init?: RequestInit) => { + if (input.toString() !== "https://api.github.com/graphql") return new Response("not found", { status: 404 }); + const query = JSON.parse(String(init?.body)).query as string; + queries.push(query); + if (!query.includes("after:")) { + return Response.json({ + data: { + repository: { + pullRequest: { + reviewThreads: { + nodes: [{ isResolved: true, isOutdated: false, path: "resolved.ts", line: 1, comments: { nodes: [{ body: "already resolved", author: { login: "scanner[bot]" } }] } }], + pageInfo: { hasNextPage: true, endCursor: "cursor-1" }, + }, + }, + }, + }, + }); + } + return Response.json({ + data: { + repository: { + pullRequest: { + reviewThreads: { + nodes: [ + { + isResolved: false, + isOutdated: false, + path: "src/hidden.ts", + line: 77, + comments: { + nodes: [ + { + body: "**P0:** Hidden second-page review thread must block", + url: "https://github.example/thread/second-page", + author: { login: "superagent-security[bot]" }, + }, + ], + }, + }, + ], + pageInfo: { hasNextPage: false, endCursor: "cursor-2" }, + }, + }, + }, + }, + }); + }); + vi.stubGlobal("fetch", fetchSpy); + + const blockers = await fetchLiveReviewThreadBlockers(env, "JSONbored/gittensory", 1781, "public-token"); + + expect(fetchSpy).toHaveBeenCalledTimes(2); + expect(queries[0]).toContain("reviewThreads(first: 50)"); + expect(queries[1]).toContain('reviewThreads(first: 50, after: "cursor-1")'); + expect(blockers).toEqual([ + expect.objectContaining({ + title: "Hidden second-page review thread must block", + priority: "P0", + path: "src/hidden.ts", + line: 77, + url: "https://github.example/thread/second-page", + }), + ]); + }); + + it("stops review-thread pagination on a repeated cursor without dropping fetched blockers", async () => { + const env = createTestEnv({ GITHUB_PUBLIC_TOKEN: "public-token" }); + let calls = 0; + vi.stubGlobal("fetch", async (input: RequestInfo | URL) => { + if (input.toString() !== "https://api.github.com/graphql") return new Response("not found", { status: 404 }); + calls += 1; + return Response.json({ + data: { + repository: { + pullRequest: { + reviewThreads: { + nodes: + calls === 1 + ? [] + : [ + { + isResolved: false, + isOutdated: false, + path: "src/repeated-cursor.ts", + line: 9, + comments: { nodes: [{ body: "**P1:** Repeated cursor blocker", author: { login: "scanner[bot]" } }] }, + }, + ], + pageInfo: { hasNextPage: true, endCursor: "cursor-1" }, + }, + }, + }, + }, + }); + }); + + const blockers = await fetchLiveReviewThreadBlockers(env, "JSONbored/gittensory", 1781, "public-token"); + + expect(calls).toBe(2); + expect(blockers).toEqual([ + expect.objectContaining({ + title: "Repeated cursor blocker", + path: "src/repeated-cursor.ts", + line: 9, + }), + ]); + }); + + it("keeps fetched review-thread blockers when a later page is malformed", async () => { + const env = createTestEnv({ GITHUB_PUBLIC_TOKEN: "public-token" }); + let calls = 0; + vi.stubGlobal("fetch", async (input: RequestInfo | URL) => { + if (input.toString() !== "https://api.github.com/graphql") return new Response("not found", { status: 404 }); + calls += 1; + if (calls === 2) { + return Response.json({ data: { repository: { pullRequest: { reviewThreads: null } } } }); + } + return Response.json({ + data: { + repository: { + pullRequest: { + reviewThreads: { + nodes: [ + { + isResolved: false, + isOutdated: false, + path: "src/fetched-before-malformed-page.ts", + line: 14, + comments: { nodes: [{ body: "**P1:** Fetched blocker before malformed page", author: { login: "scanner[bot]" } }] }, + }, + ], + pageInfo: { hasNextPage: true, endCursor: "cursor-1" }, + }, + }, + }, + }, + }); + }); + + const blockers = await fetchLiveReviewThreadBlockers(env, "JSONbored/gittensory", 1781, "public-token"); + + expect(calls).toBe(2); + expect(blockers).toEqual([ + expect.objectContaining({ + title: "Fetched blocker before malformed page", + path: "src/fetched-before-malformed-page.ts", + line: 14, + }), + ]); + }); + + it("stops review-thread pagination when GitHub omits the next cursor", async () => { + const env = createTestEnv({ GITHUB_PUBLIC_TOKEN: "public-token" }); + const fetchSpy = vi.fn(async (input: RequestInfo | URL) => { + if (input.toString() !== "https://api.github.com/graphql") return new Response("not found", { status: 404 }); + return Response.json({ + data: { + repository: { + pullRequest: { + reviewThreads: { + nodes: [ + { + isResolved: false, + isOutdated: false, + path: "src/missing-cursor.ts", + line: 12, + comments: { nodes: [{ body: "**P2:** Missing cursor blocker", author: { login: "scanner[bot]" } }] }, + }, + ], + pageInfo: { hasNextPage: true, endCursor: null }, + }, + }, + }, + }, + }); + }); + vi.stubGlobal("fetch", fetchSpy); + + const blockers = await fetchLiveReviewThreadBlockers(env, "JSONbored/gittensory", 1781, "public-token"); + + expect(fetchSpy).toHaveBeenCalledTimes(1); + expect(blockers).toEqual([ + expect.objectContaining({ + title: "Missing cursor blocker", + path: "src/missing-cursor.ts", + line: 12, + }), + ]); + }); + it("ignores resolved, outdated, own-bot, and empty review threads", async () => { const env = createTestEnv({ GITHUB_PUBLIC_TOKEN: "public-token" }); vi.stubGlobal("fetch", async (input: RequestInfo | URL) => { diff --git a/test/unit/db-parsers.test.ts b/test/unit/db-parsers.test.ts index e55ceb8c96..1faf0a52df 100644 --- a/test/unit/db-parsers.test.ts +++ b/test/unit/db-parsers.test.ts @@ -1,4 +1,4 @@ -import { describe, expect, it } from "vitest"; +import { afterEach, describe, expect, it, vi } from "vitest"; import { claimRegateFanoutSlot, countRecentDeadLetters, @@ -26,6 +26,9 @@ import { webhookEvents } from "../../src/db/schema"; import { createTestEnv } from "../helpers/d1"; describe("database row parser hardening", () => { + afterEach(() => { + vi.useRealTimers(); + }); it("caps linked issues extracted from attacker-controlled PR bodies and reports overflow", () => { const body = Array.from({ length: MAX_LINKED_ISSUE_NUMBERS + 25 }, (_, index) => `Fixes #${index + 1}`).join("\n"); @@ -89,6 +92,117 @@ describe("database row parser hardening", () => { ); }); + it("REGRESSION: adding another linked issue preserves the original shared-issue claim time", async () => { + const env = createTestEnv(); + + vi.useFakeTimers(); + vi.setSystemTime(new Date("2026-06-29T10:00:00.000Z")); + await upsertPullRequestFromGitHub(env, "owner/repo", { + number: 11, + title: "First claim", + state: "open", + user: { login: "alice" }, + labels: [], + body: "Fixes #1", + }); + const first = (await listPullRequests(env, "owner/repo")).find((p) => p.number === 11); + + vi.setSystemTime(new Date("2026-06-29T10:02:00.000Z")); + await upsertPullRequestFromGitHub(env, "owner/repo", { + number: 11, + title: "Same claim", + state: "open", + user: { login: "alice" }, + labels: [], + body: "Fixes #1", + }); + const same = (await listPullRequests(env, "owner/repo")).find((p) => p.number === 11); + expect(same).toMatchObject({ + linkedIssues: [1], + linkedIssueClaimedAt: first?.linkedIssueClaimedAt, + }); + + vi.setSystemTime(new Date("2026-06-29T10:05:00.000Z")); + await upsertPullRequestFromGitHub(env, "owner/repo", { + number: 11, + title: "Expanded claim", + state: "open", + user: { login: "alice" }, + labels: [], + body: "Fixes #1\nFixes #2", + }); + const expanded = (await listPullRequests(env, "owner/repo")).find((p) => p.number === 11); + + expect(expanded).toMatchObject({ + title: "Expanded claim", + linkedIssues: [1, 2], + linkedIssueClaimedAt: first?.linkedIssueClaimedAt, + }); + + vi.setSystemTime(new Date("2026-06-29T10:10:00.000Z")); + await upsertPullRequestFromGitHub(env, "owner/repo", { + number: 11, + title: "Disjoint claim", + state: "open", + user: { login: "alice" }, + labels: [], + body: "Fixes #3", + }); + const disjoint = (await listPullRequests(env, "owner/repo")).find((p) => p.number === 11); + + expect(disjoint).toMatchObject({ + linkedIssues: [3], + linkedIssueClaimedAt: "2026-06-29T10:10:00.000Z", + }); + + vi.setSystemTime(new Date("2026-06-29T10:15:00.000Z")); + await upsertPullRequestFromGitHub(env, "owner/repo", { + number: 11, + title: "Cleared claim", + state: "open", + user: { login: "alice" }, + labels: [], + body: "No issue link now.", + }); + const cleared = (await listPullRequests(env, "owner/repo")).find((p) => p.number === 11); + expect(cleared).toMatchObject({ + linkedIssues: [], + linkedIssueClaimedAt: null, + }); + }); + + it("falls back to the observed linked-issue claim time when the existing same-claim timestamp is missing", async () => { + const env = createTestEnv(); + + vi.useFakeTimers(); + vi.setSystemTime(new Date("2026-06-29T11:00:00.000Z")); + await upsertPullRequestFromGitHub(env, "owner/repo", { + number: 12, + title: "Missing claim timestamp", + state: "open", + user: { login: "alice" }, + labels: [], + body: "Fixes #7", + }); + await env.DB.prepare("UPDATE pull_requests SET linked_issue_claimed_at = NULL WHERE repo_full_name = ? AND number = ?").bind("owner/repo", 12).run(); + + vi.setSystemTime(new Date("2026-06-29T11:03:00.000Z")); + await upsertPullRequestFromGitHub(env, "owner/repo", { + number: 12, + title: "Same claim with repaired timestamp", + state: "open", + user: { login: "alice" }, + labels: [], + body: "Fixes #7", + }); + + const repaired = (await listPullRequests(env, "owner/repo")).find((p) => p.number === 12); + expect(repaired).toMatchObject({ + linkedIssues: [7], + linkedIssueClaimedAt: "2026-06-29T11:03:00.000Z", + }); + }); + it("markPullRequestRegated stamps the internal last_regated_at marker (sweep convergence #audit-sweep-converge)", async () => { const env = createTestEnv(); await upsertPullRequestFromGitHub(env, "owner/repo", { number: 5, title: "Stale PR", state: "open", user: { login: "alice" }, labels: [] });