Мempalace backend seam

[skuznetsov]

What does this PR do?

Introduce the first upstreamable storage seam for MemPalace without bringing in the PostgreSQL spike or any benchmark artifacts.

This change adds a small backend package with:

• BaseCollection as the minimal collection contract
• ChromaBackend/ChromaCollection as the default implementation

It then routes the main runtime collection consumers through that seam:

• palace.py
• searcher.py
• layers.py
• palace_graph.py
• mcp_server.py
• miner.status()

Behavioral constraints kept for stage 1:

• ChromaDB remains the only backend and the default path
• no config/env backend selection yet
• no PostgreSQL code
• no benchmark or research files

Important compatibility details:

• read paths call the seam with create=False so they preserve the existing "no palace found" behavior instead of creating empty palaces
• write paths keep create=True semantics through palace.get_collection()
• ChromaBackend.get_collection(create=False) now fails fast when the palace directory is missing, so read-only commands do not materialize storage as a side effect
• layer/searcher tests now patch the backend seam directly rather than relying on module-local chromadb shims

How to test

Verification:

• python3 -m py_compile mempalace/backends/__init__.py mempalace/backends/base.py mempalace/backends/chroma.py mempalace/palace.py mempalace/searcher.py mempalace/layers.py mempalace/palace_graph.py mempalace/mcp_server.py mempalace/miner.py
• ruff check .
• uv run pytest -q (538 passed, 106 deselected)

Checklist

• Tests pass (uv run pytest -q)
• Linter passes (ruff check .)
• No hardcoded paths

Refs #266

[bgauryy]

PR #413 Review: Mempalace backend seam

URL: #413
Author: skuznetsov | Status: Open | Branch: codex/mempalace-backend-seam → main
Scope: 9 files | +151 / -56 | 3 new, 6 modified

---

Executive Summary

Introduces a storage abstraction layer (BaseCollection ABC + ChromaBackend/ChromaCollection adapter) and routes all six runtime collection consumers through it. The architectural direction is sound — it cleanly decouples the codebase from chromadb internals and opens the door for alternative backends.

However, there is a behavioral regression in miner.status() that silently creates an empty palace instead of reporting "not found".

Recommendation: Fix the one bug, then merge.

---

Ratings

Dimension	Score	Notes
Correctness	7/10	One real bug in miner.status()
Security	9/10	Directory permissions preserved, no new exposure
Performance	9/10	Minor: PersistentClient no longer cached in MCP server
Maintainability	8/10	Clean abstraction, but no injection mechanism yet

---

Issues

1. [HIGH / HIGH confidence] Bug: miner.status() creates palace instead of failing gracefully

Location: mempalace/miner.py:616

The old code directly used chromadb.PersistentClient(path).get_collection(name) which raises when the collection doesn't exist. The except Exception block catches it and prints "No palace found." The new code calls get_collection(palace_path) which defaults to create=True, meaning it calls ChromaBackend.get_collection(..., create=True) which runs os.makedirs() + get_or_create_collection(). This silently creates an empty palace directory and collection instead of reporting "not found."

Old behavior (main branch):

# miner.py status() - RAISES when palace doesn't exist
client = chromadb.PersistentClient(path=palace_path)
col = client.get_collection("mempalace_drawers")  # throws -> except block fires

New behavior (PR branch):

# miner.py status() - CREATES when palace doesn't exist
col = get_collection(palace_path)  # create=True by default -> never fails

Fix:

 def status(palace_path: str):
     """Show what's been filed in the palace."""
     try:
-        col = get_collection(palace_path)
+        col = get_collection(palace_path, create=False)
     except Exception:

---

2. [MEDIUM / HIGH confidence] No backend injection mechanism

Location: mempalace/palace.py:36

_DEFAULT_BACKEND = ChromaBackend() is a module-level singleton with no setter, no config-driven selection, and no test override path. The abstraction exists but can't be leveraged. The PR description mentions this is the "first upstreamable seam," so this may be intentional scaffolding — but tests cannot inject a mock backend without patching the private module attribute.

