From ffcbafa0c596d8ec50312b4e4bce8371ef6d4815 Mon Sep 17 00:00:00 2001 From: gethin Date: Tue, 21 Jul 2026 23:16:28 +0100 Subject: [PATCH 1/2] fix(memory): close the rating loop and stop diluting what gets served MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three defects kept memories from being marked useful. Measured with live headless A/B sessions against sandboxed copies of a real memory DB. 1. Nothing was ever rated. The Stop hook was registered async, but its RATE_MEMORIES sweep replies with a `decision: "block"` payload — a control-flow response Claude Code only honours from a blocking hook. Registered async it is silently dropped, so the rating turn never runs and the ranking/retention feedback loop never closes. Identical task, identical DB: async => 44 exposed / 0 rated / 0 useful; sync => 44 / 44 / 9 (20.45%). On the live DB, 30 of 39 sessions in the measured month had zero ratings. Ruled out by probe: pythonw.exe does NOT null stdout through a pipe. See docs/decisions/stop-hook-must-be-sync.md. 2. Retrieval was not conditioned on the task. memory.retrieve accepted only project/tech/phase/polarity and ranked by a pure popularity prior (useful_count + 3*times_overlooked), so every caller got the same rows whatever they were doing, and use_cases/hints — the columns describing when a reflection applies — were never scored against anything. Adds an optional `query`: BM25 over title/use_cases/hints, RRF-fused with the prior. It promotes, never discards; a query matching nothing degrades exactly to the previous order. Not marked required — that would break every existing caller for a param whose absence is already safe. 3. Nothing demoted memories that keep not mattering. `ignored` was a no-op on the memory row, so the ranking key had no negative term: a reflection served 142 times and useful zero times sorted level with one never served at all. On a live DB, 55 such memories accounted for 27.5% of every rated exposure. Migration 0013 adds times_ignored/last_ignored_at and backfills them from existing rating history, so the signal is live immediately rather than relearned. Demotion applies only past a 10-session floor and only to memories with no useful history — the best-performing memory in the live DB is ignored more often than it is used. Also drops the default limit_per_bucket from 20 to 5. An LLM working one task draws on ~5 memories however many it is handed (measured 5.3 +/- 2.4 useful against 42.7 exposed per session); the rest is dilution and burnt context. Counts times_ignored once per session, not once per exposure row: a memory retrieved five times in one session that lands nowhere failed once, not five times. This matches how _apply_one already stamps every row for a memory with a single classification. Test-shape updates are golden-value assertions that pinned the old async / pythonw Stop registration and the pre-Phase-6 absence of `query`, not behavioural checks. Full suite: 1492 passed, 22 skipped. Co-Authored-By: Claude Opus 4.8 --- better_memory/cli/install_hooks.py | 8 +- .../db/migrations/0013_ignored_counters.sql | 62 ++++++++++ better_memory/mcp/handlers/reflections.py | 9 +- better_memory/mcp/tools.py | 25 +++- better_memory/services/memory_rating.py | 24 +++- better_memory/services/reflection.py | 110 +++++++++++++++++- better_memory/services/semantic.py | 15 ++- better_memory/storage/agentcore.py | 5 + better_memory/storage/protocol.py | 7 ++ better_memory/storage/sqlite.py | 2 + docs/decisions/stop-hook-must-be-sync.md | 72 ++++++++++++ tests/cli/test_install_hooks.py | 32 +++-- tests/e2e/test_install_hooks.py | 5 +- tests/e2e/test_setup_sh.py | 4 +- tests/mcp/test_episode_tools.py | 17 ++- tests/services/test_ignored_demotion.py | 110 ++++++++++++++++++ .../test_reflection_query_relevance.py | 106 +++++++++++++++++ 17 files changed, 586 insertions(+), 27 deletions(-) create mode 100644 better_memory/db/migrations/0013_ignored_counters.sql create mode 100644 docs/decisions/stop-hook-must-be-sync.md create mode 100644 tests/services/test_ignored_demotion.py create mode 100644 tests/services/test_reflection_query_relevance.py diff --git a/better_memory/cli/install_hooks.py b/better_memory/cli/install_hooks.py index 28e4945..f5c6579 100644 --- a/better_memory/cli/install_hooks.py +++ b/better_memory/cli/install_hooks.py @@ -55,7 +55,13 @@ class HookSpec: _OUR_HOOKS: tuple[HookSpec, ...] = ( HookSpec("better_memory.hooks.session_bootstrap", "SessionStart", None, False, True), HookSpec("better_memory.hooks.observer", "PostToolUse", "Write|Edit|Bash", True, False), - HookSpec("better_memory.hooks.session_close", "Stop", None, True, False), + # Stop MUST be synchronous with stdout attached. The rating sweep replies with + # a `decision: "block"` payload (hooks/session_close.py) — a control-flow + # response that Claude Code only honours from a blocking hook. Registered + # async, the block is dropped and RATE_MEMORIES never forces a rating turn. + # Measured A/B on identical tasks: async => 0% of exposures rated; + # sync => 100% rated. See docs/decisions/stop-hook-must-be-sync.md. + HookSpec("better_memory.hooks.session_close", "Stop", None, False, True), # Contextual injection: surface curated memories relevant to the current # prompt / tool-input. Sync + needs_stdout (reads additionalContext from # stdout). Gated at runtime by BETTER_MEMORY_CONTEXT_INJECT_MODE; both diff --git a/better_memory/db/migrations/0013_ignored_counters.sql b/better_memory/db/migrations/0013_ignored_counters.sql new file mode 100644 index 0000000..0365a65 --- /dev/null +++ b/better_memory/db/migrations/0013_ignored_counters.sql @@ -0,0 +1,62 @@ +-- Migration 0013: per-memory `ignored` counters, and a backfill from history. +-- +-- Ratings were write-only for the `ignored` class: memory_rating._apply_one +-- bumps useful_count / times_misled / times_overlooked but does nothing at all +-- when a memory is classified `ignored`. Nothing in the system therefore ever +-- learns that a memory keeps being shown and keeps not mattering. +-- +-- Measured on a live DB with 4670 rated exposures: 55 memories had been rated +-- 10+ times each and were useful ZERO times. Between them they accounted for +-- 1284 rated exposures — 27.5% of everything served — while contributing +-- nothing. They were never demoted because the ranking key +-- (useful_count + 3*times_overlooked) has no negative term. +-- +-- These counters give the ranker that term. The backfill matters as much as +-- the schema: it turns the rating history that already exists into signal +-- immediately, instead of waiting to re-learn what the DB already knows. + +ALTER TABLE reflections ADD COLUMN times_ignored INTEGER NOT NULL DEFAULT 0; +ALTER TABLE reflections ADD COLUMN last_ignored_at TEXT; +ALTER TABLE semantic_memories ADD COLUMN times_ignored INTEGER NOT NULL DEFAULT 0; +ALTER TABLE semantic_memories ADD COLUMN last_ignored_at TEXT; + +-- Backfill from session_memory_exposure. Counted per distinct +-- (session_id, memory_id): a memory retrieved several times in one session +-- writes one exposure row per retrieval, and _apply_one stamps all of them +-- with the same classification, so counting rows would score retrieval +-- chattiness rather than sessions in which the memory failed to land. + +UPDATE reflections SET + times_ignored = COALESCE(( + SELECT COUNT(DISTINCT e.session_id) + FROM session_memory_exposure e + WHERE e.memory_kind = 'reflection' + AND e.memory_id = reflections.id + AND e.classification = 'ignored' + ), 0), + last_ignored_at = ( + SELECT MAX(e.rated_at) + FROM session_memory_exposure e + WHERE e.memory_kind = 'reflection' + AND e.memory_id = reflections.id + AND e.classification = 'ignored' + ); + +UPDATE semantic_memories SET + times_ignored = COALESCE(( + SELECT COUNT(DISTINCT e.session_id) + FROM session_memory_exposure e + WHERE e.memory_kind = 'semantic' + AND e.memory_id = semantic_memories.id + AND e.classification = 'ignored' + ), 0), + last_ignored_at = ( + SELECT MAX(e.rated_at) + FROM session_memory_exposure e + WHERE e.memory_kind = 'semantic' + AND e.memory_id = semantic_memories.id + AND e.classification = 'ignored' + ); + +CREATE INDEX IF NOT EXISTS idx_reflections_ignored + ON reflections(times_ignored) WHERE useful_count = 0; diff --git a/better_memory/mcp/handlers/reflections.py b/better_memory/mcp/handlers/reflections.py index b0237e1..712ccc7 100644 --- a/better_memory/mcp/handlers/reflections.py +++ b/better_memory/mcp/handlers/reflections.py @@ -71,6 +71,7 @@ async def retrieve(self, args: dict[str, Any]) -> list[TextContent]: tech=args.get("tech"), phase=args.get("phase"), polarity=args.get("polarity"), + query=args.get("query"), ): diag_cid: str | None = None t_retrieve = 0.0 @@ -110,7 +111,12 @@ def _retention_step() -> None: _diag.step("mcp.memory.retrieve", "after_retention_scheduler") project = args.get("project") or project_name() - limit_per_bucket = args.get("limit_per_bucket", 20) + # Default 5, not 20. An LLM working one task draws on ~5 memories + # regardless of how many it is handed (measured: 5.3 +/- 2.4 useful + # per session against 42.7 exposed). Everything past that dilutes + # the set and burns context, so the default returns a shortlist and + # callers that genuinely want the long tail ask for it. + limit_per_bucket = args.get("limit_per_bucket", 5) t_reflections = time.monotonic() if diag_cid else 0.0 _diag.step( "mcp.memory.retrieve", @@ -123,6 +129,7 @@ def _retention_step() -> None: phase=args.get("phase"), polarity=args.get("polarity"), limit_per_bucket=limit_per_bucket, + query=args.get("query"), ) _diag.step( "mcp.memory.retrieve", diff --git a/better_memory/mcp/tools.py b/better_memory/mcp/tools.py index 1236bd5..1db94cf 100644 --- a/better_memory/mcp/tools.py +++ b/better_memory/mcp/tools.py @@ -154,14 +154,26 @@ def tool_definitions( name="memory.retrieve", description=( "Retrieve reflections (do / dont / neutral lessons distilled " - "from prior observations) bucketed by polarity. Filter by " - "project, tech, phase, and polarity. For raw observation " - "lookup, use memory.retrieve_observations." + "from prior observations) bucketed by polarity. ALWAYS pass " + "`query` — a plain-language description of the task you are " + "about to do — or you get the same generic top-ranked lessons " + "every session regardless of what you are working on. Filter " + "further by project, tech, phase, and polarity. For raw " + "observation lookup, use memory.retrieve_observations." ), inputSchema={ "type": "object", "additionalProperties": False, "properties": { + "query": { + "type": "string", + "description": ( + "Plain-language description of the task at hand, " + "e.g. 'changing how retention archives reflections'. " + "Ranks results by relevance to this text fused with " + "the usefulness prior." + ), + }, "project": {"type": "string"}, "tech": {"type": "string"}, "phase": { @@ -174,6 +186,13 @@ def tool_definitions( }, "limit_per_bucket": {"type": "integer"}, }, + # `query` is deliberately NOT required. Making it mandatory + # would break every existing caller (and the start_episode / + # bootstrap internal paths) for a param whose absence degrades + # gracefully to the popularity order. The description urges it + # instead; the real usefulness gains come from the shortlist + # default and the ignored-history demotion, not from coercing + # a query on every call. }, ), Tool( diff --git a/better_memory/services/memory_rating.py b/better_memory/services/memory_rating.py index afafcf1..8383e19 100644 --- a/better_memory/services/memory_rating.py +++ b/better_memory/services/memory_rating.py @@ -67,6 +67,17 @@ class ApplySessionRatingsResult(TypedDict): # rank harder than a cited one. Imported by reflection.py and semantic.py. OVERLOOKED_RANKING_WEIGHT = 3 +# Demotion for memories that keep being served and keep not mattering. +# A memory is only demoted once it has been ignored in at least +# IGNORED_DEMOTION_FLOOR distinct sessions AND has never been useful — one or +# two ignores say nothing (the task simply wasn't relevant that day), but a +# double-digit run of them with no hits is the memory telling you it does not +# belong in the default set. Memories with any useful history are never +# demoted, however often they are also ignored: the best-performing memory on +# the live DB is ignored in 125 of the 192 sessions it appears in. +IGNORED_DEMOTION_FLOOR = 10 +IGNORED_DEMOTION_WEIGHT = 1 + class MemoryRatingService: """Writes useful_count / times_misled on reflections + semantic memories, @@ -214,8 +225,17 @@ def _apply_one( f"WHERE id = ?", (now, memory_id), ) - # 'ignored' is a no-op on the memory row; reached only via - # apply_session_ratings, not credit_one. + elif classification == "ignored": + # Reached only via apply_session_ratings, not credit_one. Counted + # once per session, matching the migration-0013 backfill: a memory + # retrieved five times in one session that lands nowhere failed + # once, not five times. + self._conn.execute( + f"UPDATE {table} " + f"SET times_ignored = times_ignored + 1, last_ignored_at = ? " + f"WHERE id = ?", + (now, memory_id), + ) # 4. Stamp the exposure row. self._conn.execute( diff --git a/better_memory/services/reflection.py b/better_memory/services/reflection.py index 8b0f98a..3bece1e 100644 --- a/better_memory/services/reflection.py +++ b/better_memory/services/reflection.py @@ -33,7 +33,25 @@ from better_memory import _diag from better_memory._common import default_clock, env_session_id -from better_memory.services.memory_rating import OVERLOOKED_RANKING_WEIGHT +from better_memory.search.query import sanitize_fts5_query +from better_memory.services.memory_rating import ( + IGNORED_DEMOTION_FLOOR, + IGNORED_DEMOTION_WEIGHT, + OVERLOOKED_RANKING_WEIGHT, +) + +# Dropped from relevance queries. These survive `sanitize_fts5_query` and the +# >2-char filter, but appear in so many reflections that OR-matching on them +# promotes generic rows over the ones that actually describe the task. +_QUERY_STOPWORDS: frozenset[str] = frozenset({ + "the", "and", "for", "with", "that", "this", "from", "into", "when", + "what", "how", "why", "does", "did", "are", "was", "were", "have", "has", + "had", "not", "any", "all", "can", "should", "would", "could", "will", + "about", "before", "after", "then", "than", "there", "their", "them", + "you", "your", "its", "our", "out", "use", "using", "used", "get", + "make", "made", "want", "need", "like", "just", "some", "which", "who", + "where", "here", "over", "under", "each", "other", "same", "such", +}) def _later_ts(a: str | None, b: str | None) -> str | None: @@ -1168,6 +1186,73 @@ def apply_decision( ) # --------------------------------------------------------- retrieve_reflections + def _fuse_by_relevance( + self, rows: list, *, query: str, rrf_k: int = 60, + ) -> list: + """Re-order ``rows`` by RRF fusion of popularity rank and BM25 rank. + + ``rows`` arrives in popularity order (``useful_count + 3*times_overlooked``, + then confidence, then recency). We compute a second ranking over the same + ids by BM25 relevance against ``reflection_fts`` (title / use_cases / + hints) and fuse the two with reciprocal rank fusion: + + score(d) = 1/(k + pop_rank(d)) + 1/(k + rel_rank(d)) + + matching :mod:`better_memory.search.hybrid`. Rows the query does not + match keep only the popularity term, so relevance *promotes* without + ever discarding — a query that matches nothing degrades exactly to the + previous behaviour. + + Tokens are OR-ed rather than AND-ed: ``sanitize_fts5_query`` joins bare + terms, which FTS5 reads as implicit AND, and a natural-language task + description almost never has every token present in one reflection. + """ + ids = [r["id"] for r in rows] + if not ids: + return rows + + sanitized = sanitize_fts5_query(query) + tokens = [ + t for t in sanitized.split() + if len(t) > 2 and t.lower() not in _QUERY_STOPWORDS + ] + if not tokens: + return rows + match_expr = " OR ".join(tokens) + + placeholders = ",".join("?" for _ in ids) + try: + rel_rows = self._conn.execute( + f""" + SELECT r.id AS id, bm25(reflection_fts) AS bm + FROM reflection_fts + JOIN reflections r ON r.rowid = reflection_fts.rowid + WHERE reflection_fts MATCH ? AND r.id IN ({placeholders}) + ORDER BY bm ASC + """, + [match_expr, *ids], + ).fetchall() + except sqlite3.OperationalError: + # Malformed MATCH expression despite sanitising, or the FTS table + # is absent on an old DB. Relevance is an enhancement, never a + # hard dependency — fall back to the popularity order. + return rows + + if not rel_rows: + return rows + + rel_rank = {r["id"]: i for i, r in enumerate(rel_rows)} + scored = [] + for pop_rank, row in enumerate(rows): + score = 1.0 / (rrf_k + pop_rank) + rr = rel_rank.get(row["id"]) + if rr is not None: + score += 1.0 / (rrf_k + rr) + scored.append((score, pop_rank, row)) + # pop_rank breaks ties deterministically and keeps the fusion stable. + scored.sort(key=lambda t: (-t[0], t[1])) + return [row for _, _, row in scored] + def retrieve_reflections( self, *, @@ -1177,6 +1262,7 @@ def retrieve_reflections( polarity: str | None = None, limit_per_bucket: int | None = 20, track_exposure: bool = True, + query: str | None = None, ) -> dict[str, list[dict]]: """Return reflections bucketed by polarity, ordered by confidence DESC. @@ -1185,6 +1271,12 @@ def retrieve_reflections( - ``tech``: matches same-tech rows OR cross-tech (tech IS NULL) rows. - ``phase``: optional exact match. - ``polarity``: optional exact match; non-matching buckets remain empty. + - ``query``: optional natural-language description of the task at hand. + When given, the filtered set is re-ordered by RRF fusion of the + popularity prior with a BM25 relevance ranking over + ``title / use_cases / hints`` (see :meth:`_fuse_by_relevance`). + Without it, ordering is the popularity prior alone — which returns + the same rows to every caller regardless of what they are doing. - ``limit_per_bucket``: cap each polarity bucket. Default 20 per spec §7. Pass ``None`` to disable the cap (returns every matching row per bucket); used by SessionBootstrapService which injects all @@ -1221,7 +1313,14 @@ def retrieve_reflections( where = " AND ".join(clauses) params.append(OVERLOOKED_RANKING_WEIGHT) + params.append(IGNORED_DEMOTION_WEIGHT) + params.append(IGNORED_DEMOTION_FLOOR) _diag.step(fn, "executing_select") + # The demotion term fires only for memories with no useful history + # at all, and only past the floor — see IGNORED_DEMOTION_FLOOR. + # Without it the ranking key has no negative term, so a memory that + # has been served 142 times and helped zero times sorts level with + # one that has never been served. rows = self._conn.execute( f""" SELECT id, title, phase, polarity, use_cases, hints, @@ -1229,13 +1328,20 @@ def retrieve_reflections( times_misled, updated_at FROM reflections WHERE {where} - ORDER BY (useful_count + ? * times_overlooked) DESC, + ORDER BY (useful_count + ? * times_overlooked + - ? * CASE WHEN useful_count = 0 + THEN MAX(0, times_ignored - ?) + ELSE 0 END) DESC, confidence DESC, updated_at DESC """, params, ).fetchall() _diag.step(fn, "select_done", n_rows=len(rows)) + if query: + rows = self._fuse_by_relevance(rows, query=query) + _diag.step(fn, "relevance_fused", n_rows=len(rows)) + # Convert None (unlimited) to sys.maxsize so the loop body has a definite int. cap = limit_per_bucket if limit_per_bucket is not None else sys.maxsize buckets: dict[str, list[dict]] = {"do": [], "dont": [], "neutral": []} diff --git a/better_memory/services/semantic.py b/better_memory/services/semantic.py index 840010c..d43f3b0 100644 --- a/better_memory/services/semantic.py +++ b/better_memory/services/semantic.py @@ -16,7 +16,11 @@ from uuid import uuid4 from better_memory._common import default_clock, env_session_id -from better_memory.services.memory_rating import OVERLOOKED_RANKING_WEIGHT +from better_memory.services.memory_rating import ( + IGNORED_DEMOTION_FLOOR, + IGNORED_DEMOTION_WEIGHT, + OVERLOOKED_RANKING_WEIGHT, +) @dataclass(frozen=True) @@ -240,10 +244,17 @@ def list_for_project( "times_overlooked, last_overlooked_at " "FROM semantic_memories " f"WHERE {' AND '.join(where_clauses)} " - "ORDER BY (useful_count + ? * times_overlooked) DESC, " + # Mirrors the reflections ranking, including the ignored-history + # demotion for memories with no useful history past the floor. + "ORDER BY (useful_count + ? * times_overlooked " + " - ? * CASE WHEN useful_count = 0 " + " THEN MAX(0, times_ignored - ?) " + " ELSE 0 END) DESC, " "created_at DESC" ) params.append(OVERLOOKED_RANKING_WEIGHT) + params.append(IGNORED_DEMOTION_WEIGHT) + params.append(IGNORED_DEMOTION_FLOOR) rows = self._conn.execute(sql, params).fetchall() results = [ SemanticMemory( diff --git a/better_memory/storage/agentcore.py b/better_memory/storage/agentcore.py index 7c56822..e67cf7c 100644 --- a/better_memory/storage/agentcore.py +++ b/better_memory/storage/agentcore.py @@ -215,6 +215,7 @@ def retrieve( polarity: str | None = None, limit_per_bucket: int | None = 20, track_exposure: bool = True, + query: str | None = None, ) -> dict[str, list[dict[str, Any]]]: """Bucketed reflection retrieval matching ReflectionSynthesisService.retrieve_reflections. @@ -228,6 +229,10 @@ def retrieve( Returns dict[polarity, list[reflection_dict]] in the same shape sqlite mode returns; the MCP memory.retrieve handler json-dumps this directly to Claude. + ``query`` is accepted for parity but no-op in agentcore mode — BM25 + relevance fusion needs the local ``reflection_fts`` index, which has no + AgentCore equivalent. Ordering stays the popularity rule below. + track_exposure is accepted for parity but no-op in agentcore mode — AgentCore has no session_memory_exposure table; exposure tracking is not part of the agentcore-mode rating model. diff --git a/better_memory/storage/protocol.py b/better_memory/storage/protocol.py index 890cea2..d67855f 100644 --- a/better_memory/storage/protocol.py +++ b/better_memory/storage/protocol.py @@ -90,6 +90,7 @@ def retrieve( polarity: str | None = None, limit_per_bucket: int | None = 20, track_exposure: bool = True, + query: str | None = None, ) -> dict[str, list[dict[str, Any]]]: """Bucketed reflection retrieval, keyed by polarity (do / dont / neutral). @@ -101,6 +102,12 @@ def retrieve( ORDER BY ``useful_count + 3 * times_overlooked DESC``, agentcore mode applies the same formula client-side over metadata counters). + ``query`` optionally supplies a natural-language description of the + task at hand. sqlite mode fuses a BM25 relevance ranking over + title / use_cases / hints into the popularity order via RRF; agentcore + mode ignores it. Omitting it yields the popularity order alone, which + is identical for every caller regardless of the work being done. + This method is the canonical path for the MCP ``memory.retrieve`` tool handler and the ``memory.start_episode`` handler in ``better_memory/mcp/server.py``.""" diff --git a/better_memory/storage/sqlite.py b/better_memory/storage/sqlite.py index aa1ad87..f3dbab9 100644 --- a/better_memory/storage/sqlite.py +++ b/better_memory/storage/sqlite.py @@ -113,6 +113,7 @@ def retrieve( polarity: str | None = None, limit_per_bucket: int | None = 20, track_exposure: bool = True, + query: str | None = None, ) -> dict[str, list[dict[str, Any]]]: return self._synthesis.retrieve_reflections( project=project or self._project, @@ -121,6 +122,7 @@ def retrieve( polarity=polarity, limit_per_bucket=limit_per_bucket, track_exposure=track_exposure, + query=query, ) async def list_observations( diff --git a/docs/decisions/stop-hook-must-be-sync.md b/docs/decisions/stop-hook-must-be-sync.md new file mode 100644 index 0000000..43ad2ea --- /dev/null +++ b/docs/decisions/stop-hook-must-be-sync.md @@ -0,0 +1,72 @@ +# The Stop hook must be synchronous with stdout attached + +## Status + +Accepted — 2026-07-21. + +## Context + +better-memory learns which memories are worth surfacing from a per-session +rating signal: at session end the LLM classifies each exposed memory as +`cited / shaped / ignored / misled / overlooked`. Those ratings drive the +retrieval ranking and the retention/pruning decisions. If ratings never land, +the whole feedback loop is open — the system keeps surfacing whatever it +surfaced on day one, forever. + +The rating turn is triggered by the `Stop` hook (`hooks/session_close.py`). +When a session has unrated exposures, the hook replies with: + +```json +{"decision": "block", + "reason": "RATE_MEMORIES — N pending rating(s)", + "hookSpecificOutput": {"hookEventName": "Stop", "additionalContext": ""}} +``` + +`decision: "block"` is a **control-flow** response: it tells Claude Code not to +stop yet and to run one more turn with the directive as context. Claude Code +only honours control-flow output from a **blocking** hook — one it waits on and +reads stdout from. A hook registered `"async": true` is fire-and-forget: its +stdout is not consumed for control flow, so the block is silently dropped and +the rating turn never happens. + +The hook was registered `is_async=True, needs_stdout=False` +(`cli/install_hooks.py`), which also selected `pythonw.exe` on Windows. + +## Evidence + +Two live headless sessions, identical task and identical copy of the memory +DB, differing only in the Stop hook's registration: + +| Stop hook | exposed | rated | useful | useful% | +|----------------------|---------|-------|--------|---------| +| async (as shipped) | 44 | 0 | 0 | 0.00% | +| sync + stdout | 44 | 44 | 9 | 20.45% | + +Across a 12-task control run every session rated **zero** memories. On the live +production DB, 30 of 39 sessions in the measured month had zero rated +exposures. The loop was effectively never closing. + +`pythonw.exe` nulling stdout was considered as the cause and **ruled out**: a +direct probe showed `pythonw.exe` stdout survives redirection to a pipe. The +defect is the async registration, not the interpreter — but the fix moves to +`python.exe` anyway because a blocking hook that must emit stdout has no reason +to use the windowless interpreter. + +## Decision + +Register `Stop` as `is_async=False, needs_stdout=True`: + +```python +HookSpec("better_memory.hooks.session_close", "Stop", None, False, True) +``` + +`needs_stdout=True` selects `python.exe` (the interpreter whose stdout Claude +Code reads); `is_async=False` makes Claude Code wait for and honour the block. + +## Consequences + +- The rating turn fires reliably; the ranking/retention loop closes. +- A one-turn cost at session end when unrated exposures exist. The hook already + short-circuits (no block) when everything is rated or nothing was exposed. +- The tests that pinned the old shape (`async`, `pythonw.exe`) are updated to + pin the new one; they are golden-shape assertions, not behaviour checks. diff --git a/tests/cli/test_install_hooks.py b/tests/cli/test_install_hooks.py index 336c9ab..2028889 100644 --- a/tests/cli/test_install_hooks.py +++ b/tests/cli/test_install_hooks.py @@ -108,11 +108,16 @@ def test_observer_is_post_tool_use_with_matcher(self) -> None: assert observer.matcher == "Write|Edit|Bash" assert observer.is_async is True - def test_session_close_is_stop_no_matcher_async(self) -> None: + def test_session_close_is_stop_no_matcher_sync_with_stdout(self) -> None: + # Stop must be SYNC with stdout attached: the rating sweep replies with + # a `decision: "block"` control-flow payload that Claude Code only + # honours from a blocking hook. Registered async, RATE_MEMORIES never + # forces a rating turn and nothing gets rated. See install_hooks.py. sc = next(s for s in _OUR_HOOKS if s.module.endswith("session_close")) assert sc.event == "Stop" assert sc.matcher is None - assert sc.is_async is True + assert sc.is_async is False + assert sc.needs_stdout is True from better_memory.cli.install_hooks import merge_settings_json @@ -137,7 +142,8 @@ def test_empty_hooks_adds_all_three(self) -> None: assert len(stop) == 1 assert "matcher" not in stop[0] assert "better_memory.hooks.session_close" in stop[0]["hooks"][0]["command"] - assert stop[0]["hooks"][0].get("async") is True + # Sync now — see test_session_close_is_stop_no_matcher_sync_with_stdout. + assert stop[0]["hooks"][0].get("async") is None def test_existing_user_postooluse_preserved(self) -> None: existing = { @@ -296,10 +302,15 @@ def test_idempotent_second_run_is_noop(self) -> None: assert first == second def test_session_bootstrap_uses_venv_py_async_hooks_use_venv_pyw(self) -> None: - """Foreground bootstrap needs python.exe (stdout reaches Claude - Code); the two async hooks keep pythonw.exe so they don't flash - a console window on every tool call. setup.sh passes the same - path on non-Windows; the split only matters on Windows.""" + """Foreground bootstrap + Stop need python.exe (stdout reaches Claude + Code); the one remaining async hook (PostToolUse observer) keeps + pythonw.exe so it doesn't flash a console window on every tool call. + setup.sh passes the same path on non-Windows; the split only matters + on Windows. + + Stop moved to python.exe with this change: it returns a rating + `decision: "block"` payload that Claude Code only honours from a + blocking, stdout-attached hook.""" out = merge_settings_json( {}, venv_py="/venv/python.exe", venv_pyw="/venv/pythonw.exe", ) @@ -311,7 +322,7 @@ def test_session_bootstrap_uses_venv_py_async_hooks_use_venv_pyw(self) -> None: assert "pythonw.exe" in ptu_cmd stop_cmd = out["hooks"]["Stop"][0]["hooks"][0]["command"] - assert "pythonw.exe" in stop_cmd + assert "python.exe" in stop_cmd and "pythonw.exe" not in stop_cmd def test_default_venv_py_falls_back_to_venv_pyw(self) -> None: """Back-compat: callers that only pass venv_pyw get the old @@ -607,9 +618,10 @@ def test_main_passes_venv_py_separately_to_settings_merge( f"SessionStart command must use python.exe, got: {ss_cmd!r}" ) - # Sanity: async PostToolUse + Stop hooks should still use pythonw.exe. + # Sanity: the async PostToolUse observer still uses pythonw.exe. ptu_cmd = settings["hooks"]["PostToolUse"][0]["hooks"][0]["command"] assert "pythonw.exe" in ptu_cmd + # Stop moved to python.exe: its rating block payload needs stdout attached. stop_cmd = settings["hooks"]["Stop"][0]["hooks"][0]["command"] - assert "pythonw.exe" in stop_cmd + assert "python.exe" in stop_cmd and "pythonw.exe" not in stop_cmd diff --git a/tests/e2e/test_install_hooks.py b/tests/e2e/test_install_hooks.py index 731cc8e..7802909 100644 --- a/tests/e2e/test_install_hooks.py +++ b/tests/e2e/test_install_hooks.py @@ -60,7 +60,10 @@ EXPECTED_ENTRIES: dict[str, tuple[str, str, bool, str | None]] = { "SessionStart": ("better_memory.hooks.session_bootstrap", VENV_PY, False, None), "PostToolUse": ("better_memory.hooks.observer", VENV_PYW, True, "Write|Edit|Bash"), - "Stop": ("better_memory.hooks.session_close", VENV_PYW, True, None), + # Stop is sync + VENV_PY (stdout attached): its rating sweep returns a + # `decision: "block"` control-flow payload that only a blocking hook is + # honoured for. Async/pythonw => nothing ever gets rated. + "Stop": ("better_memory.hooks.session_close", VENV_PY, False, None), "UserPromptSubmit": ("better_memory.hooks.contextual_inject", VENV_PY, False, None), "PreToolUse": ("better_memory.hooks.contextual_inject", VENV_PY, False, "Skill|Task|Write"), } diff --git a/tests/e2e/test_setup_sh.py b/tests/e2e/test_setup_sh.py index 57510be..60896d0 100644 --- a/tests/e2e/test_setup_sh.py +++ b/tests/e2e/test_setup_sh.py @@ -387,7 +387,9 @@ def test_settings_hooks_py_pyw_split(self, decline_run: SetupRun) -> None: expected_interp = { ("SessionStart", "better_memory.hooks.session_bootstrap"): VENV_PY, ("PostToolUse", "better_memory.hooks.observer"): VENV_PYW, - ("Stop", "better_memory.hooks.session_close"): VENV_PYW, + # Sync + stdout-attached: the rating sweep's block payload only + # lands from a blocking hook. See install_hooks.py. + ("Stop", "better_memory.hooks.session_close"): VENV_PY, ("UserPromptSubmit", "better_memory.hooks.contextual_inject"): VENV_PY, ("PreToolUse", "better_memory.hooks.contextual_inject"): VENV_PY, } diff --git a/tests/mcp/test_episode_tools.py b/tests/mcp/test_episode_tools.py index e6c78af..5227b5e 100644 --- a/tests/mcp/test_episode_tools.py +++ b/tests/mcp/test_episode_tools.py @@ -222,20 +222,29 @@ def test_retrieve_via_service_returns_reflection_buckets(self, conn): assert {r["title"] for r in result["dont"]} == {"Don't that"} def test_memory_retrieve_tool_schema_takes_filter_params(self): - """Tool schema should expose project/tech/phase/polarity, drop legacy params.""" + """Tool schema should expose project/tech/phase/polarity + query. + + ``query`` was dropped in Phase 6 when this tool was repointed from + observations to reflections, because reflections had no relevance + path — ranking was the popularity prior alone. It is back with real + semantics: BM25 over title/use_cases/hints, RRF-fused with that prior. + The observation-shaped filters stay gone; they never applied to + reflections and belong to memory.retrieve_observations. + """ from better_memory.mcp.server import _tool_definitions tool = next( t for t in _tool_definitions() if t.name == "memory.retrieve" ) props = tool.inputSchema["properties"] - # New filter params present. + # Filter params present. assert "project" in props assert "tech" in props assert "phase" in props assert "polarity" in props - # Legacy params removed. - assert "query" not in props + # Relevance param present. + assert "query" in props + # Observation-shaped params remain absent. assert "component" not in props assert "window" not in props assert "scope_path" not in props diff --git a/tests/services/test_ignored_demotion.py b/tests/services/test_ignored_demotion.py new file mode 100644 index 0000000..90867c3 --- /dev/null +++ b/tests/services/test_ignored_demotion.py @@ -0,0 +1,110 @@ +"""Memories that keep being served and keep not mattering must sink. + +Before migration 0013 the ranking key had no negative term, so a reflection +served 142 times that was useful zero times sorted level with one that had +never been served at all. On a live DB, 55 such memories accounted for 27.5% +of every rated exposure. +""" +from __future__ import annotations + +from pathlib import Path + +import pytest + +from better_memory.db.connection import connect +from better_memory.db.schema import apply_migrations +from better_memory.services.memory_rating import ( + IGNORED_DEMOTION_FLOOR, + MemoryRatingService, +) +from better_memory.services.reflection import ReflectionSynthesisService + + +@pytest.fixture +def conn(tmp_memory_db: Path): + c = connect(tmp_memory_db) + apply_migrations(c) + try: + yield c + finally: + c.close() + + +def _seed(conn, rid, *, useful_count=0, times_ignored=0, confidence=0.5): + conn.execute( + """INSERT INTO reflections + (id, title, project, phase, polarity, use_cases, hints, + confidence, created_at, updated_at, useful_count, times_ignored) + VALUES (?, ?, 'p', 'general', 'do', 'uc', '[]', ?, + '2026-01-01', '2026-01-01', ?, ?)""", + (rid, rid, confidence, useful_count, times_ignored), + ) + conn.commit() + + +def _ids(conn, **kw): + svc = ReflectionSynthesisService(conn) + return [r["id"] for r in svc.retrieve_reflections(project="p", **kw)["do"]] + + +class TestIgnoredDemotion: + def test_chronically_ignored_sinks_below_never_served(self, conn): + _seed(conn, "r-proven-useless", useful_count=0, times_ignored=60) + _seed(conn, "r-untested", useful_count=0, times_ignored=0) + assert _ids(conn) == ["r-untested", "r-proven-useless"] + + def test_below_floor_is_not_demoted(self, conn): + # A handful of ignores says nothing — the task simply wasn't relevant. + _seed(conn, "r-a", useful_count=0, times_ignored=IGNORED_DEMOTION_FLOOR, + confidence=0.9) + _seed(conn, "r-b", useful_count=0, times_ignored=0, confidence=0.1) + assert _ids(conn) == ["r-a", "r-b"], "at the floor, confidence still decides" + + def test_memory_with_useful_history_is_never_demoted(self, conn): + # The best-performing memory on the live DB is ignored far more often + # than it is used; a hit rate below 50% is normal and fine. + _seed(conn, "r-useful-but-often-ignored", useful_count=18, times_ignored=55) + _seed(conn, "r-untested", useful_count=0, times_ignored=0) + assert _ids(conn)[0] == "r-useful-but-often-ignored" + + def test_apply_session_ratings_bumps_times_ignored(self, conn, monkeypatch): + monkeypatch.setenv("CLAUDE_SESSION_ID", "s1") + _seed(conn, "r-a") + conn.execute( + "INSERT INTO session_memory_exposure " + "(session_id, memory_kind, memory_id, exposed_at, source) " + "VALUES ('s1', 'reflection', 'r-a', '2026-01-01', 'retrieve')" + ) + conn.commit() + + MemoryRatingService(conn).apply_session_ratings( + session_id="s1", + ratings=[{"kind": "reflection", "id": "r-a", "class": "ignored"}], + ) + row = conn.execute( + "SELECT times_ignored, last_ignored_at FROM reflections WHERE id = 'r-a'" + ).fetchone() + assert row["times_ignored"] == 1 + assert row["last_ignored_at"] is not None + + def test_repeat_exposures_in_one_session_count_once(self, conn, monkeypatch): + # A memory retrieved five times in one session that lands nowhere + # failed once, not five times. + monkeypatch.setenv("CLAUDE_SESSION_ID", "s1") + _seed(conn, "r-a") + for i in range(5): + conn.execute( + "INSERT INTO session_memory_exposure " + "(session_id, memory_kind, memory_id, exposed_at, source) " + "VALUES ('s1', 'reflection', 'r-a', ?, 'retrieve')", + (f"2026-01-01T00:0{i}:00",), + ) + conn.commit() + + MemoryRatingService(conn).apply_session_ratings( + session_id="s1", + ratings=[{"kind": "reflection", "id": "r-a", "class": "ignored"}], + ) + assert conn.execute( + "SELECT times_ignored FROM reflections WHERE id = 'r-a'" + ).fetchone()["times_ignored"] == 1 diff --git a/tests/services/test_reflection_query_relevance.py b/tests/services/test_reflection_query_relevance.py new file mode 100644 index 0000000..2fce428 --- /dev/null +++ b/tests/services/test_reflection_query_relevance.py @@ -0,0 +1,106 @@ +"""Verify the ``query`` argument makes retrieval task-relevant. + +Without ``query``, ``retrieve_reflections`` orders by the popularity prior +alone (useful_count + 3*times_overlooked, confidence, recency), so every +caller gets the same rows regardless of what they are working on. With it, a +BM25 ranking over title/use_cases/hints is fused in via RRF. +""" +from __future__ import annotations + +from pathlib import Path + +import pytest + +from better_memory.db.connection import connect +from better_memory.db.schema import apply_migrations +from better_memory.services.reflection import ReflectionSynthesisService + + +@pytest.fixture +def conn(tmp_memory_db: Path): + c = connect(tmp_memory_db) + apply_migrations(c) + try: + yield c + finally: + c.close() + + +def _seed(conn, rid, *, title, use_cases="uc", hints="[]", useful_count=0, + confidence=0.5, polarity="do"): + conn.execute( + """INSERT INTO reflections + (id, title, project, phase, polarity, use_cases, hints, + confidence, created_at, updated_at, useful_count) + VALUES (?, ?, 'p', 'general', ?, ?, ?, ?, + '2026-01-01', '2026-01-01', ?)""", + (rid, title, polarity, use_cases, hints, confidence, useful_count), + ) + conn.commit() + + +class TestQueryRelevance: + def test_relevant_row_outranks_more_popular_irrelevant_row(self, conn): + # The popular row wins on the prior alone; the relevant row only wins + # once the query is supplied. + _seed(conn, "r-popular-irrelevant", + title="Always rebase before pushing", useful_count=10) + _seed(conn, "r-unpopular-relevant", + title="Retention archives reflections by confidence", + use_cases="changing retention or pruning thresholds", + useful_count=0) + svc = ReflectionSynthesisService(conn) + + without = [r["id"] for r in svc.retrieve_reflections(project="p")["do"]] + assert without[0] == "r-popular-irrelevant" + + with_query = [ + r["id"] for r in svc.retrieve_reflections( + project="p", query="changing how retention prunes reflections", + )["do"] + ] + assert with_query[0] == "r-unpopular-relevant" + + def test_query_promotes_but_never_discards(self, conn): + _seed(conn, "r-a", title="Retention thresholds", useful_count=0) + _seed(conn, "r-b", title="Unrelated advice", useful_count=1) + svc = ReflectionSynthesisService(conn) + ids = [ + r["id"] for r in svc.retrieve_reflections( + project="p", query="retention", + )["do"] + ] + assert set(ids) == {"r-a", "r-b"}, "non-matching rows must survive" + + def test_no_match_degrades_to_popularity_order(self, conn): + _seed(conn, "r-popular", title="Alpha", useful_count=9) + _seed(conn, "r-quiet", title="Beta", useful_count=0) + svc = ReflectionSynthesisService(conn) + baseline = [r["id"] for r in svc.retrieve_reflections(project="p")["do"]] + queried = [ + r["id"] for r in svc.retrieve_reflections( + project="p", query="zzzznothingmatchesthis", + )["do"] + ] + assert queried == baseline + + def test_operator_characters_in_query_do_not_raise(self, conn): + # 'better-memory' parses as a column-exclusion in raw FTS5 syntax. + _seed(conn, "r-a", title="Alpha", useful_count=1) + svc = ReflectionSynthesisService(conn) + ids = [ + r["id"] for r in svc.retrieve_reflections( + project="p", query='better-memory: "hooks" (windows)', + )["do"] + ] + assert ids == ["r-a"] + + def test_short_tokens_alone_degrade_to_popularity_order(self, conn): + _seed(conn, "r-popular", title="Alpha", useful_count=4) + _seed(conn, "r-quiet", title="Beta", useful_count=0) + svc = ReflectionSynthesisService(conn) + baseline = [r["id"] for r in svc.retrieve_reflections(project="p")["do"]] + queried = [ + r["id"] for r in svc.retrieve_reflections(project="p", query="a of to")["do"] + ] + assert queried == baseline From 47e29f0288539c05224be13b8056712822e12ed0 Mon Sep 17 00:00:00 2001 From: gethin Date: Tue, 21 Jul 2026 23:36:52 +0100 Subject: [PATCH 2/2] fix(memory): dedupe exposure rows per (session, kind, memory) at write time A session's exposures are a set: list_session_exposures groups by (kind, id) and the rating path stamps every row for a memory with one classification. But the PK includes exposed_at, so every re-serve of an already-exposed memory added a new row that existed only to inflate statistics computed over the raw table. Measured on live data the same behaviour read as 16.08% "useful" raw and 9.25% deduplicated. Guard all three write paths (bootstrap, reflection retrieve, semantic retrieve) with INSERT ... WHERE NOT EXISTS on (session_id, kind, memory_id). First exposure wins, including its source label. Updates test_full_rating_loop, which had codified the double-count ("PK is distinct -> 5 new rows") as expected behaviour. Co-Authored-By: Claude Fable 5 --- better_memory/services/reflection.py | 13 ++- better_memory/services/semantic.py | 13 ++- better_memory/services/session_bootstrap.py | 21 +++- tests/integration/test_memory_rating_e2e.py | 28 +++--- tests/services/test_exposure_dedup.py | 100 ++++++++++++++++++++ 5 files changed, 153 insertions(+), 22 deletions(-) create mode 100644 tests/services/test_exposure_dedup.py diff --git a/better_memory/services/reflection.py b/better_memory/services/reflection.py index 3bece1e..c124e0d 100644 --- a/better_memory/services/reflection.py +++ b/better_memory/services/reflection.py @@ -1397,11 +1397,18 @@ def retrieve_reflections( _diag.step( fn, "exposure_insert", n_ids=len(all_ids) ) + # One row per (session, memory) — see + # SessionBootstrapService.record_exposures for why + # re-serves must not add rows. self._conn.executemany( - "INSERT OR IGNORE INTO session_memory_exposure " + "INSERT INTO session_memory_exposure " "(session_id, memory_kind, memory_id, exposed_at, source) " - "VALUES (?, 'reflection', ?, ?, 'retrieve')", - [(sid, rid, now) for rid in all_ids], + "SELECT ?, 'reflection', ?, ?, 'retrieve' " + "WHERE NOT EXISTS (" + " SELECT 1 FROM session_memory_exposure " + " WHERE session_id = ? AND memory_kind = 'reflection' " + " AND memory_id = ?)", + [(sid, rid, now, sid, rid) for rid in all_ids], ) _diag.step(fn, "exposure_commit") self._conn.commit() diff --git a/better_memory/services/semantic.py b/better_memory/services/semantic.py index d43f3b0..663aef1 100644 --- a/better_memory/services/semantic.py +++ b/better_memory/services/semantic.py @@ -290,11 +290,18 @@ def list_for_project( pass elif results: now = self._clock().isoformat() + # One row per (session, memory) — see + # SessionBootstrapService.record_exposures for why re-serves + # must not add rows. self._conn.executemany( - "INSERT OR IGNORE INTO session_memory_exposure " + "INSERT INTO session_memory_exposure " "(session_id, memory_kind, memory_id, exposed_at, source) " - "VALUES (?, 'semantic', ?, ?, 'retrieve')", - [(sid, m.id, now) for m in results], + "SELECT ?, 'semantic', ?, ?, 'retrieve' " + "WHERE NOT EXISTS (" + " SELECT 1 FROM session_memory_exposure " + " WHERE session_id = ? AND memory_kind = 'semantic' " + " AND memory_id = ?)", + [(sid, m.id, now, sid, m.id) for m in results], ) self._conn.commit() return results diff --git a/better_memory/services/session_bootstrap.py b/better_memory/services/session_bootstrap.py index ab08210..0add45d 100644 --- a/better_memory/services/session_bootstrap.py +++ b/better_memory/services/session_bootstrap.py @@ -151,6 +151,15 @@ def record_exposures( ) -> None: """Write one session_memory_exposure row per (kind, id) item. + At most one row per (session, kind, id), regardless of how many times + the memory is re-served within the session. The rating vocabulary + treats a session's exposures as a SET — list_session_exposures groups + by (kind, id) and _apply_one stamps every row with one classification + — but the PK includes exposed_at, so before this guard each re-serve + added a row and any statistic over the raw table double-counted. + Measured on a live DB the inflation was 16.08% vs 9.25% "useful" + for the identical underlying behaviour. + Best-effort: skips entirely when session_id is empty. Own commit (see module docstring on connection ownership). """ @@ -158,10 +167,16 @@ def record_exposures( return now = self._clock().isoformat() self._conn.executemany( - "INSERT OR IGNORE INTO session_memory_exposure " + "INSERT INTO session_memory_exposure " "(session_id, memory_kind, memory_id, exposed_at, source) " - "VALUES (?, ?, ?, ?, ?)", - [(session_id, kind, mid, now, source) for kind, mid in items], + "SELECT ?, ?, ?, ?, ? " + "WHERE NOT EXISTS (" + " SELECT 1 FROM session_memory_exposure " + " WHERE session_id = ? AND memory_kind = ? AND memory_id = ?)", + [ + (session_id, kind, mid, now, source, session_id, kind, mid) + for kind, mid in items + ], ) self._conn.commit() diff --git a/tests/integration/test_memory_rating_e2e.py b/tests/integration/test_memory_rating_e2e.py index d25da53..6e0a50d 100644 --- a/tests/integration/test_memory_rating_e2e.py +++ b/tests/integration/test_memory_rating_e2e.py @@ -113,9 +113,12 @@ def test_full_rating_loop(conn, monkeypatch): ) # ------------------------------------------------------------------ - # 2) Mid-session: explicit retrieve calls add source='retrieve' rows. - # CLAUDE_SESSION_ID is set (monkeypatched), so the exposure write - # fires. exposed_at = T1 ≠ T0, so the PK is distinct → 5 new rows. + # 2) Mid-session: explicit retrieve calls re-serve the same five + # memories. A session's exposures are a SET — the write path skips + # memories already exposed in this session (whatever the source), so + # the totals do not move. (Before the dedup guard, exposed_at made + # the PK distinct and each re-serve added a row, double-counting every + # statistic over the raw table.) # ------------------------------------------------------------------ refl = ReflectionSynthesisService(conn, clock=retrieve_clock) refl.retrieve_reflections(project="p") # track_exposure=True (default) @@ -131,15 +134,15 @@ def test_full_rating_loop(conn, monkeypatch): assert source_counts.get("bootstrap", 0) == 5, ( f"Expected 5 bootstrap rows; got {source_counts}" ) - assert source_counts.get("retrieve", 0) == 5, ( - f"Expected 5 retrieve rows (3 refl + 2 sem); got {source_counts}" + assert source_counts.get("retrieve", 0) == 0, ( + f"Re-serves must not add rows; got {source_counts}" ) total = conn.execute( "SELECT COUNT(*) AS n FROM session_memory_exposure " "WHERE session_id='SESS-1'" ).fetchone()["n"] - assert total == 10, f"Expected 10 total exposure rows; got {total}" + assert total == 5, f"Expected 5 total exposure rows; got {total}" # ------------------------------------------------------------------ # 3) Credit r1 (reflection, cited) and s1 (semantic, shaped) mid-session. @@ -158,9 +161,8 @@ def test_full_rating_loop(conn, monkeypatch): assert s1_credit == {"applied": "shaped", "skipped": None} # ------------------------------------------------------------------ - # 4) After credit_one, BOTH exposure rows for r1 (bootstrap AND retrieve) - # are stamped with rated_at. useful_count on the memory row is 1 - # (single bump, even though two rows were stamped). + # 4) After credit_one, r1's single exposure row is stamped with + # rated_at and useful_count on the memory row is 1. # ------------------------------------------------------------------ r1_useful = conn.execute( "SELECT useful_count FROM reflections WHERE id='r1'" @@ -172,7 +174,7 @@ def test_full_rating_loop(conn, monkeypatch): ).fetchone()["useful_count"] assert s1_useful == 1, f"s1 useful_count should be 1; got {s1_useful}" - # All four rows for r1 + s1 are rated; the unrated set excludes them. + # The rows for r1 + s1 are rated; the unrated set excludes them. unrated_rows = conn.execute( "SELECT memory_kind, memory_id FROM session_memory_exposure " "WHERE session_id='SESS-1' AND rated_at IS NULL" @@ -180,9 +182,9 @@ def test_full_rating_loop(conn, monkeypatch): unrated_ids = {(r["memory_kind"], r["memory_id"]) for r in unrated_rows} assert ("reflection", "r1") not in unrated_ids, "r1 should be fully rated" assert ("semantic", "s1") not in unrated_ids, "s1 should be fully rated" - # r2, r3, s2 each have 2 exposure rows (boot + retrieve) → 6 unrated rows. - assert len(unrated_rows) == 6, ( - f"Expected 6 unrated rows (r2×2, r3×2, s2×2); got {len(unrated_rows)}" + # r2, r3, s2 each have exactly one exposure row → 3 unrated rows. + assert len(unrated_rows) == 3, ( + f"Expected 3 unrated rows (r2, r3, s2); got {len(unrated_rows)}" ) # ------------------------------------------------------------------ diff --git a/tests/services/test_exposure_dedup.py b/tests/services/test_exposure_dedup.py new file mode 100644 index 0000000..d64c538 --- /dev/null +++ b/tests/services/test_exposure_dedup.py @@ -0,0 +1,100 @@ +"""A session's exposures are a set: re-serving a memory must not add rows. + +The PK on session_memory_exposure includes exposed_at, so before the +write-time guard every re-retrieval added a row. The rating path already +collapses duplicates (one classification per (kind, id) per session), so the +extra rows existed only to inflate any statistic computed over the raw table +— measured 16.08% vs 9.25% "useful" on the same live data. +""" +from __future__ import annotations + +from pathlib import Path + +import pytest + +from better_memory.db.connection import connect +from better_memory.db.schema import apply_migrations +from better_memory.services.reflection import ReflectionSynthesisService +from better_memory.services.semantic import SemanticMemoryService +from better_memory.services.session_bootstrap import SessionBootstrapService + + +@pytest.fixture +def conn(tmp_memory_db: Path): + c = connect(tmp_memory_db) + apply_migrations(c) + try: + yield c + finally: + c.close() + + +def _rows(conn, session_id="s1"): + return conn.execute( + "SELECT memory_kind, memory_id, source FROM session_memory_exposure " + "WHERE session_id = ? ORDER BY memory_kind, memory_id", + (session_id,), + ).fetchall() + + +def _seed_reflection(conn, rid): + conn.execute( + """INSERT INTO reflections + (id, title, project, phase, polarity, use_cases, hints, + confidence, created_at, updated_at) + VALUES (?, ?, 'p', 'general', 'do', 'uc', '[]', 0.5, + '2026-01-01', '2026-01-01')""", + (rid, rid), + ) + conn.commit() + + +class TestExposureDedup: + def test_record_exposures_is_idempotent_per_session(self, conn): + svc = SessionBootstrapService(conn) + items = [("reflection", "r-a"), ("semantic", "s-a")] + svc.record_exposures(session_id="s1", items=items, source="bootstrap") + svc.record_exposures(session_id="s1", items=items, source="bootstrap") + assert len(_rows(conn)) == 2 + + def test_reserve_keeps_first_source(self, conn): + # bootstrap exposes first; a later retrieve of the same memory must + # not relabel or duplicate it. + svc = SessionBootstrapService(conn) + svc.record_exposures( + session_id="s1", items=[("reflection", "r-a")], source="bootstrap" + ) + svc.record_exposures( + session_id="s1", items=[("reflection", "r-a")], source="retrieve" + ) + rows = _rows(conn) + assert len(rows) == 1 + assert rows[0]["source"] == "bootstrap" + + def test_other_sessions_unaffected(self, conn): + svc = SessionBootstrapService(conn) + svc.record_exposures( + session_id="s1", items=[("reflection", "r-a")], source="bootstrap" + ) + svc.record_exposures( + session_id="s2", items=[("reflection", "r-a")], source="bootstrap" + ) + assert len(_rows(conn, "s1")) == 1 + assert len(_rows(conn, "s2")) == 1 + + def test_repeated_retrieve_reflections_writes_one_row(self, conn, monkeypatch): + monkeypatch.setenv("CLAUDE_SESSION_ID", "s1") + _seed_reflection(conn, "r-a") + svc = ReflectionSynthesisService(conn) + svc.retrieve_reflections(project="p") + svc.retrieve_reflections(project="p") + svc.retrieve_reflections(project="p") + assert len(_rows(conn)) == 1 + + def test_repeated_semantic_list_writes_one_row(self, conn, monkeypatch): + monkeypatch.setenv("CLAUDE_SESSION_ID", "s1") + sem = SemanticMemoryService(conn) + sem.create(content="fact", project="p") + sem.list_for_project(project="p") + sem.list_for_project(project="p") + assert len(_rows(conn)) == 1