-
Notifications
You must be signed in to change notification settings - Fork 1.1k
fix(flue-review): elide oversized diff sections before staging for the model #2393
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
Merged
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,74 @@ | ||
| // Size budget for the staged PR diff. The review agent starts by reading the | ||
| // whole diff file, so an oversized diff (generated types, lockfiles, large | ||
| // catalogs) lands in the model context verbatim and kills the model call. | ||
| // Oversized per-file sections are elided down to their headers with a note; | ||
| // the agent reads those files from the checkout instead. | ||
|
|
||
| const DEFAULT_PER_FILE_BYTES = 48 * 1024; | ||
| const DEFAULT_TOTAL_BYTES = 384 * 1024; | ||
|
|
||
| export interface DiffBudget { | ||
| readonly perFileBytes?: number; | ||
| readonly totalBytes?: number; | ||
| } | ||
|
|
||
| interface Section { | ||
| text: string; | ||
| elided: boolean; | ||
| } | ||
|
|
||
| export function elideLargeDiffSections(diff: string, budget: DiffBudget = {}): string { | ||
| const perFileBytes = budget.perFileBytes ?? DEFAULT_PER_FILE_BYTES; | ||
| const totalBytes = budget.totalBytes ?? DEFAULT_TOTAL_BYTES; | ||
| if (diff.length <= Math.min(perFileBytes, totalBytes)) return diff; | ||
|
|
||
| const sections = splitSections(diff); | ||
| for (const section of sections) { | ||
| if (!section.elided && section.text.length > perFileBytes) elide(section); | ||
| } | ||
| // Still over the total budget: elide the largest remaining sections until | ||
| // under it (or nothing left to elide). | ||
| let total = sections.reduce((n, s) => n + s.text.length, 0); | ||
| while (total > totalBytes) { | ||
| const next = sections | ||
| .filter((s) => !s.elided) | ||
| .toSorted((a, b) => b.text.length - a.text.length)[0]; | ||
| if (!next) break; | ||
| total -= next.text.length; | ||
| elide(next); | ||
| total += next.text.length; | ||
| } | ||
| return sections.map((s) => s.text).join(""); | ||
| } | ||
|
|
||
| function splitSections(diff: string): Section[] { | ||
| const starts: number[] = []; | ||
| const re = /^diff --git /gm; | ||
| for (let m = re.exec(diff); m; m = re.exec(diff)) starts.push(m.index); | ||
| if (starts.length === 0) return [{ text: diff, elided: false }]; | ||
| const sections: Section[] = []; | ||
| if (starts[0] !== 0) sections.push({ text: diff.slice(0, starts[0]), elided: true }); | ||
| for (let i = 0; i < starts.length; i++) { | ||
| const end = i + 1 < starts.length ? starts[i + 1] : diff.length; | ||
| sections.push({ text: diff.slice(starts[i], end), elided: false }); | ||
| } | ||
| return sections; | ||
| } | ||
|
|
||
| function elide(section: Section): void { | ||
| // Mark unconditionally: a section this function cannot reduce must still | ||
| // leave the total-budget loop's candidate pool, or the loop never shrinks. | ||
| section.elided = true; | ||
| const lines = section.text.split("\n"); | ||
| // Keep the file header: everything up to and including the `+++` line (or | ||
| // the whole header for binary/rename-only sections with no hunks). | ||
| let headerEnd = lines.findIndex((line) => line.startsWith("+++ ")); | ||
| if (headerEnd === -1) headerEnd = lines.findIndex((line) => line.startsWith("@@ ")) - 1; | ||
| if (headerEnd < 0) return; | ||
| const body = lines.length - (headerEnd + 1); | ||
| section.text = [ | ||
| ...lines.slice(0, headerEnd + 1), | ||
| `(diff content elided: ${body} lines over the size budget -- read this file from the checkout instead)`, | ||
| "", | ||
| ].join("\n"); | ||
| } | ||
|
Comment on lines
+58
to
+74
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,71 @@ | ||
| import { describe, expect, it } from "vitest"; | ||
|
|
||
| import { elideLargeDiffSections } from "../.flue/lib/diff-budget.js"; | ||
|
|
||
| function fileSection(path: string, lines: number, line = "+const x = 1;"): string { | ||
| return [ | ||
| `diff --git a/${path} b/${path}`, | ||
| "index 0000000..1111111 100644", | ||
| `--- a/${path}`, | ||
| `+++ b/${path}`, | ||
| "@@ -0,0 +1 @@", | ||
| ...Array.from({ length: lines }).fill(line).map(String), | ||
| "", | ||
| ].join("\n"); | ||
| } | ||
|
|
||
| describe("elideLargeDiffSections", () => { | ||
| it("returns a small diff unchanged", () => { | ||
| const diff = fileSection("src/a.ts", 10) + fileSection("src/b.ts", 20); | ||
| expect(elideLargeDiffSections(diff)).toBe(diff); | ||
| }); | ||
|
|
||
| it("elides a section over the per-file budget, keeping its header", () => { | ||
| const big = fileSection("types.d.ts", 2_000); | ||
| const small = fileSection("src/a.ts", 5); | ||
| const out = elideLargeDiffSections(small + big, { perFileBytes: 1_000 }); | ||
| expect(out).toContain(small); | ||
| expect(out).toContain("diff --git a/types.d.ts b/types.d.ts"); | ||
| expect(out).toContain("+++ b/types.d.ts"); | ||
| expect(out).toMatch(/diff content elided: \d+ lines/); | ||
| expect(out).not.toContain("+const x = 1;\n+const x = 1;\n".repeat(50)); | ||
| }); | ||
|
|
||
| it("elides largest sections first until under the total budget", () => { | ||
| const a = fileSection("a.ts", 30); | ||
| const b = fileSection("b.ts", 60); | ||
| const c = fileSection("c.ts", 10); | ||
| const out = elideLargeDiffSections(a + b + c, { | ||
| perFileBytes: 10_000, | ||
| totalBytes: a.length + c.length + 400, | ||
| }); | ||
| expect(out).toContain("diff content elided"); | ||
| expect(out).toContain(a); | ||
| expect(out).toContain(c); | ||
| expect(out).not.toContain(b); | ||
| }); | ||
|
|
||
| it("skips an unreducible headerless section instead of looping on it", () => { | ||
| const headerless = `diff --git a/blob.bin b/blob.bin\nBinary files differ\n${"x\n".repeat(1_000)}`; | ||
| const small = fileSection("src/a.ts", 5); | ||
| const out = elideLargeDiffSections(small + headerless, { | ||
| perFileBytes: 500, | ||
| totalBytes: 600, | ||
| }); | ||
| expect(out).toContain("Binary files differ"); | ||
| expect(out).toContain("+++ b/src/a.ts"); | ||
| }); | ||
|
|
||
| it("leaves a header-only section (no hunks) alone", () => { | ||
| const rename = [ | ||
| "diff --git a/old.ts b/new.ts", | ||
| "similarity index 100%", | ||
| "rename from old.ts", | ||
| "rename to new.ts", | ||
| "", | ||
| ].join("\n"); | ||
| const filler = fileSection("big.ts", 2_000); | ||
| const out = elideLargeDiffSections(rename + filler, { perFileBytes: 1_000, totalBytes: 1_500 }); | ||
| expect(out).toContain("rename from old.ts"); | ||
| }); | ||
| }); | ||
|
Comment on lines
+59
to
+71
|
||
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[needs fixing]
elide()can return without settingsection.elided, and the loops assume every processed section is marked reduced. If a diff section lacks a+++/@@header and is large enough to be selected by the total-budgetwhileloop, it is selected again on the next iteration becausesection.elidednever flips totrue, sototalnever shrinks and the Worker spins until its execution limit. Malformed or unusual formats (e.g. a headerless file section, a very large mode-only diff, or a binary patch shaped differently than expected) are enough to trigger this.The per-file loop also calls
elide()on the preface section even though it was already flaggedelided: trueinsplitSections, which is a hint thatelidedis being overloaded.Fix by (1) guarding the per-file loop so it only tries to reduce sections that aren't already marked, and (2) making
elide()always mark the section and fall back to replacing the whole section when no header boundary is found.Please also add a regression test in
infra/flue-review/test/diff-budget.test.tsthat passes a large headerless section and asserts the budget loop terminates and replaces the section instead of returning the original content.