Skip to content

security(gateway): re-resolve skills_sync path constants per call - #441

Open
hashbender wants to merge 1 commit into
mainfrom
mirror/pr-56523
Open

security(gateway): re-resolve skills_sync path constants per call#441
hashbender wants to merge 1 commit into
mainfrom
mirror/pr-56523

Conversation

@hashbender

Copy link
Copy Markdown
Owner

Summary

tools/skills_sync.py's HERMES_HOME/SKILLS_DIR/MANIFEST_FILE are resolved once at import time via get_hermes_home(), which reads a context-local ContextVar (_HERMES_HOME_OVERRIDE) set per-request under the multiplexed gateway (multiple profiles served from one process, e.g. the desktop tui_gateway). gateway/run.py calls sync_skills() directly in-process on every profile's startup:

# gateway/run.py
from tools.skills_sync import sync_skills
sync_skills(quiet=True)

Freezing the resolved path at import time pins every later profile's sync to whichever profile's HERMES_HOME happened to be active the first time this module was imported in the process — 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 (visible elsewhere in the codebase, hermes_cli/profiles.py) already flags this for the CLI/hermes update path — "Uses subprocess because sync_skills() caches HERMES_HOME at module level" — 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, guarding against a catastrophic wipe of ~/.hermes/) also benefits from this fix: a frozen wrong-profile skills_root could desync the guard from the actual skills directory being operated on.

Fix

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 (dozens of call sites) unmodified. All ~20 internal call sites in the module now resolve fresh via _hermes_home()/_skills_dir()/_manifest_file() instead of the frozen names.

Test plan

  • pytest tests/tools/test_skills_sync.py tests/tools/test_skills_list_modified_diff.py -q — 75 passed
  • pytest tests/test_profile_isolation_runtime.py -q — 14 passed (10 pre-existing + 4 new)
  • pytest tests/hermes_cli/test_skills_hub.py -q — 28 passed
  • Running these three files together in one session surfaces 3 pre-existing, unrelated TestSkillsHubPathResolution failures (a test-order-dependent state leak in tools/skills_hub.py's own test suite) — confirmed via git stash that this happens identically without this change; each file passes cleanly run on its own.
  • ruff check on all changed files — clean

Note on file co-tenancy

Open PR NousResearch#34577 ("keep bundled skill copies writable on Nix store") also touches tools/skills_sync.py, but for an unrelated concern (shutil.copytree write-permission handling on read-only source filesystems, via new _copy_file_writable/_make_tree_owner_writable helpers) — no functional overlap with this profile-isolation fix, though whichever merges second will need a small rebase around the constant-block region.


Mirror-of: NousResearch#56523
NousResearch#56523

@tenki-reviewer

tenki-reviewer Bot commented Jul 1, 2026

Copy link
Copy Markdown

Review Complete
No issues found!

Risk: 🟢 Low (18/100) — no findings · 149 LOC across 2 files


PR replaces import-time path constants in tools/skills_sync.py with PEP 562 getattr for profile-aware dynamic resolution, and adds a runtime profile isolation test. No blocking issues found.

Files Reviewed (2 files)
tests/test_profile_isolation_runtime.py
tools/skills_sync.py

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant