diff --git a/mempalace/collision_scan.py b/mempalace/collision_scan.py new file mode 100644 index 0000000..53819f2 --- /dev/null +++ b/mempalace/collision_scan.py @@ -0,0 +1,122 @@ +"""Pre-mining defense against drawer_id collisions. + +Runs immediately before a batched chromadb upsert. Computes the union of +incoming drawer_ids and existing drawer_ids that share a key with the +batch; raises ``CollisionError`` if any drawer_id appears more than once +in that union with conflicting ``(source_file, chunk_index)`` metadata. + +Under the v2 hash recipe (see :mod:`mempalace.ids`) accidental collisions +are vanishingly rare — SHA-256 truncated to 24 hex chars makes random +collision ~2^-96. The scan exists for two reasons: + +1. Catch upstream bugs that emit duplicate ``(source_file, chunk_index)`` + pairs in the same batch with conflicting content. ChromaDB would + silently let the last-write win; the scan surfaces it as an + actionable error naming both call sites. +2. Catch the astronomical-but-possible SHA-256 hash collision with a + clear message instead of a silent overwrite at upsert time. + +The scan does NOT fire on idempotent re-mines — when an incoming drawer +matches an existing one with the SAME ``(source_file, chunk_index)`` +metadata, that is normal re-write behavior, not collision. +""" + +from __future__ import annotations + +from collections import defaultdict + + +class CollisionError(Exception): + """Raised by :func:`assert_no_collisions` when the pre-mining scan + detects a drawer_id that would silently overwrite existing content + or duplicate within a batch with conflicting metadata. + + The exception message names every colliding ``drawer_id`` and the + full set of ``(source_file, chunk_index)`` pairs producing each one, + so a user fixing one collision does not have to rediscover the next + by re-running the mine. + """ + + +def _metadata_key(meta: dict) -> tuple: + """Reduce a drawer metadata dict to the tuple used for collision + discrimination. Two metadata dicts are 'the same chunk' iff their + key tuples match. Falls back to ``(source_file,)`` when + ``chunk_index`` is absent (diary entries, sentinels).""" + source_file = meta.get("source_file") + chunk_index = meta.get("chunk_index") + if chunk_index is None: + return (source_file,) + return (source_file, chunk_index) + + +def assert_no_collisions( + proposed: list[tuple[str, dict]], + collection, +) -> None: + """Abort the mine via ``CollisionError`` if any proposed drawer_id + collides with itself or with an existing drawer in ``collection``. + + Args: + proposed: list of ``(drawer_id, metadata)`` tuples for the + chunks about to be upserted. ``metadata`` must carry at + least ``source_file``; ``chunk_index`` is used when + present. + collection: a ChromaDB-shaped collection with ``get(ids=...)`` + returning a dict with ``ids`` and ``metadatas`` keys. + + Raises: + CollisionError: when a drawer_id maps to two or more distinct + ``(source_file, chunk_index)`` tuples in the union of + incoming and existing rows. + """ + if not proposed: + return + + # Build incoming map: drawer_id -> set of metadata key tuples. + # Using a set collapses duplicate-metadata cases (same chunk twice + # in the batch) without flagging them as collisions. + incoming: dict[str, set[tuple]] = defaultdict(set) + for drawer_id, meta in proposed: + incoming[drawer_id].add(_metadata_key(meta)) + + # Query existing rows for any incoming id. ChromaDB's get(ids=...) + # returns only the rows whose ids are present; missing ids are + # silently absent from the result, which is what we want. + incoming_ids = list(incoming.keys()) + result = collection.get(ids=incoming_ids, include=["metadatas"]) + existing_ids: list = result["ids"] if hasattr(result, "__getitem__") else [] + existing_metas: list = result["metadatas"] if existing_ids else [] + + # Merge existing metadata into the incoming map. A real collision is + # a drawer_id whose incoming + existing metadata key tuples are not + # all the same. + for drawer_id, meta in zip(existing_ids, existing_metas): + incoming[drawer_id].add(_metadata_key(meta or {})) + + collisions = {did: keys for did, keys in incoming.items() if len(keys) > 1} + if collisions: + raise CollisionError(_format_collisions(collisions)) + + +def _format_collisions(collisions: dict[str, set[tuple]]) -> str: + """Render a CollisionError message that enumerates every colliding + drawer_id and the metadata tuples producing it.""" + lines = [ + f"Pre-mining collision scan detected {len(collisions)} " + f"colliding drawer_id{'s' if len(collisions) != 1 else ''}:", + ] + for drawer_id, keys in sorted(collisions.items()): + lines.append(f" {drawer_id}:") + for key in sorted(keys, key=lambda k: tuple(str(part) for part in k)): + if len(key) == 1: + lines.append(f" source_file={key[0]!r}") + else: + lines.append(f" source_file={key[0]!r}, chunk_index={key[1]!r}") + lines.append( + "Each colliding drawer_id would cause the second ChromaDB upsert " + "to silently overwrite the first. Fix the upstream chunker / " + "miner to emit distinct keys, or investigate the SHA-256 hash " + "collision." + ) + return "\n".join(lines) diff --git a/mempalace/convo_miner.py b/mempalace/convo_miner.py index ee82e36..fc0b0b1 100644 --- a/mempalace/convo_miner.py +++ b/mempalace/convo_miner.py @@ -10,13 +10,14 @@ Same palace as project mining. Different ingest strategy. import os import sys -import hashlib import logging from pathlib import Path from datetime import datetime from collections import defaultdict from typing import Optional +from .collision_scan import assert_no_collisions +from .ids import ID_RECIPE, make_convo_drawer_id, make_convo_sentinel_id from .normalize import normalize from .palace import ( NORMALIZE_VERSION, @@ -83,8 +84,7 @@ def _register_file(collection, source_file: str, wing: str, agent: str, extract_ re-read and re-processed on every mine run because nothing was written to ChromaDB on the first pass. """ - sentinel_key = f"{source_file}:{extract_mode}" - sentinel_id = f"_reg_{hashlib.sha256(sentinel_key.encode()).hexdigest()[:24]}" + sentinel_id = make_convo_sentinel_id(source_file, extract_mode) collection.upsert( documents=[f"[registry] {source_file}"], ids=[sentinel_id], @@ -98,6 +98,7 @@ def _register_file(collection, source_file: str, wing: str, agent: str, extract_ "ingest_mode": "registry", "extract_mode": extract_mode, "normalize_version": NORMALIZE_VERSION, + "id_recipe": ID_RECIPE, } ], ) @@ -419,10 +420,8 @@ def _file_chunks_locked(collection, source_file, chunks, wing, room, agent, extr chunk_room = chunk.get("memory_type", room) if extract_mode == "general" else room if extract_mode == "general": room_counts_delta[chunk_room] += 1 - drawer_key = f"{source_file}:{extract_mode}:{chunk['chunk_index']}" - drawer_id = ( - f"drawer_{wing}_{chunk_room}_" - f"{hashlib.sha256(drawer_key.encode()).hexdigest()[:24]}" + drawer_id = make_convo_drawer_id( + wing, chunk_room, source_file, extract_mode, chunk["chunk_index"] ) batch_docs.append(chunk["content"]) batch_ids.append(drawer_id) @@ -438,8 +437,10 @@ def _file_chunks_locked(collection, source_file, chunks, wing, room, agent, extr "ingest_mode": "convos", "extract_mode": extract_mode, "normalize_version": NORMALIZE_VERSION, + "id_recipe": ID_RECIPE, } ) + assert_no_collisions(list(zip(batch_ids, batch_metas)), collection) try: collection.upsert( documents=batch_docs, diff --git a/mempalace/format_miner.py b/mempalace/format_miner.py index adb1aee..f64a490 100644 --- a/mempalace/format_miner.py +++ b/mempalace/format_miner.py @@ -82,6 +82,8 @@ from .palace import ( # mempalace.format_miner.. Lazy imports inside functions would not # expose these as attributes of this module, breaking the test seams. from .config import MempalaceConfig, normalize_wing_name +from .collision_scan import assert_no_collisions +from .ids import ID_RECIPE, make_drawer_id_from_chunk from .miner import ( _compute_topic_tunnels_for_wing, chunk_text, @@ -640,8 +642,7 @@ def _file_chunks_locked( batch_ids: list = [] batch_metas: list = [] for chunk in chunks[batch_start : batch_start + DRAWER_UPSERT_BATCH_SIZE]: - key = (source_file + str(chunk["chunk_index"])).encode() - drawer_id = f"drawer_{wing}_{room}_{hashlib.sha256(key).hexdigest()[:24]}" + drawer_id = make_drawer_id_from_chunk(wing, room, source_file, chunk["chunk_index"]) content = chunk["content"] meta: dict = { "wing": wing, @@ -654,6 +655,7 @@ def _file_chunks_locked( "extract_mode": "format", "normalize_version": NORMALIZE_VERSION, "hall": detect_hall(content), + "id_recipe": ID_RECIPE, } if source_mtime is not None: meta["source_mtime"] = source_mtime @@ -674,6 +676,7 @@ def _file_chunks_locked( batch_docs.append(content) batch_ids.append(drawer_id) batch_metas.append(meta) + assert_no_collisions(list(zip(batch_ids, batch_metas)), collection) try: collection.upsert( documents=batch_docs, diff --git a/mempalace/ids.py b/mempalace/ids.py new file mode 100644 index 0000000..ff20946 --- /dev/null +++ b/mempalace/ids.py @@ -0,0 +1,128 @@ +"""Centralized drawer/triple ID construction with collision-safe delimiter. + +Drawer IDs and content-addressed identifiers built by concatenating strings +without a delimiter before hashing form a defect class that allows +``hash(s1 + str(i1)) == hash(s2 + str(i2))`` whenever +``s1 + str(i1) == s2 + str(i2)``. Under ChromaDB's primary-key constraint +the second upsert silently overwrites the first, losing content with no +error raised. The styleguide's partial-scope-key-migration rule names this +shape — every concat-into-hash site is a candidate that must be triaged. + +This module is the single source of truth for ID construction in mempalace. +All call sites use the named helpers below; no module should inline +``hashlib.sha256(a + b)`` patterns. +""" + +from __future__ import annotations + +import hashlib + +# Recipe tag written to every drawer's metadata under this module's helpers. +# Audits compare like-for-like: drawers without ``id_recipe`` are treated +# as legacy ``v1`` (pre-delimiter recipe), drawers with ``id_recipe="v2"`` +# are guaranteed collision-safe within the v2 generation. The constant is +# exported so call sites use ``ids.ID_RECIPE`` rather than a magic string. +ID_RECIPE: str = "v2" + +# '|' is reserved in Windows filenames and cannot appear in source paths +# on any supported platform, making it strictly safer than ':' (which +# appears in Windows drive letters and URL ports). Matches the existing +# diary_ingest precedent at diary_ingest.py:52,76,91,98. +_DELIM: str = "|" + +# SHA-256 hex truncation lengths. Drawer IDs historically truncate at 24 +# chars; knowledge-graph triple IDs at 12. Preserved per-recipe so existing +# fixture comparisons that hard-code truncation length still parse. +_HASH_TRUNC_DRAWER: int = 24 +_HASH_TRUNC_TRIPLE: int = 12 + + +def _delimited_sha256(parts: tuple[object, ...], truncate: int) -> str: + """Hash parts joined by the unambiguous delimiter, truncate to N hex chars. + + Internal helper. Call sites should use the named ``make_*`` wrappers + below so the per-site contract is documented in code, not derived + from caller arguments. + + Each part is coerced to ``str`` before joining so the helper mirrors + the pre-v2 behavior of ``f"{a}{b}"`` for ``None`` and numeric inputs — + e.g. ``valid_from=None`` joins as the literal string ``"None"`` rather + than crashing. + """ + key = _DELIM.join(str(p) for p in parts).encode() + return hashlib.sha256(key).hexdigest()[:truncate] + + +def make_drawer_id_from_chunk(wing: str, room: str, source_file: str, chunk_index: int) -> str: + """Drawer ID for the project / format miner paths. + + Hash input is ``f"{source_file}|{chunk_index}"`` — the '|' separator + prevents the classic ``"/a1" + "23" == "/a" + "123"`` collision. + + Returns ``drawer_{wing}_{room}_{hash24}`` where hash24 is the first + 24 hex chars of SHA-256 over the delimited input. + """ + return ( + f"drawer_{wing}_{room}_" + f"{_delimited_sha256((source_file, str(chunk_index)), _HASH_TRUNC_DRAWER)}" + ) + + +def make_drawer_id_from_content(wing: str, room: str, content: str) -> str: + """Drawer ID for the MCP ``add_drawer`` tool path. + + Hash input is ``f"{wing}|{room}|{content}"`` — the delimiters prevent + ``wing="foo" + room="bar"`` colliding with ``wing="fooba" + room="r"`` + (architecturally identical defect class to the chunk-index sites, + even though astronomically rare in practice since content is large + freeform text). + """ + return f"drawer_{wing}_{room}_{_delimited_sha256((wing, room, content), _HASH_TRUNC_DRAWER)}" + + +def make_convo_drawer_id( + wing: str, room: str, source_file: str, extract_mode: str, chunk_index: int +) -> str: + """Drawer ID for the conversation miner path. + + Pre-v2 the convo miner used ':' as delimiter; this helper migrates + to '|' for codebase-wide consistency and to remove the Windows-path + / URL-source edge case that ':' carried. + + Hash input is ``f"{source_file}|{extract_mode}|{chunk_index}"``. + """ + return ( + f"drawer_{wing}_{room}_" + f"{_delimited_sha256((source_file, extract_mode, str(chunk_index)), _HASH_TRUNC_DRAWER)}" + ) + + +def make_convo_sentinel_id(source_file: str, extract_mode: str) -> str: + """Sentinel registry ID for the conversation miner zero-chunk-file path. + + Pre-v2 the sentinel used ':' as delimiter; this helper migrates to + '|' for the same reasons as ``make_convo_drawer_id``. + + Hash input is ``f"{source_file}|{extract_mode}"``. + """ + return f"_reg_{_delimited_sha256((source_file, extract_mode), _HASH_TRUNC_DRAWER)}" + + +def make_triple_id( + sub_id: str, predicate: str, obj_id: str, valid_from: str, recorded_at: str +) -> str: + """Triple ID for knowledge-graph insertion. + + Pre-v2 the recorded_at hash input was + ``f"{valid_from}{datetime.now().isoformat()}"`` with no delimiter — + two ISO datetimes concatenated could collide in principle (e.g. + ``valid_from="2026-01-01" + isoformat "T12:..."`` vs + ``valid_from="2026-01-01T12" + isoformat ":..."``). + + Returns ``t_{sub_id}_{predicate}_{obj_id}_{hash12}`` where hash12 is + the first 12 hex chars of SHA-256 over ``f"{valid_from}|{recorded_at}"``. + """ + return ( + f"t_{sub_id}_{predicate}_{obj_id}_" + f"{_delimited_sha256((valid_from, recorded_at), _HASH_TRUNC_TRIPLE)}" + ) diff --git a/mempalace/knowledge_graph.py b/mempalace/knowledge_graph.py index 774ee79..e3f075c 100644 --- a/mempalace/knowledge_graph.py +++ b/mempalace/knowledge_graph.py @@ -35,7 +35,6 @@ Usage: kg.invalidate("Max", "has_issue", "sports_injury", ended="2026-02-15") """ -import hashlib import json import os import sqlite3 @@ -44,6 +43,7 @@ from datetime import date, datetime from pathlib import Path from typing import Optional from .config import sanitize_iso_temporal +from .ids import make_triple_id DEFAULT_KG_PATH = os.path.expanduser("~/.mempalace/knowledge_graph.sqlite3") @@ -302,7 +302,9 @@ class KnowledgeGraph: if existing: return existing["id"] # Already exists and still valid - triple_id = f"t_{sub_id}_{pred}_{obj_id}_{hashlib.sha256(f'{valid_from}{datetime.now().isoformat()}'.encode()).hexdigest()[:12]}" + triple_id = make_triple_id( + sub_id, pred, obj_id, valid_from, datetime.now().isoformat() + ) conn.execute( """INSERT INTO triples ( id, subject, predicate, object, valid_from, valid_to, diff --git a/mempalace/mcp_server.py b/mempalace/mcp_server.py index 33dafd2..b943c30 100644 --- a/mempalace/mcp_server.py +++ b/mempalace/mcp_server.py @@ -85,6 +85,8 @@ from .palace_graph import ( # noqa: E402 ) from .knowledge_graph import KnowledgeGraph, DEFAULT_KG_PATH # noqa: E402 +from .collision_scan import assert_no_collisions # noqa: E402 +from .ids import ID_RECIPE, make_drawer_id_from_content # noqa: E402 def _init_logging() -> None: @@ -1132,9 +1134,7 @@ def tool_add_drawer( if not col: return _no_palace() - drawer_id = ( - f"drawer_{wing}_{room}_{hashlib.sha256((wing + room + content).encode()).hexdigest()[:24]}" - ) + drawer_id = make_drawer_id_from_content(wing, room, content) _wal_log( "add_drawer", @@ -1155,6 +1155,7 @@ def tool_add_drawer( "source_file": source_file or "", "added_by": added_by, "filed_at": datetime.now().isoformat(), + "id_recipe": ID_RECIPE, } # Idempotency. Three cases to detect a prior committed write: @@ -1216,6 +1217,7 @@ def tool_add_drawer( chunk_metas.append( {**base_meta, "chunk_index": chunk_idx, "parent_drawer_id": drawer_id} ) + assert_no_collisions(list(zip(chunk_ids, chunk_metas)), col) col.upsert(ids=chunk_ids, documents=chunk_docs, metadatas=chunk_metas) # Probe the LAST chunk id, not the first — its presence confirms # the whole batch landed, not just the leading row. diff --git a/mempalace/miner.py b/mempalace/miner.py index 32b0b7c..95a06b2 100644 --- a/mempalace/miner.py +++ b/mempalace/miner.py @@ -41,7 +41,9 @@ from .palace import ( # ``mempalace.miner.compute_hallways_for_wing``. The integration call # lives at the end of _mine_impl, alongside the existing # ``_compute_topic_tunnels_for_wing`` post-mine block. +from .collision_scan import assert_no_collisions from .hallways import compute_hallways_for_wing +from .ids import ID_RECIPE, make_drawer_id_from_chunk logger = logging.getLogger("mempalace_mcp") @@ -1225,6 +1227,7 @@ def _build_drawer_metadata( "added_by": agent, "filed_at": datetime.now().isoformat(), "normalize_version": NORMALIZE_VERSION, + "id_recipe": ID_RECIPE, } if source_mtime is not None: metadata["source_mtime"] = source_mtime @@ -1250,7 +1253,7 @@ def add_drawer( miner uses ``_build_drawer_metadata`` + a batched ``collection.upsert`` to amortize the embedding model's forward-pass cost across chunks. """ - drawer_id = f"drawer_{wing}_{room}_{hashlib.sha256((source_file + str(chunk_index)).encode()).hexdigest()[:24]}" + drawer_id = make_drawer_id_from_chunk(wing, room, source_file, chunk_index) try: source_mtime = os.path.getmtime(source_file) except OSError: @@ -1383,7 +1386,7 @@ def process_file( batch_ids: list = [] batch_metas: list = [] for chunk in chunks[batch_start : batch_start + DRAWER_UPSERT_BATCH_SIZE]: - drawer_id = f"drawer_{wing}_{room}_{hashlib.sha256((source_file + str(chunk['chunk_index'])).encode()).hexdigest()[:24]}" + drawer_id = make_drawer_id_from_chunk(wing, room, source_file, chunk["chunk_index"]) batch_docs.append(chunk["content"]) batch_ids.append(drawer_id) batch_metas.append( @@ -1400,6 +1403,7 @@ def process_file( content_date=file_content_date, ) ) + assert_no_collisions(list(zip(batch_ids, batch_metas)), collection) collection.upsert( documents=batch_docs, ids=batch_ids, @@ -1413,8 +1417,7 @@ def process_file( # fully replace the prior closets, not append to them. if closets_col and drawers_added > 0: drawer_ids = [ - f"drawer_{wing}_{room}_{hashlib.sha256((source_file + str(c['chunk_index'])).encode()).hexdigest()[:24]}" - for c in chunks + make_drawer_id_from_chunk(wing, room, source_file, c["chunk_index"]) for c in chunks ] # Pass drawer_metas so build_closet_lines can emit the Tier 6a # 4-segment pointer (``topic|entities|YYYY-MM-DD:Lstart-Lend|→ids``) diff --git a/tests/test_collision_scan.py b/tests/test_collision_scan.py new file mode 100644 index 0000000..3ac315d --- /dev/null +++ b/tests/test_collision_scan.py @@ -0,0 +1,228 @@ +"""Tests for mempalace.collision_scan — pre-mining defense against +drawer_id collisions. + +The scan runs immediately before batched chromadb upserts and aborts the +mine with an actionable error if any proposed drawer_id appears more than +once in the union of (incoming-vs-incoming) and (incoming-vs-existing). + +Under the v2 hash recipe these collisions are vanishingly rare in practice +— SHA-256 truncated to 24 hex chars makes accidental collision ~2^-96. +The scan's real value is (a) catching upstream bugs that emit duplicate +(source_file, chunk_index) pairs in the same batch, and (b) surfacing the +astronomical-but-possible SHA-256 collision with a clear error instead of +a silent overwrite at the ChromaDB upsert. +""" + +from __future__ import annotations + +from typing import Optional + +import pytest + +from mempalace.collision_scan import CollisionError, assert_no_collisions + + +class _MockGet: + """Stand-in for ChromaDB's get() result. Real ChromaDB returns a + dict-like with ``ids`` and ``metadatas`` keys; we mirror that shape.""" + + def __init__(self, ids: list[str], metadatas: list[dict]): + self._payload = {"ids": ids, "metadatas": metadatas} + + def __getitem__(self, key): + return self._payload[key] + + def get(self, key, default=None): + return self._payload.get(key, default) + + +class _MockCollection: + """Stand-in for a ChromaDB collection. Stores a fixed mapping of + drawer_id → metadata for the test to model 'existing palace state'. + ``get(ids=[...])`` returns only the rows whose ids are in storage. + """ + + def __init__(self, existing: Optional[dict[str, dict]] = None): + self._existing = existing or {} + + def get(self, ids=None, include=None, **kwargs): + ids = ids or [] + rows = [(did, self._existing[did]) for did in ids if did in self._existing] + return _MockGet( + ids=[did for did, _ in rows], + metadatas=[meta for _, meta in rows], + ) + + +# ── Happy path: no collisions ──────────────────────────────────────── + + +def test_assert_no_collisions_passes_for_clean_batch(): + """Distinct incoming ids + no overlap with existing = no error.""" + proposed = [ + ("drawer_a", {"source_file": "/file_a.md", "chunk_index": 0}), + ("drawer_b", {"source_file": "/file_a.md", "chunk_index": 1}), + ("drawer_c", {"source_file": "/file_b.md", "chunk_index": 0}), + ] + col = _MockCollection() + assert_no_collisions(proposed, col) is None + + +def test_assert_no_collisions_passes_for_clean_batch_with_existing_drawers(): + """Existing drawers with DIFFERENT ids than incoming = no error. + The scan only fires when an id appears more than once across the + union of incoming + existing.""" + proposed = [ + ("drawer_new_1", {"source_file": "/new.md", "chunk_index": 0}), + ] + col = _MockCollection( + existing={ + "drawer_old_1": {"source_file": "/old.md", "chunk_index": 0}, + "drawer_old_2": {"source_file": "/old.md", "chunk_index": 1}, + } + ) + assert_no_collisions(proposed, col) is None + + +def test_assert_no_collisions_treats_idempotent_re_mine_as_clean(): + """If incoming drawer_id matches an existing id AND the + (source_file, chunk_index) metadata also matches, that's a normal + re-mine of the same chunk — NOT a collision. The scan must let it + pass; otherwise re-mining a clean palace would always raise.""" + proposed = [ + ("drawer_same", {"source_file": "/file.md", "chunk_index": 5}), + ] + col = _MockCollection( + existing={ + "drawer_same": {"source_file": "/file.md", "chunk_index": 5}, + } + ) + assert_no_collisions(proposed, col) is None + + +# ── Incoming-vs-incoming collisions ────────────────────────────────── + + +def test_assert_no_collisions_raises_on_incoming_duplicate_with_different_metadata(): + """Two incoming chunks producing the same drawer_id with DIFFERENT + (source_file, chunk_index) pairs = an upstream bug or astronomical + SHA-256 hash collision. Either way, abort the mine.""" + proposed = [ + ("drawer_X", {"source_file": "/file_a.md", "chunk_index": 0}), + ("drawer_X", {"source_file": "/file_b.md", "chunk_index": 1}), + ] + col = _MockCollection() + with pytest.raises(CollisionError) as exc_info: + assert_no_collisions(proposed, col) + # Error message names the colliding (source_file, chunk_index) pairs + msg = str(exc_info.value) + assert "drawer_X" in msg + assert "/file_a.md" in msg + assert "/file_b.md" in msg + + +def test_assert_no_collisions_passes_on_incoming_duplicate_with_same_metadata(): + """Two incoming chunks with the SAME (source_file, chunk_index) + producing the same drawer_id = a duplicate chunk in the batch (still + an upstream bug, but the downstream collision damage is zero since + they'd write identical content). The scan does not fire on this + case because it's not the collision shape v2 was designed to + catch.""" + proposed = [ + ("drawer_X", {"source_file": "/file.md", "chunk_index": 5}), + ("drawer_X", {"source_file": "/file.md", "chunk_index": 5}), + ] + col = _MockCollection() + assert_no_collisions(proposed, col) is None + + +# ── Incoming-vs-existing collisions ────────────────────────────────── + + +def test_assert_no_collisions_raises_on_incoming_matching_existing_with_different_metadata(): + """Incoming chunk produces the same drawer_id as an existing drawer + whose stored (source_file, chunk_index) DIFFERS = SHA-256 collision + or recipe-version skew. Abort the mine so the upsert doesn't + silently overwrite the existing row.""" + proposed = [ + ("drawer_Y", {"source_file": "/incoming.md", "chunk_index": 3}), + ] + col = _MockCollection( + existing={ + "drawer_Y": {"source_file": "/existing.md", "chunk_index": 7}, + } + ) + with pytest.raises(CollisionError) as exc_info: + assert_no_collisions(proposed, col) + msg = str(exc_info.value) + assert "drawer_Y" in msg + assert "/incoming.md" in msg + assert "/existing.md" in msg + + +# ── Error message quality ──────────────────────────────────────────── + + +def test_collision_error_lists_all_collisions_not_just_first(): + """If a batch contains multiple distinct collisions, the error + surfaces ALL of them — a user fixing one and re-running shouldn't + rediscover the next one from scratch.""" + proposed = [ + ("drawer_A", {"source_file": "/f1.md", "chunk_index": 0}), + ("drawer_A", {"source_file": "/f2.md", "chunk_index": 0}), # collision 1 + ("drawer_B", {"source_file": "/f3.md", "chunk_index": 0}), + ("drawer_B", {"source_file": "/f4.md", "chunk_index": 0}), # collision 2 + ] + col = _MockCollection() + with pytest.raises(CollisionError) as exc_info: + assert_no_collisions(proposed, col) + msg = str(exc_info.value) + assert "drawer_A" in msg + assert "drawer_B" in msg + assert "/f1.md" in msg + assert "/f2.md" in msg + assert "/f3.md" in msg + assert "/f4.md" in msg + + +# ── Edge cases ─────────────────────────────────────────────────────── + + +def test_assert_no_collisions_passes_for_empty_batch(): + """An empty mining batch is trivially collision-free — the scan + must not raise on len(proposed) == 0 so callers don't have to guard + the call site.""" + assert_no_collisions([], _MockCollection()) is None + + +def test_assert_no_collisions_handles_metadata_without_chunk_index(): + """Some drawer types (e.g. diary entries, sentinels) don't carry + chunk_index. The scan should compare whatever metadata is present + without crashing on missing keys.""" + proposed = [ + ("drawer_diary_1", {"source_file": "/diary.md"}), + ] + col = _MockCollection( + existing={ + "drawer_diary_1": {"source_file": "/diary_other.md"}, + } + ) + with pytest.raises(CollisionError): + assert_no_collisions(proposed, col) + + +def test_assert_no_collisions_tolerates_chromadb_get_failure(): + """ChromaDB's get() can raise on transient backend errors. The + scan should NOT swallow those — the caller's broad-except can decide + whether to abort the mine or proceed. The scan's contract is + 'either confirms no collisions, or raises'. Hiding backend errors + behind a False is the silent-failure shape this PR was meant to + eliminate.""" + + class _RaisingCollection: + def get(self, **kwargs): + raise RuntimeError("chromadb is sad") + + proposed = [("drawer_X", {"source_file": "/f.md", "chunk_index": 0})] + with pytest.raises(RuntimeError, match="chromadb is sad"): + assert_no_collisions(proposed, _RaisingCollection()) diff --git a/tests/test_convo_miner_unit.py b/tests/test_convo_miner_unit.py index b14f8ab..253a313 100644 --- a/tests/test_convo_miner_unit.py +++ b/tests/test_convo_miner_unit.py @@ -385,6 +385,11 @@ class TestFileChunksLocked: def delete(self, *args, **kwargs): pass + def get(self, ids=None, include=None, **kwargs): + # Pre-mining collision scan probes the collection; empty + # palace under test, so nothing matches. + return {"ids": [], "metadatas": []} + def upsert(self, documents, ids, metadatas): self.batch_sizes.append(len(documents)) diff --git a/tests/test_ids.py b/tests/test_ids.py new file mode 100644 index 0000000..1037602 --- /dev/null +++ b/tests/test_ids.py @@ -0,0 +1,186 @@ +"""Tests for mempalace.ids — collision-safe ID construction. + +The RED test that pins this whole PR is +``test_make_drawer_id_from_chunk_does_not_collide_across_boundary`` — it +constructs the classic ``"/path/a1" + "23" == "/path/a" + "123"`` collision +shape and asserts the new delimiter-based recipe produces distinct IDs. +Against the pre-v2 recipe (no delimiter), this test FAILS. Against v2, +it PASSES. +""" + +from __future__ import annotations + +import hashlib + +from mempalace import ids + + +# ── ID_RECIPE constant ───────────────────────────────────────────────── + + +def test_id_recipe_constant_is_v2(): + """Audit code reads ids.ID_RECIPE to tag new drawers. The constant + must be the literal "v2" string; a typo here silently re-introduces + the ambiguity v2 was meant to fix.""" + assert ids.ID_RECIPE == "v2" + + +# ── make_drawer_id_from_chunk ───────────────────────────────────────── + + +def test_make_drawer_id_from_chunk_returns_expected_prefix(): + """Drawer IDs are namespaced by wing and room so cross-wing + collisions are impossible regardless of the hash slice.""" + result = ids.make_drawer_id_from_chunk("proj", "log", "/a", 0) + assert result.startswith("drawer_proj_log_") + + +def test_make_drawer_id_from_chunk_hash_length_is_24_hex(): + """The hash slice must be 24 hex chars to keep drawer IDs storable + in fixed-width metadata columns and to match the historical recipe + length.""" + result = ids.make_drawer_id_from_chunk("w", "r", "/file.md", 5) + hash_part = result.removeprefix("drawer_w_r_") + assert len(hash_part) == 24 + assert all(c in "0123456789abcdef" for c in hash_part) + + +def test_make_drawer_id_from_chunk_does_not_collide_across_boundary(): + """RED test pinning the whole PR. + + Classic collision: source_file="/path/a1" + chunk_index=23 produces + hash input "/path/a123" under the pre-v2 recipe. source_file="/path/a" + + chunk_index=123 produces the SAME "/path/a123" — same hash, same + drawer_id, second ChromaDB upsert overwrites the first. + + Under the v2 recipe (delimiter '|'), the two inputs become + "/path/a1|23" and "/path/a|123" — distinct strings, distinct + hashes, distinct drawer IDs. No collision. + """ + a = ids.make_drawer_id_from_chunk("w", "r", "/path/a1", 23) + b = ids.make_drawer_id_from_chunk("w", "r", "/path/a", 123) + assert a != b, f"Collision survived v2 recipe: {a!r} == {b!r}" + + +def test_make_drawer_id_from_chunk_is_deterministic(): + """Same inputs must always produce the same ID — re-mining a file + that hasn't changed must hit the same drawer slot, or + file_already_mined() loses its idempotency.""" + a = ids.make_drawer_id_from_chunk("w", "r", "/file.md", 7) + b = ids.make_drawer_id_from_chunk("w", "r", "/file.md", 7) + assert a == b + + +def test_make_drawer_id_from_chunk_windows_path_with_colon_does_not_collide(): + """Windows paths contain ':' in drive letters (C:\\Users\\...). The + v2 recipe uses '|' precisely so paths that contain ':' can never + align with a chunk index to collide. This test would FAIL on a + ':'-delimited recipe because Windows paths and URL-like paths + (https://host:8080) commonly end in ':digits'.""" + a = ids.make_drawer_id_from_chunk("w", "r", "C:\\Users\\foo", 5) + b = ids.make_drawer_id_from_chunk("w", "r", "C:\\Users\\foo:", 5) + assert a != b + + +# ── make_drawer_id_from_content ─────────────────────────────────────── + + +def test_make_drawer_id_from_content_does_not_collide_across_boundary(): + """mcp_server.py:1136 hashes wing+room+content with no delimiter. + Architecturally identical defect to the chunk-index sites: + wing="foo"+room="bar" hashes the same as wing="fooba"+room="r". + v2 delimiter breaks this.""" + a = ids.make_drawer_id_from_content("foo", "bar", "x") + b = ids.make_drawer_id_from_content("fooba", "r", "x") + assert a != b + + +def test_make_drawer_id_from_content_returns_expected_prefix(): + """Same namespacing pattern as the chunk-index helper.""" + result = ids.make_drawer_id_from_content("proj", "scratch", "hello") + assert result.startswith("drawer_proj_scratch_") + + +# ── make_convo_drawer_id ────────────────────────────────────────────── + + +def test_make_convo_drawer_id_does_not_collide_across_extract_mode_boundary(): + """convo_miner.py:422 hashes source_file+extract_mode+chunk_index. + Pre-v2 used ':' as delimiter — this test would still PASS on the ':' + recipe for clean inputs, but the migration to '|' is for + consistency with the chunk-index helpers and to remove the + Windows-path / URL-source edge case where ':' can appear in the + source_file itself.""" + a = ids.make_convo_drawer_id("w", "r", "/log.jsonl", "general", 5) + b = ids.make_convo_drawer_id("w", "r", "/log.jsonl", "extract", 5) + assert a != b + + +def test_make_convo_drawer_id_returns_expected_prefix(): + result = ids.make_convo_drawer_id("claude", "diary", "/c.jsonl", "general", 0) + assert result.startswith("drawer_claude_diary_") + + +# ── make_convo_sentinel_id ──────────────────────────────────────────── + + +def test_make_convo_sentinel_id_returns_expected_prefix(): + """Sentinel IDs are namespaced under '_reg_' so they can be + filtered out of normal drawer queries.""" + result = ids.make_convo_sentinel_id("/c.jsonl", "general") + assert result.startswith("_reg_") + + +def test_make_convo_sentinel_id_distinguishes_extract_modes(): + a = ids.make_convo_sentinel_id("/c.jsonl", "general") + b = ids.make_convo_sentinel_id("/c.jsonl", "extract") + assert a != b + + +# ── make_triple_id ──────────────────────────────────────────────────── + + +def test_make_triple_id_returns_expected_prefix(): + """Triple IDs prefix with 't_' and embed the subject/predicate/object + triple in the ID for grep-ability in SQLite.""" + result = ids.make_triple_id("sub1", "loves", "obj1", "2026-01-01", "2026-05-30T10:00:00") + assert result.startswith("t_sub1_loves_obj1_") + + +def test_make_triple_id_hash_length_is_12_hex(): + """Triple IDs historically truncate at 12 hex chars (vs 24 for + drawers) because the subject/predicate/object prefix already + supplies the bulk of the namespace.""" + result = ids.make_triple_id("s", "p", "o", "2026-01-01", "2026-05-30T10:00:00") + hash_part = result.removeprefix("t_s_p_o_") + assert len(hash_part) == 12 + + +def test_make_triple_id_does_not_collide_across_iso_datetime_boundary(): + """Pre-v2 hash input was f'{valid_from}{recorded_at}' with no + delimiter — two ISO datetimes concatenated could in principle + collide (valid_from='2026-01-01' + recorded_at='T12:00:00' == + valid_from='2026-01-01T12' + recorded_at=':00:00' for hash + purposes). v2 delimiter prevents this.""" + a = ids.make_triple_id("s", "p", "o", "2026-01-01", "T12:00:00") + b = ids.make_triple_id("s", "p", "o", "2026-01-01T12", ":00:00") + assert a != b + + +# ── _delimited_sha256 (private helper, smoke test only) ─────────────── + + +def test_private_delimited_sha256_uses_pipe_delimiter(): + """Confirms the implementation actually uses '|' and not ':' — a + subtle copy-paste from the diary_ingest precedent or a stale + ':' precedent from convo_miner could regress the delimiter without + breaking the higher-level tests.""" + result = ids._delimited_sha256(("a", "b"), 64) + expected = hashlib.sha256(b"a|b").hexdigest() + assert result == expected + + +def test_private_delimited_sha256_truncation_honoured(): + """Truncation argument actually shortens the hex output.""" + result = ids._delimited_sha256(("a", "b"), 8) + assert len(result) == 8