fix(retain): reject degenerate fact text before storage (#2520)#2794
fix(retain): reject degenerate fact text before storage (#2520)#2794handnewb wants to merge 2 commits into
Conversation
Facts with zero information content (empty strings, punctuation-only, LLM hallucination patterns like '...', '-', '--') were being stored, indexed, and surfaced in recall results. This adds a content quality guard in ProcessedFact.from_extracted_fact() that rejects degenerate text before it enters the storage pipeline. Closes vectorize-io#2520
ebarkhordar
left a comment
There was a problem hiding this comment.
Good catch on the degenerate-fact pollution. One thing to flag before this lands: making from_extracted_fact return None shortens processed_facts relative to extracted_facts, and two spots in orchestrator.py still pair those two lists positionally.
_extract_and_embed returns the full extracted_facts next to the now-filtered processed_facts, and the consumers that map chunk ids back onto facts zip them together:
- streaming/full retain:
orchestrator.py:1591for fact, processed_fact in zip(batch_extracted, batch_processed) - delta re-retain:
orchestrator.py:2185for ef, pf in zip(extracted_facts, processed_facts)
ProcessedFact has no chunk_index, so these loops read chunk_index off the extracted fact to look up the chunk id. Once a degenerate fact is dropped, the lists are off by one from that point, so every surviving fact after the dropped one gets the previous fact's chunk id, and zip silently drops the tail. The result mapping already guards this exact subset hazard (the comment at orchestrator.py:499-503, "unit_ids has 1:1 alignment with processed_facts ... any upstream drop would otherwise cause an IndexError, see #1037"), but these two zips still assume 1:1.
Reproduced against this PR's head (9c0fc5b) using your own ProcessedFact.from_extracted_fact plus the exact loop from :1591/:2185, three facts with a "..." in the middle:
len(extracted_facts) = 3
len(processed_facts) = 2 (degenerate dropped)
'Alice joined Acme in 2021' -> CID_for_chunk10
'Bob leads the ML team' -> CID_for_chunk11 (expected CID_for_chunk12)
I only exercised those two functions directly, not a full DB-backed retain, so I have not watched a wrong chunk_id get persisted. But the trigger is exactly the case this PR targets: the LLM emitting a "..."-style fact ahead of a real one in the same batch.
One option that keeps it simple is to filter extracted_facts in lockstep so the pair stays 1:1, or carry chunk_index onto ProcessedFact and iterate over processed_facts alone. A small regression test with a degenerate fact between two real ones would pin whichever shape you pick.
Summary
Facts with zero information content — empty strings, punctuation-only, and common LLM hallucination patterns (
...,-,--) — were being stored, embedded, BM25-indexed, and surfaced in recall results, polluting the memory bank.Changes
_is_degenerate_text()guard inProcessedFactthat rejects:from_extracted_fact()now returnsNonefor degenerate facts with a warning logorchestrator.pyandimporter.pyto filter outNoneresultsVerification
Closes #2520