fix(config): preserve owner on atomic writes (#56644)

tests/test_atomic_replace_symlinks.py

@@ -26,7 +26,12 @@ _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
+from utils import (
+    atomic_json_write,
+    atomic_replace,
+    atomic_roundtrip_yaml_update,
+    atomic_yaml_write,
+)
 
 
 # ─── Direct helper ────────────────────────────────────────────────────────────
@@ -139,6 +144,79 @@ def test_atomic_json_write_preserves_symlink_permissions(tmp_path: Path) -> None
     assert mode == 0o644, f"permissions drifted after symlinked write: {oct(mode)}"
 
 
+def test_atomic_yaml_write_restores_owner_on_real_symlink_target(
+    tmp_path: Path, monkeypatch: pytest.MonkeyPatch
+) -> None:
+    """Config writes through symlinks must restore the real file's owner.
+
+    Docker support hit this when a root-run setup wizard rewrote a
+    hermes-owned /opt/data/config.yaml via atomic replace, leaving the new file
+    root-owned. The test forces a preserved uid/gid so it does not need root.
+    """
+    if os.name != "posix":
+        pytest.skip("POSIX-only")
+
+    real = tmp_path / "config.yaml"
+    link = tmp_path / "link.yaml"
+    real.write_text("old: true\n", encoding="utf-8")
+    link.symlink_to(real)
+
+    chown_calls: list[tuple[Path, int, int]] = []
+    monkeypatch.setattr("utils._preserve_file_owner", lambda _path: (123, 456))
+    monkeypatch.setattr(
+        "utils.os.chown",
+        lambda path, uid, gid: chown_calls.append((Path(path), uid, gid)),
+    )
+
+    atomic_yaml_write(link, {"new": True})
+
+    assert chown_calls == [(real, 123, 456)]
+
+
+def test_atomic_json_write_restores_owner_with_explicit_mode(
+    tmp_path: Path, monkeypatch: pytest.MonkeyPatch
+) -> None:
+    if os.name != "posix":
+        pytest.skip("POSIX-only")
+
+    target = tmp_path / "state.json"
+    target.write_text("{}", encoding="utf-8")
+
+    chown_calls: list[tuple[Path, int, int]] = []
+    monkeypatch.setattr("utils._preserve_file_owner", lambda _path: (234, 567))
+    monkeypatch.setattr(
+        "utils.os.chown",
+        lambda path, uid, gid: chown_calls.append((Path(path), uid, gid)),
+    )
+
+    atomic_json_write(target, {"api_key": "secret"}, mode=0o600)
+
+    assert chown_calls == [(target, 234, 567)]
+    assert target.stat().st_mode & 0o777 == 0o600
+
+
+def test_atomic_roundtrip_yaml_update_restores_owner(
+    tmp_path: Path, monkeypatch: pytest.MonkeyPatch
+) -> None:
+    if os.name != "posix":
+        pytest.skip("POSIX-only")
+
+    target = tmp_path / "config.yaml"
+    target.write_text("model:\n  provider: openrouter\n", encoding="utf-8")
+
+    chown_calls: list[tuple[Path, int, int]] = []
+    monkeypatch.setattr("utils._preserve_file_owner", lambda _path: (345, 678))
+    monkeypatch.setattr(
+        "utils.os.chown",
+        lambda path, uid, gid: chown_calls.append((Path(path), uid, gid)),
+    )
+
+    atomic_roundtrip_yaml_update(target, "model.provider", "nvidia")
+
+    assert chown_calls == [(target, 345, 678)]
+    assert yaml.safe_load(target.read_text(encoding="utf-8"))["model"]["provider"] == "nvidia"
+
+
 # ─── Broken-symlink edge case ─────────────────────────────────────────────
 
 

utils.py

@@ -43,6 +43,34 @@ def _preserve_file_mode(path: Path) -> "int | None":
         return None
 
 
+def _preserve_file_owner(path: Path) -> "tuple[int, int] | None":
+    """Capture the owning uid/gid of *path* if the platform supports it."""
+    if os.name != "posix":
+        return None
+    try:
+        st = path.stat()
+    except OSError:
+        return None
+    return st.st_uid, st.st_gid
+
+
+def _restore_file_owner(path: Path, owner: "tuple[int, int] | None") -> None:
+    """Re-apply uid/gid after an atomic replace when permitted.
+
+    Docker and NAS-backed installs often run some commands as root while the
+    persistent volume is owned by the runtime user. ``os.replace`` swaps in the
+    temp file's owner, so a root-run config write can leave ``config.yaml`` owned
+    by root. Best-effort chown preserves the existing owner for privileged
+    callers and is harmless for unprivileged callers that cannot chown.
+    """
+    if owner is None or not hasattr(os, "chown"):
+        return
+    try:
+        os.chown(path, owner[0], owner[1])
+    except OSError:
+        pass
+
+
 def _restore_file_mode(path: Path, mode: "int | None") -> None:
     """Re-apply *mode* to *path* after an atomic replace.
 
@@ -136,6 +164,7 @@ def atomic_json_write(
     path.parent.mkdir(parents=True, exist_ok=True)
 
     original_mode = None if mode is not None else _preserve_file_mode(path)
+    original_owner = _preserve_file_owner(path)
 
     fd, tmp_path = tempfile.mkstemp(
         dir=str(path.parent),
@@ -160,13 +189,15 @@ def atomic_json_write(
             os.fsync(f.fileno())
         # Preserve symlinks — swap in-place on the real file (GitHub #16743).
         real_path = atomic_replace(tmp_path, path)
+        real_path_obj = Path(real_path)
+        _restore_file_owner(real_path_obj, original_owner)
         if mode is not None:
             try:
-                os.chmod(real_path, mode)
+                os.chmod(real_path_obj, mode)
             except OSError:
                 pass
         else:
-            _restore_file_mode(Path(real_path), original_mode)
+            _restore_file_mode(real_path_obj, original_mode)
     except BaseException:
         # Intentionally catch BaseException so temp-file cleanup still runs for
         # KeyboardInterrupt/SystemExit before re-raising the original signal.
@@ -219,6 +250,7 @@ def atomic_yaml_write(
     path.parent.mkdir(parents=True, exist_ok=True)
 
     original_mode = _preserve_file_mode(path)
+    original_owner = _preserve_file_owner(path)
 
     fd, tmp_path = tempfile.mkstemp(
         dir=str(path.parent),
@@ -248,7 +280,9 @@ def atomic_yaml_write(
             os.fsync(f.fileno())
         # 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)
+        real_path_obj = Path(real_path)
+        _restore_file_owner(real_path_obj, original_owner)
+        _restore_file_mode(real_path_obj, original_mode)
     except BaseException:
         # Match atomic_json_write: cleanup must also happen for process-level
         # interruptions before we re-raise them.
@@ -303,6 +337,7 @@ def atomic_roundtrip_yaml_update(
     current[keys[-1]] = value
 
     original_mode = _preserve_file_mode(path)
+    original_owner = _preserve_file_owner(path)
     fd, tmp_path = tempfile.mkstemp(
         dir=str(path.parent),
         prefix=f".{path.stem}_",
@@ -314,7 +349,9 @@ def atomic_roundtrip_yaml_update(
             f.flush()
             os.fsync(f.fileno())
         real_path = atomic_replace(tmp_path, path)
-        _restore_file_mode(real_path, original_mode)
+        real_path_obj = Path(real_path)
+        _restore_file_owner(real_path_obj, original_owner)
+        _restore_file_mode(real_path_obj, original_mode)
     except BaseException:
         try:
             os.unlink(tmp_path)