fix(skills): skip shadowing when external_dirs provides the skill
Fixes #28126. sync_skills() was unconditionally writing bundled skills into the local <profile_home>/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): <name>'. 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
This commit is contained in:
zccyman
Teknium
parent
a8c862900b
commit
db11849c9d
2 changed files with 182 additions and 1 deletions
|
|
@ -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."""
|
||||
|
|
|
|||
|
|
@ -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,
|
||||
}
|
||||
|
||||
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue