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..a2f688c90a 100644 --- a/src/db/schema.ts +++ b/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] }), + }), +); diff --git a/src/review/grounding-wire.ts b/src/review/grounding-wire.ts index ed9e925c77..9dcc7fe611 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,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) } diff --git a/test/unit/grounding-wiring.test.ts b/test/unit/grounding-wiring.test.ts index 72b5b4baad..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"; @@ -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 = { "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", () => {