fix: track origin hashes in skills manifest to preserve user modifications
Upgrade skills_sync manifest to v2 format (name:origin_hash). The origin hash records the MD5 of the bundled skill at the time it was last synced. On update, the user's copy is compared against the origin hash: - User copy == origin hash → unmodified → safe to update from bundled - User copy != origin hash → user customized → skip (preserve changes) v1 manifests (plain names) are auto-migrated: the user's current hash becomes the baseline, so future syncs can detect modifications. Output now shows user-modified skills: ~ whisper (user-modified, skipping) 27 tests covering all scenarios including v1→v2 migration, user modification detection, update after migration, and origin hash tracking. 2009 tests pass.
This commit is contained in:
parent
6d3804770c
commit
4f56e31dc7
2 changed files with 280 additions and 102 deletions
|
|
@ -17,42 +17,62 @@ from tools.skills_sync import (
|
||||||
|
|
||||||
class TestReadWriteManifest:
|
class TestReadWriteManifest:
|
||||||
def test_read_missing_manifest(self, tmp_path):
|
def test_read_missing_manifest(self, tmp_path):
|
||||||
with patch.object(
|
with patch(
|
||||||
__import__("tools.skills_sync", fromlist=["MANIFEST_FILE"]),
|
"tools.skills_sync.MANIFEST_FILE",
|
||||||
"MANIFEST_FILE",
|
|
||||||
tmp_path / "nonexistent",
|
tmp_path / "nonexistent",
|
||||||
):
|
):
|
||||||
result = _read_manifest()
|
result = _read_manifest()
|
||||||
assert result == set()
|
assert result == {}
|
||||||
|
|
||||||
def test_write_and_read_roundtrip(self, tmp_path):
|
def test_write_and_read_roundtrip_v2(self, tmp_path):
|
||||||
manifest_file = tmp_path / ".bundled_manifest"
|
manifest_file = tmp_path / ".bundled_manifest"
|
||||||
names = {"skill-a", "skill-b", "skill-c"}
|
entries = {"skill-a": "abc123", "skill-b": "def456", "skill-c": "789012"}
|
||||||
|
|
||||||
with patch("tools.skills_sync.MANIFEST_FILE", manifest_file):
|
with patch("tools.skills_sync.MANIFEST_FILE", manifest_file):
|
||||||
_write_manifest(names)
|
_write_manifest(entries)
|
||||||
result = _read_manifest()
|
result = _read_manifest()
|
||||||
|
|
||||||
assert result == names
|
assert result == entries
|
||||||
|
|
||||||
def test_write_manifest_sorted(self, tmp_path):
|
def test_write_manifest_sorted(self, tmp_path):
|
||||||
manifest_file = tmp_path / ".bundled_manifest"
|
manifest_file = tmp_path / ".bundled_manifest"
|
||||||
names = {"zebra", "alpha", "middle"}
|
entries = {"zebra": "hash1", "alpha": "hash2", "middle": "hash3"}
|
||||||
|
|
||||||
with patch("tools.skills_sync.MANIFEST_FILE", manifest_file):
|
with patch("tools.skills_sync.MANIFEST_FILE", manifest_file):
|
||||||
_write_manifest(names)
|
_write_manifest(entries)
|
||||||
|
|
||||||
lines = manifest_file.read_text().strip().splitlines()
|
lines = manifest_file.read_text().strip().splitlines()
|
||||||
assert lines == ["alpha", "middle", "zebra"]
|
names = [line.split(":")[0] for line in lines]
|
||||||
|
assert names == ["alpha", "middle", "zebra"]
|
||||||
|
|
||||||
def test_read_manifest_ignores_blank_lines(self, tmp_path):
|
def test_read_v1_manifest_migration(self, tmp_path):
|
||||||
|
"""v1 format (plain names, no hashes) should be read with empty hashes."""
|
||||||
manifest_file = tmp_path / ".bundled_manifest"
|
manifest_file = tmp_path / ".bundled_manifest"
|
||||||
manifest_file.write_text("skill-a\n\n \nskill-b\n")
|
manifest_file.write_text("skill-a\nskill-b\n")
|
||||||
|
|
||||||
with patch("tools.skills_sync.MANIFEST_FILE", manifest_file):
|
with patch("tools.skills_sync.MANIFEST_FILE", manifest_file):
|
||||||
result = _read_manifest()
|
result = _read_manifest()
|
||||||
|
|
||||||
assert result == {"skill-a", "skill-b"}
|
assert result == {"skill-a": "", "skill-b": ""}
|
||||||
|
|
||||||
|
def test_read_manifest_ignores_blank_lines(self, tmp_path):
|
||||||
|
manifest_file = tmp_path / ".bundled_manifest"
|
||||||
|
manifest_file.write_text("skill-a:hash1\n\n \nskill-b:hash2\n")
|
||||||
|
|
||||||
|
with patch("tools.skills_sync.MANIFEST_FILE", manifest_file):
|
||||||
|
result = _read_manifest()
|
||||||
|
|
||||||
|
assert result == {"skill-a": "hash1", "skill-b": "hash2"}
|
||||||
|
|
||||||
|
def test_read_manifest_mixed_v1_v2(self, tmp_path):
|
||||||
|
"""Manifest with both v1 and v2 lines (shouldn't happen but handle gracefully)."""
|
||||||
|
manifest_file = tmp_path / ".bundled_manifest"
|
||||||
|
manifest_file.write_text("old-skill\nnew-skill:abc123\n")
|
||||||
|
|
||||||
|
with patch("tools.skills_sync.MANIFEST_FILE", manifest_file):
|
||||||
|
result = _read_manifest()
|
||||||
|
|
||||||
|
assert result == {"old-skill": "", "new-skill": "abc123"}
|
||||||
|
|
||||||
|
|
||||||
class TestDirHash:
|
class TestDirHash:
|
||||||
|
|
@ -87,13 +107,10 @@ class TestDirHash:
|
||||||
|
|
||||||
class TestDiscoverBundledSkills:
|
class TestDiscoverBundledSkills:
|
||||||
def test_finds_skills_with_skill_md(self, tmp_path):
|
def test_finds_skills_with_skill_md(self, tmp_path):
|
||||||
# Create two skills
|
|
||||||
(tmp_path / "category" / "skill-a").mkdir(parents=True)
|
(tmp_path / "category" / "skill-a").mkdir(parents=True)
|
||||||
(tmp_path / "category" / "skill-a" / "SKILL.md").write_text("# Skill A")
|
(tmp_path / "category" / "skill-a" / "SKILL.md").write_text("# Skill A")
|
||||||
(tmp_path / "skill-b").mkdir()
|
(tmp_path / "skill-b").mkdir()
|
||||||
(tmp_path / "skill-b" / "SKILL.md").write_text("# Skill B")
|
(tmp_path / "skill-b" / "SKILL.md").write_text("# Skill B")
|
||||||
|
|
||||||
# A directory without SKILL.md — should NOT be found
|
|
||||||
(tmp_path / "not-a-skill").mkdir()
|
(tmp_path / "not-a-skill").mkdir()
|
||||||
(tmp_path / "not-a-skill" / "README.md").write_text("Not a skill")
|
(tmp_path / "not-a-skill" / "README.md").write_text("Not a skill")
|
||||||
|
|
||||||
|
|
@ -140,105 +157,198 @@ class TestSyncSkills:
|
||||||
(bundled / "old-skill" / "SKILL.md").write_text("# Old")
|
(bundled / "old-skill" / "SKILL.md").write_text("# Old")
|
||||||
return bundled
|
return bundled
|
||||||
|
|
||||||
|
def _patches(self, bundled, skills_dir, manifest_file):
|
||||||
|
"""Return context manager stack for patching sync globals."""
|
||||||
|
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_copies_all(self, tmp_path):
|
def test_fresh_install_copies_all(self, tmp_path):
|
||||||
bundled = self._setup_bundled(tmp_path)
|
bundled = self._setup_bundled(tmp_path)
|
||||||
skills_dir = tmp_path / "user_skills"
|
skills_dir = tmp_path / "user_skills"
|
||||||
manifest_file = skills_dir / ".bundled_manifest"
|
manifest_file = skills_dir / ".bundled_manifest"
|
||||||
|
|
||||||
with patch("tools.skills_sync._get_bundled_dir", return_value=bundled), \
|
with self._patches(bundled, skills_dir, manifest_file):
|
||||||
patch("tools.skills_sync.SKILLS_DIR", skills_dir), \
|
|
||||||
patch("tools.skills_sync.MANIFEST_FILE", manifest_file):
|
|
||||||
result = sync_skills(quiet=True)
|
result = sync_skills(quiet=True)
|
||||||
|
|
||||||
assert len(result["copied"]) == 2
|
assert len(result["copied"]) == 2
|
||||||
assert result["total_bundled"] == 2
|
assert result["total_bundled"] == 2
|
||||||
assert result["updated"] == []
|
assert result["updated"] == []
|
||||||
|
assert result["user_modified"] == []
|
||||||
assert result["cleaned"] == []
|
assert result["cleaned"] == []
|
||||||
assert (skills_dir / "category" / "new-skill" / "SKILL.md").exists()
|
assert (skills_dir / "category" / "new-skill" / "SKILL.md").exists()
|
||||||
assert (skills_dir / "old-skill" / "SKILL.md").exists()
|
assert (skills_dir / "old-skill" / "SKILL.md").exists()
|
||||||
# DESCRIPTION.md should also be copied
|
|
||||||
assert (skills_dir / "category" / "DESCRIPTION.md").exists()
|
assert (skills_dir / "category" / "DESCRIPTION.md").exists()
|
||||||
|
|
||||||
|
def test_fresh_install_records_origin_hashes(self, tmp_path):
|
||||||
|
"""After fresh install, manifest should have v2 format with hashes."""
|
||||||
|
bundled = self._setup_bundled(tmp_path)
|
||||||
|
skills_dir = tmp_path / "user_skills"
|
||||||
|
manifest_file = skills_dir / ".bundled_manifest"
|
||||||
|
|
||||||
|
with self._patches(bundled, skills_dir, manifest_file):
|
||||||
|
sync_skills(quiet=True)
|
||||||
|
manifest = _read_manifest()
|
||||||
|
|
||||||
|
assert "new-skill" in manifest
|
||||||
|
assert "old-skill" in manifest
|
||||||
|
# Hashes should be non-empty MD5 strings
|
||||||
|
assert len(manifest["new-skill"]) == 32
|
||||||
|
assert len(manifest["old-skill"]) == 32
|
||||||
|
|
||||||
def test_user_deleted_skill_not_re_added(self, tmp_path):
|
def test_user_deleted_skill_not_re_added(self, tmp_path):
|
||||||
"""Skill in manifest but not on disk = user deleted it. Don't re-add."""
|
"""Skill in manifest but not on disk = user deleted it. Don't re-add."""
|
||||||
bundled = self._setup_bundled(tmp_path)
|
bundled = self._setup_bundled(tmp_path)
|
||||||
skills_dir = tmp_path / "user_skills"
|
skills_dir = tmp_path / "user_skills"
|
||||||
manifest_file = skills_dir / ".bundled_manifest"
|
manifest_file = skills_dir / ".bundled_manifest"
|
||||||
skills_dir.mkdir(parents=True)
|
skills_dir.mkdir(parents=True)
|
||||||
# old-skill is in manifest but NOT on disk (user deleted it)
|
# old-skill is in manifest (v2 format) but NOT on disk
|
||||||
manifest_file.write_text("old-skill\n")
|
old_hash = _dir_hash(bundled / "old-skill")
|
||||||
|
manifest_file.write_text(f"old-skill:{old_hash}\n")
|
||||||
|
|
||||||
with patch("tools.skills_sync._get_bundled_dir", return_value=bundled), \
|
with self._patches(bundled, skills_dir, manifest_file):
|
||||||
patch("tools.skills_sync.SKILLS_DIR", skills_dir), \
|
|
||||||
patch("tools.skills_sync.MANIFEST_FILE", manifest_file):
|
|
||||||
result = sync_skills(quiet=True)
|
result = sync_skills(quiet=True)
|
||||||
|
|
||||||
# new-skill should be copied, old-skill should be skipped
|
|
||||||
assert "new-skill" in result["copied"]
|
assert "new-skill" in result["copied"]
|
||||||
assert "old-skill" not in result["copied"]
|
assert "old-skill" not in result["copied"]
|
||||||
assert "old-skill" not in result.get("updated", [])
|
assert "old-skill" not in result.get("updated", [])
|
||||||
assert not (skills_dir / "old-skill").exists()
|
assert not (skills_dir / "old-skill").exists()
|
||||||
|
|
||||||
def test_existing_skill_gets_updated(self, tmp_path):
|
def test_unmodified_skill_gets_updated(self, tmp_path):
|
||||||
"""Skill in manifest AND on disk with changed content = updated."""
|
"""Skill in manifest + on disk + user hasn't modified = update from bundled."""
|
||||||
bundled = self._setup_bundled(tmp_path)
|
bundled = self._setup_bundled(tmp_path)
|
||||||
skills_dir = tmp_path / "user_skills"
|
skills_dir = tmp_path / "user_skills"
|
||||||
manifest_file = skills_dir / ".bundled_manifest"
|
manifest_file = skills_dir / ".bundled_manifest"
|
||||||
|
|
||||||
# Pre-create old-skill on disk with DIFFERENT content
|
# Simulate: user has old version that was synced from an older bundled
|
||||||
user_skill = skills_dir / "old-skill"
|
user_skill = skills_dir / "old-skill"
|
||||||
user_skill.mkdir(parents=True)
|
user_skill.mkdir(parents=True)
|
||||||
(user_skill / "SKILL.md").write_text("# Old version from last sync")
|
(user_skill / "SKILL.md").write_text("# Old v1")
|
||||||
# Mark it in the manifest
|
old_origin_hash = _dir_hash(user_skill)
|
||||||
manifest_file.write_text("old-skill\n")
|
|
||||||
|
|
||||||
with patch("tools.skills_sync._get_bundled_dir", return_value=bundled), \
|
# Record origin hash = hash of what was synced (the old version)
|
||||||
patch("tools.skills_sync.SKILLS_DIR", skills_dir), \
|
manifest_file.write_text(f"old-skill:{old_origin_hash}\n")
|
||||||
patch("tools.skills_sync.MANIFEST_FILE", manifest_file):
|
|
||||||
|
# Now bundled has a newer version ("# Old" != "# Old v1")
|
||||||
|
with self._patches(bundled, skills_dir, manifest_file):
|
||||||
result = sync_skills(quiet=True)
|
result = sync_skills(quiet=True)
|
||||||
|
|
||||||
# old-skill should be updated
|
# Should be updated because user copy matches origin (unmodified)
|
||||||
assert "old-skill" in result["updated"]
|
assert "old-skill" in result["updated"]
|
||||||
assert (user_skill / "SKILL.md").read_text() == "# Old"
|
assert (user_skill / "SKILL.md").read_text() == "# Old"
|
||||||
|
|
||||||
def test_unchanged_skill_not_updated(self, tmp_path):
|
def test_user_modified_skill_not_overwritten(self, tmp_path):
|
||||||
"""Skill in manifest AND on disk with same content = skipped."""
|
"""Skill modified by user should NOT be overwritten even if bundled changed."""
|
||||||
bundled = self._setup_bundled(tmp_path)
|
bundled = self._setup_bundled(tmp_path)
|
||||||
skills_dir = tmp_path / "user_skills"
|
skills_dir = tmp_path / "user_skills"
|
||||||
manifest_file = skills_dir / ".bundled_manifest"
|
manifest_file = skills_dir / ".bundled_manifest"
|
||||||
|
|
||||||
# Pre-create old-skill on disk with SAME content
|
# Simulate: user had the old version synced, then modified it
|
||||||
|
user_skill = skills_dir / "old-skill"
|
||||||
|
user_skill.mkdir(parents=True)
|
||||||
|
(user_skill / "SKILL.md").write_text("# Old v1")
|
||||||
|
old_origin_hash = _dir_hash(user_skill)
|
||||||
|
|
||||||
|
# Record origin hash from what was originally synced
|
||||||
|
manifest_file.write_text(f"old-skill:{old_origin_hash}\n")
|
||||||
|
|
||||||
|
# User modifies their copy
|
||||||
|
(user_skill / "SKILL.md").write_text("# My custom version")
|
||||||
|
|
||||||
|
with self._patches(bundled, skills_dir, manifest_file):
|
||||||
|
result = sync_skills(quiet=True)
|
||||||
|
|
||||||
|
# Should NOT update — user modified it
|
||||||
|
assert "old-skill" in result["user_modified"]
|
||||||
|
assert "old-skill" not in result.get("updated", [])
|
||||||
|
assert (user_skill / "SKILL.md").read_text() == "# My custom version"
|
||||||
|
|
||||||
|
def test_unchanged_skill_not_updated(self, tmp_path):
|
||||||
|
"""Skill in sync (user == bundled == origin) = no action needed."""
|
||||||
|
bundled = self._setup_bundled(tmp_path)
|
||||||
|
skills_dir = tmp_path / "user_skills"
|
||||||
|
manifest_file = skills_dir / ".bundled_manifest"
|
||||||
|
|
||||||
|
# Copy bundled to user dir (simulating perfect sync state)
|
||||||
user_skill = skills_dir / "old-skill"
|
user_skill = skills_dir / "old-skill"
|
||||||
user_skill.mkdir(parents=True)
|
user_skill.mkdir(parents=True)
|
||||||
(user_skill / "SKILL.md").write_text("# Old")
|
(user_skill / "SKILL.md").write_text("# Old")
|
||||||
# Mark it in the manifest
|
origin_hash = _dir_hash(user_skill)
|
||||||
manifest_file.write_text("old-skill\n")
|
manifest_file.write_text(f"old-skill:{origin_hash}\n")
|
||||||
|
|
||||||
with patch("tools.skills_sync._get_bundled_dir", return_value=bundled), \
|
with self._patches(bundled, skills_dir, manifest_file):
|
||||||
patch("tools.skills_sync.SKILLS_DIR", skills_dir), \
|
|
||||||
patch("tools.skills_sync.MANIFEST_FILE", manifest_file):
|
|
||||||
result = sync_skills(quiet=True)
|
result = sync_skills(quiet=True)
|
||||||
|
|
||||||
# Should be skipped, not updated
|
|
||||||
assert "old-skill" not in result.get("updated", [])
|
assert "old-skill" not in result.get("updated", [])
|
||||||
|
assert "old-skill" not in result.get("user_modified", [])
|
||||||
assert result["skipped"] >= 1
|
assert result["skipped"] >= 1
|
||||||
|
|
||||||
|
def test_v1_manifest_migration_sets_baseline(self, tmp_path):
|
||||||
|
"""v1 manifest entries (no hash) should set baseline from user's current copy."""
|
||||||
|
bundled = self._setup_bundled(tmp_path)
|
||||||
|
skills_dir = tmp_path / "user_skills"
|
||||||
|
manifest_file = skills_dir / ".bundled_manifest"
|
||||||
|
|
||||||
|
# Pre-create skill on disk
|
||||||
|
user_skill = skills_dir / "old-skill"
|
||||||
|
user_skill.mkdir(parents=True)
|
||||||
|
(user_skill / "SKILL.md").write_text("# Old modified by user")
|
||||||
|
|
||||||
|
# v1 manifest (no hashes)
|
||||||
|
manifest_file.write_text("old-skill\n")
|
||||||
|
|
||||||
|
with self._patches(bundled, skills_dir, manifest_file):
|
||||||
|
result = sync_skills(quiet=True)
|
||||||
|
# Should skip (migration baseline set), NOT update
|
||||||
|
assert "old-skill" not in result.get("updated", [])
|
||||||
|
assert "old-skill" not in result.get("user_modified", [])
|
||||||
|
|
||||||
|
# Now check manifest was upgraded to v2 with user's hash as baseline
|
||||||
|
manifest = _read_manifest()
|
||||||
|
assert len(manifest["old-skill"]) == 32 # MD5 hash
|
||||||
|
|
||||||
|
def test_v1_migration_then_bundled_update_detected(self, tmp_path):
|
||||||
|
"""After v1 migration, a subsequent sync should detect bundled updates."""
|
||||||
|
bundled = self._setup_bundled(tmp_path)
|
||||||
|
skills_dir = tmp_path / "user_skills"
|
||||||
|
manifest_file = skills_dir / ".bundled_manifest"
|
||||||
|
|
||||||
|
# User has the SAME content as bundled (in sync)
|
||||||
|
user_skill = skills_dir / "old-skill"
|
||||||
|
user_skill.mkdir(parents=True)
|
||||||
|
(user_skill / "SKILL.md").write_text("# Old")
|
||||||
|
|
||||||
|
# v1 manifest
|
||||||
|
manifest_file.write_text("old-skill\n")
|
||||||
|
|
||||||
|
with self._patches(bundled, skills_dir, manifest_file):
|
||||||
|
# First sync: migration — sets baseline
|
||||||
|
sync_skills(quiet=True)
|
||||||
|
|
||||||
|
# Now change bundled content
|
||||||
|
(bundled / "old-skill" / "SKILL.md").write_text("# Old v2 — improved")
|
||||||
|
|
||||||
|
# Second sync: should detect bundled changed + user unmodified → update
|
||||||
|
result = sync_skills(quiet=True)
|
||||||
|
|
||||||
|
assert "old-skill" in result["updated"]
|
||||||
|
assert (user_skill / "SKILL.md").read_text() == "# Old v2 — improved"
|
||||||
|
|
||||||
def test_stale_manifest_entries_cleaned(self, tmp_path):
|
def test_stale_manifest_entries_cleaned(self, tmp_path):
|
||||||
"""Skills in manifest that no longer exist in bundled dir get cleaned."""
|
"""Skills in manifest that no longer exist in bundled dir get cleaned."""
|
||||||
bundled = self._setup_bundled(tmp_path)
|
bundled = self._setup_bundled(tmp_path)
|
||||||
skills_dir = tmp_path / "user_skills"
|
skills_dir = tmp_path / "user_skills"
|
||||||
manifest_file = skills_dir / ".bundled_manifest"
|
manifest_file = skills_dir / ".bundled_manifest"
|
||||||
skills_dir.mkdir(parents=True)
|
skills_dir.mkdir(parents=True)
|
||||||
# Add a stale entry that doesn't exist in bundled
|
manifest_file.write_text("old-skill:abc123\nremoved-skill:def456\n")
|
||||||
manifest_file.write_text("old-skill\nremoved-skill\n")
|
|
||||||
|
|
||||||
with patch("tools.skills_sync._get_bundled_dir", return_value=bundled), \
|
with self._patches(bundled, skills_dir, manifest_file):
|
||||||
patch("tools.skills_sync.SKILLS_DIR", skills_dir), \
|
|
||||||
patch("tools.skills_sync.MANIFEST_FILE", manifest_file):
|
|
||||||
result = sync_skills(quiet=True)
|
result = sync_skills(quiet=True)
|
||||||
|
|
||||||
assert "removed-skill" in result["cleaned"]
|
assert "removed-skill" in result["cleaned"]
|
||||||
# Verify manifest no longer has removed-skill
|
|
||||||
with patch("tools.skills_sync.MANIFEST_FILE", manifest_file):
|
with patch("tools.skills_sync.MANIFEST_FILE", manifest_file):
|
||||||
manifest = _read_manifest()
|
manifest = _read_manifest()
|
||||||
assert "removed-skill" not in manifest
|
assert "removed-skill" not in manifest
|
||||||
|
|
@ -249,20 +359,41 @@ class TestSyncSkills:
|
||||||
skills_dir = tmp_path / "user_skills"
|
skills_dir = tmp_path / "user_skills"
|
||||||
manifest_file = skills_dir / ".bundled_manifest"
|
manifest_file = skills_dir / ".bundled_manifest"
|
||||||
|
|
||||||
# Pre-create the skill dir with user content (not in manifest)
|
|
||||||
user_skill = skills_dir / "category" / "new-skill"
|
user_skill = skills_dir / "category" / "new-skill"
|
||||||
user_skill.mkdir(parents=True)
|
user_skill.mkdir(parents=True)
|
||||||
(user_skill / "SKILL.md").write_text("# User modified")
|
(user_skill / "SKILL.md").write_text("# User modified")
|
||||||
|
|
||||||
with patch("tools.skills_sync._get_bundled_dir", return_value=bundled), \
|
with self._patches(bundled, skills_dir, manifest_file):
|
||||||
patch("tools.skills_sync.SKILLS_DIR", skills_dir), \
|
|
||||||
patch("tools.skills_sync.MANIFEST_FILE", manifest_file):
|
|
||||||
result = sync_skills(quiet=True)
|
result = sync_skills(quiet=True)
|
||||||
|
|
||||||
# Should not overwrite user's version
|
|
||||||
assert (user_skill / "SKILL.md").read_text() == "# User modified"
|
assert (user_skill / "SKILL.md").read_text() == "# User modified"
|
||||||
|
|
||||||
def test_nonexistent_bundled_dir(self, tmp_path):
|
def test_nonexistent_bundled_dir(self, tmp_path):
|
||||||
with patch("tools.skills_sync._get_bundled_dir", return_value=tmp_path / "nope"):
|
with patch("tools.skills_sync._get_bundled_dir", return_value=tmp_path / "nope"):
|
||||||
result = sync_skills(quiet=True)
|
result = sync_skills(quiet=True)
|
||||||
assert result == {"copied": [], "updated": [], "skipped": 0, "cleaned": [], "total_bundled": 0}
|
assert result == {
|
||||||
|
"copied": [], "updated": [], "skipped": 0,
|
||||||
|
"user_modified": [], "cleaned": [], "total_bundled": 0,
|
||||||
|
}
|
||||||
|
|
||||||
|
def test_update_records_new_origin_hash(self, tmp_path):
|
||||||
|
"""After updating a skill, the manifest should record the new bundled hash."""
|
||||||
|
bundled = self._setup_bundled(tmp_path)
|
||||||
|
skills_dir = tmp_path / "user_skills"
|
||||||
|
manifest_file = skills_dir / ".bundled_manifest"
|
||||||
|
|
||||||
|
# Start with old synced version
|
||||||
|
user_skill = skills_dir / "old-skill"
|
||||||
|
user_skill.mkdir(parents=True)
|
||||||
|
(user_skill / "SKILL.md").write_text("# Old v1")
|
||||||
|
old_hash = _dir_hash(user_skill)
|
||||||
|
manifest_file.write_text(f"old-skill:{old_hash}\n")
|
||||||
|
|
||||||
|
with self._patches(bundled, skills_dir, manifest_file):
|
||||||
|
sync_skills(quiet=True) # updates to "# Old"
|
||||||
|
manifest = _read_manifest()
|
||||||
|
|
||||||
|
# New origin hash should match the bundled version
|
||||||
|
new_bundled_hash = _dir_hash(bundled / "old-skill")
|
||||||
|
assert manifest["old-skill"] == new_bundled_hash
|
||||||
|
assert manifest["old-skill"] != old_hash
|
||||||
|
|
|
||||||
|
|
@ -3,16 +3,22 @@
|
||||||
Skills Sync -- Manifest-based seeding and updating of bundled skills.
|
Skills Sync -- Manifest-based seeding and updating of bundled skills.
|
||||||
|
|
||||||
Copies bundled skills from the repo's skills/ directory into ~/.hermes/skills/
|
Copies bundled skills from the repo's skills/ directory into ~/.hermes/skills/
|
||||||
and uses a manifest to track which skills have been offered.
|
and uses a manifest to track which skills have been synced and their origin hash.
|
||||||
|
|
||||||
Behavior:
|
Manifest format (v2): each line is "skill_name:origin_hash" where origin_hash
|
||||||
- NEW skills (not in manifest): copied to user dir, added to manifest.
|
is the MD5 of the bundled skill at the time it was last synced to the user dir.
|
||||||
- EXISTING skills (in manifest, present in user dir): UPDATED from bundled.
|
Old v1 manifests (plain names without hashes) are auto-migrated.
|
||||||
- DELETED by user (in manifest, absent from user dir): respected -- not re-added.
|
|
||||||
|
Update logic:
|
||||||
|
- NEW skills (not in manifest): copied to user dir, origin hash recorded.
|
||||||
|
- EXISTING skills (in manifest, present in user dir):
|
||||||
|
* If user copy matches origin hash: user hasn't modified it → safe to
|
||||||
|
update from bundled if bundled changed. New origin hash recorded.
|
||||||
|
* If user copy differs from origin hash: user customized it → SKIP.
|
||||||
|
- DELETED by user (in manifest, absent from user dir): respected, not re-added.
|
||||||
- REMOVED from bundled (in manifest, gone from repo): cleaned from manifest.
|
- REMOVED from bundled (in manifest, gone from repo): cleaned from manifest.
|
||||||
|
|
||||||
The manifest lives at ~/.hermes/skills/.bundled_manifest and is a simple
|
The manifest lives at ~/.hermes/skills/.bundled_manifest.
|
||||||
newline-delimited list of skill names that have been offered to the user.
|
|
||||||
"""
|
"""
|
||||||
|
|
||||||
import hashlib
|
import hashlib
|
||||||
|
|
@ -20,7 +26,7 @@ import logging
|
||||||
import os
|
import os
|
||||||
import shutil
|
import shutil
|
||||||
from pathlib import Path
|
from pathlib import Path
|
||||||
from typing import List, Tuple
|
from typing import Dict, List, Tuple
|
||||||
|
|
||||||
logger = logging.getLogger(__name__)
|
logger = logging.getLogger(__name__)
|
||||||
|
|
||||||
|
|
@ -35,27 +41,38 @@ def _get_bundled_dir() -> Path:
|
||||||
return Path(__file__).parent.parent / "skills"
|
return Path(__file__).parent.parent / "skills"
|
||||||
|
|
||||||
|
|
||||||
def _read_manifest() -> set:
|
def _read_manifest() -> Dict[str, str]:
|
||||||
"""Read the set of skill names already offered to the user."""
|
"""
|
||||||
|
Read the manifest as a dict of {skill_name: origin_hash}.
|
||||||
|
|
||||||
|
Handles both v1 (plain names) and v2 (name:hash) formats.
|
||||||
|
v1 entries get an empty hash string which triggers migration on next sync.
|
||||||
|
"""
|
||||||
if not MANIFEST_FILE.exists():
|
if not MANIFEST_FILE.exists():
|
||||||
return set()
|
return {}
|
||||||
try:
|
try:
|
||||||
return set(
|
result = {}
|
||||||
line.strip()
|
for line in MANIFEST_FILE.read_text(encoding="utf-8").splitlines():
|
||||||
for line in MANIFEST_FILE.read_text(encoding="utf-8").splitlines()
|
line = line.strip()
|
||||||
if line.strip()
|
if not line:
|
||||||
)
|
continue
|
||||||
|
if ":" in line:
|
||||||
|
# v2 format: name:hash
|
||||||
|
name, _, hash_val = line.partition(":")
|
||||||
|
result[name.strip()] = hash_val.strip()
|
||||||
|
else:
|
||||||
|
# v1 format: plain name — empty hash triggers migration
|
||||||
|
result[line] = ""
|
||||||
|
return result
|
||||||
except (OSError, IOError):
|
except (OSError, IOError):
|
||||||
return set()
|
return {}
|
||||||
|
|
||||||
|
|
||||||
def _write_manifest(names: set):
|
def _write_manifest(entries: Dict[str, str]):
|
||||||
"""Write the manifest file."""
|
"""Write the manifest file in v2 format (name:hash)."""
|
||||||
MANIFEST_FILE.parent.mkdir(parents=True, exist_ok=True)
|
MANIFEST_FILE.parent.mkdir(parents=True, exist_ok=True)
|
||||||
MANIFEST_FILE.write_text(
|
lines = [f"{name}:{hash_val}" for name, hash_val in sorted(entries.items())]
|
||||||
"\n".join(sorted(names)) + "\n",
|
MANIFEST_FILE.write_text("\n".join(lines) + "\n", encoding="utf-8")
|
||||||
encoding="utf-8",
|
|
||||||
)
|
|
||||||
|
|
||||||
|
|
||||||
def _discover_bundled_skills(bundled_dir: Path) -> List[Tuple[str, Path]]:
|
def _discover_bundled_skills(bundled_dir: Path) -> List[Tuple[str, Path]]:
|
||||||
|
|
@ -105,18 +122,16 @@ def sync_skills(quiet: bool = False) -> dict:
|
||||||
"""
|
"""
|
||||||
Sync bundled skills into ~/.hermes/skills/ using the manifest.
|
Sync bundled skills into ~/.hermes/skills/ using the manifest.
|
||||||
|
|
||||||
- NEW skills (not in manifest): copied to user dir, added to manifest.
|
|
||||||
- EXISTING skills (in manifest, present in user dir): updated from bundled.
|
|
||||||
- DELETED by user (in manifest, absent from user dir): respected, not re-added.
|
|
||||||
- REMOVED from bundled (in manifest, gone from repo): cleaned from manifest.
|
|
||||||
|
|
||||||
Returns:
|
Returns:
|
||||||
dict with keys: copied (list), updated (list), skipped (int),
|
dict with keys: copied (list), updated (list), skipped (int),
|
||||||
cleaned (list), total_bundled (int)
|
user_modified (list), cleaned (list), total_bundled (int)
|
||||||
"""
|
"""
|
||||||
bundled_dir = _get_bundled_dir()
|
bundled_dir = _get_bundled_dir()
|
||||||
if not bundled_dir.exists():
|
if not bundled_dir.exists():
|
||||||
return {"copied": [], "updated": [], "skipped": 0, "cleaned": [], "total_bundled": 0}
|
return {
|
||||||
|
"copied": [], "updated": [], "skipped": 0,
|
||||||
|
"user_modified": [], "cleaned": [], "total_bundled": 0,
|
||||||
|
}
|
||||||
|
|
||||||
SKILLS_DIR.mkdir(parents=True, exist_ok=True)
|
SKILLS_DIR.mkdir(parents=True, exist_ok=True)
|
||||||
manifest = _read_manifest()
|
manifest = _read_manifest()
|
||||||
|
|
@ -125,16 +140,18 @@ def sync_skills(quiet: bool = False) -> dict:
|
||||||
|
|
||||||
copied = []
|
copied = []
|
||||||
updated = []
|
updated = []
|
||||||
|
user_modified = []
|
||||||
skipped = 0
|
skipped = 0
|
||||||
|
|
||||||
for skill_name, skill_src in bundled_skills:
|
for skill_name, skill_src in bundled_skills:
|
||||||
dest = _compute_relative_dest(skill_src, bundled_dir)
|
dest = _compute_relative_dest(skill_src, bundled_dir)
|
||||||
|
bundled_hash = _dir_hash(skill_src)
|
||||||
|
|
||||||
if skill_name not in manifest:
|
if skill_name not in manifest:
|
||||||
# New skill -- never offered before
|
# ── New skill — never offered before ──
|
||||||
try:
|
try:
|
||||||
if dest.exists():
|
if dest.exists():
|
||||||
# User already has a skill with the same name (unlikely but possible)
|
# User already has a skill with the same name — don't overwrite
|
||||||
skipped += 1
|
skipped += 1
|
||||||
else:
|
else:
|
||||||
dest.parent.mkdir(parents=True, exist_ok=True)
|
dest.parent.mkdir(parents=True, exist_ok=True)
|
||||||
|
|
@ -145,16 +162,37 @@ def sync_skills(quiet: bool = False) -> dict:
|
||||||
except (OSError, IOError) as e:
|
except (OSError, IOError) as e:
|
||||||
if not quiet:
|
if not quiet:
|
||||||
print(f" ! Failed to copy {skill_name}: {e}")
|
print(f" ! Failed to copy {skill_name}: {e}")
|
||||||
manifest.add(skill_name)
|
manifest[skill_name] = bundled_hash
|
||||||
|
|
||||||
elif dest.exists():
|
elif dest.exists():
|
||||||
# Existing skill in manifest AND on disk -- check for updates
|
# ── Existing skill — in manifest AND on disk ──
|
||||||
src_hash = _dir_hash(skill_src)
|
origin_hash = manifest.get(skill_name, "")
|
||||||
dst_hash = _dir_hash(dest)
|
user_hash = _dir_hash(dest)
|
||||||
if src_hash != dst_hash:
|
|
||||||
|
if not origin_hash:
|
||||||
|
# v1 migration: no origin hash recorded. Set baseline from
|
||||||
|
# user's current copy so future syncs can detect modifications.
|
||||||
|
manifest[skill_name] = user_hash
|
||||||
|
if user_hash == bundled_hash:
|
||||||
|
skipped += 1 # already in sync
|
||||||
|
else:
|
||||||
|
# Can't tell if user modified or bundled changed — be safe
|
||||||
|
skipped += 1
|
||||||
|
continue
|
||||||
|
|
||||||
|
if user_hash != origin_hash:
|
||||||
|
# User modified this skill — don't overwrite their changes
|
||||||
|
user_modified.append(skill_name)
|
||||||
|
if not quiet:
|
||||||
|
print(f" ~ {skill_name} (user-modified, skipping)")
|
||||||
|
continue
|
||||||
|
|
||||||
|
# User copy matches origin — check if bundled has a newer version
|
||||||
|
if bundled_hash != origin_hash:
|
||||||
try:
|
try:
|
||||||
shutil.rmtree(dest)
|
shutil.rmtree(dest)
|
||||||
shutil.copytree(skill_src, dest)
|
shutil.copytree(skill_src, dest)
|
||||||
|
manifest[skill_name] = bundled_hash
|
||||||
updated.append(skill_name)
|
updated.append(skill_name)
|
||||||
if not quiet:
|
if not quiet:
|
||||||
print(f" ↑ {skill_name} (updated)")
|
print(f" ↑ {skill_name} (updated)")
|
||||||
|
|
@ -162,15 +200,16 @@ def sync_skills(quiet: bool = False) -> dict:
|
||||||
if not quiet:
|
if not quiet:
|
||||||
print(f" ! Failed to update {skill_name}: {e}")
|
print(f" ! Failed to update {skill_name}: {e}")
|
||||||
else:
|
else:
|
||||||
skipped += 1
|
skipped += 1 # bundled unchanged, user unchanged
|
||||||
|
|
||||||
else:
|
else:
|
||||||
# In manifest but not on disk -- user deleted it, respect that
|
# ── In manifest but not on disk — user deleted it ──
|
||||||
skipped += 1
|
skipped += 1
|
||||||
|
|
||||||
# Clean stale manifest entries (skills removed from bundled dir)
|
# Clean stale manifest entries (skills removed from bundled dir)
|
||||||
cleaned = sorted(manifest - bundled_names)
|
cleaned = sorted(set(manifest.keys()) - bundled_names)
|
||||||
manifest -= set(cleaned)
|
for name in cleaned:
|
||||||
|
del manifest[name]
|
||||||
|
|
||||||
# Also copy DESCRIPTION.md files for categories (if not already present)
|
# Also copy DESCRIPTION.md files for categories (if not already present)
|
||||||
for desc_md in bundled_dir.rglob("DESCRIPTION.md"):
|
for desc_md in bundled_dir.rglob("DESCRIPTION.md"):
|
||||||
|
|
@ -189,6 +228,7 @@ def sync_skills(quiet: bool = False) -> dict:
|
||||||
"copied": copied,
|
"copied": copied,
|
||||||
"updated": updated,
|
"updated": updated,
|
||||||
"skipped": skipped,
|
"skipped": skipped,
|
||||||
|
"user_modified": user_modified,
|
||||||
"cleaned": cleaned,
|
"cleaned": cleaned,
|
||||||
"total_bundled": len(bundled_skills),
|
"total_bundled": len(bundled_skills),
|
||||||
}
|
}
|
||||||
|
|
@ -197,6 +237,13 @@ def sync_skills(quiet: bool = False) -> dict:
|
||||||
if __name__ == "__main__":
|
if __name__ == "__main__":
|
||||||
print("Syncing bundled skills into ~/.hermes/skills/ ...")
|
print("Syncing bundled skills into ~/.hermes/skills/ ...")
|
||||||
result = sync_skills(quiet=False)
|
result = sync_skills(quiet=False)
|
||||||
print(f"\nDone: {len(result['copied'])} new, {len(result['updated'])} updated, "
|
parts = [
|
||||||
f"{result['skipped']} unchanged, {len(result['cleaned'])} cleaned from manifest, "
|
f"{len(result['copied'])} new",
|
||||||
f"{result['total_bundled']} total bundled.")
|
f"{len(result['updated'])} updated",
|
||||||
|
f"{result['skipped']} unchanged",
|
||||||
|
]
|
||||||
|
if result["user_modified"]:
|
||||||
|
parts.append(f"{len(result['user_modified'])} user-modified (kept)")
|
||||||
|
if result["cleaned"]:
|
||||||
|
parts.append(f"{len(result['cleaned'])} cleaned from manifest")
|
||||||
|
print(f"\nDone: {', '.join(parts)}. {result['total_bundled']} total bundled.")
|
||||||
|
|
|
||||||
Loading…
Add table
Add a link
Reference in a new issue