mirror of
https://github.com/turnstonelabs/turnstone.git
synced 2026-08-27 22:34:51 -06:00
89b6b299f7
* 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.
460 lines
18 KiB
Python
460 lines
18 KiB
Python
"""Tests for turnstone.core.memory_relevance — scoring, formatting, context extraction."""
|
||
|
||
from unittest.mock import patch
|
||
|
||
from turnstone.core.memory_relevance import (
|
||
MemoryConfig,
|
||
build_memory_context,
|
||
extract_recent_context,
|
||
score_memories,
|
||
)
|
||
|
||
# ---------------------------------------------------------------------------
|
||
# score_memories
|
||
# ---------------------------------------------------------------------------
|
||
|
||
|
||
class TestScoreMemories:
|
||
def test_empty_memories(self):
|
||
assert score_memories([], "query") == []
|
||
|
||
def test_empty_query_returns_recent(self):
|
||
mems = [
|
||
{"name": "a", "description": "", "content": "alpha"},
|
||
{"name": "b", "description": "", "content": "beta"},
|
||
{"name": "c", "description": "", "content": "gamma"},
|
||
]
|
||
result = score_memories(mems, "", k=2)
|
||
assert len(result) == 2
|
||
assert result[0]["name"] == "a"
|
||
|
||
def test_whitespace_query_returns_recent(self):
|
||
mems = [{"name": "a", "description": "", "content": "alpha"}]
|
||
assert score_memories(mems, " ", k=5) == mems
|
||
|
||
def test_relevance_ranking(self):
|
||
mems = [
|
||
{"name": "cooking", "description": "recipes", "content": "pasta sauce tomato"},
|
||
{"name": "python", "description": "programming", "content": "python file io disk"},
|
||
{
|
||
"name": "disk_io",
|
||
"description": "file operations",
|
||
"content": "read write file disk",
|
||
},
|
||
]
|
||
result = score_memories(mems, "file disk", k=2)
|
||
names = [m["name"] for m in result]
|
||
assert "disk_io" in names
|
||
assert "python" in names
|
||
|
||
def test_k_limits_results(self):
|
||
mems = [{"name": f"m{i}", "description": "", "content": f"word{i}"} for i in range(10)]
|
||
result = score_memories(mems, "word0 word1 word2", k=2)
|
||
assert len(result) <= 2
|
||
|
||
def test_no_match_returns_empty(self):
|
||
mems = [{"name": "a", "description": "", "content": "hello world"}]
|
||
result = score_memories(mems, "zzzznotfound")
|
||
assert result == []
|
||
|
||
def test_uses_name_for_scoring(self):
|
||
mems = [
|
||
{"name": "database_config", "description": "", "content": "host=localhost"},
|
||
{"name": "unrelated", "description": "", "content": "nothing here"},
|
||
]
|
||
result = score_memories(mems, "database", k=1)
|
||
assert len(result) == 1
|
||
assert result[0]["name"] == "database_config"
|
||
|
||
def test_uses_description_for_scoring(self):
|
||
mems = [
|
||
{"name": "x", "description": "postgresql connection settings", "content": "host=db"},
|
||
{"name": "y", "description": "unrelated", "content": "nothing"},
|
||
]
|
||
result = score_memories(mems, "postgresql", k=1)
|
||
assert result[0]["name"] == "x"
|
||
|
||
|
||
# ---------------------------------------------------------------------------
|
||
# build_memory_context
|
||
# ---------------------------------------------------------------------------
|
||
|
||
|
||
class TestBuildMemoryContext:
|
||
def test_empty_memories(self):
|
||
assert build_memory_context([]) == ""
|
||
|
||
def test_single_memory(self):
|
||
mems = [{"name": "test", "type": "project", "scope": "global", "content": "hello"}]
|
||
ctx = build_memory_context(mems)
|
||
assert "<memories>" in ctx
|
||
assert "</memories>" in ctx
|
||
assert 'name="test"' in ctx
|
||
assert "hello" in ctx
|
||
|
||
def test_html_escaping(self):
|
||
mems = [
|
||
{
|
||
"name": "a<b",
|
||
"type": "project",
|
||
"scope": "global",
|
||
"content": "x & y",
|
||
"description": 'say "hi"',
|
||
}
|
||
]
|
||
ctx = build_memory_context(mems)
|
||
assert "<" in ctx
|
||
assert "&" in ctx
|
||
assert """ in ctx
|
||
|
||
def test_truncates_long_content(self):
|
||
mems = [
|
||
{
|
||
"name": "long",
|
||
"type": "project",
|
||
"scope": "global",
|
||
"content": "x" * 600,
|
||
}
|
||
]
|
||
ctx = build_memory_context(mems)
|
||
assert "..." in ctx
|
||
# Content should be truncated to 500 chars + "..."
|
||
assert "x" * 501 not in ctx
|
||
|
||
def test_description_attribute(self):
|
||
mems = [
|
||
{
|
||
"name": "test",
|
||
"type": "project",
|
||
"scope": "global",
|
||
"content": "data",
|
||
"description": "some desc",
|
||
}
|
||
]
|
||
ctx = build_memory_context(mems)
|
||
assert 'description="some desc"' in ctx
|
||
|
||
def test_no_description_attribute_when_empty(self):
|
||
mems = [{"name": "test", "type": "project", "scope": "global", "content": "data"}]
|
||
ctx = build_memory_context(mems)
|
||
assert "description=" not in ctx
|
||
|
||
|
||
# ---------------------------------------------------------------------------
|
||
# extract_recent_context
|
||
# ---------------------------------------------------------------------------
|
||
|
||
|
||
class TestExtractRecentContext:
|
||
def test_extracts_user_messages(self):
|
||
msgs = [
|
||
{"role": "user", "content": "hello"},
|
||
{"role": "assistant", "content": "hi"},
|
||
{"role": "user", "content": "world"},
|
||
]
|
||
ctx = extract_recent_context(msgs, max_messages=2)
|
||
assert "world" in ctx
|
||
assert "hello" in ctx
|
||
|
||
def test_skips_non_user(self):
|
||
msgs = [
|
||
{"role": "assistant", "content": "ignored"},
|
||
{"role": "user", "content": "included"},
|
||
]
|
||
ctx = extract_recent_context(msgs, max_messages=5)
|
||
assert "included" in ctx
|
||
assert "ignored" not in ctx
|
||
|
||
def test_respects_max_messages(self):
|
||
msgs = [
|
||
{"role": "user", "content": "first"},
|
||
{"role": "user", "content": "second"},
|
||
{"role": "user", "content": "third"},
|
||
]
|
||
ctx = extract_recent_context(msgs, max_messages=1)
|
||
assert "third" in ctx
|
||
assert "first" not in ctx
|
||
|
||
def test_handles_list_content(self):
|
||
msgs = [
|
||
{
|
||
"role": "user",
|
||
"content": [
|
||
{"type": "text", "text": "multi-part"},
|
||
{"type": "image_url", "image_url": {"url": "http://example.com"}},
|
||
],
|
||
}
|
||
]
|
||
ctx = extract_recent_context(msgs, max_messages=1)
|
||
assert "multi-part" in ctx
|
||
|
||
def test_handles_string_parts_in_list(self):
|
||
msgs = [{"role": "user", "content": ["plain string part"]}]
|
||
ctx = extract_recent_context(msgs, max_messages=1)
|
||
assert "plain string part" in ctx
|
||
|
||
def test_empty_messages(self):
|
||
assert extract_recent_context([]) == ""
|
||
|
||
|
||
# ---------------------------------------------------------------------------
|
||
# Composition candidate-selection (_init_system_messages)
|
||
# ---------------------------------------------------------------------------
|
||
|
||
|
||
def _make_mem(name: str, content: str = "", memory_id: str | None = None) -> dict[str, str]:
|
||
return {
|
||
"name": name,
|
||
"memory_id": memory_id or f"mid_{name}",
|
||
"type": "project",
|
||
"scope": "global",
|
||
"scope_id": "",
|
||
"description": "",
|
||
"content": content or name,
|
||
"updated": "2024-01-01T00:00:00",
|
||
}
|
||
|
||
|
||
def _make_session(fetch_limit: int = 5, relevance_k: int = 3, **kwargs: object):
|
||
"""Composition tests need a real ChatSession (constructor calls
|
||
``_init_system_messages`` once, unpatched, before the test gets a chance
|
||
to install patches). ``tmp_db`` initializes the storage singleton that
|
||
constructor needs; tests then patch the visibility helpers and call
|
||
``_init_system_messages`` a second time to exercise the new logic.
|
||
"""
|
||
from tests._helpers import make_chat_session
|
||
|
||
return make_chat_session(
|
||
memory_config=MemoryConfig(fetch_limit=fetch_limit, relevance_k=relevance_k),
|
||
**kwargs,
|
||
)
|
||
|
||
|
||
class TestCompositionCandidateSelection:
|
||
"""Verify the query-aware candidate set in _init_system_messages."""
|
||
|
||
def test_recency_ceiling_regression(self, tmp_db):
|
||
"""Old relevant memory not in recency top-N still injected via search path."""
|
||
session = _make_session(fetch_limit=5, relevance_k=3)
|
||
session.messages = [{"role": "user", "content": "postgres database configuration"}]
|
||
|
||
old_mem = _make_mem(
|
||
"ancient_db_config",
|
||
content="postgres database configuration connection host port",
|
||
memory_id="m_old",
|
||
)
|
||
# Recency top-5 do not include old_mem
|
||
recent = [_make_mem(f"recent_{i}", memory_id=f"mr{i}") for i in range(5)]
|
||
|
||
with (
|
||
patch.object(session, "_search_visible_memories", return_value=[old_mem]),
|
||
patch.object(session, "_list_visible_memories", return_value=recent),
|
||
):
|
||
session._init_system_messages()
|
||
|
||
joined = "\n".join(m["content"] for m in session.system_messages if m["role"] == "system")
|
||
# With the fix, old_mem enters the candidate pool via search and wins BM25
|
||
assert "ancient_db_config" in joined
|
||
|
||
def test_empty_query_falls_back_to_recency(self, tmp_db):
|
||
"""No user messages → empty context → recency path, search never called."""
|
||
session = _make_session()
|
||
session.messages = [] # extract_recent_context returns ""
|
||
|
||
recency = [_make_mem("note_alpha"), _make_mem("note_beta")]
|
||
|
||
with (
|
||
patch.object(session, "_list_visible_memories", return_value=recency),
|
||
patch.object(session, "_search_visible_memories") as search_mock,
|
||
):
|
||
session._init_system_messages()
|
||
|
||
search_mock.assert_not_called()
|
||
joined = "\n".join(m["content"] for m in session.system_messages if m["role"] == "system")
|
||
assert "note_alpha" in joined
|
||
|
||
def test_sparse_match_union_fills_candidate_pool(self, tmp_db):
|
||
"""Search returning < fetch_limit results unions with recency fillers."""
|
||
session = _make_session(fetch_limit=5, relevance_k=4)
|
||
session.messages = [{"role": "user", "content": "unique_term xyzzy"}]
|
||
|
||
hit_a = _make_mem("hit_alpha", content="unique_term xyzzy alpha", memory_id="m_ha")
|
||
hit_b = _make_mem("hit_beta", content="unique_term xyzzy beta", memory_id="m_hb")
|
||
search_hits = [hit_a, hit_b] # 2 < fetch_limit=5 → triggers union
|
||
|
||
# Recency overlaps on hit_a/hit_b and adds 3 fillers
|
||
filler = [_make_mem(f"filler_{i}", memory_id=f"mf{i}") for i in range(3)]
|
||
recency = [hit_a, hit_b] + filler
|
||
|
||
with (
|
||
patch.object(session, "_search_visible_memories", return_value=search_hits),
|
||
patch.object(session, "_list_visible_memories", return_value=recency),
|
||
):
|
||
session._init_system_messages()
|
||
|
||
joined = "\n".join(m["content"] for m in session.system_messages if m["role"] == "system")
|
||
# Both hits match "unique_term xyzzy" well → appear after BM25 ranking
|
||
assert "hit_alpha" in joined
|
||
assert "hit_beta" in joined
|
||
|
||
def test_recency_preserved_when_search_returns_noise_above_relevance_k(self, tmp_db):
|
||
"""Pool guarantee: recency-50 always reaches BM25, even when search
|
||
returns enough noise hits to clear ``relevance_k``.
|
||
|
||
Closes the narrow regression vs. the original bug — without the
|
||
``fetch_limit`` threshold, a stopword-dominated cap-search that
|
||
returned >= relevance_k irrelevant hits would short-circuit and
|
||
evict the recency-only memory the bug had been surfacing.
|
||
"""
|
||
session = _make_session(fetch_limit=10, relevance_k=3)
|
||
session.messages = [{"role": "user", "content": "configure host"}]
|
||
|
||
# Search returns relevance_k=3 noise hits — enough to skip recency
|
||
# under the OLD threshold, not enough to fill fetch_limit=10.
|
||
noise = [
|
||
_make_mem(f"noise_{i}", content="generic content", memory_id=f"mn{i}") for i in range(3)
|
||
]
|
||
# The memory the user actually wants — distinctive, in recency,
|
||
# but its content doesn't share any token with the noise hits.
|
||
wanted = _make_mem(
|
||
"host_config_v2",
|
||
content="host=localhost port=5432 db=production",
|
||
memory_id="m_wanted",
|
||
)
|
||
recency = [wanted] + [_make_mem(f"recent_{i}", memory_id=f"mr{i}") for i in range(5)]
|
||
|
||
with (
|
||
patch.object(session, "_search_visible_memories", return_value=noise),
|
||
patch.object(session, "_list_visible_memories", return_value=recency),
|
||
):
|
||
session._init_system_messages()
|
||
|
||
joined = "\n".join(m["content"] for m in session.system_messages if m["role"] == "system")
|
||
# ``wanted`` reached BM25 via the union and matched "host" → injected.
|
||
assert "host_config_v2" in joined
|
||
|
||
def test_recency_tail_preserved_when_search_adds_distinct_hits(self, tmp_db):
|
||
"""SUPERSET invariant: every recency item is in the candidate pool
|
||
when search adds hits, even if the resulting union exceeds
|
||
fetch_limit. Truncating the union at fetch_limit (the prior
|
||
behavior) evicted the recency tail — which is exactly where
|
||
ancient-but-recently-touched memories live, the recall this PR
|
||
sets out to improve.
|
||
"""
|
||
session = _make_session(fetch_limit=10, relevance_k=3)
|
||
session.messages = [{"role": "user", "content": "alpha"}]
|
||
|
||
# 5 search hits, none of which appear in recency.
|
||
search_hits = [
|
||
_make_mem(f"search_{i}", content="alpha", memory_id=f"ms{i}") for i in range(5)
|
||
]
|
||
# 10 recency items; without the union uncap, the 5 oldest of these
|
||
# would be displaced by the 5 search hits.
|
||
recency = [_make_mem(f"recency_{i}", memory_id=f"mr{i}") for i in range(10)]
|
||
|
||
with (
|
||
patch.object(session, "_search_visible_memories", return_value=search_hits),
|
||
patch.object(session, "_list_visible_memories", return_value=recency),
|
||
):
|
||
candidates, source = session._select_memory_candidates("alpha")
|
||
|
||
candidate_ids = {c["memory_id"] for c in candidates}
|
||
# Pool is search_hits ∪ recency — 15 items, no truncation.
|
||
assert len(candidates) == 15
|
||
assert source == "union"
|
||
# Every recency item present (no tail eviction).
|
||
for i in range(10):
|
||
assert f"mr{i}" in candidate_ids, f"recency item {i} evicted"
|
||
# And every search hit is also in the pool.
|
||
for i in range(5):
|
||
assert f"ms{i}" in candidate_ids, f"search hit {i} missing"
|
||
|
||
def test_coord_scope_isolated_visibility(self, tmp_db):
|
||
"""Coord composition queries the coord scope alone, never the
|
||
global/workstream/user union."""
|
||
from turnstone.core.workstream import WorkstreamKind
|
||
|
||
coord = _make_session(
|
||
fetch_limit=5,
|
||
relevance_k=3,
|
||
ws_id="coord-1",
|
||
user_id="user-1",
|
||
kind=WorkstreamKind.COORDINATOR,
|
||
)
|
||
scopes = coord._visible_scopes()
|
||
assert scopes == [("coordinator", "coord-1")]
|
||
# And: search uses those same scopes (no global/user fan-in)
|
||
coord.messages = [{"role": "user", "content": "anything"}]
|
||
with patch(
|
||
"turnstone.core.session.search_visible_structured_memories",
|
||
return_value=[],
|
||
) as search_mock:
|
||
coord._search_visible_memories("anything", limit=5)
|
||
search_mock.assert_called_once()
|
||
# Second positional arg is the scopes list
|
||
assert search_mock.call_args.args[1] == [("coordinator", "coord-1")]
|
||
|
||
|
||
class TestMemorySearchToolExecution:
|
||
"""End-to-end test of ``memory(action='search')`` through _exec_memory.
|
||
|
||
Drives the actual tool dispatch (not just the storage facade) so the
|
||
OR-of-terms fix and the coalesced ``memory.search`` log get exercised
|
||
together.
|
||
"""
|
||
|
||
def test_search_action_returns_or_of_terms_results(self, tmp_db):
|
||
"""Multi-word query returns rows where ANY term matches — not all."""
|
||
from turnstone.core.memory import save_structured_memory
|
||
|
||
save_structured_memory("postgres_notes", "host=localhost port=5432")
|
||
save_structured_memory("redis_notes", "host=redis port=6379")
|
||
save_structured_memory("unrelated", "completely different")
|
||
|
||
session = _make_session()
|
||
item = session._prepare_memory(
|
||
"call-1",
|
||
{"action": "search", "query": "postgres no_such_word_a no_such_word_b"},
|
||
)
|
||
# Sanity: prepare returned a search-ready dispatch (not an error item)
|
||
assert item.get("action") == "search"
|
||
|
||
call_id, msg = session._exec_memory(item)
|
||
assert call_id == "call-1"
|
||
assert "postgres_notes" in msg
|
||
# Other memories don't match any query term
|
||
assert "unrelated" not in msg
|
||
|
||
|
||
class TestPerTurnSearchCache:
|
||
"""The per-turn cache spares redundant SQL across mid-turn rebuilds."""
|
||
|
||
def test_repeated_search_in_same_turn_hits_cache(self, tmp_db):
|
||
from turnstone.core.memory import save_structured_memory
|
||
|
||
save_structured_memory("hello_mem", "alpha beta gamma")
|
||
session = _make_session()
|
||
with patch(
|
||
"turnstone.core.session.search_visible_structured_memories",
|
||
return_value=[],
|
||
) as backend_mock:
|
||
session._search_visible_memories("alpha beta", limit=5)
|
||
session._search_visible_memories("alpha beta", limit=5)
|
||
session._search_visible_memories("alpha beta", limit=5)
|
||
# 3 calls but only 1 backend hit — cache absorbed the rest
|
||
assert backend_mock.call_count == 1
|
||
|
||
def test_user_turn_invalidates_cache(self, tmp_db):
|
||
from turnstone.core.memory import save_structured_memory
|
||
|
||
save_structured_memory("hello_mem", "alpha")
|
||
session = _make_session()
|
||
with patch(
|
||
"turnstone.core.session.search_visible_structured_memories",
|
||
return_value=[],
|
||
) as backend_mock:
|
||
session._search_visible_memories("alpha", limit=5)
|
||
session._invalidate_memory_cache() # simulates new user turn
|
||
session._search_visible_memories("alpha", limit=5)
|
||
assert backend_mock.call_count == 2
|