fix(skills): preserve manifest file mode across atomic rewrites
_write_manifest still used a hand-rolled mkstemp + atomic_replace, so every sync reset .bundled_manifest to mkstemp's 0600, dropping a group-readable or shared mode the operator had set. Replace the block with utils.atomic_write_text(preserve_mode=True) — the same shared writer and mode-preservation contract PR #86255 applied to the skill manager's document writes. This is the remaining half of PR #14410 by @sgaofen, who reported the manifest mode reset first; the skill-manager half of that PR was superseded by the atomic_write_text refactor and #86255.
This commit is contained in:
@@ -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):
|
||||
|
||||
+10
-22
@@ -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)
|
||||
|
||||
|
||||
Reference in New Issue
Block a user