From 59da8ec4ecd1e9527c30312cf150bbe7f5850973 Mon Sep 17 00:00:00 2001 From: teknium1 <127238744+teknium1@users.noreply.github.com> Date: Wed, 13 May 2026 09:01:22 -0700 Subject: [PATCH] fix(tools): refuse skill_view name collisions instead of guessing MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit skill_view ran the direct-path strategy across every skill dir before the recursive strategy, so a top-level skill in an external dir could silently shadow a same-named nested local skill. /skills correctly listed the local version (deduped local-first by _find_all_skills) but skill_view loaded the external one — confusing, and a real bug class for users with skills.external_dirs registered alongside categorized local skills. Pick a louder fix than @polkn's PR #6136 proposed: collect every match across all dirs (direct path, recursive by parent dir name, legacy flat .md), and if there's more than one, refuse with an error that surfaces every matching path plus a hint to load by the categorized form. Local-first precedence would have replaced silent external-shadowing with silent same-name collisions between two externals, or made an externally-shadowed-by-local skill unreachable by bare name with no signal. Refusing forces the user to disambiguate once and never wonder which skill ran. Recovery: pass the full categorized path ("foundations/runtime/explore-codebase" instead of "explore-codebase"), or rename one of the colliding skills. Co-authored-by: pol --- tests/tools/test_skills_tool.py | 165 ++++++++++++++++++++++++++++++++ tools/skills_tool.py | 98 ++++++++++++------- 2 files changed, 231 insertions(+), 32 deletions(-) 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]]