fix(memory): don't wipe MEMORY.md when a read-modify-write reads it as unreadable

`_read_file` degraded any read failure to `[]`, conflating "file exists but
couldn't be read" with "empty store". That is a silent, total data-loss bug on
the `add` path.

`add` re-reads the file under lock, appends the new entry, and rewrites the
WHOLE file from the parsed entries. It deliberately skips the drift guard
(#42874: "appending never clobbers existing content") — but that reasoning only
holds when the reload actually saw the file. When `read_text` raises
(an external editor momentarily holding the file on Windows, a permission
change, a filesystem/EINTR blip), `_read_file` returns `[]`, so `add` treats
the store as empty and rewrites the file down to just the new entry — every
prior memory gone — while returning `success: True`.

Reproduced with a transient read failure during `add`:

    entries on disk before : 3   (dark-mode pref, deadline, deploy target)
    add("A brand new fact") : success=True
    entries on disk after  : 1   ("A brand new fact")   <-- the other 2 wiped

replace/remove/apply_batch were shielded only incidentally — an empty view
means `old_text` never matches, so they abort before writing — but they still
returned a misleading "no entry matched" instead of naming the real problem.

Fix: distinguish unreadable from empty. `_read_entries_checked` returns
`(entries, read_ok)`, with `read_ok=False` only when the file exists but can't
be read; absent/empty stays a clean `([], True)`. `_reload_target` returns a
`_READ_FAILED` sentinel in that case without touching in-memory state, and all
four mutation paths (add, replace, remove, apply_batch) refuse the write with a
clear "retry in a moment" error. This is the same posture as the drift guard
and the pairing/checkpoint fixes: never rewrite a file from a view that isn't
the real one. `_read_file` keeps its `[]`-on-error contract for the read-only
`load_from_disk` caller, which never persists.

tests/tools/test_memory_tool.py: new TestUnreadableFileDoesNotWipeMemory —
add/replace/remove/apply_batch all refuse and leave the file byte-identical on
a transient read failure, plus controls that an absent file is still a clean
empty store and the happy path is undisturbed. The four refusal tests fail on
main. Full suite: 90 passed, 1 pre-existing failure (`test_deduplication_on_load`,
a UnicodeDecodeError unrelated to this change, identical on clean main).

tests/tools/test_memory_tool.py

@@ -829,6 +829,112 @@ class TestExternalDriftGuard:
         assert ".bak." in r2["drift_backup"]
 
 
+class TestUnreadableFileDoesNotWipeMemory:
+    """A file that exists but can't be read must NOT be treated as empty.
+
+    ``_read_file`` degraded a failed read to ``[]``, conflating "unreadable"
+    with "empty store". ``add`` rewrites the whole file from the parsed entries,
+    so a transient read failure (an external editor holding the file on Windows,
+    a permission blip, an I/O error) turned an append into a full-file rewrite
+    down to a single entry — silently wiping every prior memory while returning
+    success. replace/remove/apply_batch were shielded only incidentally (an
+    empty view means no match, so they abort); this pins the guarantee for all
+    of them explicitly.
+    """
+
+    @staticmethod
+    def _fail_read_once(monkeypatch, path):
+        """Make ``path.read_text`` raise OSError exactly once, else pass through."""
+        real = Path.read_text
+        state = {"failed": False}
+
+        def flaky(self, *a, **k):
+            if self == path and not state["failed"]:
+                state["failed"] = True
+                raise OSError("transient: file temporarily unavailable")
+            return real(self, *a, **k)
+
+        monkeypatch.setattr(Path, "read_text", flaky)
+
+    def test_add_refuses_and_preserves_memory_on_read_failure(
+        self, store, monkeypatch,
+    ):
+        store.add("memory", "User prefers dark mode.")
+        store.add("memory", "Deploy target is Ubuntu 24.04.")
+        path = store._path_for("memory")
+        before = path.read_text(encoding="utf-8")
+
+        self._fail_read_once(monkeypatch, path)
+        result = store.add("memory", "A brand new fact.")
+
+        # Refused, not a false success — and nothing on disk changed.
+        assert result["success"] is False
+        assert "could not be read" in result["error"]
+        assert path.read_text(encoding="utf-8") == before
+        assert "dark mode" in path.read_text(encoding="utf-8")
+        assert "Ubuntu 24.04" in path.read_text(encoding="utf-8")
+
+    def test_replace_reports_read_failure_not_missing_entry(
+        self, store, monkeypatch,
+    ):
+        store.add("memory", "Entry to replace later.")
+        path = store._path_for("memory")
+        before = path.read_text(encoding="utf-8")
+
+        self._fail_read_once(monkeypatch, path)
+        result = store.replace("memory", "Entry to replace", "New value.")
+
+        assert result["success"] is False
+        # The distinct read-failure error, NOT the confusing "no entry matched".
+        assert "could not be read" in result["error"]
+        assert path.read_text(encoding="utf-8") == before
+
+    def test_remove_refuses_on_read_failure(self, store, monkeypatch):
+        store.add("memory", "Keep me safe.")
+        path = store._path_for("memory")
+        before = path.read_text(encoding="utf-8")
+
+        self._fail_read_once(monkeypatch, path)
+        result = store.remove("memory", "Keep me safe")
+
+        assert result["success"] is False
+        assert "could not be read" in result["error"]
+        assert path.read_text(encoding="utf-8") == before
+
+    def test_apply_batch_refuses_on_read_failure(self, store, monkeypatch):
+        store.add("memory", "Original batch entry.")
+        path = store._path_for("memory")
+        before = path.read_text(encoding="utf-8")
+
+        self._fail_read_once(monkeypatch, path)
+        result = store.apply_batch(
+            "memory", [{"action": "add", "content": "batched addition"}]
+        )
+
+        assert result["success"] is False
+        assert path.read_text(encoding="utf-8") == before
+
+    def test_absent_file_is_still_a_clean_empty_store(self, store):
+        """A genuinely missing file must NOT be mistaken for a read failure."""
+        path = store._path_for("memory")
+        if path.exists():
+            path.unlink()
+
+        result = store.add("memory", "First entry ever.")
+
+        assert result["success"] is True
+        assert "First entry ever." in path.read_text(encoding="utf-8")
+
+    def test_normal_add_still_works_when_read_succeeds(self, store):
+        """Control: the guard does not disturb the happy path."""
+        store.add("memory", "Fact one.")
+        result = store.add("memory", "Fact two.")
+        assert result["success"] is True
+        path = store._path_for("memory")
+        assert "Fact one." in path.read_text(encoding="utf-8")
+        assert "Fact two." in path.read_text(encoding="utf-8")
+
+
 # =========================================================================
 # Load-time snapshot sanitization — promptware defense (#496)
 #

tools/memory_tool.py

@@ -31,7 +31,7 @@ import time
 from contextlib import contextmanager
 from pathlib import Path
 from hermes_constants import get_hermes_home
-from typing import Dict, Any, List, Optional
+from typing import Dict, Any, List, Optional, Tuple
 
 from utils import atomic_replace
 
@@ -120,6 +120,32 @@ def _drift_error(path: "Path", bak_path: str) -> Dict[str, Any]:
     }
 
 
