From 881f138a4e547ce1300cdb7725f6b26c3ae56aa9 Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Sat, 11 Jul 2026 02:10:03 -0700 Subject: [PATCH] fix(review): drop whitespace-only RAG chunks and surface real embed errors ai_embed_http_400 fired 485 times over 2 weeks, still ongoing. Two fixes: 1. chunkFile only checked the WHOLE file was non-empty before splitting -- an individual slice from newlineChunks/chunkJsTs could land entirely inside a run of blank/whitespace-only lines and reach the embed API as an empty string, the most plausible cause of a 400 from Ollama's OpenAI-compatible /embeddings endpoint. Filter after chunking instead. 2. The embed error path threw only the bare status code, discarding the response body -- diagnosing this required SSHing into production and reasoning from first principles instead of just reading the error. Now captures a bounded response-body detail (best-effort, never masks the status on a body-read failure), matching what future occurrences (this fix or otherwise) will need to actually diagnose. Closes #4996 --- src/review/rag.ts | 14 +++++++++----- src/selfhost/ai.ts | 10 +++++++++- test/unit/rag.test.ts | 14 ++++++++++++++ test/unit/selfhost-ai.test.ts | 27 ++++++++++++++++++++++++--- 4 files changed, 56 insertions(+), 9 deletions(-) diff --git a/src/review/rag.ts b/src/review/rag.ts index 909bf65c12..2f5c765356 100644 --- a/src/review/rag.ts +++ b/src/review/rag.ts @@ -194,11 +194,15 @@ export function chunkFile(path: string, text: string, namespace = "", opts?: Chu // chunk is one coherent unit rather than "this function + 19 unrelated ones". Small files still collapse to // one chunk (the packer combines units up to chunkChars), so the per-repo vector budget is unaffected; a // single oversized unit falls back to newline splitting. (#282) Non-JS/TS keeps the newline chunker. - if (JS_TS_RE.test(path)) { - const logical = chunkJsTs(path, text, kind, namespace, chunkChars, chunkOverlap); - if (logical) return logical; - } - return newlineChunks(path, text, kind, namespace, chunkChars, chunkOverlap); + const chunks = JS_TS_RE.test(path) ? (chunkJsTs(path, text, kind, namespace, chunkChars, chunkOverlap) ?? newlineChunks(path, text, kind, namespace, chunkChars, chunkOverlap)) : newlineChunks(path, text, kind, namespace, chunkChars, chunkOverlap); + // #4996: the whole-FILE non-empty check above (`!text.trim()`) doesn't guarantee every individual SLICE is + // non-empty -- a newline-boundary split can land a chunk entirely inside a run of blank/whitespace-only + // lines (e.g. a large trailing gap between logical units). An empty/whitespace-only chunk reaching the embed + // API was the most plausible cause of the ai_embed_http_400 failures observed in production (Ollama's + // OpenAI-compatible /embeddings endpoint rejects it); filtering here is the single, centralized guard, since + // both chunkJsTs and newlineChunks can produce one. chunkIndex intentionally keeps its ORIGINAL (pre-filter) + // value -- it only needs to be a stable id component, not a gap-free sequence. + return chunks.filter((c) => c.text.trim().length > 0); } const JS_TS_RE = /\.(ts|tsx|js|jsx|mjs|cjs)$/i; diff --git a/src/selfhost/ai.ts b/src/selfhost/ai.ts index c1a5826db9..8e0106aff2 100644 --- a/src/selfhost/ai.ts +++ b/src/selfhost/ai.ts @@ -286,7 +286,15 @@ export function createOpenAiCompatibleAi(opts: { body: JSON.stringify({ model: opts.embedModel ?? "bge-m3", input: options.text }), signal: AbortSignal.timeout(120_000), }); - if (!res.ok) throw new Error(`ai_embed_http_${res.status}`); + // #4996: the error previously carried only the status code, with no detail from the response body -- + // diagnosing a real production ai_embed_http_400 required SSHing into the box and reasoning about + // likely causes from first principles, since the actual rejection reason (e.g. Ollama's own "input + // length exceeds..." message) was thrown away. Bounded read, best-effort (a body-read failure must + // never mask the original status in the thrown error). + if (!res.ok) { + const detail = await res.text().then((t) => t.slice(0, 300)).catch(() => ""); + throw new Error(detail ? `ai_embed_http_${res.status}: ${detail}` : `ai_embed_http_${res.status}`); + } const json = (await res.json()) as { data?: Array<{ embedding: number[] }> }; return { data: (json.data ?? []).map((d) => d.embedding) }; } diff --git a/test/unit/rag.test.ts b/test/unit/rag.test.ts index 8a6e6bc8ce..24284d8cc1 100644 --- a/test/unit/rag.test.ts +++ b/test/unit/rag.test.ts @@ -195,6 +195,20 @@ describe("rag: per-file chunking", () => { expect(chunkFile("src/a.ts", " ")).toEqual([]); }); + it("REGRESSION (#4996): filters out an individual whitespace-only chunk, even though the whole file is non-empty (the most plausible cause of production ai_embed_http_400s)", () => { + // chunkChars=10, overlap=0: chunk0 = 10 X's, chunk1 = 10 SPACES (whitespace-only -- must be dropped), + // chunk2 = 10 Y's. Non-JS path (.py) so this exercises newlineChunks, the chunker actually used in prod + // for the file types most commonly indexed by size (large generated/data-adjacent code files). + const text = "X".repeat(10) + " ".repeat(10) + "Y".repeat(10); + const chunks = chunkFile("src/big.py", text, "", { chunkChars: 10, chunkOverlap: 0 }); + expect(chunks.every((c) => c.text.trim().length > 0)).toBe(true); + expect(chunks.map((c) => c.text)).toEqual(["X".repeat(10), "Y".repeat(10)]); + // chunkIndex/id keep their ORIGINAL (pre-filter) position -- the surviving Y chunk is still index 2, not + // renumbered to 1, since it's only ever used as a stable id component, not required to be gap-free. + expect(chunks[1]?.chunkIndex).toBe(2); + expect(chunks[1]?.id).toBe("src/big.py::2"); + }); + it("splits a JS/TS file at FUNCTION boundaries, never mid-function, tagging the boundary kind (#282)", () => { const fn = (n: number) => `export function f${n}() {\n${Array.from({ length: 120 }, (_, i) => ` const v${i} = ${i}; // padding aaaaaaaaaaaaaaaaaaaaaaaa`).join("\n")}\n}\n`; const chunks = chunkFile("src/multi.ts", fn(1) + fn(2) + fn(3)); // 3 functions, each ~6k; total > CHUNK_CHARS diff --git a/test/unit/selfhost-ai.test.ts b/test/unit/selfhost-ai.test.ts index 9b1115065f..9919df867c 100644 --- a/test/unit/selfhost-ai.test.ts +++ b/test/unit/selfhost-ai.test.ts @@ -184,9 +184,30 @@ describe("createOpenAiCompatibleAi (#979)", () => { expect(out).toEqual({ data: [[0.1, 0.2], [0.3, 0.4]] }); }); - it("throws on a non-OK embeddings response", async () => { - vi.stubGlobal("fetch", vi.fn(async () => new Response("e", { status: 502 }))); - await expect(createOpenAiCompatibleAi({ baseUrl: "http://x/v1" }).run("m", { text: ["a"] })).rejects.toThrow(/ai_embed_http_502/); + it("throws on a non-OK embeddings response, including the response body detail (#4996: previously thrown away)", async () => { + vi.stubGlobal("fetch", vi.fn(async () => new Response("bad request: input exceeds max length", { status: 400 }))); + await expect(createOpenAiCompatibleAi({ baseUrl: "http://x/v1" }).run("m", { text: ["a"] })).rejects.toThrow( + "ai_embed_http_400: bad request: input exceeds max length", + ); + }); + + it("falls back to the bare status code when the error response body is empty", async () => { + vi.stubGlobal("fetch", vi.fn(async () => new Response("", { status: 502 }))); + await expect(createOpenAiCompatibleAi({ baseUrl: "http://x/v1" }).run("m", { text: ["a"] })).rejects.toThrow(/^ai_embed_http_502$/); + }); + + it("still throws the bare status code (never masked) when reading the error response body itself fails", async () => { + vi.stubGlobal( + "fetch", + vi.fn(async () => ({ ok: false, status: 503, text: () => Promise.reject(new Error("stream error")) }) as unknown as Response), + ); + await expect(createOpenAiCompatibleAi({ baseUrl: "http://x/v1" }).run("m", { text: ["a"] })).rejects.toThrow(/^ai_embed_http_503$/); + }); + + it("bounds the captured error detail to 300 chars", async () => { + vi.stubGlobal("fetch", vi.fn(async () => new Response("x".repeat(1000), { status: 400 }))); + const error = await createOpenAiCompatibleAi({ baseUrl: "http://x/v1" }).run("m", { text: ["a"] }).catch((e: Error) => e.message); + expect(error).toBe(`ai_embed_http_400: ${"x".repeat(300)}`); }); it("empty text array returns { data: [] } without a fetch", async () => {