From 8a9ab8b56b8c37ab8741e03fe6f2d280a787737c Mon Sep 17 00:00:00 2001 From: briandevans <252620095+briandevans@users.noreply.github.com> Date: Mon, 27 Jul 2026 16:58:29 -0700 Subject: [PATCH] fix(cli): stop shredding an existing MEMORY.md on hermes import-agent MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit memories/MEMORY.md is the "§"-delimited store written by MemoryStore, not a markdown document. parse_existing_memory_entries() fell back to extract_markdown_entries() -- the *source* parser for CLAUDE.md / AGENTS.md -- whenever the destination held no delimiter, which is exactly the case for a single-entry store or one that was hand-edited or shell-appended. That extractor skips fenced code blocks, skips table rows, splits a block into one entry per bullet and reflows paragraphs. The shredded result was then written straight back over the user's store and reported as "Imported", with no backup to recover from. Parse the destination the way MemoryStore._parse_entries does: split on ENTRY_DELIMITER only, so a store with no delimiter is one intact entry. extract_markdown_entries() is unchanged and still used on the sources, where it is correct. Also restore the safety net the port dropped. The openclaw migration script this module was ported from calls maybe_backup(destination) before rewriting a memory store; the port did not. Snapshot the store to .bak. (same naming as MemoryStore._backup_drifted_file), refuse to rewrite when the snapshot fails, and write via temp file + atomic rename so an interrupted import cannot leave a truncated store and a symlinked MEMORY.md stays a symlink. The identical fallback lives in openclaw_to_hermes.py, where it is reached from migrate_memory() (memories/MEMORY.md and memories/USER.md) and migrate_daily_memory(); fixed there too. --- hermes_cli/agent_import.py | 79 ++++++++++++++++-- .../scripts/openclaw_to_hermes.py | 13 ++- tests/hermes_cli/test_agent_import.py | 82 +++++++++++++++++++ tests/skills/test_openclaw_migration.py | 26 ++++++ 4 files changed, 192 insertions(+), 8 deletions(-) diff --git a/hermes_cli/agent_import.py b/hermes_cli/agent_import.py index bd686e59348ce..38e743a1fa33b 100644 --- a/hermes_cli/agent_import.py +++ b/hermes_cli/agent_import.py @@ -40,12 +40,17 @@ from __future__ import annotations import json import logging +import os import re import shutil import sys +import tempfile +import time from pathlib import Path from typing import Any, Dict, List, Optional, Sequence, Tuple +from utils import atomic_replace + logger = logging.getLogger(__name__) # Same entry delimiter as the Hermes memory store and the openclaw migration @@ -205,14 +210,68 @@ def extract_markdown_entries(text: str) -> List[str]: def parse_existing_memory_entries(path: Path) -> List[str]: + """Parse the DESTINATION memory store into entries. + + ``memories/MEMORY.md`` is the entry-delimited store written by + ``MemoryStore._write_file`` (tools/memory_tool.py), not a markdown + document, so this splits on ``ENTRY_DELIMITER`` only — exactly what + ``MemoryStore._parse_entries`` does. A store with no delimiter (a single + entry, or one that was hand-edited / shell-appended) is therefore ONE + intact entry. + + Do NOT fall back to :func:`extract_markdown_entries` here. That extractor + is correct for CLAUDE.md / AGENTS.md *sources*, but it drops fenced code + blocks and table rows and splits a block into one entry per bullet — and + the merged result is written straight back over the user's store, so the + loss is permanent. + """ if not path.exists(): return [] raw = read_text(path) if not raw.strip(): return [] - if ENTRY_DELIMITER in raw: - return [e.strip() for e in raw.split(ENTRY_DELIMITER) if e.strip()] - return extract_markdown_entries(raw) + return [e.strip() for e in raw.split(ENTRY_DELIMITER) if e.strip()] + + +def backup_memory_file(path: Path) -> Optional[Path]: + """Snapshot ``path`` before a destructive rewrite; return the backup path. + + Restores parity with the openclaw migration script this module was ported + from, which calls ``maybe_backup(destination)`` before rewriting a memory + store. Uses the same ``.bak.`` naming as + ``MemoryStore._backup_drifted_file``. Returns None when there is nothing + to back up. + """ + if not path.exists(): + return None + backup = path.with_suffix(path.suffix + f".bak.{int(time.time())}") + shutil.copy2(path, backup) + return backup + + +def atomic_write_text(path: Path, content: str) -> None: + """Write ``content`` to ``path`` via temp file + atomic rename. + + Mirrors ``MemoryStore._write_file``: an interrupted or failed write can + never leave a truncated memory store on disk, and readers always see + either the old complete file or the new one. ``atomic_replace`` also + keeps a symlinked destination a symlink. + """ + fd, tmp_path = tempfile.mkstemp( + dir=str(path.parent), suffix=".tmp", prefix=".import_" + ) + try: + with os.fdopen(fd, "w", encoding="utf-8") as handle: + handle.write(content) + handle.flush() + os.fsync(handle.fileno()) + atomic_replace(tmp_path, path) + except BaseException: + try: + os.unlink(tmp_path) + except OSError: + pass + raise def merge_entries( @@ -513,9 +572,19 @@ class AgentImporter: return if self.execute: destination.parent.mkdir(parents=True, exist_ok=True) - destination.write_text( + try: + backup = backup_memory_file(destination) + except OSError as exc: + # Never rewrite the store when the safety net failed. + self.record(kind, source, destination, "error", + f"Could not back up existing memory file: {exc}", + **details) + return + if backup is not None: + details["backup"] = str(backup) + atomic_write_text( + destination, ENTRY_DELIMITER.join(merged) + ("\n" if merged else ""), - encoding="utf-8", ) self.record(kind, source, destination, "imported", **details) else: diff --git a/optional-skills/migration/openclaw-migration/scripts/openclaw_to_hermes.py b/optional-skills/migration/openclaw-migration/scripts/openclaw_to_hermes.py index 7f6e9b3fb52e9..62cae3f6d8b91 100644 --- a/optional-skills/migration/openclaw-migration/scripts/openclaw_to_hermes.py +++ b/optional-skills/migration/openclaw-migration/scripts/openclaw_to_hermes.py @@ -450,14 +450,21 @@ def rebrand_text(text: str) -> str: def parse_existing_memory_entries(path: Path) -> List[str]: + """Parse a DESTINATION Hermes memory store (memories/MEMORY.md, USER.md). + + Splits on ``ENTRY_DELIMITER`` only, matching ``MemoryStore._parse_entries`` + in ``tools/memory_tool.py``: a store with no delimiter is ONE intact entry. + Do NOT fall back to :func:`extract_markdown_entries` — it is the *source* + parser (workspace/MEMORY.md and friends), it drops fenced code blocks and + table rows and splits a block into one entry per bullet, and the merged + result is written back over the destination. + """ if not path.exists(): return [] raw = read_text(path) if not raw.strip(): return [] - if ENTRY_DELIMITER in raw: - return [e.strip() for e in raw.split(ENTRY_DELIMITER) if e.strip()] - return extract_markdown_entries(raw) + return [e.strip() for e in raw.split(ENTRY_DELIMITER) if e.strip()] def extract_markdown_entries(text: str) -> List[str]: diff --git a/tests/hermes_cli/test_agent_import.py b/tests/hermes_cli/test_agent_import.py index b2b8dd462115b..268121e777bbf 100644 --- a/tests/hermes_cli/test_agent_import.py +++ b/tests/hermes_cli/test_agent_import.py @@ -17,11 +17,13 @@ import pytest import yaml from hermes_cli.agent_import import ( + ENTRY_DELIMITER, AgentImporter, claude_rule_to_command_pattern, detect_agents, extract_markdown_entries, is_secret_key, + parse_existing_memory_entries, sanitize_mcp_env, ) @@ -486,6 +488,86 @@ class TestMergeSemantics: assert memory_items[0]["status"] == "skipped" +# --------------------------------------------------------------------------- +# The DESTINATION memories/MEMORY.md is a §-delimited store, not a document +# --------------------------------------------------------------------------- + +# A realistic hand-edited store: one entry, no "§", with a fenced code block +# and a markdown table — exactly the content extract_markdown_entries() drops. +EXISTING_MEMORY = """Homelab runbook. Restart the ingress controller with: + +```bash +kubectl -n ingress rollout restart deploy/nginx +``` + +Escalation ladder: + +| Severity | Contact | Window | +|----------|---------|--------| +| SEV1 | on-call | 15m | +| SEV2 | #ops | 4h | + +Never page for SEV3. +""" + + +class TestExistingMemoryStorePreserved: + """An import must not shred the memory store it merges into. + + ``memories/MEMORY.md`` is the entry-delimited store written by + ``MemoryStore._write_file``; a single-entry or hand-edited store contains + no ``§`` delimiter. Parsing it with the *source* markdown extractor drops + code blocks and table rows and splits one entry into fragments, and the + merged result is written straight back over the file. + """ + + @pytest.fixture() + def seeded_home(self, hermes_home): + memory = hermes_home / "memories" / "MEMORY.md" + memory.parent.mkdir(parents=True, exist_ok=True) + memory.write_text(EXISTING_MEMORY, encoding="utf-8") + return hermes_home + + def test_undelimited_store_is_one_entry(self, seeded_home): + path = seeded_home / "memories" / "MEMORY.md" + assert ENTRY_DELIMITER not in path.read_text(encoding="utf-8") + assert parse_existing_memory_entries(path) == [EXISTING_MEMORY.strip()] + + def test_agrees_with_memory_store_parser(self, seeded_home): + from tools.memory_tool import MemoryStore + + path = seeded_home / "memories" / "MEMORY.md" + raw = path.read_text(encoding="utf-8") + assert parse_existing_memory_entries(path) == MemoryStore._parse_entries(raw) + + def test_import_preserves_existing_entry_verbatim( + self, claude_tree, seeded_home): + path = seeded_home / "memories" / "MEMORY.md" + run_import("claude-code", claude_tree, seeded_home, execute=True) + entries = path.read_text(encoding="utf-8").split(ENTRY_DELIMITER) + # The pre-existing store survives byte-intact as a SINGLE entry ... + assert entries[0] == EXISTING_MEMORY.strip() + # ... including the parts the markdown extractor would have dropped. + assert "kubectl -n ingress rollout restart deploy/nginx" in entries[0] + assert "| SEV1 | on-call | 15m |" in entries[0] + # ... and the imported entries are still appended after it. + assert any("type hints" in e for e in entries[1:]) + + def test_import_backs_up_the_previous_store(self, claude_tree, seeded_home): + memories = seeded_home / "memories" + run_import("claude-code", claude_tree, seeded_home, execute=True) + backups = sorted(memories.glob("MEMORY.md.bak.*")) + assert len(backups) == 1 + assert backups[0].read_text(encoding="utf-8") == EXISTING_MEMORY + + def test_dry_run_leaves_the_store_untouched(self, claude_tree, seeded_home): + memories = seeded_home / "memories" + run_import("claude-code", claude_tree, seeded_home, execute=False) + assert (memories / "MEMORY.md").read_text( + encoding="utf-8") == EXISTING_MEMORY + assert not list(memories.glob("MEMORY.md.bak.*")) + + # --------------------------------------------------------------------------- # CLI wiring # --------------------------------------------------------------------------- diff --git a/tests/skills/test_openclaw_migration.py b/tests/skills/test_openclaw_migration.py index e75dad8792a2a..bc7ee92e25a15 100644 --- a/tests/skills/test_openclaw_migration.py +++ b/tests/skills/test_openclaw_migration.py @@ -56,6 +56,32 @@ def test_extract_markdown_entries_promotes_heading_context(): assert "Tyler Williams > Active Projects: Hermes Agent" in entries +def test_parse_existing_memory_entries_keeps_undelimited_store_intact(tmp_path): + """The DESTINATION store is §-delimited, not a markdown document. + + ``migrate_memory`` and ``migrate_daily_memory`` read the Hermes-side + memories/MEMORY.md (and USER.md) and write the merged result back over it. + A store with no delimiter is ONE entry — running the source markdown + extractor over it would drop the code block and the table row below. + """ + mod = load_module() + raw = ( + "Homelab runbook. Restart the ingress controller with:\n" + "\n" + "```bash\n" + "kubectl -n ingress rollout restart deploy/nginx\n" + "```\n" + "\n" + "| Severity | Contact | Window |\n" + "| SEV1 | on-call | 15m |\n" + ) + path = tmp_path / "MEMORY.md" + path.write_text(raw, encoding="utf-8") + + assert mod.ENTRY_DELIMITER not in raw + assert mod.parse_existing_memory_entries(path) == [raw.strip()] + + def test_merge_entries_respects_limit_and_reports_overflow(): mod = load_module() existing = ["alpha"]