Suggestion (not blocking): Accept an optional backend parameter in get_collection():

def get_collection(
    palace_path: str,
    collection_name: str = "mempalace_drawers",
    create: bool = True,
    backend: ChromaBackend = None,
):
    backend = backend or _DEFAULT_BACKEND
    return backend.get_collection(palace_path, collection_name=collection_name, create=create)

---

3. [MEDIUM / HIGH confidence] No tests for new backends package

Three new files (base.py, chroma.py, __init__.py) with no test coverage. The BaseCollection contract, the ChromaCollection adapter delegation, and the ChromaBackend.get_collection factory (including the create=True/False branching) are all untested. Recommended minimum:

• Test that ChromaCollection correctly delegates each method
• Test that ChromaBackend.get_collection(create=False) raises when collection is missing
• Test that ChromaBackend.get_collection(create=True) creates directory + collection

---

4. [LOW / MEDIUM confidence] PersistentClient no longer cached in MCP server

Location: mempalace/mcp_server.py:105 / mempalace/backends/chroma.py:45

The old mcp_server.py had a _client_cache singleton for the PersistentClient. The new code delegates to ChromaBackend.get_collection() which creates a fresh PersistentClient on every call. Since the _collection_cache is still in place, this only fires when the cache is cold or create=True is passed. For a local SQLite-backed store this is likely negligible, but worth awareness if the backend ever becomes remote.

---

5. [LOW / MEDIUM confidence] Test compatibility shims add implicit coupling

Location: mempalace/layers.py:26-28, mempalace/searcher.py:14-16

chromadb = _chroma_backend.chromadb

This re-exports chromadb at module level so tests can still patch mempalace.layers.chromadb.PersistentClient. It's a pragmatic choice but couples production code to test internals. Consider updating the tests to patch through the backend layer instead, then removing these shims.

---

Flow Impact

Consumer	Old Pattern	New Pattern	Behavioral Change
palace.py	Direct chromadb, always creates	ChromaBackend, create param	create=False now possible (new capability)
layers.py (5 callsites)	PersistentClient + get_collection	palace.get_collection(create=False)	Correct: read-only callers fail if missing
searcher.py (2 callsites)	PersistentClient + get_collection	palace.get_collection(create=False)	Correct: same fail-if-missing behavior
palace_graph.py	PersistentClient + get_collection	palace.get_collection(create=False)	Correct
mcp_server.py	Cached client + get_or_create	palace.get_collection + local cache	Client cache lost (minor perf)
miner.status()	PersistentClient + get_collection (throws)	palace.get_collection(create=True)	BUG: creates instead of failing

---

What Looks Good

• Clean ABC with keyword-only add/upsert signatures prevents positional argument mix-ups
• The create parameter on get_collection is good design — read-only callers can explicitly declare they expect the palace to exist
• Directory permissions (0o700) and os.makedirs are correctly centralized in ChromaBackend
• Well-scoped PR — no unrelated changes, no benchmark artifacts, no PostgreSQL spike leaking in

[milla-jovovich]

Hey @skuznetsov — I've taken a look and ran this through CLI, and the Stage 1 scope here is exactly what #266 was asking for. No Postgres code, no env-based backend selection, ChromaDB stays the only path, read paths preserve the existing "no palace found" behavior via create=False — all the right calls.

One request before we pick this up for a full review: the checklist has ruff unchecked and the test section cites pytest -q (529 passed) but not the ruff pass. A clean ruff run + one rebase on top of v3.1.0 will get this reviewable.

For Stage 2 (the actual Postgres backend from your fork), let's hold it until after one other in-flight piece lands that depends on the write-path seam — don't want two backend layers in flight at the same time. You'll have first pick on Stage 2 as soon as that clears.

Thank you from Milla and Lu✨

[skuznetsov]

One small follow-up note, but not a blocker for this stage-1 PR:

The current _collection_cache in mempalace/mcp_server.py is correct for MemPalace's current single-palace / single-config process model.

