diff --git a/src/review/rag.ts b/src/review/rag.ts index 2f5c765356..20792b7a9f 100644 --- a/src/review/rag.ts +++ b/src/review/rag.ts @@ -323,36 +323,80 @@ export async function getStoredChunkMeta(storage: StorageAdapter, project: strin } // ── Embedding (fail-safe: null on any failure) ──────────────────────────────────────────────────── +/** Embed one text in isolation — the fallback when a batch call throws or comes back structurally invalid, so + * the caller can isolate exactly which item(s) are the problem instead of losing every chunk in the batch. + * WARN, not error: a per-item failure here is expected diagnostic detail, already summarized once per + * degraded batch by the caller at error level (mirrors the per-attempt-warn / exhausted-error escalation + * pattern used elsewhere in the AI-review pipeline, #5046). Never logs the text itself — only its length — + * since a RAG chunk is source code from the (possibly private) indexed repo, unlike a model's own commentary. */ +async function embedSingleText(inference: InferenceAdapter, text: string, expectedDimensions: number): Promise { + try { + const res = (await inference.run(EMBED_MODEL, { text: [text] })) as { data?: number[][] } | null; + const vec = res?.data?.[0]; + if (Array.isArray(vec) && vec.length === expectedDimensions) return Array.from(vec); + console.warn(JSON.stringify({ level: "warn", event: "rag_embed_item_invalid", chars: text.length })); + return null; + } catch (error) { + console.warn(JSON.stringify({ level: "warn", event: "rag_embed_item_error", chars: text.length, message: String(error).slice(0, 200) })); + return null; + } +} + +/** Embed every text, batched for throughput. A `null` at an index means that ONE text could not be embedded + * (oversized/malformed for the provider, or a genuine per-item failure) — everything else in its batch still + * embeds. The whole call returns `null` only when there is no inference adapter, no input, or an invalid + * batch size configured (#abc-verify's original whole-batch-fails-fast behavior stays for those). */ export async function embedTexts( inference: InferenceAdapter | undefined, texts: string[], expectedDimensions = RAG_DIMENSIONS, batchSize = EMBED_BATCH, -): Promise { +): Promise<(number[] | null)[] | null> { if (!inference || texts.length === 0) return null; const effectiveBatchSize = Math.floor(batchSize); if (!Number.isFinite(effectiveBatchSize) || effectiveBatchSize < 1) return null; - try { - const out: number[][] = []; - for (let i = 0; i < texts.length; i += effectiveBatchSize) { - const batch = texts.slice(i, i + effectiveBatchSize); + const out: (number[] | null)[] = []; + for (let i = 0; i < texts.length; i += effectiveBatchSize) { + const batch = texts.slice(i, i + effectiveBatchSize); + let data: number[][] | undefined; + let batchError: unknown; + try { const res = (await inference.run(EMBED_MODEL, { text: batch })) as { data?: number[][] } | null; - const data = res?.data; - // Validate COUNT and DIMENSION: a self-host embedding endpoint can return a structurally-valid response - // with a missing/empty/wrong-width vector — without the dim check a bad vector - // slips through and later fails Vectorize.upsert, dropping the whole batch. Fail the batch early. (#abc-verify) - if (!Array.isArray(data) || data.length !== batch.length || data.some((v) => !Array.isArray(v) || v.length !== expectedDimensions)) return null; + data = res?.data; + } catch (error) { + batchError = error; + } + // Validate COUNT and DIMENSION: a self-host embedding endpoint can return a structurally-valid response + // with a missing/empty/wrong-width vector — without the dim check a bad vector slips through and later + // fails Vectorize.upsert. (#abc-verify) + if (Array.isArray(data) && data.length === batch.length && data.every((v) => Array.isArray(v) && v.length === expectedDimensions)) { out.push(...data.map((v) => Array.from(v))); + continue; } - return out; - } catch (error) { - // ERROR level (#3894): an embedding-provider failure previously logged at console.log with no `level`, - // invisible to the central Sentry forwarder -- mirrors the already-fixed retrieveContextWithMetrics - // catch below. Shares its `review_context_fetch_failed`/contextType:"rag" umbrella so both are - // searchable together, plus the specific `ev` tag for log continuity. - console.error(JSON.stringify({ level: "error", event: "review_context_fetch_failed", contextType: "rag", ev: "rag_embed_error", message: String(error).slice(0, 200) })); - return null; + // A single oversized/malformed text (e.g. a dense/minified chunk exceeding the embed model's context + // window — the observed cause of production ai_embed_http_400s) previously failed the WHOLE batch, so up + // to EMBED_BATCH unrelated chunks lost their RAG context over one bad item. Retry one item at a time so + // only the genuinely-unembeddable item(s) are lost; one ERROR-level summary (not one per item) keeps this + // Sentry-visible without amplifying a single degraded batch into up to EMBED_BATCH issues (#5046 pattern). + const items: (number[] | null)[] = []; + for (const text of batch) items.push(await embedSingleText(inference, text, expectedDimensions)); + const failedCount = items.filter((v) => v === null).length; + if (failedCount > 0) { + console.error( + JSON.stringify({ + level: "error", + event: "review_context_fetch_failed", + contextType: "rag", + ev: "rag_embed_batch_degraded", + batchSize: batch.length, + failedCount, + ...(batchError ? { message: String(batchError).slice(0, 200) } : {}), + }), + ); + } + out.push(...items); } + return out; } // ── Index write (used by ingestion): embed + vector upsert + chunk-text store ───────────────────── @@ -368,23 +412,30 @@ export async function upsertChunks(infra: RagInfra, project: string, repo: strin const namespace = ragNamespace(project, repo); const vectors = await embedTexts(inference, chunks.map((c) => c.text), infra.embeddingDimensions ?? RAG_DIMENSIONS, infra.embedBatch ?? EMBED_BATCH); if (!vectors) return 0; + // A `null` entry means that ONE chunk's text couldn't be embedded (see embedTexts) -- everything else in the + // batch still embedded, so only the chunk(s) actually missing a vector are skipped here; a single oversized + // chunk no longer drops every other chunk in the same upsertChunks call. + const embedded = chunks + .map((c, i) => ({ chunk: c, vector: vectors[i] })) + .filter((entry): entry is { chunk: RagChunk; vector: number[] } => Array.isArray(entry.vector)); + if (embedded.length === 0) return 0; try { await vec.upsert( - chunks.map((c, i) => ({ + embedded.map(({ chunk: c, vector }) => ({ id: c.id, - values: vectors[i] as number[], + values: vector, namespace, metadata: { path: c.path, chunkIndex: c.chunkIndex, kind: c.kind }, })), ); - const stmts = chunks.map((c) => + const stmts = embedded.map(({ chunk: c }) => db.prepare( "INSERT INTO repo_chunks (id, project, repo, path, chunk_index, kind, text, blob_sha) VALUES (?,?,?,?,?,?,?,?) " + "ON CONFLICT(id) DO UPDATE SET text=excluded.text, kind=excluded.kind, chunk_index=excluded.chunk_index, blob_sha=excluded.blob_sha, updated_at=CURRENT_TIMESTAMP", ).bind(c.id, project, repo, c.path, c.chunkIndex, c.kind, c.text, blobSha ?? null), ); await db.batch(stmts); - return chunks.length; + return embedded.length; } catch (error) { // ERROR level (#3894): see embedTexts's catch above -- same invisible-to-Sentry fix, same umbrella. console.error(JSON.stringify({ level: "error", event: "review_context_fetch_failed", contextType: "rag", ev: "rag_upsert_error", message: String(error).slice(0, 200) })); diff --git a/test/unit/rag.test.ts b/test/unit/rag.test.ts index 24284d8cc1..84b29b6db3 100644 --- a/test/unit/rag.test.ts +++ b/test/unit/rag.test.ts @@ -283,12 +283,12 @@ describe("rag: fail-safe (never throws; degrades to no context)", () => { }); it("embedTexts rejects a wrong-DIMENSION embedding (a non-1024-d model / malformed vector) (#abc-verify)", async () => { - expect(await embedTexts(aiThatReturns([[0.1, 0.2]]), ["hi"])).toBeNull(); // 2-d, not 1024 + expect(await embedTexts(aiThatReturns([[0.1, 0.2]]), ["hi"])).toEqual([null]); // 2-d, not 1024, persists through the per-item retry expect((await embedTexts(ai1024, ["hi"]))?.[0]?.length).toBe(1024); }); it("embedTexts accepts a configured 768-dimension embedder without weakening the default guard", async () => { - expect(await embedTexts(ai768, ["hi"])).toBeNull(); + expect(await embedTexts(ai768, ["hi"])).toEqual([null]); // 1024-d expected by default, 768-d persists as invalid through the per-item retry expect((await embedTexts(ai768, ["hi"], 768))?.[0]?.length).toBe(768); }); @@ -744,13 +744,14 @@ describe("rag: storage/inference catch paths return their fail-safe defaults", ( expect(await countRepoChunks(storage, "p", "o/r")).toBe(0); }); - it("embedTexts returns null when inference throws", async () => { + it("embedTexts degrades to [null] when inference throws for every call (batch AND its per-item retry), still surfacing an ERROR-level Sentry-visible summary", async () => { const errSpy = vi.spyOn(console, "error").mockImplementation(() => {}); const inference: InferenceAdapter = { run: async () => { throw new Error("ai down"); } }; - expect(await embedTexts(inference, ["hi"])).toBeNull(); - // #3894: previously a no-level console.log, invisible to Sentry. + expect(await embedTexts(inference, ["hi"])).toEqual([null]); + // #3894: previously a no-level console.log, invisible to Sentry. Now logged once as a batch-degraded + // summary (#observability-unparseable-continuation), not the old whole-batch rag_embed_error. const parsed = errSpy.mock.calls.map((c) => JSON.parse(c[0] as string)); - expect(parsed.some((p) => p.level === "error" && p.event === "review_context_fetch_failed" && p.contextType === "rag" && p.ev === "rag_embed_error")).toBe(true); + expect(parsed.some((p) => p.level === "error" && p.event === "review_context_fetch_failed" && p.contextType === "rag" && p.ev === "rag_embed_batch_degraded" && p.message?.includes("ai down"))).toBe(true); errSpy.mockRestore(); }); @@ -843,20 +844,23 @@ describe("rag: formatRetrievedContext budget omission", () => { // ── embedTexts: the remaining validation branches in the OR-guard (#abc-verify) ─────────────────────── describe("rag: embedTexts validation branches", () => { - it("returns null when `data` is missing entirely (not an array)", async () => { - // res.data === undefined → !Array.isArray(data) is the FIRST OR clause - expect(await embedTexts(aiThatReturns(undefined), ["hi"])).toBeNull(); + it("returns [null] when `data` is missing entirely (not an array), even after the per-item retry", async () => { + // res.data === undefined on every call (batch AND the per-item retry) → !Array.isArray(data) is the FIRST + // OR clause, persistently -- this text is genuinely unembeddable with this (broken) adapter. + expect(await embedTexts(aiThatReturns(undefined), ["hi"])).toEqual([null]); }); - it("returns null on a COUNT mismatch (fewer vectors than inputs)", async () => { - // data.length(1) !== batch.length(2) → the SECOND OR clause; both vectors are correctly 1024-d - const ai = aiThatReturns([Array(1024).fill(0.1)]); - expect(await embedTexts(ai, ["one", "two"])).toBeNull(); + it("degrades to per-item nulls on a persistent COUNT mismatch (a provider that always returns zero vectors)", async () => { + // data.length(0) !== batch.length -- true for the batch call AND every per-item retry, so both inputs end + // up null rather than the whole call failing outright (#observability-unparseable-continuation). + const ai = aiThatReturns([]); + expect(await embedTexts(ai, ["one", "two"])).toEqual([null, null]); }); - it("returns null when an inner element is not an array (the `!Array.isArray(v)` leg)", async () => { - // a structurally-valid response whose single 'vector' is a number, not an array - expect(await embedTexts(aiThatReturns([42]), ["hi"])).toBeNull(); + it("returns [null] when an inner element is not an array (the `!Array.isArray(v)` leg), even after the per-item retry", async () => { + // a structurally-valid response whose single 'vector' is a number, not an array -- true for the batch call + // and the per-item retry (same fixed-response mock), so this text stays unembeddable. + expect(await embedTexts(aiThatReturns([42]), ["hi"])).toEqual([null]); }); it("embeds ACROSS multiple batches (>EMBED_BATCH=96 inputs → the for-loop iterates more than once)", async () => { @@ -900,18 +904,73 @@ describe("rag: embedTexts validation branches", () => { expect(calls).toEqual([50, 50, 50]); // three batches of 50, not the default 96/54 split }); - it("fails the WHOLE embed when a LATER batch is malformed (early-return mid-loop)", async () => { + it("REGRESSION (#observability-unparseable-continuation, was 'fails the WHOLE embed when a LATER batch is malformed'): a malformed LATER batch degrades to per-item nulls for ONLY that batch — the earlier good batch's real vectors are kept, not discarded", async () => { let call = 0; const inference: InferenceAdapter = { run: async (_model, options) => { const batch = (options as { text: string[] }).text; call += 1; - // first batch good (1024-d), second batch wrong width (2-d) → returns null from inside the loop + // first batch (call 1) good (1024-d); every later call (the second batch AND its per-item retries) + // wrong width (2-d) -- proves a bad SECOND batch no longer erases the first batch's real vectors. return { data: batch.map(() => (call === 1 ? Array(1024).fill(0.1) : [0.1, 0.2])) }; }, }; const texts = Array.from({ length: 150 }, (_, i) => `t${i}`); - expect(await embedTexts(inference, texts)).toBeNull(); + const out = await embedTexts(inference, texts); + expect(out).not.toBeNull(); + expect(out?.length).toBe(150); + expect(out?.slice(0, 96).every((v) => Array.isArray(v) && v.length === 1024)).toBe(true); + expect(out?.slice(96).every((v) => v === null)).toBe(true); + }); + + it("REGRESSION (#observability-unparseable-continuation, the ai_embed_http_400 production bug): a single throwing item in a batch does not fail its whole-batch siblings — only that one item is skipped", async () => { + const warnSpy = vi.spyOn(console, "warn").mockImplementation(() => {}); + const errorSpy = vi.spyOn(console, "error").mockImplementation(() => {}); + const inference: InferenceAdapter = { + run: async (_model, options) => { + const batch = (options as { text: string[] }).text; + // Simulate one oversized chunk exceeding the embed model's context window: any batch CONTAINING it + // throws (as Ollama's /embeddings endpoint does for one bad item in a batched request) -- including + // the initial 3-item batch -- but a single-item retry for the good texts succeeds, so only the + // oversized one keeps failing. + if (batch.includes("oversized")) throw new Error("ai_embed_http_400: the input length exceeds the context length"); + return { data: batch.map(() => Array(1024).fill(0.1)) }; + }, + }; + const out = await embedTexts(inference, ["fine-1", "oversized", "fine-2"]); + expect(out).not.toBeNull(); + expect(out?.length).toBe(3); + expect(out?.[0]).toEqual(Array(1024).fill(0.1)); + expect(out?.[1]).toBeNull(); + expect(out?.[2]).toEqual(Array(1024).fill(0.1)); + const degraded = errorSpy.mock.calls.map((c) => c[0]).find((l) => typeof l === "string" && l.includes("rag_embed_batch_degraded")); + expect(degraded).toBeDefined(); + expect(JSON.parse(degraded as string)).toMatchObject({ level: "error", ev: "rag_embed_batch_degraded", batchSize: 3, failedCount: 1 }); + expect(warnSpy.mock.calls.some((c) => typeof c[0] === "string" && c[0].includes("rag_embed_item_error"))).toBe(true); + warnSpy.mockRestore(); + errorSpy.mockRestore(); + }); + + it("logs nothing when a batch-level glitch self-heals — every item succeeds once retried individually (failedCount === 0)", async () => { + const errorSpy = vi.spyOn(console, "error").mockImplementation(() => {}); + let batchCalls = 0; + const inference: InferenceAdapter = { + run: async (_model, options) => { + const batch = (options as { text: string[] }).text; + // Simulate a provider that rejects a >1-item request (e.g. a self-host concurrency limit) but is + // perfectly happy embedding the SAME texts one at a time. + if (batch.length > 1) { + batchCalls += 1; + throw new Error("too many concurrent inputs"); + } + return { data: [Array(1024).fill(0.1)] }; + }, + }; + const out = await embedTexts(inference, ["one", "two"]); + expect(batchCalls).toBe(1); + expect(out).toEqual([Array(1024).fill(0.1), Array(1024).fill(0.1)]); + expect(errorSpy.mock.calls.some((c) => typeof c[0] === "string" && c[0].includes("rag_embed_batch_degraded"))).toBe(false); + errorSpy.mockRestore(); }); });