Merge pull request #48286 from kshitijk4poor/salvage/skills-list-modified-diff

feat(skills): find & diff user-modified bundled skills (salvage of #47802)

gateway/slash_commands.py

@@ -2214,7 +2214,9 @@ class GatewaySlashCommandsMixin:
         stranded).
 
         ``diff`` output is truncated for chat bubbles — the full diff lives in
-        the CLI (``/skills diff <id>``) and the pending JSON file.
+        the pending JSON file under ``~/.hermes/pending/skills/``. (Note this is
+        the write-approval ``diff <id>``; the CLI also has an unrelated
+        ``hermes skills diff <name>`` that diffs a bundled skill vs stock.)
         """
         from gateway.run import _hermes_home
         from hermes_cli.write_approval_commands import handle_pending_subcommand
@@ -2252,12 +2254,14 @@ class GatewaySlashCommandsMixin:
                     "(Search/install are CLI-only.)")
 
         # Chat bubbles can't hold a full skill diff — truncate and point at
-        # the real review surfaces.
+        # the real review surface. (Note: `hermes skills diff <name>` is a
+        # *different* command — it diffs a bundled skill against its stock
+        # version — so we point at the pending JSON file, not that command.)
         if args and args[0].lower() == "diff" and len(out) > 3000:
             pending_id = args[1] if len(args) > 1 else "<id>"
             out = (out[:3000]
-                   + f"\n… (truncated — full diff: `/skills diff {pending_id}` "
-                     f"on the CLI, or ~/.hermes/pending/skills/{pending_id}.json)")
+                   + "\n… (truncated — full diff in "
+                     f"~/.hermes/pending/skills/{pending_id}.json)")
         return out
 
     async def _handle_fast_command(self, event: MessageEvent) -> str:

hermes_cli/main.py

@@ -6073,6 +6073,10 @@ def _update_via_zip(args):
             )
         if result.get("user_modified"):
             print(f"  ~ {len(result['user_modified'])} user-modified (kept)")
+            print(
+                "    → see them: hermes skills list-modified  "
+                "(diff/reset to resume updates)"
+            )
         if result.get("cleaned"):
             print(f"  − {len(result['cleaned'])} removed from manifest")
         if not result["copied"] and not result.get("updated"):
@@ -9061,6 +9065,10 @@ def _cmd_update_impl(args, gateway_mode: bool):
                 )
             if result.get("user_modified"):
                 print(f"  ~ {len(result['user_modified'])} user-modified (kept)")
+                print(
+                    "    → see them: hermes skills list-modified  "
+                    "(diff/reset to resume updates)"
+                )
             if result.get("cleaned"):
                 print(f"  − {len(result['cleaned'])} removed from manifest")
             if not result["copied"] and not result.get("updated"):

hermes_cli/skills_hub.py

@@ -1149,6 +1149,73 @@ def do_reset(name: str, restore: bool = False,
         c.print("[dim]Use /reset to start a new session now, or --now to apply immediately (invalidates prompt cache).[/]\n")
 
 
+def do_list_modified(console: Optional[Console] = None,
+                     as_json: bool = False) -> None:
+    """List bundled skills the user has edited (which `hermes update` keeps)."""
+    from tools.skills_sync import list_user_modified_bundled_skills
+
+    c = console or _console
+    modified = list_user_modified_bundled_skills()
+
+    if as_json:
+        import json
+
+        c.print(json.dumps([m["name"] for m in modified]))
+        return
+
+    if not modified:
+        c.print("[dim]No user-modified bundled skills — everything tracks upstream.[/]\n")
+        return
+
+    c.print(f"\n[bold]{len(modified)} user-modified bundled skill(s)[/] "
+            "[dim](kept as-is by `hermes update`):[/]")
+    for entry in modified:
+        c.print(f"  [yellow]~[/] {entry['name']}")
+    c.print()
+    c.print("[dim]See changes:   hermes skills diff <name>[/]")
+    c.print("[dim]Resume updates: hermes skills reset <name>          (keep your copy, re-baseline)[/]")
+    c.print("[dim]Revert to stock: hermes skills reset <name> --restore[/]\n")
+
+
+def do_diff(name: str, console: Optional[Console] = None) -> None:
+    """Show how the user's copy of a bundled skill differs from the stock version."""
+    from tools.skills_sync import diff_bundled_skill
+
+    c = console or _console
+    result = diff_bundled_skill(name)
+
+    if not result["ok"]:
+        c.print(f"[bold red]Error:[/] {result['message']}\n")
+        return
+
+    if not result["modified"]:
+        c.print(f"[green]{result['message']}[/]\n")
+        return
+
+    c.print(f"\n[bold]{result['message']}[/]\n")
+    for entry in result["diffs"]:
+        status = entry["status"]
+        if status == "modified":
+            # Render the unified diff with light coloring.
+            for line in entry["diff"].splitlines():
+                if line.startswith("+") and not line.startswith("+++"):
+                    c.print(f"[green]{line}[/]")
+                elif line.startswith("-") and not line.startswith("---"):
+                    c.print(f"[red]{line}[/]")
+                elif line.startswith("@@"):
+                    c.print(f"[cyan]{line}[/]")
+                else:
+                    c.print(line, highlight=False)
+        elif status == "added":
+            c.print(f"[green]+ only in your copy:[/] {entry['path']}")
+        elif status == "removed":
+            c.print(f"[red]- only in stock:[/] {entry['path']}")
+        else:  # binary
+            c.print(f"[yellow]~ {entry['path']}:[/] binary file differs")
+    c.print()
+    c.print(f"[dim]Revert with: hermes skills reset {name} --restore[/]\n")
+
+
 def do_opt_out(remove: bool = False,
                console: Optional[Console] = None,
                skip_confirm: bool = False,
@@ -1624,6 +1691,10 @@ def skills_command(args) -> None:
     elif action == "reset":
         do_reset(args.name, restore=getattr(args, "restore", False),
                  skip_confirm=getattr(args, "yes", False))
+    elif action == "list-modified":
+        do_list_modified(as_json=getattr(args, "json", False))
+    elif action == "diff":
+        do_diff(args.name)
     elif action == "opt-out":
         do_opt_out(remove=getattr(args, "remove", False),
                    skip_confirm=getattr(args, "yes", False))
@@ -1654,7 +1725,7 @@ def skills_command(args) -> None:
             return
         do_tap(tap_action, repo=repo)
     else:
-        _console.print("Usage: hermes skills [browse|search|install|inspect|list|check|update|audit|uninstall|reset|opt-out|opt-in|publish|snapshot|tap]\n")
+        _console.print("Usage: hermes skills [browse|search|install|inspect|list|list-modified|diff|check|update|audit|uninstall|reset|opt-out|opt-in|publish|snapshot|tap]\n")
         _console.print("Run 'hermes skills <command> --help' for details.\n")
 
 
@@ -1826,6 +1897,15 @@ def handle_skills_slash(cmd: str, console: Optional[Console] = None) -> None:
         do_reset(name, restore=restore, console=c, skip_confirm=True,
                  invalidate_cache=invalidate_cache)
 
+    elif action in {"list-modified", "modified"}:
+        do_list_modified(console=c, as_json="--json" in args)
+
+    elif action == "diff":
+        if not args:
+            c.print("[bold red]Usage:[/] /skills diff <name>\n")
+            return
+        do_diff(args[0], console=c)
+
     elif action == "publish":
         if not args:
             c.print("[bold red]Usage:[/] /skills publish <skill-path> [--to github] [--repo owner/repo]\n")
@@ -1883,6 +1963,8 @@ def _print_skills_help(console: Console) -> None:
         "  [cyan]update[/] [name]               Update hub skills with upstream changes\n"
         "  [cyan]audit[/] [name]                Re-scan hub skills for security\n"
         "  [cyan]uninstall[/] <name>            Remove a hub-installed skill\n"
+        "  [cyan]list-modified[/]               List bundled skills you've edited (kept by update)\n"
+        "  [cyan]diff[/] <name>                 Diff your copy of a bundled skill vs the stock version\n"
         "  [cyan]reset[/] <name> [--restore]    Reset bundled-skill tracking (fix 'user-modified' flag)\n"
         "  [cyan]publish[/] <path> --repo <r>   Publish a skill to GitHub via PR\n"
         "  [cyan]snapshot[/] export|import      Export/import skill configurations\n"

hermes_cli/subcommands/skills.py

@@ -164,6 +164,35 @@ def build_skills_parser(subparsers, *, cmd_skills: Callable) -> None:
         help="Skip confirmation prompt when using --restore",
     )
 
