diff --git a/src/review/grounding-wire.ts b/src/review/grounding-wire.ts index 67e7271de1..ed9e925c77 100644 --- a/src/review/grounding-wire.ts +++ b/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, + }; + }); } /** diff --git a/src/review/review-grounding.ts b/src/review/review-grounding.ts index 8e5cef3b45..bb2a707ad6 100644 --- a/src/review/review-grounding.ts +++ b/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; } +// ── 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; diff --git a/test/unit/grounding-wiring.test.ts b/test/unit/grounding-wiring.test.ts index f225d09f1c..359c3068de 100644 --- a/test/unit/grounding-wiring.test.ts +++ b/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")); diff --git a/test/unit/review-grounding.test.ts b/test/unit/review-grounding.test.ts index 8a637c89fa..9751f51cc9 100644 --- a/test/unit/review-grounding.test.ts +++ b/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) => {