From 48f0d41b341e807119eb84b485e325063a9218ab Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Talia=20Dan=C3=ADelsd=C3=B3ttir?= Date: Tue, 28 Jul 2026 20:56:37 +0000 Subject: [PATCH 1/3] fix(memory/holographic): record retrieval_count on the live read path MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit FactRetriever.search() and .probe() are the only read paths the agent actually calls, but neither incremented retrieval_count. The single function that did — MemoryStore.search_facts() — has no callers, so the counter sat at 0 for every fact on every install. Move the bookkeeping into MemoryStore.record_retrievals() so all read paths share one implementation, and call it from search(), probe() and search_facts(). Counts reflect facts actually returned to the caller, not rows scanned. Empty result sets are guarded so no malformed UPDATE is issued. retrieval_count is metadata and is not used for ranking, so this changes no search results — it only makes the usage counter real for anything reading it (analytics, skills, plugins). Closes #17899 --- plugins/memory/holographic/retrieval.py | 5 +- plugins/memory/holographic/store.py | 27 +++++--- .../memory/test_holographic_retrieval.py | 62 +++++++++++++++++++ 3 files changed, 84 insertions(+), 10 deletions(-) diff --git a/plugins/memory/holographic/retrieval.py b/plugins/memory/holographic/retrieval.py index 2bbcb71f473e..dd5c35bacc7a 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, diff --git a/plugins/memory/holographic/store.py b/plugins/memory/holographic/store.py index fcb193d4da22..ebfa16969115 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, diff --git a/tests/plugins/memory/test_holographic_retrieval.py b/tests/plugins/memory/test_holographic_retrieval.py index ce8e3f7bae67..60aebadd9cf0 100644 --- a/tests/plugins/memory/test_holographic_retrieval.py +++ b/tests/plugins/memory/test_holographic_retrieval.py @@ -238,3 +238,65 @@ 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 From 77b190dd477ebdf656e8c9c7fa446e7aa19fc672 Mon Sep 17 00:00:00 2001 From: Talia Danielsdottir Date: Fri, 31 Jul 2026 09:40:21 +0000 Subject: [PATCH 2/3] fix(memory/holographic): count retrievals on related, reason, probe bank, and list Review feedback on #73644: the shared counter helper was only applied to search() and the direct-scoring branch of probe(). The remaining live fact_store read surfaces still returned uncounted results: - related() retrieval.py - reason() retrieval.py - _score_facts_by_vector() retrieval.py (category-bank probe branch) - list_facts() store.py (fact_store action=list) Each now calls store.record_retrievals() after limit truncation, so the counter reflects facts actually surfaced rather than rows scanned. list_facts() records outside its read lock; record_retrievals() reacquires the same RLock and commits, so the write stays out of the read critical section. Adds regression coverage for all four paths, including limit-truncation and empty-result cases. All six new tests fail against the pre-fix source. --- plugins/memory/holographic/retrieval.py | 12 ++- plugins/memory/holographic/store.py | 5 +- .../memory/test_holographic_retrieval.py | 88 +++++++++++++++++++ 3 files changed, 101 insertions(+), 4 deletions(-) diff --git a/plugins/memory/holographic/retrieval.py b/plugins/memory/holographic/retrieval.py index dd5c35bacc7a..d0e7990846a2 100644 --- a/plugins/memory/holographic/retrieval.py +++ b/plugins/memory/holographic/retrieval.py @@ -272,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, @@ -350,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, @@ -493,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 ebfa16969115..6597168435f1 100644 --- a/plugins/memory/holographic/store.py +++ b/plugins/memory/holographic/store.py @@ -406,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 60aebadd9cf0..000269615ae7 100644 --- a/tests/plugins/memory/test_holographic_retrieval.py +++ b/tests/plugins/memory/test_holographic_retrieval.py @@ -300,3 +300,91 @@ def test_record_retrievals_empty_list_is_noop(retriever_with_facts): 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 From f4201013a0f50852ade02547ae10792f698c8a99 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Talia=20Dan=C3=ADelsd=C3=B3ttir?= Date: Thu, 6 Aug 2026 02:57:19 +0000 Subject: [PATCH 3/3] test(memory/holographic): ignore retrieval_count in the hoist parity check MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit test_search_results_bit_identical_to_unhoisted landed on main after this branch was opened. It rebuilds a reference result set by re-reading rows through _fts_candidates *after* calling search(), so with retrieval counting now on the live read path the reference carries the incremented value while the returned dicts hold the pre-increment snapshot. The scored output is identical — only bookkeeping differs — so drop that one field before comparing. The hoist parity the test exists to prove is unaffected. --- tests/plugins/memory/test_holographic_retrieval.py | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/tests/plugins/memory/test_holographic_retrieval.py b/tests/plugins/memory/test_holographic_retrieval.py index 000269615ae7..047f00715ef7 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