From 03253d7a088df185c5a441463d5333b60cd4d49c Mon Sep 17 00:00:00 2001 From: Eugene Eisenstein Date: Mon, 31 Aug 2026 13:32:43 -0400 Subject: [PATCH] fix(deriver): strip NUL bytes from model-generated observations (#1095) * fix(deriver): strip NUL bytes from model-generated observations Postgres rejects NUL (0x00) in text columns and in jsonb strings. API ingress has always stripped it from user-supplied content, but the deriver's own output did not go through any equivalent: a model can emit a \u0000 escape in its tool-call arguments, which the JSON parser decodes into a real NUL byte. Seen in production when models transcribe shell output (`tr '\x00' '\n'`) or Windows paths (`c:\users\amal`). The NUL reached the exact-content dedup pre-fetch in create_documents as a bind parameter, so the query raised DataError before any row was written and the whole batch for that observer was dropped. Strip in _normalized_observation and _normalized_observation_input -- the points that already normalize text for persistence and embedding -- so the embedded text matches the stored text. premises and sources are covered too, since they ride along in internal_metadata. The emptiness check now runs after normalization, because str.strip() does not remove NUL and all-NUL content would otherwise be stored as an empty string. DocumentCreate.content gets a mode="before" validator as a backstop for callers that bypass those paths; running before the length constraint makes all-NUL content fail min_length rather than silently empty out. The NUL helpers move out of schemas/api.py into utils/sanitization.py as a single recursive strip_nul, so ingress and internal paths share one implementation. It is overloaded to keep str -> str for the callers that chain .strip(), and passes None through so optional fields need no guard. Fixes HONCHO-4XZ * fix: broaden nul strip check * chore: code simplification --------- Co-authored-by: Vineeth Voruganti <13438633+VVoruganti@users.noreply.github.com> --- src/crud/representation.py | 29 ++++- src/schemas/api.py | 31 +---- src/schemas/internal.py | 12 +- src/utils/agent_tools.py | 25 +++- src/utils/sanitization.py | 46 +++++++ tests/crud/test_representation_manager.py | 145 +++++++++++++++++++--- tests/test_schema_validations.py | 67 ++++++++++ tests/utils/test_sanitization.py | 38 ++++++ 8 files changed, 334 insertions(+), 59 deletions(-) create mode 100644 src/utils/sanitization.py create mode 100644 tests/utils/test_sanitization.py diff --git a/src/crud/representation.py b/src/crud/representation.py index 6fafb842..85b3e931 100644 --- a/src/crud/representation.py +++ b/src/crud/representation.py @@ -25,6 +25,7 @@ from src.utils.representation import ( Representation, allowlist_safe_levels, ) +from src.utils.sanitization import strip_nul from src.utils.types import embedding_call_purpose logger = logging.getLogger(__name__) @@ -38,10 +39,21 @@ def _observation_text(obs: ExplicitObservation | DeductiveObservation) -> str: def _normalized_observation( obs: ExplicitObservation | DeductiveObservation, ) -> ExplicitObservation | DeductiveObservation: - """Return an observation with its persisted/embed text normalized.""" - text = _observation_text(obs).strip() + """Return an observation with its persisted/embed text normalized. + + NUL bytes are removed here rather than closer to the database so that the + text that gets embedded is the same text that gets stored. + """ + text = strip_nul(_observation_text(obs)).strip() if isinstance(obs, DeductiveObservation): - return obs.model_copy(update={"conclusion": text}) + return obs.model_copy( + update={ + "conclusion": text, + # Premises ride along in internal_metadata, and jsonb rejects + # NUL in strings just as text columns do. + "premises": strip_nul(obs.premises), + } + ) return obs.model_copy(update={"content": text}) @@ -87,10 +99,15 @@ class RepresentationManager: logger.debug("No observations to save") return empty_result + # Normalize before the emptiness check: str.strip() does not remove + # NUL, so content that normalizes away has to be dropped afterwards. all_observations = [ - _normalized_observation(obs) - for obs in representation.deductive + representation.explicit - if _observation_text(obs).strip() + normalized + for normalized in ( + _normalized_observation(obs) + for obs in representation.deductive + representation.explicit + ) + if _observation_text(normalized) ] if not all_observations: logger.debug("No non-empty observations to save") diff --git a/src/schemas/api.py b/src/schemas/api.py index 43d91b26..34258d99 100644 --- a/src/schemas/api.py +++ b/src/schemas/api.py @@ -31,6 +31,7 @@ from src.schemas.configuration import ( SessionPeerConfig, WorkspaceConfiguration, ) +from src.utils.sanitization import NulStripped, strip_nul from src.utils.scopes import ( SCOPE_PEER_PREFIX, is_scope_peer_name, @@ -48,28 +49,6 @@ _METADATA_MAX_KEYS = 100 _METADATA_MAX_DEPTH = 5 -def _sanitize_value(v: Any) -> Any: - """Recursively strip NUL bytes from strings in nested data structures.""" - if isinstance(v, str): - return v.replace("\x00", "") - if isinstance(v, dict): - d = cast(dict[str, Any], v) - return {_sanitize_value(k): _sanitize_value(val) for k, val in d.items()} - if isinstance(v, list): - lst = cast(list[Any], v) - return [_sanitize_value(item) for item in lst] - return v - - -def _strip_nul(v: str) -> str: - """Strip NUL bytes from a string field (Postgres TEXT rejects \\x00).""" - return v.replace("\x00", "") - - -# Reusable annotation for query fields; composes with a per-field Field(...). -NulStripped = AfterValidator(_strip_nul) - - def _check_metadata_limits( data: dict[str, Any], *, @@ -97,7 +76,7 @@ def _validate_metadata(v: Any) -> Any: return v data = cast(dict[str, Any], v) _check_metadata_limits(data) - return _sanitize_value(data) + return strip_nul(data) _SanitizedMetadata = Annotated[dict[str, Any], BeforeValidator(_validate_metadata)] @@ -331,7 +310,7 @@ class PeerCardSet(BaseModel): def sanitize_peer_card(cls, v: Any) -> Any: if isinstance(v, list): return [ - item.replace("\x00", "") if isinstance(item, str) else item + strip_nul(item) if isinstance(item, str) else item for item in cast(list[Any], v) ] return v @@ -358,7 +337,7 @@ class MessageCreate(MessageBase): @field_validator("content", mode="after") @classmethod def sanitize_content(cls, v: str) -> str: - return v.replace("\x00", "") + return strip_nul(v) @property def encoded_message(self) -> list[int]: @@ -691,7 +670,7 @@ class ConclusionCreate(BaseModel): @field_validator("content", mode="after") @classmethod def sanitize_content(cls, v: str) -> str: - return v.replace("\x00", "") + return strip_nul(v) @model_validator(mode="after") def validate_token_count(self) -> Self: diff --git a/src/schemas/internal.py b/src/schemas/internal.py index e014431f..2d299feb 100644 --- a/src/schemas/internal.py +++ b/src/schemas/internal.py @@ -6,10 +6,11 @@ These are not part of the public API contract and may change without notice. from enum import Enum from typing import Annotated, Literal, Self -from pydantic import BaseModel, Field, field_validator, model_validator +from pydantic import BaseModel, Field, model_validator from src.schemas.api import MessageCreate from src.schemas.configuration import SessionPeerConfig +from src.utils.sanitization import NulStripped from src.utils.types import DocumentLevel @@ -59,7 +60,7 @@ class DocumentMetadata(BaseModel): class DocumentCreate(DocumentBase): - content: Annotated[str, Field(min_length=1, max_length=100000)] + content: Annotated[str, Field(min_length=1, max_length=100000), NulStripped] session_name: str | None = Field( default=None, description="The session from which the document was derived (NULL for global observations)", @@ -85,7 +86,7 @@ class DocumentCreate(DocumentBase): class ObservationInput(BaseModel): """Validated observation input from LLM tool calls.""" - content: Annotated[str, Field(min_length=1)] + content: Annotated[str, Field(min_length=1), NulStripped] level: DocumentLevel = "explicit" source_ids: list[str] | None = None premises: list[str] | None = None @@ -96,11 +97,6 @@ class ObservationInput(BaseModel): ) = None confidence: Literal["high", "medium", "low"] | None = None - @field_validator("content", mode="after") - @classmethod - def sanitize_content(cls, v: str) -> str: - return v.replace("\x00", "") - @model_validator(mode="after") def validate_level_fields(self) -> Self: """Validate that level-specific fields are present when required.""" diff --git a/src/utils/agent_tools.py b/src/utils/agent_tools.py index b753c462..de07e38f 100644 --- a/src/utils/agent_tools.py +++ b/src/utils/agent_tools.py @@ -36,6 +36,7 @@ from src.utils.representation import ( Representation, allowlist_safe_levels, ) +from src.utils.sanitization import strip_nul from src.utils.types import ToolResult, embedding_call_purpose, get_current_iteration logger = logging.getLogger(__name__) @@ -77,8 +78,20 @@ def _validate_peer_card_entry(line: str) -> bool: def _normalized_observation_input( obs: schemas.ObservationInput, ) -> schemas.ObservationInput: - """Return an observation input with content normalized for persistence/embedding.""" - return obs.model_copy(update={"content": obs.content.strip()}) + """Return an observation input with content normalized for persistence/embedding. + + NUL bytes are removed here rather than closer to the database so that the + text that gets embedded is the same text that gets stored. `premises` and + `sources` ride along in internal_metadata, and jsonb rejects NUL in strings + just as text columns do. + """ + return obs.model_copy( + update={ + "content": strip_nul(obs.content).strip(), + "premises": strip_nul(obs.premises), + "sources": strip_nul(obs.sources), + } + ) def _base_observation_properties() -> dict[str, Any]: @@ -986,10 +999,12 @@ async def create_observations( logger.warning("create_observations called with empty list") return ObservationsCreatedResult(created_count=0, created_levels=[], failed=[]) + # Normalize before the emptiness check: str.strip() does not remove NUL, + # so content that normalizes away has to be dropped afterwards. normalized_observations = [ - _normalized_observation_input(obs) - for obs in observations - if obs.content.strip() + normalized + for normalized in (_normalized_observation_input(obs) for obs in observations) + if normalized.content ] if not normalized_observations: logger.info("No non-empty observations to create") diff --git a/src/utils/sanitization.py b/src/utils/sanitization.py new file mode 100644 index 00000000..4879cc72 --- /dev/null +++ b/src/utils/sanitization.py @@ -0,0 +1,46 @@ +"""Helpers for stripping bytes Postgres cannot store in text columns. + +Postgres rejects NUL (0x00) in ``text``/``varchar`` values and in ``jsonb`` +strings, so any string bound into a query or persisted to those columns has to +have NUL removed first. This applies to model-generated text as much as to +user-supplied input: an LLM can emit a ``\\u0000`` escape in its tool-call +arguments, which the JSON parser decodes into a real NUL byte. +""" + +from typing import Any, cast, overload + +from pydantic import BeforeValidator + +__all__ = ["NulStripped", "strip_nul"] + + +@overload +def strip_nul(value: str) -> str: ... + + +@overload +def strip_nul(value: Any) -> Any: ... + + +def strip_nul(value: Any) -> Any: + """Recursively remove NUL bytes from strings, including nested ones. + + Dict keys are stripped alongside values. Anything that is not a string, + dict, or list -- ``None`` included -- is returned unchanged, so this can be + applied to an optional field without a guard. + """ + if isinstance(value, str): + return value.replace("\x00", "") + if isinstance(value, dict): + d = cast(dict[str, Any], value) + return {strip_nul(k): strip_nul(v) for k, v in d.items()} + if isinstance(value, list): + lst = cast(list[Any], value) + return [strip_nul(item) for item in lst] + return value + + +# Reusable annotation for string fields; composes with a per-field Field(...). +# Runs *before* the field's own constraints, so `min_length` is checked against +# the stripped value and all-NUL input is rejected instead of becoming "". +NulStripped = BeforeValidator(strip_nul) diff --git a/tests/crud/test_representation_manager.py b/tests/crud/test_representation_manager.py index 3f3d6f40..9392b78e 100644 --- a/tests/crud/test_representation_manager.py +++ b/tests/crud/test_representation_manager.py @@ -1,5 +1,5 @@ from contextlib import asynccontextmanager -from datetime import datetime, timedelta, timezone +from datetime import UTC, datetime, timedelta from unittest.mock import AsyncMock, MagicMock, patch import pytest @@ -196,7 +196,7 @@ class TestRepresentationManagerSoftDelete: db_session, test_workspace, test_peer ) - base = datetime(2026, 1, 1, tzinfo=timezone.utc) + base = datetime(2026, 1, 1, tzinfo=UTC) # Three conclusions, all reinforced once, inserted oldest-first. for i in range(3): db_session.add( @@ -484,13 +484,13 @@ class TestRepresentationManagerSave: explicit=[ ExplicitObservation( content=" ", - created_at=datetime.now(timezone.utc), + created_at=datetime.now(UTC), message_ids=[1], session_name="session", ), ExplicitObservation( content=" useful observation ", - created_at=datetime.now(timezone.utc), + created_at=datetime.now(UTC), message_ids=[1], session_name="session", ), @@ -515,7 +515,7 @@ class TestRepresentationManagerSave: representation, message_ids=[1], session_name="session", - message_created_at=datetime.now(timezone.utc), + message_created_at=datetime.now(UTC), message_level_configuration=_resolved_config(), ) @@ -540,7 +540,7 @@ class TestRepresentationManagerSave: conclusion=" ", premises=["premise a"], source_ids=["doc-a"], - created_at=datetime.now(timezone.utc), + created_at=datetime.now(UTC), message_ids=[1], session_name="session", ), @@ -548,7 +548,7 @@ class TestRepresentationManagerSave: conclusion=" inferred conclusion ", premises=["premise b"], source_ids=["doc-b"], - created_at=datetime.now(timezone.utc), + created_at=datetime.now(UTC), message_ids=[1], session_name="session", ), @@ -573,7 +573,7 @@ class TestRepresentationManagerSave: representation, message_ids=[1], session_name="session", - message_created_at=datetime.now(timezone.utc), + message_created_at=datetime.now(UTC), message_level_configuration=_resolved_config(), ) @@ -597,13 +597,13 @@ class TestRepresentationManagerSave: explicit=[ ExplicitObservation( content="", - created_at=datetime.now(timezone.utc), + created_at=datetime.now(UTC), message_ids=[1], session_name="session", ), ExplicitObservation( content="\n\t ", - created_at=datetime.now(timezone.utc), + created_at=datetime.now(UTC), message_ids=[1], session_name="session", ), @@ -626,7 +626,124 @@ class TestRepresentationManagerSave: representation, message_ids=[1], session_name="session", - message_created_at=datetime.now(timezone.utc), + message_created_at=datetime.now(UTC), + message_level_configuration=_resolved_config(), + ) + + assert len(saved.created_documents) == 0 + mock_embed.assert_not_awaited() + mock_save.assert_not_awaited() + + @pytest.mark.asyncio + async def test_save_representation_strips_nul_bytes(self): + """Models emit \\u0000 escapes when transcribing shell output or Windows + paths, and Postgres rejects NUL in text columns. The stripped text must + be what gets embedded as well as what gets stored.""" + manager = RepresentationManager( + "workspace", + observer="observer", + observed="observed", + ) + representation = Representation( + explicit=[ + ExplicitObservation( + content="ran 'cat /proc/1/environ | tr '\x00' '\\n''", + created_at=datetime.now(UTC), + message_ids=[1], + session_name="session", + ), + ], + deductive=[ + DeductiveObservation( + conclusion="the key is at c:\\\x00users\\amal", + premises=["saw c:\\\x00users in the prompt"], + created_at=datetime.now(UTC), + message_ids=[1], + session_name="session", + ), + ], + ) + + with ( + patch("src.crud.representation.tracked_db", _fake_tracked_db), + patch( + "src.crud.representation.embedding_client.simple_batch_embed", + new=AsyncMock(return_value=[[0.1], [0.2]]), + ) as mock_embed, + patch.object( + manager, + "_save_representation_internal", + new=AsyncMock( + return_value=CreateDocumentsResult(created_documents=[MagicMock()]) + ), + ) as mock_save, + ): + await manager.save_representation( + representation, + message_ids=[1], + session_name="session", + message_created_at=datetime.now(UTC), + message_level_configuration=_resolved_config(), + ) + + # Deductive observations are embedded ahead of explicit ones. + mock_embed.assert_awaited_once_with( + [ + "the key is at c:\\users\\amal", + "ran 'cat /proc/1/environ | tr '' '\\n''", + ], + on_oversize="truncate", + ) + + saved_observations = _saved_observations(mock_save) + deductive = next( + obs for obs in saved_observations if isinstance(obs, DeductiveObservation) + ) + explicit = next( + obs for obs in saved_observations if isinstance(obs, ExplicitObservation) + ) + assert explicit.content == "ran 'cat /proc/1/environ | tr '' '\\n''" + assert deductive.conclusion == "the key is at c:\\users\\amal" + # premises land in internal_metadata, and jsonb rejects NUL too + assert deductive.premises == ["saw c:\\users in the prompt"] + + @pytest.mark.asyncio + async def test_save_representation_skips_observations_that_are_only_nul(self): + """str.strip() does not remove NUL, so the emptiness check has to run + after normalization or an empty document gets written.""" + manager = RepresentationManager( + "workspace", + observer="observer", + observed="observed", + ) + representation = Representation( + explicit=[ + ExplicitObservation( + content="\x00\x00", + created_at=datetime.now(UTC), + message_ids=[1], + session_name="session", + ), + ] + ) + + with ( + patch("src.crud.representation.tracked_db", _fake_tracked_db), + patch( + "src.crud.representation.embedding_client.simple_batch_embed", + new=AsyncMock(), + ) as mock_embed, + patch.object( + manager, + "_save_representation_internal", + new=AsyncMock(), + ) as mock_save, + ): + saved = await manager.save_representation( + representation, + message_ids=[1], + session_name="session", + message_created_at=datetime.now(UTC), message_level_configuration=_resolved_config(), ) @@ -646,7 +763,7 @@ class TestRepresentationManagerSave: explicit=[ ExplicitObservation( content="short fact", - created_at=datetime.now(timezone.utc), + created_at=datetime.now(UTC), message_ids=[1], session_name="session", ) @@ -656,7 +773,7 @@ class TestRepresentationManagerSave: conclusion="inferred fact", premises=["premise"], source_ids=["doc-a"], - created_at=datetime.now(timezone.utc), + created_at=datetime.now(UTC), message_ids=[1], session_name="session", ) @@ -681,7 +798,7 @@ class TestRepresentationManagerSave: representation, message_ids=[1], session_name="session", - message_created_at=datetime.now(timezone.utc), + message_created_at=datetime.now(UTC), message_level_configuration=_resolved_config(), ) diff --git a/tests/test_schema_validations.py b/tests/test_schema_validations.py index 97b8b64a..e3a285f2 100644 --- a/tests/test_schema_validations.py +++ b/tests/test_schema_validations.py @@ -5,9 +5,11 @@ from pydantic import ValidationError from src.config import settings from src.schemas import ( + DialecticOptions, DocumentCreate, DocumentMetadata, MessageCreate, + ObservationInput, PeerCreate, ReasoningConfiguration, ResolvedConfiguration, @@ -275,3 +277,68 @@ class TestReasoningCustomInstructionsValidation: configuration = ReasoningConfiguration(custom_instructions=custom_instructions) assert configuration.custom_instructions == custom_instructions + + +class TestNulByteSanitization: + """Postgres rejects NUL (0x00) in text columns and in jsonb strings. + + Models emit these as `\\u0000` escapes in tool-call arguments, which the + JSON parser decodes into real NUL bytes, so model-generated text needs the + same treatment as user-supplied input. + """ + + def test_document_content_strips_nul(self): + document = DocumentCreate( + content="the key is at c:\\\x00users\\amal", + metadata=DocumentMetadata(message_ids=[1], message_created_at="2026-08-28"), + embedding=[0.1], + ) + + assert document.content == "the key is at c:\\users\\amal" + + def test_all_nul_document_content_is_rejected_not_emptied(self): + """The validator runs before `min_length`, so content that is nothing + but NUL fails validation rather than being stored as an empty string.""" + with pytest.raises(ValidationError): + DocumentCreate( + content="\x00\x00", + metadata=DocumentMetadata( + message_ids=[1], message_created_at="2026-08-28" + ), + embedding=[0.1], + ) + + def test_message_content_strips_nul(self): + message = MessageCreate(peer_id="peer", content="before\x00after") + + assert message.content == "beforeafter" + + def test_metadata_strips_nul_at_every_depth(self): + message = MessageCreate( + peer_id="peer", + content="hi", + metadata={"a\x00b": {"c": ["d\x00e", 1]}}, + ) + + assert message.metadata == {"ab": {"c": ["de", 1]}} + + def test_observation_content_strips_nul(self): + observation = ObservationInput(content="before\x00after") + + assert observation.content == "beforeafter" + + def test_all_nul_observation_content_is_rejected_not_emptied(self): + """Sanitization runs before `min_length`, so an all-NUL observation is + reported back to the model as a validation failure rather than saved + as an empty document.""" + with pytest.raises(ValidationError): + ObservationInput(content="\x00\x00") + + def test_all_nul_query_is_rejected_not_emptied(self): + """`NulStripped` runs before the field's own constraints, so a query + that is nothing but NUL fails `min_length` instead of reaching the + dialectic as an empty prompt.""" + options = DialecticOptions.model_validate({"query": "before\x00after"}) + assert options.query == "beforeafter" + with pytest.raises(ValidationError): + DialecticOptions.model_validate({"query": "\x00"}) diff --git a/tests/utils/test_sanitization.py b/tests/utils/test_sanitization.py new file mode 100644 index 00000000..dc84647f --- /dev/null +++ b/tests/utils/test_sanitization.py @@ -0,0 +1,38 @@ +from typing import Any + +import pytest + +from src.utils.sanitization import strip_nul + + +@pytest.mark.parametrize( + ("value", "expected"), + [ + pytest.param("before\x00after", "beforeafter", id="string"), + pytest.param("no nul here", "no nul here", id="string-unchanged"), + pytest.param("\x00\x00", "", id="string-all-nul"), + pytest.param(["a\x00b", "c"], ["ab", "c"], id="list"), + pytest.param({"k\x00": "v\x00"}, {"k": "v"}, id="dict-key-and-value"), + pytest.param( + {"a": [{"b": "c\x00d"}]}, + {"a": [{"b": "cd"}]}, + id="nested", + ), + # Optional fields are passed in without a guard, so None has to survive. + pytest.param(None, None, id="none"), + pytest.param(7, 7, id="int"), + pytest.param(True, True, id="bool"), + pytest.param([], [], id="empty-list"), + ], +) +def test_strip_nul(value: Any, expected: Any) -> None: + assert strip_nul(value) == expected + + +def test_strip_nul_does_not_mutate_its_argument() -> None: + original = {"a": ["b\x00c"]} + + stripped = strip_nul(original) + + assert stripped == {"a": ["bc"]} + assert original == {"a": ["b\x00c"]}