If MemPalace later grows dynamic palace switching or backend switching inside one long-lived process, that cache will probably need to become key-based instead of process-global, e.g. keyed by (palace_path, collection_name, backend).

I don't think that should complicate this PR, but I wanted to leave the constraint explicit so neither side forgets it later.

[web3guru888]

Review: MemPalace Backend Seam (Storage Abstraction)

This is the kind of architectural change that needs careful review. The goal — making MemPalace backend-swappable — is valuable. We maintain a knowledge_graph_bridge.py that keeps ChromaDB and our bi-temporal KG in sync, so we're directly affected by storage abstraction changes.

Design Assessment

BaseCollection ABC — Minimal and correct. The 6-method contract (add, upsert, query, get, delete, count) covers exactly the ChromaDB surface area that MemPalace actually uses. No over-abstraction.

ChromaCollection thin adapter — Good approach. Pure delegation, no logic. This means the adapter can't drift from the underlying ChromaDB behavior.

ChromaBackend.get_collection() — Combines directory creation, permission setting, and client creation. The create=False path raises FileNotFoundError when the palace directory is missing — this is correct and matches the "don't create empty palaces on read" principle from #423.

Concerns

⚠️ Keyword-only arguments on BaseCollection:

def add(self, *, documents, ids, metadatas=None):

The * forces keyword-only args. This is fine if all callers already use collection.add(documents=[...], ids=[...]), but some existing code might use positional args. Did you verify all callsites? The test suite passing (538 passed) suggests yes, but worth confirming.

⚠️ Client lifetime / connection pooling:
ChromaBackend.get_collection() creates a new PersistentClient on every call. The existing caching in mcp_server.py protects against this being called repeatedly, but other paths (layers, searcher) call _get_collection() per operation. If a future backend (Postgres, etc.) has connection overhead, this factory pattern will need a connection pool. Consider adding a note or a # TODO: pool clients per palace_path for future backends.

⚠️ palace.py module-level singleton:

_DEFAULT_BACKEND = ChromaBackend()

This creates a ChromaBackend at import time. Since ChromaBackend.__init__ is empty, this is harmless today. But when backend selection is added (env var, config), this singleton pattern won't work for dynamic dispatch. Might be cleaner as a factory function now to avoid the future refactor.

Migration Path

The test changes are clean — replacing patch("...chromadb.PersistentClient", ...) with patch("..._get_collection", ...) is the right pattern for backend-agnostic tests. Every test that previously mocked ChromaDB directly now mocks the seam.

Missing from tests: There's no test that verifies BaseCollection implementations actually conform to the interface (e.g., a contract test). For now ChromaDB is the only backend so this is fine, but when a second backend is added, consider adding an abstract test fixture that runs the same assertions against any BaseCollection implementation.

Conflict potential

This PR and #423 touch many of the same files (palace.py, mcp_server.py, layers.py, searcher.py, tests). They're solving different problems but will definitely conflict on merge. Coordinate with @gut-puncture on merge order — whichever goes in first will require the other to rebase significantly.

Clean abstraction, well-scoped stage 1. The behavioral constraints (ChromaDB only, no config selection, no Postgres) are the right approach for a safe first step.

🔭 Reviewed as part of the MemPalace-AGI integration project — autonomous research with perfect memory. Community interaction updates are posted regularly on the dashboard.

[skuznetsov]

Quick merge-order note after comparing the overlap with #423:

I think this PR should land first, and then #423 should rebase on top of it.

Reason:

• both PRs touch the same collection-access surface (palace.py, mcp_server.py, miner.py, layers.py, searcher.py, plus overlapping tests)
• #413 is the lower-level seam / access abstraction
• #423 adds behavioral logic on top of that same surface: configured drawer resolution, ~ expansion, keyed MCP cache, paged metadata reads, and consistent default-collection reads

So the clean order seems to be:

1. land #413 as the storage seam
2. rebase #423 onto that seam and carry its behavior fixes forward

That should minimize rebase churn and reduce the chance of flattening the configured-drawer behavior while introducing the seam.

[milla-jovovich]

Looks good

[bensig]

@skuznetsov conflict