From 4e74a4814a19d68dbe00625c5796269155ecc634 Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Thu, 9 Jul 2026 16:35:38 -0700 Subject: [PATCH 1/2] fix(review): cache grounding's full-file-content fetch by (repo, path, head SHA) makeGithubFileFetcher re-fetched every changed file's full post-change body from GitHub on every invocation with zero caching. Grounding bypasses the durable ai_review result cache (a dynamic feature), so the only throttle was the 30-minute non-cacheable cooldown -- after which, or on any non-push webhook event (review comments, review submissions, thread resolutions -- none of which change the head SHA), the full multi-file re-fetch repeated from scratch. Adds grounding_file_content_cache, keyed by (repo, path, head_sha) without a pull number (file content at an immutable commit is universal, not PR-specific). Only a genuinely successful fetch is cached -- a transient network/timeout failure stays retryable rather than being treated as a confirmed-permanent condition. Closes #4499. --- .../0130_grounding_file_content_cache.sql | 16 ++++ src/db/repositories.ts | 41 +++++++++ src/db/schema.ts | 19 ++++ src/review/grounding-wire.ts | 15 +++- test/unit/grounding-wiring.test.ts | 86 +++++++++++++++++++ 5 files changed, 175 insertions(+), 2 deletions(-) create mode 100644 migrations/0130_grounding_file_content_cache.sql diff --git a/migrations/0130_grounding_file_content_cache.sql b/migrations/0130_grounding_file_content_cache.sql new file mode 100644 index 0000000000..092cfd108b --- /dev/null +++ b/migrations/0130_grounding_file_content_cache.sql @@ -0,0 +1,16 @@ +-- Grounding file-content cache (#4499): makeGithubFileFetcher re-fetches every changed file's FULL post-change +-- body from GitHub on every invocation with zero caching -- content for a given (repo, path, head_sha) triple +-- is a git blob at an immutable commit, so it genuinely never changes and is safe to cache durably, not just +-- with a short TTL, mirroring linked_issue_satisfaction_cache (migration 0124). Keyed WITHOUT a pull number +-- (unlike that cache): file content at a given head SHA is universal, not PR-specific, so two PRs that happen +-- to share a (repo, path, head_sha) triple (e.g. a cherry-pick) correctly share one cached row. Only a +-- SUCCESSFUL fetch is ever stored -- a transient network/timeout failure must not be cached as if it were a +-- confirmed-permanent condition (binary/oversized/inaccessible), or a later retry would wrongly skip forever. +CREATE TABLE IF NOT EXISTS grounding_file_content_cache ( + repo_full_name TEXT NOT NULL, + path TEXT NOT NULL, + head_sha TEXT NOT NULL, + content TEXT NOT NULL, + fetched_at TEXT NOT NULL DEFAULT CURRENT_TIMESTAMP, + PRIMARY KEY (repo_full_name, path, head_sha) +); diff --git a/src/db/repositories.ts b/src/db/repositories.ts index 35960f6daf..69207eaa82 100644 --- a/src/db/repositories.ts +++ b/src/db/repositories.ts @@ -4699,6 +4699,47 @@ export async function putCachedLinkedIssueSatisfaction( .run(); } +/** #4499 (grounding-file-content-cache): the stored file content for (repo, path, head SHA), or null on a + * miss. Unlike linked_issue_satisfaction_cache, every stored row is durable with NO input-fingerprint + * dimension -- file content at an immutable head SHA has exactly one correct value, so a hit is always safe + * to reuse verbatim. A nullish head SHA is always a miss (mirrors the sibling caches' contract). */ +export async function getCachedGroundingFileContent( + env: Env, + repoFullName: string, + path: string, + headSha: string | null | undefined, +): Promise { + if (!headSha) return null; + const row = await env.DB + .prepare("SELECT content FROM grounding_file_content_cache WHERE repo_full_name = ? AND path = ? AND head_sha = ?") + .bind(repoFullName, path, headSha) + .first<{ content: string }>(); + return row?.content ?? null; +} + +/** #4499 (grounding-file-content-cache): upsert the fetched file content for (repo, path, head SHA). A + * nullish head SHA is a no-op (mirrors the sibling caches). The caller is responsible for only calling this + * with a genuinely fetched, non-null content string -- never a fetch failure/skip, which must stay retryable + * rather than being cached as if it were a confirmed-permanent binary/oversized/inaccessible condition. */ +export async function putCachedGroundingFileContent( + env: Env, + repoFullName: string, + path: string, + headSha: string | null | undefined, + content: string, +): Promise { + if (!headSha) return; + await env.DB + .prepare( + `INSERT INTO grounding_file_content_cache (repo_full_name, path, head_sha, content, fetched_at) + VALUES (?, ?, ?, ?, ?) + ON CONFLICT(repo_full_name, path, head_sha) DO UPDATE SET + content = excluded.content, fetched_at = excluded.fetched_at`, + ) + .bind(repoFullName, path, headSha, content, nowIso()) + .run(); +} + export async function replaceCollisionEdges(env: Env, repoFullName: string, edges: CollisionEdgeRecord[]): Promise { const db = getDb(env.DB); await env.DB.prepare("DELETE FROM collision_edges WHERE repo_full_name = ?").bind(repoFullName).run(); diff --git a/src/db/schema.ts b/src/db/schema.ts index 9d70fdc523..18c63a45f7 100644 --- a/src/db/schema.ts +++ b/src/db/schema.ts @@ -1449,3 +1449,22 @@ export const linkedIssueSatisfactionCache = sqliteTable( primary: primaryKey({ columns: [table.repoFullName, table.pullNumber, table.headSha, table.linkedIssueNumber] }), }), ); + +// Grounding file-content cache (#4499): makeGithubFileFetcher re-fetches every changed file's full post-change +// body from GitHub with zero caching; content for a given (repo, path, headSha) triple is a git blob at an +// immutable commit, so it's safe to cache durably. NOT scoped to pullNumber (unlike linkedIssueSatisfactionCache +// above) -- file content at a given head SHA is universal, not PR-specific. Only a successful fetch is ever +// stored; a transient failure must never be cached as if it were a confirmed-permanent one. +export const groundingFileContentCache = sqliteTable( + "grounding_file_content_cache", + { + repoFullName: text("repo_full_name").notNull(), + path: text("path").notNull(), + headSha: text("head_sha").notNull(), + content: text("content").notNull(), + fetchedAt: text("fetched_at").notNull().$defaultFn(() => nowIso()), + }, + (table) => ({ + primary: primaryKey({ columns: [table.repoFullName, table.path, table.headSha] }), + }), +); diff --git a/src/review/grounding-wire.ts b/src/review/grounding-wire.ts index ed9e925c77..bbbde0a125 100644 --- a/src/review/grounding-wire.ts +++ b/src/review/grounding-wire.ts @@ -13,6 +13,7 @@ import { createInstallationToken } from "../github/app"; import { githubRateLimitAdmissionKeyForInstallation, timeoutFetch, type GitHubRateLimitAdmissionKey } from "../github/client"; +import { getCachedGroundingFileContent, putCachedGroundingFileContent } from "../db/repositories"; import type { CheckSummaryRecord, PullRequestFileRecord } from "../types"; import { repoParts } from "../utils/json"; import { isConvergenceRepoAllowed } from "./cutover-gate"; @@ -135,6 +136,12 @@ export async function makeGithubFileFetcher(env: Env, repoFullName: string, inst const { owner, name } = repoParts(repoFullName); return { async getFileContent(path: string, ref: string, maxChars = 24_001): Promise { + // #4499: content for a given (repo, path, ref) is a git blob at an immutable commit -- it never changes, + // so a cache hit is always safe to reuse verbatim, skipping the GitHub call entirely. Checked BEFORE the + // network fetch below; only a genuinely successful fetch is ever written back (see the .catch-free write + // after the try block), so a transient failure is never mistaken for a confirmed-permanent one. + const cached = await getCachedGroundingFileContent(env, repoFullName, path, ref).catch(() => null); + if (cached !== null) return cached; try { const url = `https://api.github.com/repos/${encodeURIComponent(owner)}/${encodeURIComponent(name)}/contents/${path .split("/") @@ -142,6 +149,7 @@ export async function makeGithubFileFetcher(env: Env, repoFullName: string, inst .join("/")}?ref=${encodeURIComponent(ref)}`; const controller = new AbortController(); const timeout = setTimeout(() => controller.abort(), 10_000); + let content: string | null; try { const response = await timeoutFetch(url, { signal: controller.signal, @@ -157,11 +165,14 @@ export async function makeGithubFileFetcher(env: Env, repoFullName: string, inst }); if (!response.ok) return null; const contentLength = response.headers.get("content-length"); - if (contentLength && Number(contentLength) > maxChars) return " ".repeat(maxChars + 1); - return await readTextWithLimit(response, maxChars); + content = contentLength && Number(contentLength) > maxChars ? " ".repeat(maxChars + 1) : await readTextWithLimit(response, maxChars); } finally { clearTimeout(timeout); } + if (content !== null) { + await putCachedGroundingFileContent(env, repoFullName, path, ref, content).catch(() => undefined); + } + return content; } catch { return null; // network / decode failure → skip this file (fail-safe) } diff --git a/test/unit/grounding-wiring.test.ts b/test/unit/grounding-wiring.test.ts index 72b5b4baad..2158ae1df9 100644 --- a/test/unit/grounding-wiring.test.ts +++ b/test/unit/grounding-wiring.test.ts @@ -377,6 +377,92 @@ describe("makeGithubFileFetcher (GitHub Contents-API-backed FileFetcher)", () => fetchSpy.mockRestore(); }); + it("INVARIANT (#4499): a second getFileContent call for the SAME (repo, path, ref) makes ZERO additional GitHub fetches, reusing the cached content", async () => { + const env = createTestEnv({ GITHUB_PUBLIC_TOKEN: "ghp_test" }); + let fetchCount = 0; + const fetchSpy = vi.spyOn(globalThis, "fetch").mockImplementation(async (url) => { + const u = String(url); + if (u.includes("/contents/cached.ts")) { + fetchCount += 1; + return new Response("export const cached = true;", { status: 200 }); + } + return new Response("missing", { status: 404 }); + }); + // A brand-new fetcher instance each time -- mirrors a fresh review pass creating its own + // makeGithubFileFetcher via a NEW GitHub App token, while sharing the SAME durable DB. + const first = await (await makeGithubFileFetcher(env, "acme/widgets", null)).getFileContent("cached.ts", "sha7"); + const second = await (await makeGithubFileFetcher(env, "acme/widgets", null)).getFileContent("cached.ts", "sha7"); + expect(first).toBe("export const cached = true;"); + expect(second).toBe("export const cached = true;"); + expect(fetchCount).toBe(1); + fetchSpy.mockRestore(); + }); + + it("REGRESSION (#4499, grounding-refetch incident): repeated cooldown-driven calls on an unchanged head SHA only fetch once total, not once per call", async () => { + const env = createTestEnv({ GITHUB_PUBLIC_TOKEN: "ghp_test" }); + let fetchCount = 0; + const fetchSpy = vi.spyOn(globalThis, "fetch").mockImplementation(async (url) => { + const u = String(url); + if (u.includes("/contents/repeat.ts")) { + fetchCount += 1; + return new Response("export const repeat = 1;", { status: 200 }); + } + return new Response("missing", { status: 404 }); + }); + // Simulates 5 separate review passes for the SAME unchanged PR head (e.g. non-push webhook events, or + // scheduled sweep ticks past the 30-minute non-cacheable cooldown) -- previously each one re-fetched the + // full file body from GitHub from scratch. + for (let i = 0; i < 5; i += 1) { + const fetcher = await makeGithubFileFetcher(env, "acme/widgets", null); + // eslint-disable-next-line no-await-in-loop -- sequential passes, mirroring separate review invocations + const content = await fetcher.getFileContent("repeat.ts", "unchanged-sha"); + expect(content).toBe("export const repeat = 1;"); + } + expect(fetchCount).toBe(1); + fetchSpy.mockRestore(); + }); + + it("a genuinely NEW head SHA still triggers a fresh fetch (the cache never masks a real code change)", async () => { + const env = createTestEnv({ GITHUB_PUBLIC_TOKEN: "ghp_test" }); + const responses: Record = { "sha-old": "export const v = 1;", "sha-new": "export const v = 2;" }; + let fetchCount = 0; + const fetchSpy = vi.spyOn(globalThis, "fetch").mockImplementation(async (url) => { + const u = String(url); + const shaMatch = /ref=(sha-\w+)/.exec(u); + const sha = shaMatch?.[1]; + if (u.includes("/contents/changed.ts") && sha && sha in responses) { + fetchCount += 1; + return new Response(responses[sha], { status: 200 }); + } + return new Response("missing", { status: 404 }); + }); + const first = await (await makeGithubFileFetcher(env, "acme/widgets", null)).getFileContent("changed.ts", "sha-old"); + const second = await (await makeGithubFileFetcher(env, "acme/widgets", null)).getFileContent("changed.ts", "sha-new"); + expect(first).toBe("export const v = 1;"); + expect(second).toBe("export const v = 2;"); + expect(fetchCount).toBe(2); + fetchSpy.mockRestore(); + }); + + it("a failed fetch (non-OK response) is never cached, so a later retry still attempts a fresh fetch", async () => { + const env = createTestEnv({ GITHUB_PUBLIC_TOKEN: "ghp_test" }); + let attempt = 0; + const fetchSpy = vi.spyOn(globalThis, "fetch").mockImplementation(async (url) => { + const u = String(url); + if (u.includes("/contents/flaky.ts")) { + attempt += 1; + return attempt === 1 ? new Response("server error", { status: 500 }) : new Response("export const recovered = true;", { status: 200 }); + } + return new Response("missing", { status: 404 }); + }); + const first = await (await makeGithubFileFetcher(env, "acme/widgets", null)).getFileContent("flaky.ts", "sha7"); + const second = await (await makeGithubFileFetcher(env, "acme/widgets", null)).getFileContent("flaky.ts", "sha7"); + expect(first).toBeNull(); + expect(second).toBe("export const recovered = true;"); + expect(attempt).toBe(2); + fetchSpy.mockRestore(); + }); + it("never throws — a fetch rejection resolves to null", async () => { const env = createTestEnv(); const fetchSpy = vi.spyOn(globalThis, "fetch").mockRejectedValue(new Error("boom")); From f1f6d682dd0daf01f0ab471cbe41bb83eff7ceb1 Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Thu, 9 Jul 2026 16:58:54 -0700 Subject: [PATCH 2/2] test(review): close branch-coverage gaps on the grounding file-content cache Adds direct tests for getCachedGroundingFileContent/putCachedGroundingFileContent's nullish-headSha no-op paths, and for the cache read/write fail-safe .catch() handlers (a throwing DB read/write must never block the underlying file fetch). Documents two genuinely-unreachable-via-the-real-code-path branches (a Drizzle-query-builder-only schema default never exercised by the raw-SQL write path, and readTextWithLimit's defensive-only null return type) with v8 ignore comments explaining why. --- src/db/schema.ts | 3 ++ src/review/grounding-wire.ts | 4 ++ test/unit/grounding-wiring.test.ts | 65 +++++++++++++++++++++++++++++- 3 files changed, 71 insertions(+), 1 deletion(-) diff --git a/src/db/schema.ts b/src/db/schema.ts index 18c63a45f7..a2f688c90a 100644 --- a/src/db/schema.ts +++ b/src/db/schema.ts @@ -1462,6 +1462,9 @@ export const groundingFileContentCache = sqliteTable( path: text("path").notNull(), headSha: text("head_sha").notNull(), content: text("content").notNull(), + /* v8 ignore next -- this default only fires for a Drizzle query-builder insert omitting fetchedAt; + * putCachedGroundingFileContent always writes via raw SQL with an explicit fetched_at value, so this + * callback is never actually invoked by the real code path (defensive schema-level default only). */ fetchedAt: text("fetched_at").notNull().$defaultFn(() => nowIso()), }, (table) => ({ diff --git a/src/review/grounding-wire.ts b/src/review/grounding-wire.ts index bbbde0a125..9dcc7fe611 100644 --- a/src/review/grounding-wire.ts +++ b/src/review/grounding-wire.ts @@ -169,6 +169,10 @@ export async function makeGithubFileFetcher(env: Env, repoFullName: string, inst } finally { clearTimeout(timeout); } + /* v8 ignore next -- readTextWithLimit's `string | null` return type is defensive; both of its actual + * return paths (text.slice(...) / text) always produce a string, never null, so this guard's false + * side is unreachable via the current implementation. Kept so a future readTextWithLimit change that + * legitimately returns null can never get cached as if it were real fetched content. */ if (content !== null) { await putCachedGroundingFileContent(env, repoFullName, path, ref, content).catch(() => undefined); } diff --git a/test/unit/grounding-wiring.test.ts b/test/unit/grounding-wiring.test.ts index 2158ae1df9..6f18746b48 100644 --- a/test/unit/grounding-wiring.test.ts +++ b/test/unit/grounding-wiring.test.ts @@ -8,7 +8,7 @@ import { isGroundingEnabled, makeGithubFileFetcher, } from "../../src/review/grounding-wire"; -import { upsertCheckSummary, upsertRepositoryFromGitHub } from "../../src/db/repositories"; +import { getCachedGroundingFileContent, putCachedGroundingFileContent, upsertCheckSummary, upsertRepositoryFromGitHub } from "../../src/db/repositories"; import * as githubApp from "../../src/github/app"; import { githubRateLimitAdmissionKeyForInstallation, latestGitHubRestRateLimitObservation } from "../../src/github/client"; import type { Advisory, CheckSummaryRecord, JsonValue, PullRequestFileRecord, RepositorySettings } from "../../src/types"; @@ -463,6 +463,44 @@ describe("makeGithubFileFetcher (GitHub Contents-API-backed FileFetcher)", () => fetchSpy.mockRestore(); }); + it("a throwing cache READ degrades to a fresh live fetch (fail-safe, never blocks the file fetch)", async () => { + const env = createTestEnv({ GITHUB_PUBLIC_TOKEN: "ghp_test" }); + const prepareSpy = vi.spyOn(env.DB, "prepare").mockImplementation(() => { + throw new Error("cache read boom"); + }); + const fetchSpy = vi.spyOn(globalThis, "fetch").mockResolvedValue(new Response("export const ok = true;", { status: 200 })); + try { + const fetcher = await makeGithubFileFetcher(env, "acme/widgets", null); + expect(await fetcher.getFileContent("cacheread.ts", "sha7")).toBe("export const ok = true;"); + } finally { + prepareSpy.mockRestore(); + fetchSpy.mockRestore(); + } + }); + + it("a throwing cache WRITE is swallowed (fail-safe) -- the fetched content is still returned even though it couldn't be cached", async () => { + const env = createTestEnv({ GITHUB_PUBLIC_TOKEN: "ghp_test" }); + const realPrepare = env.DB.prepare.bind(env.DB); + const prepareSpy = vi.spyOn(env.DB, "prepare").mockImplementation((sql: string) => { + if (/INSERT INTO grounding_file_content_cache/i.test(sql)) throw new Error("cache write boom"); + return realPrepare(sql); + }); + // A fresh Response each call -- mockResolvedValue would reuse ONE Response instance across both calls, and + // a Response body can only be read once, which would make the second call's read return empty regardless + // of caching behavior. + const fetchSpy = vi.spyOn(globalThis, "fetch").mockImplementation(async () => new Response("export const ok = true;", { status: 200 })); + try { + const fetcher = await makeGithubFileFetcher(env, "acme/widgets", null); + expect(await fetcher.getFileContent("cachewrite.ts", "sha7")).toBe("export const ok = true;"); + // The write failed, so a SECOND call must still fetch live rather than (incorrectly) finding a cached row. + expect(await fetcher.getFileContent("cachewrite.ts", "sha7")).toBe("export const ok = true;"); + expect(fetchSpy).toHaveBeenCalledTimes(2); + } finally { + prepareSpy.mockRestore(); + fetchSpy.mockRestore(); + } + }); + it("never throws — a fetch rejection resolves to null", async () => { const env = createTestEnv(); const fetchSpy = vi.spyOn(globalThis, "fetch").mockRejectedValue(new Error("boom")); @@ -655,6 +693,31 @@ describe("makeGithubFileFetcher (GitHub Contents-API-backed FileFetcher)", () => }); }); +// ── getCachedGroundingFileContent / putCachedGroundingFileContent (#4499) ─────────────────────────── + +describe("grounding_file_content_cache repository helpers", () => { + it("getCachedGroundingFileContent is a miss for a nullish head SHA, without touching the DB", async () => { + const env = createTestEnv(); + expect(await getCachedGroundingFileContent(env, "acme/widgets", "src/a.ts", null)).toBeNull(); + expect(await getCachedGroundingFileContent(env, "acme/widgets", "src/a.ts", undefined)).toBeNull(); + }); + + it("putCachedGroundingFileContent is a no-op for a nullish head SHA -- a later real-headSha read still misses", async () => { + const env = createTestEnv(); + await putCachedGroundingFileContent(env, "acme/widgets", "src/a.ts", null, "should not be stored"); + await putCachedGroundingFileContent(env, "acme/widgets", "src/a.ts", undefined, "should not be stored either"); + expect(await getCachedGroundingFileContent(env, "acme/widgets", "src/a.ts", "sha7")).toBeNull(); + }); + + it("round-trips a genuinely stored value for a real (repo, path, head SHA), and a write overwrites an existing row for the SAME key", async () => { + const env = createTestEnv(); + await putCachedGroundingFileContent(env, "acme/widgets", "src/a.ts", "sha7", "export const a = 1;"); + expect(await getCachedGroundingFileContent(env, "acme/widgets", "src/a.ts", "sha7")).toBe("export const a = 1;"); + await putCachedGroundingFileContent(env, "acme/widgets", "src/a.ts", "sha7", "export const a = 2; // updated"); + expect(await getCachedGroundingFileContent(env, "acme/widgets", "src/a.ts", "sha7")).toBe("export const a = 2; // updated"); + }); +}); + // ── checkSummaryText empty fallback + outer-catch fail-safe ───────────────────────────────────────── describe("buildCheckAggregate / buildReviewGroundingText edge branches", () => {