diff --git a/README.md b/README.md index 21303f5..2db59e9 100644 --- a/README.md +++ b/README.md @@ -275,14 +275,108 @@ Add to your editor's MCP config: ### AI Tool Integration -Automatically detect and configure AI coding assistants with Deepgram skills. +Install the Deepgram agent skills from +[`deepgram/skills`](https://github.com/deepgram/skills) into the AI coding +tools on this machine. Each skill is installed as a folder, into the +user-scope skills directory the tool's own documentation names. ```bash -dg skills status # Detect AI tools +dg skills status # Detect AI tools and show their skills directories dg skills setup # Interactive setup wizard dg skills install --all # Install for all detected tools +dg skills list # Show what is installed, and from which ref +dg skills update # Reinstall from upstream +dg skills remove --all # Uninstall (--cli NAME for one tool) ``` +| Tool | Skills directory | +| --- | --- | +| Claude Code | `~/.claude/skills/` | +| OpenAI Codex | `~/.agents/skills/` | +| Gemini CLI | `~/.gemini/skills/` | +| Cursor | `~/.cursor/skills/` | +| OpenCode | `~/.config/opencode/skills/` | +| Cline | `~/.cline/skills/` | + +Amazon Q Developer and Aider have no skills mechanism, so `dg skills` prints +`npx skills add deepgram/skills` for those rather than writing a file they +would not read. + +Installs are pinned to a released `deepgram/skills` tag so the same deepctl +version always installs the same skills. Override with `--ref` or the +`DEEPCTL_SKILLS_REF` environment variable: + +```bash +dg skills install --all --ref main # track the upstream default branch +``` + +A failed download, an unknown ref, or an upstream manifest that does not match +the directories it lists is a hard failure (exit 1) with nothing written — a +partial install is indistinguishable from a complete one once it is on disk. + +**deepctl only ever touches skill folder paths it installed.** Those +directories are shared: your own skills and other publishers' skills live in +them too. So `dg skills` records every folder it writes in +`~/.deepctl/skills/skills.json` and works on that list alone. + +- `install`, `update` and `setup` refuse to overwrite a folder that is not on + the list — if you already have a skill called `api`, the install exits 1 and + writes nothing, naming the folder so you can rename it. +- `remove` deletes only the recorded folders. An unrelated skill in the same + directory stays. A recorded folder it *could not* delete — a permission + error, a read-only mount — stays recorded and `remove` exits 1, so the next + `remove` or `update` can still reach it. Dropping the record there would + leave Deepgram's own folders behind with nothing able to touch them. +- `status` counts only the recorded folders, not everything with a `SKILL.md`. +- Those exit codes are for the `dg skills` subcommands. Two other commands + install skills. `dg login` offers the same install after a successful + login, but only at an interactive prompt and only while nothing is + recorded as installed yet. `dg plugin install/update/remove` refreshes what + is already installed, unless `auto_update` is set to `false` in + `skills.json`. Both go through the same ownership rules, but a collision + or a download failure there is a warning rather than a failure — a skills + problem never changes whether the login or the plugin operation succeeded. + Run `dg skills install` to see the error and get the exit code. +- If you delete `skills.json`, deepctl can no longer prove it installed + anything: `remove` deletes nothing and `install` reports the collision rather + than reclaiming the folders. Delete them by hand, then install again. +- The list holds *paths*, not fingerprints. Delete a folder deepctl installed + and put your own folder — or a file — at the same path without running + `dg skills remove --cli `, and deepctl still counts it as its own: the + next `update` replaces it and `remove` deletes it. Where the filesystem + ignores case, as macOS and Windows do by default, `API` and `api` are one path + for this purpose. So run `dg skills remove --cli ` first, or drop the + entry from `skills.json`, before reusing a name deepctl installed under. +- A *symlink* is the exception: deepctl never writes or deletes through one. + Put a symlink where a recorded skill folder was and that path stops being + deepctl's — `install`, `update` and `setup` exit 1 naming it rather than + replacing it, and `remove` reports where it is, drops it from the list and + leaves it on disk rather than following it to whatever it points at. Delete + the symlink yourself to hand the name back; until you do, installing under + that name keeps failing. + +#### Upgrading from deepctl 0.2.16 through 0.3.0 + +Those versions wrote to paths that are not skills directories, so `install`, +`update`, `setup` and `remove` clear them for the tools that run — a command +that exits early, such as an install that hits a collision or cannot download, +clears nothing. Otherwise four stale skills would sit next to fourteen fresh +ones. These paths are the only thing `dg skills` touches outside its own skill +folders and its own `~/.deepctl/` directory, and the list is scoped to what +0.3.0 wrote: + +| Path | What happens | +| --- | --- | +| `~/.claude/commands/deepgram/` | Deletes `api.md`, `docs.md`, `setup-mcp.md`, `starters.md` and `deepgram.md` by name, whoever wrote them; a command you added under any other name stays, and the directory goes only if that empties it. If `deepgram` is itself a symlink — dotfiles kept in a repo — nothing is deleted through it | +| `~/.codex/instructions.md`, `~/.gemini/GEMINI.md`, `~/.opencode/agents.md` | Cuts out only the section between `` and ``; the rest of the file is yours and is kept. If the opening marker is there without the closing one — what a write cut short leaves behind — everything after it counts as that unfinished section and goes. These files are followed through a symlink, because only deepctl's own marked section is ever touched | +| `~/.cursor/rules/deepctl.mdc`, `~/.cline/rules/deepctl.md`, `~/.amazonq/rules/deepctl.md` | Deleted — 0.3.0 created these files and nothing else writes them | +| `~/.aider.conf.yml` | Drops the stale `read:` entry pointing at deepctl's old conventions file | + +deepctl 0.2.15 and earlier wrote one combined file at +`~/.claude/commands/deepctl.md` instead, with no marker around it. Nothing +distinguishes it from a `/deepctl` slash command you wrote yourself, so the +cleanup leaves it alone. Delete it by hand if it is there. + ### Starter Apps Scaffold a new project from Deepgram templates. diff --git a/packages/deepctl-cmd-login/src/deepctl_cmd_login/command.py b/packages/deepctl-cmd-login/src/deepctl_cmd_login/command.py index 1b4de0b..07cf152 100644 --- a/packages/deepctl-cmd-login/src/deepctl_cmd_login/command.py +++ b/packages/deepctl-cmd-login/src/deepctl_cmd_login/command.py @@ -1,5 +1,6 @@ """Login command for deepctl.""" +from pathlib import Path from typing import Any from deepctl_core import ( @@ -237,41 +238,72 @@ def _maybe_prompt_skills_setup(self) -> None: console.print("[dim]No tools selected.[/dim]") return - # Install skills for selected tools + # Install skills for selected tools. The same shared helper + # 'dg skills install' uses, so login cannot drift from the + # ownership contract: one fetch for every tool, every + # destination preflighted before anything is written, and the + # record saved as each tool lands rather than at the end. + # Best-effort here only in that a failure is reported and the + # login still succeeds -- never in that a folder is written + # without deepctl recording that it owns it. + from deepctl_core.skill_bundle import SkillFetchError from deepctl_core.skill_generator import ( - _commands_hash, collect_command_metadata, + install_skills_for, save_skills_state, ) console.print("\n[blue]Installing Deepgram skills...[/blue]") import importlib.metadata - from datetime import datetime, timezone - commands = collect_command_metadata() try: version = importlib.metadata.version("deepctl") except importlib.metadata.PackageNotFoundError: version = "0.0.0" - for gen in selected: - paths = gen.install(commands, version) - cmd_hash = _commands_hash(commands) - state["installed_skills"][gen.cli_name] = { - "paths": [str(p) for p in paths], - "installed_at": datetime.now(timezone.utc).isoformat(), - "version": version, - "commands_hash": cmd_hash, - } + def announce(gen: Any, paths: list[Path]) -> None: for p in paths: console.print(f" [green]✓[/green] {gen.display_name} → {p}") + try: + report = install_skills_for( + selected, + state, + commands=collect_command_metadata(), + version=version, + on_installed=announce, + best_effort=True, + ) + except SkillFetchError as exc: + # Nothing was written, so there is no ownership to save. + # Say so rather than leaving the banner above unanswered. + console.print( + f"[yellow] Could not download the Deepgram skills: " + f"{exc}. Run 'dg skills install' to retry.[/yellow]" + ) + return + for gen in report.unsupported: + console.print(f"[yellow] {gen.manual_hint()}[/yellow]") + # Someone else's skill folder has one of these names, so the + # tool was skipped rather than have their work overwritten. + for display_name, path in report.conflicts: + console.print( + f"[yellow] Skipped {display_name}: {path} is not " + "deepctl's to replace.[/yellow]" + ) + for display_name, failure in report.failures: + console.print( + f"[yellow] {display_name}: {failure}. Run " + "'dg skills install' to retry.[/yellow]" + ) save_skills_state(state) - console.print( - "\n[green]Skills installed![/green] " - "[dim]Run 'dg skills update' after plugin changes.[/dim]" - ) + + if report.total_written: + console.print( + "\n[green]Skills installed![/green] " + "[dim]Run 'dg skills update' after plugin changes.[/dim]" + ) except Exception: pass # Best-effort — never fail the login diff --git a/packages/deepctl-cmd-login/tests/unit/test_login_command.py b/packages/deepctl-cmd-login/tests/unit/test_login_command.py index fa0d53d..6a2a898 100644 --- a/packages/deepctl-cmd-login/tests/unit/test_login_command.py +++ b/packages/deepctl-cmd-login/tests/unit/test_login_command.py @@ -1,5 +1,6 @@ """Tests for the login command.""" +from pathlib import Path from unittest.mock import MagicMock, Mock, call, patch import pytest @@ -11,6 +12,22 @@ from deepctl_cmd_login.models import LoginResult, LogoutResult from deepctl_core import AuthManager, Config, DeepgramClient from deepctl_core.models import ProfileInfo, ProfilesResult +from deepctl_core.skill_bundle import RepoSkill + + +@pytest.fixture(autouse=True) +def _no_real_skill_installs(): + """Keep the post-login skills prompt off this machine. + + A successful login calls ``_maybe_prompt_skills_setup()``, whose only + guard is ``sys.stdout.isatty()``. Under ``pytest -s`` that is True, and + the prompt then downloads the deepgram/skills bundle and installs it + into the real ``~/.claude/skills`` and friends. Reporting no detected + tools stops it at the first branch; the tests that exercise the prompt + itself patch this same function and win over this fixture. + """ + with patch("deepctl_core.skill_generator.detect_ai_clis", return_value=[]): + yield @pytest.fixture @@ -475,3 +492,122 @@ def test_env_key_without_profile_labeled_env( profile_key=None, ) assert result.key_source == "DEEPGRAM_API_KEY (env)" + + +class TestLoginRecordsTheSameStateAsSkillsInstall: + """`dg login` writes the record `dg skills list/update/remove` then read.""" + + def _generator(self, cli_name, display_name, root, paths): + gen = MagicMock() + gen.cli_name = cli_name + gen.display_name = display_name + gen.skills_root.return_value = root + gen.install_conflicts.return_value = [] + gen.install_skills.return_value = paths + gen.prune_retired.return_value = [] + gen.manual_hint.return_value = f"{display_name} has no skills directory." + return gen + + def _run(self, generators, state, skills=("api", "docs")): + cmd = LoginCommand() + cmd._guided = True + bundle = [ + RepoSkill(name=name, path=Path("/upstream") / name) for name in skills + ] + with ( + patch("sys.stdout") as mock_stdout, + patch( + "deepctl_core.skill_generator.detect_ai_clis", return_value=generators + ), + patch( + "deepctl_core.skill_generator.get_skills_state", return_value=state + ), + patch("deepctl_core.skill_generator.save_skills_state"), + patch( + "deepctl_core.skill_generator.collect_command_metadata", + return_value=[], + ), + patch( + "deepctl_core.skill_generator.fetch_repo_skills", return_value=bundle + ) as fetch, + patch("deepctl_cmd_login.command.Prompt.ask", return_value="all"), + ): + mock_stdout.isatty.return_value = True + cmd._maybe_prompt_skills_setup() + self.fetch = fetch + return state + + def test_it_records_the_upstream_ref_and_skill_names(self, tmp_path): + """Without these, `dg skills list` prints '?' for the ref it pinned.""" + from deepctl_core.skill_bundle import DEFAULT_SKILLS_REF + + root = tmp_path / ".claude" / "skills" + gen = self._generator( + "claude", "Claude Code", root, [root / "api", root / "docs"] + ) + state = self._run([gen], {"installed_skills": {}}) + + entry = state["installed_skills"]["claude"] + assert entry["skills_ref"] == DEFAULT_SKILLS_REF + assert entry["skills"] == ["api", "docs"] + assert [Path(p).name for p in entry["paths"]] == ["api", "docs"] + + def test_a_tool_with_no_skills_directory_is_not_recorded(self): + """Nothing was written for it, so nothing may claim it was.""" + gen = self._generator("amazonq", "Amazon Q Developer", None, []) + with patch("deepctl_cmd_login.command.console") as printer: + state = self._run([gen], {"installed_skills": {}}) + + assert state["installed_skills"] == {} + # The empty map is also what this started as, so on its own it + # would pass if the whole block had thrown into login's bare + # `except`. The hint only prints from the far side of the + # install, which is what pins down that it ran and declined. + printed = " ".join(str(c) for c in printer.print.call_args_list) + assert "Amazon Q Developer has no skills directory." in printed + gen.install_skills.assert_not_called() + + def test_a_second_tool_failing_leaves_the_first_recorded(self, tmp_path): + """Login used to save state only after the whole loop. + + It installed tool by tool, refetching the bundle each time, and a + later failure hit the bare `except` before `save_skills_state`. + Whatever the earlier tools had written was then folders deepctl + would neither update nor remove. + """ + root = tmp_path / ".claude" / "skills" + first = self._generator("claude", "Claude Code", root, [root / "api"]) + second = self._generator( + "cursor", "Cursor", tmp_path / ".cursor" / "skills", [] + ) + second.install_skills.side_effect = OSError(30, "Read-only file system") + + with patch("deepctl_cmd_login.command.console") as printer: + state = self._run( + [first, second], {"installed_skills": {}}, skills=("api",) + ) + + entry = state["installed_skills"]["claude"] + assert [Path(p).name for p in entry["paths"]] == ["api"] + assert "cursor" not in state["installed_skills"] + # And the user is told, rather than the login going quiet on it. + # Not just "Cursor" -- every detected tool is named in the menu + # printed before the install, so that would match either way. + printed = " ".join(str(c) for c in printer.print.call_args_list) + assert "Cursor: [Errno 30] Read-only file system" in printed + assert "Run 'dg skills install' to retry" in printed + + def test_the_bundle_is_fetched_once_for_every_tool(self, tmp_path): + """Two fetches could install two different revisions side by side.""" + claude_root = tmp_path / ".claude" / "skills" + cursor_root = tmp_path / ".cursor" / "skills" + generators = [ + self._generator( + "claude", "Claude Code", claude_root, [claude_root / "api"] + ), + self._generator("cursor", "Cursor", cursor_root, [cursor_root / "api"]), + ] + + self._run(generators, {"installed_skills": {}}, skills=("api",)) + + assert self.fetch.call_count == 1 diff --git a/packages/deepctl-cmd-plugin/src/deepctl_cmd_plugin/command.py b/packages/deepctl-cmd-plugin/src/deepctl_cmd_plugin/command.py index 755112c..cfc7754 100644 --- a/packages/deepctl-cmd-plugin/src/deepctl_cmd_plugin/command.py +++ b/packages/deepctl-cmd-plugin/src/deepctl_cmd_plugin/command.py @@ -1173,11 +1173,12 @@ def _get_plugin_commands(self, plugin_name: str) -> str: def _maybe_update_skills(self) -> None: """Regenerate AI CLI skills if installed (best-effort).""" try: + from deepctl_core.skill_bundle import SkillFetchError from deepctl_core.skill_generator import ( - _commands_hash, collect_command_metadata, get_all_generators, get_skills_state, + install_skills_for, save_skills_state, ) @@ -1185,23 +1186,55 @@ def _maybe_update_skills(self) -> None: if not state.get("installed_skills") or not state.get("auto_update", True): return - commands = collect_command_metadata() - version = importlib.metadata.version("deepctl") generators = {g.cli_name: g for g in get_all_generators()} + # Only the tools already recorded, and only those deepctl can + # install to. A tool with no skills directory keeps whatever + # record it has: this refresh is not the place to revise it. + targets = [ + gen + for cli_name in state["installed_skills"] + if (gen := generators.get(cli_name)) is not None + and gen.skills_root() is not None + ] + if not targets: + return - for cli_name, info in state["installed_skills"].items(): - gen = generators.get(cli_name) - if gen: - paths = gen.install(commands, version) - info.update( - { - "paths": [str(p) for p in paths], - "version": version, - "commands_hash": _commands_hash(commands), - } - ) - + # The shared helper 'dg skills update' uses, so a refresh + # triggered by a plugin change obeys the same contract: one + # fetch, every destination preflighted, and the ownership + # record saved as each tool lands. Best-effort only in that a + # failure is reported instead of failing the plugin command. + try: + report = install_skills_for( + targets, + state, + commands=collect_command_metadata(), + version=importlib.metadata.version("deepctl"), + best_effort=True, + ) + except SkillFetchError as exc: + # Nothing was written, so the records still describe what + # is on disk. Say the refresh did not happen. + console.print( + f"[yellow]AI assistant skills not updated: {exc}. " + "Run 'dg skills update' to retry[/yellow]" + ) + return save_skills_state(state) - console.print("[dim]AI assistant skills updated[/dim]") + + for display_name, path in report.conflicts: + # Not deepctl's folder to replace. Left alone, and so is + # the record describing the install that is on disk. + console.print( + f"[yellow]Skipped {display_name} skills: {path} is not " + "deepctl's to replace[/yellow]" + ) + for display_name, failure in report.failures: + console.print( + f"[yellow]{display_name} skills not updated: {failure}. " + "Run 'dg skills update' to retry[/yellow]" + ) + if report.written: + console.print("[dim]AI assistant skills updated[/dim]") except Exception: pass # Non-fatal diff --git a/packages/deepctl-cmd-plugin/tests/unit/test_plugin_command.py b/packages/deepctl-cmd-plugin/tests/unit/test_plugin_command.py index 5c4cfa4..4427b5b 100644 --- a/packages/deepctl-cmd-plugin/tests/unit/test_plugin_command.py +++ b/packages/deepctl-cmd-plugin/tests/unit/test_plugin_command.py @@ -16,6 +16,27 @@ from deepctl_core.auth import AuthManager from deepctl_core.client import DeepgramClient from deepctl_core.config import Config +from deepctl_core.skill_bundle import RepoSkill + + +# Captured before the autouse fixture below replaces the attribute, so the +# one class that does want to exercise it still can. +_REAL_MAYBE_UPDATE_SKILLS = PluginCommand._maybe_update_skills + + +@pytest.fixture(autouse=True) +def _no_real_skill_installs(): + """Keep the skills refresh that follows a plugin operation off this machine. + + Every successful plugin install, update or uninstall calls + ``_maybe_update_skills()``, which downloads the deepgram/skills bundle + and reinstalls it into the real ``~/.claude/skills`` and friends, + deleting ``~/.amazonq/rules/deepctl.md`` and rewriting + ``~/.aider.conf.yml`` on the way. It swallows every exception, so a + test suite doing that leaves no trace in its own output. + """ + with patch.object(PluginCommand, "_maybe_update_skills", return_value=None): + yield class TestPluginCommand: @@ -607,3 +628,148 @@ def test_needs_isolated_venv(self) -> None: assert self.command._needs_isolated_venv(InstallMethod.PIP) is False assert self.command._needs_isolated_venv(InstallMethod.PIPX) is False assert self.command._needs_isolated_venv(InstallMethod.UV) is False + + +class TestSkillsRefreshAfterAPluginChange: + """`_maybe_update_skills` writes the record `dg skills list` then reads.""" + + def _generator(self, cli_name, root, paths): + gen = MagicMock() + gen.cli_name = cli_name + gen.display_name = cli_name + gen.skills_root.return_value = root + gen.install_conflicts.return_value = [] + gen.install_skills.return_value = paths + gen.prune_retired.return_value = [] + return gen + + def _run(self, generators, state, skills=("api", "docs")): + command = PluginCommand() + bundle = [ + RepoSkill(name=name, path=Path("/upstream") / name) for name in skills + ] + with ( + patch( + "deepctl_core.skill_generator.get_skills_state", return_value=state + ), + patch("deepctl_core.skill_generator.save_skills_state"), + patch( + "deepctl_core.skill_generator.collect_command_metadata", + return_value=[], + ), + patch( + "deepctl_core.skill_generator.get_all_generators", + return_value=generators, + ), + patch( + "deepctl_core.skill_generator.fetch_repo_skills", return_value=bundle + ) as fetch, + ): + # The autouse fixture stubs this out for every other test here. + _REAL_MAYBE_UPDATE_SKILLS(command) + self.fetch = fetch + return state + + def test_it_keeps_the_upstream_ref_and_skill_names(self, tmp_path): + from deepctl_core.skill_bundle import DEFAULT_SKILLS_REF + + root = tmp_path / ".claude" / "skills" + gen = self._generator("claude", root, [root / "api", root / "docs"]) + state = { + "installed_skills": { + "claude": {"paths": [], "skills_ref": "old", "skills": []} + }, + "auto_update": True, + } + self._run([gen], state) + + entry = state["installed_skills"]["claude"] + assert entry["skills_ref"] == DEFAULT_SKILLS_REF + assert entry["skills"] == ["api", "docs"] + + def test_a_tool_with_no_skills_directory_is_left_alone(self): + """Nothing is installed for it, so nothing is fetched or rewritten.""" + gen = self._generator("amazonq", None, []) + state = { + "installed_skills": {"amazonq": {"paths": []}}, + "auto_update": True, + } + self._run([gen], state) + + gen.install_skills.assert_not_called() + self.fetch.assert_not_called() + assert state["installed_skills"]["amazonq"] == {"paths": []} + + def test_only_the_tool_with_no_skills_directory_is_left_alone(self, tmp_path): + """The three negatives above also hold if the refresh threw. + + `_maybe_update_skills` ends in a bare `except Exception: pass`, + so "nothing happened" is what a crash on line one looks like + too. A sibling that must be refreshed in the same run is the + positive signal that the code reached the per-tool loop. + """ + root = tmp_path / ".claude" / "skills" + claude = self._generator("claude", root, [root / "api"]) + amazonq = self._generator("amazonq", None, []) + state = { + "installed_skills": { + "claude": {"paths": [], "skills": []}, + "amazonq": {"paths": []}, + }, + "auto_update": True, + } + self._run([claude, amazonq], state, skills=("api",)) + + claude.install_skills.assert_called_once() + assert state["installed_skills"]["claude"]["skills"] == ["api"] + amazonq.install_skills.assert_not_called() + assert state["installed_skills"]["amazonq"] == {"paths": []} + + def test_a_second_tool_failing_leaves_the_first_recorded(self, tmp_path): + """The refresh used to save state only after the whole loop. + + A later failure reached the bare `except` with the earlier tool's + new folders already written and nothing recording them. + """ + claude_root = tmp_path / ".claude" / "skills" + cursor_root = tmp_path / ".cursor" / "skills" + first = self._generator("claude", claude_root, [claude_root / "api"]) + second = self._generator("cursor", cursor_root, []) + second.install_skills.side_effect = OSError(30, "Read-only file system") + state = { + "installed_skills": { + "claude": {"paths": [], "skills_ref": "old", "skills": []}, + "cursor": {"paths": [], "skills_ref": "old", "skills": []}, + }, + "auto_update": True, + } + + with patch("deepctl_cmd_plugin.command.console") as printer: + self._run([first, second], state, skills=("api",)) + + # And the user is told, rather than the refresh going quiet on it. + printed = " ".join(str(c) for c in printer.print.call_args_list) + assert "cursor skills not updated" in printed + assert "Read-only file system" in printed + + assert state["installed_skills"]["claude"]["skills"] == ["api"] + # Nothing landed for the tool that failed, so its record still + # describes the install that is actually on disk. + assert state["installed_skills"]["cursor"]["skills_ref"] == "old" + + def test_the_bundle_is_fetched_once_for_every_tool(self, tmp_path): + """Two fetches could install two different revisions side by side.""" + claude_root = tmp_path / ".claude" / "skills" + cursor_root = tmp_path / ".cursor" / "skills" + generators = [ + self._generator("claude", claude_root, [claude_root / "api"]), + self._generator("cursor", cursor_root, [cursor_root / "api"]), + ] + state = { + "installed_skills": {"claude": {"paths": []}, "cursor": {"paths": []}}, + "auto_update": True, + } + + self._run(generators, state, skills=("api",)) + + assert self.fetch.call_count == 1 diff --git a/packages/deepctl-cmd-skills/src/deepctl_cmd_skills/command.py b/packages/deepctl-cmd-skills/src/deepctl_cmd_skills/command.py index 9db92d9..691ad0c 100644 --- a/packages/deepctl-cmd-skills/src/deepctl_cmd_skills/command.py +++ b/packages/deepctl-cmd-skills/src/deepctl_cmd_skills/command.py @@ -3,8 +3,8 @@ from __future__ import annotations import importlib.metadata -from datetime import datetime, timezone -from typing import Any +from pathlib import Path +from typing import TYPE_CHECKING, Any import click from deepctl_core.auth import AuthManager @@ -12,29 +12,62 @@ from deepctl_core.client import DeepgramClient from deepctl_core.config import Config from deepctl_core.output import print_info, print_success, print_warning +from deepctl_core.skill_bundle import ( + DEFAULT_SKILLS_REF, + REF_ENV_VAR, + SkillFetchError, +) from rich.console import Console from rich.table import Table +if TYPE_CHECKING: + from deepctl_core.skill_bundle import RepoSkill + from deepctl_core.skill_generator import SkillInstallReport + console = Console() +#: Printed after a successful install: the one skill that needs a follow-up. +_MCP_HINT = ( + "One of the installed skills is 'setup-mcp' — ask your assistant to " + "set up the Deepgram MCP server, or run 'dg mcp' to start it directly." +) + class SkillsCommand(BaseGroupCommand): """AI coding assistant skill management.""" name = "skills" - help = "Manage AI coding assistant integrations for deepctl" + help = "Install Deepgram agent skills into AI coding assistants" examples = [ "dg skills status", "dg skills install", "dg skills install --all", + "dg skills install --all --ref main", "dg skills update", "dg skills remove --all", ] agent_help = ( - "Manage skill files that teach AI coding assistants (Claude Code, " - "Codex, Gemini CLI, etc.) how to use deepctl. Use 'skills status' to " - "detect which AI CLIs are installed, 'skills install' to generate " - "integration files, and 'skills update' to regenerate after plugin changes." + "Install the Deepgram agent skills from github.com/deepgram/skills " + "into AI coding assistants (Claude Code, Codex, Gemini CLI, Cursor, " + "OpenCode, Cline). Each skill is copied as a folder into the " + "user-scope skills directory that tool reads. Use 'skills status' to " + "see which tools are present and where their skills go, " + "'skills install' to install, 'skills list' to see what is installed " + "and from which upstream ref, and 'skills update' to reinstall. " + "Installs are pinned to a released deepgram/skills tag; override with " + "--ref or DEEPCTL_SKILLS_REF. A fetch failure exits non-zero with " + "nothing written rather than installing a subset. Those skills " + "directories are shared with the user's own skills and other " + "publishers', so every subcommand operates only on the folders " + "deepctl recorded installing: install refuses to overwrite an " + "unrecorded folder of the same name and remove never deletes one. " + "'skills setup' runs the same install, so the same rules apply to it. " + "A recorded path that is now a symlink is not deepctl's either — " + "install and update exit non-zero naming it, and remove reports it " + "instead of deleting through it. A recorded folder remove cannot " + "delete stays recorded and remove exits non-zero, so a later remove " + "or update can still reach it. 'dg login' and 'dg plugin' follow the " + "same ownership rules but warn and exit 0 instead of failing." ) def execute(self, ctx: click.Context, **kwargs: Any) -> None: @@ -110,7 +143,7 @@ def _create_install_command(self, context_wrapper: Any) -> click.Command: @click.command( name="install", - help="Detect AI CLIs and install skill files", + help="Detect AI CLIs and install the Deepgram skill folders", ) @click.option( "--all", @@ -123,6 +156,16 @@ def _create_install_command(self, context_wrapper: Any) -> click.Command: "cli_name", help="Install for a specific AI CLI only", ) + @click.option( + "--ref", + "ref", + metavar="REF", + help=( + "Install from this deepgram/skills git ref instead of the " + f"pinned release ({DEFAULT_SKILLS_REF}). Also settable with " + f"{REF_ENV_VAR}." + ), + ) def install_cmd(**kwargs: Any) -> None: pass @@ -136,13 +179,22 @@ def _create_update_command(self, context_wrapper: Any) -> click.Command: @click.command( name="update", - help="Regenerate all installed skill files from current metadata", + help="Reinstall every installed tool's skills from upstream", + ) + @click.option( + "--ref", + "ref", + metavar="REF", + help=( + "Install from this deepgram/skills git ref instead of the " + f"pinned release ({DEFAULT_SKILLS_REF})." + ), ) def update_cmd(**kwargs: Any) -> None: pass update_cmd.callback = context_wrapper( - lambda config, auth_manager, client, **kw: self._handle_update() + lambda config, auth_manager, client, **kw: self._handle_update(**kw) ) return update_cmd @@ -151,18 +203,18 @@ def _create_remove_command(self, context_wrapper: Any) -> click.Command: @click.command( name="remove", - help="Remove installed skill files", + help=("Remove only the skill folders deepctl installed"), ) @click.option( "--all", "remove_all", is_flag=True, - help="Remove all installed skill files", + help="Remove deepctl's skills from every tool it installed into", ) @click.option( "--cli", "cli_name", - help="Remove skill files for a specific AI CLI", + help="Remove deepctl's skills for a specific AI CLI", ) def remove_cmd(**kwargs: Any) -> None: pass @@ -177,7 +229,7 @@ def _create_list_command(self, context_wrapper: Any) -> click.Command: @click.command( name="list", - help="Show installed skills with paths and versions", + help="Show what is installed, from which upstream ref, and where", ) def list_cmd(**kwargs: Any) -> None: pass @@ -200,6 +252,15 @@ def _create_setup_command(self, context_wrapper: Any) -> click.Command: is_flag=True, help="Install for all detected tools without prompting", ) + @click.option( + "--ref", + "ref", + metavar="REF", + help=( + "Install from this deepgram/skills git ref instead of the " + f"pinned release ({DEFAULT_SKILLS_REF})." + ), + ) def setup_cmd(**kwargs: Any) -> None: pass @@ -208,34 +269,160 @@ def setup_cmd(**kwargs: Any) -> None: ) return setup_cmd + # ------------------------------------------------------------------ + # Helpers + # ------------------------------------------------------------------ + + @staticmethod + def _installed_records(state: dict[str, Any]) -> dict[str, Any] | None: + """``installed_skills`` as a map of tool to record, or None. + + A hand-edited ``skills.json`` can carry a list or a string here, + and every handler below iterates it. None means the file cannot + be read as records, so the caller says which file is wrong -- + ``main.py`` would otherwise turn the attribute error into + "Error: 'list' object has no attribute 'keys'", which names a + Python type rather than the file to fix. + """ + installed = state.get("installed_skills") + return installed if isinstance(installed, dict) else None + + @staticmethod + def _unreadable_records(consequence: str) -> str: + """Message for a ``skills.json`` whose records are not a map.""" + from deepctl_core.skill_generator import _STATE_FILE + + return ( + "deepctl cannot read its own records: 'installed_skills' in " + f"{_STATE_FILE} is not a set of entries. {consequence}" + ) + + def _fetch_skills(self, ref: str | None) -> list[RepoSkill]: + """Fetch the upstream skills, or fail the command outright. + + A partial install is worse than none: once the files are on disk + there is nothing to tell the user that four of fourteen skills + arrived. ClickException is what main.py turns into exit 1. + """ + from deepctl_core.skill_generator import fetch_repo_skills + + try: + return fetch_repo_skills(ref, force=True) + except SkillFetchError as exc: + raise click.ClickException( + f"{exc}\n\nNo skills were installed. Retry when the " + "network is available, or pass --ref to pick another " + "deepgram/skills revision." + ) + + def _install_for( + self, + generators: list[Any], + state: dict[str, Any], + ref: str | None, + ) -> SkillInstallReport: + """Install for every selected tool, then report what landed. + + The ownership contract lives in + :func:`deepctl_core.skill_generator.install_skills_for`, which + login and the plugin refresh call too. All this adds is the exit + code: a collision is a failed command, not a warning, so it + becomes the ClickException main.py turns into exit 1. + """ + from deepctl_core.skill_generator import ( + SkillOwnershipError, + collect_command_metadata, + install_skills_for, + ) + + def announce(gen: Any, paths: list[Path]) -> None: + # Printed as each tool lands, not once they all have: a later + # tool failing must not hide the ones that did install and + # are now recorded as deepctl's. + print_success( + f" {gen.display_name}: {len(paths)} skills -> {gen.skills_root()}" + ) + + try: + report = install_skills_for( + generators, + state, + commands=collect_command_metadata(), + version=_deepctl_version(), + ref=ref, + fetch=lambda: self._fetch_skills(ref), + on_installed=announce, + ) + except SkillOwnershipError as exc: + raise click.ClickException(str(exc)) + + for gen in report.unsupported: + print_warning(f" {gen.manual_hint()}") + return report + # ------------------------------------------------------------------ # Handlers # ------------------------------------------------------------------ def _handle_status(self) -> None: """Show detected AI CLIs and whether skills are installed.""" - from deepctl_core.skill_generator import get_all_generators, get_skills_state + from deepctl_core.skill_generator import ( + SKILLS_CLI_HINT, + get_all_generators, + get_skills_state, + recorded_skill_paths, + ) generators = get_all_generators() state = get_skills_state() - installed = state.get("installed_skills", {}) + installed = self._installed_records(state) table = Table(title="AI Coding Assistant Status") table.add_column("CLI", style="cyan", no_wrap=True) table.add_column("Detected", style="white") - table.add_column("Skills Installed", style="white") + table.add_column("Deepgram Skills", style="white") + table.add_column("Skills Directory", style="dim") for gen in generators: detected = gen.detect() - has_skills = gen.cli_name in installed + root = gen.skills_root() + # Deepgram's own skills only. These directories are shared, so + # counting every folder in them would report the user's skills + # and other publishers' skills as deepctl installs. + count = len( + gen.installed_skill_paths(recorded_skill_paths(state, gen.cli_name)) + ) + if root is None: + installed_cell = "[dim]n/a[/dim]" + root_cell = "[dim]no skills directory[/dim]" + else: + installed_cell = f"[green]{count}[/green]" if count else "[dim]No[/dim]" + root_cell = _tilde(root) table.add_row( gen.display_name, "[green]Yes[/green]" if detected else "[dim]No[/dim]", - "[green]Yes[/green]" if has_skills else "[dim]No[/dim]", + installed_cell, + root_cell, ) console.print(table) + if installed is None: + # The table above is still worth printing -- it is how a user + # finds which tools are present and where their skills go -- + # but every count in it read as zero, so say why. + print_warning( + self._unreadable_records( + "Nothing is counted as deepctl's until that file is " + "fixed or deleted." + ) + ) + + if any(g.detect() and g.skills_root() is None for g in generators): + print_info( + f"Tools with no skills directory: install with '{SKILLS_CLI_HINT}'." + ) + detected_count = sum(1 for g in generators if g.detect()) if detected_count > 0 and not installed: print_info( @@ -246,11 +433,10 @@ def _handle_install( self, install_all: bool = False, cli_name: str | None = None, + ref: str | None = None, ) -> None: - """Detect AI CLIs, prompt user, generate & install skill files.""" + """Detect AI CLIs, prompt the user, and install the Deepgram skills.""" from deepctl_core.skill_generator import ( - _commands_hash, - collect_command_metadata, detect_ai_clis, get_all_generators, get_skills_state, @@ -276,8 +462,8 @@ def _handle_install( # Abort rather than return: these subcommands are plain # click callbacks, so nothing maps a returned result to an # exit code and a bare return exits 0 -- indistinguishable - # from a successful install. main.py turns Abort into the - # documented exit 2 for user cancellation. + # from a successful install. main.py turns Abort into + # exit 2, which it reserves for user cancellation. raise click.Abort() else: generators = detect_ai_clis() @@ -289,114 +475,125 @@ def _handle_install( print_info(f" - {g.display_name}") return - # Collect metadata - commands = collect_command_metadata() - try: - version = importlib.metadata.version("deepctl") - except importlib.metadata.PackageNotFoundError: - version = "0.0.0" + selected = [ + g + for g in generators + if install_all + or cli_name + or self.confirm( + f"Install Deepgram skills for {g.display_name}?", + default=True, + ) + ] state = get_skills_state() - total_written: list[str] = [] - - for gen in generators: - if ( - not install_all - and not cli_name - and not self.confirm( - f"Install deepctl skills for {gen.display_name}?", - default=True, - ) - ): - continue - - paths = gen.install(commands, version) - cmd_hash = _commands_hash(commands) - state["installed_skills"][gen.cli_name] = { - "paths": [str(p) for p in paths], - "installed_at": datetime.now(timezone.utc).isoformat(), - "version": version, - "commands_hash": cmd_hash, - } - for p in paths: - total_written.append(str(p)) - print_success(f" Wrote {p}") - + report = self._install_for(selected, state, ref) save_skills_state(state) - if total_written: - print_success(f"\nInstalled skills: {len(total_written)} file(s)") - print_info("Run /deepgram:setup-mcp to configure the Deepgram MCP server.") - else: + if report.total_written: + print_success( + f"\nInstalled {report.total_written} skill folder(s) " + f"from deepgram/skills@{report.ref}" + ) + print_info(_MCP_HINT) + elif not report.unsupported: print_info("No skills were installed.") - def _handle_update(self) -> None: - """Regenerate all installed skill files from current metadata.""" + def _handle_update(self, ref: str | None = None) -> None: + """Reinstall every installed tool's skills from upstream.""" from deepctl_core.skill_generator import ( - _commands_hash, - collect_command_metadata, get_all_generators, get_skills_state, save_skills_state, ) state = get_skills_state() - installed = state.get("installed_skills", {}) + installed = self._installed_records(state) + + if installed is None: + print_info( + self._unreadable_records( + "There is no list of tools to update. Fix or delete " + "that file, then run 'dg skills install'." + ) + ) + return if not installed: print_info("No skills installed. Run 'deepctl skills install' first.") return - commands = collect_command_metadata() - try: - version = importlib.metadata.version("deepctl") - except importlib.metadata.PackageNotFoundError: - version = "0.0.0" - generators = {g.cli_name: g for g in get_all_generators()} - updated_count = 0 - + targets = [] for cli_key in list(installed.keys()): gen = generators.get(cli_key) if gen is None: print_warning(f"Unknown CLI '{cli_key}', skipping.") continue + # A tool with no skills directory is handed on rather than + # filtered out here. install_skills_for() is what cleans up + # its deepctl <= 0.3.0 files and drops the record that should + # never have existed; skipping it meant update printed the + # same "no skills directory" warning on every run forever, + # with no command on this path that would ever resolve it. + targets.append(gen) + + if not targets: + print_info("Nothing to update.") + return - paths = gen.install(commands, version) - cmd_hash = _commands_hash(commands) - state["installed_skills"][cli_key].update( - { - "paths": [str(p) for p in paths], - "version": version, - "commands_hash": cmd_hash, - } - ) - updated_count += 1 - for p in paths: - print_success(f" Updated {p}") - + report = self._install_for(targets, state, ref) save_skills_state(state) - print_success(f"Updated {updated_count} skill(s)") - if updated_count: - print_info("Run /deepgram:setup-mcp to configure the Deepgram MCP server.") + if report.written: + print_success( + f"Updated {len(report.written)} tool(s) from " + f"deepgram/skills@{report.ref}" + ) + print_info(_MCP_HINT) + elif not report.unsupported: + print_info("Nothing to update.") def _handle_remove( self, remove_all: bool = False, cli_name: str | None = None, ) -> None: - """Remove installed skill files.""" + """Remove the skill folders deepctl recorded installing. + + Only those. These are shared directories, so a folder deepctl has + no record of installing belongs to the user or another publisher + and is never deleted — including when ``skills.json`` is gone, in + which case there is nothing deepctl can prove it owns. + """ from deepctl_core.skill_generator import ( get_all_generators, get_skills_state, + recorded_skill_paths, save_skills_state, ) state = get_skills_state() - installed = state.get("installed_skills", {}) + installed = self._installed_records(state) + + # Reported separately from "nothing installed": the records were + # not deleted, the file is unreadable, and only one of those two + # is fixed by deleting folders. + if installed is None: + print_info( + self._unreadable_records( + "It will not guess which folders are its, so nothing " + "was removed. Fix or delete that file, then remove " + "the skill folders by hand." + ) + ) + return if not installed: - print_info("No skills are installed.") + print_info( + "No skills are installed according to deepctl's records. " + "deepctl only removes folders it recorded installing, so if " + "those records were deleted, remove the skill folders by hand." + ) return generators = {g.cli_name: g for g in get_all_generators()} @@ -411,26 +608,153 @@ def _handle_remove( elif remove_all: targets = list(installed.keys()) else: - print_info("Specify --all to remove all, or --cli NAME.") - return - - for cli_key in targets: - gen = generators.get(cli_key) - if gen: - removed = gen.remove() + # A usage error, which main.py turns into exit 1. Printing + # the hint and exiting 0 made "you forgot a flag" + # indistinguishable from "everything was removed". + raise click.UsageError("Specify --all to remove all, or --cli NAME.") + + total_removed = 0 + tools_cleaned = 0 + stranded_total = 0 + cleaned_in_place = 0 + # A filesystem call in here can raise -- clean_legacy unlinks + # and rewrites files without a guard. The records already + # updated describe deletions that have happened, so they are + # saved either way rather than thrown away with the traceback. + try: + for cli_key in targets: + gen = generators.get(cli_key) + if gen is None: + # No generator, so no skills root to check a path against + # and nothing deepctl can prove about these folders. The + # record is the only thing it can honestly drop. + print_warning( + f" Unknown CLI '{cli_key}': dropping its record. Delete " + "any folders it left behind by hand." + ) + del state["installed_skills"][cli_key] + continue + + recorded = recorded_skill_paths(state, cli_key) + owned = gen.owned_skill_paths(recorded) + removed = gen.remove(recorded) + # remove() also reports the deepctl <= 0.3.0 artifacts it + # cleaned up, and those do not always go away: a shared + # context file keeps the user's own text, and the legacy + # command directory keeps a command they added. A bare + # "Removed" would name a path they can still see. + deleted = [p for p in removed if not p.exists()] for p in removed: - print_info(f" Removed {p}") - del state["installed_skills"][cli_key] + if p.exists(): + print_info(f" Removed deepctl's content from {p}") + else: + print_info(f" Removed {p}") + total_removed += len(deleted) + # Counted apart from total_removed, which is a count of + # *folders that are gone*. A path deepctl only cut its own + # content out of is still there, so it must not inflate + # that number -- but it did happen, and the closing + # "Nothing was removed." would contradict the line naming + # it that was just printed. + cleaned_in_place += len(removed) - len(deleted) + if deleted: + tools_cleaned += 1 + + # A recorded path deepctl can no longer claim — the folder + # was replaced by a symlink, or the entry was hand-edited to + # point outside the skills root. Never deleted, so say where + # it is instead of dropping the record silently. + for path in self._unownable(recorded, owned): + print_warning( + f" {gen.display_name}: {path} is no longer deepctl's to " + "delete. Remove it by hand." + ) + + # Ownership outlives a failed deletion. rmtree can lose to a + # permission error or a read-only mount, and dropping the + # record then would strand Deepgram's own folders: the next + # update refuses to overwrite what it cannot prove is its, + # and the next remove has nothing left to act on. + stranded = [p for p in owned if p.exists()] + if stranded: + stranded_total += len(stranded) + entry = state["installed_skills"][cli_key] + entry["paths"] = [str(p) for p in stranded] + entry["skills"] = [p.name for p in stranded] + print_warning( + f" {gen.display_name}: {len(stranded)} folder(s) could not " + "be removed and are still recorded as deepctl's. Fix the " + f"permissions and run 'dg skills remove --cli {cli_key}' " + "again." + ) + else: + del state["installed_skills"][cli_key] + if not removed: + print_warning(f" {gen.display_name}: nothing left to remove.") + finally: + save_skills_state(state) + + if total_removed: + print_success( + f"Removed {total_removed} folder(s) from {tools_cleaned} tool(s)." + ) + elif not stranded_total and not cleaned_in_place: + print_info("Nothing was removed.") + + if stranded_total: + # The command did not do what was asked, and the README + # documents 1 for a failed command. Raised after the state + # is saved, so the retained ownership survives the failure. + raise click.ClickException( + f"{stranded_total} recorded skill folder(s) could not be " + "removed. They are still recorded as deepctl's, so fix the " + "permissions and run the remove again." + ) - save_skills_state(state) - print_success(f"Removed {len(targets)} skill(s).") + @staticmethod + def _unownable(recorded: list[str], owned: list[Path]) -> list[Path]: + """Recorded paths still on disk that deepctl may no longer touch. + + ``owned`` has already dropped them — a symlink standing where a + skill folder was, or an entry pointing outside the skills root. + Reported rather than deleted, because deleting either one is how + deepctl would destroy something that is not its. + + Both sides are compared after ``expanduser()``, the same form + :meth:`SkillGenerator.owned_skill_paths` keeps, so a record + written as ``~/.claude/skills/api`` is not mistaken for a path + deepctl may no longer touch. A relative entry is skipped for the + same reason that method drops it: it names nothing deepctl can + resolve, and resolving it against the working directory would + point this warning at an unrelated file. + """ + keep = {str(p) for p in owned} + out = [] + seen = set(keep) + for entry in recorded: + path = Path(entry).expanduser() + if not path.is_absolute() or str(path) in seen: + continue + seen.add(str(path)) + if path.is_symlink() or path.exists(): + out.append(path) + return out def _handle_list(self) -> None: - """Show installed skills with paths and versions.""" + """Show installed skills with locations, versions and upstream ref.""" from deepctl_core.skill_generator import get_skills_state state = get_skills_state() - installed = state.get("installed_skills", {}) + installed = self._installed_records(state) + + if installed is None: + print_info( + self._unreadable_records( + "There is nothing it can list. Fix or delete that " + "file, then run 'dg skills install'." + ) + ) + return if not installed: print_info( @@ -440,33 +764,36 @@ def _handle_list(self) -> None: table = Table(title="Installed Skills") table.add_column("CLI", style="cyan", no_wrap=True) - table.add_column("Version", style="green") - table.add_column("Paths", style="white") + table.add_column("deepctl", style="green") + table.add_column("Skills ref", style="green") + table.add_column("Skills", style="white") + table.add_column("Location", style="dim") for cli_key, info in installed.items(): - paths = "\n".join(info.get("paths", [])) - table.add_row(cli_key, info.get("version", "?"), paths) + names = info.get("skills") or [] + paths = info.get("paths") or [] + location = _tilde(Path(paths[0]).parent) if paths else "?" + table.add_row( + cli_key, + info.get("version", "?"), + info.get("skills_ref", "?"), + f"{len(names) or len(paths)}", + location, + ) console.print(table) + print_info("[dim]Run 'dg skills update' to reinstall from upstream.[/dim]") - auto = state.get("auto_update", True) - if auto: - print_info( - "[dim]Auto-update is enabled — skills regenerate on plugin changes.[/dim]" - ) - - def _handle_setup(self, install_all: bool = False) -> None: + def _handle_setup(self, install_all: bool = False, ref: str | None = None) -> None: """Interactive first-run setup: detect AI tools and install skills. - Downloads Deepgram skills from the deepgram/skills GitHub repo and - installs both the repo skills and the local deepctl command reference - for each selected AI coding tool. + Fetches the Deepgram skills from the deepgram/skills repo and + installs each one, as a folder, into the skills directory the + selected tool actually reads. """ import sys from deepctl_core.skill_generator import ( - _commands_hash, - collect_command_metadata, detect_ai_clis, get_all_generators, get_skills_state, @@ -523,35 +850,35 @@ def _handle_setup(self, install_all: bool = False) -> None: # Non-TTY without --all: install for all detected selected = list(detected) - # 3. Collect command metadata and install for selected tools + # 3. Install the upstream skills for each selected tool console.print("\n[blue]Installing Deepgram skills...[/blue]") - commands = collect_command_metadata() - try: - version = importlib.metadata.version("deepctl") - except importlib.metadata.PackageNotFoundError: - version = "0.0.0" state = get_skills_state() - total_written: list[str] = [] - - for gen in selected: - paths = gen.install(commands, version) - cmd_hash = _commands_hash(commands) - state["installed_skills"][gen.cli_name] = { - "paths": [str(p) for p in paths], - "installed_at": datetime.now(timezone.utc).isoformat(), - "version": version, - "commands_hash": cmd_hash, - } - for p in paths: - total_written.append(str(p)) - print_success(f" {gen.display_name} → {p}") - + report = self._install_for(selected, state, ref) save_skills_state(state) - if total_written: + if report.total_written: console.print() - print_success(f"Setup complete — {len(total_written)} file(s) installed") - print_info("Run /deepgram:setup-mcp to configure the Deepgram MCP server.") - else: + print_success( + f"Setup complete - {report.total_written} skill folder(s) from " + f"deepgram/skills@{report.ref}" + ) + print_info(_MCP_HINT) + elif not report.unsupported: print_info("No skills were installed.") + + +def _tilde(path: Path) -> str: + """Render a path under the user's home as ~/... so tables stay readable.""" + try: + return f"~/{path.relative_to(Path.home())}" + except ValueError: + return str(path) + + +def _deepctl_version() -> str: + """Return the installed deepctl version, or a placeholder.""" + try: + return importlib.metadata.version("deepctl") + except importlib.metadata.PackageNotFoundError: + return "0.0.0" diff --git a/packages/deepctl-cmd-skills/tests/unit/test_skills_command.py b/packages/deepctl-cmd-skills/tests/unit/test_skills_command.py index 8564186..0bf25e6 100644 --- a/packages/deepctl-cmd-skills/tests/unit/test_skills_command.py +++ b/packages/deepctl-cmd-skills/tests/unit/test_skills_command.py @@ -1,10 +1,12 @@ """Unit tests for skills command.""" +from pathlib import Path from unittest.mock import MagicMock, patch import click import pytest from deepctl_cmd_skills.command import SkillsCommand +from deepctl_core.skill_bundle import DEFAULT_SKILLS_REF, RepoSkill, SkillFetchError class TestSkillsCommand: @@ -37,6 +39,14 @@ def test_setup_commands_returns_subcommands(self): assert "list" in names assert "status" in names + def test_install_update_and_setup_accept_a_ref(self): + """Pinning has to be overridable without editing the source.""" + cmd = SkillsCommand() + by_name = {c.name: c for c in cmd.setup_commands()} + for name in ("install", "update", "setup"): + options = {p.name for p in by_name[name].params} + assert "ref" in options, name + def test_declining_install_anyway_aborts_instead_of_exiting_zero(self): """Declining the prompt must exit 2, not 0. @@ -62,16 +72,20 @@ def test_declining_install_anyway_aborts_instead_of_exiting_zero(self): with pytest.raises(click.Abort): cmd._handle_install(cli_name="claude") - generator.install.assert_not_called() + generator.install_skills.assert_not_called() - def test_accepting_install_anyway_does_not_abort(self): + def test_accepting_install_anyway_does_not_abort(self, tmp_path): """Positive control: confirming must proceed to the install.""" cmd = SkillsCommand() generator = MagicMock() generator.cli_name = "claude" generator.display_name = "Claude Code" generator.detect.return_value = False - generator.install.return_value = [] + root = tmp_path / ".claude" / "skills" + generator.skills_root.return_value = root + generator.install_conflicts.return_value = [] + generator.install_skills.return_value = [root / "api"] + generator.prune_retired.return_value = [] with ( patch( @@ -80,18 +94,22 @@ def test_accepting_install_anyway_does_not_abort(self): ), patch( "deepctl_core.skill_generator.collect_command_metadata", - return_value={}, + return_value=[], ), patch( "deepctl_core.skill_generator.get_skills_state", return_value={"installed_skills": {}}, ), patch("deepctl_core.skill_generator.save_skills_state"), + patch( + "deepctl_core.skill_generator.fetch_repo_skills", + return_value=[RepoSkill(name="api", path=tmp_path / "api")], + ), patch.object(cmd, "confirm", return_value=True), ): cmd._handle_install(cli_name="claude") - generator.install.assert_called_once() + generator.install_skills.assert_called_once() def test_unknown_cli_exits_one(self): """An unknown --cli must exit 1, not print an error and exit 0.""" @@ -124,6 +142,808 @@ def test_removing_skills_that_are_not_installed_exits_one(self): assert "No skills installed for 'cursor'" in str(exc.value) +class TestFetchFailuresAreFatal: + """A partial install is worse than a failed one, so it must exit non-zero.""" + + @pytest.mark.parametrize( + "message", + [ + "Could not download deepgram/skills@main: no network", + "deepgram/skills has no ref 'nope' (HTTP 404)", + "Skill manifest .claude-plugin/marketplace.json is not valid JSON", + ], + ) + def test_fetch_error_becomes_a_click_exception(self, message): + cmd = SkillsCommand() + with patch( + "deepctl_core.skill_generator.fetch_repo_skills", + side_effect=SkillFetchError(message), + ): + with pytest.raises(click.ClickException) as excinfo: + cmd._fetch_skills(None) + rendered = str(excinfo.value) + assert message in rendered + assert "No skills were installed" in rendered + + def test_install_does_not_swallow_the_failure(self, tmp_path): + """`skills install` used to print a notice and exit 0 with no files.""" + cmd = SkillsCommand() + generator = MagicMock() + generator.cli_name = "claude" + generator.display_name = "Claude Code" + generator.detect.return_value = True + generator.skills_root.return_value = tmp_path / ".claude" / "skills" + + with ( + patch( + "deepctl_core.skill_generator.detect_ai_clis", return_value=[generator] + ), + patch( + "deepctl_core.skill_generator.collect_command_metadata", return_value=[] + ), + patch( + "deepctl_core.skill_generator.get_skills_state", + return_value={"installed_skills": {}}, + ), + patch("deepctl_core.skill_generator.save_skills_state") as save, + patch( + "deepctl_core.skill_generator.fetch_repo_skills", + side_effect=SkillFetchError("no network"), + ), + ): + with pytest.raises(click.ClickException): + cmd._handle_install(install_all=True) + + generator.install_skills.assert_not_called() + save.assert_not_called() + + +class TestInstallRecordsWhatItDid: + def test_state_records_the_ref_and_every_skill(self, tmp_path): + cmd = SkillsCommand() + generator = MagicMock() + generator.cli_name = "claude" + generator.display_name = "Claude Code" + generator.detect.return_value = True + root = tmp_path / ".claude" / "skills" + generator.skills_root.return_value = root + generator.install_conflicts.return_value = [] + generator.install_skills.return_value = [root / "api", root / "docs"] + + skills = [ + RepoSkill(name="api", path=tmp_path / "api"), + RepoSkill(name="docs", path=tmp_path / "docs"), + ] + state = {"installed_skills": {}} + + with ( + patch( + "deepctl_core.skill_generator.detect_ai_clis", return_value=[generator] + ), + patch( + "deepctl_core.skill_generator.collect_command_metadata", return_value=[] + ), + patch("deepctl_core.skill_generator.get_skills_state", return_value=state), + patch("deepctl_core.skill_generator.save_skills_state"), + patch( + "deepctl_core.skill_generator.fetch_repo_skills", return_value=skills + ), + ): + cmd._handle_install(install_all=True) + + entry = state["installed_skills"]["claude"] + assert entry["skills"] == ["api", "docs"] + assert entry["skills_ref"] == DEFAULT_SKILLS_REF + assert [Path(p).name for p in entry["paths"]] == ["api", "docs"] + + def test_a_tool_without_a_skills_directory_gets_the_one_liner(self, capsys): + cmd = SkillsCommand() + generator = MagicMock() + generator.cli_name = "amazonq" + generator.display_name = "Amazon Q Developer" + generator.detect.return_value = True + generator.skills_root.return_value = None + generator.manual_hint.return_value = ( + "Amazon Q Developer has no documented skills directory. " + "For the Deepgram skills, run: npx skills add deepgram/skills" + ) + state = {"installed_skills": {}} + + with ( + patch( + "deepctl_core.skill_generator.detect_ai_clis", return_value=[generator] + ), + patch( + "deepctl_core.skill_generator.collect_command_metadata", return_value=[] + ), + patch("deepctl_core.skill_generator.get_skills_state", return_value=state), + patch("deepctl_core.skill_generator.save_skills_state"), + patch("deepctl_core.skill_generator.fetch_repo_skills") as fetch, + ): + cmd._handle_install(install_all=True) + + # Nothing to install means nothing to download. + fetch.assert_not_called() + # Advisory output belongs on stderr, so stdout stays parseable. + captured = capsys.readouterr() + assert "npx skills add deepgram/skills" in " ".join(captured.err.split()) + assert captured.out == "" + assert state["installed_skills"] == {} + + +class TestTheCommandTouchesOnlyWhatItInstalled: + """`dg skills` shares these directories with the user and other publishers.""" + + def _generator(self, tmp_path, cli_name="claude"): + generator = MagicMock() + generator.cli_name = cli_name + generator.display_name = "Claude Code" + generator.detect.return_value = True + root = tmp_path / ".claude" / "skills" + generator.skills_root.return_value = root + generator.install_conflicts.return_value = [] + generator.install_skills.return_value = [root / "api"] + generator.remove.return_value = [] + generator.prune_retired.return_value = [] + generator.installed_skill_paths.return_value = [] + return generator, root + + def test_install_refuses_an_unowned_collision_and_writes_nothing(self, tmp_path): + cmd = SkillsCommand() + generator, root = self._generator(tmp_path) + generator.install_conflicts.return_value = [root / "api"] + skills = [RepoSkill(name="api", path=tmp_path / "api")] + state = {"installed_skills": {}} + + with ( + patch( + "deepctl_core.skill_generator.detect_ai_clis", return_value=[generator] + ), + patch( + "deepctl_core.skill_generator.collect_command_metadata", return_value=[] + ), + patch("deepctl_core.skill_generator.get_skills_state", return_value=state), + patch("deepctl_core.skill_generator.save_skills_state") as save, + patch( + "deepctl_core.skill_generator.fetch_repo_skills", return_value=skills + ), + ): + with pytest.raises(click.ClickException) as excinfo: + cmd._handle_install(install_all=True) + + rendered = str(excinfo.value) + assert "Refusing to overwrite" in rendered + assert str(root / "api") in rendered + assert "Claude Code" in rendered + generator.install_skills.assert_not_called() + save.assert_not_called() + assert state["installed_skills"] == {} + + def test_install_hands_the_recorded_paths_to_the_generator(self, tmp_path): + cmd = SkillsCommand() + generator, root = self._generator(tmp_path) + recorded = [str(root / "api")] + state = {"installed_skills": {"claude": {"paths": list(recorded)}}} + skills = [RepoSkill(name="api", path=tmp_path / "api")] + + with ( + patch( + "deepctl_core.skill_generator.detect_ai_clis", return_value=[generator] + ), + patch( + "deepctl_core.skill_generator.collect_command_metadata", return_value=[] + ), + patch("deepctl_core.skill_generator.get_skills_state", return_value=state), + patch("deepctl_core.skill_generator.save_skills_state"), + patch( + "deepctl_core.skill_generator.fetch_repo_skills", return_value=skills + ), + ): + cmd._handle_install(install_all=True) + + generator.install_conflicts.assert_called_once_with(skills, recorded) + generator.install_skills.assert_called_once_with(skills, recorded) + + def test_remove_hands_the_recorded_paths_to_the_generator(self, tmp_path): + cmd = SkillsCommand() + generator, root = self._generator(tmp_path) + recorded = [str(root / "api"), str(root / "docs")] + # Everything it owned is gone, so the record has nothing left to + # describe. Only then may the tool's entry go. + generator.owned_skill_paths.return_value = [root / "api", root / "docs"] + state = {"installed_skills": {"claude": {"paths": list(recorded)}}} + + with ( + patch( + "deepctl_core.skill_generator.get_all_generators", + return_value=[generator], + ), + patch("deepctl_core.skill_generator.get_skills_state", return_value=state), + patch("deepctl_core.skill_generator.save_skills_state"), + ): + cmd._handle_remove(remove_all=True) + + generator.remove.assert_called_once_with(recorded) + assert state["installed_skills"] == {} + + def test_remove_with_no_record_deletes_nothing(self, capsys): + """Deleting skills.json leaves deepctl nothing it can prove it owns.""" + cmd = SkillsCommand() + generator = MagicMock() + generator.cli_name = "claude" + + with ( + patch( + "deepctl_core.skill_generator.get_all_generators", + return_value=[generator], + ), + patch( + "deepctl_core.skill_generator.get_skills_state", + return_value={"installed_skills": {}}, + ), + patch("deepctl_core.skill_generator.save_skills_state") as save, + ): + cmd._handle_remove(remove_all=True) + + generator.remove.assert_not_called() + save.assert_not_called() + captured = capsys.readouterr() + assert "by hand" in " ".join((captured.out + captured.err).split()) + + def test_a_failed_deletion_keeps_the_record_and_can_be_retried( + self, tmp_path, capsys + ): + """Ownership has to outlive an rmtree that could not delete. + + `SkillGenerator.remove()` asks the filesystem before reporting a + folder gone, but the command used to drop the tool's whole + skills.json entry regardless. One permission error or read-only + mount then stranded Deepgram's own folders: the next update + refuses to overwrite what deepctl cannot prove is its, and the + next remove has no record left to act on. + """ + from deepctl_core.skill_generator import ClaudeCodeGenerator + + cmd = SkillsCommand() + gen = ClaudeCodeGenerator() + root = tmp_path / ".claude" / "skills" + (root / "api").mkdir(parents=True) + (root / "api" / "SKILL.md").write_text("---\nname: api\n---\n") + state = { + "installed_skills": { + "claude": {"paths": [str(root / "api")], "skills": ["api"]} + } + } + + def denied(path, ignore_errors=False, **kwargs): + """What rmtree(ignore_errors=True) does on a read-only mount.""" + if not ignore_errors: + raise PermissionError(13, "Permission denied", str(path)) + + with ( + patch.object(gen, "skills_root", return_value=root), + patch.object(gen, "legacy_paths", return_value=[]), + patch( + "deepctl_core.skill_generator.get_all_generators", return_value=[gen] + ), + patch("deepctl_core.skill_generator.get_skills_state", return_value=state), + patch("deepctl_core.skill_generator.save_skills_state"), + patch("deepctl_core.skill_generator.shutil.rmtree", denied), + ): + # A remove that could not remove is a failed command: the + # README documents exit 1 for that, and exiting 0 is how the + # user would never learn the folders are still there. + with pytest.raises(click.ClickException) as excinfo: + cmd._handle_remove(remove_all=True) + + assert "could not be removed" in str(excinfo.value) + assert (root / "api" / "SKILL.md").is_file() + assert state["installed_skills"]["claude"]["paths"] == [str(root / "api")] + assert "could not" in " ".join(capsys.readouterr().err.split()) + + # Retried once the permission is fixed, and now the record goes. + with ( + patch.object(gen, "skills_root", return_value=root), + patch.object(gen, "legacy_paths", return_value=[]), + patch( + "deepctl_core.skill_generator.get_all_generators", return_value=[gen] + ), + patch("deepctl_core.skill_generator.get_skills_state", return_value=state), + patch("deepctl_core.skill_generator.save_skills_state"), + ): + cmd._handle_remove(cli_name="claude") + + assert not (root / "api").exists() + assert state["installed_skills"] == {} + + def test_remove_drops_the_record_for_a_cli_it_no_longer_supports( + self, tmp_path, capsys + ): + """No generator means no root to check a path against. + + deepctl cannot prove anything about those folders, so the record + is the only thing it can honestly drop -- and it has to say so + rather than let the user think something was deleted. + """ + cmd = SkillsCommand() + state = {"installed_skills": {"retired-tool": {"paths": ["/somewhere/api"]}}} + + with ( + patch("deepctl_core.skill_generator.get_all_generators", return_value=[]), + patch("deepctl_core.skill_generator.get_skills_state", return_value=state), + patch("deepctl_core.skill_generator.save_skills_state") as save, + ): + cmd._handle_remove(remove_all=True) + + assert state["installed_skills"] == {} + save.assert_called_once() + rendered = " ".join(capsys.readouterr().err.split()) + assert "retired-tool" in rendered + assert "by hand" in rendered + + def test_remove_reports_a_recorded_path_it_may_no_longer_touch( + self, tmp_path, capsys + ): + """A symlink where a skill folder was is nobody's to delete. + + Dropping the record without a word would leave it in a shared + directory with deepctl no longer able to name it, and following + it would delete whatever it points at. + """ + from deepctl_core.skill_generator import ClaudeCodeGenerator + + cmd = SkillsCommand() + gen = ClaudeCodeGenerator() + root = tmp_path / ".claude" / "skills" + root.mkdir(parents=True) + target = tmp_path / "my-own-work" + target.mkdir() + (target / "SKILL.md").write_text("---\nname: api\n---\n\nmine\n") + link = root / "api" + try: + link.symlink_to(target, target_is_directory=True) + except (OSError, NotImplementedError): # unprivileged Windows + pytest.skip("this filesystem does not allow creating symlinks") + state = {"installed_skills": {"claude": {"paths": [str(link)]}}} + + with ( + patch.object(gen, "skills_root", return_value=root), + patch.object(gen, "legacy_paths", return_value=[]), + patch( + "deepctl_core.skill_generator.get_all_generators", return_value=[gen] + ), + patch("deepctl_core.skill_generator.get_skills_state", return_value=state), + patch("deepctl_core.skill_generator.save_skills_state"), + ): + cmd._handle_remove(remove_all=True) + + assert link.is_symlink() + assert (target / "SKILL.md").read_text().endswith("mine\n") + # Rich wraps the path across lines, so compare without whitespace. + rendered = "".join(capsys.readouterr().err.split()) + assert str(link) in rendered + assert "byhand" in rendered + # The record goes, because the path is not deepctl's any more and + # nothing it can do would ever clear it. README documents this. + assert state["installed_skills"] == {} + + def test_a_tilde_record_is_not_reported_twice_when_it_cannot_be_removed( + self, tmp_path, capsys, monkeypatch + ): + """`~/x` and its expansion are one path, so they get one verdict. + + A stranded folder is already reported as "could not be removed". + Matching the raw record against the expanded owned path missed + it, so the same folder was also reported as one deepctl may no + longer touch — two contradictory instructions for one path. + """ + from deepctl_core.skill_generator import ClaudeCodeGenerator + + cmd = SkillsCommand() + gen = ClaudeCodeGenerator() + home = tmp_path / "home" + root = home / ".claude" / "skills" + (root / "api").mkdir(parents=True) + (root / "api" / "SKILL.md").write_text("---\nname: api\n---\n") + state = {"installed_skills": {"claude": {"paths": ["~/.claude/skills/api"]}}} + + def denied(path, ignore_errors=False, **kwargs): + if not ignore_errors: + raise PermissionError(13, "Permission denied", str(path)) + + # expanduser() reads $HOME, not Path.home, so both are pointed at + # the throwaway directory -- otherwise this resolves against the + # developer's own home. + monkeypatch.setenv("HOME", str(home)) + monkeypatch.setenv("USERPROFILE", str(home)) + with ( + patch.object(Path, "home", staticmethod(lambda: home)), + patch.object(gen, "skills_root", return_value=root), + patch.object(gen, "legacy_paths", return_value=[]), + patch( + "deepctl_core.skill_generator.get_all_generators", return_value=[gen] + ), + patch("deepctl_core.skill_generator.get_skills_state", return_value=state), + patch("deepctl_core.skill_generator.save_skills_state"), + patch("deepctl_core.skill_generator.shutil.rmtree", denied), + ): + with pytest.raises(click.ClickException): + cmd._handle_remove(remove_all=True) + + rendered = " ".join(capsys.readouterr().err.split()) + assert "could not be removed" in rendered + assert "no longer deepctl's" not in rendered + # Still recorded, so the retry the message asks for can find it. + assert state["installed_skills"]["claude"]["paths"] == [ + str(root / "api") + ] + + def test_remove_without_all_or_cli_is_a_usage_error(self): + """Forgetting a flag must not look like a successful removal.""" + cmd = SkillsCommand() + with ( + patch( + "deepctl_core.skill_generator.get_skills_state", + return_value={"installed_skills": {"claude": {"paths": []}}}, + ), + patch("deepctl_core.skill_generator.get_all_generators", return_value=[]), + patch("deepctl_core.skill_generator.save_skills_state") as save, + ): + with pytest.raises(click.UsageError) as excinfo: + cmd._handle_remove() + + assert "--all" in str(excinfo.value) + save.assert_not_called() + + def test_remove_survives_a_hand_edited_state_file(self, capsys): + """A list where a dict belongs must not raise AttributeError.""" + cmd = SkillsCommand() + with ( + patch( + "deepctl_core.skill_generator.get_skills_state", + return_value={"installed_skills": ["claude"]}, + ), + patch("deepctl_core.skill_generator.get_all_generators", return_value=[]), + patch("deepctl_core.skill_generator.save_skills_state") as save, + ): + cmd._handle_remove(remove_all=True) + + save.assert_not_called() + assert "by hand" in " ".join(capsys.readouterr().err.split()) + + def test_remove_does_not_claim_it_deleted_a_path_that_survived( + self, tmp_path, capsys + ): + """clean_legacy leaves the user's half of a shared path behind. + + A context file keeps their own text, and the legacy command + directory keeps a command they added. Both are still on disk + afterwards, so a bare "Removed" points at something they can + still see -- and counted towards "Removed N folder(s)". + """ + cmd = SkillsCommand() + generator, root = self._generator(tmp_path) + shared = tmp_path / "GEMINI.md" + shared.write_text("my own notes\n") + generator.remove.return_value = [shared] + generator.owned_skill_paths.return_value = [] + state = {"installed_skills": {"claude": {"paths": []}}} + + with ( + patch( + "deepctl_core.skill_generator.get_all_generators", + return_value=[generator], + ), + patch("deepctl_core.skill_generator.get_skills_state", return_value=state), + patch("deepctl_core.skill_generator.save_skills_state"), + ): + cmd._handle_remove(remove_all=True) + + combined = " ".join( + (lambda c: c.out + c.err)(capsys.readouterr()).split() + ) + assert "Removed deepctl's content from" in combined + assert "Removed 1 folder(s)" not in combined + # ...and does not then deny it. The path is excluded from the + # folder count because the folder is still there, which is not + # the same as deepctl having done nothing to it. + assert "Nothing was removed" not in combined + assert shared.read_text() == "my own notes\n" + + def test_update_reports_the_tools_it_refreshed(self, tmp_path, capsys): + cmd = SkillsCommand() + generator, root = self._generator(tmp_path) + skills = [RepoSkill(name="api", path=tmp_path / "api")] + state = {"installed_skills": {"claude": {"paths": [str(root / "api")]}}} + + with ( + patch( + "deepctl_core.skill_generator.get_all_generators", + return_value=[generator], + ), + patch( + "deepctl_core.skill_generator.collect_command_metadata", return_value=[] + ), + patch("deepctl_core.skill_generator.get_skills_state", return_value=state), + patch("deepctl_core.skill_generator.save_skills_state"), + patch( + "deepctl_core.skill_generator.fetch_repo_skills", return_value=skills + ), + ): + cmd._handle_update() + + assert state["installed_skills"]["claude"]["skills"] == ["api"] + rendered = " ".join(capsys.readouterr().err.split()) + assert "Updated 1 tool(s)" in rendered + + def test_update_skips_a_cli_it_no_longer_supports(self, capsys): + """An unknown key must not take the whole update down.""" + cmd = SkillsCommand() + state = {"installed_skills": {"retired-tool": {"paths": []}}} + + with ( + patch("deepctl_core.skill_generator.get_all_generators", return_value=[]), + patch("deepctl_core.skill_generator.get_skills_state", return_value=state), + patch("deepctl_core.skill_generator.save_skills_state") as save, + patch("deepctl_core.skill_generator.fetch_repo_skills") as fetch, + ): + cmd._handle_update() + + fetch.assert_not_called() + save.assert_not_called() + rendered = " ".join(capsys.readouterr().err.split()) + assert "retired-tool" in rendered + assert "Nothing to update" in rendered + + def test_update_retires_a_tool_it_cannot_install_to(self, capsys): + """The warning has to stop, and only dropping the record stops it. + + Filtering these out before the shared installer meant `update` + never reached the code that cleans up their deepctl <= 0.3.0 + files and drops the record, so it reprinted the same hint on + every run with nothing on that path that would ever resolve it. + """ + cmd = SkillsCommand() + generator = MagicMock() + generator.cli_name = "amazonq" + generator.display_name = "Amazon Q Developer" + generator.skills_root.return_value = None + generator.manual_hint.return_value = "Amazon Q Developer has no ..." + state = {"installed_skills": {"amazonq": {"paths": []}}} + + with ( + patch( + "deepctl_core.skill_generator.get_all_generators", + return_value=[generator], + ), + patch( + "deepctl_core.skill_generator.collect_command_metadata", return_value=[] + ), + patch("deepctl_core.skill_generator.get_skills_state", return_value=state), + patch("deepctl_core.skill_generator.save_skills_state"), + patch("deepctl_core.skill_generator.fetch_repo_skills") as fetch, + ): + cmd._handle_update() + + generator.clean_legacy.assert_called_once() + assert state["installed_skills"] == {} + # Nothing to install into means nothing to download. + fetch.assert_not_called() + rendered = " ".join(capsys.readouterr().err.split()) + assert "Amazon Q Developer has no" in rendered + # ...and it does not then claim a tool was refreshed. + assert "Updated" not in rendered + + def test_list_shows_the_ref_and_count_it_recorded(self, tmp_path, capsys): + """`dg skills list` is how a user checks which revision they have.""" + cmd = SkillsCommand() + root = tmp_path / ".claude" / "skills" + state = { + "installed_skills": { + "claude": { + "paths": [str(root / "api"), str(root / "docs")], + "version": "0.4.0", + "skills_ref": "deepgram-skills-v1.6.0", + "skills": ["api", "docs"], + } + } + } + + with patch( + "deepctl_core.skill_generator.get_skills_state", return_value=state + ): + cmd._handle_list() + + rendered = "".join(capsys.readouterr().out.split()) + assert "deepgram-skills-v1.6.0" in rendered + assert "0.4.0" in rendered + assert "claude" in rendered + + def test_list_says_so_when_nothing_is_installed(self, capsys): + cmd = SkillsCommand() + with patch( + "deepctl_core.skill_generator.get_skills_state", + return_value={"installed_skills": {}}, + ): + cmd._handle_list() + + combined = capsys.readouterr() + assert "No skills installed" in " ".join( + (combined.out + combined.err).split() + ) + + def test_setup_without_a_tty_installs_for_everything_detected(self, tmp_path): + """CI has no prompt to answer, so setup must not wait for one.""" + cmd = SkillsCommand() + cmd._guided = False + generator, root = self._generator(tmp_path) + skills = [RepoSkill(name="api", path=tmp_path / "api")] + state = {"installed_skills": {}} + + with ( + patch("sys.stdout") as stdout, + patch( + "deepctl_core.skill_generator.detect_ai_clis", return_value=[generator] + ), + patch( + "deepctl_core.skill_generator.get_all_generators", + return_value=[generator], + ), + patch( + "deepctl_core.skill_generator.collect_command_metadata", return_value=[] + ), + patch("deepctl_core.skill_generator.get_skills_state", return_value=state), + patch("deepctl_core.skill_generator.save_skills_state"), + patch( + "deepctl_core.skill_generator.fetch_repo_skills", return_value=skills + ), + ): + stdout.isatty.return_value = False + cmd._handle_setup() + + generator.install_skills.assert_called_once() + assert state["installed_skills"]["claude"]["skills"] == ["api"] + + def test_status_asks_the_generator_only_for_recorded_folders( + self, tmp_path, capsys + ): + cmd = SkillsCommand() + generator, root = self._generator(tmp_path) + recorded = [str(root / "api")] + state = {"installed_skills": {"claude": {"paths": list(recorded)}}} + generator.installed_skill_paths.return_value = [root / "api"] + + with ( + patch( + "deepctl_core.skill_generator.get_all_generators", + return_value=[generator], + ), + patch("deepctl_core.skill_generator.get_skills_state", return_value=state), + ): + cmd._handle_status() + + generator.installed_skill_paths.assert_called_once_with(recorded) + # And the table shows that count, not a directory listing. Read + # out of the row rather than searched for in the whole screen: + # the skills directory is a tmp_path, and a "1" anywhere in it + # would pass for a skill count even when the cell reads "No". + row = next( + line + for line in capsys.readouterr().out.splitlines() + if "Claude Code" in line + ) + assert [c.strip() for c in row.split("│")][3] == "1" + + +class TestARecordsFileThatIsNotRecords: + """`installed_skills` holding something other than a map of tools. + + The README tells users to drop an entry from skills.json by hand, so + the file does get edited. Every handler here iterates that value, and + main.py turns the resulting attribute error into "Error: 'list' + object has no attribute 'keys'" -- a message that names a Python type + instead of the file to fix. Each one has to name the file instead. + """ + + BROKEN = [["claude"], "claude", 7] + + def _generator(self): + generator = MagicMock() + generator.cli_name = "claude" + generator.display_name = "Claude Code" + generator.detect.return_value = True + generator.skills_root.return_value = Path("/nowhere/.claude/skills") + generator.installed_skill_paths.return_value = [] + return generator + + @staticmethod + def _said(capsys): + captured = capsys.readouterr() + return " ".join((captured.out + captured.err).split()) + + @pytest.mark.parametrize("broken", BROKEN) + def test_status_still_prints_the_table_and_names_the_file( + self, broken, capsys + ): + """Status is how a user finds their tools, so it must still run.""" + cmd = SkillsCommand() + generator = self._generator() + + with ( + patch( + "deepctl_core.skill_generator.get_all_generators", + return_value=[generator], + ), + patch( + "deepctl_core.skill_generator.get_skills_state", + return_value={"installed_skills": broken}, + ), + ): + cmd._handle_status() + + said = self._said(capsys) + assert "Claude Code" in said + assert "cannot read its own records" in said + + @pytest.mark.parametrize("broken", BROKEN) + def test_list_names_the_file(self, broken, capsys): + cmd = SkillsCommand() + with patch( + "deepctl_core.skill_generator.get_skills_state", + return_value={"installed_skills": broken}, + ): + cmd._handle_list() + + assert "cannot read its own records" in self._said(capsys) + + @pytest.mark.parametrize("broken", BROKEN) + def test_update_names_the_file_and_downloads_nothing(self, broken, capsys): + cmd = SkillsCommand() + generator = self._generator() + + with ( + patch( + "deepctl_core.skill_generator.get_all_generators", + return_value=[generator], + ), + patch( + "deepctl_core.skill_generator.get_skills_state", + return_value={"installed_skills": broken}, + ), + patch( + "deepctl_core.skill_generator.fetch_repo_skills" + ) as fetch, + patch("deepctl_core.skill_generator.save_skills_state") as save, + ): + cmd._handle_update() + + assert "cannot read its own records" in self._said(capsys) + fetch.assert_not_called() + save.assert_not_called() + + @pytest.mark.parametrize("broken", BROKEN) + def test_remove_names_the_file_and_deletes_nothing(self, broken, capsys): + cmd = SkillsCommand() + generator = self._generator() + + with ( + patch( + "deepctl_core.skill_generator.get_all_generators", + return_value=[generator], + ), + patch( + "deepctl_core.skill_generator.get_skills_state", + return_value={"installed_skills": broken}, + ), + patch("deepctl_core.skill_generator.save_skills_state") as save, + ): + cmd._handle_remove(remove_all=True) + + assert "cannot read its own records" in self._said(capsys) + generator.remove.assert_not_called() + save.assert_not_called() + + class TestSkillsStartupCheck: """Test startup check module.""" diff --git a/packages/deepctl-core/src/deepctl_core/skill_bundle.py b/packages/deepctl-core/src/deepctl_core/skill_bundle.py new file mode 100644 index 0000000..bb2a5de --- /dev/null +++ b/packages/deepctl-core/src/deepctl_core/skill_bundle.py @@ -0,0 +1,355 @@ +"""Fetch the deepgram/skills bundle and expose it as installable skill folders. + +A Deepgram agent skill is a *folder* — ``SKILL.md`` plus whatever supporting +files it ships, notably a ``references/`` subdirectory. The authoritative list +of skills lives in the upstream repo's ``.claude-plugin/marketplace.json``, +which that repo's CI validates against the directories on disk in both +directions on every pull request. Reading that manifest is therefore the only +way to stay in step with upstream without hardcoding a list that goes stale. + +The whole repository is fetched as a single tarball rather than file-by-file: +one request gets the manifest, every ``SKILL.md`` and every ``references/`` +file at a consistent revision, and it cannot half-succeed the way a loop of +per-file requests can. +""" + +from __future__ import annotations + +import json +import os +import shutil +import tarfile +import tempfile +import urllib.error +import urllib.request +from dataclasses import dataclass +from pathlib import Path +from typing import IO, TYPE_CHECKING, Any + +if TYPE_CHECKING: + from collections.abc import Iterator + +__all__ = [ + "DEFAULT_SKILLS_REF", + "RepoSkill", + "SkillFetchError", + "bundle_url", + "fetch_skill_bundle", + "resolve_skills_ref", +] + +# Pinned to a released tag, not a branch: an install of a given deepctl +# version should produce the same skills today and in six months. Override +# with `--ref` or DEEPCTL_SKILLS_REF to track `main` or test a branch. +DEFAULT_SKILLS_REF = "deepgram-skills-v1.6.0" + +SKILLS_REPO = "deepgram/skills" +REF_ENV_VAR = "DEEPCTL_SKILLS_REF" + +_MANIFEST_PATH = ".claude-plugin/marketplace.json" +_PLUGIN_NAME = "deepgram" +_SKILL_ENTRY_FILE = "SKILL.md" +_DOWNLOAD_TIMEOUT = 30 +_MAX_BUNDLE_BYTES = 64 * 1024 * 1024 + + +class SkillFetchError(RuntimeError): + """Raised when the upstream skill bundle cannot be fetched or trusted. + + Always fatal: installing a subset of the skills, or a stale cached + subset, is worse than not installing at all because the user has no way + to tell the difference from a complete install. + """ + + +@dataclass(frozen=True) +class RepoSkill: + """One skill from the upstream repo, as a directory on disk.""" + + name: str + path: Path + + @property + def entry_file(self) -> Path: + """Path to this skill's ``SKILL.md``.""" + return self.path / _SKILL_ENTRY_FILE + + +def resolve_skills_ref(ref: str | None = None) -> str: + """Resolve which upstream ref to install from. + + Precedence: explicit argument, then ``DEEPCTL_SKILLS_REF``, then the + pinned default tag. + """ + if ref: + return ref + from_env = os.environ.get(REF_ENV_VAR, "").strip() + return from_env or DEFAULT_SKILLS_REF + + +def bundle_url(ref: str) -> str: + """Return the codeload tarball URL for ``ref`` (tag, branch or SHA).""" + return f"https://codeload.github.com/{SKILLS_REPO}/tar.gz/{ref}" + + +def fetch_skill_bundle( + ref: str | None = None, + *, + cache_dir: Path | None = None, + force: bool = False, +) -> list[RepoSkill]: + """Download the upstream skill bundle and return its skills. + + Args: + ref: Upstream git ref. Defaults to :func:`resolve_skills_ref`. + cache_dir: Where extracted bundles are kept. Defaults to + ``~/.deepctl/skills/repo_cache``. + force: Re-download even if this ref is already cached. + + Returns: + One :class:`RepoSkill` per entry in the upstream manifest, in + manifest order. + + Raises: + SkillFetchError: The bundle could not be downloaded, unpacked, or + reconciled with its manifest. + """ + resolved = resolve_skills_ref(ref) + root = cache_dir or (Path.home() / ".deepctl" / "skills" / "repo_cache") + target = root / _cache_key(resolved) + + if force or not (target / _MANIFEST_PATH).is_file(): + _download_and_extract(resolved, target) + + return read_manifest_skills(target) + + +def read_manifest_skills(root: Path) -> list[RepoSkill]: + """Read ``.claude-plugin/marketplace.json`` under ``root``. + + Raises: + SkillFetchError: The manifest is missing, malformed, lists no + skills, or names a directory that is not a skill folder. + """ + manifest_path = root / _MANIFEST_PATH + try: + raw = json.loads(manifest_path.read_text(encoding="utf-8")) + except FileNotFoundError: + raise SkillFetchError( + f"Skill manifest {_MANIFEST_PATH} is missing from the {SKILLS_REPO} bundle." + ) + except (OSError, UnicodeDecodeError) as exc: + raise SkillFetchError(f"Could not read {manifest_path}: {exc}") + except json.JSONDecodeError as exc: + raise SkillFetchError( + f"Skill manifest {_MANIFEST_PATH} is not valid JSON: {exc}" + ) + + entries = _manifest_skill_entries(raw) + + skills: list[RepoSkill] = [] + seen: set[str] = set() + for entry in entries: + name = _skill_name(entry) + if name in seen: + raise SkillFetchError(f"Skill manifest lists {name!r} more than once.") + seen.add(name) + path = _resolve_skill_dir(root, entry) + skills.append(RepoSkill(name=name, path=path)) + + return skills + + +# --------------------------------------------------------------------------- +# Manifest parsing +# --------------------------------------------------------------------------- + + +def _manifest_skill_entries(raw: object) -> list[str]: + """Pull ``plugins[deepgram].skills`` out of a parsed manifest.""" + if not isinstance(raw, dict): + raise SkillFetchError("Skill manifest is not a JSON object.") + + plugins = raw.get("plugins") + if not isinstance(plugins, list) or not plugins: + raise SkillFetchError("Skill manifest has no 'plugins' array.") + + entries: object = None + found = False + for candidate in plugins: + if isinstance(candidate, dict) and candidate.get("name") == _PLUGIN_NAME: + entries = candidate.get("skills") + found = True + break + if not found: + raise SkillFetchError(f"Skill manifest has no plugin named {_PLUGIN_NAME!r}.") + + if not isinstance(entries, list) or not entries: + raise SkillFetchError( + f"Plugin {_PLUGIN_NAME!r} in the skill manifest lists no skills." + ) + if not all(isinstance(e, str) and e.strip() for e in entries): + raise SkillFetchError( + f"Plugin {_PLUGIN_NAME!r} in the skill manifest has a " + "non-string skill entry." + ) + return [str(e) for e in entries] + + +def _skill_name(entry: str) -> str: + """Derive a skill's directory name from a manifest entry.""" + name = entry.strip().strip("/").rsplit("/", 1)[-1] + if not name or name in {".", ".."}: + raise SkillFetchError(f"Skill manifest entry {entry!r} has no name.") + return name + + +def _resolve_skill_dir(root: Path, entry: str) -> Path: + """Resolve a manifest entry to a skill directory inside ``root``.""" + relative = _relative_parts(entry) + if relative is None: + raise SkillFetchError( + f"Skill manifest entry {entry!r} escapes the bundle root." + ) + + path = root.joinpath(*relative) + if not path.is_dir(): + raise SkillFetchError( + f"Skill manifest lists {entry!r} but that directory is not in " + f"the {SKILLS_REPO} bundle." + ) + if not (path / _SKILL_ENTRY_FILE).is_file(): + raise SkillFetchError( + f"Skill manifest lists {entry!r} but it has no {_SKILL_ENTRY_FILE}." + ) + return path + + +def _relative_parts(entry: str) -> list[str] | None: + """Split a manifest entry into safe relative path parts, or None.""" + parts: list[str] = [] + for part in entry.strip().split("/"): + if part in ("", "."): + continue + if part == ".." or part.startswith("/"): + return None + parts.append(part) + return parts or None + + +# --------------------------------------------------------------------------- +# Download + extraction +# --------------------------------------------------------------------------- + + +def _cache_key(ref: str) -> str: + """Filesystem-safe directory name for a ref.""" + return "".join(c if c.isalnum() or c in "-._" else "_" for c in ref) + + +def _download_and_extract(ref: str, target: Path) -> None: + """Download the bundle for ``ref`` and replace ``target`` with it.""" + url = bundle_url(ref) + with tempfile.TemporaryDirectory(prefix="deepctl-skills-") as tmp: + tmp_path = Path(tmp) + archive = tmp_path / "bundle.tar.gz" + _download(url, ref, archive) + + unpacked = tmp_path / "unpacked" + unpacked.mkdir() + _extract(archive, unpacked, ref) + + roots = [p for p in unpacked.iterdir() if p.is_dir()] + if len(roots) != 1: + raise SkillFetchError( + f"The {SKILLS_REPO}@{ref} bundle does not have the expected " + "single top-level directory." + ) + + # Validate before publishing to the cache, so a bad bundle never + # replaces a good one. + read_manifest_skills(roots[0]) + + target.parent.mkdir(parents=True, exist_ok=True) + if target.exists(): + shutil.rmtree(target) + shutil.move(str(roots[0]), str(target)) + + +def _download(url: str, ref: str, dest: Path) -> None: + """Fetch ``url`` into ``dest``, mapping every failure to SkillFetchError.""" + try: + with urllib.request.urlopen(url, timeout=_DOWNLOAD_TIMEOUT) as resp: + _copy_limited(resp, dest) + except urllib.error.HTTPError as exc: + if exc.code == 404: + raise SkillFetchError( + f"{SKILLS_REPO} has no ref {ref!r} (HTTP 404 from {url}). " + "Check the --ref value." + ) + raise SkillFetchError( + f"Could not download {SKILLS_REPO}@{ref}: HTTP {exc.code} from {url}." + ) + except (urllib.error.URLError, OSError, ValueError) as exc: + raise SkillFetchError( + f"Could not download {SKILLS_REPO}@{ref} from {url}: {exc}" + ) + + +def _copy_limited(src: IO[bytes], dest: Path) -> None: + """Stream ``src`` to ``dest``, refusing an implausibly large bundle.""" + total = 0 + with dest.open("wb") as fh: + while chunk := src.read(64 * 1024): + total += len(chunk) + if total > _MAX_BUNDLE_BYTES: + raise SkillFetchError( + f"The {SKILLS_REPO} bundle exceeded " + f"{_MAX_BUNDLE_BYTES} bytes; refusing to unpack it." + ) + fh.write(chunk) + + +def _extract(archive: Path, dest: Path, ref: str) -> None: + """Unpack ``archive`` into ``dest``, rejecting unsafe members.""" + try: + with tarfile.open(archive, "r:gz") as tar: + # _safe_members already rejects anything that escapes dest; the + # stdlib filter is belt-and-braces where the interpreter has it + # (3.12+, and the backports in 3.10.12 / 3.11.4). + extra: dict[str, Any] = {} + if hasattr(tarfile, "data_filter"): + extra["filter"] = "data" + tar.extractall(dest, members=_safe_members(tar, dest), **extra) + except SkillFetchError: + raise + except (tarfile.TarError, OSError, EOFError) as exc: + raise SkillFetchError( + f"The {SKILLS_REPO}@{ref} download is not a readable tar.gz archive: {exc}" + ) + + +def _safe_members(tar: tarfile.TarFile, dest: Path) -> Iterator[tarfile.TarInfo]: + """Yield only regular files and directories that stay inside ``dest``. + + ``tarfile``'s ``filter="data"`` argument is not available on every + Python version this CLI supports, so the checks are explicit. + """ + root = dest.resolve() + for member in tar: + if not (member.isfile() or member.isdir()): + # Symlinks, hardlinks and devices have no place in a skill + # bundle and are how tar extraction turns into arbitrary writes. + continue + name = member.name + if name.startswith("/") or ".." in Path(name).parts: + raise SkillFetchError( + f"Refusing to unpack {name!r}: it escapes the bundle root." + ) + resolved = (root / name).resolve() + if resolved != root and root not in resolved.parents: + raise SkillFetchError( + f"Refusing to unpack {name!r}: it escapes the bundle root." + ) + member.mode = 0o755 if member.isdir() else 0o644 + yield member diff --git a/packages/deepctl-core/src/deepctl_core/skill_generator.py b/packages/deepctl-core/src/deepctl_core/skill_generator.py index f6a9e10..c09610e 100644 --- a/packages/deepctl-core/src/deepctl_core/skill_generator.py +++ b/packages/deepctl-core/src/deepctl_core/skill_generator.py @@ -1,7 +1,21 @@ -"""Skill generator for AI coding assistant integration. - -Generates skill/instruction files that teach AI coding CLIs (Claude Code, -Codex, Gemini CLI, etc.) how to use deepctl. +"""Install Deepgram skills into AI coding assistants. + +Two different artifacts live in this module, and keeping them apart is the +point: + +* The **Deepgram skills** themselves, fetched from deepgram/skills by + :mod:`deepctl_core.skill_bundle`. A skill is a *folder* — ``SKILL.md`` + plus, for some, a ``references/`` subdirectory — and it is installed + verbatim into whatever directory the target tool loads skills from. +* A generated **deepctl developer guide** (:func:`render_developer_guide`), + for tools that have no skills directory and only read one long context or + rules file. That is the one thing that legitimately gets merged into a + file the user also edits, under HTML markers. + +Mixing the two is what produced ``~/.claude/commands/deepgram/api.md`` (the +slash-command directory, holding a skill) and a 58 KB ``instructions.md`` +with four skills concatenated inside a marker that claimed to be a CLI +reference. """ from __future__ import annotations @@ -13,7 +27,58 @@ from dataclasses import dataclass from importlib import metadata from pathlib import Path -from typing import Any +from typing import TYPE_CHECKING, Any + +from deepctl_core.skill_bundle import fetch_skill_bundle + +if TYPE_CHECKING: + from collections.abc import Callable, Iterable, Sequence + + from deepctl_core.skill_bundle import RepoSkill + +# The cross-tool installer that owns the directory conventions this module +# targets. Quoted verbatim to users whose tool has no skills directory yet. +SKILLS_CLI_HINT = "npx skills add deepgram/skills" + +#: Filename that marks a directory as a skill. +SKILL_ENTRY_FILE = "SKILL.md" + +#: What deepctl <= 0.3.0 could put in ~/.claude/commands/deepgram/: one +#: file per repo skill it knew about. `deepgram.md` is the generated +#: guide, which only a dead `generate()` path ever produced -- listed so +#: a machine that has one is cleaned, not because a release wrote it. +_LEGACY_CLAUDE_COMMAND_FILES = ( + "api.md", + "docs.md", + "setup-mcp.md", + "starters.md", + "deepgram.md", +) + + +class SkillOwnershipError(Exception): + """A destination already exists and deepctl did not put it there. + + Every tool deepctl installs into reads a *shared* skills directory — + ``~/.claude/skills`` and friends hold skills from the user and from + other publishers too. deepctl therefore only ever replaces or deletes + a folder it recorded installing itself, and raises this instead of + touching anything else. + """ + + def __init__(self, conflicts: Sequence[tuple[str, Path]]) -> None: + self.conflicts = list(conflicts) + listing = "\n".join(f" {name}: {path}" for name, path in self.conflicts) + super().__init__( + "Refusing to overwrite skills deepctl did not install:\n" + f"{listing}\n\n" + f"Either {_STATE_FILE} has no record of deepctl installing " + "them, so they belong to you or to another publisher, or the " + "path is now a symlink, which deepctl never writes through. " + "Rename or delete them and run the install again. Nothing " + "was installed." + ) + # --------------------------------------------------------------------------- # Data model @@ -44,8 +109,6 @@ class CommandMetadata: _SKILLS_DIR = Path.home() / ".deepctl" / "skills" _STATE_FILE = _SKILLS_DIR / "skills.json" _REPO_CACHE_DIR = _SKILLS_DIR / "repo_cache" -_SKILLS_REPO = "deepgram/skills" -_SKILLS_BRANCH = "main" def get_skills_state() -> dict[str, Any]: @@ -63,61 +126,64 @@ def save_skills_state(state: dict[str, Any]) -> None: _STATE_FILE.write_text(json.dumps(state, indent=2)) -def fetch_repo_skills(force: bool = False) -> dict[str, str]: - """Download skill markdown files from the deepgram/skills GitHub repo. - - Returns a mapping of skill name -> markdown content. - Caches locally to avoid repeated network requests. +def recorded_skill_paths(state: dict[str, Any], cli_name: str) -> list[str]: + """Paths ``skills.json`` records deepctl having installed for one tool. + + This is deepctl's only claim of ownership over anything in a tool's + skills directory. It is a *claim*, not a guarantee — every consumer + re-checks each path against the tool's skills root before writing to + it or deleting it, so a stale state file cannot point an operation at + an unrelated directory. It is not a sandbox against a *hostile* one: + :meth:`SkillGenerator.owned_skill_paths` lstats only the final + component, so a hand-written entry whose intermediate component is a + symlink can still resolve to a direct child of the root and be + accepted. Anyone who can rewrite ``skills.json`` can already rewrite + anything else under the same home directory, so that is not a + boundary worth pretending to hold. + + A user who deletes ``skills.json`` therefore leaves deepctl unable to + prove it owns anything: install refuses to overwrite the folders it + previously wrote, and remove deletes nothing. That is the safe + direction to fail in — the folders are still there to delete by hand. + + Every shape a hand-edited file can hold is tolerated by claiming + nothing: ``installed_skills`` set to a list or a string, one tool's + entry set to a null, ``paths`` that is not a list, and a non-string + inside it all yield an empty result rather than an exception. Callers + render tables and loops from this, so raising here turns a bad file + into an error message naming a Python type instead of the file. """ - import urllib.request - - cache_marker = _REPO_CACHE_DIR / ".fetched" - if not force and cache_marker.exists(): - # Check if cache is less than 1 hour old - import time - - try: - age = time.time() - cache_marker.stat().st_mtime - if age < 3600: # 1 hour - return _read_cached_skills() - except OSError: - pass - - base = f"https://raw.githubusercontent.com/{_SKILLS_REPO}/{_SKILLS_BRANCH}" - skill_names = ["api", "docs", "setup-mcp", "starters"] - skills: dict[str, str] = {} - - _REPO_CACHE_DIR.mkdir(parents=True, exist_ok=True) + installed = state.get("installed_skills") + entry = installed.get(cli_name) if isinstance(installed, dict) else None + if not isinstance(entry, dict): + return [] + paths = entry.get("paths") + if not isinstance(paths, list): + return [] + return [p for p in paths if isinstance(p, str)] - for name in skill_names: - url = f"{base}/skills/{name}/SKILL.md" - try: - with urllib.request.urlopen(url, timeout=10) as resp: - content = resp.read().decode("utf-8") - skills[name] = content - ( # Cache to disk - _REPO_CACHE_DIR / f"{name}.md" - ).write_text(content) - except Exception: - # Use cached version if available - cached = _REPO_CACHE_DIR / f"{name}.md" - if cached.exists(): - skills[name] = cached.read_text() - if skills: - cache_marker.write_text("1") +def fetch_repo_skills( + ref: str | None = None, + *, + force: bool = False, +) -> list[RepoSkill]: + """Fetch every skill published by deepgram/skills. - return skills + The list comes from the upstream ``.claude-plugin/marketplace.json`` + manifest, never from a list in this repo: upstream CI checks that + manifest against the directories on disk in both directions on every + pull request, so it is the one place that cannot drift. + Args: + ref: Upstream git ref. Defaults to the pinned release tag. + force: Re-download even if the ref is already cached. -def _read_cached_skills() -> dict[str, str]: - """Read previously cached repo skills.""" - skills: dict[str, str] = {} - if not _REPO_CACHE_DIR.exists(): - return skills - for md_file in _REPO_CACHE_DIR.glob("*.md"): - skills[md_file.stem] = md_file.read_text() - return skills + Raises: + SkillFetchError: The bundle could not be fetched or trusted. This + is deliberately fatal — see :class:`SkillFetchError`. + """ + return fetch_skill_bundle(ref, cache_dir=_REPO_CACHE_DIR, force=force) def _commands_hash(commands: list[CommandMetadata]) -> str: @@ -673,434 +739,587 @@ def render_skill_content( # --------------------------------------------------------------------------- +@dataclass(frozen=True) +class LegacyArtifact: + """Something deepctl <= 0.3.0 wrote that the tool does not read as a skill. + + Cleaned up on install and remove so an upgrade does not leave a stale + copy of four skills lying around next to a fresh copy of fourteen. + """ + + path: Path + #: True when the path is a file the user also edits, so only deepctl's + #: own marked-off section may be removed. + shared: bool = False + #: For a directory: the exact filenames deepctl <= 0.3.0 wrote into + #: it. Only those are deleted, and the directory itself only if that + #: leaves it empty, so a file the user put alongside survives — even + #: one with the same extension. Empty means deepctl owned the whole + #: directory. + contents: tuple[str, ...] = () + + class SkillGenerator(ABC): - """Base class for AI CLI skill file generators.""" + """Base class for installing Deepgram skills into one AI coding tool. + + A tool that loads skill *folders* overrides :meth:`skills_root` with the + directory it reads. Skills are copied there verbatim — ``SKILL.md``, + ``references/`` and anything else the skill ships. + + A tool with no skills directory installs nothing and reports + :meth:`manual_hint` instead. Writing a Deepgram blob into a context file + that the tool may or may not read, under a marker claiming to be + something else, is how this command came to write 58 KB into + ``~/.codex/instructions.md`` — a path current Codex does not read at all. + + **Nothing here touches a folder path deepctl did not install.** Those + directories are shared: ``~/.claude/skills`` holds the user's own + skills and other publishers' skills next to Deepgram's. So install, + update and remove all take the paths ``skills.json`` recorded for this + tool, keep only those that are a direct child of :meth:`skills_root`, + and work on that set alone. An existing folder deepctl cannot prove it + installed is never replaced (:class:`SkillOwnershipError`) and never + deleted. + + Ownership is by path, not by content. A recorded path stays deepctl's + until ``dg skills remove`` drops the record, so a *folder* someone + puts back at that path without removing first is replaced like + deepctl's own — and on a case-insensitive filesystem ``API`` and + ``api`` are the same path here. The one exception is a **symlink**: + deepctl never writes or deletes through one, so a recorded path that + became a symlink stops being deepctl's, install and update refuse it, + and remove reports it instead of following it. Closing the rest would + need a fingerprint or a marker file + inside each installed skill, which also decides whether ``update`` may + refresh a skill the user has edited; that is a product decision, not a + detail of this class. + """ cli_name: str = "" display_name: str = "" + #: Markers written by deepctl <= 0.3.0. They claimed to delimit a CLI + #: reference but actually wrapped four concatenated skills. Retained + #: only so that section can be found and removed again. + _LEGACY_BEGIN = "" + _LEGACY_END = "" + @abstractmethod def detect(self) -> bool: """Return True if this AI CLI is installed/available.""" - @abstractmethod - def get_skill_paths(self) -> list[Path]: - """Return the file paths where skills will be written.""" + def skills_root(self) -> Path | None: + """User-scope directory this tool loads skill folders from. - @abstractmethod - def generate( - self, commands: list[CommandMetadata], version: str - ) -> dict[Path, str]: - """Generate skill file contents. + ``None`` means the tool has no documented skills directory, so + skills cannot honestly be installed for it by copying files. + """ + return None + + def legacy_paths(self) -> list[LegacyArtifact]: + """Paths written by earlier deepctl versions, to be cleaned up.""" + return [] + + def owned_skill_paths(self, recorded: Iterable[str | Path]) -> list[Path]: + """The recorded paths this tool may safely write to or delete. + + ``recorded`` comes from :func:`recorded_skill_paths`. Each entry + has to survive three checks before it counts as deepctl's: + + * it is absolute, + * its own final component is not a symlink, and + * it resolves to a *direct child* of this tool's skills root, + with symlinks followed on both sides. + + Those last two are what make a hand-edited or stale + ``skills.json`` harmless. The resolved-parent check drops an entry + pointing at ``~/Documents`` or at + ``~/.claude/skills/api/../../..``. The symlink check drops a skill + folder someone replaced with a symlink *wherever it points*: + resolving alone would let ``skills/api -> skills/my-own-skill`` + pass, because the target is a direct child of the same root, and + deepctl would then unlink the name and bury their work. + + Only the final component is tested, so a record written through a + symlinked ancestor -- ``/tmp`` for ``/private/tmp`` on macOS, a + home directory reached through a link -- still counts as ours. + """ + root = self.skills_root() + if root is None: + return [] + resolved_root = root.expanduser().resolve() + owned: list[Path] = [] + seen: set[Path] = set() + for entry in recorded: + path = Path(entry).expanduser() + if not path.is_absolute(): + continue + if path.is_symlink(): + continue + if path.resolve().parent != resolved_root: + continue + if path in seen: + continue + seen.add(path) + owned.append(path) + return owned + + def installed_skill_paths(self, recorded: Iterable[str | Path]) -> list[Path]: + """Skill folders deepctl installed for this tool that are still there. + + Deliberately *not* "every folder under the skills root with a + ``SKILL.md``": that would count the user's own skills, and every + other publisher's, as Deepgram's. + """ + return sorted( + p + for p in self.owned_skill_paths(recorded) + if (p / SKILL_ENTRY_FILE).is_file() + ) + + def install_conflicts( + self, + skills: list[RepoSkill], + recorded: Iterable[str | Path] = (), + ) -> list[Path]: + """Destinations that already exist and deepctl cannot claim. - Returns: - Mapping of file path -> content string + An upstream skill named ``api`` must not quietly replace a folder + called ``api`` that somebody else wrote. """ + root = self.skills_root() + if root is None: + return [] + # Compare resolved paths, not the strings: a record written under + # one spelling of the same directory (/tmp vs /private/tmp, a home + # reached through a symlink) still describes the folder deepctl + # installed, and matching on text would call it a stranger's. + owned = {p.resolve() for p in self.owned_skill_paths(recorded)} + conflicts: list[Path] = [] + for skill in skills: + dest = root / skill.name + if not (dest.exists() or dest.is_symlink()): + continue + # A symlink standing where a skill folder belongs is never + # ours, whatever it points at -- including another folder in + # this same root, which would otherwise resolve into `owned`. + if dest.is_symlink() or dest.resolve() not in owned: + conflicts.append(dest) + return conflicts def install( self, commands: list[CommandMetadata], # noqa: ARG002 version: str, # noqa: ARG002 + *, + ref: str | None = None, + recorded: Iterable[str | Path] = (), ) -> list[Path]: - """Fetch skills from deepgram/skills repo and install them.""" - repo_skills = fetch_repo_skills(force=True) - if not repo_skills: + """Fetch the upstream skills and install them for this tool. + + One tool, one fetch, and **no ownership record written**. Call + :func:`install_skills_for` instead for anything a user runs: + looping over this method is what left folders on disk that + ``skills.json`` did not know about, because the bundle was + refetched per tool, no destination was checked against the other + tools', and the state was saved only after the loop. + + Raises: + SkillFetchError: Upstream could not be fetched or trusted. + SkillOwnershipError: A destination exists that deepctl did + not install. + """ + if self.skills_root() is None: + self.clean_legacy() return [] - return self._write_repo_skills(repo_skills) + return self.install_skills( + fetch_repo_skills(ref, force=True), recorded=recorded + ) - def _write_repo_skills(self, repo_skills: dict[str, str]) -> list[Path]: - """Write combined repo skill content to this tool's skill paths.""" - combined = "\n\n---\n\n".join(repo_skills.values()) + def install_skills( + self, + skills: list[RepoSkill], + recorded: Iterable[str | Path] = (), + ) -> list[Path]: + """Copy each skill folder into this tool's skills directory. + + Raises: + SkillOwnershipError: One of the destinations already exists + and is not recorded as deepctl's. Checked for every skill + up front, so a collision on the tenth leaves the first + nine unwritten rather than half-installing. + """ + root = self.skills_root() + if root is None: + return [] + recorded = list(recorded) + conflicts = self.install_conflicts(skills, recorded) + if conflicts: + raise SkillOwnershipError([(self.display_name, p) for p in conflicts]) + + self.clean_legacy() + root.mkdir(parents=True, exist_ok=True) written: list[Path] = [] - for path in self.get_skill_paths(): - path.parent.mkdir(parents=True, exist_ok=True) - path.write_text(combined) - written.append(path) + for skill in skills: + dest = root / skill.name + # Replace rather than merge: when a skill drops a reference + # file upstream it has to disappear here too, or the assistant + # keeps reading a page that no longer exists. Only reachable + # for a destination install_conflicts just cleared as ours. + if dest.is_dir() and not dest.is_symlink(): + shutil.rmtree(dest) + elif dest.exists() or dest.is_symlink(): + dest.unlink() + shutil.copytree(skill.path, dest) + written.append(dest) return written - def remove(self) -> list[Path]: - """Remove installed skill files. Returns paths removed.""" - removed: list[Path] = [] - for path in self.get_skill_paths(): - if path.exists(): - path.unlink() + def prune_retired( + self, + recorded: Iterable[str | Path], + skills: list[RepoSkill], + ) -> list[Path]: + """Delete folders deepctl installed that upstream no longer ships. + + Without this, a skill renamed or retired in ``deepgram/skills`` + stays on disk forever: the next install records only the skills + that exist now, so the leftover drops off the ownership list and + becomes something deepctl will neither update nor remove — and + something it would refuse to overwrite if the name ever came back. + + Call it after the install has been recorded, never before. The + record is what makes the folders just written deepctl's, so it + has to land first; the cost is that a crash between the two + leaves a retired folder behind with no record of it. That is the + cheaper of the two failures — a stale folder the user can delete, + rather than fourteen fresh ones deepctl would refuse to touch. + """ + keep = {skill.name for skill in skills} + pruned: list[Path] = [] + for path in self.owned_skill_paths(recorded): + if path.name in keep or path.is_symlink() or not path.is_dir(): + continue + shutil.rmtree(path, ignore_errors=True) + if not path.exists(): + pruned.append(path) + return pruned + + def remove(self, recorded: Iterable[str | Path] = ()) -> list[Path]: + """Remove the skill folders deepctl recorded installing for this tool. + + Only those. A folder deepctl did not install is left alone even + when it sits in the same directory and looks exactly like a skill, + because it is somebody else's work. Symlinks never reach this + method: :meth:`owned_skill_paths` drops them, so deepctl cannot + delete through one. + """ + removed = self.clean_legacy() + for path in self.owned_skill_paths(recorded): + if not path.exists(): + continue + if path.is_dir(): + shutil.rmtree(path, ignore_errors=True) + else: + # A recorded destination someone replaced with a plain + # file. Still deepctl's path, and install would unlink + # it to write the skill there, so remove has to be able + # to finish the job too -- otherwise the record can + # never be cleared and every later remove repeats the + # same warning with no action that would resolve it. + try: + path.unlink() + except OSError: + pass + # Both deletions swallow their errors, so ask the filesystem + # rather than reporting a deletion that did not happen — the + # caller drops the record on the strength of this list. + if not path.exists(): removed.append(path) + # The skills root itself is left standing even when this emptied + # it. deepctl did not necessarily create it -- ~/.agents/skills + # is Codex's and `npx skills add`'s too -- and "only ever touch + # folders deepctl installed" has to hold for the directory those + # folders sat in as well. An empty directory costs nothing. return removed - def is_installed(self) -> bool: - """Check if skill files exist.""" - return any(p.exists() for p in self.get_skill_paths()) + def clean_legacy(self) -> list[Path]: + """Remove what deepctl <= 0.3.0 wrote for this tool.""" + removed: list[Path] = [] + for artifact in self.legacy_paths(): + if _clean_legacy_artifact(artifact, self._LEGACY_BEGIN, self._LEGACY_END): + removed.append(artifact.path) + return removed + + def manual_hint(self) -> str | None: + """How to get Deepgram skills into a tool deepctl cannot install to.""" + if self.skills_root() is not None: + return None + return ( + f"{self.display_name} has no documented skills directory. " + f"For the Deepgram skills, run: {SKILLS_CLI_HINT}" + ) + + +def _clean_legacy_artifact(artifact: LegacyArtifact, begin: str, end: str) -> bool: + """Remove one legacy artifact. Returns True if anything changed.""" + path = artifact.path + if not path.exists(): + return False + + if path.is_dir(): + # Never through a symlink, the same rule owned_skill_paths() + # applies to skill folders. A dotfiles setup that links + # ~/.claude/commands/deepgram at a directory of the user's own + # slash commands would otherwise have `api.md` deleted out of + # it -- a file deepctl never wrote. Nothing is lost by stopping: + # deepctl <= 0.3.0 wrote the real directory, not a link to one. + if path.is_symlink(): + return False + if not artifact.contents: + shutil.rmtree(path, ignore_errors=True) + return True + # A directory deepctl <= 0.3.0 created but does not exclusively + # own: delete only the files it wrote there, and the directory + # itself only once nothing else is left in it. + changed = False + for name in artifact.contents: + child = path / name + if child.is_file() and not child.is_symlink(): + child.unlink() + changed = True + if not any(path.iterdir()): + try: + path.rmdir() + except OSError: + pass + return changed + + if not artifact.shared: + path.unlink() + return True + + # A file the user also writes: take out only deepctl's own section. + try: + content = path.read_text() + except (OSError, UnicodeDecodeError): + return False + if begin not in content: + return False + + head, _, rest = content.partition(begin) + _, found, tail = rest.partition(end) + # An unterminated marker means a truncated write; dropping the tail is + # safer than leaving half a generated blob in the user's instructions. + remaining = (head + (tail if found else "")).strip() + if remaining: + path.write_text(remaining + "\n") + else: + path.unlink() + return True # --------------------------------------------------------------------------- # Concrete generators +# +# Every destination below is the user-scope skills directory each tool's own +# documentation names. Where a tool documents a native directory that is its +# own, skills go there so that `dg skills remove --cli ` has exactly +# one thing to undo. Codex is the exception: its only documented user-scope +# location is the cross-tool ~/.agents/skills, and its ~/.codex/skills is +# marked deprecated in Codex's own source. # --------------------------------------------------------------------------- class ClaudeCodeGenerator(SkillGenerator): - """Generator for Claude Code (Anthropic).""" + """Claude Code — https://code.claude.com/docs/en/skills.""" cli_name = "claude" display_name = "Claude Code" - @property - def _skill_dir(self) -> Path: - return Path.home() / ".claude" / "commands" / "deepgram" - def detect(self) -> bool: return ( Path.home().joinpath(".claude").is_dir() or shutil.which("claude") is not None ) - def get_skill_paths(self) -> list[Path]: + def skills_root(self) -> Path | None: + return Path.home() / ".claude" / "skills" + + def legacy_paths(self) -> list[LegacyArtifact]: + # ~/.claude/commands/ is the single-file prompt directory. Claude + # Code will not read a references/ folder next to a file there, and + # a command file does not accept the `name:` key every SKILL.md has. + # + # Scoped to named filenames, never a `*.md` glob: a slash command + # the user added here is also a .md file, so a glob would take it. return [ - self._skill_dir / f"{name}.md" - for name in ["api", "docs", "setup-mcp", "starters"] + LegacyArtifact( + Path.home() / ".claude" / "commands" / "deepgram", + contents=_LEGACY_CLAUDE_COMMAND_FILES, + ) ] - def generate( - self, commands: list[CommandMetadata], version: str - ) -> dict[Path, str]: - content = render_skill_content(commands, version, include_frontmatter=True) - return {self._skill_dir / "deepgram.md": content} - - def _write_repo_skills(self, repo_skills: dict[str, str]) -> list[Path]: - self._skill_dir.mkdir(parents=True, exist_ok=True) - written: list[Path] = [] - for name, content in repo_skills.items(): - path = self._skill_dir / f"{name}.md" - path.write_text(content) - written.append(path) - return written - - def remove(self) -> list[Path]: - removed: list[Path] = [] - if self._skill_dir.exists(): - for f in self._skill_dir.glob("*.md"): - f.unlink() - removed.append(f) - try: - self._skill_dir.rmdir() - except OSError: - pass - return removed - class CodexGenerator(SkillGenerator): - """Generator for OpenAI Codex CLI.""" + """OpenAI Codex CLI — https://developers.openai.com/codex/skills.""" cli_name = "codex" display_name = "OpenAI Codex" - _BEGIN = "" - _END = "" - def detect(self) -> bool: return ( Path.home().joinpath(".codex").is_dir() or shutil.which("codex") is not None ) - def get_skill_paths(self) -> list[Path]: - return [Path.home() / ".codex" / "instructions.md"] - - def generate( - self, commands: list[CommandMetadata], version: str - ) -> dict[Path, str]: - content = render_skill_content(commands, version) - wrapped = f"{self._BEGIN}\n{content}{self._END}\n" - path = self.get_skill_paths()[0] - return {path: self._merge(path, wrapped)} - - def _merge(self, path: Path, section: str) -> str: - """Merge delimited section into existing file content.""" - if not path.exists(): - return section - existing = path.read_text() - if self._BEGIN in existing: - before = existing[: existing.index(self._BEGIN)] - after_end = existing.find(self._END) - after = existing[after_end + len(self._END) :] if after_end != -1 else "" - return before + section + after.lstrip("\n") - return existing.rstrip("\n") + "\n\n" + section - - def _write_repo_skills(self, repo_skills: dict[str, str]) -> list[Path]: - combined = "\n\n---\n\n".join(repo_skills.values()) - wrapped = f"{self._BEGIN}\n{combined}\n{self._END}\n" - path = self.get_skill_paths()[0] - path.parent.mkdir(parents=True, exist_ok=True) - path.write_text(self._merge(path, wrapped)) - return [path] - - def remove(self) -> list[Path]: - path = self.get_skill_paths()[0] - if not path.exists(): - return [] - content = path.read_text() - if self._BEGIN not in content: - return [] - before = content[: content.index(self._BEGIN)] - after_end = content.find(self._END) - after = content[after_end + len(self._END) :] if after_end != -1 else "" - remaining = (before + after).strip() - if remaining: - path.write_text(remaining + "\n") - else: - path.unlink() - return [path] + def skills_root(self) -> Path | None: + # Codex documents exactly one user-scope location, the cross-tool + # one. ~/.codex/skills also loads, but Codex's source marks it + # "Deprecated user skills location ... kept for backward + # compatibility", so new installs should not go there. + return Path.home() / ".agents" / "skills" + + def legacy_paths(self) -> list[LegacyArtifact]: + # ~/.codex/instructions.md does not appear in current Codex docs or + # source at all; global instructions are ~/.codex/AGENTS.md. It is + # treated as shared anyway, in case a user adopted the file. + return [LegacyArtifact(Path.home() / ".codex" / "instructions.md", shared=True)] class GeminiGenerator(SkillGenerator): - """Generator for Google Gemini CLI.""" + """Gemini CLI — google-gemini/gemini-cli docs/cli/skills.md.""" cli_name = "gemini" display_name = "Gemini CLI" - _BEGIN = "" - _END = "" - def detect(self) -> bool: return ( Path.home().joinpath(".gemini").is_dir() or shutil.which("gemini") is not None ) - def get_skill_paths(self) -> list[Path]: - return [Path.home() / ".gemini" / "GEMINI.md"] - - def generate( - self, commands: list[CommandMetadata], version: str - ) -> dict[Path, str]: - content = render_skill_content(commands, version) - wrapped = f"{self._BEGIN}\n{content}{self._END}\n" - path = self.get_skill_paths()[0] - return {path: self._merge(path, wrapped)} - - def _merge(self, path: Path, section: str) -> str: - if not path.exists(): - return section - existing = path.read_text() - if self._BEGIN in existing: - before = existing[: existing.index(self._BEGIN)] - after_end = existing.find(self._END) - after = existing[after_end + len(self._END) :] if after_end != -1 else "" - return before + section + after.lstrip("\n") - return existing.rstrip("\n") + "\n\n" + section - - def _write_repo_skills(self, repo_skills: dict[str, str]) -> list[Path]: - combined = "\n\n---\n\n".join(repo_skills.values()) - wrapped = f"{self._BEGIN}\n{combined}\n{self._END}\n" - path = self.get_skill_paths()[0] - path.parent.mkdir(parents=True, exist_ok=True) - path.write_text(self._merge(path, wrapped)) - return [path] - - def remove(self) -> list[Path]: - path = self.get_skill_paths()[0] - if not path.exists(): - return [] - content = path.read_text() - if self._BEGIN not in content: - return [] - before = content[: content.index(self._BEGIN)] - after_end = content.find(self._END) - after = content[after_end + len(self._END) :] if after_end != -1 else "" - remaining = (before + after).strip() - if remaining: - path.write_text(remaining + "\n") - else: - path.unlink() - return [path] - + def skills_root(self) -> Path | None: + return Path.home() / ".gemini" / "skills" -class AmazonQGenerator(SkillGenerator): - """Generator for Amazon Q Developer CLI.""" - - cli_name = "amazonq" - display_name = "Amazon Q Developer" + def legacy_paths(self) -> list[LegacyArtifact]: + # GEMINI.md is a real global context file, which is exactly why + # deepctl should not be pasting 58 KB of skills into it. + return [LegacyArtifact(Path.home() / ".gemini" / "GEMINI.md", shared=True)] - def detect(self) -> bool: - return Path.home().joinpath(".amazonq").is_dir() - - def get_skill_paths(self) -> list[Path]: - return [Path.home() / ".amazonq" / "rules" / "deepctl.md"] - - def generate( - self, commands: list[CommandMetadata], version: str - ) -> dict[Path, str]: - content = render_skill_content(commands, version) - return {self.get_skill_paths()[0]: content} - - -class AiderGenerator(SkillGenerator): - """Generator for Aider CLI.""" - cli_name = "aider" - display_name = "Aider" +class CursorGenerator(SkillGenerator): + """Cursor — https://cursor.com/docs/context/skills.""" - _SKILL_FILE = Path.home() / ".deepctl" / "skills" / "deepctl-conventions.md" + cli_name = "cursor" + display_name = "Cursor" def detect(self) -> bool: - return shutil.which("aider") is not None - - def get_skill_paths(self) -> list[Path]: - return [self._SKILL_FILE] - - def generate( - self, commands: list[CommandMetadata], version: str - ) -> dict[Path, str]: - content = render_skill_content(commands, version) - return {self._SKILL_FILE: content} - - def install(self, commands: list[CommandMetadata], version: str) -> list[Path]: - written = super().install(commands, version) - # Add read reference to aider config if not already present - self._ensure_config_ref() - return written - - def _ensure_config_ref(self) -> None: - """Add the skill file as a read reference in ~/.aider.conf.yml.""" - conf_path = Path.home() / ".aider.conf.yml" - ref = str(self._SKILL_FILE) - try: - import yaml - - if conf_path.exists(): - data = yaml.safe_load(conf_path.read_text()) or {} - else: - data = {} - read_list = data.get("read", []) - if not isinstance(read_list, list): - read_list = [read_list] if read_list else [] - if ref not in read_list: - read_list.append(ref) - data["read"] = read_list - conf_path.write_text(yaml.dump(data, default_flow_style=False)) - except Exception: - pass + return ( + Path.home().joinpath(".cursor").is_dir() + or shutil.which("cursor") is not None + ) - def remove(self) -> list[Path]: - removed = super().remove() - # Remove reference from aider config - conf_path = Path.home() / ".aider.conf.yml" - ref = str(self._SKILL_FILE) - try: - import yaml + def skills_root(self) -> Path | None: + return Path.home() / ".cursor" / "skills" - if conf_path.exists(): - data = yaml.safe_load(conf_path.read_text()) or {} - read_list = data.get("read", []) - if isinstance(read_list, list) and ref in read_list: - read_list.remove(ref) - data["read"] = read_list - conf_path.write_text(yaml.dump(data, default_flow_style=False)) - except Exception: - pass - return removed + def legacy_paths(self) -> list[LegacyArtifact]: + # Cursor rules are project-scoped .cursor/rules/*.mdc; user-scope + # rules are a settings-UI feature, so ~/.cursor/rules/deepctl.mdc + # was never read by anything. + return [LegacyArtifact(Path.home() / ".cursor" / "rules" / "deepctl.mdc")] class OpenCodeGenerator(SkillGenerator): - """Generator for OpenCode CLI.""" + """OpenCode — https://opencode.ai/docs/skills.""" cli_name = "opencode" display_name = "OpenCode" - _BEGIN = "" - _END = "" - def detect(self) -> bool: return ( Path.home().joinpath(".opencode").is_dir() + or Path.home().joinpath(".config", "opencode").is_dir() or shutil.which("opencode") is not None ) - def get_skill_paths(self) -> list[Path]: - return [Path.home() / ".opencode" / "agents.md"] - - def generate( - self, commands: list[CommandMetadata], version: str - ) -> dict[Path, str]: - content = render_skill_content(commands, version) - wrapped = f"{self._BEGIN}\n{content}{self._END}\n" - path = self.get_skill_paths()[0] - return {path: self._merge(path, wrapped)} - - def _merge(self, path: Path, section: str) -> str: - if not path.exists(): - return section - existing = path.read_text() - if self._BEGIN in existing: - before = existing[: existing.index(self._BEGIN)] - after_end = existing.find(self._END) - after = existing[after_end + len(self._END) :] if after_end != -1 else "" - return before + section + after.lstrip("\n") - return existing.rstrip("\n") + "\n\n" + section - - def _write_repo_skills(self, repo_skills: dict[str, str]) -> list[Path]: - combined = "\n\n---\n\n".join(repo_skills.values()) - wrapped = f"{self._BEGIN}\n{combined}\n{self._END}\n" - path = self.get_skill_paths()[0] - path.parent.mkdir(parents=True, exist_ok=True) - path.write_text(self._merge(path, wrapped)) - return [path] - - def remove(self) -> list[Path]: - path = self.get_skill_paths()[0] - if not path.exists(): - return [] - content = path.read_text() - if self._BEGIN not in content: - return [] - before = content[: content.index(self._BEGIN)] - after_end = content.find(self._END) - after = content[after_end + len(self._END) :] if after_end != -1 else "" - remaining = (before + after).strip() - if remaining: - path.write_text(remaining + "\n") - else: - path.unlink() - return [path] + def skills_root(self) -> Path | None: + return Path.home() / ".config" / "opencode" / "skills" + def legacy_paths(self) -> list[LegacyArtifact]: + return [LegacyArtifact(Path.home() / ".opencode" / "agents.md", shared=True)] -class CursorGenerator(SkillGenerator): - """Generator for Cursor IDE CLI.""" - cli_name = "cursor" - display_name = "Cursor" +class ClineGenerator(SkillGenerator): + """Cline — https://docs.cline.bot/features/skills.""" + + cli_name = "cline" + display_name = "Cline" def detect(self) -> bool: - return ( - Path.home().joinpath(".cursor").is_dir() - or shutil.which("cursor") is not None - ) + return Path.home().joinpath(".cline").is_dir() - def get_skill_paths(self) -> list[Path]: - return [Path.home() / ".cursor" / "rules" / "deepctl.mdc"] + def skills_root(self) -> Path | None: + return Path.home() / ".cline" / "skills" - def generate( - self, commands: list[CommandMetadata], version: str - ) -> dict[Path, str]: - content = render_skill_content(commands, version) - return {self.get_skill_paths()[0]: content} + def legacy_paths(self) -> list[LegacyArtifact]: + return [LegacyArtifact(Path.home() / ".cline" / "rules" / "deepctl.md")] -class ClineGenerator(SkillGenerator): - """Generator for Cline CLI.""" +class AmazonQGenerator(SkillGenerator): + """Amazon Q Developer CLI — no skills mechanism to install into. - cli_name = "cline" - display_name = "Cline" + Q Developer has custom agents (``~/.aws/amazonq/cli-agents/*.json``) + and project-scoped ``.amazonq/rules/`` pulled in through an agent's + ``resources``. Neither is a skills directory, and the + ``~/.amazonq/rules/deepctl.md`` this command used to write is not a + path Q reads. So it reports the one-liner instead of writing a file. + """ + + cli_name = "amazonq" + display_name = "Amazon Q Developer" def detect(self) -> bool: - return Path.home().joinpath(".cline").is_dir() + return Path.home().joinpath(".amazonq").is_dir() + + def legacy_paths(self) -> list[LegacyArtifact]: + return [LegacyArtifact(Path.home() / ".amazonq" / "rules" / "deepctl.md")] + - def get_skill_paths(self) -> list[Path]: - return [Path.home() / ".cline" / "rules" / "deepctl.md"] +class AiderGenerator(SkillGenerator): + """Aider — no skills mechanism; it reads whole files listed in config.""" + + cli_name = "aider" + display_name = "Aider" + + _LEGACY_FILE = Path.home() / ".deepctl" / "skills" / "deepctl-conventions.md" + + def detect(self) -> bool: + return shutil.which("aider") is not None + + def legacy_paths(self) -> list[LegacyArtifact]: + return [LegacyArtifact(self._LEGACY_FILE)] + + def clean_legacy(self) -> list[Path]: + removed = super().clean_legacy() + self._drop_config_ref() + return removed + + def _drop_config_ref(self) -> None: + """Drop the stale read reference from ~/.aider.conf.yml.""" + conf_path = Path.home() / ".aider.conf.yml" + ref = str(self._LEGACY_FILE) + try: + import yaml - def generate( - self, commands: list[CommandMetadata], version: str - ) -> dict[Path, str]: - content = render_skill_content(commands, version) - return {self.get_skill_paths()[0]: content} + if not conf_path.exists(): + return + data = yaml.safe_load(conf_path.read_text()) or {} + read_list = data.get("read", []) + if isinstance(read_list, list) and ref in read_list: + read_list.remove(ref) + data["read"] = read_list + conf_path.write_text(yaml.dump(data, default_flow_style=False)) + except Exception: + pass # --------------------------------------------------------------------------- @@ -1111,11 +1330,11 @@ def generate( ClaudeCodeGenerator, CodexGenerator, GeminiGenerator, - AmazonQGenerator, - AiderGenerator, - OpenCodeGenerator, CursorGenerator, + OpenCodeGenerator, ClineGenerator, + AmazonQGenerator, + AiderGenerator, ] @@ -1127,3 +1346,268 @@ def get_all_generators() -> list[SkillGenerator]: def detect_ai_clis() -> list[SkillGenerator]: """Return generators for detected AI CLIs.""" return [g for g in get_all_generators() if g.detect()] + + +def installable_generators( + generators: list[SkillGenerator], +) -> tuple[list[SkillGenerator], list[SkillGenerator]]: + """Split generators into those with a skills directory and those without.""" + supported = [g for g in generators if g.skills_root() is not None] + unsupported = [g for g in generators if g.skills_root() is None] + return supported, unsupported + + +@dataclass +class SkillInstallReport: + """What :func:`install_skills_for` did, tool by tool. + + Callers render this; the core never prints. ``conflicts`` and + ``failures`` are only ever non-empty in best-effort mode, because + otherwise the corresponding exception is raised instead. + """ + + #: The deepgram/skills revision that was installed. + ref: str + #: The bundle that was fetched, empty when nothing needed fetching. + skills: list[RepoSkill] + #: cli_name -> the skill folders written for it, in install order. + written: dict[str, list[Path]] + #: Selected tools that have no skills directory to install into. + unsupported: list[SkillGenerator] + #: (display_name, destination) pairs deepctl refused to overwrite. + conflicts: list[tuple[str, Path]] + #: (display_name, error) for tools that raised part-way through. + failures: list[tuple[str, Exception]] + + @property + def total_written(self) -> int: + return sum(len(paths) for paths in self.written.values()) + + +def _ownership_after_failure( + gen: SkillGenerator, + skills: list[RepoSkill], + recorded: Iterable[str | Path], +) -> list[Path]: + """Every folder of this tool's that deepctl must stay able to touch. + + Used when an install raises part-way: whatever landed at a + destination the preflight already cleared as ours, plus anything + previously recorded that is still on disk. Recording less would turn + a half-written bundle into folders deepctl will neither update nor + remove, and would later refuse to overwrite. + + Never called for :class:`SkillOwnershipError`, because that is + raised before the first byte is written and the destinations it + names are precisely the ones that are *not* deepctl's. A symlink is + excluded for the same reason: it is never deepctl's, whatever it + points at, so claiming one would both break that rule and strand the + record on a path :meth:`SkillGenerator.remove` will not follow. + """ + root = gen.skills_root() + landed = ( + { + root / skill.name + for skill in skills + if (root / skill.name).exists() and not (root / skill.name).is_symlink() + } + if root is not None + else set() + ) + surviving = {p for p in gen.owned_skill_paths(recorded) if p.exists()} + return sorted(landed | surviving) + + +def _retire_unsupported( + unsupported: Iterable[SkillGenerator], + installed: dict[str, Any], +) -> bool: + """Clean up after tools deepctl cannot install to, and un-record them. + + Nothing was written for them, so nothing may claim it was: an entry + here would make ``dg skills list`` show a tool as installed with no + skills, and ``dg skills update`` chase it every run. + + Returns: + True when a record was dropped, so the caller knows to save. + """ + changed = False + for gen in unsupported: + try: + gen.clean_legacy() + except OSError: + # Best-effort: these are deepctl <= 0.3.0 leftovers for a tool + # nothing is being installed to. An unreadable ~/.gemini/GEMINI.md + # must not abort an install that is about to write real skill + # folders for every other tool. + pass + # `in` rather than the return of pop(): a hand-edited skills.json + # can hold a null for a tool, and popping that would look like + # nothing was dropped and leave the record unsaved. + if gen.cli_name in installed: + del installed[gen.cli_name] + changed = True + return changed + + +def install_skills_for( + generators: Sequence[SkillGenerator], + state: dict[str, Any], + *, + commands: list[CommandMetadata], + version: str, + ref: str | None = None, + fetch: Callable[[], list[RepoSkill]] | None = None, + on_installed: Callable[[SkillGenerator, list[Path]], None] | None = None, + best_effort: bool = False, +) -> SkillInstallReport: + """Install the upstream skills for several tools under one contract. + + Every route that writes skill folders goes through here -- ``dg + skills install`` and ``update``, the post-login prompt, and the + refresh a plugin change triggers -- so they cannot drift apart on the + one thing that matters: a folder on disk always has an ownership + record. + + The contract is: + + * **Fetch once.** One bundle for every tool, so two destinations + cannot end up holding different revisions. + * **Preflight every destination first.** A collision in the last tool + stops the first from being written at all, rather than leaving a + half-applied update. + * **Save after each tool.** If a later one fails, the folders already + written stay deepctl's to update and remove. + * **Record what landed even on failure**, so a tool that raises + part-way still owns the folders that exist. + * **Prune retired skills after recording**, never before: a crash in + between should leave a stale folder, not an untracked one. + + ``state`` is mutated and saved in place. + + Args: + generators: The tools to install for. Ones with no skills + directory are reported in ``unsupported`` and never recorded. + state: The loaded ``skills.json``. + commands: Command metadata, for the recorded ``commands_hash``. + version: The deepctl version to record. + ref: The deepgram/skills revision, or ``None`` for the default. + fetch: Overrides how the bundle is obtained, for a caller that + reports a download failure in its own words. Called at most + once, and only when there is something to install. + on_installed: Called with each tool and its folders as that tool + lands, so a caller can report it before a later tool fails. + best_effort: Collect conflicts and errors into the report and + keep going instead of raising, for callers such as login that + must not fail the command they are attached to. + + Returns: + A :class:`SkillInstallReport` for the caller to render. + + Raises: + SkillFetchError: Upstream could not be fetched or trusted. Raised + in both modes, because nothing has been written yet. + SkillOwnershipError: A destination exists that deepctl did not + install. Not raised when ``best_effort`` is set. + """ + from datetime import datetime, timezone + + from deepctl_core.skill_bundle import resolve_skills_ref + + supported, unsupported = installable_generators(list(generators)) + report = SkillInstallReport( + ref=resolve_skills_ref(ref), + skills=[], + written={}, + unsupported=unsupported, + conflicts=[], + failures=[], + ) + # Not setdefault: a hand-edited skills.json can carry a null or a + # list here, and every write below would then raise instead of + # installing. recorded_skill_paths() tolerates the same damage. + installed = state.get("installed_skills") + if not isinstance(installed, dict): + installed = {} + state["installed_skills"] = installed + + if not supported: + # Nothing to install means nothing to download. + if _retire_unsupported(unsupported, installed): + save_skills_state(state) + return report + + skills = fetch() if fetch is not None else fetch_repo_skills(ref, force=True) + report.skills = skills + + targets: list[SkillGenerator] = [] + for gen in supported: + found = gen.install_conflicts(skills, recorded_skill_paths(state, gen.cli_name)) + if found: + report.conflicts.extend((gen.display_name, p) for p in found) + else: + targets.append(gen) + if report.conflicts and not best_effort: + raise SkillOwnershipError(report.conflicts) + + # After the fetch and the preflight, so a download failure or a + # collision leaves these tools' files alone -- but before the writes, + # so a tool failing part-way through the loop cannot skip it and + # leave a stale record behind. + if _retire_unsupported(unsupported, installed): + save_skills_state(state) + + commands_hash = _commands_hash(commands) + now = datetime.now(timezone.utc).isoformat() + for gen in targets: + recorded = recorded_skill_paths(state, gen.cli_name) + try: + paths = gen.install_skills(skills, recorded) + except Exception as exc: + # A collision is raised before anything is written, and the + # destinations it names are the ones that are NOT deepctl's. + # Claiming them here would convert a refusal to touch + # someone else's folder into a record saying it is ours, + # which the next install would then delete. + kept = ( + [] + if isinstance(exc, SkillOwnershipError) + else _ownership_after_failure(gen, skills, recorded) + ) + if kept: + installed[gen.cli_name] = { + "paths": [str(p) for p in kept], + "installed_at": now, + "version": version, + "commands_hash": commands_hash, + "skills_ref": report.ref, + "skills": [p.name for p in kept], + } + save_skills_state(state) + report.failures.append((gen.display_name, exc)) + if not best_effort: + raise + continue + + installed[gen.cli_name] = { + "paths": [str(p) for p in paths], + "installed_at": now, + "version": version, + "commands_hash": commands_hash, + "skills_ref": report.ref, + "skills": [s.name for s in skills], + } + # Record each tool as it lands. If the next one raises -- a + # read-only mount, a full disk -- the folders already written + # stay deepctl's to update and remove, instead of becoming + # unowned litter it will later refuse to touch. + save_skills_state(state) + gen.prune_retired(recorded, skills) + report.written[gen.cli_name] = paths + # Report each tool as it lands, not once the loop is over: a + # later tool raising must not hide the ones that did install and + # are now recorded. + if on_installed is not None: + on_installed(gen, paths) + + return report diff --git a/packages/deepctl-core/tests/unit/test_skill_bundle.py b/packages/deepctl-core/tests/unit/test_skill_bundle.py new file mode 100644 index 0000000..0c8f6e6 --- /dev/null +++ b/packages/deepctl-core/tests/unit/test_skill_bundle.py @@ -0,0 +1,294 @@ +"""Unit tests for the upstream skill bundle fetcher.""" + +import io +import json +import tarfile +import urllib.error +from pathlib import Path +from unittest.mock import patch + +import pytest +from deepctl_core.skill_bundle import ( + DEFAULT_SKILLS_REF, + REF_ENV_VAR, + SkillFetchError, + bundle_url, + fetch_skill_bundle, + read_manifest_skills, + resolve_skills_ref, +) + +SKILL_NAMES = [ + "speech-to-text", + "text-to-speech", + "voice-agent", + "audio-intelligence", + "text-intelligence", + "browser-agent", + "api", + "docs", + "starters", + "recipes", + "examples", + "cli", + "setup-mcp", + "self-hosted", +] + +# Mirrors the two upstream skills that ship a references/ subdirectory. +SKILLS_WITH_REFERENCES = {"api": ["listen.md", "speak.md"], "self-hosted": ["k8s.md"]} + + +def _manifest(names, plugin_name="deepgram"): + return { + "name": "deepgram-agent-skills", + "plugins": [ + { + "name": plugin_name, + "source": "./", + "skills": [f"./skills/{n}" for n in names], + } + ], + } + + +def _build_repo(root: Path, names=None, manifest=None) -> Path: + """Create a fake deepgram/skills checkout under ``root``.""" + names = SKILL_NAMES if names is None else names + (root / ".claude-plugin").mkdir(parents=True, exist_ok=True) + payload = _manifest(names) if manifest is None else manifest + (root / ".claude-plugin" / "marketplace.json").write_text( + json.dumps(payload) if not isinstance(payload, str) else payload + ) + for name in names: + skill_dir = root / "skills" / name + skill_dir.mkdir(parents=True, exist_ok=True) + (skill_dir / "SKILL.md").write_text( + f"---\nname: {name}\ndescription: Test skill {name}\n---\n\n# {name}\n" + ) + for ref_file in SKILLS_WITH_REFERENCES.get(name, []): + refs = skill_dir / "references" + refs.mkdir(exist_ok=True) + (refs / ref_file).write_text(f"# {name} / {ref_file}\n") + return root + + +def _tarball(source: Path, top="skills-deepgram-skills-v1.6.0") -> bytes: + buf = io.BytesIO() + with tarfile.open(fileobj=buf, mode="w:gz") as tar: + tar.add(source, arcname=top) + return buf.getvalue() + + +class _FakeResponse(io.BytesIO): + """Minimal stand-in for urlopen's context-managed response.""" + + def __enter__(self): + return self + + def __exit__(self, *exc): + self.close() + return False + + +class TestResolveSkillsRef: + def test_defaults_to_the_pinned_tag(self, monkeypatch): + monkeypatch.delenv(REF_ENV_VAR, raising=False) + assert resolve_skills_ref() == DEFAULT_SKILLS_REF + assert DEFAULT_SKILLS_REF.startswith("deepgram-skills-v") + + def test_env_var_overrides_the_default(self, monkeypatch): + monkeypatch.setenv(REF_ENV_VAR, "main") + assert resolve_skills_ref() == "main" + + def test_explicit_ref_wins_over_env(self, monkeypatch): + monkeypatch.setenv(REF_ENV_VAR, "main") + assert resolve_skills_ref("my-branch") == "my-branch" + + def test_blank_env_var_falls_back(self, monkeypatch): + monkeypatch.setenv(REF_ENV_VAR, " ") + assert resolve_skills_ref() == DEFAULT_SKILLS_REF + + def test_bundle_url_uses_the_ref(self): + assert bundle_url("v1.2.3").endswith("/deepgram/skills/tar.gz/v1.2.3") + + +class TestReadManifestSkills: + def test_returns_every_manifest_entry_in_order(self, tmp_path): + skills = read_manifest_skills(_build_repo(tmp_path)) + assert [s.name for s in skills] == SKILL_NAMES + assert len(skills) == 14 + + def test_skill_paths_point_into_the_bundle_that_was_read(self, tmp_path): + """read_manifest_skills() only returns folders that exist, so the + interesting part is *where*: a caller copies from these paths, and + one resolving outside the extracted bundle would copy the wrong + tree. The entry file is named too, because that is what the + installer and `status` look for. + """ + root = _build_repo(tmp_path) + for skill in read_manifest_skills(root): + assert skill.path.parent == root / "skills" + assert skill.path.name == skill.name + assert skill.entry_file == skill.path / "SKILL.md" + assert skill.entry_file.is_file() + + def test_missing_manifest(self, tmp_path): + with pytest.raises(SkillFetchError, match="marketplace.json is missing"): + read_manifest_skills(tmp_path) + + def test_malformed_json(self, tmp_path): + _build_repo(tmp_path, manifest="{not json") + with pytest.raises(SkillFetchError, match="not valid JSON"): + read_manifest_skills(tmp_path) + + def test_manifest_without_plugins(self, tmp_path): + _build_repo(tmp_path, manifest={"name": "x"}) + with pytest.raises(SkillFetchError, match="no 'plugins' array"): + read_manifest_skills(tmp_path) + + def test_manifest_without_the_deepgram_plugin(self, tmp_path): + _build_repo(tmp_path, manifest=_manifest(SKILL_NAMES, plugin_name="other")) + with pytest.raises(SkillFetchError, match="no plugin named 'deepgram'"): + read_manifest_skills(tmp_path) + + def test_manifest_with_empty_skill_list(self, tmp_path): + _build_repo(tmp_path, manifest=_manifest([])) + with pytest.raises(SkillFetchError, match="lists no skills"): + read_manifest_skills(tmp_path) + + def test_manifest_with_non_string_entry(self, tmp_path): + payload = _manifest(["api"]) + payload["plugins"][0]["skills"] = [{"path": "./skills/api"}] + _build_repo(tmp_path, names=["api"], manifest=payload) + with pytest.raises(SkillFetchError, match="non-string skill entry"): + read_manifest_skills(tmp_path) + + def test_manifest_entry_without_a_directory(self, tmp_path): + """A manifest/disk mismatch must fail, never silently install a subset.""" + _build_repo(tmp_path, names=["api"], manifest=_manifest(["api", "ghost"])) + with pytest.raises(SkillFetchError, match="'./skills/ghost'"): + read_manifest_skills(tmp_path) + + def test_manifest_entry_without_a_skill_file(self, tmp_path): + _build_repo(tmp_path, names=["api"]) + (tmp_path / "skills" / "api" / "SKILL.md").unlink() + with pytest.raises(SkillFetchError, match="no SKILL.md"): + read_manifest_skills(tmp_path) + + def test_duplicate_manifest_entries(self, tmp_path): + _build_repo(tmp_path, names=["api"], manifest=_manifest(["api", "api"])) + with pytest.raises(SkillFetchError, match="more than once"): + read_manifest_skills(tmp_path) + + def test_traversing_manifest_entry_is_rejected(self, tmp_path): + # Matched on the message: without the traversal guard the entry + # falls through to "that directory is not in the bundle", which + # is also a SkillFetchError, so a bare raises() proves nothing. + _build_repo(tmp_path, names=["api"], manifest=_manifest(["../../etc"])) + with pytest.raises(SkillFetchError, match="escapes the bundle root"): + read_manifest_skills(tmp_path) + + +class TestFetchSkillBundle: + def _fetch(self, tmp_path, payload, **kwargs): + cache = tmp_path / "cache" + with patch( + "urllib.request.urlopen", return_value=_FakeResponse(payload) + ) as opener: + skills = fetch_skill_bundle(cache_dir=cache, **kwargs) + return skills, opener, cache + + def test_extracts_all_fourteen_skills(self, tmp_path): + payload = _tarball(_build_repo(tmp_path / "repo")) + skills, _, _ = self._fetch(tmp_path, payload) + assert [s.name for s in skills] == SKILL_NAMES + + def test_preserves_reference_subdirectories(self, tmp_path): + payload = _tarball(_build_repo(tmp_path / "repo")) + skills, _, _ = self._fetch(tmp_path, payload) + by_name = {s.name: s for s in skills} + for name, files in SKILLS_WITH_REFERENCES.items(): + refs = by_name[name].path / "references" + assert refs.is_dir(), f"{name} lost its references/ directory" + assert sorted(p.name for p in refs.iterdir()) == sorted(files) + + def test_requests_the_pinned_tag_by_default(self, tmp_path, monkeypatch): + monkeypatch.delenv(REF_ENV_VAR, raising=False) + payload = _tarball(_build_repo(tmp_path / "repo")) + _, opener, _ = self._fetch(tmp_path, payload) + assert DEFAULT_SKILLS_REF in opener.call_args[0][0] + + def test_second_call_uses_the_cache(self, tmp_path): + payload = _tarball(_build_repo(tmp_path / "repo")) + _, opener, cache = self._fetch(tmp_path, payload) + assert opener.call_count == 1 + with patch("urllib.request.urlopen", side_effect=AssertionError) as second: + skills = fetch_skill_bundle(cache_dir=cache) + assert second.call_count == 0 + assert len(skills) == 14 + + def test_network_failure_raises(self, tmp_path): + with patch( + "urllib.request.urlopen", + side_effect=urllib.error.URLError("Name or service not known"), + ): + with pytest.raises(SkillFetchError, match="Could not download"): + fetch_skill_bundle(cache_dir=tmp_path / "cache") + + def test_unknown_ref_reports_a_404(self, tmp_path): + err = urllib.error.HTTPError( + "https://codeload.github.com/x", 404, "Not Found", {}, None + ) + with patch("urllib.request.urlopen", side_effect=err): + with pytest.raises(SkillFetchError, match="has no ref 'nope'"): + fetch_skill_bundle("nope", cache_dir=tmp_path / "cache") + + def test_server_error_reports_the_status(self, tmp_path): + err = urllib.error.HTTPError( + "https://codeload.github.com/x", 503, "Unavailable", {}, None + ) + with patch("urllib.request.urlopen", side_effect=err): + with pytest.raises(SkillFetchError, match="HTTP 503"): + fetch_skill_bundle(cache_dir=tmp_path / "cache") + + def test_corrupt_archive_raises(self, tmp_path): + with pytest.raises(SkillFetchError, match="not a readable tar.gz"): + self._fetch(tmp_path, b"this is not a tarball") + + def test_malformed_manifest_does_not_replace_a_good_cache(self, tmp_path): + good = _tarball(_build_repo(tmp_path / "repo")) + _, _, cache = self._fetch(tmp_path, good) + + bad_repo = _build_repo(tmp_path / "bad", manifest="{broken") + with patch( + "urllib.request.urlopen", return_value=_FakeResponse(_tarball(bad_repo)) + ): + with pytest.raises(SkillFetchError, match="not valid JSON"): + fetch_skill_bundle(cache_dir=cache, force=True) + + # The previously cached, valid bundle survived the failed refresh. + assert len(fetch_skill_bundle(cache_dir=cache)) == 14 + + def test_absolute_member_is_rejected(self, tmp_path): + buf = io.BytesIO() + with tarfile.open(fileobj=buf, mode="w:gz") as tar: + info = tarfile.TarInfo("/etc/passwd") + info.size = 3 + tar.addfile(info, io.BytesIO(b"bad")) + with pytest.raises(SkillFetchError, match="escapes the bundle root"): + self._fetch(tmp_path, buf.getvalue()) + + def test_symlink_members_are_skipped(self, tmp_path): + """A skill bundle has no business shipping links.""" + repo = _build_repo(tmp_path / "repo") + buf = io.BytesIO() + with tarfile.open(fileobj=buf, mode="w:gz") as tar: + tar.add(repo, arcname="top") + link = tarfile.TarInfo("top/escape") + link.type = tarfile.SYMTYPE + link.linkname = "/etc/passwd" + tar.addfile(link) + skills, _, cache = self._fetch(tmp_path, buf.getvalue()) + assert len(skills) == 14 + assert not (cache / "deepgram-skills-v1.6.0" / "escape").exists() diff --git a/packages/deepctl-core/tests/unit/test_skill_generator.py b/packages/deepctl-core/tests/unit/test_skill_generator.py index 9074564..5065fec 100644 --- a/packages/deepctl-core/tests/unit/test_skill_generator.py +++ b/packages/deepctl-core/tests/unit/test_skill_generator.py @@ -1,11 +1,15 @@ """Unit tests for skill generator module.""" import json +import shutil from pathlib import Path from unittest.mock import MagicMock, patch import pytest +from deepctl_core import skill_generator +from deepctl_core.skill_bundle import DEFAULT_SKILLS_REF, SkillFetchError from deepctl_core.skill_generator import ( + AiderGenerator, AmazonQGenerator, ClaudeCodeGenerator, ClineGenerator, @@ -13,11 +17,14 @@ CommandMetadata, CursorGenerator, GeminiGenerator, + LegacyArtifact, + SkillOwnershipError, _commands_hash, collect_command_metadata, detect_ai_clis, get_all_generators, get_skills_state, + recorded_skill_paths, render_developer_guide, render_skill_content, save_skills_state, @@ -25,6 +32,62 @@ ) +#: Set by the autouse fixture below for the duration of each test. The guard +#: test reads it from here rather than requesting the fixture, so it fails if +#: the fixture ever stops being autouse. +_ACTIVE_THROWAWAY_HOME: Path | None = None + + +@pytest.fixture(autouse=True) +def _throwaway_home(tmp_path, monkeypatch): + """Point every home-derived path in this module at a throwaway directory. + + Nothing here may read or write the home of whoever is running pytest. + Two routes reach it and neither is obvious at the call site: + + * ``install_skills()`` runs ``clean_legacy()``, which resolves + ``Path.home()`` when it is called, so patching only ``skills_root`` + still let six tests delete the real + ``~/.claude/commands/deepgram/*.md``. + * ``_SKILLS_DIR``, ``_STATE_FILE``, ``_REPO_CACHE_DIR`` and + ``AiderGenerator._LEGACY_FILE`` are evaluated at import time, so they + keep pointing at the real home however ``Path.home`` is patched. + """ + home = tmp_path / "throwaway-home" + home.mkdir() + monkeypatch.setattr(Path, "home", staticmethod(lambda: home)) + monkeypatch.setenv("HOME", str(home)) + monkeypatch.setenv("USERPROFILE", str(home)) + + skills_dir = home / ".deepctl" / "skills" + monkeypatch.setattr(skill_generator, "_SKILLS_DIR", skills_dir) + monkeypatch.setattr(skill_generator, "_STATE_FILE", skills_dir / "skills.json") + monkeypatch.setattr(skill_generator, "_REPO_CACHE_DIR", skills_dir / "repo_cache") + monkeypatch.setattr( + AiderGenerator, "_LEGACY_FILE", skills_dir / "deepctl-conventions.md" + ) + + global _ACTIVE_THROWAWAY_HOME + _ACTIVE_THROWAWAY_HOME = home + yield home + _ACTIVE_THROWAWAY_HOME = None + + +def test_no_test_in_this_module_can_reach_the_real_home(): + """The guard above is the finding, so it gets its own assertion.""" + home = _ACTIVE_THROWAWAY_HOME + assert home is not None, "the throwaway-home fixture is no longer autouse" + assert Path.home() == home + assert skill_generator._STATE_FILE.is_relative_to(home) + assert skill_generator._REPO_CACHE_DIR.is_relative_to(home) + assert AiderGenerator._LEGACY_FILE.is_relative_to(home) + for gen in get_all_generators(): + for artifact in gen.legacy_paths(): + assert artifact.path.is_relative_to(home), gen.cli_name + root = gen.skills_root() + assert root is None or root.is_relative_to(home), gen.cli_name + + def _make_command(**overrides): """Create a CommandMetadata with sensible defaults.""" defaults = { @@ -54,7 +117,9 @@ def test_create(self): assert cmd.examples == ["dg test foo"] def test_create_with_parent_group(self): - cmd = _make_command(name="audio", parent_group="debug", full_command="deepctl debug audio") + cmd = _make_command( + name="audio", parent_group="debug", full_command="deepctl debug audio" + ) assert cmd.parent_group == "debug" assert cmd.full_command == "deepctl debug audio" @@ -63,7 +128,10 @@ class TestCommandsHash: """Test _commands_hash.""" def test_deterministic(self): - cmds = [_make_command(), _make_command(name="other", full_command="deepctl other")] + cmds = [ + _make_command(), + _make_command(name="other", full_command="deepctl other"), + ] h1 = _commands_hash(cmds) h2 = _commands_hash(cmds) assert h1 == h2 @@ -90,23 +158,26 @@ def test_get_skills_state_missing_file(self, tmp_path): def test_save_and_get_skills_state(self, tmp_path): state_file = tmp_path / "skills.json" - with patch("deepctl_core.skill_generator._STATE_FILE", state_file), \ - patch("deepctl_core.skill_generator._SKILLS_DIR", tmp_path): - save_skills_state({"installed_skills": {"claude": {}}, "auto_update": False}) + with ( + patch("deepctl_core.skill_generator._STATE_FILE", state_file), + patch("deepctl_core.skill_generator._SKILLS_DIR", tmp_path), + ): + save_skills_state( + {"installed_skills": {"claude": {}}, "auto_update": False} + ) result = get_skills_state() assert result["installed_skills"] == {"claude": {}} assert result["auto_update"] is False def test_skills_need_update_no_installed(self): - with patch("deepctl_core.skill_generator.get_skills_state", return_value={"installed_skills": {}}): + with patch( + "deepctl_core.skill_generator.get_skills_state", + return_value={"installed_skills": {}}, + ): assert skills_need_update([_make_command()]) is False def test_skills_need_update_stale_hash(self): - state = { - "installed_skills": { - "claude": {"commands_hash": "sha256:old"} - } - } + state = {"installed_skills": {"claude": {"commands_hash": "sha256:old"}}} with patch("deepctl_core.skill_generator.get_skills_state", return_value=state): assert skills_need_update([_make_command()]) is True @@ -196,133 +267,692 @@ def test_render_skill_content_frontmatter(self): assert "description:" in content -class TestClaudeCodeGenerator: - """Test ClaudeCodeGenerator.""" +def _fake_skill(tmp_path, name, references=()): + """Build a skill folder the way the upstream bundle ships one.""" + from deepctl_core.skill_bundle import RepoSkill - def test_detect_with_dir(self, tmp_path): - gen = ClaudeCodeGenerator() - with patch.object(Path, "joinpath", return_value=tmp_path): - with patch.object(tmp_path.__class__, "is_dir", return_value=True): - assert gen.detect() is True + skill_dir = tmp_path / "bundle" / name + skill_dir.mkdir(parents=True, exist_ok=True) + (skill_dir / "SKILL.md").write_text(f"---\nname: {name}\n---\n\n# {name}\n") + for ref in references: + refs = skill_dir / "references" + refs.mkdir(exist_ok=True) + (refs / ref).write_text(f"# {ref}\n") + return RepoSkill(name=name, path=skill_dir) + + +class TestSkillsRoots: + """Every destination is the directory the tool's own docs name.""" + + EXPECTED = { + "claude": Path(".claude") / "skills", + # Codex documents only the cross-tool location; ~/.codex/skills is + # marked deprecated in Codex's own source. + "codex": Path(".agents") / "skills", + "gemini": Path(".gemini") / "skills", + "cursor": Path(".cursor") / "skills", + "opencode": Path(".config") / "opencode" / "skills", + "cline": Path(".cline") / "skills", + } + + # Neither tool has a skills mechanism to install into. + NO_SKILLS_DIRECTORY = {"amazonq", "aider"} + + def test_each_generator_targets_its_documented_directory(self): + by_name = {g.cli_name: g for g in get_all_generators()} + for cli_name, expected in self.EXPECTED.items(): + root = by_name[cli_name].skills_root() + assert root == Path.home() / expected, cli_name + + def test_tools_without_a_skills_directory_install_nothing(self): + by_name = {g.cli_name: g for g in get_all_generators()} + for cli_name in self.NO_SKILLS_DIRECTORY: + gen = by_name[cli_name] + assert gen.skills_root() is None + # clean_legacy is patched out: install() calls it, and for + # these two it edits the developer's real ~/.amazonq and + # ~/.aider.conf.yml while the unit suite runs. + with patch.object(gen, "clean_legacy", return_value=[]): + assert gen.install([_make_command()], "1.0.0") == [] + hint = gen.manual_hint() + assert hint and "npx skills add deepgram/skills" in hint + + def test_no_generator_writes_a_slash_command_or_rules_file(self): + """The old destinations were commands/ and rules/ files, not skills.""" + for gen in get_all_generators(): + root = gen.skills_root() + if root is None: + continue + assert root.name == "skills", gen.cli_name + assert "commands" not in root.parts, gen.cli_name + assert "rules" not in root.parts, gen.cli_name + + +class TestInstallSkills: + """Installing copies whole skill folders, references and all.""" - def test_skill_path(self): + def _install(self, tmp_path, skills): gen = ClaudeCodeGenerator() - paths = gen.get_skill_paths() - assert len(paths) > 0 - assert all("deepgram" in str(p) for p in paths) - assert all("commands" in str(p) for p in paths) - assert all(p.suffix == ".md" for p in paths) + root = tmp_path / "home" / ".claude" / "skills" + with patch.object(gen, "skills_root", return_value=root): + return gen, root, gen.install_skills(skills) - def test_generate_includes_frontmatter(self): + def test_writes_one_directory_per_skill(self, tmp_path): + skills = [_fake_skill(tmp_path, n) for n in ("api", "docs", "cli")] + _, root, written = self._install(tmp_path, skills) + assert len(written) == 3 + assert {p.name for p in written} == {"api", "docs", "cli"} + for path in written: + assert (path / "SKILL.md").is_file() + + def test_preserves_reference_subdirectories(self, tmp_path): + """skills/api and skills/self-hosted ship a references/ folder.""" + skills = [ + _fake_skill(tmp_path, "api", references=("listen.md", "speak.md")), + _fake_skill(tmp_path, "docs"), + ] + _, root, _ = self._install(tmp_path, skills) + refs = root / "api" / "references" + assert refs.is_dir() + assert sorted(p.name for p in refs.iterdir()) == ["listen.md", "speak.md"] + + def test_reinstall_drops_files_that_disappeared_upstream(self, tmp_path): + skills = [_fake_skill(tmp_path, "api", references=("old.md",))] + gen, root, written = self._install(tmp_path, skills) + assert (root / "api" / "references" / "old.md").is_file() + + fresh = tmp_path / "bundle2" + (fresh / "api").mkdir(parents=True) + (fresh / "api" / "SKILL.md").write_text("---\nname: api\n---\n") + from deepctl_core.skill_bundle import RepoSkill + + with patch.object(gen, "skills_root", return_value=root): + gen.install_skills([RepoSkill(name="api", path=fresh / "api")], written) + assert not (root / "api" / "references").exists() + + def test_frontmatter_survives_verbatim(self, tmp_path): + skills = [_fake_skill(tmp_path, "api")] + _, root, _ = self._install(tmp_path, skills) + assert (root / "api" / "SKILL.md").read_text().startswith("---\nname: api") + + def test_installed_skill_paths_reports_what_deepctl_installed(self, tmp_path): + skills = [_fake_skill(tmp_path, n) for n in ("api", "docs")] + gen, root, written = self._install(tmp_path, skills) + with patch.object(gen, "skills_root", return_value=root): + assert [p.name for p in gen.installed_skill_paths(written)] == [ + "api", + "docs", + ] + # A folder nobody recorded is not reported, even though it + # sits in the same directory and has a SKILL.md of its own. + (root / "mine").mkdir() + (root / "mine" / "SKILL.md").write_text("---\nname: mine\n---\n") + assert [p.name for p in gen.installed_skill_paths(written)] == [ + "api", + "docs", + ] + + def test_remove_deletes_every_installed_skill(self, tmp_path): + skills = [_fake_skill(tmp_path, n) for n in ("api", "docs")] + gen, root, written = self._install(tmp_path, skills) + with patch.object(gen, "skills_root", return_value=root): + with patch.object(gen, "legacy_paths", return_value=[]): + removed = gen.remove(written) + assert len(removed) == 2 + assert gen.installed_skill_paths(written) == [] + # Every skill folder is gone; the shared directory they sat in + # stays, because deepctl does not own that either. + assert sorted(p.name for p in root.iterdir()) == [] + + def test_install_propagates_a_fetch_failure(self, tmp_path): + """A partial install must never look like a complete one.""" gen = ClaudeCodeGenerator() - cmds = [_make_command()] - result = gen.generate(cmds, "1.0.0") - assert len(result) == 1 - content = list(result.values())[0] - assert content.startswith("---\n") + with patch.object(gen, "skills_root", return_value=tmp_path / "skills"): + with patch( + "deepctl_core.skill_generator.fetch_repo_skills", + side_effect=SkillFetchError("no network"), + ): + with pytest.raises(SkillFetchError): + gen.install([_make_command()], "1.0.0") + + +class TestOwnership: + """These skills directories are shared. deepctl touches only its own. - def test_install_writes_individual_skill_files(self, tmp_path): - from unittest.mock import PropertyMock + ``~/.claude/skills`` and every other destination here hold skills from + the user and from other publishers. Treating each child folder with a + ``SKILL.md`` as deepctl's made ``dg skills remove`` delete a + developer's unrelated work and let an install overwrite it. + """ + + def _gen(self, tmp_path): gen = ClaudeCodeGenerator() - skill_dir = tmp_path / "commands" / "deepgram" - with patch.object(type(gen), "_skill_dir", new_callable=PropertyMock, return_value=skill_dir): - with patch("deepctl_core.skill_generator.fetch_repo_skills", return_value={"api": "# API\n", "docs": "# Docs\n"}): - written = gen.install([_make_command()], "1.0.0") - assert len(written) == 2 - assert skill_dir / "api.md" in written - assert (skill_dir / "api.md").read_text() == "# API\n" - assert (skill_dir / "docs.md").read_text() == "# Docs\n" - - def test_install_returns_empty_when_no_repo_skills(self, tmp_path): - from unittest.mock import PropertyMock + root = tmp_path / "home" / ".claude" / "skills" + root.mkdir(parents=True) + return gen, root + + def _unrelated(self, root, name="my-private-skill"): + """A skill folder somebody other than deepctl put there.""" + folder = root / name + folder.mkdir(parents=True, exist_ok=True) + (folder / "SKILL.md").write_text(f"---\nname: {name}\n---\n\nmine\n") + return folder + + # -- remove ------------------------------------------------------ + + def test_remove_preserves_an_unrelated_skill(self, tmp_path): + """The reported bug: `dg skills remove` erased user-owned folders.""" + gen, root = self._gen(tmp_path) + mine = self._unrelated(root) + skills = [_fake_skill(tmp_path, n) for n in ("api", "docs")] + with patch.object(gen, "skills_root", return_value=root): + with patch.object(gen, "legacy_paths", return_value=[]): + written = gen.install_skills(skills) + removed = gen.remove(written) + + assert sorted(p.name for p in removed) == ["api", "docs"] + assert mine.is_dir() + assert (mine / "SKILL.md").read_text().endswith("mine\n") + + def test_remove_without_a_record_deletes_nothing(self, tmp_path): + """A hand-deleted skills.json leaves deepctl unable to prove ownership.""" + gen, root = self._gen(tmp_path) + mine = self._unrelated(root) + skills = [_fake_skill(tmp_path, "api")] + with patch.object(gen, "skills_root", return_value=root): + with patch.object(gen, "legacy_paths", return_value=[]): + gen.install_skills(skills) + assert gen.remove([]) == [] + + assert (root / "api" / "SKILL.md").is_file() + assert mine.is_dir() + + def test_remove_ignores_a_recorded_path_outside_the_skills_root(self, tmp_path): + """A tampered or stale skills.json cannot aim a delete elsewhere.""" + gen, root = self._gen(tmp_path) + elsewhere = tmp_path / "home" / "Documents" / "thesis" + elsewhere.mkdir(parents=True) + (elsewhere / "chapter-1.md").write_text("years of work") + nested = root / "api" / "references" + nested.mkdir(parents=True) + + with patch.object(gen, "skills_root", return_value=root): + with patch.object(gen, "legacy_paths", return_value=[]): + removed = gen.remove( + [ + str(elsewhere), + str(root / ".." / ".." / "Documents"), + str(nested), # a grandchild, not a direct child + "relative/path", + ] + ) + + assert removed == [] + assert (elsewhere / "chapter-1.md").is_file() + assert nested.is_dir() + + def test_remove_ignores_a_recorded_path_that_became_a_symlink(self, tmp_path): + """Resolve first: a symlinked name must not delete its target.""" + gen, root = self._gen(tmp_path) + target = tmp_path / "home" / "real-work" + target.mkdir(parents=True) + (target / "SKILL.md").write_text("---\nname: api\n---\n") + link = root / "api" + try: + link.symlink_to(target, target_is_directory=True) + except (OSError, NotImplementedError): # unprivileged Windows + pytest.skip("this filesystem does not allow creating symlinks") + + with patch.object(gen, "skills_root", return_value=root): + with patch.object(gen, "legacy_paths", return_value=[]): + assert gen.remove([str(link)]) == [] + + assert target.is_dir() + assert (target / "SKILL.md").is_file() + + def test_remove_clears_a_recorded_path_someone_replaced_with_a_file( + self, tmp_path + ): + """Skipping it left a record no retry could ever clear. + + install already unlinks a plain file standing at a recorded + destination, so remove has to be able to finish the same job -- + otherwise ownership outliving a failed delete means the warning + repeats forever with no action that resolves it. + """ + gen, root = self._gen(tmp_path) + stray = root / "api" + stray.write_text("not a skill folder\n") + + with patch.object(gen, "skills_root", return_value=root): + with patch.object(gen, "legacy_paths", return_value=[]): + removed = gen.remove([str(stray)]) + + assert removed == [stray] + assert not stray.exists() + + def test_remove_reports_nothing_for_a_folder_it_could_not_delete(self, tmp_path): + """The caller drops the record on the strength of this list.""" + gen, root = self._gen(tmp_path) + skills = [_fake_skill(tmp_path, "api")] + + def denied(path, ignore_errors=False, **kwargs): + """What rmtree(ignore_errors=True) does on a read-only mount.""" + if not ignore_errors: + raise PermissionError(13, "Permission denied", str(path)) + + with patch.object(gen, "skills_root", return_value=root): + with patch.object(gen, "legacy_paths", return_value=[]): + written = gen.install_skills(skills) + with patch.object(skill_generator.shutil, "rmtree", denied): + assert gen.remove(written) == [] + + assert (root / "api" / "SKILL.md").is_file() + + def test_remove_leaves_the_shared_skills_root_standing(self, tmp_path): + """The directory is not deepctl's either, only the folders in it. + + ~/.agents/skills is Codex's and `npx skills add`'s as much as + deepctl's, and none of the six roots is created by deepctl alone. + Emptying one is not a licence to delete it. + """ + gen, root = self._gen(tmp_path) + skills = [_fake_skill(tmp_path, "api")] + + with patch.object(gen, "skills_root", return_value=root): + with patch.object(gen, "legacy_paths", return_value=[]): + written = gen.install_skills(skills) + assert gen.remove(written) == written + + assert root.is_dir() + assert list(root.iterdir()) == [] + + # -- install ----------------------------------------------------- + + def test_install_refuses_a_same_name_collision(self, tmp_path): + """An unrecorded folder named `api` is somebody else's `api`.""" + gen, root = self._gen(tmp_path) + mine = self._unrelated(root, "api") + skills = [_fake_skill(tmp_path, n) for n in ("api", "docs")] + + with patch.object(gen, "skills_root", return_value=root): + with patch.object(gen, "legacy_paths", return_value=[]): + with pytest.raises(SkillOwnershipError) as excinfo: + gen.install_skills(skills) + + assert [p for _, p in excinfo.value.conflicts] == [root / "api"] + assert "Refusing to overwrite" in str(excinfo.value) + assert str(root / "api") in str(excinfo.value) + # The user's file is untouched... + assert (mine / "SKILL.md").read_text().endswith("mine\n") + # ...and nothing else was installed either: a collision on one + # skill must not leave a half-written bundle behind. + assert not (root / "docs").exists() + + def test_install_replaces_only_what_deepctl_recorded(self, tmp_path): + gen, root = self._gen(tmp_path) + mine = self._unrelated(root, "mine") + skills = [_fake_skill(tmp_path, "api", references=("old.md",))] + with patch.object(gen, "skills_root", return_value=root): + with patch.object(gen, "legacy_paths", return_value=[]): + written = gen.install_skills(skills) + # Recorded, so a reinstall may replace it. + again = gen.install_skills(skills, written) + assert again == written + assert (root / "api" / "references" / "old.md").is_file() + # "only": the neighbour in the same directory that deepctl never + # recorded came through both installs untouched. + assert (mine / "SKILL.md").read_text().endswith("mine\n") + + def test_a_record_written_through_a_symlinked_home_still_counts(self, tmp_path): + """/tmp vs /private/tmp is the same folder, so it is still ours.""" + gen, root = self._gen(tmp_path) + link_root = tmp_path / "link-home" / ".claude" / "skills" + link_root.parent.mkdir(parents=True) + try: + link_root.symlink_to(root, target_is_directory=True) + except (OSError, NotImplementedError): # unprivileged Windows + pytest.skip("this filesystem does not allow creating symlinks") + + skills = [_fake_skill(tmp_path, "api")] + with patch.object(gen, "skills_root", return_value=root): + with patch.object(gen, "legacy_paths", return_value=[]): + gen.install_skills(skills) + # Recorded under the other spelling of the same directory. + assert gen.install_conflicts(skills, [str(link_root / "api")]) == [] + + def test_a_symlink_to_another_skill_in_the_same_root_is_not_owned(self, tmp_path): + """A recorded name replaced by a symlink is dropped wherever it points. + + Resolving alone is not enough: `skills/api -> skills/mine` has the + same parent once resolved, so the resolved-parent check called it + deepctl's, and an update would have unlinked the name and buried + the user's folder under the Deepgram skill. + """ + gen, root = self._gen(tmp_path) + mine = self._unrelated(root, "my-private-skill") + link = root / "api" + try: + link.symlink_to(mine, target_is_directory=True) + except (OSError, NotImplementedError): # unprivileged Windows + pytest.skip("this filesystem does not allow creating symlinks") + + skills = [_fake_skill(tmp_path, "api")] + with patch.object(gen, "skills_root", return_value=root): + with patch.object(gen, "legacy_paths", return_value=[]): + assert gen.owned_skill_paths([str(link)]) == [] + assert gen.installed_skill_paths([str(link)]) == [] + # Recorded or not, it is a destination deepctl must refuse. + assert gen.install_conflicts(skills, [str(link)]) == [link] + with pytest.raises(SkillOwnershipError): + gen.install_skills(skills, [str(link)]) + + assert link.is_symlink() + assert (mine / "SKILL.md").read_text().endswith("mine\n") + + def test_install_conflicts_lists_every_unowned_destination(self, tmp_path): + gen, root = self._gen(tmp_path) + self._unrelated(root, "api") + self._unrelated(root, "docs") + skills = [_fake_skill(tmp_path, n) for n in ("api", "docs", "cli")] + with patch.object(gen, "skills_root", return_value=root): + conflicts = gen.install_conflicts(skills) + assert conflicts == [root / "api", root / "docs"] + + def test_a_fresh_install_over_an_existing_folder_fails_rather_than_overwrites( + self, tmp_path + ): + """No skills.json yet is exactly the case with no proof of ownership.""" + gen, root = self._gen(tmp_path) + mine = self._unrelated(root, "api") + with patch.object(gen, "skills_root", return_value=root): + with patch.object(gen, "legacy_paths", return_value=[]): + # Patched: install() would otherwise download the real + # bundle into the developer's own ~/.deepctl cache. + with patch( + "deepctl_core.skill_generator.fetch_repo_skills", + return_value=[_fake_skill(tmp_path, "api")], + ): + with pytest.raises(SkillOwnershipError): + gen.install([_make_command()], "1.0.0", recorded=[]) + assert (mine / "SKILL.md").read_text().endswith("mine\n") + + def test_prune_removes_a_skill_that_disappeared_upstream(self, tmp_path): + """Otherwise a retired skill is left behind and becomes unownable.""" + gen, root = self._gen(tmp_path) + mine = self._unrelated(root) + first = [_fake_skill(tmp_path, n) for n in ("api", "retired")] + with patch.object(gen, "skills_root", return_value=root): + with patch.object(gen, "legacy_paths", return_value=[]): + written = gen.install_skills(first) + pruned = gen.prune_retired(written, [_fake_skill(tmp_path, "api")]) + + assert pruned == [root / "retired"] + assert not (root / "retired").exists() + assert (root / "api").is_dir() + # And it still leaves everything it does not own alone. + assert mine.is_dir() + + # -- status ------------------------------------------------------ + + def test_status_does_not_count_unowned_folders(self, tmp_path): + gen, root = self._gen(tmp_path) + self._unrelated(root) + self._unrelated(root, "someone-elses-api") + skills = [_fake_skill(tmp_path, "api")] + with patch.object(gen, "skills_root", return_value=root): + with patch.object(gen, "legacy_paths", return_value=[]): + written = gen.install_skills(skills) + assert gen.installed_skill_paths(written) == [root / "api"] + + def test_status_drops_a_recorded_folder_the_user_deleted(self, tmp_path): + gen, root = self._gen(tmp_path) + skills = [_fake_skill(tmp_path, n) for n in ("api", "docs")] + with patch.object(gen, "skills_root", return_value=root): + with patch.object(gen, "legacy_paths", return_value=[]): + written = gen.install_skills(skills) + shutil.rmtree(root / "docs") + assert gen.installed_skill_paths(written) == [root / "api"] + + # -- the record itself ------------------------------------------- + + def test_recorded_skill_paths_tolerates_a_mangled_state_file(self): + assert recorded_skill_paths({}, "claude") == [] + assert recorded_skill_paths({"installed_skills": None}, "claude") == [] + assert recorded_skill_paths({"installed_skills": {}}, "claude") == [] + # A truthy non-map got as far as calling .get() on it, which is + # an attribute error, not an empty result. The status table calls + # this once per tool, so it took `dg skills status` down with it. + assert recorded_skill_paths({"installed_skills": ["claude"]}, "claude") == [] + assert recorded_skill_paths({"installed_skills": "claude"}, "claude") == [] + assert recorded_skill_paths({"installed_skills": 7}, "claude") == [] + assert ( + recorded_skill_paths({"installed_skills": {"claude": "nope"}}, "claude") + == [] + ) + assert ( + recorded_skill_paths( + {"installed_skills": {"claude": {"paths": "not-a-list"}}}, "claude" + ) + == [] + ) + assert recorded_skill_paths( + {"installed_skills": {"claude": {"paths": ["/a", 7, None, "/b"]}}}, + "claude", + ) == ["/a", "/b"] + + def test_tools_without_a_skills_directory_own_nothing(self): + gen = AmazonQGenerator() + assert gen.owned_skill_paths(["/anywhere"]) == [] + assert gen.install_conflicts([], ["/anywhere"]) == [] + + +class TestLegacyCleanup: + """deepctl <= 0.3.0 wrote files these tools do not read as skills.""" + + def test_claude_slash_command_directory_is_removed(self, tmp_path): + legacy = tmp_path / ".claude" / "commands" / "deepgram" + legacy.mkdir(parents=True) + (legacy / "api.md").write_text("---\nname: api\n---\n") + gen = ClaudeCodeGenerator() - skill_dir = tmp_path / "commands" / "deepgram" - with patch.object(type(gen), "_skill_dir", new_callable=PropertyMock, return_value=skill_dir): - with patch("deepctl_core.skill_generator.fetch_repo_skills", return_value={}): - written = gen.install([_make_command()], "1.0.0") - assert written == [] - - def test_remove_deletes_skill_dir(self, tmp_path): - from unittest.mock import PropertyMock + with patch.object(gen, "legacy_paths", return_value=[LegacyArtifact(legacy)]): + removed = gen.clean_legacy() + assert removed == [legacy] + assert not legacy.exists() + + def test_claude_cleanup_only_takes_the_markdown_it_wrote(self, tmp_path): + """0.3.0 wrote `*.md` here and removed `*.md`; so does the cleanup.""" + legacy = tmp_path / ".claude" / "commands" / "deepgram" + legacy.mkdir(parents=True) + names = ClaudeCodeGenerator().legacy_paths()[0].contents + for name in names: + (legacy / name).write_text(f"---\nname: {name}\n---\n") + # A slash command the user wrote. It is a .md file in the same + # directory, which is exactly why a *.md glob is not safe here. + (legacy / "deploy.md").write_text("my own slash command") + (legacy / "mine").mkdir() + gen = ClaudeCodeGenerator() - skill_dir = tmp_path / "deepgram" - skill_dir.mkdir() - (skill_dir / "api.md").write_text("hello") - with patch.object(type(gen), "_skill_dir", new_callable=PropertyMock, return_value=skill_dir): - removed = gen.remove() - assert len(removed) == 1 - assert not (skill_dir / "api.md").exists() - - def test_is_installed(self, tmp_path): - from unittest.mock import PropertyMock + with patch.object( + gen, "legacy_paths", return_value=[LegacyArtifact(legacy, contents=names)] + ): + removed = gen.clean_legacy() + + assert removed == [legacy] + assert sorted(p.name for p in legacy.iterdir()) == ["deploy.md", "mine"] + assert (legacy / "deploy.md").read_text() == "my own slash command" + + def test_claude_cleanup_does_not_reach_through_a_symlinked_directory( + self, tmp_path + ): + """Keeping dotfiles in a repo is how this path becomes a link. + + `api.md` and `docs.md` are plausible names for slash commands + someone wrote, and the cleanup deletes exactly those names. It + must not follow a link to find them. + """ + mine = tmp_path / "dotfiles" / "claude-commands" + mine.mkdir(parents=True) + (mine / "api.md").write_text("my own /api command") + legacy = tmp_path / ".claude" / "commands" / "deepgram" + legacy.parent.mkdir(parents=True) + try: + legacy.symlink_to(mine, target_is_directory=True) + except (OSError, NotImplementedError): + pytest.skip("this filesystem does not allow creating symlinks") + gen = ClaudeCodeGenerator() - skill_dir = tmp_path / "deepgram" - with patch.object(type(gen), "_skill_dir", new_callable=PropertyMock, return_value=skill_dir): - assert gen.is_installed() is False - skill_dir.mkdir() - (skill_dir / "api.md").write_text("hello") - assert gen.is_installed() is True + names = ClaudeCodeGenerator().legacy_paths()[0].contents + with patch.object( + gen, "legacy_paths", return_value=[LegacyArtifact(legacy, contents=names)] + ): + removed = gen.clean_legacy() + assert removed == [] + assert (mine / "api.md").read_text() == "my own /api command" + assert legacy.is_symlink() -class TestCodexGenerator: - """Test CodexGenerator (append-mode).""" + def test_a_symlinked_whole_directory_artifact_is_left_alone(self, tmp_path): + """rmtree already refused this one, but reported it as cleaned.""" + mine = tmp_path / "dotfiles" / "rules" + mine.mkdir(parents=True) + (mine / "notes.md").write_text("mine") + legacy = tmp_path / ".cursor" / "rules" + legacy.parent.mkdir(parents=True) + try: + legacy.symlink_to(mine, target_is_directory=True) + except (OSError, NotImplementedError): + pytest.skip("this filesystem does not allow creating symlinks") - def test_generate_wraps_in_delimiters(self): - gen = CodexGenerator() - cmds = [_make_command()] - result = gen.generate(cmds, "1.0.0") - path = gen.get_skill_paths()[0] - content = result[path] - assert "" in content + gen = ClaudeCodeGenerator() + with patch.object(gen, "legacy_paths", return_value=[LegacyArtifact(legacy)]): + assert gen.clean_legacy() == [] + assert (mine / "notes.md").read_text() == "mine" - def test_merge_into_existing(self, tmp_path): - gen = CodexGenerator() - target = tmp_path / "instructions.md" - target.write_text("# My instructions\n\nSome content\n") + def test_claude_cleanup_removes_the_directory_once_it_is_empty(self, tmp_path): + legacy = tmp_path / ".claude" / "commands" / "deepgram" + legacy.mkdir(parents=True) + (legacy / "api.md").write_text("stale") - with patch.object(gen, "get_skill_paths", return_value=[target]): - result = gen.generate([_make_command()], "1.0.0") - content = result[target] - assert content.startswith("# My instructions\n") - assert "\n" - "old content\n" + "four concatenated skills\n" "\n" - "after\n" + "more of my notes\n" ) + gen = CodexGenerator() + with patch.object( + gen, + "legacy_paths", + return_value=[LegacyArtifact(target, shared=True)], + ): + removed = gen.clean_legacy() + assert removed == [target] + text = target.read_text() + assert "BEGIN deepctl" not in text + assert "four concatenated skills" not in text + assert "my own notes" in text + assert "more of my notes" in text - with patch.object(gen, "get_skill_paths", return_value=[target]): - result = gen.generate([_make_command()], "1.0.0") - content = result[target] - assert "old content" not in content - assert "before\n" in content - assert "after\n" in content + def test_shared_file_is_deleted_when_only_deepctl_wrote_it(self, tmp_path): + target = tmp_path / "instructions.md" + target.write_text( + "\n" + "blob\n" + "\n" + ) + gen = CodexGenerator() + with patch.object( + gen, + "legacy_paths", + return_value=[LegacyArtifact(target, shared=True)], + ): + gen.clean_legacy() + assert not target.exists() - def test_remove_section(self, tmp_path): + def test_shared_file_without_markers_is_untouched(self, tmp_path): + target = tmp_path / "instructions.md" + target.write_text("purely the user's own file\n") gen = CodexGenerator() + with patch.object( + gen, + "legacy_paths", + return_value=[LegacyArtifact(target, shared=True)], + ): + assert gen.clean_legacy() == [] + assert target.read_text() == "purely the user's own file\n" + + def test_unterminated_marker_does_not_leave_half_a_blob(self, tmp_path): target = tmp_path / "instructions.md" target.write_text( - "before\n" + "keep me\n" "\n" - "content\n" - "\n" - "after\n" + "truncated blob with no end marker\n" ) + gen = CodexGenerator() + with patch.object( + gen, + "legacy_paths", + return_value=[LegacyArtifact(target, shared=True)], + ): + gen.clean_legacy() + assert target.read_text() == "keep me\n" + + def test_installing_cleans_up_the_old_location(self, tmp_path): + legacy = tmp_path / "commands" / "deepgram" + legacy.mkdir(parents=True) + (legacy / "api.md").write_text("stale") + + gen = ClaudeCodeGenerator() + root = tmp_path / "skills" + with patch.object(gen, "skills_root", return_value=root): + with patch.object( + gen, "legacy_paths", return_value=[LegacyArtifact(legacy)] + ): + gen.install_skills([_fake_skill(tmp_path, "api")]) + assert not legacy.exists() + assert (root / "api" / "SKILL.md").is_file() - with patch.object(gen, "get_skill_paths", return_value=[target]): - removed = gen.remove() - assert target in removed - text = target.read_text() - assert "BEGIN deepctl" not in text - assert "before" in text - assert "after" in text + def test_no_generator_still_writes_the_cli_reference_markers(self, tmp_path): + """The markers exist only to be cleaned up, never to be written.""" + skills = [_fake_skill(tmp_path, "api")] + for gen in get_all_generators(): + if gen.skills_root() is None: + continue + root = tmp_path / gen.cli_name + with patch.object(gen, "skills_root", return_value=root): + with patch.object(gen, "legacy_paths", return_value=[]): + gen.install_skills(skills) + for path in root.rglob("*"): + if path.is_file(): + assert "BEGIN deepctl CLI Reference" not in path.read_text() class TestGetAllGenerators: @@ -349,12 +979,417 @@ class TestDetectAiClis: """Test detect_ai_clis.""" def test_returns_only_detected(self): - with patch.object(ClaudeCodeGenerator, "detect", return_value=True), \ - patch.object(CodexGenerator, "detect", return_value=False), \ - patch.object(GeminiGenerator, "detect", return_value=False), \ - patch.object(AmazonQGenerator, "detect", return_value=False), \ - patch.object(CursorGenerator, "detect", return_value=False), \ - patch.object(ClineGenerator, "detect", return_value=False): + with ( + patch.object(ClaudeCodeGenerator, "detect", return_value=True), + patch.object(CodexGenerator, "detect", return_value=False), + patch.object(GeminiGenerator, "detect", return_value=False), + patch.object(AmazonQGenerator, "detect", return_value=False), + patch.object(CursorGenerator, "detect", return_value=False), + patch.object(ClineGenerator, "detect", return_value=False), + ): detected = detect_ai_clis() claude = [g for g in detected if g.cli_name == "claude"] assert len(claude) >= 1 + + +class TestInstallSkillsForKeepsOwnership: + """Every route that writes skill folders shares this one contract. + + `dg skills install` already fetched once, preflighted every + destination and saved after each tool. Login and the plugin refresh + looped over `gen.install()` instead and saved once at the end, so a + failure part-way through left folders on disk with no ownership + record -- exactly the unowned litter the primary flow was redesigned + to prevent. They all call this helper now. + """ + + def _gen(self, tmp_path, cli_name): + gen = ClaudeCodeGenerator() + gen.cli_name = cli_name + gen.display_name = cli_name + root = tmp_path / cli_name / "skills" + root.mkdir(parents=True) + return gen, root + + def _run(self, generators, roots, skills, state, **kwargs): + def skills_root(self, _roots=roots): + return _roots[self.cli_name] + + with ( + patch.object(ClaudeCodeGenerator, "skills_root", skills_root), + patch.object(ClaudeCodeGenerator, "legacy_paths", lambda self: []), + patch.object(skill_generator, "save_skills_state") as save, + patch.object(skill_generator, "fetch_repo_skills", return_value=skills), + ): + # Also hung off the instance, because the tests that matter + # most here run inside pytest.raises and never see a return + # value. Mutating `state` is not the contract -- reaching + # save_skills_state before the exception does is. + self.save = save + report = skill_generator.install_skills_for( + generators, + state, + commands=[_make_command()], + version="9.9.9", + **kwargs, + ) + return report, save + + def test_a_second_tool_failing_leaves_the_first_recorded(self, tmp_path): + """The bug: tool one's folders became litter when tool two raised.""" + first, first_root = self._gen(tmp_path, "claude") + second, second_root = self._gen(tmp_path, "cursor") + roots = {"claude": first_root, "cursor": second_root} + skills = [_fake_skill(tmp_path, n) for n in ("api", "docs")] + state = {"installed_skills": {}} + + real_install = ClaudeCodeGenerator.install_skills + + def install_skills(self, bundle, recorded=()): + if self.cli_name == "cursor": + raise OSError(30, "Read-only file system") + return real_install(self, bundle, recorded) + + with patch.object(ClaudeCodeGenerator, "install_skills", install_skills): + with pytest.raises(OSError): + self._run([first, second], roots, skills, state) + + entry = state["installed_skills"]["claude"] + assert [Path(p).name for p in entry["paths"]] == ["api", "docs"] + assert entry["skills_ref"] == DEFAULT_SKILLS_REF + assert (first_root / "api" / "SKILL.md").is_file() + # And it was written out before cursor raised. Asserting only the + # in-memory dict would still pass with the per-tool save moved + # back after the loop, which is the bug itself. + self.save.assert_called_once_with(state) + # Nothing was written for the tool that failed, so nothing claims + # it was -- but the tool that succeeded stays deepctl's. + assert "cursor" not in state["installed_skills"] + + def test_best_effort_reports_the_failure_instead_of_raising(self, tmp_path): + """Login and the plugin refresh must not fail their own command.""" + first, first_root = self._gen(tmp_path, "claude") + second, second_root = self._gen(tmp_path, "cursor") + roots = {"claude": first_root, "cursor": second_root} + skills = [_fake_skill(tmp_path, "api")] + state = {"installed_skills": {}} + + real_install = ClaudeCodeGenerator.install_skills + + def install_skills(self, bundle, recorded=()): + if self.cli_name == "cursor": + raise OSError(30, "Read-only file system") + return real_install(self, bundle, recorded) + + with patch.object(ClaudeCodeGenerator, "install_skills", install_skills): + report, _ = self._run( + [first, second], roots, skills, state, best_effort=True + ) + + assert [name for name, _ in report.failures] == ["cursor"] + assert list(report.written) == ["claude"] + assert state["installed_skills"]["claude"]["skills"] == ["api"] + assert "cursor" not in state["installed_skills"] + + def test_a_half_written_bundle_is_still_recorded_as_owned(self, tmp_path): + """Whatever landed before the error must stay deepctl's to fix.""" + gen, root = self._gen(tmp_path, "claude") + skills = [_fake_skill(tmp_path, n) for n in ("api", "docs", "cli")] + state = {"installed_skills": {}} + real_copytree = shutil.copytree + + def copytree(src, dst, *args, **kwargs): + if Path(dst).name == "cli": + raise OSError(28, "No space left on device") + return real_copytree(src, dst, *args, **kwargs) + + with patch.object(skill_generator.shutil, "copytree", copytree): + with pytest.raises(OSError): + self._run([gen], {"claude": root}, skills, state) + + entry = state["installed_skills"]["claude"] + assert [Path(p).name for p in entry["paths"]] == ["api", "docs"] + # On disk, not just in the dict: the folders outlive the process + # that wrote them, so the record has to as well. + self.save.assert_called_once_with(state) + + def test_a_collision_in_the_last_tool_writes_nothing_at_all(self, tmp_path): + """Preflight covers every destination before the first byte lands.""" + first, first_root = self._gen(tmp_path, "claude") + second, second_root = self._gen(tmp_path, "cursor") + theirs = second_root / "api" + theirs.mkdir() + (theirs / "SKILL.md").write_text("---\nname: api\n---\n\nmine\n") + skills = [_fake_skill(tmp_path, "api")] + state = {"installed_skills": {}} + + with pytest.raises(SkillOwnershipError): + self._run( + [first, second], + {"claude": first_root, "cursor": second_root}, + skills, + state, + ) + + assert not (first_root / "api").exists() + assert state["installed_skills"] == {} + assert (theirs / "SKILL.md").read_text().endswith("mine\n") + + def test_nothing_installable_means_nothing_downloaded(self, tmp_path): + """A tool with no skills directory must not trigger a download.""" + gen, _ = self._gen(tmp_path, "amazonq") + state = {"installed_skills": {"amazonq": {"paths": []}}} + + with ( + patch.object(ClaudeCodeGenerator, "skills_root", lambda self: None), + patch.object(ClaudeCodeGenerator, "legacy_paths", lambda self: []), + patch.object(skill_generator, "save_skills_state"), + patch.object(skill_generator, "fetch_repo_skills") as fetch, + ): + report = skill_generator.install_skills_for( + [gen], state, commands=[_make_command()], version="9.9.9" + ) + + fetch.assert_not_called() + assert report.unsupported == [gen] + # Nothing was written for it, so nothing may claim it was. + assert state["installed_skills"] == {} + + def test_best_effort_skips_a_conflicting_tool_and_installs_the_rest( + self, tmp_path + ): + """Login and the plugin refresh must not lose every tool to one.""" + first, first_root = self._gen(tmp_path, "claude") + second, second_root = self._gen(tmp_path, "cursor") + theirs = second_root / "api" + theirs.mkdir() + (theirs / "SKILL.md").write_text("---\nname: api\n---\n\nmine\n") + skills = [_fake_skill(tmp_path, "api")] + state = {"installed_skills": {}} + + report, _ = self._run( + [first, second], + {"claude": first_root, "cursor": second_root}, + skills, + state, + best_effort=True, + ) + + assert [name for name, _ in report.conflicts] == ["cursor"] + assert list(report.written) == ["claude"] + assert (first_root / "api" / "SKILL.md").is_file() + assert (theirs / "SKILL.md").read_text().endswith("mine\n") + assert "cursor" not in state["installed_skills"] + + def test_a_late_collision_never_becomes_a_claim_of_ownership(self, tmp_path): + """The refusal must not be recorded as "these folders are ours". + + `install_skills` re-checks its own destinations, so a folder that + appears between the all-tool preflight and the write raises after + the loop has started. Recording what is on disk at that moment + would hand deepctl a claim over the very folder it just refused + to touch, and the next install would delete it. + """ + gen, root = self._gen(tmp_path, "claude") + skills = [_fake_skill(tmp_path, "api")] + state = {"installed_skills": {}} + theirs = root / "api" + + real_conflicts = ClaudeCodeGenerator.install_conflicts + calls = {"n": 0} + + def install_conflicts(self, bundle, recorded=()): + calls["n"] += 1 + if calls["n"] > 1: + # Someone else got there between preflight and write. + theirs.mkdir(exist_ok=True) + (theirs / "SKILL.md").write_text("---\nname: api\n---\n\nmine\n") + return real_conflicts(self, bundle, recorded) + + with patch.object(ClaudeCodeGenerator, "install_conflicts", install_conflicts): + report, _ = self._run( + [gen], {"claude": root}, skills, state, best_effort=True + ) + + assert [name for name, _ in report.failures] == ["claude"] + assert state["installed_skills"] == {} + assert (theirs / "SKILL.md").read_text().endswith("mine\n") + + def test_each_tool_is_reported_as_it_lands_not_after_the_last_one( + self, tmp_path + ): + """A later tool failing must not hide the ones already recorded.""" + first, first_root = self._gen(tmp_path, "claude") + second, second_root = self._gen(tmp_path, "cursor") + skills = [_fake_skill(tmp_path, "api")] + state = {"installed_skills": {}} + announced = [] + + real_install = ClaudeCodeGenerator.install_skills + + def install_skills(self, bundle, recorded=()): + if self.cli_name == "cursor": + raise OSError(30, "Read-only file system") + return real_install(self, bundle, recorded) + + with patch.object(ClaudeCodeGenerator, "install_skills", install_skills): + with pytest.raises(OSError): + self._run( + [first, second], + {"claude": first_root, "cursor": second_root}, + skills, + state, + on_installed=lambda gen, paths: announced.append(gen.cli_name), + ) + + assert announced == ["claude"] + + def test_a_null_installed_skills_does_not_crash_the_install(self, tmp_path): + """A hand-edited skills.json must not take the command down.""" + gen, root = self._gen(tmp_path, "claude") + skills = [_fake_skill(tmp_path, "api")] + state = {"installed_skills": None} + + self._run([gen], {"claude": root}, skills, state) + + assert state["installed_skills"]["claude"]["skills"] == ["api"] + + def test_a_tool_failing_still_retires_the_unsupported_ones(self, tmp_path): + """Cleanup ran after the loop, so a raise part-way skipped it. + + The unsupported tool's stale record then survived an install that + had already written and recorded another tool's folders. + """ + first, first_root = self._gen(tmp_path, "claude") + second, second_root = self._gen(tmp_path, "cursor") + unsupported, _ = self._gen(tmp_path, "amazonq") + roots = {"claude": first_root, "cursor": second_root, "amazonq": None} + skills = [_fake_skill(tmp_path, "api")] + state = {"installed_skills": {"amazonq": {"paths": []}}} + + real_install = ClaudeCodeGenerator.install_skills + + def install_skills(self, bundle, recorded=()): + if self.cli_name == "cursor": + raise OSError(30, "Read-only file system") + return real_install(self, bundle, recorded) + + with patch.object(ClaudeCodeGenerator, "install_skills", install_skills): + with pytest.raises(OSError): + self._run([first, second, unsupported], roots, skills, state) + + assert state["installed_skills"]["claude"]["skills"] == ["api"] + assert "amazonq" not in state["installed_skills"] + + def test_retiring_an_unsupported_tool_is_saved_by_the_core(self, tmp_path): + """The pop is the whole point, so it cannot wait for the caller.""" + unsupported, _ = self._gen(tmp_path, "amazonq") + state = {"installed_skills": {"amazonq": {"paths": []}}} + + with ( + patch.object(ClaudeCodeGenerator, "skills_root", lambda self: None), + patch.object(ClaudeCodeGenerator, "legacy_paths", lambda self: []), + patch.object(skill_generator, "save_skills_state") as save, + patch.object(skill_generator, "fetch_repo_skills") as fetch, + ): + skill_generator.install_skills_for( + [unsupported], state, commands=[_make_command()], version="9.9.9" + ) + + fetch.assert_not_called() + save.assert_called_once() + assert state["installed_skills"] == {} + + def test_a_null_record_for_an_unsupported_tool_is_dropped_and_saved( + self, tmp_path + ): + """pop()'s return cannot tell "absent" from "present but null". + + A hand-edited skills.json holding a null for a tool left the key + gone in memory but the save skipped, so the bad record came back + on the next run and 'dg skills update' kept chasing it. + """ + unsupported, _ = self._gen(tmp_path, "amazonq") + state = {"installed_skills": {"amazonq": None}} + + with ( + patch.object(ClaudeCodeGenerator, "skills_root", lambda self: None), + patch.object(ClaudeCodeGenerator, "legacy_paths", lambda self: []), + patch.object(skill_generator, "save_skills_state") as save, + patch.object(skill_generator, "fetch_repo_skills"), + ): + skill_generator.install_skills_for( + [unsupported], state, commands=[_make_command()], version="9.9.9" + ) + + assert state["installed_skills"] == {} + save.assert_called_once() + + def test_legacy_cleanup_failing_does_not_abort_the_whole_install( + self, tmp_path + ): + """Those files belong to a tool nothing is being installed to. + + Cleanup moved ahead of the writes so no later failure could skip + it; unguarded, an unreadable ~/.gemini/GEMINI.md then took down + an install that was about to write folders for every other tool. + """ + supported, root = self._gen(tmp_path, "claude") + unsupported, _ = self._gen(tmp_path, "amazonq") + skills = [_fake_skill(tmp_path, "api")] + state = {"installed_skills": {}} + + def skills_root(self): + return root if self.cli_name == "claude" else None + + with ( + patch.object(ClaudeCodeGenerator, "skills_root", skills_root), + patch.object(ClaudeCodeGenerator, "legacy_paths", lambda self: []), + patch.object( + unsupported, + "clean_legacy", + side_effect=PermissionError(13, "Permission denied"), + ), + patch.object(skill_generator, "save_skills_state"), + patch.object( + skill_generator, "fetch_repo_skills", return_value=skills + ), + ): + report = skill_generator.install_skills_for( + [supported, unsupported], + state, + commands=[_make_command()], + version="9.9.9", + ) + + assert list(report.written) == ["claude"] + assert (root / "api" / "SKILL.md").is_file() + + def test_a_fetch_failure_leaves_an_unsupported_tool_alone(self, tmp_path): + """Nothing was installed, so nothing of theirs may be cleaned up.""" + supported, root = self._gen(tmp_path, "claude") + unsupported, _ = self._gen(tmp_path, "amazonq") + state = {"installed_skills": {}} + + def skills_root(self): + return root if self.cli_name == "claude" else None + + with ( + patch.object(ClaudeCodeGenerator, "skills_root", skills_root), + patch.object(skill_generator, "save_skills_state"), + patch.object(unsupported, "clean_legacy") as clean, + patch.object( + skill_generator, + "fetch_repo_skills", + side_effect=SkillFetchError("no network"), + ), + ): + with pytest.raises(SkillFetchError): + skill_generator.install_skills_for( + [supported, unsupported], + state, + commands=[_make_command()], + version="9.9.9", + ) + + clean.assert_not_called() diff --git a/tests/e2e/test_skills_install.py b/tests/e2e/test_skills_install.py new file mode 100644 index 0000000..60bf29d --- /dev/null +++ b/tests/e2e/test_skills_install.py @@ -0,0 +1,418 @@ +"""End-to-end install of the real deepgram/skills bundle. + +Runs ``dg skills install`` as a subprocess against a throwaway ``HOME``, +then checks what actually landed on disk: every skill the upstream +manifest lists, as a folder, with its ``references/`` intact and +frontmatter a real YAML parser can read. + +Opt-in, because it reaches the network. Set ``RUN_SKILLS_E2E=1``. +""" + +from __future__ import annotations + +import json +import os +import subprocess +import sys +from pathlib import Path + +import pytest +import yaml + +pytestmark = pytest.mark.skipif( + os.environ.get("RUN_SKILLS_E2E") != "1", + reason="RUN_SKILLS_E2E must be set to 1 (this test downloads deepgram/skills)", +) + +# Every tool deepctl can install skills for, and the user-scope directory +# each one's own documentation names. All six, because a destination that +# nothing exercises end to end is a destination nobody has checked. +EXPECTED_ROOTS = { + "claude": Path(".claude") / "skills", + "codex": Path(".agents") / "skills", + "gemini": Path(".gemini") / "skills", + "cursor": Path(".cursor") / "skills", + "opencode": Path(".config") / "opencode" / "skills", + "cline": Path(".cline") / "skills", +} + +# How `dg skills status` labels each of them. +TOOL_DISPLAY_NAMES = { + "claude": "Claude Code", + "codex": "OpenAI Codex", + "gemini": "Gemini CLI", + "cursor": "Cursor", + "opencode": "OpenCode", + "cline": "Cline", +} + +# The directory whose presence makes each tool "detected". OpenCode and +# Cline are detected by their own config directories, not by the skills +# directory deepctl writes into. +DETECTION_MARKERS = [ + Path(".claude"), + Path(".codex"), + Path(".gemini"), + Path(".cursor"), + Path(".config") / "opencode", + Path(".cline"), +] + +# Directories deepctl <= 0.3.0 wrote, none of which are skills directories. +LEGACY_PATHS = [ + Path(".claude") / "commands" / "deepgram", + Path(".codex") / "instructions.md", + Path(".gemini") / "GEMINI.md", + Path(".cursor") / "rules" / "deepctl.mdc", + Path(".opencode") / "agents.md", + Path(".cline") / "rules" / "deepctl.md", +] + + +def _deepctl_executable() -> Path: + """The installed console script, next to the interpreter running pytest.""" + for name in ("dg", "deepctl", "dg.exe", "deepctl.exe"): + candidate = Path(sys.executable).parent / name + if candidate.exists(): + return candidate + # A failure, not a skip. RUN_SKILLS_E2E=1 is an explicit request to + # run this suite; reporting "skipped" for a missing console script + # tells the person who asked for it that it ran and found nothing + # wrong. Install the package into the interpreter running pytest. + raise AssertionError( + "RUN_SKILLS_E2E=1 was set but no deepctl console script sits " + f"next to {sys.executable}. Install deepctl into this " + "interpreter's environment (e.g. 'uv sync') and run it again." + ) + + +def _run(args: list[str], home: Path) -> subprocess.CompletedProcess[str]: + env = { + **os.environ, + "HOME": str(home), + "USERPROFILE": str(home), + # Never let a developer's real credentials or config leak in. + "DEEPGRAM_API_KEY": "", + "NO_COLOR": "1", + # Rich sizes the status table to the terminal; without this the + # 80-column default truncates the longest skills path and the + # assertions on it fail for reasons that have nothing to do with + # what the command did. + "COLUMNS": "200", + # A developer's own pin must not decide which ref the test asserts. + "DEEPCTL_SKILLS_REF": "", + } + return subprocess.run( + [str(_deepctl_executable()), *args], + capture_output=True, + text=True, + env=env, + timeout=180, + ) + + +@pytest.fixture +def home(tmp_path: Path) -> Path: + """A throwaway HOME with all six tools' marker directories present.""" + fake = tmp_path / "home" + for marker in DETECTION_MARKERS: + (fake / marker).mkdir(parents=True) + return fake + + +@pytest.fixture +def installed(home: Path) -> Path: + result = _run(["skills", "install", "--all"], home) + assert result.returncode == 0, result.stderr + return home + + +def _state(home: Path) -> dict: + return json.loads((home / ".deepctl" / "skills" / "skills.json").read_text()) + + +def _status_counts(result: subprocess.CompletedProcess[str]) -> dict[str, str]: + """The "Deepgram Skills" cell of the `skills status` table, per tool. + + Parses the rendered table rather than searching the whole screen for a + number, so "14" appearing in a path cannot pass for a skill count. + """ + assert result.returncode == 0, result.stderr + counts: dict[str, str] = {} + for line in result.stdout.splitlines(): + cells = [cell.strip() for cell in line.split("│")] + # Rich draws the row as: "" | CLI | Detected | Installed | Dir | "" + if len(cells) != 6: + continue + if cells[1] in TOOL_DISPLAY_NAMES.values(): + counts[cells[1]] = cells[3] + return counts + + +class TestSkillsLandWhereTheToolReadsThem: + def test_every_manifest_skill_is_installed_for_every_tool( + self, installed: Path + ) -> None: + expected = _state(installed)["installed_skills"]["claude"]["skills"] + assert len(expected) == 14, expected + + for cli_name, relative in EXPECTED_ROOTS.items(): + root = installed / relative + assert root.is_dir(), f"{cli_name}: {root} was not created" + found = sorted(p.name for p in root.iterdir() if p.is_dir()) + assert found == sorted(expected), cli_name + + def test_each_skill_is_a_folder_with_a_skill_file(self, installed: Path) -> None: + for relative in EXPECTED_ROOTS.values(): + for skill_dir in (installed / relative).iterdir(): + assert skill_dir.is_dir() + assert (skill_dir / "SKILL.md").is_file() + + def test_reference_subdirectories_survive(self, installed: Path) -> None: + """skills/api and skills/self-hosted each ship a references/ folder.""" + for relative in EXPECTED_ROOTS.values(): + root = installed / relative + for name in ("api", "self-hosted"): + refs = root / name / "references" + assert refs.is_dir(), f"{root / name} lost its references/" + files = [p for p in refs.iterdir() if p.suffix == ".md"] + assert files, f"{refs} is empty" + + def test_frontmatter_parses_and_names_match_their_directories( + self, installed: Path + ) -> None: + for relative in EXPECTED_ROOTS.values(): + for skill_dir in sorted((installed / relative).iterdir()): + text = (skill_dir / "SKILL.md").read_text() + assert text.startswith("---\n"), skill_dir + _, _, rest = text.partition("---\n") + front, sep, _ = rest.partition("\n---") + assert sep, f"{skill_dir}: unterminated frontmatter" + data = yaml.safe_load(front) + assert isinstance(data, dict), skill_dir + assert data.get("name") == skill_dir.name, skill_dir + assert data.get("description"), skill_dir + + def test_nothing_lands_in_the_old_locations(self, installed: Path) -> None: + for relative in LEGACY_PATHS: + assert not (installed / relative).exists(), relative + + def test_skills_are_byte_identical_across_tools(self, installed: Path) -> None: + """Each tool gets the same bundle, not a per-tool rendering of it.""" + roots = [installed / relative for relative in EXPECTED_ROOTS.values()] + reference = roots[0] + for path in reference.rglob("*"): + if not path.is_file(): + continue + relative = path.relative_to(reference) + for other in roots[1:]: + assert (other / relative).read_bytes() == path.read_bytes(), relative + + def test_state_records_the_pinned_ref(self, installed: Path) -> None: + from deepctl_core.skill_bundle import DEFAULT_SKILLS_REF + + state = _state(installed) + for entry in state["installed_skills"].values(): + assert entry["skills_ref"] == DEFAULT_SKILLS_REF + + def test_every_tool_is_recorded_as_installed(self, installed: Path) -> None: + """Each of the six, with the folders it wrote, so remove can undo it.""" + state = _state(installed) + assert set(state["installed_skills"]) == set(EXPECTED_ROOTS) + for cli_name, relative in EXPECTED_ROOTS.items(): + recorded = state["installed_skills"][cli_name]["paths"] + assert len(recorded) == 14, cli_name + assert sorted(recorded) == sorted( + str(p) for p in (installed / relative).iterdir() + ), cli_name + + def test_status_reports_every_tool_as_installed(self, installed: Path) -> None: + result = _run(["skills", "status"], installed) + assert result.returncode == 0, result.stderr + rendered = " ".join(result.stdout.split()) + for relative in EXPECTED_ROOTS.values(): + assert f"~/{relative.as_posix()}" in rendered, relative + counts = _status_counts(result) + assert counts == {name: "14" for name in TOOL_DISPLAY_NAMES.values()} + + +class TestUnrelatedSkillsAreNotDeepctlsToTouch: + """These directories hold other people's skills. deepctl leaves them.""" + + NAME = "my-private-skill" + + def _seed_unrelated(self, home: Path, name: str) -> list[Path]: + seeded = [] + for relative in EXPECTED_ROOTS.values(): + folder = home / relative / name + folder.mkdir(parents=True) + (folder / "SKILL.md").write_text( + f"---\nname: {name}\ndescription: Mine, not Deepgram's.\n---\n" + ) + seeded.append(folder) + return seeded + + def test_remove_preserves_an_unrelated_skill(self, home: Path) -> None: + seeded = self._seed_unrelated(home, self.NAME) + + assert _run(["skills", "install", "--all"], home).returncode == 0 + removed = _run(["skills", "remove", "--all"], home) + assert removed.returncode == 0, removed.stderr + + for folder in seeded: + assert folder.is_dir(), f"{folder} was deleted" + assert "not Deepgram's" in (folder / "SKILL.md").read_text() + for relative in EXPECTED_ROOTS.values(): + remaining = sorted(p.name for p in (home / relative).iterdir()) + assert remaining == [self.NAME], relative + + def test_install_refuses_to_overwrite_an_unrelated_skill(self, home: Path) -> None: + """A folder called `api` that deepctl did not write is not its `api`.""" + seeded = self._seed_unrelated(home, "api") + + result = _run(["skills", "install", "--all"], home) + assert result.returncode != 0 + combined = " ".join((result.stdout + result.stderr).split()) + assert "Refusing to overwrite" in combined + + for folder in seeded: + assert "not Deepgram's" in (folder / "SKILL.md").read_text() + # Nothing was installed anywhere, not even for tools with no clash. + for relative in EXPECTED_ROOTS.values(): + assert sorted(p.name for p in (home / relative).iterdir()) == ["api"] + assert not (home / ".deepctl" / "skills" / "skills.json").exists() + + def test_a_clash_in_one_tool_installs_nothing_for_the_others( + self, home: Path + ) -> None: + """Five clean destinations must not be written when the sixth clashes.""" + clash = home / EXPECTED_ROOTS["cline"] / "api" + clash.mkdir(parents=True) + (clash / "SKILL.md").write_text("---\nname: api\n---\nMine, not Deepgram's.\n") + + result = _run(["skills", "install", "--all"], home) + assert result.returncode != 0 + assert "Refusing to overwrite" in " ".join( + (result.stdout + result.stderr).split() + ) + assert "not Deepgram's" in (clash / "SKILL.md").read_text() + for cli_name, relative in EXPECTED_ROOTS.items(): + if cli_name == "cline": + continue + root = home / relative + assert not root.exists() or not list(root.iterdir()), cli_name + assert not (home / ".deepctl" / "skills" / "skills.json").exists() + + def test_status_does_not_count_an_unrelated_skill(self, home: Path) -> None: + self._seed_unrelated(home, self.NAME) + counts = _status_counts(_run(["skills", "status"], home)) + assert set(counts) == set(TOOL_DISPLAY_NAMES.values()) + for tool, cell in counts.items(): + assert cell == "No", f"{tool} counted a skill it did not install: {cell}" + + +class TestUpgradeFromTheOldLayout: + def test_install_clears_what_deepctl_0_3_0_wrote(self, home: Path) -> None: + legacy_dir = home / ".claude" / "commands" / "deepgram" + legacy_dir.mkdir(parents=True) + (legacy_dir / "api.md").write_text("---\nname: api\n---\n\nstale\n") + + instructions = home / ".codex" / "instructions.md" + instructions.write_text( + "# My own Codex notes\n\n" + "\n" + "four skills concatenated into one blob\n" + "\n" + "# More of my own notes\n" + ) + + # Every remaining artifact 0.3.0 wrote, so each tool's cleanup is + # exercised rather than only Claude Code's and Codex's. + deleted_outright = [] + for relative in ( + Path(".cursor") / "rules" / "deepctl.mdc", + Path(".cline") / "rules" / "deepctl.md", + Path(".amazonq") / "rules" / "deepctl.md", + ): + target = home / relative + target.parent.mkdir(parents=True, exist_ok=True) + target.write_text("stale deepctl rules\n") + deleted_outright.append(target) + + shared = [] + for relative in ( + Path(".gemini") / "GEMINI.md", + Path(".opencode") / "agents.md", + ): + target = home / relative + target.parent.mkdir(parents=True, exist_ok=True) + target.write_text( + "# My own notes\n" + "\n" + "four skills concatenated into one blob\n" + "\n" + ) + shared.append(target) + + result = _run(["skills", "install", "--all"], home) + assert result.returncode == 0, result.stderr + + assert not legacy_dir.exists() + for target in deleted_outright: + assert not target.exists(), target + for target in shared: + text = target.read_text() + assert "BEGIN deepctl" not in text, target + assert "# My own notes" in text, target + remaining = instructions.read_text() + assert "BEGIN deepctl" not in remaining + assert "concatenated into one blob" not in remaining + # The user's own content is not collateral damage. + assert "# My own Codex notes" in remaining + assert "# More of my own notes" in remaining + + def test_cleanup_leaves_a_slash_command_the_user_added(self, home: Path) -> None: + """A slash command is a .md file too, so the cleanup names its files.""" + legacy_dir = home / ".claude" / "commands" / "deepgram" + legacy_dir.mkdir(parents=True) + (legacy_dir / "api.md").write_text("---\nname: api\n---\n\nstale\n") + (legacy_dir / "deploy.md").write_text("my own slash command") + + result = _run(["skills", "install", "--all"], home) + assert result.returncode == 0, result.stderr + + assert not (legacy_dir / "api.md").exists() + assert (legacy_dir / "deploy.md").read_text() == "my own slash command" + + +class TestFailurePathsExitNonZero: + def test_unknown_ref_fails_loudly(self, home: Path) -> None: + result = _run( + ["skills", "install", "--all", "--ref", "no-such-tag-12345"], home + ) + assert result.returncode != 0 + combined = " ".join((result.stdout + result.stderr).split()) + assert "404" in combined or "no ref" in combined + assert not (home / ".claude" / "skills").exists() + + def test_no_network_fails_loudly(self, home: Path) -> None: + env_overrides = { + "https_proxy": "http://127.0.0.1:9", + "HTTPS_PROXY": "http://127.0.0.1:9", + "http_proxy": "http://127.0.0.1:9", + } + previous = {k: os.environ.get(k) for k in env_overrides} + os.environ.update(env_overrides) + try: + result = _run(["skills", "install", "--all"], home) + finally: + for key, value in previous.items(): + if value is None: + os.environ.pop(key, None) + else: + os.environ[key] = value + + assert result.returncode != 0 + combined = " ".join((result.stdout + result.stderr).split()) + assert "No skills were installed" in combined + assert not (home / ".claude" / "skills").exists() diff --git a/web/src/pages/index.astro b/web/src/pages/index.astro index 18748bc..bf4f91d 100644 --- a/web/src/pages/index.astro +++ b/web/src/pages/index.astro @@ -171,9 +171,9 @@ const commandCards: CommandCard[] = [ desc: 'Expose every CLI command to Claude, Cursor, and other MCP clients.', lines: ['dg mcp'] }, { cmd: 'skills', icon: 'robot', color: 'brand', - title: 'Regenerate AI skill files', - desc: 'Keep your coding agent\'s context current with the latest commands.', - lines: ['dg skills update'] }, + title: 'Install Deepgram agent skills', + desc: 'Put the Deepgram skills where Claude Code, Cursor, and the rest read them.', + lines: ['dg skills install'] }, // ── plugins & updates ─────────────────────────────────────────────────── { cmd: 'plugin', icon: 'puzzle', color: 'subtle',