diff --git a/src/review/review-diff.ts b/src/review/review-diff.ts index 8736ba3b9..bda991a55 100644 --- a/src/review/review-diff.ts +++ b/src/review/review-diff.ts @@ -101,29 +101,54 @@ function splitHunks(patch: string): string[] { * loses boilerplate/context, instead of a blind head-slice that drops whatever is at the tail. Kept * hunks are emitted in original order so the diff still reads top-to-bottom. */ +const TRUNCATED_NOTICE = "… (this file's diff truncated)"; +/** The "\n" + notice appended when hunks are dropped; its length is RESERVED before selecting hunks so the + * combined string fits `budget` (#10017). */ +const droppedNotice = (dropped: number): string => `\n… (${dropped} lower-signal hunk(s) dropped)`; + export function keepHighSignalHunks(patch: string, budget: number): string { - if (budget <= 0) return "… (this file's diff truncated)"; + if (budget <= 0) return TRUNCATED_NOTICE; const hunks = splitHunks(patch); if (hunks.length <= 1) { - return patch.length > budget ? `${patch.slice(0, budget)}\n… (this file's diff truncated)` : patch; + // Reserve the notice's length so `slice + notice` fits budget (#10017): slice to budget - notice.length, + // never below 0. If even the notice does not fit, return just the (truncated) notice. + if (patch.length <= budget) return patch; + const room = budget - (TRUNCATED_NOTICE.length + 1); // +1 for the "\n" before the notice + return room > 0 ? `${patch.slice(0, room)}\n${TRUNCATED_NOTICE}` : TRUNCATED_NOTICE.slice(0, budget); } const ranked = hunks.map((h, i) => ({ i, len: h.length, sig: addedLineCount(h) })).sort((a, b) => b.sig - a.sig); - const keep = new Set(); - let used = 0; + // A dropped-hunk notice is emitted only when something is actually dropped, so budget for the whole set FIRST + // without reserving it; if all hunks fit, no notice is needed and the join alone fits (#10017). `chosen` + // preserves ranked (signal) order so the lowest-signal kept hunk is the one trimmed if the notice must fit. + const fits = (hunkCount: number, chars: number, notice: number): boolean => chars + Math.max(0, hunkCount - 1) + notice <= budget; + const chosen: number[] = []; + let usedChars = 0; for (const r of ranked) { - // Kept hunks are emitted with `.join("\n")` below — N hunks use N-1 separators — so charge the - // separator only for hunks AFTER the first. Charging `+ 1` for every hunk over-counted the output by - // one and dropped a hunk that fit exactly at the budget boundary. - const sep = keep.size > 0 ? 1 : 0; - if (used + r.len + sep > budget) continue; - keep.add(r.i); - used += r.len + sep; + if (!fits(chosen.length + 1, usedChars + r.len, 0)) continue; + chosen.push(r.i); + usedChars += r.len; + } + let droppedCount = hunks.length - chosen.length; + if (chosen.length > 0 && droppedCount > 0) { + // Some hunks dropped: the notice now counts. Trim lowest-signal kept hunks until kept + notice fits. + while (chosen.length > 1 && !fits(chosen.length, usedChars, droppedNotice(droppedCount + 1).length)) { + const removed = chosen.pop()!; // ranked order → this is the lowest-signal kept hunk + usedChars -= hunks[removed]!.length; + droppedCount += 1; + } + } + if (chosen.length === 0 || (chosen.length === 1 && !fits(1, usedChars, droppedNotice(droppedCount).length))) { + // Not even one whole hunk fits alongside the notice: return the highest-signal hunk's CONTENT truncated to + // fit (with the notice) -- same shape as the single-hunk path, never nothing, never over budget (#10017). + const top = ranked[0]!; + const notice = droppedNotice(hunks.length - 1); + const room = budget - notice.length; + return room > 0 ? `${hunks[top.i]!.slice(0, room)}${notice}` : notice.slice(0, budget); } - const top = ranked[0]; - if (keep.size === 0 && top) keep.add(top.i); // always keep the single highest-signal hunk - const dropped = hunks.length - keep.size; - const kept = hunks.filter((_, i) => keep.has(i)).join("\n"); - return dropped > 0 ? `${kept}\n… (${dropped} lower-signal hunk(s) dropped)` : kept; + // Emit the kept hunks in FILE order (as the original filter+join did), independent of the signal ranking + // used only for selection/trimming above. + const kept = [...chosen].sort((a, b) => a - b).map((i) => hunks[i]!).join("\n"); + return droppedCount > 0 ? `${kept}${droppedNotice(droppedCount)}` : kept; } /** A changed file, shape-agnostic so any caller's file record can map into it. The explicit `| undefined` @@ -147,13 +172,16 @@ export function buildUnifiedReviewDiff(files: ReviewDiffFile[], budget: number = const ordered = [...files].sort( (a, b) => diffFilePriority(a.path) - diffFilePriority(b.path) || addedLineCount(b.patch) - addedLineCount(a.patch), ); + const truncationNotice = `### …diff truncated (${files.length} files total)\n`; let diff = ""; for (const file of ordered) { const status = file.status ?? "modified"; const header = `### ${file.path} (${status}) +${file.additions ?? 0}/-${file.deletions ?? 0}\n`; const remaining = budget - diff.length; if (remaining < 240) { - diff += `### …diff truncated (${files.length} files total)\n`; + // The 240 floor comfortably exceeds this notice's length, and each file's body budget already reserved it + // (see the `- truncationNotice.length` below), so appending it here keeps the total within budget (#10017). + diff += truncationNotice; break; } if (!file.patch) { @@ -162,8 +190,10 @@ export function buildUnifiedReviewDiff(files: ReviewDiffFile[], budget: number = } let body = file.patch; if (header.length + body.length + 2 > remaining) { - // Hunk-aware: keep the highest-signal hunks that fit rather than a blind head-slice. - body = keepHighSignalHunks(file.patch, remaining - header.length - 4); + // Hunk-aware: keep the highest-signal hunks that fit rather than a blind head-slice. Reserve the + // truncation notice's length too, so if a LATER file triggers the break there is always room to append + // it without the total exceeding budget (#10017). + body = keepHighSignalHunks(file.patch, remaining - header.length - 4 - truncationNotice.length); } diff += `${header}${body}\n\n`; } diff --git a/test/unit/review-diff.test.ts b/test/unit/review-diff.test.ts index 02ab5efa9..21cb9fc02 100644 --- a/test/unit/review-diff.test.ts +++ b/test/unit/review-diff.test.ts @@ -148,12 +148,23 @@ describe("buildUnifiedReviewDiff — the #1528 fix: never silently drop the file it("keeps every hunk when they fit exactly (the join uses N-1 separators, not N)", () => { // Two 10-char hunks joined with one "\n" = 21 chars, exactly the budget. Charging a separator for - // BOTH hunks over-counts by one and wrongly drops the second even though it fits. + // BOTH hunks over-counts by one and wrongly drops the second even though it fits. No hunk is dropped, so + // no notice is emitted and the exact patch survives. const patch = "@@ a\n+x\n+y\n@@ b\n+p\n+q"; expect(patch.length).toBe(21); - expect(keepHighSignalHunks(patch, 21)).toBe(patch); // no hunk dropped - // One char short → the second hunk genuinely does not fit and is announced as dropped. - expect(keepHighSignalHunks(patch, 20)).toContain("dropped"); + expect(keepHighSignalHunks(patch, 21)).toBe(patch); // no hunk dropped, no notice + }); + + it("keeps kept-hunks + the dropped notice within budget when a hunk is dropped (#10017)", () => { + // A high-signal hunk and a bigger low-signal one. The budget fits the high-signal hunk AND the dropped + // notice, but not both hunks -- the returned string (kept + notice) must stay within budget. + const highSignal = "@@ hi\n+a\n+b\n+c"; + const lowSignal = `@@ lo\n${" ctx".repeat(30)}`; + const budget = highSignal.length + 40; // room for the high-signal hunk + the ~35-char notice + const out = keepHighSignalHunks(`${lowSignal}\n${highSignal}`, budget); + expect(out.length).toBeLessThanOrEqual(budget); + expect(out).toContain("+a"); // the high-signal hunk survives + expect(out).toContain("dropped"); }); }); @@ -166,15 +177,56 @@ describe("keepHighSignalHunks — non-positive budget guard (#5849)", () => { expect(keepHighSignalHunks("@@ a\n+x\n+y", -25)).toBe("… (this file's diff truncated)"); }); - it("head-slices a single oversized hunk rather than dropping it whole", () => { + it("head-slices a single oversized hunk rather than dropping it whole, reserving the notice (#10017)", () => { // No second "@@" header → hunks.length <= 1, so the single-hunk branch head-slices to fit the budget. + // The slice reserves the truncation notice's length so slice + notice stays within budget. const single = `@@ only\n${"+padding line\n".repeat(20)}`; - const out = keepHighSignalHunks(single, 30); - expect(out.startsWith(single.slice(0, 30))).toBe(true); + const budget = 120; + const out = keepHighSignalHunks(single, budget); + expect(out.length).toBeLessThanOrEqual(budget); + expect(out.startsWith("@@ only")).toBe(true); // real content, head-sliced expect(out).toContain("… (this file's diff truncated)"); }); }); +describe("keepHighSignalHunks never exceeds budget (#10017)", () => { + it("two hunks, smallest 500 chars, budget 100: returns <= 100 with a char of the higher-signal hunk", () => { + const highSignal = `@@ hi\n${"+critical\n".repeat(60)}`; // most added lines -> highest signal + const lowSignal = `@@ lo\n${" context line\n".repeat(40)}`; // >= 500 chars, no added lines + expect(lowSignal.length).toBeGreaterThanOrEqual(500); + const out = keepHighSignalHunks(`${lowSignal}\n${highSignal}`, 100); + expect(out.length).toBeLessThanOrEqual(100); + // At least one character of the high-signal hunk's added lines survives. + expect(out).toContain("+critical"); + }); + + it("single-hunk path with budget 50 over a 5,000-char patch returns <= 50", () => { + const single = `@@ only\n${"+padding\n".repeat(600)}`; // > 5,000 chars, one hunk + expect(single.length).toBeGreaterThan(5000); + expect(keepHighSignalHunks(single, 50).length).toBeLessThanOrEqual(50); + }); + + it("returns a single hunk unchanged when it already fits the budget", () => { + const single = "@@ only\n+a\n+b"; // one hunk, well under budget + expect(keepHighSignalHunks(single, 1000)).toBe(single); + }); + + it("single-hunk path with a budget smaller than the notice returns just the (truncated) notice, still <= budget", () => { + const single = `@@ only\n${"+padding\n".repeat(50)}`; + const budget = 10; // < the ~30-char truncation notice, so there is no room for content + const out = keepHighSignalHunks(single, budget); + expect(out.length).toBeLessThanOrEqual(budget); + expect("… (this file's diff truncated)").toContain(out); // a prefix of the notice + }); + + it("holds the invariant across a spread of budgets and hunk counts", () => { + const hunks = Array.from({ length: 6 }, (_, i) => `@@ h${i}\n${`+line${i}\n`.repeat(i + 1)}`).join("\n"); + for (const budget of [1, 10, 37, 50, 100, 500]) { + expect(keepHighSignalHunks(hunks, budget).length).toBeLessThanOrEqual(budget); + } + }); +}); + describe("buildUnifiedReviewDiff — header defaults + budget-floor truncation (#5849)", () => { it("defaults missing additions/deletions to +0/-0 and a missing status to 'modified'", () => { const diff = buildUnifiedReviewDiff([{ path: "src/a.ts", patch: "@@ a\n+x" }]); @@ -203,4 +255,12 @@ describe("buildUnifiedReviewDiff — header defaults + budget-floor truncation ( expect(diff).toContain("### logo.bin (added) +0/-0"); expect(diff).toContain("(no inline patch — binary or too large)"); }); + + it("never returns a string longer than budget, even with the truncation notice appended (#10017)", () => { + const big = `@@ big\n${"+line of added content\n".repeat(30)}`; + const files = Array.from({ length: 5 }, (_, i) => ({ path: `src/f${i}.ts`, patch: big, status: "modified", additions: 30, deletions: 0 })); + for (const budget of [50, 120, 300, 320, 700, 2000]) { + expect(buildUnifiedReviewDiff(files, budget).length).toBeLessThanOrEqual(budget); + } + }); });