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
13 changes: 12 additions & 1 deletion src/db/repositories.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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<PullRequestFileRecord[]> {
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);
}

Expand Down
29 changes: 29 additions & 0 deletions test/unit/backfill-file-hydration-scoping.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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));
});
});
});
4 changes: 3 additions & 1 deletion test/unit/rag-index.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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 },
]);
});

Expand Down