From ed2acf1895d4ff66addd5115ecbec02033291d63 Mon Sep 17 00:00:00 2001 From: Andrew Park Date: Mon, 13 Apr 2026 15:03:39 -0700 Subject: [PATCH] fix(skills): implement skill remove and search CLI commands (#240) --- src/openjarvis/cli/skill_cmd.py | 116 ++++++++++++++++++++++++++++--- src/openjarvis/skills/manager.py | 64 ++++++++++++++--- tests/cli/test_skill_cmd.py | 100 ++++++++++++++++++++++++++ tests/skills/test_manager.py | 55 ++++++++++++++- 4 files changed, 314 insertions(+), 21 deletions(-) diff --git a/src/openjarvis/cli/skill_cmd.py b/src/openjarvis/cli/skill_cmd.py index c935c7e7..1e612a02 100644 --- a/src/openjarvis/cli/skill_cmd.py +++ b/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 :[/dim]" ) diff --git a/src/openjarvis/skills/manager.py b/src/openjarvis/skills/manager.py index 487de511..4549f6b8 100644 --- a/src/openjarvis/skills/manager.py +++ b/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 # --------------------------------------------------------------------------- diff --git a/tests/cli/test_skill_cmd.py b/tests/cli/test_skill_cmd.py index 7dea402b..2ba684f4 100644 --- a/tests/cli/test_skill_cmd.py +++ b/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"]) diff --git a/tests/skills/test_manager.py b/tests/skills/test_manager.py index 017fb80a..74ee4be1 100644 --- a/tests/skills/test_manager.py +++ b/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])