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
6 changes: 5 additions & 1 deletion src/db/repositories.ts
Original file line number Diff line number Diff line change
@@ -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,
Expand Down Expand Up @@ -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);
}
Expand Down
19 changes: 19 additions & 0 deletions test/unit/duplicate-winner.test.ts
Original file line number Diff line number Diff line change
@@ -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", () => {
Expand Down Expand Up @@ -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
});
});
Loading