From 00ccf0b218c2ec1a060b0b291a1bbccb02021c72 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Nicol=C3=B2=20Boschi?= Date: Mon, 9 Mar 2026 15:04:36 +0100 Subject: [PATCH] fix(consolidation): respect bank mission over ephemeral-state heuristic (#525) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * Add Hindsight as git subtree + BCGU noise filtering tests Adds hindsight server source as a subtree under hindsight-api/ so we can iterate on server-side fixes directly. test_bcgu_noise_filtering.py proves that a well-crafted retain_custom_instructions (BCGU_RETAIN_MISSION) can suppress talking-head noise at fact extraction time — eliminating the need for client-side --filter-vision-noise preprocessing. Tests cover: - Default mode extracts 3 noise facts from talking-head frame (problem documented) - BCGU mission produces 0 noise facts from same talking-head frame - BCGU mission still extracts 2 high-value ChatGPT screen facts correctly - Mixed doc (2 talking-head + 2 screen): 0% noise ratio with BCGU mission - Pure talking-head doc: 0 facts extracted All 5 tests pass in ~32s using gpt-4o-mini. * fix(consolidation): respect mission context over ephemeral-state heuristic Two related fixes for the consolidation engine when a bank mission is configured: 1. **Mission override for ephemeral-state filter** (`prompts.py`): The system prompt previously instructed the LLM to discard any fact that looked like "ephemeral state" (e.g. current position, transient actions). When a mission is active the mission itself defines what is valuable — timestamped screen actions, session events, tool interactions may all be mission-critical even though they look ephemeral. Added a MISSION OVERRIDE block that explicitly tells the LLM the mission takes priority over the generic ephemeral-state guidance. 2. **Remove contradictory durable-knowledge nudge** (`consolidator.py`): The user-prompt builder was injecting "Focus on DURABLE knowledge that serves this mission, not ephemeral state" alongside the mission text. This phrasing contradicted missions that intentionally capture timestamped events. Replaced with a neutral directive that simply signals the mission overrides general rules. 3. **JSON control-character sanitisation** (`consolidator.py`): LLMs occasionally embed literal ASCII control characters (0x00–0x1f) inside JSON string values, causing `json.loads` to raise a JSONDecodeError. Added a try/except that strips control characters and retries the parse before re-raising, preventing spurious failures. * refactor(consolidation): move sanitize_llm_output to llm_wrapper, reuse in consolidator - Add `sanitize_llm_output()` to `llm_wrapper.py` as the single canonical function for stripping characters that break downstream systems (ASCII control chars 0x00-0x08/0x0B-0x0C/0x0E-0x1F/0x7F and Unicode surrogates). Tab, newline, and carriage-return are preserved. - Reduce `_sanitize_text()` in `fact_extraction.py` to a thin wrapper that delegates to `sanitize_llm_output()`. - Update `consolidator.py` to import and call `sanitize_llm_output()` directly instead of reimplementing the logic inline. - Remove test_bcgu_noise_filtering.py (should not have been committed). * fix(consolidation): apply sanitize_llm_output to observation text fields sanitize_llm_output was imported but unused after the old _call_llm_once path was removed. The batch flow uses structured Pydantic output so there's no raw json.loads call — instead, apply sanitization via field_validator on _CreateAction.text and _UpdateAction.text so control characters are stripped before observation text reaches the database. * fix(entity-resolver): correct mention_count for new entities in batch retain When the same entity (e.g. "Bob") appears across N items in a single batch retain, _resolve_entities_batch_impl deduplicates them into one name group before inserting, then queued only ONE _EntityStat regardless of N. The flush therefore always incremented mention_count by 1 beyond the INSERT value — giving 2 for any number of mentions. Two-part fix: - INSERT with mention_count=0 so the post-transaction flush is the single source of truth for the count (avoids an off-by-one for N=1 as well). - Append one _EntityStat per original mention (len(g.indices)) instead of one per unique name, so flush_pending_stats() adds the correct total N. This makes the batch path consistent with the single-entity path, which already accumulates one stat per mention via entities_to_update. --- .../engine/consolidation/consolidator.py | 13 ++++++++++- .../engine/consolidation/prompts.py | 2 +- .../hindsight_api/engine/entity_resolver.py | 10 ++++++--- .../hindsight_api/engine/llm_wrapper.py | 22 +++++++++++++++++++ .../engine/retain/fact_extraction.py | 22 ++----------------- 5 files changed, 44 insertions(+), 25 deletions(-) diff --git a/hindsight-api/hindsight_api/engine/consolidation/consolidator.py b/hindsight-api/hindsight_api/engine/consolidation/consolidator.py index 276b98d8..80aafd42 100644 --- a/hindsight-api/hindsight_api/engine/consolidation/consolidator.py +++ b/hindsight-api/hindsight_api/engine/consolidation/consolidator.py @@ -24,9 +24,10 @@ from datetime import datetime, timezone from itertools import combinations from typing import TYPE_CHECKING, Any -from pydantic import BaseModel +from pydantic import BaseModel, field_validator from ...config import get_config +from ..llm_wrapper import sanitize_llm_output from ..memory_engine import fq_table from ..retain import embedding_utils from .prompts import build_batch_consolidation_prompt @@ -45,12 +46,22 @@ class _CreateAction(BaseModel): text: str source_fact_ids: list[str] # memory UUIDs from the NEW FACTS list + @field_validator("text", mode="before") + @classmethod + def sanitize_text(cls, v: str) -> str: + return sanitize_llm_output(v) or "" + class _UpdateAction(BaseModel): text: str observation_id: str # UUID of the existing observation to update source_fact_ids: list[str] # memory UUIDs from the NEW FACTS list + @field_validator("text", mode="before") + @classmethod + def sanitize_text(cls, v: str) -> str: + return sanitize_llm_output(v) or "" + class _DeleteAction(BaseModel): observation_id: str # UUID of the observation to remove diff --git a/hindsight-api/hindsight_api/engine/consolidation/prompts.py b/hindsight-api/hindsight_api/engine/consolidation/prompts.py index 67db5c48..17987a4b 100644 --- a/hindsight-api/hindsight_api/engine/consolidation/prompts.py +++ b/hindsight-api/hindsight_api/engine/consolidation/prompts.py @@ -29,7 +29,7 @@ Compare the facts against existing observations: - Same topic as an existing observation → UPDATE it (observation_id + source_fact_ids) - New topic with durable knowledge → CREATE a new observation (source_fact_ids) - Cross-reference facts within the batch: a later fact may resolve a vague reference in an earlier one -- Purely ephemeral facts → omit them (no create/update needed)""" +- Purely ephemeral facts → omit them unless the MISSION above explicitly targets such data (e.g. timestamped events, session state, screen content)""" # Output format — JSON braces escaped as {{ }} so .format() leaves them literal _BATCH_OUTPUT_FORMAT = """ diff --git a/hindsight-api/hindsight_api/engine/entity_resolver.py b/hindsight-api/hindsight_api/engine/entity_resolver.py index f166e8a8..5bb1421f 100644 --- a/hindsight-api/hindsight_api/engine/entity_resolver.py +++ b/hindsight-api/hindsight_api/engine/entity_resolver.py @@ -459,10 +459,12 @@ class EntityResolver: entity_dates = [g.event_date for _, g in sorted_groups] # INSERT ... ON CONFLICT DO NOTHING — no row lock on already-existing entities. + # mention_count starts at 0 here; flush_pending_stats() is the sole source of + # truth for mention counting (one stat per original mention in the batch). inserted_rows = await conn.fetch( f""" INSERT INTO {fq_table("entities")} (bank_id, canonical_name, first_seen, last_seen, mention_count) - SELECT $1, name, COALESCE(event_date, now()), COALESCE(event_date, now()), 1 + SELECT $1, name, COALESCE(event_date, now()), COALESCE(event_date, now()), 0 FROM unnest($2::text[], $3::timestamptz[]) AS t(name, event_date) ON CONFLICT (bank_id, LOWER(canonical_name)) DO NOTHING @@ -489,13 +491,15 @@ class EntityResolver: for row in existing_rows: id_by_name[row["name_lower"]] = row["id"] - # Assign entity IDs back and queue for post-txn stats flush. + # Assign entity IDs back and queue one stat per original mention so that + # flush_pending_stats() increments mention_count by the true mention count, + # not just 1 per unique name. for name_lower, g in sorted_groups: entity_id = id_by_name.get(name_lower) if entity_id: for original_idx in g.indices: entity_ids[original_idx] = entity_id - pending.append(_EntityStat(entity_id=entity_id, event_date=g.event_date)) + pending.append(_EntityStat(entity_id=entity_id, event_date=g.event_date)) # Accumulate into the resolver's pending list; the orchestrator flushes # these with await entity_resolver.flush_pending_stats() after the txn. diff --git a/hindsight-api/hindsight_api/engine/llm_wrapper.py b/hindsight-api/hindsight_api/engine/llm_wrapper.py index 8f1cab4d..8bfae4a8 100644 --- a/hindsight-api/hindsight_api/engine/llm_wrapper.py +++ b/hindsight-api/hindsight_api/engine/llm_wrapper.py @@ -48,6 +48,28 @@ _llm_max_concurrent = int(os.getenv(ENV_LLM_MAX_CONCURRENT, str(DEFAULT_LLM_MAX_ _global_llm_semaphore = asyncio.Semaphore(_llm_max_concurrent) +def sanitize_llm_output(text: str | None) -> str | None: + """ + Sanitize text by removing characters that break downstream systems. + + Removes: + - ASCII control characters (0x00-0x08, 0x0B-0x0C, 0x0E-0x1F, 0x7F): break + json.loads and PostgreSQL UTF-8 encoding; tab (0x09), newline (0x0A), and + carriage return (0x0D) are preserved as they are valid in text and JSON. + - Unicode surrogates (U+D800-U+DFFF): Invalid in UTF-8, break LLM APIs + + Surrogate characters are used in UTF-16 encoding but cannot be encoded + in UTF-8. They can appear in Python strings from improperly decoded data + (e.g., from JavaScript or broken files). Control characters commonly appear + in LLM output embedded inside JSON string values. + """ + if text is None: + return None + if not text: + return text + return re.sub(r"[\x00-\x08\x0b\x0c\x0e-\x1f\x7f\ud800-\udfff]", "", text) + + class OutputTooLongError(Exception): """ Bridge exception raised when LLM output exceeds token limits. diff --git a/hindsight-api/hindsight_api/engine/retain/fact_extraction.py b/hindsight-api/hindsight_api/engine/retain/fact_extraction.py index c6819933..f7c897b5 100644 --- a/hindsight-api/hindsight_api/engine/retain/fact_extraction.py +++ b/hindsight-api/hindsight_api/engine/retain/fact_extraction.py @@ -15,7 +15,7 @@ from typing import Literal, cast from pydantic import BaseModel, ConfigDict, Field, create_model, field_validator from ...config import get_config -from ..llm_wrapper import LLMConfig, OutputTooLongError +from ..llm_wrapper import LLMConfig, OutputTooLongError, sanitize_llm_output from ..response_models import TokenUsage from .entity_labels import ( EntityLabelsConfig, @@ -66,25 +66,7 @@ def _infer_temporal_date(fact_text: str, event_date: datetime | None) -> str | N def _sanitize_text(text: str | None) -> str | None: - """ - Sanitize text by removing characters that break downstream systems. - - Removes: - - Null bytes (\\x00): Invalid in PostgreSQL UTF-8 encoding - - Unicode surrogates (U+D800-U+DFFF): Invalid in UTF-8, break LLM APIs - - Surrogate characters are used in UTF-16 encoding but cannot be encoded - in UTF-8. They can appear in Python strings from improperly decoded data - (e.g., from JavaScript or broken files). Null bytes commonly appear in - OCR output, PDF extraction, or copy-paste from binary sources. - """ - if text is None: - return None - if not text: - return text - # Remove null bytes and surrogate characters - text = text.replace("\x00", "") - return re.sub(r"[\ud800-\udfff]", "", text) + return sanitize_llm_output(text) class Entity(BaseModel):