From 3a97fb3d477299637a6cb6253cad705b38388733 Mon Sep 17 00:00:00 2001 From: j0sephz Date: Thu, 23 Apr 2026 07:48:10 +0300 Subject: [PATCH] fix(skills_sync): don't poison manifest on new-skill collision MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When a new bundled skill's name collided with a pre-existing user skill (from hub, custom, or leftover), sync_skills() recorded the bundled hash in the manifest even though the on-disk copy was unrelated to bundled. On the next sync, user_hash != origin_hash (bundled_hash) marked the skill as "user-modified" permanently, blocking all bundled updates for that skill until the user ran `hermes skills reset`. Fix: only baseline the manifest entry when the user's on-disk copy is byte-identical to bundled (safe to track — this is the reset re-sync or coincidentally-identical install case). Otherwise skip the manifest write entirely: the on-disk skill is unrelated to bundled and shouldn't be tracked as if it were. This preserves reset_bundled_skill()'s re-baseline flow (its post-delete sync still writes to the manifest when user copy matches bundled) while fixing the poisoning scenario for genuinely unrelated collisions. Adds two tests following the existing test_failed_copy_does_not_poison_manifest pattern: one verifying the manifest stays clean after a collision with differing content, one verifying no false user_modified flag on resync. --- tests/tools/test_skills_sync.py | 58 +++++++++++++++++++++++++++++++++ tools/skills_sync.py | 13 ++++++-- 2 files changed, 69 insertions(+), 2 deletions(-) diff --git a/tests/tools/test_skills_sync.py b/tests/tools/test_skills_sync.py index 683f6503b..668c4c74e 100644 --- a/tests/tools/test_skills_sync.py +++ b/tests/tools/test_skills_sync.py @@ -402,6 +402,64 @@ class TestSyncSkills: assert (user_skill / "SKILL.md").read_text() == "# User modified" + def test_collision_does_not_poison_manifest(self, tmp_path): + """Collision with an unmanifested user skill must NOT record bundled_hash. + + Otherwise the next sync compares user_hash against the recorded + bundled_hash, finds a mismatch, and permanently flags the skill as + 'user-modified' — even though the user never touched a bundled copy. + """ + bundled = self._setup_bundled(tmp_path) + skills_dir = tmp_path / "user_skills" + manifest_file = skills_dir / ".bundled_manifest" + + # Pre-existing user skill (e.g. from hub, custom, or leftover) that + # happens to share a name with a newly bundled skill. + user_skill = skills_dir / "category" / "new-skill" + user_skill.mkdir(parents=True) + (user_skill / "SKILL.md").write_text("# From hub — unrelated to bundled") + + with self._patches(bundled, skills_dir, manifest_file): + sync_skills(quiet=True) + + # User file must survive (existing invariant). + assert (user_skill / "SKILL.md").read_text() == ( + "# From hub — unrelated to bundled" + ) + + # Manifest must NOT contain the skill — it was never synced from bundled. + with patch("tools.skills_sync.MANIFEST_FILE", manifest_file): + manifest = _read_manifest() + assert "new-skill" not in manifest, ( + "Collision path wrote bundled_hash to the manifest even though " + "the on-disk copy is unrelated to bundled. This poisons update " + "detection: the next sync will mark the skill as 'user-modified'." + ) + + def test_collision_does_not_trigger_false_user_modified_on_resync(self, tmp_path): + """End-to-end: after a collision, a second sync must not flag user_modified. + + Pre-fix bug: first sync wrote bundled_hash to the manifest; second + sync then diffed user_hash vs bundled_hash, mismatched, and shoved + the skill into the user_modified bucket forever. + """ + bundled = self._setup_bundled(tmp_path) + skills_dir = tmp_path / "user_skills" + manifest_file = skills_dir / ".bundled_manifest" + + user_skill = skills_dir / "category" / "new-skill" + user_skill.mkdir(parents=True) + (user_skill / "SKILL.md").write_text("# From hub — unrelated to bundled") + + with self._patches(bundled, skills_dir, manifest_file): + sync_skills(quiet=True) # first sync: collision path + result2 = sync_skills(quiet=True) # second sync: must not flag + + assert "new-skill" not in result2["user_modified"], ( + "Second sync after a collision falsely flagged the user's skill " + "as 'user-modified' — the manifest was poisoned on the first sync." + ) + def test_nonexistent_bundled_dir(self, tmp_path): with patch("tools.skills_sync._get_bundled_dir", return_value=tmp_path / "nope"): result = sync_skills(quiet=True) diff --git a/tools/skills_sync.py b/tools/skills_sync.py index 867566b6c..3acaf2fbb 100644 --- a/tools/skills_sync.py +++ b/tools/skills_sync.py @@ -206,9 +206,18 @@ def sync_skills(quiet: bool = False) -> dict: # ── New skill — never offered before ── try: if dest.exists(): - # User already has a skill with the same name — don't overwrite + # User already has a skill with the same name — don't overwrite. + # Only baseline in the manifest when the on-disk copy is + # byte-identical to bundled (e.g. a reset that re-syncs, or + # a coincidentally identical install); that case is harmless + # to track. If the copy differs (custom skill, hub-installed, + # or user-edited) skip the manifest write: recording + # bundled_hash there would poison update detection by making + # user_hash != origin_hash read as "user-modified" on every + # subsequent sync, permanently blocking bundled updates. skipped += 1 - manifest[skill_name] = bundled_hash + if _dir_hash(dest) == bundled_hash: + manifest[skill_name] = bundled_hash else: dest.parent.mkdir(parents=True, exist_ok=True) shutil.copytree(skill_src, dest)