fix(review): keep the bounded review diff actually within its char budget

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<number>();
-  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`;
   }

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);
+    }
+  });
 });