From d71284194b8eb72935c67fe0fb0daf9b1f9f2c90 Mon Sep 17 00:00:00 2001
From: Mazen Tamer Salah <78306991+mazen-salah@users.noreply.github.com>
Date: Mon, 8 Jun 2026 19:54:45 +0300
Subject: [PATCH] fix(memory): only delete memories the model explicitly drops
 in tidy (#3455)
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

* fix(memory): only delete memories the model explicitly drops in tidy

The AI memory-tidy path computed deletions as the complement of the model's
`keep` list (`if mid not in keep_ids: continue`). When the model returned a
valid response that simply omitted some existing ids — a common LLM lapse — every
omitted memory was silently deleted, even though it was neither a duplicate nor
listed in `drop`.

Honor the explicit `drop` set instead: delete only ids the model dropped (minus
any it saw only truncated), and preserve everything else, still applying cleaned
text/category from `keep`.

Adds tests/test_consolidate_memory_explicit_drops.py: a memory the model omits
from both keep and drop survives; an explicitly dropped one is removed.

* refactor(memory): remove now-dead keep_ids from tidy

After deletion switched to drop_ids and text/category rewrites to cleaned_by_id,
keep_ids was written but never read. Remove the init, the .add(mid) in the keep
loop, and the truncated .update() (its truncated-protection is already covered by
`drop_ids -= truncated_ids`). Pure deletion, no behavior change; tests stay green.

Addresses review feedback on #3455.

---------

Co-authored-by: Kenny Van de Maele <kenny@kvandemaele.be>
---
 src/builtin_actions.py                        | 23 +++++---
 .../test_consolidate_memory_explicit_drops.py | 57 +++++++++++++++++++
 2 files changed, 72 insertions(+), 8 deletions(-)
 create mode 100644 tests/test_consolidate_memory_explicit_drops.py

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"