diff --git a/tests/tools/test_skills_tool.py b/tests/tools/test_skills_tool.py index d95fc0671d4..9502467546e 100644 --- a/tests/tools/test_skills_tool.py +++ b/tests/tools/test_skills_tool.py @@ -1076,3 +1076,168 @@ Do the legacy thing. assert result["setup_needed"] is False assert result["missing_required_environment_variables"] == [] assert result["readiness_status"] == "available" + + +class TestSkillViewCollisionDetection: + """Regression tests for skill_view name collision handling. + + When a skill name resolves to multiple paths across the local skills + dir and external_dirs, skill_view must refuse to guess. Silent + shadowing — where ``/skills`` shows the local version but + ``skill_view`` loads the external one — is the bug class this guards + against. Reproduces with `skills.external_dirs` registered in + config.yaml and a same-name skill nested under a category locally. + + Adapted from a regression suite originally proposed by @polkn in PR + #6136 (which used local-first precedence). The collision-refusal + behavior preserves the same protection without silently picking a + side, and gives the user an actionable hint (use the categorized + path) to recover. + """ + + def _patch_dirs(self, local_dir, external_dirs): + """Patch SKILLS_DIR (module-level) and get_external_skills_dirs at source.""" + return ( + patch("tools.skills_tool.SKILLS_DIR", local_dir), + patch( + "agent.skill_utils.get_external_skills_dirs", + return_value=list(external_dirs), + ), + ) + + def test_nested_local_collides_with_top_level_external(self, tmp_path): + """The original bug scenario: nested local + top-level external, + same name. Now refuses with both paths surfaced.""" + local_dir = tmp_path / "local" + external_dir = tmp_path / "external" + local_dir.mkdir() + external_dir.mkdir() + + _make_skill( + local_dir, + "explore-codebase", + category="foundations/runtime", + body="LOCAL VERSION", + ) + _make_skill(external_dir, "explore-codebase", body="EXTERNAL VERSION") + + p1, p2 = self._patch_dirs(local_dir, [external_dir]) + with p1, p2: + raw = skill_view("explore-codebase") + + result = json.loads(raw) + assert result["success"] is False + assert "Ambiguous skill name 'explore-codebase'" in result["error"] + assert "matches" in result + assert len(result["matches"]) == 2 + # Both paths surfaced + assert any("foundations/runtime" in p for p in result["matches"]) + assert any("external" in p for p in result["matches"]) + assert "hint" in result + + def test_top_level_local_collides_with_external(self, tmp_path): + """Top-level local + top-level external with the same name also + refuses — same-name shadowing is ambiguous regardless of nesting.""" + local_dir = tmp_path / "local" + external_dir = tmp_path / "external" + local_dir.mkdir() + external_dir.mkdir() + + _make_skill(local_dir, "shared-name", body="LOCAL VERSION") + _make_skill(external_dir, "shared-name", body="EXTERNAL VERSION") + + p1, p2 = self._patch_dirs(local_dir, [external_dir]) + with p1, p2: + raw = skill_view("shared-name") + + result = json.loads(raw) + assert result["success"] is False + assert "Ambiguous" in result["error"] + assert len(result["matches"]) == 2 + + def test_collision_resolvable_via_categorized_path(self, tmp_path): + """User can recover from a collision by passing the full + categorized path — the bare name is ambiguous, the path is not.""" + local_dir = tmp_path / "local" + external_dir = tmp_path / "external" + local_dir.mkdir() + external_dir.mkdir() + + _make_skill( + local_dir, + "explore-codebase", + category="foundations/runtime", + body="LOCAL VERSION", + ) + _make_skill(external_dir, "explore-codebase", body="EXTERNAL VERSION") + + p1, p2 = self._patch_dirs(local_dir, [external_dir]) + with p1, p2: + raw = skill_view("foundations/runtime/explore-codebase") + + result = json.loads(raw) + assert result["success"] is True + assert "LOCAL VERSION" in result["content"] + + def test_external_skill_resolves_when_no_collision(self, tmp_path): + """External-only skills still resolve normally when there's no + local skill of the same name.""" + local_dir = tmp_path / "local" + external_dir = tmp_path / "external" + local_dir.mkdir() + external_dir.mkdir() + + _make_skill(external_dir, "external-only", body="EXTERNAL BODY") + + p1, p2 = self._patch_dirs(local_dir, [external_dir]) + with p1, p2: + raw = skill_view("external-only") + + result = json.loads(raw) + assert result["success"] is True + assert "EXTERNAL BODY" in result["content"] + + def test_two_externals_same_name_also_refuse(self, tmp_path): + """Collision detection is symmetric — two external dirs with + same-name skills also trigger the refusal.""" + local_dir = tmp_path / "local" + ext_a = tmp_path / "ext_a" + ext_b = tmp_path / "ext_b" + local_dir.mkdir() + ext_a.mkdir() + ext_b.mkdir() + + _make_skill(ext_a, "pr", body="EXT_A VERSION") + _make_skill(ext_b, "pr", body="EXT_B VERSION") + + p1, p2 = self._patch_dirs(local_dir, [ext_a, ext_b]) + with p1, p2: + raw = skill_view("pr") + + result = json.loads(raw) + assert result["success"] is False + assert "Ambiguous" in result["error"] + assert len(result["matches"]) == 2 + + def test_local_only_skill_loads_normally(self, tmp_path): + """Sanity: a single local skill (no external collision) loads + without any error.""" + local_dir = tmp_path / "local" + external_dir = tmp_path / "external" + local_dir.mkdir() + external_dir.mkdir() + + _make_skill( + local_dir, + "my-skill", + category="foundations/runtime", + body="LOCAL BODY", + ) + + p1, p2 = self._patch_dirs(local_dir, [external_dir]) + with p1, p2: + raw = skill_view("my-skill") + + result = json.loads(raw) + assert result["success"] is True + assert "LOCAL BODY" in result["content"] diff --git a/tools/skills_tool.py b/tools/skills_tool.py index 32296729fe2..0fcd449b80b 100644 --- a/tools/skills_tool.py +++ b/tools/skills_tool.py @@ -956,49 +956,83 @@ def skill_view( skill_dir = None skill_md = None - # Search all dirs: local first, then external (first match wins) + # Collision detection: collect ALL candidates across every dir using + # every lookup strategy (direct path, recursive by parent dir name, + # legacy flat .md). If more than one matches, refuse and tell + # the caller — silent shadowing of a local skill by a same-named + # external skill is a real bug class (`/skills` shows one, agent + # loaded the other) so we surface it loudly instead of guessing. + from agent.skill_utils import iter_skill_index_files + + candidates: List[Tuple[Optional[Path], Path]] = [] # (skill_dir, skill_md) + seen_md: set = set() + + def _record(sd: Optional[Path], smd: Path) -> None: + try: + key = smd.resolve() + except Exception: + key = smd + if key in seen_md: + return + seen_md.add(key) + candidates.append((sd, smd)) + for search_dir in all_dirs: - # Try direct path first (e.g., "mlops/axolotl") + # Strategy 1: direct path (e.g., "mlops/axolotl" or bare "axolotl" + # at the top of the dir). direct_path = search_dir / name if direct_path.is_dir() and (direct_path / "SKILL.md").exists(): - skill_dir = direct_path - skill_md = direct_path / "SKILL.md" - break + _record(direct_path, direct_path / "SKILL.md") elif direct_path.with_suffix(".md").exists(): - skill_md = direct_path.with_suffix(".md") - break + _record(None, direct_path.with_suffix(".md")) + + # Strategy 1b: categorized form for plugin namespace fall-through + # (e.g., a "myplugin:explore" name with no plugin registered also + # tries the on-disk path "myplugin/explore"). if local_category_name: categorized_path = search_dir / local_category_name if categorized_path.is_dir() and (categorized_path / "SKILL.md").exists(): - skill_dir = categorized_path - skill_md = categorized_path / "SKILL.md" - break + _record(categorized_path, categorized_path / "SKILL.md") elif categorized_path.with_suffix(".md").exists(): - skill_md = categorized_path.with_suffix(".md") - break + _record(None, categorized_path.with_suffix(".md")) - # Search by directory name across all dirs - if not skill_md: - for search_dir in all_dirs: - from agent.skill_utils import iter_skill_index_files + # Strategy 2: recursive by directory name (catches nested skills + # like "foundations/runtime/explore-codebase" called by bare name). + for found_skill_md in iter_skill_index_files(search_dir, "SKILL.md"): + if found_skill_md.parent.name == name: + _record(found_skill_md.parent, found_skill_md) - 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 - break - if skill_md: - break + # Strategy 3: legacy flat .md files anywhere under the dir. + for found_md in search_dir.rglob(f"{name}.md"): + if found_md.name != "SKILL.md": + _record(None, found_md) - # Legacy: flat .md files - if not skill_md: - for search_dir in all_dirs: - for found_md in search_dir.rglob(f"{name}.md"): - if found_md.name != "SKILL.md": - skill_md = found_md - break - if skill_md: - break + if len(candidates) > 1: + paths = [str(smd) for _, smd in candidates] + logging.getLogger(__name__).warning( + "Skill name collision for '%s': %d candidates — %s", + name, len(candidates), "; ".join(paths), + ) + return json.dumps( + { + "success": False, + "error": ( + f"Ambiguous skill name '{name}': {len(candidates)} skills " + "match across your local skills dir and external_dirs. " + "Refusing to guess — load one explicitly by its categorized path." + ), + "matches": paths, + "hint": ( + "Pass the full relative path instead of the bare name " + "(e.g., 'category/skill-name'), or rename one of the " + "colliding skills so each name is unique." + ), + }, + ensure_ascii=False, + ) + + if candidates: + skill_dir, skill_md = candidates[0] if not skill_md or not skill_md.exists(): available = [s["name"] for s in _sort_skills(_find_all_skills())[:20]]