diff --git a/plugins/memory/holographic/retrieval.py b/plugins/memory/holographic/retrieval.py index 2bbcb71f473e9..d0e7990846a25 100644 --- a/plugins/memory/holographic/retrieval.py +++ b/plugins/memory/holographic/retrieval.py @@ -119,6 +119,7 @@ def search( # Strip raw HRR bytes — callers expect JSON-serializable dicts for fact in results: fact.pop("hrr_vector", None) + self.store.record_retrievals([f["fact_id"] for f in results]) return results def probe( @@ -199,7 +200,9 @@ def probe( scored.append(fact) scored.sort(key=lambda x: x["score"], reverse=True) - return scored[:limit] + results = scored[:limit] + self.store.record_retrievals([f["fact_id"] for f in results]) + return results def related( self, @@ -269,7 +272,9 @@ def related( scored.append(fact) scored.sort(key=lambda x: x["score"], reverse=True) - return scored[:limit] + results = scored[:limit] + self.store.record_retrievals([f["fact_id"] for f in results]) + return results def reason( self, @@ -347,7 +352,9 @@ def reason( scored.append(fact) scored.sort(key=lambda x: x["score"], reverse=True) - return scored[:limit] + results = scored[:limit] + self.store.record_retrievals([f["fact_id"] for f in results]) + return results def contradict( self, @@ -490,7 +497,9 @@ def _score_facts_by_vector( scored.append(fact) scored.sort(key=lambda x: x["score"], reverse=True) - return scored[:limit] + results = scored[:limit] + self.store.record_retrievals([f["fact_id"] for f in results]) + return results def _fts_candidates( self, diff --git a/plugins/memory/holographic/store.py b/plugins/memory/holographic/store.py index fcb193d4da227..6597168435f10 100644 --- a/plugins/memory/holographic/store.py +++ b/plugins/memory/holographic/store.py @@ -277,16 +277,25 @@ def search_facts( rows = self._conn.execute(sql, params).fetchall() results = [self._row_to_dict(r) for r in rows] - if results: - ids = [r["fact_id"] for r in results] - placeholders = ",".join("?" * len(ids)) - self._conn.execute( - f"UPDATE facts SET retrieval_count = retrieval_count + 1 WHERE fact_id IN ({placeholders})", - ids, - ) - self._conn.commit() + self.record_retrievals([r["fact_id"] for r in results]) + return results - return results + def record_retrievals(self, fact_ids: list[int]) -> None: + """Increment retrieval_count for the given facts. + + Retrieval bookkeeping lives here so every read path can share it. + Callers pass the ids they actually returned to the caller, so the + counter reflects facts surfaced rather than rows scanned. + """ + if not fact_ids: + return + with self._lock: + placeholders = ",".join("?" * len(fact_ids)) + self._conn.execute( + f"UPDATE facts SET retrieval_count = retrieval_count + 1 WHERE fact_id IN ({placeholders})", + fact_ids, + ) + self._conn.commit() def update_fact( self, @@ -397,7 +406,10 @@ def list_facts( LIMIT ? """ rows = self._conn.execute(sql, params).fetchall() - return [self._row_to_dict(r) for r in rows] + facts = [self._row_to_dict(r) for r in rows] + # Outside the read lock: record_retrievals takes the same RLock and commits. + self.record_retrievals([f["fact_id"] for f in facts]) + return facts def record_feedback(self, fact_id: int, helpful: bool) -> dict: """Record user feedback and adjust trust asymmetrically. diff --git a/tests/plugins/memory/test_holographic_retrieval.py b/tests/plugins/memory/test_holographic_retrieval.py index ce8e3f7bae678..047f00715ef76 100644 --- a/tests/plugins/memory/test_holographic_retrieval.py +++ b/tests/plugins/memory/test_holographic_retrieval.py @@ -196,6 +196,14 @@ def test_search_results_bit_identical_to_unhoisted(hoisted_retriever): for fact in old_results: fact.pop("hrr_vector", None) + # ``retrieval_count`` is bookkeeping, not scoring: the reference rows are + # re-read from the DB *after* search() has already recorded its retrievals, + # so they carry the incremented value while the returned dicts hold the + # pre-increment snapshot. Compare everything else — the hoist parity this + # test exists to prove is about the scored output. + for fact in (*new_results, *old_results): + fact.pop("retrieval_count", None) + assert new_results == old_results @@ -238,3 +246,153 @@ def test_search_without_vectors_never_encodes(hoisted_retriever, monkeypatch): f"encode_text called {len(calls)}x with zero vector candidates — " "lazy hoist regressed to eager" ) +# --------------------------------------------------------------------------- +# retrieval_count bookkeeping — the retriever is the live read path, so it +# must record retrievals too. Previously only MemoryStore.search_facts (which +# has no callers) incremented, leaving the counter permanently zero. +# --------------------------------------------------------------------------- + +def _counts(store): + return dict( + store._conn.execute("SELECT fact_id, retrieval_count FROM facts").fetchall() + ) + + +def test_search_increments_retrieval_count(retriever_with_facts): + """search() bumps the counter for facts it actually returns.""" + store = retriever_with_facts.store + assert set(_counts(store).values()) == {0} + + results = retriever_with_facts.search("compaction") + assert len(results) >= 1 + + after = _counts(store) + for fact in results: + assert after[fact["fact_id"]] == 1 + # Facts that were not returned stay untouched + returned = {f["fact_id"] for f in results} + assert all(c == 0 for fid, c in after.items() if fid not in returned) + + +def test_probe_increments_retrieval_count(retriever_with_facts): + """probe() shares the same bookkeeping path as search().""" + store = retriever_with_facts.store + results = retriever_with_facts.probe("compaction") + + after = _counts(store) + for fact in results: + assert after[fact["fact_id"]] >= 1 + + +def test_zero_hit_search_records_nothing(retriever_with_facts): + """An empty result set must not touch any counter.""" + store = retriever_with_facts.store + before = _counts(store) + retriever_with_facts.search("zzzznomatchzzzz") + assert _counts(store) == before + + +def test_record_retrievals_is_idempotent_per_call(retriever_with_facts): + """Two searches produce two increments — the counter accumulates.""" + store = retriever_with_facts.store + first = retriever_with_facts.search("compaction") + fact_id = first[0]["fact_id"] + assert _counts(store)[fact_id] == 1 + retriever_with_facts.search("compaction") + assert _counts(store)[fact_id] == 2 + + +def test_record_retrievals_empty_list_is_noop(retriever_with_facts): + """Guard clause: empty id list must not issue a malformed UPDATE.""" + store = retriever_with_facts.store + before = _counts(store) + store.record_retrievals([]) + assert _counts(store) == before + + +def test_related_increments_retrieval_count(retriever_with_facts): + """related() returns facts to the caller, so it counts them too.""" + store = retriever_with_facts.store + results = retriever_with_facts.related("deployment") + assert len(results) >= 1 + + after = _counts(store) + for fact in results: + assert after[fact["fact_id"]] >= 1 + returned = {f["fact_id"] for f in results} + assert all(c == 0 for fid, c in after.items() if fid not in returned) + + +def test_reason_increments_retrieval_count(retriever_with_facts): + """reason() is a live fact_store read path — its results count.""" + store = retriever_with_facts.store + results = retriever_with_facts.reason(["deployment", "migration"]) + assert len(results) >= 1 + + after = _counts(store) + for fact in results: + assert after[fact["fact_id"]] >= 1 + + +def test_probe_category_bank_branch_increments(retriever_with_facts): + """The category-bank branch of probe() returns via _score_facts_by_vector. + + That helper has its own return path, which was previously uncounted even + though probe()'s direct-scoring branch was fixed. + """ + store = retriever_with_facts.store + results = retriever_with_facts.probe("compaction", category="tool") + assert len(results) >= 1 + + after = _counts(store) + for fact in results: + assert after[fact["fact_id"]] >= 1 + + +def test_score_facts_by_vector_increments_directly(retriever_with_facts): + """_score_facts_by_vector counts whatever it hands back.""" + import numpy as np + + from plugins.memory.holographic import holographic as hrr + + store = retriever_with_facts.store + target = hrr.encode_text("compaction", retriever_with_facts.hrr_dim) + results = retriever_with_facts._score_facts_by_vector(target, limit=2) + assert len(results) >= 1 + + after = _counts(store) + for fact in results: + assert after[fact["fact_id"]] >= 1 + assert isinstance(target, np.ndarray) + + +def test_list_facts_increments_retrieval_count(retriever_with_facts): + """fact_store(action='list') surfaces facts, so list_facts() counts them.""" + store = retriever_with_facts.store + assert set(_counts(store).values()) == {0} + + facts = store.list_facts() + assert len(facts) == 3 + + after = _counts(store) + for fact in facts: + assert after[fact["fact_id"]] == 1 + + +def test_list_facts_respects_limit_when_counting(retriever_with_facts): + """Only the truncated result set is counted, not every scanned row.""" + store = retriever_with_facts.store + facts = store.list_facts(limit=1) + assert len(facts) == 1 + + after = _counts(store) + assert after[facts[0]["fact_id"]] == 1 + assert sum(after.values()) == 1 + + +def test_list_facts_empty_result_records_nothing(retriever_with_facts): + """A filter that matches nothing must leave every counter alone.""" + store = retriever_with_facts.store + before = _counts(store) + assert store.list_facts(min_trust=99.0) == [] + assert _counts(store) == before