From 70370e089c86323ec849e5eca13a9c6eee4e450f Mon Sep 17 00:00:00 2001 From: kshitijk4poor <82637225+kshitijk4poor@users.noreply.github.com> Date: Sun, 30 Aug 2026 00:55:48 +0530 Subject: [PATCH] fix(skills): skill_view directory file_path + skill_manage categorized name resolution MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two agent-facing errors that recur constantly in optimization audit logs (thousands of occurrences over five months): 1. skill_view(name, file_path='references') returned a raw '[Errno 21] Is a directory' OS error. The local-skill branch gated on target_file.exists(); a directory passes exists(), fell through to read_text(), and raised. The plugin-skill sibling branch already gated on is_file() — this aligns the local branch so a directory request gets the same helpful not-found payload with available_files listing instead of an OS error. 2. skill_manage rejected categorized names ('category/skill-name') with 'not found in active profile'. _find_skill matched only the bare directory name, while skill_view's own ambiguity hint tells the caller to use exactly the categorized form — every call that followed the hint failed. _find_skill now also matches the full relative path of the skill dir, giving skill_manage resolution parity with skill_view across edit/patch/delete/write_file/ remove_file. Both fixes are covered by regression tests that fail on main. --- tests/tools/test_skill_manager_tool.py | 33 ++++++++++++++++++++++++++ tests/tools/test_skills_tool.py | 23 ++++++++++++++++++ tools/skill_manager_tool.py | 19 ++++++++++++++- tools/skills_tool.py | 6 ++++- 4 files changed, 79 insertions(+), 2 deletions(-) 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": [],