perf(review): skip duplicate full-file grounding fetch for modified files

src/review/grounding-wire.ts

@@ -101,9 +101,20 @@ export function buildCheckAggregate(checks: CheckSummaryRecord[]): CheckAggregat
   return { state, passing, failingDetails };
 }
 
-/** Map gittensory's PR file records to the subset the grounding engine reads (filename + status). */
+/** Map gittensory's PR file records to the subset the grounding engine reads (filename + status, plus the
+ *  patch/additions/deletions a MODIFIED file's diffFullyCoversFile check needs to skip a redundant fetch
+ *  when the diff already carries the whole file — see review-grounding.ts). */
 function toGroundingFiles(files: PullRequestFileRecord[]): PullRequestFile[] {
-  return files.map((file) => ({ filename: file.path, ...(file.status ? { status: file.status } : {}) }));
+  return files.map((file) => {
+    const patch = typeof file.payload?.patch === "string" ? file.payload.patch : undefined;
+    return {
+      filename: file.path,
+      ...(file.status ? { status: file.status } : {}),
+      ...(patch !== undefined ? { patch } : {}),
+      additions: file.additions,
+      deletions: file.deletions,
+    };
+  });
 }
 
 /**

src/review/review-grounding.ts

@@ -36,10 +36,16 @@ export interface ChangedFileContent {
   truncated?: boolean;
 }
 
-/** A changed file in the PR (subset of reviewbot's PullRequestFile that grounding reads). */
+/** A changed file in the PR (subset of reviewbot's PullRequestFile that grounding reads). `patch` +
+ *  `additions`/`deletions` are optional so a caller that hasn't wired them through still type-checks;
+ *  without them a modified file is simply never recognized as fully-covered-by-diff (falls back to
+ *  fetching, same as before -- see `diffFullyCoversFile`). */
 export interface PullRequestFile {
   filename: string;
   status?: string;
+  patch?: string;
+  additions?: number;
+  deletions?: number;
 }
 
 export interface ReviewGrounding {
@@ -124,6 +130,51 @@ export interface FileFetcher {
   getFileContent(path: string, ref: string, maxChars?: number): Promise<string | null>;
 }
 
+// ── Modified-file dedup (#3897 follow-up): a hunk-header check for "the diff already IS the whole file" ──
+
+/** Unified-diff hunk header: `@@ -oldStart,oldCount +newStart,newCount @@` (a bare number with no comma
+ *  means count 1, standard unified-diff shorthand). */
+const HUNK_HEADER = /^@@ -(\d+)(?:,(\d+))? \+(\d+)(?:,(\d+))? @@/;
+
+/** git's default unified-diff context window (`-U3`): the MAXIMUM number of unchanged lines it will ever
+ *  show around a change, whether or not more unchanged content follows. This makes it a strict ambiguity
+ *  boundary, not slack — a hunk carrying FEWER than this many trailing unchanged lines proves the file
+ *  truly ends there (git would never truncate below its own configured context), but a hunk carrying
+ *  EXACTLY this many is ambiguous: it could be the true end of file, or git could have capped a longer
+ *  unchanged tail at the context window. Only the unambiguous case may skip the fetch. */
+const DIFF_CONTEXT_LINES = 3;
+
+/**
+ * True when a MODIFIED file's diff hunk already PROVABLY covers its entire post-change body, making the
+ * separate full-file fetch a byte-for-byte duplicate of what the diff itself sent — the same waste #3918
+ * targeted for added files (since reverted for them by #3976, whose reasoning does NOT apply here: this
+ * check, unlike a blanket status==="added" skip, is proven per-file from the hunk math itself, not assumed
+ * from status alone). A single hunk starting at line 1 on both sides, whose unchanged-line count (old/new
+ * count minus deletions/additions) is STRICTLY BELOW `DIFF_CONTEXT_LINES`, cannot have any untouched tail:
+ * if more unchanged file remained, git's default context would have shown exactly `DIFF_CONTEXT_LINES`
+ * lines regardless, so seeing fewer proves there was nothing left to show (verified against real `git
+ * diff` output). Seeing exactly `DIFF_CONTEXT_LINES` is deliberately treated as NOT proven — the context
+ * window could be capping a longer tail — so this falls back to fetching, the safe default.
+ * Deliberately scoped to `status === "modified"` (or absent, for a caller that never sets it) only --
+ * "added"/"renamed"/other statuses fall through to `false` (the safe default: fetch the full file) so
+ * this can never re-skip an added file's fetch, which #3976 restored because GitHub can omit/truncate an
+ * added file's patch on large files without any hunk-count anomaly for this check to catch.
+ */
+export function diffFullyCoversFile(file: PullRequestFile): boolean {
+  if (file.status !== undefined && file.status !== "modified") return false;
+  if (!file.patch || file.additions === undefined || file.deletions === undefined) return false;
+  const hunks = file.patch.split("\n").filter((line) => line.startsWith("@@"));
+  if (hunks.length !== 1) return false; // multiple hunks ⇒ an unchanged (and unseen) gap sits between them
+  const match = HUNK_HEADER.exec(hunks[0]!);
+  if (!match) return false;
+  const oldStart = Number.parseInt(match[1]!, 10);
+  const oldCount = match[2] !== undefined ? Number.parseInt(match[2], 10) : 1;
+  const newStart = Number.parseInt(match[3]!, 10);
+  const newCount = match[4] !== undefined ? Number.parseInt(match[4], 10) : 1;
+  if (oldStart > 1 || newStart > 1) return false; // leading unchanged lines exist before the hunk
+  return oldCount - file.deletions < DIFF_CONTEXT_LINES && newCount - file.additions < DIFF_CONTEXT_LINES;
+}
+
 /** Centrally fetch the FULL post-change content of changed files (the one grounding input no lane fetches
  *  otherwise). Flag-gated + bounded + fully fail-safe — returns undefined when off or on any error. The
  *  caller passes the already-resolved head ref + a FileFetcher (vs reviewbot's RunContext/ReviewTarget). */
@@ -137,8 +188,11 @@ export async function fetchFullFileContents(
   // Source-first ordering (the diff's own priority) so the most-relevant files are inlined before the budget runs out.
   // Added files still need grounding: the review diff is budgeted and GitHub can omit inline patches for
   // large/binary-ish files, so the full-file fallback must not assume every added line reached the prompt.
+  // A MODIFIED file is excluded only when its own hunk header proves it already carries the whole file
+  // (see diffFullyCoversFile) -- a strictly narrower, provable case than "added", so it can't reintroduce
+  // the #3976 gap: anything less than full-hunk proof still falls through to the fetch.
   const candidates = files
-    .filter((file) => file.status !== "removed" && !SKIP_EXT.test(file.filename))
+    .filter((file) => file.status !== "removed" && !SKIP_EXT.test(file.filename) && !diffFullyCoversFile(file))
     .sort((a, b) => diffFilePriority(a.filename) - diffFilePriority(b.filename));
   const out: ChangedFileContent[] = [];
   let used = 0;

test/unit/grounding-wiring.test.ts

@@ -308,6 +308,21 @@ describe("review-grounding wired into the AI reviewer (flag GITTENSORY_REVIEW_GR
     fetchSpy.mockRestore();
   });
 
+  it("FLAG-ON: a file record with no status still grounds (toGroundingFiles' status field is optional)", async () => {
+    const env = createTestEnv({ GITTENSORY_REVIEW_GROUNDING: "true", GITHUB_PUBLIC_TOKEN: "ghp_test" });
+    const fetchSpy = vi.spyOn(globalThis, "fetch").mockResolvedValue(new Response("export const A = 1;", { status: 200 }));
+    const noStatusFile: PullRequestFileRecord = { repoFullName: "acme/widgets", pullNumber: 7, path: "src/a.ts", additions: 1, deletions: 0, changes: 1, payload: {} };
+    const out = await buildReviewGroundingText(env, {
+      repoFullName: "acme/widgets",
+      headSha: "sha7",
+      files: [noStatusFile],
+      checks: [check()],
+      installationId: null,
+    });
+    expect(out.promptSection).toContain("export const A = 1;");
+    fetchSpy.mockRestore();
+  });
+
   it("FLAG-ON fail-safe: a throwing fetch degrades to no file section (never throws), CI still grounds", async () => {
     const env = createTestEnv({ GITTENSORY_REVIEW_GROUNDING: "true" });
     const fetchSpy = vi.spyOn(globalThis, "fetch").mockRejectedValue(new Error("network down"));

test/unit/review-grounding.test.ts

@@ -2,6 +2,7 @@ import { describe, expect, it } from "vitest";
 import {
   buildGrounding,
   diffFilePriority,
+  diffFullyCoversFile,
   fetchFullFileContents,
   type FileFetcher,
   formatGroundingSections,
@@ -204,6 +205,110 @@ describe("review-grounding: fetchFullFileContents (injected FileFetcher, fail-sa
     expect(out).toEqual([{ path: "src/new.ts", text: "export const hidden = true;" }]);
   });
 
+  it("skips the full-file fetch for a MODIFIED file rewritten in one hunk that already covers it (#3897 follow-up)", async () => {
+    const reads: string[] = [];
+    const fetcher: FileFetcher = {
+      getFileContent: async (path) => {
+        reads.push(path);
+        return "SHOULD_NOT_FETCH";
+      },
+    };
+    const rewritten: PullRequestFile = {
+      filename: "src/rewritten.ts",
+      status: "modified",
+      patch: "@@ -1,5 +1,5 @@\n-old1\n-old2\n-old3\n-old4\n-old5\n+new1\n+new2\n+new3\n+new4\n+new5",
+      additions: 5,
+      deletions: 5,
+    };
+    const out = await fetchFullFileContents({ ciGrounding: false, fullFileContext: true }, "sha", [rewritten], fetcher);
+    // No fetch at all -- the hunk already carries every line of the file, so grounding would be a duplicate.
+    expect(reads).toEqual([]);
+    expect(out).toBeUndefined();
+  });
+
+  it("still fetches a MODIFIED file whose hunk only covers part of it (context proves an untouched tail)", async () => {
+    const reads: string[] = [];
+    const fetcher: FileFetcher = {
+      getFileContent: async (path) => {
+        reads.push(path);
+        return "export const full = 'post-change body';";
+      },
+    };
+    const partial: PullRequestFile = {
+      filename: "src/partial.ts",
+      status: "modified",
+      // Only the first 2 of 10 lines changed; git's default 3-line trailing context pulls 3 unchanged
+      // lines into the hunk, so oldCount/newCount (5) sit well above deletions/additions (2) -- proof
+      // real unchanged file content exists beyond the hunk.
+      patch: "@@ -1,5 +1,5 @@\n-old1\n-old2\n+new1\n+new2\n line3\n line4\n line5",
+      additions: 2,
+      deletions: 2,
+    };
+    const out = await fetchFullFileContents({ ciGrounding: false, fullFileContext: true }, "sha", [partial], fetcher);
+    expect(reads).toEqual(["src/partial.ts"]);
+    expect(out).toEqual([{ path: "src/partial.ts", text: "export const full = 'post-change body';" }]);
+  });
+
+  it("still fetches an ADDED file even when its single hunk shape would otherwise look fully-covering (status gate holds)", async () => {
+    const reads: string[] = [];
+    const fetcher: FileFetcher = {
+      getFileContent: async (path) => {
+        reads.push(path);
+        return "export const hidden = true;";
+      },
+    };
+    const added: PullRequestFile = {
+      filename: "src/new.ts",
+      status: "added",
+      patch: "@@ -0,0 +1,2 @@\n+line1\n+line2",
+      additions: 2,
+      deletions: 0,
+    };
+    const out = await fetchFullFileContents({ ciGrounding: false, fullFileContext: true }, "sha", [added], fetcher);
+    // diffFullyCoversFile is scoped to status "modified" -- an added file is still fetched unconditionally (#3976).
+    expect(reads).toEqual(["src/new.ts"]);
+    expect(out).toEqual([{ path: "src/new.ts", text: "export const hidden = true;" }]);
+  });
+
+  describe("diffFullyCoversFile", () => {
+    it("returns false for multiple hunks (an unseen gap sits between them)", () => {
+      expect(
+        diffFullyCoversFile({
+          filename: "src/multi.ts",
+          status: "modified",
+          patch: "@@ -1,2 +1,2 @@\n-a1\n+b1\n@@ -10,2 +10,2 @@\n-a2\n+b2",
+          additions: 2,
+          deletions: 2,
+        }),
+      ).toBe(false);
+    });
+
+    it("handles the bare single-line hunk header shorthand (no comma count)", () => {
+      // `@@ -1 +1 @@` means count 1 on both sides -- a one-line file rewritten in place.
+      expect(
+        diffFullyCoversFile({
+          filename: "src/one-line.ts",
+          status: "modified",
+          patch: "@@ -1 +1 @@\n-old\n+new",
+          additions: 1,
+          deletions: 1,
+        }),
+      ).toBe(true);
+    });
+
+    it("returns false when the hunk does not start at line 1 on either side (leading unchanged lines exist)", () => {
+      expect(
+        diffFullyCoversFile({
+          filename: "src/tail.ts",
+          status: "modified",
+          patch: "@@ -5,2 +5,2 @@\n-old\n+new",
+          additions: 1,
+          deletions: 1,
+        }),
+      ).toBe(false);
+    });
+  });
+
   it("degrades to skipping a file when the fetcher throws (never throws itself)", async () => {
     const fetcher: FileFetcher = {
       getFileContent: async (path) => {