From edac49e473705bf72ea25304165d67dac87771ab Mon Sep 17 00:00:00 2001 From: Carry00 Date: Thu, 27 Aug 2026 23:33:50 +0800 Subject: [PATCH] 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. --- agent/skill_utils.py | 1 + tests/tools/test_credential_files.py | 40 ++++++++++++++++++++++++++++ tools/credential_files.py | 31 ++++++++++++++++++--- 3 files changed, 69 insertions(+), 3 deletions(-) diff --git a/agent/skill_utils.py b/agent/skill_utils.py index a3ab4133ec..e23647cce8 100644 --- a/agent/skill_utils.py +++ b/agent/skill_utils.py @@ -31,6 +31,7 @@ EXCLUDED_SKILL_DIRS = frozenset( ".github", ".hub", ".archive", + ".curator_backups", ".venv", "venv", "node_modules", diff --git a/tests/tools/test_credential_files.py b/tests/tools/test_credential_files.py index cb48d2a721..138246d654 100644 --- a/tests/tools/test_credential_files.py +++ b/tests/tools/test_credential_files.py @@ -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() diff --git a/tools/credential_files.py b/tools/credential_files.py index e7cc8f028e..31e480eaaa 100644 --- a/tools/credential_files.py +++ b/tools/credential_files.py @@ -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}",