diff --git a/src/review/review-grounding.ts b/src/review/review-grounding.ts index a9c3922af..8e5cef3b4 100644 --- a/src/review/review-grounding.ts +++ b/src/review/review-grounding.ts @@ -135,11 +135,10 @@ export async function fetchFullFileContents( ): Promise { if (!flags.fullFileContext || !ref) return undefined; // Source-first ordering (the diff's own priority) so the most-relevant files are inlined before the budget runs out. - // A newly ADDED file is excluded here (#3897): every line of it is already a `+` line in the diff itself - // (sent in the same prompt), so fetching + inlining its full content again is a byte-for-byte duplicate -- - // wasted GitHub API round-trip and wasted prompt budget that a genuinely modified file needs more. + // 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. const candidates = files - .filter((file) => file.status !== "removed" && file.status !== "added" && !SKIP_EXT.test(file.filename)) + .filter((file) => file.status !== "removed" && !SKIP_EXT.test(file.filename)) .sort((a, b) => diffFilePriority(a.filename) - diffFilePriority(b.filename)); const out: ChangedFileContent[] = []; let used = 0; diff --git a/test/unit/review-grounding.test.ts b/test/unit/review-grounding.test.ts index f4159ee33..8a637c89f 100644 --- a/test/unit/review-grounding.test.ts +++ b/test/unit/review-grounding.test.ts @@ -174,20 +174,36 @@ describe("review-grounding: fetchFullFileContents (injected FileFetcher, fail-sa ["assets/icon.heic"], ["dist/pkg.tgz"], ["old.ts", "removed"], - ["src/new.ts", "added"], ), fetcher, ); expect(out).toBeDefined(); - // source (priority 0) before docs (priority 2); binary + removed + added excluded before fetch + // source (priority 0) before docs (priority 2); binary + removed excluded before fetch expect(out?.map((f) => f.path)).toEqual(["src/a.ts", "README.md"]); expect(reads).toEqual(["src/a.ts", "README.md"]); - // #3897: an ADDED file's entire body is already every `+` line of the diff -- fetching it again - // would duplicate that content in the prompt, so it's excluded the same way "removed" is. - expect(reads).not.toContain("src/new.ts"); for (const path of binary) expect(reads).not.toContain(path); }); + it("fetches added files because the bounded diff may omit their content", async () => { + const reads: string[] = []; + const fetcher: FileFetcher = { + getFileContent: async (path) => { + reads.push(path); + return path === "src/new.ts" ? "export const hidden = true;" : null; + }, + }; + + const out = await fetchFullFileContents( + { ciGrounding: false, fullFileContext: true }, + "sha", + files(["src/new.ts", "added"]), + fetcher, + ); + + expect(reads).toEqual(["src/new.ts"]); + expect(out).toEqual([{ path: "src/new.ts", text: "export const hidden = true;" }]); + }); + it("degrades to skipping a file when the fetcher throws (never throws itself)", async () => { const fetcher: FileFetcher = { getFileContent: async (path) => { @@ -256,4 +272,31 @@ describe("review-grounding: fetchFullFileContents (injected FileFetcher, fail-sa // The over-budget file is NOT fetched — the budget guard short-circuits before the read. expect(reads).not.toContain("src/d.ts"); }); + + it("stays budget-bounded when EVERY file in the PR is newly added (no longer excluded from fetch)", async () => { + // Same budget-exhaustion shape as the test above, but every file carries status "added" -- proving + // that restoring added-file fetching doesn't bypass the shared FILE_CONTENT_BUDGET when a PR adds + // several large new files at once: the budget guard still trips per-file, not per-status. + const chunk = "z".repeat(20_000); // < MAX_SINGLE_FILE so each is individually inlinable + const map: Record = { "src/a.ts": chunk, "src/b.ts": chunk, "src/c.ts": chunk, "src/d.ts": chunk }; + const reads: string[] = []; + const fetcher: FileFetcher = { + getFileContent: async (path) => { + reads.push(path); + return map[path] ?? null; + }, + }; + const out = await fetchFullFileContents( + { ciGrounding: false, fullFileContext: true }, + "sha", + files(["src/a.ts", "added"], ["src/b.ts", "added"], ["src/c.ts", "added"], ["src/d.ts", "added"]), + fetcher, + ); + expect(out).toBeDefined(); + // First three fill the 60k budget exactly; the fourth trips the guard before it is fetched. + expect(out?.filter((f) => !f.truncated).map((f) => f.path)).toEqual(["src/a.ts", "src/b.ts", "src/c.ts"]); + const dEntry = out?.find((f) => f.path === "src/d.ts"); + expect(dEntry).toEqual({ path: "src/d.ts", text: "", truncated: true }); + expect(reads).not.toContain("src/d.ts"); + }); });