diff --git a/agent/skill_commands.py b/agent/skill_commands.py index a4345ca8c..9c130ab84 100644 --- a/agent/skill_commands.py +++ b/agent/skill_commands.py @@ -345,7 +345,7 @@ def scan_skill_commands() -> Dict[str, Dict[str, Any]]: _skill_commands = {} try: from tools.skills_tool import SKILLS_DIR, _parse_frontmatter, skill_matches_platform, _get_disabled_skill_names - from agent.skill_utils import get_external_skills_dirs + from agent.skill_utils import get_external_skills_dirs, iter_skill_index_files disabled = _get_disabled_skill_names() seen_names: set = set() @@ -356,7 +356,7 @@ def scan_skill_commands() -> Dict[str, Dict[str, Any]]: dirs_to_scan.extend(get_external_skills_dirs()) for scan_dir in dirs_to_scan: - for skill_md in scan_dir.rglob("SKILL.md"): + for skill_md in iter_skill_index_files(scan_dir, "SKILL.md"): if any(part in ('.git', '.github', '.hub') for part in skill_md.parts): continue try: diff --git a/tests/agent/test_skill_commands.py b/tests/agent/test_skill_commands.py index e399db619..bf8742690 100644 --- a/tests/agent/test_skill_commands.py +++ b/tests/agent/test_skill_commands.py @@ -38,6 +38,18 @@ description: Description for {name}. return skill_dir +def _symlink_category(skills_dir: Path, linked_root: Path, category: str) -> Path: + """Create a category symlink under skills_dir pointing outside the tree.""" + external_category = linked_root / category + external_category.mkdir(parents=True, exist_ok=True) + symlink_path = skills_dir / category + try: + symlink_path.symlink_to(external_category, target_is_directory=True) + except (OSError, NotImplementedError) as exc: + pytest.skip(f"symlinks unavailable in test environment: {exc}") + return external_category + + class TestScanSkillCommands: def test_finds_skills(self, tmp_path): with patch("tools.skills_tool.SKILLS_DIR", tmp_path): @@ -101,6 +113,20 @@ class TestScanSkillCommands: assert "/enabled-skill" in result assert "/disabled-skill" not in result + def test_finds_skills_in_symlinked_category_dir(self, tmp_path): + external_root = tmp_path / "repo" + skills_root = tmp_path / "skills" + skills_root.mkdir() + + external_category = _symlink_category(skills_root, external_root, "linked") + _make_skill(external_category.parent, "knowledge-brain", category="linked") + + with patch("tools.skills_tool.SKILLS_DIR", skills_root): + result = scan_skill_commands() + + assert "/knowledge-brain" in result + assert result["/knowledge-brain"]["name"] == "knowledge-brain" + def test_special_chars_stripped_from_cmd_key(self, tmp_path): """Skill names with +, /, or other special chars produce clean cmd keys.""" diff --git a/tests/tools/test_skills_tool.py b/tests/tools/test_skills_tool.py index 2a21f06b5..3cdfa98a9 100644 --- a/tests/tools/test_skills_tool.py +++ b/tests/tools/test_skills_tool.py @@ -44,6 +44,18 @@ description: Description for {name}. return skill_dir +def _symlink_category(skills_dir: Path, linked_root: Path, category: str) -> Path: + """Create a category symlink under skills_dir pointing outside the tree.""" + external_category = linked_root / category + external_category.mkdir(parents=True, exist_ok=True) + symlink_path = skills_dir / category + try: + symlink_path.symlink_to(external_category, target_is_directory=True) + except (OSError, NotImplementedError) as exc: + pytest.skip(f"symlinks unavailable in test environment: {exc}") + return external_category + + # --------------------------------------------------------------------------- # _parse_frontmatter # --------------------------------------------------------------------------- @@ -255,6 +267,20 @@ class TestFindAllSkills: assert len(skills) == 1 assert skills[0]["name"] == "real-skill" + def test_finds_skills_in_symlinked_category_dir(self, tmp_path): + external_root = tmp_path / "repo" + skills_root = tmp_path / "skills" + skills_root.mkdir() + + external_category = _symlink_category(skills_root, external_root, "linked") + _make_skill(external_category.parent, "knowledge-brain", category="linked") + + with patch("tools.skills_tool.SKILLS_DIR", skills_root): + skills = _find_all_skills() + + assert [s["name"] for s in skills] == ["knowledge-brain"] + assert skills[0]["category"] == "linked" + # --------------------------------------------------------------------------- # skills_list @@ -288,6 +314,23 @@ class TestSkillsList: assert result["count"] == 1 assert result["skills"][0]["name"] == "skill-a" + def test_category_filter_finds_symlinked_category(self, tmp_path): + external_root = tmp_path / "repo" + skills_root = tmp_path / "skills" + skills_root.mkdir() + + external_category = _symlink_category(skills_root, external_root, "linked") + _make_skill(external_category.parent, "knowledge-brain", category="linked") + + with patch("tools.skills_tool.SKILLS_DIR", skills_root): + raw = skills_list(category="linked") + + result = json.loads(raw) + assert result["success"] is True + assert result["count"] == 1 + assert result["categories"] == ["linked"] + assert result["skills"][0]["name"] == "knowledge-brain" + # --------------------------------------------------------------------------- # skill_view @@ -389,6 +432,35 @@ class TestSkillView: result = json.loads(raw) assert result["success"] is True + def test_view_finds_skill_in_symlinked_category_dir(self, tmp_path): + external_root = tmp_path / "repo" + skills_root = tmp_path / "skills" + skills_root.mkdir() + + external_category = _symlink_category(skills_root, external_root, "linked") + _make_skill(external_category.parent, "knowledge-brain", category="linked") + + with patch("tools.skills_tool.SKILLS_DIR", skills_root): + raw = skill_view("knowledge-brain") + + result = json.loads(raw) + assert result["success"] is True + assert result["name"] == "knowledge-brain" + + def test_not_found_hint_uses_same_order_as_skills_list(self, tmp_path): + with patch("tools.skills_tool.SKILLS_DIR", tmp_path): + _make_skill(tmp_path, "zeta", category="z-cat") + _make_skill(tmp_path, "alpha", category="a-cat") + _make_skill(tmp_path, "beta", category="a-cat") + + list_result = json.loads(skills_list()) + view_result = json.loads(skill_view("missing-skill")) + + assert view_result["success"] is False + assert view_result["available_skills"] == [ + skill["name"] for skill in list_result["skills"] + ] + class TestSkillViewSecureSetupOnLoad: def test_requests_missing_required_env_and_continues(self, tmp_path, monkeypatch): diff --git a/tools/skills_tool.py b/tools/skills_tool.py index 40a6990ea..8bf92ef08 100644 --- a/tools/skills_tool.py +++ b/tools/skills_tool.py @@ -554,7 +554,7 @@ def _find_all_skills(*, skip_disabled: bool = False) -> List[Dict[str, Any]]: Returns: List of skill metadata dicts (name, description, category). """ - from agent.skill_utils import get_external_skills_dirs + from agent.skill_utils import get_external_skills_dirs, iter_skill_index_files skills = [] seen_names: set = set() @@ -569,7 +569,7 @@ def _find_all_skills(*, skip_disabled: bool = False) -> List[Dict[str, Any]]: dirs_to_scan.extend(get_external_skills_dirs()) for scan_dir in dirs_to_scan: - for skill_md in scan_dir.rglob("SKILL.md"): + for skill_md in iter_skill_index_files(scan_dir, "SKILL.md"): if any(part in _EXCLUDED_SKILL_DIRS for part in skill_md.parts): continue @@ -620,6 +620,11 @@ def _find_all_skills(*, skip_disabled: bool = False) -> List[Dict[str, Any]]: return skills +def _sort_skills(skills: List[Dict[str, Any]]) -> List[Dict[str, Any]]: + """Keep every skill listing path ordered the same way.""" + return sorted(skills, key=lambda s: (s.get("category") or "", s["name"])) + + def _load_category_description(category_dir: Path) -> Optional[str]: """ Load category description from DESCRIPTION.md if it exists. @@ -709,7 +714,7 @@ def skills_list(category: str = None, task_id: str = None) -> str: all_skills = [s for s in all_skills if s.get("category") == category] # Sort by category then name - all_skills.sort(key=lambda s: (s.get("category") or "", s["name"])) + all_skills = _sort_skills(all_skills) # Extract unique categories categories = sorted( @@ -926,7 +931,9 @@ def skill_view(name: str, file_path: str = None, task_id: str = None) -> str: # Search by directory name across all dirs if not skill_md: for search_dir in all_dirs: - for found_skill_md in search_dir.rglob("SKILL.md"): + from agent.skill_utils import iter_skill_index_files + + for found_skill_md in iter_skill_index_files(search_dir, "SKILL.md"): if found_skill_md.parent.name == name: skill_dir = found_skill_md.parent skill_md = found_skill_md @@ -945,7 +952,7 @@ def skill_view(name: str, file_path: str = None, task_id: str = None) -> str: break if not skill_md or not skill_md.exists(): - available = [s["name"] for s in _find_all_skills()[:20]] + available = [s["name"] for s in _sort_skills(_find_all_skills())[:20]] return json.dumps( { "success": False,