From 2dcca9877d763e2b12542b615df3967f373c7ddc Mon Sep 17 00:00:00 2001 From: Zhengchao Liu Date: Mon, 10 Aug 2026 15:05:02 -0700 Subject: [PATCH 1/5] fix: install every bundled skill file, not just SKILL.md `parallel-cli skills install` only ever downloaded SKILL.md, so any skill shipping bundled resources landed incomplete. `migrate-to-parallel` installed 1 of its 10 files, leaving its SKILL.md pointing at 7 reference documents and a scan script that were never fetched. The CDN already published everything needed: each index entry carries a manifest_url, and the per-skill manifest lists every file with a URL, sha256, and size. _skills_from_index discarded manifest_url and the install loop hard coded a single SKILL.md write. Resolve each skill's full file list from its manifest and write every entry, preserving relative paths. Verify sha256 when the manifest supplies one, and reject absolute or parent-traversing paths so a manifest cannot write outside the skill directory. Indexes without a manifest_url, and manifests with an empty file list, keep the previous SKILL.md-only behavior, so custom indexes set via PARALLEL_SKILLS_INDEX_URL still install. Install and reinstall now report the file count so a truncated install is visible instead of silent. Verified against the live CDN: all 11 skills install to 20 files, byte identical to the 0.8.0 plugin release. Co-Authored-By: Claude Opus 5 (1M context) --- parallel_web_tools/cli/skills.py | 2 + parallel_web_tools/core/skills.py | 97 +++++++++++++++++++++++++-- tests/test_skills.py | 108 ++++++++++++++++++++++++++++-- 3 files changed, 197 insertions(+), 10 deletions(-) diff --git a/parallel_web_tools/cli/skills.py b/parallel_web_tools/cli/skills.py index 72d4b8a..b9f3936 100644 --- a/parallel_web_tools/cli/skills.py +++ b/parallel_web_tools/cli/skills.py @@ -112,6 +112,7 @@ def skills_install(project: bool, skill_names: tuple[str, ...], output_json: boo console.print(f"Location: [cyan]{result['install_dir']}[/cyan]") 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]") @skills.command(name="uninstall") @click.option( @@ -196,5 +197,6 @@ def skills_reinstall(project: bool, skill_names: tuple[str, ...], output_json: b 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]") return skills diff --git a/parallel_web_tools/core/skills.py b/parallel_web_tools/core/skills.py index 74d58aa..463f7a8 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 contextlib import contextmanager -from pathlib import Path +from pathlib import Path, PurePosixPath from typing import Any import httpx @@ -140,11 +141,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 +159,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 +168,74 @@ 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.""" + 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) + if posix_path.is_absolute() or any(part in ("..", "") for part in posix_path.parts): + raise SkillsDownloadError(f"Manifest for skill '{skill_name}' contained an unsafe file path: {raw_path}") + + return str(posix_path) + + +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: @@ -244,13 +319,23 @@ def install_skills( if skill_dir.exists() and skill_dir.is_dir(): shutil.rmtree(skill_dir) + file_count = 0 for skill_name in requested: + skill_files = _resolve_skill_files(client, skill_name, available_skills[skill_name]) + 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) + + 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) + target = skill_dir / file_entry["path"] + target.parent.mkdir(parents=True, exist_ok=True) + target.write_bytes(content) + + file_count += len(skill_files) _write_manifest(install_dir, resolved_ref, requested) return { @@ -258,6 +343,7 @@ def install_skills( "ref": resolved_ref, "installed_skills": requested, "count": len(requested), + "file_count": file_count, } @@ -302,4 +388,5 @@ def reinstall_skills( "installed_skills": install_result["installed_skills"], "removed_count": uninstall_result["count"], "installed_count": install_result["count"], + "file_count": install_result["file_count"], } diff --git a/tests/test_skills.py b/tests/test_skills.py index 9f25383..d04c5f4 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 @@ -96,14 +97,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 +116,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,7 +146,7 @@ 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") @@ -183,3 +184,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" From c229e6ff9a3e6c52db32931e08aef4b93c4fc053 Mon Sep 17 00:00:00 2001 From: Zhengchao Liu Date: Mon, 10 Aug 2026 17:27:12 -0700 Subject: [PATCH 2/5] feat: install skills where Claude Code can find them MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `parallel-cli skills install` wrote only to `.agents/skills`. Claude Code does not read that path — it discovers skills under `.claude/skills`, plugin directories, and managed policy dirs, and nothing else. Every skill installed by this CLI was therefore invisible to Claude Code, silently: the install reported success and wrote a complete, correct tree the agent never loaded. `.agents/skills` stays canonical and stays first. When a Claude Code configuration directory is present — `CLAUDE_CONFIG_DIR` or `~/.claude` for a global install, `/.claude` for `--project` — the same skills are written to its `skills/` subdirectory too. Absence of that directory means no Claude Code on this machine, so nothing extra is created. An explicit `PARALLEL_SKILLS_GLOBAL_DIR` still targets exactly one directory. install/uninstall/reinstall now operate over a list of directories. Files are downloaded once and written to each target, so a mid-download failure leaves no location half-installed, and targets that resolve to the same path are written once — a symlinked `~/.claude/skills` would otherwise have its tree and manifest rewritten twice. Each directory carries its own manifest, so uninstall reconciles them independently and still leaves unmanaged skills alone. Results report `install_dirs`. The former singular `install_dir` is dropped rather than kept as an alias: it would always be `install_dirs[0]`, so any caller reading it would silently miss the Claude Code location. Nothing in either repo consumed it. Verified against the live CDN in a sandboxed HOME: with `.claude` present, all 11 skills install to both locations, 21 files each, byte-identical trees; uninstall clears both; without `.claude`, only `.agents/skills` is written and no `.claude` directory is created. A fresh Claude Code session loads all 11. Co-Authored-By: Claude Opus 5 (1M context) --- parallel_web_tools/cli/skills.py | 34 ++++--- parallel_web_tools/core/skills.py | 162 +++++++++++++++++++++--------- tests/test_cli.py | 43 ++++++-- tests/test_skills.py | 159 +++++++++++++++++++++++++++++ 4 files changed, 330 insertions(+), 68 deletions(-) diff --git a/parallel_web_tools/cli/skills.py b/parallel_web_tools/cli/skills.py index b9f3936..d7fa2dd 100644 --- a/parallel_web_tools/cli/skills.py +++ b/parallel_web_tools/cli/skills.py @@ -36,9 +36,17 @@ 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]") + @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 +94,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,7 +117,7 @@ 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]") @@ -123,11 +131,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: @@ -139,11 +147,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") @@ -170,13 +178,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: @@ -193,7 +201,7 @@ 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]") diff --git a/parallel_web_tools/core/skills.py b/parallel_web_tools/core/skills.py index 463f7a8..6212909 100644 --- a/parallel_web_tools/core/skills.py +++ b/parallel_web_tools/core/skills.py @@ -7,7 +7,7 @@ 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, PurePosixPath from typing import Any @@ -19,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" @@ -68,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() @@ -79,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() @@ -92,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: @@ -281,18 +342,29 @@ 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. 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) @@ -305,41 +377,44 @@ 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 + for install_dir in targets: + previously_managed = _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) - - file_count = 0 - for skill_name in requested: - skill_files = _resolve_skill_files(client, skill_name, available_skills[skill_name]) + stale_dir = install_dir / skill_name + if stale_dir.exists() and stale_dir.is_dir(): + shutil.rmtree(stale_dir) + for skill_name, payload in downloads.items(): skill_dir = install_dir / skill_name if skill_dir.exists(): shutil.rmtree(skill_dir) skill_dir.mkdir(parents=True, exist_ok=True) - 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) - target = skill_dir / file_entry["path"] + for relative_path, content in payload: + target = skill_dir / relative_path target.parent.mkdir(parents=True, exist_ok=True) target.write_bytes(content) - file_count += len(skill_files) + file_count += len(payload) + + _write_manifest(install_dir, resolved_ref, requested) - _write_manifest(install_dir, resolved_ref, requested) return { - "install_dir": str(install_dir), + "install_dirs": [str(directory) for directory in targets], "ref": resolved_ref, "installed_skills": requested, "count": len(requested), @@ -347,42 +422,39 @@ def install_skills( } -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) + uninstall_result = uninstall_skills(install_dirs) + install_result = install_skills(install_dirs, 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"], 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 d04c5f4..e1aa24b 100644 --- a/tests/test_skills.py +++ b/tests/test_skills.py @@ -60,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", @@ -153,6 +225,93 @@ def test_install_skills_ignores_ref_override(self, monkeypatch, tmp_path): 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 TestRemoteChannel: def test_get_remote_skills_channel(self, monkeypatch): monkeypatch.setattr(skills, "_skills_client", _fake_skills_client) From a077059dad06259fecadb38e8def8ec0d22d1943 Mon Sep 17 00:00:00 2001 From: Zhengchao Liu Date: Mon, 10 Aug 2026 17:27:12 -0700 Subject: [PATCH 3/5] chore: sync uv.lock with the 0.8.1 version bump The 0.8.1 bump (#144) updated pyproject.toml, npm/package.json, __init__.py, and the cloud function requirements, but left uv.lock pinning the editable parallel-web-tools entry at 0.7.1. Any uv sync regenerates it, so the lock has been showing as dirty in working trees since that release. Co-Authored-By: Claude Opus 5 (1M context) --- uv.lock | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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" }, From 66f06c23bd4ad8790eff39f05265ee995394685c Mon Sep 17 00:00:00 2001 From: Zhengchao Liu Date: Mon, 10 Aug 2026 17:37:47 -0700 Subject: [PATCH 4/5] fix: never overwrite a skill this CLI did not install MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Install unconditionally rmtree'd any directory matching a skill name before writing, so a user's own skill was destroyed if it happened to share a name with one of ours — then recorded in the manifest, so the following uninstall deleted what was left. Two of the published skills are named `result` and `status`, which is exactly the kind of name someone picks for a personal skill. Harmless while `.agents/skills` was the only target, since in practice nothing but this CLI writes there. Installing into `.claude/skills` changed that: it is where people keep hand-written skills. Each directory's manifest already records what we installed there, so it can answer the ownership question. A skill directory that exists but is absent from the manifest belongs to someone else: skip it, report it in `skipped_skills`, and leave it out of the manifest so uninstall cannot claim it later. A directory that is in the manifest is our own copy, and replacing it stays an upgrade. Skips are per directory, so a name can install into `.agents/skills` and be skipped in `.claude/skills`. `installed_skills` now reports what was actually written rather than what was requested. Also fixes two defects found while reviewing this path: Manifest paths were validated with POSIX rules only, so `C:/outside/payload` passed and pathlib then joined it on Windows by discarding the skill directory entirely, letting a hostile index write anywhere writable. Release builds include a windows-x64 target, so this was reachable. Both path flavours are now rejected, plus a resolved-containment backstop at write time that does not depend on anticipating each platform's join semantics. `reinstall_skills` consumed a one-shot iterable during uninstall and then raised on the empty remainder, after the existing skills had already been removed. Targets are normalized once and reused. Verified against the live CDN: a planted `~/.claude/skills/status` keeps its contents through install and uninstall, is reported as skipped, and the same skill still installs into `.agents/skills`; an unrelated personal skill is untouched throughout. Co-Authored-By: Claude Opus 5 (1M context) --- parallel_web_tools/cli/skills.py | 14 ++++ parallel_web_tools/core/skills.py | 76 +++++++++++++++++---- tests/test_skills.py | 106 ++++++++++++++++++++++++++++++ 3 files changed, 183 insertions(+), 13 deletions(-) diff --git a/parallel_web_tools/cli/skills.py b/parallel_web_tools/cli/skills.py index d7fa2dd..9589fb8 100644 --- a/parallel_web_tools/cli/skills.py +++ b/parallel_web_tools/cli/skills.py @@ -47,6 +47,18 @@ def print_locations(result: dict) -> None: 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: @@ -121,6 +133,7 @@ def skills_install(project: bool, skill_names: tuple[str, ...], output_json: boo 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( @@ -206,5 +219,6 @@ def skills_reinstall(project: bool, skill_names: tuple[str, ...], output_json: b 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 6212909..c40e677 100644 --- a/parallel_web_tools/core/skills.py +++ b/parallel_web_tools/core/skills.py @@ -9,7 +9,7 @@ import time from collections.abc import Iterable, Iterator from contextlib import contextmanager -from pathlib import Path, PurePosixPath +from pathlib import Path, PurePosixPath, PureWindowsPath from typing import Any import httpx @@ -230,18 +230,45 @@ def _download_skill_file(client: httpx.Client, skill_name: str, skill_url: str) def _safe_skill_file_path(skill_name: str, raw_path: str) -> str: - """Validate a manifest-declared relative path stays inside the skill directory.""" + """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) - if posix_path.is_absolute() or any(part in ("..", "") for part in posix_path.parts): + 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. @@ -358,8 +385,10 @@ def install_skills( """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. Every file is downloaded once and written to each - directory, so a mid-download failure leaves no location partially installed. + 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 @@ -388,8 +417,10 @@ def install_skills( downloads[skill_name] = payload file_count = 0 + installed_anywhere: set[str] = set() + skipped: list[dict[str, str]] = [] for install_dir in targets: - previously_managed = _managed_skills(install_dir) + previously_managed = set(_managed_skills(install_dir)) install_dir.mkdir(parents=True, exist_ok=True) for skill_name in previously_managed: @@ -398,26 +429,38 @@ def install_skills( if stale_dir.exists() and stale_dir.is_dir(): shutil.rmtree(stale_dir) + installed_here: list[str] = [] for skill_name, payload in downloads.items(): skill_dir = install_dir / skill_name + + # 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 leave it alone and keep it out of the manifest. + if skill_dir.exists() and skill_name not in previously_managed: + skipped.append({"skill": skill_name, "install_dir": str(install_dir)}) + continue + if skill_dir.exists(): shutil.rmtree(skill_dir) skill_dir.mkdir(parents=True, exist_ok=True) for relative_path, content in payload: - target = skill_dir / relative_path + target = _contained_target(skill_name, skill_dir, relative_path) target.parent.mkdir(parents=True, exist_ok=True) target.write_bytes(content) + installed_here.append(skill_name) + installed_anywhere.add(skill_name) file_count += len(payload) - _write_manifest(install_dir, resolved_ref, requested) + _write_manifest(install_dir, resolved_ref, installed_here) return { "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, } @@ -450,14 +493,21 @@ def reinstall_skills( 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_dirs) - install_result = install_skills(install_dirs, 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_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_skills.py b/tests/test_skills.py index e1aa24b..bac4bd6 100644 --- a/tests/test_skills.py +++ b/tests/test_skills.py @@ -312,6 +312,112 @@ def test_empty_target_list_is_rejected(self, tmp_path): 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 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) From e38580be029834e51cf5ec11140a105d80abe78a Mon Sep 17 00:00:00 2001 From: Zhengchao Liu Date: Mon, 10 Aug 2026 18:00:06 -0700 Subject: [PATCH 5/5] fix: keep a killed install repairable MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The ownership manifest was written after a directory's skills were on disk, so an install interrupted between the two — Ctrl-C, full disk — left skill directories that no manifest claimed. The next install read them as a user's own work and skipped them, reporting success, and uninstall ignored them for the same reason. Nothing short of deleting the directory by hand recovered. The collision guard introduced that trap: before it, the next install simply overwrote the partial tree. Ownership is now decided first, then recorded, then the files are written. A crashed install leaves directories this CLI still recognizes as its own, so the next install replaces them and uninstall can remove them. Deciding before recording is what keeps a user's skill out of the manifest even when a later write in the same run fails. Co-Authored-By: Claude Opus 5 (1M context) --- parallel_web_tools/core/skills.py | 29 +++++++++++++-------- tests/test_skills.py | 43 +++++++++++++++++++++++++++++++ 2 files changed, 61 insertions(+), 11 deletions(-) diff --git a/parallel_web_tools/core/skills.py b/parallel_web_tools/core/skills.py index c40e677..29a293c 100644 --- a/parallel_web_tools/core/skills.py +++ b/parallel_web_tools/core/skills.py @@ -429,31 +429,38 @@ def install_skills( if stale_dir.exists() and stale_dir.is_dir(): shutil.rmtree(stale_dir) - installed_here: list[str] = [] - for skill_name, payload in downloads.items(): + # 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 - - # 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 leave it alone and keep it out of the manifest. 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) - for relative_path, content in payload: + 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_here.append(skill_name) installed_anywhere.add(skill_name) - file_count += len(payload) - - _write_manifest(install_dir, resolved_ref, installed_here) + file_count += len(downloads[skill_name]) return { "install_dirs": [str(directory) for directory in targets], diff --git a/tests/test_skills.py b/tests/test_skills.py index bac4bd6..b124a09 100644 --- a/tests/test_skills.py +++ b/tests/test_skills.py @@ -389,6 +389,49 @@ def test_reinstall_accepts_a_one_shot_iterable(self, monkeypatch, tmp_path): 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",