mirror of
https://github.com/turnstonelabs/turnstone.git
synced 2026-08-15 00:12:26 -06:00
Patrick Buckley
2fd0c29a92
* fix(memory): query-aware candidate selection + OR-of-terms search The system-message memory composition path used a recency-ordered candidate set (`_list_visible_memories(limit=fetch_limit)`). On deployments with more than `fetch_limit` (default 50) visible memories, BM25 only ever ranked the 50 most-recently-touched memories — a relevant memory written months ago was silently invisible regardless of how well it matched the recent context. Multi-word search at the SQL layer used AND-of-terms, killing recall on any multi-word query without an exact field overlap. ## Functional changes - `_init_system_messages` (`turnstone/core/session.py`): extract recent context first, then `_search_visible_memories(context)` to pull query-aware candidates. Search hits below `fetch_limit` union with the recency list (deduped by memory_id) so the BM25 candidate pool is always a SUPERSET of the prior recency-only pool — even on noisy queries where the cap fills with stopwords, the recency-50 the original bug surfaced still reaches BM25. Empty context skips search entirely. Candidate-selection logic extracted into `_select_memory_candidates`. - `search_structured_memories` (PostgreSQL + SQLite): per-term clauses join with OR instead of AND. A row matches if ANY term matches ANY of name/description/content. Downstream BM25 narrows back down by relevance. ## Perf hardening - Collapse the 1-3 fanned scope queries into a single SQL. New backend methods `list_visible_structured_memories` / `search_visible_structured_memories` union the visibility scopes into one WHERE OR-group, so a composition rebuild now hits the DB at most twice (search + recency) instead of up to six times. - Cap and normalize search terms. Composition can hand a multi-KB pasted message to ILIKE-based search; without a cap, every distinct token would emit one unindexable predicate per scope-fanned query. `normalize_search_terms` (`storage/_utils.py`) de-dupes case-insensitively, drops <2-char tokens, and hard-caps at 16. - Per-turn search cache. `_init_system_messages` fires from many call sites within one turn (state transitions, MCP refresh, tool results) and the recent-context query is identical across them. Session-instance cache keyed by (query, mem_type, limit) absorbs the duplicates; invalidated in `_append_user_turn` and after memory save/delete tool actions. - Stable secondary sort by `memory_id`. `updated` is second-precision and `touch_structured_memories` can land a batch on identical timestamps; without a tie-breaker SQL returns rows in implementation-defined order, BM25 input shuffles, and the LLM-side prompt cache misses across calls. All four backend ORDER BYs now break ties on `memory_id ASC`. ## Quality cleanups - Coalesce `memory.search.term_count` + `memory.search.zero_results` into a single `memory.search` log carrying both `term_count` and `result_count`. - New `memory.composition` log: source / candidates / injected. - Promote a shared `make_chat_session` factory to `tests/_helpers.py`. - Rename SQL builder local `extra` -> `scope_filters` for clarity. - Add docstrings on `search_structured_memories` so the AND->OR flip survives future readers. ## Tests Adds 20 tests across `tests/test_structured_memory.py`, `tests/test_structured_memory_storage.py`, and `tests/test_memory_relevance.py`: recency-ceiling regression, empty-query fallback, sparse-match union, recency-preserved-when- search-returns-noise (locks in the pool-superset invariant), OR-of-terms on both backends, scope filtering preserved, search-facade multi-word behavior, term-cap normalization, the new visible-scope helpers (list + search + empty-scopes guard), coord-scope composition isolation, end-to-end `memory(action='search')` tool execution, per-turn cache hit + invalidation, and stable ordering under tied `updated` timestamps. Memory test sweep: 102/102. Broader regression (session, storage, coordinator, load_skill): 411/411. * fix(memory): address Copilot review on PR #468 Three follow-ups from Copilot's inline review: 1. SUPERSET invariant violation (Copilot, session.py:5510). `(search_hits + extra)[:fetch_limit]` capped the union back down to fetch_limit, evicting the recency tail when search added distinct hits. Recency tail is exactly where ancient-but-recently-touched memories live — the recall this PR is supposed to improve — so tail eviction recreated the bug for the narrow case where a query term fell off the 16-cap and the matching memory sat in recency[40-49]. Drop the cap; both halves are already SQL-capped at fetch_limit, so the union is at most 2 × fetch_limit (~100 with defaults). BM25 over 100 candidates in pure Python is sub-ms; irrelevant recency fillers get score=0 and don't pollute ranking. Updates the docstring to actually be honest about the invariant. Adds `test_recency_tail_preserved_when_search_adds_distinct_hits` that locks the behavior in: 5 search hits + 10 recency = 15-item pool, every recency item present, source="union". 2. Unbounded `query.split()` in normalize_search_terms (Copilot, _utils.py:74). `str.split()` allocates the full token list before the cap-after-16 break, so a 100KB pasted query did MB of throwaway work even though only 16 tokens entered SQL. Switch to `re.finditer(r'\S+', query)` — streaming iterator, stops scanning at the first 16 normalized terms regardless of input size. 3. Misleading + unbounded log term_count (Copilot, session.py:8571). `len(item["query"].split())` had two problems: same unbounded split as #2, and the value reported the raw input token count rather than the normalized term count that actually hit the SQL WHERE clause — misleading metric for an operator trying to understand storage-side behavior. Switch to `len(normalize_search_terms(item["query"]))` — accurate count, and bounded for free via #2. Refuted: github-code-quality flagged `...` bodies in the new Protocol methods as "statement has no effect." False positive — `...` is the canonical Protocol body convention, used 213 other times in the same file. Memory test sweep: 103/103. Broader regression: 411/411.