From 552134c49b9aafd62cc2d548afa528ced815ce76 Mon Sep 17 00:00:00 2001 From: jsdevninja Date: Mon, 20 Jul 2026 08:01:32 -0500 Subject: [PATCH] fix(review): guarantee newlineChunks forward progress under high overlap Co-authored-by: Cursor --- src/review/rag.ts | 11 +++++++---- test/unit/rag.test.ts | 40 ++++++++++++++++++++++++++++++++++++++++ 2 files changed, 47 insertions(+), 4 deletions(-) diff --git a/src/review/rag.ts b/src/review/rag.ts index 4b843a1a58..4ecbc75f2f 100644 --- a/src/review/rag.ts +++ b/src/review/rag.ts @@ -195,9 +195,9 @@ export function chunkFile(path: string, text: string, namespace = "", opts?: Chu // file re-failed the whole upsert batch (up to 49 otherwise-good chunks rolled back with it) on every // push and cron retry (GITTENSORY-D). text = text.replace(/(?![\t\n\r])\p{Cc}/gu, " "); - // Clamp to safe ranges: chunkChars must be >= 1 (a 0/negative budget makes newlineChunks loop forever on an - // oversized file — a misconfig DOS) and overlap must be < chunkChars (else `end - overlap` can't advance). - // (#rag-verify infinite-loop guard) + // Clamp to safe ranges: chunkChars must be >= 1 (a 0/negative budget never advances the window) and + // overlap must be < chunkChars. Overlap alone is NOT enough to prevent stalls — newline snap can shrink + // `end` so `end - overlap <= start`; newlineChunks floors the next start at `start + 1` (#7447 / #rag-verify). const chunkChars = Math.max(1, Math.floor(opts?.chunkChars ?? CHUNK_CHARS)); const chunkOverlap = Math.min(Math.max(0, Math.floor(opts?.chunkOverlap ?? CHUNK_OVERLAP)), chunkChars - 1); // JS/TS: cut on LOGICAL boundaries (function/class/export) instead of arbitrary newlines, so a retrieved @@ -291,7 +291,10 @@ function newlineChunks(path: string, text: string, kind: RagKind, namespace: str chunks.push({ id: chunkId(namespace, path, idx), path, chunkIndex: idx, kind, text: text.slice(start, end), boundary: "file" }); idx += 1; if (end >= text.length) break; - start = Math.max(0, end - chunkOverlap); + // #7447: newline snap can shrink `end` well below `start + chunkChars`, so `end - chunkOverlap` may not + // advance past the previous `start` even when overlap < chunkChars. Floor at start + 1 so every + // iteration makes forward progress (Math.max(0, …) is redundant once start >= 0, which the loop maintains). + start = Math.max(start + 1, end - chunkOverlap); } return chunks; } diff --git a/test/unit/rag.test.ts b/test/unit/rag.test.ts index 0f87001cc6..ce283ee8e5 100644 --- a/test/unit/rag.test.ts +++ b/test/unit/rag.test.ts @@ -240,6 +240,46 @@ describe("rag: per-file chunking", () => { expect(chunks.every((c) => c.text.length > 0)).toBe(true); }); + it("REGRESSION (#7447): does NOT hang when newline snap + near-budget overlap would stall `end - overlap <= start`", () => { + // Exact repro from the issue: early qualifying newline (~index 60 > chunkChars/2) shrinks `end`, and + // chunkOverlap=99 (allowed by the old clamp of chunkChars-1) makes `end - overlap` land at/before the + // previous start. Without the start+1 floor this never returns. + const text = `${"x".repeat(60)}\n${"y".repeat(5000)}`; + const chunks = chunkFile("f.py", text, "", { chunkChars: 100, chunkOverlap: 99 }); + expect(chunks.length).toBeGreaterThan(1); + // Worst case every iteration advances exactly 1 char → at most text.length chunks. + expect(chunks.length).toBeLessThanOrEqual(text.length); + expect(chunks.every((c) => c.text.length > 0)).toBe(true); + }); + + it.each([ + { chunkChars: 100, chunkOverlap: 99 }, + { chunkChars: 100, chunkOverlap: 50 }, + { chunkChars: 50, chunkOverlap: 49 }, + { chunkChars: 20, chunkOverlap: 19 }, + { chunkChars: 1, chunkOverlap: 0 }, + { chunkChars: 1, chunkOverlap: 999 }, // clamped to 0 + ])( + "REGRESSION (#7447): chunkFile returns finitely for chunkChars=$chunkChars chunkOverlap=$chunkOverlap (forward progress)", + ({ chunkChars, chunkOverlap }) => { + const text = `${"x".repeat(60)}\n${"y".repeat(5000)}`; + const chunks = chunkFile("f.py", text, "", { chunkChars, chunkOverlap }); + expect(chunks.length).toBeGreaterThan(0); + expect(chunks.length).toBeLessThanOrEqual(text.length); + }, + ); + + it("REGRESSION (#7447): chunkJsTs oversized-unit fallback through newlineChunks also cannot stall on high overlap", () => { + // Two logical units so chunkJsTs runs; the first exceeds chunkChars and falls back to newlineChunks with + // the caller-supplied (near-budget) overlap — the other production path that must stay hang-free. + const huge = `export function big() {\n${"x".repeat(60)}\n${"y".repeat(5000)}\n}\n`; + const small = "export function small() { return 1; }\n"; + const text = huge + small; + const chunks = chunkFile("src/huge.ts", text, "", { chunkChars: 100, chunkOverlap: 99 }); + expect(chunks.length).toBeGreaterThan(0); + expect(chunks.length).toBeLessThanOrEqual(text.length); + }); + it("PACKS a small multi-function JS file into one chunk (free-tier vector budget unaffected) (#282)", () => { const chunks = chunkFile("src/small.ts", "export function a(){return 1;}\nexport function b(){return 2;}\nexport function c(){return 3;}\n"); expect(chunks).toHaveLength(1);