fix(skills): skill_view directory file_path + skill_manage categorized name resolution
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.
This commit is contained in:
@@ -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)
|
||||
|
||||
Reference in New Issue
Block a user