diff --git a/tests/test_profile_isolation_runtime.py b/tests/test_profile_isolation_runtime.py new file mode 100644 index 000000000..ec80265e4 --- /dev/null +++ b/tests/test_profile_isolation_runtime.py @@ -0,0 +1,207 @@ +"""Profile-isolation regression tests for single-process multi-profile runtimes. + +In runtimes that serve every profile from one OS process (the desktop +``tui_gateway``), the profile boundary is the context-local +``_HERMES_HOME_OVERRIDE`` ContextVar, not the process environment. State that +escapes the request call stack — import-time-frozen path constants, direct +``os.environ`` reads, or worker threads that don't inherit the request context — +silently reverts to the launch/default profile and leaks one profile's data +into another. + +These tests drive each previously-leaking site under override A then override B +with real temp HERMES_HOME directories (no mocks) and assert the *active* +profile's path is used. They are the productionized form of the manual smoke +probes used to confirm the bug class. +""" + +import threading +from pathlib import Path + +import pytest + +from hermes_constants import ( + get_hermes_home, + reset_hermes_home_override, + set_hermes_home_override, +) + + +@pytest.fixture +def two_profiles(tmp_path): + """Two distinct profile HERMES_HOME dirs with the dir skeleton created.""" + prof_a = tmp_path / "profA" + prof_b = tmp_path / "profB" + for p in (prof_a, prof_b): + (p / "skills").mkdir(parents=True, exist_ok=True) + (p / "state").mkdir(parents=True, exist_ok=True) + (p / "cache").mkdir(parents=True, exist_ok=True) + return prof_a, prof_b + + +def _under_override(home: Path, fn): + """Run ``fn`` with the profile override set to ``home`` and reset after.""" + token = set_hermes_home_override(str(home)) + try: + return fn() + finally: + reset_hermes_home_override(token) + + +# --------------------------------------------------------------------------- +# M1 — import-time path globals / direct os.environ reads +# --------------------------------------------------------------------------- + +class TestSkillsHubPathResolution: + """tools/skills_hub.py path constants must reflect the active profile.""" + + def test_skills_dir_follows_override(self, two_profiles): + prof_a, prof_b = two_profiles + import tools.skills_hub as sh + + # Importing/touching under A must NOT pin the path for B. + a_seen = _under_override(prof_a, lambda: Path(sh.SKILLS_DIR)) + b_seen = _under_override(prof_b, lambda: Path(sh.SKILLS_DIR)) + + assert a_seen == prof_a / "skills" + assert b_seen == prof_b / "skills" + assert a_seen != b_seen + + def test_hub_derived_paths_follow_override(self, two_profiles): + prof_a, prof_b = two_profiles + import tools.skills_hub as sh + + b_lock = _under_override(prof_b, lambda: Path(sh.LOCK_FILE)) + b_audit = _under_override(prof_b, lambda: Path(sh.AUDIT_LOG)) + b_index = _under_override(prof_b, lambda: Path(sh.INDEX_CACHE_DIR)) + + assert b_lock == prof_b / "skills" / ".hub" / "lock.json" + assert b_audit == prof_b / "skills" / ".hub" / "audit.log" + assert b_index == prof_b / "skills" / ".hub" / "index-cache" + + def test_lockfile_default_arg_resolves_active_profile(self, two_profiles): + prof_a, prof_b = two_profiles + from tools.skills_hub import HubLockFile, TapsManager + + lock_b = _under_override(prof_b, lambda: HubLockFile()) + taps_b = _under_override(prof_b, lambda: TapsManager()) + + assert lock_b.path == prof_b / "skills" / ".hub" / "lock.json" + assert taps_b.path == prof_b / "skills" / ".hub" / "taps.json" + + +class TestGatewayCacheDirResolution: + """gateway/platforms/base.py cache getters must follow the active profile.""" + + def test_image_cache_dir_follows_override(self, two_profiles): + prof_a, prof_b = two_profiles + import gateway.platforms.base as gb + + a_seen = _under_override(prof_a, lambda: gb.get_image_cache_dir()) + b_seen = _under_override(prof_b, lambda: gb.get_image_cache_dir()) + + assert str(a_seen).startswith(str(prof_a)) + assert str(b_seen).startswith(str(prof_b)) + assert a_seen != b_seen + + def test_all_cache_getters_follow_override(self, two_profiles): + _prof_a, prof_b = two_profiles + import gateway.platforms.base as gb + + getters = ( + gb.get_image_cache_dir, + gb.get_audio_cache_dir, + gb.get_video_cache_dir, + gb.get_document_cache_dir, + ) + for getter in getters: + seen = _under_override(prof_b, getter) + assert str(seen).startswith(str(prof_b)), f"{getter.__name__} leaked: {seen}" + + def test_monkeypatched_constant_still_wins(self, two_profiles, monkeypatch, tmp_path): + """The existing test seam (monkeypatch the module constant) is preserved.""" + _prof_a, _prof_b = two_profiles + import gateway.platforms.base as gb + + forced = tmp_path / "forced_img" + monkeypatch.setattr("gateway.platforms.base.IMAGE_CACHE_DIR", forced) + # Even with an active override, an explicit monkeypatch takes precedence. + seen = _under_override(_prof_b, lambda: gb.get_image_cache_dir()) + assert seen == forced + + +class TestRichSentStorePathResolution: + """gateway/rich_sent_store.py must honor the override, not read os.environ.""" + + def test_store_path_follows_override(self, two_profiles, monkeypatch): + prof_a, prof_b = two_profiles + # Ensure no ambient HERMES_HOME env masks the test. + monkeypatch.delenv("HERMES_HOME", raising=False) + import gateway.rich_sent_store as rss + + b_seen = _under_override(prof_b, lambda: rss._store_path()) + assert b_seen.startswith(str(prof_b)) + assert b_seen.endswith("state/rich_sent_index.json") + + +# --------------------------------------------------------------------------- +# M2 — thread / executor context propagation +# --------------------------------------------------------------------------- + +class TestThreadContextPropagation: + """Worker threads must inherit the spawning turn's profile override.""" + + def test_raw_thread_loses_override(self, two_profiles): + """Document the underlying hazard: a bare thread does NOT inherit it.""" + _prof_a, prof_b = two_profiles + seen = {} + + def worker(): + seen["home"] = str(get_hermes_home()) + + def run(): + t = threading.Thread(target=worker) + t.start() + t.join() + + _under_override(prof_b, run) + # A bare thread falls back to the process default — this is WHY the fix + # primitive is needed. (Asserted as the hazard, not the desired state.) + assert seen["home"] != str(prof_b) + + def test_propagate_primitive_preserves_override(self, two_profiles): + _prof_a, prof_b = two_profiles + from tools.thread_context import propagate_context_to_thread + + seen = {} + + def worker(): + seen["home"] = str(get_hermes_home()) + + def run(): + t = threading.Thread(target=propagate_context_to_thread(worker)) + t.start() + t.join() + + _under_override(prof_b, run) + assert seen["home"] == str(prof_b) + + def test_run_async_worker_preserves_override(self, two_profiles): + """model_tools._run_async's worker-thread branch must keep the override. + + This is the generic sync->async bridge for every async tool; if it + leaks, every async tool that resolves get_hermes_home() leaks. + """ + import asyncio + + _prof_a, prof_b = two_profiles + import model_tools + + async def reads_home(): + return str(get_hermes_home()) + + async def driver(): + # Inside a running loop, _run_async spawns a worker thread + loop. + return model_tools._run_async(reads_home()) + + seen = _under_override(prof_b, lambda: asyncio.run(driver())) + assert seen == str(prof_b) diff --git a/tools/skills_hub.py b/tools/skills_hub.py index 3863cbc97..b7d0237d7 100644 --- a/tools/skills_hub.py +++ b/tools/skills_hub.py @@ -62,36 +62,55 @@ logger = logging.getLogger(__name__) INDEX_CACHE_TTL = 3600 # 1 hour (defined early; referenced below) +# The legacy path names (SKILLS_DIR, HUB_DIR, ...) are not real module +# attributes — they are synthesized on access by the PEP 562 ``__getattr__`` +# below so they reflect the active profile override. Tests, however, set them +# as *real* module attributes via ``patch.object(hub, "SKILLS_DIR", ...)`` / +# ``monkeypatch.setattr``. ``_override`` lets each resolver honor such an +# injected real attribute (the test seam) before falling back to dynamic +# resolution. ``globals().get`` returns None when only the __getattr__-backed +# name exists (no real attribute set), so dynamic resolution wins by default. +def _override(name: str): + return globals().get(name) + + def _hermes_home() -> Path: return get_hermes_home() def _skills_dir() -> Path: - return _hermes_home() / "skills" + forced = _override("SKILLS_DIR") + return Path(forced) if forced is not None else _hermes_home() / "skills" def _hub_dir() -> Path: - return _skills_dir() / ".hub" + forced = _override("HUB_DIR") + return Path(forced) if forced is not None else _skills_dir() / ".hub" def _lock_file() -> Path: - return _hub_dir() / "lock.json" + forced = _override("LOCK_FILE") + return Path(forced) if forced is not None else _hub_dir() / "lock.json" def _quarantine_dir() -> Path: - return _hub_dir() / "quarantine" + forced = _override("QUARANTINE_DIR") + return Path(forced) if forced is not None else _hub_dir() / "quarantine" def _audit_log() -> Path: - return _hub_dir() / "audit.log" + forced = _override("AUDIT_LOG") + return Path(forced) if forced is not None else _hub_dir() / "audit.log" def _taps_file() -> Path: - return _hub_dir() / "taps.json" + forced = _override("TAPS_FILE") + return Path(forced) if forced is not None else _hub_dir() / "taps.json" def _index_cache_dir() -> Path: - return _hub_dir() / "index-cache" + forced = _override("INDEX_CACHE_DIR") + return Path(forced) if forced is not None else _hub_dir() / "index-cache" _DYNAMIC_PATH_RESOLVERS = { @@ -110,7 +129,8 @@ def __getattr__(name: str): """Resolve the legacy path constants dynamically per access. PEP 562 module ``__getattr__``: only called for names NOT found as real - module attributes, so it does not slow down ordinary lookups. This lets + module attributes, so it does not slow down ordinary lookups and a test's + ``patch.object``-set real attribute shadows it. This lets ``tools.skills_hub.SKILLS_DIR`` (and the rest) reflect the active profile override instead of an import-time snapshot. """