fix(embeddings): fail loudly instead of storing zero vectors when embedding fails

[lfnothias]

Summary

An embedding provider that fails must not be allowed to substitute a zero vector. Chroma accepts a zero vector and then reports cosine distance 1.0 against every document, so score = 1/(1+distance) collapses to a constant 0.5: every query returns the same passages, in arbitrary order, while the API reports success: true.

This makes the failure loud at both ends — the write path and the read path — and makes an already-poisoned knowledge base visible.

Operational note

Retrieval is currently broken at runtime regardless of this change. The configured embedder is OpenAI text-embedding-3-large and its key's quota is exhausted (429 insufficient_quota). The operator must restore the quota, or set knowledge_base.embedding_model to a local model such as all-MiniLM-L6-v2, before live retrieval works at all. Before this change that condition was silent; after it, it is an error.

What was actually wrong

The reported per-item zero-fill (embedding_item_skipped) lives on the branch asbb/p0-kb-doc, introduced by 77f0f331, and is not on main. On main, LiteLLMEmbeddingProvider.embed already re-raises. What main did have:

• add_documents rejected None embeddings but stored zero vectors without complaint. Nothing in the codebase checked a vector's norm on either the write or the read side.
• search passed a zero-norm query straight to Chroma, which returns distance 1.0 for every document rather than raising. Confirmed empirically against chromadb 1.5.9: a zero vector on either side is indistinguishable at the score layer.
• add_papers swallowed every per-paper exception, so even a raised provider error surfaced as added_chunks: 0 with a 200.
• LiteLLMEmbeddingProvider.embed and the sentence-transformer provider drop empty texts, returning a list shorter than their input. screening.py indexed that list positionally against paper boundaries, so one empty reference abstract silently scored candidates against the wrong paper.

Changes

• llm/embeddings.py: add EmbeddingFailedError and is_zero_vector. The provider protocol now states the contract: raise on failure, never substitute a zero vector; a zero vector is only valid for input that is itself empty.
• retrieval/chroma_store.py: add_documents wraps the provider call, rejects a vector count that does not match the text count, and rejects a zero-norm vector produced for a non-empty chunk. search rejects a zero-norm query embedding. It is the only caller of coll.query in the codebase, so this covers every retriever.
• rag/dynamic_kb.py: add_papers re-raises EmbeddingFailedError instead of swallowing it. Ordinary per-paper errors (an unparseable PDF) are still tolerated.
• search/screening.py: refuse to score when the reference vectors are misaligned with the reference texts.
• web/app.py: one exception handler turns EmbeddingFailedError into 502 {"success": false, ...} for every route, rather than a bare 500.
• web/routers/kb.py: GET /api/kb/{name}/stats returns an embedding_health block (probed_chunks, zero_vector_chunks, degraded) from a bounded sample, so a poisoned KB is visible without reading Chroma by hand.

Empty input is still handled exactly as before: embed(["", " "]) returns zero vectors, and that remains legal.

Verification

• New tests/unit/test_embedding_degeneracy.py (12 tests): a raising provider aborts the ingest and writes nothing; a zero vector for a non-empty chunk is rejected and nothing is persisted; a short vector list is rejected; a zero-norm query raises rather than returning uniform scores; a healthy query still searches; empty strings stay graceful; add_papers propagates an embedding failure but still tolerates a bad PDF.
• _probe_embedding_health exercised against real Chroma collections: healthy → degraded: false; all-zero → 2/2 degraded; mixed → 1/2 degraded; empty → 0.
• Full unit suite with CI's exclusions: 2418 passed, 0 failed (see the follow-up section below).
• ruff: no new findings against the main baseline for the touched files.
• mypy src/ fails identically before and after this change (5 errors originating in numpy stubs, because [tool.mypy] python_version = "3.11" disagrees with the interpreter). Pre-existing, not addressed here.

Follow-up: holes found by adversarially reviewing this PR

Raising an error only helps if nobody swallows it, and guarding one write path only helps if there is no second one. A review pass found four defects in the first version of this branch; all are fixed here, each with a test that fails without the fix.

1. The read-side guard was swallowed by every fan-out. MultiKBRetriever.search, query_chunks_across_collections, AdvancedRAGMode._wrrf_retrieval and both DeepResearch retrieval paths wrap each per-collection search in except Exception: ... continue. The query embedding is computed once and reused for every collection, so a degenerate query made every collection fail and every failure was dropped — returning an empty result with success: true. That is the same silent degradation this PR exists to remove, and the single-KB path of the same MCP tools already returned success: false. Each site now re-raises EmbeddingFailedError ahead of its broad handler; an ordinary missing-collection failure is still tolerated.

2. The write-side guard was bypassed by callers that pre-embed. add_documents only validated vectors it produced itself, but capsule_builder, capsule_reader and local_docs set chunk.embedding before calling it. Those vectors went straight to coll.add. Every vector about to be written is now screened.

3. An empty chunk could still store a zero vector. A chunk with no text and no title composes to an empty embedding text, and CachedEmbeddingProvider — which is in the production factory — returns an aligned zero vector for it. The rule is now simply that no zero vector is ever stored: for a text with content it means the embedder failed, and for an empty text the chunk has nothing to retrieve.

4. Re-raising broke a resumable ingest. ingest_dois_into_kb marks each DOI "added" the moment it is retrieved, before the batch is embedded, and remaining_ids() never re-offers an "added" id. Previously the swallow let the run finish and delete the checkpoint; the re-raise aborted and left those DOIs marked as ingested for ever. On an embedding failure the marks are now undone, so a resume retries them. Chroma ignores duplicate ids, so re-ingesting any paper that did land is harmless.

Known consequence, not fixed here

add_papers writes per paper, so an embedding failure part-way through a batch leaves the earlier papers in Chroma while the caller's paper_count / chunk_count updates are skipped (github_kb.py, zotero_ingest.py, search_to_kb.py all update counts after the call returns). The stored vectors are valid, and GET /api/kb/{name}/stats recomputes chunk_count from coll.count(), so the visible numbers stay honest. bibtex_kb.py already handles this by deleting the collection and re-raising. Making the other three consistent is a separate change.

Suite after these fixes: 2418 passed, 0 failed.

Not addressed

• mcp/server.py still builds its embedding provider with use_local_fallback=True, so a remote failure falls back to 384-dim MiniLM while the KB is stamped with the 3072-dim primary model name. web/state.py already disables this deliberately, with a comment explaining the hazard. Worth a separate change.
• The per-item zero-fill on asbb/p0-kb-doc is fixed separately on that branch; merging it unfixed would reintroduce the silent write, which the new add_documents guard would then catch.