diff --git a/tests/tools/test_skill_manager_tool.py b/tests/tools/test_skill_manager_tool.py index 30a2761021..1d5bcb4629 100644 --- a/tests/tools/test_skill_manager_tool.py +++ b/tests/tools/test_skill_manager_tool.py @@ -19,6 +19,7 @@ from tools.skill_manager_tool import ( _delete_skill, _write_file, _remove_file, + _find_skill, skill_manage, ) from agent.skill_utils import ( @@ -200,6 +201,38 @@ class TestEditSkill: assert "Updated description" in content + def test_edit_existing_skill_by_categorized_path(self, tmp_path): + """Categorized names (``category/skill``) must resolve in skill_manage. + + skill_view's ambiguity hint explicitly tells the caller to use the + full categorized path (``category/skill-name``), and skills_list + reports skills with their category. But ``_find_skill`` only matched + the bare directory name, so every skill_manage call that followed + that hint failed with "not found in active profile" — the agent then + retried repeatedly, burning LLM round trips (top recurring audit + finding). Resolution parity with skill_view is the fix. + """ + with _skill_dir(tmp_path): + _create_skill("my-skill", VALID_SKILL_CONTENT, category="software-development") + result = _edit_skill("software-development/my-skill", VALID_SKILL_CONTENT_2) + assert result["success"] is True, result.get("error") + content = (tmp_path / "software-development" / "my-skill" / "SKILL.md").read_text() + assert "Updated description" in content + + def test_find_skill_accepts_categorized_path(self, tmp_path): + with _skill_dir(tmp_path): + _create_skill("my-skill", VALID_SKILL_CONTENT, category="mlops") + found = _find_skill("mlops/my-skill") + assert found is not None + assert found["path"] == tmp_path / "mlops" / "my-skill" + + def test_find_skill_bare_name_still_resolves_nested_skill(self, tmp_path): + with _skill_dir(tmp_path): + _create_skill("my-skill", VALID_SKILL_CONTENT, category="mlops") + found = _find_skill("my-skill") + assert found is not None + assert found["path"] == tmp_path / "mlops" / "my-skill" + def test_edit_invalid_content_rejected(self, tmp_path): with _skill_dir(tmp_path): _create_skill("my-skill", VALID_SKILL_CONTENT) diff --git a/tests/tools/test_skills_tool.py b/tests/tools/test_skills_tool.py index 656d02e355..99bcb4c8ac 100644 --- a/tests/tools/test_skills_tool.py +++ b/tests/tools/test_skills_tool.py @@ -376,6 +376,29 @@ class TestSkillView: assert skill["linked_files"] is not None assert "references" in skill["linked_files"] + def test_view_file_path_directory_returns_available_files(self, tmp_path): + """Requesting a directory (e.g. 'references') must not raise. + + Regression: the local-skill file_path branch checked + ``target_file.exists()`` and fell through to ``read_text()`` on a + directory, surfacing a raw ``[Errno 21] Is a directory`` error from + deep inside the OS instead of the helpful not-found payload with + available_files that a missing file gets. The plugin-skill sibling + branch already gates on ``is_file()``; this aligns the local path. + """ + with patch("tools.skills_tool.SKILLS_DIR", tmp_path): + skill_dir = _make_skill(tmp_path, "my-skill") + refs_dir = skill_dir / "references" + refs_dir.mkdir() + (refs_dir / "api.md").write_text("# API Docs\nEndpoint info.") + + result = json.loads(skill_view("my-skill", file_path="references")) + + assert result["success"] is False + assert "not found" in result["error"] + # The caller gets the same helpful listing as a truly missing file. + assert "references/api.md" in result["available_files"]["references"] + def test_disabled_skill_blocked_enabled_allowed(self, tmp_path): with ( patch("tools.skills_tool.SKILLS_DIR", tmp_path), diff --git a/tools/skill_manager_tool.py b/tools/skill_manager_tool.py index 8e1416ab98..2905e4ad9b 100644 --- a/tools/skill_manager_tool.py +++ b/tools/skill_manager_tool.py @@ -683,15 +683,32 @@ def _find_skill(name: str) -> Optional[Dict[str, Any]]: Searches the local skills dir (~/.hermes/skills/) first, then any external dirs configured via skills.external_dirs. Returns {"path": Path} or None. + + Accepts both the bare directory name (``axolotl``) and the categorized + relative path (``mlops/axolotl``) — the same two forms skill_view + resolves, and the form skill_view's ambiguity hint explicitly tells + the caller to use. Matching bare-name suffix keeps bare lookups + working for nested skills. """ from agent.skill_utils import get_all_skills_dirs, is_excluded_skill_path + + def _matches(skill_md: Path) -> bool: + if skill_md.parent.name == name: + return True + # Categorized form: the full relative path of the skill dir. + try: + rel = skill_md.parent.resolve().relative_to(_skills_dir().resolve()) + except ValueError: + return False + return str(rel) == name + for skills_dir in get_all_skills_dirs(): if not skills_dir.exists(): continue for skill_md in skills_dir.rglob("SKILL.md"): if is_excluded_skill_path(skill_md): continue - if skill_md.parent.name == name: + if _matches(skill_md): return {"path": skill_md.parent} return None diff --git a/tools/skills_tool.py b/tools/skills_tool.py index 5021b8ccc0..53d7947aa8 100644 --- a/tools/skills_tool.py +++ b/tools/skills_tool.py @@ -1542,7 +1542,11 @@ def skill_view( }, ensure_ascii=False, ) - if not target_file.exists(): + # Gate on is_file(), not exists(): a directory (e.g. requesting + # 'references' bare) must take the not-found listing branch, not + # fall through to read_text() and surface a raw [Errno 21] + # "Is a directory" OS error. Matches the plugin-skill branch above. + if not target_file.is_file(): # List available files in the skill directory, organized by type available_files = { "references": [],