From 78de8f532283b466280f8556c3cb555bfdf26bc5 Mon Sep 17 00:00:00 2001 From: John Paul Soliva Date: Sat, 26 Sep 2026 15:59:22 +0900 Subject: [PATCH] fix(profiles): an owned skill category keeps the skills an installer added to it A distribution that lists a category in distribution_owned (the docs' own example is `skills/research/`) had that category replaced wholesale on update. `hermes skills install` and agent-created skills land in skills// too, so an installer's own skill in the same category was deleted, the #25120 loss the per-root merge already fixed for the default top-level ownership: after update: ['web-search'] # the installer's my-notes skill is gone An owned directory that holds no files of its own is a container of roots at any depth, so it is merged per root like a top-level one: the author's skills are replaced whole and the installer's survive. An owned skill root (it holds SKILL.md) is still replaced whole. Symlinked sub-containers are refused before the first write, as they already are under a top-level owned dir. (cherry picked from commit 6e35c6e27096ba83588d6b2ded1591f55046b38a) --- hermes_cli/profile_distribution.py | 11 +++- tests/hermes_cli/test_profile_distribution.py | 63 +++++++++++++++++++ 2 files changed, 72 insertions(+), 2 deletions(-) diff --git a/hermes_cli/profile_distribution.py b/hermes_cli/profile_distribution.py index bc75209d75..3c486935e3 100644 --- a/hermes_cli/profile_distribution.py +++ b/hermes_cli/profile_distribution.py @@ -483,7 +483,7 @@ def _refuse_symlinked_targets(target: Path, entries) -> None: for part in rel_parts[:depth]: path = path / part _refuse_symlink(path) - if src.is_dir() and len(rel_parts) == 1: + if src.is_dir() and (len(rel_parts) == 1 or _is_container(src)): _refuse_symlinked_containers(src, path, rel_parts) @@ -494,7 +494,8 @@ def _copy_dist_payload(staged: Path, target: Path, manifest: DistributionManifes ``preserve_config`` is False (fresh install / ``--force-config``). ``.env.template`` lands as ``.env.EXAMPLE`` so it never shadows a real ``.env``. - A top-level owned directory is merged per authored root. ``cron/jobs.json`` is + A top-level owned directory, and an owned category holding only roots, is merged per + authored root. ``cron/jobs.json`` is special: it is one multi-record runtime store, so shipped definitions merge by job id instead of replacing the file.""" target.mkdir(parents=True, exist_ok=True) @@ -522,6 +523,12 @@ def _copy_dist_payload(staged: Path, target: Path, manifest: DistributionManifes if src.is_dir(): _merge_dir(src, _real_dir(target, rel_parts), rel_parts) continue + elif _is_container(src): + # An owned category (``skills/research/``) holds skill roots, not files: merge it per + # root like a top-level dir, so skills the installer added to it (``hermes skills + # install`` and agent-created skills land in ``skills//``) survive. + _merge_dir(src, _real_dir(target, rel_parts), rel_parts) + continue _replace_entry(src, _real_dir(target, rel_parts[:-1]) / rel_parts[-1]) # Emit .env.EXAMPLE from manifest if the staged tree didn't ship one diff --git a/tests/hermes_cli/test_profile_distribution.py b/tests/hermes_cli/test_profile_distribution.py index 58d9e3d136..a023a934f0 100644 --- a/tests/hermes_cli/test_profile_distribution.py +++ b/tests/hermes_cli/test_profile_distribution.py @@ -464,6 +464,69 @@ class TestUpdate: assert (custom / "SKILL.md").read_text(encoding="utf-8") == "custom skill\n" assert (plan.target_dir / "cron" / "mine.json").exists() + @staticmethod + def _owned_category(profile_env, name): + """A distribution owning only ``skills/research/`` (the docs' own example) plus SOUL.md.""" + mf = DistributionManifest(name=name, version="0.1.0", distribution_owned=["SOUL.md", "skills/research/"]) + staged = _make_staging_dir(profile_env, name, manifest=mf) + (staged / "skills" / "research" / "web-search").mkdir(parents=True) + (staged / "skills" / "research" / "web-search" / "SKILL.md").write_text("author skill\n", encoding="utf-8") + return staged, install_distribution(str(staged), name=name) + + def test_an_owned_category_keeps_skills_the_installer_added_to_it(self, profile_env): + """``distribution_owned: [skills/research/]`` owns the author's research skills, not the + category: ``hermes skills install`` and agent-created skills land in ``skills//`` + too. The category was replaced wholesale, deleting them (the #25120 loss, still live for + the explicit form the docs show).""" + staged, plan = self._owned_category(profile_env, "rb") + research = plan.target_dir / "skills" / "research" + mine = research / "my-notes" + mine.mkdir() + (mine / "SKILL.md").write_text("my own skill\n", encoding="utf-8") + (research / "web-search" / "stale.txt").write_text("old\n", encoding="utf-8") + (staged / "skills" / "research" / "web-search" / "SKILL.md").write_text("author v2\n", encoding="utf-8") + (staged / "skills" / "research" / "arxiv").mkdir() + (staged / "skills" / "research" / "arxiv" / "SKILL.md").write_text("new author skill\n", encoding="utf-8") + + update_distribution("rb") + + assert (mine / "SKILL.md").read_text(encoding="utf-8") == "my own skill\n" + assert (research / "web-search" / "SKILL.md").read_text(encoding="utf-8") == "author v2\n" + assert not (research / "web-search" / "stale.txt").exists() # an owned root is still replaced whole + assert (research / "arxiv" / "SKILL.md").exists() + + def test_an_owned_skill_root_is_still_replaced_whole(self, profile_env): + """A listed skill dir holds SKILL.md: it is a root, so files retired upstream disappear.""" + mf = DistributionManifest(name="one", version="0.1.0", + distribution_owned=["SOUL.md", "skills/research/web-search/"]) + staged = _make_staging_dir(profile_env, "one", manifest=mf) + (staged / "skills" / "research" / "web-search").mkdir(parents=True) + (staged / "skills" / "research" / "web-search" / "SKILL.md").write_text("author\n", encoding="utf-8") + plan = install_distribution(str(staged), name="one") + stale = plan.target_dir / "skills" / "research" / "web-search" / "stale.txt" + stale.write_text("old\n", encoding="utf-8") + + update_distribution("one") + + assert not stale.exists() + + def test_an_owned_category_refuses_a_symlinked_subcategory_before_writing(self, profile_env, tmp_path): + staged, plan = self._owned_category(profile_env, "rb") + (staged / "skills" / "research" / "papers" / "summarize").mkdir(parents=True) + (staged / "skills" / "research" / "papers" / "summarize" / "SKILL.md").write_text("s\n", encoding="utf-8") + outside = tmp_path / "outside" + outside.mkdir() + (plan.target_dir / "skills" / "research" / "papers").symlink_to(outside, target_is_directory=True) + soul = plan.target_dir / "SOUL.md" + before = soul.read_text(encoding="utf-8") + (staged / "SOUL.md").write_text("changed\n", encoding="utf-8") + + with pytest.raises(DistributionError, match="symlink"): + update_distribution("rb") + + assert not any(outside.iterdir()) + assert soul.read_text(encoding="utf-8") == before # refused before the first write + def test_update_merges_cron_jobs_without_losing_local_state(self, profile_env): """Updating one shipped definition cannot replace the profile's whole cron store.""" from cron.jobs import create_job, list_jobs, pause_job, resume_job, update_job, use_cron_store