Skip to content

fix(searcher): scope neighbor expansion by parent_drawer_id (#1580) - #1582

Merged
igorls merged 1 commit into
MemPalace:developfrom
mvalentsev:fix/1580-neighbor-scope-parent-drawer-id
Jun 14, 2026
Merged

fix(searcher): scope neighbor expansion by parent_drawer_id (#1580)#1582
igorls merged 1 commit into
MemPalace:developfrom
mvalentsev:fix/1580-neighbor-scope-parent-drawer-id

Conversation

@mvalentsev

@mvalentsev mvalentsev commented May 22, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

What does this PR do?

Closes #1580. After #1539 the chunked tool_add_drawer path (mempalace/mcp_server.py:1212) writes per-chunk drawers tagged with a parent_drawer_id metadata field linking them to their logical group. Neighbor enrichment in search_memories (the inlined block at mempalace/searcher.py:967+, marked # Drawer-grep enrichment) and the sibling helper _expand_with_neighbors (mempalace/searcher.py:219+) filtered only by source_file + chunk_index, so two unrelated logical drawers that happened to pass the same source_file value (for example two pastes tagged "chat.log") had their chunks stitched together as if they were sequential context. Pre-#1539 the same call wrote a single drawer, so this could not happen.

Triggering condition. The bug fires when the matched chunk has a non-empty source_file shared with another parent_drawer_id group and the source is closet-boosted (the live enrichment block runs only for matched_via == "drawer+closet" hits). The repro shape in the issue (tool_add_drawer(content=...) with the default source_file) does not fire on develop because both call sites already guard with if not src (mempalace/searcher.py:239) and if not full_source: continue (mempalace/searcher.py:977); the default source_file or "" from mempalace/mcp_server.py:1150 is falsy and the enrichment short-circuits before the bad query runs. The repro that does fire passes an explicit shared non-empty source_file value.

Fix. A new module-private _scoped_source_filter (mempalace/searcher.py:194) builds the Chroma where clause:

  • when parent_drawer_id is supplied, returns a 2-clause $and scoping by (source_file, parent_drawer_id);
  • otherwise returns the original 1-clause {"source_file": ...} shape.

This mirrors the conditional-$and precedent in build_where_filter (mempalace/searcher.py:169). Both the inlined block in search_memories and the helper _expand_with_neighbors route through it. The neighbor fetch inside _expand_with_neighbors keeps its existing chunk_index $in target_indexes clause and conditionally appends the parent_drawer_id clause, building a 3-clause $and when present. total_drawers is scoped consistently so the returned count matches the returned text for chunked drawers (for example total_drawers == 2 for a 2-chunk group, not the full source-file row count).

Backwards compatibility. Drawers without parent_drawer_id (single-chunk tool_add_drawer writes, pre-#1539 palaces, and diary_ingest chunks grouped by real file path) keep the file-global 1-clause query shape. Verified by test_expand_backwards_compat_no_parent_drawer_id_returns_all_source_neighbors and the pre-existing test_expand_returns_matched_plus_neighbors. An empty-string parent_drawer_id value is treated the same way as absent, pinned by test_expand_empty_string_parent_drawer_id_treated_as_absent.

Scope. This PR addresses the chunked tool_add_drawer path only. tool_diary_write (mempalace/mcp_server.py:1705) chunks tag a different metadata key (parent_entry_id) and are written without a source_file field, so the existing if not src / if not full_source guards already prevent them from entering this enrichment path; no change there. Cross-parent_drawer_id closet boost behavior (one closet's source key boosts every drawer sharing source_file, including chunks from a different logical group) is orthogonal to the neighbor-stitching bug and not changed by this PR.

How to test

# Targeted: TestDrawerGrepExpansion now has 11 tests
# (6 pre-existing + 5 added by this PR).
uv run pytest tests/test_closets.py::TestDrawerGrepExpansion -v

# Search-related coverage:
uv run pytest tests/test_closets.py::TestDrawerGrepExpansion tests/test_searcher.py tests/test_hybrid_search.py tests/test_hybrid_candidate_union.py -v

# Full suite (no benchmarks):
uv run pytest tests/ -v --ignore=tests/benchmarks

# Lint per CI ruff pin:
uvx --from 'ruff==0.15.9' ruff check . && uvx --from 'ruff==0.15.9' ruff format --check .

New tests in tests/test_closets.py class TestDrawerGrepExpansion:

  • test_expand_isolates_chunks_by_parent_drawer_id_when_source_file_shared (tests/test_closets.py:1337): helper-level RED on develop, GREEN on this branch. Two parent_drawer_id groups (2 chunks each) under shared source_file="shared.log"; asserts _expand_with_neighbors returns only group A's chunks and total_drawers == 2 (scoped to group).
  • test_expand_backwards_compat_no_parent_drawer_id_returns_all_source_neighbors (tests/test_closets.py:1420): passes pre- and post-fix; pins the 1-clause fallback for legacy drawers and diary_ingest chunks.
  • test_hybrid_search_enrichment_isolates_chunks_across_drawers_sharing_source_file (tests/test_closets.py:1443): end-to-end through search_memories. Two groups under shared source_file, closet boost on group A, search query targeting group A content. Asserts the returned text contains only group A's content (no group B leakage), top["total_drawers"] == 2 (scoped count on the live path), and that the internal scoring-loop keys (_parent_drawer_id, _source_file_full, _chunk_index, _sort_key) are scrubbed from every returned hit.
  • test_expand_isolates_asymmetric_groups_under_shared_source_file (tests/test_closets.py:1551): asymmetric coverage (group A has 1 chunk, group B has 3) under shared source_file. Catches a regression where total_drawers accidentally drifts back to the unscoped file-global count when one group dominates the row mix.
  • test_expand_empty_string_parent_drawer_id_treated_as_absent (tests/test_closets.py:1613): contract pin. An empty-string parent_drawer_id value degrades to the 2-clause file-global filter, mirroring the empty-string handling for source_file in the entry guard.

Checklist

  • Tests pass (uv run pytest tests/ -v --ignore=tests/benchmarks)
  • No hardcoded paths
  • Linter passes (uvx --from 'ruff==0.15.9' ruff check . && uvx --from 'ruff==0.15.9' ruff format --check .)

gemini-code-assist[bot]
gemini-code-assist Bot reviewed May 22, 2026
View reviewed changes

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Code Review

This pull request introduces a mechanism to isolate memory chunks by parent_drawer_id when they share the same source_file. By adding the _scoped_source_filter helper and updating _expand_with_neighbors and search_memories, the system now correctly scopes neighbor expansion and total drawer counts to specific logical groups, preventing data leakage between unrelated drawers. Extensive regression tests have been added to ensure correct behavior for both new and legacy data. I have no feedback to provide as there were no review comments to assess.

@mvalentsev
mvalentsev marked this pull request as ready for review May 22, 2026 00:15
@mvalentsev
mvalentsev force-pushed the fix/1580-neighbor-scope-parent-drawer-id branch 5 times, most recently from 7913288 to 08d8ba2 Compare May 24, 2026 23:26
@mvalentsev mvalentsev mentioned this pull request May 26, 2026
Open
9 tasks
milla-jovovich added a commit that referenced this pull request May 29, 2026
@milla-jovovich
fix(searcher): scope drawer-grep enrichment by parent_drawer_id (#1580,…
06ab20d 
… second site)

@fatkobra cold-read the searcher on PR #1628 and caught a missed
#1580 site: the drawer-grep enrichment block in `search_memories`
(`searcher.py:1054`) still fetched by `source_file` alone after
amendment-2 scoped `_expand_with_neighbors`. Two unrelated chunked
drawer groups sharing the same `source_file` could still leak across
the enrichment boundary into a single rendered result.

@mvalentsev had filed PR #1582 carrying the same fix in a cleaner
shape — a shared `_scoped_source_filter` helper applied at both
`_expand_with_neighbors` and the enrichment block, with
`_parent_drawer_id` threaded through internal search hits and
stripped before return. We carry his patch into this amendment
verbatim; PR #1582 to be closed after merge.

Three corrections / honest acknowledgments

  1. @mvalentsev also flagged the amendment-2 commit body's claim
     that `.documents` attribute access "raised AttributeError in
     every code path." That was empirically wrong. `_expand_with_neighbors`
     receives a `ChromaCollection` wrapper from `palace.get_collection()`,
     whose `get()` returns a `GetResult` dataclass that inherits
     `_DictCompatMixin` — both `.documents` and `["documents"]` resolve.
     The crash I observed was in my amendment-2 test against a raw
     `chromadb.PersistentClient(...)`; I extrapolated to production
     without tracing `drawers_col` back through the wrapper. /bt miss
     by me. The subscript change can stay in (the test does guard
     against the wrapper ever dropping the mixin), but the rationale
     in the amendment-2 body is wrong.

  2. The new amendment-2 test (test_neighbors_do_not_cross_parent_drawer_id
     in tests/test_additive_mining_preservation.py) was updated to use
     `palace.get_collection()` (the production path) instead of raw
     chromadb. Matches the production exercise; passes against the
     wrapper as it should.

  3. mvalentsev's filter uses a 3-clause `$and` in `_expand_with_neighbors`
     (source_file + chunk_index + parent_drawer_id). Gemini's note on
     amendment-2 about a "2-only $and limit" turned out to be
     overcautious — empirically verified by both mvalentsev's test
     in PR #1582 and our new search_memories test running green
     against the wrapper. We adopt mvalentsev's 3-clause shape.

§1.8 styleguide audit — every bare `where={"source_file": ...}` in mempalace/

Per the new §1.8 styleguide panel (partial-scope-key-migration), a fix
that introduces a new scope key must grep every query against the
OLD coarser key and either migrate it or document why it is exempt.
Full repo audit, six external sites:

  - searcher.py
      Two sites already in scope (both call `_scoped_source_filter`
      via mvalentsev's patch). Strict grep for bare
      `where={"source_file":` in searcher.py returns empty.
      VERDICT: clean.

  - sync.py:305
      `closets_col.get(where={"source_file": {"$in": ...}})` —
      closet bulk purge during sync. Closets don't carry
      parent_drawer_id (their `upsert` at `palace.py:421` only takes
      the base metadata dict). `$in` over a list explicitly signals
      bulk file-global intent.
      VERDICT: not a candidate.

  - diary_ingest.py:235
      `drawers_col.delete(where={"source_file": source_file})` —
      inside the `if full_rebuild:` branch. Comment is explicit:
      must clear ALL drawers for the source (legacy `drawer_diary_`
      prefix, v2 prior-pass orphans, entry-boundary shifts).
      File-global is the whole point of full_rebuild.
      VERDICT: not a candidate.

  - palace.py:395
      `closets_col.delete(where={"source_file": source_file})` —
      closet purge before re-mine. Same family as sync.py:305 above:
      closets don't carry parent_drawer_id.
      VERDICT: not a candidate.

  - palace.py:763
      `collection.get(where={"source_file": source_file}, limit=1)`
      in `file_already_mined()` (extract_mode-is-None branch).
      Under PR #1593's additive mining, multiple parent_drawer_id
      groups for the same source_file can exist (one per mining pass);
      `limit=1` returns whichever ChromaDB happens to order first,
      and the function then checks `source_mtime` against that
      arbitrary group. If a stale group is picked when the latest
      group's mtime matches, the function returns False and the
      additive miner writes yet another duplicate layer.
      VERDICT: real bug, separate scope. Fixing correctly requires
      either iterating all groups or a fuzzy-mtime query;
      ChromaDB's `where` doesn't support fuzzy float equality, so
      the fix is bigger than a one-line patch and exceeds this
      amendment's scope. Filing follow-up issue.

  - palace.py:771
      Paginated `collection.get(where={"source_file": source_file},
      limit=1000)` in `file_already_mined()` (extract_mode-is-set
      branch). Full paginated scan with Python-side mode filtering
      downstream. File-global by design.
      VERDICT: not a candidate.

  - convo_miner.py:118
      Paginated `collection.get(where={"source_file": source_file},
      limit=1000)`. Same shape as palace.py:771. Comment confirms
      file-global intent ("deleting newer general-mode drawers for
      the same transcript").
      VERDICT: not a candidate.

Verification

  - macOS Python 3.12 (local) full pytest        : 2281 passed, 0 failed
  - Linux Python 3.9.25  (OrbStack) full pytest  : 2273 passed, 0 failed
                                                   (one isolation flake
                                                    in test_mcp_server.py
                                                    on first run, passed
                                                    on rerun in isolation
                                                    AND on full-suite
                                                    rerun — unrelated to
                                                    this amendment)
  - Linux Python 3.11.15 (OrbStack) full pytest  : 2275 passed, 0 failed
  - Linux Python 3.13.13 (OrbStack) full pytest  : 2275 passed, 0 failed
  - ruff check + ruff format --check             : all clean
  - empirically verified 3-clause `$and` works against ChromaCollection
    wrapper (both new tests pass)

Two new tests pin the failure space

  - test_expand_isolates_chunks_by_parent_drawer_id_when_source_file_shared
      Adapted from mvalentsev's PR #1582 test — `_expand_with_neighbors`
      with two parent_drawer_id groups sharing source_file.

  - test_search_memories_enrichment_isolates_by_parent_drawer_id
      New Cedar test — the search_memories enrichment site fatkobra
      flagged. Same fixture pattern as mvalentsev's test plus a closet
      pointing at group A's chunk to trigger `matched_via == "drawer+closet"`
      and exercise the enrichment block. Confirmed RED on broken code
      (BRAVO_GROUP_B leaks into ALPHA_GROUP_A's enriched result),
      GREEN on the fixed code.

Credit

  - @fatkobra — cold-read review caught the missed enrichment site
  - @mvalentsev — pre-existing PR #1582 with the cleaner helper shape,
    the dict-compat-mixin correction, and the regression test for
    `_expand_with_neighbors`
  - Wick (cold-read instance) — sphere walk of the styleguide §1.8
    requirement, the 6-external-site audit, and identification of
    palace.py:763 as the worth-second-look site

Closes #1580 (the second site).
milla-jovovich added a commit that referenced this pull request May 29, 2026
@milla-jovovich
test(searcher): carry remaining 3 #1582 regression tests + correct $a…
4233153 
…nd-limit claim

Amendment-3 carried @mvalentsev's `_scoped_source_filter` patch but
only one of his five regression tests. @mvalentsev surfaced the gap
in his follow-up review on PR #1628 and offered to have them carried
here. This amendment carries the three remaining ones verbatim.

Three new tests (all in tests/test_closets.py, TestDrawerGrepExpansion):

  - test_expand_backwards_compat_no_parent_drawer_id_returns_all_source_neighbors
      Pins legacy-drawer behaviour: when a hit has no `parent_drawer_id`
      in its metadata (drawers written before the field existed),
      neighbor expansion still returns ALL chunks under the
      `source_file` scope. Catches a regression where the new scoping
      helper would accidentally narrow the legacy path.

  - test_expand_isolates_asymmetric_groups_under_shared_source_file
      Asymmetric coverage — group A has 1 chunk, group B has 3 chunks,
      shared source_file. Defensive edge case that catches a regression
      where the helper would over-count or under-fetch when one group
      is much smaller than the other.

  - test_expand_empty_string_parent_drawer_id_treated_as_absent
      Contract pin — an empty-string `parent_drawer_id` value
      (`""`, e.g. from a stripped JSON field) degrades to the
      2-clause file-global filter, matching the `if parent_id:`
      truthiness check in `_scoped_source_filter`. Without this
      pin, a future refactor that switched the check to
      `if parent_id is not None:` would silently change behaviour
      for the empty-string case.

Cedar's amendment-3 test (`test_search_memories_enrichment_isolates_by_parent_drawer_id`)
overlaps with @mvalentsev's `test_hybrid_search_enrichment_isolates_chunks_across_drawers_sharing_source_file`
in surface and intent. Both stay — different fixture setups exercise
the same code path slightly differently, and the redundancy is the
cheap kind. Net new in this amendment: 3 tests.

Honest correction on the `$and` limit claim — round 2

The amendment-2 commit body said gemini's "ChromaDB $and only accepts
exactly two dictionaries" finding was the reason I rewrote
_expand_with_neighbors to drop `source_file` when `parent_id` is
present. The amendment-3 body softened this to "overcautious for the
wrapper layer" and adopted @mvalentsev's 3-clause shape. @mvalentsev
went further and cited the actual ChromaDB source — `validate_where`
at chromadb/api/types.py requires `$and`/`$or` to hold AT LEAST two
expressions (`len(value) <= 1 -> raise`), with NO upper bound. The
codebase already builds a 3-clause $and at `mcp_server.py:1764`
(diary list with wing + room + agent), through the same wrapper.

So gemini was directionally wrong (it's a minimum, not a maximum),
and my 2-clause amendment-2 workaround was unnecessary all along.
@mvalentsev's 3-clause shape (which we already carried in
amendment-3) is the correct production form. Keeping `source_file`
in the neighbor filter does not lean on `parent_drawer_id` always
being derived from `source_file`, and costs nothing.

The styleguide §1.8 panel referenced gemini's 2-only claim as the
amplifier of the partial-scope-key-migration defect class. That
reference is now wrong and should be removed or rewritten when
someone next touches §1.8. Filing a separate small PR to update the
private styleguide; not in scope for this amendment to mempalace.

Verification

  - macOS Python 3.12 (local) full pytest  : 2284 passed, 0 failed
  - Linux Python 3.9.25  (OrbStack)        : 2277 passed, 0 failed
  - Linux Python 3.11.15 (OrbStack)        : 2278 passed, 0 failed
  - Linux Python 3.13.13 (OrbStack)        : 2278 passed, 0 failed
  - ruff check + ruff format --check       : all clean
  - TestDrawerGrepExpansion class (11 tests, was 8 before this amendment):
    all green

Credit

  - @mvalentsev — PR #1582 owner, author of all 3 tests carried here,
    empirical $and-limit verification with chromadb source citation,
    and the lifting reminder that we carry not just the helper but
    the regression coverage that goes with it.
@mvalentsev
mvalentsev force-pushed the fix/1580-neighbor-scope-parent-drawer-id branch from 08d8ba2 to fb0c2a1 Compare May 30, 2026 17:45
@mvalentsev
mvalentsev force-pushed the fix/1580-neighbor-scope-parent-drawer-id branch from fb0c2a1 to c4da6d5 Compare June 6, 2026 13:48
@igorls
igorls merged commit 0a2937d into MemPalace:develop Jun 14, 2026
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@milla-jovovich milla-jovovich Awaiting requested review from milla-jovovich milla-jovovich is a code owner
@igorls igorls Awaiting requested review from igorls igorls is a code owner
1 more reviewer
@gemini-code-assist gemini-code-assist[bot] gemini-code-assist[bot] left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

searcher: _expand_with_neighbors stitches unrelated chunks across MCP drawers sharing empty source_file

2 participants

@mvalentsev @igorls