feat(backends): embedder-identity contract + three-state enforcement (RFC 001) - #1731
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request implements RFC 001, introducing embedder-identity tracking and enforcement across various backends (Chroma, PGVector, and SQLite) to prevent silent retrieval degradation when swapping models of the same dimension. It adds the EmbedderIdentity dataclass, verification logic, a CLI command (mempalace palace set-embedder) to manage identities, and comprehensive tests. The review feedback highlights several robust error-handling improvements, specifically regarding the validation of parsed JSON structures (ensuring they are dictionaries before accessing keys) and using the self._collection_name() helper instead of accessing self._collection.name directly in the Chroma backend.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| def get_stored_embedder_identity(self): | ||
| from .base import EmbedderIdentity | ||
|
|
||
| path = self._embedder_sidecar_path() | ||
| if not path or not os.path.isfile(path): | ||
| return None | ||
| try: | ||
| with open(path, encoding="utf-8") as f: | ||
| data = json.load(f) | ||
| except (OSError, json.JSONDecodeError): | ||
| return None | ||
| entry = (data or {}).get(self._collection.name) | ||
| if not isinstance(entry, dict) or not entry.get("model_name"): | ||
| return None | ||
| return EmbedderIdentity( | ||
| model_name=str(entry["model_name"]), | ||
| dimension=int(entry.get("dimension") or 0), | ||
| ) |
There was a problem hiding this comment.
There are two issues in this method:
json.load(f)can return a non-dictionary type (like a list or string) if the JSON file is corrupted or malformed. Ifdatais not a dictionary, calling(data or {}).get(...)will raise an unhandledAttributeError.self._collection.namecan be a callable in some environments or test doubles (which is why theself._collection_name()helper exists on line 1533). Accessing it directly instead of callingself._collection_name()will fail to retrieve the correct entry.
We should validate that data is a dictionary and use self._collection_name() to safely retrieve the collection name.
def get_stored_embedder_identity(self):
from .base import EmbedderIdentity
path = self._embedder_sidecar_path()
if not path or not os.path.isfile(path):
return None
try:
with open(path, encoding=| def set_embedder_identity(self, identity) -> None: | ||
| path = self._embedder_sidecar_path() | ||
| if not path or not identity or not identity.model_name: | ||
| return | ||
| data: dict = {} | ||
| if os.path.isfile(path): | ||
| try: | ||
| with open(path, encoding="utf-8") as f: | ||
| loaded = json.load(f) | ||
| if isinstance(loaded, dict): | ||
| data = loaded | ||
| except (OSError, json.JSONDecodeError): | ||
| data = {} | ||
| data[self._collection.name] = { | ||
| "model_name": str(identity.model_name), | ||
| "dimension": int(identity.dimension or 0), | ||
| } | ||
| try: | ||
| with open(path, "w", encoding="utf-8") as f: | ||
| json.dump(data, f, indent=2, ensure_ascii=False) | ||
| os.chmod(path, 0o600) | ||
| except (OSError, NotImplementedError): | ||
| pass |
There was a problem hiding this comment.
self._collection.name can be a callable in some environments or test doubles (which is why the self._collection_name() helper exists on line 1533). Accessing it directly instead of calling self._collection_name() will use the callable object as a dictionary key, which will raise a TypeError during json.dump because callables are not JSON-serializable.
We should use self._collection_name() to safely retrieve the collection name.
def set_embedder_identity(self, identity) -> None:
path = self._embedder_sidecar_path()
if not path or not identity or not identity.model_name:
return
name = self._collection_name()
if not name:
return
data: dict = {}
if os.path.isfile(path):
try:
with open(path, encoding=| def _get_embedder_identity(self, palace: PalaceRef, collection_name: str): | ||
| from .base import EmbedderIdentity | ||
|
|
||
| try: | ||
| marker = self._read_marker(palace) | ||
| except BackendMismatchError: | ||
| return None | ||
| if not isinstance(marker, dict): | ||
| return None | ||
| entry = (marker.get("embedders") or {}).get(collection_name) | ||
| if not isinstance(entry, dict) or not entry.get("model_name"): | ||
| return None | ||
| return EmbedderIdentity( | ||
| model_name=str(entry["model_name"]), | ||
| dimension=int(entry.get("dimension") or 0), | ||
| ) |
There was a problem hiding this comment.
If the marker file is corrupted or malformed, marker.get("embedders") might return a non-dictionary type (like a list or string). If that happens, calling .get(collection_name) on it will raise an unhandled AttributeError.
We should validate that embedders is a dictionary before calling .get().
def _get_embedder_identity(self, palace: PalaceRef, collection_name: str):
from .base import EmbedderIdentity
try:
marker = self._read_marker(palace)
except BackendMismatchError:
return None
if not isinstance(marker, dict):
return None
embedders = marker.get("embedders")
if not isinstance(embedders, dict):
return None
entry = embedders.get(collection_name)
if not isinstance(entry, dict) or not entry.get("model_name"):
return None
return EmbedderIdentity(
model_name=str(entry["model_name"]),
dimension=int(entry.get("dimension") or 0),
)| try: | ||
| marker = self._read_marker(palace) or {} | ||
| except BackendMismatchError: | ||
| return | ||
| embedders = marker.get("embedders") | ||
| if not isinstance(embedders, dict): | ||
| embedders = {} | ||
| embedders[collection_name] = { | ||
| "model_name": str(identity.model_name), | ||
| "dimension": int(identity.dimension or 0), | ||
| } | ||
| marker["embedders"] = embedders |
There was a problem hiding this comment.
If _read_marker returns a non-dictionary type (such as a list or string) due to corruption or manual editing, marker will not be a dictionary. This will cause marker.get("embedders") to raise an AttributeError and marker["embedders"] = embedders to raise a TypeError.
We should validate that marker is a dictionary and default to an empty dictionary if it is not.
| try: | |
| marker = self._read_marker(palace) or {} | |
| except BackendMismatchError: | |
| return | |
| embedders = marker.get("embedders") | |
| if not isinstance(embedders, dict): | |
| embedders = {} | |
| embedders[collection_name] = { | |
| "model_name": str(identity.model_name), | |
| "dimension": int(identity.dimension or 0), | |
| } | |
| marker["embedders"] = embedders | |
| try: | |
| marker = self._read_marker(palace) | |
| except BackendMismatchError: | |
| return | |
| if not isinstance(marker, dict): | |
| marker = {} | |
| embedders = marker.get("embedders") | |
| if not isinstance(embedders, dict): | |
| embedders = {} | |
| embedders[collection_name] = { | |
| "model_name": str(identity.model_name), | |
| "dimension": int(identity.dimension or 0), | |
| } | |
| marker["embedders"] = embedders |
There was a problem hiding this comment.
Pull request overview
Implements RFC 001’s embedder-identity contract so collections persist which embedding model produced their vectors and fail fast on same-dimension model swaps that would otherwise silently degrade retrieval.
Changes:
- Adds an
EmbedderIdentityvalue object + three-statecheck_embedder_identity()helper and collection hooks for storing/enforcing identity. - Persists embedder identity for
sqlite_exact,pgvector(marker JSON), andchroma(sidecar JSON), and wires enforcement intopalace.get_collection. - Introduces a
mempalace palace set-embedderCLI command plus a dedicated test suite covering the three-state logic, persistence, and enforcement.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/test_embedder_identity.py | New test suite for identity helper, backend persistence, wrapper delegation, and palace enforcement. |
| pyproject.toml | Adds pytest warning filter to reduce legacy “unknown identity” warning noise during tests. |
| mempalace/palace.py | Adds per-process identity validation cache, enforcement on open, and set_palace_embedder_identity(). |
| mempalace/embedding.py | Adds cheap canonical model-name resolver and cached dimension probe for identity construction. |
| mempalace/cli.py | Adds palace set-embedder subcommand wiring and output. |
| mempalace/backends/sqlite_exact.py | Persists model identity in sqlite meta table keyed per collection. |
| mempalace/backends/pgvector.py | Persists model identity in the local marker JSON and preserves it across marker rewrites. |
| mempalace/backends/embedding_wrapper.py | Explicitly forwards identity methods to avoid BaseCollection method shadowing. |
| mempalace/backends/chroma.py | Persists identity in a palace-dir sidecar JSON keyed by collection name. |
| mempalace/backends/base.py | Defines identity dataclass/protocol/warning and default collection identity hooks. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| try: | ||
| model_name = current_model_name() | ||
| except Exception: | ||
| return | ||
| if not model_name: | ||
| return # nameless embedder — cannot enforce identity | ||
| key = (str(palace_path), str(collection_name), model_name) | ||
| if key in _VALIDATED_IDENTITY: | ||
| return | ||
|
|
||
| try: | ||
| stored = collection.get_stored_embedder_identity() | ||
| except Exception: | ||
| logger.debug("embedder-identity read failed for %s", collection_name, exc_info=True) | ||
| return | ||
|
|
||
| current = EmbedderIdentity(model_name=model_name, dimension=0) | ||
| try: | ||
| state = check_embedder_identity(stored, current) |
| configured = MempalaceConfig().embedding_model | ||
| target = (model or configured or "").strip().lower() | ||
| if target and target == (configured or "").strip().lower(): | ||
| # Recording the in-use model — probe its dimension (already loaded). |
| """Record (or force-override) a palace's embedder identity (RFC 001). | ||
|
|
||
| Resolves the ``unknown`` state for a legacy palace, or switches a palace to | ||
| a new model with ``--model`` (which also updates the configured model so | ||
| subsequent opens stay consistent). ``--force`` overwrites an existing, | ||
| differently-named identity. |
| p_set_embedder.add_argument( | ||
| "--model", | ||
| default=None, | ||
| help="Embedder model to record (default: current configured model). " | ||
| "When given, also updates the configured model.", | ||
| ) |
…(RFC 001, #1724) The explicit-embedding backends (pgvector, qdrant, sqlite_exact) embed through EmbeddingCollection and persist nothing about which model produced their vectors. Swapping two same-dimension models (minilm <-> embeddinggemma, both 384-d) silently corrupts retrieval with no error — the worst failure class for a verbatim-recall system. This records the model name and refuses a swap on open. - base.py: EmbedderIdentity dataclass, Embedder protocol, EmbedderIdentityUnknownWarning, and a three-state check_embedder_identity() helper (known_match / known_mismatch / unknown) raising EmbedderIdentityMismatchError / DimensionMismatchError. BaseCollection gains get/set/effective_embedder_identity hooks. - embedding.py: current_model_name() (cheap) + get_embedder_identity() (probed dimension, cached per process). - Persistence per backend: sqlite_exact meta table, pgvector marker JSON (preserved across rewrites), chroma sidecar JSON. EmbeddingCollection forwards the hooks explicitly (BaseCollection methods shadow __getattr__). - palace.get_collection enforces at open: a model swap raises; a brand-new empty collection records the current model; a populated-but-unrecorded legacy palace warns and is resolved via the CLI. The check needs no model load. - CLI: `mempalace palace set-embedder [--model NAME] [--force]` records/overrides identity without mutating global config or loading a foreign model. Chroma additionally keeps its native embedding-function check. Qdrant identity persistence is a fast-follow (no local metadata slot — needs a companion store). Closes #1724. Refs #743.
a1bcc53 to
212051d
Compare
Close the last open items on #743 before merge: - Conformance: document two isolation arms (cross-id for all backends; same-id/different-namespace for supports_namespace_isolation advertisers). - No silent drop: non-advertising backends must raise UnsupportedCapabilityError when PalaceRef.namespace is set, rather than accept-and-ignore. - Wire require_namespace_support() into chroma/sqlite_exact; add conformance test. - Refresh implementation-status banner now that #1727/#1731/#1732/#1734 landed.
Summary
Implements the embedder-identity contract from RFC 001 (#743) — the "hard dependency" tracked in #1724.
The explicit-embedding backends (
pgvector,qdrant,sqlite_exact) embed throughEmbeddingCollectionand persisted nothing about which model produced their vectors. Swapping two same-dimension models (minilm↔embeddinggemma, both 384-d) silently corrupted retrieval with no error — the worst failure class for a verbatim-recall system. This records the model name on write and refuses a swap on open.What changed
backends/base.py):EmbedderIdentitydataclass, anEmbedderprotocol,EmbedderIdentityUnknownWarning, and a pure three-statecheck_embedder_identity()—known_match/known_mismatch(raisesEmbedderIdentityMismatchError, orDimensionMismatchErrorfor a width change, checked first) /unknown.BaseCollectiongains optionalget/set/effective_embedder_identityhooks (default: no-op → permanentlyunknown, which is safe).embedding.py):current_model_name()(cheap, from config — the canonical name, not the EF's spoofed"default") andget_embedder_identity()(dimension from a one-time cached probe).sqlite_exactmetatable ·pgvectormarker JSON (preserved across marker rewrites) ·chromasidecar JSON in the palace dir.EmbeddingCollectionforwards the hooks explicitly —BaseCollectiondefines them as concrete methods, so__getattr__would otherwise shadow the wrapped backend (same trap caught on the previous metric PR).palace.get_collection: a recorded model that differs from the current one raises (fail fast, before any silently-degraded query); a brand-new empty collection records the current model; a populated-but-unrecorded legacy palace warns and is resolved with the CLI. The check uses only the configured model name, so it needs no model load. A per-process cache keeps the hot path at one metadata read. Identity bookkeeping never breaks memory ops — only the deliberate mismatch propagates.mempalace palace set-embedder [--model NAME] [--force]records or force-overrides identity. It does not mutate global config and never loads a foreign model (an explicit--modeloverride records the name withdimension=0).Scope / notes
get_stored_embedder_identityfor tooling) without touching the fragile collection metadata.unknown→ warn, never a hard fail. The check only raises once an identity has been recorded and the model then changes.unknown/safe until then.Tests
tests/test_embedder_identity.py(18 tests, no model loads): the three-state helper; per-backend persistence roundtrips (sqlite, chroma sidecar, pgvector marker-preservation — all connection-free);EmbeddingCollectiondelegation; and enforcement viapalace.get_collection(match / model-swap-raises / brand-new-records / legacy-warns / nameless-noop / forced override). Full suite green: 2469 passed, 82.45% coverage,ruff check+format --checkclean.Closes #1724. Refs #743, #1730.