fix(retain): recover recoverable batch-extraction JSON instead of dropping facts#2720
Merged
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
The Batch API result parser in
hindsight-api-slim/hindsight_api/engine/retain/fact_extraction.py(inextract_facts_from_contents_batch_api) parsed each chunk's response with a barejson.loads(content_str). Onjson.JSONDecodeErrorit logged the failure, recorded the chunk withfact_count=0, andcontinued — so all facts in that chunk were permanently lost, with no recovery.Meanwhile a lenient parser already existed and was not used on this path:
parse_llm_json()inhindsight_api/engine/llm_wrapper.py. It strips markdown code fences (json …) and, on failure, retries after replacing embedded control characters (\x00-\x1f,\x7f) — the exact quirks some models (e.g. Gemini) emit inside JSON string values.Production evidence: transient Gemini batch parse failures that succeeded on manual reprocessing.
Fix (layer 1 — the cheap, immediate one)
Swap the bare
json.loads(content_str)in the batch result path for the existingparse_llm_json(content_str)helper:parse_llm_jsonreturns the parsed object and re-raisesjson.JSONDecodeErrorif the text is still unrecoverable, so the existing error path is preserved unchanged (recordcustom_id: failed to parse JSON, appendChunkMetadata(fact_count=0, …), continue). Only the successful-recovery path is new.Tests
Added to
tests/test_batch_api.py(pure mocked, no LLM):test_batch_api_recovers_fenced_and_control_char_json— batch content wrapped in ```json fences and containing a raw\x01control character inside a string value (asserted unparseable by bare `json.loads`); now yields `fact_count == 1` and the facts are present rather than lost.test_batch_api_unparseable_json_still_records_error— genuinely unparseable content still records the error and yieldsfact_count == 0(behavior preserved).Both pass:
Follow-up (out of scope here)
This is only "layer 1" (in-place lenient re-parse). A "layer 2" — resubmitting a still-unparseable chunk through the interactive (non-batch) extraction path for a full retry — is a larger change and is left as a documented follow-up.
Fixes #2701