fix(review): cache grounding's full-file-content fetch by (repo, path, head SHA)

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)
+);

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<string | null> {
+  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<void> {
+  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<void> {
   const db = getDb(env.DB);
   await env.DB.prepare("DELETE FROM collision_edges WHERE repo_full_name = ?").bind(repoFullName).run();

src/db/schema.ts

@@ -1449,3 +1449,25 @@ 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(),
+    /* 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) => ({
+    primary: primaryKey({ columns: [table.repoFullName, table.path, table.headSha] }),
+  }),
+);

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,13 +136,20 @@ 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<string | null> {
+      // #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://github.com/@api/repos/${encodeURIComponent(owner)}/${encodeURIComponent(name)}/contents/${path
           .split("/")
           .map(encodeURIComponent)
           .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,18 @@ 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);
         }
+        /* 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);
+        }
+        return content;
       } catch {
         return null; // network / decode failure → skip this file (fail-safe)
       }

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";
@@ -377,6 +377,130 @@ 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<string, string> = { "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("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"));
@@ -569,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", () => {