From 76cd535359129ad750763593103ceaa211e06b05 Mon Sep 17 00:00:00 2001 From: srojk34 <286497132+srojk34@users.noreply.github.com> Date: Wed, 1 Jul 2026 19:56:05 +0300 Subject: [PATCH] security(gateway): re-resolve skills_sync path constants per call tools/skills_sync.py's HERMES_HOME/SKILLS_DIR/MANIFEST_FILE are resolved once at import time via get_hermes_home(), which is a context-local ContextVar under the multiplexed gateway (multiple profiles sharing one process). gateway/run.py calls sync_skills() directly in-process on every profile's startup, so freezing the path at import time pins every later profile's sync to whichever profile's HERMES_HOME was active when this module was first imported -- a later-starting profile silently syncs bundled skills into the FIRST profile's directory instead of its own, and never gets its own bundled skills synced at all. The module's own comment already flagged this ("Uses subprocess because sync_skills() caches HERMES_HOME at module level") for the CLI/hermes-update path, which routes around it via subprocess isolation -- but gateway/run.py's in-process call has no such isolation. _rmtree_writable's safety guard (SKILLS_DIR.resolve() as the scope floor for a destructive rmtree) also benefited from this fix: a frozen wrong- profile skills_root could desync the guard from the actual skills directory being operated on. Adopt tools/skills_hub.py's already-established fix for this exact bug class: a module __getattr__ (PEP 562) that resolves HERMES_HOME/ SKILLS_DIR/MANIFEST_FILE dynamically per access, honoring the active profile override, while a test's patch("tools.skills_sync.SKILLS_DIR", ...) still sets a real module attribute that shadows dynamic resolution entirely -- preserving every existing test seam in tests/tools/test_skills_sync.py unmodified. All ~20 internal call sites in the module now resolve fresh via the corresponding _hermes_home()/ _skills_dir()/_manifest_file() functions instead of the frozen names. --- tests/test_profile_isolation_runtime.py | 48 +++++++++++ tools/skills_sync.py | 101 ++++++++++++++++++------ 2 files changed, 123 insertions(+), 26 deletions(-) diff --git a/tests/test_profile_isolation_runtime.py b/tests/test_profile_isolation_runtime.py index ec80265e43d0..5e9131a9bb32 100644 --- a/tests/test_profile_isolation_runtime.py +++ b/tests/test_profile_isolation_runtime.py @@ -89,6 +89,54 @@ def test_lockfile_default_arg_resolves_active_profile(self, two_profiles): assert taps_b.path == prof_b / "skills" / ".hub" / "taps.json" +class TestSkillsSyncPathResolution: + """tools/skills_sync.py path constants must reflect the active profile — + otherwise gateway/run.py's in-process sync_skills() call on every + profile's startup syncs bundled skills into whichever profile imported + this module first, under the multiplexed gateway.""" + + def test_skills_dir_follows_override(self, two_profiles): + prof_a, prof_b = two_profiles + import tools.skills_sync as ss + + a_seen = _under_override(prof_a, lambda: ss._skills_dir()) + b_seen = _under_override(prof_b, lambda: ss._skills_dir()) + + assert a_seen == prof_a / "skills" + assert b_seen == prof_b / "skills" + assert a_seen != b_seen + + def test_manifest_file_follows_override(self, two_profiles): + prof_a, prof_b = two_profiles + import tools.skills_sync as ss + + b_seen = _under_override(prof_b, lambda: ss._manifest_file()) + assert b_seen == prof_b / "skills" / ".bundled_manifest" + + def test_legacy_attribute_access_follows_override(self, two_profiles): + """External `from tools.skills_sync import SKILLS_DIR`-style access + (PEP 562 module __getattr__) must also reflect the active profile.""" + prof_a, prof_b = two_profiles + import tools.skills_sync as ss + + a_seen = _under_override(prof_a, lambda: Path(ss.SKILLS_DIR)) + b_seen = _under_override(prof_b, lambda: Path(ss.HERMES_HOME)) + + assert a_seen == prof_a / "skills" + assert b_seen == prof_b + + def test_monkeypatched_constant_still_wins(self, two_profiles, monkeypatch, tmp_path): + """The existing test seam (patch the module constant, see + tests/tools/test_skills_sync.py) is preserved.""" + _prof_a, prof_b = two_profiles + import tools.skills_sync as ss + + forced = tmp_path / "forced_skills" + monkeypatch.setattr("tools.skills_sync.SKILLS_DIR", forced) + seen = _under_override(prof_b, lambda: ss._skills_dir()) + assert seen == forced + + class TestGatewayCacheDirResolution: """gateway/platforms/base.py cache getters must follow the active profile.""" diff --git a/tools/skills_sync.py b/tools/skills_sync.py index 2c0f41c47a62..7befc6a9c292 100644 --- a/tools/skills_sync.py +++ b/tools/skills_sync.py @@ -36,9 +36,55 @@ logger = logging.getLogger(__name__) -HERMES_HOME = get_hermes_home() -SKILLS_DIR = HERMES_HOME / "skills" -MANIFEST_FILE = SKILLS_DIR / ".bundled_manifest" +# Resolved per-call (not frozen at import) so the profile override is +# honored — an import-time constant pins every later sync_skills() call in +# the process to whichever profile's HERMES_HOME was active when this module +# was first imported, so a later-starting profile's gateway silently syncs +# bundled skills into the FIRST profile's directory instead of its own under +# the multiplexed gateway (single-process multi-profile runtimes, e.g. the +# desktop tui_gateway). Legacy names (HERMES_HOME, SKILLS_DIR, MANIFEST_FILE) +# are re-exposed via __getattr__ below (PEP 562) so external +# `from tools.skills_sync import SKILLS_DIR` callers still work, and a test's +# `patch("tools.skills_sync.SKILLS_DIR", ...)` sets a real module attribute +# that shadows dynamic resolution entirely (mirrors tools/skills_hub.py's +# already-fixed instance of this exact bug class). + + +# _override lets a test-injected real module attribute (patch/monkeypatch on +# SKILLS_DIR etc.) win over dynamic resolution; None means resolve live. +def _override(name: str): + return globals().get(name) + + +def _hermes_home() -> Path: + forced = _override("HERMES_HOME") + return Path(forced) if forced is not None else get_hermes_home() + + +def _skills_dir() -> Path: + forced = _override("SKILLS_DIR") + return Path(forced) if forced is not None else _hermes_home() / "skills" + + +def _manifest_file() -> Path: + forced = _override("MANIFEST_FILE") + return Path(forced) if forced is not None else _skills_dir() / ".bundled_manifest" + + +_DYNAMIC_PATH_RESOLVERS = { + "HERMES_HOME": _hermes_home, + "SKILLS_DIR": _skills_dir, + "MANIFEST_FILE": _manifest_file, +} + + +def __getattr__(name: str): + """Resolve legacy path constants dynamically (PEP 562) so they reflect the + active profile override; a test's patch-set real attribute shadows it.""" + resolver = _DYNAMIC_PATH_RESOLVERS.get(name) + if resolver is not None: + return resolver() + raise AttributeError(f"module {__name__!r} has no attribute {name!r}") # Marker file written by `hermes profile create --no-skills` (named profiles) # and by the installer's `--no-skills` flag (the default ~/.hermes profile). @@ -101,11 +147,12 @@ def _read_manifest() -> Dict[str, str]: Handles both v1 (plain names) and v2 (name:hash) formats. v1 entries get an empty hash string which triggers migration on next sync. """ - if not MANIFEST_FILE.exists(): + manifest_file = _manifest_file() + if not manifest_file.exists(): return {} try: result = {} - for line in MANIFEST_FILE.read_text(encoding="utf-8").splitlines(): + for line in manifest_file.read_text(encoding="utf-8").splitlines(): line = line.strip() if not line: continue @@ -133,7 +180,7 @@ def _read_suppressed_names() -> set: return read_suppressed_names() except Exception: - path = SKILLS_DIR / ".curator_suppressed" + path = _skills_dir() / ".curator_suppressed" if not path.exists(): return set() names = set() @@ -155,12 +202,13 @@ def _write_manifest(entries: Dict[str, str]): """ import tempfile - MANIFEST_FILE.parent.mkdir(parents=True, exist_ok=True) + manifest_file = _manifest_file() + manifest_file.parent.mkdir(parents=True, exist_ok=True) data = "\n".join(f"{name}:{hash_val}" for name, hash_val in sorted(entries.items())) + "\n" try: fd, tmp_path = tempfile.mkstemp( - dir=str(MANIFEST_FILE.parent), + dir=str(manifest_file.parent), prefix=".bundled_manifest_", suffix=".tmp", ) @@ -169,7 +217,7 @@ def _write_manifest(entries: Dict[str, str]): f.write(data) f.flush() os.fsync(f.fileno()) - atomic_replace(tmp_path, MANIFEST_FILE) + atomic_replace(tmp_path, manifest_file) except BaseException: try: os.unlink(tmp_path) @@ -177,7 +225,7 @@ def _write_manifest(entries: Dict[str, str]): pass raise except Exception as e: - logger.debug("Failed to write skills manifest %s: %s", MANIFEST_FILE, e, exc_info=True) + logger.debug("Failed to write skills manifest %s: %s", manifest_file, e, exc_info=True) def _read_skill_name(skill_md: Path, fallback: str) -> str: @@ -226,7 +274,7 @@ def _compute_relative_dest(skill_dir: Path, bundled_dir: Path) -> Path: e.g., bundled/skills/mlops/axolotl -> ~/.hermes/skills/mlops/axolotl """ rel = skill_dir.relative_to(bundled_dir) - return SKILLS_DIR / rel + return _skills_dir() / rel def _dir_hash(directory: Path) -> str: @@ -304,7 +352,7 @@ def _optional_skill_index() -> Dict[str, Tuple[str, str, Path]]: def _move_to_restore_backup(path: Path, backup_root: Path) -> str: """Move an existing skill directory into a restore backup, preserving rel path.""" - rel = path.relative_to(SKILLS_DIR) + rel = path.relative_to(_skills_dir()) target = backup_root / rel target.parent.mkdir(parents=True, exist_ok=True) if target.exists(): @@ -337,10 +385,11 @@ def restore_official_optional_skill(name: str, *, restore: bool = False) -> dict restored: List[str] = [] backed_up: List[str] = [] timestamp = datetime.now(timezone.utc).strftime("%Y%m%d-%H%M%S") - backup_root = SKILLS_DIR / ".restore-backups" / f"official-optional-{timestamp}" + skills_dir = _skills_dir() + backup_root = skills_dir / ".restore-backups" / f"official-optional-{timestamp}" for folder_name, install_path, src in targets: - dest = SKILLS_DIR / Path(*install_path.split("/")) + dest = skills_dir / Path(*install_path.split("/")) src_hash = _dir_hash(src) canonical_ok = dest.exists() and _dir_hash(dest) == src_hash @@ -348,13 +397,13 @@ def restore_official_optional_skill(name: str, *, restore: bool = False) -> dict # or folder slug, even if curator moved it into another category. src_frontmatter = _read_skill_name(src / "SKILL.md", folder_name) matches: List[Path] = [] - if SKILLS_DIR.exists(): - for skill_md in sorted(SKILLS_DIR.rglob("SKILL.md")): + if skills_dir.exists(): + for skill_md in sorted(skills_dir.rglob("SKILL.md")): if is_excluded_skill_path(skill_md): continue candidate = skill_md.parent try: - candidate.relative_to(SKILLS_DIR) + candidate.relative_to(skills_dir) except ValueError: continue candidate_name = _read_skill_name(skill_md, candidate.name) @@ -400,7 +449,7 @@ def _backfill_optional_provenance(quiet: bool = False) -> List[str]: if not optional_dir.exists(): return [] - lock_path = SKILLS_DIR / ".hub" / "lock.json" + lock_path = _skills_dir() / ".hub" / "lock.json" try: data = json.loads(lock_path.read_text()) if lock_path.exists() else {"version": 1, "installed": {}} except (json.JSONDecodeError, OSError): @@ -423,7 +472,7 @@ 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 - dest = SKILLS_DIR / Path(*install_path.split("/")) + dest = _skills_dir() / Path(*install_path.split("/")) if not dest.exists() or not dest.is_dir(): continue if _dir_hash(dest) != _dir_hash(src): @@ -493,7 +542,7 @@ def sync_skills(quiet: bool = False) -> dict: # empty-result shape with skipped_opt_out lets callers report "opted out" # instead of "synced 0 / failed". This is the default-profile counterpart # to seed_profile_skills()'s marker check for named profiles. - if (HERMES_HOME / NO_BUNDLED_SKILLS_MARKER).exists(): + if (_hermes_home() / NO_BUNDLED_SKILLS_MARKER).exists(): if not quiet: print(" (skipped — profile opted out of bundled skills via .no-bundled-skills)") return { @@ -510,7 +559,7 @@ def sync_skills(quiet: bool = False) -> dict: "optional_provenance_backfilled": [], } - SKILLS_DIR.mkdir(parents=True, exist_ok=True) + _skills_dir().mkdir(parents=True, exist_ok=True) manifest = _read_manifest() bundled_skills = _discover_bundled_skills(bundled_dir) bundled_names = {name for name, _ in bundled_skills} @@ -696,7 +745,7 @@ def sync_skills(quiet: bool = False) -> dict: # Also copy DESCRIPTION.md files for categories (if not already present) for desc_md in bundled_dir.rglob("DESCRIPTION.md"): rel = desc_md.relative_to(bundled_dir) - dest_desc = SKILLS_DIR / rel + dest_desc = _skills_dir() / rel if not dest_desc.exists(): try: dest_desc.parent.mkdir(parents=True, exist_ok=True) @@ -741,7 +790,7 @@ def _rmtree_writable(path: Path) -> None: # ``shutil.rmtree(~/.hermes)`` into a loud, recoverable ``ValueError`` # instead of silently destroying the user's install. target = Path(path).resolve() - skills_root = SKILLS_DIR.resolve() + skills_root = _skills_dir().resolve() # Every legitimate caller passes a skill directory or its ``.bak`` # sibling — always a strict child of the skills root. The skills root # itself must never be removed: a ``dest`` that collapses to @@ -1049,11 +1098,11 @@ def set_bundled_skills_opt_out(enabled: bool) -> dict: dict with keys: ok (bool), changed (bool), marker (str path), message (str). """ - marker = HERMES_HOME / NO_BUNDLED_SKILLS_MARKER + marker = _hermes_home() / NO_BUNDLED_SKILLS_MARKER existed = marker.exists() try: if enabled: - HERMES_HOME.mkdir(parents=True, exist_ok=True) + _hermes_home().mkdir(parents=True, exist_ok=True) marker.write_text( "This profile opted out of bundled-skill seeding " "(`hermes skills opt-out`).\n" @@ -1087,7 +1136,7 @@ def set_bundled_skills_opt_out(enabled: bool) -> dict: def is_bundled_skills_opt_out() -> bool: """Return True if the active profile carries the opt-out marker.""" - return (HERMES_HOME / NO_BUNDLED_SKILLS_MARKER).exists() + return (_hermes_home() / NO_BUNDLED_SKILLS_MARKER).exists() def remove_pristine_bundled_skills(dry_run: bool = False) -> dict: