fix(tools): refuse skill_view name collisions instead of guessing

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 <name>.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 <pol.kuijken@gmail.com>

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"]

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 <name>.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 <name>.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]]