diff --git a/tests/tools/test_skills_sync.py b/tests/tools/test_skills_sync.py index 98ccc3b291..d3ca5c97ea 100644 --- a/tests/tools/test_skills_sync.py +++ b/tests/tools/test_skills_sync.py @@ -2,6 +2,8 @@ import shutil import json +import os +import stat import pytest from pathlib import Path from unittest.mock import patch @@ -49,6 +51,18 @@ class TestReadWriteManifest: assert result == {"old-skill": "", "new-skill": "abc123"} + @pytest.mark.skipif(os.name == "nt", reason="POSIX permission bits are platform-specific") + def test_write_manifest_preserves_existing_file_mode(self, tmp_path): + manifest_file = tmp_path / ".bundled_manifest" + manifest_file.write_text("old-skill:oldhash\n", encoding="utf-8") + os.chmod(manifest_file, 0o660) + + with patch("tools.skills_sync.MANIFEST_FILE", manifest_file): + _write_manifest({"new-skill": "newhash"}) + + assert manifest_file.read_text(encoding="utf-8") == "new-skill:newhash\n" + assert stat.S_IMODE(manifest_file.stat().st_mode) == 0o660 + class TestDirHash: def test_hash_reflects_content_only(self, tmp_path): diff --git a/tools/skills_sync.py b/tools/skills_sync.py index 0a8106d669..8ecca29329 100644 --- a/tools/skills_sync.py +++ b/tools/skills_sync.py @@ -48,7 +48,7 @@ for _stream in (sys.stdout, sys.stderr): from hermes_constants import get_bundled_skills_dir, get_hermes_home, get_optional_skills_dir from agent.skill_utils import is_excluded_skill_path from typing import Dict, List, Optional, Set, Tuple -from utils import atomic_replace +from utils import atomic_replace, atomic_write_text logger = logging.getLogger(__name__) @@ -166,32 +166,20 @@ def _read_suppressed_names() -> set: def _write_manifest(entries: Dict[str, str]): """Write the manifest file atomically in v2 format (name:hash). - Uses a temp file + os.replace() to avoid corruption if the process - crashes or is interrupted mid-write. + Uses the shared atomic writer so an existing manifest's permission + bits (and owner, best-effort) survive the replace instead of being + reset to mkstemp's 0600 — the same mode-preservation contract as the + skill manager's document writes. """ - import tempfile - - MANIFEST_FILE.parent.mkdir(parents=True, exist_ok=True) data = "\n".join(f"{name}:{hash_val}" for name, hash_val in sorted(entries.items())) + "\n" try: - fd, tmp_path = tempfile.mkstemp( - dir=str(MANIFEST_FILE.parent), - prefix=".bundled_manifest_", - suffix=".tmp", + atomic_write_text( + MANIFEST_FILE, + data, + tmp_prefix=".bundled_manifest_", + preserve_mode=True, ) - try: - with os.fdopen(fd, "w", encoding="utf-8") as f: - f.write(data) - f.flush() - os.fsync(f.fileno()) - atomic_replace(tmp_path, MANIFEST_FILE) - except BaseException: - try: - os.unlink(tmp_path) - except OSError: - pass - raise except Exception as e: logger.debug("Failed to write skills manifest %s: %s", MANIFEST_FILE, e, exc_info=True)