+# Sentinel returned by ``_reload_target`` when the target file EXISTS but could
+# not be read. Distinct from a drift-backup path (``str``) and from a clean
+# reload (``None``): the caller must abort the mutation rather than persist over
+# an unreadable file.
+_READ_FAILED = object()
+
+
+def _read_failed_error(path: "Path") -> Dict[str, Any]:
+    """Build the error dict returned when the on-disk memory file is unreadable.
+
+    A file that exists but cannot be read is NOT an empty store. Reading it as
+    ``[]`` and then persisting would rewrite the whole file from an empty entry
+    list — wiping the user's memory. We refuse the write so nothing is lost.
+    """
+    return {
+        "success": False,
+        "error": (
+            f"Refusing to write {path.name}: the file exists on disk but could "
+            f"not be read right now (temporarily locked by another program, a "
+            f"permission change, or a filesystem error). Treating an unreadable "
+            f"file as empty and saving would wipe existing memory, so the write "
+            f"is refused. Nothing was changed — retry in a moment."
+        ),
+    }
+
+
 class MemoryStore:
     """
     Bounded curated memory with file persistence. One instance per AIAgent.
@@ -294,7 +320,7 @@ class MemoryStore:
             return mem_dir / "USER.md"
         return mem_dir / "MEMORY.md"
 
-    def _reload_target(self, target: str, *, skip_drift: bool = False) -> Optional[str]:
+    def _reload_target(self, target: str, *, skip_drift: bool = False):
         """Re-read entries from disk into in-memory state.
 
         Called under file lock to get the latest state before mutating.
@@ -303,15 +329,27 @@ class MemoryStore:
         parser/serializer, OR an entry larger than the store's char limit).
         When drift is detected the caller must abort the mutation —
         flushing would discard the un-roundtrippable content.
