Repository navigation
fix(memory): read the complete scope in the substring search fallback - #6732
Conversation
_search_substring iterated list_facts without paginating, so the adapter-less fallback only ever saw the default page of 100 facts. With 150 matching facts in a scope (max_facts allows up to 500), memory_search returned 100 results for top_k=150 and silently dropped every match past the first page; DeerMem.search keeps these results whenever they are non-empty, so its uncapped fallback never ran. Read the complete scope instead, as the scoped index rebuild already does.
github-actions
Bot
added
area:agents
willem-bd
left a comment
willem-bd
left a comment
There was a problem hiding this comment.
Reviewed at 32ad7b4. The fix is correct and minimal: the new in-place filter is predicate-identical to list_facts (storage.py:1747 on this branch vs the new line 1957), load returns deep copies on both of its return paths so the copy isolation callers got through list_facts is preserved, and both paths already cost one load per scope so there is no read-amplification regression. Nothing in the package subclasses or monkeypatches list_facts, so bypassing it breaks no dispatch, and the manager-level DeerMem._substring_search was already uncapped, confirming this was the only capped path. I ran the new regression test against the PR head (21 passed, 1 skipped in the file) and against main source (fails at the len(results) == 150 assertion with 100 returned), so the red/green claim checks out. One nit on the test's boundary id below.
|
|
||
| assert len(results) == 150 | ||
| returned_ids = {item["fact"]["id"] for item in results} | ||
| assert {"fact-100", "fact-101", "fact-150"} <= returned_ids |
There was a problem hiding this comment.
nit: fact-100 is not past-page evidence — the old list_facts page (matched[0:100], insertion order) was exactly fact-001..fact-100, and I confirmed against main source that the pre-fix search returns fact-100 but not fact-101. The ids that actually carry the regression signal are fact-101 and fact-150 (plus the len(results) == 150 assertion above, which is where the red run fails). Consider dropping fact-100 or keeping it with a boundary note so a future reader does not mistake it for proof the search got past the old page.
There was a problem hiding this comment.
Dropped fact-100 in db32bd3 and left a boundary note in its place: the assertion now checks {"fact-101", "fact-150"} with a comment stating fact-001..fact-100 sat inside the old first page (matched[0:100], insertion order), so only fact-101/fact-150 are past-page evidence. The len(results) == 150 check stays as the red-run failure point.
fact-100 was the last id inside list_facts' old first page (``matched[0:100]`` in insertion order), so its presence does not distinguish the fixed search from the pre-fix page; fact-101 and fact-150 (plus the ``len(results) == 150`` check) carry the regression signal. Keep a boundary note so the remaining ids are not misread.
Why
FileMemoryStorage._search_substring— the search path used when the retrieval adapter is disabled (retrieval_adapter: ""is a documentedDeerMemConfigmode) — iteratedself.list_facts(filters=filters, **self._scope_kwargs(scope))(storage.py:1954 onmain).list_factsis paginated with a defaultlimit: int = 100(storage.py:1709) and this caller never paged, so a search only ever saw the first 100 facts of each scope.With 150 matching facts in one scope,
memory_searchreturned 100 results fortop_k=150and dropped every match past the first page — violating theMemoryManager.searchcontract, "Search the bucket's memory for facts matchingquery; return up totop_kranked by relevance" (manager.py:347-349). The cap propagates to callers becauseDeerMem.searchkeeps the indexed results whenever they are non-empty (results = indexed or self._substring_search(...), deer_mem.py:387), so the manager-level uncapped substring fallback never runs.The scoped
rebuild_indexpath already reads the complete scope for the same reason — "The bulk replace below drops every row this rebuild does not supply, so read the complete scope rather than a list_facts page" (storage.py:2021-2023). A ranking search must see the whole scope too.What changed
_search_substringnow reads the complete scope the same way the scoped rebuild does —self.load(**self._scope_kwargs(scope)).get("facts", [])— and applies thefiltersdict in place withlist_facts' exact match semantics.memory_searchnow returns up totop_kmatches from the whole scope instead of stopping at the first 100.loadreturns deep copies (storage.py:1474, 1478), so the copy isolation callers previously got throughlist_factsis unchanged.Surface area
frontend/backend/applanggraph.json, or prompt changedocker/or sandboxed executionskills/backend/pyproject.tomlorfrontend/package.jsontop_kcontract)Bug fix verification
backend/tests/test_memory_retrieval_adapter.py::test_substring_fallback_searches_every_fact_of_a_large_scopemainand green on this branch? yesmain:AssertionError: assert 100 == 150(150 matching facts seeded,top_k=200)1 passedValidation
cd backend && make lint—All checks passed!/2037 files already formattedbackend/tests/that import the touched storage module:775 passed, 2 skipped in 22.74sretrieval_adapter=""and 150 facts on disk:memory_search("widget", top_k=150)now returns 150 results, 50 of them from facts refactor: stess the time range while using web search tools #100–feat: add searxng search tool support #149 (was 100 before).cd backend && make test(four parallel shards, 29,660 passed): 6 failures, all in deadline/multiprocess-sensitive files unrelated to memory (test_skills_custom_multiprocess.py,test_skillscan_env_review_regressions.py,test_file_outline_atx.py); all 86 tests of those three files pass in isolation on this branch, and none of them import the memory module.AI assistance
Tool(s) used: Claude Code
How you used it: AI traced the
memory_searchpath, wrote the regression test, and wrote the one-method fix mirroring the scopedrebuild_indexpattern; I reviewed every line against the storage contract and the surrounding code.