diff --git a/tests/tools/test_skills_tool_discovery_cache.py b/tests/tools/test_skills_tool_discovery_cache.py new file mode 100644 index 00000000000..b262077d479 --- /dev/null +++ b/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"] diff --git a/tools/skills_tool.py b/tools/skills_tool.py index a9087232c4d..ae063e38e0e 100644 --- a/tools/skills_tool.py +++ b/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