fix(db): order pull-request list queries deterministically - #6388
Conversation
|
Superagent didn't find any vulnerabilities or security issues in this PR. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #6388 +/- ##
=======================================
Coverage 95.60% 95.60%
=======================================
Files 600 600
Lines 47245 47246 +1
Branches 15035 15036 +1
=======================================
+ Hits 45167 45168 +1
Misses 1291 1291
Partials 787 787
Flags with carried forward coverage won't be shown. Click here to find out more.
|
|
Caution 🛑 LoopOver review result - fixes requiredReview updated: 2026-07-16 06:45:37 UTC
Review summary Nits — 6 non-blocking
Why this is blocked
📋 Copy for AI agents — paste into your coding agentCI checks failing
Decision drivers
Context & advisory signals — never blocks the verdict
Review context
Contributor next steps
Signal definitions
🧪 Chat with LoopOverAsk LoopOver a question about this PR directly in a comment — grounded only in the same cached, public-safe facts shown above, never a new claim.
Full command reference: https://loopover.ai/docs/loopover-commands 🧪 Experimental — new and may change. 🟩 Safe / merged · 🟦 Advisory · 🟨 Held for review · 🟥 Blocked / closed 💰 Earn for open-source contributions like this. Gittensor lets GitHub contributors earn for the work they already do — register to start earning →. Checked by LoopOver, a quiet PR intelligence layer for OSS maintainers.
|
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.
7dd92a5 to
1809b82
Compare
…eatedAt 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.
Summary
Test plan
test/unit/db-parsers.test.ts(63 tests), plus the four heaviest consumer test files (backfill,backfill-2,contributor-open-pr-monitor,operator-dashboard; 286 tests) all pass unmodifiednpm run test:cigate green