diff --git a/tests/tools/test_skill_manager_tool.py b/tests/tools/test_skill_manager_tool.py index bf4f4a03b8868..01ada7dd34761 100644 --- a/tests/tools/test_skill_manager_tool.py +++ b/tests/tools/test_skill_manager_tool.py @@ -242,6 +242,92 @@ def test_edit_invalid_content_rejected(self, tmp_path): content = (tmp_path / "my-skill" / "SKILL.md").read_text() assert "A test skill" in content +# --------------------------------------------------------------------------- +# Profiles that install skills as per-skill symlinks into a shared tree +# --------------------------------------------------------------------------- + + +@contextmanager +def _symlinked_skill_lane(tmp_path): + """A profile whose skills root holds real category dirs and SYMLINKED skills. + + The profile installs every skill as a per-skill symlink into the shared tree + (``profiles/

/skills//`` -> ``//``), so its + skills are links, not directories. Yields ``(lane_root, canonical_skill_dir)`` + with both skill-dir lookups patched to the lane root. + """ + canonical = tmp_path / "shared" / "ops" / "linked-skill" + canonical.mkdir(parents=True) + (canonical / "SKILL.md").write_text(VALID_SKILL_CONTENT) + lane_root = tmp_path / "lane" / "skills" + (lane_root / "ops").mkdir(parents=True) + (lane_root / "ops" / "linked-skill").symlink_to(canonical, target_is_directory=True) + with patch("tools.skill_manager_tool.SKILLS_DIR", lane_root), \ + patch("agent.skill_utils.get_all_skills_dirs", return_value=[lane_root]): + yield lane_root, canonical + + +class TestSymlinkedSkillDirs: + """skill_manage must resolve the same skills skill_view resolves. + + ``Path.rglob`` does not descend into symlinked directories, so the write path + saw none of that profile's skills while the read path (``iter_skill_index_files``, + which walks with ``followlinks=True``) resolved all of them. Every write failed + with "not found in active profile", and the error even named the profiles that + DO "have" the skill, because a profile whose skills root IS a symlink works + with rglob. + """ + + def test_find_skill_resolves_per_skill_symlink(self, tmp_path): + with _symlinked_skill_lane(tmp_path) as (lane_root, _canonical): + found = _find_skill("linked-skill") + assert found is not None, "a lane's symlinked skill must be discoverable" + assert found["path"] == lane_root / "ops" / "linked-skill" + + def test_find_skill_resolves_categorized_name_for_symlink(self, tmp_path): + with _symlinked_skill_lane(tmp_path) as (lane_root, _canonical): + found = _find_skill("ops/linked-skill") + assert found is not None, "categorized form must resolve through a symlink too" + assert found["path"] == lane_root / "ops" / "linked-skill" + + def test_patch_edits_canonical_file_behind_symlink(self, tmp_path): + with _symlinked_skill_lane(tmp_path) as (_lane_root, canonical): + result = _patch_skill("linked-skill", "Do the thing.", "Do the new thing.") + assert result["success"] is True, result.get("error") + assert "Do the new thing." in (canonical / "SKILL.md").read_text() + + def test_write_file_lands_in_canonical_dir_behind_symlink(self, tmp_path): + with _symlinked_skill_lane(tmp_path) as (lane_root, canonical): + result = _write_file("linked-skill", "references/notes.md", "lane note") + lane_side = lane_root / "ops" / "linked-skill" / "references" / "notes.md" + assert result["success"] is True, result.get("error") + assert (canonical / "references" / "notes.md").read_text() == "lane note" + assert lane_side.read_text() == "lane note", "same file through the link" + + def test_dispatcher_patches_through_symlink(self, tmp_path): + """The model-facing entry point, not only the private helper.""" + with _symlinked_skill_lane(tmp_path) as (_lane_root, canonical): + raw = skill_manage(action="patch", name="linked-skill", + old_string="Do the thing.", new_string="Do the new thing.") + result = json.loads(raw) + assert result["success"] is True, result.get("error") + assert "Do the new thing." in (canonical / "SKILL.md").read_text() + + def test_whole_root_symlink_still_resolves(self, tmp_path): + """No behaviour change for a profile whose skills root IS the symlink.""" + shared = tmp_path / "shared-root" + (shared / "cat" / "rooted-skill").mkdir(parents=True) + (shared / "cat" / "rooted-skill" / "SKILL.md").write_text(VALID_SKILL_CONTENT) + root_link = tmp_path / "linked-root" + root_link.symlink_to(shared, target_is_directory=True) + with patch("tools.skill_manager_tool.SKILLS_DIR", root_link), \ + patch("agent.skill_utils.get_all_skills_dirs", return_value=[root_link]): + bare = _find_skill("rooted-skill") + categorized = _find_skill("cat/rooted-skill") + assert bare is not None and bare["path"] == root_link / "cat" / "rooted-skill" + assert categorized is not None, "categorized form must keep resolving here" + + class TestPatchSkill: def test_patch_unique_match(self, tmp_path): with _skill_dir(tmp_path): diff --git a/tools/skill_manager_tool.py b/tools/skill_manager_tool.py index 00a7ce8c0bffa..46cb9c2963fe3 100644 --- a/tools/skill_manager_tool.py +++ b/tools/skill_manager_tool.py @@ -193,10 +193,19 @@ def _resolve_skill_dir(name: str, category: str = None) -> Path: def _iter_skill_dirs(root: Path): - from agent.skill_utils import is_excluded_skill_path - for skill_md in root.rglob("SKILL.md"): - if not is_excluded_skill_path(skill_md): - yield skill_md.parent + """Skill dirs under ``root`` (every directory holding a ``SKILL.md``). + + Delegates to the canonical index walk the READ path uses (skill_view / + skills_list). ``Path.rglob`` does not descend into symlinked directories, so a + profile that installs each skill as a per-skill symlink into a shared tree + (``profiles/

