Skip to content

Add a skills management command to parallel cli#89

Merged
sergei1152 merged 5 commits intomainfrom
serj.add_skills_install_command
May 5, 2026
Merged

Add a skills management command to parallel cli#89
sergei1152 merged 5 commits intomainfrom
serj.add_skills_install_command

Conversation

@sergei1152
Copy link
Copy Markdown
Contributor

This will be useful for agentic onboarding so we can ensure skills are installed regardless of what agent harness someone is using.

Tested locally with uv run parallel-cli install

@sergei1152 sergei1152 force-pushed the serj.add_skills_install_command branch from d6c3544 to bc75c8e Compare May 5, 2026 01:35
@NormallyGaussian
Copy link
Copy Markdown
Contributor

Hi Serj — Claude here again, on Matt's behalf. I checked out the branch and ran the new tests (20 pass, ~0.4s). The shape is good: clean separation of CLI wiring vs implementation, lazy imports, Protocol for HandleError, NoReturn correctly applied. The path-traversal protection in _extract_repo_archive is solid (rejects .., empty/absolute first parts, verifies resolved target is inside dest; symlinks aren't a concern since zipfile doesn't preserve them). A few things worth thinking about:

Worth addressing

  • Subtle destructive behavior in install --skill X. The reconciliation logic removes any previously-managed skill not in the new request. So if a user has [A, B, C] installed and runs parallel-cli skills install --skill A, B and C get deleted. Same for reinstall --skill X. This is documented in install_skills's docstring but not surfaced in --help. Either:

    • mention it in the option help: "Skill name to install (repeatable). Defaults to all. Skills not listed will be removed.", or
    • split into separate verbs (e.g., add/remove for additive ops, keep install --skill as the "set" operation).
  • No skills list CLI command. list_remote_skills() exists in core/skills.py but isn't wired up. Users currently have to read the GitHub repo to know what --skill values are valid — and SkillsInputError is the only feedback for typos. Adding parallel-cli skills list would close the loop cheaply.

  • Module placement may collide with PR Add some nix setup tooling to parallel-cli #88. parallel_web_tools/core/skills.py is an HTTP/archive installer, not data-enrichment internals. The tach rules in Add some nix setup tooling to parallel-cli #88 scope core.** to enrichment; if both PRs land, this lives somewhere it doesn't really belong. Consider parallel_web_tools/skills.py (top-level) or keeping the implementation under cli/.

  • from parallel_web_tools.core import skills exported as a module via __all__. This exposes the entire module surface (including _extract_repo_archive, etc.) as public API. Recommend exporting specific symbols you intend to be stable (install_skills, uninstall_skills, etc.) or nothing at all if this is purely CLI-internal.

Minor / polish

  • Non-atomic install. install_skills removes "previously-managed-not-in-requested" before copying new skills. If shutil.copytree fails partway (disk full, perms), the user ends up with neither old nor new. Probably fine for v1, but a stage-and-swap pattern (extract to sibling temp dir, rename) would be safer.
  • GitHub API is unauthenticated (60 req/hr per IP). Fine for end-user installs, but CI fleets all egressing the same NAT could hit the limit. No retry/backoff on 5xx or 429 — _download_repo_archive just raises on any 4xx/5xx. Not a blocker; worth a TODO.
  • Hardcoded org/repo. Ref is overridable via PARALLEL_SKILLS_REPO_REF, but not the org/repo. Adding env overrides for SKILLS_REPO_OWNER/SKILLS_REPO_NAME would help testing against forks.
  • Help text for uninstall: "Uninstall from .agents/skills..." reads as directory-scoped, but the operation is manifest-scoped (only removes parallel-cli-managed entries — which is good, but worth saying so).
  • The repeated try/except/handle_error blocks across install/uninstall/reinstall could be a small helper, but it's not bad as-is.

No concerns

  • parallel-web/parallel-agent-skills is a public repo (verified with a HEAD on the API URL — 200).
  • Path-traversal hardening is thorough.
  • Test coverage hits the meaningful paths: env-override, project-root detection, archive extraction, install/uninstall reconciliation, unknown-skill rejection.
  • _handle_error does call sys.exit, so the NoReturn annotation reflects reality.

Nothing here is a blocker. The destructive --skill semantics is the one I'd most want addressed (or at least documented in --help) before merge — it's the kind of footgun that produces confused bug reports.

@sergei1152 sergei1152 force-pushed the serj.better_env_setup branch from 8882ffa to aa876ca Compare May 5, 2026 22:07
@sergei1152 sergei1152 force-pushed the serj.add_skills_install_command branch from bc75c8e to d16b466 Compare May 5, 2026 22:19
@sergei1152
Copy link
Copy Markdown
Contributor Author

^ Addressed

Base automatically changed from serj.better_env_setup to main May 5, 2026 23:34
@sergei1152 sergei1152 force-pushed the serj.add_skills_install_command branch from d16b466 to a475532 Compare May 5, 2026 23:35
@sergei1152 sergei1152 merged commit 78b8481 into main May 5, 2026
7 checks passed
@sergei1152 sergei1152 deleted the serj.add_skills_install_command branch May 5, 2026 23:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants