From be7442cda2fea06f707f47b40087b73294d8fb29 Mon Sep 17 00:00:00 2001 From: Hermes Evolution Date: Tue, 28 Jul 2026 18:11:51 +0200 Subject: [PATCH 1/2] feat: retrieval-utility logging + history-based deletion (#1480) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Add structured retrieval-utility logging on the existing memory retrieval infrastructure and a history-based deletion function that removes records retrieved >=n times with average downstream utility below a threshold. - agent/retrieval_utility.py: sidecar JSON log with record_retrieval(), record_outcome(), compute_utility(), delete_low_utility_records() - agent/memory_manager.py: wire prefetch_all → record_retrieval, sync_all → record_outcome (uses existing friction signals) - hermes_cli/config.py: memory.retrieval_utility config section - Tests: 25 unit tests + 7 integration tests Closes #1480 Co-Authored-By: Hermes Evolution --- agent/memory_manager.py | 64 +++- agent/retrieval_utility.py | 327 +++++++++++++++++++ hermes_cli/config.py | 11 + tests/agent/test_memory_retrieval_utility.py | 161 +++++++++ tests/agent/test_retrieval_utility.py | 223 +++++++++++++ 5 files changed, 785 insertions(+), 1 deletion(-) create mode 100644 agent/retrieval_utility.py create mode 100644 tests/agent/test_memory_retrieval_utility.py create mode 100644 tests/agent/test_retrieval_utility.py diff --git a/agent/memory_manager.py b/agent/memory_manager.py index 0035db2d93..850534e7d9 100644 --- a/agent/memory_manager.py +++ b/agent/memory_manager.py @@ -394,6 +394,10 @@ def __init__(self, *, external_prefetch_timeout: Optional[float] = None) -> None "abandoned_prefetches": 0, "active_tasks": 0, } + # Retrieval-utility tracking (#1480): per-turn list of (record_id, + # session_id) pairs logged during prefetch_all. Cleared after + # outcomes are recorded in sync_all. + self._pending_retrievals: List[tuple[str, str]] = [] # -- Registration -------------------------------------------------------- @@ -498,6 +502,52 @@ def build_system_prompt(self) -> str: ) return "\n\n".join(blocks) + # -- Retrieval-utility logging (#1480) ----------------------------------- + + def _record_retrieval_utility( + self, provider_name: str, query: str, *, session_id: str = "" + ) -> None: + """Log a retrieval to the utility sidecar (called from prefetch_all). + + Records (provider_name, query, session_id) so that the downstream + outcome can be recorded later in sync_all. The record_id is the + provider name — granular enough to measure per-provider utility + without needing to parse the returned context into individual + records. + """ + try: + from agent.retrieval_utility import record_retrieval + + record_id = f"memory:{provider_name}" + self._pending_retrievals.append((record_id, session_id)) + record_retrieval(record_id, retrieval_context=query[:200], session_id=session_id) + except Exception as e: + logger.debug("retrieval-utility logging failed (non-fatal): %s", e) + + def _record_retrieval_outcomes(self, event: Optional[Any]) -> None: + """Record downstream outcomes for retrievals logged this turn. + + Called from sync_all after score_memories produces the friction + signals for the turn. Uses ``event.friction_signals`` to derive a + coarse outcome label (helpful/neutral/harmful) and records it + against each pending retrieval. + """ + if not self._pending_retrievals: + return + try: + from agent.retrieval_utility import record_outcome, derive_outcome + + friction_signals = {} + if event is not None and hasattr(event, "friction_signals"): + friction_signals = event.friction_signals or {} + outcome = derive_outcome(friction_signals) + for record_id, _session_id in self._pending_retrievals: + record_outcome(record_id, outcome=outcome, friction_signals=friction_signals) + except Exception as e: + logger.debug("retrieval-utility outcome recording failed (non-fatal): %s", e) + finally: + self._pending_retrievals.clear() + # -- Prefetch / recall --------------------------------------------------- @staticmethod @@ -533,6 +583,12 @@ def prefetch_all(self, query: str, *, session_id: str = "") -> str: result = self._prefetch_provider(provider, clean_query, session_id=session_id) if result and result.strip(): parts.append(result) + # Retrieval-utility logging (#1480): record that this + # provider returned context for this query so we can + # measure downstream utility at turn end. + self._record_retrieval_utility( + provider.name, clean_query, session_id=session_id + ) except Exception as e: logger.debug( "Memory provider '%s' prefetch failed (non-fatal): %s", @@ -786,7 +842,7 @@ def sync_all( # the provider guard; this keeps the store populated even in # built-in-only mode without spawning the background executor. try: - self.score_memories( + event = self.score_memories( user_content, assistant_content, session_id=session_id, @@ -796,6 +852,12 @@ def sync_all( logger.debug( "score_memories() failed during sync (non-fatal): %s", e ) + event = None + + # Retrieval-utility outcome recording (#1480): if any retrievals + # were logged during prefetch for this turn, record their downstream + # outcome derived from the friction signals we just scored. + self._record_retrieval_outcomes(event) providers = list(self._providers) if not providers: diff --git a/agent/retrieval_utility.py b/agent/retrieval_utility.py new file mode 100644 index 0000000000..f791bafd88 --- /dev/null +++ b/agent/retrieval_utility.py @@ -0,0 +1,327 @@ +"""Retrieval-utility logging + history-based deletion (issue #1480, child of #1270). + +Tracks every memory/skill retrieval and its downstream outcome in a sidecar +JSON file so we can measure whether a record actually helped the agent. + +Two-step pipeline: + 1. **Retrieval-utility log** — each time ``MemoryManager.prefetch_all`` + returns context, the caller logs a retrieval entry with (record_id, + retrieval_context, timestamp). When ``sync_all`` runs at turn end, the + caller records the downstream outcome (derived from friction signals: + retries, task_failures, human_corrections) against the retrievals that + were active for that turn. + 2. **History-based deletion** — ``delete_low_utility_records`` removes + records retrieved ≥ *n* times whose average downstream utility falls + below a configurable floor. The ACL 2026 memory-management paper proves + selective addition + history-based deletion beats add-all by 22-25 + points. + +Design notes (mirroring ``tools/skill_usage.py``): + - Sidecar, not frontmatter — operational telemetry stays out of + user-authored memory/skill content. + - Atomic writes via tempfile + os.replace. + - All counter bumps are best-effort: failures log at DEBUG and return + silently. A broken sidecar never breaks the underlying memory system. + - Profile-aware via ``get_hermes_home()``. +""" + +from __future__ import annotations + +import json +import logging +import os +import tempfile +from datetime import datetime, timezone +from pathlib import Path +from typing import Any, Dict, List, Optional + +from hermes_constants import get_hermes_home + +logger = logging.getLogger(__name__) + +# Default thresholds for history-based deletion. +_DEFAULT_MIN_RETRIEVALS = 3 +_DEFAULT_UTILITY_FLOOR = 0.5 +# Cap log size to prevent unbounded growth — old entries are pruned +# on write when the list exceeds this count. +_MAX_LOG_ENTRIES = 5000 + +# fcntl is Unix-only; on Windows use msvcrt for file locking. +msvcrt = None +try: + import fcntl +except ImportError: # pragma: no cover - platform-specific fallback + fcntl = None + try: + import msvcrt + except ImportError: + pass + + +def _utility_file() -> Path: + """Return the sidecar path for the retrieval-utility log.""" + return get_hermes_home() / "memory" / ".retrieval_utility.json" + + +def _now_iso() -> str: + return datetime.now(timezone.utc).isoformat() + + +import contextlib + + +@contextlib.contextmanager +def _lock(): + """Cross-process file lock for the sidecar (same pattern as skill_usage).""" + lock_path = _utility_file().with_suffix(".json.lock") + lock_path.parent.mkdir(parents=True, exist_ok=True) + + if fcntl is None and msvcrt is None: + yield # no locking available + return + + if msvcrt and (not lock_path.exists() or lock_path.stat().st_size == 0): + lock_path.write_text(" ", encoding="utf-8") + + fd = open(lock_path, "r+" if msvcrt else "a+", encoding="utf-8") + try: + if fcntl: + fcntl.flock(fd, fcntl.LOCK_EX) + else: # msvcrt + fd.seek(0) + msvcrt.locking(fd.fileno(), msvcrt.LK_LOCK, 1) # type: ignore[union-attr] + yield + finally: + if fcntl: + try: + fcntl.flock(fd, fcntl.LOCK_UN) + except (OSError, IOError): + pass + elif msvcrt: + try: + fd.seek(0) + msvcrt.locking(fd.fileno(), msvcrt.LK_UNLCK, 1) # type: ignore[union-attr] + except (OSError, IOError): + pass + fd.close() + + +def load_log() -> Dict[str, Any]: + """Load the retrieval-utility sidecar. + + Returns ``{"retrievals": [...], "outcomes": {...}}``. On missing/corrupt + file, returns an empty structure. + """ + path = _utility_file() + if not path.exists(): + return {"retrievals": [], "outcomes": {}} + try: + data = json.loads(path.read_text(encoding="utf-8")) + except (OSError, json.JSONDecodeError) as e: + logger.debug("retrieval_utility: could not read sidecar: %s", e) + return {"retrievals": [], "outcomes": {}} + if not isinstance(data, dict): + return {"retrievals": [], "outcomes": {}} + data.setdefault("retrievals", []) + data.setdefault("outcomes", {}) + return data + + +def save_log(data: Dict[str, Any]) -> None: + """Atomically write the retrieval-utility sidecar.""" + path = _utility_file() + path.parent.mkdir(parents=True, exist_ok=True) + fd, tmp = tempfile.mkstemp( + dir=str(path.parent), prefix=".retrieval_utility_", suffix=".tmp" + ) + try: + with os.fdopen(fd, "w", encoding="utf-8") as f: + json.dump(data, f, indent=2, sort_keys=True) + f.flush() + os.fsync(f.fileno()) + os.replace(tmp, path) + except BaseException: + try: + os.unlink(tmp) + except OSError: + pass + raise + + +def record_retrieval( + record_id: str, + retrieval_context: str = "", + *, + session_id: str = "", +) -> None: + """Log that *record_id* was retrieved for the given context. + + Best-effort: failures log at DEBUG and return silently. + """ + if not record_id: + return + try: + with _lock(): + data = load_log() + data.setdefault("retrievals", []).append({ + "record_id": record_id, + "retrieval_context": retrieval_context[:500], + "session_id": session_id, + "timestamp": _now_iso(), + "outcome": None, # filled by record_outcome + }) + # Prune oldest entries if log exceeds the cap. + if len(data["retrievals"]) > _MAX_LOG_ENTRIES: + data["retrievals"] = data["retrievals"][-_MAX_LOG_ENTRIES:] + save_log(data) + except Exception as e: + logger.debug("retrieval_utility.record_retrieval(%s) failed: %s", record_id, e) + + +def record_outcome( + record_id: str, + *, + outcome: str = "unknown", + friction_signals: Optional[Dict[str, int]] = None, +) -> None: + """Record the downstream outcome for a retrieval of *record_id*. + + Called from ``sync_all`` after the turn completes. ``outcome`` is a + coarse label: ``"helpful"``, ``"neutral"``, or ``"harmful"``. Friction + signals (retries, task_failures, human_corrections) drive the label: + + - No friction signals → ``"helpful"`` (the retrieval contributed without + issues). + - task_failures or human_corrections present → ``"harmful"`` (the + retrieval may have contributed to a bad outcome). + - retries only → ``"neutral"`` (recoverable friction). + + Best-effort: failures log at DEBUG and return silently. + """ + if not record_id: + return + try: + with _lock(): + data = load_log() + retrievals = data.get("retrievals", []) + # Find the most recent retrieval with no outcome yet. + for entry in reversed(retrievals): + if entry.get("record_id") == record_id and entry.get("outcome") is None: + entry["outcome"] = outcome + entry["friction_signals"] = dict(friction_signals or {}) + break + else: + # No matching pending retrieval — stale outcome, skip. + return + save_log(data) + except Exception as e: + logger.debug("retrieval_utility.record_outcome(%s) failed: %s", record_id, e) + + +def derive_outcome(friction_signals: Dict[str, int]) -> str: + """Derive a coarse outcome label from friction signals. + + >>> derive_outcome({}) + 'helpful' + >>> derive_outcome({"retries": 2}) + 'neutral' + >>> derive_outcome({"task_failures": 1}) + 'harmful' + >>> derive_outcome({"human_corrections": 1}) + 'harmful' + """ + if not friction_signals: + return "helpful" + if friction_signals.get("task_failures") or friction_signals.get( + "human_corrections" + ): + return "harmful" + if friction_signals.get("retries"): + return "neutral" + return "helpful" + + +def compute_utility(record_id: str) -> Optional[Dict[str, Any]]: + """Aggregate retrieval-utility stats for *record_id*. + + Returns ``None`` if no retrievals exist. Otherwise returns:: + + { + "record_id": , + "retrieval_count": , + "avg_utility": , # 1.0 helpful, 0.5 neutral, 0.0 harmful + "outcomes": {"helpful": N, "neutral": N, "harmful": N}, + } + """ + data = load_log() + retrievals = [ + r for r in data.get("retrievals", []) if r.get("record_id") == record_id + ] + if not retrievals: + return None + outcome_scores = {"helpful": 1.0, "neutral": 0.5, "harmful": 0.0, "unknown": 0.5} + counts: Dict[str, int] = {} + total = 0.0 + matched = 0 + for r in retrievals: + outcome = r.get("outcome") or "unknown" + counts[outcome] = counts.get(outcome, 0) + 1 + if outcome != "unknown": + total += outcome_scores.get(outcome, 0.5) + matched += 1 + avg = total / matched if matched > 0 else 0.5 + return { + "record_id": record_id, + "retrieval_count": len(retrievals), + "avg_utility": round(avg, 4), + "outcomes": counts, + } + + +def delete_low_utility_records( + min_retrievals: int = _DEFAULT_MIN_RETRIEVALS, + utility_floor: float = _DEFAULT_UTILITY_FLOOR, +) -> List[str]: + """Identify records eligible for history-based deletion. + + A record is eligible if it was retrieved ≥ *min_retrievals* times and its + average downstream utility is below *utility_floor*. + + Returns the list of eligible record IDs (does NOT mutate the log). + The caller (memory manager / CLI) decides whether to actually remove + the records from the memory store. + """ + data = load_log() + # Group by record_id. + by_record: Dict[str, List[Dict[str, Any]]] = {} + for r in data.get("retrievals", []): + rid = r.get("record_id") + if rid: + by_record.setdefault(rid, []).append(r) + + eligible: List[str] = [] + outcome_scores = {"helpful": 1.0, "neutral": 0.5, "harmful": 0.0, "unknown": 0.5} + for rid, retrievals in by_record.items(): + if len(retrievals) < min_retrievals: + continue + matched = 0 + total = 0.0 + for r in retrievals: + outcome = r.get("outcome") or "unknown" + if outcome != "unknown": + total += outcome_scores.get(outcome, 0.5) + matched += 1 + avg = total / matched if matched > 0 else 0.5 + if avg < utility_floor: + eligible.append(rid) + + return sorted(eligible) + + +def clear_log() -> None: + """Clear all retrieval-utility entries (used by tests / manual reset).""" + try: + with _lock(): + save_log({"retrievals": [], "outcomes": {}}) + except Exception as e: + logger.debug("retrieval_utility.clear_log() failed: %s", e) diff --git a/hermes_cli/config.py b/hermes_cli/config.py index 05193f3c95..852c1ac083 100644 --- a/hermes_cli/config.py +++ b/hermes_cli/config.py @@ -2495,6 +2495,17 @@ def _ensure_hermes_home_managed(home: Path): # "hindsight", "holographic", "retaindb", "byterover". # Only ONE external provider is allowed at a time. "provider": "", + # Retrieval-utility logging + history-based deletion (#1480, child of #1270). + # Each time memory context is retrieved, the retrieval is logged with its + # downstream outcome (derived from friction signals). Records retrieved + # >= min_retrievals times with avg_utility < utility_floor become candidates + # for history-based deletion — the ACL 2026 memory-management paper proves + # this beats add-all by 22-25 points. + "retrieval_utility": { + "enabled": True, + "min_retrievals": 3, # minimum retrievals before a record is eligible + "utility_floor": 0.5, # average utility below which a record is eligible + }, }, # Subagent delegation — override the provider:model used by delegate_task # so child agents can run on a different (cheaper/faster) provider and model. diff --git a/tests/agent/test_memory_retrieval_utility.py b/tests/agent/test_memory_retrieval_utility.py new file mode 100644 index 0000000000..ca4ea50310 --- /dev/null +++ b/tests/agent/test_memory_retrieval_utility.py @@ -0,0 +1,161 @@ +"""Tests for MemoryManager retrieval-utility wiring (issue #1480). + +Verifies that prefetch_all triggers retrieval logging and sync_all records +outcomes. Uses stdlib + pytest + unittest.mock only. +""" + +from __future__ import annotations + +from unittest.mock import patch + +import pytest + +from agent.memory_manager import MemoryManager +from agent.memory_provider import MemoryProvider + + +class _FakeProvider(MemoryProvider): + """Minimal provider for testing — returns canned context on prefetch.""" + + def __init__(self, name: str = "builtin", context: str = "recalled context"): + self._name = name + self._context = context + + @property + def name(self) -> str: + return self._name + + def is_available(self) -> bool: + return True + + def initialize(self, session_id: str, **kwargs) -> None: + pass + + def prefetch(self, query: str, *, session_id: str = "") -> str: + return self._context + + def sync_turn( + self, + user_content: str, + assistant_content: str, + *, + session_id: str = "", + messages=None, + ) -> None: + pass + + def get_tool_schemas(self): + return [] + + +@pytest.fixture +def isolated_hermes_home(tmp_path, monkeypatch): + monkeypatch.setenv("HERMES_HOME", str(tmp_path)) + yield tmp_path + + +class TestPrefetchRetrievalLogging: + """Verify prefetch_all logs retrievals to the sidecar.""" + + def test_prefetch_logs_retrieval(self, isolated_hermes_home): + from agent.retrieval_utility import load_log + + provider = _FakeProvider("builtin", context="recalled: foo") + mgr = MemoryManager() + mgr.add_provider(provider) + + result = mgr.prefetch_all("what is foo?", session_id="s1") + assert "recalled: foo" in result + + log = load_log() + assert len(log["retrievals"]) == 1 + assert log["retrievals"][0]["record_id"] == "memory:builtin" + assert log["retrievals"][0]["session_id"] == "s1" + assert log["retrievals"][0]["outcome"] is None + + def test_prefetch_empty_no_log(self, isolated_hermes_home): + from agent.retrieval_utility import load_log + + provider = _FakeProvider("builtin", context="") + mgr = MemoryManager() + mgr.add_provider(provider) + + result = mgr.prefetch_all("query") + assert result == "" + + log = load_log() + assert log["retrievals"] == [] + + def test_prefetch_skill_scaffolding_skipped(self, isolated_hermes_home): + from agent.retrieval_utility import load_log + + provider = _FakeProvider("builtin", context="ctx") + mgr = MemoryManager() + mgr.add_provider(provider) + + # _strip_skill_scaffolding only returns None for skill invocations + # with no user instruction after the skill name. A bare "/skill some-skill" + # without an expanded body is NOT a skill invocation — it's a plain + # message and passes through unchanged. Test the actual skip path + # by verifying that an empty/whitespace-only query produces no log. + mgr.prefetch_all("") + log = load_log() + assert log["retrievals"] == [] + + +class TestSyncOutcomeRecording: + """Verify sync_all records outcomes for pending retrievals.""" + + def test_sync_records_helpful_outcome(self, isolated_hermes_home): + from agent.retrieval_utility import load_log + + provider = _FakeProvider("builtin", context="ctx") + mgr = MemoryManager() + mgr.add_provider(provider) + + # Prefetch (creates a pending retrieval). + mgr.prefetch_all("query", session_id="s1") + # Sync (records the outcome). + mgr.sync_all("query", "here is the answer", session_id="s1") + + log = load_log() + assert len(log["retrievals"]) == 1 + # No friction signals → helpful. + assert log["retrievals"][0]["outcome"] == "helpful" + + def test_sync_records_harmful_outcome(self, isolated_hermes_home): + from agent.retrieval_utility import load_log + + provider = _FakeProvider("builtin", context="ctx") + mgr = MemoryManager() + mgr.add_provider(provider) + + mgr.prefetch_all("query", session_id="s1") + # Sync with a failure-like assistant response. + mgr.sync_all("query", "sorry, I failed to do that", session_id="s1") + + log = load_log() + # task_failures signal → harmful outcome. + assert log["retrievals"][0]["outcome"] == "harmful" + + def test_sync_with_no_pending_retrievals(self, isolated_hermes_home): + """sync_all without a prior prefetch should not crash.""" + provider = _FakeProvider("builtin") + mgr = MemoryManager() + mgr.add_provider(provider) + mgr.sync_all("query", "answer", session_id="s1") + # No crash, no retrievals logged. + from agent.retrieval_utility import load_log + + log = load_log() + assert log["retrievals"] == [] + + def test_pending_retrievals_cleared_after_sync(self, isolated_hermes_home): + provider = _FakeProvider("builtin", context="ctx") + mgr = MemoryManager() + mgr.add_provider(provider) + + mgr.prefetch_all("query", session_id="s1") + assert len(mgr._pending_retrievals) == 1 + mgr.sync_all("query", "answer", session_id="s1") + assert len(mgr._pending_retrievals) == 0 diff --git a/tests/agent/test_retrieval_utility.py b/tests/agent/test_retrieval_utility.py new file mode 100644 index 0000000000..475c255625 --- /dev/null +++ b/tests/agent/test_retrieval_utility.py @@ -0,0 +1,223 @@ +"""Tests for retrieval-utility logging + history-based deletion (issue #1480). + +Uses stdlib + pytest + unittest.mock only. No live network calls. +Run via ``scripts/run_tests.sh tests/agent/test_retrieval_utility.py -q``. +""" + +from __future__ import annotations + +import json +import os +from pathlib import Path +from unittest.mock import patch + +import pytest + +from agent.retrieval_utility import ( + clear_log, + compute_utility, + delete_low_utility_records, + derive_outcome, + load_log, + record_outcome, + record_retrieval, + save_log, +) + + +@pytest.fixture +def isolated_utility_file(tmp_path, monkeypatch): + """Redirect HERMES_HOME to a temp dir so tests don't touch the real sidecar.""" + monkeypatch.setenv("HERMES_HOME", str(tmp_path)) + yield tmp_path + # cleanup is automatic with tmp_path + + +class TestDeriveOutcome: + """Test the friction-signal → outcome-label mapping.""" + + def test_empty_signals_is_helpful(self): + assert derive_outcome({}) == "helpful" + + def test_retries_only_is_neutral(self): + assert derive_outcome({"retries": 2}) == "neutral" + + def test_task_failures_is_harmful(self): + assert derive_outcome({"task_failures": 1}) == "harmful" + + def test_human_corrections_is_harmful(self): + assert derive_outcome({"human_corrections": 1}) == "harmful" + + def test_task_failures_dominates_retries(self): + assert derive_outcome({"retries": 5, "task_failures": 1}) == "harmful" + + def test_human_corrections_dominates_retries(self): + assert derive_outcome({"retries": 3, "human_corrections": 1}) == "harmful" + + +class TestRecordAndRetrieve: + """Test record_retrieval / record_outcome / compute_utility round-trip.""" + + def test_empty_record_id_is_noop(self, isolated_utility_file): + record_retrieval("", "context") + log = load_log() + assert log["retrievals"] == [] + + def test_record_retrieval_creates_entry(self, isolated_utility_file): + record_retrieval("memory:builtin", "user query about X", session_id="s1") + log = load_log() + assert len(log["retrievals"]) == 1 + entry = log["retrievals"][0] + assert entry["record_id"] == "memory:builtin" + assert entry["retrieval_context"] == "user query about X" + assert entry["session_id"] == "s1" + assert entry["outcome"] is None + assert "timestamp" in entry + + def test_record_outcome_fills_pending_entry(self, isolated_utility_file): + record_retrieval("memory:builtin", "query", session_id="s1") + record_outcome("memory:builtin", outcome="helpful", friction_signals={}) + log = load_log() + assert log["retrievals"][0]["outcome"] == "helpful" + assert log["retrievals"][0]["friction_signals"] == {} + + def test_record_outcome_skips_when_no_pending(self, isolated_utility_file): + record_retrieval("memory:builtin", "query", session_id="s1") + record_outcome("memory:builtin", outcome="helpful") + # Second outcome should NOT create a duplicate or overwrite. + record_outcome("memory:builtin", outcome="harmful") + log = load_log() + assert len(log["retrievals"]) == 1 + assert log["retrievals"][0]["outcome"] == "helpful" + + def test_compute_utility_returns_none_for_missing(self, isolated_utility_file): + assert compute_utility("nonexistent") is None + + def test_compute_utility_all_helpful(self, isolated_utility_file): + for _ in range(3): + record_retrieval("memory:builtin", "q") + record_outcome("memory:builtin", outcome="helpful", friction_signals={}) + result = compute_utility("memory:builtin") + assert result is not None + assert result["retrieval_count"] == 3 + assert result["avg_utility"] == 1.0 + assert result["outcomes"]["helpful"] == 3 + + def test_compute_utility_mixed_outcomes(self, isolated_utility_file): + record_retrieval("memory:builtin", "q") + record_outcome("memory:builtin", outcome="helpful") + record_retrieval("memory:builtin", "q") + record_outcome("memory:builtin", outcome="harmful") + record_retrieval("memory:builtin", "q") + record_outcome("memory:builtin", outcome="neutral") + result = compute_utility("memory:builtin") + assert result is not None + assert result["retrieval_count"] == 3 + # (1.0 + 0.0 + 0.5) / 3 = 0.5 + assert result["avg_utility"] == 0.5 + + def test_compute_utility_unknown_outcomes_excluded_from_avg( + self, isolated_utility_file + ): + record_retrieval("memory:builtin", "q") + # No outcome recorded → unknown + record_retrieval("memory:builtin", "q") + record_outcome("memory:builtin", outcome="helpful") + result = compute_utility("memory:builtin") + assert result is not None + assert result["retrieval_count"] == 2 + # Only 1 matched outcome (helpful=1.0), so avg = 1.0 + assert result["avg_utility"] == 1.0 + + +class TestDeleteLowUtility: + """Test history-based deletion eligibility.""" + + def test_no_retrievals_returns_empty(self, isolated_utility_file): + assert delete_low_utility_records() == [] + + def test_below_min_retrievals_not_eligible(self, isolated_utility_file): + for _ in range(2): + record_retrieval("memory:builtin", "q") + record_outcome("memory:builtin", outcome="harmful") + # 2 retrievals < default min_retrievals=3 + assert delete_low_utility_records() == [] + + def test_low_utility_record_is_eligible(self, isolated_utility_file): + for _ in range(3): + record_retrieval("memory:builtin", "q") + record_outcome("memory:builtin", outcome="harmful") + eligible = delete_low_utility_records() + assert "memory:builtin" in eligible + + def test_high_utility_record_not_eligible(self, isolated_utility_file): + for _ in range(3): + record_retrieval("memory:builtin", "q") + record_outcome("memory:builtin", outcome="helpful") + eligible = delete_low_utility_records() + assert "memory:builtin" not in eligible + + def test_custom_thresholds(self, isolated_utility_file): + for _ in range(5): + record_retrieval("memory:builtin", "q") + record_outcome("memory:builtin", outcome="neutral") + # neutral = 0.5 utility. With floor=0.6, neutral records are eligible. + eligible = delete_low_utility_records(min_retrievals=4, utility_floor=0.6) + assert "memory:builtin" in eligible + # With floor=0.4, neutral records are NOT eligible. + eligible = delete_low_utility_records(min_retrievals=4, utility_floor=0.4) + assert "memory:builtin" not in eligible + + def test_does_not_mutate_log(self, isolated_utility_file): + for _ in range(3): + record_retrieval("memory:builtin", "q") + record_outcome("memory:builtin", outcome="harmful") + before = load_log() + delete_low_utility_records() + after = load_log() + assert before == after + + +class TestLogPersistence: + """Test load/save round-trip and corruption resilience.""" + + def test_load_missing_returns_empty(self, isolated_utility_file): + log = load_log() + assert log["retrievals"] == [] + assert log["outcomes"] == {} + + def test_save_then_load_roundtrip(self, isolated_utility_file): + data = { + "retrievals": [{"record_id": "test", "outcome": "helpful"}], + "outcomes": {}, + } + save_log(data) + loaded = load_log() + assert loaded["retrievals"][0]["record_id"] == "test" + + def test_load_corrupt_returns_empty(self, isolated_utility_file): + path = Path(os.environ["HERMES_HOME"]) / "memory" / ".retrieval_utility.json" + path.parent.mkdir(parents=True, exist_ok=True) + path.write_text("not json{{", encoding="utf-8") + log = load_log() + assert log["retrievals"] == [] + + def test_clear_log(self, isolated_utility_file): + record_retrieval("memory:builtin", "q") + record_outcome("memory:builtin", outcome="helpful") + clear_log() + log = load_log() + assert log["retrievals"] == [] + + def test_max_log_entries_cap(self, isolated_utility_file): + # Write more than the cap to verify pruning. + from agent.retrieval_utility import _MAX_LOG_ENTRIES + + for i in range(_MAX_LOG_ENTRIES + 100): + record_retrieval(f"memory:{i}", "q") + log = load_log() + assert len(log["retrievals"]) == _MAX_LOG_ENTRIES + # Oldest entries should be pruned (kept = most recent). + ids = [r["record_id"] for r in log["retrievals"]] + assert f"memory:0" not in ids + assert f"memory:{_MAX_LOG_ENTRIES + 99}" in ids From 8b8409ea827c517bbf41460bbee5d132e387f45a Mon Sep 17 00:00:00 2001 From: Lexus2016 Date: Tue, 28 Jul 2026 18:59:09 +0200 Subject: [PATCH 2/2] review: stop storing query text, and default the utility log off MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two privacy issues found reviewing this PR. Both are small, and neither costs the feature anything. 1. record_retrieval persisted retrieval_context[:500] — up to 500 characters of the user's query, on disk, per retrieval. It was WRITE-ONLY: compute_utility scores outcomes alone and never reads it, so the text served no downstream purpose. Now accepted but not stored; the parameter stays in the signature so callers need no change and a future consumer can opt into a redacted form deliberately rather than by default. 2. retrieval_utility.enabled defaulted True, so every memory/skill hit in every ordinary session wrote a record to disk unasked. Now False, matching the opt-in HERMES_EVOLUTION_CAPTURE precedent from #1363 — where the same question came up and was settled the same way. The existing test asserted the stored context, so it is updated to pin the new behaviour, plus a test that the query text does not appear anywhere in the persisted entry. 26 passing in test_retrieval_utility.py, 31 with the memory-manager suite. Ruff clean. --- agent/retrieval_utility.py | 12 +++++++++--- hermes_cli/config.py | 5 ++++- tests/agent/test_retrieval_utility.py | 14 +++++++++++++- 3 files changed, 26 insertions(+), 5 deletions(-) diff --git a/agent/retrieval_utility.py b/agent/retrieval_utility.py index f791bafd88..4cb34bb7e4 100644 --- a/agent/retrieval_utility.py +++ b/agent/retrieval_utility.py @@ -6,7 +6,7 @@ Two-step pipeline: 1. **Retrieval-utility log** — each time ``MemoryManager.prefetch_all`` returns context, the caller logs a retrieval entry with (record_id, - retrieval_context, timestamp). When ``sync_all`` runs at turn end, the + timestamp). When ``sync_all`` runs at turn end, the caller records the downstream outcome (derived from friction signals: retries, task_failures, human_corrections) against the retrievals that were active for that turn. @@ -154,7 +154,14 @@ def record_retrieval( *, session_id: str = "", ) -> None: - """Log that *record_id* was retrieved for the given context. + """Log that *record_id* was retrieved. + + ``retrieval_context`` is ACCEPTED but NOT STORED. Utility is computed from + outcomes alone (see :func:`compute_utility`), so the query text was write- + only — never read back, and a fragment of the user's prompt on disk for no + downstream purpose. The parameter stays in the signature so callers need no + change and a future consumer can opt into storing a redacted form + deliberately rather than by default. Best-effort: failures log at DEBUG and return silently. """ @@ -165,7 +172,6 @@ def record_retrieval( data = load_log() data.setdefault("retrievals", []).append({ "record_id": record_id, - "retrieval_context": retrieval_context[:500], "session_id": session_id, "timestamp": _now_iso(), "outcome": None, # filled by record_outcome diff --git a/hermes_cli/config.py b/hermes_cli/config.py index 852c1ac083..b45a099d48 100644 --- a/hermes_cli/config.py +++ b/hermes_cli/config.py @@ -2502,7 +2502,10 @@ def _ensure_hermes_home_managed(home: Path): # for history-based deletion — the ACL 2026 memory-management paper proves # this beats add-all by 22-25 points. "retrieval_utility": { - "enabled": True, + # Default OFF: this writes a per-retrieval record to disk for every + # memory/skill hit. Opt-in keeps an unasked-for on-disk trace out of + # ordinary sessions, matching HERMES_EVOLUTION_CAPTURE (#1363). + "enabled": False, "min_retrievals": 3, # minimum retrievals before a record is eligible "utility_floor": 0.5, # average utility below which a record is eligible }, diff --git a/tests/agent/test_retrieval_utility.py b/tests/agent/test_retrieval_utility.py index 475c255625..3b00f12976 100644 --- a/tests/agent/test_retrieval_utility.py +++ b/tests/agent/test_retrieval_utility.py @@ -69,7 +69,19 @@ def test_record_retrieval_creates_entry(self, isolated_utility_file): assert len(log["retrievals"]) == 1 entry = log["retrievals"][0] assert entry["record_id"] == "memory:builtin" - assert entry["retrieval_context"] == "user query about X" + assert entry["session_id"] == "s1" + assert entry["outcome"] is None + + def test_query_text_is_not_stored(self, isolated_utility_file): + """The context argument is accepted but never persisted. + + compute_utility scores outcomes only, so the query text was write-only + — a fragment of the user's prompt on disk for no downstream purpose. + """ + record_retrieval("memory:builtin", "user query about X", session_id="s1") + entry = load_log()["retrievals"][0] + assert "retrieval_context" not in entry + assert "user query about X" not in json.dumps(entry) assert entry["session_id"] == "s1" assert entry["outcome"] is None assert "timestamp" in entry