Conversation
|
The docs for this PR live here. All of your documentation changes will be reflected on that endpoint. The docs are available until 30 days after the last update. |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
Co-Authored-By: Claude Sonnet 4.6 <[email protected]>
| * `add`: Download a skill and install it for an AI... | ||
| * `preview`: Print the generated SKILL.md to stdout. |
There was a problem hiding this comment.
I feel that hf skills add and hf skills preview description could be harmonized now that we have more than 1 skill that can be installed
|
Thanks for the review @Wauplin. I've responded to each item and cut down the bloat. |
JiwaniZakir
left a comment
JiwaniZakir
left a comment
There was a problem hiding this comment.
The removal of --codex, --cursor, and --opencode flags from hf skills add in the docs (and presumably in the CLI implementation) is a breaking change with no deprecation path mentioned. Users relying on those flags in scripts or CI pipelines will get silent failures or unrecognized option errors after upgrading. At minimum, keeping the flags but printing a deprecation warning would be safer.
In install_marketplace_skill, the two consecutive if install_dir.exists() checks are misleading — the second block is only reachable when force=True, but that's implicit rather than explicit. Using elif force: would make the control flow clearer and prevent any future regression if the first guard is accidentally changed.
The constant GITHUB_API_TIMEOUT appears to govern requests made via get_session() to the HuggingFace Hub, not GitHub's API — the name is a misnomer that will cause confusion when someone goes looking for where GitHub calls are rate-limited or configured. Renaming it to HF_API_TIMEOUT or HUB_REQUEST_TIMEOUT would match the actual usage.
|
@JiwaniZakir thanks for the tokens, but we got this. |
Wauplin
left a comment
Wauplin
left a comment
There was a problem hiding this comment.
Thanks @burtenshaw ! Left a few more comments. Mostly nits and cosmetic changes except for the test suite that can be simplified IMO
| $ hf skills add --claude --cursor | ||
| $ hf skills add --codex --opencode --cursor --global | ||
| $ hf skills add --claude | ||
| $ hf skills add --claude --global |
There was a problem hiding this comment.
| $ hf skills add --claude --global | |
| $ hf skills add huggingface-gradio --claude --global |
(nit) let's add an example showing extension skill can also be installed globally
| try: | ||
| if dest: | ||
| if claude or global_: | ||
| print("--dest cannot be combined with --claude or --global.") | ||
| raise typer.Exit(code=1) | ||
| skill_dest = _install_to(dest, name, force) | ||
| print(f"Installed '{name}' to {skill_dest}") | ||
| return | ||
|
|
||
| # Install to central location | ||
| central_path = CENTRAL_GLOBAL if global_ else CENTRAL_LOCAL | ||
| central_skill_path = _install_to(central_path, name, force) | ||
| print(f"Installed '{name}' to central location: {central_skill_path}") | ||
|
|
||
| if claude: | ||
| agent_target = CLAUDE_GLOBAL if global_ else CLAUDE_LOCAL | ||
| link_path = _create_symlink(agent_target, name, central_skill_path, force) | ||
| print(f"Created symlink: {link_path}") | ||
| except CLIError as exc: | ||
| print(str(exc)) | ||
| raise typer.Exit(code=1) from exc |
There was a problem hiding this comment.
no need for the try/except. The CLIError is properly caught in hf.py directly (for all commands at once)
| try: | |
| if dest: | |
| if claude or global_: | |
| print("--dest cannot be combined with --claude or --global.") | |
| raise typer.Exit(code=1) | |
| skill_dest = _install_to(dest, name, force) | |
| print(f"Installed '{name}' to {skill_dest}") | |
| return | |
| # Install to central location | |
| central_path = CENTRAL_GLOBAL if global_ else CENTRAL_LOCAL | |
| central_skill_path = _install_to(central_path, name, force) | |
| print(f"Installed '{name}' to central location: {central_skill_path}") | |
| if claude: | |
| agent_target = CLAUDE_GLOBAL if global_ else CLAUDE_LOCAL | |
| link_path = _create_symlink(agent_target, name, central_skill_path, force) | |
| print(f"Created symlink: {link_path}") | |
| except CLIError as exc: | |
| print(str(exc)) | |
| raise typer.Exit(code=1) from exc | |
| if dest: | |
| if claude or global_: | |
| print("--dest cannot be combined with --claude or --global.") | |
| raise typer.Exit(code=1) | |
| skill_dest = _install_to(dest, name, force) | |
| print(f"Installed '{name}' to {skill_dest}") | |
| return | |
| # Install to central location | |
| central_path = CENTRAL_GLOBAL if global_ else CENTRAL_LOCAL | |
| central_skill_path = _install_to(central_path, name, force) | |
| print(f"Installed '{name}' to central location: {central_skill_path}") | |
| if claude: | |
| agent_target = CLAUDE_GLOBAL if global_ else CLAUDE_LOCAL | |
| link_path = _create_symlink(agent_target, name, central_skill_path, force) | |
| print(f"Created symlink: {link_path}") |
| roots = _resolve_update_roots( | ||
| claude=claude, | ||
| global_=global_, | ||
| dest=dest, | ||
| ) |
There was a problem hiding this comment.
(nit)
| roots = _resolve_update_roots( | |
| claude=claude, | |
| global_=global_, | |
| dest=dest, | |
| ) | |
| roots = _resolve_update_roots(claude=claude, global_=global_, dest=dest) |
| ] = None, | ||
| ) -> None: | ||
| """Upgrade installed Hugging Face marketplace skills.""" | ||
| try: |
There was a problem hiding this comment.
| try: |
Same here, no need for the try/except
| from __future__ import annotations | ||
|
|
||
| import base64 |
There was a problem hiding this comment.
| from __future__ import annotations | |
| import base64 | |
| import base64 |
no need anymore (minimal version is now Python3.10)
There was a problem hiding this comment.
I find the entire test suite quite verbose and doesn't actually test the feature. In huggingface_hub we usually try to test features without monkeypatching (i.e. with remote calls).
Here I think it's fine if create a tmp directory, run hf skills add huggingface-gradio, checks it's installed in the tmp dir, run hf skills upgrade and check it has been correctly upgraded (or at least check the output that it has not been upgraded but a remote check has been done).
Such testing would have a much smaller scope but at least it would be self-explainable and mimic an actual use of the CLI
There was a problem hiding this comment.
should be removed from PR (likely just a dev artifact on your side)
|
|
||
| from __future__ import annotations | ||
|
|
There was a problem hiding this comment.
| from __future__ import annotations |
(nit) (not needed anymore)
| DEFAULT_SKILLS_REPO_OWNER = "huggingface" | ||
| DEFAULT_SKILLS_REPO_NAME = "skills" |
There was a problem hiding this comment.
| DEFAULT_SKILLS_REPO_OWNER = "huggingface" | |
| DEFAULT_SKILLS_REPO_NAME = "skills" | |
| DEFAULT_SKILLS_REPO_OWNER, DEFAULT_SKILLS_REPO_NAME = DEFAULT_SKILLS_REPO_ID.split("/") |
There was a problem hiding this comment.
I've read this module and "it looks good" but can't say that I reviewed it in-depth. I think it's fine though given the scope is non-critical.
|
Agreed with @Wauplin — scoping this to the HF skills marketplace keeps the surface area manageable and avoids the complexity of validating arbitrary third-party sources. The |
Also agree with him, what a visionary! |
|
The dependency on the naming simplification in |
|
do you also agree with @julien-c? |
|
The manifest-based approach for tracking upstream revisions is a solid design for |
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
| print(f"Created symlink: {link_path}") | ||
| except CLIError as exc: | ||
| print(str(exc)) | ||
| raise typer.Exit(code=1) from exc |
There was a problem hiding this comment.
CLIError caught locally bypasses global stderr error handler
Low Severity
Both skills_add and skills_upgrade catch CLIError locally and print() to stdout, then raise typer.Exit. The global handler in hf.py prints CLIError to stderr with an "Error: " prefix and a debug hint. This means these two commands output errors to a different stream and in a different format than every other hf subcommand, which breaks scripting that relies on stdout/stderr separation. Reviewer @Wauplin already noted the try/except is unnecessary since CLIError is handled globally.
Additional Locations (1)
|
Agree with @Wauplin — scoping to the default HF marketplace keeps the surface area small and the mental model simple for users. If we open up arbitrary marketplace installs in the CLI, we'll need to think through trust/verification concerns that aren't worth tackling right now. Once the naming simplification in huggingface/skills#99 lands, this should be straightforward to merge as-is. |
hanouticelina
left a comment
hanouticelina
left a comment
There was a problem hiding this comment.
Thanks for the PR! I left some comments, mostly readabilitycleanups.
(feel free to ask cursor to address all the review comments from @Wauplin and myself 😄)
| *, | ||
| claude: bool, | ||
| global_: bool, | ||
| dest: Optional[Path], |
There was a problem hiding this comment.
we recently bump the minimum Python version to 3.10
| dest: Optional[Path], | |
| dest: Path | None, |
| ) | ||
| def skills_upgrade( | ||
| name: Annotated[ | ||
| Optional[str], |
There was a problem hiding this comment.
| Optional[str], | |
| str | None, |
| ), | ||
| ] = False, | ||
| dest: Annotated[ | ||
| Optional[Path], |
There was a problem hiding this comment.
| Optional[Path], | |
| Path | None, |
| skill_file = skill_dir / "SKILL.md" | ||
| if not skill_file.is_file(): | ||
| raise RuntimeError(f"Installed skill is missing SKILL.md: {skill_file}") | ||
| skill_file.read_text(encoding="utf-8") |
There was a problem hiding this comment.
is reading the file needed here? the is_file() check already validates existence
| skill_file.read_text(encoding="utf-8") |
| manifest, error = read_installed_skill_manifest(skill_dir) | ||
| if manifest is None: | ||
| return SkillUpdateInfo( | ||
| name=skill_dir.name, | ||
| skill_dir=skill_dir, | ||
| status="invalid_metadata" if error is not None else "unmanaged", | ||
| detail=error, | ||
| ) | ||
|
|
||
| skill = marketplace_skills.get(skill_dir.name.lower()) | ||
| if skill is None: | ||
| return SkillUpdateInfo( | ||
| name=skill_dir.name, | ||
| skill_dir=skill_dir, | ||
| status="source_unreachable", | ||
| detail=f"Skill '{skill_dir.name}' is no longer available in {DEFAULT_SKILLS_REPO_ID}.", | ||
| current_revision=manifest.installed_revision, | ||
| ) | ||
|
|
||
| current_revision = manifest.installed_revision | ||
| try: | ||
| available_revision = _resolve_available_revision(skill) | ||
| except Exception as exc: | ||
| return SkillUpdateInfo( | ||
| name=skill_dir.name, | ||
| skill_dir=skill_dir, | ||
| status="source_unreachable", | ||
| detail=str(exc), | ||
| current_revision=current_revision, | ||
| ) | ||
|
|
||
| if available_revision == current_revision: | ||
| return SkillUpdateInfo( | ||
| name=skill_dir.name, | ||
| skill_dir=skill_dir, | ||
| status="up_to_date", | ||
| current_revision=current_revision, | ||
| available_revision=available_revision, | ||
| ) | ||
|
|
||
| return SkillUpdateInfo( | ||
| name=skill_dir.name, | ||
| skill_dir=skill_dir, | ||
| status="update_available", | ||
| detail="update available", | ||
| current_revision=current_revision, | ||
| available_revision=available_revision, | ||
| ) |
There was a problem hiding this comment.
small preference for using dataclasses.replace when dealing with frozen dataclasses
| manifest, error = read_installed_skill_manifest(skill_dir) | |
| if manifest is None: | |
| return SkillUpdateInfo( | |
| name=skill_dir.name, | |
| skill_dir=skill_dir, | |
| status="invalid_metadata" if error is not None else "unmanaged", | |
| detail=error, | |
| ) | |
| skill = marketplace_skills.get(skill_dir.name.lower()) | |
| if skill is None: | |
| return SkillUpdateInfo( | |
| name=skill_dir.name, | |
| skill_dir=skill_dir, | |
| status="source_unreachable", | |
| detail=f"Skill '{skill_dir.name}' is no longer available in {DEFAULT_SKILLS_REPO_ID}.", | |
| current_revision=manifest.installed_revision, | |
| ) | |
| current_revision = manifest.installed_revision | |
| try: | |
| available_revision = _resolve_available_revision(skill) | |
| except Exception as exc: | |
| return SkillUpdateInfo( | |
| name=skill_dir.name, | |
| skill_dir=skill_dir, | |
| status="source_unreachable", | |
| detail=str(exc), | |
| current_revision=current_revision, | |
| ) | |
| if available_revision == current_revision: | |
| return SkillUpdateInfo( | |
| name=skill_dir.name, | |
| skill_dir=skill_dir, | |
| status="up_to_date", | |
| current_revision=current_revision, | |
| available_revision=available_revision, | |
| ) | |
| return SkillUpdateInfo( | |
| name=skill_dir.name, | |
| skill_dir=skill_dir, | |
| status="update_available", | |
| detail="update available", | |
| current_revision=current_revision, | |
| available_revision=available_revision, | |
| ) | |
| base = SkillUpdateInfo(name=skill_dir.name, skill_dir=skill_dir, status="unmanaged") | |
| manifest, error = read_installed_skill_manifest(skill_dir) | |
| if manifest is None: | |
| return replace(base, status="invalid_metadata" if error else "unmanaged", detail=error) | |
| skill = marketplace_skills.get(skill_dir.name.lower()) | |
| if skill is None: | |
| return replace( | |
| base, | |
| status="source_unreachable", | |
| detail=f"Skill '{skill_dir.name}' is no longer available in {DEFAULT_SKILLS_REPO_ID}.", | |
| current_revision=manifest.installed_revision, | |
| ) | |
| current_revision = manifest.installed_revision | |
| try: | |
| available_revision = _resolve_available_revision(skill) | |
| except Exception as exc: | |
| return replace(base, status="source_unreachable", detail=str(exc), current_revision=current_revision) | |
| status = "up_to_date" if available_revision == current_revision else "update_available" | |
| return replace( | |
| base, | |
| status=status, | |
| detail="update available" if status == "update_available" else None, | |
| current_revision=current_revision, | |
| available_revision=available_revision, | |
| ) |
| if update.status in {"up_to_date", "unmanaged", "invalid_metadata", "source_unreachable"}: | ||
| return update |
There was a problem hiding this comment.
iiuc, only "update_available" should trigger an install
| if update.status in {"up_to_date", "unmanaged", "invalid_metadata", "source_unreachable"}: | |
| return update | |
| if update.status != "update_available": | |
| return update |
| manifest, error = read_installed_skill_manifest(update.skill_dir) | ||
| if manifest is None: | ||
| detail = error or "missing skill manifest" | ||
| return SkillUpdateInfo( | ||
| name=update.name, | ||
| skill_dir=update.skill_dir, | ||
| status="invalid_metadata", | ||
| detail=detail, | ||
| current_revision=update.current_revision, | ||
| available_revision=update.available_revision, | ||
| ) | ||
|
|
||
| try: | ||
| skill = get_marketplace_skill(update.skill_dir.name) | ||
| install_marketplace_skill(skill, update.skill_dir.parent, force=True) | ||
| except Exception as exc: | ||
| return SkillUpdateInfo( | ||
| name=update.name, | ||
| skill_dir=update.skill_dir, | ||
| status="source_unreachable", | ||
| detail=str(exc), | ||
| current_revision=update.current_revision, | ||
| available_revision=update.available_revision, | ||
| ) | ||
|
|
||
| refreshed_manifest, manifest_error = read_installed_skill_manifest(update.skill_dir) | ||
| if refreshed_manifest is None: | ||
| detail = manifest_error or "missing skill manifest after upgrade" | ||
| return SkillUpdateInfo( | ||
| name=update.name, | ||
| skill_dir=update.skill_dir, | ||
| status="invalid_metadata", | ||
| detail=detail, | ||
| current_revision=update.current_revision, | ||
| available_revision=update.available_revision, | ||
| ) | ||
|
|
||
| return SkillUpdateInfo( | ||
| name=update.name, | ||
| skill_dir=update.skill_dir, | ||
| status="updated", | ||
| detail="updated", | ||
| current_revision=update.current_revision, | ||
| available_revision=refreshed_manifest.installed_revision, | ||
| ) |
There was a problem hiding this comment.
two things here:
- Removing the manifest re-read before install,
_evaluate_updatealready validated it. - Removing the manifest re-read after install, if
install_marketplace_skillsucceeds, the manifest was written. No need to read it back to confirm
| manifest, error = read_installed_skill_manifest(update.skill_dir) | |
| if manifest is None: | |
| detail = error or "missing skill manifest" | |
| return SkillUpdateInfo( | |
| name=update.name, | |
| skill_dir=update.skill_dir, | |
| status="invalid_metadata", | |
| detail=detail, | |
| current_revision=update.current_revision, | |
| available_revision=update.available_revision, | |
| ) | |
| try: | |
| skill = get_marketplace_skill(update.skill_dir.name) | |
| install_marketplace_skill(skill, update.skill_dir.parent, force=True) | |
| except Exception as exc: | |
| return SkillUpdateInfo( | |
| name=update.name, | |
| skill_dir=update.skill_dir, | |
| status="source_unreachable", | |
| detail=str(exc), | |
| current_revision=update.current_revision, | |
| available_revision=update.available_revision, | |
| ) | |
| refreshed_manifest, manifest_error = read_installed_skill_manifest(update.skill_dir) | |
| if refreshed_manifest is None: | |
| detail = manifest_error or "missing skill manifest after upgrade" | |
| return SkillUpdateInfo( | |
| name=update.name, | |
| skill_dir=update.skill_dir, | |
| status="invalid_metadata", | |
| detail=detail, | |
| current_revision=update.current_revision, | |
| available_revision=update.available_revision, | |
| ) | |
| return SkillUpdateInfo( | |
| name=update.name, | |
| skill_dir=update.skill_dir, | |
| status="updated", | |
| detail="updated", | |
| current_revision=update.current_revision, | |
| available_revision=refreshed_manifest.installed_revision, | |
| ) | |
| try: | |
| skill = get_marketplace_skill(update.skill_dir.name) | |
| install_marketplace_skill(skill, update.skill_dir.parent, force=True) | |
| except Exception as exc: | |
| return replace(update, status="source_unreachable", detail=str(exc)) | |
| return replace(update, status="updated", detail="updated") |
|
|
||
|
|
||
| def _extract_remote_github_path(revision: str, source_path: str, install_dir: Path) -> None: | ||
| tar_bytes = _github_api_get_bytes(f"tarball/{revision}") |
There was a problem hiding this comment.
_github_api_get_bytes is a one liner with a single call. let's juste inline it
| tar_bytes = _github_api_get_bytes(f"tarball/{revision}") | |
| tar_bytes = _github_api_get(f"tarball/{revision}").content |
| def _github_api_get_json(endpoint: str, params: dict[str, Any] | None = None) -> Any: | ||
| response = _github_api_get(endpoint, params=params) | ||
| try: | ||
| return response.json() | ||
| except Exception as exc: # noqa: BLE001 | ||
| raise CLIError(f"Failed to decode GitHub API response for '{endpoint}': {exc}") from exc | ||
|
|
||
|
|
||
| def _github_api_get_bytes(endpoint: str, params: dict[str, Any] | None = None) -> bytes: | ||
| return _github_api_get(endpoint, params=params).content | ||
|
|
||
|
|
||
| def _github_api_get(endpoint: str, params: dict[str, Any] | None = None): | ||
| url = f"https://api.github.com/repos/{DEFAULT_SKILLS_REPO_OWNER}/{DEFAULT_SKILLS_REPO_NAME}/{endpoint.lstrip('/')}" | ||
| try: | ||
| response = get_session().get( | ||
| url, | ||
| params=params, | ||
| headers={"Accept": "application/vnd.github+json"}, | ||
| follow_redirects=True, | ||
| timeout=GITHUB_API_TIMEOUT, | ||
| ) | ||
| response.raise_for_status() | ||
| except Exception as exc: # noqa: BLE001 | ||
| raise CLIError(f"Failed to fetch '{endpoint}' from {DEFAULT_SKILLS_REPO_ID}: {exc}") from exc | ||
| return response |
There was a problem hiding this comment.
this can be one helper and then replace _github_api_get_json / _github_api_get_bytes / _github_api_get calls with _fetch_from_skills_repo(..., as_json=True) or _fetch_from_skills_repo(...) respectively
| def _github_api_get_json(endpoint: str, params: dict[str, Any] | None = None) -> Any: | |
| response = _github_api_get(endpoint, params=params) | |
| try: | |
| return response.json() | |
| except Exception as exc: # noqa: BLE001 | |
| raise CLIError(f"Failed to decode GitHub API response for '{endpoint}': {exc}") from exc | |
| def _github_api_get_bytes(endpoint: str, params: dict[str, Any] | None = None) -> bytes: | |
| return _github_api_get(endpoint, params=params).content | |
| def _github_api_get(endpoint: str, params: dict[str, Any] | None = None): | |
| url = f"https://api.github.com/repos/{DEFAULT_SKILLS_REPO_OWNER}/{DEFAULT_SKILLS_REPO_NAME}/{endpoint.lstrip('/')}" | |
| try: | |
| response = get_session().get( | |
| url, | |
| params=params, | |
| headers={"Accept": "application/vnd.github+json"}, | |
| follow_redirects=True, | |
| timeout=GITHUB_API_TIMEOUT, | |
| ) | |
| response.raise_for_status() | |
| except Exception as exc: # noqa: BLE001 | |
| raise CLIError(f"Failed to fetch '{endpoint}' from {DEFAULT_SKILLS_REPO_ID}: {exc}") from exc | |
| return response | |
| def _fetch_from_skills_repo(endpoint: str, params: dict[str, Any] | None = None, *, as_json: bool = False) -> Any: | |
| url = f"https://api.github.com/repos/{DEFAULT_SKILLS_REPO_OWNER}/{DEFAULT_SKILLS_REPO_NAME}/{endpoint.lstrip('/')}" | |
| try: | |
| response = get_session().get( | |
| url, | |
| params=params, | |
| headers={"Accept": "application/vnd.github+json"}, | |
| follow_redirects=True, | |
| timeout=GITHUB_API_TIMEOUT, | |
| ) | |
| response.raise_for_status() | |
| except Exception as exc: # noqa: BLE001 | |
| raise CLIError(f"Failed to fetch '{endpoint}' from {DEFAULT_SKILLS_REPO_ID}: {exc}") from exc | |
| if as_json: | |
| try: | |
| return response.json() | |
| except Exception as exc: # noqa: BLE001 | |
| raise CLIError(f"Failed to decode GitHub API response for '{endpoint}': {exc}") from exc | |
| return response.content |
5 participants
This PR allows uses to add and update skills from
huggingface/skillsYou can test it like this.
The naming in
huggingface/skillsshould be simplified before we merge this. i.e.hugging-face-trackio>>trackio. That's added hereNote
Medium Risk
Adds new CLI behavior that downloads and extracts skill content from GitHub tarballs and overwrites local directories during upgrades, so failures could impact users’ local skill installs. The change is scoped to the
hf skillscommand group but introduces new network/file I/O and archive extraction logic.Overview
hf skills addnow installs any marketplace skill (defaulting tohf-cli) by fetching the skill’s directory from thehuggingface/skillsGitHub repo and writing a local.hf-skill-manifest.jsonwith the installed revision.Adds
hf skills upgradeto scan one or more skills roots (central and optional--claude, or a custom--dest), detect upstream revision changes, and reinstall skills in place with an atomic directory swap; unmanaged/invalid installs are reported rather than updated.Updates the generated CLI docs accordingly and adds comprehensive tests with a fake GitHub API session/tarball generator to validate install, overwrite, manifest writing, symlink behavior, and upgrade flows.
Written by Cursor Bugbot for commit 2c73db8. This will update automatically on new commits. Configure here.