diff --git a/parallel_web_tools/cli/skills.py b/parallel_web_tools/cli/skills.py index 72d4b8a..9589fb8 100644 --- a/parallel_web_tools/cli/skills.py +++ b/parallel_web_tools/cli/skills.py @@ -36,9 +36,29 @@ def skills() -> None: """Install and manage Parallel agent skills. Downloads come from skills.parallel.ai. Set PARALLEL_SKILLS_INDEX_URL to use a custom index. + + Skills install to .agents/skills, and also to .claude/skills when a Claude Code + configuration directory is present, since Claude Code reads only its own directory. """ pass + def print_locations(result: dict) -> None: + locations = result["install_dirs"] + label = "Locations" if len(locations) > 1 else "Location" + console.print(f"{label}: [cyan]{', '.join(locations)}[/cyan]") + + def print_skipped(result: dict) -> None: + skipped = result.get("skipped_skills") or [] + if not skipped: + return + + console.print(f"[yellow]Skipped ({len(skipped)}):[/yellow]") + for entry in skipped: + console.print( + f"[yellow] {entry['skill']} — {entry['install_dir']}/{entry['skill']} already exists " + f"and was not installed by parallel-cli, so it was left untouched[/yellow]" + ) + @skills.command(name="list") @click.option("--json", "output_json", is_flag=True, help="Output as JSON") def skills_list(output_json: bool) -> None: @@ -86,13 +106,13 @@ def skills_install(project: bool, skill_names: tuple[str, ...], output_json: boo SkillsInputError, SkillsInstallLocationError, install_skills, - resolve_install_dir, + resolve_install_dirs, ) try: - install_dir = resolve_install_dir(project=project) + install_dirs = resolve_install_dirs(project=project) result = install_skills( - install_dir=install_dir, + install_dirs=install_dirs, selected_skills=list(skill_names) or None, ) except SkillsInstallLocationError as e: @@ -109,9 +129,11 @@ def skills_install(project: bool, skill_names: tuple[str, ...], output_json: boo return console.print("[bold green]Skills installed[/bold green]") - console.print(f"Location: [cyan]{result['install_dir']}[/cyan]") + print_locations(result) console.print(f"Ref: [cyan]{result['ref']}[/cyan]") console.print(f"Installed ({result['count']}): [cyan]{', '.join(result['installed_skills'])}[/cyan]") + console.print(f"Files written: [cyan]{result['file_count']}[/cyan]") + print_skipped(result) @skills.command(name="uninstall") @click.option( @@ -122,11 +144,11 @@ def skills_install(project: bool, skill_names: tuple[str, ...], output_json: boo @click.option("--json", "output_json", is_flag=True, help="Output as JSON") def skills_uninstall(project: bool, output_json: bool) -> None: """Uninstall skills previously installed by parallel-cli.""" - from parallel_web_tools.core.skills import SkillsInstallLocationError, resolve_install_dir, uninstall_skills + from parallel_web_tools.core.skills import SkillsInstallLocationError, resolve_install_dirs, uninstall_skills try: - install_dir = resolve_install_dir(project=project) - result = uninstall_skills(install_dir=install_dir) + install_dirs = resolve_install_dirs(project=project) + result = uninstall_skills(install_dirs=install_dirs) except SkillsInstallLocationError as e: handle_error(e, output_json=output_json, exit_code=exit_bad_input, prefix="Skills uninstall failed") except Exception as e: @@ -138,11 +160,11 @@ def skills_uninstall(project: bool, output_json: bool) -> None: if result["count"] == 0: console.print("[yellow]No managed skills found to uninstall[/yellow]") - console.print(f"Location: [cyan]{result['install_dir']}[/cyan]") + print_locations(result) return console.print("[bold green]Skills uninstalled[/bold green]") - console.print(f"Location: [cyan]{result['install_dir']}[/cyan]") + print_locations(result) console.print(f"Removed ({result['count']}): [cyan]{', '.join(result['removed_skills'])}[/cyan]") @skills.command(name="reinstall") @@ -169,13 +191,13 @@ def skills_reinstall(project: bool, skill_names: tuple[str, ...], output_json: b SkillsInputError, SkillsInstallLocationError, reinstall_skills, - resolve_install_dir, + resolve_install_dirs, ) try: - install_dir = resolve_install_dir(project=project) + install_dirs = resolve_install_dirs(project=project) result = reinstall_skills( - install_dir=install_dir, + install_dirs=install_dirs, selected_skills=list(skill_names) or None, ) except SkillsInstallLocationError as e: @@ -192,9 +214,11 @@ def skills_reinstall(project: bool, skill_names: tuple[str, ...], output_json: b return console.print("[bold green]Skills reinstalled[/bold green]") - console.print(f"Location: [cyan]{result['install_dir']}[/cyan]") + print_locations(result) console.print(f"Ref: [cyan]{result['ref']}[/cyan]") console.print(f"Removed ({result['removed_count']}): [cyan]{', '.join(result['removed_skills'])}[/cyan]") console.print(f"Installed ({result['installed_count']}): [cyan]{', '.join(result['installed_skills'])}[/cyan]") + console.print(f"Files written: [cyan]{result['file_count']}[/cyan]") + print_skipped(result) return skills diff --git a/parallel_web_tools/core/skills.py b/parallel_web_tools/core/skills.py index 74d58aa..29a293c 100644 --- a/parallel_web_tools/core/skills.py +++ b/parallel_web_tools/core/skills.py @@ -2,13 +2,14 @@ from __future__ import annotations +import hashlib import json import os import shutil import time -from collections.abc import Iterator +from collections.abc import Iterable, Iterator from contextlib import contextmanager -from pathlib import Path +from pathlib import Path, PurePosixPath, PureWindowsPath from typing import Any import httpx @@ -18,6 +19,7 @@ DEFAULT_SKILLS_REPO_REF = "main" SKILLS_REPO_REF_ENV = "PARALLEL_SKILLS_REPO_REF" GLOBAL_SKILLS_DIR_ENV = "PARALLEL_SKILLS_GLOBAL_DIR" +CLAUDE_CONFIG_DIR_ENV = "CLAUDE_CONFIG_DIR" PROJECT_ROOT_MARKERS = (".git", "pyproject.toml", "package.json") MANIFEST_FILE_NAME = ".parallel-cli-skills-manifest.json" @@ -67,6 +69,14 @@ def get_global_skills_dir() -> Path: return Path.home() / ".agents" / "skills" +def get_claude_config_dir() -> Path: + """Return the Claude Code configuration directory.""" + configured = os.environ.get(CLAUDE_CONFIG_DIR_ENV) + if configured and configured.strip(): + return Path(configured.strip()).expanduser() + return Path.home() / ".claude" + + def find_project_root(start: Path | None = None) -> Path | None: """Find a project root by walking upward for known root markers.""" cursor = (start or Path.cwd()).resolve() @@ -78,7 +88,7 @@ def find_project_root(start: Path | None = None) -> Path | None: def resolve_install_dir(project: bool, start: Path | None = None) -> Path: - """Resolve install directory for global or project-local skills.""" + """Resolve the canonical ``.agents/skills`` install directory.""" if not project: return get_global_skills_dir() @@ -91,6 +101,58 @@ def resolve_install_dir(project: bool, start: Path | None = None) -> Path: return root / ".agents" / "skills" +def resolve_install_dirs(project: bool, start: Path | None = None) -> list[Path]: + """Resolve every directory skills should be installed into. + + ``.agents/skills`` is the canonical cross-agent location and always comes first. + Claude Code does not read it — it only discovers skills under ``.claude/skills`` — + so when a Claude Code configuration directory is present we install there too. + Agents that read both (Cursor, for example) de-duplicate by skill name. + + An explicit ``PARALLEL_SKILLS_GLOBAL_DIR`` override targets exactly one directory, + on the assumption that a caller naming a path wants only that path written. + """ + canonical = resolve_install_dir(project=project, start=start) + if not project and os.environ.get(GLOBAL_SKILLS_DIR_ENV): + return [canonical] + + claude_config_dir = canonical.parent.parent / ".claude" if project else get_claude_config_dir() + if not claude_config_dir.is_dir(): + return [canonical] + + return _dedupe_dirs([canonical, claude_config_dir / "skills"]) + + +def _dedupe_dirs(dirs: Iterable[Path]) -> list[Path]: + """Drop directories that resolve to the same location, preserving order. + + A user who symlinked ``~/.claude/skills`` at ``~/.agents/skills`` would otherwise + have the same tree installed, and its manifest rewritten, twice. + """ + seen: set[Path] = set() + unique: list[Path] = [] + for directory in dirs: + resolved = Path(directory).expanduser().resolve() + if resolved in seen: + continue + seen.add(resolved) + unique.append(Path(directory)) + return unique + + +def _normalize_install_dirs(install_dirs: Path | str | Iterable[Path | str]) -> list[Path]: + """Accept a single directory or a collection of them and return a clean list.""" + if isinstance(install_dirs, (str, Path)): + candidates: list[Path | str] = [install_dirs] + else: + candidates = list(install_dirs) + + if not candidates: + raise SkillsInstallLocationError("No skills install directory was provided.") + + return _dedupe_dirs(Path(candidate) for candidate in candidates) + + @contextmanager def _skills_client() -> Iterator[httpx.Client]: with httpx.Client(timeout=30, follow_redirects=True) as client: @@ -140,11 +202,17 @@ def _skills_from_index(index: dict[str, Any]) -> dict[str, dict[str, str]]: if not isinstance(skill_url, str) or not skill_url.strip(): raise SkillsDownloadError(f"Skills index entry '{name}' is missing a valid skill_url") - parsed[name.strip()] = { + entry = { "name": name.strip(), "skill_url": skill_url.strip(), } + manifest_url = raw_skill.get("manifest_url") + if isinstance(manifest_url, str) and manifest_url.strip(): + entry["manifest_url"] = manifest_url.strip() + + parsed[name.strip()] = entry + return parsed @@ -152,7 +220,7 @@ def _list_skills_from_index(index: dict[str, Any]) -> list[str]: return sorted(_skills_from_index(index)) -def _download_skill_markdown(client: httpx.Client, skill_name: str, skill_url: str) -> bytes: +def _download_skill_file(client: httpx.Client, skill_name: str, skill_url: str) -> bytes: response = client.get(skill_url) if response.status_code >= 400: raise SkillsDownloadError( @@ -161,6 +229,101 @@ def _download_skill_markdown(client: httpx.Client, skill_name: str, skill_url: s return response.content +def _safe_skill_file_path(skill_name: str, raw_path: str) -> str: + """Validate a manifest-declared relative path stays inside the skill directory. + + Both path flavours are checked, not just the host's. POSIX rules alone accept + ``C:/outside/payload``, which pathlib then joins on Windows by discarding the + skill directory entirely — so a drive letter has to be rejected here even when + this code is running on Linux or macOS. + """ + candidate = raw_path.strip().replace("\\", "/") + if not candidate: + raise SkillsDownloadError(f"Manifest for skill '{skill_name}' contained an empty file path") + + posix_path = PurePosixPath(candidate) + windows_path = PureWindowsPath(candidate) + unsafe = ( + posix_path.is_absolute() + or windows_path.is_absolute() + or bool(windows_path.drive) + or bool(windows_path.root) + or any(part in ("..", "") for part in posix_path.parts) + ) + if unsafe: + raise SkillsDownloadError(f"Manifest for skill '{skill_name}' contained an unsafe file path: {raw_path}") + + return str(posix_path) + + +def _contained_target(skill_name: str, skill_dir: Path, relative_path: str) -> Path: + """Join relative_path onto skill_dir, refusing anything that lands outside it. + + A backstop for _safe_skill_file_path: string validation has to anticipate every + platform's join semantics, whereas this compares the resolved result. + """ + target = skill_dir / relative_path + resolved_root = skill_dir.resolve() + if not target.resolve().is_relative_to(resolved_root): + raise SkillsDownloadError(f"Manifest for skill '{skill_name}' resolved outside its directory: {relative_path}") + return target + + +def _resolve_skill_files(client: httpx.Client, skill_name: str, entry: dict[str, str]) -> list[dict[str, str]]: + """Resolve every file belonging to a skill. + + Indexes that advertise a ``manifest_url`` carry the full file list (references, + scripts, bundled agents). Older or custom indexes without one fall back to the + single ``SKILL.md`` document. + """ + skill_only = [{"path": "SKILL.md", "url": entry["skill_url"], "sha256": ""}] + + manifest_url = entry.get("manifest_url") + if not manifest_url: + return skill_only + + manifest = _fetch_json(client, manifest_url, f"manifest for skill '{skill_name}'") + raw_files = manifest.get("files") + if not isinstance(raw_files, list) or not raw_files: + return skill_only + + resolved: list[dict[str, str]] = [] + for raw_file in raw_files: + if not isinstance(raw_file, dict): + raise SkillsDownloadError(f"Manifest for skill '{skill_name}' contained an invalid file entry") + + raw_path = raw_file.get("path") + file_url = raw_file.get("url") + if not isinstance(raw_path, str) or not isinstance(file_url, str) or not file_url.strip(): + raise SkillsDownloadError(f"Manifest for skill '{skill_name}' contained a file entry missing path or url") + + checksum = raw_file.get("sha256") + resolved.append( + { + "path": _safe_skill_file_path(skill_name, raw_path), + "url": file_url.strip(), + "sha256": checksum.strip().lower() if isinstance(checksum, str) else "", + } + ) + + if not any(file_entry["path"] == "SKILL.md" for file_entry in resolved): + raise SkillsDownloadError(f"Manifest for skill '{skill_name}' does not include a SKILL.md entry") + + return resolved + + +def _verify_skill_file_checksum(skill_name: str, file_entry: dict[str, str], content: bytes) -> None: + expected = file_entry.get("sha256") + if not expected: + return + + actual = hashlib.sha256(content).hexdigest() + if actual != expected: + raise SkillsDownloadError( + f"Checksum mismatch for '{skill_name}/{file_entry['path']}': expected {expected}, got {actual}" + ) + + def get_remote_skills_channel() -> str: """Return the channel advertised by the remote CDN index.""" with _skills_client() as client: @@ -206,18 +369,31 @@ def _read_manifest(install_dir: Path) -> dict: return data if isinstance(data, dict) else {} +def _managed_skills(install_dir: Path) -> list[str]: + """Return the skill names parallel-cli previously installed into install_dir.""" + managed_raw = _read_manifest(install_dir).get("installed_skills") + if not isinstance(managed_raw, list): + return [] + return [name for name in managed_raw if isinstance(name, str)] + + def install_skills( - install_dir: Path, + install_dirs: Path | str | Iterable[Path | str], selected_skills: list[str] | None = None, ref: str | None = None, ) -> dict: - """Install selected (or all) skills into install_dir. + """Install selected (or all) skills into every directory in install_dirs. Only skills previously managed by parallel-cli are reconciled. Unmanaged skill - directories are left untouched. + directories are left untouched, including one that merely shares a name with a + skill being installed — that one is reported in ``skipped_skills`` instead. Every + file is downloaded once and written to each directory, so a mid-download failure + leaves no location partially installed. """ del ref + targets = _normalize_install_dirs(install_dirs) + with _skills_client() as client: index = _fetch_skills_index(client) resolved_ref = _index_channel(index) @@ -230,76 +406,116 @@ def install_skills( f"Unknown skills requested: {', '.join(missing)}. Available skills: {', '.join(available)}" ) - manifest = _read_manifest(install_dir) - managed_raw = manifest.get("installed_skills") - previously_managed: list[str] = ( - [name for name in managed_raw if isinstance(name, str)] if isinstance(managed_raw, list) else [] - ) - + downloads: dict[str, list[tuple[str, bytes]]] = {} + for skill_name in requested: + skill_files = _resolve_skill_files(client, skill_name, available_skills[skill_name]) + payload: list[tuple[str, bytes]] = [] + for file_entry in skill_files: + content = _download_skill_file(client, skill_name, file_entry["url"]) + _verify_skill_file_checksum(skill_name, file_entry, content) + payload.append((file_entry["path"], content)) + downloads[skill_name] = payload + + file_count = 0 + installed_anywhere: set[str] = set() + skipped: list[dict[str, str]] = [] + for install_dir in targets: + previously_managed = set(_managed_skills(install_dir)) install_dir.mkdir(parents=True, exist_ok=True) for skill_name in previously_managed: if skill_name not in requested: - skill_dir = install_dir / skill_name - if skill_dir.exists() and skill_dir.is_dir(): - shutil.rmtree(skill_dir) - - for skill_name in requested: + stale_dir = install_dir / skill_name + if stale_dir.exists() and stale_dir.is_dir(): + shutil.rmtree(stale_dir) + + # Ownership is decided before anything is written. A directory we did not + # install is someone else's skill: overwriting it would destroy their work, + # and recording it would make uninstall delete it later, so it is left alone + # and kept out of the manifest. + to_install: list[str] = [] + for skill_name in downloads: + skill_dir = install_dir / skill_name + if skill_dir.exists() and skill_name not in previously_managed: + skipped.append({"skill": skill_name, "install_dir": str(install_dir)}) + continue + to_install.append(skill_name) + + # Claim ownership up front. An install killed partway through — Ctrl-C, full + # disk — then leaves directories this CLI still recognizes as its own, so the + # next install replaces them and uninstall can clean them up. Writing the + # manifest afterwards instead would strand those partial trees: absent from + # the manifest, they would be mistaken for a user's work and skipped forever. + _write_manifest(install_dir, resolved_ref, to_install) + + for skill_name in to_install: skill_dir = install_dir / skill_name if skill_dir.exists(): shutil.rmtree(skill_dir) skill_dir.mkdir(parents=True, exist_ok=True) - skill_bytes = _download_skill_markdown(client, skill_name, available_skills[skill_name]["skill_url"]) - (skill_dir / "SKILL.md").write_bytes(skill_bytes) - _write_manifest(install_dir, resolved_ref, requested) + for relative_path, content in downloads[skill_name]: + target = _contained_target(skill_name, skill_dir, relative_path) + target.parent.mkdir(parents=True, exist_ok=True) + target.write_bytes(content) + + installed_anywhere.add(skill_name) + file_count += len(downloads[skill_name]) + return { - "install_dir": str(install_dir), + "install_dirs": [str(directory) for directory in targets], "ref": resolved_ref, - "installed_skills": requested, - "count": len(requested), + "installed_skills": sorted(installed_anywhere), + "count": len(installed_anywhere), + "skipped_skills": skipped, + "file_count": file_count, } -def uninstall_skills(install_dir: Path) -> dict: - """Uninstall only manifest-managed skills from install_dir.""" - manifest = _read_manifest(install_dir) - managed_raw = manifest.get("installed_skills") - managed: list[str] = ( - [name for name in managed_raw if isinstance(name, str)] if isinstance(managed_raw, list) else [] - ) - removed: list[str] = [] +def uninstall_skills(install_dirs: Path | str | Iterable[Path | str]) -> dict: + """Uninstall only manifest-managed skills from every directory in install_dirs.""" + targets = _normalize_install_dirs(install_dirs) + removed: set[str] = set() - for skill_name in managed: - skill_path = install_dir / skill_name - if skill_path.exists() and skill_path.is_dir(): - shutil.rmtree(skill_path) - removed.append(skill_name) + for install_dir in targets: + for skill_name in _managed_skills(install_dir): + skill_path = install_dir / skill_name + if skill_path.exists() and skill_path.is_dir(): + shutil.rmtree(skill_path) + removed.add(skill_name) - manifest_path = _manifest_path(install_dir) - if manifest_path.exists(): - manifest_path.unlink() + manifest_path = _manifest_path(install_dir) + if manifest_path.exists(): + manifest_path.unlink() return { - "install_dir": str(install_dir), + "install_dirs": [str(directory) for directory in targets], "removed_skills": sorted(removed), "count": len(removed), } def reinstall_skills( - install_dir: Path, + install_dirs: Path | str | Iterable[Path | str], selected_skills: list[str] | None = None, ref: str | None = None, ) -> dict: - """Reinstall skills by uninstalling managed set then installing fresh.""" - uninstall_result = uninstall_skills(install_dir) - install_result = install_skills(install_dir, selected_skills=selected_skills, ref=ref) + """Reinstall skills by uninstalling managed set then installing fresh. + + Targets are normalized once up front: a one-shot iterable would otherwise be + consumed by the uninstall, leaving the install with nothing to write after the + existing skills had already been removed. + """ + targets = _normalize_install_dirs(install_dirs) + uninstall_result = uninstall_skills(targets) + install_result = install_skills(targets, selected_skills=selected_skills, ref=ref) return { - "install_dir": install_result["install_dir"], + "install_dirs": install_result["install_dirs"], "ref": install_result["ref"], "removed_skills": uninstall_result["removed_skills"], "installed_skills": install_result["installed_skills"], + "skipped_skills": install_result["skipped_skills"], "removed_count": uninstall_result["count"], "installed_count": install_result["count"], + "file_count": install_result["file_count"], } diff --git a/tests/test_cli.py b/tests/test_cli.py index 78e45d6..f45e606 100644 --- a/tests/test_cli.py +++ b/tests/test_cli.py @@ -2481,12 +2481,12 @@ def test_skills_list_json(self, runner): def test_skills_install_global_default(self, runner): with ( mock.patch( - "parallel_web_tools.core.skills.resolve_install_dir", return_value="/tmp/.agents/skills" + "parallel_web_tools.core.skills.resolve_install_dirs", return_value=["/tmp/.agents/skills"] ) as mock_dir, mock.patch( "parallel_web_tools.core.skills.install_skills", return_value={ - "install_dir": "/tmp/.agents/skills", + "install_dirs": ["/tmp/.agents/skills"], "ref": "main", "installed_skills": ["parallel-web-search"], "count": 1, @@ -2504,12 +2504,12 @@ def test_skills_install_global_default(self, runner): def test_skills_install_project_sets_project_flag(self, runner): with ( mock.patch( - "parallel_web_tools.core.skills.resolve_install_dir", return_value="/repo/.agents/skills" + "parallel_web_tools.core.skills.resolve_install_dirs", return_value=["/repo/.agents/skills"] ) as mock_dir, mock.patch( "parallel_web_tools.core.skills.install_skills", return_value={ - "install_dir": "/repo/.agents/skills", + "install_dirs": ["/repo/.agents/skills"], "ref": "main", "installed_skills": ["parallel-web-search"], "count": 1, @@ -2525,7 +2525,7 @@ def test_skills_install_project_root_not_found(self, runner): from parallel_web_tools.core.skills import SkillsInstallLocationError with mock.patch( - "parallel_web_tools.core.skills.resolve_install_dir", + "parallel_web_tools.core.skills.resolve_install_dirs", side_effect=SkillsInstallLocationError("no project root"), ): result = runner.invoke(main, ["skills", "install", "--project", "--json"]) @@ -2538,7 +2538,7 @@ def test_skills_install_invalid_skill_is_bad_input(self, runner): from parallel_web_tools.core.skills import SkillsInputError with ( - mock.patch("parallel_web_tools.core.skills.resolve_install_dir", return_value="/tmp/.agents/skills"), + mock.patch("parallel_web_tools.core.skills.resolve_install_dirs", return_value=["/tmp/.agents/skills"]), mock.patch( "parallel_web_tools.core.skills.install_skills", side_effect=SkillsInputError("unknown skill"), @@ -2550,13 +2550,36 @@ def test_skills_install_invalid_skill_is_bad_input(self, runner): payload = json.loads(result.output) assert payload["error"]["message"] == "unknown skill" + def test_skills_install_reports_every_location(self, runner): + with ( + mock.patch( + "parallel_web_tools.core.skills.resolve_install_dirs", + return_value=["/tmp/.agents/skills", "/tmp/.claude/skills"], + ), + mock.patch( + "parallel_web_tools.core.skills.install_skills", + return_value={ + "install_dirs": ["/tmp/.agents/skills", "/tmp/.claude/skills"], + "ref": "main", + "installed_skills": ["parallel-web-search"], + "count": 1, + "file_count": 2, + }, + ), + ): + result = runner.invoke(main, ["skills", "install"]) + + assert result.exit_code == 0 + assert "Locations:" in result.output + assert "/tmp/.claude/skills" in result.output + def test_skills_uninstall_json(self, runner): with ( - mock.patch("parallel_web_tools.core.skills.resolve_install_dir", return_value="/tmp/.agents/skills"), + mock.patch("parallel_web_tools.core.skills.resolve_install_dirs", return_value=["/tmp/.agents/skills"]), mock.patch( "parallel_web_tools.core.skills.uninstall_skills", return_value={ - "install_dir": "/tmp/.agents/skills", + "install_dirs": ["/tmp/.agents/skills"], "removed_skills": ["parallel-web-search"], "count": 1, }, @@ -2570,11 +2593,11 @@ def test_skills_uninstall_json(self, runner): def test_skills_reinstall_json(self, runner): with ( - mock.patch("parallel_web_tools.core.skills.resolve_install_dir", return_value="/tmp/.agents/skills"), + mock.patch("parallel_web_tools.core.skills.resolve_install_dirs", return_value=["/tmp/.agents/skills"]), mock.patch( "parallel_web_tools.core.skills.reinstall_skills", return_value={ - "install_dir": "/tmp/.agents/skills", + "install_dirs": ["/tmp/.agents/skills"], "ref": "main", "removed_skills": ["parallel-web-search"], "installed_skills": ["parallel-web-extract"], diff --git a/tests/test_skills.py b/tests/test_skills.py index 9f25383..b124a09 100644 --- a/tests/test_skills.py +++ b/tests/test_skills.py @@ -1,5 +1,6 @@ """Tests for skills helper module.""" +import hashlib import json from contextlib import contextmanager @@ -59,6 +60,78 @@ def test_project_fails_without_root_markers(self, tmp_path): skills.resolve_install_dir(project=True, start=start) +class TestResolveInstallDirs: + def test_global_adds_claude_dir_when_present(self, monkeypatch, tmp_path): + monkeypatch.delenv(skills.GLOBAL_SKILLS_DIR_ENV, raising=False) + monkeypatch.delenv(skills.CLAUDE_CONFIG_DIR_ENV, raising=False) + monkeypatch.setattr("parallel_web_tools.core.skills.Path.home", lambda: tmp_path) + (tmp_path / ".claude").mkdir() + + assert skills.resolve_install_dirs(project=False) == [ + tmp_path / ".agents" / "skills", + tmp_path / ".claude" / "skills", + ] + + def test_global_skips_claude_dir_when_absent(self, monkeypatch, tmp_path): + monkeypatch.delenv(skills.GLOBAL_SKILLS_DIR_ENV, raising=False) + monkeypatch.delenv(skills.CLAUDE_CONFIG_DIR_ENV, raising=False) + monkeypatch.setattr("parallel_web_tools.core.skills.Path.home", lambda: tmp_path) + + assert skills.resolve_install_dirs(project=False) == [tmp_path / ".agents" / "skills"] + + def test_global_honors_claude_config_dir_env(self, monkeypatch, tmp_path): + monkeypatch.delenv(skills.GLOBAL_SKILLS_DIR_ENV, raising=False) + monkeypatch.setattr("parallel_web_tools.core.skills.Path.home", lambda: tmp_path) + custom_claude = tmp_path / "elsewhere" / "claude-config" + custom_claude.mkdir(parents=True) + monkeypatch.setenv(skills.CLAUDE_CONFIG_DIR_ENV, str(custom_claude)) + + assert skills.resolve_install_dirs(project=False) == [ + tmp_path / ".agents" / "skills", + custom_claude / "skills", + ] + + def test_global_env_override_targets_single_dir(self, monkeypatch, tmp_path): + monkeypatch.setattr("parallel_web_tools.core.skills.Path.home", lambda: tmp_path) + (tmp_path / ".claude").mkdir() + monkeypatch.setenv(skills.GLOBAL_SKILLS_DIR_ENV, str(tmp_path / "custom-skills")) + + assert skills.resolve_install_dirs(project=False) == [tmp_path / "custom-skills"] + + def test_project_adds_claude_dir_when_present(self, tmp_path): + project_root = tmp_path / "repo" + nested = project_root / "src" / "module" + nested.mkdir(parents=True) + (project_root / "pyproject.toml").write_text("[project]\nname='x'\n") + (project_root / ".claude").mkdir() + + assert skills.resolve_install_dirs(project=True, start=nested) == [ + project_root / ".agents" / "skills", + project_root / ".claude" / "skills", + ] + + def test_project_ignores_claude_config_dir_env(self, monkeypatch, tmp_path): + project_root = tmp_path / "repo" + project_root.mkdir() + (project_root / "pyproject.toml").write_text("[project]\nname='x'\n") + global_claude = tmp_path / "home" / ".claude" + global_claude.mkdir(parents=True) + monkeypatch.setenv(skills.CLAUDE_CONFIG_DIR_ENV, str(global_claude)) + + assert skills.resolve_install_dirs(project=True, start=project_root) == [project_root / ".agents" / "skills"] + + def test_symlinked_claude_dir_is_deduped(self, monkeypatch, tmp_path): + monkeypatch.delenv(skills.GLOBAL_SKILLS_DIR_ENV, raising=False) + monkeypatch.delenv(skills.CLAUDE_CONFIG_DIR_ENV, raising=False) + monkeypatch.setattr("parallel_web_tools.core.skills.Path.home", lambda: tmp_path) + agents_skills = tmp_path / ".agents" / "skills" + agents_skills.mkdir(parents=True) + (tmp_path / ".claude").mkdir() + (tmp_path / ".claude" / "skills").symlink_to(agents_skills, target_is_directory=True) + + assert skills.resolve_install_dirs(project=False) == [agents_skills] + + def _make_index() -> dict: return { "channel": "main", @@ -96,14 +169,14 @@ def test_list_remote_skills_ignores_ref_override(self, monkeypatch): def test_install_skills_from_index(self, monkeypatch, tmp_path): install_dir = tmp_path / "install" - def fake_download_skill_markdown(client, skill_name: str, skill_url: str) -> bytes: + def fake_download_skill_file(client, skill_name: str, skill_url: str) -> bytes: assert skill_name == "parallel-web-search" assert skill_url.endswith("/parallel-web-search/SKILL.md") return b"search" monkeypatch.setattr(skills, "_skills_client", _fake_skills_client) monkeypatch.setattr(skills, "_fetch_skills_index", lambda client: _make_index()) - monkeypatch.setattr(skills, "_download_skill_markdown", fake_download_skill_markdown) + monkeypatch.setattr(skills, "_download_skill_file", fake_download_skill_file) result = skills.install_skills(install_dir, selected_skills=["parallel-web-search"], ref="main") @@ -115,12 +188,12 @@ def fake_download_skill_markdown(client, skill_name: str, skill_url: str) -> byt def test_install_subset_removes_previously_managed_skills(self, monkeypatch, tmp_path): install_dir = tmp_path / "install" - def fake_download_skill_markdown(client, skill_name: str, skill_url: str) -> bytes: + def fake_download_skill_file(client, skill_name: str, skill_url: str) -> bytes: return skill_name.encode() monkeypatch.setattr(skills, "_skills_client", _fake_skills_client) monkeypatch.setattr(skills, "_fetch_skills_index", lambda client: _make_index()) - monkeypatch.setattr(skills, "_download_skill_markdown", fake_download_skill_markdown) + monkeypatch.setattr(skills, "_download_skill_file", fake_download_skill_file) skills.install_skills(install_dir, ref="main") skills.install_skills(install_dir, selected_skills=["parallel-web-search"], ref="main") @@ -145,13 +218,249 @@ def test_install_skills_ignores_ref_override(self, monkeypatch, tmp_path): monkeypatch.setattr(skills, "_skills_client", _fake_skills_client) monkeypatch.setattr(skills, "_fetch_skills_index", lambda client: _make_index()) - monkeypatch.setattr(skills, "_download_skill_markdown", lambda client, skill_name, skill_url: b"search") + monkeypatch.setattr(skills, "_download_skill_file", lambda client, skill_name, skill_url: b"search") result = skills.install_skills(install_dir, selected_skills=["parallel-web-search"], ref="feature/test-branch") assert result["ref"] == "main" +class TestMultiTargetInstall: + def _patch(self, monkeypatch) -> None: + monkeypatch.setattr(skills, "_skills_client", _fake_skills_client) + monkeypatch.setattr(skills, "_fetch_skills_index", lambda client: _make_index()) + monkeypatch.setattr(skills, "_download_skill_file", lambda client, skill_name, url: skill_name.encode()) + + def test_install_writes_every_target(self, monkeypatch, tmp_path): + self._patch(monkeypatch) + agents_dir = tmp_path / ".agents" / "skills" + claude_dir = tmp_path / ".claude" / "skills" + + result = skills.install_skills([agents_dir, claude_dir], selected_skills=["parallel-web-search"]) + + assert result["install_dirs"] == [str(agents_dir), str(claude_dir)] + assert result["file_count"] == 2 + for install_dir in (agents_dir, claude_dir): + assert (install_dir / "parallel-web-search" / "SKILL.md").read_bytes() == b"parallel-web-search" + assert (install_dir / skills.MANIFEST_FILE_NAME).exists() + + def test_install_downloads_each_file_once(self, monkeypatch, tmp_path): + downloaded: list[str] = [] + + monkeypatch.setattr(skills, "_skills_client", _fake_skills_client) + monkeypatch.setattr(skills, "_fetch_skills_index", lambda client: _make_index()) + monkeypatch.setattr( + skills, + "_download_skill_file", + lambda client, skill_name, url: (downloaded.append(url), skill_name.encode())[1], + ) + + skills.install_skills([tmp_path / "a", tmp_path / "b", tmp_path / "c"], selected_skills=["parallel-web-search"]) + + assert len(downloaded) == 1 + + def test_failed_download_leaves_no_target_written(self, monkeypatch, tmp_path): + monkeypatch.setattr(skills, "_skills_client", _fake_skills_client) + monkeypatch.setattr(skills, "_fetch_skills_index", lambda client: _make_index()) + + def explode(client, skill_name, url): + raise skills.SkillsDownloadError("network down") + + monkeypatch.setattr(skills, "_download_skill_file", explode) + agents_dir = tmp_path / ".agents" / "skills" + + with pytest.raises(skills.SkillsDownloadError): + skills.install_skills([agents_dir, tmp_path / ".claude" / "skills"]) + + assert not agents_dir.exists() + + def test_uninstall_clears_every_target(self, monkeypatch, tmp_path): + self._patch(monkeypatch) + agents_dir = tmp_path / ".agents" / "skills" + claude_dir = tmp_path / ".claude" / "skills" + skills.install_skills([agents_dir, claude_dir], selected_skills=["parallel-web-search"]) + + result = skills.uninstall_skills([agents_dir, claude_dir]) + + assert result["removed_skills"] == ["parallel-web-search"] + assert result["count"] == 1 + for install_dir in (agents_dir, claude_dir): + assert not (install_dir / "parallel-web-search").exists() + assert not (install_dir / skills.MANIFEST_FILE_NAME).exists() + + def test_install_prunes_dropped_skills_from_every_target(self, monkeypatch, tmp_path): + self._patch(monkeypatch) + targets = [tmp_path / ".agents" / "skills", tmp_path / ".claude" / "skills"] + skills.install_skills(targets) + skills.install_skills(targets, selected_skills=["parallel-web-search"]) + + for install_dir in targets: + assert (install_dir / "parallel-web-search").exists() + assert not (install_dir / "parallel-web-extract").exists() + + def test_duplicate_targets_are_written_once(self, monkeypatch, tmp_path): + self._patch(monkeypatch) + install_dir = tmp_path / ".agents" / "skills" + + result = skills.install_skills([install_dir, install_dir], selected_skills=["parallel-web-search"]) + + assert result["install_dirs"] == [str(install_dir)] + assert result["file_count"] == 1 + + def test_empty_target_list_is_rejected(self, tmp_path): + with pytest.raises(skills.SkillsInstallLocationError): + skills.install_skills([]) + + +class TestCollisionGuard: + def _patch(self, monkeypatch) -> None: + monkeypatch.setattr(skills, "_skills_client", _fake_skills_client) + monkeypatch.setattr(skills, "_fetch_skills_index", lambda client: _make_index()) + monkeypatch.setattr(skills, "_download_skill_file", lambda client, skill_name, url: skill_name.encode()) + + def test_install_leaves_unmanaged_same_named_skill_untouched(self, monkeypatch, tmp_path): + self._patch(monkeypatch) + install_dir = tmp_path / ".claude" / "skills" + mine = install_dir / "parallel-web-search" + mine.mkdir(parents=True) + (mine / "SKILL.md").write_text("hand written") + + result = skills.install_skills([install_dir], selected_skills=["parallel-web-search"]) + + assert (mine / "SKILL.md").read_text() == "hand written" + assert result["installed_skills"] == [] + assert result["count"] == 0 + assert result["file_count"] == 0 + assert result["skipped_skills"] == [{"skill": "parallel-web-search", "install_dir": str(install_dir)}] + + def test_skipped_skill_is_absent_from_manifest_and_survives_uninstall(self, monkeypatch, tmp_path): + self._patch(monkeypatch) + install_dir = tmp_path / ".claude" / "skills" + mine = install_dir / "parallel-web-search" + mine.mkdir(parents=True) + (mine / "SKILL.md").write_text("hand written") + + skills.install_skills([install_dir]) + manifest = json.loads((install_dir / skills.MANIFEST_FILE_NAME).read_text()) + assert "parallel-web-search" not in manifest["installed_skills"] + assert "parallel-web-extract" in manifest["installed_skills"] + + result = skills.uninstall_skills([install_dir]) + + assert (mine / "SKILL.md").read_text() == "hand written" + assert result["removed_skills"] == ["parallel-web-extract"] + + def test_our_own_skill_is_still_replaced_on_reinstall(self, monkeypatch, tmp_path): + self._patch(monkeypatch) + install_dir = tmp_path / ".agents" / "skills" + skills.install_skills([install_dir], selected_skills=["parallel-web-search"]) + (install_dir / "parallel-web-search" / "stale.md").write_text("stale") + + result = skills.install_skills([install_dir], selected_skills=["parallel-web-search"]) + + assert result["installed_skills"] == ["parallel-web-search"] + assert result["skipped_skills"] == [] + assert not (install_dir / "parallel-web-search" / "stale.md").exists() + + def test_skip_is_per_directory(self, monkeypatch, tmp_path): + self._patch(monkeypatch) + agents_dir = tmp_path / ".agents" / "skills" + claude_dir = tmp_path / ".claude" / "skills" + mine = claude_dir / "parallel-web-search" + mine.mkdir(parents=True) + (mine / "SKILL.md").write_text("hand written") + + result = skills.install_skills([agents_dir, claude_dir], selected_skills=["parallel-web-search"]) + + assert (agents_dir / "parallel-web-search" / "SKILL.md").read_bytes() == b"parallel-web-search" + assert (mine / "SKILL.md").read_text() == "hand written" + assert result["installed_skills"] == ["parallel-web-search"] + assert result["skipped_skills"] == [{"skill": "parallel-web-search", "install_dir": str(claude_dir)}] + + def test_reinstall_accepts_a_one_shot_iterable(self, monkeypatch, tmp_path): + self._patch(monkeypatch) + install_dir = tmp_path / ".agents" / "skills" + skills.install_skills([install_dir], selected_skills=["parallel-web-search"]) + + result = skills.reinstall_skills((path for path in [install_dir]), selected_skills=["parallel-web-search"]) + + assert result["removed_skills"] == ["parallel-web-search"] + assert result["installed_skills"] == ["parallel-web-search"] + assert (install_dir / "parallel-web-search" / "SKILL.md").exists() + + +class TestInterruptedInstall: + def _patch(self, monkeypatch, doomed: str = "") -> None: + """Patch the CDN, and make writing the doomed skill fail like a full disk would.""" + real = skills._contained_target + + def flaky(skill_name, skill_dir, relative_path): + if skill_name == doomed: + raise RuntimeError("disk full") + return real(skill_name, skill_dir, relative_path) + + monkeypatch.setattr(skills, "_skills_client", _fake_skills_client) + monkeypatch.setattr(skills, "_fetch_skills_index", lambda client: _make_index()) + monkeypatch.setattr(skills, "_download_skill_file", lambda client, name, url: name.encode()) + monkeypatch.setattr(skills, "_contained_target", flaky) + + def test_crashed_install_is_repaired_rather_than_skipped(self, monkeypatch, tmp_path): + install_dir = tmp_path / ".claude" / "skills" + self._patch(monkeypatch, doomed="parallel-web-search") + with pytest.raises(RuntimeError, match="disk full"): + skills.install_skills([install_dir]) + + monkeypatch.undo() + self._patch(monkeypatch) + result = skills.install_skills([install_dir]) + + assert result["skipped_skills"] == [] + assert (install_dir / "parallel-web-search" / "SKILL.md").read_bytes() == b"parallel-web-search" + + def test_crashed_install_does_not_claim_a_users_skill(self, monkeypatch, tmp_path): + install_dir = tmp_path / ".claude" / "skills" + mine = install_dir / "parallel-web-extract" + mine.mkdir(parents=True) + (mine / "SKILL.md").write_text("hand written") + self._patch(monkeypatch, doomed="parallel-web-search") + + with pytest.raises(RuntimeError, match="disk full"): + skills.install_skills([install_dir]) + + manifest = json.loads((install_dir / skills.MANIFEST_FILE_NAME).read_text()) + assert manifest["installed_skills"] == ["parallel-web-search"] + assert (mine / "SKILL.md").read_text() == "hand written" + + +class TestUnsafeManifestPaths: + @pytest.mark.parametrize( + "raw_path", + ["C:/outside/payload", "C:\\outside\\payload", "/etc/passwd", "../escape.md", "a/../../escape.md"], + ) + def test_rejects_paths_that_escape_the_skill_directory(self, raw_path): + with pytest.raises(skills.SkillsDownloadError, match="unsafe file path"): + skills._safe_skill_file_path("migrate-to-parallel", raw_path) + + @pytest.mark.parametrize("raw_path", ["SKILL.md", "references/exa.md", "scripts/scan.py"]) + def test_accepts_ordinary_relative_paths(self, raw_path): + assert skills._safe_skill_file_path("migrate-to-parallel", raw_path) == raw_path + + def test_contained_target_rejects_escapes_that_slip_past_validation(self, tmp_path): + skill_dir = tmp_path / "skills" / "migrate-to-parallel" + skill_dir.mkdir(parents=True) + + with pytest.raises(skills.SkillsDownloadError, match="resolved outside its directory"): + skills._contained_target("migrate-to-parallel", skill_dir, "../../escaped.md") + + def test_contained_target_allows_nested_paths(self, tmp_path): + skill_dir = tmp_path / "skills" / "migrate-to-parallel" + skill_dir.mkdir(parents=True) + + assert skills._contained_target("migrate-to-parallel", skill_dir, "references/exa.md") == ( + skill_dir / "references" / "exa.md" + ) + + class TestRemoteChannel: def test_get_remote_skills_channel(self, monkeypatch): monkeypatch.setattr(skills, "_skills_client", _fake_skills_client) @@ -183,3 +492,100 @@ def test_uninstall_only_removes_manifest_managed_skills(self, tmp_path): assert not managed.exists() assert unmanaged.exists() assert not (install_dir / skills.MANIFEST_FILE_NAME).exists() + + +def _make_manifest_index() -> dict: + return { + "channel": "main", + "skills": [ + { + "name": "migrate-to-parallel", + "skill_url": "https://skills.parallel.ai/migrate-to-parallel/SKILL.md", + "manifest_url": "https://skills.parallel.ai/migrate-to-parallel/manifest.json", + }, + ], + } + + +def _make_manifest(files: list[dict]) -> dict: + return {"schema_version": 1, "name": "migrate-to-parallel", "files": files} + + +def _file_entry(path: str, content: bytes) -> dict: + return { + "path": path, + "url": f"https://skills.parallel.ai/migrate-to-parallel/{path}", + "sha256": hashlib.sha256(content).hexdigest(), + "size": len(content), + } + + +class TestManifestInstall: + def _patch(self, monkeypatch, manifest: dict, contents: dict[str, bytes]) -> None: + monkeypatch.setattr(skills, "_skills_client", _fake_skills_client) + monkeypatch.setattr(skills, "_fetch_skills_index", lambda client: _make_manifest_index()) + monkeypatch.setattr(skills, "_fetch_json", lambda client, url, description: manifest) + monkeypatch.setattr( + skills, + "_download_skill_file", + lambda client, skill_name, url: contents[url.rsplit("/migrate-to-parallel/", 1)[1]], + ) + + def test_install_downloads_every_manifest_file(self, monkeypatch, tmp_path): + install_dir = tmp_path / "install" + contents = { + "SKILL.md": b"skill body", + "references/exa.md": b"exa reference", + "scripts/scan_provider_usage.py": b"print('scan')", + "agents/openai.yaml": b"name: openai", + } + manifest = _make_manifest([_file_entry(path, body) for path, body in contents.items()]) + self._patch(monkeypatch, manifest, contents) + + result = skills.install_skills(install_dir) + + skill_dir = install_dir / "migrate-to-parallel" + assert result["file_count"] == 4 + assert (skill_dir / "SKILL.md").read_bytes() == b"skill body" + assert (skill_dir / "references" / "exa.md").read_bytes() == b"exa reference" + assert (skill_dir / "scripts" / "scan_provider_usage.py").read_bytes() == b"print('scan')" + assert (skill_dir / "agents" / "openai.yaml").read_bytes() == b"name: openai" + + def test_install_rejects_checksum_mismatch(self, monkeypatch, tmp_path): + contents = {"SKILL.md": b"skill body"} + manifest = _make_manifest([_file_entry("SKILL.md", b"different bytes")]) + self._patch(monkeypatch, manifest, contents) + + with pytest.raises(skills.SkillsDownloadError, match="Checksum mismatch"): + skills.install_skills(tmp_path / "install") + + def test_install_rejects_path_traversal(self, monkeypatch, tmp_path): + contents = {"SKILL.md": b"skill body"} + manifest = _make_manifest( + [ + _file_entry("SKILL.md", b"skill body"), + {"path": "../../escaped.md", "url": "https://skills.parallel.ai/x/escaped.md", "sha256": ""}, + ] + ) + self._patch(monkeypatch, manifest, contents) + + with pytest.raises(skills.SkillsDownloadError, match="unsafe file path"): + skills.install_skills(tmp_path / "install") + + def test_install_rejects_manifest_without_skill_md(self, monkeypatch, tmp_path): + contents = {"references/exa.md": b"exa reference"} + manifest = _make_manifest([_file_entry("references/exa.md", b"exa reference")]) + self._patch(monkeypatch, manifest, contents) + + with pytest.raises(skills.SkillsDownloadError, match="does not include a SKILL.md"): + skills.install_skills(tmp_path / "install") + + def test_install_falls_back_when_manifest_has_no_files(self, monkeypatch, tmp_path): + install_dir = tmp_path / "install" + contents = {"SKILL.md": b"skill body"} + self._patch(monkeypatch, _make_manifest([]), contents) + + result = skills.install_skills(install_dir) + + assert result["file_count"] == 1 + assert (install_dir / "migrate-to-parallel" / "SKILL.md").read_bytes() == b"skill body" diff --git a/uv.lock b/uv.lock index da8fd05..87823d9 100644 --- a/uv.lock +++ b/uv.lock @@ -1674,7 +1674,7 @@ wheels = [ [[package]] name = "parallel-web-tools" -version = "0.7.1" +version = "0.8.1" source = { editable = "." } dependencies = [ { name = "click" },