fix(skills): implement skill remove and search CLI commands (#240)

src/openjarvis/cli/skill_cmd.py

@@ -381,18 +381,118 @@ def sources():
 
 @skill.command("remove")
 @click.argument("skill_name")
-def remove(skill_name: str):
-    """Remove an installed skill."""
-    Console().print("[yellow]Skill removal not yet implemented.[/yellow]")
+@click.option(
+    "--yes",
+    "-y",
+    is_flag=True,
+    default=False,
+    help="Skip confirmation prompt.",
+)
+def remove(skill_name: str, yes: bool):
+    """Remove an installed skill by name.
+
+    Searches ``~/.openjarvis/skills/`` and ``./skills`` for a directory whose
+    name (or parsed manifest name) matches ``skill_name`` and deletes it.
+    """
+    console = Console()
+    mgr = SkillManager(bus=EventBus())
+    paths = mgr.find_installed_paths(skill_name, roots=_get_skill_paths())
+    if not paths:
+        console.print(f"[red]No installed skill named '{skill_name}' found.[/red]")
+        raise SystemExit(1)
+
+    console.print(f"[bold]Will remove {len(paths)} location(s):[/bold]")
+    for p in paths:
+        console.print(f"  - {p}")
+
+    if not yes:
+        if not click.confirm("Proceed?", default=False):
+            console.print("[dim]Aborted.[/dim]")
+            return
+
+    try:
+        removed = mgr.remove(skill_name, roots=_get_skill_paths())
+    except FileNotFoundError as exc:
+        console.print(f"[red]{exc}[/red]")
+        raise SystemExit(1)
+    for p in removed:
+        console.print(f"[green]Removed:[/green] {p}")
 
 
 @skill.command("search")
 @click.argument("query")
-def search(query: str):
-    """Search available skills."""
-    Console().print(
-        "[yellow]Skill search not yet implemented. "
-        "Run 'jarvis skill update' first.[/yellow]"
+@click.option(
+    "--source",
+    "-s",
+    default="",
+    help="Restrict search to a single configured source.",
+)
+def search(query: str, source: str):
+    """Search available skills across configured sources.
+
+    Matches ``query`` (case-insensitive substring) against skill name,
+    description, and tags.
+    """
+    console = Console()
+    cfg = load_config()
+
+    if not cfg.skills.sources:
+        console.print(
+            "[yellow]No skill sources configured. "
+            "Add entries to [skills.sources] in config.toml.[/yellow]"
+        )
+        raise SystemExit(1)
+
+    sources_to_search = [
+        s for s in cfg.skills.sources if not source or s.source == source
+    ]
+    if source and not sources_to_search:
+        console.print(f"[red]No configured source named '{source}'.[/red]")
+        raise SystemExit(1)
+
+    q = query.lower().strip()
+    rows: list[tuple[str, str, str, str]] = []  # source, name, category, description
+    for src_cfg in sources_to_search:
+        try:
+            resolver = _get_resolver(src_cfg.source, url=src_cfg.url)
+            resolver.sync()
+        except Exception as exc:
+            console.print(f"[yellow]Skipped {src_cfg.source}: {exc}[/yellow]")
+            continue
+
+        for resolved in resolver.list_skills():
+            haystack = " ".join(
+                [
+                    resolved.name or "",
+                    resolved.description or "",
+                    resolved.category or "",
+                ]
+            ).lower()
+            if q in haystack:
+                rows.append(
+                    (
+                        src_cfg.source,
+                        resolved.name,
+                        getattr(resolved, "category", "") or "",
+                        (getattr(resolved, "description", "") or "")[:60],
+                    )
+                )
+
+    if not rows:
+        console.print(f"[dim]No skills matching '{query}'.[/dim]")
+        return
+
+    table = Table(title=f"Search results for '{query}'")
+    table.add_column("Source", style="cyan")
+    table.add_column("Name", style="bold")
+    table.add_column("Category")
+    table.add_column("Description", max_width=60)
+    for row in rows:
+        table.add_row(*row)
+    console.print(table)
+    console.print(
+        f"[dim]{len(rows)} match(es). "
+        f"Install with: jarvis skill install <source>:<name>[/dim]"
     )
 
 

src/openjarvis/skills/manager.py

@@ -54,9 +54,7 @@ class SkillManager:
             except Exception:
                 pass
             if overlay_dir is None:
-                overlay_dir = Path(
-                    "~/.openjarvis/learning/skills/"
-                ).expanduser()
+                overlay_dir = Path("~/.openjarvis/learning/skills/").expanduser()
         self._overlay_dir = Path(overlay_dir).expanduser()
 
     # ------------------------------------------------------------------
@@ -364,16 +362,62 @@ class SkillManager:
         self._tool_executor = tool_executor
 
     # ------------------------------------------------------------------
-    # Lifecycle stubs (not yet implemented)
+    # Lifecycle
     # ------------------------------------------------------------------
 
-    def install(self, source: Any, *, verify: bool = True) -> None:
-        """Install a skill from a remote source.  Not yet implemented."""
-        raise NotImplementedError("Skill installation not yet implemented")
+    def find_installed_paths(
+        self, name: str, *, roots: Optional[List[Path]] = None
+    ) -> List[Path]:
+        """Return on-disk skill directories matching ``name``.
 
-    def remove(self, name: Any) -> None:
-        """Remove a skill by name.  Not yet implemented."""
-        raise NotImplementedError("Skill removal not yet implemented")
+        A directory matches when it contains ``skill.toml`` or ``SKILL.md``
+        and either the directory name equals ``name`` or its parsed
+        manifest's ``name`` field equals ``name``.
+        """
+        if roots is None:
+            roots = [Path("~/.openjarvis/skills/").expanduser(), Path("./skills")]
+
+        matches: List[Path] = []
+        for root in roots:
+            if not root.exists():
+                continue
+            for candidate in root.rglob("*"):
+                if not candidate.is_dir():
+                    continue
+                toml = candidate / "skill.toml"
+                md = candidate / "SKILL.md"
+                if not (toml.exists() or md.exists()):
+                    continue
+                if candidate.name == name:
+                    matches.append(candidate)
+                    continue
+                # Fall back to parsed manifest name
+                try:
+                    from openjarvis.skills.loader import load_skill_directory
+
+                    manifest = load_skill_directory(candidate)
+                    if manifest is not None and manifest.name == name:
+                        matches.append(candidate)
+                except Exception:
+                    continue
+        return matches
+
+    def remove(self, name: str, *, roots: Optional[List[Path]] = None) -> List[Path]:
+        """Remove an installed skill by name.
+
+        Returns the list of directories that were removed.  Raises
+        :class:`FileNotFoundError` when no matching skill exists on disk.
+        """
+        import shutil
+
+        paths = self.find_installed_paths(name, roots=roots)
+        if not paths:
+            raise FileNotFoundError(f"No installed skill named {name!r}")
+        for p in paths:
+            shutil.rmtree(p)
+        # Drop from in-memory catalog
+        self._skills.pop(name, None)
+        return paths
 
 
 # ---------------------------------------------------------------------------

tests/cli/test_skill_cmd.py

@@ -93,6 +93,106 @@ class TestSkillCmd:
             assert "test_author" in result.output
 
 
+class TestSkillRemoveCommand:
+    def test_remove_help(self) -> None:
+        result = CliRunner().invoke(cli, ["skill", "remove", "--help"])
+        assert result.exit_code == 0
+
+    def test_remove_missing_skill(self, tmp_path: Path) -> None:
+        with patch(
+            "openjarvis.cli.skill_cmd._get_skill_paths",
+            return_value=[tmp_path],
+        ):
+            result = CliRunner().invoke(cli, ["skill", "remove", "ghost", "--yes"])
+            assert result.exit_code != 0
+            assert "no installed skill" in result.output.lower()
+
+    def test_remove_deletes_directory(self, tmp_path: Path) -> None:
+        skill_dir = tmp_path / "to_remove"
+        skill_dir.mkdir()
+        (skill_dir / "skill.toml").write_text(
+            textwrap.dedent("""\
+            [skill]
+            name = "to_remove"
+            description = "doomed"
+
+            [[skill.steps]]
+            tool_name = "echo"
+            output_key = "x"
+        """)
+        )
+        with patch(
+            "openjarvis.cli.skill_cmd._get_skill_paths",
+            return_value=[tmp_path],
+        ):
+            result = CliRunner().invoke(cli, ["skill", "remove", "to_remove", "--yes"])
+            assert result.exit_code == 0, result.output
+            assert "Removed" in result.output
+            assert not skill_dir.exists()
+
+
+class TestSkillSearchCommand:
+    def test_search_help(self) -> None:
+        result = CliRunner().invoke(cli, ["skill", "search", "--help"])
+        assert result.exit_code == 0
+
+    def test_search_no_sources_configured(self) -> None:
+        from openjarvis.core.config import JarvisConfig, SkillsConfig
+
+        cfg = JarvisConfig()
+        cfg.skills = SkillsConfig(sources=[])
+        with patch("openjarvis.cli.skill_cmd.load_config", return_value=cfg):
+            result = CliRunner().invoke(cli, ["skill", "search", "anything"])
+            assert result.exit_code != 0
+            assert "no skill sources" in result.output.lower()
+
+    def test_search_filters_results(self) -> None:
+        from pathlib import Path as _P
+
+        from openjarvis.core.config import (
+            JarvisConfig,
+            SkillsConfig,
+            SkillSourceConfig,
+        )
+        from openjarvis.skills.sources.base import ResolvedSkill
+
+        class _FakeResolver:
+            def sync(self) -> None:
+                return None
+
+            def list_skills(self):
+                return [
+                    ResolvedSkill(
+                        name="apple-notes",
+                        source="hermes",
+                        path=_P("/tmp"),
+                        category="apple",
+                        description="Take notes on macOS",
+                        commit="abc",
+                    ),
+                    ResolvedSkill(
+                        name="github-prs",
+                        source="hermes",
+                        path=_P("/tmp"),
+                        category="dev",
+                        description="List pull requests",
+                        commit="def",
+                    ),
+                ]
+
+        cfg = JarvisConfig()
+        cfg.skills = SkillsConfig(sources=[SkillSourceConfig(source="hermes")])
+        with patch("openjarvis.cli.skill_cmd.load_config", return_value=cfg):
+            with patch(
+                "openjarvis.cli.skill_cmd._get_resolver",
+                return_value=_FakeResolver(),
+            ):
+                result = CliRunner().invoke(cli, ["skill", "search", "notes"])
+                assert result.exit_code == 0, result.output
+                assert "apple-notes" in result.output
+                assert "github-prs" not in result.output
+
+
 class TestSkillInstallCommand:
     def test_install_help(self) -> None:
         result = CliRunner().invoke(cli, ["skill", "install", "--help"])

tests/skills/test_manager.py

@@ -480,9 +480,7 @@ class TestSkillManagerOverlayLoading:
 
         with patch("openjarvis.core.config.load_config", return_value=cfg):
             mgr = SkillManager(bus=EventBus())
-            assert (
-                mgr._overlay_dir == (tmp_path / "configured-overlays").expanduser()
-            )
+            assert mgr._overlay_dir == (tmp_path / "configured-overlays").expanduser()
 
     def test_discover_with_empty_paths_still_loads_overlays(
         self, tmp_path: Path
@@ -518,3 +516,54 @@ class TestSkillManagerOverlayLoading:
 
         manifest = mgr.resolve("seeded-skill")
         assert manifest.description == "Optimized seeded description"
+
+
+class TestSkillManagerRemove:
+    def test_find_installed_paths_returns_empty_when_missing(
+        self, tmp_path: Path
+    ) -> None:
+        mgr = SkillManager(bus=EventBus())
+        assert mgr.find_installed_paths("ghost", roots=[tmp_path]) == []
+
+    def test_find_installed_paths_matches_directory_name(self, tmp_path: Path) -> None:
+        _write_toml_skill(tmp_path, "alpha")
+        mgr = SkillManager(bus=EventBus())
+        paths = mgr.find_installed_paths("alpha", roots=[tmp_path])
+        assert paths == [tmp_path / "alpha"]
+
+    def test_find_installed_paths_matches_manifest_name(self, tmp_path: Path) -> None:
+        # Directory name differs from manifest name (mimics imported layout)
+        skill_dir = tmp_path / "some-other-dirname"
+        skill_dir.mkdir()
+        (skill_dir / "skill.toml").write_text(
+            textwrap.dedent("""\
+                [skill]
+                name = "real-name"
+                description = "renamed"
+
+                [[skill.steps]]
+                tool_name = "echo"
+                output_key = "x"
+            """)
+        )
+        mgr = SkillManager(bus=EventBus())
+        paths = mgr.find_installed_paths("real-name", roots=[tmp_path])
+        assert paths == [skill_dir]
+
+    def test_remove_deletes_directory_and_drops_from_catalog(
+        self, tmp_path: Path
+    ) -> None:
+        _write_toml_skill(tmp_path, "doomed")
+        mgr = SkillManager(bus=EventBus())
+        mgr.discover(paths=[tmp_path])
+        assert "doomed" in mgr.skill_names()
+
+        removed = mgr.remove("doomed", roots=[tmp_path])
+        assert removed == [tmp_path / "doomed"]
+        assert not (tmp_path / "doomed").exists()
+        assert "doomed" not in mgr.skill_names()
+
+    def test_remove_raises_when_skill_missing(self, tmp_path: Path) -> None:
+        mgr = SkillManager(bus=EventBus())
+        with pytest.raises(FileNotFoundError):
+            mgr.remove("ghost", roots=[tmp_path])