fix(journey): route memory edit/delete through MemoryStore._mutate
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
This commit is contained in:
@@ -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)
|
||||
|
||||
@@ -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"
|
||||
|
||||
@@ -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:
|
||||
|
||||
Reference in New Issue
Block a user