From 9d6ef41a53d2886e5e33ab4f85e244df88f68299 Mon Sep 17 00:00:00 2001 From: kshitijk4poor <82637225+kshitijk4poor@users.noreply.github.com> Date: Sun, 2 Aug 2026 14:45:28 +0530 Subject: [PATCH] refactor(cli): review follow-ups for the config.yaml import guard MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - agent_import.dump_yaml_file now calls utils.atomic_yaml_write instead of hand-rolling safe_dump + atomic_write_text — same temp+fsync+atomic rename and symlink preservation, plus mode/owner preservation a 0600-secured config.yaml needs - openclaw script: the EXDEV/EBUSY copy fallback gains copystat + target fsync so the docstring's 'mirrors utils.atomic_replace' durability claim is true on cross-device deployments - trim load_yaml_file's docstring to the behavior contract --- hermes_cli/agent_import.py | 35 +++++-------------- .../scripts/openclaw_to_hermes.py | 11 ++++++ tests/hermes_cli/test_agent_import.py | 2 +- 3 files changed, 21 insertions(+), 27 deletions(-) diff --git a/hermes_cli/agent_import.py b/hermes_cli/agent_import.py index ddca7cb355d04..7439133a807e1 100644 --- a/hermes_cli/agent_import.py +++ b/hermes_cli/agent_import.py @@ -47,7 +47,7 @@ import time from pathlib import Path from typing import Any, Dict, List, Optional, Sequence, Tuple -from utils import atomic_write_text +from utils import atomic_write_text, atomic_yaml_write logger = logging.getLogger(__name__) @@ -107,19 +107,9 @@ class ConfigReadError(RuntimeError): def load_yaml_file(path: Path) -> Dict[str, Any]: """Load a YAML mapping, distinguishing "absent" from "unreadable". - Every caller of this function reads ``config.yaml``, merges a section into - it, and writes the whole mapping straight back over the original. So - collapsing a present-but-unreadable file to ``{}`` is destructive: a YAML - syntax error, a permission problem, or a broken mount would make the - importer replace every setting the user had with only the one or two keys - it merged — and still report the item as ``imported``. - - This is the same invariant ``hermes_cli.config`` enforces for its own - writers via ``require_readable_config_before_write`` / ``atomic_config_write`` - ("``read_raw_config()`` returns ``{}`` for BOTH an absent file and an - unreadable-but-present file"), and that ``set_config_value`` gained for - YAML syntax errors. This module has its own private helper pair and so - was never covered by either. + Callers read ``config.yaml``, merge a section in, and write the whole + mapping back — so collapsing a present-but-unreadable file to ``{}`` + would replace every existing setting with just the merged keys. - Absent, or present but empty -> ``{}``; first-time creation still works. - Present but unreadable, unparseable, or not a mapping -> raise @@ -158,24 +148,17 @@ def load_yaml_file(path: Path) -> Dict[str, Any]: def dump_yaml_file(path: Path, data: Dict[str, Any]) -> None: - """Write ``data`` as YAML via temp file + fsync + atomic rename. + """Write ``data`` as YAML atomically (temp file + fsync + rename). Only ever reached after :func:`load_yaml_file` has successfully read the same path, so the mapping being written is the real file's content plus the merged section — never a silently-empty stand-in. - ``atomic_write_text`` (already used by this module for the memory store) - means an interrupted import cannot leave a truncated ``config.yaml`` - behind, and a symlinked config stays a symlink. It creates the parent - directory itself. + ``atomic_yaml_write`` keeps a symlinked config a symlink, creates the + parent directory, and preserves the previous file's mode/owner (a + ``0o600``-secured config stays ``0o600``). """ - import yaml - - atomic_write_text( - path, - yaml.safe_dump(data, default_flow_style=False, sort_keys=False, - allow_unicode=True), - ) + atomic_yaml_write(path, data) # --------------------------------------------------------------------------- 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 8d5a5a3af00ae..199ce94a0fac0 100644 --- a/optional-skills/migration/openclaw-migration/scripts/openclaw_to_hermes.py +++ b/optional-skills/migration/openclaw-migration/scripts/openclaw_to_hermes.py @@ -439,6 +439,17 @@ def dump_yaml_file(path: Path, data: Dict[str, Any]) -> None: if exc.errno not in (errno.EXDEV, errno.EBUSY): raise shutil.copyfile(tmp_path, target) + try: + shutil.copystat(tmp_path, target) + except OSError: + pass + # fsync the copied target so the durability claim holds on the + # cross-device path too (mirrors utils.atomic_replace). + target_fd = os.open(target, os.O_RDONLY) + try: + os.fsync(target_fd) + finally: + os.close(target_fd) os.unlink(tmp_path) except BaseException: try: diff --git a/tests/hermes_cli/test_agent_import.py b/tests/hermes_cli/test_agent_import.py index 1f01d24af18a2..d00ae59f3ad37 100644 --- a/tests/hermes_cli/test_agent_import.py +++ b/tests/hermes_cli/test_agent_import.py @@ -667,7 +667,7 @@ class TestExistingConfigPreserved: def boom(*_args, **_kwargs): raise OSError("no space left on device") - monkeypatch.setattr(agent_import, "atomic_write_text", boom) + monkeypatch.setattr(agent_import, "atomic_yaml_write", boom) with pytest.raises(OSError): agent_import.dump_yaml_file(config_path, {"model": "replacement"})