mirrors/hermes-agent
1
0
Fork 0
mirror of https://github.com/NousResearch/hermes-agent.git synced 2026-07-29 18:46:59 +00:00
Code Issues Projects Releases Packages Wiki Activity Actions 7

fix(skills): avoid redundant bind-mount scans (#72622)

Browse source 
Skip hashing active copies when the bundled origin is unchanged, build rename and optional migration indexes lazily, and reuse one optional-skill directory index per sync.

Add regression coverage for unchanged copies, deferred modification detection, lazy rename recovery, and optional provenance scans.
This commit is contained in:
Booker 2026-07-28 17:24:50 +08:00 • committed by GitHub
parent 30526baab5
commit 1dfe781edd
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
2 changed files with 183 additions and 26 deletions
Download patch file Download diff file Expand all files Collapse all files

127
tests/tools/test_skills_sync.py
View file

@ -700,6 +700,71 @@ class TestSyncSkills:
assert "old-skill" not in result.get("user_modified", [])
assert result["skipped"] >= 1
def test_unchanged_skill_does_not_hash_user_copy(self, tmp_path):
"""An unchanged bundled origin must not read the bind-mounted copy."""
bundled = self._setup_bundled(tmp_path)
skills_dir = tmp_path / "user_skills"
manifest_file = skills_dir / ".bundled_manifest"
user_skill = skills_dir / "old-skill"
user_skill.mkdir(parents=True)
(user_skill / "SKILL.md").write_text("# Old")
origin_hash = _dir_hash(bundled / "old-skill")
manifest_file.write_text(f"old-skill:{origin_hash}\n")
real_dir_hash = _dir_hash
def reject_user_hash(directory):
if directory == user_skill:
pytest.fail("unchanged sync read the user skill tree")
return real_dir_hash(directory)
with self._patches(bundled, skills_dir, manifest_file), \
patch("tools.skills_sync._index_active_skills", return_value={}), \
patch("tools.skills_sync._dir_hash", side_effect=reject_user_hash):
result = sync_skills(quiet=True)
assert result["skipped"] >= 1
def test_unchanged_skills_do_not_build_rename_index(self, tmp_path):
"""Rename recovery should not scan the active tree when every dest exists."""
bundled = self._setup_bundled(tmp_path)
skills_dir = tmp_path / "user_skills"
manifest_file = skills_dir / ".bundled_manifest"
user_skill = skills_dir / "old-skill"
user_skill.mkdir(parents=True)
(user_skill / "SKILL.md").write_text("# Old")
origin_hash = _dir_hash(bundled / "old-skill")
manifest_file.write_text(f"old-skill:{origin_hash}\n")
with self._patches(bundled, skills_dir, manifest_file), \
patch(
"tools.skills_sync._index_active_skills",
side_effect=AssertionError("rename index scanned eagerly"),
):
result = sync_skills(quiet=True)
assert result["skipped"] >= 1
def test_fast_path_defers_user_hash_until_bundled_update(self, tmp_path):
"""Local edits remain protected when a later bundled update arrives."""
bundled = self._setup_bundled(tmp_path)
skills_dir = tmp_path / "user_skills"
manifest_file = skills_dir / ".bundled_manifest"
user_skill = skills_dir / "old-skill"
user_skill.mkdir(parents=True)
(user_skill / "SKILL.md").write_text("# Old")
origin_hash = _dir_hash(bundled / "old-skill")
manifest_file.write_text(f"old-skill:{origin_hash}\n")
(user_skill / "SKILL.md").write_text("# My local edit")
with self._patches(bundled, skills_dir, manifest_file):
unchanged = sync_skills(quiet=True)
(bundled / "old-skill" / "SKILL.md").write_text("# Upstream v2")
changed = sync_skills(quiet=True)
assert "old-skill" not in unchanged["user_modified"]
assert "old-skill" in changed["user_modified"]
assert (user_skill / "SKILL.md").read_text() == "# My local edit"
def test_v1_manifest_migration_sets_baseline(self, tmp_path):
"""v1 manifest entries (no hash) should set baseline from user's current copy."""
bundled = self._setup_bundled(tmp_path)
@ -948,6 +1013,68 @@ class TestSyncSkills:
assert entry["source"] == "official"
assert entry["install_path"] == "mlops/chroma"
def test_optional_backfill_scans_active_tree_once(self, tmp_path):
"""Missing optional candidates must share one active-tree index."""
bundled = self._setup_bundled(tmp_path)
optional = tmp_path / "optional-skills"
for name in ("optional-a", "optional-b", "optional-c"):
skill = optional / "category" / name
skill.mkdir(parents=True)
(skill / "SKILL.md").write_text(f"---\nname: {name}\n---\n")
skills_dir = tmp_path / "user_skills"
manifest_file = skills_dir / ".bundled_manifest"
real_rglob = Path.rglob
active_tree_scans = 0
def count_active_tree_scans(path, pattern):
nonlocal active_tree_scans
if path == skills_dir:
active_tree_scans += 1
return real_rglob(path, pattern)
with self._patches(bundled, skills_dir, manifest_file), \
patch("tools.skills_sync._get_optional_dir", return_value=optional), \
patch.object(Path, "rglob", count_active_tree_scans):
sync_skills(quiet=True)
assert active_tree_scans <= 1
def test_optional_backfill_skips_already_tracked_skill_before_hashing(self, tmp_path):
"""Existing hub provenance must bypass bind-mounted content hashing."""
bundled = self._setup_bundled(tmp_path)
optional = tmp_path / "optional-skills"
optional_skill = optional / "category" / "tracked-skill"
optional_skill.mkdir(parents=True)
(optional_skill / "SKILL.md").write_text("# tracked\n")
skills_dir = tmp_path / "user_skills"
manifest_file = skills_dir / ".bundled_manifest"
active = skills_dir / "category" / "tracked-skill"
active.mkdir(parents=True)
(active / "SKILL.md").write_text("# tracked\n")
lock_path = skills_dir / ".hub" / "lock.json"
lock_path.parent.mkdir(parents=True)
lock_path.write_text(json.dumps({
"version": 1,
"installed": {
"tracked-skill": {"install_path": "category/tracked-skill"},
},
}))
real_dir_hash = _dir_hash
def reject_active_hash(directory):
if directory == active:
pytest.fail("tracked optional skill was hashed again")
return real_dir_hash(directory)
with self._patches(bundled, skills_dir, manifest_file), \
patch("tools.skills_sync._get_optional_dir", return_value=optional), \
patch("tools.skills_sync._dir_hash", side_effect=reject_active_hash):
result = sync_skills(quiet=True)
assert result["optional_provenance_backfilled"] == []
def test_relocated_backfill_still_requires_identical_content(self, tmp_path):
"""The name fallback must not weaken the content check.

82
tools/skills_sync.py
View file

@ -12,9 +12,10 @@ Old v1 manifests (plain names without hashes) are auto-migrated.
Update logic:
- NEW skills (not in manifest): copied to user dir, origin hash recorded.
- EXISTING skills (in manifest, present in user dir):
* If user copy matches origin hash: user hasn't modified it → safe to
update from bundled if bundled changed. New origin hash recorded.
* If user copy differs from origin hash: user customized it → SKIP.
* If bundled still matches origin hash: no update → skip without reading
the user copy.
* If bundled changed and user copy matches origin hash: safe to update.
* If bundled changed and user copy differs: user customized it → SKIP.
- DELETED by user (in manifest, absent from user dir): respected, not re-added.
- REMOVED from bundled (in manifest, gone from repo): cleaned from manifest.
@ -410,7 +411,28 @@ def restore_official_optional_skill(name: str, *, restore: bool = False) -> dict
}
def _find_installed_skill_dir_by_name(skill_dir_name: str) -> Optional[Path]:
def _index_installed_skill_dirs_by_name() -> Dict[str, List[Path]]:
"""Index installed skills by directory name with one active-tree scan."""
index: Dict[str, List[Path]] = {}
if not SKILLS_DIR.exists():
return index
for skill_md in SKILLS_DIR.rglob("SKILL.md"):
if is_excluded_skill_path(skill_md):
continue
candidate = skill_md.parent
# Never reach outside the skills tree (symlinked/external dirs).
try:
candidate.resolve().relative_to(SKILLS_DIR.resolve())
except (OSError, ValueError):
continue
index.setdefault(candidate.name, []).append(candidate)
return index
def _find_installed_skill_dir_by_name(
skill_dir_name: str,
installed_index: Optional[Dict[str, List[Path]]] = None,
) -> Optional[Path]:
"""Locate an installed skill directory by its directory name.
Used only as a fallback when the repo-derived install path doesn't exist in
@ -422,19 +444,9 @@ def _find_installed_skill_dir_by_name(skill_dir_name: str) -> Optional[Path]:
"""
if not skill_dir_name or not SKILLS_DIR.exists():
return None
matches: List[Path] = []
for skill_md in SKILLS_DIR.rglob("SKILL.md"):
if is_excluded_skill_path(skill_md):
continue
candidate = skill_md.parent
if candidate.name != skill_dir_name:
continue
# Never reach outside the skills tree (symlinked/external dirs).
try:
candidate.resolve().relative_to(SKILLS_DIR.resolve())
except (OSError, ValueError):
continue
matches.append(candidate)
if installed_index is None:
installed_index = _index_installed_skill_dirs_by_name()
matches = installed_index.get(skill_dir_name, [])
if len(matches) != 1:
return None
return matches[0]
@ -467,6 +479,7 @@ def _backfill_optional_provenance(quiet: bool = False) -> List[str]:
backfilled: List[str] = []
changed = False
installed_dir_index: Optional[Dict[str, List[Path]]] = None
for skill_md in sorted(optional_dir.rglob("SKILL.md")):
if is_excluded_skill_path(skill_md):
continue
@ -476,6 +489,9 @@ def _backfill_optional_provenance(quiet: bool = False) -> List[str]:
except ValueError as e:
logger.debug("Skipping optional skill with unsafe path %s: %s", src, e)
continue
lock_name = src.name
if lock_name in installed or install_path in existing_paths:
continue
dest = SKILLS_DIR / Path(*install_path.split("/"))
if not dest.exists() or not dest.is_dir():
# The active tree may hold the same skill under a DIFFERENT
@ -486,7 +502,9 @@ def _backfill_optional_provenance(quiet: bool = False) -> List[str]:
# silently skips them forever. Fall back to a unique
# same-directory-name match anywhere in the tree, then still
# require a byte-identical hash below before claiming provenance.
dest = _find_installed_skill_dir_by_name(src.name)
if installed_dir_index is None:
installed_dir_index = _index_installed_skill_dirs_by_name()
dest = _find_installed_skill_dir_by_name(src.name, installed_dir_index)
if dest is None:
continue
try:
@ -494,11 +512,9 @@ def _backfill_optional_provenance(quiet: bool = False) -> List[str]:
except ValueError as e:
logger.debug("Skipping relocated optional skill %s: %s", dest, e)
continue
if _dir_hash(dest) != _dir_hash(src):
if install_path in existing_paths:
continue
lock_name = src.name
if lock_name in installed or install_path in existing_paths:
if _dir_hash(dest) != _dir_hash(src):
continue
timestamp = datetime.now(timezone.utc).isoformat()
@ -694,9 +710,10 @@ def sync_skills(quiet: bool = False) -> dict:
# Index of skills already provided by external_dirs (skip writing them)
external_index = _build_external_skill_index()
shadowed_by_external: List[str] = []
# Rename recovery indexes, built once per sync (see _recover_renamed_skill).
active_index = _index_active_skills()
hub_paths = _read_hub_install_paths()
# Rename recovery indexes are expensive on host bind mounts. Build them
# only if a tracked skill is actually missing from its canonical path.
active_index: Optional[Dict[str, List[Path]]] = None
hub_paths: Optional[Set[str]] = None
copied = []
updated = []
@ -742,12 +759,15 @@ def sync_skills(quiet: bool = False) -> dict:
# "in manifest but not on disk" branch below misreads the skill as
# user-deleted, stranding the old copy at its stale path forever.
if not dest.exists() and skill_name in manifest:
if active_index is None:
active_index = _index_active_skills()
hub_paths = _read_hub_install_paths()
_moved_from = _recover_renamed_skill(
skill_name,
manifest.get(skill_name, ""),
dest,
active_index,
hub_paths,
hub_paths or set(),
quiet,
)
if _moved_from:
@ -816,6 +836,16 @@ def sync_skills(quiet: bool = False) -> dict:
elif dest.exists():
# ── Existing skill — in manifest AND on disk ──
origin_hash = manifest.get(skill_name, "")
# If the bundled source still matches the version recorded when
# it was installed, there is no update to apply. Avoid recursively
# hashing the user's copy just to rediscover that fact; when the
# bundled source changes, the normal user-modification check below
# still protects local edits before any overwrite.
if origin_hash and bundled_hash == origin_hash:
skipped += 1
continue
user_hash = _dir_hash(dest)
if not origin_hash: