From 43d3d4e851479fab9e3bee7d8d209cd308fe782d Mon Sep 17 00:00:00 2001 From: brooklyn! Date: Thu, 24 Sep 2026 04:41:23 -0500 Subject: [PATCH] fix(journey): route memory edit/delete through MemoryStore._mutate MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A Journey memory edit or delete (Desktop panel, `hermes journey`, TUI `learning.*`) rewrote the whole MEMORY.md / USER.md from an unlocked snapshot via `_read_file` + `_write_file`: a memory the agent stored in between was dropped, and hand-edited content that does not round-trip through the § parser was reformatted with no .bak. Both mutations now go through `MemoryStore._mutate` — the memory tool's cross-process lock, re-read under lock and drift guard. The node id is resolved to its entry text inside the lock and matched by exact text against the re-read entries; a vanished target, drift or an unreadable file is refused with the store's own message instead of written over. Router/CLI/TUI response shapes are unchanged. Fixes #119668 --- agent/learning_mutations.py | 47 +++++++++----- tests/agent/test_learning_mutations.py | 89 ++++++++++++++++++++++++++ tools/memory_tool_store.py | 5 +- 3 files changed, 124 insertions(+), 17 deletions(-) diff --git a/agent/learning_mutations.py b/agent/learning_mutations.py index 2ee948af1b..05c2b69d61 100644 --- a/agent/learning_mutations.py +++ b/agent/learning_mutations.py @@ -5,7 +5,7 @@ Node ids (from ``agent.learning_graph``): skills → the skill name; memories for USER.md; ``index`` = position in the combined card list, MEMORY.md first). Shared by CLI ``hermes journey``, the TUI ``/journey`` overlay and the desktop. Deleting a skill *archives* it (``hermes curator restore`` recovers it); -deleting a memory rewrites its file. +deleting a memory rewrites its file under the memory tool's lock. """ from __future__ import annotations @@ -14,6 +14,7 @@ from pathlib import Path from typing import Any, Callable _MEMORY_FILES = {"memory": "MEMORY.md", "profile": "USER.md"} +_STORE_TARGETS = {"memory": "memory", "profile": "user"} # journey source -> MemoryStore target def parse_node_kind(node_id: str) -> str: @@ -35,7 +36,8 @@ def _locate_memory(node_id: str) -> tuple[Path, list[str], int]: """Resolve a memory node id to (file, all §-delimited entries, local index). Entries come from ``MemoryStore._read_file`` — the memory tool's own parser — so journey indices stay aligned with what the graph renders; a profile card's - local index is its global index minus the MEMORY.md card count.""" + local index is its global index minus the MEMORY.md card count. Read-only view: + mutations resolve the id again INSIDE ``_mutate_memory``'s lock.""" from hermes_constants import get_hermes_home from agent.learning_graph import _memory_cards from tools.memory_tool import MemoryStore @@ -56,11 +58,32 @@ def _locate_memory(node_id: str) -> tuple[Path, list[str], int]: return path, chunks, local -def _write_memory(path: Path, chunks: list[str]) -> None: - """Atomic temp-file + rename via the memory tool, so a concurrent reader - never sees a half-written file (and the §-join stays single-sourced).""" - from tools.memory_tool import MemoryStore - MemoryStore._write_file(path, [c.strip() for c in chunks if c.strip()]) +def _mutate_memory(node_id: str, replacement: str | None) -> dict[str, Any]: + """Replace (or, with ``replacement=None``, remove) the entry *node_id* names, through + ``MemoryStore._mutate`` — the memory tool's cross-process lock, re-read under lock and + drift guard (``.bak`` snapshot + refusal when the file wouldn't round-trip). The file is + shared with the live agent, so a read-modify-write from an unlocked snapshot silently + dropped whatever the agent stored in between and reformatted hand-edited files + (#119668). The id is resolved to its entry text INSIDE the lock and matched by exact + text against the store's re-read entries; a target gone under the lock is refused.""" + from tools.memory_tool import load_on_disk_store + + source, _ = _parse_memory_id(node_id) + name = _MEMORY_FILES[source] + message = f"deleted memory from {name}" if replacement is None else f"updated memory in {name}" + + def _apply(entries, _limit): + _, chunks, local = _locate_memory(node_id) + text = chunks[local].strip() + if text not in entries: + return {"success": False, "error": "memory node id is stale — refresh the graph"} + idx = entries.index(text) + return entries[:idx] + ([] if replacement is None else [replacement]) + entries[idx + 1:], message + + result = load_on_disk_store()._mutate(_STORE_TARGETS[source], _apply) + if not result.get("success"): + return {"ok": False, "message": result.get("error", f"{name} write failed")} + return {"ok": True, "message": message} def _clear_skill_cache() -> None: @@ -125,10 +148,7 @@ def _delete_skill(name: str) -> dict[str, Any]: def _delete_memory(node_id: str) -> dict[str, Any]: - path, chunks, local = _locate_memory(node_id) - del chunks[local] - _write_memory(path, chunks) - return {"ok": True, "message": f"deleted memory from {path.name}"} + return _mutate_memory(node_id, None) # ── Edit ──────────────────────────────────────────────────────────────────── @@ -151,7 +171,4 @@ def _edit_memory(node_id: str, content: str) -> dict[str, Any]: body = content.strip() if not body: return {"ok": False, "message": "empty memory — use delete to remove it"} - path, chunks, local = _locate_memory(node_id) - chunks[local] = body - _write_memory(path, chunks) - return {"ok": True, "message": f"updated memory in {path.name}"} + return _mutate_memory(node_id, body) diff --git a/tests/agent/test_learning_mutations.py b/tests/agent/test_learning_mutations.py index 7d2ef8d963..2927d6f58f 100644 --- a/tests/agent/test_learning_mutations.py +++ b/tests/agent/test_learning_mutations.py @@ -6,6 +6,8 @@ against a temp HERMES_HOME, never mocks — the id→file mapping is the whole p from __future__ import annotations +import threading + import pytest from agent import learning_mutations as lm @@ -91,3 +93,90 @@ def test_memory_writes_match_memory_tool_format(home): assert entries == ["alpha rewritten", "beta note"] assert path.read_text(encoding="utf-8") == ENTRY_DELIMITER.join(entries) + + +# ── Locking / drift (issue #119668) ───────────────────────────────────────── +# A Journey mutation shares MEMORY.md with the live agent's memory tool, so it must +# take the same lock, re-read under it and honour the drift guard — otherwise a +# memory the agent stored in the meantime is rewritten away from a stale snapshot. + + +def _race_memory_tool_add(monkeypatch, content: str) -> threading.Thread: + """Start a lock-respecting ``memory_tool`` writer that appends *content* the + moment the Journey mutation resolves its node id (the read half of its + read-modify-write), then give it a moment to land. Under a correct lock the + writer blocks until the mutation has written; without one it interleaves and + the mutation's write clobbers it.""" + from agent import learning_graph + from tools.memory_tool import MemoryStore + + located, landed = threading.Event(), threading.Event() + real_cards = learning_graph._memory_cards + + def _cards_then_let_writer_in(): + cards = real_cards() + located.set() + landed.wait(timeout=0.5) + return cards + + def _writer(): + located.wait(timeout=5) + MemoryStore().add("memory", content) + landed.set() + + monkeypatch.setattr(learning_graph, "_memory_cards", _cards_then_let_writer_in) + thread = threading.Thread(target=_writer, daemon=True) + thread.start() + return thread + + +def test_delete_memory_keeps_concurrent_memory_tool_add(home, monkeypatch): + from tools.memory_tool import MemoryStore + + writer = _race_memory_tool_add(monkeypatch, "gamma note") + assert lm.delete_node("memory:memory:0")["ok"] + writer.join(timeout=5) + + assert MemoryStore._read_file(home / "memories" / "MEMORY.md") == ["beta note", "gamma note"] + + +def test_edit_memory_keeps_concurrent_memory_tool_add(home, monkeypatch): + from tools.memory_tool import MemoryStore + + writer = _race_memory_tool_add(monkeypatch, "gamma note") + assert lm.edit_node("memory:memory:0", "alpha rewritten")["ok"] + writer.join(timeout=5) + + assert MemoryStore._read_file(home / "memories" / "MEMORY.md") == ["alpha rewritten", "beta note", "gamma note"] + + +@pytest.mark.parametrize("mutate", [lambda: lm.delete_node("memory:memory:0"), + lambda: lm.edit_node("memory:memory:0", "alpha rewritten")], + ids=["delete", "edit"]) +def test_memory_drift_is_refused_with_backup_like_memory_tool(home, mutate): + """Hand-edited content that wouldn't round-trip through the § parser is what + the memory tool's drift guard exists for: snapshot to .bak, refuse, leave the + file untouched. A Journey mutation must not reformat it silently.""" + from tools.memory_tool_store import _drift_error + + path = home / "memories" / "MEMORY.md" + raw = "alpha note\n§\n\n§\nbeta note\n" + path.write_text(raw, encoding="utf-8") + + res = mutate() + + assert not res["ok"] + assert path.read_text(encoding="utf-8") == raw + (backup,) = home.glob("memories/MEMORY.md.bak.*") + assert backup.read_text(encoding="utf-8") == raw + assert res["message"] == _drift_error(path, str(backup))["error"] + + +def test_unreadable_memory_file_is_refused_unchanged(home): + path = home / "memories" / "MEMORY.md" + path.write_bytes(b"alpha note\n\xc3\x28\n\xc2\xa7\nbeta") + + res = lm.delete_node("memory:memory:0") + + assert not res["ok"] and "could not be read" in res["message"] + assert path.read_bytes() == b"alpha note\n\xc3\x28\n\xc2\xa7\nbeta" diff --git a/tools/memory_tool_store.py b/tools/memory_tool_store.py index 2e29c83767..d340333ee1 100644 --- a/tools/memory_tool_store.py +++ b/tools/memory_tool_store.py @@ -462,8 +462,9 @@ class MemoryStore: @staticmethod def _write_file(path: Path, entries: List[str]): - """Atomic temp-file + rename: readers never see a truncated file. Also used by - agent/learning_mutations.py.""" + """Atomic temp-file + rename: readers never see a truncated file. Callers + hold ``_file_lock`` (via ``_mutate``): a bare write from an earlier snapshot + drops concurrent entries (#119668).""" try: atomic_write_text(path, ENTRY_DELIMITER.join(entries), tmp_prefix=".mem_") except OSError as e: