diff --git a/src/review/review-grounding.ts b/src/review/review-grounding.ts index 576f76d847..84b5e11b0f 100644 --- a/src/review/review-grounding.ts +++ b/src/review/review-grounding.ts @@ -307,13 +307,12 @@ export async function fetchFullFileContents( used += text.length; continue; } - const sampled = sampleHeadAndTail(text, share); - if (!sampled) { - // The remaining share was too thin for even a head+tail sample to carry signal — same as unreadable. - out.push({ path: file.filename, text: "", truncated: true }); - used = FILE_CONTENT_BUDGET; - continue; - } + // This file WAS genuinely fetched, so it must never render as the empty "(no content available)" + // placeholder a never-fetched file uses -- that breaks the module's own "never omitted again" + // guarantee (#8646). When the remaining share is thinner than MIN_SAMPLE_CHARS, sample at that floor so + // a fetched file always yields at least a minimal, distinguishing head+tail (a small, bounded overrun of + // the overall budget on the very last thin file, not a per-file unbounded cost). + const sampled = sampleHeadAndTail(text, Math.max(share, MIN_SAMPLE_CHARS)); out.push({ path: file.filename, text: sampled, truncated: true }); used += sampled.length; } diff --git a/test/unit/review-grounding.test.ts b/test/unit/review-grounding.test.ts index 4b47a6e35f..83f545547b 100644 --- a/test/unit/review-grounding.test.ts +++ b/test/unit/review-grounding.test.ts @@ -545,10 +545,12 @@ describe("review-grounding: fetchFullFileContents (injected FileFetcher, fail-sa expect(after).toEqual({ path: "src/after.ts", text: "ok" }); }); - it("falls all the way back to full omission when the remaining share is too thin for even a sample", async () => { - // Two fillers each just under MAX_SINGLE_FILE leave only 200 chars of the 96k budget for the third file - // -- below MIN_SAMPLE_CHARS, so sampleHeadAndTail itself declines rather than rendering a garbled sliver, - // and fetchFullFileContents degrades that to the same full-omission shape as an unreadable file. + it("still yields a minimal distinguishing sample for a genuinely-fetched file under extreme budget pressure (#8646)", async () => { + // Two fillers each just under MAX_SINGLE_FILE leave only ~200 chars of the 96k budget for the third file -- + // below MIN_SAMPLE_CHARS. Previously fetchFullFileContents degraded that fetched file to the same empty + // { text: "", truncated: true } shape a NEVER-fetched file uses, breaking the module's "never rendered as + // omitted again" guarantee. It must now sample at the MIN_SAMPLE_CHARS floor so a fetched file always + // carries at least some distinguishing real content. const filler = "f".repeat(MAX_SINGLE_FILE - 100); const map: Record = { "src/a.ts": filler, "src/b.ts": filler, "src/huge.ts": "z".repeat(1_000_000) }; const fetcher: FileFetcher = { getFileContent: async (path) => map[path] ?? null }; @@ -559,7 +561,12 @@ describe("review-grounding: fetchFullFileContents (injected FileFetcher, fail-sa fetcher, ); const huge = out?.find((f) => f.path === "src/huge.ts"); - expect(huge).toEqual({ path: "src/huge.ts", text: "", truncated: true }); + // The fetched file is truncated but NOT empty -- it carries real sampled bytes + the omission marker, so + // its rendered output is distinguishable from a never-fetched file's empty placeholder. + expect(huge?.truncated).toBe(true); + expect(huge?.text.length).toBeGreaterThan(0); + expect(huge?.text).toContain("omitted from the middle of this file"); + expect(huge?.text).toContain("z"); // genuine content from the fetched file, not just the marker }); it("returns undefined when nothing readable was inlined", async () => {