Skip to content

fix: guard Layer3.search_raw against None doc/meta from ChromaDB (#1011)#1013

Merged
bensig merged 1 commit intodevelopfrom
fix/layer3-search-raw-none-guard-1011
Apr 18, 2026
Merged

fix: guard Layer3.search_raw against None doc/meta from ChromaDB (#1011)#1013
bensig merged 1 commit intodevelopfrom
fix/layer3-search-raw-none-guard-1011

Conversation

@bensig
Copy link
Copy Markdown
Collaborator

@bensig bensig commented Apr 18, 2026

Summary

Fixes #1011.

Layer3.search_raw() in mempalace/layers.py has the same unguarded meta.get(...) pattern that #1007 / PR #999 fix in searcher.py. Applies the same two-line defensive coercion at the top of the results loop.

Change

One file, defensive guard added at the top of the search_raw results loop:

 for ...
+    meta = meta or {}
+    doc = doc or ""
     # ... downstream meta.get() / doc.* calls

Test plan

Related

Copy link
Copy Markdown
Member

@igorls igorls left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested locally (py 3.12, clean worktree on pr-1013):

Reproduction confirms fix. Wrote a small script that patches _get_collection to return the partial-flush shape ChromaDB emits (valid hit + None meta + None doc):

from unittest.mock import patch
from mempalace.layers import Layer3

class FakeCol:
    def query(self, **_):
        return {
            "documents": [["real doc text", None]],
            "metadatas": [[{"wing": "w1", "room": "r1", "source_file": "/a/b.md"}, None]],
            "distances": [[0.1, 0.42]],
        }

with patch("mempalace.layers._get_collection", return_value=FakeCol()):
    hits = Layer3(palace).search_raw("anything", n_results=2)
  • Without the fix (checked out develop's layers.py): AttributeError: 'NoneType' object has no attribute 'get' at layers.py:333 — exactly matches issue #1011.
  • With the fix (pr-1013 layers.py): 2 hits, second falls through cleanly with wing="unknown", room="unknown", source_file="?", text="".

Full test suite: 973 passed. The two-line guard exactly mirrors #999's pattern in searcher.py, so the code-review surface is trivial.

Worth a small follow-up (not this PR): a unit test that pins the None-meta behavior so neither this nor the searcher.py sibling regresses. The reproduction above is essentially that test.

LGTM.

@bensig
Copy link
Copy Markdown
Collaborator Author

bensig commented Apr 18, 2026

Code review

Found 2 issues:

  1. The sibling method Layer3.search has the identical unguarded meta.get(...) pattern and is NOT covered by this PR. It iterates over the same ChromaDB `zip(docs, metas, dists)` shape and calls `meta.get("wing", "?")`, `meta.get("room", "?")`, `meta.get("source_file", ...)` without a preceding `meta = meta or {}` guard — so the exact partial-flush / mid-delete failure mode this PR fixes for `search_raw` still raises `AttributeError` on `Layer3.search`. Ideally this PR extends the guard to both methods, or a follow-up does.

return "No results found."
lines = [f'## L3 — SEARCH RESULTS for "{query}"']
for i, (doc, meta, dist) in enumerate(zip(docs, metas, dists), 1):
similarity = round(1 - dist, 3)
wing_name = meta.get("wing", "?")
room_name = meta.get("room", "?")
source = Path(meta.get("source_file", "")).name if meta.get("source_file") else ""

  1. This is the third per-site guard for the same bug class (after fix(searcher): guard against None metadata in CLI print path #999 for searcher.py and this one for layers.py). Reviewer @jphein on #999 explicitly proposed consolidating the coercion at the ChromaDB adapter boundary (backends/chroma.py — coerce None metadatas to {} at the `ChromaCollection.query()` / `.get()` layer), calling the per-site approach "whack-a-mole across 8 call sites" and noting adapter-level consolidation would let every future caller inherit the guard for free. Worth a maintainer decision before this lands: either accept per-site guards as policy, or block in favor of an adapter-level fix that would render fix(searcher): guard against None metadata in CLI print path #999, fix: guard Layer3.search_raw against None doc/meta from ChromaDB (#1011) #1013, and any future sibling PR redundant.

"""ChromaDB-backed MemPalace collection adapter."""
import logging
import os
import sqlite3
import chromadb
from .base import BaseCollection
logger = logging.getLogger(__name__)
def _fix_blob_seq_ids(palace_path: str):
"""Fix ChromaDB 0.6.x -> 1.5.x migration bug: BLOB seq_ids -> INTEGER.
ChromaDB 0.6.x stored seq_id as big-endian 8-byte BLOBs. ChromaDB 1.5.x
expects INTEGER. The auto-migration doesn't convert existing rows, causing
the Rust compactor to crash with "mismatched types; Rust type u64 (as SQL
type INTEGER) is not compatible with SQL type BLOB".

🤖 Generated with Claude Code

- If this code review was useful, please react with 👍. Otherwise, react with 👎.

Same class of bug as #1007: ChromaDB's query() can return None in the
documents and metadatas arrays when a drawer's HNSW vector entry exists
but its metadata/document rows haven't been materialized. The code in
Layer3.search_raw (mempalace/layers.py) calls meta.get("wing", ...),
meta.get("room", ...), meta.get("source_file", ...) directly without
null safety, so it raises:

  AttributeError: 'NoneType' object has no attribute 'get'

Two-line defensive coercion matching the pattern in #1009 /
PR #999 for searcher.py: meta = meta or {}, doc = doc or "".
The hit still appears with its real distance; source/wing/room
fall back to their fallback values where the metadata row is missing.

Frequently hit on chromadb 1.5.x (root cause #1006). Even after the
chromadb floor lands (#1010), partial-state results remain possible
during interrupted mines and schema upgrade boundaries, so the guard
is worth having on its own.

Fixes #1011.
@bensig bensig force-pushed the fix/layer3-search-raw-none-guard-1011 branch from 66f33a6 to 49e9e04 Compare April 18, 2026 20:31
@bensig
Copy link
Copy Markdown
Collaborator Author

bensig commented Apr 18, 2026

Amended to also guard Layer3.search (sibling method with the identical pattern). Scorer confirmed it had the same unguarded meta.get() calls that would raise AttributeError under the same partial-flush conditions this PR addresses for search_raw. Two-line coercion mirrors the existing guard — no new comment block since the one above search_raw's guard already documents the pattern.

Remaining finding (adapter-level consolidation per @jphein on #999) is a maintainer design call — not pulling that into this PR.

@bensig bensig merged commit 6426669 into develop Apr 18, 2026
6 checks passed
@bensig bensig deleted the fix/layer3-search-raw-none-guard-1011 branch April 18, 2026 20:41
jphein added a commit to jphein/mempalace that referenced this pull request Apr 19, 2026
…uard

Merges MemPalace#990 (RFC 002 spec), MemPalace#1014 (BaseSourceAdapter/PalaceContext scaffolding),
MemPalace#1013 (Layer3.search_raw None guard), MemPalace#1012 (docs), MemPalace#1010 (chromadb >=1.5.4),
and MemPalace#998 (sweeper/tandem transcript safety net).

Fork changes preserved:
- quarantine_stale_hnsw() in chroma.py (guards HNSW/sqlite drift segfault)
- get-then-create instead of get_or_create (guards ChromaDB 1.5.x metadata segfault)
- paginated status() loop (guards SQLite variable limit on large palaces)
- searcher hits-loop, BM25 fallback, _count_in_scope
- .jsonl exempt from JUNK_FILE_SIZE cap (Claude Code transcripts can be large)
- _validate_where() + operator constants taken from upstream

Co-Authored-By: Claude Sonnet 4.6 <[email protected]>
eldar702 added a commit to eldar702/mempalace that referenced this pull request Apr 19, 2026
Chroma 1.5.x can return ``None`` inside the ``metadatas`` / ``documents``
lists of a query/get result for partially-flushed rows. The codebase
already has a systemic None-guard pattern (merged MemPalace#999, MemPalace#1013, MemPalace#1019)
but three call sites were still unguarded:

* ``mcp_server.tool_check_duplicate`` (``mcp_server.py:487-488``) —
  ``meta = results["metadatas"][0][i]`` followed by ``meta.get(...)``
  raises ``AttributeError: 'NoneType' object has no attribute 'get'``.
  The broad ``except Exception`` wrapper (line 504) swallows it and
  returns an uninformative ``"Duplicate check failed"``.

* ``layers.Layer1.generate`` (``layers.py:126``) — iterates
  ``zip(docs, metas)`` and calls ``meta.get(key)`` in the importance
  loop. A single None metadata blows up the entire wake-up render.

* ``layers.Layer2.retrieve`` (``layers.py:224``) — same pattern, same
  crash path for the on-demand render.

Apply the same ``meta = meta or {}`` / ``doc = doc or ""`` idiom used
by the merged guards in the search path. Three-line additions, no
behaviour change on well-formed results.

Tests added:

* ``test_check_duplicate_handles_none_metadata`` — mocks the collection
  query to return ``None`` for one metadata and document, asserts the
  call does not crash and the sentinel-rendered entry has wing/room "?"
  and empty content.
* ``test_layer1_handles_none_metadata`` / ``_handles_none_document``
* ``test_layer2_handles_none_metadata``

Relationship to other open PRs:

* **MemPalace#1019** guarded ``searcher.py`` loops. This PR extends the same
  guard to the three call sites MemPalace#1019 did not touch.
* **MemPalace#979** fixed ``tool_check_duplicate`` negative similarity but left
  the None-metadata path unguarded.
* Does not overlap **MemPalace#1013** (``Layer3.search_raw``) or **MemPalace#999**.
igorls added a commit that referenced this pull request Apr 19, 2026
Version bumps across pyproject.toml, mempalace/version.py, README badge,
uv.lock, and plugin manifests (.claude-plugin/*, .codex-plugin/*).

CHANGELOG aligned with main (post-3.3.1) and a new [3.3.2] section added
covering the 11 PRs merged on develop since v3.3.1 — silent-transcript-drop
fix + tandem sweeper (#998), None-metadata guards (#999, #1013),
chromadb ≥1.5.4 for Py 3.13/3.14 (#1010), Windows Unicode (#681),
HNSW quarantine recovery (#1000), PID stacking guard (#1023), doc-path
cleanup (#996, #1012), and RFC 001/002 internal scaffolding (#995, #1014, #990).
@igorls igorls mentioned this pull request Apr 19, 2026
8 tasks
jphein added a commit to jphein/mempalace that referenced this pull request Apr 24, 2026
…ndary

ChromaDB 1.5.x can return None inside the `metadatas` list even when
the write stored a dict. Three prior PRs added per-site `meta = meta
or {}` guards to compensate:

- MemPalace#999 — guards in searcher.py (CLI + API + closet-boost),
  miner.status(), and 4 mcp_server.py handlers
- MemPalace#1013 — guard in Layer3.search_raw()
- Various other paths picked up the same defensive pattern
  organically (e.g. palace_graph, layers.py L1 pre-filter)

That scatter works but violates the typed contract QueryResult and
GetResult declare: both advertise `metadatas: list[dict]`, never
`list[Optional[dict]]`. Every call site that forgot the guard is a
latent `AttributeError: 'NoneType' object has no attribute 'get'`.

This PR moves the coercion to the single place the type contract is
produced — ChromaCollection.query() and ChromaCollection.get() in
backends/chroma.py. Each individual None dict gets coerced to {} at
the same boundary where the outer list is already normalized via
_none_list_to_empty(). Downstream callers receive a guaranteed
list[dict] matching the declared return shape.

## Per-site guards

Leaving the existing guards in place as defensive belt-and-suspenders.
They become redundant after this boundary coercion but removing them
risks regressing any call path that might bypass the typed wrappers.
A follow-up cleanup PR can remove them once we're confident nothing
else reaches chromadb directly.

## Tests

Six new tests in tests/test_backends.py cover the boundary's behavior
on the known None-metadata shapes:
- query(): mixed None / dict in a single batch
- query(): all-None inner list
- query(): None dicts across multiple queries in one call
- get(): mixed None / dict
- get(): padding regression (short metas list → {} pads, never None)
- get(): metadatas not requested → empty list passthrough

Full test_backends.py suite: 35 passed (6 new).
Full repo suite: 1072 passed.
Ruff format + check clean.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

bug: Layer3.search_raw() in layers.py crashes on None metadata (same pattern as #1007)

2 participants