From 8c2450fcf9cc2d46f5583087eabe77da83743654 Mon Sep 17 00:00:00 2001 From: Neo Lehmann <274279297+legacynode@users.noreply.github.com> Date: Wed, 22 Jul 2026 21:15:47 +0530 Subject: [PATCH] fix(skills_sync): keep bundled skill copies writable on Nix store MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit shutil.copy2 and shutil.copytree both preserve source-file mode bits. When bundled skills are sourced from a read-only filesystem — the Nix store (mode 0444/0555), squashfs, or an OCI image layer — the user copy in ~/.hermes/skills/ inherits those bits. Later edits via skill_manage, the curator, or even the shutil.rmtree(*.bak) cleanup at the end of an update then fail with PermissionError (silently swallowed by ignore_errors=True, so stale *.bak directories accumulate). Three helpers fix the root cause at the copy boundary: - _ensure_owner_writable(path) — adds the owner-write bit on a single file or directory without touching other mode bits (skips symlinks: os.chmod follows them by default, and a copied symlink pointing outside the profile/tree must not have its external target mutated). - _copy_file_writable(src, dst) — drop-in shutil.copy2 replacement, also used as copytree's copy_function. - _copytree_writable(src, dst) — drop-in shutil.copytree replacement; sweeps the destination tree afterwards since copytree reapplies source-directory metadata after file copies. Wired into every copy site that may read from a read-only bundled source: tools/skills_sync.py (sync_skills new + update paths, restore_official_optional_skill, the DESCRIPTION.md copy, and the reset_bundled_skill rmtree), hermes_cli/profiles.py (--clone and --clone-all), and hermes_cli/profile_distribution.py (apply_distribution). Also repairs two states the copy-boundary fix alone doesn't reach: - Migration for existing installs: a user copy that predates this fix can be hash-identical to the bundled source, so sync_skills takes the "unchanged" no-op path and would never repair it. Both the v1-migration branch and the "bundled unchanged, user unchanged" branch now sweep _make_tree_owner_writable(dest) unconditionally. - Symlink safety in the profile-clone repair sweep: profiles.py clones skills with shutil.copytree(..., symlinks=True, ...), so a skill that is itself a symlink to a shared/vendored directory outside the profile reaches the writable-mode repair as a real symlink entry. _ensure_owner_writable skips symlinks rather than chmod-ing through them into whatever they still point at. Salvages closed PR #20135 (closed by author, not maintainer-rejected) and extends its coverage. Complements #34860 (83a7d0b60 / 8ae0802d5), which fixed the removal side (_rmtree_writable making read-only trees removable) — this fixes the copy side (preventing read-only trees from being created in the first place). --- AGENTS.md | 36 ++++ hermes_cli/profile_distribution.py | 9 + hermes_cli/profiles.py | 20 +- tests/hermes_cli/test_profiles.py | 49 +++++ tests/tools/test_skills_sync.py | 304 ++++++++++++++++++++++++++++- tools/skills_sync.py | 100 +++++++++- 6 files changed, 505 insertions(+), 13 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index cb53e95eb0b..c42115e7d51 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -1221,6 +1221,42 @@ Use `get_hermes_home()` from `hermes_constants` for code paths. Use `display_her for user-facing print/log messages. Hardcoding `~/.hermes` breaks profiles — each profile has its own `HERMES_HOME` directory. This was the source of 5 bugs fixed in PR #3575. +### `shutil.copytree` from a read-only filesystem preserves unwritable mode bits +`shutil.copytree` and `shutil.copy2` both copy source-file permissions by +default. When the source lives on the Nix store +(`/nix/store/.../share/hermes-agent/skills`, mode `0444`/`0555`) — or any +other read-only filesystem (squashfs, OCI image layer) — the user copy in +`~/.hermes/skills/` inherits those bits. Later edits via `skill_manage`, +the curator, or even a follow-up `shutil.rmtree(..., ignore_errors=True)` +of a `*.bak` directory then fail with `PermissionError`, which gets +silently swallowed and accumulates stale `*.bak` directories. + +Use `tools.skills_sync._copytree_writable(src, dst)` (drop-in replacement +for `shutil.copytree`) or `_copy_file_writable(src, dst)` (drop-in +replacement for `shutil.copy2`) whenever the source might be a Nix-store +path. For trees that already exist on disk and might still carry the +inherited bits, `_make_tree_owner_writable(root)` repairs them in place. + +Affected call sites that already use these helpers: + +- `tools/skills_sync.py` — `sync_skills` (new + update paths, including the + no-op "bundled unchanged, user unchanged" and v1-migration branches, which + repair pre-existing hash-identical read-only copies that predate this fix), + `restore_official_optional_skill`, `reset_bundled_skill`, DESCRIPTION.md + copy +- `hermes_cli/profiles.py` — `--clone` and `--clone-all` +- `hermes_cli/profile_distribution.py` — `apply_distribution` + +`_make_tree_owner_writable` / `_ensure_owner_writable` skip symlink entries +(`path.is_symlink()`) rather than chmod-ing them. `os.chmod` follows +symlinks by default, so walking a copied tree without that guard would +mutate the mode of whatever external file a symlink still points at, not +anything the copy owns. This matters because `hermes_cli/profiles.py` clones +skills with `shutil.copytree(..., symlinks=True, ...)`, which preserves +symlinks as symlinks (does not traverse into their targets) — a skill that +is itself a symlink to a shared/vendored directory outside the profile is a +real, not hypothetical, case. + ### DO NOT introduce new `simple_term_menu` usage Existing call sites in `hermes_cli/main.py` remain for legacy fallback only; the preferred UI is curses (stdlib) because `simple_term_menu` has diff --git a/hermes_cli/profile_distribution.py b/hermes_cli/profile_distribution.py index c981015d4b0..4923570e2d2 100644 --- a/hermes_cli/profile_distribution.py +++ b/hermes_cli/profile_distribution.py @@ -583,8 +583,17 @@ def _copy_dist_payload( else [] ), ) + # Distributed payloads can originate from a read-only Nix store; + # restore owner-writable mode so the new profile's tooling can + # edit the files later (curator, skill_manage, plugin overrides). + from tools.skills_sync import _make_tree_owner_writable + + _make_tree_owner_writable(dest) else: shutil.copy2(entry, dest) + from tools.skills_sync import _ensure_owner_writable + + _ensure_owner_writable(dest) # Emit .env.EXAMPLE from manifest if the staged tree didn't ship one if manifest.env_requires and not (target / ENV_EXAMPLE_FILENAME).exists(): diff --git a/hermes_cli/profiles.py b/hermes_cli/profiles.py index 55231104fa2..de2ed3d9cd1 100644 --- a/hermes_cli/profiles.py +++ b/hermes_cli/profiles.py @@ -1057,13 +1057,20 @@ def create_profile( ) if clone_all and source_dir: - # Full copy of source profile (exclude sibling ~/.hermes/profiles/) + # Full copy of source profile (exclude sibling ~/.hermes/profiles/). + # If the source still carries Nix-store-inherited read-only mode bits + # on its skills tree (pre-fix Hermes builds), the clone target would + # inherit them via copy2. Restore writability afterwards so the new + # profile's tooling (curator, skill_manage) can edit files in-place. shutil.copytree( source_dir, profile_dir, symlinks=True, ignore=_clone_all_copytree_ignore(source_dir), ) + from tools.skills_sync import _make_tree_owner_writable + + _make_tree_owner_writable(profile_dir) # Strip runtime files for stale in _CLONE_ALL_STRIP: (profile_dir / stale).unlink(missing_ok=True) @@ -1096,7 +1103,18 @@ def create_profile( # same agent capabilities as the source profile. source_skills = source_dir / "skills" if source_skills.is_dir(): + # symlinks=True: copy symlinks as symlinks rather than + # recursing into their targets (avoids traversal blowups + # on cyclic/external links in a skills tree). shutil.copytree(source_skills, profile_dir / "skills", symlinks=True, dirs_exist_ok=True) + # Source skills tree may carry Nix-store-inherited read-only + # mode bits (pre-fix Hermes builds); restore writability so + # curator / skill_manage on the new profile work in-place. + # _make_tree_owner_writable skips symlink entries so it never + # chmods through a copied symlink into its external target. + from tools.skills_sync import _make_tree_owner_writable + + _make_tree_owner_writable(profile_dir / "skills") # Clone memory and other subdirectory files for relpath in _CLONE_SUBDIR_FILES: diff --git a/tests/hermes_cli/test_profiles.py b/tests/hermes_cli/test_profiles.py index c61bc36dd4b..1fe12981836 100644 --- a/tests/hermes_cli/test_profiles.py +++ b/tests/hermes_cli/test_profiles.py @@ -254,6 +254,55 @@ class TestCreateProfile: / "SKILL.md" ).read_text() == "---\nname: installed-skill\n---\n" + def test_clone_config_skills_with_external_symlink_does_not_chmod_target( + self, profile_env + ): + """Regression: the skills tree is cloned with ``symlinks=True`` so a + skill that is itself a symlink to something outside the profile (a + shared/vendored skill directory, a dev checkout) is copied as a + symlink rather than traversed into. The post-copy writable-mode + repair must not follow that symlink and chmod the external target — + doing so would mutate a file the clone has no business touching. + """ + import os + import stat + + tmp_path = profile_env + default_home = tmp_path / ".hermes" + + # An external skill directory living outside any profile, locked + # down read-only (e.g. a Nix-store-backed shared skill mount). + external_target = tmp_path / "external-shared-skill" + external_target.mkdir(parents=True) + (external_target / "SKILL.md").write_text("---\nname: shared\n---\n") + os.chmod(external_target / "SKILL.md", 0o444) + os.chmod(external_target, 0o555) + + skills_dir = default_home / "skills" / "custom" + skills_dir.mkdir(parents=True) + link_path = skills_dir / "linked-skill" + link_path.symlink_to(external_target, target_is_directory=True) + + try: + profile_dir = create_profile("coder", clone_config=True, no_alias=True) + + cloned_link = profile_dir / "skills" / "custom" / "linked-skill" + assert cloned_link.is_symlink(), ( + "symlinks=True must copy the symlink itself, not its target's " + "contents" + ) + + # The external target's mode bits must be untouched by the + # clone's writable-mode repair sweep. + assert stat.S_IMODE(os.stat(external_target).st_mode) == 0o555 + assert ( + stat.S_IMODE(os.stat(external_target / "SKILL.md").st_mode) + == 0o444 + ) + finally: + os.chmod(external_target / "SKILL.md", 0o644) + os.chmod(external_target, 0o755) + def test_clone_all_copies_entire_tree(self, profile_env): tmp_path = profile_env default_home = tmp_path / ".hermes" diff --git a/tests/tools/test_skills_sync.py b/tests/tools/test_skills_sync.py index 42d59d78e1e..7f454a85742 100644 --- a/tests/tools/test_skills_sync.py +++ b/tests/tools/test_skills_sync.py @@ -7,7 +7,11 @@ from pathlib import Path from unittest.mock import patch from tools.skills_sync import ( + _copy_file_writable, + _copytree_writable, + _ensure_owner_writable, _get_bundled_dir, + _make_tree_owner_writable, _read_manifest, _read_skill_name, _write_manifest, @@ -1118,14 +1122,10 @@ class TestResetBundledSkill: assert result["ok"] is True assert result["action"] == "restored" - # Bundled version was re-copied over the (deleted) user copy. assert "upstream" in (dest / "SKILL.md").read_text() - # The read-only nested user dir/file was fully removed, not left behind. assert not (sub / "ref.md").exists() - # sync ran and re-copied the skill (not stuck in limbo). assert "google-workspace" in result["synced"]["copied"] finally: - # Restore perms so tmp_path teardown can remove anything left. for p in (sub, dest): if p.exists(): os.chmod(p, stat.S_IRWXU) @@ -1145,8 +1145,6 @@ class TestResetBundledSkill: "google-workspace:STALEHASH000000000000000000000000\n" ) - # Simulate an unremovable tree (e.g. a busy mountpoint or a path even - # chmod can't rescue) by making the removal helper raise. def _boom(_path): raise PermissionError(13, "Permission denied") @@ -1155,16 +1153,306 @@ class TestResetBundledSkill: ): result = reset_bundled_skill("google-workspace", restore=True) - # Restore failed, and the manifest must be left untouched. assert result["ok"] is False assert result["action"] == "not_reset" assert "Manifest entry preserved" in result["message"] manifest_after = manifest_file.read_text() assert "google-workspace" in manifest_after - # User copy is still on disk (we changed nothing). assert (dest / "SKILL.md").exists() +class TestEnsureOwnerWritable: + """Unit tests for the writable-mode helpers.""" + + def test_handles_missing_path(self, tmp_path): + # Should not raise on missing path + _ensure_owner_writable(tmp_path / "does-not-exist") + + def test_grants_user_write_to_readonly_file(self, tmp_path): + import os + import stat as stat_mod + + f = tmp_path / "readonly.md" + f.write_text("x") + os.chmod(f, 0o444) # mimic Nix store mode + + _ensure_owner_writable(f) + + assert os.stat(f).st_mode & stat_mod.S_IWUSR + + def test_preserves_executable_bit(self, tmp_path): + import os + import stat as stat_mod + + script = tmp_path / "skill.sh" + script.write_text("#!/bin/sh\n") + os.chmod(script, 0o555) # executable, read-only — Nix-store-style + + _ensure_owner_writable(script) + + m = stat_mod.S_IMODE(os.stat(script).st_mode) + assert m & stat_mod.S_IWUSR + assert m & stat_mod.S_IXUSR # executable still set + + def test_idempotent_on_writable_target(self, tmp_path): + import os + import stat as stat_mod + + f = tmp_path / "ok.txt" + f.write_text("x") + before = os.stat(f).st_mode + + _ensure_owner_writable(f) + + # Same or only the (already-set) user-write bit changed + after = os.stat(f).st_mode + assert after | stat_mod.S_IWUSR == before | stat_mod.S_IWUSR + + +class TestCopyFileWritable: + """``_copy_file_writable`` is the ``copy_function`` for copytree, plus a + drop-in replacement for ``shutil.copy2``.""" + + def test_readonly_source_yields_writable_destination(self, tmp_path): + import os + import stat as stat_mod + + src = tmp_path / "src.md" + dst = tmp_path / "dst.md" + src.write_text("hello") + os.chmod(src, 0o444) + + _copy_file_writable(src, dst) + + assert dst.read_text() == "hello" + assert os.stat(dst).st_mode & stat_mod.S_IWUSR + + def test_executable_source_keeps_executable_bit(self, tmp_path): + import os + import stat as stat_mod + + src = tmp_path / "script.sh" + dst = tmp_path / "script-copy.sh" + src.write_text("#!/bin/sh\n") + os.chmod(src, 0o555) + + _copy_file_writable(src, dst) + + m = stat_mod.S_IMODE(os.stat(dst).st_mode) + assert m & stat_mod.S_IWUSR + assert m & stat_mod.S_IXUSR + + +class TestMakeTreeOwnerWritable: + """``_make_tree_owner_writable`` walks an existing tree and grants the + owner-write bit to every entry.""" + + def test_grants_user_write_to_all_descendants(self, tmp_path): + import os + import stat as stat_mod + + sub = tmp_path / "sub" + nested = sub / "nested" + nested.mkdir(parents=True) + f = nested / "file.txt" + f.write_text("x") + # Lock everything down depth-first like the Nix store would. + os.chmod(f, 0o444) + os.chmod(nested, 0o555) + os.chmod(sub, 0o555) + os.chmod(tmp_path, 0o555) + + try: + _make_tree_owner_writable(tmp_path) + + assert os.stat(tmp_path).st_mode & stat_mod.S_IWUSR + assert os.stat(sub).st_mode & stat_mod.S_IWUSR + assert os.stat(nested).st_mode & stat_mod.S_IWUSR + assert os.stat(f).st_mode & stat_mod.S_IWUSR + finally: + # Restore writable mode on the whole tree so pytest tmp_path + # teardown does not error if an assertion above failed midway. + os.chmod(tmp_path, 0o755) + os.chmod(sub, 0o755) + os.chmod(nested, 0o755) + os.chmod(f, 0o644) + + def test_handles_missing_root(self, tmp_path): + # No-op on missing path + _make_tree_owner_writable(tmp_path / "ghost") + + +class TestCopytreeWritable: + """End-to-end: ``_copytree_writable`` produces a fully editable copy of + a read-only source tree.""" + + def test_readonly_source_tree_yields_editable_destination(self, tmp_path): + import os + import stat as stat_mod + + src = tmp_path / "src" + nested = src / "category" / "skill-x" + nested.mkdir(parents=True) + (nested / "SKILL.md").write_text("# X\n") + (nested / "main.py").write_text("print(1)\n") + # Apply Nix-store-style modes depth-first. + for path in sorted(src.rglob("*"), reverse=True): + os.chmod(path, 0o444 if path.is_file() else 0o555) + os.chmod(src, 0o555) + + dst = tmp_path / "dst" + try: + _copytree_writable(src, dst) + + sk = dst / "category" / "skill-x" / "SKILL.md" + assert sk.exists() + assert os.stat(sk).st_mode & stat_mod.S_IWUSR + assert os.stat(dst / "category" / "skill-x").st_mode & stat_mod.S_IWUSR + # Append-edit must succeed (this is the regression: previously + # raised PermissionError because mode 0444 was preserved). + with sk.open("a") as fh: + fh.write("\nappended\n") + finally: + for path in sorted(src.rglob("*"), reverse=True): + os.chmod(path, 0o755 if path.is_dir() else 0o644) + os.chmod(src, 0o755) + + +class TestSyncSkillsReadOnlyBundledSource: + """Regression: bundled skills served from a read-only filesystem (such as + the Nix store, mode 0444/0555) must end up writable in ~/.hermes/skills/. + + Without the fix, the user copy inherits the read-only mode bits via + ``shutil.copy2`` / ``shutil.copytree`` and a later ``skill_manage`` / + curator edit fails with ``PermissionError``. + """ + + def _setup_readonly_bundled(self, tmp_path): + bundled = tmp_path / "bundled_skills_ro" + skill_dir = bundled / "category" / "ro-skill" + skill_dir.mkdir(parents=True) + (skill_dir / "SKILL.md").write_text( + "---\nname: ro-skill\n---\n# RO\n" + ) + (skill_dir / "main.py").write_text("print(1)\n") + # Lock down depth-first like the Nix store. + import os + + for path in sorted(bundled.rglob("*"), reverse=True): + if path.is_dir(): + os.chmod(path, 0o555) + else: + os.chmod(path, 0o444) + os.chmod(bundled, 0o555) + return bundled + + def _patches(self, bundled, skills_dir, manifest_file): + from contextlib import ExitStack + + stack = ExitStack() + stack.enter_context( + patch("tools.skills_sync._get_bundled_dir", return_value=bundled) + ) + stack.enter_context( + patch("tools.skills_sync.SKILLS_DIR", skills_dir) + ) + stack.enter_context( + patch("tools.skills_sync.MANIFEST_FILE", manifest_file) + ) + return stack + + def test_fresh_install_user_copy_is_writable(self, tmp_path): + import os + import stat as stat_mod + + bundled = self._setup_readonly_bundled(tmp_path) + skills_dir = tmp_path / "user_skills" + manifest_file = skills_dir / ".bundled_manifest" + + try: + with self._patches(bundled, skills_dir, manifest_file): + result = sync_skills(quiet=True) + + assert result["copied"] == ["ro-skill"] + user_skill = skills_dir / "category" / "ro-skill" / "SKILL.md" + assert user_skill.exists() + assert os.stat(user_skill).st_mode & stat_mod.S_IWUSR, ( + "User copy of bundled skill must be writable after sync; see " + "tools.skills_sync._copytree_writable" + ) + # skill_manage emulation: append must succeed. + with user_skill.open("a") as fh: + fh.write("\nappended\n") + finally: + # Restore writability on bundled tree so pytest tmp_path + # teardown does not fail. + for path in sorted(bundled.rglob("*"), reverse=True): + os.chmod(path, 0o755 if path.is_dir() else 0o644) + os.chmod(bundled, 0o755) + + def test_preexisting_hash_identical_readonly_copy_is_repaired(self, tmp_path): + """Migration regression: a user copy made *before* the writable-copy + fix landed can be hash-identical to the bundled source (so the sync + logic takes the "bundled unchanged, user unchanged" no-op branch) + while still carrying the inherited 0444/0555 mode bits from that + earlier, unfixed copy. Nothing about the content differs, so this + skill would otherwise never reach an update/copy path that could + repair it — it would stay unwritable forever. sync_skills must sweep + write-permissions onto it on every run regardless. + """ + import os + import stat as stat_mod + + bundled = self._setup_readonly_bundled(tmp_path) + skills_dir = tmp_path / "user_skills" + manifest_file = skills_dir / ".bundled_manifest" + + # Pre-seed a user copy that is byte-identical to the bundled skill + # (simulating a copy made by a pre-fix Hermes build) and lock it + # down to Nix-store-style read-only modes, with a manifest entry + # already recording the matching origin hash. + bundled_skill_dir = bundled / "category" / "ro-skill" + bundled_hash = _dir_hash(bundled_skill_dir) + + dest = skills_dir / "category" / "ro-skill" + dest.mkdir(parents=True) + (dest / "SKILL.md").write_text( + (bundled_skill_dir / "SKILL.md").read_text() + ) + (dest / "main.py").write_text((bundled_skill_dir / "main.py").read_text()) + os.chmod(dest / "SKILL.md", 0o444) + os.chmod(dest / "main.py", 0o444) + os.chmod(dest, 0o555) + + manifest_file.parent.mkdir(parents=True, exist_ok=True) + manifest_file.write_text(f"ro-skill:{bundled_hash}\n") + + try: + with self._patches(bundled, skills_dir, manifest_file): + result = sync_skills(quiet=True) + + # No-op from the sync's perspective: not re-copied, just repaired. + assert "ro-skill" not in result["copied"] + assert "ro-skill" not in result["updated"] + + user_skill = dest / "SKILL.md" + assert os.stat(user_skill).st_mode & stat_mod.S_IWUSR, ( + "Pre-existing hash-identical read-only user copy must be " + "repaired in place by sync_skills" + ) + assert os.stat(dest).st_mode & stat_mod.S_IWUSR + # skill_manage emulation: append must succeed. + with user_skill.open("a") as fh: + fh.write("\nappended\n") + finally: + for path in sorted(bundled.rglob("*"), reverse=True): + os.chmod(path, 0o755 if path.is_dir() else 0o644) + os.chmod(bundled, 0o755) + for path in sorted(dest.rglob("*"), reverse=True): + os.chmod(path, 0o755 if path.is_dir() else 0o644) + os.chmod(dest, 0o755) + + class TestNoBundledSkillsOptOut: """The .no-bundled-skills marker makes sync_skills() a no-op. diff --git a/tools/skills_sync.py b/tools/skills_sync.py index 2c0f41c47a6..957a0987e67 100644 --- a/tools/skills_sync.py +++ b/tools/skills_sync.py @@ -26,6 +26,7 @@ import json import logging import os import shutil +import stat from datetime import datetime, timezone from pathlib import Path, PurePosixPath from hermes_constants import get_bundled_skills_dir, get_hermes_home, get_optional_skills_dir @@ -36,6 +37,73 @@ from utils import atomic_replace logger = logging.getLogger(__name__) +def _ensure_owner_writable(path: Path) -> None: + """Add the owner-write bit on ``path`` without dropping any other mode bits. + + The Nix store stores files with mode ``0444`` and directories with ``0555``. + ``shutil.copy2`` and ``shutil.copytree`` preserve those source mode bits on + the destination, leaving user-side copies of bundled skills unmodifiable. + Helpers in this module call this routine on every freshly-copied path so + later edits via ``skill_manage`` / curator / ``shutil.rmtree`` succeed. + + Symlinks are skipped: ``os.chmod`` follows symlinks by default (this + platform has no ``os.chmod(follow_symlinks=False)`` support in the + common case), so chmod-ing a copied symlink would mutate the mode of + whatever external file it still points at rather than anything we own. + Copytree call sites here pass ``symlinks=True``, so real symlink entries + reach this function and must be left alone. + + Failures are logged at DEBUG and swallowed: the worst case is the original + (read-only) behaviour, which the caller can already cope with. + """ + if path.is_symlink(): + return + try: + mode = stat.S_IMODE(os.stat(path).st_mode) + os.chmod(path, mode | stat.S_IWUSR) + except OSError as exc: + logger.debug("chmod on %s failed: %s", path, exc) + + +def _copy_file_writable(src, dst) -> None: + """Drop-in replacement for ``shutil.copy2`` that always leaves ``dst`` + owner-writable. + + Used as the ``copy_function`` for ``shutil.copytree`` so files in the + destination tree are editable even when the source lives on a read-only + filesystem (Nix store / immutable OCI image layer / squashfs). + """ + shutil.copy2(src, dst) + _ensure_owner_writable(Path(dst)) + + +def _make_tree_owner_writable(root: Path) -> None: + """Apply :func:`_ensure_owner_writable` to ``root`` and every descendant. + + ``shutil.copytree`` reapplies source-directory metadata (mode, mtime) to + each copied subdirectory *after* file copies, so the per-file ``copy2`` + override above is not enough on its own. We sweep the tree once afterwards + to grant the user-write bit on directories too. + """ + if not root.exists(): + return + _ensure_owner_writable(root) + for path in root.rglob("*"): + _ensure_owner_writable(path) + + +def _copytree_writable(src, dst) -> None: + """Like ``shutil.copytree`` but the destination is always owner-writable. + + Use this in place of ``shutil.copytree`` whenever the source might come + from a read-only filesystem (Nix store, container image layer, squashfs). + The per-file copy uses :func:`_copy_file_writable`; afterwards directory + modes are restored via :func:`_make_tree_owner_writable`. + """ + shutil.copytree(src, dst, copy_function=_copy_file_writable) + _make_tree_owner_writable(Path(dst)) + + HERMES_HOME = get_hermes_home() SKILLS_DIR = HERMES_HOME / "skills" MANIFEST_FILE = SKILLS_DIR / ".bundled_manifest" @@ -371,7 +439,9 @@ def restore_official_optional_skill(name: str, *, restore: bool = False) -> dict backed_up.append(_move_to_restore_backup(dest, backup_root)) if not dest.exists(): dest.parent.mkdir(parents=True, exist_ok=True) - shutil.copytree(src, dest) + # Bundled source may live on a read-only Nix store; restore + # owner-writable mode on the destination so future edits work. + _copytree_writable(src, dest) restored.append(folder_name) elif not canonical_ok: continue @@ -606,7 +676,9 @@ def sync_skills(quiet: bool = False) -> dict: ) else: dest.parent.mkdir(parents=True, exist_ok=True) - shutil.copytree(skill_src, dest) + # Bundled source may live on a read-only Nix store; the + # writable wrapper restores owner-write on the new copy. + _copytree_writable(skill_src, dest) copied.append(skill_name) manifest[skill_name] = bundled_hash if not quiet: @@ -627,6 +699,12 @@ def sync_skills(quiet: bool = False) -> dict: manifest[skill_name] = user_hash if user_hash == bundled_hash: skipped += 1 # already in sync + # Same read-only-mode-bit migration as the "bundled + # unchanged, user unchanged" branch below: an identical + # copy predating the writable-copy fix may still be + # unwritable, and content parity means it will never + # hit the update path to get repaired otherwise. + _make_tree_owner_writable(dest) else: # Can't tell if user modified or bundled changed — be safe skipped += 1 @@ -652,7 +730,12 @@ def sync_skills(quiet: bool = False) -> dict: _rmtree_writable(backup) shutil.move(str(dest), str(backup)) try: - shutil.copytree(skill_src, dest) + # Writable wrapper: bundled source may live on a + # read-only Nix store. Backup also gets the + # owner-write bit restored so the rmtree below + # cannot silently leak the *.bak directory. + _copytree_writable(skill_src, dest) + _make_tree_owner_writable(backup) manifest[skill_name] = bundled_hash updated.append(skill_name) if not quiet: @@ -683,6 +766,13 @@ def sync_skills(quiet: bool = False) -> dict: print(f" ! Failed to update {skill_name}: {e}") else: skipped += 1 # bundled unchanged, user unchanged + # Migration for installs that predate the writable-copy fix: + # the user copy may still carry read-only mode bits inherited + # from a Nix-store bundled source at the time it was first + # copied. Repair it in place on every sync so it doesn't stay + # unwritable forever just because the content never changed + # again afterwards. + _make_tree_owner_writable(dest) else: # ── In manifest but not on disk — user deleted it ── @@ -700,7 +790,9 @@ def sync_skills(quiet: bool = False) -> dict: if not dest_desc.exists(): try: dest_desc.parent.mkdir(parents=True, exist_ok=True) - shutil.copy2(desc_md, dest_desc) + # Writable wrapper: bundled source may live on a read-only + # Nix store; preserve copy2 metadata + grant owner-write. + _copy_file_writable(desc_md, dest_desc) except (OSError, IOError) as e: logger.debug("Could not copy %s: %s", desc_md, e)