From db11849c9dc9b03a058c5473a64754832efaeb98 Mon Sep 17 00:00:00 2001 From: zccyman <16263913+zccyman@users.noreply.github.com> Date: Thu, 28 May 2026 10:55:29 +0800 Subject: [PATCH] fix(skills): skip shadowing when external_dirs provides the skill Fixes #28126. sync_skills() was unconditionally writing bundled skills into the local /skills/ tree even when the profile's config.yaml delegated skill resolution to an external directory via skills.external_dirs. The skill loader then saw two candidates for the same name (local shadow + external canonical), refused to resolve on collision, and every worker that auto-loaded such a skill crashed with 'Unknown skill(s): '. Changes: - _build_external_skill_index() indexes skills available in external dirs (by directory name and frontmatter name) - sync_skills() skips writing a bundled skill when it finds the same name in the external index; records the hash in the manifest so subsequent syncs treat it as already handled - Self-healing: removes stale local shadows left by prior buggy syncs (only when origin_hash == bundled_hash == user_hash, i.e. we wrote it and user didn't touch it) - New 'shadowed_by_external' key in sync_skills() return dict 3 new tests in TestExternalDirsIndexing (all passing). All 48 tests in test_skills_sync.py pass. Closes #28126 --- tests/tools/test_skills_sync.py | 123 ++++++++++++++++++++++++++++++++ tools/skills_sync.py | 60 +++++++++++++++- 2 files changed, 182 insertions(+), 1 deletion(-) diff --git a/tests/tools/test_skills_sync.py b/tests/tools/test_skills_sync.py index fa35f01f2..42d59d78e 100644 --- a/tests/tools/test_skills_sync.py +++ b/tests/tools/test_skills_sync.py @@ -272,6 +272,129 @@ class TestRmtreeWritableScopeGuard: assert not sub.exists() +class TestExternalDirsIndexing: + """Tests for external_dirs awareness in sync_skills (#28126).""" + + def _setup_bundled(self, tmp_path): + """Create a fake bundled skills directory.""" + bundled = tmp_path / "bundled_skills" + (bundled / "devops" / "clair-qa").mkdir(parents=True) + (bundled / "devops" / "clair-qa" / "SKILL.md").write_text("# bundled clair") + (bundled / "creative" / "ascii-art").mkdir(parents=True) + (bundled / "creative" / "ascii-art" / "SKILL.md").write_text("# bundled ascii") + return bundled + + def _setup_external(self, tmp_path): + """Create a fake external skills directory.""" + ext_dir = tmp_path / "external_skills" + (ext_dir / "devops" / "clair-qa").mkdir(parents=True) + (ext_dir / "devops" / "clair-qa" / "SKILL.md").write_text("# external clair") + (ext_dir / "devops" / "clair-qa" / "main.py").write_text("print('ext')") + return ext_dir + + 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._get_optional_dir", return_value=bundled.parent / "optional-skills")) + 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_shadowed_skill_skipped_and_deferred(self, tmp_path): + """When external dir provides the skill, sync_skills should not write it locally.""" + bundled = self._setup_bundled(tmp_path) + skills_dir = tmp_path / "user_skills" + manifest_file = skills_dir / ".bundled_manifest" + ext_dir = self._setup_external(tmp_path) + + with self._patches(bundled, skills_dir, manifest_file): + with patch("agent.skill_utils.get_external_skills_dirs", return_value=[ext_dir]): + result = sync_skills(quiet=True) + + assert "clair-qa" in result["shadowed_by_external"] + assert "clair-qa" not in result["copied"] + assert "ascii-art" in result["copied"] + assert not (skills_dir / "devops" / "clair-qa").exists() + + def test_shadowed_skill_not_recorded_in_manifest(self, tmp_path): + """A skill we never wrote locally must NOT be baselined in the manifest. + + Recording bundled_hash for a deferred skill would later make the + loader misclassify the external copy as a user-deleted bundled skill + and poison update detection. The shadowed name stays out of the + manifest entirely. + """ + bundled = self._setup_bundled(tmp_path) + skills_dir = tmp_path / "user_skills" + manifest_file = skills_dir / ".bundled_manifest" + ext_dir = self._setup_external(tmp_path) + + with self._patches(bundled, skills_dir, manifest_file): + with patch("agent.skill_utils.get_external_skills_dirs", return_value=[ext_dir]): + sync_skills(quiet=True) + manifest = _read_manifest() + + assert "clair-qa" not in manifest + # The non-shadowed skill is still synced and baselined normally. + assert "ascii-art" in manifest + + def test_stale_shadow_self_healed(self, tmp_path): + """A byte-identical-to-bundled local shadow is removed when the same + skill is now provided by external_dirs (heals profiles broken by an + earlier sync that ran before external_dirs was configured).""" + bundled = self._setup_bundled(tmp_path) + skills_dir = tmp_path / "user_skills" + manifest_file = skills_dir / ".bundled_manifest" + ext_dir = self._setup_external(tmp_path) + + # Pre-seed a shadow identical to the bundled source. + shadow = skills_dir / "devops" / "clair-qa" + shadow.mkdir(parents=True) + (shadow / "SKILL.md").write_text("# bundled clair") + + with self._patches(bundled, skills_dir, manifest_file): + with patch("agent.skill_utils.get_external_skills_dirs", return_value=[ext_dir]): + result = sync_skills(quiet=True) + + assert "clair-qa" in result["shadowed_by_external"] + assert not shadow.exists() + + def test_user_customized_shadow_preserved(self, tmp_path): + """A local skill that DIFFERS from bundled is the user's own — it must + never be deleted even when external_dirs provides the same name.""" + bundled = self._setup_bundled(tmp_path) + skills_dir = tmp_path / "user_skills" + manifest_file = skills_dir / ".bundled_manifest" + ext_dir = self._setup_external(tmp_path) + + custom = skills_dir / "devops" / "clair-qa" + custom.mkdir(parents=True) + (custom / "SKILL.md").write_text("# my own customized clair") + + with self._patches(bundled, skills_dir, manifest_file): + with patch("agent.skill_utils.get_external_skills_dirs", return_value=[ext_dir]): + result = sync_skills(quiet=True) + + assert "clair-qa" in result["shadowed_by_external"] + assert custom.exists() + assert (custom / "SKILL.md").read_text() == "# my own customized clair" + + def test_no_external_dirs_unchanged(self, tmp_path): + """Without external_dirs, all bundled skills should be copied normally.""" + 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): + with patch("agent.skill_utils.get_external_skills_dirs", return_value=[]): + result = sync_skills(quiet=True) + + assert "clair-qa" in result["copied"] + assert "ascii-art" in result["copied"] + assert result["shadowed_by_external"] == [] + + class TestSyncSkills: def _setup_bundled(self, tmp_path): """Create a fake bundled skills directory.""" diff --git a/tools/skills_sync.py b/tools/skills_sync.py index a8a7265f9..2c0f41c47 100644 --- a/tools/skills_sync.py +++ b/tools/skills_sync.py @@ -30,7 +30,7 @@ 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 from agent.skill_utils import is_excluded_skill_path -from typing import Dict, List, Optional, Tuple +from typing import Dict, List, Optional, Set, Tuple from utils import atomic_replace logger = logging.getLogger(__name__) @@ -65,6 +65,35 @@ def _get_optional_dir() -> Path: return get_optional_skills_dir(Path(__file__).parent.parent / "optional-skills") +def _build_external_skill_index() -> Set[str]: + """Index every skill available in external_dirs by name and frontmatter name. + + Returns a set of skill names that are already provided by external dirs. + Used to prevent sync_skills from shadowing externally-delegated skills. + """ + try: + from agent.skill_utils import get_external_skills_dirs, _external_dirs_cache_clear + except ImportError: + return set() + + # Clear the external dirs cache so a config edit (or a test patch) is seen. + _external_dirs_cache_clear() + + external_names: Set[str] = set() + for ext_dir in get_external_skills_dirs(): + for skill_md in ext_dir.rglob("SKILL.md"): + if is_excluded_skill_path(skill_md): + continue + skill_dir = skill_md.parent + # Index by directory name (how _find_skill resolves skills) + external_names.add(skill_dir.name) + # Also index by frontmatter name (alternate identifier) + frontmatter_name = _read_skill_name(skill_md, "") + if frontmatter_name: + external_names.add(frontmatter_name) + return external_names + + def _read_manifest() -> Dict[str, str]: """ Read the manifest as a dict of {skill_name: origin_hash}. @@ -486,6 +515,9 @@ def sync_skills(quiet: bool = False) -> dict: bundled_skills = _discover_bundled_skills(bundled_dir) bundled_names = {name for name, _ in bundled_skills} suppressed = _read_suppressed_names() + # Index of skills already provided by external_dirs (skip writing them) + external_index = _build_external_skill_index() + shadowed_by_external: List[str] = [] copied = [] updated = [] @@ -524,6 +556,31 @@ def sync_skills(quiet: bool = False) -> dict: exc_info=True, ) + if skill_name in external_index: + # An external_dirs source already provides this skill. Writing it + # into the profile-local tree would create a name collision the + # loader refuses to resolve (#28126). Defer to the external copy + # for ALL manifest states (new, previously-synced, user-deleted). + shadowed_by_external.append(skill_name) + skipped += 1 + if not quiet: + print( + f" ⇢ {skill_name} (deferred to external_dirs, " + "not written to local tree)" + ) + # Self-healing: a prior sync (before external_dirs was configured, + # or an older buggy sync) may have left a local shadow that now + # collides. We own that shadow only when it is byte-identical to + # the bundled source — a user's own customized skill by the same + # name differs, so never delete or re-baseline it. Drop the stale + # manifest entry so the skill isn't later misread as user-deleted. + if dest.exists() and _dir_hash(dest) == bundled_hash: + _rmtree_writable(dest) + if not quiet: + print(f" ✓ removed stale shadow of {skill_name}") + manifest.pop(skill_name, None) + continue + if skill_name not in manifest: # ── New skill — never offered before ── try: @@ -659,6 +716,7 @@ def sync_skills(quiet: bool = False) -> dict: "suppressed": suppressed_skipped, "total_bundled": len(bundled_skills), "optional_provenance_backfilled": optional_provenance_backfilled, + "shadowed_by_external": shadowed_by_external, }