Port from paperclipai/paperclip#10978: skip locally-edited hub skills on update unless --force
paperclip#10978 made destructive replacement an explicit caller choice in their skill-sync and package-import paths: a rerun must never remove operator edits by default. Our hub-skill updater had the same hazard -- 'hermes skills update' calls do_install(force=True), which rmtree-replaces the skill directory even when the user edited it after install. do_update now compares the on-disk content hash against the hash the lockfile recorded at install time; drifted skills are skipped with a notice and only overwritten with the new --force flag (CLI + /skills slash path). Bundled skills already had this protection via the user-modified manifest in hermes update; this brings hub-installed skills to parity. Sabotage-verified: disabling the drift check makes the new skip test fail.
This commit is contained in:
Teknium
parent
358d55051e
commit
a8d5adfb2e
|
|
@ -1052,9 +1052,21 @@ def do_check(name: Optional[str] = None, console: Optional[Console] = None) -> N
|
|||
c.print(f"[dim]{update_count} update(s) available across {len(results)} checked skill(s)[/]\n")
|
||||
|
||||
|
||||
def do_update(name: Optional[str] = None, console: Optional[Console] = None) -> None:
|
||||
"""Update hub-installed skills with upstream changes."""
|
||||
from tools.skills_hub import HubLockFile, check_for_skill_updates
|
||||
def do_update(name: Optional[str] = None, console: Optional[Console] = None,
|
||||
force: bool = False) -> None:
|
||||
"""Update hub-installed skills with upstream changes.
|
||||
|
||||
Skills whose on-disk content no longer matches the hash recorded at
|
||||
install time have been edited locally; updating them would silently
|
||||
destroy the user's work (``do_install(force=True)`` rmtree-replaces the
|
||||
directory). Those are skipped by default and only overwritten when
|
||||
``force=True``. Mirrors the user-modified protection bundled skills
|
||||
already get from ``hermes update`` (ported from
|
||||
paperclipai/paperclip#10978's explicit-merge-mode rule: destructive
|
||||
replacement must be an explicit caller choice, never a rerun default).
|
||||
"""
|
||||
from tools.skills_hub import SKILLS_DIR, HubLockFile, check_for_skill_updates
|
||||
from tools.skills_guard import content_hash
|
||||
|
||||
c = console or _console
|
||||
lock = HubLockFile()
|
||||
|
|
@ -1063,9 +1075,25 @@ def do_update(name: Optional[str] = None, console: Optional[Console] = None) ->
|
|||
c.print("[dim]No updates available.[/]\n")
|
||||
return
|
||||
|
||||
skipped_local: list[str] = []
|
||||
for entry in updates:
|
||||
installed = lock.get_installed(entry["name"])
|
||||
category = _derive_category_from_install_path(installed.get("install_path", "")) if installed else ""
|
||||
if installed and not force:
|
||||
recorded_hash = installed.get("content_hash", "")
|
||||
skill_path = SKILLS_DIR / installed.get("install_path", "")
|
||||
if recorded_hash and skill_path.is_dir():
|
||||
try:
|
||||
disk_hash = content_hash(skill_path)
|
||||
except OSError:
|
||||
disk_hash = recorded_hash
|
||||
if disk_hash != recorded_hash:
|
||||
skipped_local.append(entry["name"])
|
||||
c.print(
|
||||
f"[yellow]Skipping:[/] {entry['name']} — you have local edits "
|
||||
"(update would overwrite them)."
|
||||
)
|
||||
continue
|
||||
c.print(f"[bold]Updating:[/] {entry['name']}")
|
||||
# Pin the update to the source registry recorded in the lockfile.
|
||||
# Without this, a bare (slash-less) identifier such as "reddit" falls
|
||||
|
|
@ -1082,7 +1110,15 @@ def do_update(name: Optional[str] = None, console: Optional[Console] = None) ->
|
|||
source_id=entry.get("source", "") or None,
|
||||
)
|
||||
|
||||
c.print(f"[bold green]Updated {len(updates)} skill(s).[/]\n")
|
||||
updated_count = len(updates) - len(skipped_local)
|
||||
if updated_count:
|
||||
c.print(f"[bold green]Updated {updated_count} skill(s).[/]\n")
|
||||
if skipped_local:
|
||||
c.print(
|
||||
f"[dim]{len(skipped_local)} skill(s) kept your local edits: "
|
||||
f"{', '.join(sorted(skipped_local))}.[/]"
|
||||
)
|
||||
c.print("[dim]Overwrite with: hermes skills update <name> --force[/]\n")
|
||||
|
||||
|
||||
def do_audit(name: Optional[str] = None, console: Optional[Console] = None,
|
||||
|
|
@ -1745,7 +1781,8 @@ def skills_command(args) -> None:
|
|||
elif action == "check":
|
||||
do_check(name=getattr(args, "name", None))
|
||||
elif action == "update":
|
||||
do_update(name=getattr(args, "name", None))
|
||||
do_update(name=getattr(args, "name", None),
|
||||
force=getattr(args, "force", False))
|
||||
elif action == "audit":
|
||||
do_audit(name=getattr(args, "name", None),
|
||||
deep=getattr(args, "deep", False))
|
||||
|
|
@ -1929,8 +1966,10 @@ def handle_skills_slash(cmd: str, console: Optional[Console] = None) -> None:
|
|||
do_check(name=name, console=c)
|
||||
|
||||
elif action == "update":
|
||||
name = args[0] if args else None
|
||||
do_update(name=name, console=c)
|
||||
force = "--force" in args
|
||||
pos = [a for a in args if not a.startswith("--")]
|
||||
name = pos[0] if pos else None
|
||||
do_update(name=name, console=c, force=force)
|
||||
|
||||
elif action == "audit":
|
||||
name = args[0] if args and not args[0].startswith("--") else None
|
||||
|
|
|
|||
|
|
@ -139,6 +139,11 @@ def build_skills_parser(subparsers, *, cmd_skills: Callable) -> None:
|
|||
nargs="?",
|
||||
help="Specific skill to update (default: all outdated skills)",
|
||||
)
|
||||
skills_update.add_argument(
|
||||
"--force",
|
||||
action="store_true",
|
||||
help="Overwrite skills you have edited locally (they are skipped by default)",
|
||||
)
|
||||
|
||||
skills_audit = skills_subparsers.add_parser(
|
||||
"audit", help="Re-scan installed hub skills"
|
||||
|
|
|
|||
|
|
@ -313,3 +313,84 @@ def test_do_search_json_flag_emits_full_identifiers(capsys):
|
|||
# Table render must be suppressed — sink should be empty (no "Searching for:" header).
|
||||
assert "Searching for:" not in sink.getvalue()
|
||||
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# Local-edit protection in do_update (ported from paperclipai/paperclip#10978)
|
||||
# ---------------------------------------------------------------------------
|
||||
|
||||
|
||||
def _update_env(monkeypatch, tmp_path, *, edit_after_install: bool):
|
||||
"""Install a fake hub skill on disk, optionally edit it, and wire mocks.
|
||||
|
||||
Returns (console_sink, installs_list).
|
||||
"""
|
||||
import hermes_cli.skills_hub as cli_hub
|
||||
import tools.skills_hub as hub
|
||||
from tools.skills_guard import content_hash
|
||||
|
||||
skills_dir = tmp_path / "skills"
|
||||
skill_dir = skills_dir / "category" / "hub-skill"
|
||||
skill_dir.mkdir(parents=True)
|
||||
(skill_dir / "SKILL.md").write_text("# hub-skill\noriginal\n")
|
||||
|
||||
recorded = content_hash(skill_dir)
|
||||
if edit_after_install:
|
||||
(skill_dir / "SKILL.md").write_text("# hub-skill\nuser edited\n")
|
||||
|
||||
monkeypatch.setattr(hub, "SKILLS_DIR", skills_dir)
|
||||
monkeypatch.setattr(hub, "check_for_skill_updates", lambda **_kwargs: [{
|
||||
"name": "hub-skill",
|
||||
"identifier": "someone/hub-skill",
|
||||
"source": "github",
|
||||
"status": "update_available",
|
||||
}])
|
||||
monkeypatch.setattr(hub, "HubLockFile", lambda: type("L", (), {
|
||||
"get_installed": lambda self, name: {
|
||||
"install_path": "category/hub-skill",
|
||||
"content_hash": recorded,
|
||||
}
|
||||
})())
|
||||
|
||||
installs = []
|
||||
monkeypatch.setattr(
|
||||
cli_hub, "do_install",
|
||||
lambda identifier, category="", force=False, console=None, source_id=None:
|
||||
installs.append(identifier),
|
||||
)
|
||||
|
||||
sink = StringIO()
|
||||
console = Console(file=sink, force_terminal=False, color_system=None)
|
||||
return console, sink, installs
|
||||
|
||||
|
||||
def test_do_update_skips_locally_edited_skill(monkeypatch, tmp_path):
|
||||
"""A hub skill whose on-disk hash drifted from the lockfile is skipped."""
|
||||
console, sink, installs = _update_env(monkeypatch, tmp_path, edit_after_install=True)
|
||||
|
||||
do_update(console=console)
|
||||
|
||||
assert installs == []
|
||||
out = sink.getvalue()
|
||||
assert "local edits" in out
|
||||
assert "--force" in out
|
||||
|
||||
|
||||
def test_do_update_force_overwrites_local_edits(monkeypatch, tmp_path):
|
||||
"""--force restores the destructive replace for edited skills."""
|
||||
console, sink, installs = _update_env(monkeypatch, tmp_path, edit_after_install=True)
|
||||
|
||||
do_update(console=console, force=True)
|
||||
|
||||
assert installs == ["someone/hub-skill"]
|
||||
assert "local edits" not in sink.getvalue()
|
||||
|
||||
|
||||
def test_do_update_unmodified_skill_updates_normally(monkeypatch, tmp_path):
|
||||
"""No local drift -> the update proceeds without --force."""
|
||||
console, sink, installs = _update_env(monkeypatch, tmp_path, edit_after_install=False)
|
||||
|
||||
do_update(console=console)
|
||||
|
||||
assert installs == ["someone/hub-skill"]
|
||||
assert "Updated 1 skill(s)" in sink.getvalue()
|
||||
|
|
|
|||
|
|
@ -754,10 +754,13 @@ The hub now tracks enough provenance to re-check upstream copies of installed sk
|
|||
hermes skills check # Report which installed hub skills changed upstream
|
||||
hermes skills update # Reinstall only the skills with updates available
|
||||
hermes skills update react # Update one specific installed hub skill
|
||||
hermes skills update react --force # Overwrite a skill you've edited locally
|
||||
```
|
||||
|
||||
This uses the stored source identifier plus the current upstream bundle content hash to detect drift.
|
||||
|
||||
Skills you have edited locally (the on-disk content no longer matches the hash recorded at install time) are **skipped** by `hermes skills update` so your changes are never silently overwritten. Pass `--force` to replace them with the upstream version anyway.
|
||||
|
||||
:::tip GitHub rate limits
|
||||
Skills hub operations use the GitHub API, which has a rate limit of 60 requests/hour for unauthenticated users. If you see rate-limit errors during install or search, set `GITHUB_TOKEN` in your `.env` file to increase the limit to 5,000 requests/hour. The error message includes an actionable hint when this happens.
|
||||
:::
|
||||
|
|
|
|||
Loading…
Reference in New Issue