-        Returns None on clean reload.
+        Returns ``None`` on clean reload.
+
+        Returns the ``_READ_FAILED`` sentinel when the file EXISTS but could not
+        be read. The caller MUST abort: the on-disk entries are unknown, so
+        overwriting from an assumed-empty view would wipe them. This is the real
+        exposure behind ``add`` — it skips the drift guard because appending is
+        safe, but that reasoning only holds when the reload actually saw the
+        file. A failed read reported as ``[]`` turned ``add`` into a full-file
+        rewrite down to a single entry.
 
         When *skip_drift* is True the round-trip / entry-size check is
         bypassed.  Used by the ``add`` action which appends without
         rewriting, so existing content is never clobbered.
         """
         path = self._path_for(target)
+        fresh, read_ok = self._read_entries_checked(path)
+        if not read_ok:
+            # Leave in-memory entries untouched and tell the caller to abort;
+            # persisting over an unreadable file would destroy it.
+            return _READ_FAILED
         bak = None if skip_drift else self._detect_external_drift(target)
-        fresh = self._read_file(path)
         fresh = list(dict.fromkeys(fresh))  # deduplicate
         self._set_entries(target, fresh)
         return bak
@@ -361,7 +399,14 @@ class MemoryStore:
             # tool-written entries in the same session are harmless.  The drift
             # guard remains active for replace/remove where full-file rewrite
             # would discard un-roundtrippable content (issue #26045).
-            self._reload_target(target, skip_drift=True)
+            #
+            # But "append never clobbers" only holds when the reload actually
+            # read the file. add rewrites the WHOLE file from the parsed
+            # entries, so a file that exists but read as empty (transient lock,
+            # permission blip, I/O error) would be rewritten down to just the
+            # new entry — wiping every prior memory. Refuse instead.
+            if self._reload_target(target, skip_drift=True) is _READ_FAILED:
+                return _read_failed_error(self._path_for(target))
 
             entries = self._entries_for(target)
             limit = self._char_limit(target)
@@ -411,6 +456,8 @@ class MemoryStore:
 
         with self._file_lock(self._path_for(target)):
             bak = self._reload_target(target)
+            if bak is _READ_FAILED:
+                return _read_failed_error(self._path_for(target))
             if bak:
                 return _drift_error(self._path_for(target), bak)
 
@@ -472,6 +519,8 @@ class MemoryStore:
 
         with self._file_lock(self._path_for(target)):
             bak = self._reload_target(target)
+            if bak is _READ_FAILED:
+                return _read_failed_error(self._path_for(target))
             if bak:
                 return _drift_error(self._path_for(target), bak)
 
@@ -532,6 +581,8 @@ class MemoryStore:
 
         with self._file_lock(self._path_for(target)):
             bak = self._reload_target(target)
+            if bak is _READ_FAILED:
+                return _read_failed_error(self._path_for(target))
             if bak:
                 return _drift_error(self._path_for(target), bak)
 
@@ -690,26 +741,46 @@ class MemoryStore:
         return f"{separator}\n{header}\n{separator}\n{content}"
 
     @staticmethod
-    def _read_file(path: Path) -> List[str]:
-        """Read a memory file and split into entries.
+    def _read_entries_checked(path: Path) -> Tuple[List[str], bool]:
+        """Read + parse a memory file, distinguishing unreadable from empty.
+
+        Returns ``(entries, read_ok)``. ``read_ok`` is False ONLY when the file
+        EXISTS but could not be read — an absent or empty file is a clean
+        ``([], True)``. Read-modify-write callers must treat ``read_ok=False``
+        as "abort" rather than "empty store", or a transient read failure would
+        let them persist over — and wipe — the on-disk memory (issue #26045 is
+        about the same class: never rewrite a file from a view that isn't the
+        real one).
 
         No file locking needed: _write_file uses atomic rename, so readers
         always see either the previous complete file or the new complete file.
         """
         if not path.exists():
-            return []
+            return [], True
         try:
             raw = path.read_text(encoding="utf-8")
         except (OSError, IOError):
-            return []
+            return [], False
 
         if not raw.strip():
-            return []
+            return [], True
 
         # Use ENTRY_DELIMITER for consistency with _write_file. Splitting by "§"
         # alone would incorrectly split entries that contain "§" in their content.
         entries = [e.strip() for e in raw.split(ENTRY_DELIMITER)]
-        return [e for e in entries if e]
+        return [e for e in entries if e], True
+
+    @staticmethod
+    def _read_file(path: Path) -> List[str]:
+        """Read a memory file and split into entries (empty list on any error).
+
+        Retained for read-only callers (``load_from_disk``) that build in-memory
+        state without persisting; a failed read degrading to ``[]`` there is
+        harmless because nothing is written back. Read-modify-write paths use
+        ``_read_entries_checked`` so they can refuse to overwrite an unreadable
+        file — see ``_reload_target``.
+        """
+        return MemoryStore._read_entries_checked(path)[0]
 
     def _detect_external_drift(self, target: str) -> Optional[str]:
         """Return a backup-path string if on-disk content shows external drift.