diff --git a/EvoScientist/tools/skill_manager.py b/EvoScientist/tools/skill_manager.py index d1d24d9..ee9e612 100644 --- a/EvoScientist/tools/skill_manager.py +++ b/EvoScientist/tools/skill_manager.py @@ -1,5 +1,6 @@ """Skill management tool (LangChain @tool wrapper).""" +from pathlib import Path from typing import Literal from langchain_core.tools import tool @@ -67,15 +68,47 @@ def skill_manager( "a GitHub URL, or a local directory path." ) result = install_skill(source) - if result["success"]: - return ( - f"Successfully installed skill: {result['name']}\n" - f"Description: {result.get('description', '(none)')}\n" - f"Path: {result['path']}\n\n" + if not result["success"]: + # ``_batch_install_local`` returns ``{"success": False, "batch": + # True, "installed": [], "failed": [...]}`` with no top-level + # ``error`` key — a bare ``result['error']`` KeyErrors here. + if result.get("batch"): + failed = result.get("failed") or [] + if not failed: + return "Failed to install skills: no skills found in source" + lines = ["Failed to install skills:"] + for f in failed: + lines.append(f" - {f.get('name', '?')}: {f.get('error', '?')}") + return "\n".join(lines) + return f"Failed to install skill: {result['error']}" + + # Success path — batch and single-install shapes handled uniformly. + # Report ``Path: /skills/``: the sandbox-visible virtual mount + # segment (a directory name), not the host filesystem path. Surfacing + # ``result['path']`` (e.g. ``/home/.../EvoScientist/skills/``) + # invited ``cd && …`` chains that fail in the sandbox. + entries = result["installed"] if result.get("batch") else [result] + blocks = [] + for entry in entries: + mount = Path(entry["path"]).name + blocks.append( + f"Successfully installed skill: {entry['name']}\n" + f"Description: {entry.get('description', '(none)')}\n" + f"Path: /skills/{mount}\n\n" f"Read its SKILL.md for full instructions." ) - else: - return f"Failed to install skill: {result['error']}" + # Batch installs can partially fail — surface each failure rather than + # dropping it silently below the success blocks. + if result.get("batch"): + failed = result.get("failed") or [] + if failed: + fail_lines = ["Partial install — the following skills failed:"] + for f in failed: + fail_lines.append( + f" - {f.get('name', '?')}: {f.get('error', '?')}" + ) + blocks.append("\n".join(fail_lines)) + return "\n\n".join(blocks) elif action == "list": skills = list_skills(include_system=include_system) @@ -148,11 +181,15 @@ def skill_manager( f"Use action='list' with include_system=True to see all available skills." ) tags_str = f"\nTags: {', '.join(info.tags)}" if info.tags else "" + # ``Path`` reports the sandbox-visible virtual mount segment (the skill's + # directory name under ``/skills/``), not the host filesystem path. + # Surfacing the host path (e.g. ``/home/.../EvoScientist/skills/``) + # invited ``cd && …`` chains that fail in the sandbox. return ( f"Name: {info.name}\n" f"Description: {info.description}\n" f"Source: {info.source}\n" - f"Path: {info.path}{tags_str}" + f"Path: /skills/{info.path.name}{tags_str}" ) else: diff --git a/tests/test_skills_manager.py b/tests/test_skills_manager.py index ec862ce..f541239 100644 --- a/tests/test_skills_manager.py +++ b/tests/test_skills_manager.py @@ -943,3 +943,250 @@ class TestSkillManagerList: result = skill_manager.invoke({"action": "list", "include_system": False}) assert "No user skills installed" in result + + +class TestSkillManagerInfo: + """Tests for the skill_manager() tool's action='info' output. + + Guards the sandbox-visible ``Path: /skills/`` shape and the absence + of any host filesystem path in the agent-visible response. Agents burn + turns on ``cd && …`` chains whenever the host path leaks. + """ + + def _make_skill(self, parent, name, description="A skill"): + skill_dir = parent / name + skill_dir.mkdir() + (skill_dir / "SKILL.md").write_text( + f"---\nname: {name}\ndescription: {description}\n---\n" + ) + return skill_dir + + def test_info_reports_virtual_mount_path(self, tmp_path): + """``Path:`` is the sandbox-visible ``/skills/``, not the host path.""" + from EvoScientist.tools.skill_manager import skill_manager + + workspace_dir = tmp_path / "workspace" + workspace_dir.mkdir() + global_dir = tmp_path / "global-empty" + global_dir.mkdir() + self._make_skill(tmp_path, "info-skill") + install_skill(str(tmp_path / "info-skill"), str(workspace_dir)) + + with ( + patch("EvoScientist.paths.USER_SKILLS_DIR", workspace_dir), + patch("EvoScientist.paths.GLOBAL_SKILLS_DIR", global_dir), + ): + result = skill_manager.invoke({"action": "info", "name": "info-skill"}) + + assert "Path: /skills/info-skill" in result + + def test_info_omits_host_path(self, tmp_path): + """No host filesystem path leaks into the response. + + Stronger than a label-only check: catches any future refactor that + keeps the path visible under a different label (``Local:``, + ``Installed at:``, embedded in ``Source: …``). + """ + from EvoScientist.tools.skill_manager import skill_manager + from EvoScientist.tools.skills_manager import get_skill_info + + workspace_dir = tmp_path / "workspace" + workspace_dir.mkdir() + global_dir = tmp_path / "global-empty" + global_dir.mkdir() + self._make_skill(tmp_path, "host-leak-guard") + install_skill(str(tmp_path / "host-leak-guard"), str(workspace_dir)) + + with ( + patch("EvoScientist.paths.USER_SKILLS_DIR", workspace_dir), + patch("EvoScientist.paths.GLOBAL_SKILLS_DIR", global_dir), + ): + info = get_skill_info("host-leak-guard") + result = skill_manager.invoke({"action": "info", "name": "host-leak-guard"}) + + assert str(info.path) not in result + + +class TestSkillManagerInstall: + """Tests for the skill_manager() tool's action='install' output shape. + + Covers both single-install and batch-install returns: + - Single: ``{"success": True, "name": ..., "path": ..., "description": ...}``. + - Batch: ``{"success": ..., "batch": True, "installed": [...], "failed": [...]}`` + with no top-level ``name``, ``path``, ``description``, or ``error``. + """ + + def _make_skill(self, parent, name, description="A skill"): + skill_dir = parent / name + skill_dir.mkdir() + (skill_dir / "SKILL.md").write_text( + f"---\nname: {name}\ndescription: {description}\n---\n" + ) + return skill_dir + + def test_install_single_reports_virtual_mount_path(self, tmp_path): + """Single install: ``Path: /skills/``, no host path.""" + from EvoScientist.tools.skill_manager import skill_manager + + workspace_dir = tmp_path / "workspace" + workspace_dir.mkdir() + global_dir = tmp_path / "global-empty" + global_dir.mkdir() + self._make_skill(tmp_path, "solo-skill") + + with ( + patch("EvoScientist.paths.USER_SKILLS_DIR", workspace_dir), + patch("EvoScientist.paths.GLOBAL_SKILLS_DIR", global_dir), + ): + result = skill_manager.invoke( + {"action": "install", "source": str(tmp_path / "solo-skill")} + ) + + assert "Successfully installed skill: solo-skill" in result + assert "Path: /skills/solo-skill" in result + + def test_install_single_omits_host_path(self, tmp_path): + """Single install: no host filesystem path leaks into the response. + + ``install_skill(source)`` defaults to ``global_install=True``, so the + skill lands under ``GLOBAL_SKILLS_DIR`` rather than ``USER_SKILLS_DIR``. + Checking against a narrower directory (e.g. workspace_dir) would pass + even without the scrub - the check has to cover every path the tool + might resolve to. ``tmp_path`` covers both patched dirs and the source + path used by ``install_skill``. + """ + from EvoScientist.tools.skill_manager import skill_manager + + workspace_dir = tmp_path / "workspace" + workspace_dir.mkdir() + global_dir = tmp_path / "global-empty" + global_dir.mkdir() + self._make_skill(tmp_path, "leak-guard-install") + + with ( + patch("EvoScientist.paths.USER_SKILLS_DIR", workspace_dir), + patch("EvoScientist.paths.GLOBAL_SKILLS_DIR", global_dir), + ): + result = skill_manager.invoke( + {"action": "install", "source": str(tmp_path / "leak-guard-install")} + ) + + assert str(tmp_path) not in result + + def test_install_batch_lists_each_skill_with_virtual_path(self, tmp_path): + """Batch install: one block per installed skill, each with ``Path: /skills/``.""" + from EvoScientist.tools.skill_manager import skill_manager + + workspace_dir = tmp_path / "workspace" + workspace_dir.mkdir() + global_dir = tmp_path / "global-empty" + global_dir.mkdir() + pack = tmp_path / "pack" + pack.mkdir() + self._make_skill(pack, "alpha", description="first") + self._make_skill(pack, "beta", description="second") + + with ( + patch("EvoScientist.paths.USER_SKILLS_DIR", workspace_dir), + patch("EvoScientist.paths.GLOBAL_SKILLS_DIR", global_dir), + ): + result = skill_manager.invoke({"action": "install", "source": str(pack)}) + + assert "Successfully installed skill: alpha" in result + assert "Successfully installed skill: beta" in result + assert "Path: /skills/alpha" in result + assert "Path: /skills/beta" in result + # Same leak guard as ``test_install_single_omits_host_path``: batch + # returns must not surface any host path either. Cover every dir the + # install might resolve to. + assert str(tmp_path) not in result + + def test_install_batch_all_fail_returns_error_list(self, tmp_path): + """Batch install where every skill fails must not KeyError on the + missing top-level ``error`` field. + + Pre-fix behavior: ``result['error']`` crashed because + ``_batch_install_local`` returns ``{"success": False, "batch": True, + "installed": [], "failed": [{"name": ..., "error": ...}]}`` with no + top-level ``error`` key. This test pins the guard. + """ + from EvoScientist.tools.skill_manager import skill_manager + + workspace_dir = tmp_path / "workspace" + workspace_dir.mkdir() + global_dir = tmp_path / "global-empty" + global_dir.mkdir() + + batch_result = { + "success": False, + "batch": True, + "installed": [], + "failed": [ + {"name": "broken-a", "error": "corrupt frontmatter"}, + {"name": "broken-b", "error": "missing SKILL.md"}, + ], + } + + with ( + patch("EvoScientist.paths.USER_SKILLS_DIR", workspace_dir), + patch("EvoScientist.paths.GLOBAL_SKILLS_DIR", global_dir), + patch( + "EvoScientist.tools.skills_manager.install_skill", + return_value=batch_result, + ), + ): + result = skill_manager.invoke( + {"action": "install", "source": "some/source"} + ) + + # No KeyError, and every failure surfaced. + assert "broken-a" in result + assert "corrupt frontmatter" in result + assert "broken-b" in result + assert "missing SKILL.md" in result + + def test_install_batch_partial_fail_surfaces_both(self, tmp_path): + """Batch install with partial failure lists successes AND failures. + + Pre-fix behavior: partial failures were silently dropped; only the + success blocks reached the agent. + """ + from EvoScientist.tools.skill_manager import skill_manager + + workspace_dir = tmp_path / "workspace" + workspace_dir.mkdir() + global_dir = tmp_path / "global-empty" + global_dir.mkdir() + + partial_result = { + "success": True, + "batch": True, + "installed": [ + { + "name": "worked", + "path": str(workspace_dir / "worked"), + "description": "installed cleanly", + }, + ], + "failed": [ + {"name": "broken", "error": "corrupt frontmatter"}, + ], + } + + with ( + patch("EvoScientist.paths.USER_SKILLS_DIR", workspace_dir), + patch("EvoScientist.paths.GLOBAL_SKILLS_DIR", global_dir), + patch( + "EvoScientist.tools.skills_manager.install_skill", + return_value=partial_result, + ), + ): + result = skill_manager.invoke( + {"action": "install", "source": "some/source"} + ) + + assert "Successfully installed skill: worked" in result + + assert "Path: /skills/worked" in result + assert "broken" in result + assert "corrupt frontmatter" in result