Skip to content

fix: bump retrieval_count in list_facts() - #35152

Open
angrywudu wants to merge 1 commit into
NousResearch:mainfrom
angrywudu:fix-list-retrieval-count
Open

fix: bump retrieval_count in list_facts()#35152
angrywudu wants to merge 1 commit into
NousResearch:mainfrom
angrywudu:fix-list-retrieval-count

Conversation

@angrywudu

Copy link
Copy Markdown

Bug

HolographicStore.list_facts() did not increment retrieval_count on returned facts. All other retrieval methods (search, probe, related, reason, contradict) correctly call _bump_retrieval_count(), but list — which is the primary way agents browse the fact store — was silently skipping it.

This meant facts frequently accessed via list never had their usage tracked, so the trust score system's frequency data was incomplete.

Fix

Added the same UPDATE facts SET retrieval_count = retrieval_count + 1 pattern used in search_facts() to list_facts(), applied after the SELECT and before returning results.

Testing

  • Verified fix produces incrementing retrieval_count values across consecutive list calls
  • Confirmed SQL UPDATE fires correctly against live database
  • Gateway restart required to load patched module (old process had cached bytecode)
  • All existing search/probe/related/reason/contradict bump behavior unchanged

The list_facts() method was missing the retrieval_count increment
that search_facts() and all HRR-based retrieval methods (probe,
related, reason, contradict) already had. This meant any fact
retrieved via the 'list' action would never have its usage counter
tracked, breaking the trust score system's ability to surface
frequently-used facts.
@alt-glitch alt-glitch added type/bug Something isn't working P3 Low — cosmetic, nice to have comp/plugins Plugin system and bundled plugins tool/memory Memory tool and memory providers labels May 30, 2026
@alt-glitch

Copy link
Copy Markdown
Collaborator

Duplicate of #24527 and overlaps with omnibus #23221 — all fix retrieval_count not being incremented in holographic memory retrieval paths. See also #17899 (tracking issue).

@teknium1

Copy link
Copy Markdown
Contributor

Thanks for fixing the verified list_facts() omission. Current main returns selected list rows directly at plugins/memory/holographic/store.py:399-400, and the proposed batched update correctly mirrors the existing search_facts() update at plugins/memory/holographic/store.py:280-287.

Problems

  • The PR is incomplete for the retrieval-count contract described in its body. A repository-wide search finds the only current increment at plugins/memory/holographic/store.py:284; FactRetriever.search, probe, related, reason, and _score_facts_by_vector return results without incrementing at plugins/memory/holographic/retrieval.py:112, :190, :258, :336, and :479.
  • No regression test is included. Existing list tests in tests/plugins/memory/test_holographic_store.py invoke list_facts() but do not assert retrieval_count.

Suggested changes

  • Centralize ID-based count bumps on MemoryStore, apply it consistently to every retrieval result path, and specify the behavior for both facts in a contradiction pair.
  • Add persistence-focused tests for list retrieval, empty results, and each retrieval strategy.

Automated hermes-sweeper review.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

comp/plugins Plugin system and bundled plugins P3 Low — cosmetic, nice to have sweeper:blast-contained Sweeper blast radius: contained — one narrow path / opt-in / few users tool/memory Memory tool and memory providers type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants