Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion src/review/review-grounding.ts
Original file line number Diff line number Diff line change
Expand Up @@ -135,8 +135,11 @@ export async function fetchFullFileContents(
): Promise<ChangedFileContent[] | undefined> {
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.
const candidates = files
.filter((file) => file.status !== "removed" && !SKIP_EXT.test(file.filename))
.filter((file) => file.status !== "removed" && file.status !== "added" && !SKIP_EXT.test(file.filename))
.sort((a, b) => diffFilePriority(a.filename) - diffFilePriority(b.filename));
const out: ChangedFileContent[] = [];
let used = 0;
Expand Down
6 changes: 5 additions & 1 deletion test/unit/review-grounding.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -174,13 +174,17 @@ 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 excluded before fetch
// source (priority 0) before docs (priority 2); binary + removed + added 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);
});

Expand Down