From 1809b82225cb710a99b192cc2aba4d7bea1144ab Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Wed, 15 Jul 2026 22:51:26 -0700 Subject: [PATCH 1/2] fix(db): order pull-request list queries deterministically listPullRequests and listAllPullRequests both cap their row count (500/2000) with no ORDER BY -- Postgres gives no ordering guarantee without one, so each returned an arbitrary, non-representative slice. Confirmed live: on a 2930-row repo, the unordered 500-row cap produced a sample where a higher slop-severity band merged MORE often than a lower one, tripping src/services/outcome-calibration.ts's discrimination check and firing a false "slop score not discriminating" ops_anomaly. Over the true full population the score discriminates correctly (monotonically decreasing merge rate as severity rises) -- the scoring rubric itself was never the problem. Orders by descending PR number (listPullRequests, single-repo) and descending createdAt (listAllPullRequests, cross-repo -- PR numbers reset per repo so createdAt is the only globally comparable field). Every other caller (MCP tools, gate-precision, quality metrics, recap) benefits the same way: recent PRs are the relevant population for almost all of them, an arbitrary old slice never was. --- src/db/repositories.ts | 14 +++++++++-- test/unit/db-parsers.test.ts | 49 ++++++++++++++++++++++++++++++++++++ 2 files changed, 61 insertions(+), 2 deletions(-) diff --git a/src/db/repositories.ts b/src/db/repositories.ts index a95f404cb4..7af3cbe137 100644 --- a/src/db/repositories.ts +++ b/src/db/repositories.ts @@ -4254,15 +4254,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"]); + }); +}); From 198c35459282d76407b6b4cf8c7f8199239a209b Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Wed, 15 Jul 2026 23:47:04 -0700 Subject: [PATCH 2/2] fix(db): persist GitHub's real PR creation time into pull_requests.createdAt upsertPullRequestFromGitHub never wrote pr.created_at into the createdAt column on insert, so it always fell back to the schema's $defaultFn(() => nowIso()) -- the column silently recorded "when loopover's own webhook/sync pipeline first saw this row" instead of GitHub's actual PR creation time, even though PullRequestRecord.createdAt is explicitly documented as GitHub's own creation time (src/types.ts) and used for #dup-winner duplicate-cluster election. This is what broke listAllPullRequests' new ORDER BY createdAt DESC: the column never reflected real creation order, only insert order, so its own regression test failed on any set of PRs synced out of chronological order. Sets createdAt: pr.created_at ?? undefined ONLY in the initial .values() insert -- deliberately absent from onConflictDoUpdate's set block, since a PR's real creation date must never change on resync. --- src/db/repositories.ts | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/db/repositories.ts b/src/db/repositories.ts index 7af3cbe137..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({