fix: harden skill-discovery cache signature + TTL

Review findings on the cherry-picked cache (follow-up to #58985):

- The cache key was the max mtime of only the TOP-LEVEL scan dirs.
  Adding/removing a skill inside a category subdir bumps the category
  dir's mtime, NOT the root's, so the cache served a stale list
  indefinitely. Replace with a per-dir signature covering roots +
  immediate children (one scandir per dir; mirrors
  hermes_cli/profiles.py::_count_skills from d5eee133e).
- The disabled-set is config-driven and changes with no filesystem
  mtime bump; fold it into the signature so /skills disable takes
  effect without a restart.
- Platform is part of the signature (gateway processes serve multiple
  platform scopes; scan results are platform-filtered).
- Add a 30s TTL to bound staleness from in-place SKILL.md edits (file
  mtime is invisible to any directory signature).
- The original also keyed dirs off the module-level SKILLS_DIR constant;
  the scan itself uses _skills_dir() (live profile HERMES_HOME) — use
  the same resolution for the signature.

Mutation-verified: nested-add, disabled-set, and TTL tests fail against
the pre-fix cache and pass with it.

tests/tools/test_skills_tool_discovery_cache.py

@@ -0,0 +1,121 @@
+"""Regression tests for the _find_all_skills discovery cache (#58985 salvage).
+
+Covers the cache-signature fix layered on the cherry-picked contributor
+commit: the original keyed the cache on the max mtime of only the TOP-LEVEL
+scan dirs, so adding/removing a skill inside a category subdir (which bumps
+the category dir's mtime, not the root's) served a stale list indefinitely.
+The signature now covers roots + immediate children (mirroring
+hermes_cli/profiles.py::_count_skills) plus the disabled-set, with a short
+TTL bounding in-place SKILL.md edit staleness.
+"""
+
+import time
+
+import pytest
+
+import tools.skills_tool as st
+
+
+@pytest.fixture(autouse=True)
+def _fresh_cache(monkeypatch, tmp_path):
+    """Isolate every test: clear the module cache and point the scan at
+    an empty external-dirs list + a tmp skills root."""
+    st._SKILLS_CACHE.clear()
+    monkeypatch.setattr(st, "_skills_dir", lambda: tmp_path / "skills")
+    monkeypatch.setattr(
+        "agent.skill_utils.get_external_skills_dirs", lambda: []
+    )
+    monkeypatch.setattr(st, "_get_disabled_skill_names", lambda: set())
+    yield
+    st._SKILLS_CACHE.clear()
+
+
+def _write_skill(root, category, name, description="a skill"):
+    d = root / "skills" / category / name
+    d.mkdir(parents=True, exist_ok=True)
+    (d / "SKILL.md").write_text(
+        f"---\nname: {name}\ndescription: {description}\n---\n# {name}\n",
+        encoding="utf-8",
+    )
+    return d
+
+
+def test_cache_hit_serves_same_result_without_rescan(tmp_path):
+    _write_skill(tmp_path, "cat-a", "skill-one")
+    first = st._find_all_skills()
+    assert [s["name"] for s in first] == ["skill-one"]
+
+    second = st._find_all_skills()
+    assert second is first  # cache hit returns the SAME object, no rescan
+
+
+def test_nested_category_skill_add_invalidates(tmp_path):
+    """THE bug in the original PR: a new skill inside an existing category
+    bumps the category dir's mtime only — the root-mtime key missed it."""
+    _write_skill(tmp_path, "cat-a", "skill-one")
+    first = st._find_all_skills()
+    assert [s["name"] for s in first] == ["skill-one"]
+
+    # Freeze the ROOT dir's mtime so only the category-child signature moves
+    # (guards against filesystems bumping the parent too).
+    root = tmp_path / "skills"
+    root_stat = root.stat()
+    _write_skill(tmp_path, "cat-a", "skill-two")
+    import os
+    os.utime(root, (root_stat.st_atime, root_stat.st_mtime))
+
+    names = sorted(s["name"] for s in st._find_all_skills())
+    assert names == ["skill-one", "skill-two"], (
+        "category-nested skill add must invalidate the cache"
+    )
+
+
+def test_disabled_set_change_invalidates(tmp_path, monkeypatch):
+    """Disabling a skill is a config change with NO filesystem mtime bump —
+    it must still invalidate."""
+    _write_skill(tmp_path, "cat-a", "skill-one")
+    _write_skill(tmp_path, "cat-a", "skill-two")
+    names = sorted(s["name"] for s in st._find_all_skills())
+    assert names == ["skill-one", "skill-two"]
+
+    monkeypatch.setattr(st, "_get_disabled_skill_names", lambda: {"skill-two"})
+    names = sorted(s["name"] for s in st._find_all_skills())
+    assert names == ["skill-one"], "disabled-set change must invalidate the cache"
+
+
+def test_ttl_expiry_forces_rescan(tmp_path, monkeypatch):
+    """In-place SKILL.md edits are invisible to any directory signature;
+    the TTL bounds that staleness."""
+    skill_dir = _write_skill(tmp_path, "cat-a", "skill-one", "old description")
+    first = st._find_all_skills()
+    assert first[0]["description"] == "old description"
+
+    # Edit the file in place; keep every directory mtime identical.
+    import os
+    cat = tmp_path / "skills" / "cat-a"
+    root = tmp_path / "skills"
+    stats = {p: p.stat() for p in (root, cat, skill_dir)}
+    (skill_dir / "SKILL.md").write_text(
+        "---\nname: skill-one\ndescription: new description\n---\n# skill-one\n",
+        encoding="utf-8",
+    )
+    for p, s in stats.items():
+        os.utime(p, (s.st_atime, s.st_mtime))
+
+    # Within TTL: stale (documented trade-off).
+    assert st._find_all_skills()[0]["description"] == "old description"
+
+    # Past TTL: fresh.
+    monkeypatch.setattr(st, "_SKILLS_CACHE_TTL_SECONDS", 0.0)
+    assert st._find_all_skills()[0]["description"] == "new description"
+
+
+def test_disabled_and_full_views_cached_separately(tmp_path, monkeypatch):
+    _write_skill(tmp_path, "cat-a", "skill-one")
+    _write_skill(tmp_path, "cat-a", "skill-two")
+    monkeypatch.setattr(st, "_get_disabled_skill_names", lambda: {"skill-two"})
+
+    filtered = sorted(s["name"] for s in st._find_all_skills())
+    everything = sorted(s["name"] for s in st._find_all_skills(skip_disabled=True))
+    assert filtered == ["skill-one"]
+    assert everything == ["skill-one", "skill-two"]

tools/skills_tool.py

@@ -68,6 +68,7 @@ Usage:
 
 import json
 import logging
+import time
 
 from hermes_constants import get_hermes_home, display_hermes_home
 import os
@@ -88,14 +89,53 @@ logger = logging.getLogger(__name__)
 
 # Per-session skill discovery cache.  _find_all_skills() re-reads every
 # SKILL.md on every call; with hundreds of skills this is wasteful.
-# We cache by the max mtime across all scanned skill directories so a
-# write by skill_manage (which touches the directory) invalidates the
-# cache automatically.  skip_disabled True/False are cached separately.
-_SKILLS_CACHE: dict = {}          # {cache_key: (max_mtime, skills_list)}
+# Cache validation (mirrors hermes_cli/profiles.py::_count_skills, d5eee133e):
+#   - signature = per-dir max mtime of the dir AND its immediate children
+#     (one scandir per dir; catches skill add/remove inside categories,
+#     which does NOT bump the root dir's mtime), plus the disabled-set
+#     (config-driven — changes with no filesystem mtime bump at all)
+#   - a short TTL bounds staleness from in-place SKILL.md edits, which
+#     bump only the file's mtime, invisible to any directory signature.
+# skip_disabled True/False are cached separately.
+_SKILLS_CACHE: dict = {}          # {cache_key: (signature, timestamp, skills_list)}
+_SKILLS_CACHE_TTL_SECONDS = 30.0
 _SKILLS_CACHE_KEY_DISABLED = "with_disabled"
 _SKILLS_CACHE_KEY_FILTERED = "filtered"
 
 
+def _skills_scan_signature(dirs_to_scan, disabled) -> tuple:
+    """Cheap change-signature for the skill scan inputs.
+
+    O(#dirs + #categories) stat calls, not a recursive walk. Includes the
+    platform the scan's ``skill_matches_platform`` filter will use (read
+    from ``agent.skill_utils``'s ``sys`` so test patches of that module
+    are honored) — the scan result is platform-dependent.
+    """
+    from agent import skill_utils as _skill_utils
+
+    platform = getattr(getattr(_skill_utils, "sys", None), "platform", "")
+    sig = []
+    for d in dirs_to_scan:
+        try:
+            m = d.stat().st_mtime
+        except OSError:
+            continue
+        try:
+            with os.scandir(d) as it:
+                for entry in it:
+                    try:
+                        if entry.is_dir(follow_symlinks=False):
+                            em = entry.stat(follow_symlinks=False).st_mtime
+                            if em > m:
+                                m = em
+                    except OSError:
+                        continue
+        except OSError:
+            pass
+        sig.append((str(d), m))
+    return (tuple(sig), frozenset(disabled), platform)
+
+
 # All skills live in ~/.hermes/skills/ (seeded from bundled skills/ on install).
 # This is the single source of truth -- agent edits, hub installs, and bundled
 # skills all coexist here without polluting the git repo.
@@ -637,50 +677,43 @@ def _find_all_skills(*, skip_disabled: bool = False) -> List[Dict[str, Any]]:
     Returns:
         List of skill metadata dicts (name, description, category).
 
-    Results are cached per-session; the cache is invalidated when any
-    scanned skill directory's mtime changes (a skill write touches the
-    directory, triggering a re-scan on the next call).
+    Results are cached per-session; the cache is invalidated when the scan
+    signature changes (dir/category mtimes or the disabled-set) and expires
+    after a short TTL to bound staleness from in-place SKILL.md edits.
     """
     from agent.skill_utils import get_external_skills_dirs, iter_skill_index_files
 
     cache_key = _SKILLS_CACHE_KEY_DISABLED if skip_disabled else _SKILLS_CACHE_KEY_FILTERED
 
-    # Collect directories to scan (same set as the scan loop below).
-    dirs_to_scan: list = []
-    if SKILLS_DIR.exists():
-        dirs_to_scan.append(SKILLS_DIR)
-    dirs_to_scan.extend(get_external_skills_dirs())
-
-    # Compute the freshest mtime across all scanned directories.
-    max_mtime = 0.0
-    for d in dirs_to_scan:
-        try:
-            st = d.stat()
-            if st.st_mtime > max_mtime:
-                max_mtime = st.st_mtime
-        except OSError:
-            continue
-
-    # Serve from cache when nothing changed since last scan.
-    cached = _SKILLS_CACHE.get(cache_key)
-    if cached is not None:
-        cached_mtime, cached_skills = cached
-        if cached_mtime >= max_mtime:
-            return cached_skills
-
-    skills = []
-    seen_names: set = set()
-
-    # Load disabled set once (not per-skill)
+    # Load disabled set once (not per-skill). Part of the cache signature:
+    # disabling a skill is a config change with no filesystem mtime bump.
     disabled = set() if skip_disabled else _get_disabled_skill_names()
 
-    # Scan local dir first, then external dirs (local takes precedence)
-    dirs_to_scan = []
+    # Collect directories to scan — same resolution as the scan loop below
+    # (_skills_dir() resolves the LIVE profile HERMES_HOME; the module-level
+    # SKILLS_DIR can be stale in long-lived runtimes).
+    dirs_to_scan: list = []
     active_skills_dir = _skills_dir()
     if active_skills_dir.exists():
         dirs_to_scan.append(active_skills_dir)
     dirs_to_scan.extend(get_external_skills_dirs())
 
+    signature = _skills_scan_signature(dirs_to_scan, disabled)
+    now = time.monotonic()
+
+    cached = _SKILLS_CACHE.get(cache_key)
+    if (
+        cached is not None
+        and cached[0] == signature
+        and (now - cached[1]) < _SKILLS_CACHE_TTL_SECONDS
+    ):
+        return cached[2]
+
+    skills = []
+    seen_names: set = set()
+
+    # Scan local dir first, then external dirs (local takes precedence) —
+    # dirs_to_scan already resolved above for the signature.
     for scan_dir in dirs_to_scan:
         for skill_md in iter_skill_index_files(scan_dir, "SKILL.md"):
             if any(part in _EXCLUDED_SKILL_DIRS for part in skill_md.parts):
@@ -733,10 +766,10 @@ def _find_all_skills(*, skip_disabled: bool = False) -> List[Dict[str, Any]]:
                 )
                 continue
 
-    # Store in cache keyed by the freshest directory mtime seen during
-    # this scan.  Any subsequent skill write that touches a directory
-    # will bump the mtime past the cached value and trigger a re-scan.
-    _SKILLS_CACHE[cache_key] = (max_mtime, skills)
+    # Store in cache keyed by the scan signature computed BEFORE the scan
+    # (a write racing the scan changes the signature, so the next call
+    # re-scans rather than serving the torn result past the TTL).
+    _SKILLS_CACHE[cache_key] = (signature, now, skills)
     return skills