From 9ce90c25d134aa3cea3ff624e591f862b686627b Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Thu, 25 Jun 2026 17:23:56 -0700 Subject: [PATCH] fix(queue): elect the duplicate-winner from live sibling state, not the stale cache MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The duplicate-cluster winner (#dup-winner) was elected from the stored open-PR cache (listOtherOpenPullRequests). That cache lags GitHub, so a sibling closed or merged on GitHub but still cached `open` kept "winning" the cluster — the real lowest-OPEN PR was treated as a loser, given the duplicate_pr_risk blocker, and one-shot auto-closed. Reconcile the cluster's open siblings against live GitHub state before the winner is elected: re-fetch the LIVE state of just the lower-numbered overlapping siblings (the only ones that can demote this PR) and drop any no longer open, so isDuplicateClusterWinner sees ground truth. Fail-open — a sibling is dropped only on a positive non-open confirmation, and the whole path is gated behind GITTENSORY_DUPLICATE_WINNER, so flag-off is byte-identical with no extra API calls. Adds fetchLivePullRequestState; covers both reconcile arms, the new helper's branches, and the previously-uncovered linkedIssueDuplicatePullRequestsForGate non-overlap arm. --- src/github/backfill.ts | 9 ++ src/queue/processors.ts | 53 ++++++- test/unit/duplicate-winner.test.ts | 35 ++++- test/unit/fetch-live-pr-state.test.ts | 30 ++++ .../reconcile-live-duplicate-siblings.test.ts | 148 ++++++++++++++++++ 5 files changed, 271 insertions(+), 4 deletions(-) create mode 100644 test/unit/fetch-live-pr-state.test.ts create mode 100644 test/unit/reconcile-live-duplicate-siblings.test.ts diff --git a/src/github/backfill.ts b/src/github/backfill.ts index d0079cab59..a830339e04 100644 --- a/src/github/backfill.ts +++ b/src/github/backfill.ts @@ -2141,6 +2141,15 @@ export async function fetchLivePullRequestMergeState(env: Env, repoFullName: str return result?.data.mergeable_state ?? undefined; } +/** The PR's LIVE state ("open" / "closed") via REST `GET /pulls/{n}`. The stored open-PR cache lags GitHub, so a + * sibling closed/merged on GitHub can still read `open` locally; the duplicate-winner election (#dup-winner / + * audit #15) confirms a lower sibling's live state before treating this PR as a cluster loser. Best-effort: + * returns undefined on any error so the caller fails open to the stored state. */ +export async function fetchLivePullRequestState(env: Env, repoFullName: string, prNumber: number, token: string | undefined): Promise { + const result = await githubJsonWithHeaders<{ state?: string | null }>(env, repoFullName, `/pulls/${prNumber}`, token).catch(() => undefined); + return result?.data.state ?? undefined; +} + /** Resolve the OPEN PRs associated with a commit SHA via the REST `GET /repos/{owner}/{repo}/commits/{sha}/pulls` * endpoint. This is the only PR↔commit resolution that works for FORK (cross-repo) PRs, whose CI-completion * webhooks (`check_suite`/`check_run`) carry an EMPTY `pull_requests[]`. Returns the de-duplicated open PR numbers. diff --git a/src/queue/processors.ts b/src/queue/processors.ts index 4ddd253196..8f2ecb10ea 100644 --- a/src/queue/processors.ts +++ b/src/queue/processors.ts @@ -74,6 +74,7 @@ import { fetchLiveCiAggregate, fetchLivePullRequestMergeState, fetchLivePullRequestReviewDecision, + fetchLivePullRequestState, fetchOpenPullRequestNumbersForCommit, fetchRequiredStatusContexts, refreshContributorActivity, @@ -902,10 +903,13 @@ async function reReviewStoredPullRequest( // a rebase fired a synchronize, or CI is still running — the synchronize / CI-completion webhook re-triggers // once the head is current and CI has settled (the sweep backstops a missed event). if (!(await prReadyForReview(env, installationId, repoFullName, pr, settings, deliveryId))) return; - const [otherOpenPullRequests, linkedIssueAuthorLogins] = await Promise.all([ + const [cachedOtherOpenPullRequests, linkedIssueAuthorLogins] = await Promise.all([ listOtherOpenPullRequests(env, repoFullName, prNumber), resolveLinkedIssueAuthorLogins(env, installationId, repoFullName, pr.linkedIssues, settings.selfAuthoredLinkedIssueGateMode === "block"), ]); + // #dup-winner / audit #15: drop any cached-open duplicate sibling already closed on GitHub before the advisory + // (and the disposition below) elect the cluster winner, so the real lowest-OPEN PR is never demoted+auto-closed. + const otherOpenPullRequests = await reconcileLiveDuplicateSiblings(env, installationId, repoFullName, pr, cachedOtherOpenPullRequests); const advisory = buildPullRequestAdvisory(repo, pr, { otherOpenPullRequests, requireLinkedIssue: shouldCollectLinkedIssueEvidence(settings), @@ -1687,11 +1691,14 @@ async function processGitHubWebhook(env: Env, deliveryId: string, eventName: str } // Resolve settings first so the self-authored live-fetch fallback only fires when its gate is in block mode. const settings = await resolveRepositorySettings(env, repoFullName); - const [repo, otherOpenPullRequests, linkedIssueAuthorLogins] = await Promise.all([ + const [repo, cachedOtherOpenPullRequests, linkedIssueAuthorLogins] = await Promise.all([ getRepository(env, repoFullName), listOtherOpenPullRequests(env, repoFullName, pr.number), resolveLinkedIssueAuthorLogins(env, installationId, repoFullName, pr.linkedIssues, settings.selfAuthoredLinkedIssueGateMode === "block"), ]); + // #dup-winner / audit #15: drop any cached-open duplicate sibling already closed on GitHub before the + // advisory (and the disposition) elect the cluster winner, so the real lowest-OPEN PR is never auto-closed. + const otherOpenPullRequests = await reconcileLiveDuplicateSiblings(env, installationId, repoFullName, pr, cachedOtherOpenPullRequests); const advisory = buildPullRequestAdvisory(repo, pr, { otherOpenPullRequests, requireLinkedIssue: shouldCollectLinkedIssueEvidence(settings), @@ -2273,7 +2280,47 @@ export function dupWinnerLinkedDuplicateCount(openSiblingNumbers: number[], prNu return openSiblingNumbers.length; } -function linkedIssueDuplicatePullRequestsForGate(pr: PullRequestRecord, pullRequests: PullRequestRecord[]): number[] { +/** + * Live-reconcile the duplicate cluster's open siblings before the winner is elected (#dup-winner / audit #15). + * + * The stored open-PR cache ({@link listOtherOpenPullRequests}) lags GitHub: a sibling that was closed/merged on + * GitHub but is still cached `open` would keep "winning" the duplicate cluster, demoting the real lowest-OPEN PR + * to a loser and auto-closing it via the `duplicate_pr_risk` blocker. Only a LOWER-numbered overlapping sibling + * can demote this PR from winner, so re-fetch the LIVE state of just those siblings and drop any that are no + * longer open. Then the downstream election ({@link isDuplicateClusterWinner}) reflects ground truth. + * + * FAIL-OPEN to the stored state: a sibling is dropped ONLY on a positive "not open" confirmation — an unreadable + * live fetch keeps it, so a transient GitHub hiccup never newly spares a real loser. Flag-OFF (default), no + * linked issues, or no lower overlapping sibling ⇒ returns the input unchanged with no extra API calls. + */ +export async function reconcileLiveDuplicateSiblings( + env: Env, + installationId: number | null, + repoFullName: string, + pr: PullRequestRecord, + otherOpenPullRequests: PullRequestRecord[], +): Promise { + if (env.GITTENSORY_DUPLICATE_WINNER !== "true") return otherOpenPullRequests; + const linkedIssues = new Set(pr.linkedIssues); + if (linkedIssues.size === 0) return otherOpenPullRequests; + const lowerOverlapping = otherOpenPullRequests.filter( + (other) => other.number < pr.number && other.state === "open" && other.linkedIssues.some((issue) => linkedIssues.has(issue)), + ); + if (lowerOverlapping.length === 0) return otherOpenPullRequests; + const installationToken = installationId === null ? undefined : await createInstallationToken(env, installationId).catch(() => undefined); + const token = installationToken ?? env.GITHUB_PUBLIC_TOKEN; + const staleClosed = new Set(); + await Promise.all( + lowerOverlapping.map(async (sibling) => { + const liveState = await fetchLivePullRequestState(env, repoFullName, sibling.number, token).catch(() => undefined); + if (liveState !== undefined && liveState !== "open") staleClosed.add(sibling.number); + }), + ); + if (staleClosed.size === 0) return otherOpenPullRequests; + return otherOpenPullRequests.filter((other) => !staleClosed.has(other.number)); +} + +export function linkedIssueDuplicatePullRequestsForGate(pr: PullRequestRecord, pullRequests: PullRequestRecord[]): number[] { const linkedIssues = new Set(pr.linkedIssues); if (linkedIssues.size === 0) return []; return [ diff --git a/test/unit/duplicate-winner.test.ts b/test/unit/duplicate-winner.test.ts index 92bc5141ed..4ba49829d8 100644 --- a/test/unit/duplicate-winner.test.ts +++ b/test/unit/duplicate-winner.test.ts @@ -1,6 +1,7 @@ import { describe, expect, it } from "vitest"; import { isDuplicateClusterWinner } from "../../src/signals/duplicate-winner"; -import { dupWinnerLinkedDuplicateCount } from "../../src/queue/processors"; +import { dupWinnerLinkedDuplicateCount, linkedIssueDuplicatePullRequestsForGate } from "../../src/queue/processors"; +import type { PullRequestRecord } from "../../src/types"; import { listOtherOpenPullRequests, upsertPullRequestFromGitHub } from "../../src/db/repositories"; import { createTestEnv } from "../helpers/d1"; @@ -53,6 +54,38 @@ describe("dupWinnerLinkedDuplicateCount (#dup-winner close-reason seam)", () => }); }); +describe("linkedIssueDuplicatePullRequestsForGate (#dup-winner open-sibling source)", () => { + const pr = (number: number, state: string, linkedIssues: number[]): PullRequestRecord => ({ + repoFullName: "owner/repo", + number, + title: `PR ${number}`, + state, + labels: [], + linkedIssues, + }); + + it("the PR links no issue ⇒ no cluster siblings", () => { + expect(linkedIssueDuplicatePullRequestsForGate(pr(9, "open", []), [pr(5, "open", [1])])).toEqual([]); + }); + + it("includes an OPEN sibling that overlaps the linked-issue set, sorted + de-duplicated", () => { + const subject = pr(9, "open", [1, 2]); + const others = [pr(7, "open", [2]), pr(5, "open", [1]), pr(5, "open", [1])]; + expect(linkedIssueDuplicatePullRequestsForGate(subject, others)).toEqual([5, 7]); + }); + + it("excludes a sibling that does NOT overlap the linked-issue set (the false ternary arm)", () => { + const subject = pr(9, "open", [1]); + expect(linkedIssueDuplicatePullRequestsForGate(subject, [pr(5, "open", [2])])).toEqual([]); + }); + + it("excludes self and any non-open sibling", () => { + const subject = pr(9, "open", [1]); + const others = [pr(9, "open", [1]), pr(5, "closed", [1])]; + expect(linkedIssueDuplicatePullRequestsForGate(subject, others)).toEqual([]); + }); +}); + describe("listOtherOpenPullRequests ordering (#audit-3.9)", () => { it("orders by ascending number so the lowest open sibling survives the 100-row cap", async () => { const env = createTestEnv(); diff --git a/test/unit/fetch-live-pr-state.test.ts b/test/unit/fetch-live-pr-state.test.ts new file mode 100644 index 0000000000..a8294dfe2f --- /dev/null +++ b/test/unit/fetch-live-pr-state.test.ts @@ -0,0 +1,30 @@ +import { afterEach, describe, expect, it, vi } from "vitest"; +import { fetchLivePullRequestState } from "../../src/github/backfill"; +import { createTestEnv } from "../helpers/d1"; + +describe("fetchLivePullRequestState (#dup-winner / audit #15)", () => { + afterEach(() => { + vi.unstubAllGlobals(); + }); + + it("returns the live state from GET /pulls/{n}", async () => { + const env = createTestEnv({ GITHUB_PUBLIC_TOKEN: "public-token" }); + vi.stubGlobal("fetch", async (input: RequestInfo | URL) => { + expect(String(input)).toContain("/repos/owner/repo/pulls/7"); + return Response.json({ state: "closed" }); + }); + expect(await fetchLivePullRequestState(env, "owner/repo", 7, "tok")).toBe("closed"); + }); + + it("returns undefined when the payload omits state", async () => { + const env = createTestEnv({ GITHUB_PUBLIC_TOKEN: "public-token" }); + vi.stubGlobal("fetch", async () => Response.json({})); + expect(await fetchLivePullRequestState(env, "owner/repo", 7, "tok")).toBeUndefined(); + }); + + it("returns undefined (fail-open) when the fetch errors", async () => { + const env = createTestEnv({ GITHUB_PUBLIC_TOKEN: "public-token" }); + vi.stubGlobal("fetch", async () => new Response("nope", { status: 500 })); + expect(await fetchLivePullRequestState(env, "owner/repo", 7, "tok")).toBeUndefined(); + }); +}); diff --git a/test/unit/reconcile-live-duplicate-siblings.test.ts b/test/unit/reconcile-live-duplicate-siblings.test.ts new file mode 100644 index 0000000000..b169ee82b9 --- /dev/null +++ b/test/unit/reconcile-live-duplicate-siblings.test.ts @@ -0,0 +1,148 @@ +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; +import { reconcileLiveDuplicateSiblings } from "../../src/queue/processors"; +import { createInstallationToken } from "../../src/github/app"; +import type { PullRequestRecord } from "../../src/types"; +import { createTestEnv } from "../helpers/d1"; + +// reconcileLiveDuplicateSiblings re-fetches a lower duplicate sibling's LIVE state via createInstallationToken + +// the REST /pulls/{n} call, so mock the token mint (the test env holds no App key) and stub fetch per case. +vi.mock("../../src/github/app", async (importOriginal) => ({ + ...(await importOriginal()), + createInstallationToken: vi.fn(), +})); +const mockedToken = vi.mocked(createInstallationToken); + +function makePr(number: number, state: string, linkedIssues: number[]): PullRequestRecord { + return { repoFullName: "owner/repo", number, title: `PR ${number}`, state, labels: [], linkedIssues }; +} + +/** Stub fetch so any /pulls/{n} returns the mapped live state; an unmapped path 404s (→ undefined live state). */ +function stubLiveStates(states: Record): void { + vi.stubGlobal("fetch", async (input: RequestInfo | URL) => { + const url = String(input); + for (const [number, state] of Object.entries(states)) { + if (url.includes(`/pulls/${number}`)) return new Response(JSON.stringify({ state }), { status: 200 }); + } + return new Response("not found", { status: 404 }); + }); +} + +describe("reconcileLiveDuplicateSiblings (#dup-winner / audit #15)", () => { + beforeEach(() => { + mockedToken.mockReset(); + mockedToken.mockRejectedValue(new Error("no app key in test env")); + }); + afterEach(() => { + vi.unstubAllGlobals(); + }); + + it("flag OFF ⇒ returns the cached list untouched and never hits GitHub", async () => { + const env = createTestEnv(); + env.GITTENSORY_DUPLICATE_WINNER = "false"; + vi.stubGlobal("fetch", async () => { + throw new Error("fetch must not be called when the flag is off"); + }); + const siblings = [makePr(5, "open", [1])]; + const pr = makePr(9, "open", [1]); + expect(await reconcileLiveDuplicateSiblings(env, null, "owner/repo", pr, siblings)).toBe(siblings); + }); + + it("flag ON but the PR links no issue ⇒ unchanged (no cluster to adjudicate)", async () => { + const env = createTestEnv(); + env.GITTENSORY_DUPLICATE_WINNER = "true"; + const siblings = [makePr(5, "open", [1])]; + const pr = makePr(9, "open", []); + expect(await reconcileLiveDuplicateSiblings(env, null, "owner/repo", pr, siblings)).toBe(siblings); + }); + + it("flag ON but every overlapping sibling is HIGHER-numbered ⇒ this PR already wins, unchanged (no fetch)", async () => { + const env = createTestEnv(); + env.GITTENSORY_DUPLICATE_WINNER = "true"; + vi.stubGlobal("fetch", async () => { + throw new Error("higher-numbered siblings cannot demote the winner; no live fetch expected"); + }); + const siblings = [makePr(12, "open", [1])]; + const pr = makePr(9, "open", [1]); + expect(await reconcileLiveDuplicateSiblings(env, null, "owner/repo", pr, siblings)).toBe(siblings); + }); + + it("flag ON, a lower sibling that does NOT overlap the issue set ⇒ unchanged (not a cluster member)", async () => { + const env = createTestEnv(); + env.GITTENSORY_DUPLICATE_WINNER = "true"; + const siblings = [makePr(5, "open", [2])]; + const pr = makePr(9, "open", [1]); + expect(await reconcileLiveDuplicateSiblings(env, null, "owner/repo", pr, siblings)).toBe(siblings); + }); + + it("flag ON, a lower sibling already cached non-open ⇒ unchanged (the cache already excludes it)", async () => { + const env = createTestEnv(); + env.GITTENSORY_DUPLICATE_WINNER = "true"; + const siblings = [makePr(5, "closed", [1])]; + const pr = makePr(9, "open", [1]); + expect(await reconcileLiveDuplicateSiblings(env, null, "owner/repo", pr, siblings)).toBe(siblings); + }); + + it("flag ON, a lower overlapping sibling LIVE-closed ⇒ dropped so the real lowest-OPEN PR is the winner", async () => { + const env = createTestEnv(); + env.GITTENSORY_DUPLICATE_WINNER = "true"; + stubLiveStates({ 5: "closed" }); + const siblings = [makePr(5, "open", [1])]; + const pr = makePr(9, "open", [1]); + const result = await reconcileLiveDuplicateSiblings(env, null, "owner/repo", pr, siblings); + expect(result.map((p) => p.number)).toEqual([]); + }); + + it("flag ON, a lower overlapping sibling LIVE-open ⇒ kept (this PR is genuinely a loser)", async () => { + const env = createTestEnv(); + env.GITTENSORY_DUPLICATE_WINNER = "true"; + stubLiveStates({ 5: "open" }); + const siblings = [makePr(5, "open", [1])]; + const pr = makePr(9, "open", [1]); + const result = await reconcileLiveDuplicateSiblings(env, null, "owner/repo", pr, siblings); + expect(result.map((p) => p.number)).toEqual([5]); + }); + + it("flag ON, an unreadable live fetch ⇒ fails OPEN (sibling kept; no new spare on a transient hiccup)", async () => { + const env = createTestEnv(); + env.GITTENSORY_DUPLICATE_WINNER = "true"; + stubLiveStates({}); // every /pulls/{n} 404s ⇒ undefined live state + const siblings = [makePr(5, "open", [1])]; + const pr = makePr(9, "open", [1]); + const result = await reconcileLiveDuplicateSiblings(env, null, "owner/repo", pr, siblings); + expect(result.map((p) => p.number)).toEqual([5]); + }); + + it("flag ON, two lower overlapping siblings (one live-closed, one live-open) ⇒ only the closed one is dropped", async () => { + const env = createTestEnv(); + env.GITTENSORY_DUPLICATE_WINNER = "true"; + stubLiveStates({ 4: "closed", 5: "open" }); + const siblings = [makePr(4, "open", [1]), makePr(5, "open", [1])]; + const pr = makePr(9, "open", [1]); + const result = await reconcileLiveDuplicateSiblings(env, null, "owner/repo", pr, siblings); + expect(result.map((p) => p.number)).toEqual([5]); + }); + + it("flag ON, a numeric installation whose token mint THROWS ⇒ falls back to the public token and still reconciles", async () => { + const env = createTestEnv(); + env.GITTENSORY_DUPLICATE_WINNER = "true"; + env.GITHUB_PUBLIC_TOKEN = "public-tok"; + stubLiveStates({ 5: "closed" }); + const siblings = [makePr(5, "open", [1])]; + const pr = makePr(9, "open", [1]); + const result = await reconcileLiveDuplicateSiblings(env, 4242, "owner/repo", pr, siblings); + expect(mockedToken).toHaveBeenCalledWith(env, 4242); + expect(result.map((p) => p.number)).toEqual([]); + }); + + it("flag ON, a numeric installation whose token mint SUCCEEDS ⇒ uses the installation token to reconcile", async () => { + const env = createTestEnv(); + env.GITTENSORY_DUPLICATE_WINNER = "true"; + mockedToken.mockResolvedValue("inst-tok"); + stubLiveStates({ 5: "closed" }); + const siblings = [makePr(5, "open", [1])]; + const pr = makePr(9, "open", [1]); + const result = await reconcileLiveDuplicateSiblings(env, 4242, "owner/repo", pr, siblings); + expect(mockedToken).toHaveBeenCalledWith(env, 4242); + expect(result.map((p) => p.number)).toEqual([]); + }); +});