fix(skills): _resolve_skill_dir writes to user dir, fix tests for HERMES_USER_SKILLS_DIR

- _resolve_skill_dir now uses _USER_SKILLS_DIR instead of SKILLS_DIR
- _create_skill computes relative_to from correct parent
- Test fixtures patch _USER_SKILLS_DIR alongside SKILLS_DIR
- test_local_always_first updated for 3-dir ordering (local > user > external)
This commit is contained in:
wkh 2026-04-21 00:08:57 +08:00
parent 01c61465d3
commit 24968b5bcc
3 changed files with 25 additions and 9 deletions

View file

@ -93,18 +93,21 @@ class TestGetExternalSkillsDirs:
class TestGetAllSkillsDirs:
def test_local_always_first(self, hermes_home, external_skills_dir):
def test_local_always_first(self, hermes_home, external_skills_dir, tmp_path):
user_skills_dir = tmp_path / "user-skills"
user_skills_dir.mkdir()
(hermes_home / "config.yaml").write_text(
f"skills:\n external_dirs:\n - {external_skills_dir}\n"
)
with patch.dict(os.environ, {
"HERMES_HOME": str(hermes_home),
"HERMES_USER_SKILLS_DIR": "", # empty → user dir is still resolved but deduplicated below
"HERMES_USER_SKILLS_DIR": str(user_skills_dir),
}):
from agent.skill_utils import get_all_skills_dirs
result = get_all_skills_dirs()
assert result[0] == hermes_home / "skills"
assert result[1] == external_skills_dir.resolve()
assert result[1] == user_skills_dir.resolve()
assert result[2] == external_skills_dir.resolve()
class TestExternalSkillsInFindAll:

View file

@ -29,9 +29,10 @@ from tools.skill_manager_tool import (
@contextmanager
def _skill_dir(tmp_path):
"""Patch both SKILLS_DIR and get_all_skills_dirs so _find_skill searches
only the temp directory not the real ~/.hermes/skills/."""
"""Patch both SKILLS_DIR, _USER_SKILLS_DIR, and get_all_skills_dirs so
_find_skill and _resolve_skill_dir use only the temp directory."""
with patch("tools.skill_manager_tool.SKILLS_DIR", tmp_path), \
patch("tools.skill_manager_tool._USER_SKILLS_DIR", tmp_path), \
patch("agent.skill_utils.get_all_skills_dirs", return_value=[tmp_path]):
yield
@ -225,6 +226,7 @@ class TestCreateSkill:
skills_dir.mkdir()
with patch("tools.skill_manager_tool.SKILLS_DIR", skills_dir), \
patch("tools.skill_manager_tool._USER_SKILLS_DIR", skills_dir), \
patch("agent.skill_utils.get_all_skills_dirs", return_value=[skills_dir]):
result = _create_skill("my-skill", VALID_SKILL_CONTENT, category="../escape")
@ -238,6 +240,7 @@ class TestCreateSkill:
outside = tmp_path / "outside"
with patch("tools.skill_manager_tool.SKILLS_DIR", skills_dir), \
patch("tools.skill_manager_tool._USER_SKILLS_DIR", skills_dir), \
patch("agent.skill_utils.get_all_skills_dirs", return_value=[skills_dir]):
result = _create_skill("my-skill", VALID_SKILL_CONTENT, category=str(outside))

View file

@ -213,10 +213,14 @@ def _validate_content_size(content: str, label: str = "SKILL.md") -> Optional[st
def _resolve_skill_dir(name: str, category: str = None) -> Path:
"""Build the directory path for a new skill, optionally under a category."""
"""Build the directory path for a new skill, optionally under a category.
User-created skills go to _USER_SKILLS_DIR (configurable via
HERMES_USER_SKILLS_DIR env var), keeping them separate from bundled skills.
"""
if category:
return SKILLS_DIR / category / name
return SKILLS_DIR / name
return _USER_SKILLS_DIR / category / name
return _USER_SKILLS_DIR / name
def _find_skill(name: str) -> Optional[Dict[str, Any]]:
@ -354,10 +358,16 @@ def _create_skill(name: str, content: str, category: str = None) -> Dict[str, An
shutil.rmtree(skill_dir, ignore_errors=True)
return {"success": False, "error": scan_error}
# Compute relative path from the appropriate skills root
try:
rel_path = str(skill_dir.relative_to(SKILLS_DIR))
except ValueError:
rel_path = str(skill_dir.relative_to(_USER_SKILLS_DIR))
result = {
"success": True,
"message": f"Skill '{name}' created.",
"path": str(skill_dir.relative_to(SKILLS_DIR)),
"path": rel_path,
"skill_md": str(skill_md),
}
if category: