diff --git a/src/db/repositories.ts b/src/db/repositories.ts index a95f404cb4..4715da2fbb 100644 --- a/src/db/repositories.ts +++ b/src/db/repositories.ts @@ -430,6 +430,11 @@ export async function upsertPullRequestFromGitHub( headShaObservedAt, lastSeenOpenAt, payloadJson: jsonString(payload), + // GitHub's own PR creation time (see PullRequestRecord.createdAt's doc comment, src/types.ts) -- + // set ONLY here, on first insert, and deliberately absent from onConflictDoUpdate's `set` below so + // a resync never overwrites it. `?? undefined` falls through to the column's own $defaultFn when a + // sparse payload omits created_at, matching every other optional GitHub-sourced field's convention. + createdAt: pr.created_at ?? undefined, updatedAt: syncedAt, }) .onConflictDoUpdate({ @@ -4254,15 +4259,25 @@ export async function markUnseenOpenPullRequestsClosed(env: Env, fullName: strin return Number(result.meta.changes ?? 0); } +// Ordered by DESCENDING PR number so a repo with >500 PRs keeps its MOST RECENT ones, not an arbitrary/ +// unordered slice a plain LIMIT would otherwise return (Postgres gives no ordering guarantee without +// ORDER BY -- confirmed live: an unordered 500-row cap on a 2930-row repo produced a skewed, non- +// representative sample that inverted src/services/outcome-calibration.ts's slop-band merge-rate check, +// which reads this exact function, and fired a false "score not discriminating" ops_anomaly). Every other +// caller (MCP tools, gate-precision, quality metrics, recap) benefits the same way: recent PRs are the +// relevant population for almost every one of them, an arbitrary old slice never was. export async function listPullRequests(env: Env, fullName: string): Promise { const db = getDb(env.DB); - const rows = await db.select().from(pullRequests).where(eq(pullRequests.repoFullName, fullName)).limit(500); + const rows = await db.select().from(pullRequests).where(eq(pullRequests.repoFullName, fullName)).orderBy(desc(pullRequests.number)).limit(500); return rows.map(toPullRequestRecordFromRow); } +// Same ordering rationale as listPullRequests above, but cross-repo -- PR number resets per repo, so +// createdAt (an ISO 8601 UTC string, sortable lexicographically) is the only field that's globally +// comparable for "most recent" across every repo at once. export async function listAllPullRequests(env: Env): Promise { const db = getDb(env.DB); - const rows = await db.select().from(pullRequests).limit(2000); + const rows = await db.select().from(pullRequests).orderBy(desc(pullRequests.createdAt)).limit(2000); return rows.map(toPullRequestRecordFromRow); } diff --git a/test/unit/db-parsers.test.ts b/test/unit/db-parsers.test.ts index 551ec2cbcb..f5eb4453b3 100644 --- a/test/unit/db-parsers.test.ts +++ b/test/unit/db-parsers.test.ts @@ -17,6 +17,7 @@ import { getLatestScoringModelSnapshot, getFreshOfficialMinerDetection, getPullRequest, + listAllPullRequests, listPullRequests, listPullRequestDetailSyncStates, listRepoSyncSegments, @@ -1336,3 +1337,51 @@ describe("database row parser hardening", () => { expect(JSON.parse(raw?.snapshot_json ?? "{}")).toMatchObject({ githubId: "", githubUsername: "", issueLabels: [] }); }); }); + +// #ops-anomaly-calibration-sample-order: both list functions cap their row count, and a LIMIT with no ORDER BY +// gives no ordering guarantee -- confirmed live on a 2930-row repo, where the resulting arbitrary 500-row slice +// inverted src/services/outcome-calibration.ts's slop-band merge-rate comparison (which reads listPullRequests) +// and fired a false ops_anomaly "score not discriminating" alert, even though the score discriminates correctly +// (monotonically decreasing merge rate by rising severity) over the true full population. Ordering by recency +// fixes this at the source, for every caller, not just calibration. +describe("listPullRequests / listAllPullRequests ordering (#ops-anomaly-calibration-sample-order)", () => { + it("listPullRequests returns a repo's PRs ordered by DESCENDING number, regardless of insert order", async () => { + const env = createTestEnv(); + for (const number of [3, 1, 4, 2]) { + await upsertPullRequestFromGitHub(env, "owner/repo", { + number, + title: `PR #${number}`, + state: "open", + user: { login: "contributor1" }, + labels: [], + body: null, + }); + } + + const numbers = (await listPullRequests(env, "owner/repo")).map((pr) => pr.number); + expect(numbers).toEqual([4, 3, 2, 1]); + }); + + it("listAllPullRequests returns PRs ordered by DESCENDING createdAt across repos, regardless of insert order", async () => { + const env = createTestEnv(); + const seeds: Array<{ repoFullName: string; number: number; createdAt: string }> = [ + { repoFullName: "owner/repo-a", number: 1, createdAt: "2026-01-01T00:00:00.000Z" }, + { repoFullName: "owner/repo-b", number: 1, createdAt: "2026-03-01T00:00:00.000Z" }, + { repoFullName: "owner/repo-a", number: 2, createdAt: "2026-02-01T00:00:00.000Z" }, + ]; + for (const seed of seeds) { + await upsertPullRequestFromGitHub(env, seed.repoFullName, { + number: seed.number, + title: `${seed.repoFullName}#${seed.number}`, + state: "open", + user: { login: "contributor1" }, + labels: [], + body: null, + created_at: seed.createdAt, + }); + } + + const ordered = (await listAllPullRequests(env)).map((pr) => `${pr.repoFullName}#${pr.number}`); + expect(ordered).toEqual(["owner/repo-b#1", "owner/repo-a#2", "owner/repo-a#1"]); + }); +});