PR Review: fix(mcp): harden error messages and add read-only server mode

Executive Summary

Affected Areas: mcp_server.py (tool handlers, handle_request, main()), test_mcp_server.py (4 new test classes)

Business Impact: Prevents leaking internal paths/schema details to MCP clients; enables safe read-only deployments for shared/demo palaces.

Flow Changes: Write tools (add_drawer, delete_drawer, kg_add, kg_invalidate, diary_write) blocked in read-only mode. tools/list hides them. Error responses no longer expose exception internals.

Ratings

PR Health

• Has clear description
• References ticket/issue (if applicable)
• Appropriate size (or justified if large)
• Has relevant tests (if applicable)

High Priority Issues

(Must fix before merge)

🐛 #1: _get_collection() lost client/collection caching — performance regression

Location: mempalace/mcp_server.py:_get_collection | Confidence: ✅ HIGH

The fork's _get_collection() creates a new chromadb.PersistentClient on every call. Current main caches both the client and collection in module globals (_client_cache, _collection_cache), so repeated tool invocations reuse the connection. This PR silently removes that caching — likely because the fork was branched before caching was added. Every MCP tool call will now pay the PersistentClient init cost, which is significant for ChromaDB (file locks, HNSW index load).

- def _get_collection(create=False):
-     """Return the ChromaDB collection, or None on failure."""
-     try:
-         client = chromadb.PersistentClient(path=_config.palace_path)
-         if create:
-             return client.get_or_create_collection(_config.collection_name)
-         return client.get_collection(_config.collection_name)
-     except Exception:
-         return None
+ # Rebase onto current main to preserve the _client_cache / _collection_cache pattern.
+ # Only change the error-handling lines within the existing _get_collection().

Fix: Rebase the branch onto current main so caching is preserved. Apply only the intended error-hardening + read-only changes.

🐛 #2: tool_add_drawer unintentionally changed duplicate detection and ID generation

Location: mempalace/mcp_server.py:tool_add_drawer | Confidence: ✅ HIGH

On main, drawer IDs are deterministic (content hash only) and the insert uses upsert — making the operation idempotent. The fork switches to timestamp-based IDs (content[:100] + datetime.now().isoformat()) and add with a semantic similarity pre-check via tool_check_duplicate(threshold=0.9). This is a behavioral change not described in the PR:

1. Same content added twice within the same second gets different IDs (timestamp resolution)
2. Semantic dedup (0.9 threshold) is fuzzier than exact-match — could reject legitimately different content or miss exact duplicates with different embeddings
3. add instead of upsert means a collision would raise instead of silently succeed

- # Current main (deterministic + idempotent)
- drawer_id = f"drawer_{wing}_{room}_{hashlib.md5(content.encode()).hexdigest()[:16]}"
- col.upsert(ids=[drawer_id], ...)
+ # PR (timestamp-based + semantic dedup)
+ drawer_id = f"drawer_{wing}_{room}_{hashlib.md5((content[:100] + datetime.now().isoformat()).encode()).hexdigest()[:16]}"
+ dup = tool_check_duplicate(content, threshold=0.9)
+ col.add(ids=[drawer_id], ...)

Fix: Rebase onto current main. Keep the original ID generation and upsert strategy. Only apply the str(e) → generic message change within the except block.

Medium Priority Issues

(Should fix, not blocking)

🚨 #3: TestErrorHardening doesn't exercise the actual exception path

Location: tests/test_mcp_server.py:TestErrorHardening.test_error_responses_do_not_leak_exceptions | Confidence: ✅ HIGH

The test calls delete_drawer with a non-existent ID ("nope"), but this hits the normal if not existing["ids"]: return {"success": False, "error": f"Drawer not found: {drawer_id}"} path — not the except Exception path that was hardened. The test passes because "Drawer not found: nope" trivially doesn't contain "Traceback" or "Exception".

To actually verify the hardening, the test should force an exception (e.g., monkeypatch col.delete to raise) and verify the generic message is returned instead of the exception string.

+ def test_exception_does_not_leak_to_client(self, monkeypatch, config, palace_path, kg):
+     _patch_mcp_server(monkeypatch, config, palace_path, kg)
+     col = _get_collection(palace_path, create=True)
+     col.add(ids=["test_id"], documents=["test"], metadatas=[{"wing": "w", "room": "r"}])
+     # Force col.delete to raise
+     monkeypatch.setattr(col, "delete", lambda **kw: (_ for _ in ()).throw(RuntimeError("secret/path/leak")))
+     from mempalace.mcp_server import tool_delete_drawer
+     result = tool_delete_drawer("test_id")
+     assert result["success"] is False
+     assert "secret" not in result["error"]

🎨 #4: Read-only error uses -32600 (Invalid Request) instead of a custom code

Location: mempalace/mcp_server.py:handle_request tools/call branch | Confidence: ⚠️ MED

JSON-RPC -32600 means "Invalid Request" (malformed JSON-RPC). A write tool call in read-only mode is a valid request that the server chooses to reject. A custom server error code (e.g., -32001) or returning a normal result with an error payload would be more semantically accurate. MCP clients that parse -32600 may retry or misattribute the failure.

-             "error": {
-                 "code": -32600,
-                 "message": f"Server is in read-only mode. Tool '{tool_name}' is not available.",
-             },
+             "error": {
+                 "code": -32001,
+                 "message": f"Server is in read-only mode. Tool '{tool_name}' is not available.",
+             },

Low Priority Issues

(Nice to have)

🎨 #5: _read_only cleanup in tests is manual

Location: tests/test_mcp_server.py:TestReadOnlyMode | Confidence: ⚠️ MED

Each read-only test manually resets _read_only = False at the end. If a test fails mid-way, the reset is skipped, potentially polluting subsequent tests. Since monkeypatch is already being used (which auto-reverts), the explicit reset is redundant — but only because monkeypatch.setattr is used to set it. The pattern is safe but slightly misleading.

Created by Octocode MCP https://octocode.ai 🔍🐙

Hi, thanks for the contribution.

This PR has merge conflicts with develop, and the branch has not been updated in over 7 days, which puts it before our most recent release. The conflicts are likely against work that landed in that release.

Could you rebase onto develop so we can take another look?

If this change is no longer relevant, feel free to close the PR.

(This message is part of a periodic backlog pass, sent to all open PRs that match this state.)