fix(skills): stop syncing bookkeeping dirs to sandboxes
iter_skills_files() walked the skills tree with a bare rglob("*"), so the
.hub download cache, .archive, curator backups, and any node_modules/.git
under a skill package were uploaded to the sandbox on every sync. The
sandbox never reads them: skill content is resolved host-side.
EXCLUDED_SKILL_DIRS is already the canonical exclusion set, honoured by
discovery and backup. Apply it to the sync path too, across all three
roots iter_skills_files() walks (local, external, project-local), and add
.curator_backups to the set.
Measured on a local install: 900 files / 67.3 MB -> 771 files / 8.4 MB.
This is not just wasted bandwidth on the SSH backend, where the oversized
payload can exceed the 120s _ssh_bulk_upload deadline and surface as the
agent hanging on every tool call.
The filter intentionally does not reuse is_excluded_skill_path(), which
also prunes references/, templates/, assets/ and scripts/ -- those hold
support files and bundled scripts the sandbox does read and execute.
This commit is contained in:
@@ -31,6 +31,7 @@ EXCLUDED_SKILL_DIRS = frozenset(
|
||||
".github",
|
||||
".hub",
|
||||
".archive",
|
||||
".curator_backups",
|
||||
".venv",
|
||||
"venv",
|
||||
"node_modules",
|
||||
|
||||
@@ -147,6 +147,46 @@ class TestIterSkillsFiles:
|
||||
# Symlink should be excluded
|
||||
assert not any("evil" in f["container_path"] for f in files)
|
||||
|
||||
def test_skips_excluded_bookkeeping_dirs(self, tmp_path):
|
||||
"""Bookkeeping and dependency dirs must not be uploaded to a sandbox.
|
||||
|
||||
The sync path used a bare rglob("*"), so the .hub download cache,
|
||||
.archive, curator backups and any node_modules/.git under a skills
|
||||
tree were packed up on every sync even though the sandbox never
|
||||
reads them. Sync now honours EXCLUDED_SKILL_DIRS like discovery.
|
||||
"""
|
||||
hermes_home = tmp_path / ".hermes"
|
||||
skills_dir = hermes_home / "skills"
|
||||
(skills_dir / "cat" / "myskill").mkdir(parents=True)
|
||||
(skills_dir / "cat" / "myskill" / "SKILL.md").write_text("# skill")
|
||||
# Progressive-disclosure support files must still be synced.
|
||||
(skills_dir / "cat" / "myskill" / "references").mkdir()
|
||||
(skills_dir / "cat" / "myskill" / "references" / "api.md").write_text("ref")
|
||||
|
||||
for excluded in (".hub", ".archive", ".curator_backups", "node_modules"):
|
||||
junk = skills_dir / excluded / "vendored"
|
||||
junk.mkdir(parents=True)
|
||||
(junk / "SKILL.md").write_text("# stale copy")
|
||||
# Also nested inside an otherwise-valid skill package.
|
||||
cache = skills_dir / "cat" / "myskill" / "__pycache__"
|
||||
cache.mkdir()
|
||||
(cache / "helper.cpython-311.pyc").write_text("bytecode")
|
||||
|
||||
with patch.dict(os.environ, {"HERMES_HOME": str(hermes_home)}):
|
||||
files = iter_skills_files()
|
||||
|
||||
paths = {f["container_path"] for f in files}
|
||||
assert "/root/.hermes/skills/cat/myskill/SKILL.md" in paths
|
||||
assert "/root/.hermes/skills/cat/myskill/references/api.md" in paths
|
||||
for excluded in (
|
||||
".hub",
|
||||
".archive",
|
||||
".curator_backups",
|
||||
"node_modules",
|
||||
"__pycache__",
|
||||
):
|
||||
assert not any(excluded in path for path in paths), excluded
|
||||
|
||||
def test_empty_when_no_skills_dir(self, tmp_path):
|
||||
hermes_home = tmp_path / ".hermes"
|
||||
hermes_home.mkdir()
|
||||
|
||||
@@ -28,6 +28,8 @@ from pathlib import Path
|
||||
from typing import Dict, List, Optional
|
||||
from hermes_cli.config import cfg_get
|
||||
|
||||
from agent.skill_utils import EXCLUDED_SKILL_DIRS
|
||||
|
||||
try: # pragma: no cover - exercised via the fail-closed test below
|
||||
from agent.file_safety import get_read_block_error
|
||||
except ImportError: # noqa: F401 - sentinel consumed in register_credential_file
|
||||
@@ -344,15 +346,32 @@ def _safe_skills_path(skills_dir: Path) -> str:
|
||||
return str(safe_dir)
|
||||
|
||||
|
||||
def _is_excluded_skills_dir(rel: Path) -> bool:
|
||||
"""True if *rel* sits under a directory excluded from skill scanning.
|
||||
|
||||
Reuses ``agent.skill_utils.EXCLUDED_SKILL_DIRS`` so the sync path agrees
|
||||
with discovery. These are local bookkeeping and dependency directories
|
||||
(``.hub`` download cache, ``.archive``, ``.curator_backups``,
|
||||
``node_modules``, ``__pycache__``, ``.git``, ...) that the remote agent
|
||||
never reads.
|
||||
|
||||
This deliberately does not use ``is_excluded_skill_path()``, which also
|
||||
prunes ``references/``, ``templates/``, ``assets/`` and ``scripts/``.
|
||||
Those hold progressive-disclosure support files and bundled scripts the
|
||||
sandbox does execute, so they must keep syncing.
|
||||
"""
|
||||
return any(part in EXCLUDED_SKILL_DIRS for part in rel.parts[:-1])
|
||||
|
||||
|
||||
def iter_skills_files(
|
||||
container_base: str = "/root/.hermes",
|
||||
) -> List[Dict[str, str]]:
|
||||
"""Yield individual (host_path, container_path) entries for skills files.
|
||||
|
||||
Includes both the local skills dir and any external dirs configured via
|
||||
skills.external_dirs. Skips symlinks entirely. Preferred for backends
|
||||
that upload files individually (Daytona, Modal) rather than mounting a
|
||||
directory.
|
||||
skills.external_dirs. Skips symlinks and anything under
|
||||
EXCLUDED_SKILL_DIRS entirely. Preferred for backends that upload files
|
||||
individually (Daytona, Modal) rather than mounting a directory.
|
||||
"""
|
||||
result: List[Dict[str, str]] = []
|
||||
|
||||
@@ -364,6 +383,8 @@ def iter_skills_files(
|
||||
if item.is_symlink() or not item.is_file():
|
||||
continue
|
||||
rel = item.relative_to(skills_dir)
|
||||
if _is_excluded_skills_dir(rel):
|
||||
continue
|
||||
result.append({
|
||||
"host_path": str(item),
|
||||
"container_path": f"{container_root}/{rel}",
|
||||
@@ -380,6 +401,8 @@ def iter_skills_files(
|
||||
if item.is_symlink() or not item.is_file():
|
||||
continue
|
||||
rel = item.relative_to(ext_dir)
|
||||
if _is_excluded_skills_dir(rel):
|
||||
continue
|
||||
result.append({
|
||||
"host_path": str(item),
|
||||
"container_path": f"{container_root}/{rel}",
|
||||
@@ -392,6 +415,8 @@ def iter_skills_files(
|
||||
if item.is_symlink() or not item.is_file():
|
||||
continue
|
||||
rel = item.relative_to(proj_dir)
|
||||
if _is_excluded_skills_dir(rel):
|
||||
continue
|
||||
result.append({
|
||||
"host_path": str(item),
|
||||
"container_path": f"{container_root}/{rel}",
|
||||
|
||||
Reference in New Issue
Block a user