diff --git a/src/builtin_actions.py b/src/builtin_actions.py index 3fdeedc71..b48ed94fa 100644 --- a/src/builtin_actions.py +++ b/src/builtin_actions.py @@ -168,7 +168,6 @@ async def action_consolidate_memory(owner: str, **kwargs) -> Tuple[str, bool]: drop_items = decision.get("drop") if isinstance(decision, dict) else None if isinstance(keep_items, list) and isinstance(drop_items, list): by_id = {m.get("id"): m for m in group_memories if m.get("id")} - keep_ids = set() cleaned_by_id = {} for item in keep_items: if not isinstance(item, dict): @@ -179,7 +178,6 @@ async def action_consolidate_memory(owner: str, **kwargs) -> Tuple[str, bool]: text = (item.get("text") or "").strip() if not text: continue - keep_ids.add(mid) cleaned = { "category": (item.get("category") or by_id[mid].get("category") or "fact").strip(), } @@ -188,11 +186,20 @@ async def action_consolidate_memory(owner: str, **kwargs) -> Tuple[str, bool]: cleaned["text"] = text cleaned_by_id[mid] = cleaned - # If the model only saw a truncated memory, do not let - # that partial view delete or rewrite the full memory. - keep_ids.update(mid for mid in truncated_ids if mid in by_id) + # Delete only memories the model EXPLICITLY dropped, never + # ones it merely omitted from `keep`. Treating the + # complement of `keep` as deletions meant a model that + # forgot to re-list an id (common) silently destroyed that + # memory. Honor the explicit `drop` set instead. + drop_ids = { + d.get("id") + for d in drop_items + if isinstance(d, dict) and d.get("id") in by_id + } + # Never delete a memory the model only saw truncated. + drop_ids -= truncated_ids - if keep_ids: + if drop_ids or cleaned_by_id: changed_text = 0 group_ref_ids = {id(m) for m in group_memories} kept_all = [] @@ -201,7 +208,7 @@ async def action_consolidate_memory(owner: str, **kwargs) -> Tuple[str, bool]: kept_all.append(mem) continue mid = mem.get("id") - if mid not in keep_ids: + if mid in drop_ids: continue cleaned = cleaned_by_id.get(mid) or {} if mid in truncated_ids: @@ -213,7 +220,7 @@ async def action_consolidate_memory(owner: str, **kwargs) -> Tuple[str, bool]: mem["category"] = cleaned["category"] kept_all.append(mem) - removed = len(group_memories) - len(keep_ids) + removed = sum(1 for m in group_memories if m.get("id") in drop_ids) total_scanned += len(group_memories) if removed or changed_text: all_memories = kept_all diff --git a/tests/test_consolidate_memory_explicit_drops.py b/tests/test_consolidate_memory_explicit_drops.py new file mode 100644 index 000000000..ed9bc0234 --- /dev/null +++ b/tests/test_consolidate_memory_explicit_drops.py @@ -0,0 +1,57 @@ +"""Memory consolidation must delete only memories the model explicitly drops. + +The AI tidy path computed deletions as the complement of the model's `keep` +list, so any memory the model simply omitted (a common LLM lapse) was silently +deleted. The fix honors the explicit `drop` set, so an omitted memory survives. +""" +import asyncio +import json + +import src.builtin_actions as ba + + +class _FakeMM: + saved = None + + def __init__(self, *args, **kwargs): + pass + + def load_all(self): + return [ + {"id": "a", "owner": "alice", "text": "Likes dark roast coffee", "category": "preference"}, + {"id": "b", "owner": "alice", "text": "Likes dark roast coffee too", "category": "preference"}, + {"id": "c", "owner": "alice", "text": "Lives in Cairo", "category": "fact"}, + ] + + def save(self, entries): + _FakeMM.saved = list(entries) + + +def test_omitted_memory_survives_only_explicit_drop(monkeypatch): + import src.memory + import src.endpoint_resolver + import src.llm_core + + _FakeMM.saved = None + monkeypatch.setattr(src.memory, "MemoryManager", _FakeMM) + monkeypatch.setattr( + src.endpoint_resolver, "resolve_endpoint", + lambda kind, owner=None: ("http://x/v1", "model", {}), + ) + + async def fake_llm(**kwargs): + # Model keeps 'a', drops 'b', and OMITS 'c' entirely. + return json.dumps({ + "keep": [{"id": "a", "text": "Likes dark roast coffee", "category": "preference"}], + "drop": [{"id": "b", "reason": "duplicate of a"}], + }) + + monkeypatch.setattr(src.llm_core, "llm_call_async", fake_llm) + + msg, ok = asyncio.run(ba.action_consolidate_memory("alice")) + + assert ok, msg + ids = {m["id"] for m in _FakeMM.saved} + assert "c" in ids, "omitted memory must NOT be deleted" + assert "a" in ids + assert "b" not in ids, "explicitly dropped memory should be removed"