/skills//`` -> ``//``) had none of + its skills visible to the write path while every read resolved them: writes + failed with "not found in active profile". Keep this on ``iter_skill_index_files`` + -- write resolution and read resolution disagreeing IS the bug. + """ + from agent.skill_utils import iter_skill_index_files + for skill_md in iter_skill_index_files(root, "SKILL.md"): + yield skill_md.parent def _find_skill(name: str) -> Optional[Dict[str, Any]]: @@ -206,25 +215,32 @@ def _find_skill(name: str) -> Optional[Dict[str, Any]]: categorized relative path (``mlops/axolotl``) — the two forms skill_view resolves. The categorized form matches RELATIVE to the local root only (relative_to raises for external dirs).""" from agent.skill_utils import get_all_skills_dirs - local_root = None + local_roots: List[Path] = [] if "/" in name or "\\" in name: + # BOTH forms of the root: a profile that keeps its skills as per-skill symlinks + # into a shared tree would fail a resolved comparison (it points outside the root), + # while a profile whose skills root is itself a symlink resolves past the configured + # path. Lexical first, resolved as a fallback. + local_roots.append(_skills_dir()) try: - local_root = _skills_dir().resolve() + resolved_root = _skills_dir().resolve() except OSError: logger.debug( - "skills dir resolve failed; categorized lookups fall back to the unresolved path", + "skills dir resolve failed; categorized lookups use the unresolved path only", exc_info=True) - local_root = _skills_dir() + else: + if resolved_root != local_roots[0]: + local_roots.append(resolved_root) for skills_dir in get_all_skills_dirs(): if not skills_dir.exists(): continue for skill_dir in _iter_skill_dirs(skills_dir): if skill_dir.name == name: return {"path": skill_dir} - if local_root is not None: - resolved = skill_dir.resolve() - if (resolved.is_relative_to(local_root) - and resolved.relative_to(local_root).as_posix() == name): # POSIX form + for local_root in local_roots: + if not skill_dir.is_relative_to(local_root): + continue + if skill_dir.relative_to(local_root).as_posix() == name: # POSIX form return {"path": skill_dir} return None