fix(skills): restoring a built-in skill no longer fails on leftovers, and a failed restore is recorded as bundled

Post-fix telemetry still showed `hermes skills install llm-wiki` failing as
failure_class=filesystem_error, source=hub. Two restore paths failed for good:

- A skill dir left behind without its SKILL.md (a manual delete that kept user
  files, or a copy that died partway) made copytree raise EEXIST on every retry.
  The restore now copies around the leftovers (dirs_exist_ok); a dir with no
  SKILL.md is no skill, and user files in it are kept.
- A skill present behind a symlinked category (which the loader follows, so it
  works) was missed by the rglob scan, so the restore ran into the containment
  guard and failed. The canonical SKILL.md now counts as present.

A restore that still fails (read-only dir, a symlinked category with the skill
missing) was recorded as source=hub, because the failed exit returned no bundle
and _record_skill_install fell back to "hub". The bundled branch now marks the
attempt origin=bundled, so every bundled-path outcome, raised or returned,
records source=bundled.

hermes_cli/skills_hub.py

@@ -778,7 +778,7 @@ def _record_skill_install(identifier: str, bundle, outcome: str, attempt: Option
     its type inside the metrics guard."""
     from hermes_cli.observability.shared_metrics_events import record_extension_install
     attempt = attempt or {}
-    origin = getattr(bundle, "source", None) or (
+    origin = getattr(bundle, "source", None) or attempt.get("origin") or (
         "official" if identifier.startswith("official/")
         else "url" if identifier.startswith(("http://", "https://")) else "hub")
     source = _SKILL_METRIC_SOURCES.get(origin, "hub")
@@ -835,7 +835,8 @@ def _install_skill(identifier: str, category: str, force: bool, c: Console, skip
     ensure_hub_dirs()
     from tools.skills_sync_bundled_ops import bundled_skill_for_install
     if not source_id and not name_override and (builtin := bundled_skill_for_install(identifier)):
-        attempt["failure_class"] = "filesystem_error"  # read only when the restore fails
+        # Read only when the restore fails or raises: either way it was a bundled install, not a hub one.
+        attempt.update(failure_class="filesystem_error", origin="bundled")
         return _install_bundled(c, builtin, invalidate_cache)
     attempt["registry"] = "unresolved"
     sources = _pinned_sources(c, _sources(), source_id, identifier)

tests/hermes_cli/test_skills_hub.py

@@ -744,3 +744,37 @@ def test_install_by_name_resolves_to_the_skill_it_names(monkeypatch, tmp_path, i
     assert f"'{name}' is a built-in skill" in error and f"`hermes skills uninstall {name}`" in error
     assert "already available" not in sink.getvalue()
     assert (stranger / "SKILL.md").read_text().endswith("stranger\n")
+
+
+def test_built_in_restore_survives_leftovers_and_records_every_outcome_as_bundled(monkeypatch, tmp_path):
+    """Post-fix telemetry still showed `llm-wiki` failing as source=hub, filesystem_error: the
+    restore copied with copytree onto a dest dir left behind without its SKILL.md (EEXIST on every
+    retry), refused a skill already present behind a symlinked category the loader follows, and a
+    failed restore was labelled a hub install."""
+    import hermes_cli.skills_hub as cli_hub
+    import tools.skills_hub as hub
+    from tools import skill_usage, skills_sync
+
+    for stale in [n for n in vars(hub) if n.isupper() and n.endswith(("_DIR", "_FILE", "_LOG"))]:
+        monkeypatch.delitem(vars(hub), stale)
+    monkeypatch.setattr(cli_hub, "_sources", lambda: pytest.fail("a bundled skill must not hit the hub"))
+    rows = []
+    monkeypatch.setattr("hermes_cli.observability.shared_metrics_events.record_extension_install",
+                        lambda **row: rows.append((row["source"], row["outcome"])))
+    src = dict(skills_sync._discover_bundled_skills(skills_sync._get_bundled_dir()))["llm-wiki"]
+    dest = skill_usage._skills_dir() / src.parent.name / "llm-wiki"
+    console, _sink = _sink_console()
+
+    (dest / "wiki").mkdir(parents=True)  # stranded: user data kept, SKILL.md gone
+    (dest / "wiki" / "index.md").write_text("mine\n")
+    assert cli_hub.do_install("llm-wiki", console=console, skip_confirm=True) is True
+    assert (dest / "SKILL.md").is_file() and (dest / "wiki" / "index.md").read_text() == "mine\n"
+
+    real = tmp_path / "real-category"
+    dest.parent.rename(real)
+    dest.parent.symlink_to(real, target_is_directory=True)
+    assert cli_hub.do_install("llm-wiki", console=console, skip_confirm=True) is None  # present, no-op
+
+    (real / "llm-wiki" / "SKILL.md").unlink()  # missing behind the symlink: refused, and still bundled
+    assert cli_hub.do_install("llm-wiki", console=console, skip_confirm=True) is False
+    assert rows == [("bundled", "success"), ("bundled", "failed")]

tools/skills_sync_bundled_ops.py

@@ -1,10 +1,11 @@
 """Bundled-skill maintenance ops: reset, diff, list-modified, opt-out, remove-pristine.
 Profile-scoped paths and patchable helpers resolve through ``_ss()`` at call time."""
 
+import shutil
 from pathlib import Path
 from typing import List, Optional, Tuple
 
-from tools.skills_sync_optional import _skill_file_list, _ss
+from tools.skills_sync_optional import _ignore_runtime_cache, _skill_file_list, _ss
 
 
 def _bundled_state():
@@ -88,15 +89,21 @@ def ensure_bundled_skill(name: str) -> dict:
             f"'{name}' is a built-in skill, but a skill installed from the hub uses the same name and "
             f"shadows it. To get the built-in back: `hermes skills uninstall {name}`, then "
             f"`hermes skills install {name}`.")}
+    # The rglob walk does not descend a symlinked category the loader follows, so the canonical
+    # SKILL.md counts as present too.
     active = next((md.parent for md in ss._iter_active_skill_mds()
-                   if ss._read_skill_name(md, md.parent.name) == name), None)
+                   if ss._read_skill_name(md, md.parent.name) == name), None) or (
+        dest if (dest / "SKILL.md").is_file() else None)
     if active is not None or name in ss._build_external_skill_index():
         return {"ok": True, "action": "present", "path": active, "message": ""}
     try:  # the hub's containment check: a symlinked category must not carry the copy out of skills/
         from tools.skills_hub_install import _resolve_lock_install_path
         rel = dest.relative_to(ss._skills_dir()).as_posix()
         _resolve_lock_install_path(rel, dest.name)
-        ss._copy_dir(src, dest)
+        # A dir with no SKILL.md (a delete or earlier copy that died partway) is no skill; copying
+        # around its leftovers beats failing with EEXIST on every retry.
+        dest.parent.mkdir(parents=True, exist_ok=True)
+        shutil.copytree(src, dest, ignore=_ignore_runtime_cache, dirs_exist_ok=True)
     except (OSError, ValueError) as e:
         return {"ok": False, "action": "not_restored", "path": dest,
                 "message": f"Could not copy the built-in skill '{name}' to {dest}: {e}"}