Skip to content

feat(search): expose drawer_id in mempalace_search results - #1219

Open
pepo72 wants to merge 2 commits into
MemPalace:developfrom
pepo72:feat/expose-drawer-id-in-search
Open

feat(search): expose drawer_id in mempalace_search results#1219
pepo72 wants to merge 2 commits into
MemPalace:developfrom
pepo72:feat/expose-drawer-id-in-search

Conversation

@pepo72

@pepo72 pepo72 commented Apr 26, 2026

Copy link
Copy Markdown

Closes #1026.

Problem

mempalace_search returns text, wing, room, source_file, similarity etc. but not drawer_id. Any caller that wants to follow up with mempalace_delete_drawer, mempalace_get_drawer, or mempalace_update_drawer (all of which require drawer_id) has to drop into SQLite and reverse-lookup by content/metadata. This breaks the MCP abstraction and turns simple cleanup workflows into bespoke scripts.

Fix

Chroma's query() already returns ids on every result. Thread it through search_memories (searcher.py) into each result entry as drawer_id. Three lines.

Diff

     scored: list = []
-    for doc, meta, dist in zip(
+    for drawer_id, doc, meta, dist in zip(
+        _first_or_empty(drawer_results, "ids"),
         _first_or_empty(drawer_results, "documents"),
         _first_or_empty(drawer_results, "metadatas"),
         _first_or_empty(drawer_results, "distances"),
     ):
         ...
         entry = {
+            "drawer_id": drawer_id,
             "text": doc,
             ...

Test

Smoke-tested against a real palace (~1100 drawers) on develop-equivalent v3.3.3:

{"drawer_id": "drawer_wing_peter_health_2c4fb820a4186c5c", "wing": "wing_peter", "room": "health", ...}

Scope

Intentionally minimal — no rename, no schema, no behavior change beyond adding the field. Re-rank / hydration / closet-boost paths preserve the new key (they only pop _sort_key / _source_file_full / _chunk_index).

Closes MemPalace#1026.

Without drawer_id, callers must drop into SQLite to find a drawer's ID
before calling delete_drawer/get_drawer/update_drawer. Chroma already
returns ids on every query — just thread it through search_memories
into each result entry.
@jphein

jphein commented Apr 26, 2026

Copy link
Copy Markdown
Collaborator

Independent confirmation — landed the same change in our fork at 9a8bb77 yesterday. Same diff shape, +1 to the approach.

We extended it to two adjacent payloads with the same "consumer wants to follow up by ID" problem: tool_diary_read entries (for mempalace_diary_read) and tool_session_recovery_read entries (fork-only MCP tool for the dedicated session-recovery collection split). Happy to fold those into this PR, or land yours narrow and I'll file a follow-up — either works.

One tip from cherry-picking ours: a couple existing test mocks omit ids in mock_col.query.return_value (e.g. the BM25 hybrid rerank test in tests/test_searcher.py). Threading _first_or_empty(drawer_results, "ids") makes those fixtures collapse to zero hits because zip truncates to the shortest input. Defensive pad worked for us:

ids = _first_or_empty(drawer_results, "ids")
if not ids and drawer_results.get("documents"):
    ids = [None] * len(_first_or_empty(drawer_results, "documents"))

Production chromadb always returns ids — purely a test-fixture compatibility shim, but might be worth including here to avoid mock churn during review.

Closes #1026 either way.


Update 2026-04-27: going with the follow-up path — your PR can land narrow, and I'll file the diary + recovery slices as a small separate PR after. No need to expand scope here.

jphein added a commit to techempower-org/mempalace that referenced this pull request Apr 27, 2026
Production chromadb always returns ids, but some existing test
mocks omit them in mock_col.query.return_value. zip() truncates
to the shortest input, collapsing those fixtures to zero hits
once ids is threaded into the loop. Pad with None entries when
ids is empty but documents are present.
@pepo72

pepo72 commented Apr 27, 2026

Copy link
Copy Markdown
Author

Thanks for the review and the heads-up on the test mocks — added the defensive pad in 490e5b1. Happy to land narrow as suggested; looking forward to your follow-up PR for the diary and recovery slices.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/search Search and retrieval enhancement New feature or request storage

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Expose Drawer IDs in MemPalace Tools

3 participants