+    skills_list_modified = skills_subparsers.add_parser(
+        "list-modified",
+        help="List bundled skills you've edited (which `hermes update` keeps)",
+        description=(
+            "Show the bundled skills whose local copy differs from the version last "
+            "synced, i.e. the ones `hermes update` reports as user-modified and skips. "
+            "Use `hermes skills diff <name>` to see changes and `hermes skills reset "
+            "<name>` to resume updates."
+        ),
+    )
+    skills_list_modified.add_argument(
+        "--json",
+        action="store_true",
+        help="Output the list as JSON",
+    )
+
+    skills_diff = skills_subparsers.add_parser(
+        "diff",
+        help="Show how your copy of a bundled skill differs from the stock version",
+        description=(
+            "Print a unified diff between your local copy of a bundled skill and the "
+            "current bundled (stock) version, so you can confirm what changed before "
+            "running `hermes skills reset`."
+        ),
+    )
+    skills_diff.add_argument(
+        "name", help="Skill name to diff (e.g. google-workspace)"
+    )
+
     skills_opt_out = skills_subparsers.add_parser(
         "opt-out",
         help="Stop bundled skills from being seeded into this profile",

tests/hermes_cli/test_update_modified_notice.py

@@ -0,0 +1,50 @@
+"""Guard: every `hermes update` path that reports user-modified skills must
+also tell the user how to find them.
+
+`hermes update` keeps (does not overwrite) bundled skills the user edited and
+prints a ``~ N user-modified (kept)`` count. There are two independent update
+code paths in ``hermes_cli/main.py`` that print this notice (the git-pull path
+in ``_cmd_update_impl`` and the unpack/install path). Both must point the user
+at ``hermes skills list-modified`` so the count is actionable — otherwise,
+depending on which path a user hits, they may never learn the discovery command
+exists.
+
+This is an *invariant* test (the two sibling notices must agree), not a literal
+snapshot: it asserts the relationship "count line ⇒ discovery hint", so it
+keeps holding if the wording is reworded, as long as both sites stay in sync.
+"""
+
+import re
+from pathlib import Path
+
+import hermes_cli.main as main_mod
+
+
+_COUNT_RE = re.compile(r"user-modified \(kept\)")
+_HINT_RE = re.compile(r"hermes skills list-modified")
+
+
+def _source_lines() -> list[str]:
+    return Path(main_mod.__file__).read_text(encoding="utf-8").splitlines()
+
+
+def test_every_user_modified_notice_points_at_list_modified():
+    lines = _source_lines()
+    count_sites = [i for i, ln in enumerate(lines) if _COUNT_RE.search(ln)]
+
+    # There are at least two such notices today; the bug was that only one of
+    # them carried the discovery hint. Assert each is followed (within a small
+    # window — the count print + the hint print) by the list-modified pointer.
+    assert len(count_sites) >= 2, (
+        "expected at least two 'user-modified (kept)' notices in main.py; "
+        f"found {len(count_sites)}"
+    )
+
+    for idx in count_sites:
+        window = "\n".join(lines[idx : idx + 5])
+        assert _HINT_RE.search(window), (
+            "a 'user-modified (kept)' notice near line "
+            f"{idx + 1} of main.py does not point users at "
+            "`hermes skills list-modified` within the following lines — the "
+            "two update paths have drifted apart again:\n" + window
+        )

tests/tools/test_skills_list_modified_diff.py

@@ -0,0 +1,132 @@
+"""Tests for discovering and diffing user-modified bundled skills.
+
+`hermes update` keeps (does not overwrite) bundled skills the user edited
+locally, but historically only printed a *count* — there was no way to find
+which skills, or see what changed. These tests cover the two helpers that close
+that gap, exercising the real sync pipeline (no mocks of the comparison logic):
+
+* ``list_user_modified_bundled_skills()`` — the discovery half of the exact
+  test the sync loop uses to decide what to skip.
+* ``diff_bundled_skill()`` — a unified diff of the user copy vs the stock copy.
+
+Revert already exists (``reset_bundled_skill``); the last test confirms it
+clears the modified state so the two stay consistent.
+"""
+
+from contextlib import ExitStack
+from unittest.mock import patch
+
+from tools.skills_sync import (
+    sync_skills,
+    reset_bundled_skill,
+    list_user_modified_bundled_skills,
+    diff_bundled_skill,
+)
+
+
+def _make_bundled(tmp_path):
+    """A fake bundled skills tree with one skill: category/foo."""
+    bundled = tmp_path / "bundled_skills"
+    foo = bundled / "category" / "foo"
+    foo.mkdir(parents=True)
+    (foo / "SKILL.md").write_text("---\nname: foo\n---\n# Foo Skill\n")
+    (foo / "helper.py").write_text("print('stock')\n")
+    return bundled
+
+
+def _patches(bundled, skills_dir, manifest_file):
+    stack = ExitStack()
+    stack.enter_context(
+        patch("tools.skills_sync._get_bundled_dir", return_value=bundled)
+    )
+    stack.enter_context(
+        patch(
+            "tools.skills_sync._get_optional_dir",
+            return_value=bundled.parent / "optional-skills",
+        )
+    )
+    stack.enter_context(patch("tools.skills_sync.SKILLS_DIR", skills_dir))
+    stack.enter_context(patch("tools.skills_sync.MANIFEST_FILE", manifest_file))
+    return stack
+
+
+def _env(tmp_path):
+    bundled = _make_bundled(tmp_path)
+    skills_dir = tmp_path / "user_skills"
+    manifest_file = skills_dir / ".bundled_manifest"
+    return bundled, skills_dir, manifest_file
+
+
+def test_pristine_skill_is_not_listed_as_modified(tmp_path):
+    bundled, skills_dir, manifest_file = _env(tmp_path)
+    with _patches(bundled, skills_dir, manifest_file):
+        sync_skills(quiet=True)
+        assert list_user_modified_bundled_skills() == []
+
+
+def test_edited_skill_is_listed_as_modified(tmp_path):
+    bundled, skills_dir, manifest_file = _env(tmp_path)
+    with _patches(bundled, skills_dir, manifest_file):
+        sync_skills(quiet=True)
+        (skills_dir / "category" / "foo" / "helper.py").write_text("print('mine')\n")
+
+        modified = list_user_modified_bundled_skills()
+        names = [m["name"] for m in modified]
+        assert names == ["foo"]
+        entry = modified[0]
+        assert entry["dest"] == skills_dir / "category" / "foo"
+        assert entry["bundled_src"] == bundled / "category" / "foo"
+
+
+def test_diff_reports_no_changes_when_pristine(tmp_path):
+    bundled, skills_dir, manifest_file = _env(tmp_path)
+    with _patches(bundled, skills_dir, manifest_file):
+        sync_skills(quiet=True)
+        result = diff_bundled_skill("foo")
+        assert result["ok"] is True
+        assert result["modified"] is False
+        assert result["diffs"] == []
+
+
+def test_diff_shows_modified_and_added_files(tmp_path):
+    bundled, skills_dir, manifest_file = _env(tmp_path)
+    with _patches(bundled, skills_dir, manifest_file):
+        sync_skills(quiet=True)
+        user_foo = skills_dir / "category" / "foo"
+        (user_foo / "helper.py").write_text("print('mine')\n")
+        (user_foo / "extra.txt").write_text("local note\n")
+
+        result = diff_bundled_skill("foo")
+        assert result["ok"] is True
+        assert result["modified"] is True
+
+        by_path = {d["path"]: d for d in result["diffs"]}
+        assert by_path["helper.py"]["status"] == "modified"
+        # The unified diff shows the user's line replacing the stock line.
+        assert "print('mine')" in by_path["helper.py"]["diff"]
+        assert "print('stock')" in by_path["helper.py"]["diff"]
+        # A file only in the user copy is reported as added.
+        assert by_path["extra.txt"]["status"] == "added"
+
+
+def test_diff_unknown_skill_is_not_ok(tmp_path):
+    bundled, skills_dir, manifest_file = _env(tmp_path)
+    with _patches(bundled, skills_dir, manifest_file):
+        sync_skills(quiet=True)
+        result = diff_bundled_skill("does-not-exist")
+        assert result["ok"] is False
+        assert result["found"] is False
+
+
+def test_reset_clears_modified_state(tmp_path):
+    """Revert (existing) and discovery (new) must agree: after reset, not modified."""
+    bundled, skills_dir, manifest_file = _env(tmp_path)
+    with _patches(bundled, skills_dir, manifest_file):
+        sync_skills(quiet=True)
+        (skills_dir / "category" / "foo" / "helper.py").write_text("print('mine')\n")
+        assert [m["name"] for m in list_user_modified_bundled_skills()] == ["foo"]
+
+        # Restore from the stock source, then it must no longer be flagged.
+        result = reset_bundled_skill("foo", restore=True)
+        assert result["ok"] is True
+        assert list_user_modified_bundled_skills() == []

tools/skills_sync.py

@@ -30,7 +30,7 @@ from datetime import datetime, timezone
 from pathlib import Path, PurePosixPath
 from hermes_constants import get_bundled_skills_dir, get_hermes_home, get_optional_skills_dir
 from agent.skill_utils import is_excluded_skill_path
-from typing import Dict, List, Tuple
+from typing import Dict, List, Optional, Tuple
 from utils import atomic_replace
 
 logger = logging.getLogger(__name__)
@@ -785,6 +785,160 @@ def reset_bundled_skill(name: str, restore: bool = False) -> dict:
     return {"ok": True, "action": action, "message": message, "synced": synced}
 
 
+def list_user_modified_bundled_skills() -> List[dict]:
+    """Return the bundled skills that ``hermes update`` keeps because the user
+    edited them locally.
+
+    A skill counts as user-modified when its on-disk copy no longer matches the
+    origin hash recorded in the manifest the last time it was synced — the exact
+    same test the sync loop uses to decide what to skip. This is the discovery
+    half of that behavior, so a user can find the names the ``~ N user-modified
+    (kept)`` notice only counts.
+
+    Returns a list (sorted by name) of dicts:
+        ``{"name": str, "dest": Path, "bundled_src": Path}``
+    where ``dest`` is the user's copy and ``bundled_src`` is the current stock
+    copy (so callers can diff or restore).
+    """
+    manifest = _read_manifest()
+    if not manifest:
+        return []
+    bundled_dir = _get_bundled_dir()
+    modified: List[dict] = []
+    for skill_name, skill_dir in _discover_bundled_skills(bundled_dir):
+        origin_hash = manifest.get(skill_name)
+        # No entry, or a v1 entry not yet baselined (empty hash): not a tracked
+        # modification — the next sync handles it.
+        if not origin_hash:
+            continue
+        dest = _compute_relative_dest(skill_dir, bundled_dir)
+        if not dest.exists():
+            continue
+        if _dir_hash(dest) != origin_hash:
+            modified.append(
+                {"name": skill_name, "dest": dest, "bundled_src": skill_dir}
+            )
+    modified.sort(key=lambda e: e["name"])
+    return modified
+
+
+def _read_text_for_diff(path: Path) -> Optional[str]:
+    """Return file text for diffing, or ``None`` if the file is binary/unreadable."""
+    try:
+        data = path.read_bytes()
+    except OSError:
+        return None
+    if b"\x00" in data:
+        return None
+    try:
+        return data.decode("utf-8")
+    except UnicodeDecodeError:
+        return None
+
+
+def diff_bundled_skill(name: str) -> dict:
+    """Diff a user's copy of a bundled skill against the current stock version.
+
+    Lets a user see exactly what diverged before deciding whether to keep their
+    edits or ``hermes skills reset`` back to upstream.
+
+    Returns a dict:
+        ``ok`` (bool), ``name`` (str), ``found`` (bool — bundled source exists),
+        ``user_present`` (bool), ``modified`` (bool), ``message`` (str),
+        ``diffs``: list of ``{"path": str, "status": str, "diff": str}`` where
+        status is one of ``modified`` / ``added`` (only in user copy) /
+        ``removed`` (only in bundled) / ``binary``.
+    """
+    import difflib
+
+    bundled_dir = _get_bundled_dir()
+    bundled_by_name = dict(_discover_bundled_skills(bundled_dir))
+    bundled_src = bundled_by_name.get(name)
+    if bundled_src is None:
+        return {
+            "ok": False,
+            "name": name,
+            "found": False,
+            "user_present": False,
+            "modified": False,
+            "diffs": [],
+            "message": (
+                f"'{name}' is not a tracked bundled skill (no stock version to "
+                f"diff against). Hub-installed skills use `hermes skills inspect`."
+            ),
+        }
+    dest = _compute_relative_dest(bundled_src, bundled_dir)
+    if not dest.exists():
+        return {
+            "ok": False,
+            "name": name,
+            "found": True,
+            "user_present": False,
+            "modified": False,
+            "diffs": [],
+            "message": f"No local copy of '{name}' found at {dest}.",
+        }
+
+    user_files = {
+        p.relative_to(dest).as_posix() for p in dest.rglob("*") if p.is_file()
+    }
+    stock_files = {
+        p.relative_to(bundled_src).as_posix()
+        for p in bundled_src.rglob("*")
+        if p.is_file()
+    }
+
+    diffs: List[dict] = []
+    for rel in sorted(user_files | stock_files):
+        in_user = rel in user_files
+        in_stock = rel in stock_files
+        user_text = _read_text_for_diff(dest / rel) if in_user else None
+        stock_text = _read_text_for_diff(bundled_src / rel) if in_stock else None
+
+        if in_user and in_stock:
+            if user_text is None or stock_text is None:
+                # At least one side is binary — report only if bytes differ.
+                if (dest / rel).read_bytes() != (bundled_src / rel).read_bytes():
+                    diffs.append(
+                        {"path": rel, "status": "binary", "diff": "<binary file differs>"}
+                    )
+                continue
+            if user_text == stock_text:
+                continue
+            text = "".join(
+                difflib.unified_diff(
+                    stock_text.splitlines(keepends=True),
+                    user_text.splitlines(keepends=True),
+                    fromfile=f"stock/{rel}",
+                    tofile=f"yours/{rel}",
+                )
+            )
+            diffs.append({"path": rel, "status": "modified", "diff": text})
+        elif in_user:
+            diffs.append(
+                {"path": rel, "status": "added", "diff": f"+ only in your copy: {rel}"}
+            )
+        else:
+            diffs.append(
+                {"path": rel, "status": "removed", "diff": f"- only in stock: {rel}"}
+            )
+
+    modified = bool(diffs)
+    return {
+        "ok": True,
+        "name": name,
+        "found": True,
+        "user_present": True,
+        "modified": modified,
+        "diffs": diffs,
+        "message": (
+            f"'{name}' matches the stock version."
+            if not modified
+            else f"'{name}' differs from the stock version in {len(diffs)} file(s)."
+        ),
+    }
+
+
 def set_bundled_skills_opt_out(enabled: bool) -> dict:
     """Toggle the .no-bundled-skills opt-out marker for the active profile.