diff --git a/src/db/repositories.ts b/src/db/repositories.ts index 61bca53ca9..48e6bda531 100644 --- a/src/db/repositories.ts +++ b/src/db/repositories.ts @@ -1,4 +1,4 @@ -import { and, desc, eq, gte, inArray, not, or, sql, type SQL } from "drizzle-orm"; +import { and, asc, desc, eq, gte, inArray, not, or, sql, type SQL } from "drizzle-orm"; import { getDb } from "./client"; import { advisories, @@ -2675,6 +2675,10 @@ export async function listOtherOpenPullRequests(env: Env, fullName: string, numb .select() .from(pullRequests) .where(and(eq(pullRequests.repoFullName, fullName), eq(pullRequests.state, "open"), not(eq(pullRequests.number, number)))) + // Order by ascending PR number so the 100-row cap always retains the LOWEST-numbered open siblings. The + // duplicate-winner adjudication elects the minimum open number as the winner, so an unordered LIMIT could + // drop the true winner on a repo with >100 open PRs and mis-elect a higher-numbered sibling. (#audit-3.9) + .orderBy(asc(pullRequests.number)) .limit(100); return rows.map(toPullRequestRecordFromRow); } diff --git a/test/unit/duplicate-winner.test.ts b/test/unit/duplicate-winner.test.ts index af3fda9ce0..92bc5141ed 100644 --- a/test/unit/duplicate-winner.test.ts +++ b/test/unit/duplicate-winner.test.ts @@ -1,6 +1,8 @@ import { describe, expect, it } from "vitest"; import { isDuplicateClusterWinner } from "../../src/signals/duplicate-winner"; import { dupWinnerLinkedDuplicateCount } from "../../src/queue/processors"; +import { listOtherOpenPullRequests, upsertPullRequestFromGitHub } from "../../src/db/repositories"; +import { createTestEnv } from "../helpers/d1"; describe("isDuplicateClusterWinner (#dup-winner)", () => { it("the lowest open sibling number wins", () => { @@ -50,3 +52,20 @@ describe("dupWinnerLinkedDuplicateCount (#dup-winner close-reason seam)", () => expect(dupWinnerLinkedDuplicateCount([], 12, false)).toBe(0); }); }); + +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(); + // Insert the LOWEST number (#1) LAST so an unordered insertion-order LIMIT(100) would drop it (and thus + // mis-elect the duplicate-winner, which is the minimum open number). + const numbers = [...Array.from({ length: 101 }, (_, i) => i + 2), 1]; // 2..102, then 1 + for (const n of numbers) { + await upsertPullRequestFromGitHub(env, "owner/repo", { number: n, title: `PR ${n}`, state: "open", user: { login: "c" }, head: { sha: `s${n}` }, labels: [], body: "x" }); + } + const siblings = await listOtherOpenPullRequests(env, "owner/repo", 200); // siblings of a non-existent #200 + const siblingNumbers = siblings.map((p) => p.number); + expect(siblings).toHaveLength(100); // capped + expect(Math.min(...siblingNumbers)).toBe(1); // the true winner #1 is retained despite being inserted last + expect(siblingNumbers).not.toContain(102); // the lowest 100 (1..100) are returned, not the first-inserted 100 + }); +});