fix(review): stop one oversized RAG chunk from failing its whole embedding batch

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<number[] | null> {
+  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<number[][] | null> {
+): 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) }));

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