From 4792298621004338b45c40f9e0a89964ac27f18c Mon Sep 17 00:00:00 2001 From: vominh1919 Date: Tue, 28 Apr 2026 09:34:55 +0700 Subject: [PATCH 1/2] fix: preserve symlinks during atomic file writes (#16743) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit os.replace(tmp, path) replaces the symlink itself with a regular file, breaking users who symlink config.yaml, SOUL.md, or .env from ~/.hermes/ to a dotfiles repo or managed profile package. Fix: resolve symlinks via os.path.realpath() before os.replace(), so the real file is overwritten in-place while the symlink survives. Fixed in 7 files covering all os.replace call sites: - utils.py (atomic_json_write, atomic_yaml_write — fixes save_config) - hermes_cli/config.py (env sanitizer, save_env_value, remove_env_value) - tools/skill_manager_tool.py (_atomic_write_text — SOUL.md writes) - tools/memory_tool.py (memory file writes) - tools/skills_sync.py (manifest writes) - cron/jobs.py (job state + output file writes) - agent/shell_hooks.py (hook file writes) Fixes NousResearch/hermes-agent#16743 --- agent/shell_hooks.py | 4 +++- cron/jobs.py | 8 ++++++-- hermes_cli/config.py | 12 +++++++++--- tools/memory_tool.py | 5 ++++- tools/skill_manager_tool.py | 4 +++- tools/skills_sync.py | 4 +++- utils.py | 14 ++++++++++---- 7 files changed, 38 insertions(+), 13 deletions(-) diff --git a/agent/shell_hooks.py b/agent/shell_hooks.py index d0645cb3a889..13ebab4eb731 100644 --- a/agent/shell_hooks.py +++ b/agent/shell_hooks.py @@ -568,7 +568,9 @@ def save_allowlist(data: Dict[str, Any]) -> None: try: with os.fdopen(fd, "w") as fh: fh.write(json.dumps(data, indent=2, sort_keys=True)) - os.replace(tmp_path, p) + # Resolve symlinks so os.replace writes to the real file (GitHub #16743). + real_path = os.path.realpath(p) if os.path.islink(p) else p + os.replace(tmp_path, real_path) except Exception: try: os.unlink(tmp_path) diff --git a/cron/jobs.py b/cron/jobs.py index 6c0a2405b2ec..11a74118016a 100644 --- a/cron/jobs.py +++ b/cron/jobs.py @@ -367,7 +367,9 @@ def save_jobs(jobs: List[Dict[str, Any]]): json.dump({"jobs": jobs, "updated_at": _hermes_now().isoformat()}, f, indent=2) f.flush() os.fsync(f.fileno()) - os.replace(tmp_path, JOBS_FILE) + # Resolve symlinks so os.replace writes to the real file (GitHub #16743). + real_path = os.path.realpath(JOBS_FILE) if os.path.islink(JOBS_FILE) else JOBS_FILE + os.replace(tmp_path, real_path) _secure_file(JOBS_FILE) except BaseException: try: @@ -863,7 +865,9 @@ def save_job_output(job_id: str, output: str): f.write(output) f.flush() os.fsync(f.fileno()) - os.replace(tmp_path, output_file) + # Resolve symlinks so os.replace writes to the real file (GitHub #16743). + real_path = os.path.realpath(output_file) if os.path.islink(output_file) else output_file + os.replace(tmp_path, real_path) _secure_file(output_file) except BaseException: try: diff --git a/hermes_cli/config.py b/hermes_cli/config.py index 5d9c78928331..88746f2801cf 100644 --- a/hermes_cli/config.py +++ b/hermes_cli/config.py @@ -3666,7 +3666,9 @@ def sanitize_env_file() -> int: f.writelines(sanitized) f.flush() os.fsync(f.fileno()) - os.replace(tmp_path, env_path) + # Resolve symlinks so os.replace writes to the real file (GitHub #16743). + real_path = os.path.realpath(env_path) if os.path.islink(env_path) else env_path + os.replace(tmp_path, real_path) except BaseException: try: os.unlink(tmp_path) @@ -3769,7 +3771,9 @@ def save_env_value(key: str, value: str): f.writelines(lines) f.flush() os.fsync(f.fileno()) - os.replace(tmp_path, env_path) + # Resolve symlinks so os.replace writes to the real file (GitHub #16743). + real_path = os.path.realpath(env_path) if os.path.islink(env_path) else env_path + os.replace(tmp_path, real_path) # Restore original permissions before _secure_file may tighten them. if original_mode is not None: try: @@ -3825,7 +3829,9 @@ def remove_env_value(key: str) -> bool: f.writelines(new_lines) f.flush() os.fsync(f.fileno()) - os.replace(tmp_path, env_path) + # Resolve symlinks so os.replace writes to the real file (GitHub #16743). + real_path = os.path.realpath(env_path) if os.path.islink(env_path) else env_path + os.replace(tmp_path, real_path) if original_mode is not None: try: os.chmod(env_path, original_mode) diff --git a/tools/memory_tool.py b/tools/memory_tool.py index eef64e709669..6c0527ab425e 100644 --- a/tools/memory_tool.py +++ b/tools/memory_tool.py @@ -448,7 +448,10 @@ def _write_file(path: Path, entries: List[str]): f.write(content) f.flush() os.fsync(f.fileno()) - os.replace(tmp_path, str(path)) # Atomic on same filesystem + # Resolve symlinks so os.replace writes to the real file (GitHub #16743). + path_str = str(path) + real_path = os.path.realpath(path_str) if os.path.islink(path_str) else path_str + os.replace(tmp_path, real_path) except BaseException: # Clean up temp file on any failure try: diff --git a/tools/skill_manager_tool.py b/tools/skill_manager_tool.py index c28f421a7f98..734e3fa7270a 100644 --- a/tools/skill_manager_tool.py +++ b/tools/skill_manager_tool.py @@ -309,7 +309,9 @@ def _atomic_write_text(file_path: Path, content: str, encoding: str = "utf-8") - try: with os.fdopen(fd, "w", encoding=encoding) as f: f.write(content) - os.replace(temp_path, file_path) + # Resolve symlinks so os.replace writes to the real file (GitHub #16743). + real_path = os.path.realpath(file_path) if os.path.islink(file_path) else file_path + os.replace(temp_path, real_path) except Exception: # Clean up temp file on error try: diff --git a/tools/skills_sync.py b/tools/skills_sync.py index cb7955c01921..b82b1219faa6 100644 --- a/tools/skills_sync.py +++ b/tools/skills_sync.py @@ -98,7 +98,9 @@ def _write_manifest(entries: Dict[str, str]): f.write(data) f.flush() os.fsync(f.fileno()) - os.replace(tmp_path, MANIFEST_FILE) + # Resolve symlinks so os.replace writes to the real file (GitHub #16743). + real_path = os.path.realpath(MANIFEST_FILE) if os.path.islink(MANIFEST_FILE) else MANIFEST_FILE + os.replace(tmp_path, real_path) except BaseException: try: os.unlink(tmp_path) diff --git a/utils.py b/utils.py index f3d38006d145..dc2a3e099eaf 100644 --- a/utils.py +++ b/utils.py @@ -99,8 +99,11 @@ def atomic_json_write( ) f.flush() os.fsync(f.fileno()) - os.replace(tmp_path, path) - _restore_file_mode(path, original_mode) + # Resolve symlinks so os.replace writes to the real file instead of + # replacing the symlink with a regular file (GitHub #16743). + real_path = os.path.realpath(path) if os.path.islink(path) else path + os.replace(tmp_path, real_path) + _restore_file_mode(real_path, original_mode) except BaseException: # Intentionally catch BaseException so temp-file cleanup still runs for # KeyboardInterrupt/SystemExit before re-raising the original signal. @@ -150,8 +153,11 @@ def atomic_yaml_write( f.write(extra_content) f.flush() os.fsync(f.fileno()) - os.replace(tmp_path, path) - _restore_file_mode(path, original_mode) + # Resolve symlinks so os.replace writes to the real file instead of + # replacing the symlink with a regular file (GitHub #16743). + real_path = os.path.realpath(path) if os.path.islink(path) else path + os.replace(tmp_path, real_path) + _restore_file_mode(real_path, original_mode) except BaseException: # Match atomic_json_write: cleanup must also happen for process-level # interruptions before we re-raise them. From 4ad210c00c41b486c27f4bcac5fbdf40bf0ce832 Mon Sep 17 00:00:00 2001 From: Teknium Date: Tue, 28 Apr 2026 04:51:38 -0700 Subject: [PATCH 2/2] refactor: consolidate symlink-safe atomic replace into shared helper Extract the islink/realpath guard from the 16743 fix into a single atomic_replace() helper in utils.py, then migrate every os.replace() call site in the codebase to use it. The original PR #16777 correctly identified and fixed the bug, but only patched 9 of ~24 call sites. The same bug class (managed deployments that symlink state files silently losing the link on every write) still existed at auth.json, sessions file, gateway config, env_loader, webhook subscriptions, debug store, model catalog, pairing, google OAuth, nous rate guard, and more. Rather than add another 10+ copies of the same three-line guard, consolidate into atomic_replace(tmp, target) which: - resolves symlinks via os.path.realpath before os.replace - returns the resolved real path so callers can re-apply permissions - is a drop-in replacement for os.replace at the use sites Changes: - utils.py: new atomic_replace() helper + atomic_json_write / atomic_yaml_write now call it instead of inlining the guard - 16 files: all os.replace() call sites migrated to atomic_replace() - agent/{google_oauth, nous_rate_guard, shell_hooks}.py - cron/jobs.py - gateway/{pairing, session, platforms/telegram}.py - hermes_cli/{auth, config, debug, env_loader, model_catalog, webhook}.py - tools/{memory_tool, skill_manager_tool, skills_sync}.py Tests: tests/test_atomic_replace_symlinks.py pins the invariant for atomic_replace + atomic_json_write + atomic_yaml_write, covers plain files, first-time creates, broken symlinks, and permission preservation. Refs #16743 Builds on #16777 by @vominh1919. --- agent/google_oauth.py | 3 +- agent/nous_rate_guard.py | 3 +- agent/shell_hooks.py | 5 +- cron/jobs.py | 9 +- gateway/pairing.py | 3 +- gateway/platforms/telegram.py | 3 +- gateway/session.py | 3 +- hermes_cli/auth.py | 3 +- hermes_cli/config.py | 13 +-- hermes_cli/debug.py | 3 +- hermes_cli/env_loader.py | 3 +- hermes_cli/model_catalog.py | 3 +- hermes_cli/webhook.py | 3 +- tests/test_atomic_replace_symlinks.py | 160 ++++++++++++++++++++++++++ tools/memory_tool.py | 7 +- tools/skill_manager_tool.py | 6 +- tools/skills_sync.py | 5 +- utils.py | 36 ++++-- 18 files changed, 225 insertions(+), 46 deletions(-) create mode 100644 tests/test_atomic_replace_symlinks.py diff --git a/agent/google_oauth.py b/agent/google_oauth.py index 4fda090fc66d..231e7e129205 100644 --- a/agent/google_oauth.py +++ b/agent/google_oauth.py @@ -98,6 +98,7 @@ # Regex patterns for fallback scraping from an installed gemini-cli. import re as _re +from utils import atomic_replace _CLIENT_ID_PATTERN = _re.compile( r"OAUTH_CLIENT_ID\s*=\s*['\"]([0-9]+-[a-z0-9]+\.apps\.googleusercontent\.com)['\"]" ) @@ -499,7 +500,7 @@ def save_credentials(creds: GoogleCredentials) -> Path: fh.flush() os.fsync(fh.fileno()) os.chmod(tmp_path, stat.S_IRUSR | stat.S_IWUSR) - os.replace(tmp_path, path) + atomic_replace(tmp_path, path) finally: try: if tmp_path.exists(): diff --git a/agent/nous_rate_guard.py b/agent/nous_rate_guard.py index ea866f2e080d..b28803122c5f 100644 --- a/agent/nous_rate_guard.py +++ b/agent/nous_rate_guard.py @@ -18,6 +18,7 @@ import tempfile import time from typing import Any, Mapping, Optional +from utils import atomic_replace logger = logging.getLogger(__name__) @@ -118,7 +119,7 @@ def record_nous_rate_limit( try: with os.fdopen(fd, "w") as f: json.dump(state, f) - os.replace(tmp_path, path) + atomic_replace(tmp_path, path) except Exception: # Clean up temp file on failure try: diff --git a/agent/shell_hooks.py b/agent/shell_hooks.py index 13ebab4eb731..94750d520410 100644 --- a/agent/shell_hooks.py +++ b/agent/shell_hooks.py @@ -76,6 +76,7 @@ fcntl = None # type: ignore[assignment] from hermes_constants import get_hermes_home +from utils import atomic_replace logger = logging.getLogger(__name__) @@ -568,9 +569,7 @@ def save_allowlist(data: Dict[str, Any]) -> None: try: with os.fdopen(fd, "w") as fh: fh.write(json.dumps(data, indent=2, sort_keys=True)) - # Resolve symlinks so os.replace writes to the real file (GitHub #16743). - real_path = os.path.realpath(p) if os.path.islink(p) else p - os.replace(tmp_path, real_path) + atomic_replace(tmp_path, p) except Exception: try: os.unlink(tmp_path) diff --git a/cron/jobs.py b/cron/jobs.py index 11a74118016a..5bdb1122fe65 100644 --- a/cron/jobs.py +++ b/cron/jobs.py @@ -21,6 +21,7 @@ logger = logging.getLogger(__name__) from hermes_time import now as _hermes_now +from utils import atomic_replace try: from croniter import croniter @@ -367,9 +368,7 @@ def save_jobs(jobs: List[Dict[str, Any]]): json.dump({"jobs": jobs, "updated_at": _hermes_now().isoformat()}, f, indent=2) f.flush() os.fsync(f.fileno()) - # Resolve symlinks so os.replace writes to the real file (GitHub #16743). - real_path = os.path.realpath(JOBS_FILE) if os.path.islink(JOBS_FILE) else JOBS_FILE - os.replace(tmp_path, real_path) + atomic_replace(tmp_path, JOBS_FILE) _secure_file(JOBS_FILE) except BaseException: try: @@ -865,9 +864,7 @@ def save_job_output(job_id: str, output: str): f.write(output) f.flush() os.fsync(f.fileno()) - # Resolve symlinks so os.replace writes to the real file (GitHub #16743). - real_path = os.path.realpath(output_file) if os.path.islink(output_file) else output_file - os.replace(tmp_path, real_path) + atomic_replace(tmp_path, output_file) _secure_file(output_file) except BaseException: try: diff --git a/gateway/pairing.py b/gateway/pairing.py index 09b61fef2249..d5f7ec6b96ea 100644 --- a/gateway/pairing.py +++ b/gateway/pairing.py @@ -28,6 +28,7 @@ from typing import Optional from hermes_constants import get_hermes_dir +from utils import atomic_replace # Unambiguous alphabet -- excludes 0/O, 1/I to prevent confusion @@ -59,7 +60,7 @@ def _secure_write(path: Path, data: str) -> None: f.write(data) f.flush() os.fsync(f.fileno()) - os.replace(tmp_path, str(path)) + atomic_replace(tmp_path, path) try: os.chmod(path, 0o600) except OSError: diff --git a/gateway/platforms/telegram.py b/gateway/platforms/telegram.py index c5643cf3ed10..ce536f515157 100644 --- a/gateway/platforms/telegram.py +++ b/gateway/platforms/telegram.py @@ -84,6 +84,7 @@ class _MockContextTypes: discover_fallback_ips, parse_fallback_ip_env, ) +from utils import atomic_replace def check_telegram_requirements() -> bool: @@ -554,7 +555,7 @@ def _persist_dm_topic_thread_id(self, chat_id: int, topic_name: str, thread_id: _yaml.dump(config, f, default_flow_style=False, sort_keys=False) f.flush() os.fsync(f.fileno()) - os.replace(tmp_path, config_path) + atomic_replace(tmp_path, config_path) except BaseException: try: os.unlink(tmp_path) diff --git a/gateway/session.py b/gateway/session.py index 36e187fe1ec2..0cd7fbca1f40 100644 --- a/gateway/session.py +++ b/gateway/session.py @@ -64,6 +64,7 @@ def _hash_chat_id(value: str) -> str: canonical_whatsapp_identifier, normalize_whatsapp_identifier, ) +from utils import atomic_replace @dataclass @@ -705,7 +706,7 @@ def _save(self) -> None: json.dump(data, f, indent=2) f.flush() os.fsync(f.fileno()) - os.replace(tmp_path, sessions_file) + atomic_replace(tmp_path, sessions_file) except BaseException: try: os.unlink(tmp_path) diff --git a/hermes_cli/auth.py b/hermes_cli/auth.py index 700aa7a2e9f6..61a5760dcb58 100644 --- a/hermes_cli/auth.py +++ b/hermes_cli/auth.py @@ -43,6 +43,7 @@ from hermes_cli.config import get_hermes_home, get_config_path, read_raw_config from hermes_constants import OPENROUTER_BASE_URL +from utils import atomic_replace logger = logging.getLogger(__name__) @@ -828,7 +829,7 @@ def _save_auth_store(auth_store: Dict[str, Any]) -> Path: handle.write(payload) handle.flush() os.fsync(handle.fileno()) - os.replace(tmp_path, auth_file) + atomic_replace(tmp_path, auth_file) try: dir_fd = os.open(str(auth_file.parent), os.O_RDONLY) except OSError: diff --git a/hermes_cli/config.py b/hermes_cli/config.py index 88746f2801cf..ddb3dec0acb7 100644 --- a/hermes_cli/config.py +++ b/hermes_cli/config.py @@ -227,6 +227,7 @@ def get_container_exec_info() -> Optional[dict]: # Re-export from hermes_constants — canonical definition lives there. from hermes_constants import get_hermes_home # noqa: F811,E402 +from utils import atomic_replace def get_config_path() -> Path: """Get the main config file path.""" @@ -3666,9 +3667,7 @@ def sanitize_env_file() -> int: f.writelines(sanitized) f.flush() os.fsync(f.fileno()) - # Resolve symlinks so os.replace writes to the real file (GitHub #16743). - real_path = os.path.realpath(env_path) if os.path.islink(env_path) else env_path - os.replace(tmp_path, real_path) + atomic_replace(tmp_path, env_path) except BaseException: try: os.unlink(tmp_path) @@ -3771,9 +3770,7 @@ def save_env_value(key: str, value: str): f.writelines(lines) f.flush() os.fsync(f.fileno()) - # Resolve symlinks so os.replace writes to the real file (GitHub #16743). - real_path = os.path.realpath(env_path) if os.path.islink(env_path) else env_path - os.replace(tmp_path, real_path) + atomic_replace(tmp_path, env_path) # Restore original permissions before _secure_file may tighten them. if original_mode is not None: try: @@ -3829,9 +3826,7 @@ def remove_env_value(key: str) -> bool: f.writelines(new_lines) f.flush() os.fsync(f.fileno()) - # Resolve symlinks so os.replace writes to the real file (GitHub #16743). - real_path = os.path.realpath(env_path) if os.path.islink(env_path) else env_path - os.replace(tmp_path, real_path) + atomic_replace(tmp_path, env_path) if original_mode is not None: try: os.chmod(env_path, original_mode) diff --git a/hermes_cli/debug.py b/hermes_cli/debug.py index e2eac544f269..71c5443516f7 100644 --- a/hermes_cli/debug.py +++ b/hermes_cli/debug.py @@ -18,6 +18,7 @@ from typing import Optional from hermes_constants import get_hermes_home +from utils import atomic_replace # --------------------------------------------------------------------------- @@ -79,7 +80,7 @@ def _save_pending(entries: list[dict]) -> None: path.parent.mkdir(parents=True, exist_ok=True) tmp = path.with_suffix(".json.tmp") tmp.write_text(json.dumps(entries, indent=2), encoding="utf-8") - os.replace(tmp, path) + atomic_replace(tmp, path) except OSError: # Non-fatal — worst case the user has to run ``hermes debug delete`` # manually. diff --git a/hermes_cli/env_loader.py b/hermes_cli/env_loader.py index 009f3de273b2..61824672c070 100644 --- a/hermes_cli/env_loader.py +++ b/hermes_cli/env_loader.py @@ -7,6 +7,7 @@ from pathlib import Path from dotenv import load_dotenv +from utils import atomic_replace # Env var name suffixes that indicate credential values. These are the @@ -127,7 +128,7 @@ def _sanitize_env_file_if_needed(path: Path) -> None: f.writelines(sanitized) f.flush() os.fsync(f.fileno()) - os.replace(tmp, path) + atomic_replace(tmp, path) except BaseException: try: os.unlink(tmp) diff --git a/hermes_cli/model_catalog.py b/hermes_cli/model_catalog.py index 500910d57f45..51873adf145c 100644 --- a/hermes_cli/model_catalog.py +++ b/hermes_cli/model_catalog.py @@ -54,6 +54,7 @@ from typing import Any from hermes_cli import __version__ as _HERMES_VERSION +from utils import atomic_replace logger = logging.getLogger(__name__) @@ -190,7 +191,7 @@ def _write_disk_cache(data: dict[str, Any]) -> None: with open(tmp, "w") as fh: json.dump(data, fh, indent=2) fh.write("\n") - os.replace(tmp, path) + atomic_replace(tmp, path) except OSError as exc: logger.info("model catalog cache write failed: %s", exc) diff --git a/hermes_cli/webhook.py b/hermes_cli/webhook.py index 378f11b4a7ec..116464dde6e2 100644 --- a/hermes_cli/webhook.py +++ b/hermes_cli/webhook.py @@ -19,6 +19,7 @@ from typing import Dict from hermes_constants import display_hermes_home +from utils import atomic_replace _SUBSCRIPTIONS_FILENAME = "webhook_subscriptions.json" @@ -52,7 +53,7 @@ def _save_subscriptions(subs: Dict[str, dict]) -> None: json.dumps(subs, indent=2, ensure_ascii=False), encoding="utf-8", ) - os.replace(str(tmp_path), str(path)) + atomic_replace(tmp_path, path) def _get_webhook_config() -> dict: diff --git a/tests/test_atomic_replace_symlinks.py b/tests/test_atomic_replace_symlinks.py new file mode 100644 index 000000000000..f6b849183294 --- /dev/null +++ b/tests/test_atomic_replace_symlinks.py @@ -0,0 +1,160 @@ +"""Regression tests for GitHub #16743 — atomic writes must preserve symlinks. + +``os.replace(tmp, target)`` replaces whatever exists at ``target`` — including +symlinks, which it swaps for a regular file. Managed deployments that +symlink ``~/.hermes/config.yaml`` (and other state files) to a git-tracked +profile package were silently detached on every config write. + +The fix: a shared ``atomic_replace`` helper in ``utils.py`` that resolves the +target through ``os.path.realpath`` when it is a symlink, so the real file is +overwritten in-place while the symlink survives. All atomic-write sites in +the codebase were migrated to the helper; these tests pin that invariant. +""" +from __future__ import annotations + +import json +import os +import sys +from pathlib import Path + +import pytest +import yaml + +# Ensure the repo root is importable when running via `pytest tests/...`. +_REPO_ROOT = Path(__file__).resolve().parent.parent +if str(_REPO_ROOT) not in sys.path: + sys.path.insert(0, str(_REPO_ROOT)) + +from utils import atomic_json_write, atomic_replace, atomic_yaml_write + + +# ─── Direct helper ──────────────────────────────────────────────────────────── + + +def _write_tmp(dir_: Path, content: str) -> Path: + tmp = dir_ / ".src.tmp" + tmp.write_text(content, encoding="utf-8") + return tmp + + +def test_atomic_replace_preserves_symlink(tmp_path: Path) -> None: + real = tmp_path / "real.yaml" + link = tmp_path / "link.yaml" + real.write_text("original\n", encoding="utf-8") + link.symlink_to(real) + + tmp = _write_tmp(tmp_path, "updated\n") + returned = atomic_replace(tmp, link) + + assert link.is_symlink(), "symlink must not be replaced with a regular file" + assert real.read_text(encoding="utf-8") == "updated\n" + assert Path(returned) == real + # Follow the symlink — same content. + assert link.read_text(encoding="utf-8") == "updated\n" + + +def test_atomic_replace_regular_file(tmp_path: Path) -> None: + target = tmp_path / "plain.yaml" + target.write_text("old\n", encoding="utf-8") + + tmp = _write_tmp(tmp_path, "fresh\n") + returned = atomic_replace(tmp, target) + + assert Path(returned) == target + assert target.read_text(encoding="utf-8") == "fresh\n" + assert not target.is_symlink() + + +def test_atomic_replace_first_time_create(tmp_path: Path) -> None: + target = tmp_path / "new.yaml" + assert not target.exists() + + tmp = _write_tmp(tmp_path, "brand new\n") + returned = atomic_replace(tmp, target) + + assert Path(returned) == target + assert target.read_text(encoding="utf-8") == "brand new\n" + + +def test_atomic_replace_accepts_pathlike_and_str(tmp_path: Path) -> None: + target = tmp_path / "dual.json" + target.write_text("{}", encoding="utf-8") + + # str inputs + tmp1 = _write_tmp(tmp_path, "1") + atomic_replace(str(tmp1), str(target)) + assert target.read_text(encoding="utf-8") == "1" + + # Path inputs + tmp2 = _write_tmp(tmp_path, "2") + atomic_replace(tmp2, target) + assert target.read_text(encoding="utf-8") == "2" + + +# ─── atomic_json_write / atomic_yaml_write wiring ────────────────────────── + + +def test_atomic_json_write_preserves_symlink(tmp_path: Path) -> None: + real = tmp_path / "real.json" + link = tmp_path / "link.json" + real.write_text("{}", encoding="utf-8") + link.symlink_to(real) + + atomic_json_write(link, {"hello": "world"}) + + assert link.is_symlink() + loaded = json.loads(real.read_text(encoding="utf-8")) + assert loaded == {"hello": "world"} + + +def test_atomic_yaml_write_preserves_symlink(tmp_path: Path) -> None: + real = tmp_path / "real.yaml" + link = tmp_path / "link.yaml" + real.write_text("placeholder: true\n", encoding="utf-8") + link.symlink_to(real) + + atomic_yaml_write(link, {"model": {"provider": "openrouter"}}) + + assert link.is_symlink() + data = yaml.safe_load(real.read_text(encoding="utf-8")) + assert data == {"model": {"provider": "openrouter"}} + + +def test_atomic_json_write_preserves_symlink_permissions(tmp_path: Path) -> None: + """Symlinked targets keep the real file's permission bits.""" + if os.name != "posix": + pytest.skip("POSIX-only") + + real = tmp_path / "real.json" + link = tmp_path / "link.json" + real.write_text("{}", encoding="utf-8") + os.chmod(real, 0o644) + link.symlink_to(real) + + atomic_json_write(link, {"x": 1}) + + import stat as _stat + mode = _stat.S_IMODE(real.stat().st_mode) + assert mode == 0o644, f"permissions drifted after symlinked write: {oct(mode)}" + + +# ─── Broken-symlink edge case ───────────────────────────────────────────── + + +def test_atomic_replace_broken_symlink_creates_target(tmp_path: Path) -> None: + """A symlink pointing at a missing file: the write should create the + real target (resolving via realpath) rather than leaving the dangling + link in place as a regular file. + """ + missing = tmp_path / "does_not_exist_yet.yaml" + link = tmp_path / "link.yaml" + link.symlink_to(missing) + assert link.is_symlink() + assert not missing.exists() + + tmp = _write_tmp(tmp_path, "created-through-link\n") + atomic_replace(tmp, link) + + assert link.is_symlink(), "symlink must be preserved" + assert missing.exists(), "real target should now exist" + assert missing.read_text(encoding="utf-8") == "created-through-link\n" diff --git a/tools/memory_tool.py b/tools/memory_tool.py index 6c0527ab425e..0de12a64f383 100644 --- a/tools/memory_tool.py +++ b/tools/memory_tool.py @@ -33,6 +33,8 @@ from hermes_constants import get_hermes_home from typing import Dict, Any, List, Optional +from utils import atomic_replace + # fcntl is Unix-only; on Windows use msvcrt for file locking msvcrt = None try: @@ -448,10 +450,7 @@ def _write_file(path: Path, entries: List[str]): f.write(content) f.flush() os.fsync(f.fileno()) - # Resolve symlinks so os.replace writes to the real file (GitHub #16743). - path_str = str(path) - real_path = os.path.realpath(path_str) if os.path.islink(path_str) else path_str - os.replace(tmp_path, real_path) + atomic_replace(tmp_path, path) except BaseException: # Clean up temp file on any failure try: diff --git a/tools/skill_manager_tool.py b/tools/skill_manager_tool.py index 734e3fa7270a..cece3f4fca8e 100644 --- a/tools/skill_manager_tool.py +++ b/tools/skill_manager_tool.py @@ -42,6 +42,8 @@ from hermes_constants import get_hermes_home, display_hermes_home from typing import Dict, Any, Optional, Tuple +from utils import atomic_replace + logger = logging.getLogger(__name__) # Import security scanner — external hub installs always get scanned; @@ -309,9 +311,7 @@ def _atomic_write_text(file_path: Path, content: str, encoding: str = "utf-8") - try: with os.fdopen(fd, "w", encoding=encoding) as f: f.write(content) - # Resolve symlinks so os.replace writes to the real file (GitHub #16743). - real_path = os.path.realpath(file_path) if os.path.islink(file_path) else file_path - os.replace(temp_path, real_path) + atomic_replace(temp_path, file_path) except Exception: # Clean up temp file on error try: diff --git a/tools/skills_sync.py b/tools/skills_sync.py index b82b1219faa6..98cd85c39404 100644 --- a/tools/skills_sync.py +++ b/tools/skills_sync.py @@ -28,6 +28,7 @@ from pathlib import Path from hermes_constants import get_hermes_home from typing import Dict, List, Tuple +from utils import atomic_replace logger = logging.getLogger(__name__) @@ -98,9 +99,7 @@ def _write_manifest(entries: Dict[str, str]): f.write(data) f.flush() os.fsync(f.fileno()) - # Resolve symlinks so os.replace writes to the real file (GitHub #16743). - real_path = os.path.realpath(MANIFEST_FILE) if os.path.islink(MANIFEST_FILE) else MANIFEST_FILE - os.replace(tmp_path, real_path) + atomic_replace(tmp_path, MANIFEST_FILE) except BaseException: try: os.unlink(tmp_path) diff --git a/utils.py b/utils.py index dc2a3e099eaf..595c3e831c4f 100644 --- a/utils.py +++ b/utils.py @@ -58,6 +58,30 @@ def _restore_file_mode(path: Path, mode: "int | None") -> None: pass +def atomic_replace(tmp_path: Union[str, Path], target: Union[str, Path]) -> str: + """Atomically move *tmp_path* onto *target*, preserving symlinks. + + ``os.replace(tmp, target)`` atomically swaps ``tmp`` into place at + ``target``. When ``target`` is a symlink, the symlink itself is + replaced with a regular file — silently detaching managed deployments + that symlink ``config.yaml`` / ``SOUL.md`` / ``auth.json`` etc. from + ``~/.hermes/`` to a git-tracked profile package or dotfiles repo + (GitHub #16743). + + This helper resolves the symlink first so ``os.replace`` writes to + the real file in-place while the symlink survives. For non-symlink + and non-existent paths the behavior is identical to a plain + ``os.replace`` call. + + Returns the resolved real path used for the replace, so callers that + need to re-apply permissions can target it instead of the symlink. + """ + target_str = str(target) + real_path = os.path.realpath(target_str) if os.path.islink(target_str) else target_str + os.replace(str(tmp_path), real_path) + return real_path + + def atomic_json_write( path: Union[str, Path], data: Any, @@ -99,10 +123,8 @@ def atomic_json_write( ) f.flush() os.fsync(f.fileno()) - # Resolve symlinks so os.replace writes to the real file instead of - # replacing the symlink with a regular file (GitHub #16743). - real_path = os.path.realpath(path) if os.path.islink(path) else path - os.replace(tmp_path, real_path) + # Preserve symlinks — swap in-place on the real file (GitHub #16743). + real_path = atomic_replace(tmp_path, path) _restore_file_mode(real_path, original_mode) except BaseException: # Intentionally catch BaseException so temp-file cleanup still runs for @@ -153,10 +175,8 @@ def atomic_yaml_write( f.write(extra_content) f.flush() os.fsync(f.fileno()) - # Resolve symlinks so os.replace writes to the real file instead of - # replacing the symlink with a regular file (GitHub #16743). - real_path = os.path.realpath(path) if os.path.islink(path) else path - os.replace(tmp_path, real_path) + # Preserve symlinks — swap in-place on the real file (GitHub #16743). + real_path = atomic_replace(tmp_path, path) _restore_file_mode(real_path, original_mode) except BaseException: # Match atomic_json_write: cleanup must also happen for process-level