From 2830fb1d5fb819c0d90369a7ba06d1fc7f1baf5c Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Thu, 9 Jul 2026 14:28:19 -0700 Subject: [PATCH 1/2] fix(review): make listPullRequestFiles order deterministic to stabilize content fingerprints listPullRequestFiles had no ORDER BY, so row order was whatever the query planner returned -- not guaranteed stable across repeat calls for the same unchanged PR. buildUnifiedReviewDiff only fully orders files by (priority bucket, added-line count); files tied on both fall through to this function's own order, so an unstable order silently reorders the diff string and flips any hash built from it. Confirmed live: JSONbored/metagraphed#4532 re-ran its linked-issue-satisfaction LLM call 12 times across 7 hours on one unchanged head SHA, despite a matching cache row already existing, because the fingerprint (which includes the diff text) never matched between passes. Adds an explicit ORDER BY path (unique per repo+pull per the table's own index, so no secondary tie-break is needed) and a regression test pinning the deterministic order. --- src/db/repositories.ts | 13 ++++++++- .../backfill-file-hydration-scoping.test.ts | 29 +++++++++++++++++++ 2 files changed, 41 insertions(+), 1 deletion(-) diff --git a/src/db/repositories.ts b/src/db/repositories.ts index b202db5fa2..35960f6daf 100644 --- a/src/db/repositories.ts +++ b/src/db/repositories.ts @@ -4097,12 +4097,23 @@ export async function deletePullRequestFiles(env: Env, fullName: string, pullNum await db.delete(pullRequestFiles).where(and(eq(pullRequestFiles.repoFullName, fullName), eq(pullRequestFiles.pullNumber, pullNumber))); } +// #linked-issue-satisfaction-cache-fingerprint-stability: an explicit deterministic order is load-bearing, +// not cosmetic. Without it, row order is whatever the query planner happens to return -- unstable across +// otherwise-identical repeat calls for the SAME unchanged PR -- and downstream diff building +// (buildUnifiedReviewDiff) only fully orders files by (priority bucket, added-line count); two files tied on +// both fall back to THIS function's own (undefined) order. That untied order then flows straight into a +// SHA-256 content fingerprint (linkedIssueSatisfactionCacheInputFingerprint and friends), so a silent reorder +// alone changes the hash and defeats the cache even though the diff's actual content never changed -- +// confirmed live: JSONbored/metagraphed#4532 re-ran its linked-issue-satisfaction LLM call 12 times across 7 +// hours on one unchanged head SHA. `path` is unique per (repoFullName, pullNumber) (see the table's own +// unique index), so it alone is a total, stable order -- no secondary tie-break needed. export async function listPullRequestFiles(env: Env, fullName: string, pullNumber: number): Promise { const db = getDb(env.DB); const rows = await db .select() .from(pullRequestFiles) - .where(and(eq(pullRequestFiles.repoFullName, fullName), eq(pullRequestFiles.pullNumber, pullNumber))); + .where(and(eq(pullRequestFiles.repoFullName, fullName), eq(pullRequestFiles.pullNumber, pullNumber))) + .orderBy(pullRequestFiles.path); return rows.map(toPullRequestFileRecord); } diff --git a/test/unit/backfill-file-hydration-scoping.test.ts b/test/unit/backfill-file-hydration-scoping.test.ts index bd965c6860..6924b49320 100644 --- a/test/unit/backfill-file-hydration-scoping.test.ts +++ b/test/unit/backfill-file-hydration-scoping.test.ts @@ -519,4 +519,33 @@ describe("GitHub PR file hydration scoping (#audit-rate-headroom)", () => { expect(await getPullRequestDetailSyncState(env, "JSONbored/gittensory", 91)).toMatchObject({ headSha: null }); }); }); + + describe("listPullRequestFiles ordering (#linked-issue-satisfaction-cache-fingerprint-stability)", () => { + it("REGRESSION: returns files in a stable path order regardless of write order, so a downstream content fingerprint never flips for an unchanged PR", async () => { + const env = createTestEnv(); + // Written deliberately out of path order -- without an explicit ORDER BY, a query planner is free to + // return rows in ITS OWN order, which is not guaranteed to match write order or stay stable call to + // call. buildUnifiedReviewDiff only fully orders by (priority bucket, added-line count); files tied on + // both (as these three are: all source, all 0 additions/0 deletions) fall through to this function's + // own order, so an unstable order here silently reorders the diff string and flips any hash built from + // it (e.g. linkedIssueSatisfactionCacheInputFingerprint) even though nothing about the diff changed. + const paths = ["src/z.ts", "src/a.ts", "src/m.ts"]; + for (const path of paths) { + await upsertPullRequestFile(env, { + repoFullName: "JSONbored/gittensory", + pullNumber: 200, + path, + status: "modified", + additions: 0, + deletions: 0, + changes: 0, + payload: {}, + }); + } + const first = await listPullRequestFiles(env, "JSONbored/gittensory", 200); + const second = await listPullRequestFiles(env, "JSONbored/gittensory", 200); + expect(first.map((file) => file.path)).toEqual(["src/a.ts", "src/m.ts", "src/z.ts"]); + expect(second.map((file) => file.path)).toEqual(first.map((file) => file.path)); + }); + }); }); From 57f4589c3a43517d262443803fe71546612a74e3 Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Thu, 9 Jul 2026 14:42:52 -0700 Subject: [PATCH 2/2] test(rag): update rag-index expected path order for the deterministic file-listing fix listPullRequestFiles now returns files in path-ascending order (previous commit). This test's fixture seeded files as [src/a.ts, README.md] and asserted that exact (insertion) order, which only happened to match the old undefined query order by coincidence -- README.md sorts before src/a.ts alphabetically. Full unsharded suite confirmed this was the only test relying on the old order (12844 passed). --- test/unit/rag-index.test.ts | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/test/unit/rag-index.test.ts b/test/unit/rag-index.test.ts index f74421f7a1..62690f69a5 100644 --- a/test/unit/rag-index.test.ts +++ b/test/unit/rag-index.test.ts @@ -992,8 +992,10 @@ describe("merged-PR incremental re-index trigger (webhook)", () => { // REGRESSION (#rate-limit-admission-attribution): installationId must be threaded through so the queue's // admission check attributes this job to the repo's OWN installation bucket, not the shared public-token // bucket -- omitting it starves an installed repo's re-index behind unrelated public-token traffic. + // paths reflects listPullRequestFiles's deterministic path-ascending order (#linked-issue-satisfaction-cache-fingerprint-stability), + // not the seed/insertion order above -- "README.md" sorts before "src/a.ts". expect(ragJobs).toEqual([ - { type: "rag-index-repo", requestedBy: "webhook", repoFullName: "JSONbored/gittensory", paths: ["src/a.ts", "README.md"], installationId: 123 }, + { type: "rag-index-repo", requestedBy: "webhook", repoFullName: "JSONbored/gittensory", paths: ["README.md", "src/a.ts"], installationId: 123 }, ]); });