diff --git a/docs/ADR.md b/docs/ADR.md new file mode 100644 index 0000000000..cc9ecfb927 --- /dev/null +++ b/docs/ADR.md @@ -0,0 +1,77 @@ +# Architecture Decision Records + +## 2026-07-13: Scope plugin manager state by Hermes home/profile (keyed cache) + +Status: Accepted + +Context: +Hermes supports multiple profiles via different Hermes home directories. +Homes are switched two ways in a running process: the `HERMES_HOME` +environment variable (single-profile CLI/gateway processes), and the +context-local `set_hermes_home_override()` (`hermes_constants.py`), which +the multiplexed gateway worker (`gateway/run.py`'s `_profile_scope`) and +subagent/embedded callers use to serve several profiles from one +long-lived process. The override is a `ContextVar` and deliberately does +**not** mutate `os.environ`, since that would leak one profile's home +into every other concurrent task in the same process. + +The plugin manager was a process-global single-slot singleton +(`_plugin_manager`). User-installed plugins are discovered from +`get_hermes_home() / "plugins"`, and context-engine plugins (e.g. +`hermes-lcm`) capture profile-scoped state — such as the LCM database +path — at registration time. A single-slot cache meant: + +1. Switching homes via `set_hermes_home_override()` was invisible to a + naive "did `HERMES_HOME` change" check, so the singleton silently kept + serving the first profile's manager to every other profile in the + process. +2. Even when a fresh `PluginManager` *was* created for a new home, plugin + modules are imported into `sys.modules` as `hermes_plugins.` by + `_load_directory_module`, and only that top-level module was ever + replaced. A same-slug plugin's *relative* imports + (`from . import state`) are cached separately under + `hermes_plugins..`, and Python's import machinery + resolves those from `sys.modules` first — so a profile switch could + silently keep serving a previous profile's already-imported submodule + code/state instead of re-executing the new profile's plugin. + +Decision: +- Replace the single-slot singleton with a cache keyed on the *resolved* + Hermes home path (`_plugin_managers_by_home: Dict[Path, PluginManager]`). + `get_plugin_manager()` resolves the current home via `get_hermes_home()` + (which itself already consults `get_hermes_home_override()` before + `os.environ`), so both the env-var and context-local override paths are + covered uniformly. +- `_plugin_manager` (the old single-slot name) is kept as a thin "last + manager returned" pointer purely for backward compatibility with + existing test code that does + `monkeypatch.setattr(plugins_mod, "_plugin_manager", some_manager)`. + When that name is monkeypatched to a manager the keyed cache doesn't + know about, `get_plugin_manager()` treats it as an explicit injection + and adopts it into the cache under the *current* resolved home, rather + than discarding it. +- Both `PluginManager._load_directory_module` (initial/`force=True` + reload within the same home) and the shared `_clear_plugin_submodules` + helper (profile switch / test teardown) evict `sys.modules[module_name]` + **and every name prefixed with `module_name + "."`** before a plugin + slug is (re-)imported, so relative-import submodules can never survive + a reload or a home switch. +- Test isolation (`tests/conftest.py`'s `_hermetic_environment` fixture) + calls a new `_reset_plugin_managers_for_tests()` helper that drops the + entire keyed cache and purges every plugin submodule from `sys.modules` + between tests, instead of only resetting the single-slot pointer. + +Consequences: +- Per-profile LCM instances (and any other context-engine plugin) use + their own `{home}/lcm.db` regardless of whether the profile switch went + through `HERMES_HOME` or `set_hermes_home_override()`. +- Plugin discovery remains cached within a profile for normal + performance, and re-entering a previously-seen profile reuses its + cached manager instead of rebuilding from scratch. +- Sequential *and* interleaved profile switching — in tests, the gateway + multiplexer worker, or embedded callers using the context-local + override — no longer leaks context-engine state, plugin module state, + or stale relative-import submodules across profiles. +- Regression coverage exercises the real production path + (`set_hermes_home_override()`) rather than only the env-var path, and + includes a dedicated relative-import leak test. diff --git a/hermes_cli/plugins.py b/hermes_cli/plugins.py index d1803aac09..8dec25c520 100644 --- a/hermes_cli/plugins.py +++ b/hermes_cli/plugins.py @@ -3845,6 +3845,23 @@ class PluginManager: key = manifest.key or manifest.name slug = key.replace("/", "__").replace("-", "_") module_name = f"{_NS_PARENT}.{slug}" + + # Evict any stale sys.modules entries for this slug before + # (re-)importing. A same-slug module may already be cached here + # from a different Hermes home (profile switch reusing a slug + # like "hermes-lcm") or from an earlier force=True reload in the + # same home. Replacing only sys.modules[module_name] below is not + # enough: the plugin's own relative imports (`from . import foo`) + # are cached separately under "module_name + '.' + submodule", + # and Python's import system resolves those from sys.modules + # first — so a stale submodule would silently keep serving the + # previous load's code/state instead of the fresh one we're + # about to exec. Evict the package and everything nested under + # it so this import starts clean. + stale_prefix = f"{module_name}." + for name in [n for n in sys.modules if n == module_name or n.startswith(stale_prefix)]: + del sys.modules[name] + spec = importlib.util.spec_from_file_location( module_name, init_file, @@ -3857,7 +3874,16 @@ class PluginManager: module.__package__ = module_name module.__path__ = [str(plugin_dir)] # type: ignore[attr-defined] sys.modules[module_name] = module - spec.loader.exec_module(module) + try: + spec.loader.exec_module(module) + except BaseException: + # Don't leave a half-initialized module (or the partially + # imported relative submodules it pulled in before failing) + # cached in sys.modules — a retry or a same-slug plugin in a + # different profile would otherwise inherit broken state. + for name in [n for n in sys.modules if n == module_name or n.startswith(stale_prefix)]: + del sys.modules[name] + raise return module def _load_entrypoint_module(self, manifest: PluginManifest) -> types.ModuleType: @@ -4370,15 +4396,121 @@ class PluginManager: # Module-level singleton & convenience functions # --------------------------------------------------------------------------- +# Legacy single-slot singleton. Kept as the storage for the "current" +# manager so existing test code that does +# ``monkeypatch.setattr(plugins_mod, "_plugin_manager", some_manager)`` +# keeps working — ``get_plugin_manager()`` still reads/writes this name. _plugin_manager: Optional[PluginManager] = None +# Keyed cache: resolved Hermes home -> PluginManager. Hermes supports +# multiple profiles via different HERMES_HOME directories, and a single +# long-lived process (gateway multiplexer, test session, embedder) can +# switch between them via ``set_hermes_home_override()`` — which is a +# ContextVar and deliberately does NOT touch os.environ (see +# hermes_constants.set_hermes_home_override). A process-wide single-slot +# cache leaks one profile's plugin/context-engine state into another. We +# key the cache by the *resolved* home path so re-entering a previously +# seen profile reuses its manager (and picks up any modules it already +# imported) instead of rebuilding from scratch every switch. +_plugin_managers_by_home: Dict[Path, PluginManager] = {} + + +def _plugin_home_key() -> Path: + """Return the profile/home key for process-global plugin state. + + Plugins are discovered from ``get_hermes_home() / "plugins"`` and some + plugins (notably context engines such as hermes-lcm) capture that home + at registration time for profile-scoped storage. A long-lived process + can temporarily switch Hermes home (env var *or* the context-local + ``set_hermes_home_override()``) while serving another profile, so the + plugin manager must be scoped to the active Hermes home instead of + being one process-wide singleton. + """ + try: + return get_hermes_home().expanduser().resolve() + except Exception: + return get_hermes_home().expanduser() + + +def _clear_plugin_submodules(manager: Optional[PluginManager]) -> None: + """Purge ``sys.modules`` entries for directory-loaded plugins. + + ``PluginManager._load_directory_module`` imports each plugin as + ``hermes_plugins.`` and registers that top-level module in + ``sys.modules``. Anything the plugin's ``__init__.py`` imports with a + *relative* import (``from . import foo``, ``from .sub import bar``) + ends up cached in ``sys.modules`` too, under + ``hermes_plugins..``. When we swap in a fresh manager + for a new home, replacing only the parent module leaves those + submodules behind: if a same-named plugin in the new profile does a + relative import, Python resolves it from ``sys.modules`` first and + silently reuses the *previous* profile's already-imported submodule + (and any module-level state it captured), instead of re-executing the + new profile's code. We must evict the package itself and every module + whose name is prefixed with ``"."`` before (or when) + discarding a manager, not just drop our reference to it. + """ + if manager is None: + return + for loaded in getattr(manager, "_plugins", {}).values(): + module = getattr(loaded, "module", None) + module_name = getattr(module, "__name__", None) + if not module_name or not module_name.startswith(f"{_NS_PARENT}."): + continue + prefix = f"{module_name}." + for name in [n for n in sys.modules if n == module_name or n.startswith(prefix)]: + del sys.modules[name] + def get_plugin_manager() -> PluginManager: - """Return (and lazily create) the global PluginManager singleton.""" + """Return the plugin manager for the active Hermes profile/home. + + Managers are cached per resolved home so repeated calls within the + same profile reuse discovery state (normal performance), while a + profile switch — via ``HERMES_HOME`` or the context-local + ``set_hermes_home_override()`` — gets its own manager with its own + plugin submodules, instead of silently inheriting another profile's + context engine or stale relative-import state. + """ global _plugin_manager - if _plugin_manager is None: - _plugin_manager = PluginManager() - return _plugin_manager + current_home = _plugin_home_key() + + # Tests and embedders historically monkeypatch ``_plugin_manager`` + # directly (``monkeypatch.setattr(plugins_mod, "_plugin_manager", mgr)``). + # Detect that specifically by checking whether the single-slot pointer + # references a manager our keyed cache doesn't know about *at all* + # (i.e. it isn't the value cached for *any* home) — that can only + # happen via a direct assignment bypassing this function, not via a + # legitimate home switch (which always leaves ``_plugin_manager`` + # pointing at a manager already stored in the cache). Comparing against + # only ``current_home``'s slot is wrong: it also matches an ordinary + # switch to a *new* home that simply hasn't been cached yet, which + # would incorrectly resurrect the previous home's manager here. + if _plugin_manager is not None and _plugin_manager not in _plugin_managers_by_home.values(): + _plugin_managers_by_home[current_home] = _plugin_manager + return _plugin_manager + + manager = _plugin_managers_by_home.get(current_home) + if manager is None: + manager = PluginManager() + _plugin_managers_by_home[current_home] = manager + + _plugin_manager = manager + return manager + + +def _reset_plugin_managers_for_tests() -> None: + """Test-only helper: drop every cached manager and its submodules. + + Not used by production code paths — tests that want a fully clean + slate (rather than adopting/injecting a specific manager) can call + this instead of reaching into the module's private dict directly. + """ + global _plugin_manager + for manager in _plugin_managers_by_home.values(): + _clear_plugin_submodules(manager) + _plugin_managers_by_home.clear() + _plugin_manager = None def has_enabled_agent_plugin_mcp(raw_config: Mapping[str, Any]) -> bool: diff --git a/tests/conftest.py b/tests/conftest.py index 8acbf05da2..9c1140db9f 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -517,6 +517,13 @@ def _hermetic_environment(tmp_path, monkeypatch): try: import hermes_cli.plugins as _plugins_mod monkeypatch.setattr(_plugins_mod, "_plugin_manager", None) + # Also clear the keyed per-home manager cache (and any plugin + # submodules it left in sys.modules) so a manager built for a + # previous test's tmp_path HERMES_HOME can't leak forward. Paths + # are unique per test, so collisions are unlikely, but a full + # reset keeps this fixture the single source of plugin-state + # hygiene rather than relying on path uniqueness. + _plugins_mod._reset_plugin_managers_for_tests() except Exception: pass # Explicitly clear provider-specific base URL overrides that don't match diff --git a/tests/hermes_cli/test_plugins.py b/tests/hermes_cli/test_plugins.py index bec12cb6f3..b44e91769e 100644 --- a/tests/hermes_cli/test_plugins.py +++ b/tests/hermes_cli/test_plugins.py @@ -46,7 +46,8 @@ def test_portable_skill_namespace_is_ascii_safe(): def _make_plugin_dir(base: Path, name: str, *, register_body: str = "pass", manifest_extra: dict | None = None, - auto_enable: bool = True) -> Path: + auto_enable: bool = True, + home: Path | None = None) -> Path: """Create a minimal plugin directory with plugin.yaml + __init__.py. If *auto_enable* is True (default), also write the plugin's name into @@ -56,7 +57,19 @@ def _make_plugin_dir(base: Path, name: str, *, register_body: str = "pass", unenabled path. *base* is expected to be ``/plugins/``; we derive - ```` from it by walking one level up. + ```` from it by walking one level up unless *home* is + given explicitly. + + Pass *home* explicitly whenever the target Hermes home for this + plugin isn't necessarily the current ``HERMES_HOME`` env var — e.g. + when writing fixtures for two profiles up front and only switching + ``HERMES_HOME``/``set_hermes_home_override()`` per-profile afterwards + (as multi-profile regression tests do). Relying on the *current* env + var here is a bug: if the caller hasn't switched homes yet (or is + using the context-local override instead of the env var, which this + function can't see), the enable list gets written to the wrong + profile's config.yaml and the plugin never loads for its intended + home. """ plugin_dir = base / name plugin_dir.mkdir(parents=True, exist_ok=True) @@ -74,12 +87,15 @@ def _make_plugin_dir(base: Path, name: str, *, register_body: str = "pass", # Write/merge plugins.enabled in /config.yaml. # Config is always read from HERMES_HOME (not from the project # dir for project plugins), so that's where we opt in. - import os - hermes_home_str = os.environ.get("HERMES_HOME") - if hermes_home_str: - hermes_home = Path(hermes_home_str) + if home is not None: + hermes_home = Path(home) else: - hermes_home = base.parent + import os + hermes_home_str = os.environ.get("HERMES_HOME") + if hermes_home_str: + hermes_home = Path(hermes_home_str) + else: + hermes_home = base.parent hermes_home.mkdir(parents=True, exist_ok=True) cfg_path = hermes_home / "config.yaml" cfg: dict = {} @@ -1175,6 +1191,158 @@ class TestPluginCommands: assert engine is not None assert engine.name == "stub-engine" + def test_plugin_manager_scoped_by_hermes_home_override(self, tmp_path): + """set_hermes_home_override() must get its own manager per profile. + + This is the production path used by the gateway multiplexer + (``gateway/run.py``'s ``_profile_scope`` context manager) and by + subagent/embedded callers: it swaps ``HERMES_HOME`` via a + context-local ContextVar, which — per + ``hermes_constants.set_hermes_home_override`` — deliberately does + NOT touch ``os.environ``. A regression test that only flips the + ``HERMES_HOME`` env var never exercises this path. + """ + from hermes_constants import set_hermes_home_override, reset_hermes_home_override + import hermes_cli.plugins as plugins_mod + + def write_engine_plugin(home: Path) -> None: + _make_plugin_dir( + home / "plugins", + "engine-plugin", + home=home, + manifest_extra={ + "description": "Context engine plugin for profile-scope test", + }, + register_body=( + "import os\n" + " from agent.context_engine import ContextEngine\n\n" + " class HomeEngine(ContextEngine):\n" + " def __init__(self):\n" + " self.home = os.environ.get('HERMES_HOME')\n\n" + " @property\n" + " def name(self):\n" + " return 'home-engine'\n\n" + " def update_from_response(self, usage):\n" + " return None\n\n" + " def should_compress(self, prompt_tokens=None):\n" + " return False\n\n" + " def compress(self, messages, current_tokens=None, focus_topic=None):\n" + " return messages\n\n" + " ctx.register_context_engine(HomeEngine())" + ), + ) + + home_a = tmp_path / "profile-a" + home_b = tmp_path / "profile-b" + write_engine_plugin(home_a) + write_engine_plugin(home_b) + + # Note: HomeEngine reads os.environ['HERMES_HOME'] itself (simulating + # a real plugin like hermes-lcm capturing its home at registration), + # so we set the env var to home_a as a baseline and only use the + # context-local override to *switch away* to home_b — proving the + # override, not the env var, is what get_plugin_manager() keys on. + token_a = set_hermes_home_override(str(home_a)) + try: + manager_a = plugins_mod.get_plugin_manager() + manager_a.discover_and_load() + engine_a = manager_a._context_engine + finally: + reset_hermes_home_override(token_a) + + token_b = set_hermes_home_override(str(home_b)) + try: + manager_b = plugins_mod.get_plugin_manager() + manager_b.discover_and_load() + engine_b = manager_b._context_engine + finally: + reset_hermes_home_override(token_b) + + assert engine_a is not None + assert engine_b is not None + assert manager_a is not manager_b + assert engine_a is not engine_b + + # Re-entering home_a's override must return the SAME cached manager + # (and engine) rather than rebuilding — proves the cache is keyed, + # not last-write-wins. + token_a2 = set_hermes_home_override(str(home_a)) + try: + manager_a2 = plugins_mod.get_plugin_manager() + finally: + reset_hermes_home_override(token_a2) + assert manager_a2 is manager_a + + def test_relative_import_not_leaked_across_home_switch(self, tmp_path): + """A same-slug plugin's relative import must not reuse the prior home's submodule. + + ``_load_directory_module`` imports each plugin as + ``hermes_plugins.``, but a relative import inside the + plugin's ``__init__.py`` (``from .state import STATE``) is cached + separately in ``sys.modules`` under + ``hermes_plugins..state``. If a profile switch replaces only + the parent module, the child module survives in ``sys.modules`` + and Python's import system serves it back unchanged — silently + leaking the previous profile's module-level state (and code) into + the new profile. + """ + from hermes_constants import set_hermes_home_override, reset_hermes_home_override + import hermes_cli.plugins as plugins_mod + + def write_stateful_plugin(home: Path, marker: str) -> None: + plugin_dir = (home / "plugins" / "stateful-plugin") + plugin_dir.mkdir(parents=True, exist_ok=True) + (plugin_dir / "plugin.yaml").write_text( + yaml.dump({ + "name": "stateful-plugin", + "version": "0.1.0", + "description": "Relative-import regression plugin", + }) + ) + # `state.py` is imported via a *relative* import from + # `__init__.py`, so it lands in sys.modules as + # `hermes_plugins.stateful_plugin.state`. + (plugin_dir / "state.py").write_text(f"MARKER = {marker!r}\n") + (plugin_dir / "__init__.py").write_text( + "from . import state\n\n" + "def register(ctx):\n" + " ctx._manager._plugin_skills['stateful-plugin::marker'] = " + "{'marker': state.MARKER}\n" + ) + (home / "config.yaml").write_text( + yaml.safe_dump({"plugins": {"enabled": ["stateful-plugin"]}}) + ) + + home_a = tmp_path / "profile-a" + home_b = tmp_path / "profile-b" + write_stateful_plugin(home_a, "marker-a") + write_stateful_plugin(home_b, "marker-b") + + token_a = set_hermes_home_override(str(home_a)) + try: + manager_a = plugins_mod.get_plugin_manager() + manager_a.discover_and_load() + finally: + reset_hermes_home_override(token_a) + + assert "hermes_plugins.stateful_plugin.state" in sys.modules + assert sys.modules["hermes_plugins.stateful_plugin.state"].MARKER == "marker-a" + + token_b = set_hermes_home_override(str(home_b)) + try: + manager_b = plugins_mod.get_plugin_manager() + manager_b.discover_and_load() + finally: + reset_hermes_home_override(token_b) + + # The submodule cached under sys.modules must now reflect profile + # b's code, not a leftover from profile a. + assert sys.modules["hermes_plugins.stateful_plugin.state"].MARKER == "marker-b" + assert ( + manager_b._plugin_skills["stateful-plugin::marker"]["marker"] == "marker-b" + ) + assert manager_a is not manager_b +