From f620809bed08b34b189c16baaffe656dd25c76af Mon Sep 17 00:00:00 2001 From: shushu Date: Sat, 9 May 2026 12:38:28 +0800 Subject: [PATCH 1/4] fix(mcp): clean lone surrogates before ChromaDB write (issue #1235) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit MCP clients can emit lone surrogates (\udc00–\udfff) that cause Python's str.encode('utf-8') to raise UnicodeEncodeError, which bubbles up as -32000 Internal Error from ChromaDB. Add _clean(text) helper that uses 'surrogatepass'/'replace' to remove lone surrogates before the string reaches ChromaDB. Apply it in tool_add_drawer and tool_diary_write, and use 'surrogatepass' error handler on the SHA256 hash inputs for defensive idempotency. --- mempalace/mcp_server.py | 23 +++++++++++++++++++++-- 1 file changed, 21 insertions(+), 2 deletions(-) diff --git a/mempalace/mcp_server.py b/mempalace/mcp_server.py index 5bfda3f..3c00ea9 100644 --- a/mempalace/mcp_server.py +++ b/mempalace/mcp_server.py @@ -152,6 +152,23 @@ _init_logging() logger = logging.getLogger("mempalace_mcp") +def _clean(text: str) -> str: + """Remove lone surrogates that break UTF-8 encoding (issue #1235). + + MCP clients (Claude Desktop, WorkBuddy) can emit lone surrogates + (``\\udc00``–``\\udfff``) when relaying binary-in-Unicode or + corrupted text. Python's ``str.encode('utf-8')`` raises + ``UnicodeEncodeError`` on these; ChromaDB's ``add()`` / + ``upsert()`` then crashes with -32000 Internal Error. + + Replace lone surrogates with U+FFFD (REPLACEMENT CHARACTER) so + the string is legal UTF-8 while preserving as much content as + possible. Paired surrogates (valid UTF-16) are left alone — + the Python interpreter already decoded them correctly. + """ + return text.encode("utf-8", "surrogatepass").decode("utf-8", "replace") + + def _parse_args(): parser = argparse.ArgumentParser(description="MemPalace MCP Server") parser.add_argument( @@ -1120,6 +1137,7 @@ def tool_add_drawer( wing = sanitize_name(wing, "wing") room = sanitize_name(room, "room") content = sanitize_content(content) + content = _clean(content) except ValueError as e: return {"success": False, "error": str(e)} @@ -1128,7 +1146,7 @@ def tool_add_drawer( return _no_palace() drawer_id = ( - f"drawer_{wing}_{room}_{hashlib.sha256((wing + room + content).encode()).hexdigest()[:24]}" + f"drawer_{wing}_{room}_{hashlib.sha256((wing + room + content).encode('utf-8', 'surrogatepass')).hexdigest()[:24]}" ) _wal_log( @@ -1611,6 +1629,7 @@ def tool_diary_write(agent_name: str, entry: str, topic: str = "general", wing: try: agent_name = sanitize_name(agent_name, "agent_name").lower() entry = sanitize_content(entry) + entry = _clean(entry) topic = sanitize_name(topic, "topic") except ValueError as e: return {"success": False, "error": str(e)} @@ -1627,7 +1646,7 @@ def tool_diary_write(agent_name: str, entry: str, topic: str = "general", wing: now = datetime.now() entry_id = ( f"diary_{wing}_{now.strftime('%Y%m%d_%H%M%S%f')}_" - f"{hashlib.sha256(entry.encode()).hexdigest()[:12]}" + f"{hashlib.sha256(entry.encode('utf-8', 'surrogatepass')).hexdigest()[:12]}" ) _wal_log( From 43457726f8e8a7f6676ee1f95ad91aadecafa109 Mon Sep 17 00:00:00 2001 From: shushu Date: Sat, 9 May 2026 13:12:49 +0800 Subject: [PATCH 2/4] fix(mcp): address code review feedback (PR #1422) - Fix _clean() docstring (paired surrogate description was misleading) - Apply _clean() to source_file and added_by metadata fields - Remove redundant surrogatepass from SHA256 hashes (content already cleaned) - Apply _clean() to content in tool_check_duplicate - Apply _clean() to new_doc in tool_update_drawer - Apply _clean() to sanitized query in tool_search Addresses feedback from gemini-code-assist[bot] on PR #1422. --- mempalace/mcp_server.py | 19 +++++++++++++++---- 1 file changed, 15 insertions(+), 4 deletions(-) diff --git a/mempalace/mcp_server.py b/mempalace/mcp_server.py index 3c00ea9..397d66e 100644 --- a/mempalace/mcp_server.py +++ b/mempalace/mcp_server.py @@ -163,8 +163,13 @@ def _clean(text: str) -> str: Replace lone surrogates with U+FFFD (REPLACEMENT CHARACTER) so the string is legal UTF-8 while preserving as much content as - possible. Paired surrogates (valid UTF-16) are left alone — - the Python interpreter already decoded them correctly. + possible. Note: ``encode('utf-8', 'surrogatepass')`` also + treats suspected paired surrogates (individual ``\ud800``–``\udfff`` + code points) as lone surrogates, replacing them with U+FFFD. + This is correct — a Python ``str`` containing such code points + is already corrupt from the perspective of the UTF-8 channel. + Properly decoded characters (e.g. a real emoji) contain no + surrogates and pass through unchanged. """ return text.encode("utf-8", "surrogatepass").decode("utf-8", "replace") @@ -911,6 +916,7 @@ def tool_search( dist = (1.0 - min_similarity) if min_similarity is not None else max_distance # Mitigate system prompt contamination (Issue #333) sanitized = sanitize_query(query) + sanitized["clean_query"] = _clean(sanitized["clean_query"]) # Ensure the vector-disabled probe has been run via the safe # sqlite/pickle path before we touch chromadb. Calling _get_client() # here would defeat the fallback — it constructs a PersistentClient @@ -981,6 +987,7 @@ def tool_check_duplicate(content: str, threshold: float = 0.9): if not col: return _no_palace() try: + content = _clean(content) results = col.query( query_texts=[content], n_results=5, @@ -1138,6 +1145,9 @@ def tool_add_drawer( room = sanitize_name(room, "room") content = sanitize_content(content) content = _clean(content) + if source_file: + source_file = _clean(source_file) + added_by = _clean(added_by) except ValueError as e: return {"success": False, "error": str(e)} @@ -1146,7 +1156,7 @@ def tool_add_drawer( return _no_palace() drawer_id = ( - f"drawer_{wing}_{room}_{hashlib.sha256((wing + room + content).encode('utf-8', 'surrogatepass')).hexdigest()[:24]}" + f"drawer_{wing}_{room}_{hashlib.sha256((wing + room + content).encode()).hexdigest()[:24]}" ) _wal_log( @@ -1435,6 +1445,7 @@ def tool_update_drawer(drawer_id: str, content: str = None, wing: str = None, ro if content is not None: try: new_doc = sanitize_content(content) + new_doc = _clean(new_doc) except ValueError as e: return {"success": False, "error": str(e)} @@ -1646,7 +1657,7 @@ def tool_diary_write(agent_name: str, entry: str, topic: str = "general", wing: now = datetime.now() entry_id = ( f"diary_{wing}_{now.strftime('%Y%m%d_%H%M%S%f')}_" - f"{hashlib.sha256(entry.encode('utf-8', 'surrogatepass')).hexdigest()[:12]}" + f"{hashlib.sha256(entry.encode()).hexdigest()[:12]}" ) _wal_log( From 997ec1b22b7db25689bc102b187b0f2d08af1110 Mon Sep 17 00:00:00 2001 From: shushu Date: Mon, 18 May 2026 08:56:49 +0800 Subject: [PATCH 3/4] test(mcp): add lone-surrogate sanitisation tests (issue #1235) Add tests/test_clean_lone_surrogates.py covering: Unit tests (TestCleanLoneSurrogates, 11 cases): - _clean() passes normal ASCII and CJK strings unchanged - lone surrogates (high/low, single/multiple) are replaced with U+FFFD - real emoji (\U0001f600 astral code points) pass through unchanged - empty string, all-surrogate string, SHA-256-hash-after-clean - the specific \udcad surrogate observed in WorkBuddy production logs Integration tests (TestLoneSurrogateCleaning, 6 cases): - tool_add_drawer: surrogate in content and in metadata fields - tool_check_duplicate: surrogate in query - tool_search: surrogate in search query - tool_update_drawer: surrogate in updated content - tool_diary_write: surrogate in diary entry Fix test environment issue: - conftest.py redirects HOME to a temp dir, causing chromadb's ONNXMiniLM_L6_V2 to look for its ONNX model in the wrong location and trigger a 79 MB network download on every run. - Fix: at module import time, recover the real USERPROFILE from conftest._original_env and patch ONNXMiniLM_L6_V2.DOWNLOAD_PATH before any ChromaDB collection fixture is invoked. --- tests/test_clean_lone_surrogates.py | 330 ++++++++++++++++++++++++++++ 1 file changed, 330 insertions(+) create mode 100644 tests/test_clean_lone_surrogates.py diff --git a/tests/test_clean_lone_surrogates.py b/tests/test_clean_lone_surrogates.py new file mode 100644 index 0000000..027ab70 --- /dev/null +++ b/tests/test_clean_lone_surrogates.py @@ -0,0 +1,330 @@ +""" +Tests for lone-surrogate sanitisation in MCP server tool handlers (issue #1235). + +Covers: +- Unit: _clean() helper edge cases (lone surrogates, valid pairs, empty, etc.) +- Integration: all 5 tool paths that call _clean() receive surrogate payloads + +Note on U+FFFD replacement count +---------------------------------- +``str.encode('utf-8', 'surrogatepass')`` encodes each lone surrogate into the +3-byte CESU-8 sequence (0xED 0xAx 0xBx). When that byte sequence is decoded +with ``errors='replace'``, *each individual invalid byte* is swapped for one +U+FFFD, yielding **3** replacement characters per lone surrogate. Tests must +assert ``"\ufffd" * 3`` (i.e. ``\ufffd\ufffd\ufffd``) for every lone surrogate +that was stripped, not a single ``\ufffd``. + + Note on surrogate pairs in Python source +------------------------------------------ +Writing ``"\\ud83d\\ude00"`` in Python source code creates a string with *two* +independent lone-surrogate code points (U+D83D and U+DE00). Python 3 does not +silently merge them into the astral emoji U+1F600. Therefore these strings +are treated as lone surrogates by ``_clean()`` and are replaced, just like any +other lone surrogate. To embed a real emoji use the ``\\U`` escape: +``"\\U0001f600"`` (one code point, no surrogates). +""" + +import hashlib +import os +import sys +from pathlib import Path + +import pytest + +from mempalace.mcp_server import _clean + + +# ── ONNX model cache fix ─────────────────────────────────────────────────── +# conftest.py redirects HOME to a temp directory *at module level*, before any +# chromadb import, so that mempalace module-level initialisations don't touch +# the real user profile. Unfortunately this causes chromadb's ONNXMiniLM_L6_V2 +# to compute its DOWNLOAD_PATH (``Path.home() / ".cache" / "chroma" / ...``) +# pointing at the temp dir, triggering a 79 MB model download on every run. +# +# Fix: at module import time (i.e. right now, before any fixture), retrieve the +# *original* USERPROFILE that conftest saved in ``_original_env``, and use it +# to patch DOWNLOAD_PATH back to the real user cache. The patch is applied at +# module level so it takes effect before the conftest ``collection`` fixture +# creates a ChromaDB client. + +def _get_real_chroma_cache() -> Path: + """Return the real user's chroma ONNX model cache path. + + conftest.py stores the original env in ``_original_env`` before + redirecting HOME. We reach into that dict to recover the real + USERPROFILE / HOME so we can locate the pre-downloaded model. + Falls back to the current HOME if conftest is not loaded yet. + """ + conftest = sys.modules.get("conftest") + if conftest is not None: + orig_env = getattr(conftest, "_original_env", {}) + real_home = ( + orig_env.get("USERPROFILE") + or orig_env.get("HOME") + ) + if real_home: + return Path(real_home) / ".cache" / "chroma" / "onnx_models" / "all-MiniLM-L6-v2" + # Fallback: use current env (works when conftest doesn't redirect HOME) + return Path(os.environ.get("USERPROFILE", os.path.expanduser("~"))) / ".cache" / "chroma" / "onnx_models" / "all-MiniLM-L6-v2" + + +def _patch_onnx_download_path() -> None: + """Patch ONNXMiniLM_L6_V2.DOWNLOAD_PATH to the real user cache (once). + + Called at module import time so the patch is in place before any ChromaDB + collection is created inside a fixture. + """ + try: + from chromadb.utils.embedding_functions.onnx_mini_lm_l6_v2 import ONNXMiniLM_L6_V2 + except ImportError: + return + real_cache = _get_real_chroma_cache() + if real_cache.exists(): + ONNXMiniLM_L6_V2.DOWNLOAD_PATH = real_cache + + +_patch_onnx_download_path() + + +# ── Unit tests for _clean() ──────────────────────────────────────────────── + +class TestCleanLoneSurrogates: + """Unit tests for the _clean() helper.""" + + def test_clean_passthrough_normal(self): + """Normal ASCII + CJK strings pass through unchanged.""" + assert _clean("hello world") == "hello world" + assert _clean("你好世界") == "你好世界" + assert _clean("mixed 中 English 文") == "mixed 中 English 文" + + def test_clean_removes_high_surrogate(self): + """U+DC00–U+DFFF lone surrogates are replaced. + + Each lone surrogate encodes to 3 CESU-8 bytes; decode('replace') then + emits one U+FFFD per invalid byte, giving 3 replacement chars. + """ + # one lone surrogate → 3 × U+FFFD + assert _clean("hello\udc95world") == "hello\ufffd\ufffd\ufffdworld" + # three lone surrogates → 9 × U+FFFD + assert _clean("\udcff\udc00\udcaf") == "\ufffd" * 9 + + def test_clean_removes_low_surrogate(self): + """U+D800–U+DBFF lone surrogates are replaced.""" + assert _clean("test\ud800more") == "test\ufffd\ufffd\ufffdmore" + assert _clean("\ud800\udbff") == "\ufffd" * 6 + + def test_clean_removes_multiple_surrogates(self): + """Multiple surrogates at different positions are all replaced.""" + result = _clean("a\udca1b\udcffc") + # each surrogate → 3 U+FFFD + assert result == "a" + "\ufffd" * 3 + "b" + "\ufffd" * 3 + "c" + assert "\ufffd" in result + + def test_clean_preserves_real_emoji(self): + """Real emoji (astral code points via \\U escape) pass through unchanged. + + Note: ``\\ud83d\\ude00`` written in Python source is NOT the emoji — it + is two lone surrogates. Use ``\\U0001f600`` for the real grinning face. + """ + # Grinning face U+1F600 + assert _clean("\U0001f600") == "\U0001f600" + # Rocket U+1F680 + assert _clean("\U0001f680") == "\U0001f680" + # Emoji in sentence + assert _clean("hello \U0001f600 world") == "hello \U0001f600 world" + + def test_clean_emoji_with_trailing_lone_surrogate(self): + """Real emoji followed by a lone surrogate: emoji preserved, lone replaced.""" + # U+1F600 is one code point (no surrogates); \udc95 is a lone surrogate → 3 U+FFFD + result = _clean("\U0001f600\udc95") + assert result == "\U0001f600" + "\ufffd" * 3 + assert "\U0001f600" in result + assert "\ufffd" in result + + def test_clean_emoji_with_leading_lone_surrogate(self): + """Real emoji preceded by a lone surrogate: lone replaced, emoji preserved.""" + result = _clean("\udc95\U0001f600") + assert result == "\ufffd" * 3 + "\U0001f600" + assert "\U0001f600" in result + + def test_clean_empty_string(self): + """Empty string is returned unchanged, not crashed.""" + assert _clean("") == "" + + def test_clean_only_surrogates(self): + """String containing only lone surrogates becomes all U+FFFD (3 each).""" + assert _clean("\udc95\udcff") == "\ufffd" * 6 + assert _clean("\ud800\udbff\udc00\udfff") == "\ufffd" * 12 + + def test_clean_sha256_hashable_after(self): + """Cleaned string can be hashed with SHA256 (the actual crash path).""" + dirty = "content\udc95with\ud800surrogate" + clean = _clean(dirty) + # Must not raise UnicodeEncodeError + h = hashlib.sha256(clean.encode("utf-8")).hexdigest() + assert len(h) == 64 + + def test_clean_workbuddy_injected_surrogate(self): + """ + The specific surrogate WorkBuddy injects during MCP relay. + + MCP clients can emit lone surrogates when relaying binary-in-Unicode + or corrupted text. \\udcad is the character observed in production logs. + Each lone surrogate → 3 U+FFFD after the encode/decode round-trip. + """ + result = _clean("2026-04-27\udcadworkBuddy relay") + assert result == "2026-04-27" + "\ufffd" * 3 + "workBuddy relay" + # SHA256 must not crash (was the original failure mode) + hashlib.sha256(result.encode()).hexdigest() + + +# ── Integration tests for tool paths ──────────────────────────────────────── + +def _patch_mcp_server(monkeypatch, config, kg): + """Swap production config and KG with test doubles (mirrors test_mcp_server.py).""" + from mempalace import mcp_server + + monkeypatch.setattr(mcp_server, "_config", config) + monkeypatch.setattr(mcp_server, "_get_kg", lambda: kg) + + +def _get_collection(palace_path, create=False): + """Helper to open the test ChromaDB collection.""" + import chromadb + + client = chromadb.PersistentClient(path=str(palace_path)) + if create: + return ( + client, + client.get_or_create_collection( + "mempalace_drawers", + metadata={"hnsw:space": "cosine"}, + ), + ) + return client, client.get_collection("mempalace_drawers") + + +class TestLoneSurrogateCleaning: + """Integration tests: lone surrogates flow through all 5 tool handler paths.""" + + def test_add_drawer_with_surrogate_in_content( + self, monkeypatch, collection, config, kg + ): + """ + tool_add_drawer: _clean(content) prevents ChromaDB UnicodeEncodeError. + + The original crash occurred in ChromaDB's upsert() when the content + string contained lone surrogates injected by the MCP client. + """ + _patch_mcp_server(monkeypatch, config, kg) + + from mempalace.mcp_server import tool_add_drawer + + result = tool_add_drawer( + wing="test", + room="surrogate", + content="drawer content with \udc95 surrogate", + ) + assert result["success"] is True + assert "drawer_id" in result + assert result["drawer_id"].startswith("drawer_test_surrogate_") + + def test_add_drawer_with_surrogate_in_metadata( + self, monkeypatch, collection, config, kg + ): + """ + tool_add_drawer: source_file and added_by also go through _clean(). + + These metadata fields can carry paths or names that contain + surrogates if the user's environment generates them. + """ + _patch_mcp_server(monkeypatch, config, kg) + + from mempalace.mcp_server import tool_add_drawer + + result = tool_add_drawer( + wing="test", + room="meta", + content="content here", + source_file="path/to/\udcadfile.txt", + added_by="user\udc95agent", + ) + assert result["success"] is True + + def test_check_duplicate_with_surrogate( + self, monkeypatch, collection, config, kg + ): + """ + tool_check_duplicate: _clean(content) before ChromaDB query(). + + Without _clean(), col.query() raises UnicodeEncodeError when the + query string contains lone surrogates. + """ + _patch_mcp_server(monkeypatch, config, kg) + + from mempalace.mcp_server import tool_check_duplicate + + result = tool_check_duplicate(content="exact\udc95match") + # Must not raise; UnicodeEncodeError would surface as a traceback + assert isinstance(result, dict) + assert "is_duplicate" in result or "error" in result or result.get("results") is not None + + def test_search_with_surrogate_in_query( + self, monkeypatch, collection, config, kg + ): + """ + tool_search: _clean(query) before ChromaDB query(). + + User search queries containing lone surrogates (e.g. pasted from + corrupted clipboard) should not crash the search path. + """ + _patch_mcp_server(monkeypatch, config, kg) + + from mempalace.mcp_server import tool_search + + result = tool_search(query="search\udc95term") + assert isinstance(result, dict) + assert "success" in result or "error" in result or "results" in result + + def test_update_drawer_with_surrogate( + self, monkeypatch, collection, config, kg + ): + """ + tool_update_drawer: _clean(new_doc) before ChromaDB upsert(). + + Updating a drawer's content with surrogate-laden text must persist + cleanly without ChromaDB crashes. + """ + _patch_mcp_server(monkeypatch, config, kg) + + from mempalace.mcp_server import tool_add_drawer, tool_update_drawer + + add_result = tool_add_drawer(wing="test", room="update", content="original content") + assert add_result["success"] is True + drawer_id = add_result["drawer_id"] + + update_result = tool_update_drawer( + drawer_id=drawer_id, + content="updated\udc95content", + ) + assert update_result["success"] is True + + def test_diary_write_with_surrogate( + self, monkeypatch, collection, config, kg + ): + """ + tool_diary_write: _clean(entry) before KG/ChromaDB write. + + Diary entries containing surrogates (e.g. copied from a corrupted + document) must be accepted without crashing. + """ + _patch_mcp_server(monkeypatch, config, kg) + + from mempalace.mcp_server import tool_diary_write + + result = tool_diary_write( + agent_name="数数", + entry="今日工作\udc95完成了，修复了Chromadb crash", + topic="log", + ) + assert result["success"] is True From 3e001921eb8feef33625dca7a9ea2ef14c115865 Mon Sep 17 00:00:00 2001 From: Igor Lins e Silva <4753812+igorls@users.noreply.github.com> Date: Sun, 24 May 2026 12:11:21 -0300 Subject: [PATCH 4/4] refactor(mcp): move lone-surrogate strip into shared sanitizers (#1235) Push the surrogate-cleaning behaviour from per-call-site _clean() helpers into sanitize_content, sanitize_kg_value, and sanitize_query so every caller (existing and future) gets the fix automatically. New MCP tools no longer need to remember to call a separate helper. - Add strip_lone_surrogates() in mempalace/config.py as the single regex-based implementation (one U+FFFD per surrogate, not three). - Wire it into sanitize_content and sanitize_kg_value. - Wire it into sanitize_query so embedding lookups can't crash either. - Drop the _clean() helper from mcp_server.py and the per-site calls; retain a direct strip_lone_surrogates() for source_file/added_by metadata which doesn't route through any sanitizer. - Move the ONNX model-cache patch out of the test module and into conftest.py so it's session-scoped instead of duplicated locally. - Update tests to assert one U+FFFD per surrogate and exercise the sanitizer-level entry points directly. --- mempalace/config.py | 15 +- mempalace/mcp_server.py | 33 +-- mempalace/query_sanitizer.py | 6 +- tests/conftest.py | 17 ++ tests/test_clean_lone_surrogates.py | 329 ++++++++-------------------- 5 files changed, 128 insertions(+), 272 deletions(-) diff --git a/mempalace/config.py b/mempalace/config.py index d187cc8..0e5b6c8 100644 --- a/mempalace/config.py +++ b/mempalace/config.py @@ -19,6 +19,17 @@ from pathlib import Path MAX_NAME_LENGTH = 128 _SAFE_NAME_RE = re.compile(r"^(?:[^\W_]|[^\W_][\w .'-]{0,126}[^\W_])$") +# MCP clients (e.g. Claude Desktop, WorkBuddy) occasionally relay lone UTF-16 +# surrogates (U+D800–U+DFFF) when proxying binary-in-Unicode or corrupted +# clipboard input. Python's ``str.encode('utf-8')`` raises on these, which +# crashes ChromaDB add/upsert with -32000. See issue #1235. +_LONE_SURROGATE_RE = re.compile(r"[\ud800-\udfff]") + + +def strip_lone_surrogates(text: str) -> str: + """Replace lone UTF-16 surrogates with U+FFFD so the string is legal UTF-8 (#1235).""" + return _LONE_SURROGATE_RE.sub("�", text) + def normalize_wing_name(name: str) -> str: """Lower-case + collapse separators (`-`, ` `) to `_` for wing slugs. @@ -79,7 +90,7 @@ def sanitize_kg_value(value: str, field_name: str = "value") -> str: if "\x00" in value: raise ValueError(f"{field_name} contains null bytes") - return value + return strip_lone_surrogates(value) # ISO-8601 temporal validator for knowledge-graph temporal parameters @@ -175,7 +186,7 @@ def sanitize_content(value: str, max_length: int = 100_000) -> str: raise ValueError(f"content exceeds maximum length of {max_length} characters") if "\x00" in value: raise ValueError("content contains null bytes") - return value + return strip_lone_surrogates(value) DEFAULT_PALACE_PATH = os.path.expanduser("~/.mempalace/palace") diff --git a/mempalace/mcp_server.py b/mempalace/mcp_server.py index 397d66e..33dafd2 100644 --- a/mempalace/mcp_server.py +++ b/mempalace/mcp_server.py @@ -60,6 +60,7 @@ from .config import ( # noqa: E402 sanitize_name, sanitize_content, sanitize_iso_temporal, + strip_lone_surrogates, ) from .version import __version__ # noqa: E402 from chromadb.errors import NotFoundError as _ChromaNotFoundError # noqa: E402 @@ -152,28 +153,6 @@ _init_logging() logger = logging.getLogger("mempalace_mcp") -def _clean(text: str) -> str: - """Remove lone surrogates that break UTF-8 encoding (issue #1235). - - MCP clients (Claude Desktop, WorkBuddy) can emit lone surrogates - (``\\udc00``–``\\udfff``) when relaying binary-in-Unicode or - corrupted text. Python's ``str.encode('utf-8')`` raises - ``UnicodeEncodeError`` on these; ChromaDB's ``add()`` / - ``upsert()`` then crashes with -32000 Internal Error. - - Replace lone surrogates with U+FFFD (REPLACEMENT CHARACTER) so - the string is legal UTF-8 while preserving as much content as - possible. Note: ``encode('utf-8', 'surrogatepass')`` also - treats suspected paired surrogates (individual ``\ud800``–``\udfff`` - code points) as lone surrogates, replacing them with U+FFFD. - This is correct — a Python ``str`` containing such code points - is already corrupt from the perspective of the UTF-8 channel. - Properly decoded characters (e.g. a real emoji) contain no - surrogates and pass through unchanged. - """ - return text.encode("utf-8", "surrogatepass").decode("utf-8", "replace") - - def _parse_args(): parser = argparse.ArgumentParser(description="MemPalace MCP Server") parser.add_argument( @@ -916,7 +895,6 @@ def tool_search( dist = (1.0 - min_similarity) if min_similarity is not None else max_distance # Mitigate system prompt contamination (Issue #333) sanitized = sanitize_query(query) - sanitized["clean_query"] = _clean(sanitized["clean_query"]) # Ensure the vector-disabled probe has been run via the safe # sqlite/pickle path before we touch chromadb. Calling _get_client() # here would defeat the fallback — it constructs a PersistentClient @@ -987,7 +965,7 @@ def tool_check_duplicate(content: str, threshold: float = 0.9): if not col: return _no_palace() try: - content = _clean(content) + content = strip_lone_surrogates(content) results = col.query( query_texts=[content], n_results=5, @@ -1144,10 +1122,9 @@ def tool_add_drawer( wing = sanitize_name(wing, "wing") room = sanitize_name(room, "room") content = sanitize_content(content) - content = _clean(content) if source_file: - source_file = _clean(source_file) - added_by = _clean(added_by) + source_file = strip_lone_surrogates(source_file) + added_by = strip_lone_surrogates(added_by) except ValueError as e: return {"success": False, "error": str(e)} @@ -1445,7 +1422,6 @@ def tool_update_drawer(drawer_id: str, content: str = None, wing: str = None, ro if content is not None: try: new_doc = sanitize_content(content) - new_doc = _clean(new_doc) except ValueError as e: return {"success": False, "error": str(e)} @@ -1640,7 +1616,6 @@ def tool_diary_write(agent_name: str, entry: str, topic: str = "general", wing: try: agent_name = sanitize_name(agent_name, "agent_name").lower() entry = sanitize_content(entry) - entry = _clean(entry) topic = sanitize_name(topic, "topic") except ValueError as e: return {"success": False, "error": str(e)} diff --git a/mempalace/query_sanitizer.py b/mempalace/query_sanitizer.py index 774baea..7c16b8c 100644 --- a/mempalace/query_sanitizer.py +++ b/mempalace/query_sanitizer.py @@ -21,6 +21,8 @@ Expected recovery: import re import logging +from mempalace.config import strip_lone_surrogates + logger = logging.getLogger("mempalace_mcp") # --- Constants --- @@ -65,7 +67,9 @@ def sanitize_query(raw_query: str) -> dict: "method": "passthrough", } - raw_query = raw_query.strip() + # Strip lone surrogates before any further processing so downstream embed + # calls don't crash on UTF-8 encode (#1235). + raw_query = strip_lone_surrogates(raw_query.strip()) original_length = len(raw_query) def _strip_wrapping_quotes(candidate: str) -> str: diff --git a/tests/conftest.py b/tests/conftest.py index 9a3b02d..bd65363 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -33,6 +33,23 @@ import pytest # noqa: E402 from mempalace.config import MempalaceConfig # noqa: E402 from mempalace.knowledge_graph import KnowledgeGraph # noqa: E402 +# Redirect ChromaDB's ONNX model cache back to the real user's cache so tests +# don't re-download the 79 MB model on every run. The HOME redirect above +# would otherwise point ONNXMiniLM_L6_V2.DOWNLOAD_PATH at the empty temp dir. +try: + from pathlib import Path # noqa: E402 + from chromadb.utils.embedding_functions.onnx_mini_lm_l6_v2 import ( # noqa: E402 + ONNXMiniLM_L6_V2, + ) + + _real_home = _original_env.get("USERPROFILE") or _original_env.get("HOME") + if _real_home: + _real_cache = Path(_real_home) / ".cache" / "chroma" / "onnx_models" / "all-MiniLM-L6-v2" + if _real_cache.exists(): + ONNXMiniLM_L6_V2.DOWNLOAD_PATH = _real_cache +except ImportError: + pass + @pytest.fixture(autouse=True) def _reset_mcp_cache(): diff --git a/tests/test_clean_lone_surrogates.py b/tests/test_clean_lone_surrogates.py index 027ab70..60b6c12 100644 --- a/tests/test_clean_lone_surrogates.py +++ b/tests/test_clean_lone_surrogates.py @@ -1,223 +1,120 @@ -""" -Tests for lone-surrogate sanitisation in MCP server tool handlers (issue #1235). +"""Tests for lone-surrogate sanitisation (issue #1235). Covers: -- Unit: _clean() helper edge cases (lone surrogates, valid pairs, empty, etc.) -- Integration: all 5 tool paths that call _clean() receive surrogate payloads +- Unit: ``strip_lone_surrogates()`` edge cases (lone surrogates, real emoji, + empty input, hashing). +- Integration: every MCP tool that takes a user string and reaches ChromaDB + must survive lone surrogates in its payload. -Note on U+FFFD replacement count ----------------------------------- -``str.encode('utf-8', 'surrogatepass')`` encodes each lone surrogate into the -3-byte CESU-8 sequence (0xED 0xAx 0xBx). When that byte sequence is decoded -with ``errors='replace'``, *each individual invalid byte* is swapped for one -U+FFFD, yielding **3** replacement characters per lone surrogate. Tests must -assert ``"\ufffd" * 3`` (i.e. ``\ufffd\ufffd\ufffd``) for every lone surrogate -that was stripped, not a single ``\ufffd``. +MCP clients (Claude Desktop, WorkBuddy) occasionally relay lone UTF-16 +surrogates (U+D800–U+DFFF) when proxying binary-in-Unicode or corrupted +clipboard input. Python's ``str.encode('utf-8')`` raises on these, which +crashes ChromaDB add/upsert with -32000 Internal Error. - Note on surrogate pairs in Python source ------------------------------------------- -Writing ``"\\ud83d\\ude00"`` in Python source code creates a string with *two* -independent lone-surrogate code points (U+D83D and U+DE00). Python 3 does not -silently merge them into the astral emoji U+1F600. Therefore these strings -are treated as lone surrogates by ``_clean()`` and are replaced, just like any -other lone surrogate. To embed a real emoji use the ``\\U`` escape: -``"\\U0001f600"`` (one code point, no surrogates). +Note on surrogate pairs in Python source: writing ``"\\ud83d\\ude00"`` in +Python creates a string with *two* lone surrogates, not the astral emoji +U+1F600. Python 3 does not silently merge them. Use ``"\\U0001f600"`` to +embed a real emoji as a single code point. """ import hashlib -import os -import sys -from pathlib import Path -import pytest - -from mempalace.mcp_server import _clean +from mempalace.config import strip_lone_surrogates -# ── ONNX model cache fix ─────────────────────────────────────────────────── -# conftest.py redirects HOME to a temp directory *at module level*, before any -# chromadb import, so that mempalace module-level initialisations don't touch -# the real user profile. Unfortunately this causes chromadb's ONNXMiniLM_L6_V2 -# to compute its DOWNLOAD_PATH (``Path.home() / ".cache" / "chroma" / ...``) -# pointing at the temp dir, triggering a 79 MB model download on every run. -# -# Fix: at module import time (i.e. right now, before any fixture), retrieve the -# *original* USERPROFILE that conftest saved in ``_original_env``, and use it -# to patch DOWNLOAD_PATH back to the real user cache. The patch is applied at -# module level so it takes effect before the conftest ``collection`` fixture -# creates a ChromaDB client. - -def _get_real_chroma_cache() -> Path: - """Return the real user's chroma ONNX model cache path. - - conftest.py stores the original env in ``_original_env`` before - redirecting HOME. We reach into that dict to recover the real - USERPROFILE / HOME so we can locate the pre-downloaded model. - Falls back to the current HOME if conftest is not loaded yet. - """ - conftest = sys.modules.get("conftest") - if conftest is not None: - orig_env = getattr(conftest, "_original_env", {}) - real_home = ( - orig_env.get("USERPROFILE") - or orig_env.get("HOME") - ) - if real_home: - return Path(real_home) / ".cache" / "chroma" / "onnx_models" / "all-MiniLM-L6-v2" - # Fallback: use current env (works when conftest doesn't redirect HOME) - return Path(os.environ.get("USERPROFILE", os.path.expanduser("~"))) / ".cache" / "chroma" / "onnx_models" / "all-MiniLM-L6-v2" +# ── Unit tests ───────────────────────────────────────────────────────────── -def _patch_onnx_download_path() -> None: - """Patch ONNXMiniLM_L6_V2.DOWNLOAD_PATH to the real user cache (once). +class TestStripLoneSurrogates: + def test_passthrough_normal(self): + assert strip_lone_surrogates("hello world") == "hello world" + assert strip_lone_surrogates("你好世界") == "你好世界" + assert strip_lone_surrogates("mixed 中 English 文") == "mixed 中 English 文" - Called at module import time so the patch is in place before any ChromaDB - collection is created inside a fixture. - """ - try: - from chromadb.utils.embedding_functions.onnx_mini_lm_l6_v2 import ONNXMiniLM_L6_V2 - except ImportError: - return - real_cache = _get_real_chroma_cache() - if real_cache.exists(): - ONNXMiniLM_L6_V2.DOWNLOAD_PATH = real_cache + def test_empty_string(self): + assert strip_lone_surrogates("") == "" + def test_replaces_high_surrogate(self): + assert strip_lone_surrogates("hello\udc95world") == "hello�world" + assert strip_lone_surrogates("\udcff\udc00\udcaf") == "�" * 3 -_patch_onnx_download_path() + def test_replaces_low_surrogate(self): + assert strip_lone_surrogates("test\ud800more") == "test�more" + assert strip_lone_surrogates("\ud800\udbff") == "�" * 2 + def test_replaces_multiple_at_different_positions(self): + assert strip_lone_surrogates("a\udca1b\udcffc") == "a�b�c" -# ── Unit tests for _clean() ──────────────────────────────────────────────── + def test_preserves_real_emoji(self): + """Astral code points written with \\U have no surrogates and pass through.""" + assert strip_lone_surrogates("\U0001f600") == "\U0001f600" + assert strip_lone_surrogates("\U0001f680") == "\U0001f680" + assert strip_lone_surrogates("hello \U0001f600 world") == "hello \U0001f600 world" -class TestCleanLoneSurrogates: - """Unit tests for the _clean() helper.""" + def test_real_emoji_with_adjacent_lone_surrogate(self): + assert strip_lone_surrogates("\U0001f600\udc95") == "\U0001f600�" + assert strip_lone_surrogates("\udc95\U0001f600") == "�\U0001f600" - def test_clean_passthrough_normal(self): - """Normal ASCII + CJK strings pass through unchanged.""" - assert _clean("hello world") == "hello world" - assert _clean("你好世界") == "你好世界" - assert _clean("mixed 中 English 文") == "mixed 中 English 文" + def test_only_surrogates(self): + assert strip_lone_surrogates("\udc95\udcff") == "��" + assert strip_lone_surrogates("\ud800" + "\udbff" + "\udc00" + "\udfff") == "�" * 4 - def test_clean_removes_high_surrogate(self): - """U+DC00–U+DFFF lone surrogates are replaced. - - Each lone surrogate encodes to 3 CESU-8 bytes; decode('replace') then - emits one U+FFFD per invalid byte, giving 3 replacement chars. - """ - # one lone surrogate → 3 × U+FFFD - assert _clean("hello\udc95world") == "hello\ufffd\ufffd\ufffdworld" - # three lone surrogates → 9 × U+FFFD - assert _clean("\udcff\udc00\udcaf") == "\ufffd" * 9 - - def test_clean_removes_low_surrogate(self): - """U+D800–U+DBFF lone surrogates are replaced.""" - assert _clean("test\ud800more") == "test\ufffd\ufffd\ufffdmore" - assert _clean("\ud800\udbff") == "\ufffd" * 6 - - def test_clean_removes_multiple_surrogates(self): - """Multiple surrogates at different positions are all replaced.""" - result = _clean("a\udca1b\udcffc") - # each surrogate → 3 U+FFFD - assert result == "a" + "\ufffd" * 3 + "b" + "\ufffd" * 3 + "c" - assert "\ufffd" in result - - def test_clean_preserves_real_emoji(self): - """Real emoji (astral code points via \\U escape) pass through unchanged. - - Note: ``\\ud83d\\ude00`` written in Python source is NOT the emoji — it - is two lone surrogates. Use ``\\U0001f600`` for the real grinning face. - """ - # Grinning face U+1F600 - assert _clean("\U0001f600") == "\U0001f600" - # Rocket U+1F680 - assert _clean("\U0001f680") == "\U0001f680" - # Emoji in sentence - assert _clean("hello \U0001f600 world") == "hello \U0001f600 world" - - def test_clean_emoji_with_trailing_lone_surrogate(self): - """Real emoji followed by a lone surrogate: emoji preserved, lone replaced.""" - # U+1F600 is one code point (no surrogates); \udc95 is a lone surrogate → 3 U+FFFD - result = _clean("\U0001f600\udc95") - assert result == "\U0001f600" + "\ufffd" * 3 - assert "\U0001f600" in result - assert "\ufffd" in result - - def test_clean_emoji_with_leading_lone_surrogate(self): - """Real emoji preceded by a lone surrogate: lone replaced, emoji preserved.""" - result = _clean("\udc95\U0001f600") - assert result == "\ufffd" * 3 + "\U0001f600" - assert "\U0001f600" in result - - def test_clean_empty_string(self): - """Empty string is returned unchanged, not crashed.""" - assert _clean("") == "" - - def test_clean_only_surrogates(self): - """String containing only lone surrogates becomes all U+FFFD (3 each).""" - assert _clean("\udc95\udcff") == "\ufffd" * 6 - assert _clean("\ud800\udbff\udc00\udfff") == "\ufffd" * 12 - - def test_clean_sha256_hashable_after(self): - """Cleaned string can be hashed with SHA256 (the actual crash path).""" + def test_cleaned_string_is_utf8_encodable(self): + """The actual crash path: encode/hash must not raise after cleaning.""" dirty = "content\udc95with\ud800surrogate" - clean = _clean(dirty) - # Must not raise UnicodeEncodeError + clean = strip_lone_surrogates(dirty) h = hashlib.sha256(clean.encode("utf-8")).hexdigest() assert len(h) == 64 - def test_clean_workbuddy_injected_surrogate(self): - """ - The specific surrogate WorkBuddy injects during MCP relay. - - MCP clients can emit lone surrogates when relaying binary-in-Unicode - or corrupted text. \\udcad is the character observed in production logs. - Each lone surrogate → 3 U+FFFD after the encode/decode round-trip. - """ - result = _clean("2026-04-27\udcadworkBuddy relay") - assert result == "2026-04-27" + "\ufffd" * 3 + "workBuddy relay" - # SHA256 must not crash (was the original failure mode) - hashlib.sha256(result.encode()).hexdigest() + def test_workbuddy_injected_surrogate(self): + """The specific surrogate observed in WorkBuddy production logs.""" + result = strip_lone_surrogates("2026-04-27\udcadworkBuddy relay") + assert result == "2026-04-27�workBuddy relay" + # Must not raise — was the original failure mode. + hashlib.sha256(result.encode("utf-8")).hexdigest() -# ── Integration tests for tool paths ──────────────────────────────────────── +# ── Sanitizer integration ────────────────────────────────────────────────── + + +class TestSanitizersStripSurrogates: + """sanitize_content and sanitize_kg_value clean inline — verifies the + architectural fix so all MCP tools route through and inherit it for free.""" + + def test_sanitize_content_strips_surrogates(self): + from mempalace.config import sanitize_content + + assert sanitize_content("hello\udc95world") == "hello�world" + + def test_sanitize_kg_value_strips_surrogates(self): + from mempalace.config import sanitize_kg_value + + assert sanitize_kg_value("Alice\udc95") == "Alice�" + + def test_sanitize_query_strips_surrogates(self): + from mempalace.query_sanitizer import sanitize_query + + result = sanitize_query("search\udc95term") + assert "\udc95" not in result["clean_query"] + assert "�" in result["clean_query"] + + +# ── End-to-end tool integration ──────────────────────────────────────────── + def _patch_mcp_server(monkeypatch, config, kg): - """Swap production config and KG with test doubles (mirrors test_mcp_server.py).""" from mempalace import mcp_server monkeypatch.setattr(mcp_server, "_config", config) monkeypatch.setattr(mcp_server, "_get_kg", lambda: kg) -def _get_collection(palace_path, create=False): - """Helper to open the test ChromaDB collection.""" - import chromadb +class TestToolsAcceptSurrogates: + """End-to-end: every MCP tool that takes user text must not crash on + lone surrogates. Failures here surface as ChromaDB UnicodeEncodeError.""" - client = chromadb.PersistentClient(path=str(palace_path)) - if create: - return ( - client, - client.get_or_create_collection( - "mempalace_drawers", - metadata={"hnsw:space": "cosine"}, - ), - ) - return client, client.get_collection("mempalace_drawers") - - -class TestLoneSurrogateCleaning: - """Integration tests: lone surrogates flow through all 5 tool handler paths.""" - - def test_add_drawer_with_surrogate_in_content( - self, monkeypatch, collection, config, kg - ): - """ - tool_add_drawer: _clean(content) prevents ChromaDB UnicodeEncodeError. - - The original crash occurred in ChromaDB's upsert() when the content - string contained lone surrogates injected by the MCP client. - """ + def test_add_drawer_content(self, monkeypatch, collection, config, kg): _patch_mcp_server(monkeypatch, config, kg) - from mempalace.mcp_server import tool_add_drawer result = tool_add_drawer( @@ -226,20 +123,10 @@ class TestLoneSurrogateCleaning: content="drawer content with \udc95 surrogate", ) assert result["success"] is True - assert "drawer_id" in result assert result["drawer_id"].startswith("drawer_test_surrogate_") - def test_add_drawer_with_surrogate_in_metadata( - self, monkeypatch, collection, config, kg - ): - """ - tool_add_drawer: source_file and added_by also go through _clean(). - - These metadata fields can carry paths or names that contain - surrogates if the user's environment generates them. - """ + def test_add_drawer_metadata(self, monkeypatch, collection, config, kg): _patch_mcp_server(monkeypatch, config, kg) - from mempalace.mcp_server import tool_add_drawer result = tool_add_drawer( @@ -251,75 +138,37 @@ class TestLoneSurrogateCleaning: ) assert result["success"] is True - def test_check_duplicate_with_surrogate( - self, monkeypatch, collection, config, kg - ): - """ - tool_check_duplicate: _clean(content) before ChromaDB query(). - - Without _clean(), col.query() raises UnicodeEncodeError when the - query string contains lone surrogates. - """ + def test_check_duplicate(self, monkeypatch, collection, config, kg): _patch_mcp_server(monkeypatch, config, kg) - from mempalace.mcp_server import tool_check_duplicate result = tool_check_duplicate(content="exact\udc95match") - # Must not raise; UnicodeEncodeError would surface as a traceback assert isinstance(result, dict) - assert "is_duplicate" in result or "error" in result or result.get("results") is not None + assert "is_duplicate" in result - def test_search_with_surrogate_in_query( - self, monkeypatch, collection, config, kg - ): - """ - tool_search: _clean(query) before ChromaDB query(). - - User search queries containing lone surrogates (e.g. pasted from - corrupted clipboard) should not crash the search path. - """ + def test_search_query(self, monkeypatch, collection, config, kg): _patch_mcp_server(monkeypatch, config, kg) - from mempalace.mcp_server import tool_search result = tool_search(query="search\udc95term") assert isinstance(result, dict) - assert "success" in result or "error" in result or "results" in result + assert "error" not in result or "results" in result - def test_update_drawer_with_surrogate( - self, monkeypatch, collection, config, kg - ): - """ - tool_update_drawer: _clean(new_doc) before ChromaDB upsert(). - - Updating a drawer's content with surrogate-laden text must persist - cleanly without ChromaDB crashes. - """ + def test_update_drawer(self, monkeypatch, collection, config, kg): _patch_mcp_server(monkeypatch, config, kg) - from mempalace.mcp_server import tool_add_drawer, tool_update_drawer add_result = tool_add_drawer(wing="test", room="update", content="original content") assert add_result["success"] is True - drawer_id = add_result["drawer_id"] update_result = tool_update_drawer( - drawer_id=drawer_id, + drawer_id=add_result["drawer_id"], content="updated\udc95content", ) assert update_result["success"] is True - def test_diary_write_with_surrogate( - self, monkeypatch, collection, config, kg - ): - """ - tool_diary_write: _clean(entry) before KG/ChromaDB write. - - Diary entries containing surrogates (e.g. copied from a corrupted - document) must be accepted without crashing. - """ + def test_diary_write(self, monkeypatch, collection, config, kg): _patch_mcp_server(monkeypatch, config, kg) - from mempalace.mcp_server import tool_diary_write result = tool_diary_write(