From 65d132a81b17bb9d58a1e2217d98ad45b5fc505a Mon Sep 17 00:00:00 2001 From: shin-core <153108882+shin-core@users.noreply.github.com> Date: Fri, 31 Jul 2026 15:17:29 +0900 Subject: [PATCH] fix(review): keep the bounded review diff actually within its char budget MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `keepHighSignalHunks` promised to fit a file's patch into `budget` chars but could exceed it three ways: the ≥2-hunk path force-kept the highest-signal hunk WHOLE when none fit, and appended the dropped-hunk notice AFTER selecting, and the single-hunk path sliced to exactly `budget` then appended the truncation notice on top. So the "bounded" diff the reviewer prompt is built from was not bounded. Reserve the notice length before selecting hunks (not after); when no whole hunk fits, return the highest-signal hunk's CONTENT truncated to fit — the same shape the single-hunk path already uses — never nothing, never over budget. The single-hunk path reserves the notice too. `buildUnifiedReviewDiff` reserves the "…diff truncated" line's length in each file's body budget so appending it on the break can never push the total over, upholding its own boundary invariant. Kept hunks are still emitted in file order; the signal ranking, DEFAULT_DIFF_BUDGET, diffFilePriority parity, and the patch-less-file listing are unchanged. Closes #10017 --- src/review/review-diff.ts | 68 +++++++++++++++++++++++--------- test/unit/review-diff.test.ts | 74 +++++++++++++++++++++++++++++++---- 2 files changed, 116 insertions(+), 26 deletions(-) diff --git a/src/review/review-diff.ts b/src/review/review-diff.ts index 8736ba3b90..bda991a555 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 02ab5efa9f..21cb9fc023 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); + } + }); });