mirror of
https://github.com/NousResearch/hermes-agent.git
synced 2026-07-31 19:16:29 +00:00
fix(skills_sync): keep bundled skill copies writable on Nix store
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).
This commit is contained in:
parent
cbc1054e23
commit
8c2450fcf9
6 changed files with 505 additions and 13 deletions
36
AGENTS.md
36
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
|
||||
|
|
|
|||
|
|
@ -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():
|
||||
|
|
|
|||
|
|
@ -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:
|
||||
|
|
|
|||
|
|
@ -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"
|
||||
|
|
|
|||
|
|
@ -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.
|
||||
|
||||
|
|
|
|||
|
|
@ -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)
|
||||
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue