From aeff56aa3583fd6500a9d5f75ce1421dc93b679f Mon Sep 17 00:00:00 2001 From: teknium1 <127238744+teknium1@users.noreply.github.com> Date: Mon, 21 Sep 2026 22:38:28 -0700 Subject: [PATCH 001/173] fix(plugins): loader gates read the running version, survive SystemExit, quarantine deps everywhere - requires_hermes compared against stale editable-install dist metadata (0.21.0) while the checkout ran 0.21.4, skipping plugins that required the release in use; hermes_cli.__version__ is now the source of truth, dist metadata only a fallback. - PEP 440 pre/post suffixes glued to a segment (99.0.0rc1) made a clause permissive and an rc running version disabled every gate; the segment parser drops the suffix. - A plugin calling sys.exit() at import or in register() propagated SystemExit out of discovery: the whole registry emptied and `hermes chat` exited 3 with no output. Load isolation now covers SystemExit (KeyboardInterrupt still propagates). - uv reads [tool.uv] exclude-newer from the cwd project only, so lazy/plugin dep installs launched from $HOME, a gateway service or the Desktop backend were never quarantined; the uv tier now runs from the checkout root when one exists. - A flat user/project manifest naming a bundled key from a differently named directory no longer displaces the bundled plugin (warn + skip); a same-named override is logged at INFO. --- hermes_cli/plugins.py | 7 ++++--- hermes_cli/plugins_discovery.py | 28 +++++++++++++++++++++++++++- hermes_cli/plugins_loader.py | 23 ++++++++++++++++------- hermes_cli/plugins_manifest.py | 31 ++++++++++++++++++++++--------- tools/lazy_deps.py | 12 +++++++++++- 5 files changed, 80 insertions(+), 21 deletions(-) diff --git a/hermes_cli/plugins.py b/hermes_cli/plugins.py index 15fa39d715..8d8bfddf5c 100644 --- a/hermes_cli/plugins.py +++ b/hermes_cli/plugins.py @@ -43,7 +43,7 @@ from hermes_cli.plugins_manifest import ( # noqa: F401 — re-exported ) from hermes_cli.plugins_discovery import ( # noqa: F401 — re-exported ENTRY_POINTS_GROUP, _get_disabled_plugins, _get_enabled_plugins, collect_directory_manifests, - discover_entrypoint_manifests, gate_manifest, scan_directory, + discover_entrypoint_manifests, gate_manifest, resolve_manifest_winners, scan_directory, ) from hermes_cli.plugins_loader import ( PluginLoaderMixin, _BARE_MODULE_SCOPE, _MODULE_NAMESPACE_LOCK, _NS_PARENT, _evict_modules, @@ -1332,9 +1332,10 @@ class PluginManager(PluginLoaderMixin, PluginDispatchMixin, PluginLedgerMixin): logger.warning("Removed Hermes plugin %s is still listed in plugins.enabled; " "remove it and configure native Relay plugins with %s", ", ".join(stale_relay_keys), RELAY_PLUGINS_CONFIG_ENV) - # Later sources win on key collision (project > user > bundled); gate the winners, then + # Later sources win on key collision (project > user > bundled) except a flat impostor claiming a + # bundled key from another directory (resolve_manifest_winners); gate the winners, then # load survivors in requires_plugins order (see resolve_plugin_load_order). - winners = {manifest_key(m): m for m in manifests} + winners = resolve_manifest_winners(manifests) to_load = {k: m for k, m in winners.items() if self._gate_manifest(m, disabled, enabled)} for lookup_key in resolve_plugin_load_order(to_load): manifest = to_load[lookup_key] diff --git a/hermes_cli/plugins_discovery.py b/hermes_cli/plugins_discovery.py index dd38a5edb8..de17dec580 100644 --- a/hermes_cli/plugins_discovery.py +++ b/hermes_cli/plugins_discovery.py @@ -10,7 +10,7 @@ import importlib.metadata import logging from dataclasses import dataclass from pathlib import Path -from typing import Any, List, Optional, Set +from typing import Any, Dict, List, Optional, Set from hermes_constants import get_hermes_home from hermes_cli.config import cfg_get @@ -167,6 +167,32 @@ def collect_directory_manifests() -> List[PluginManifest]: return manifests +def resolve_manifest_winners(manifests: List[PluginManifest]) -> Dict[str, PluginManifest]: + """Later sources win on key collision (project > user > bundled): a same-named copy under + ``~/.hermes/plugins/`` is the documented way to override a bundled plugin, and is logged. A flat + user/project manifest that claims a bundled key from a *differently named* directory is an impostor, not + an override (``impostor_dir/plugin.yaml`` with ``name: kanban``): it is skipped with a warning so + ``hermes plugins enable kanban`` never activates unrelated code under the bundled name.""" + winners: Dict[str, PluginManifest] = {} + for manifest in manifests: + key = manifest_key(manifest) + shadowed = winners.get(key) + if shadowed is not None and shadowed.source == "bundled" and manifest.source in {"user", "project"}: + own_dir = Path(manifest.path).name if manifest.path else "" + bundled_dir = Path(shadowed.path).name if shadowed.path else "" + if own_dir and bundled_dir and own_dir != bundled_dir: + logger.warning( + "Ignoring %s plugin at %s: its manifest name '%s' is a bundled plugin's key but the " + "directory is named '%s'; rename the directory to '%s' to override the bundled plugin", + manifest.source, manifest.path, key, own_dir, bundled_dir, + ) + continue + logger.info("Plugin '%s' at %s (%s) shadows the bundled copy at %s", key, manifest.path, + manifest.source, shadowed.path) + winners[key] = manifest + return winners + + @dataclass(frozen=True) class ManifestGate: """Routing verdict for one winning manifest (see :func:`gate_manifest`).""" diff --git a/hermes_cli/plugins_loader.py b/hermes_cli/plugins_loader.py index 3af0120ebf..416eb12f1a 100644 --- a/hermes_cli/plugins_loader.py +++ b/hermes_cli/plugins_loader.py @@ -64,6 +64,13 @@ def _plugin_home_scope(home: Path): reset_hermes_home_override(token) +def _load_error_text(exc: BaseException) -> str: + """Human-readable load failure; ``sys.exit(0)`` has an empty ``str()`` so name the class and code.""" + if isinstance(exc, SystemExit): + return f"SystemExit({exc.code!r}) raised during import/register()" + return str(exc) + + def _dist_installed(req: str) -> Optional[bool]: """Best-effort presence probe on a requirement's distribution name; ``None`` when unprobeable.""" dist = re.split(r"[<>=!~\[;\s]", req, maxsplit=1)[0].strip() @@ -199,7 +206,7 @@ class PluginLoaderMixin: "Deferred platform '%s': pre-registered %d client tool(s) %s", lookup_key, len(registered), registered, ) - except Exception as exc: + except (Exception, SystemExit) as exc: # Tools registered before the raise are live: credit them or `hermes plugins list` under-reports # (and _load_plugin's later diff would miss them too). Never break discovery (the platform stays # deferred), but a broken tools.py IS the symptom, so warn — and say where it failed first. @@ -314,15 +321,17 @@ class PluginLoaderMixin: from hermes_cli.plugins_ledger import _hook_source_of self._drop_fallback_hooks(_hook_source_of(manifest.name, module)) - except Exception as exc: + except (Exception, SystemExit) as exc: + # SystemExit too: a plugin module with an unguarded ``main()``/``sys.exit()`` must not take the + # whole process (and every other plugin's registry) down with it; KeyboardInterrupt still propagates. owned = [r for r in self._registration_order if r.plugin_key == plugin_key] self._dispose_registrations(owned) self._forget_registrations(owned) - loaded.error = str(exc) + loaded.error = _load_error_text(exc) # register() may have subscribed before raising; a failed plugin must leave no callable reachable # from later event dispatch. self._remove_plugin_subscriptions(plugin_key) - logger.warning("Failed to load plugin '%s': %s", manifest.name, exc, exc_info=_PLUGINS_DEBUG) + logger.warning("Failed to load plugin '%s': %s", manifest.name, _load_error_text(exc), exc_info=_PLUGINS_DEBUG) # The failure path swept this plugin's whole ledger (not just the registration_start slice), so # discovery-time pre-registrations are gone too. # There is no live tool left to credit — attribution and the registry agree at zero. Only the @@ -400,9 +409,9 @@ class PluginLoaderMixin: continue self._portable_mcp_servers[internal_name] = dict(config) loaded.enabled = True - except Exception as exc: - loaded.error = str(exc) - logger.warning("Failed to load Agent Plugin '%s': %s", lookup_key, exc) + except (Exception, SystemExit) as exc: + loaded.error = _load_error_text(exc) + logger.warning("Failed to load Agent Plugin '%s': %s", lookup_key, loaded.error) self._plugins[lookup_key] = loaded def _directory_module_name(self, manifest: PluginManifest) -> str: diff --git a/hermes_cli/plugins_manifest.py b/hermes_cli/plugins_manifest.py index 92b36904b3..1fd45dc037 100644 --- a/hermes_cli/plugins_manifest.py +++ b/hermes_cli/plugins_manifest.py @@ -375,22 +375,35 @@ _VERSION_COMPARATOR_RE = re.compile(r"^\s*(>=|<=|==|!=|>|<)\s*(.+?)\s*$") def running_hermes_version() -> str: - """Installed ``hermes-agent`` distribution version, else ``hermes_cli.__version__`` (source checkout).""" + """Version of the Hermes code that is running: ``hermes_cli.__version__``. Distribution metadata is only + a fallback — on an editable/source install it is frozen at ``pip install -e`` time and drifts from the + checkout after every ``git pull`` (dist said 0.21.0 while the code was 0.21.4), so gating on it skipped + plugins that required exactly the release the user was running.""" try: - return importlib.metadata.version("hermes-agent") - except Exception: from hermes_cli import __version__ - return __version__ + if __version__: + return str(__version__) + except Exception: + pass + return importlib.metadata.version("hermes-agent") + + +_VERSION_SEGMENT_RE = re.compile(r"^\d+") def _version_tuple(v: str) -> Optional[tuple]: - """``v1.2.3-rc1`` → ``(1, 2, 3)``; ``None`` when a segment is non-numeric.""" + """``v1.2.3-rc1`` / ``1.2.3rc1`` / ``1.2.3.post1`` → ``(1, 2, 3)``; ``None`` when a segment has no + leading digits. PEP 440 pre/post/dev suffixes glued to a segment (``0rc1``) are dropped so an rc + *target* still gates and an rc *running* version does not disable every gate.""" parts = re.split(r"[-+]", str(v).strip().lstrip("v"), 1)[0].split(".") parts += ["0"] * (3 - len(parts)) - try: - return tuple(int(x) for x in parts[:3]) - except ValueError: - return None + out = [] + for x in parts[:3]: + m = _VERSION_SEGMENT_RE.match(x.strip()) + if m is None: + return None + out.append(int(m.group(0))) + return tuple(out) def version_satisfies(spec: str, current: str) -> bool: diff --git a/tools/lazy_deps.py b/tools/lazy_deps.py index 675dd5d21f..c9b4e16c02 100644 --- a/tools/lazy_deps.py +++ b/tools/lazy_deps.py @@ -528,6 +528,15 @@ def _run_installer(cmd: list[str], **kw) -> subprocess.CompletedProcess: return subprocess.run(cmd, **_SUBPROCESS_KW, creationflags=windows_hide_flags(), **kw) +def _uv_policy_cwd() -> Optional[str]: + """Directory uv must run from so the checkout's ``[tool.uv]`` policy (``exclude-newer`` quarantine and its + per-package exceptions) applies: uv reads it from the *current directory's* project only, so a lazy or + plugin install launched from ``$HOME``, a gateway service or the Desktop backend was never quarantined. + ``None`` (inherit cwd) when this is not a source checkout.""" + root = Path(__file__).resolve().parent.parent + return str(root) if (root / "pyproject.toml").is_file() else None + + def _uv_binary() -> Optional[str]: """Managed uv first ($HERMES_HOME/bin is never on PATH), then PATH. A lookup, not ensure_uv(): downloading uv mid-turn is more than the caller asked for; pip covers no-uv.""" @@ -599,7 +608,8 @@ def _venv_pip_install(specs: tuple[str, ...], *, timeout: int = 300, constraint_ if pip_index_url: uv_env["UV_INDEX_URL"] = pip_index_url try: - r = _run_installer([uv_bin, "pip", "install", "--compile-bytecode", *extra_args, *specs], timeout=timeout, env=uv_env) + r = _run_installer([uv_bin, "pip", "install", "--compile-bytecode", *extra_args, *specs], + timeout=timeout, env=uv_env, cwd=_uv_policy_cwd()) if r.returncode != 0: logger.debug("uv pip install failed: %s", r.stderr) # A uv resolver failure is authoritative: falling through to pip would discard uv From 89f0ae80d43b256403431f9fe21dcd9ba5db38e0 Mon Sep 17 00:00:00 2001 From: liuhao1024 Date: Mon, 7 Sep 2026 00:08:15 +0800 Subject: [PATCH 002/173] fix(plugins): accept str skill paths in register_skill Plugin register() helpers commonly pass the SKILL.md location as a filesystem string (PluginManifest.path itself is stored as str), but register_skill() called path.exists() directly, so a valid string path aborted the whole plugin load with "'str' object has no attribute 'exists'" instead of registering. Coerce to Path up front so the registry entry and find_plugin_skill() keep their Path contract, and a missing location still fails with FileNotFoundError. Fixes #104404 --- hermes_cli/plugins.py | 4 ++++ tests/tools/test_plugin_skills.py | 20 ++++++++++++++++++++ 2 files changed, 24 insertions(+) diff --git a/hermes_cli/plugins.py b/hermes_cli/plugins.py index 8d8bfddf5c..23dff081af 100644 --- a/hermes_cli/plugins.py +++ b/hermes_cli/plugins.py @@ -1007,6 +1007,10 @@ class PluginContext: f"plugin name '{self.manifest.name}' automatically).") if not name or not _NAMESPACE_RE.match(name): raise ValueError(f"Invalid skill name '{name}'. Must match [a-zA-Z0-9_-]+.") + # Plugin register() helpers commonly pass the SKILL.md location as str + # (PluginManifest.path is stored as str); the registry and find_plugin_skill() + # promise a Path downstream. + path = Path(path) if not path.exists(): raise FileNotFoundError(f"SKILL.md not found at {path}") namespace = self.manifest.skill_namespace or self.manifest.name diff --git a/tests/tools/test_plugin_skills.py b/tests/tools/test_plugin_skills.py index b2f49061fa..5a58b3ad6c 100644 --- a/tests/tools/test_plugin_skills.py +++ b/tests/tools/test_plugin_skills.py @@ -139,6 +139,26 @@ class TestPluginContextRegisterSkill: with pytest.raises(FileNotFoundError): ctx.register_skill("foo", tmp_path / "nonexistent.md") + def test_accepts_str_path(self, ctx, tmp_path): + # Plugin register() helpers commonly pass the SKILL.md location as str + # (#104404); this used to abort the whole plugin load with + # "'str' object has no attribute 'exists'" instead of registering. + from pathlib import Path + + skill_md = tmp_path / "skills" / "my-skill" / "SKILL.md" + skill_md.parent.mkdir(parents=True) + skill_md.write_text("---\nname: my-skill\n---\nContent.\n") + + ctx.register_skill("my-skill", str(skill_md), "A test skill") + + found = ctx._manager.find_plugin_skill("testplugin:my-skill") + assert found == skill_md + assert isinstance(found, Path) + + def test_missing_str_path_raises_filenotfound(self, ctx, tmp_path): + with pytest.raises(FileNotFoundError): + ctx.register_skill("foo", str(tmp_path / "nonexistent.md")) + def test_duplicate_qualified_name_is_rejected(self, ctx, tmp_path): ctx.manifest.portable = True first = tmp_path / "first" / "SKILL.md" From 9778f00970fb640f43afff907846f81b52cf6aab Mon Sep 17 00:00:00 2001 From: mannnrachman Date: Mon, 27 Jul 2026 00:26:19 +0800 Subject: [PATCH 003/173] fix(plugins): register entry-point plugins declared as module:function MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit EntryPoint.load() resolves the module:function form to the referenced attribute (typically the register callable itself), not its module. The loader then looked for a .register attribute on that function object, found none, and warned "Plugin '' has no register() function" on every discovery pass — so pip plugins with module:function entry points never registered at all. Detect a non-module callable from ep.load() and use it directly as the register function, resolving LoadedPlugin.module from sys.modules via the callable's __module__ for attribution. The existing entry-point test masked this because its mocked ep.load() returned the module even though its declared value was module:function; the new regression test mirrors real importlib behavior. Fixes #72052 Co-Authored-By: Claude Fable 5 --- hermes_cli/plugins_loader.py | 16 ++++++++--- tests/hermes_cli/test_plugins.py | 48 ++++++++++++++++++++++++++++++++ 2 files changed, 60 insertions(+), 4 deletions(-) diff --git a/hermes_cli/plugins_loader.py b/hermes_cli/plugins_loader.py index 416eb12f1a..e072ae9aaf 100644 --- a/hermes_cli/plugins_loader.py +++ b/hermes_cli/plugins_loader.py @@ -19,7 +19,7 @@ import types from contextlib import contextmanager from functools import wraps from pathlib import Path -from typing import TYPE_CHECKING, Any, Dict, List, Mapping, Optional +from typing import TYPE_CHECKING, Any, Callable, Dict, List, Mapping, Optional, Union from hermes_constants import get_hermes_home, reset_hermes_home_override, set_hermes_home_override from registration_lifecycle import replacement_coordinator @@ -309,8 +309,15 @@ class PluginLoaderMixin: module = self._load_directory_module(manifest, module_name=module_name) elif module is None: module = self._load_entrypoint_module(manifest) + register_fn = None + if module is not None and not isinstance(module, types.ModuleType) and callable(module): + # An entry point declared as ``module:function`` resolves to the function object itself via + # ``ep.load()``, not its module (#72052). + register_fn = module + module = sys.modules.get(getattr(register_fn, "__module__", "")) loaded.module = module - register_fn = getattr(module, "register", None) + if register_fn is None: + register_fn = getattr(module, "register", None) if register_fn is None: loaded.error = "no register() function" logger.warning("Plugin '%s' has no register() function", manifest.name) @@ -470,8 +477,9 @@ class PluginLoaderMixin: raise return module - def _load_entrypoint_module(self, manifest: PluginManifest) -> types.ModuleType: - """Load a pip-installed plugin via its entry-point reference.""" + def _load_entrypoint_module(self, manifest: PluginManifest) -> Union[types.ModuleType, Callable[..., Any]]: + """Load a pip-installed plugin via its entry-point reference: the module for a bare ``module`` target, + the referenced attribute (normally ``register``) for the ``module:function`` form.""" for ep in _select_entry_point_group(importlib.metadata.entry_points(), ENTRY_POINTS_GROUP): if ep.name == manifest.name: return ep.load() diff --git a/tests/hermes_cli/test_plugins.py b/tests/hermes_cli/test_plugins.py index 0459d26500..d4e5e0ca65 100644 --- a/tests/hermes_cli/test_plugins.py +++ b/tests/hermes_cli/test_plugins.py @@ -395,6 +395,54 @@ class TestPluginDiscovery: + def test_entry_point_function_form_registers(self, tmp_path, monkeypatch): + """Entry points declared as ``module:function`` register via the callable. + + Regression for #72052: real ``EntryPoint.load()`` returns the referenced + attribute for the ``module:function`` form, not the module. The loader + used to look for ``.register`` on that function object, find nothing, + and warn "no register() function" on every discovery pass. + """ + hermes_home = tmp_path / "hermes_test" + hermes_home.mkdir(parents=True, exist_ok=True) + monkeypatch.setenv("HERMES_HOME", str(hermes_home)) + # Entry-point plugins load only when opted into plugins.enabled. + (hermes_home / "config.yaml").write_text( + yaml.safe_dump({"plugins": {"enabled": ["fn_plugin"]}}) + ) + + fake_module = types.ModuleType("fake_fn_plugin") + register_calls = [] + + def register(ctx): + register_calls.append(ctx) + + register.__module__ = "fake_fn_plugin" + fake_module.register = register # type: ignore[attr-defined] + monkeypatch.setitem(sys.modules, "fake_fn_plugin", fake_module) + + fake_ep = MagicMock() + fake_ep.name = "fn_plugin" + fake_ep.value = "fake_fn_plugin:register" + fake_ep.group = ENTRY_POINTS_GROUP + # Mirror real importlib behavior: load() resolves to the attribute. + fake_ep.load.return_value = register + + def fake_entry_points(): + result = MagicMock() + result.select = MagicMock(return_value=[fake_ep]) + return result + + with patch("importlib.metadata.entry_points", fake_entry_points): + mgr = PluginManager() + mgr.discover_and_load() + + entry = mgr._plugins["fn_plugin"] + assert entry.error is None, entry.error + assert entry.enabled + assert len(register_calls) == 1 + assert entry.module is fake_module + def test_force_rediscover_clears_all_plugin_registries(self, monkeypatch): """force=True must clear every plugin-populated registry. From f8dfa68fb39b21e503e78eeb55358705df84d35b Mon Sep 17 00:00:00 2001 From: Christopher <210261288+Christopher-Schulze@users.noreply.github.com> Date: Sat, 15 Aug 2026 15:23:25 +0200 Subject: [PATCH 004/173] fix(plugins): skip dunder dirs and contain plugin-scan OSErrors Walking __pycache__ and other dunder children let an unreadable directory raise out of plugin discovery and fail unrelated tool calls. Skip those names and treat iterdir/is_dir OSError as an empty scan for that node. Fixes #86996 --- hermes_cli/plugins_discovery.py | 12 +++- .../test_plugin_scan_dunder_dirs.py | 56 +++++++++++++++++++ 2 files changed, 67 insertions(+), 1 deletion(-) create mode 100644 tests/hermes_cli/test_plugin_scan_dunder_dirs.py diff --git a/hermes_cli/plugins_discovery.py b/hermes_cli/plugins_discovery.py index de17dec580..8c2a8451dd 100644 --- a/hermes_cli/plugins_discovery.py +++ b/hermes_cli/plugins_discovery.py @@ -109,7 +109,17 @@ def scan_directory( manifests: List[PluginManifest] = [] if not path.is_dir(): return manifests - for child in sorted(path.iterdir()): + try: + children = sorted(path.iterdir()) + except OSError as exc: + logger.warning("Failed to scan plugin directory %s: %s", path, exc) + return manifests + for child in children: + # Cache/dunder dirs (__pycache__, __MACOSX__, …) are never + # plugins. Walking them can raise PermissionError and take + # down every subsequent tool call (#86996). + if child.name.startswith("__") and child.name.endswith("__"): + continue try: if not child.is_dir() or (depth == 0 and skip_names and child.name in skip_names): continue diff --git a/tests/hermes_cli/test_plugin_scan_dunder_dirs.py b/tests/hermes_cli/test_plugin_scan_dunder_dirs.py new file mode 100644 index 0000000000..f409a4be18 --- /dev/null +++ b/tests/hermes_cli/test_plugin_scan_dunder_dirs.py @@ -0,0 +1,56 @@ +"""Plugin directory scans must skip dunder dirs and survive OSError (#86996).""" + +from __future__ import annotations + +from pathlib import Path + +from hermes_cli.plugins import PluginManager + + +def test_scan_skips_dunder_dirs_and_still_loads_real_plugin(tmp_path: Path): + root = tmp_path / "plugins" + demo = root / "demo" + demo.mkdir(parents=True) + (demo / "plugin.yaml").write_text("name: demo\nversion: 0.1.0\ndescription: x\n") + (demo / "__pycache__").mkdir() + pycache = root / "__pycache__" + pycache.mkdir() + pycache.chmod(0o000) + try: + found = PluginManager()._scan_directory(root, "user") + finally: + pycache.chmod(0o700) + + assert [manifest.name for manifest in found] == ["demo"] + + +def test_scan_survives_iterdir_oserror(tmp_path: Path, monkeypatch): + root = tmp_path / "plugins" + root.mkdir() + + def _boom(_self): + raise PermissionError("unreadable plugin root") + + monkeypatch.setattr(Path, "iterdir", _boom, raising=False) + found = PluginManager()._scan_directory(root, "user") + assert found == [] + + +def test_scan_skips_child_whose_is_dir_raises(tmp_path: Path, monkeypatch): + root = tmp_path / "plugins" + demo = root / "demo" + demo.mkdir(parents=True) + (demo / "plugin.yaml").write_text("name: demo\nversion: 0.1.0\ndescription: x\n") + spooky = root / "spooky" + spooky.mkdir() + + original_is_dir = Path.is_dir + + def _is_dir(self): + if self.name == "spooky": + raise PermissionError("stat failed") + return original_is_dir(self) + + monkeypatch.setattr(Path, "is_dir", _is_dir) + found = PluginManager()._scan_directory(root, "user") + assert [manifest.name for manifest in found] == ["demo"] From d0c1498a0dd2a11beaccd891413f976675da7f8e Mon Sep 17 00:00:00 2001 From: Christopher <210261288+Christopher-Schulze@users.noreply.github.com> Date: Sat, 15 Aug 2026 22:26:09 +0200 Subject: [PATCH 005/173] fix(plugins): skip dunder dirs without POSIX chmod in the scanner test Name-based dunder skipping does not need an unreadable __pycache__. Drop the chmod(0) setup so the test is portable, and log skipped dunder paths at debug. --- hermes_cli/plugins_discovery.py | 3 +++ tests/hermes_cli/test_plugin_scan_dunder_dirs.py | 11 ++++------- 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/hermes_cli/plugins_discovery.py b/hermes_cli/plugins_discovery.py index 8c2a8451dd..79c9f691fd 100644 --- a/hermes_cli/plugins_discovery.py +++ b/hermes_cli/plugins_discovery.py @@ -119,7 +119,10 @@ def scan_directory( # plugins. Walking them can raise PermissionError and take # down every subsequent tool call (#86996). if child.name.startswith("__") and child.name.endswith("__"): + logger.debug("Skipping dunder plugin path %s", child) continue + # pathlib.Path.is_dir() swallows OSError, but injected Path-likes + # and test doubles can still raise. Fail closed per child. try: if not child.is_dir() or (depth == 0 and skip_names and child.name in skip_names): continue diff --git a/tests/hermes_cli/test_plugin_scan_dunder_dirs.py b/tests/hermes_cli/test_plugin_scan_dunder_dirs.py index f409a4be18..cc90dd9c25 100644 --- a/tests/hermes_cli/test_plugin_scan_dunder_dirs.py +++ b/tests/hermes_cli/test_plugin_scan_dunder_dirs.py @@ -13,13 +13,10 @@ def test_scan_skips_dunder_dirs_and_still_loads_real_plugin(tmp_path: Path): demo.mkdir(parents=True) (demo / "plugin.yaml").write_text("name: demo\nversion: 0.1.0\ndescription: x\n") (demo / "__pycache__").mkdir() - pycache = root / "__pycache__" - pycache.mkdir() - pycache.chmod(0o000) - try: - found = PluginManager()._scan_directory(root, "user") - finally: - pycache.chmod(0o700) + (root / "__pycache__").mkdir() + (root / "__MACOSX__").mkdir() + + found = PluginManager()._scan_directory(root, "user") assert [manifest.name for manifest in found] == ["demo"] From c60693278ab5c63bd657a1fc9d578de435a8645f Mon Sep 17 00:00:00 2001 From: liuhao1024 Date: Thu, 3 Sep 2026 15:46:56 +0800 Subject: [PATCH 006/173] fix(plugins): skip foreign-harness manifest dirs during discovery MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Multi-harness plugin repos (e.g. obra/superpowers) ship one plugin.json per OTHER agent harness inside .claude-plugin/, .codex-plugin/, .cursor-plugin/, .devin-plugin/ and .kimi-plugin/. Those manifests can never satisfy the Agent Plugins v1 schema, so every discovery pass rejected each one and logged a warning — ~1,500 warnings/day on the reporter's install (#101962). Skip these well-known per-harness convention directories during directory scanning. A plugin's real Hermes manifest (.hermes-plugin/plugin.yaml or a top-level plugin.yaml/plugin.json) is unaffected, and genuinely broken portable manifests still warn. Fixes #101962 --- hermes_cli/plugins_discovery.py | 10 +++ .../test_plugin_scanner_recursion.py | 69 +++++++++++++++++++ 2 files changed, 79 insertions(+) diff --git a/hermes_cli/plugins_discovery.py b/hermes_cli/plugins_discovery.py index 79c9f691fd..21d5850696 100644 --- a/hermes_cli/plugins_discovery.py +++ b/hermes_cli/plugins_discovery.py @@ -27,6 +27,13 @@ logger = logging.getLogger("hermes_cli.plugins") ENTRY_POINTS_GROUP = "hermes_agent.plugins" ENTRY_POINT_CAPABILITIES_GROUP = "hermes_agent.plugin_capabilities" +# Per-harness manifest directories plugin repos ship for OTHER agent harnesses (e.g. obra/superpowers keeps one +# plugin.json per harness). Their plugin.json is not an Agent Plugins v1 manifest and can never validate, so +# parsing it on every discovery pass only spams warnings (#101962). +_FOREIGN_HARNESS_MANIFEST_DIRS = frozenset({ + ".claude-plugin", ".codex-plugin", ".cursor-plugin", ".devin-plugin", ".kimi-plugin", +}) + def _select_entry_point_group(entry_points: Any, group: str) -> list: """Return one metadata entry-point group across supported Python APIs.""" @@ -121,6 +128,9 @@ def scan_directory( if child.name.startswith("__") and child.name.endswith("__"): logger.debug("Skipping dunder plugin path %s", child) continue + if child.name in _FOREIGN_HARNESS_MANIFEST_DIRS: + logger.debug("Skipping %s (foreign-harness manifest convention)", child) + continue # pathlib.Path.is_dir() swallows OSError, but injected Path-likes # and test doubles can still raise. Fail closed per child. try: diff --git a/tests/hermes_cli/test_plugin_scanner_recursion.py b/tests/hermes_cli/test_plugin_scanner_recursion.py index 4a0614f959..8ac876f43d 100644 --- a/tests/hermes_cli/test_plugin_scanner_recursion.py +++ b/tests/hermes_cli/test_plugin_scanner_recursion.py @@ -8,6 +8,7 @@ still opt-in; exclusive kind skipped; unknown kinds → standalone warning). from __future__ import annotations +import json from pathlib import Path from typing import Any, Dict @@ -113,6 +114,74 @@ class TestCategoryNamespaceRecursion: assert non_bundled == [] +# ── Foreign-harness manifest dirs (#101962) ──────────────────────────────── + + +class TestForeignHarnessManifestDirs: + def test_foreign_harness_dirs_skipped_without_warnings( + self, tmp_path, monkeypatch, caplog + ): + """Multi-harness plugin repos (e.g. obra/superpowers) ship one + ``plugin.json`` per OTHER agent harness inside ``.claude-plugin/``, + ``.codex-plugin/`` etc. Those manifests can never satisfy the Agent + Plugins v1 schema, so scanning them warned on every discovery pass. + They must be skipped silently; the plugin's real Hermes manifest + (``.hermes-plugin/plugin.yaml``) is still discovered.""" + import os + hermes_home = Path(os.environ["HERMES_HOME"]) # set by hermetic conftest fixture + sp = hermes_home / "plugins" / "superpowers" + (sp / ".hermes-plugin").mkdir(parents=True) + (sp / ".hermes-plugin" / "plugin.yaml").write_text( + yaml.dump( + { + "name": "superpowers", + "version": "6.3.0", + "description": "multi-harness plugin", + } + ) + ) + for harness in ( + ".claude-plugin", + ".codex-plugin", + ".cursor-plugin", + ".devin-plugin", + ".kimi-plugin", + ): + harness_dir = sp / harness + harness_dir.mkdir(parents=True) + (harness_dir / "plugin.json").write_text( + json.dumps({"name": "superpowers", "version": "6.3.0"}) + ) + + with caplog.at_level("WARNING", logger="hermes_cli.plugins"): + mgr = PluginManager() + mgr.discover_and_load() + + assert "superpowers/.hermes-plugin" in mgr._plugins + parse_warnings = [ + r for r in caplog.records if "Failed to parse" in r.getMessage() + ] + assert parse_warnings == [] + + def test_broken_portable_plugin_still_warns(self, tmp_path, monkeypatch, caplog): + """A genuinely broken portable plugin.json (not a foreign-harness + convention directory) must still surface its parse warning.""" + import os + hermes_home = Path(os.environ["HERMES_HOME"]) # set by hermetic conftest fixture + broken = hermes_home / "plugins" / "broken-portable" + broken.mkdir(parents=True) + (broken / "plugin.json").write_text(json.dumps({"name": "broken"})) + + with caplog.at_level("WARNING", logger="hermes_cli.plugins"): + mgr = PluginManager() + mgr.discover_and_load() + + assert any( + "Failed to parse" in r.getMessage() and "broken-portable" in r.getMessage() + for r in caplog.records + ) + + # ── Kind parsing ─────────────────────────────────────────────────────────── From de6374194c4023135d3b637ced80dc8fd3a406ca Mon Sep 17 00:00:00 2001 From: KoNit-K Date: Wed, 16 Sep 2026 01:15:14 +0800 Subject: [PATCH 007/173] fix(plugins): clean failed sibling modules --- plugins/plugin_loader.py | 2 + tests/plugins/test_plugin_loader.py | 70 +++++++++++++++++++++++++++++ 2 files changed, 72 insertions(+) create mode 100644 tests/plugins/test_plugin_loader.py diff --git a/plugins/plugin_loader.py b/plugins/plugin_loader.py index 84111d6704..9c93a12644 100644 --- a/plugins/plugin_loader.py +++ b/plugins/plugin_loader.py @@ -120,6 +120,8 @@ def load_plugin_module(module_name: str, plugin_dir: Path, *, parents: Tuple[str sub_mod = _new_module(full_sub_name, sub_file) if _exec(sub_mod, logger): loaded_submodules.append((sub_file.stem, sub_mod)) + else: + sys.modules.pop(full_sub_name, None) if not _exec(mod, logger): sys.modules.pop(module_name, None) return None diff --git a/tests/plugins/test_plugin_loader.py b/tests/plugins/test_plugin_loader.py new file mode 100644 index 0000000000..1ed3e8e1a4 --- /dev/null +++ b/tests/plugins/test_plugin_loader.py @@ -0,0 +1,70 @@ +"""Regression tests for directory-plugin module loading.""" + +from __future__ import annotations + +import logging +import sys + +from plugins.plugin_loader import load_plugin_module + + +def test_failed_sibling_is_removed_before_init_handles_missing_import(tmp_path): + """A failed eager sibling import must remain catchable as ModuleNotFoundError.""" + plugin_dir = tmp_path / "plugin" + plugin_dir.mkdir() + (plugin_dir / "broken.py").write_text( + "from .missing_dependency import value\n", + encoding="utf-8", + ) + (plugin_dir / "__init__.py").write_text( + "try:\n" + " from .broken import value\n" + "except ModuleNotFoundError:\n" + " fallback_used = True\n", + encoding="utf-8", + ) + module_name = "test_plugin_loader_package.failed_sibling" + + try: + module = load_plugin_module( + module_name, + plugin_dir, + parents=(), + logger=logging.getLogger(__name__), + ) + + assert module is not None + assert module.fallback_used is True + assert f"{module_name}.broken" not in sys.modules + finally: + for name in tuple(sys.modules): + if name == module_name or name.startswith(f"{module_name}."): + sys.modules.pop(name, None) + + +def test_successful_sibling_remains_available_on_loaded_module(tmp_path): + """Cleaning failed siblings must not alter the eager success path.""" + plugin_dir = tmp_path / "plugin" + plugin_dir.mkdir() + (plugin_dir / "helper.py").write_text("value = 42\n", encoding="utf-8") + (plugin_dir / "__init__.py").write_text( + "from .helper import value\n", + encoding="utf-8", + ) + module_name = "test_plugin_loader_package.successful_sibling" + + try: + module = load_plugin_module( + module_name, + plugin_dir, + parents=(), + logger=logging.getLogger(__name__), + ) + + assert module is not None + assert module.value == 42 + assert module.helper.value == 42 + finally: + for name in tuple(sys.modules): + if name == module_name or name.startswith(f"{module_name}."): + sys.modules.pop(name, None) From 82d6ab99c474b01c4d9f83dd58f9e58231c1de1a Mon Sep 17 00:00:00 2001 From: tachyon-r <291518778+tachyon-r@users.noreply.github.com> Date: Sat, 29 Aug 2026 04:03:32 -0400 Subject: [PATCH 008/173] fix: declare security-guidance hooks in manifest --- plugins/security-guidance/plugin.yaml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/plugins/security-guidance/plugin.yaml b/plugins/security-guidance/plugin.yaml index 9756729995..0224a28029 100644 --- a/plugins/security-guidance/plugin.yaml +++ b/plugins/security-guidance/plugin.yaml @@ -2,6 +2,6 @@ name: security-guidance version: "0.1.0" description: "Append security warnings to file-write tool results when the new content contains known-dangerous patterns (pickle.load, yaml.load, eval(, os.system, dangerouslySetInnerHTML, verify=False, ECB, XXE, GitHub Actions injection, ...). 25 regex/substring rules forked from Anthropic's claude-plugins-official under Apache-2.0. Non-blocking — the file is written and the warning rides back to the model in the next turn so it can self-correct." author: "Anthropic (patterns, Apache-2.0) / NousResearch (Hermes plugin port)" -hooks: +provides_hooks: - transform_tool_result - pre_tool_call From 5b331a8327420715c85edf0f39323fe0eb0189c3 Mon Sep 17 00:00:00 2001 From: tachyon-r <291518778+tachyon-r@users.noreply.github.com> Date: Sun, 30 Aug 2026 13:22:43 -0400 Subject: [PATCH 009/173] test(security-guidance): verify declared hooks --- .../plugins/test_security_guidance_plugin.py | 23 ++++++++++++++++++- 1 file changed, 22 insertions(+), 1 deletion(-) diff --git a/tests/plugins/test_security_guidance_plugin.py b/tests/plugins/test_security_guidance_plugin.py index a00bafddcd..47d7dab640 100644 --- a/tests/plugins/test_security_guidance_plugin.py +++ b/tests/plugins/test_security_guidance_plugin.py @@ -262,13 +262,34 @@ class TestPreToolCallHook: # --------------------------------------------------------------------------- class TestPluginDiscovery: + def test_manifest_declares_registered_hooks(self): + """Manifest metadata must use the field consumed by plugin discovery.""" + import yaml + + plugin_dir = _repo_root() / "plugins" / "security-guidance" + manifest = yaml.safe_load( + (plugin_dir / "plugin.yaml").read_text(encoding="utf-8") + ) + mod = _load_plugin_init() + registered = [] + + class HookContext: + def register_hook(self, name, _callback): + registered.append(name) + + mod.register(HookContext()) + assert set(manifest["provides_hooks"]) == set(registered) + assert "hooks" not in manifest + def test_loads_via_plugin_manager(self, _isolate_env, monkeypatch): """End-to-end: enable in config.yaml and verify the PluginManager picks it up via the standard discovery path.""" import yaml config = {"plugins": {"enabled": ["security-guidance"]}} - (_isolate_env / "config.yaml").write_text(yaml.safe_dump(config)) + (_isolate_env / "config.yaml").write_text( + yaml.safe_dump(config), encoding="utf-8" + ) # Wipe any cached plugin state from earlier tests in this worker. for k in list(sys.modules): From 8ccbe5eb6af77a9ac7b1fd942ebf0634ba1e7cac Mon Sep 17 00:00:00 2001 From: fangliquanflq Date: Sat, 12 Sep 2026 00:07:08 +0800 Subject: [PATCH 010/173] fix(plugins): declare bundled plugin hooks --- plugins/disk-cleanup/plugin.yaml | 2 +- plugins/google_meet/plugin.yaml | 2 +- plugins/memory/byterover/plugin.yaml | 2 +- plugins/memory/hindsight/plugin.yaml | 2 +- plugins/memory/holographic/plugin.yaml | 2 +- plugins/memory/honcho/plugin.yaml | 2 +- plugins/memory/openviking/plugin.yaml | 2 +- plugins/observability/langfuse/plugin.yaml | 2 +- 8 files changed, 8 insertions(+), 8 deletions(-) diff --git a/plugins/disk-cleanup/plugin.yaml b/plugins/disk-cleanup/plugin.yaml index fe005c8849..75b163db7a 100644 --- a/plugins/disk-cleanup/plugin.yaml +++ b/plugins/disk-cleanup/plugin.yaml @@ -2,6 +2,6 @@ name: disk-cleanup version: 2.0.0 description: "Auto-track and clean up ephemeral files (test scripts, temp outputs, cron logs) created during Hermes sessions. Runs via plugin hooks — no agent action required." author: "@LVT382009 (original), NousResearch (plugin port)" -hooks: +provides_hooks: - post_tool_call - on_session_end diff --git a/plugins/google_meet/plugin.yaml b/plugins/google_meet/plugin.yaml index 519d6e09c8..0d6b2e0f35 100644 --- a/plugins/google_meet/plugin.yaml +++ b/plugins/google_meet/plugin.yaml @@ -12,5 +12,5 @@ provides_tools: - meet_status - meet_transcript - meet_say -hooks: +provides_hooks: - on_session_end diff --git a/plugins/memory/byterover/plugin.yaml b/plugins/memory/byterover/plugin.yaml index a6645c3c52..74dc06b0eb 100644 --- a/plugins/memory/byterover/plugin.yaml +++ b/plugins/memory/byterover/plugin.yaml @@ -5,5 +5,5 @@ external_dependencies: - name: brv install: "curl -fsSL https://byterover.dev/install.sh | sh" check: "brv --version" -hooks: +provides_hooks: - on_pre_compress diff --git a/plugins/memory/hindsight/plugin.yaml b/plugins/memory/hindsight/plugin.yaml index 9dfa763af7..126c8a9c38 100644 --- a/plugins/memory/hindsight/plugin.yaml +++ b/plugins/memory/hindsight/plugin.yaml @@ -4,5 +4,5 @@ description: "Hindsight — long-term memory with knowledge graph, entity resolu pip_dependencies: - "hindsight-client>=0.6.1" requires_env: [] -hooks: +provides_hooks: - on_session_end diff --git a/plugins/memory/holographic/plugin.yaml b/plugins/memory/holographic/plugin.yaml index ae7d78f8da..b30814fc91 100644 --- a/plugins/memory/holographic/plugin.yaml +++ b/plugins/memory/holographic/plugin.yaml @@ -1,5 +1,5 @@ name: holographic version: 0.1.0 description: "Holographic memory — local SQLite fact store with FTS5 search, trust scoring, and HRR-based compositional retrieval." -hooks: +provides_hooks: - on_session_end diff --git a/plugins/memory/honcho/plugin.yaml b/plugins/memory/honcho/plugin.yaml index 38a0612c97..4e76199b44 100644 --- a/plugins/memory/honcho/plugin.yaml +++ b/plugins/memory/honcho/plugin.yaml @@ -3,5 +3,5 @@ version: 1.0.0 description: "Honcho AI-native memory — cross-session user modeling with dialectic Q&A, semantic search, and persistent conclusions." pip_dependencies: - honcho-ai -hooks: +provides_hooks: - on_session_end diff --git a/plugins/memory/openviking/plugin.yaml b/plugins/memory/openviking/plugin.yaml index 18b8ea7874..6656830c47 100644 --- a/plugins/memory/openviking/plugin.yaml +++ b/plugins/memory/openviking/plugin.yaml @@ -4,5 +4,5 @@ description: "OpenViking context database — session-managed memory with automa pip_dependencies: - httpx requires_env: [] -hooks: +provides_hooks: - on_session_end diff --git a/plugins/observability/langfuse/plugin.yaml b/plugins/observability/langfuse/plugin.yaml index e1244e610a..9627e16e36 100644 --- a/plugins/observability/langfuse/plugin.yaml +++ b/plugins/observability/langfuse/plugin.yaml @@ -5,7 +5,7 @@ author: NousResearch requires_env: - HERMES_LANGFUSE_PUBLIC_KEY - HERMES_LANGFUSE_SECRET_KEY -hooks: +provides_hooks: - pre_api_request - post_api_request - api_request_error From 72ee40fa68079a7aba6aed02f03f12db81484d4a Mon Sep 17 00:00:00 2001 From: fangliquanflq Date: Sat, 12 Sep 2026 00:26:00 +0800 Subject: [PATCH 011/173] fix(plugins): remove memory lifecycle hook declarations --- plugins/memory/byterover/plugin.yaml | 2 -- plugins/memory/hindsight/plugin.yaml | 2 -- plugins/memory/holographic/plugin.yaml | 2 -- plugins/memory/honcho/plugin.yaml | 2 -- plugins/memory/openviking/plugin.yaml | 2 -- 5 files changed, 10 deletions(-) diff --git a/plugins/memory/byterover/plugin.yaml b/plugins/memory/byterover/plugin.yaml index 74dc06b0eb..bdfc1045f1 100644 --- a/plugins/memory/byterover/plugin.yaml +++ b/plugins/memory/byterover/plugin.yaml @@ -5,5 +5,3 @@ external_dependencies: - name: brv install: "curl -fsSL https://byterover.dev/install.sh | sh" check: "brv --version" -provides_hooks: - - on_pre_compress diff --git a/plugins/memory/hindsight/plugin.yaml b/plugins/memory/hindsight/plugin.yaml index 126c8a9c38..3495aa63cc 100644 --- a/plugins/memory/hindsight/plugin.yaml +++ b/plugins/memory/hindsight/plugin.yaml @@ -4,5 +4,3 @@ description: "Hindsight — long-term memory with knowledge graph, entity resolu pip_dependencies: - "hindsight-client>=0.6.1" requires_env: [] -provides_hooks: - - on_session_end diff --git a/plugins/memory/holographic/plugin.yaml b/plugins/memory/holographic/plugin.yaml index b30814fc91..497cc2e903 100644 --- a/plugins/memory/holographic/plugin.yaml +++ b/plugins/memory/holographic/plugin.yaml @@ -1,5 +1,3 @@ name: holographic version: 0.1.0 description: "Holographic memory — local SQLite fact store with FTS5 search, trust scoring, and HRR-based compositional retrieval." -provides_hooks: - - on_session_end diff --git a/plugins/memory/honcho/plugin.yaml b/plugins/memory/honcho/plugin.yaml index 4e76199b44..8717aa2a39 100644 --- a/plugins/memory/honcho/plugin.yaml +++ b/plugins/memory/honcho/plugin.yaml @@ -3,5 +3,3 @@ version: 1.0.0 description: "Honcho AI-native memory — cross-session user modeling with dialectic Q&A, semantic search, and persistent conclusions." pip_dependencies: - honcho-ai -provides_hooks: - - on_session_end diff --git a/plugins/memory/openviking/plugin.yaml b/plugins/memory/openviking/plugin.yaml index 6656830c47..faa1b69074 100644 --- a/plugins/memory/openviking/plugin.yaml +++ b/plugins/memory/openviking/plugin.yaml @@ -4,5 +4,3 @@ description: "OpenViking context database — session-managed memory with automa pip_dependencies: - httpx requires_env: [] -provides_hooks: - - on_session_end From 95481bb54d00551fc6452efedfedf5b757a0be58 Mon Sep 17 00:00:00 2001 From: teknium1 <127238744+teknium1@users.noreply.github.com> Date: Mon, 21 Sep 2026 22:52:10 -0700 Subject: [PATCH 012/173] fix(plugins): LAZY_DEPS mirror plugin.yaml ranges; manifest parser names list-typed files and reads hooks: - hindsight-client==0.6.1 / mem0ai==2.0.10 exact pins made _is_satisfied() reject every newer compatible release, so hermes update kept downgrading a working client and broke embedded daemons whose DB a newer client had migrated (#86992, #39424, #98407, #99317). The lazy entries now mirror the plugin manifests (>=0.6.1,<1 and >=2.0.10,<3); pyproject extras stay the floor install. Slim redo of #99557 (nateEc) / #98416 / #98527 (ttomiczek) / #39754. - A list-typed plugin.yaml is refused with "top level must be a mapping" instead of an AttributeError swallowed as "Failed to parse" (#14066, discovery side). - ``hooks:`` (the spelling bundled manifests carried) still populates provides_hooks (#108371). --- hermes_cli/plugins_manifest.py | 8 +- pyproject.toml | 10 ++- tests/hermes_cli/test_plugin_manifest_v2.py | 99 +++++++++++++++++++++ tests/tools/test_lazy_deps.py | 35 ++++++++ tools/lazy_deps.py | 8 +- 5 files changed, 153 insertions(+), 7 deletions(-) diff --git a/hermes_cli/plugins_manifest.py b/hermes_cli/plugins_manifest.py index 1fd45dc037..e81fe50822 100644 --- a/hermes_cli/plugins_manifest.py +++ b/hermes_cli/plugins_manifest.py @@ -477,6 +477,10 @@ def parse_manifest_file( logger.warning("PyYAML not installed – cannot load %s", manifest_file) return None data = fast_safe_load(manifest_file.read_text(encoding="utf-8")) or {} + if not isinstance(data, Mapping): + logger.warning("Failed to parse %s: top level must be a mapping, got %s (#14066)", + manifest_file, type(data).__name__) + return None name = data.get("name", plugin_dir.name) key = f"{prefix}/{plugin_dir.name}" if prefix else name kind = _manifest_kind(data, key, plugin_dir) @@ -487,7 +491,9 @@ def parse_manifest_file( description=data.get("description", ""), author=_display_author(data.get("author", "")), requires_env=data.get("requires_env", []), provides_tools=data.get("provides_tools", []), - provides_hooks=data.get("provides_hooks", []), source=source, path=str(plugin_dir), + # ``hooks:`` is the spelling the bundled manifests carried for months; external copies of it + # must keep declaring the same thing (#108371). + provides_hooks=data.get("provides_hooks", data.get("hooks", [])), source=source, path=str(plugin_dir), kind=kind, key=key, requires_hermes=str(data.get("requires_hermes") or "").strip(), capabilities=_parse_declared_capabilities(data.get("capabilities"), name), **_parse_manifest_v2_fields(data, key), emits=data.get("emits") or [], diff --git a/pyproject.toml b/pyproject.toml index e366b8459d..e38e5a8349 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -258,10 +258,12 @@ wake = [ ] honcho = ["honcho-ai==2.2.0"] # Cloud memory providers — opt-in, lazy-installed via tools/lazy_deps.py -# (memory.supermemory / memory.mem0) at first use. Exact pins MUST match the -# LAZY_DEPS pins (enforced by tests/test_project_metadata.py). Deliberately -# excluded from [all] like honcho/hindsight so a quarantined upstream release -# can't break fresh installs. +# (memory.supermemory / memory.mem0) at first use. Where BOTH pin exactly the +# versions MUST match (enforced by tests/test_project_metadata.py); LAZY_DEPS +# entries that mirror a plugin.yaml range (mem0ai, hindsight-client) keep the +# extra as the floor install and never downgrade a newer compatible release. +# Deliberately excluded from [all] like honcho/hindsight so a quarantined +# upstream release can't break fresh installs. supermemory = ["supermemory==3.50.0"] mem0 = ["mem0ai==2.0.10"] # Image resize recovery for the vision tools. Pillow is now a CORE dependency diff --git a/tests/hermes_cli/test_plugin_manifest_v2.py b/tests/hermes_cli/test_plugin_manifest_v2.py index d31385dbaf..73a64f7a9e 100644 --- a/tests/hermes_cli/test_plugin_manifest_v2.py +++ b/tests/hermes_cli/test_plugin_manifest_v2.py @@ -412,6 +412,27 @@ class TestCtxHasPlugin: class TestRequiresHermes: + def test_gate_reads_the_running_code_version_not_dist_metadata(self, monkeypatch): + """An editable install's dist metadata is frozen at install time (0.21.0 here) while the checkout runs + 0.21.4; the gate must compare against the code that is running.""" + import importlib.metadata + from hermes_cli import plugins_manifest + monkeypatch.setattr(importlib.metadata, "version", lambda name: "0.21.0") + monkeypatch.setattr("hermes_cli.__version__", "0.21.4") + assert plugins_manifest.running_hermes_version() == "0.21.4" + assert plugins_manifest.version_satisfies(">=0.21.4", plugins_manifest.running_hermes_version()) + + @pytest.mark.parametrize("spec, current, expected", [ + (">=99.0.0rc1", "0.21.4", False), # rc target used to parse as None -> clause silently dropped + (">=0.23.0", "0.22.0rc1", False), # rc running version used to disable every gate + (">=0.21.0", "0.22.0rc1", True), + (">=1.2.3.post1", "1.2.3", True), + ("banana", "0.21.4", True), # documented: unparseable target stays permissive + ]) + def test_prerelease_spellings_gate(self, spec, current, expected): + from hermes_cli.plugins_manifest import version_satisfies + assert version_satisfies(spec, current) is expected + def test_unsatisfied_requires_hermes_skips_without_importing(self, hermes_home, monkeypatch): """A too-new ``requires_hermes`` records an error and never runs register(); a satisfied one loads.""" import sys @@ -434,6 +455,84 @@ class TestRequiresHermes: delattr(sys, attr) +class TestLoadIsolation: + def test_sys_exit_in_plugin_is_isolated_and_named(self, hermes_home, caplog): + """A plugin calling ``sys.exit()`` at import used to propagate SystemExit out of discovery: the whole + registry emptied, ``_discovered`` reset and ``hermes chat`` exited 3 with no output. It must be + recorded as that plugin's error while later plugins still load.""" + _write_plugin(hermes_home / "plugins", "b_exit") + (hermes_home / "plugins" / "b_exit" / "__init__.py").write_text("import sys\nsys.exit(0)\n") + _write_plugin(hermes_home / "plugins", "c_after") + _enable(hermes_home, ["b_exit", "c_after"]) + mgr = PluginManager() + with caplog.at_level(logging.WARNING, logger="hermes_cli.plugins"): + mgr.discover_and_load() # must not raise + assert mgr._discovered is True + assert mgr._plugins["c_after"].enabled + assert not mgr._plugins["b_exit"].enabled + assert "SystemExit(0)" in (mgr._plugins["b_exit"].error or "") + + def test_keyboard_interrupt_still_propagates(self, hermes_home): + _write_plugin(hermes_home / "plugins", "ctrlc") + (hermes_home / "plugins" / "ctrlc" / "__init__.py").write_text("raise KeyboardInterrupt\n") + _enable(hermes_home, ["ctrlc"]) + with pytest.raises(KeyboardInterrupt): + PluginManager().discover_and_load() + + +class TestBundledKeyShadowing: + def test_impostor_dir_cannot_claim_a_bundled_key(self, tmp_path, monkeypatch, caplog): + """``~/.hermes/plugins/impostor_dir/plugin.yaml`` with ``name: `` used to displace the + bundled plugin silently, so ``hermes plugins enable `` enabled unrelated code. The bundled + manifest wins and the impostor is warned about; a same-named user copy still overrides (documented).""" + home = tmp_path / "home" + (home / "plugins").mkdir(parents=True) + bundled = tmp_path / "bundled" + monkeypatch.setenv("HERMES_HOME", str(home)) + monkeypatch.setenv("HERMES_ENABLE_PROJECT_PLUGINS", "0") + monkeypatch.setenv("HERMES_BUNDLED_PLUGINS", str(bundled)) + _write_plugin(bundled, "genuine", register_body="import sys; sys._shadow_probe = 'bundled'") + _write_plugin(bundled, "overridable", register_body="import sys; sys._override_probe = 'bundled'") + _write_plugin(home / "plugins", "impostor_dir", manifest_extra={"name": "genuine"}, + register_body="import sys; sys._shadow_probe = 'impostor'") + (home / "plugins" / "impostor_dir" / "plugin.yaml").write_text( + yaml.dump({"name": "genuine", "version": "0.1.0", "description": "impostor"})) + _write_plugin(home / "plugins", "overridable", register_body="import sys; sys._override_probe = 'user'") + _enable(home, ["genuine", "overridable"]) + import sys + try: + with caplog.at_level(logging.INFO, logger="hermes_cli.plugins"): + mgr = PluginManager() + mgr.discover_and_load() + assert mgr._plugins["genuine"].manifest.source == "bundled" + assert sys._shadow_probe == "bundled" + assert "impostor_dir" in caplog.text and "rename the directory" in caplog.text + assert mgr._plugins["overridable"].manifest.source == "user" + assert sys._override_probe == "user" + assert "shadows the bundled copy" in caplog.text + finally: + for attr in ("_shadow_probe", "_override_probe"): + if hasattr(sys, attr): + delattr(sys, attr) + + +class TestManifestParsingRobustness: + def test_list_manifest_is_rejected_with_a_clear_reason_and_hooks_alias(self, hermes_home, caplog): + """A list-typed plugin.yaml (#14066) names the actual problem instead of an AttributeError; the + long-standing ``hooks:`` spelling still populates ``provides_hooks`` (#108371).""" + from hermes_cli.plugins_discovery import scan_directory + bad = hermes_home / "plugins" / "listy" + bad.mkdir() + (bad / "plugin.yaml").write_text("- name: listy\n") + good = _write_plugin(hermes_home / "plugins", "hooky", manifest_extra={"hooks": ["pre_tool_call"]}) + with caplog.at_level(logging.WARNING, logger="hermes_cli.plugins"): + manifests = {m.name: m for m in scan_directory(hermes_home / "plugins", "user")} + assert "listy" not in manifests + assert "top level must be a mapping" in caplog.text + assert manifests["hooky"].provides_hooks == ["pre_tool_call"] + assert manifests["hooky"].path == str(good) + + class TestDirectoryPluginKeepsIdentityOverEntryPoint: """A pyproject-wrapper plugin depends on a pip package that ships a ``hermes_agent.plugins`` entry point under the SAME name. The installed directory must stay the plugin's identity (it diff --git a/tests/tools/test_lazy_deps.py b/tests/tools/test_lazy_deps.py index 5adf2ba8c3..32a082636c 100644 --- a/tests/tools/test_lazy_deps.py +++ b/tests/tools/test_lazy_deps.py @@ -223,6 +223,13 @@ class TestIsSatisfiedVersionAware: self._fake_version(monkeypatch, {"mautrix": "0.20.0"}) assert ld._is_satisfied("mautrix[encryption]==0.21.0") is False + def test_plugin_owned_sdk_newer_compatible_release_is_satisfied(self, monkeypatch): + """A newer release inside the plugin.yaml range must not be re-pinned downward on refresh + (#86992 hindsight-client 0.9.x -> 0.6.1, #98407 mem0ai 2.0.19 -> 2.0.10).""" + self._fake_version(monkeypatch, {"hindsight-client": "0.9.2", "mem0ai": "2.0.19"}) + assert ld.feature_missing("memory.hindsight") == () + assert ld.feature_missing("memory.mem0") == () + def test_trace_upload_hub_at_core_locked_version_is_current(self, monkeypatch): """#60783 regression: refresh must not churn the shared hub install. @@ -426,6 +433,34 @@ class TestRefreshActiveFeatures: class TestInstallSpecs: + def test_uv_tier_runs_from_the_checkout_so_exclude_newer_applies(self, monkeypatch, tmp_path): + """uv reads ``[tool.uv] exclude-newer`` from the cwd project only; a plugin-dep install launched from + $HOME or a gateway service must still run under the checkout's quarantine, so the uv invocation + carries the checkout root as cwd (#L1-3 of the 2026-09 plugin audit).""" + import subprocess + from pathlib import Path + + calls = [] + + def fake_run(cmd, **kw): + calls.append((cmd, kw)) + return subprocess.CompletedProcess(cmd, 0, stdout="", stderr="") + + monkeypatch.setattr(ld, "_run_installer", fake_run) + monkeypatch.setattr(ld, "_uv_binary", lambda: "/fake/uv") + monkeypatch.setattr(ld, "_lazy_install_target", lambda: None) + monkeypatch.setattr(ld, "_after_successful_install", lambda *a, **kw: None) + monkeypatch.chdir(tmp_path) + project_root = Path(ld.__file__).resolve().parent.parent + assert (project_root / "pyproject.toml").is_file() + + result = ld._venv_pip_install(("requests==2.32.0",)) + + assert result.success + (cmd, kw), = calls + assert cmd[:3] == ["/fake/uv", "pip", "install"] + assert kw.get("cwd") == str(project_root) + def test_empty_specs_is_trivially_ok(self, monkeypatch): monkeypatch.setattr( ld, "_venv_pip_install", diff --git a/tools/lazy_deps.py b/tools/lazy_deps.py index c9b4e16c02..7e33d431ac 100644 --- a/tools/lazy_deps.py +++ b/tools/lazy_deps.py @@ -107,11 +107,15 @@ LAZY_DEPS: dict[str, tuple[str, ...]] = { # ─── Memory providers ────────────────────────────────────────────────── "memory.honcho": ("honcho-ai==2.2.0",), - "memory.hindsight": ("hindsight-client==0.6.1",), + # Plugin-owned SDKs mirror the range their plugin.yaml declares instead of an exact pin: an exact pin + # made _is_satisfied() reject every newer compatible release, so `hermes update` (and the hindsight + # plugin's own >=_MIN_CLIENT_VERSION auto-upgrade) kept downgrading a working 0.9.x client to 0.6.1 + # and broke embedded daemons whose DB a newer client had migrated (#86992, #39424, #98407). + "memory.hindsight": ("hindsight-client>=0.6.1,<1",), # Cloud memory SDKs MUST be allowlisted + ensure()'d at the import site, or they never # install on the sealed Docker image (durable-target only). "memory.supermemory": ("supermemory==3.50.0",), - "memory.mem0": ("mem0ai==2.0.10",), + "memory.mem0": ("mem0ai>=2.0.10,<3",), # ─── Messaging platforms (lazy-installable on demand) ────────────────── "platform.telegram": ("python-telegram-bot[webhooks]==22.8",), From 569893f8cdc310d82ce688dc1b8a76e6f6475082 Mon Sep 17 00:00:00 2001 From: byjaps Date: Sat, 19 Sep 2026 22:38:02 +0100 Subject: [PATCH 013/173] fix(cli): don't flag plugin toolsets as unknown at startup Plugins register their toolsets during background discovery, but the CLI validates the configured toolset list while the instance is being constructed -- i.e. before that thread has landed. Judging by the live registry alone therefore reports every configured plugin toolset as a typo on every launch. The warning is not merely cosmetic: it is written to the console, so one-shot and quiet runs (-q / -Q, --format stream-json) hand it to whatever parses their output; an integration reading the response can receive the warning line instead of the answer. Skip names the plugin registry knows about, mirroring how MCP server names are already skipped here. Names persisted by the previous launch's discovery sweep are served by get_plugin_toolset_keys_nowait, so this stays non-blocking on startup. A genuinely unknown name still warns. Refs #71650 (#95529 is the duplicate report of the same bug) Co-authored-by: adamkrawczyk --- .../emails/byjaps@users.noreply.github.com | 2 + hermes_cli/cli_init_mixin.py | 11 ++++- tests/hermes_cli/test_cli_init.py | 48 +++++++++++++++++++ 3 files changed, 60 insertions(+), 1 deletion(-) create mode 100644 contributors/emails/byjaps@users.noreply.github.com diff --git a/contributors/emails/byjaps@users.noreply.github.com b/contributors/emails/byjaps@users.noreply.github.com new file mode 100644 index 0000000000..b2b0e65491 --- /dev/null +++ b/contributors/emails/byjaps@users.noreply.github.com @@ -0,0 +1,2 @@ +byjaps +# PR #116425 fix(cli) plugin toolsets must not be flagged as unknown at startup diff --git a/hermes_cli/cli_init_mixin.py b/hermes_cli/cli_init_mixin.py index 772d9bbbac..adb51f2130 100644 --- a/hermes_cli/cli_init_mixin.py +++ b/hermes_cli/cli_init_mixin.py @@ -214,7 +214,16 @@ class CLIInitMixin: if toolsets and "all" not in toolsets and "*" not in toolsets: # MCP server names only resolve after discover_mcp_tools runs; skip them here. mcp_names = set((CLI_CONFIG.get("mcp_servers") or {}).keys()) - invalid = [t for t in toolsets if not validate_toolset(t) and t not in mcp_names] + # Plugin toolsets register during plugin discovery, which startup runs on a background thread + # that has not necessarily landed yet; names it declared (or the previous launch persisted, which + # get_plugin_toolset_keys_nowait serves) are not typos (#71650). + try: + from hermes_cli.plugins import get_plugin_toolset_keys_nowait + plugin_ts_names = get_plugin_toolset_keys_nowait() + except Exception: + plugin_ts_names = set() + invalid = [t for t in toolsets + if not validate_toolset(t) and t not in mcp_names and t not in plugin_ts_names] if invalid: self._console_print(f"[bold red]Warning: Unknown toolsets: {', '.join(invalid)}[/]") diff --git a/tests/hermes_cli/test_cli_init.py b/tests/hermes_cli/test_cli_init.py index 7bcb49b163..83ba5d041b 100644 --- a/tests/hermes_cli/test_cli_init.py +++ b/tests/hermes_cli/test_cli_init.py @@ -763,4 +763,52 @@ class TestRootLevelProviderOverride: assert result["model"]["provider"] == "auto" +class TestPluginToolsetStartupValidation: + """A toolset that is merely *not registered yet* must not be reported as unknown. + + Plugins register their toolsets during background discovery, while the CLI validates + the configured list during construction -- i.e. before that thread has landed. Judging + by the live registry alone therefore flags every configured plugin toolset as a typo on + every launch, including one-shot/quiet runs whose stdout is machine-parsed. + """ + + @staticmethod + def _init_toolsets(monkeypatch, toolsets, *, registry, plugin_keys): + import cli as _cli_mod + + stub = object.__new__(_cli_mod.HermesCLI) + printed: list[str] = [] + stub._console_print = printed.append + monkeypatch.setattr(_cli_mod, "validate_toolset", lambda name: name in registry) + monkeypatch.setattr(_cli_mod, "CLI_CONFIG", {"agent": {}}) + monkeypatch.setattr( + "hermes_cli.plugins.get_plugin_toolset_keys_nowait", + lambda: set(plugin_keys), + ) + stub._init_toolsets(list(toolsets)) + return stub, printed + + def test_plugin_toolset_not_yet_registered_is_not_flagged(self, monkeypatch): + stub, printed = self._init_toolsets( + monkeypatch, + ["terminal", "voice_stack"], + registry={"terminal"}, + plugin_keys={"voice_stack"}, + ) + assert printed == [] + # The configured list is kept verbatim; only the false warning is silenced. + assert stub.enabled_toolsets == ["terminal", "voice_stack"] + + def test_real_typo_still_warns(self, monkeypatch): + _, printed = self._init_toolsets( + monkeypatch, + ["terminal", "voice_stak"], + registry={"terminal"}, + plugin_keys={"voice_stack"}, + ) + assert len(printed) == 1 + assert "voice_stak" in printed[0] + assert "voice_stack" not in printed[0] + + From 74f726c17675642c2eff3033632e812f5f0e6614 Mon Sep 17 00:00:00 2001 From: teknium1 <127238744+teknium1@users.noreply.github.com> Date: Mon, 21 Sep 2026 23:12:07 -0700 Subject: [PATCH 014/173] test(plugins): langfuse manifest test reads provides_hooks (renamed from hooks:, #108371) --- tests/plugins/test_langfuse_plugin.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/plugins/test_langfuse_plugin.py b/tests/plugins/test_langfuse_plugin.py index 3887491d0a..4e5d58cf83 100644 --- a/tests/plugins/test_langfuse_plugin.py +++ b/tests/plugins/test_langfuse_plugin.py @@ -27,8 +27,8 @@ class TestManifest: data = yaml.safe_load((PLUGIN_DIR / "plugin.yaml").read_text()) assert data["name"] == "langfuse" assert data["version"] - # All eleven hooks the plugin implements. - assert set(data["hooks"]) == { + # All eleven hooks the plugin implements, declared under the field discovery/validate read (#108371). + assert set(data["provides_hooks"]) == { "pre_api_request", "post_api_request", "api_request_error", "pre_llm_call", "post_llm_call", "pre_tool_call", "post_tool_call", From 9a2d96ca1109f92572dc20507af624065985ed86 Mon Sep 17 00:00:00 2001 From: teknium1 <127238744+teknium1@users.noreply.github.com> Date: Mon, 21 Sep 2026 22:34:00 -0700 Subject: [PATCH 015/173] fix(config): flag quoted list/mapping values in doctor + startup, guard the list slots config set missed MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A list/mapping slot holding ONE quoted string (`plugins:\n enabled: '["a","b"]'`, `model_catalog:\n excluded_providers: '["openai-api"]'`) is skipped by every isinstance-gated reader (`plugins_cmd._config_name_set` -> set(), `plugins._names`, `inventory.excluded_providers` -> []) while `config get` echoes it back, so user plugins silently unmount and provider exclusions silently lapse. Neither `hermes doctor` nor the startup `print_config_warnings` banner said a word. Reader/doctor half: one schema-aware pass in `validate_config_structure` (`_validate_quoted_containers`) walks `DEFAULT_CONFIG` (sections included) plus `_KNOWN_CONTAINER_TYPES` and warns when the user value is a string that parses to a list/mapping. The message names the key, the quoted value and the remedy (`hermes config set ''`). Finding only: the file is never rewritten. String-typed keys (`approvals.mode: "[off]"`), the `model: ` shorthand and the `parse_config_string_list`-read slots (`agent.disabled_toolsets`, `skills.disabled`) are not flagged. Feeds both the doctor "Config Structure" section and the startup banner. Writer half (audit of every path that can put a string in a container slot): - `hermes config set` (`hermes_cli/config.py::set_config_value`): already parses bracket/brace literals (#88163) and refuses wrong-shaped values (`_refuse_container_type_mismatch`), BUT the guard only knew slots present in DEFAULT_CONFIG or `_KNOWN_CONTAINER_TYPES`. `plugins.enabled`, `plugins.disabled` and `model_catalog.excluded_providers` are deliberately absent from DEFAULT_CONFIG, so `config set plugins.enabled foo` / `plugins.enabled a,b` / `model_catalog.excluded_providers openai-api` stored a plain string (live repro on base). Added the three keys to `_KNOWN_CONTAINER_TYPES`: those writes are now refused with the literal hint. - `cli.py::save_config_value` + callers (cli_*_mixin, gateway/slash_commands, gateway/run_busy): every caller passes a bool/enum string for scalar keys; no list-slot caller. Not reachable. - `hermes_cli/plugins_cmd.py::_save_plugin_sets` / `_write_config_value` and `plugins_cmd_catalog` (via `_save_enabled_set`): write `sorted(set)` — real lists. Not reachable. - Dashboard `PUT /api/config` (`web_routers/config_env.py::update_config` -> `_denormalize_config_from_web` -> `save_config`): schema-driven form; `web/src/components/AutoField.tsx` splits list-typed fields into a real array before the PUT. Not reachable. - tui_gateway `config.set`: fixed `_CONFIG_SETTERS` table of scalar keys only (out of this lane's files anyway). Not reachable. Conclusion: the quoted shapes on real machines are leftovers of pre-#88163 `config set` runs plus the DEFAULT_CONFIG-absent slots fixed here. Live repro (fake HOME/HERMES_HOME, both quoted shapes in config.yaml): before: validate_config_structure() -> []; startup banner silent; doctor has no Config Structure section; _config_name_set('plugins','enabled') -> set() after: two warnings naming plugins.enabled / model_catalog.excluded_providers with `hermes config set ... '["a","b"]'`; doctor prints them under Config Structure; running the remedy yields real lists, validate -> [], _config_name_set -> {'a','b'} writer: `config set plugins.enabled a,b` wrote `enabled: a,b` on base; now refused ("must be a list ... nothing was written"). Fixes #83308 Fixes #105706 credit: @fangliquanflq #105725 (schema-aware detection in validate_config_structure; slim redo) credit: @Luna161 #83313 (first report + per-key warning in plugins_cmd) --- hermes_cli/config.py | 49 +++++++++++++++++++ .../hermes_cli/test_config_set_list_values.py | 13 +++++ tests/hermes_cli/test_config_validation.py | 24 +++++++++ website/docs/reference/cli-commands.md | 4 +- 4 files changed, 89 insertions(+), 1 deletion(-) diff --git a/hermes_cli/config.py b/hermes_cli/config.py index f2f0a17bb2..02858f0ca2 100644 --- a/hermes_cli/config.py +++ b/hermes_cli/config.py @@ -16,6 +16,7 @@ import logging import os import platform import re +import shlex import shutil import stat import subprocess @@ -1217,6 +1218,48 @@ def _validate_web_backends(config: Dict[str, Any], issues: List[ConfigIssue]) -> "Run 'hermes tools' and pick a different Web Search & Extract provider") +def _container_slots() -> Dict[str, str]: + """Dotted key -> ``"list"``/``"mapping"`` for every slot the schema fixes to a container: + ``DEFAULT_CONFIG`` (sections included) plus the known-container table for roots it omits.""" + slots: Dict[str, str] = {} + + def walk(node: Dict[str, Any], prefix: str) -> None: + for key, value in node.items(): + path = f"{prefix}.{key}" if prefix else key + if isinstance(value, dict): + slots[path] = "mapping" + walk(value, path) + elif isinstance(value, list): + slots[path] = "list" + + walk(DEFAULT_CONFIG, "") + slots.update(_KNOWN_CONTAINER_TYPES) + return slots + + +def _validate_quoted_containers(config: Dict[str, Any], issues: List[ConfigIssue]) -> None: + """A container slot holding ONE quoted string (``enabled: '["a","b"]'``) is skipped by every + isinstance-gated reader while ``config get`` echoes it back, so plugins silently unmount and + exclusions silently lapse (#83308, #105706). Finding only — the file is never rewritten.""" + for key, kind in _container_slots().items(): + # ``parse_config_string_list`` readers accept the quoted form; nothing is ignored there. + if key in _SCALAR_AS_ONE_ITEM_LIST_KEYS: + continue + value = cfg_get(config, *key.split(".")) + if not isinstance(value, str) or not _looks_structured_value(value): + continue + try: + parsed = yaml.safe_load(value) + except yaml.YAMLError: + continue + if isinstance(parsed, (list, dict)): + _issue(issues, "warning", + f"{key} is the quoted string {value!r} — Hermes expects a YAML {kind} here " + "and every reader ignores the string", + f"Run: hermes config set {key} {shlex.quote(value)} (stores a real {kind}), " + "or remove the quotes in config.yaml") + + def validate_config_structure(config: Optional[Dict[str, Any]] = None) -> List["ConfigIssue"]: """Validate config.yaml structure and return detected issues (accepts a pre-loaded dict). Catches common YAML mistakes that otherwise surface as confusing runtime errors.""" @@ -1256,6 +1299,7 @@ def validate_config_structure(config: Optional[Dict[str, Any]] = None) -> List[" f"Move '{key}' under the appropriate section") _validate_web_backends(config, issues) + _validate_quoted_containers(config, issues) return issues @@ -3254,6 +3298,11 @@ _KNOWN_CONTAINER_TYPES = { "providers": "mapping", "model.aliases": "mapping", "model_aliases": "mapping", + # Omitted from DEFAULT_CONFIG on purpose (an empty default would clobber a user allow-list), + # so without these rows `config set plugins.enabled foo` stored a string every reader ignored. + "plugins.enabled": "list", + "plugins.disabled": "list", + "model_catalog.excluded_providers": "list", } # List slots whose readers go through ``parse_config_string_list``: a bare name is one entry. _SCALAR_AS_ONE_ITEM_LIST_KEYS = frozenset({"agent.disabled_toolsets", "skills.disabled"}) diff --git a/tests/hermes_cli/test_config_set_list_values.py b/tests/hermes_cli/test_config_set_list_values.py index 2ee932618c..3c81e23421 100644 --- a/tests/hermes_cli/test_config_set_list_values.py +++ b/tests/hermes_cli/test_config_set_list_values.py @@ -161,3 +161,16 @@ def test_round_trip_through_load_config(user_home): set_config_value("platform_toolsets.line", '["clarify", "file", "web"]') cfg = load_config() assert cfg["platform_toolsets"]["line"] == ["clarify", "file", "web"] + + +def test_bare_string_into_list_slot_absent_from_defaults_is_refused(user_home, capsys): + """`plugins.enabled` / `model_catalog.excluded_providers` are omitted from DEFAULT_CONFIG, so the + container guard did not know them and `config set plugins.enabled a,b` stored a string every + isinstance(list) reader ignored (#83308, #105706).""" + from hermes_cli.config import set_config_value, read_raw_config + + for key in ("plugins.enabled", "model_catalog.excluded_providers"): + with pytest.raises(SystemExit): + set_config_value(key, "a,b") + assert "must be a list" in capsys.readouterr().err + assert read_raw_config() in (None, {}) diff --git a/tests/hermes_cli/test_config_validation.py b/tests/hermes_cli/test_config_validation.py index 9e6081566f..cabf72cd8d 100644 --- a/tests/hermes_cli/test_config_validation.py +++ b/tests/hermes_cli/test_config_validation.py @@ -193,3 +193,27 @@ class TestUnknownTopLevelKeys: assert any("base_url" in i.message for i in misplaced) assert any("api_key" in i.message for i in misplaced) + + +class TestQuotedContainerValues: + """A list/mapping slot holding one quoted string is ignored by every reader (#83308, #105706).""" + + def test_quoted_list_in_container_slot_is_flagged_with_remedy(self): + issues = validate_config_structure({ + "plugins": {"enabled": '["a","b"]'}, + "model_catalog": {"excluded_providers": '["openai-api"]'}, + }) + flagged = {i.message.split(" ", 1)[0]: i for i in issues if "quoted string" in i.message} + assert set(flagged) == {"plugins.enabled", "model_catalog.excluded_providers"} + assert "hermes config set plugins.enabled '[\"a\",\"b\"]'" in flagged["plugins.enabled"].hint + + def test_string_typed_and_tolerant_slots_are_not_flagged(self): + """`approvals.mode` is a string in the schema; `model: name` is the documented shorthand; + `agent.disabled_toolsets` readers parse the quoted form themselves.""" + issues = validate_config_structure({ + "approvals": {"mode": "[off]"}, + "model": "gpt-4o", + "agent": {"disabled_toolsets": '["web"]'}, + "plugins": {"enabled": ["a"]}, + }) + assert not [i for i in issues if "quoted string" in i.message] diff --git a/website/docs/reference/cli-commands.md b/website/docs/reference/cli-commands.md index 48fe123aae..1f01c6c217 100644 --- a/website/docs/reference/cli-commands.md +++ b/website/docs/reference/cli-commands.md @@ -978,6 +978,8 @@ Custom-endpoint config checks (both warn-only; `--fix` does not rewrite them): - `custom_providers` that is not a YAML list (for example a string left by a bad `config set`) is reported as an error naming the key and the received type — the runtime ignores every custom endpoint until it is a list again. - A legacy `custom_providers` list entry with no matching `providers:` entry (same endpoint URL) is reported with the move to make: such an entry is still served from the retired list store (the model picker and the Custom Endpoints page dual-read it) rather than the `providers:` map every other surface edits, and the one-shot v12 migration that moved the list into `providers:` does not run again. +**Config Structure** also flags any list/mapping setting stored as one quoted string (`plugins.enabled: '["a","b"]'`, `model_catalog.excluded_providers: '["openai-api"]'` — the shape older `config set` versions wrote): every reader ignores such a string, so the plugins silently stay unmounted and the exclusion never applies. The finding names the key and the `hermes config set ''` command that stores a real list; the same warning appears in the startup banner. `--fix` does not rewrite the file. + ## `hermes dump` ```bash @@ -1324,7 +1326,7 @@ Subcommands: | `show` | Show current config values. | | `edit` | Open `config.yaml` in your editor. | | `get [--json] [--raw]` | Print a single config value by dotted key (e.g. `hermes config get model.default`). `--json` emits machine-readable output. Credential-shaped values (`api_key`, `*_TOKEN`, `*_SECRET`, `password`, …) are masked (`sk-o...7890`) because the agent runs this from sessions whose transcripts persist; `--raw` prints the real value (or set `security.redact_secrets: false`). A nested key under a known section that the schema does not define (`compression.compressor.enabled`) still prints its file value, plus a stderr notice that Hermes may not read it; stdout and the exit code (0) are unchanged. | -| `set [--force]` | Set a config value. Dotted paths go to `config.yaml`; every `UPPER_SNAKE` name (`OPENROUTER_API_KEY`, `DISCORD_HOME_CHANNEL`, `TELEGRAM_GROUP_ALLOWED_USERS`, `HERMES_TIMEZONE`, …) is an environment variable and goes to `.env` — the same file the platform setup flows and `/sethome` write, and the one every runtime reader resolves against. `config set` never writes an `UPPER_SNAKE` key into `config.yaml`, `--force` included; names on the env writer's denylist (`HERMES_YOLO_MODE`, `PATH`, …) are refused outright; any other `UPPER_SNAKE` name is saved to `.env` as-is (plugins, skills and external tools read it from the process environment). A known key written under the wrong prefix (`gateway.discord.foo`, where `discord.foo` is itself a known key) is refused with a did-you-mean and nothing is written; any other unknown path under a known section (`agent.max_turnz`, or a runtime-read key that has no seeded default) is written with a did-you-mean notice, and an unknown lowercase *top-level* key is written with a notice (top-level scalars are bridged into the environment for skills). `--force` writes the refused wrong-prefix path too. Values are type-checked against the schema: a key that must hold a list or a mapping (`custom_providers`, `model.aliases`, `display.platforms`, any key already holding one) refuses a plain string or a wrong-shaped literal, and a value that looks like a list/mapping but is not valid YAML/JSON is refused instead of being stored as a string — nothing is written and the error names the expected type. Pass a YAML/JSON literal (`hermes config set custom_providers '[{name: x, base_url: https://...}]'`); to store a string that merely starts with `[` or `{`, quote it in YAML (`"'[text'"`). `--force` still replaces a whole mapping section; a non-list in a list slot has no override, except that a bare name for a list of names read leniently (`agent.disabled_toolsets`, `skills.disabled`) is stored as a one-item list. | +| `set [--force]` | Set a config value. Dotted paths go to `config.yaml`; every `UPPER_SNAKE` name (`OPENROUTER_API_KEY`, `DISCORD_HOME_CHANNEL`, `TELEGRAM_GROUP_ALLOWED_USERS`, `HERMES_TIMEZONE`, …) is an environment variable and goes to `.env` — the same file the platform setup flows and `/sethome` write, and the one every runtime reader resolves against. `config set` never writes an `UPPER_SNAKE` key into `config.yaml`, `--force` included; names on the env writer's denylist (`HERMES_YOLO_MODE`, `PATH`, …) are refused outright; any other `UPPER_SNAKE` name is saved to `.env` as-is (plugins, skills and external tools read it from the process environment). A known key written under the wrong prefix (`gateway.discord.foo`, where `discord.foo` is itself a known key) is refused with a did-you-mean and nothing is written; any other unknown path under a known section (`agent.max_turnz`, or a runtime-read key that has no seeded default) is written with a did-you-mean notice, and an unknown lowercase *top-level* key is written with a notice (top-level scalars are bridged into the environment for skills). `--force` writes the refused wrong-prefix path too. Values are type-checked against the schema: a key that must hold a list or a mapping (`custom_providers`, `model.aliases`, `display.platforms`, `plugins.enabled`/`plugins.disabled`, `model_catalog.excluded_providers`, any key already holding one) refuses a plain string or a wrong-shaped literal, and a value that looks like a list/mapping but is not valid YAML/JSON is refused instead of being stored as a string — nothing is written and the error names the expected type. Pass a YAML/JSON literal (`hermes config set custom_providers '[{name: x, base_url: https://...}]'`); to store a string that merely starts with `[` or `{`, quote it in YAML (`"'[text'"`). `--force` still replaces a whole mapping section; a non-list in a list slot has no override, except that a bare name for a list of names read leniently (`agent.disabled_toolsets`, `skills.disabled`) is stored as a one-item list. | | `unset ` | Remove a config key, reverting it to the built-in default. For `UPPER_SNAKE` names this removes the `.env` entry and also drops a stale top-level `config.yaml` copy left by older `config set` runs (`get` reports such a copy as stale). | | `path` | Print the config file path. | | `env-path` | Print the `.env` file path. | From c3021aff0dcb51bb266f965ba4ad00d8667d6fe1 Mon Sep 17 00:00:00 2001 From: teknium1 <127238744+teknium1@users.noreply.github.com> Date: Mon, 21 Sep 2026 22:36:34 -0700 Subject: [PATCH 016/173] ci(skills-index): re-mint the App token before the catalog stars probe; keep fetched_at honest on failure The `Probe plugin catalog stars` step reused the GitHub App installation token minted at job start. That token lives 1 h; `build_skills_index.py` (the step before it) has walked ClawHub for 60-85 min since Sep 17, so every scheduled probe since Sep 18 ran on an expired token and logged `GraphQL probe failed (HTTP Error 401: Unauthorized); keeping previous counts`. 114 of the 213 catalog repos ended up without a star count and the "Most starred" default sort fell to alphabetical for half the catalog. Run-log evidence (job step timestamps; `gh run view --json jobs` + job logs): run 35569229751 (Sep 21): mint 06:36:52Z -> probe 08:01:50Z (85 min) -> 401, "Probed 213 repos ... wrote 99 star counts" run 35426522555 (Sep 19): mint 06:24:52Z -> probe 07:39:59Z (75 min) -> 401 run 35133575877 (Sep 16, last success): mint 18:18:39Z -> probe 18:40:12Z (21 min) -> "Probed 100 repos ... wrote 100 star counts" Fix: a second `./.github/actions/get-app-token` step (`probe-token`) immediately before the probe; the probe reads that token. Ordering stays (the probe does not depend on the walk, but the artifact upload wants both files and the step comment already documents the walk timing). Freshness: `main()` stamped `fetched_at = now` whenever the written map differed from the previous one, so a FAILED probe still moved the timestamp forward and the catalog footer read "popularity ranking as of " over Sep-16 counts (live `plugin-stars.json`: fetched_at 2026-09-21 with Sep-16 data). `probe_stars` now returns `(stars, probed)`; `fetched_at` advances only when GitHub actually answered, otherwise the previous timestamp is kept, a `::warning::` names how many catalog repos have no count, and the summary line says "Probe failed; wrote N cached star counts (as of )" instead of "Probed N repos". Local repro (3 catalog repos, cache from Sep 16 with 2 of them, GraphQL 401): before: fetched_at restamped to now; "Probed 3 repos ... wrote 2 star counts" after: fetched_at stays 2026-09-16T18:40:16+00:00; "::warning::plugin star probe failed; reused 2 cached counts from 2026-09-16..., 1 of 3 catalog repos have no star count" Not taken from #118129: the per-repo REST fallback and the payload-shape validation. The transport was never the problem (the same expired token 401s on REST too), and one GraphQL request per run is the rate-limit rule this script documents. Fixes #118113 credit: @fangliquanflq #118129 (honest fetched_at on a failed probe + ::warning; slim redo) --- .github/workflows/skills-index.yml | 13 +++++++++++- tests/website/test_fetch_plugin_stars.py | 17 +++++++++++++++ website/scripts/fetch-plugin-stars.py | 27 ++++++++++++++++-------- 3 files changed, 47 insertions(+), 10 deletions(-) diff --git a/.github/workflows/skills-index.yml b/.github/workflows/skills-index.yml index 6bcbbaaaa1..4fec0fb43f 100644 --- a/.github/workflows/skills-index.yml +++ b/.github/workflows/skills-index.yml @@ -54,9 +54,20 @@ jobs: # Plugin-catalog star counts follow the same rule as the index: GitHub is consulted # only here, on the schedule (one GraphQL request for every catalog repo), and docs # deploys reuse the artifact / live copy without touching the API. + # + # Fresh token: the App installation token minted at job start lives 1 h, and the + # index walk above runs 60-85 min, so the probe used an expired token and 401'd on + # every scheduled run from Sep 18 (#118113). Mint again right before the probe. + - name: Get GitHub App token for the stars probe + id: probe-token + uses: ./.github/actions/get-app-token + with: + client-id: ${{ vars.APP_CLIENT_ID }} + private-key: ${{ secrets.APP_PRIVATE_KEY }} + - name: Probe plugin catalog stars env: - GITHUB_TOKEN: ${{ steps.app-token.outputs.token }} + GITHUB_TOKEN: ${{ steps.probe-token.outputs.token }} run: python website/scripts/fetch-plugin-stars.py --probe - name: Upload index artifact diff --git a/tests/website/test_fetch_plugin_stars.py b/tests/website/test_fetch_plugin_stars.py index b0d1134494..b9d430105d 100644 --- a/tests/website/test_fetch_plugin_stars.py +++ b/tests/website/test_fetch_plugin_stars.py @@ -78,3 +78,20 @@ def test_probe_is_one_graphql_request_and_a_failure_keeps_previous_counts(mod, t monkeypatch.setattr(mod, "_graphql", limited) assert mod.main(catalog_dir=cat, output=out, probe=True, live_url=None, token="t") == 0 assert json.loads(out.read_text())["stars"] == {"a/one": 42, "b/two": 9} + + +def test_failed_probe_keeps_the_previous_timestamp_and_warns(mod, tmp_path, monkeypatch, capsys): + """An expired-token 401 must not restamp ``fetched_at``: the catalog footer reads it as + "ranking as of " and showed today's date over five-day-old counts (#118113).""" + cat = _catalog(tmp_path, "https://github.com/a/one", "https://github.com/b/two") + out = tmp_path / "plugin-stars.json" + out.write_text(json.dumps({"fetched_at": "2026-09-16T18:40:16+00:00", "stars": {"a/one": 7}}), encoding="utf-8") + + def unauthorized(query, token): + raise urllib.error.HTTPError("u", 401, "Unauthorized", hdrs=None, fp=None) + monkeypatch.setattr(mod, "_graphql", unauthorized) + + assert mod.main(catalog_dir=cat, output=out, probe=True, live_url=None, token="expired") == 0 + data = json.loads(out.read_text()) + assert data == {"fetched_at": "2026-09-16T18:40:16+00:00", "stars": {"a/one": 7}} + assert "::warning::" in capsys.readouterr().out diff --git a/website/scripts/fetch-plugin-stars.py b/website/scripts/fetch-plugin-stars.py index e10e665543..9f16f01970 100644 --- a/website/scripts/fetch-plugin-stars.py +++ b/website/scripts/fetch-plugin-stars.py @@ -111,18 +111,21 @@ def stars_query(slugs: list[str]) -> str: return "query {\n" + fields + "\n}" -def probe_stars(slugs: list[str], previous: dict[str, int], token: str | None) -> dict[str, int]: - """One GraphQL request for all repos; on failure keep every previous count (never regress to 0).""" +def probe_stars(slugs: list[str], previous: dict[str, int], token: str | None) -> tuple[dict[str, int], bool]: + """One GraphQL request for all repos -> ``(stars, probed)``. On failure keep every previous + count (never regress to 0) and report ``probed=False`` so the caller does not restamp + ``fetched_at`` over counts that are days old (#118113).""" + kept = {s: previous[s] for s in slugs if s in previous} if not slugs: - return {} + return {}, True if not token: _log("no GITHUB_TOKEN; keeping previous counts without probing") - return {s: previous[s] for s in slugs if s in previous} + return kept, False try: payload = _graphql(stars_query(slugs), token) except (urllib.error.URLError, OSError, ValueError) as e: _log(f"GraphQL probe failed ({e}); keeping previous counts") - return {s: previous[s] for s in slugs if s in previous} + return kept, False data = payload.get("data") or {} for err in payload.get("errors") or []: _log(f"GraphQL: {err.get('message')}") # e.g. a renamed/deleted repo; its previous count is kept @@ -133,7 +136,7 @@ def probe_stars(slugs: list[str], previous: dict[str, int], token: str | None) - stars[slug] = node["stargazerCount"] elif slug in previous: stars[slug] = previous[slug] - return stars + return stars, True def main(catalog_dir: Path = DEFAULT_CATALOG_DIR, output: Path = DEFAULT_OUTPUT, @@ -150,11 +153,17 @@ def main(catalog_dir: Path = DEFAULT_CATALOG_DIR, output: Path = DEFAULT_OUTPUT, slugs = catalog_slugs(catalog_dir) prev_stars = {k: int(v) for k, v in (previous.get("stars") or {}).items() if isinstance(v, (int, float))} - stars = probe_stars(slugs, prev_stars, token or os.environ.get("GITHUB_TOKEN") or os.environ.get("GH_TOKEN")) - probed = stars != prev_stars or not previous - fetched_at = datetime.now(timezone.utc).isoformat() if probed or stars else str(previous.get("fetched_at") or "") + stars, probed = probe_stars(slugs, prev_stars, token or os.environ.get("GITHUB_TOKEN") or os.environ.get("GH_TOKEN")) + fetched_at = datetime.now(timezone.utc).isoformat() if probed else previous.get("fetched_at") output.write_text(json.dumps({"fetched_at": fetched_at, "stars": stars}, separators=(",", ":")), encoding="utf-8") + if not probed: + # Visible in the run summary: the ranking page would otherwise claim today's date over stale counts. + missing = len(slugs) - len(stars) + print(f"::warning::plugin star probe failed; reused {len(stars)} cached counts from {fetched_at}, " + f"{missing} of {len(slugs)} catalog repos have no star count") + print(f"Probe failed; wrote {len(stars)} cached star counts (as of {fetched_at}) to {output}") + return 0 print(f"Probed {len(slugs)} repos in one GraphQL request, wrote {len(stars)} star counts to {output}") return 0 From 07347414ed242ce05d2e582423cbb775fb10914d Mon Sep 17 00:00:00 2001 From: teknium1 <127238744+teknium1@users.noreply.github.com> Date: Mon, 21 Sep 2026 22:43:59 -0700 Subject: [PATCH 017/173] fix: keep cron-provider plugins out of the general PluginManager With `chronos` in plugins.enabled every start warned "Failed to load plugin 'chronos': 'PluginContext' object has no attribute 'register_cron_scheduler'": collect_directory_manifests skipped the bundled memory/context_engine/model-providers categories but not cron_providers, and a user-installed cron provider under ~/.hermes/plugins/ was never auto-coerced to kind=exclusive. Cron providers activate through plugins.cron_providers (cron.provider config), so the general manager must only record them, exactly like memory providers. Fixes #62951 credit: @wdmason #100362 (slim redo; #100396 is the consolidated duplicate) --- hermes_cli/plugins_discovery.py | 3 +- hermes_cli/plugins_manifest.py | 10 ++-- tests/hermes_cli/test_plugins.py | 41 ++++++++++++++++ .../test_single_query_plugin_cli_ref.py | 48 +++++++++++++++++++ 4 files changed, 97 insertions(+), 5 deletions(-) create mode 100644 tests/hermes_cli/test_single_query_plugin_cli_ref.py diff --git a/hermes_cli/plugins_discovery.py b/hermes_cli/plugins_discovery.py index 21d5850696..03c594423c 100644 --- a/hermes_cli/plugins_discovery.py +++ b/hermes_cli/plugins_discovery.py @@ -176,7 +176,8 @@ def collect_directory_manifests() -> List[PluginManifest]: # Excluded bundled top-level categories have their own discovery; platforms scan separately. repo_plugins = _origin.get_bundled_plugins_dir() logger.debug("Scanning bundled plugins: %s", repo_plugins) - _scan("bundled (top-level)", repo_plugins, "bundled", {"memory", "context_engine", "platforms", "model-providers"}) + _scan("bundled (top-level)", repo_plugins, "bundled", + {"memory", "context_engine", "platforms", "model-providers", "cron_providers"}) _scan("bundled/platforms", repo_plugins / "platforms", "bundled") user_dir = get_hermes_home() / "plugins" logger.debug("Scanning user plugins: %s", user_dir) diff --git a/hermes_cli/plugins_manifest.py b/hermes_cli/plugins_manifest.py index e81fe50822..d0fadf5070 100644 --- a/hermes_cli/plugins_manifest.py +++ b/hermes_cli/plugins_manifest.py @@ -249,10 +249,12 @@ def resolve_plugin_load_order(manifests: Mapping[str, "PluginManifest"]) -> List def _detect_kind_from_source(source_text: str) -> Optional[str]: - """Kind implied by source markers (mirrors plugins/memory ``_is_memory_provider_dir``): memory-provider - markers -> ``exclusive``; ``register_provider`` + ``ProviderProfile`` -> ``model-provider``; else - ``None``. Keeps both kinds out of the general manager's eager import.""" - if "register_memory_provider" in source_text or "MemoryProvider" in source_text: + """Kind implied by source markers (mirrors plugins/memory ``_is_memory_provider_dir`` and + plugins/cron_providers ``_is_cron_provider_dir``): memory- or cron-provider markers -> ``exclusive``; + ``register_provider`` + ``ProviderProfile`` -> ``model-provider``; else ``None``. Keeps these kinds out + of the general manager's eager import (its PluginContext has no ``register_cron_scheduler``, #62951).""" + if any(marker in source_text for marker in ( + "register_memory_provider", "MemoryProvider", "register_cron_scheduler", "CronScheduler")): return "exclusive" if "register_provider" in source_text and "ProviderProfile" in source_text: return "model-provider" diff --git a/tests/hermes_cli/test_plugins.py b/tests/hermes_cli/test_plugins.py index d4e5e0ca65..c28a26abcc 100644 --- a/tests/hermes_cli/test_plugins.py +++ b/tests/hermes_cli/test_plugins.py @@ -555,6 +555,47 @@ class TestPluginLoading: assert entry.module is None assert "exclusive" in (entry.error or "").lower() + def test_bundled_cron_provider_is_not_loaded_by_general_manager(self, tmp_path, monkeypatch): + """``plugins/cron_providers/`` has its own discovery (``plugins.cron_providers``); the general + manager's PluginContext has no ``register_cron_scheduler``, so importing chronos from here + warned ``Failed to load plugin 'chronos'`` on every start it was enabled (#62951).""" + bundled = tmp_path / "bundled" + chronos = bundled / "cron_providers" / "chronos" + chronos.mkdir(parents=True) + (chronos / "plugin.yaml").write_text(yaml.dump({"name": "chronos"}), encoding="utf-8") + (chronos / "__init__.py").write_text( + "def register(ctx):\n ctx.register_cron_scheduler(object())\n", encoding="utf-8") + hermes_home = tmp_path / "hermes_test" + hermes_home.mkdir(exist_ok=True) + (hermes_home / "config.yaml").write_text(yaml.safe_dump({"plugins": {"enabled": ["chronos"]}})) + monkeypatch.setenv("HERMES_HOME", str(hermes_home)) + from hermes_cli import plugins as plugins_mod + monkeypatch.setattr(plugins_mod, "get_bundled_plugins_dir", lambda: bundled) + + mgr = PluginManager() + mgr.discover_and_load() + + assert not any(key.endswith("chronos") for key in mgr._plugins) + + def test_user_cron_plugin_auto_coerced_to_exclusive(self, tmp_path, monkeypatch): + """A user-installed cron provider (no ``kind:``) routes to ``plugins.cron_providers`` like a + memory provider does, instead of being imported by the general manager (#62951).""" + hermes_home = tmp_path / "hermes_test" + plugin_dir = hermes_home / "plugins" / "mycron" + plugin_dir.mkdir(parents=True) + (plugin_dir / "plugin.yaml").write_text(yaml.dump({"name": "mycron"}), encoding="utf-8") + (plugin_dir / "__init__.py").write_text( + "def register(ctx):\n ctx.register_cron_scheduler(object())\n", encoding="utf-8") + (hermes_home / "config.yaml").write_text(yaml.safe_dump({"plugins": {"enabled": ["mycron"]}})) + monkeypatch.setenv("HERMES_HOME", str(hermes_home)) + + mgr = PluginManager() + mgr.discover_and_load() + + entry = mgr._plugins["mycron"] + assert entry.manifest.kind == "exclusive" + assert entry.module is None and not entry.enabled + def test_entrypoint_memory_provider_auto_coerced_to_exclusive( self, tmp_path, monkeypatch ): diff --git a/tests/hermes_cli/test_single_query_plugin_cli_ref.py b/tests/hermes_cli/test_single_query_plugin_cli_ref.py new file mode 100644 index 0000000000..e3e74f8edc --- /dev/null +++ b/tests/hermes_cli/test_single_query_plugin_cli_ref.py @@ -0,0 +1,48 @@ +"""``hermes chat -q``/``-Q`` runs give plugins the CLI reference like the interactive loop does. + +Only ``HermesCLI.run()`` set ``PluginManager._cli_ref``, so a plugin tool dispatched from a one-shot +turn saw ``_cli_ref is None`` and ``PluginContext.dispatch_tool`` injected no ``parent_agent`` (#67597). +""" + +from __future__ import annotations + +from types import SimpleNamespace + +import pytest + +import cli +from hermes_cli.plugins import get_plugin_manager + + +@pytest.fixture(autouse=True) +def _one_shot_seams(monkeypatch): + monkeypatch.delenv("HERMES_KANBAN_GOAL_MODE", raising=False) + monkeypatch.setattr(cli, "_should_seed_interactive", lambda *a, **k: False) + monkeypatch.setattr(cli, "_collect_query_images", lambda q, i: (q, [])) + monkeypatch.setattr(cli, "_collect_kanban_task_images", lambda imgs: []) + monkeypatch.setattr(cli, "_finalize_single_query", lambda c: None) + monkeypatch.setattr(get_plugin_manager(), "_cli_ref", None) + + +def _stub(**extra): + return SimpleNamespace( + _single_query_mode=False, _claim_active_session=lambda *a, **k: True, + console=SimpleNamespace(print=lambda *a, **k: None), _show_security_advisories=lambda: None, + chat=lambda *a, **k: "response", _print_exit_summary=lambda **k: None, + _last_turn_result={"failed": False}, **extra, + ) + + +def test_one_shot_turn_binds_the_cli_for_plugins(): + stub = _stub() + with pytest.raises(SystemExit): + cli._run_single_query_mode(stub, "do the thing", None, False, True) + assert get_plugin_manager()._cli_ref is stub + + +def test_quiet_turn_binds_the_cli_for_plugins(): + stub = _stub(_ensure_runtime_credentials=lambda: False, _credentials_rate_limited=False, + session_id="s1", model="m") + with pytest.raises(SystemExit): + cli._run_single_query_mode(stub, "do the thing", None, True, True) + assert get_plugin_manager()._cli_ref is stub From 01eae47b3af9538dde40c0c5e9c8b9479c9ba583 Mon Sep 17 00:00:00 2001 From: teknium1 <127238744+teknium1@users.noreply.github.com> Date: Mon, 21 Sep 2026 22:44:00 -0700 Subject: [PATCH 018/173] fix: dispatch only the context kwargs a tool handler declares ToolRegistry.dispatch spread every injected keyword (task_id, session_id, user_task, parent_agent, ...) into the handler, so a third-party plugin tool written as `def handle(args)` raised TypeError on every call. The plugin contract (plugins/AGENTS.md) says optional kwargs are signature-inspected, not forwarded unconditionally; hooks already do this in plugins_dispatch. Handlers taking **kwargs still receive the full payload. Docs updated: the narrow signature is supported, **kwargs opts into the whole context. Fixes #68318 credit: @vveerrgg #22146 (slim redo; #68636 is the later duplicate) --- tests/tools/test_registry.py | 22 +++++++++++++++++++ tools/registry.py | 18 +++++++++++++++ website/docs/developer-guide/plugins/index.md | 8 ++++--- 3 files changed, 45 insertions(+), 3 deletions(-) diff --git a/tests/tools/test_registry.py b/tests/tools/test_registry.py index 9390b34414..d36e491819 100644 --- a/tests/tools/test_registry.py +++ b/tests/tools/test_registry.py @@ -43,6 +43,28 @@ class TestRegisterAndDispatch: result = json.loads(reg.dispatch("alpha", {})) assert result == {"ok": True} + def test_dispatch_withholds_injected_kwargs_from_narrow_plugin_handler(self): + """model_tools injects task_id/session_id/user_task on every call; a third-party ``handle(args)`` + must receive only what its signature declares, and a ``**kwargs`` handler everything (#68318).""" + reg = ToolRegistry() + seen = {} + + def narrow(args, session_id=None): + seen["narrow"] = session_id + return json.dumps({"ok": True}) + + def wide(args, **kwargs): + seen["wide"] = kwargs + return json.dumps({"ok": True}) + + reg.register(name="narrow", toolset="core", schema=_make_schema("narrow"), handler=narrow) + reg.register(name="wide", toolset="core", schema=_make_schema("wide"), handler=wide) + injected = {"task_id": "t1", "session_id": "s1", "user_task": "do it"} + assert json.loads(reg.dispatch("narrow", {}, **injected)) == {"ok": True} + assert seen["narrow"] == "s1" + assert json.loads(reg.dispatch("wide", {}, **injected)) == {"ok": True} + assert seen["wide"] == injected + def test_register_rejects_non_dict_parameters(self): """A list/str ``parameters`` fails at registration, not in a provider request (pi acaa253cc).""" reg = ToolRegistry() diff --git a/tools/registry.py b/tools/registry.py index fccfb2d448..468c97a0f7 100644 --- a/tools/registry.py +++ b/tools/registry.py @@ -8,6 +8,7 @@ model_tools.""" import ast import functools import importlib +import inspect import json import logging import sys @@ -884,6 +885,10 @@ class ToolRegistry: if not entry: return tool_error(f"Unknown tool: {name}") try: + # Plugin contract (plugins/AGENTS.md): optional context kwargs (task_id, session_id, user_task, + # parent_agent, ...) are signature-inspected like hook payloads, so a narrow ``handle(args)`` + # plugin handler is not broken by every field the dispatcher injects (#68318). + kwargs = _kwargs_accepted_by(entry.handler, kwargs) if entry.is_async: from model_tools import _run_async result = _run_async(entry.handler(args, **kwargs)) @@ -1010,3 +1015,16 @@ def tool_error(message, **extra) -> str: def tool_result(data=None, **kwargs) -> str: """JSON-encode a dict positional arg *or* keyword arguments (not both).""" return json.dumps(data if data is not None else kwargs, ensure_ascii=False) + + +def _kwargs_accepted_by(handler: Callable, kwargs: dict) -> dict: + """*kwargs* narrowed to what *handler*'s signature declares; everything when it takes ``**kwargs`` or + cannot be introspected (builtins, some C callables).""" + try: + parameters = inspect.signature(handler).parameters + except (TypeError, ValueError): + return kwargs + if any(p.kind is inspect.Parameter.VAR_KEYWORD for p in parameters.values()): + return kwargs + keyword_kinds = {inspect.Parameter.POSITIONAL_OR_KEYWORD, inspect.Parameter.KEYWORD_ONLY} + return {k: v for k, v in kwargs.items() if k in parameters and parameters[k].kind in keyword_kinds} diff --git a/website/docs/developer-guide/plugins/index.md b/website/docs/developer-guide/plugins/index.md index cade14e784..3442920e07 100644 --- a/website/docs/developer-guide/plugins/index.md +++ b/website/docs/developer-guide/plugins/index.md @@ -520,7 +520,9 @@ def unit_convert(args: dict, **kwargs) -> str: 1. **Signature:** `def my_handler(args: dict, **kwargs) -> str` 2. **Return:** Always a JSON string. Success and errors alike. 3. **Never raise:** Catch all exceptions, return error JSON instead. -4. **Accept `**kwargs`:** Hermes may pass additional context in the future. +4. **Accept `**kwargs`:** Hermes injects context keywords (`task_id`, `session_id`, `user_task`, + `parent_agent`, ...) and only forwards the ones your signature names, so `def handler(args)` + works; `**kwargs` is how you opt into the full, additively growing context. ## Step 5: Write the registration @@ -1780,11 +1782,11 @@ def handler(args, **kwargs): **Missing `**kwargs` in handler signature:** ```python -# Wrong — will break if Hermes passes extra context +# Works — the dispatcher only forwards the context keywords a signature names def handler(args): ... -# Right +# Better — receives every injected context field (task_id, session_id, parent_agent, ...) def handler(args, **kwargs): ... ``` From 069d6a1f9e76f3950e82797df2c4a269a368b162 Mon Sep 17 00:00:00 2001 From: teknium1 <127238744+teknium1@users.noreply.github.com> Date: Mon, 21 Sep 2026 22:44:02 -0700 Subject: [PATCH 019/173] fix: give plugins the CLI reference in `hermes chat -q/-Q` too PluginManager._cli_ref was set only by the interactive run loop (_tui_init_run_state), so a one-shot query never bound it and PluginContext.dispatch_tool injected no parent_agent into plugin tools. Bind it once in _run_single_query_mode before the turn; the quiet, stream_json and kanban-goal branches all pass through that line, and the interactive-seed branch calls cli.run(), which already binds it. Fixes #67597 credit: @webtecnica #67610 (slim redo at the current seam; #67611 and #67633 are later duplicates) --- hermes_cli/cli_single_query.py | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/hermes_cli/cli_single_query.py b/hermes_cli/cli_single_query.py index f1a06dfbc0..ed35d53ee5 100644 --- a/hermes_cli/cli_single_query.py +++ b/hermes_cli/cli_single_query.py @@ -436,6 +436,10 @@ def _run_single_query_mode(cli, query, image, quiet, oneshot, stream_json: bool cli._seeded_first_message = _SeededQueryMessage(seeded_query, seeded_images) return cli.run() cli._single_query_mode = True # agent waits the full MCP cold-start before its only tool snapshot + # Only the interactive run loop set this, so plugin tools dispatched from a `-q`/`-Q` turn got no + # parent_agent (PluginContext.dispatch_tool reads it) — #67597. + from hermes_cli.plugins import get_plugin_manager + get_plugin_manager()._cli_ref = cli # No user can answer approval prompts: the approval gate takes the deterministic path. # One-shot mode: no between-turns MCP late-binding refresh, so the agent must wait the full MCP # cold-start bound before its first (and only) tool snapshot. See #51316. From a947002e7bbdf5f99f5d3a756b19cdc8a57db23a Mon Sep 17 00:00:00 2001 From: teknium1 <127238744+teknium1@users.noreply.github.com> Date: Mon, 21 Sep 2026 22:33:29 -0700 Subject: [PATCH 020/173] fix: treat core memory-provider sentinels as built-in, not plugins MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `memory.provider: builtin` (also `default`, `built-in`, `none`) selects the built-in store, yet doctor short-circuited only on an empty name and printed "⚠ builtin plugin not found run: hermes memory setup", and the provider migration (`hermes update`, agent init) printed "⚠ Memory provider 'builtin' is configured but not installed and not in the plugin catalog". update_cmd_deps and web_server_memory each carried their own (differing) inline sentinel set. One predicate, `agent.memory_provider.is_core_memory_provider`, now owns the sentinel set next to the MemoryProvider ABC; doctor_state, memory_provider_migration, update_cmd_deps, web_server_memory and agent_init's provider activation all use it. Fixes #75647 Fixes #115113 credit: @Christopher-Schulze #75679 credit: @Luna161 #83317 credit: @kokhlo #115117 credit: @KnowBotDev #115302 --- agent/agent_init.py | 3 +- agent/memory_provider.py | 9 ++++++ hermes_cli/doctor_state.py | 3 +- hermes_cli/memory_provider_migration.py | 3 +- hermes_cli/update_cmd_deps.py | 5 +-- hermes_cli/web_server_memory.py | 3 +- .../test_memory_provider_core_sentinels.py | 32 +++++++++++++++++++ 7 files changed, 52 insertions(+), 6 deletions(-) create mode 100644 tests/hermes_cli/test_memory_provider_core_sentinels.py diff --git a/agent/agent_init.py b/agent/agent_init.py index 902a6a6f00..4be3e3852a 100644 --- a/agent/agent_init.py +++ b/agent/agent_init.py @@ -26,6 +26,7 @@ from agent.context_compressor import ContextCompressor from agent.agent_runtime_helpers import _ra from agent.iteration_budget import IterationBudget, normalize_budget_warning_ratio from agent.memory_manager import StreamingContextScrubber +from agent.memory_provider import is_core_memory_provider from agent.session_activity import ActivityProvenance from agent.model_metadata import ( MINIMUM_CONTEXT_LENGTH, fetch_model_metadata, is_local_endpoint, query_ollama_num_ctx @@ -1295,7 +1296,7 @@ def _init_memory(agent, _agent_cfg, skip_memory, platform): if not skip_memory: try: _mem_provider_name = mem_config.get("provider", "") if mem_config else "" - if _mem_provider_name and _mem_provider_name.strip(): + if not is_core_memory_provider(_mem_provider_name): from agent.memory_manager import MemoryManager as _MemoryManager from plugins.memory import load_memory_provider as _load_mem agent._memory_manager = _MemoryManager() diff --git a/agent/memory_provider.py b/agent/memory_provider.py index 938fd0b003..f9f4bcd4f6 100644 --- a/agent/memory_provider.py +++ b/agent/memory_provider.py @@ -39,6 +39,15 @@ PRE_COMPRESS_CHECKPOINT_API_VERSION = 2 # Default glyph for recall indicators; providers may use their own brand mark. INDICATOR_GLYPH = "🧠" +# ``memory.provider`` values that mean "the built-in store, no external plugin". The built-in +# store is core: doctor, migration and dependency refresh must never look these up as plugins. +CORE_MEMORY_PROVIDER_SENTINELS = frozenset({"", "default", "builtin", "built-in", "none"}) + + +def is_core_memory_provider(name: Optional[str]) -> bool: + """True when ``memory.provider`` selects the built-in store rather than an external plugin.""" + return str(name or "").strip().lower() in CORE_MEMORY_PROVIDER_SENTINELS + @dataclass(frozen=True) class RecallStatus: diff --git a/hermes_cli/doctor_state.py b/hermes_cli/doctor_state.py index 60972cdf4e..f287511e65 100644 --- a/hermes_cli/doctor_state.py +++ b/hermes_cli/doctor_state.py @@ -531,8 +531,9 @@ def _memory_provider_generic(name: str) -> None: @doctor_check() def _check_memory_provider(should_fix: bool, f: Finding) -> None: from hermes_cli.doctor import HERMES_HOME + from agent.memory_provider import is_core_memory_provider name = _doctor_memory_config(HERMES_HOME).get("provider", "") - if not name: + if is_core_memory_provider(name): check_ok("Built-in memory active", "(no external provider configured — this is fine)") return checker, missing_row, missing_issue, label = _MEMORY_PROVIDER_CHECKS.get(name, (None, None, None, name)) diff --git a/hermes_cli/memory_provider_migration.py b/hermes_cli/memory_provider_migration.py index 8e6205e7c5..2fec8417f3 100644 --- a/hermes_cli/memory_provider_migration.py +++ b/hermes_cli/memory_provider_migration.py @@ -60,7 +60,8 @@ def migrate_home(home: Path, *, install: Callable[[str], dict], say: Callable[[s update or the agent down with it. """ name = configured_provider(home) - if not name or provider_present(name, home): + from agent.memory_provider import is_core_memory_provider + if is_core_memory_provider(name) or provider_present(name, home): return None if catalog_source(name) is None: say(f" ⚠ Memory provider '{name}' is configured but not installed and not in the plugin catalog. " diff --git a/hermes_cli/update_cmd_deps.py b/hermes_cli/update_cmd_deps.py index aa366f0782..6cee9656c0 100644 --- a/hermes_cli/update_cmd_deps.py +++ b/hermes_cli/update_cmd_deps.py @@ -386,8 +386,9 @@ def _refresh_active_memory_provider_dependencies() -> None: return provider = str(memory_cfg.get("provider") or "").strip() - # "default"/empty is the built-in file store — no pip deps. - if not provider or provider in {"default", "builtin", "none"}: + # The built-in file store has no pip deps. + from agent.memory_provider import is_core_memory_provider + if is_core_memory_provider(provider): return try: diff --git a/hermes_cli/web_server_memory.py b/hermes_cli/web_server_memory.py index 379dd81dd4..6902a93579 100644 --- a/hermes_cli/web_server_memory.py +++ b/hermes_cli/web_server_memory.py @@ -24,8 +24,9 @@ _MEMORY_PROVIDER_IMPORT_NAMES = { def _normalize_memory_provider_name(name: Any) -> str: + from agent.memory_provider import is_core_memory_provider provider = str(name or "").strip() - return "" if provider.lower() in {"built-in", "builtin", "none"} else provider + return "" if is_core_memory_provider(provider) else provider def _load_memory_provider(name: str): diff --git a/tests/hermes_cli/test_memory_provider_core_sentinels.py b/tests/hermes_cli/test_memory_provider_core_sentinels.py new file mode 100644 index 0000000000..dc78af8b97 --- /dev/null +++ b/tests/hermes_cli/test_memory_provider_core_sentinels.py @@ -0,0 +1,32 @@ +"""``memory.provider`` set to a core sentinel (``builtin``/``default``/``none``) names the built-in +store, not a plugin: doctor and the provider migration must not report it as missing (#75647, #115113).""" + +from pathlib import Path + +import pytest + +from hermes_cli import doctor, doctor_state +from hermes_cli import memory_provider_migration as mig + + +@pytest.mark.parametrize("sentinel", ["builtin", "Default", "none"]) +def test_doctor_treats_core_sentinel_as_builtin_memory(tmp_path: Path, monkeypatch, capsys, sentinel): + (tmp_path / "config.yaml").write_text(f"memory:\n provider: {sentinel}\n") + monkeypatch.setattr(doctor, "HERMES_HOME", tmp_path) + + finding = doctor_state._check_memory_provider(False) + out = capsys.readouterr().out + + assert "Built-in memory active" in out + assert "plugin not found" not in out + assert finding.issues == [] + + +def test_migration_never_installs_a_core_sentinel(tmp_path: Path, monkeypatch): + monkeypatch.setenv("HERMES_HOME", str(tmp_path)) + (tmp_path / "config.yaml").write_text("memory:\n provider: builtin\n") + monkeypatch.setattr(mig, "catalog_source", lambda name: pytest.fail("catalog must not be consulted")) + said: list[str] = [] + + assert mig.migrate_home(tmp_path, install=lambda n: pytest.fail("must not install"), say=said.append) is None + assert said == [] From 9cbc221dcbde96d9a2a875a4c78355d12d01e754 Mon Sep 17 00:00:00 2001 From: teknium1 <127238744+teknium1@users.noreply.github.com> Date: Mon, 21 Sep 2026 22:36:09 -0700 Subject: [PATCH 021/173] fix: clone plugin context engines via clone_for_agent(), not blind deepcopy MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A general-plugin context engine is one shared instance; agent init copied it per agent with copy.deepcopy() only. Engines that hold a SQLite connection or lock (hermes-lcm) already expose clone_for_agent() for exactly this, but it was never called, so every init logged "could not be safely copied … falling back to built-in compressor" and the engine was unusable through the plugin system. ContextEngine grows clone_for_agent() (default: deepcopy, the previous behaviour) and _select_context_engine calls it; the failure message now names the hook to override. Docs: context-engine-plugin.md documents the per-agent clone contract. Test change (existing on main): tests/agent/test_context_engine.py:: test_agent_init_source_deepcopies_singleton_not_aliases was a source-reading pin on the literal `copy.deepcopy(_candidate)` line, which this fix intentionally replaces. It is superseded by tests/agent/test_plugin_context_engine_clone.py, which drives the real _select_context_engine seam and asserts the invariant it guarded (child update_model() never mutates the shared singleton) plus the new clone_for_agent() path. Fixes #99640 credit: @stephenschoettler #62374 credit: @686f6c61 #99677 --- agent/agent_init.py | 16 +++-- agent/context_engine.py | 8 +++ tests/agent/test_context_engine.py | 38 +----------- .../agent/test_plugin_context_engine_clone.py | 62 +++++++++++++++++++ .../developer-guide/context-engine-plugin.md | 16 +++++ 5 files changed, 96 insertions(+), 44 deletions(-) create mode 100644 tests/agent/test_plugin_context_engine_clone.py diff --git a/agent/agent_init.py b/agent/agent_init.py index 4be3e3852a..90d7cc6fc0 100644 --- a/agent/agent_init.py +++ b/agent/agent_init.py @@ -1870,22 +1870,20 @@ def _select_context_engine(_agent_cfg): except Exception: _candidate = None if _candidate is not None and _candidate.name == _engine_name: - # Deep-copy the shared singleton so a child's update_model() can't mutate the - # parent's. Uncopyable state (locks, DB conns) → built-in with an ACCURATE message. - import copy + # The plugin system holds ONE shared instance; each agent gets its own so a child's + # update_model() can't mutate the parent's (#42449). clone_for_agent() defaults to + # deepcopy; engines with uncopyable state (locks, DB conns) override it. A failure + # falls back to the built-in compressor with an ACCURATE message, not "not found". try: - # Copy can fail for engines holding uncopyable state (locks, DB connections, clients); in - # that case fall back to the built-in compressor with an ACCURATE message rather than - # silently mislabelling it "not found". See #42449. - _selected_engine = copy.deepcopy(_candidate) + _selected_engine = _candidate.clone_for_agent() except Exception as _copy_err: _copy_failed = True _ra().logger.warning( "Context engine '%s' could not be safely copied for this " "agent (%s) — falling back to built-in compressor. Plugin " "engines that hold uncopyable state (locks, DB connections) " - "should implement __deepcopy__ to copy only mutable budget " - "state.", + "should override clone_for_agent() (or __deepcopy__) to copy " + "only mutable budget state.", _engine_name, _copy_err, ) diff --git a/agent/context_engine.py b/agent/context_engine.py index a052021b27..3fb272a50e 100644 --- a/agent/context_engine.py +++ b/agent/context_engine.py @@ -8,6 +8,7 @@ should_compress() / compress() -> on_session_end() at real session boundaries on (CLI exit, /reset, gateway expiry), never per-turn. """ +import copy import json from abc import ABC, abstractmethod from typing import Any, Dict, List, Optional @@ -222,6 +223,13 @@ class ContextEngine(ABC): "compression_count": self.compression_count, } + def clone_for_agent(self) -> "ContextEngine": + """Per-agent instance of a plugin-registered engine (the plugin system holds ONE shared + instance; every AIAgent gets its own so a child's update_model() cannot mutate the parent's). + Override when the engine holds uncopyable state (locks, DB connections): return a fresh + engine sharing the durable backend and copying only mutable budget state.""" + return copy.deepcopy(self) + def update_model( self, model: str, context_length: int, base_url: str = "", api_key: str = "", provider: str = "", api_mode: str = "", diff --git a/tests/agent/test_context_engine.py b/tests/agent/test_context_engine.py index c84bf6b126..c6af348feb 100644 --- a/tests/agent/test_context_engine.py +++ b/tests/agent/test_context_engine.py @@ -237,13 +237,9 @@ class TestInitAgentDoesNotMutatePluginSingleton: """Regression coverage for #42449: a child agent's init must not mutate the shared plugin context-engine singleton via update_model(). - Note: ``test_child_init_does_not_corrupt_parent_singleton`` replicates the - init_agent selection-block *pattern* (it cannot cheaply spin up a full - init_agent), so it documents/verifies the deepcopy approach but does NOT by - itself guard a production revert. The real revert guard is - ``test_agent_init_source_deepcopies_singleton_not_aliases`` (source-pin), - and ``test_unpicklable_engine_falls_back_gracefully`` covers the - copy-failure path. + Note: these replicate the init_agent selection-block *pattern*; the production + seam (``_select_context_engine`` → ``clone_for_agent()``) is driven directly by + ``tests/agent/test_plugin_context_engine_clone.py``. """ def test_child_init_does_not_corrupt_parent_singleton(self, monkeypatch): @@ -317,31 +313,3 @@ class TestInitAgentDoesNotMutatePluginSingleton: assert selected is None # The original engine is untouched (no partial mutation). assert engine.context_length == 1_000_000 - - def test_agent_init_source_deepcopies_singleton_not_aliases(self): - """Source-pin guarding the production fix in agent/agent_init.py: - the plugin-singleton fallback MUST deepcopy the candidate, not alias - it (`_selected_engine = _candidate`). Full init_agent is too heavy to - drive here, so this pins the exact line so a future revert to direct - assignment fails CI. Regression for #42449.""" - import inspect - import re - import agent.agent_init as _ai - - src = inspect.getsource(_ai) - # The candidate fetched from the plugin singleton must be deep-copied - # before becoming _selected_engine (which is later mutated by - # update_model). A bare `_selected_engine = _candidate` is the bug. - assert re.search( - r"_selected_engine\s*=\s*(copy|_copy)\.deepcopy\(\s*_candidate\s*\)", - src, - ), ( - "agent_init must deepcopy the plugin context-engine singleton " - "(`_selected_engine = copy.deepcopy(_candidate)`) — a bare " - "`_selected_engine = _candidate` re-introduces #42449 (child " - "update_model corrupts the parent's shared singleton)." - ) - # And the bug-shape alias must NOT be present on that path. - assert not re.search( - r"_selected_engine\s*=\s*_candidate\b", src - ), "found the #42449 bug-shape alias `_selected_engine = _candidate`" diff --git a/tests/agent/test_plugin_context_engine_clone.py b/tests/agent/test_plugin_context_engine_clone.py new file mode 100644 index 0000000000..de469e9a22 --- /dev/null +++ b/tests/agent/test_plugin_context_engine_clone.py @@ -0,0 +1,62 @@ +"""A plugin-registered context engine is one shared instance; agent init hands each agent its own +copy through ``clone_for_agent()`` (default deepcopy), so engines with uncopyable state (locks, +SQLite connections — hermes-lcm) stay selectable and a child's model never leaks into the parent +(#99640, #42449).""" + +import threading +from unittest.mock import patch + +from agent.agent_init import _select_context_engine +from agent.context_engine import ContextEngine + + +class _Engine(ContextEngine): + engine_name = "lcm" + + @property + def name(self): + return self.engine_name + + def update_from_response(self, usage): + pass + + def should_compress(self, prompt_tokens=None): + return False + + def compress(self, messages, current_tokens=None): + return messages + + +class _LockedEngine(_Engine): + """Holds a lock (deepcopy raises) and hands out per-agent clones like hermes-lcm does.""" + + def __init__(self): + super().__init__() + self._lock = threading.Lock() + self.clones = 0 + + def clone_for_agent(self): + self.clones += 1 + return _LockedEngine() + + +def _select(engine): + with (patch("plugins.context_engine.load_context_engine", return_value=None), + patch("hermes_cli.plugins.get_plugin_context_engine", return_value=engine)): + return _select_context_engine({"context": {"engine": engine.name}}) + + +def test_engine_with_uncopyable_state_is_selected_via_clone_for_agent(): + singleton = _LockedEngine() + selected = _select(singleton) + assert isinstance(selected, _LockedEngine) and selected is not singleton + assert singleton.clones == 1 + + +def test_default_clone_isolates_parent_from_child_update_model(): + singleton = _Engine() + singleton.update_model(model="big", context_length=1_000_000) + child = _select(singleton) + child.update_model(model="small", context_length=204_800) + assert child is not singleton + assert (singleton.context_length, child.context_length) == (1_000_000, 204_800) diff --git a/website/docs/developer-guide/context-engine-plugin.md b/website/docs/developer-guide/context-engine-plugin.md index 0768f6ef45..6403818a25 100644 --- a/website/docs/developer-guide/context-engine-plugin.md +++ b/website/docs/developer-guide/context-engine-plugin.md @@ -99,6 +99,7 @@ These have sensible defaults in the ABC. Override as needed: | `get_status()` | Standard token/threshold dict | You have custom metrics to expose | | `select_context(request_messages, *, conversation_messages, incoming_message, budget_tokens)` | Returns `None` (no-op) | You select/route which context enters **this** request (retrieval, topic routing) — see below | | `on_turn_complete(messages, usage=None, **kwargs)` | No-op | You ingest/index/observe the finished turn — see below | +| `clone_for_agent()` | `copy.deepcopy(self)` | Your engine holds uncopyable state (locks, SQLite/DB connections) — see [Via general plugin system](#via-general-plugin-system) | ## Per-turn context selection and observation @@ -204,6 +205,21 @@ def register(ctx): Only one engine can be registered. A second plugin attempting to register is rejected with a warning. +The registered instance is shared process-wide, but every `AIAgent` (parent, subagents, gateway +sessions) needs its own engine so a child's `update_model()` cannot mutate the parent's budget. +Hermes therefore calls `engine.clone_for_agent()` on the registered instance at each agent init. +The default is `copy.deepcopy(self)`; override it when the engine holds state that cannot be +deep-copied (locks, SQLite or HTTP connections) and return a fresh engine sharing the durable +backend while copying only the mutable budget fields. If the clone raises, the agent falls back to +the built-in compressor and logs `Context engine 'X' could not be safely copied for this agent`. + +```python +def clone_for_agent(self): + clone = LCMEngine(db_path=self.db_path) # reopens its own connection + clone.threshold_percent = self.threshold_percent + return clone +``` + ## Lifecycle ``` From 913c045c53800a24956a74cc1fe6db0dd4421490 Mon Sep 17 00:00:00 2001 From: teknium1 <127238744+teknium1@users.noreply.github.com> Date: Mon, 21 Sep 2026 22:38:11 -0700 Subject: [PATCH 022/173] fix: resolve user-installed context engines from $HERMES_HOME/plugins/ MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit plugins/context_engine.load_context_engine scanned only the bundled directory. An engine dropped into $HERMES_HOME/plugins/ with `context.engine: ` was reachable only through the general plugin system, which skips any user plugin not listed in plugins.enabled — so every agent init logged "Context engine '' not found — falling back to built-in compressor" although the engine was installed and named in config. Live probe on base (fake HOME, plugins/ctx_demo with register(ctx), context.engine: ctx_demo): the warning fired on EVERY init, not only the first; adding the plugin to plugins.enabled made it load through the general fallback. `context.engine` is the activation signal (as memory.provider / cron.provider are for their kinds), so the engine loader now resolves bundled then user dirs the way plugins/cron_providers does: same `user_plugins_dir()` seam, cheap source heuristic (register_context_engine / ContextEngine), user engines imported under a synthetic namespace, bundled wins on collision, and discover_context_engines() lists them for `hermes plugins` / the dashboard. Fixes #61839 credit: @giggling-ginger #61995 --- plugins/context_engine/__init__.py | 59 +++++++++++++++---- tests/plugins/test_context_engine_user_dir.py | 44 ++++++++++++++ .../developer-guide/context-engine-plugin.md | 2 +- 3 files changed, 93 insertions(+), 12 deletions(-) create mode 100644 tests/plugins/test_context_engine_user_dir.py diff --git a/plugins/context_engine/__init__.py b/plugins/context_engine/__init__.py index dee2c5da37..9468a3827f 100644 --- a/plugins/context_engine/__init__.py +++ b/plugins/context_engine/__init__.py @@ -1,6 +1,8 @@ -"""Context engine plugin discovery: ``plugins/context_engine//`` → ``ContextEngine``. -Engines ship in the repo, separate from the general plugin system; only one is active -(``context.engine`` in config.yaml; default ``"compressor"``, the built-in ContextCompressor).""" +"""Context engine plugin discovery: bundled ``plugins/context_engine//`` then user +``$HERMES_HOME/plugins//`` (bundled wins on collision) → ``ContextEngine``. Separate from the +general plugin system: ``context.engine`` in config.yaml names the active engine (default +``"compressor"``, the built-in ContextCompressor), so a user-installed engine needs no +``plugins.enabled`` entry to be selectable.""" from __future__ import annotations @@ -13,20 +15,53 @@ from plugins import plugin_loader as _loader logger = logging.getLogger(__name__) _CONTEXT_ENGINE_PLUGINS_DIR = Path(__file__).parent +# Synthetic parent package for user-installed engines (keeps them out of the bundled namespace). +_USER_NAMESPACE = "_hermes_user_context_engine" + + +def _is_context_engine_dir(path: Path) -> bool: + """Cheap text heuristic: ``__init__.py`` mentions the context engine contract.""" + init_file = path / "__init__.py" + try: + source = init_file.read_text(errors="replace", encoding="utf-8")[:8192] + except OSError: + return False + return "register_context_engine" in source or "ContextEngine" in source + + +def _iter_engine_dirs() -> List[Tuple[str, Path]]: + """``(name, path)`` for bundled then user engines; bundled wins on collisions.""" + dirs = [(child.name, child) for child in _loader.iter_plugin_dirs(_CONTEXT_ENGINE_PLUGINS_DIR)] + seen = {name for name, _ in dirs} + user_dir = _loader.user_plugins_dir() + if user_dir: + dirs.extend((child.name, child) for child in _loader.iter_plugin_dirs(user_dir) + if child.name not in seen and _is_context_engine_dir(child)) + return dirs def discover_context_engines() -> List[Tuple[str, str, bool]]: - """Return ``[(name, description, is_available), ...]`` for every bundled engine.""" - return [(child.name, _loader.read_plugin_description(child), + """Return ``[(name, description, is_available), ...]`` for every bundled and user engine.""" + return [(name, _loader.read_plugin_description(child), _loader.probe_availability(lambda c=child: _load_engine_from_dir(c))) - for child in _loader.iter_plugin_dirs(_CONTEXT_ENGINE_PLUGINS_DIR)] + for name, child in _iter_engine_dirs()] + + +def find_engine_dir(name: str) -> Optional[Path]: + """Resolve an engine name to its directory (bundled first, then user-installed).""" + bundled = _CONTEXT_ENGINE_PLUGINS_DIR / name + if bundled.is_dir(): + return bundled + user_dir = _loader.user_plugins_dir() + user = user_dir / name if user_dir else None + return user if user and user.is_dir() and _is_context_engine_dir(user) else None def load_context_engine(name: str) -> Optional["ContextEngine"]: # noqa: F821 """Load a ContextEngine instance by name; None if not found or it fails to load.""" - engine_dir = _CONTEXT_ENGINE_PLUGINS_DIR / name - if not engine_dir.is_dir(): - logger.debug("Context engine '%s' not found in %s", name, _CONTEXT_ENGINE_PLUGINS_DIR) + engine_dir = find_engine_dir(name) + if engine_dir is None: + logger.debug("Context engine '%s' not found in bundled or user plugins", name) return None return _loader.load_named( name, engine_dir, _load_engine_from_dir, kind="Context engine", noun="engine", logger=logger @@ -37,9 +72,11 @@ def _load_engine_from_dir(engine_dir: Path) -> Optional["ContextEngine"]: # noq """Import an engine module and extract its ContextEngine (register(ctx) or subclass).""" from agent.context_engine import ContextEngine name = engine_dir.name + is_bundled = engine_dir.parent == _CONTEXT_ENGINE_PLUGINS_DIR + module_name = f"plugins.context_engine.{name}" if is_bundled else f"{_USER_NAMESPACE}.{name}" mod = _loader.load_plugin_module( - f"plugins.context_engine.{name}", engine_dir, - parents=("plugins", "plugins.context_engine"), logger=logger) + module_name, engine_dir, parents=("plugins", "plugins.context_engine"), logger=logger, + synthetic_namespace=None if is_bundled else _USER_NAMESPACE) return mod and _loader.instance_from_module( mod, collector=_EngineCollector(engine_name=name), collected_attr="engine", base_cls=ContextEngine, name=name, logger=logger) diff --git a/tests/plugins/test_context_engine_user_dir.py b/tests/plugins/test_context_engine_user_dir.py new file mode 100644 index 0000000000..1206cd277b --- /dev/null +++ b/tests/plugins/test_context_engine_user_dir.py @@ -0,0 +1,44 @@ +"""``context.engine: `` names the active engine, so an engine dropped into +``$HERMES_HOME/plugins//`` loads without a ``plugins.enabled`` entry and never trips the +"Context engine 'X' not found — falling back to built-in compressor" warning (#61839).""" + +import logging +from pathlib import Path +from textwrap import dedent + +_ENGINE_SRC = dedent(''' + from agent.context_engine import ContextEngine + + class Demo(ContextEngine): + @property + def name(self): + return "ctx_demo" + def update_from_response(self, usage): + pass + def should_compress(self, prompt_tokens=None): + return False + def compress(self, messages, current_tokens=None): + return messages + + def register(ctx): + ctx.register_context_engine(Demo()) +''') + + +def test_user_installed_engine_is_selected_by_name(tmp_path: Path, monkeypatch, caplog): + engine_dir = tmp_path / "plugins" / "ctx_demo" + engine_dir.mkdir(parents=True) + (engine_dir / "__init__.py").write_text(_ENGINE_SRC) + (tmp_path / "plugins" / "notes").mkdir() # unrelated user plugin: never treated as an engine + (tmp_path / "plugins" / "notes" / "__init__.py").write_text("def register(ctx): pass\n") + monkeypatch.setenv("HERMES_HOME", str(tmp_path)) + + from agent.agent_init import _select_context_engine + from plugins.context_engine import discover_context_engines, load_context_engine + + assert load_context_engine("notes") is None + assert "ctx_demo" in {name for name, _, _ in discover_context_engines()} + with caplog.at_level(logging.WARNING, logger="run_agent"): + engine = _select_context_engine({"context": {"engine": "ctx_demo"}}) + assert engine is not None and engine.name == "ctx_demo" + assert "not found" not in caplog.text diff --git a/website/docs/developer-guide/context-engine-plugin.md b/website/docs/developer-guide/context-engine-plugin.md index 6403818a25..d124ce46d9 100644 --- a/website/docs/developer-guide/context-engine-plugin.md +++ b/website/docs/developer-guide/context-engine-plugin.md @@ -191,7 +191,7 @@ Engine tools are injected into the agent's tool list at startup and dispatched a ### Via directory (recommended) -Place your engine in `plugins/context_engine//`. The `__init__.py` must export a `ContextEngine` subclass. The discovery system finds and instantiates it automatically. +Place your engine in `plugins/context_engine//` (bundled) or `~/.hermes/plugins//` (user-installed; `$HERMES_HOME/plugins//`). The `__init__.py` must export a `ContextEngine` subclass or a `register(ctx)` that calls `ctx.register_context_engine(...)`. Setting `context.engine: ` is the activation — a user-installed engine does not need a `plugins.enabled` entry. Bundled names win on collision. ### Via general plugin system From 592163d299edee9443dc145a5be97bec009a4bdd Mon Sep 17 00:00:00 2001 From: liuhao1024 Date: Tue, 8 Sep 2026 22:29:26 +0800 Subject: [PATCH 023/173] fix(plugins): route execution middleware through lazy discovery (#105827) `_run_execution_chain` read `get_plugin_manager()._middleware` directly, bypassing the lazy discovery every other delivery entry point gained in the #64178 parity work, so `tool_execution`/`llm_execution` middleware registered by user plugins silently failed to fire (fail-open) on surfaces that never run discovery at startup: query mode `chat -q`/`-z`, cron delivery, dashboard, TUI slash workers. Route it through `_delivery_manager()` like invoke_hook/invoke_middleware/has_middleware. (cherry picked from commit 06b9d44095733f8e46f40f0331b3f6dff352e98e) Fixes #105827 Salvages #105832 --- hermes_cli/middleware.py | 4 +-- tests/hermes_cli/test_plugins.py | 43 ++++++++++++++++++++++++++++++++ 2 files changed, 45 insertions(+), 2 deletions(-) diff --git a/hermes_cli/middleware.py b/hermes_cli/middleware.py index e5d42c3a2c..1c6ed2ae24 100644 --- a/hermes_cli/middleware.py +++ b/hermes_cli/middleware.py @@ -159,10 +159,10 @@ class _DownstreamExecutionError(Exception): def _run_execution_chain(kind: str, terminal_call: Callable[[Any], Any], **kwargs: Any) -> Any: - from hermes_cli.plugins import get_plugin_manager + from hermes_cli.plugins import _delivery_manager payload_key = "request" if "request" in kwargs else "args" - manager = get_plugin_manager() + manager = _delivery_manager() callbacks = list(manager._middleware.get(kind, [])) if not callbacks: return terminal_call(kwargs[payload_key]) diff --git a/tests/hermes_cli/test_plugins.py b/tests/hermes_cli/test_plugins.py index c28a26abcc..6df94835a4 100644 --- a/tests/hermes_cli/test_plugins.py +++ b/tests/hermes_cli/test_plugins.py @@ -31,6 +31,7 @@ from hermes_cli.middleware import ( VALID_MIDDLEWARE, apply_llm_request_middleware, apply_tool_request_middleware, + run_llm_execution_middleware, run_tool_execution_middleware, ) @@ -1018,6 +1019,48 @@ class TestDeliveryParity: assert plugins_mod.invoke_hook("anything") == ["stubbed"] + def test_execution_chain_lazily_discovers(self, monkeypatch): + """Execution middleware must fire on cold surfaces too (#105827). + + ``run_tool_execution_middleware`` / ``run_llm_execution_middleware`` deliver via + ``_run_execution_chain``, which used to read ``get_plugin_manager()._middleware`` + directly — no lazy discovery — so a registered fail-closed policy gate was silently + skipped (fail-open) on surfaces that never ran discovery at startup (query mode + ``chat -q``, cron delivery, dashboard, TUI slash workers). The chain must route + through ``_delivery_manager()`` like every other delivery entry point (#64178). + """ + fired = [] + terminal_calls = [] + + def _tool_gate(**kw): + fired.append(kw.get("tool_name")) + return {"denied": True} # fail-closed: returns without calling next_call + + def _llm_gate(**kw): + fired.append("llm") + return {"denied": True} + + def _register(m): + m._middleware.setdefault("tool_execution", []).append(_tool_gate) + m._middleware.setdefault("llm_execution", []).append(_llm_gate) + + mgr = self._fresh_manager(monkeypatch, _register) + + def _terminal(payload): + terminal_calls.append(payload) + return "terminal-ran" + + tool_result = run_tool_execution_middleware("terminal", {"path": "x"}, _terminal) + assert mgr._discovered is True, "execution chain must lazily discover on cold surfaces" + assert fired == ["terminal"] + assert tool_result == {"denied": True} + assert terminal_calls == [], "a fail-closed gate must not be bypassed by terminal execution" + + llm_result = run_llm_execution_middleware({"messages": []}, _terminal) + assert fired == ["terminal", "llm"] + assert llm_result == {"denied": True} + assert terminal_calls == [] + class TestAsyncHookCallbacks: """``async def`` hook callbacks run and their values land in the results (#12449).""" From 89de6083171d0f70e5356bef05105231344e371d Mon Sep 17 00:00:00 2001 From: KoNit-K Date: Sun, 13 Sep 2026 12:57:15 +0800 Subject: [PATCH 024/173] fix(plugins): fail closed on policy hook exceptions A `pre_tool_call` guard that raised failed OPEN (only `_report_hook_failure`, results stayed empty, the tool ran) while one that timed out failed CLOSED with a block directive. For a veto hook a crashing guard is the control quietly disappearing; make both failure modes of `_HOOK_TIMEOUT_FAIL_CLOSED_HOOKS` consistent: an exception appends a block directive in addition to the warning. Fixes #109624 Salvages #109632 (cherry picked from commit 6ccf5d8a4082fc6b9af80270aae9f13b0320cfb8) --- hermes_cli/plugins_dispatch.py | 2 ++ tests/hermes_cli/test_plugins.py | 36 ++++++++++++++++++++++++++++++++ 2 files changed, 38 insertions(+) diff --git a/hermes_cli/plugins_dispatch.py b/hermes_cli/plugins_dispatch.py index 55b469fdb5..7aa7cb34c0 100644 --- a/hermes_cli/plugins_dispatch.py +++ b/hermes_cli/plugins_dispatch.py @@ -220,6 +220,8 @@ class PluginDispatchMixin: results.append(ret) except (Exception, SystemExit) as exc: self._report_hook_failure(hook_name, cb, kwargs, exc) + if fail_closed: + results.append({"action": "block", "message": _PRE_TOOL_CALL_TIMEOUT_BLOCK_MESSAGE}) return results def _report_hook_failure( diff --git a/tests/hermes_cli/test_plugins.py b/tests/hermes_cli/test_plugins.py index 6df94835a4..05b0ef5321 100644 --- a/tests/hermes_cli/test_plugins.py +++ b/tests/hermes_cli/test_plugins.py @@ -1268,6 +1268,42 @@ class TestForceReloadSymmetry: assert mgr.invoke_hook("post_tool_call") == ["survived"] assert "bounded plugin requested process exit" in caplog.text + def test_pre_tool_call_direct_callback_exception_fails_closed(self, monkeypatch): + """A synchronous policy callback error must block rather than allow the tool.""" + from hermes_cli.plugins import _PRE_TOOL_CALL_TIMEOUT_BLOCK_MESSAGE + + monkeypatch.setattr( + "hermes_cli.plugins._resolve_hook_callback_timeout", lambda: 0.0 + ) + + def boom(**_kwargs): + raise RuntimeError("policy plugin blew up") + + mgr = PluginManager() + mgr._hooks["pre_tool_call"] = [boom] + + assert mgr.invoke_hook("pre_tool_call", tool_name="terminal", args={}) == [ + {"action": "block", "message": _PRE_TOOL_CALL_TIMEOUT_BLOCK_MESSAGE} + ] + + def test_pre_tool_call_timeout_worker_exception_fails_closed(self, monkeypatch): + """A bounded policy callback error must use the same fail-closed directive.""" + from hermes_cli.plugins import _PRE_TOOL_CALL_TIMEOUT_BLOCK_MESSAGE + + monkeypatch.setattr( + "hermes_cli.plugins._resolve_hook_callback_timeout", lambda: 1.0 + ) + + def boom(**_kwargs): + raise RuntimeError("policy plugin blew up") + + mgr = PluginManager() + mgr._hooks["pre_tool_call"] = [boom] + + assert mgr.invoke_hook("pre_tool_call", tool_name="terminal", args={}) == [ + {"action": "block", "message": _PRE_TOOL_CALL_TIMEOUT_BLOCK_MESSAGE} + ] + def test_hook_callback_timeout_reads_config(self, tmp_path, monkeypatch): hermes_home = tmp_path / "hermes_test" hermes_home.mkdir(parents=True, exist_ok=True) From 32518f024f5b7506253b18d5dd6178f27f0d9313 Mon Sep 17 00:00:00 2001 From: teknium1 <127238744+teknium1@users.noreply.github.com> Date: Mon, 21 Sep 2026 22:36:48 -0700 Subject: [PATCH 025/173] fix(plugins): name the callback and error in the raise-fail-closed block Follow-up trim of the #109632 salvage: the block directive a raising `pre_tool_call` guard produces reused the timeout wording, so an operator could not tell a crashing guard from a slow one from the tool result. Build it from the callback name and `TypeError: ...` (error text truncated like `_report_hook_failure`). The two salvaged tests collapse into one parametrized invariant (caller-thread and bounded-worker path) that also pins the message shape and that a sibling callback's result still flows. Part of #109624 Salvages #109632 --- hermes_cli/plugins_dispatch.py | 13 +++++++++-- tests/hermes_cli/test_plugins.py | 37 +++++++++----------------------- 2 files changed, 21 insertions(+), 29 deletions(-) diff --git a/hermes_cli/plugins_dispatch.py b/hermes_cli/plugins_dispatch.py index 7aa7cb34c0..e49b3cef26 100644 --- a/hermes_cli/plugins_dispatch.py +++ b/hermes_cli/plugins_dispatch.py @@ -52,6 +52,15 @@ _HOOK_CALLER_THREAD_HOOKS: Set[str] = {"subagent_stop"} _HOOK_TIMEOUT_SUPPRESSION_SECONDS = 60.0 _PRE_TOOL_CALL_TIMEOUT_BLOCK_MESSAGE = "pre_tool_call plugin callback timed out or is still running" + +def _policy_error_block_directive(hook_name: str, cb: Callable, exc: BaseException) -> Dict[str, str]: + """Block directive for a fail-closed hook whose callback raised: names the callback and the + error (truncated — a hook that embeds tool args in its exception must not grow the tool + result) so the operator can tell a crashing guard from a slow one.""" + callback_name = getattr(cb, "__name__", repr(cb)) + return {"action": "block", + "message": f"{hook_name} plugin callback {callback_name} raised {type(exc).__name__}: {str(exc)[:200]}"} + # System-prompt sections are tightly bounded: they become high-trust prompt bytes charged every turn. SYSTEM_PROMPT_SECTION_POSITIONS = frozenset({"after_memory"}) DEFAULT_SYSTEM_PROMPT_SECTION_MAX_CHARS = 4_000 @@ -220,8 +229,8 @@ class PluginDispatchMixin: results.append(ret) except (Exception, SystemExit) as exc: self._report_hook_failure(hook_name, cb, kwargs, exc) - if fail_closed: - results.append({"action": "block", "message": _PRE_TOOL_CALL_TIMEOUT_BLOCK_MESSAGE}) + if fail_closed: # a guard that raised made no decision: same veto as a timeout + results.append(_policy_error_block_directive(hook_name, cb, exc)) return results def _report_hook_failure( diff --git a/tests/hermes_cli/test_plugins.py b/tests/hermes_cli/test_plugins.py index 05b0ef5321..3455b75ae0 100644 --- a/tests/hermes_cli/test_plugins.py +++ b/tests/hermes_cli/test_plugins.py @@ -1268,41 +1268,24 @@ class TestForceReloadSymmetry: assert mgr.invoke_hook("post_tool_call") == ["survived"] assert "bounded plugin requested process exit" in caplog.text - def test_pre_tool_call_direct_callback_exception_fails_closed(self, monkeypatch): - """A synchronous policy callback error must block rather than allow the tool.""" - from hermes_cli.plugins import _PRE_TOOL_CALL_TIMEOUT_BLOCK_MESSAGE - + @pytest.mark.parametrize("timeout", [0.0, 1.0], ids=["caller-thread", "bounded-worker"]) + def test_pre_tool_call_callback_exception_fails_closed(self, monkeypatch, timeout): + """A policy callback that raises made no decision: it must block like a timeout does + (#109624), on both the caller-thread and the bounded-worker path, and the block message + names the callback and the error so a crashing guard is distinguishable from a slow one.""" monkeypatch.setattr( - "hermes_cli.plugins._resolve_hook_callback_timeout", lambda: 0.0 + "hermes_cli.plugins._resolve_hook_callback_timeout", lambda: timeout ) def boom(**_kwargs): raise RuntimeError("policy plugin blew up") mgr = PluginManager() - mgr._hooks["pre_tool_call"] = [boom] + mgr._hooks["pre_tool_call"] = [boom, lambda **_kw: {"action": "approve"}] - assert mgr.invoke_hook("pre_tool_call", tool_name="terminal", args={}) == [ - {"action": "block", "message": _PRE_TOOL_CALL_TIMEOUT_BLOCK_MESSAGE} - ] - - def test_pre_tool_call_timeout_worker_exception_fails_closed(self, monkeypatch): - """A bounded policy callback error must use the same fail-closed directive.""" - from hermes_cli.plugins import _PRE_TOOL_CALL_TIMEOUT_BLOCK_MESSAGE - - monkeypatch.setattr( - "hermes_cli.plugins._resolve_hook_callback_timeout", lambda: 1.0 - ) - - def boom(**_kwargs): - raise RuntimeError("policy plugin blew up") - - mgr = PluginManager() - mgr._hooks["pre_tool_call"] = [boom] - - assert mgr.invoke_hook("pre_tool_call", tool_name="terminal", args={}) == [ - {"action": "block", "message": _PRE_TOOL_CALL_TIMEOUT_BLOCK_MESSAGE} - ] + results = mgr.invoke_hook("pre_tool_call", tool_name="terminal", args={}) + assert [r.get("action") for r in results] == ["block", "approve"] + assert "boom" in results[0]["message"] and "RuntimeError: policy plugin blew up" in results[0]["message"] def test_hook_callback_timeout_reads_config(self, tmp_path, monkeypatch): hermes_home = tmp_path / "hermes_test" From 37555209167d622699f6375e02f9eb5d87e2a93c Mon Sep 17 00:00:00 2001 From: teknium1 <127238744+teknium1@users.noreply.github.com> Date: Mon, 21 Sep 2026 22:39:33 -0700 Subject: [PATCH 026/173] fix(plugins): a hung pre_tool_call guard no longer blocks every tool call until restart MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `_run_hook_callback_bounded` treated any live abandoned worker for a callback (`bool(self._hook_abandoned.get(suppression_key))`) as "still running", so one never-returning `pre_tool_call` callback made every later tool call fail closed with the timeout message until the process restarted. The timeout path self-heals through the 60s suppression window; the abandoned path never did. Policy now: while the suppression window is open the callback is skipped as before. After it expires a fresh call id may start a new worker even though the abandoned one is still alive — capped at `_HOOK_MAX_ABANDONED_WORKERS` (3) live abandoned workers per callback so a hung plugin cannot leak a thread per call (the #98382 constraint). At the cap the callback keeps being skipped (fail-closed for pre_tool_call) with a WARNING naming the callback and its module, until one of its workers finishes and frees a slot. Test changes: `test_hung_worker_blocks_new_call_identity_after_suppression` encoded the removed behaviour (exactly one worker, forever); it becomes `test_hung_worker_caps_new_call_identities_after_suppression`, which pins the same invariant it was protecting — bounded leak, never one per call — at the new bound and checks the warning. `test_hung_worker_does_not_fail_closed_forever` is the #105223 regression (red on base: call-c returned the block directive). Redone slim against the per-call-id gate that landed in #111177; #105241 targeted the pre-#111177 shape and needed plugins_ledger/__init__ changes for a one-retry-then-quarantine policy. Its analysis and shape informed this fix. Fixes #105223 Supersedes #105241 Co-authored-by: fangliquanflq --- hermes_cli/plugins_dispatch.py | 27 ++++++++---- tests/hermes_cli/test_plugins.py | 51 ++++++++++++++++++++--- website/docs/user-guide/features/hooks.md | 2 +- 3 files changed, 66 insertions(+), 14 deletions(-) diff --git a/hermes_cli/plugins_dispatch.py b/hermes_cli/plugins_dispatch.py index e49b3cef26..837dcb3fc5 100644 --- a/hermes_cli/plugins_dispatch.py +++ b/hermes_cli/plugins_dispatch.py @@ -50,6 +50,8 @@ _HOOK_TIMEOUT_FAIL_CLOSED_HOOKS: Set[str] = {"pre_tool_call"} _HOOK_CALLER_THREAD_HOOKS: Set[str] = {"subagent_stop"} # After a timeout, suppress the same callback this long so a hung hook cannot pile up threads. _HOOK_TIMEOUT_SUPPRESSION_SECONDS = 60.0 +# Live workers a hung callback may accumulate before it is skipped outright (#105223 / #98382). +_HOOK_MAX_ABANDONED_WORKERS = 3 _PRE_TOOL_CALL_TIMEOUT_BLOCK_MESSAGE = "pre_tool_call plugin callback timed out or is still running" @@ -261,8 +263,9 @@ class PluginDispatchMixin: self, hook_name: str, cb: Callable, kwargs: Dict[str, Any], timeout: float ) -> Any: """Run one callback on a daemon worker with a wall-clock cap; ``_HOOK_SKIPPED`` when - suppressed, still running, timed out (worker abandoned, never joined), or the worker - could not be started. Exceptions propagate.""" + suppressed, still running for this call id, over the abandoned-worker cap, timed out + (worker abandoned, never joined), or the worker could not be started. Exceptions + propagate.""" callback_name = getattr(cb, "__name__", repr(cb)) # Suppression is a fact about the CALLBACK — a hung one must keep its back-off — # so that key stays coarse. The gate must instead tell CONCURRENT CALLS apart. @@ -271,15 +274,25 @@ class PluginDispatchMixin: token = object() with self._hook_timeout_lock: suppressed_until = self._hook_timeout_suppressed_until.get(suppression_key) - # A worker abandoned on timeout still holds a thread; a fresh call id must not - # start a second one for the same callback, or a hung plugin leaks a thread per call. - running = (gate_key in self._hook_running_callbacks - or bool(self._hook_abandoned.get(suppression_key))) - if (suppressed_until is not None and suppressed_until > time.monotonic()) or running: + if (gate_key in self._hook_running_callbacks + or (suppressed_until is not None and suppressed_until > time.monotonic())): logger.warning( "Hook '%s' callback %s skipped after previous " "timeout or while still running", hook_name, callback_name) return _HOOK_SKIPPED + # Workers abandoned on timeout still hold threads. Once the suppression window has + # passed, a fresh call id may start a new worker (a hung guard must not fail every + # later tool call closed until restart, #105223), but only up to a small cap per + # callback — expiring the bookkeeping while the hung worker lives must not leak a + # thread per call (#98382). At the cap the callback keeps being skipped (fail-closed + # for pre_tool_call) until one of its workers finishes and releases its slot. + abandoned = self._hook_abandoned.get(suppression_key) + if abandoned and len(abandoned) >= _HOOK_MAX_ABANDONED_WORKERS: + logger.warning( + "Hook '%s' callback %s (%s) skipped: %d abandoned worker(s) still running — " + "the plugin is hung; fix or disable it (retried when a worker finishes)", + hook_name, callback_name, getattr(cb, "__module__", "unknown plugin"), len(abandoned)) + return _HOOK_SKIPPED if suppressed_until is not None: self._hook_timeout_suppressed_until.pop(suppression_key, None) self._hook_running_callbacks[gate_key] = token diff --git a/tests/hermes_cli/test_plugins.py b/tests/hermes_cli/test_plugins.py index 3455b75ae0..5440f40a14 100644 --- a/tests/hermes_cli/test_plugins.py +++ b/tests/hermes_cli/test_plugins.py @@ -1469,9 +1469,14 @@ class TestForceReloadSymmetry: hold.set() first.join(5.0) - def test_hung_worker_blocks_new_call_identity_after_suppression(self, monkeypatch): - """A worker abandoned on timeout still occupies its callback: a later call with a - fresh id must be skipped, not given a second thread (one leak, not one per call).""" + def test_hung_worker_caps_new_call_identities_after_suppression(self, monkeypatch, caplog): + """Workers abandoned on timeout still occupy their callback: once the suppression window + has passed, later calls with fresh ids may start a replacement, but only up to + ``_HOOK_MAX_ABANDONED_WORKERS`` live ones — a hung plugin leaks a bounded few threads, + never one per call (#98382), and past the cap it is skipped with a warning that names + the callback (#105223).""" + import hermes_cli.plugins_dispatch as dispatch + monkeypatch.setattr( "hermes_cli.plugins._resolve_hook_callback_timeout", lambda: 0.1 ) @@ -1488,10 +1493,44 @@ class TestForceReloadSymmetry: mgr._hook_timeout_suppression_seconds = 0.0 # isolate the gate from suppression mgr._hooks["post_tool_call"] = [blocker] - assert mgr.invoke_hook("post_tool_call", tool_name="read_file", tool_call_id="call-a") == [] - assert mgr.invoke_hook("post_tool_call", tool_name="read_file", tool_call_id="call-b") == [] + with caplog.at_level(logging.WARNING, logger="hermes_cli.plugins"): + for i in range(dispatch._HOOK_MAX_ABANDONED_WORKERS + 3): + assert mgr.invoke_hook("post_tool_call", tool_name="read_file", tool_call_id=f"call-{i}") == [] - assert len(starts) == 1 + assert len(starts) == dispatch._HOOK_MAX_ABANDONED_WORKERS + assert "blocker" in caplog.text and "abandoned worker(s) still running" in caplog.text + hold.set() + + def test_hung_worker_does_not_fail_closed_forever(self, monkeypatch): + """One never-returning pre_tool_call guard must not block every later tool call until + restart: after the suppression window a fresh call id runs a new worker, so a callback + that has recovered decides again (#105223).""" + import time + + from hermes_cli.plugins import _PRE_TOOL_CALL_TIMEOUT_BLOCK_MESSAGE + + monkeypatch.setattr( + "hermes_cli.plugins._resolve_hook_callback_timeout", lambda: 0.1 + ) + hold = threading.Event() + starts = [] + + def guard(**_kwargs): + starts.append(1) + if len(starts) == 1: + hold.wait(timeout=10.0) # the first fire hangs for good + return None # later fires decide: allow + + mgr = PluginManager() + mgr._hook_timeout_suppression_seconds = 0.2 + mgr._hooks["pre_tool_call"] = [guard] + + blocked = [{"action": "block", "message": _PRE_TOOL_CALL_TIMEOUT_BLOCK_MESSAGE}] + assert mgr.invoke_hook("pre_tool_call", tool_name="read_file", tool_call_id="call-a") == blocked + assert mgr.invoke_hook("pre_tool_call", tool_name="read_file", tool_call_id="call-b") == blocked # in window + time.sleep(0.3) # suppression window passes; the first worker is still hung + assert mgr.invoke_hook("pre_tool_call", tool_name="read_file", tool_call_id="call-c") == [] + assert len(starts) == 2 hold.set() def test_worker_finishing_at_timeout_does_not_leave_phantom_abandoned_entry(self, monkeypatch): diff --git a/website/docs/user-guide/features/hooks.md b/website/docs/user-guide/features/hooks.md index a3d235f332..d29dce466b 100644 --- a/website/docs/user-guide/features/hooks.md +++ b/website/docs/user-guide/features/hooks.md @@ -572,7 +572,7 @@ Shell hooks also accept the Claude Code-compatible format: Both formats are normalized internally to `{"action": "modify", "args": {...}}`. -If a `pre_tool_call` callback exceeds `plugins.hook_callback_timeout` (or is still running from a previous timed-out fire), Hermes **fails closed**: the tool is blocked with a timeout message rather than proceeding without a policy decision. +If a `pre_tool_call` callback exceeds `plugins.hook_callback_timeout` (or is still running from a previous timed-out fire), Hermes **fails closed**: the tool is blocked with a timeout message rather than proceeding without a policy decision. The same applies to a callback that raises: the block message names the callback and the error. A hung callback is skipped for a 60s suppression window; after that a new tool call runs it again (up to three abandoned workers per callback, so a permanently hung plugin blocks tool calls with a warning naming it instead of silently wedging the agent until restart). **Use cases:** Logging, audit trails, tool call counters, blocking dangerous operations, rate limiting, per-user policy enforcement, argument sanitization, path rewriting, injecting default parameters. From 02ad41df3a33a5913c05d35ac33abbcc4376a570 Mon Sep 17 00:00:00 2001 From: teknium1 <127238744+teknium1@users.noreply.github.com> Date: Mon, 21 Sep 2026 22:42:16 -0700 Subject: [PATCH 027/173] fix(plugins): a pre_tool_call block outranks an earlier plugin's approve `_get_pre_tool_call_directive_details` returned the first valid block-or-approve in registration order, so a plugin registered earlier that returned `approve` hid a later security plugin's `block`; under `approvals.mode: off` an approve means no prompt at all, so the veto was dropped silently. Precedence is now `block` > `approve` > none: a valid block still returns immediately (modify directives seen before it stay attached, as before), a valid approve is held back until the whole result list has been scanned for a veto, and among approves the first valid one (with its rule_key) still wins. Modify accumulation is unchanged and now also keeps modify directives that follow the winning approve, since the scan no longer stops there. Docstring and hooks.md no longer describe "first valid directive wins". Slim redo of #68644 (earliest) and #87449 against the modify-aware shape of the function on main; both PRs predate it and could not be cherry-picked. Fixes #87420 Supersedes #68644 Supersedes #87449 Co-authored-by: synscott <1563043+synscott@users.noreply.github.com> Co-authored-by: Jack Lau <72348727+jackulau@users.noreply.github.com> --- hermes_cli/plugins.py | 19 ++++++++---- tests/hermes_cli/test_plugins.py | 35 +++++++++++++++++++++++ website/docs/user-guide/features/hooks.md | 6 ++-- 3 files changed, 52 insertions(+), 8 deletions(-) diff --git a/hermes_cli/plugins.py b/hermes_cli/plugins.py index 23dff081af..529ad7f61f 100644 --- a/hermes_cli/plugins.py +++ b/hermes_cli/plugins.py @@ -1810,8 +1810,10 @@ def _get_pre_tool_call_directive_details( ) -> _PreToolCallDirective: """Check ``pre_tool_call`` hooks for ``{"action": "block", "message"}`` (veto; message becomes the tool result) or ``{"action": "approve", "message", "rule_key"?}`` (escalate ANY tool to the - human-approval gate; ``rule_key`` picks the ``[a]lways`` allowlist grain). First valid directive - wins; irrelevant returns are ignored.""" + human-approval gate; ``rule_key`` picks the ``[a]lways`` allowlist grain). Precedence is + ``block`` > ``approve`` > none, not registration order: any plugin's valid veto wins over an + earlier plugin's request for human confirmation (#87420); among approves the first valid one + wins. Irrelevant returns are ignored.""" allowed = getattr(_thread_tool_whitelist, "allowed", None) if allowed is not None and tool_name not in allowed: fmt = getattr(_thread_tool_whitelist, "fmt", "Tool '{tool_name}' denied") @@ -1823,6 +1825,7 @@ def _get_pre_tool_call_directive_details( api_request_id=api_request_id, middleware_trace=list(middleware_trace or []), ) modified_args: Optional[Dict[str, Any]] = None + first_approve: Optional[Tuple[Optional[str], Optional[str]]] = None # (message, rule_key) for result in hook_results: if not isinstance(result, dict): continue @@ -1843,9 +1846,15 @@ def _get_pre_tool_call_directive_details( # A block directive requires a message (it becomes the tool result); approve's is optional. if action == "block" and not message: continue - rule_key = result.get("rule_key") if action == "approve" else None - rule_key = (rule_key.strip() or None) if isinstance(rule_key, str) else None - return _PreToolCallDirective(action=action, message=message, rule_key=rule_key, modified_args=modified_args) + if action == "block": + return _PreToolCallDirective(action="block", message=message, modified_args=modified_args) + # approve is held back until the whole list has been scanned for a veto. + if first_approve is None: + rule_key = result.get("rule_key") + first_approve = (message, (rule_key.strip() or None) if isinstance(rule_key, str) else None) + if first_approve is not None: + return _PreToolCallDirective(action="approve", message=first_approve[0], rule_key=first_approve[1], + modified_args=modified_args) return _PreToolCallDirective(modified_args=modified_args) diff --git a/tests/hermes_cli/test_plugins.py b/tests/hermes_cli/test_plugins.py index 5440f40a14..2711512344 100644 --- a/tests/hermes_cli/test_plugins.py +++ b/tests/hermes_cli/test_plugins.py @@ -1808,6 +1808,41 @@ class TestPreToolCallDirective: ) assert get_pre_tool_call_directive("write_file", {}) == ("approve", None) + def test_later_block_outranks_earlier_approve(self, monkeypatch): + """Precedence is block > approve, not registration order: a security plugin's veto must + not be shadowed by an earlier plugin's approve (#87420). Under approvals.mode off an + approve means no prompt at all, so the veto would otherwise be dropped silently.""" + from hermes_cli.plugins import _get_pre_tool_call_directive_details + monkeypatch.setattr( + "hermes_cli.plugins.invoke_hook", + lambda hook_name, **kwargs: [ + {"action": "modify", "args": {"path": "/safe"}}, + {"action": "approve", "message": "earlier plugin approves", "rule_key": "k"}, + {"action": "block", "message": "later security plugin blocks"}, + ], + ) + details = _get_pre_tool_call_directive_details("write_file", {"path": "/unsafe"}) + assert (details.action, details.message, details.rule_key) == ( + "block", "later security plugin blocks", None) + assert details.modified_args == {"path": "/safe"} # modify before the veto stays visible + + def test_first_approve_wins_among_approves_and_keeps_later_modify(self, monkeypatch): + """Holding approve back for a veto scan must not change which approve wins (first valid, + incl. its rule_key) and must keep accumulating modify directives that follow it.""" + from hermes_cli.plugins import _get_pre_tool_call_directive_details + monkeypatch.setattr( + "hermes_cli.plugins.invoke_hook", + lambda hook_name, **kwargs: [ + {"action": "block"}, # message-less block is invalid and ignored + {"action": "approve", "message": "first", "rule_key": " write_file:ssh "}, + {"action": "modify", "args": {"content": "fixed"}}, + {"action": "approve", "message": "second", "rule_key": "write_file:other"}, + ], + ) + details = _get_pre_tool_call_directive_details("write_file", {"path": "/p"}) + assert (details.action, details.message, details.rule_key) == ("approve", "first", "write_file:ssh") + assert details.modified_args == {"path": "/p", "content": "fixed"} + class TestResolvePreToolBlock: """Tests for the single dispatch-site chokepoint that resolves a diff --git a/website/docs/user-guide/features/hooks.md b/website/docs/user-guide/features/hooks.md index d29dce466b..7303ef3f03 100644 --- a/website/docs/user-guide/features/hooks.md +++ b/website/docs/user-guide/features/hooks.md @@ -438,7 +438,7 @@ Payload fields below are the exact event-specific fields supplied by each call s | Hook | Category | Exact timing and return behavior | Explicit payload fields | Privacy / sensitivity | |---|---|---|---|---| -| [`pre_tool_call`](#pre_tool_call) | Directive/control | Once before execution; first valid `block` or `approve` directive wins, and `modify` returns are shallow-merged into the tool arguments. | `tool_name`, `args`, `task_id`, `session_id`, `tool_call_id`, `turn_id`, `api_request_id`, `middleware_trace` | Raw arguments may contain user content, paths, commands, or secrets. | +| [`pre_tool_call`](#pre_tool_call) | Directive/control | Once before execution; any valid `block` wins over any `approve` (then the first valid `approve`), and `modify` returns are shallow-merged into the tool arguments. | `tool_name`, `args`, `task_id`, `session_id`, `tool_call_id`, `turn_id`, `api_request_id`, `middleware_trace` | Raw arguments may contain user content, paths, commands, or secrets. | | `post_tool_call` | Observer | After blocked, error, or successful result; return ignored. | `tool_name`, `args`, `result`, `task_id`, `session_id`, `tool_call_id`, `turn_id`, `api_request_id`, `duration_ms`, `status`, `error_type`, `error_message`, `middleware_trace` | Result/error text may contain arbitrary tool or user content and secrets. | | `transform_tool_result` | Transform | After `post_tool_call`, before conversation append; first string replaces the result. | `tool_name`, `args`, `result`, `task_id`, `session_id`, `tool_call_id`, `turn_id`, `api_request_id`, `duration_ms`, `status`, `error_type`, `error_message` | Exposes the full model-bound result and arguments. | | `transform_terminal_output` | Transform | After bounded foreground process capture, before final output limiting; first string replaces output. | `command`, `output`, `returncode`, `task_id`, `env_type` | Command/output may contain credentials. | @@ -554,7 +554,7 @@ return {"action": "block", "message": "Reason the tool call was blocked"} return {"action": "approve", "message": "Why approval is required", "rule_key": "optional:scope"} ``` -The first valid directive wins (Python plugins registered first, then shell hooks). `block` requires a non-empty `message` and short-circuits the tool with that text as the error returned to the model. `approve` escalates the call to the existing human-approval gate; `message` and `rule_key` are optional, and denial, timeout, or gate error fails closed. Other return values are ignored, so existing observer-only callbacks keep working unchanged. +Precedence is `block` > `approve` > no directive, regardless of registration order: any plugin's valid `block` wins over an earlier plugin's `approve`, and among `approve` directives the first valid one wins (Python plugins are registered first, then shell hooks). `block` requires a non-empty `message` and short-circuits the tool with that text as the error returned to the model. `approve` escalates the call to the existing human-approval gate; `message` and `rule_key` are optional, and denial, timeout, or gate error fails closed. Other return values are ignored, so existing observer-only callbacks keep working unchanged. **Return value — rewrite the tool's arguments:** @@ -1919,7 +1919,7 @@ Shell hooks run with **your full user credentials** — same trust boundary as a ### Ordering and precedence -Both Python plugin hooks and shell hooks flow through the same `invoke_hook()` dispatcher. Python plugins are registered first (`discover_and_load()`), shell hooks second (`register_from_config()`), so Python `pre_tool_call` block decisions take precedence in tie cases. The first valid block wins — the aggregator returns as soon as any callback produces `{"action": "block", "message": str}` with a non-empty message. +Both Python plugin hooks and shell hooks flow through the same `invoke_hook()` dispatcher. Python plugins are registered first (`discover_and_load()`), shell hooks second (`register_from_config()`), so Python `pre_tool_call` decisions take precedence in tie cases. The first valid block wins — the aggregator returns as soon as any callback produces `{"action": "block", "message": str}` with a non-empty message — and a block anywhere in the list outranks an `approve` returned earlier. ## Outbound Webhooks From 30b38d139b1fd91670e3b5bd88cd50f71287a12f Mon Sep 17 00:00:00 2001 From: teknium1 <127238744+teknium1@users.noreply.github.com> Date: Mon, 21 Sep 2026 22:44:06 -0700 Subject: [PATCH 028/173] fix(hooks): shell pre_tool_call hooks can escalate to the approval gate with "approve" MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `agent/shell_hooks.py::_parse_pre_tool_call` translated only the block and modify dialects, so a shell hook printing the documented `{"action": "approve", ...}` parsed to None and the tool ran with no approval prompt — silently, with exit 0, valid JSON and `hermes hooks doctor` green. The Python-plugin side already accepts approve and routes it through `_resolve_block_from_details` → `request_tool_approval`; the shell parser now yields the same `{"action": "approve", "message"?, "rule_key"?}` shape (optional fields kept only as non-empty stripped strings), so `hermes hooks test` prints it under `parsed:` and the dispatcher escalates it. the `decision` dialect's `{"decision": "approve"}` means auto-ALLOW, not "ask a human", so it is deliberately not mapped; that dialect has no top-level ask dialect to mirror. Slim redo with credit: #92562 (earliest) bundled a larger policy-authority rework; #110325 carried the same parser change plus an unrelated rule_key default change and 10+ tests. Fixes #92553 Supersedes #92562 Supersedes #110325 Co-authored-by: fangliquanflq --- agent/shell_hooks.py | 9 ++++++ tests/agent/test_shell_hooks.py | 39 ++++++++++++++++++++++- website/docs/user-guide/features/hooks.md | 4 +++ 3 files changed, 51 insertions(+), 1 deletion(-) diff --git a/agent/shell_hooks.py b/agent/shell_hooks.py index 424d2d9e6d..ca7ac6d6b0 100644 --- a/agent/shell_hooks.py +++ b/agent/shell_hooks.py @@ -445,6 +445,15 @@ def _parse_pre_tool_call(data: Dict[str, Any]) -> Optional[Dict[str, Any]]: for verb, _, _, payload in _PRE_TOOL_DIALECTS: if data.get(verb) == "modify" and isinstance(data.get(payload), dict): return {"action": "modify", "args": data[payload]} + # Hermes-only escalation to the human-approval gate (#92553). Claude-Code's ``decision: + # approve`` means auto-ALLOW, so it is deliberately not mapped onto this. + if data.get("action") == "approve": + directive: Dict[str, Any] = {"action": "approve"} + for key in ("message", "rule_key"): + value = data.get(key) + if isinstance(value, str) and value.strip(): + directive[key] = value.strip() + return directive return None diff --git a/tests/agent/test_shell_hooks.py b/tests/agent/test_shell_hooks.py index ab376c5b8a..e2238b448a 100644 --- a/tests/agent/test_shell_hooks.py +++ b/tests/agent/test_shell_hooks.py @@ -49,7 +49,18 @@ class TestParseResponse: ) assert r == {"action": "block", "message": "nope"} - + @pytest.mark.parametrize("stdout, expected", [ + ('{"action": "approve", "message": " needs a human ", "rule_key": " terminal:rm "}', + {"action": "approve", "message": "needs a human", "rule_key": "terminal:rm"}), + ('{"action": "approve", "message": "", "rule_key": 7}', {"action": "approve"}), + # Claude-Code's ``decision: approve`` means auto-ALLOW, not "ask a human": never mapped. + ('{"decision": "approve", "reason": "ok"}', None), + ('{"action": "approve", "decision": "block", "reason": "no"}', {"action": "block", "message": "no"}), + ]) + def test_approve_is_parsed_like_the_plugin_directive(self, stdout, expected): + """The documented ``approve`` action used to parse to None, so the tool ran with no + approval prompt (#92553). It now yields the same shape Python plugins return.""" + assert shell_hooks._parse_response("pre_tool_call", stdout) == expected def test_empty_stdout_returns_none(self): assert shell_hooks._parse_response("pre_tool_call", "") is None @@ -201,6 +212,32 @@ class TestCallbackSubprocess: ) assert msg == "blocked-by-shell" + def test_approve_reaches_the_human_gate_through_plugin_manager(self, tmp_path, monkeypatch): + """End to end: a shell hook's approve directive escalates to request_tool_approval with its + message and rule_key, and the gate's denial blocks the tool (#92553).""" + from hermes_cli import plugins + + script = _write_script( + tmp_path, "approve.sh", + "#!/usr/bin/env bash\n" + 'printf \'{"action": "approve", "message": "risky", "rule_key": "terminal:rm"}\\n\'\n', + ) + monkeypatch.setenv("HERMES_HOME", str(tmp_path / "home")) + monkeypatch.setenv("HERMES_ACCEPT_HOOKS", "1") + plugins._plugin_manager = plugins.PluginManager() + cfg = {"hooks": {"pre_tool_call": [{"matcher": "terminal", "command": str(script)}]}} + assert len(shell_hooks.register_from_config(cfg, accept_hooks=True)) == 1 + + seen = [] + + def _gate(tool_name, reason, **kwargs): + seen.append((tool_name, reason, kwargs.get("rule_key"))) + return {"approved": False, "message": "denied by human"} + + monkeypatch.setattr("tools.approval.request_tool_approval", _gate) + assert plugins.resolve_pre_tool_block("terminal", {"command": "rm"}) == "denied by human" + assert seen == [("terminal", "risky", "terminal:rm")] + def test_matcher_regex_filters_callback(self, tmp_path, monkeypatch): """A matcher set to 'terminal' must not fire for 'web_search'.""" calls = tmp_path / "calls.log" diff --git a/website/docs/user-guide/features/hooks.md b/website/docs/user-guide/features/hooks.md index 7303ef3f03..dba7b42013 100644 --- a/website/docs/user-guide/features/hooks.md +++ b/website/docs/user-guide/features/hooks.md @@ -1729,6 +1729,10 @@ profile's `HERMES_HOME`. `tool_name` and `tool_input` are `null` for non-tool ev {"action": "modify", "args": {"new_string": "fixed content"}} // Hermes-canonical {"decision": "modify", "tool_input": {"new_string": "fixed content"}} // Claude-Code style +// Escalate a pre_tool_call to the human-approval gate (Hermes-only; `message` and `rule_key` +// are optional). Claude-Code's `{"decision": "approve"}` means auto-allow and is NOT mapped here: +{"action": "approve", "message": "Why approval is required", "rule_key": "optional:scope"} + // Inject context for pre_llm_call: {"context": "Today is Friday, 2026-04-17"} From 727e7342cf2003d27ad9fde328018573c1a8b99e Mon Sep 17 00:00:00 2001 From: Todd Dailey <251794714+twidtwid@users.noreply.github.com> Date: Sun, 13 Sep 2026 12:36:03 -0700 Subject: [PATCH 029/173] fix(gateway): await async pre_gateway_dispatch callbacks on the event loop `GatewayInboundMixin._hm_pre_gateway_dispatch_hook` was a plain `def` calling the sync `hermes_cli.lifecycle.invoke_hook` from the async `_hm_admit_event`, so an `async def pre_gateway_dispatch` callback was resolved through `resolve_plugin_command_result` on a helper thread with its own loop: the gateway loop blocked for the callback's whole duration and any loop-bound await (an `asyncio.Event` set by a loop task, a loop-bound aiohttp session, `asyncio.to_thread`) could never complete, failing at 30s. Add `PluginManager.ainvoke_hook` (+ `hermes_cli.plugins.ainvoke_hook` / `hermes_cli.lifecycle.ainvoke_hook`): same payload narrowing (shared `_hook_callback_kwargs`), observer + isolation semantics and result contract as `invoke_hook`, but awaitable results are awaited on the caller's loop. `pre_gateway_dispatch` stays intentionally unbounded. The inbound hook becomes `async def` and `_hm_admit_event` awaits it; the sync `invoke_hook` is untouched for every other caller. Existing tests that stubbed the hook synchronously are adapted to the async seam. Fixes #110241 Salvages #110265 (cherry picked from commit 22bb10d305c7992f43bbfdf1481ad0ef0015b2b7) --- gateway/run_inbound.py | 8 +-- hermes_cli/lifecycle.py | 9 +++ hermes_cli/plugins.py | 6 ++ hermes_cli/plugins_dispatch.py | 65 ++++++++++++++++++---- tests/gateway/test_bot_loop_guard.py | 12 +++- tests/gateway/test_pre_gateway_dispatch.py | 39 ++++++++++++- tests/hermes_cli/test_plugins.py | 51 +++++++++++++++++ 7 files changed, 169 insertions(+), 21 deletions(-) diff --git a/gateway/run_inbound.py b/gateway/run_inbound.py index 4ca82c962c..72e126098c 100644 --- a/gateway/run_inbound.py +++ b/gateway/run_inbound.py @@ -63,15 +63,15 @@ def strip_discord_triggering_note(event: Any, message_text: Any) -> Any: class GatewayInboundMixin: """Inbound message pipeline (_handle_message, text/media preparation, durable-turn markers, plugin injection) for GatewayRunner.""" - def _hm_pre_gateway_dispatch_hook( + async def _hm_pre_gateway_dispatch_hook( self, event: "MessageEvent", source: SessionSource ) -> Optional["MessageEvent"]: """Run the ``pre_gateway_dispatch`` plugin hook; None = drop, else the (maybe rewritten) event. Results: ``{"action": "skip"}`` → drop; ``{"action": "rewrite", "text"}`` → replace ``event.text``; ``allow``/None → normal dispatch. Runs BEFORE auth so plugins can handle unauthorized senders.""" try: - from hermes_cli.lifecycle import invoke_hook as _invoke_hook - _hook_results = _invoke_hook( + from hermes_cli.lifecycle import ainvoke_hook as _ainvoke_hook + _hook_results = await _ainvoke_hook( "pre_gateway_dispatch", event=event, gateway=self, # getattr: bare-runner tests build GatewayRunner via object.__new__ without __init__. session_store=getattr(self, "session_store", None), @@ -222,7 +222,7 @@ class GatewayInboundMixin: # scale-to-zero: only real user-originated inbound stamps the last-inbound clock; # counting internal/system events would keep a genuinely idle gateway awake. self._scale_to_zero_note_real_inbound() - event = self._hm_pre_gateway_dispatch_hook(event, source) + event = await self._hm_pre_gateway_dispatch_hook(event, source) if event is None: return None source = event.source diff --git a/hermes_cli/lifecycle.py b/hermes_cli/lifecycle.py index bdd11a7a97..4ea636a1e5 100644 --- a/hermes_cli/lifecycle.py +++ b/hermes_cli/lifecycle.py @@ -29,6 +29,15 @@ def invoke_hook(hook_name: str, **kwargs: Any) -> List[Any]: return _plugin_hooks(hook_name, **kwargs) +async def ainvoke_hook(hook_name: str, **kwargs: Any) -> List[Any]: + """:func:`invoke_hook` for callers on an event loop: same observers-then-plugins + composition, with ``async def`` plugin callbacks awaited on that loop.""" + _observe(hook_name, **kwargs) + from hermes_cli import plugins + + return await plugins.ainvoke_hook(hook_name, **kwargs) + + def has_hook(hook_name: str) -> bool: """Return whether a first-party observer or plugin consumes a hook.""" try: diff --git a/hermes_cli/plugins.py b/hermes_cli/plugins.py index 529ad7f61f..342fde3c6c 100644 --- a/hermes_cli/plugins.py +++ b/hermes_cli/plugins.py @@ -1716,6 +1716,12 @@ def invoke_hook(hook_name: str, **kwargs: Any) -> List[Any]: return _delivery_manager().invoke_hook(hook_name, **kwargs) +async def ainvoke_hook(hook_name: str, **kwargs: Any) -> List[Any]: + """:func:`invoke_hook` for callers on an event loop: ``async def`` callbacks are awaited + there instead of bridged through a helper thread (see ``PluginManager.ainvoke_hook``).""" + return await _delivery_manager().ainvoke_hook(hook_name, **kwargs) + + def render_system_prompt_sections(session_info: Mapping[str, Any]) -> List[RenderedPluginSystemPromptSection]: """Render plugin prompt sections after idempotent plugin discovery.""" return _ensure_plugins_discovered().render_system_prompt_sections(session_info) diff --git a/hermes_cli/plugins_dispatch.py b/hermes_cli/plugins_dispatch.py index 837dcb3fc5..be1950cfe0 100644 --- a/hermes_cli/plugins_dispatch.py +++ b/hermes_cli/plugins_dispatch.py @@ -6,6 +6,7 @@ the origin (tests patch it there) and is looked up lazily. from __future__ import annotations +import asyncio import contextvars import copy import inspect @@ -179,7 +180,23 @@ def _hook_uses_callback_timeout(hook_name: str, timeout: float) -> bool: class PluginDispatchMixin: @staticmethod - def _invoke_hook_callback(callback: Callable, payload: Dict[str, Any]) -> Any: + def _hook_callback_kwargs(callback: Callable, payload: Dict[str, Any]) -> Dict[str, Any]: + """The slice of *payload* a callback accepts: everything for ``**kwargs`` (or + un-introspectable) callbacks, only declared names for narrow legacy signatures.""" + try: + parameters = inspect.signature(callback).parameters + except (TypeError, ValueError): + return dict(payload) # no introspectable signature + if any(p.kind == inspect.Parameter.VAR_KEYWORD for p in parameters.values()): + return dict(payload) + keyword_kinds = {inspect.Parameter.POSITIONAL_OR_KEYWORD, inspect.Parameter.KEYWORD_ONLY} + return { + name: value for name, value in payload.items() + if name in parameters and parameters[name].kind in keyword_kinds + } + + @classmethod + def _invoke_hook_callback(cls, callback: Callable, payload: Dict[str, Any]) -> Any: """Invoke a hook while withholding additive fields from narrow legacy callbacks. An ``async def`` callback returns a coroutine; resolve it the way plugin slash commands @@ -187,17 +204,7 @@ class PluginDispatchMixin: plugin's body never runs (#12449). """ from hermes_cli.plugins import resolve_plugin_command_result - try: - parameters = inspect.signature(callback).parameters - except (TypeError, ValueError): - return resolve_plugin_command_result(callback(**payload)) # no introspectable signature - if any(p.kind == inspect.Parameter.VAR_KEYWORD for p in parameters.values()): - return resolve_plugin_command_result(callback(**payload)) - keyword_kinds = {inspect.Parameter.POSITIONAL_OR_KEYWORD, inspect.Parameter.KEYWORD_ONLY} - return resolve_plugin_command_result(callback(**{ - name: value for name, value in payload.items() - if name in parameters and parameters[name].kind in keyword_kinds - })) + return resolve_plugin_command_result(callback(**cls._hook_callback_kwargs(callback, payload))) def invoke_hook(self, hook_name: str, **kwargs: Any) -> List[Any]: """Call all callbacks for *hook_name*; return their non-``None`` results. @@ -473,6 +480,40 @@ class PluginDispatchMixin: """Return True when at least one callback is registered for a hook.""" return bool(self._hooks.get(hook_name)) + async def ainvoke_hook(self, hook_name: str, **kwargs: Any) -> List[Any]: + """:meth:`invoke_hook` for callers that are already on an event loop. + + Same payload narrowing, per-callback isolation and result contract. The difference is + where an ``async def`` callback runs: here it is awaited on the caller's own loop, so a + callback that awaits anything scheduled on that loop can make progress. Through the + sync path it runs on a helper thread while the caller blocks in ``done.wait()`` — on the + gateway that stalls the whole event loop for the callback's duration. Sync callbacks + run inline. Bounded hooks keep ``plugins.hook_callback_timeout`` via ``asyncio.wait_for`` + (the coroutine is cancelled, not abandoned); a timed-out ``pre_tool_call`` fails closed. + """ + from hermes_cli.plugins import _resolve_hook_callback_timeout + if hook_name != "gateway_platform_event": + kwargs.setdefault("telemetry_schema_version", OBSERVER_SCHEMA_VERSION) + results: List[Any] = [] + timeout = _resolve_hook_callback_timeout() + use_timeout = _hook_uses_callback_timeout(hook_name, timeout) + fail_closed = hook_name in _HOOK_TIMEOUT_FAIL_CLOSED_HOOKS + for cb in self._hooks.get(hook_name, []): + callback_name = getattr(cb, "__name__", repr(cb)) + try: + ret = cb(**self._hook_callback_kwargs(cb, kwargs)) + if inspect.isawaitable(ret): + ret = await (asyncio.wait_for(ret, timeout) if use_timeout else ret) + if ret is not None: + results.append(ret) + except asyncio.TimeoutError: + logger.warning("Hook '%s' callback %s timed out after %.0fs", hook_name, callback_name, timeout) + if fail_closed: # policy hook: fail closed with a block directive + results.append({"action": "block", "message": _PRE_TOOL_CALL_TIMEOUT_BLOCK_MESSAGE}) + except Exception as exc: + logger.warning("Hook '%s' callback %s raised: %s", hook_name, callback_name, exc) + return results + def iter_hook_callbacks(self, hook_name: str) -> tuple[Callable, ...]: """Return a stable snapshot of callbacks registered for a hook.""" return tuple(self._hooks.get(hook_name, ())) diff --git a/tests/gateway/test_bot_loop_guard.py b/tests/gateway/test_bot_loop_guard.py index 8b9052bf83..91f4111e66 100644 --- a/tests/gateway/test_bot_loop_guard.py +++ b/tests/gateway/test_bot_loop_guard.py @@ -97,7 +97,9 @@ async def test_ingress_gate_counts_an_authorized_bot_once_and_drops_it_when_refu runner = object.__new__(GatewayRunner) runner._scale_to_zero_note_real_inbound = lambda: None - runner._hm_pre_gateway_dispatch_hook = lambda event, source: event + async def _passthrough_hook(event, source): # the inbound path awaits the hook + return event + runner._hm_pre_gateway_dispatch_hook = _passthrough_hook runner._is_user_authorized_for_source = lambda source, **kw: True admitted = [] runner._admit_bot_message = lambda source: admitted.append(source.user_id) or source.user_id != BOT_B @@ -136,7 +138,9 @@ async def test_busy_path_counts_a_bot_message_once_before_steering(monkeypatch, assert steer.await_count == 1 runner._scale_to_zero_note_real_inbound = lambda: None - runner._hm_pre_gateway_dispatch_hook = lambda event, source: event + async def _passthrough_hook(event, source): # the inbound path awaits the hook + return event + runner._hm_pre_gateway_dispatch_hook = _passthrough_hook runner._is_user_authorized_for_source = lambda source, **kw: True runner._admit_bot_message = lambda source: pytest.fail("the busy path already charged this event") assert (await runner._hm_admit_event(events[0]))[0] is events[0] @@ -158,7 +162,9 @@ async def test_routed_bot_traffic_is_metered_by_the_transport_profiles_policy(tm runner._principal_authorized = lambda *a, **kw: True runner._adapter_profile_for_source = lambda source: "transport" runner._scale_to_zero_note_real_inbound = lambda: None - runner._hm_pre_gateway_dispatch_hook = lambda event, source: event + async def _passthrough_hook(event, source): # the inbound path awaits the hook + return event + runner._hm_pre_gateway_dispatch_hook = _passthrough_hook def _routed_bot(i: int) -> MessageEvent: source = _bot(BOT_A) diff --git a/tests/gateway/test_pre_gateway_dispatch.py b/tests/gateway/test_pre_gateway_dispatch.py index 7c6ccf5e21..3a2243eed9 100644 --- a/tests/gateway/test_pre_gateway_dispatch.py +++ b/tests/gateway/test_pre_gateway_dispatch.py @@ -102,13 +102,14 @@ async def test_hook_fires_without_session_store_attribute(monkeypatch): seen = {} - def _fake_hook(name, **kwargs): + async def _fake_hook(name, **kwargs): if name == "pre_gateway_dispatch": seen["session_store"] = kwargs.get("session_store", "MISSING") return [{"action": "skip", "reason": "plugin-handled"}] return [] - monkeypatch.setattr("hermes_cli.plugins.invoke_hook", _fake_hook) + # The inbound path awaits the hook, so the seam is the async entry point. + monkeypatch.setattr("hermes_cli.plugins.ainvoke_hook", _fake_hook) runner, adapter = _make_runner(Platform.WHATSAPP) del runner.session_store @@ -118,3 +119,37 @@ async def test_hook_fires_without_session_store_attribute(monkeypatch): # Hook actually fired (skip short-circuited before auth) with a None store. assert seen == {"session_store": None} adapter.send.assert_not_awaited() + + +@pytest.mark.asyncio +async def test_async_hook_callback_is_awaited_on_the_gateway_loop(monkeypatch): + """An ``async def`` pre_gateway_dispatch callback is awaited on the gateway's own loop. + + Regression: the inbound path called the sync ``invoke_hook``, which (since #109196) runs an + async callback on a helper thread while the calling loop blocks in ``done.wait()``. A + callback that awaits anything scheduled on the gateway loop could never complete, and every + message stalled the loop for the callback's whole duration. Here the callback waits for a + sibling task on the same loop to release it; that is only possible if the hook is awaited + in place. + """ + import asyncio + + _clear_auth_env(monkeypatch) + gate = asyncio.Event() + + async def _hook(name, **kwargs): + assert name == "pre_gateway_dispatch" + await gate.wait() + return [{"action": "skip", "reason": "gated"}] + + monkeypatch.setattr("hermes_cli.plugins.ainvoke_hook", _hook) + + async def _release(): + await asyncio.sleep(0) + gate.set() + + runner, adapter = _make_runner(Platform.WHATSAPP) + asyncio.create_task(_release()) + result = await asyncio.wait_for(runner._handle_message(_make_event("hi")), timeout=5) + assert result is None + adapter.send.assert_not_awaited() diff --git a/tests/hermes_cli/test_plugins.py b/tests/hermes_cli/test_plugins.py index 2711512344..fe5d394919 100644 --- a/tests/hermes_cli/test_plugins.py +++ b/tests/hermes_cli/test_plugins.py @@ -2876,3 +2876,54 @@ class TestDispatchToolWithoutCliRef: assert calls[0][1].get("parent_agent") is None finally: registry.deregister("_test_dispatch_probe") + + +class TestAsyncHookOnCallerLoop: + """``ainvoke_hook`` awaits ``async def`` callbacks on the caller's own event loop. + + #109196 made async callbacks run under ``invoke_hook`` by bridging them through a helper + thread; the caller blocks in ``done.wait()`` until the callback finishes. For a hook fired + from a coroutine (``pre_gateway_dispatch`` on the gateway loop) that stalls the loop, and a + callback that awaits anything scheduled on that loop can never complete. The async twin keeps + the callback on the caller's loop. + """ + + def test_callback_that_needs_the_caller_loop_completes(self): + import asyncio + + mgr = PluginManager() + + async def driver(): + gate = asyncio.Event() + + async def async_hook(**kwargs): + await gate.wait() # only a sibling task on THIS loop can release it + return {"action": "allow"} + + async def release(): + await asyncio.sleep(0) + gate.set() + + mgr._hooks.setdefault("pre_gateway_dispatch", []).append(async_hook) + asyncio.create_task(release()) + return await asyncio.wait_for( + mgr.ainvoke_hook("pre_gateway_dispatch", event="e", gateway="g"), timeout=5) + + assert asyncio.run(driver()) == [{"action": "allow"}] + + def test_narrow_legacy_signature_still_gets_only_its_fields(self): + """Payload narrowing is shared with ``invoke_hook``: a callback declaring only ``event`` + must not receive the additive ``gateway`` / ``telemetry_schema_version`` fields.""" + import asyncio + + mgr = PluginManager() + + def narrow(event): + return {"seen": event} + + async def narrow_async(event): + return {"seen_async": event} + + mgr._hooks.setdefault("pre_gateway_dispatch", []).extend([narrow, narrow_async]) + results = asyncio.run(mgr.ainvoke_hook("pre_gateway_dispatch", event="e", gateway="g")) + assert results == [{"seen": "e"}, {"seen_async": "e"}] From 75e9567ca757a427ca4bd2ae12add9b53846dc06 Mon Sep 17 00:00:00 2001 From: teknium1 <127238744+teknium1@users.noreply.github.com> Date: Mon, 21 Sep 2026 22:46:43 -0700 Subject: [PATCH 030/173] fix(plugins): ainvoke_hook shares the sync path's failure contract MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Follow-up trim of the #110265 salvage. `ainvoke_hook` logged raising callbacks with a bare warning; route them through `_report_hook_failure` (warn-once per distinct failure, #111922) and, for `_HOOK_TIMEOUT_FAIL_CLOSED_HOOKS`, append the same named block directive the sync path emits (#109624), so the async twin cannot drift into a fail-open policy path. Tests trimmed to the salvage bar: the in-loop await is proven once through the real `_handle_message` path (`test_async_hook_callback_is_awaited_on_the_gateway_loop`); the manager-level duplicate is dropped and the narrowing test also pins failure isolation. Docs: `pre_gateway_dispatch` callbacks may be `async def` and stay unbounded. Credit order for the three PRs fixing this gap: #102485 (dmspark, earliest, pre-decomposition `gateway/run.py`), #110253 (KoNit-K, bounded the hook — rejected by design: neither fail mode is acceptable for a policy gate), #110265 (twidtwid, reporter; cherry-picked because it matches the ainvoke_hook shape, keeps the hook unbounded, and adapts the existing sync test seams honestly). Part of #110241 Supersedes #102485 Supersedes #110253 Co-authored-by: David Marcus Co-authored-by: KoNit-K --- hermes_cli/plugins_dispatch.py | 8 +++-- tests/hermes_cli/test_plugins.py | 40 +++++++---------------- website/docs/user-guide/features/hooks.md | 2 ++ 3 files changed, 20 insertions(+), 30 deletions(-) diff --git a/hermes_cli/plugins_dispatch.py b/hermes_cli/plugins_dispatch.py index be1950cfe0..d1f1df19ed 100644 --- a/hermes_cli/plugins_dispatch.py +++ b/hermes_cli/plugins_dispatch.py @@ -510,8 +510,12 @@ class PluginDispatchMixin: logger.warning("Hook '%s' callback %s timed out after %.0fs", hook_name, callback_name, timeout) if fail_closed: # policy hook: fail closed with a block directive results.append({"action": "block", "message": _PRE_TOOL_CALL_TIMEOUT_BLOCK_MESSAGE}) - except Exception as exc: - logger.warning("Hook '%s' callback %s raised: %s", hook_name, callback_name, exc) + except (Exception, SystemExit) as exc: + # Same isolation + failure contract as the sync path (#111922 warn-once, #109624 + # a raising policy guard fails closed). + self._report_hook_failure(hook_name, cb, kwargs, exc) + if fail_closed: + results.append(_policy_error_block_directive(hook_name, cb, exc)) return results def iter_hook_callbacks(self, hook_name: str) -> tuple[Callable, ...]: diff --git a/tests/hermes_cli/test_plugins.py b/tests/hermes_cli/test_plugins.py index fe5d394919..2e9c5bc146 100644 --- a/tests/hermes_cli/test_plugins.py +++ b/tests/hermes_cli/test_plugins.py @@ -2888,32 +2888,11 @@ class TestAsyncHookOnCallerLoop: the callback on the caller's loop. """ - def test_callback_that_needs_the_caller_loop_completes(self): - import asyncio - - mgr = PluginManager() - - async def driver(): - gate = asyncio.Event() - - async def async_hook(**kwargs): - await gate.wait() # only a sibling task on THIS loop can release it - return {"action": "allow"} - - async def release(): - await asyncio.sleep(0) - gate.set() - - mgr._hooks.setdefault("pre_gateway_dispatch", []).append(async_hook) - asyncio.create_task(release()) - return await asyncio.wait_for( - mgr.ainvoke_hook("pre_gateway_dispatch", event="e", gateway="g"), timeout=5) - - assert asyncio.run(driver()) == [{"action": "allow"}] - - def test_narrow_legacy_signature_still_gets_only_its_fields(self): - """Payload narrowing is shared with ``invoke_hook``: a callback declaring only ``event`` - must not receive the additive ``gateway`` / ``telemetry_schema_version`` fields.""" + def test_narrow_legacy_signature_still_gets_only_its_fields(self, caplog): + """Payload narrowing and failure isolation are shared with ``invoke_hook``: a callback + declaring only ``event`` must not receive the additive ``gateway`` / + ``telemetry_schema_version`` fields, and a raising callback is reported once and skipped + without losing its siblings' results.""" import asyncio mgr = PluginManager() @@ -2921,9 +2900,14 @@ class TestAsyncHookOnCallerLoop: def narrow(event): return {"seen": event} + async def boom(**_kw): + raise RuntimeError("async plugin blew up") + async def narrow_async(event): return {"seen_async": event} - mgr._hooks.setdefault("pre_gateway_dispatch", []).extend([narrow, narrow_async]) - results = asyncio.run(mgr.ainvoke_hook("pre_gateway_dispatch", event="e", gateway="g")) + mgr._hooks.setdefault("pre_gateway_dispatch", []).extend([narrow, boom, narrow_async]) + with caplog.at_level(logging.WARNING, logger="hermes_cli.plugins"): + results = asyncio.run(mgr.ainvoke_hook("pre_gateway_dispatch", event="e", gateway="g")) assert results == [{"seen": "e"}, {"seen_async": "e"}] + assert "async plugin blew up" in caplog.text diff --git a/website/docs/user-guide/features/hooks.md b/website/docs/user-guide/features/hooks.md index dba7b42013..f05a308db3 100644 --- a/website/docs/user-guide/features/hooks.md +++ b/website/docs/user-guide/features/hooks.md @@ -1214,6 +1214,8 @@ def my_callback(event, gateway, session_store, **kwargs): **Return value:** `None` or a dict. The first recognized action dict wins; remaining plugin results are ignored. Exceptions in plugin callbacks are caught and logged; the gateway always falls through to normal dispatch on error. +Callbacks may be `async def`: they are awaited on the gateway's own event loop, so awaiting loop-bound work (an `asyncio.Event`, an aiohttp session, `asyncio.to_thread`) makes progress and other inbound messages keep flowing while the callback runs. The hook is intentionally not bounded by `plugins.hook_callback_timeout` — dropping or passing a message on timeout are both wrong for a policy gate — so a callback that never returns holds up dispatch of that message. + | Return | Effect | |--------|--------| | `{"action": "skip", "reason": "..."}` | Drop the message — no agent reply, no pairing flow, no auth. Plugin is assumed to have handled it (e.g. silent-ingested into the transcript). | From 5943347a2aa986ba98569c8b46f2eacaba9847f2 Mon Sep 17 00:00:00 2001 From: chelsealong Date: Sat, 12 Sep 2026 00:50:02 +0000 Subject: [PATCH 031/173] fix(gateway): bind session context for plugin slash command handlers MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Plugin-registered slash commands were dispatched before HERMES_SESSION_* was bound, so a handler calling get_session_env("HERMES_SESSION_KEY") (or any other HERMES_SESSION_* var) always saw an empty value — the agent-turn path binds them via _set_session_env, but the plugin command path in _hm_dispatch_quick_and_plugin_commands never did. Factor the existing set/clear pair into a _session_env_scope context manager and wrap the plugin_handler() call with it, resolving session_key from the source since no session_entry exists yet at this point in dispatch. Fixes #108698. (cherry picked from commit 07c7ac47370aabed930c47161ad0c06d94c5a9d6) (cherry picked from commit cca11a9cb9cde2a895cbfd211746ee4db32a33e7) --- gateway/run.py | 11 ++++++++ gateway/run_inbound.py | 17 +++++++++--- tests/gateway/test_session_env.py | 43 +++++++++++++++++++++++++++++++ 3 files changed, 67 insertions(+), 4 deletions(-) diff --git a/gateway/run.py b/gateway/run.py index 0c75256565..62f69cf98d 100644 --- a/gateway/run.py +++ b/gateway/run.py @@ -4281,6 +4281,17 @@ class GatewayRunner( from gateway.session_context import clear_session_vars clear_session_vars(tokens) + @_contextmanager + def _session_env_scope(self, context: SessionContext): + """Bind session context variables for the duration of the block, e.g. a plugin command + handler invoked outside the normal agent-turn path (``_set_session_env`` is otherwise only + reached there). Always cleared on exit, including on exception.""" + tokens = self._set_session_env(context) + try: + yield + finally: + self._clear_session_env(tokens) + async def _run_in_executor_with_context(self, func, *args): """Run blocking work in the thread pool while preserving session contextvars.""" loop = asyncio.get_running_loop() diff --git a/gateway/run_inbound.py b/gateway/run_inbound.py index 72e126098c..b7e26d99cc 100644 --- a/gateway/run_inbound.py +++ b/gateway/run_inbound.py @@ -26,7 +26,8 @@ from gateway.run_inbound_unauthorized import ( unauthorized_owner_hint, ) from gateway.session import ( - SessionSource, is_shared_multi_user_session, neutralize_untrusted_inline_text + SessionSource, build_session_context, is_shared_multi_user_session, + neutralize_untrusted_inline_text, ) from gateway.turn_lease import TurnLeaseTimeoutError from typing import Any, Dict, List, Optional, Tuple @@ -1058,9 +1059,17 @@ class GatewayInboundMixin: from hermes_cli.plugins import get_plugin_command_handler plugin_handler = get_plugin_command_handler(command.replace("_", "-")) if plugin_handler: - result = plugin_handler(event.get_command_args().strip()) - if asyncio.iscoroutine(result): - result = await result + # The agent-turn path binds HERMES_SESSION_* via _set_session_env before running; + # this dispatch sits outside that path, so a handler reading get_session_env() + # (directly, or through code it calls: message delivery, cron, kanban, approvals) + # would otherwise see an empty/stale session (#108698). No session_entry exists + # yet here, so session_key is resolved from source instead of a persisted one. + _plugin_context = build_session_context(source, self.config) + _plugin_context.session_key = self._session_key_for_source(source) + with self._session_env_scope(_plugin_context): + result = plugin_handler(event.get_command_args().strip()) + if asyncio.iscoroutine(result): + result = await result return True, str(result) if result else None, command except Exception as e: logger.warning("Plugin command dispatch failed: %s", e) diff --git a/tests/gateway/test_session_env.py b/tests/gateway/test_session_env.py index e40a5266e5..8206d85f53 100644 --- a/tests/gateway/test_session_env.py +++ b/tests/gateway/test_session_env.py @@ -273,3 +273,46 @@ def test_cron_session_set_clear_and_reset_tristate(monkeypatch): reset_session_vars() assert get_session_env("HERMES_CRON_SESSION") == "1" + +@pytest.mark.asyncio +async def test_plugin_slash_command_sees_session_env(monkeypatch): + """A plugin-registered slash command handler must see the same HERMES_SESSION_* + contextvars an agent turn would for that event (#108698): the agent-turn path binds + them via _set_session_env before running, but plugin command dispatch is a separate, + earlier path that previously called the handler with nothing bound.""" + from gateway.config import GatewayConfig, PlatformConfig + from gateway.platforms.event import MessageEvent + + monkeypatch.delenv("HERMES_SESSION_KEY", raising=False) + monkeypatch.delenv("HERMES_SESSION_CHAT_ID", raising=False) + + runner = object.__new__(GatewayRunner) + runner.config = GatewayConfig(platforms={Platform.TELEGRAM: PlatformConfig(enabled=True, token="***")}) + runner._draining = False + + source = SessionSource( + platform=Platform.TELEGRAM, chat_id="c1", user_id="u1", user_name="tester", chat_type="dm", + ) + event = MessageEvent(text="/gsd bind", source=source, message_id="m1") + + seen = {} + + def _handler(raw_args): + seen["session_key"] = get_session_env("HERMES_SESSION_KEY") + seen["chat_id"] = get_session_env("HERMES_SESSION_CHAT_ID") + return f"Bound: {raw_args}" + + from hermes_cli import plugins as _plugins_mod + monkeypatch.setattr(_plugins_mod, "get_plugin_command_handler", + lambda name: _handler if name == "gsd-bind" else None) + + handled, result, command = await runner._hm_dispatch_quick_and_plugin_commands(event, source, "gsd_bind") + + assert handled is True + assert result == "Bound: bind" + assert seen["session_key"] == runner._session_key_for_source(source) + assert seen["session_key"] != "" + assert seen["chat_id"] == "c1" + # Bound only for the handler call, not leaked past dispatch + assert get_session_env("HERMES_SESSION_KEY") == "" + From e794bb31cb75d2dc8937f928e736fb73155e674c Mon Sep 17 00:00:00 2001 From: Konstantin Khlopkov Date: Mon, 7 Sep 2026 18:55:00 +0000 Subject: [PATCH 032/173] fix(gateway): run sync plugin command handlers off the event loop A synchronous plugin slash-command handler executed inline on the gateway event loop thread; blocking I/O inside it stalled shutdown_watchdog liveness probes and the gateway was hard-killed with exit 75. Sync handlers now run on the gateway pool, async handlers keep being awaited directly. Salvage note: the PR used asyncio.to_thread; resolved onto the session-env scope from #108703 using the existing GatewayRunner._run_in_executor_with_context helper (same seam the skill-slash path uses for #111091), so the HERMES_SESSION_* contextvars bound around the call travel with the handler onto the worker thread. Fixes #105279. (cherry picked from commit 69c52af43265b835a3b585ef59072a956d4b2262) (cherry picked from commit a82a6f20f9381c752821ab1971a2efa8a1b62bda) --- gateway/run_inbound.py | 21 ++++++++++------ tests/gateway/test_unknown_command.py | 35 +++++++++++++++++++++++++++ 2 files changed, 48 insertions(+), 8 deletions(-) diff --git a/gateway/run_inbound.py b/gateway/run_inbound.py index b7e26d99cc..614434de50 100644 --- a/gateway/run_inbound.py +++ b/gateway/run_inbound.py @@ -1059,17 +1059,22 @@ class GatewayInboundMixin: from hermes_cli.plugins import get_plugin_command_handler plugin_handler = get_plugin_command_handler(command.replace("_", "-")) if plugin_handler: - # The agent-turn path binds HERMES_SESSION_* via _set_session_env before running; - # this dispatch sits outside that path, so a handler reading get_session_env() - # (directly, or through code it calls: message delivery, cron, kanban, approvals) - # would otherwise see an empty/stale session (#108698). No session_entry exists - # yet here, so session_key is resolved from source instead of a persisted one. + # The agent-turn path binds HERMES_SESSION_* via _set_session_env; this dispatch + # sits before it, so a handler reading get_session_env() would see an empty or a + # foreign (cron agent's os.environ) session (#108698). No session_entry exists yet, + # so session_key is derived from source. Sync handlers run on the gateway pool + # (contextvars carried), never the loop thread: blocking I/O there starves the + # liveness watchdog and the process exits 75 mid-handler (#105279). _plugin_context = build_session_context(source, self.config) _plugin_context.session_key = self._session_key_for_source(source) + user_args = event.get_command_args().strip() with self._session_env_scope(_plugin_context): - result = plugin_handler(event.get_command_args().strip()) - if asyncio.iscoroutine(result): - result = await result + if asyncio.iscoroutinefunction(plugin_handler): + result = await plugin_handler(user_args) + else: + result = await self._run_in_executor_with_context(plugin_handler, user_args) + if asyncio.iscoroutine(result): + result = await result return True, str(result) if result else None, command except Exception as e: logger.warning("Plugin command dispatch failed: %s", e) diff --git a/tests/gateway/test_unknown_command.py b/tests/gateway/test_unknown_command.py index abe23da48c..e232e7a3c9 100644 --- a/tests/gateway/test_unknown_command.py +++ b/tests/gateway/test_unknown_command.py @@ -225,3 +225,38 @@ async def test_command_hook_rewrite_routes_to_plugin(monkeypatch): # First emit_collect fires on the original command; after rewrite the # dispatcher does NOT re-fire for the new command (one decision per turn). assert call_log == ["command:status"] + + +@pytest.mark.asyncio +async def test_sync_plugin_command_runs_off_loop_thread(monkeypatch): + """A sync plugin handler doing blocking I/O must run off the gateway loop thread, + else the loop stops answering shutdown_watchdog probes and is killed with exit 75.""" + import threading + import time + + import gateway.run as gateway_run + from hermes_cli import plugins as _plugins_mod + + runner = _make_runner() + seen_threads = [] + + def _blocking_handler(args: str) -> str: + seen_threads.append(threading.current_thread().name) + time.sleep(0.15) + return f"sync {args}" + + monkeypatch.setattr( + _plugins_mod, + "get_plugin_command_handler", + lambda name: _blocking_handler if name == "slow-api" else None, + ) + monkeypatch.setattr( + gateway_run, "_resolve_runtime_agent_kwargs", lambda: {"api_key": "***"} + ) + + result = await runner._handle_message(_make_event("/slow-api arg")) + + assert result == "sync arg" + assert seen_threads, "handler must have run" + # pytest-asyncio runs the loop on the main thread; the blocking handler must not. + assert seen_threads[0] != threading.main_thread().name From 188f0d4251d69fbafdab97396fc4bcfca84c254e Mon Sep 17 00:00:00 2001 From: thomasscottbeck-sudo <244846147+thomasscottbeck-sudo@users.noreply.github.com> Date: Fri, 18 Sep 2026 13:55:25 -0600 Subject: [PATCH 033/173] fix(plugins): fire transform_tool_result for agent-runtime tools MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Agent-runtime tools (todo_list, session_search, memory, clarify, delegate_task, the preview/terminal readers, context-engine and memory-provider tools) are dispatched inline and never reach handle_function_call, which is the only place transform_tool_result ran. A registered transform silently did nothing for them, although the hook is documented as applying to every tool. Apply the same helper on both runtime executor paths, after the terminal post_tool_call so the observer still sees the untransformed result: the concurrent path in invoke_tool's inline branch, and the sequential path in _publish_sequential_result. The sequential registry dispatch marks itself transform_applied so handle_function_call stays the single invocation for registry tools and nothing double-fires. The sequential result classification (failure detection and the logged result length) moves below the transform so it reads the result the model actually sees, matching the registry and concurrent paths. Co-Authored-By: Claude Fable 5.1 (cherry picked from commit bc31efff0a6e2fd8177edd73cfe1a7112df9f78d) Fixes #72836 Salvages #115397 (thomasscottbeck-sudo); supersedes #72860 (webtecnica, earliest — sequential path only). (cherry picked from commit d6b5e969b3e08a557a62ed85c8b828537857b3d8) --- agent/agent_runtime_helpers.py | 16 +++++--- agent/inline_tool_executors.py | 25 ++++++++++++ agent/tool_executor.py | 23 ++++++++--- tests/agent/test_run_agent.py | 70 ++++++++++++++++++++++++++++++++++ 4 files changed, 123 insertions(+), 11 deletions(-) diff --git a/agent/agent_runtime_helpers.py b/agent/agent_runtime_helpers.py index 74986d6a9d..5fb2dee9d6 100644 --- a/agent/agent_runtime_helpers.py +++ b/agent/agent_runtime_helpers.py @@ -2370,7 +2370,8 @@ def invoke_tool(agent, function_name: str, function_args: dict, effective_task_i no display logic. Used by the concurrent path; the sequential path keeps its own inline invocation for display.""" from agent.inline_tool_executors import ( - InlineToolContext, emit_terminal_post_tool_call, resolve_invoke_tool_executor, tool_hook_ids + InlineToolContext, apply_transform_tool_result, emit_terminal_post_tool_call, + resolve_invoke_tool_executor, tool_hook_ids ) if not isinstance(function_args, dict): function_args = {} @@ -2407,14 +2408,17 @@ def invoke_tool(agent, function_name: str, function_args: dict, effective_task_i def _execute(next_args: dict) -> Any: result = inline_executor(agent, next_args, inline_ctx) + call_args = next_args if isinstance(next_args, dict) else function_args + duration_ms = int((time.monotonic() - tool_start_time) * 1000) emit_terminal_post_tool_call( - agent, function_name=function_name, - function_args=next_args if isinstance(next_args, dict) else function_args, + agent, function_name=function_name, function_args=call_args, result=result, effective_task_id=effective_task_id, tool_call_id=tool_call_id, - duration_ms=int((time.monotonic() - tool_start_time) * 1000), - middleware_trace=_tool_middleware_trace, + duration_ms=duration_ms, middleware_trace=_tool_middleware_trace, + ) + return apply_transform_tool_result( + agent, function_name=function_name, function_args=call_args, result=result, + effective_task_id=effective_task_id, tool_call_id=tool_call_id, duration_ms=duration_ms, ) - return result else: def _execute(next_args: dict) -> Any: dispatch_kwargs = dict( diff --git a/agent/inline_tool_executors.py b/agent/inline_tool_executors.py index 10d22c7d9a..76a675e8f5 100644 --- a/agent/inline_tool_executors.py +++ b/agent/inline_tool_executors.py @@ -58,6 +58,31 @@ def emit_terminal_post_tool_call( pass +def apply_transform_tool_result( + agent, + *, + function_name: str, + function_args: dict, + result: Any, + effective_task_id: str, + tool_call_id: Optional[str], + duration_ms: int = 0, +) -> Any: + """Apply ``transform_tool_result`` to an inline-dispatched tool's result. + + Registry tools get this inside ``handle_function_call``; inline executors never + reach it, so the agent paths call the same helper (after the terminal + ``post_tool_call``) to keep the hook's "every tool" contract. Fail-open.""" + try: + from model_tools import _CallIds, _apply_transform_tool_result_hook + return _apply_transform_tool_result_hook( + function_name, function_args, result, duration_ms, + _CallIds(**tool_hook_ids(agent, effective_task_id, tool_call_id)), + ) + except Exception: + return result + + @dataclass class InlineToolContext: """Per-call state an inline executor may need beyond its arguments.""" diff --git a/agent/tool_executor.py b/agent/tool_executor.py index f6f84dcf11..26779b0107 100644 --- a/agent/tool_executor.py +++ b/agent/tool_executor.py @@ -33,6 +33,7 @@ from agent.message_sanitization import coalesce_tool_call_id from agent.inline_tool_executors import ( INLINE_TOOL_EXECUTORS, InlineToolContext, + apply_transform_tool_result, emit_terminal_post_tool_call, tool_hook_ids, ) @@ -1592,6 +1593,7 @@ class _SequentialDispatch: is_delegate: bool = False finish_spinner: bool = True finish_in_finally: bool = True # inline tools print their completion line only on success + transform_applied: bool = False # True when execute already fired transform_tool_result def _resolve_sequential_dispatch(agent, ref: _ToolCallRef, messages: list) -> _SequentialDispatch: @@ -1656,6 +1658,7 @@ def _resolve_sequential_dispatch(agent, ref: _ToolCallRef, messages: list) -> _S error_log="handle_function_call raised for %s: %s", handles_keyboard_interrupt=True, finish_spinner=bool(agent.quiet_mode), + transform_applied=True, # handle_function_call fires transform_tool_result itself ) @@ -1726,19 +1729,28 @@ def _run_sequential_call( return managed, tool_duration -def _publish_sequential_result(agent, messages: list, ref: _ToolCallRef, managed: _ManagedToolResult, *, tool_duration: float, index: int, budget: BudgetConfig) -> bool: +def _publish_sequential_result(agent, messages: list, ref: _ToolCallRef, managed: _ManagedToolResult, *, tool_duration: float, index: int, budget: BudgetConfig, transform_applied: bool) -> bool: """Terminal hook → observe → commit → completion callbacks/print for one sequential result; False when the incremental flush failed (the caller must stop the batch).""" ref.args, ref.trace, function_result = managed.args, managed.middleware_trace, managed.result _execution_timed_out = isinstance(function_result, (_ToolTimeoutResult, _ToolCancelledResult)) - # Multimodal dict results (_multimodal=True) are not sliceable as strings. - _result_len = len(function_result) if isinstance(function_result, str) else len(str(function_result)) - _is_error_result, _ = _detect_tool_failure(ref.name, function_result) # Inline-dispatched runtime tools never reach handle_function_call, so the # executor owns the one terminal post_tool_call per tool_call_id (the inner # observer is suppressed); also stops an abandoned timeout worker reporting late. + # transform_tool_result follows the observer, unless the dispatch already fired it. if not managed.blocked and not _execution_timed_out: ref.emit_post(agent, function_result, duration_ms=int(tool_duration * 1000)) + if not transform_applied: + function_result = apply_transform_tool_result( + agent, function_name=ref.name, function_args=ref.args, result=function_result, + effective_task_id=ref.task_id, tool_call_id=ref.call_id, + duration_ms=int(tool_duration * 1000), + ) + # Classify the result the model will actually see, i.e. after any transform; the + # registry and concurrent paths both classify post-transform. + # Multimodal dict results (_multimodal=True) are not sliceable as strings. + _result_len = len(function_result) if isinstance(function_result, str) else len(str(function_result)) + _is_error_result, _ = _detect_tool_failure(ref.name, function_result) committed = _commit_tool_result( agent, messages, ref, function_result, budget=budget, tool_duration=tool_duration, is_error=_is_error_result, blocked=managed.blocked, @@ -1809,7 +1821,8 @@ def _execute_tool_calls_sequential(agent, assistant_message, messages: list, eff display_index=i, tool_start_time=tool_start_time, ) - if not _publish_sequential_result(agent, messages, ref, managed, tool_duration=tool_duration, index=i, budget=_tool_budget): + if not _publish_sequential_result(agent, messages, ref, managed, tool_duration=tool_duration, index=i, + budget=_tool_budget, transform_applied=dispatch.transform_applied): return if agent._interrupt_requested and i < len(tool_calls): diff --git a/tests/agent/test_run_agent.py b/tests/agent/test_run_agent.py index f3ae1534fc..c5f54eeeeb 100644 --- a/tests/agent/test_run_agent.py +++ b/tests/agent/test_run_agent.py @@ -2612,6 +2612,76 @@ class TestAgentRuntimePostHookOwnershipSync: } +class TestRuntimeToolTransformToolResult: + """A registered ``transform_tool_result`` replaces what the model sees for an + agent-runtime tool, on both the sequential and the concurrent executor path.""" + + @staticmethod + def _install_rewriting_transform(agent, monkeypatch): + monkeypatch.setattr( + "hermes_cli.plugins._dispatch_pre_tool_call_hooks", + lambda *args, **kwargs: (None, None), + ) + monkeypatch.setattr("hermes_cli.lifecycle.has_hook", lambda name: True) + monkeypatch.setattr( + "hermes_cli.lifecycle.invoke_hook", + lambda hook_name, **kwargs: ( + [f'REWRITTEN[{kwargs["tool_name"]}]{kwargs["result"]}'] + if hook_name == "transform_tool_result" + else [] + ), + ) + monkeypatch.setattr("tools.todo_tool.todo_tool", lambda **kwargs: '{"ok":true}') + agent._memory_manager = None + + def test_concurrent_path_applies_transform(self, agent, monkeypatch): + self._install_rewriting_transform(agent, monkeypatch) + messages = [] + + agent._execute_tool_calls_concurrent( + _mock_assistant_msg( + content="", + tool_calls=[ + _mock_tool_call( + name="todo_list", arguments=json.dumps({"todos": []}), call_id=call_id + ) + for call_id in ("todo-c1", "todo-c2") + ], + ), + messages, + "task-concurrent", + ) + + tool_results = [m for m in messages if m.get("role") == "tool"] + assert [m["tool_call_id"] for m in tool_results] == ["todo-c1", "todo-c2"] + # Exactly once per call: a second invocation would nest the prefix. + assert [str(m["content"]) for m in tool_results] == ['REWRITTEN[todo_list]{"ok":true}'] * 2 + + def test_sequential_path_applies_transform(self, agent, monkeypatch): + self._install_rewriting_transform(agent, monkeypatch) + messages = [] + + agent._execute_tool_calls_sequential( + _mock_assistant_msg( + content="", + tool_calls=[ + _mock_tool_call( + name="todo_list", + arguments=json.dumps({"todos": []}), + call_id="todo-sequential", + ) + ], + ), + messages, + "task-sequential", + ) + + tool_results = [m for m in messages if m.get("role") == "tool"] + assert tool_results, "sequential path appended no tool result" + # Exactly once: a second invocation would nest the prefix. + assert str(tool_results[-1]["content"]) == 'REWRITTEN[todo_list]{"ok":true}' + + class TestPathsOverlap: """Unit tests for the _paths_overlap helper.""" From 0ff5fa93a964eb26e351d4752b2d6413bc7a6291 Mon Sep 17 00:00:00 2001 From: teknium1 <127238744+teknium1@users.noreply.github.com> Date: Mon, 21 Sep 2026 22:35:21 -0700 Subject: [PATCH 034/173] fix: transform_terminal_output fires for background-process output too MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A plugin registered on transform_terminal_output only ever saw foreground `terminal` output: tools/terminal_tool_result.py::_apply_output_transform_hook runs from finalize_foreground_result and nowhere else. Background output reached the model through a different seam — process_manage poll/wait/log/kill results, `list` previews and the completion/heartbeat/watch notifications all pass through tools/process_registry.py::_redact_process_result — which redacted but never transformed, so a fleet redaction or summarising plugin silently did nothing for backgrounded commands. Apply the same hook helper at that shared seam (one new transform_process_output wrapper) and at the two gateway agent-notify sites that read session.output_buffer directly. The order matches the foreground path and teknium1's review note on #71401: hook first, redaction after, so a replacement the plugin returns is still masked. returncode is None while the process runs; env_type is not recorded per process and is passed empty. Not changed: the spawn acknowledgement ("Background process started") carries no command output, and the persistent local shell already goes through _run_foreground and was transformed — the issue's reading of that branch was wrong; the real gap was the process_manage/notification seam. Fixes #70760 Slim redo of #71401 (Christopher-Schulze): same seam and ordering, without the ANSI-stripping relocation and render helper. Co-authored-by: Christopher <210261288+Christopher-Schulze@users.noreply.github.com> (cherry picked from commit 84661de52f78ccb84054e1ded92b07166e40546d) --- gateway/run_notifications.py | 9 ++++- tests/tools/test_process_registry.py | 45 +++++++++++++++++++++++ tools/process_registry.py | 25 ++++++++++--- website/docs/user-guide/features/hooks.md | 2 + 4 files changed, 74 insertions(+), 7 deletions(-) diff --git a/gateway/run_notifications.py b/gateway/run_notifications.py index e2b6071dd6..b79a0891bb 100644 --- a/gateway/run_notifications.py +++ b/gateway/run_notifications.py @@ -1903,10 +1903,14 @@ class GatewayNotificationsMixin: """Last ``limit`` chars of process output through the secret redactors (unconditional floor).""" from gateway.run import _redact_gateway_user_facing_secrets from tools.ansi_strip import strip_ansi + from tools.process_registry import transform_process_output new_output = strip_ansi(session.output_buffer[-limit:]) if session.output_buffer else "" if new_output: from agent.redact import redact_terminal_output - new_output = redact_terminal_output(new_output, getattr(session, "command", "") or "") + _command = getattr(session, "command", "") or "" + new_output = transform_process_output(new_output, command=_command, returncode=session.exit_code, + task_id=getattr(session, "task_id", "") or "") + new_output = redact_terminal_output(new_output, _command) # redact_terminal_output() is unforced (raw when security.redact_secrets is off); this goes # straight to the adapter, so apply the same unconditional floor as agent-notify. new_output = _redact_gateway_user_facing_secrets(new_output) @@ -1939,8 +1943,11 @@ class GatewayNotificationsMixin: from gateway.run import _redact_gateway_user_facing_secrets from agent.redact import redact_terminal_output from tools.ansi_strip import strip_ansi + from tools.process_registry import transform_process_output _command = getattr(session, "command", "") or "" _raw = strip_ansi(session.output_buffer) if session.output_buffer else "" + _raw = transform_process_output(_raw, command=_command, returncode=session.exit_code, + task_id=getattr(session, "task_id", "") or "") if _raw else _raw _raw = redact_terminal_output(_raw, _command) # Keep the last ~2000 chars snapped to a line boundary, with a marker when cut. _LIMIT = 2000 diff --git a/tests/tools/test_process_registry.py b/tests/tools/test_process_registry.py index 7aae151fbf..dfac3336ad 100644 --- a/tests/tools/test_process_registry.py +++ b/tests/tools/test_process_registry.py @@ -2044,6 +2044,51 @@ class TestHandleProcessRedaction: assert "zzzopaque1234567890abcdef" in out["output"] +class TestHandleProcessTransformHook: + """Background-process output goes through the same ``transform_terminal_output`` plugin seam + as the foreground ``terminal`` result — issue #70760 — hook FIRST, redaction AFTER, so a + replacement the plugin returns is still masked (the ordering the foreground path documents).""" + + def _setup(self, monkeypatch, output, *, hook): + import agent.redact as _r + monkeypatch.setattr(_r, "_REDACT_ENABLED", True) + monkeypatch.setattr("hermes_cli.lifecycle.invoke_hook", hook) + from tools import process_registry as pr + reg = ProcessRegistry() + sess = _make_session(sid="proc_xform1", command="python app.py") + sess.output_buffer = output + sess.exited = True + sess.exit_code = 3 + reg._running[sess.id] = sess + monkeypatch.setattr(pr, "process_registry", reg) + return pr, sess + + def test_poll_wait_log_kill_results_are_transformed(self, monkeypatch): + seen = [] + + def hook(hook_name, **kw): + seen.append((hook_name, kw.get("command"), kw.get("returncode"), kw.get("task_id"))) + return ["REWRITTEN:" + kw["output"]] if hook_name == "transform_terminal_output" else [] + + pr, sess = self._setup(monkeypatch, "raw line\n", hook=hook) + for action, key in (("poll", "output_preview"), ("log", "output"), ("wait", "output"), ("kill", "output")): + out = json.loads(pr._handle_process({"action": action, "session_id": sess.id}, task_id="task-bg")) + assert out[key].startswith("REWRITTEN:raw line"), (action, out) + assert [s for s in seen if s[0] == "transform_terminal_output"] + # The hook sees the command, the recorded exit code (None while running) and the caller's task_id. + assert ("transform_terminal_output", "python app.py", 3, "task-bg") in seen + + def test_hook_replacement_is_still_redacted(self, monkeypatch): + secret = "sk-proj-abc123def456ghi789jkl012mno345" + pr, sess = self._setup( + monkeypatch, "plain output", + hook=lambda hook_name, **kw: [f"OPENAI_API_KEY={secret}"] if hook_name == "transform_terminal_output" else [], + ) + out = json.loads(pr._handle_process({"action": "log", "session_id": sess.id})) + assert secret not in out["output"] + assert "OPENAI_API_KEY=" in out["output"] + + # ========================================================================= # Reader loop: orphaned grandchild holding the stdout pipe (issue #68915) # ========================================================================= diff --git a/tools/process_registry.py b/tools/process_registry.py index be771756c0..ae924176c7 100644 --- a/tools/process_registry.py +++ b/tools/process_registry.py @@ -2488,11 +2488,22 @@ PROCESS_SCHEMA = { } -def _redact_process_result(result: dict) -> dict: - """Redact secrets from background-process output before it reaches the model, - session.db and CLI, mirroring the foreground ``terminal`` redaction so the two - surfaces can't diverge. Respects ``security.redact_secrets``; ``redact_terminal_output`` - picks ``code_file`` from the recorded command. The command itself is redacted too. +def transform_process_output(output: str, *, command: str, returncode: Optional[int], task_id: str = "") -> str: + """``transform_terminal_output`` seam for background-process output — the poll/wait/log/kill + results and the completion/heartbeat/watch notifications — so a plugin that rewrites terminal + output sees the same command output whether it ran in the foreground or not (#70760). + Same helper as the foreground path; callers redact AFTER it, never before, so a replacement + the plugin returns is still masked. ``returncode`` is None while the process is running and + ``env_type`` is not recorded per process, so it is passed empty.""" + from tools.terminal_tool_result import _apply_output_transform_hook + return _apply_output_transform_hook(command, output, returncode, task_id or "", "") + + +def _redact_process_result(result: dict, *, task_id: str = "") -> dict: + """Transform, then redact secrets from background-process output before it reaches the + model, session.db and CLI, mirroring the foreground ``terminal`` pipeline (hook first, + redaction after) so the two surfaces can't diverge. Respects ``security.redact_secrets``; + ``redact_terminal_output`` picks ``code_file`` from the recorded command. The command string itself is also redacted in case it carried an inline credential. See #43025. """ @@ -2501,8 +2512,10 @@ def _redact_process_result(result: dict) -> dict: from agent.redact import redact_sensitive_text, redact_terminal_output command = result.get("command") or "" + task_id = task_id or str(result.get("task_id") or "") for key in ("output", "output_preview"): if isinstance(value := result.get(key), str) and value: + value = transform_process_output(value, command=command, returncode=result.get("exit_code"), task_id=task_id) result[key] = redact_terminal_output(value, command) if isinstance(command, str) and command: result["command"] = redact_sensitive_text(command, code_file=True) @@ -2592,7 +2605,7 @@ def _handle_process(args, **kw): return tool_error(f"session_id is required for {action}") handler, redact = _SESSION_ACTIONS[action] result = handler(session_id, args) - return json.dumps(_redact_process_result(result) if redact else result, ensure_ascii=False) + return json.dumps(_redact_process_result(result, task_id=kw.get("task_id") or "") if redact else result, ensure_ascii=False) return tool_error(f"Unknown process action: {action}. Use: list, poll, log, wait, kill, write, submit, close, handoff") diff --git a/website/docs/user-guide/features/hooks.md b/website/docs/user-guide/features/hooks.md index f05a308db3..67189fb463 100644 --- a/website/docs/user-guide/features/hooks.md +++ b/website/docs/user-guide/features/hooks.md @@ -1520,6 +1520,8 @@ Applies to every tool. For terminal-only rewriting see `transform_terminal_outpu Fires inside the `terminal` tool after foreground process capture has already been bounded by the environment, and before the final output limit. It lets plugins replace the captured stdout/stderr; the replacement is still subject to the final output limit. +It also fires for background-process output on its way to the model or the chat: the `process_manage` `poll` / `wait` / `log` / `kill` results (and `list` previews) and the completion, heartbeat and watch-pattern notifications. There `returncode` is `None` while the process is still running and `env_type` is an empty string (the environment is not recorded per process). In both cases the hook runs *before* secret redaction, so a replacement that still carries a credential is masked. + **Callback signature:** ```python From a28890a4c829e06ad65ff84ece8d0079d05a0fbe Mon Sep 17 00:00:00 2001 From: teknium1 <127238744+teknium1@users.noreply.github.com> Date: Mon, 21 Sep 2026 22:52:16 -0700 Subject: [PATCH 035/173] fix: transform_llm_output result is what the session stores and replays MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A plugin's transform_llm_output replacement reached final_response only. agent/turn_finalizer.py::finalize_turn fired the hook after the assistant row had already been persisted — and the row is first written even earlier, in agent/turn_final_response.py::finish_text_response's durable flush — so messages[-1], the SQLite/JSON session, /resume and the next turn's replay all kept the raw model text while the user had seen the rewritten one. Writing the transformed text back after that flush cannot work: SQLite treats a non-blank assistant row as settled (resolve_and_repair_transcript_batch adopts the stored content instead of overwriting it), so the only correct seam is BEFORE the row is first persisted. apply_llm_output_transform (new, in turn_finalizer) fires the hook once per turn_id and records the outcome; finish_text_response calls it ahead of append+flush and writes the result into the row (api_content for the promoted-reasoning sidecar), finalize_turn's _persist_step calls it ahead of the recovery-path tail close, and _apply_output_hooks reads the recorded outcome (firing only when no earlier seam saw a response) before post_llm_call. Only the current turn's not-yet- written text changes — earlier turns and the system prompt are untouched. post_llm_call is unchanged: it is an observer whose return is ignored by contract, so there is nothing of it to persist (#14913/#44253's premise). Fixes #44239 Slim redo of #44244 (AIalliAI, earliest; same sync-then-persist idea, moved to the pre-flush seam) — also supersedes #65921 (SingleVirgin, sibling fix). Co-authored-by: AIalliAI <285906080+AIalliAI@users.noreply.github.com> (cherry picked from commit 5fe02a0aeecef422a4ffb4ff4385015f6a1528a0) --- agent/turn_final_response.py | 16 +++ agent/turn_finalizer.py | 73 ++++++++++--- .../test_transform_llm_output_persistence.py | 101 ++++++++++++++++++ website/docs/user-guide/features/hooks.md | 2 +- 4 files changed, 176 insertions(+), 16 deletions(-) create mode 100644 tests/agent/test_transform_llm_output_persistence.py diff --git a/agent/turn_final_response.py b/agent/turn_final_response.py index af36bc717f..779a473003 100644 --- a/agent/turn_final_response.py +++ b/agent/turn_final_response.py @@ -301,6 +301,22 @@ def finish_text_response( final_response = None return _verdict("continue") + # Plugins rewrite the reply BEFORE it is appended and flushed: SQLite treats a non-blank + # assistant row as settled, so a transform after this write would reach the user but never + # the stored/replayed transcript (#44239). finalize_turn reads the recorded outcome; like + # there, an interrupted turn keeps the raw text. + from agent.turn_finalizer import apply_llm_output_transform + _transformed = False + if not getattr(agent, "_interrupt_requested", False): + final_response, _transformed, _ = apply_llm_output_transform( + agent, final_response, turn_id=getattr(agent, "_current_turn_id", "") or "", logger=logger, + ) + if _transformed: + if _promoted: + final_msg["api_content"] = final_response + else: + final_msg["content"] = final_response + append_message(messages, final_msg) # Make the answer durable before leaving the loop (_DB_PERSISTED_MARKER keeps # _persist_session idempotent). Failure must NOT abort the turn: finalize retries. diff --git a/agent/turn_finalizer.py b/agent/turn_finalizer.py index e1c2de569e..3db49dd22f 100644 --- a/agent/turn_finalizer.py +++ b/agent/turn_finalizer.py @@ -413,21 +413,16 @@ def _apply_output_hooks( agent, final_response, logger, *, platform, effective_task_id, turn_id, original_user_message, messages, ) -> Tuple[Any, bool, Optional[Any]]: - """Fire ``transform_llm_output`` then ``post_llm_call`` once per turn after the tool loop. - Returns ``(final_response, transformed, pre_transform_response)``.""" - transformed, pre_transform = False, None - # First hook to return a string wins; None/empty leaves the text unchanged. - for _hook_result in _invoke_hook_safely( - "transform_llm_output", logger, - response_text=final_response, - session_id=agent.session_id or "", - model=agent.model, - platform=platform, - turn_id=turn_id, # per-turn identity for the hook callback gate - ): - if isinstance(_hook_result, str) and _hook_result: - pre_transform, final_response, transformed = final_response, _hook_result, True - break + """Resolve the turn's ``transform_llm_output`` outcome, then fire ``post_llm_call`` once per + turn after the tool loop. Returns ``(final_response, transformed, pre_transform_response)``. + + The transform itself normally already ran before the assistant row was first persisted + (``apply_llm_output_transform`` from ``finish_text_response`` / ``_persist_step``); this + call returns that recorded outcome, and only fires the hook here when no earlier seam saw a + response (e.g. text that only appeared through ``_explain_abnormal_exit``).""" + final_response, transformed, pre_transform = apply_llm_output_transform( + agent, final_response, turn_id=turn_id, platform=platform, logger=logger, + ) # Detached forks are internal work and must not publish turns under the parent's session ID. if not getattr(agent, "_persist_disabled", False): _invoke_hook_safely( @@ -444,6 +439,48 @@ def _apply_output_hooks( return final_response, transformed, pre_transform +def apply_llm_output_transform( + agent, final_response, *, turn_id, platform=None, logger=None, +) -> Tuple[Any, bool, Optional[Any]]: + """Fire ``transform_llm_output`` once per turn and return + ``(final_response, transformed, pre_transform_response)``. + + Called BEFORE the final assistant row is first persisted — from ``finish_text_response`` + ahead of its durable flush, and from ``finalize_turn._persist_step`` ahead of the + recovery-path tail close — so the text the user sees is the text stored in SQLite/JSON and + replayed next turn (#44239). SQLite treats a non-blank assistant row as settled (a re-flush + adopts the stored content rather than overwriting it), so transforming after that first + write can never reach the durable store. Idempotent per ``turn_id``: later callers in the + same turn get the recorded outcome instead of a second hook firing. Only the current + turn's not-yet-written text is touched — earlier turns and the system prompt are never + rewritten (prompt-cache invariant).""" + if logger is None: + from agent.conversation_loop import logger + recorded = getattr(agent, "_llm_output_transform", None) + if isinstance(recorded, tuple) and len(recorded) == 3 and recorded[0] == turn_id: + _, transformed, pre_transform = recorded + return final_response, transformed, pre_transform + if not final_response: + return final_response, False, None + if platform is None: + platform = getattr(agent, "platform", None) or "" + transformed, pre_transform = False, None + # First hook to return a string wins; None/empty leaves the text unchanged. + for _hook_result in _invoke_hook_safely( + "transform_llm_output", logger, + response_text=final_response, + session_id=agent.session_id or "", + model=agent.model, + platform=platform, + turn_id=turn_id, # per-turn identity for the hook callback gate + ): + if isinstance(_hook_result, str) and _hook_result: + pre_transform, final_response, transformed = final_response, _hook_result, True + break + agent._llm_output_transform = (turn_id, transformed, pre_transform) + return final_response, transformed, pre_transform + + def finalize_turn( agent, *, final_response, api_call_count, interrupted, failed, messages, conversation_history, effective_task_id, turn_id, user_message, original_user_message, _should_review_memory, @@ -508,6 +545,12 @@ def finalize_turn( final_response, _recovered_from_stream = _recover_final_from_stream( agent, final_response, interrupted, failed ) + # Recovery paths (stream-recovered / prior-turn text) reach here with a response no + # earlier seam transformed; the normal text turn already did this before its flush and + # gets the recorded outcome back. Either way the tail close below writes the text the + # user will see, never the raw model text (#44239). + if final_response and not interrupted: + final_response, _, _ = apply_llm_output_transform(agent, final_response, turn_id=turn_id, logger=logger) _close_transcript_tail(agent, messages, final_response, interrupted, _recovered_from_stream) if not interrupted and not failed: _micro_compact_after_turn(agent, messages, final_response, logger) diff --git a/tests/agent/test_transform_llm_output_persistence.py b/tests/agent/test_transform_llm_output_persistence.py new file mode 100644 index 0000000000..3bef122dec --- /dev/null +++ b/tests/agent/test_transform_llm_output_persistence.py @@ -0,0 +1,101 @@ +"""#44239: what ``transform_llm_output`` shows the user is what the session stores and replays. + +The hook used to fire after the final assistant row had been flushed to SQLite, so +``final_response`` carried the rewritten text while ``messages[-1]``, the SQLite row and the +next turn's replay kept the raw model text. +""" + +import json +from types import SimpleNamespace +from unittest.mock import MagicMock, patch + +import pytest + +from hermes_state import SessionDB +from run_agent import AIAgent + + +def _rewriting_hook(calls): + def invoke_hook(hook_name, **kwargs): + calls.append(hook_name) + if hook_name == "transform_llm_output": + return ["REWRITTEN:" + kwargs["response_text"]] + return [] + return invoke_hook + + +def _fake_completion(text): + def create(**kwargs): + msg = SimpleNamespace(content=text, tool_calls=None, reasoning=None) + return SimpleNamespace( + choices=[SimpleNamespace(message=msg, finish_reason="stop")], + usage=SimpleNamespace(prompt_tokens=10, completion_tokens=5, total_tokens=15), model="fake/model", + ) + return create + + +@pytest.fixture +def db_agent(tmp_path, monkeypatch): + monkeypatch.setenv("HERMES_HOME", str(tmp_path / ".hermes")) + (tmp_path / ".hermes").mkdir() + db = SessionDB(db_path=tmp_path / ".hermes" / "state.db") + with ( + patch("model_tools.get_tool_definitions", return_value=[]), + patch("model_tools.check_toolset_requirements", return_value={}), + patch("agent.process_bootstrap.OpenAI"), + ): + agent = AIAgent( + api_key="test-key-1234567890", base_url="https://openrouter.ai/api/v1", model="fake/model", + quiet_mode=True, skip_context_files=True, skip_memory=True, platform="cli", + session_id="sess-44239", session_db=db, + ) + agent.client = MagicMock() + return agent, db + + +def test_transformed_reply_is_the_stored_and_replayed_text(db_agent, monkeypatch): + agent, db = db_agent + calls = [] + monkeypatch.setattr("hermes_cli.lifecycle.invoke_hook", _rewriting_hook(calls)) + agent.client.chat.completions.create = _fake_completion("RAW MODEL TEXT") + + result = agent.run_conversation("hello") + + last_assistant = next(m for m in reversed(result["messages"]) if m.get("role") == "assistant") + stored = [r["content"] for r in db.get_messages("sess-44239") if r["role"] == "assistant"] + assert result["final_response"] == "REWRITTEN:RAW MODEL TEXT" + assert last_assistant["content"] == result["final_response"] + assert stored == [result["final_response"]] + assert result["response_transformed"] is True + assert result["pre_transform_response"] == "RAW MODEL TEXT" + # Exactly once per turn: a second firing would nest the prefix. + assert calls.count("transform_llm_output") == 1 + + +def test_recovery_path_tail_row_carries_transformed_text(db_agent, monkeypatch): + """A recovery ``break`` returns ``final_response`` with no closing assistant row; the row + finalize_turn appends must be the transformed text, and the hook still fires once.""" + from agent.turn_finalizer import finalize_turn + + agent, _db = db_agent + calls = [] + monkeypatch.setattr("hermes_cli.lifecycle.invoke_hook", _rewriting_hook(calls)) + agent._persist_session = lambda *a, **k: None + agent._current_turn_id = "turn-r" + messages = [ + {"role": "user", "content": "do a thing"}, + {"role": "assistant", "content": "", "tool_calls": [{"id": "c1", "function": {"name": "read_file", "arguments": "{}"}}]}, + {"role": "tool", "tool_call_id": "c1", "content": json.dumps({"ok": True})}, + ] + + result = finalize_turn( + agent, final_response="RECOVERED TEXT", api_call_count=1, interrupted=False, failed=False, + messages=messages, conversation_history=None, effective_task_id="task-1", turn_id="turn-r", + user_message="do a thing", original_user_message="do a thing", _should_review_memory=False, + _turn_exit_reason="partial_stream_recovery", + ) + + assert result["final_response"].startswith("REWRITTEN:RECOVERED TEXT") + assert messages[-1]["role"] == "assistant" + assert messages[-1]["content"] == "REWRITTEN:RECOVERED TEXT" + assert calls.count("transform_llm_output") == 1 diff --git a/website/docs/user-guide/features/hooks.md b/website/docs/user-guide/features/hooks.md index 67189fb463..51c74a01d2 100644 --- a/website/docs/user-guide/features/hooks.md +++ b/website/docs/user-guide/features/hooks.md @@ -1563,7 +1563,7 @@ Pairs with `transform_tool_result`, which runs afterward for every tool, includi ### `transform_llm_output` -Fires **once per turn** after the tool-calling loop completes and the model has produced a final response, **before** that response is delivered to the user (CLI, gateway, or programmatic caller). Lets a plugin rewrite the assistant's final text using classical-programming methods — no extra inference tokens burned on SOUL flavor text or a skill-driven transform. +Fires **once per turn** after the tool-calling loop completes and the model has produced a final response, **before** that response is delivered to the user (CLI, gateway, or programmatic caller) and **before** the assistant row is persisted — the replacement is what the session stores, what `/resume` shows and what the next turn replays, so the transcript never diverges from what the user saw. Hermes' own trailers (the file-mutation warning, the abnormal-exit note) are appended afterwards and are not part of `response_text`. Lets a plugin rewrite the assistant's final text using classical-programming methods — no extra inference tokens burned on SOUL flavor text or a skill-driven transform. **Callback signature:** From 152bef7531bb2e36a0dc9b242f11fc33a5f5c96c Mon Sep 17 00:00:00 2001 From: teknium1 <127238744+teknium1@users.noreply.github.com> Date: Mon, 21 Sep 2026 23:40:20 -0700 Subject: [PATCH 036/173] fix: keep _redact_process_result's one-arg signature; hook task_id is the process owner's The #70760 commit widened _redact_process_result(result, *, task_id=) and passed the caller's task_id from _handle_process. tests/gateway/test_completion_delivery.py monkeypatches the function with a one-arg stub (CI red on #118845), and the caller's id is the wrong identity anyway: a poll/log/wait result belongs to the process OWNER, whose task_id the session record already carries. Resolve it inside the function from result["task_id"] or the session looked up by result["session_id"]; the test pins the owner's id ("t1") instead of the caller's. --- tests/tools/test_process_registry.py | 6 ++++-- tools/process_registry.py | 9 ++++++--- 2 files changed, 10 insertions(+), 5 deletions(-) diff --git a/tests/tools/test_process_registry.py b/tests/tools/test_process_registry.py index dfac3336ad..69de2ef115 100644 --- a/tests/tools/test_process_registry.py +++ b/tests/tools/test_process_registry.py @@ -2075,8 +2075,10 @@ class TestHandleProcessTransformHook: out = json.loads(pr._handle_process({"action": action, "session_id": sess.id}, task_id="task-bg")) assert out[key].startswith("REWRITTEN:raw line"), (action, out) assert [s for s in seen if s[0] == "transform_terminal_output"] - # The hook sees the command, the recorded exit code (None while running) and the caller's task_id. - assert ("transform_terminal_output", "python app.py", 3, "task-bg") in seen + # The hook sees the command, the recorded exit code (None while running) and the process + # OWNER's task_id (the session's, not the caller's — a sibling polling a handed-off process + # is still observing that owner's output). + assert ("transform_terminal_output", "python app.py", 3, "t1") in seen def test_hook_replacement_is_still_redacted(self, monkeypatch): secret = "sk-proj-abc123def456ghi789jkl012mno345" diff --git a/tools/process_registry.py b/tools/process_registry.py index ae924176c7..2b417d618c 100644 --- a/tools/process_registry.py +++ b/tools/process_registry.py @@ -2499,7 +2499,7 @@ def transform_process_output(output: str, *, command: str, returncode: Optional[ return _apply_output_transform_hook(command, output, returncode, task_id or "", "") -def _redact_process_result(result: dict, *, task_id: str = "") -> dict: +def _redact_process_result(result: dict) -> dict: """Transform, then redact secrets from background-process output before it reaches the model, session.db and CLI, mirroring the foreground ``terminal`` pipeline (hook first, redaction after) so the two surfaces can't diverge. Respects ``security.redact_secrets``; @@ -2512,7 +2512,10 @@ def _redact_process_result(result: dict, *, task_id: str = "") -> dict: from agent.redact import redact_sensitive_text, redact_terminal_output command = result.get("command") or "" - task_id = task_id or str(result.get("task_id") or "") + # The hook's task_id is the process OWNER's (poll/log/wait results carry only session_id). + task_id = str(result.get("task_id") or "") + if not task_id and (session := process_registry.get(str(result.get("session_id") or ""))) is not None: + task_id = str(getattr(session, "task_id", "") or "") for key in ("output", "output_preview"): if isinstance(value := result.get(key), str) and value: value = transform_process_output(value, command=command, returncode=result.get("exit_code"), task_id=task_id) @@ -2605,7 +2608,7 @@ def _handle_process(args, **kw): return tool_error(f"session_id is required for {action}") handler, redact = _SESSION_ACTIONS[action] result = handler(session_id, args) - return json.dumps(_redact_process_result(result, task_id=kw.get("task_id") or "") if redact else result, ensure_ascii=False) + return json.dumps(_redact_process_result(result) if redact else result, ensure_ascii=False) return tool_error(f"Unknown process action: {action}. Use: list, poll, log, wait, kill, write, submit, close, handoff") From 35fdb4608aa8af455d2597664cff1754a3722cd1 Mon Sep 17 00:00:00 2001 From: teknium1 <127238744+teknium1@users.noreply.github.com> Date: Mon, 21 Sep 2026 23:45:24 -0700 Subject: [PATCH 037/173] fix(tui_gateway): plugin slash commands run under the session's HERMES_SESSION_* binding TUI/Desktop sibling of #108698: `command.dispatch` and `slash.exec` ran a plugin command handler on the RPC thread with only HERMES_HOME/cwd bound (`_session_home_scope`), never the session vars, so `get_session_env()` inside the handler returned "" or the launch process's inherited values. `_run_plugin_command` now wraps the handler in the turn path's own `_set_session_context` / `_clear_session_context` for the invoking session; both call sites pass it. These RPCs already run off the loop, so the #105279 half does not apply here. Test red on base: `assert '' == 'agent:tui:key-p'`. --- tests/tui_gateway/test_tui_gateway_server.py | 24 ++++++++++++++++++++ tui_gateway/methods_tools.py | 19 ++++++++++++---- 2 files changed, 39 insertions(+), 4 deletions(-) diff --git a/tests/tui_gateway/test_tui_gateway_server.py b/tests/tui_gateway/test_tui_gateway_server.py index 27d0d7754e..a061a2f0ed 100644 --- a/tests/tui_gateway/test_tui_gateway_server.py +++ b/tests/tui_gateway/test_tui_gateway_server.py @@ -11918,6 +11918,30 @@ def test_commands_catalog_includes_plugin_commands(monkeypatch): assert "/lcm" in dict(plugin_cat["pairs"]) +def test_plugin_slash_command_runs_under_the_session_env(monkeypatch): + # TUI/Desktop sibling of #108698: command.dispatch and slash.exec ran plugin handlers on the RPC + # thread with no HERMES_SESSION_* binding, so a handler reading get_session_env() saw "" (or the + # launch process's inherited values) instead of the session it was invoked from. + from gateway.session_context import get_session_env + + seen = {} + + def handler(arg): + seen["key"] = get_session_env("HERMES_SESSION_KEY") + return f"ok:{arg}" + + monkeypatch.setattr("hermes_cli.plugins.get_plugin_command_handler", + lambda name: handler if name == "whoami" else None) + monkeypatch.setattr(server, "_sessions", {"sid-p": {"session_key": "agent:tui:key-p", "cwd": ""}}) + + res = server._methods["command.dispatch"]("d", {"name": "whoami", "arg": "x", "session_id": "sid-p"}) + + assert res["result"] == {"type": "plugin", "output": "ok:x"} + assert seen["key"] == "agent:tui:key-p" + # Nothing leaks past the RPC. + assert get_session_env("HERMES_SESSION_KEY") in ("", None) + + def test_session_status_reads_live_gateway_agent(monkeypatch): agent = types.SimpleNamespace( model="live-model", diff --git a/tui_gateway/methods_tools.py b/tui_gateway/methods_tools.py index ae7bfc4e56..6926240797 100644 --- a/tui_gateway/methods_tools.py +++ b/tui_gateway/methods_tools.py @@ -527,8 +527,19 @@ def _plugin_command_handler(name: str): return None -def _run_plugin_command(handler, arg: str) -> str: - return str(_tools_mod("hermes_cli.plugins").resolve_plugin_command_result(handler(arg)) or "") +def _run_plugin_command(handler, arg: str, session=None) -> str: + """Run a plugin slash-command handler under the session's ``HERMES_SESSION_*`` binding. + + Plugin handlers read ``get_session_env()`` for the chat/session they serve; these RPCs run on + the socket/worker thread where nothing upstream binds it (only the turn path does), so a handler + saw ``""`` or the launch process's inherited values. Same class as the messaging gateway's + #108698; ``_set_session_context`` is the turn path's own seam.""" + plugins = _tools_mod("hermes_cli.plugins") + tokens = _set_session_context(session.get("session_key", "") or "", cwd=str(session.get("cwd") or "")) if session else [] + try: + return str(plugins.resolve_plugin_command_result(handler(arg)) or "") + finally: + _clear_session_context(tokens) @contextlib.contextmanager @@ -569,7 +580,7 @@ def _is_profile_skill_command(session: dict, base: str) -> bool: def _dispatch_plugin(rid, params, session, name, arg): if handler := _plugin_command_handler(name): with contextlib.suppress(Exception): - return _ok(rid, {"type": "plugin", "output": _run_plugin_command(handler, arg)}) + return _ok(rid, {"type": "plugin", "output": _run_plugin_command(handler, arg, session)}) return None @@ -905,7 +916,7 @@ def _(rid, params: dict) -> dict: return _err(rid, 4018, f"skill command: use command.dispatch for /{base}") if plugin_handler := _plugin_command_handler(base) if base else None: try: - return _ok(rid, {"output": _run_plugin_command(plugin_handler, arg) or "(no output)"}) + return _ok(rid, {"output": _run_plugin_command(plugin_handler, arg, session) or "(no output)"}) except Exception as e: return _ok(rid, {"output": f"Plugin command error: {e}"}) worker = session.get("slash_worker") From 024f1b07bd510aaecf4af6f6e12ca49f29b749f5 Mon Sep 17 00:00:00 2001 From: bsdnn <1600332149@qq.com> Date: Mon, 31 Aug 2026 19:19:34 -0700 Subject: [PATCH 038/173] fix(plugins): unload the doctor's temp plugin before removing its home MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `plugins doctor` loads a plugin under a temporary HERMES_HOME via a throwaway PluginManager, restores the global registries, and closes the temp dir — but never calls manager.unload(), so host-owned ctx.on_unload(...) callbacks never run. A context-engine plugin that opened SQLite under the temp home left the DB handle open, and TemporaryDirectory removal then failed with WinError 32 on Windows (#99918). Call manager.unload() at the top of the teardown finally, while the temporary home still exists, so registration disposal and on_unload callbacks (e.g. closing the SQLite handle) run before the directory is removed. Best-effort: the existing snapshot restore remains the authoritative registry cleanup, so an unload hiccup never masks it or the original exception. Reported by @supplefrog. --- hermes_cli/plugin_dev.py | 12 +++ tests/hermes_cli/test_plugin_doctor_unload.py | 77 +++++++++++++++++++ 2 files changed, 89 insertions(+) create mode 100644 tests/hermes_cli/test_plugin_doctor_unload.py diff --git a/hermes_cli/plugin_dev.py b/hermes_cli/plugin_dev.py index ec19804610..2972b30e0d 100644 --- a/hermes_cli/plugin_dev.py +++ b/hermes_cli/plugin_dev.py @@ -90,6 +90,18 @@ def _doctor_runtime(plugin_path: Path): manifest=manifest, manager=manager, registered_tools=tuple(sorted(loaded.tools_registered)), registered_hooks=tuple(loaded.hooks_registered), registered_providers=()) finally: + # Dispose the plugin's own registrations FIRST, while the temporary + # HERMES_HOME still exists. This runs the host-owned ctx.on_unload(...) + # callbacks — e.g. closing a SQLite handle a context-engine plugin + # opened under that home. Without it the DB stays open and stack.close() + # below (TemporaryDirectory removal) fails with WinError 32 on Windows + # (#99918). Best-effort: the snapshot restore below remains the + # authoritative registry cleanup, so an unload hiccup never masks it or + # the original exception. + try: + manager.unload() + except Exception: + pass entries_after = {entry.name: entry for entry in registry._snapshot_entries()} changed_names = { name diff --git a/tests/hermes_cli/test_plugin_doctor_unload.py b/tests/hermes_cli/test_plugin_doctor_unload.py new file mode 100644 index 0000000000..b95c7f9b38 --- /dev/null +++ b/tests/hermes_cli/test_plugin_doctor_unload.py @@ -0,0 +1,77 @@ +"""`plugins doctor` must unload the plugin before removing the temp home (#99918). + +Doctor loads a plugin under a temporary ``HERMES_HOME`` through a throwaway +``PluginManager``. It restored the global registries and closed the temp dir but +never called ``manager.unload()``, so host-owned ``ctx.on_unload(...)`` callbacks +never ran. A context-engine plugin that opened SQLite under the temp home thus +left the DB open, and ``TemporaryDirectory`` removal failed with ``WinError 32`` +on Windows. + +The platform-independent contract these tests pin is the root cause itself: the +``on_unload`` callback must fire during doctor teardown (which is what closes the +DB handle and makes the Windows removal succeed). We assert the callback ran via +a sentinel written outside the temp home, so the test catches the regression on +every platform — not only where the filesystem locks open files. +""" +from __future__ import annotations + +import os +from pathlib import Path +from unittest.mock import patch + + +def _write_plugin(root: Path, marker: Path) -> Path: + plugin = root / "lifecycle-plugin" + plugin.mkdir() + (plugin / "plugin.yaml").write_text("name: lifecycle-plugin\n", encoding="utf-8") + # register() stashes a durable resource and registers on_unload to release + # it — the exact shape of a context-engine plugin holding a SQLite handle. + (plugin / "__init__.py").write_text( + "import os\n" + "from pathlib import Path\n\n" + "def register(ctx):\n" + " marker = os.environ['DOCTOR_UNLOAD_MARKER']\n" + " ctx.on_unload(lambda: Path(marker).write_text('unloaded'))\n", + encoding="utf-8", + ) + return plugin + + +def test_doctor_runs_on_unload_during_teardown(tmp_path: Path) -> None: + from hermes_cli.plugin_dev import doctor_plugin + + marker = tmp_path / "unloaded.marker" # outside the temp HERMES_HOME + plugin = _write_plugin(tmp_path, marker) + + with patch.dict(os.environ, {"DOCTOR_UNLOAD_MARKER": str(marker)}, clear=False): + report = doctor_plugin(plugin) + + assert report.ok, report.format_text() + assert marker.exists(), "ctx.on_unload must run before the temp home is removed" + assert marker.read_text() == "unloaded" + + +def test_doctor_still_reports_and_cleans_up_for_unload_plugin(tmp_path: Path) -> None: + # The added unload must not disturb the existing teardown contract: registry + # entries restored, no hermes_plugins.* module leak. + import sys + + from hermes_cli.plugin_dev import doctor_plugin + from tools.registry import registry + + marker = tmp_path / "unloaded2.marker" + plugin = _write_plugin(tmp_path, marker) + before_modules = { + name for name in sys.modules + if name == "hermes_plugins" or name.startswith("hermes_plugins.") + } + + with patch.dict(os.environ, {"DOCTOR_UNLOAD_MARKER": str(marker)}, clear=False): + report = doctor_plugin(plugin) + + assert report.ok, report.format_text() + after_modules = { + name for name in sys.modules + if name == "hermes_plugins" or name.startswith("hermes_plugins.") + } + assert after_modules == before_modules From 4a564706da2437b221e57d423673a4fce64841ca Mon Sep 17 00:00:00 2001 From: teknium1 <127238744+teknium1@users.noreply.github.com> Date: Mon, 21 Sep 2026 22:42:13 -0700 Subject: [PATCH 039/173] fix(plugins): subdir installs stay updatable via re-install from the recorded source A plugin installed from a repository subdirectory ships only /; the .git stays behind in the temp clone, so `hermes plugins update ` (and the dashboard update) refused with "not installed from git" forever. The install metadata already records the canonical source (URL#subdir), so when the tree has no .git the shared update core re-runs the installer from that source with force=True and swaps the fresh tree in; revision bookkeeping is the installer's. Pinned installs are still refused first, unchanged. Fixes #65314. Credit: @webtecnica (#65337) for the report and first fix attempt. --- hermes_cli/plugins_cmd.py | 23 +++++++++++++++++-- tests/hermes_cli/test_plugins_cmd.py | 33 ++++++++++++++++++++++++++++ 2 files changed, 54 insertions(+), 2 deletions(-) diff --git a/hermes_cli/plugins_cmd.py b/hermes_cli/plugins_cmd.py index b7c65fe521..736711080e 100644 --- a/hermes_cli/plugins_cmd.py +++ b/hermes_cli/plugins_cmd.py @@ -890,7 +890,8 @@ def cmd_install( def _pull_plugin_update(target: Path, pinned_msg, not_git_msg, before_pull=None) -> str: - """Shared ``update`` core: refuse pinned / non-git checkouts, ``git pull``, record the new + """Shared ``update`` core: refuse pinned checkouts, ``git pull`` (or re-install from the + recorded source when the tree carries no ``.git`` — subdirectory installs), record the new revision. Returns the pull output; raises :class:`PluginOperationError` on any refusal. *pinned_msg(install_record)* / *not_git_msg()* build the caller-specific error text.""" metadata = _read_install_metadata() @@ -898,7 +899,12 @@ def _pull_plugin_update(target: Path, pinned_msg, not_git_msg, before_pull=None) if install_record.get("pinned") is True: raise PluginOperationError(pinned_msg(install_record)) if not (target / ".git").exists(): - raise PluginOperationError(not_git_msg()) + source = install_record.get("source") + if not isinstance(source, str) or not source: + raise PluginOperationError(not_git_msg()) + if before_pull is not None: + before_pull() + return _reclone_plugin_update(source, install_record.get("revision")) # A URL install whose name/repo later landed on the kill list must not keep pulling new code. from hermes_cli import plugins_cmd_catalog as catalog catalog.refuse_if_installed_removed(target.name, target) @@ -916,6 +922,19 @@ def _pull_plugin_update(target: Path, pinned_msg, not_git_msg, before_pull=None) return output +def _reclone_plugin_update(source: str, previous_revision: object) -> str: + """Update a plugin whose tree is not a git checkout: a subdirectory install ships only + ``/``, so the ``.git`` stays in the temp clone (#65314). Re-run the install + from the recorded source (same URL, same subdir) and swap the fresh tree in; the metadata + revision is rewritten by the installer. Returns pull-shaped output for the callers.""" + new_target, _manifest, _name = _install_plugin_core(source, force=True) + revision = str(_read_install_metadata().get(new_target.name, {}).get("revision") or "") + previous = previous_revision if isinstance(previous_revision, str) else "" + if revision and revision == previous: + return "Already up to date." + return f"Re-installed from {source}: {previous[:8]}..{revision[:8]}" + + def cmd_update(name: str) -> None: """Update an installed plugin by pulling latest from its git remote.""" from rich.markup import escape diff --git a/tests/hermes_cli/test_plugins_cmd.py b/tests/hermes_cli/test_plugins_cmd.py index 598a72966a..d6f3af7b4c 100644 --- a/tests/hermes_cli/test_plugins_cmd.py +++ b/tests/hermes_cli/test_plugins_cmd.py @@ -776,6 +776,39 @@ class TestSubdirInstallE2E: with pytest.raises(PluginOperationError, match="does not exist"): pc._install_plugin_core(identifier, force=False) + def test_subdir_install_stays_updatable(self, tmp_path, monkeypatch): + """A subdir install ships no ``.git`` (it stays in the temp clone), so ``plugins update`` + must re-install from the recorded source instead of refusing (#65314).""" + if shutil.which("git") is None: + pytest.skip("git not available") + import subprocess as sp + + from hermes_cli import plugins_cmd as pc + + repo_root = tmp_path / "monorepo" + self._make_repo_with_subdir_plugin(repo_root) + plugins_dir = tmp_path / "installed" + plugins_dir.mkdir() + monkeypatch.setattr(pc, "_plugins_dir", lambda: plugins_dir) + monkeypatch.setattr(pc, "_install_metadata_path", lambda: plugins_dir / ".install-metadata.json") + target, _manifest, _name = pc._install_plugin_core(f"file://{repo_root}#my-plugin", force=False) + assert not (target / ".git").exists() + + (repo_root / "my-plugin" / "__init__.py").write_text("VERSION = 2\n", encoding="utf-8") + env = {**os.environ, "GIT_AUTHOR_NAME": "t", "GIT_AUTHOR_EMAIL": "t@t", + "GIT_COMMITTER_NAME": "t", "GIT_COMMITTER_EMAIL": "t@t"} + sp.run(["git", "commit", "-qam", "v2"], cwd=repo_root, check=True, env=env) + new_sha = sp.run(["git", "rev-parse", "HEAD"], cwd=repo_root, check=True, + capture_output=True, text=True).stdout.strip() + + output = pc._pull_plugin_update(target, lambda rec: "pinned", lambda: "not git") + + assert "VERSION = 2" in (target / "__init__.py").read_text(encoding="utf-8") + assert pc._read_install_metadata()["my-plugin"]["revision"] == new_sha + assert "Already up to date" not in output + # A second update with nothing new upstream reports up to date, like `git pull`. + assert "Already up to date" in pc._pull_plugin_update(target, lambda rec: "pinned", lambda: "not git") + def test_installs_portable_root_package_disabled(self, tmp_path, monkeypatch): if shutil.which("git") is None: pytest.skip("git not available") From 92e6cc189bb8ed4d48d4a80e10288850964d7c52 Mon Sep 17 00:00:00 2001 From: teknium1 <127238744+teknium1@users.noreply.github.com> Date: Mon, 21 Sep 2026 22:42:28 -0700 Subject: [PATCH 040/173] fix(update,plugins): address autostash entries by sha / bare index, never stash@{N} On native Windows the MSYS runtime re-parses git.exe's argv when the parent is a non-MSYS process (always the case for python.exe) and strips the braces, so `stash@{0}` reaches git as `stash@0` and is rejected. For `hermes plugins update` the autostash apply failed every time and the user's edits sat in a stash the tool claimed to have handled; for `hermes update` the drop failed and stash entries piled up unbounded. - plugins_cmd: `_autostash_dirty_tree` returns the autostash commit sha; `_reapply_stash` applies that sha and drops positionally (bare `git stash drop`) only while refs/stash is still that sha. No brace selector in any argv. - update_cmd_stash: `_resolve_stash_selector` returns the bare index `N` git accepts wherever `stash@{N}` is valid; the copy-pasteable guidance drops the braces too. Existing assertion change: `test_autostash_dirty_tree_promotes_intent_to_add_entries` asserted `(True, "")`; the first element is now the stash sha, asserted against `git rev-parse refs/stash`. Fixes #87542. Credit: @PRATHAMESH75 (#87571) for the diagnosis and the parent-process table. --- hermes_cli/plugins_cmd.py | 42 ++++++++++++++--------- hermes_cli/update_cmd_stash.py | 10 ++++-- tests/hermes_cli/test_plugins_cmd.py | 31 ++++++++++++++++- tests/hermes_cli/test_update_autostash.py | 30 ++++++++++++++++ 4 files changed, 93 insertions(+), 20 deletions(-) diff --git a/hermes_cli/plugins_cmd.py b/hermes_cli/plugins_cmd.py index 736711080e..c21871f167 100644 --- a/hermes_cli/plugins_cmd.py +++ b/hermes_cli/plugins_cmd.py @@ -2072,23 +2072,31 @@ def _stash_ref(git_exe: str, target: Path) -> str: return probe.stdout.strip() if probe.returncode == 0 else "" -def _reapply_stash(git_exe: str, target: Path) -> bool: - """``stash apply`` the autostash; drop it on a clean apply. False when it applied with - errors or left unmerged paths (the stash entry is kept in that case).""" - restore = _run_plugin_git(git_exe, target, "stash", "apply", "stash@{0}") +def _reapply_stash(git_exe: str, target: Path, stash_sha: str) -> bool: + """``stash apply`` the autostash commit *stash_sha*; drop it on a clean apply. False when it + applied with errors or left unmerged paths (the stash entry is kept in that case). + + Git is addressed by the stash's commit sha, never a ``stash@{N}`` selector: on native Windows + the MSYS runtime re-parses git.exe's argv and strips the braces, so ``stash@{0}`` reaches git + as ``stash@0`` and both the apply and the drop fail (#87542).""" + restore = _run_plugin_git(git_exe, target, "stash", "apply", stash_sha) unmerged = _run_plugin_git(git_exe, target, "diff", "--name-only", "--diff-filter=U") if restore.returncode != 0 or unmerged.stdout.strip(): return False - _run_plugin_git(git_exe, target, "stash", "drop", "stash@{0}") + # `stash drop` only takes a selector; a bare `drop` targets the newest entry, so drop + # positionally only while the newest entry is still our autostash. + if _stash_ref(git_exe, target) == stash_sha: + _run_plugin_git(git_exe, target, "stash", "drop") return True -def _autostash_dirty_tree(git_exe: str, target: Path) -> tuple[bool, str]: - """Stash local edits before a pull. Returns ``(stash_created, error)``; a non-empty error means - the tree is dirty but nothing was saved, so the pull must not run.""" +def _autostash_dirty_tree(git_exe: str, target: Path) -> tuple[str, str]: + """Stash local edits before a pull. Returns ``(stash_sha, error)``; *stash_sha* is empty when + the tree was clean, and a non-empty error means the tree is dirty but nothing was saved, so + the pull must not run.""" status = _run_plugin_git(git_exe, target, "status", "--porcelain", "-z") if status.returncode != 0 or not status.stdout.strip(): - return False, "" + return "", "" # `git add -N` entries make `git stash push` fail outright (see update_cmd_stash), so promote them # to real staged adds first; the checkout's own local edits are otherwise unstashable. from hermes_cli.update_cmd_stash import _intent_to_add_paths @@ -2102,7 +2110,7 @@ def _autostash_dirty_tree(git_exe: str, target: Path) -> tuple[bool, str]: post_stash = _stash_ref(git_exe, target) if not post_stash or post_stash == pre_stash: err = _safe_git_error(push) - return False, ( + return "", ( "Local changes in the plugin checkout could not be " "stashed; update aborted before touching the checkout." + (f"\n{err}" if err else "")) @@ -2110,7 +2118,7 @@ def _autostash_dirty_tree(git_exe: str, target: Path) -> tuple[bool, str]: # Saved-but-couldn't-clean (undeletable untracked files): the stash entry is complete; # reset tracked mods so the pull isn't blocked by a still-dirty tree. _run_plugin_git(git_exe, target, "reset", "--hard", "HEAD") - return True, "" + return post_stash, "" def _git_pull_plugin_dir(target: Path) -> tuple[bool, str]: @@ -2127,26 +2135,26 @@ def _git_pull_plugin_dir(target: Path) -> tuple[bool, str]: if not git_exe: return False, "git is not installed or not in PATH." try: - stash_created, err = _autostash_dirty_tree(git_exe, target) + stash_sha, err = _autostash_dirty_tree(git_exe, target) if err: return False, err origin = _run_plugin_git(git_exe, target, "remote", "get-url", "origin", timeout=15) result = _run_plugin_git(git_exe, target, "pull", "--ff-only", auth_url=origin.stdout.strip()) if result.returncode != 0: err = _safe_git_error(result) or "git pull failed." - if not stash_created: + if not stash_sha: return False, err # Put the user's edits back before reporting the failure. - if _reapply_stash(git_exe, target): + if _reapply_stash(git_exe, target, stash_sha): note = "Local changes were restored." else: note = "Local changes are preserved in git stash (restore with: git stash pop)." return False, f"{err}\n{note}" pulled = result.stdout.strip() - if not stash_created: + if not stash_sha: return True, pulled - if _reapply_stash(git_exe, target): + if _reapply_stash(git_exe, target, stash_sha): return True, pulled + "\nLocal changes were re-applied on top of the update." # Conflicted re-apply: leave the plugin importable on the updated @@ -2155,7 +2163,7 @@ def _git_pull_plugin_dir(target: Path) -> tuple[bool, str]: return True, pulled + ( "\n⚠ Local changes in this plugin conflicted with the update and " "were NOT re-applied. They are preserved in git stash — inspect " - "with `git stash show -p stash@{0}` and re-apply with " + "with `git stash show -p` and re-apply with " f"`git stash pop` inside {target}.") except FileNotFoundError: return False, "git is not installed or not in PATH." diff --git a/hermes_cli/update_cmd_stash.py b/hermes_cli/update_cmd_stash.py index 1c5b2841cf..b2971858a6 100644 --- a/hermes_cli/update_cmd_stash.py +++ b/hermes_cli/update_cmd_stash.py @@ -5,6 +5,7 @@ Origin helpers are imported lazily per function (no cycle; test patches on the o """ import logging +import re import subprocess from datetime import datetime, timedelta, timezone from pathlib import Path @@ -123,12 +124,17 @@ def _stash_local_changes_if_needed(git_cmd: list[str], cwd: Path) -> Optional[st def _resolve_stash_selector(git_cmd: list[str], cwd: Path, stash_ref: str) -> Optional[str]: + """Selector for the stash entry whose commit is *stash_ref*, as the bare index ``N`` + (git accepts it wherever ``stash@{N}`` is valid). Never ``stash@{N}`` itself: on native + Windows the MSYS runtime strips the braces from git.exe's argv, so ``stash@{0}`` reaches git + as ``stash@0`` and the drop fails (#87542).""" from hermes_cli.update_cmd import _git_run stash_list = _git_run(git_cmd, ["stash", "list", "--format=%gd %H"], cwd, check=True) for line in stash_list.stdout.splitlines(): selector, _, commit = line.partition(" ") if commit.strip() == stash_ref: - return selector.strip() + match = re.fullmatch(r"stash@\{(\d+)\}", selector.strip()) + return match.group(1) if match else selector.strip() return None @@ -199,7 +205,7 @@ def _print_stash_cleanup_guidance(stash_ref: str, stash_selector: Optional[str] if stash_selector: print(f" Remove it with: git stash drop {stash_selector}") else: - print(f" Look for commit {stash_ref}, then drop its selector with: git stash drop stash@{{N}}") + print(f" Look for commit {stash_ref}, then drop it by index with: git stash drop ") def _stash_apply_failed_only_on_existing_untracked(stderr: str) -> bool: diff --git a/tests/hermes_cli/test_plugins_cmd.py b/tests/hermes_cli/test_plugins_cmd.py index d6f3af7b4c..99da7f8946 100644 --- a/tests/hermes_cli/test_plugins_cmd.py +++ b/tests/hermes_cli/test_plugins_cmd.py @@ -295,6 +295,34 @@ class TestGitPullPluginDirAutostash: assert ok is True assert "Already up to date" in msg + def test_autostash_addresses_git_by_sha_never_brace_selector(self, tmp_path, monkeypatch): + """Native Windows: MSYS strips the braces from ``stash@{0}`` in git.exe's argv, so the + apply and the drop must target the autostash by its commit sha / positionally (#87542).""" + import hermes_cli.plugins_cmd as pc + + if not pc._resolve_git_executable(): + pytest.skip("git not available") + origin, checkout, git = self._make_repos(tmp_path) + self._set_line(origin, "VALUE", "VALUE = 2") + git(origin, "commit", "-qam", "bump value") + self._set_line(checkout, "OTHER", "OTHER = 'local'") + + argv_log: list[tuple[str, ...]] = [] + real_run = pc._run_plugin_git + + def recording_run(git_exe, target, *args, **kwargs): + argv_log.append(args) + return real_run(git_exe, target, *args, **kwargs) + + monkeypatch.setattr(pc, "_run_plugin_git", recording_run) + ok, msg = pc._git_pull_plugin_dir(checkout) + + assert ok is True and "re-applied" in msg + assert git(checkout, "stash", "list").strip() == "" + assert not any("{" in arg or "}" in arg for args in argv_log for arg in args), argv_log + applied = [args for args in argv_log if args[:2] == ("stash", "apply")] + assert len(applied) == 1 and len(applied[0][2]) == 40, applied # by commit sha + # ── _repo_name_from_url ────────────────────────────────────────────────── @@ -992,7 +1020,8 @@ def test_autostash_dirty_tree_promotes_intent_to_add_entries(tmp_path): stashed, error = _autostash_dirty_tree("git", tmp_path) - assert (stashed, error) == (True, ""), "the plugin autostash must not be blocked by i-t-a entries" + assert error == "", "the plugin autostash must not be blocked by i-t-a entries" + assert stashed == git("rev-parse", "refs/stash").stdout.strip() # the autostash commit sha assert git("status", "--porcelain").stdout == "" diff --git a/tests/hermes_cli/test_update_autostash.py b/tests/hermes_cli/test_update_autostash.py index 8d48a6d12d..47db53b97e 100644 --- a/tests/hermes_cli/test_update_autostash.py +++ b/tests/hermes_cli/test_update_autostash.py @@ -736,6 +736,36 @@ def test_update_autostash_survives_undeletable_untracked_dir(tmp_path): os.chmod(pkg, 0o755) +def test_stash_selector_is_a_bare_index_never_a_brace_selector(tmp_path): + """The updater drops its autostash through a selector read back from ``git stash list``; on + native Windows MSYS strips the braces from ``stash@{N}`` in git.exe's argv, so the selector + must be the bare index git accepts everywhere (#87542).""" + import subprocess + + import hermes_cli.update_cmd_stash as stash_mod + + def git(*args): + return subprocess.run(["git", *args], cwd=tmp_path, capture_output=True, text=True, check=True) + + git("init", "-q", "-b", "main") + git("config", "user.email", "t@example.com") + git("config", "user.name", "t") + (tmp_path / "f.txt").write_text("v1\n") + git("add", "-A") + git("commit", "-qm", "init") + (tmp_path / "f.txt").write_text("older\n") + git("stash", "push", "-q", "-m", "older") + target_sha = git("rev-parse", "refs/stash").stdout.strip() + (tmp_path / "f.txt").write_text("newer\n") + git("stash", "push", "-q", "-m", "newer") + + selector = stash_mod._resolve_stash_selector(["git"], tmp_path, target_sha) + + assert selector == "1" + git("stash", "drop", selector) + assert target_sha not in git("stash", "list", "--format=%H").stdout + + def test_autostash_survives_intent_to_add_entries(tmp_path): """An index entry from `git add -N` must not block the update autostash. From b9dff48435224857dafc9ba0e8ab38cbf0f44010 Mon Sep 17 00:00:00 2001 From: teknium1 <127238744+teknium1@users.noreply.github.com> Date: Mon, 21 Sep 2026 22:42:40 -0700 Subject: [PATCH 041/173] fix(plugin-packs): refuse and strip secret/consent keys at every depth of a config seed `validate_config_seed` and `_sanitized_entry_config` only inspected top-level keys, so `config: {demo: {settings: {api_key: LEAK}}}` was accepted on import and a nested `password:` under plugins.entries. was written into an exported pack. One recursive key policy (`_first_forbidden_key` / `_strip_forbidden_keys`) now backs both: nested mappings and mappings inside lists are held to the same contract as the top level; ordinary nested config is preserved. Fixes #85050. Credit: @michaelversluis (#85057) for the report and the recursive-policy direction. --- hermes_cli/plugin_packs.py | 66 ++++++++++++++++++++------- tests/hermes_cli/test_plugin_packs.py | 26 +++++++++++ 2 files changed, 76 insertions(+), 16 deletions(-) diff --git a/hermes_cli/plugin_packs.py b/hermes_cli/plugin_packs.py index d88c93507a..ac44ee27ca 100644 --- a/hermes_cli/plugin_packs.py +++ b/hermes_cli/plugin_packs.py @@ -76,25 +76,65 @@ def _entry_label(item: Any, index: int) -> str: return f"#{index + 1}" +def _forbidden_key_reason(key: str) -> Optional[str]: + """``"reserved"`` / ``"secret"`` when a config key may never travel in a pack, else None.""" + if key in _RESERVED_ENTRY_KEYS or key.startswith("allow_"): + return "reserved" + if _SECRET_KEY_RE.search(key): + return "secret" + return None + + +def _first_forbidden_key(value: Any, path: str = "") -> Optional[tuple[str, str]]: + """``(dotted key, reason)`` of the first forbidden key at ANY depth of *value*, else None. + A nested mapping (or a mapping inside a list) is the same contract as the top level (#85050).""" + if isinstance(value, dict): + for key, child in value.items(): + if isinstance(key, str) and (reason := _forbidden_key_reason(key)): + return f"{path}{key}", reason + if found := _first_forbidden_key(child, f"{path}{key}."): + return found + elif isinstance(value, list): + for child in value: + if found := _first_forbidden_key(child, path): + return found + return None + + +def _strip_forbidden_keys(value: Any) -> Any: + """Copy of *value* with forbidden keys and non-YAML-scalar leaves removed at every depth.""" + if isinstance(value, dict): + return { + key: _strip_forbidden_keys(child) for key, child in value.items() + if isinstance(key, str) and _forbidden_key_reason(key) is None + and (child is None or isinstance(child, (str, int, float, bool, list, dict))) + } + if isinstance(value, list): + return [_strip_forbidden_keys(child) for child in value] + return value + + def validate_config_seed(plugin_id: str, seed: Any) -> dict[str, Any]: """Validate one plugin's config seed mapping and return a copy. Rejects non-dict seeds, - reserved consent keys, ``allow_*`` trust gates, and secret-shaped keys.""" + reserved consent keys, ``allow_*`` trust gates, and secret-shaped keys — at any depth.""" if not isinstance(seed, dict): raise PackError( f"Pack config for plugin '{plugin_id}' must be a mapping of plugins.entries.{plugin_id} keys.") for key in seed: if not isinstance(key, str) or not key.strip(): raise PackError(f"Pack config for plugin '{plugin_id}' has an invalid key: {key!r}.") - if key in _RESERVED_ENTRY_KEYS or key.startswith("allow_"): + found = _first_forbidden_key(seed) + if found is not None: + key, reason = found + if reason == "reserved": raise PackError( f"Pack config for plugin '{plugin_id}' sets reserved key " f"'{key}': packs cannot pre-grant capabilities or trust gates. " "Capability consent happens interactively at install time.") - if _SECRET_KEY_RE.search(key): - raise PackError( - f"Pack config for plugin '{plugin_id}' sets secret-shaped key " - f"'{key}': secrets never travel in packs. Declare the secret in " - "the plugin's requires_env instead — it is prompted at install.") + raise PackError( + f"Pack config for plugin '{plugin_id}' sets secret-shaped key " + f"'{key}': secrets never travel in packs. Declare the secret in " + "the plugin's requires_env instead — it is prompted at install.") return dict(seed) @@ -398,7 +438,8 @@ def _source_to_repo_subdir(source: str) -> tuple[Optional[str], Optional[str]]: def _sanitized_entry_config(plugin_id: str) -> dict[str, Any]: - """Exportable plugins.entries. keys: scalars only, secrets stripped.""" + """Exportable plugins.entries. keys: YAML scalars/containers only, reserved and + secret-shaped keys stripped at every depth.""" try: from hermes_cli.config import load_config @@ -408,14 +449,7 @@ def _sanitized_entry_config(plugin_id: str) -> dict[str, Any]: entry = ((config.get("plugins") or {}).get("entries") or {}).get(plugin_id) if not isinstance(entry, dict): return {} - return { - key: value for key, value in entry.items() - if isinstance(key, str) - and key not in _RESERVED_ENTRY_KEYS - and not key.startswith("allow_") - and not _SECRET_KEY_RE.search(key) - and (value is None or isinstance(value, (str, int, float, bool, list, dict))) - } + return _strip_forbidden_keys(entry) def export_pack(*, enabled_only: bool = False, pack_name: str = "my-hermes-pack") -> tuple[str, List[str]]: diff --git a/tests/hermes_cli/test_plugin_packs.py b/tests/hermes_cli/test_plugin_packs.py index c9c80cc73a..e1e5e06cb0 100644 --- a/tests/hermes_cli/test_plugin_packs.py +++ b/tests/hermes_cli/test_plugin_packs.py @@ -150,6 +150,32 @@ def test_config_seed_rejects_capability_and_trust_gate_keys(): assert "reserved" in str(exc.value) +@pytest.mark.parametrize("seed, fragment", [ + ({"settings": {"api_key": "LEAK"}}, "secret-shaped key 'settings.api_key'"), + ({"security": {"granted_capabilities": ["tools"]}}, "reserved key 'security.granted_capabilities'"), + ({"profiles": [{"allow_tool_override": True}]}, "reserved key 'profiles.allow_tool_override'"), +]) +def test_config_seed_rejects_forbidden_keys_at_any_depth(seed, fragment): + """Nesting a secret/consent key under another mapping (or a list) is the same contract + violation as setting it at the top level (#85050).""" + with pytest.raises(PackError) as exc: + validate_config_seed("p", seed) + assert fragment in str(exc.value) + assert validate_config_seed("p", {"settings": {"voice": "nova", "tags": ["a"]}}) == { + "settings": {"voice": "nova", "tags": ["a"]}} + + +def test_sanitized_entry_config_strips_forbidden_keys_at_any_depth(): + fake_cfg = {"plugins": {"entries": {"tts": { + "smtp": {"host": "mail.example", "password": "hunter2"}, + "rules": [{"name": "r1", "auth_token": "t"}], + "voice": "nova", + }}}} + with mock.patch("hermes_cli.config.load_config", return_value=fake_cfg): + assert real_sanitized_entry_config("tts") == { + "smtp": {"host": "mail.example"}, "rules": [{"name": "r1"}], "voice": "nova"} + + def test_parse_pack_validates_config_section(): text = _pack_yaml(config={"tts-plugin": {"granted_capabilities": ["tools"]}}) with pytest.raises(PackError): From 665bf373e7d3d7e95c875aa36def6a3ec291535c Mon Sep 17 00:00:00 2001 From: teknium1 <127238744+teknium1@users.noreply.github.com> Date: Mon, 21 Sep 2026 22:43:09 -0700 Subject: [PATCH 042/173] chore(contributors): map bsdnn's email for attribution --- contributors/emails/1600332149@qq.com | 1 + 1 file changed, 1 insertion(+) create mode 100644 contributors/emails/1600332149@qq.com diff --git a/contributors/emails/1600332149@qq.com b/contributors/emails/1600332149@qq.com new file mode 100644 index 0000000000..8632f31d04 --- /dev/null +++ b/contributors/emails/1600332149@qq.com @@ -0,0 +1 @@ +bsdnn From 21453b30587d659366361d859639a6587c326f3a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Muhammed=20Furkan=20Ak=C4=B1nc=C4=B1?= Date: Tue, 22 Sep 2026 08:20:02 +0300 Subject: [PATCH 043/173] fix(scratch): preserve trees when idle scans are incomplete --- hermes_constants_scratch.py | 11 +++--- tests/test_scratch_scan_failures.py | 60 +++++++++++++++++++++++++++++ 2 files changed, 66 insertions(+), 5 deletions(-) create mode 100644 tests/test_scratch_scan_failures.py diff --git a/hermes_constants_scratch.py b/hermes_constants_scratch.py index 46d6e03c8f..68c54805dc 100644 --- a/hermes_constants_scratch.py +++ b/hermes_constants_scratch.py @@ -28,14 +28,15 @@ def subtree_touched_since(path: Path, cutoff: float) -> bool: Stops at the first recent entry, so a live tree costs one hit and only a truly idle tree pays for the full walk (once, right before it is deleted). Symlinks are never followed: a link into the repo would make the target's activity keep the entry alive. + An unreadable entry is kept: an incomplete scan cannot establish that it is idle. """ try: if os.lstat(path).st_mtime >= cutoff: return True + if not path.is_dir() or path.is_symlink(): + return False except OSError: - return False - if not path.is_dir() or path.is_symlink(): - return False + return True stack = [str(path)] while stack: try: @@ -45,11 +46,11 @@ def subtree_touched_since(path: Path, cutoff: float) -> bool: if child.stat(follow_symlinks=False).st_mtime >= cutoff: return True except OSError: - continue + return True if child.is_dir(follow_symlinks=False): stack.append(child.path) except OSError: - continue + return True return False diff --git a/tests/test_scratch_scan_failures.py b/tests/test_scratch_scan_failures.py new file mode 100644 index 0000000000..7850a7226a --- /dev/null +++ b/tests/test_scratch_scan_failures.py @@ -0,0 +1,60 @@ +"""Cleanup needs a complete idle scan before deleting a scratch tree.""" + +import os +import time + +import pytest + +import hermes_constants_scratch as scratch + + +@pytest.mark.parametrize("failed_probe", ["root-stat", "directory-scan", "child-stat"]) +def test_unreadable_subtree_is_preserved_until_it_can_be_scanned(tmp_path, monkeypatch, failed_probe): + root = tmp_path / "scratch" + entry = root / "lane" + entry.mkdir(parents=True) + result = entry / "result.txt" + result.write_text("keep until activity can be checked", encoding="utf-8") + old = time.time() - 48 * 3600 + os.utime(result, (old, old)) + os.utime(entry, (old, old)) + # Process inspection is unrelated to determining whether the files are idle. + monkeypatch.setattr(scratch, "reap_processes_rooted_in", lambda *_: 0) + real_lstat, real_scandir = os.lstat, os.scandir + with monkeypatch.context() as probe: + if failed_probe == "root-stat": + def lstat(path, *args, **kwargs): + if path == entry: + raise PermissionError("cannot inspect entry") + return real_lstat(path, *args, **kwargs) + probe.setattr(scratch.os, "lstat", lstat) + else: + def scandir(path): + if path == str(entry): + if failed_probe == "directory-scan": + raise PermissionError("cannot inspect subtree") + + class UnreadableChild: + name = "result.txt" + + def stat(self, **_kwargs): + raise OSError("cannot inspect child activity") + + def is_dir(self, **_kwargs): + return False + + class Scan: + def __enter__(self): + return iter([UnreadableChild()]) + + def __exit__(self, *_args): + pass + + return Scan() + return real_scandir(path) + probe.setattr(scratch.os, "scandir", scandir) + assert scratch.prune_idle_entries(root, 24, frozenset()) == 0 + assert result.read_text(encoding="utf-8") == "keep until activity can be checked" + # Once observation succeeds, the same genuinely idle tree is reclaimable. + assert scratch.prune_idle_entries(root, 24, frozenset()) == 1 + assert not entry.exists() From f5ed9097497f9fccc55d669c64f7b22c34914918 Mon Sep 17 00:00:00 2001 From: teknium1 <127238744+teknium1@users.noreply.github.com> Date: Mon, 21 Sep 2026 23:58:02 -0700 Subject: [PATCH 044/173] chore: map contributor email for @mfurkanakinci --- contributors/emails/mfurkanakinci@gmail.com | 1 + 1 file changed, 1 insertion(+) create mode 100644 contributors/emails/mfurkanakinci@gmail.com diff --git a/contributors/emails/mfurkanakinci@gmail.com b/contributors/emails/mfurkanakinci@gmail.com new file mode 100644 index 0000000000..2d32ae7e29 --- /dev/null +++ b/contributors/emails/mfurkanakinci@gmail.com @@ -0,0 +1 @@ +mfurkanakinci From b54c9568e70d3309bfc1fe786d0b731d58eb4a86 Mon Sep 17 00:00:00 2001 From: Siddharth Balyan <52913345+alt-glitch@users.noreply.github.com> Date: Tue, 22 Sep 2026 12:42:07 +0530 Subject: [PATCH 045/173] feat(desktop): choose plugin install profile (#118889) --- .../settings/plugin-install-modal.test.tsx | 46 +++++++++++- .../src/app/settings/plugin-install-modal.tsx | 70 +++++++++++++------ apps/desktop/src/i18n/en.ts | 1 + apps/desktop/src/i18n/ru.ts | 1 + apps/desktop/src/i18n/types.ts | 1 + apps/desktop/src/i18n/zh.ts | 1 + 6 files changed, 96 insertions(+), 24 deletions(-) diff --git a/apps/desktop/src/app/settings/plugin-install-modal.test.tsx b/apps/desktop/src/app/settings/plugin-install-modal.test.tsx index e2c1a59cdc..5ad620907a 100644 --- a/apps/desktop/src/app/settings/plugin-install-modal.test.tsx +++ b/apps/desktop/src/app/settings/plugin-install-modal.test.tsx @@ -24,7 +24,7 @@ import { closePluginInstallRequest, openPluginInstallRequest } from '@/store/plugin-install-request' -import { $activeGatewayProfile } from '@/store/profile' +import { $activeGatewayProfile, $profiles } from '@/store/profile' import { $connection, $gatewayState } from '@/store/session' import { PluginsTab } from '../capabilities/plugins/plugins-tab' @@ -46,10 +46,32 @@ const renderFlow = () => beforeEach(() => { vi.clearAllMocks() + Object.defineProperty(Element.prototype, 'scrollIntoView', { configurable: true, value: vi.fn() }) queryClient.clear() closePluginInstallRequest() $gatewayState.set('idle') $activeGatewayProfile.set('default') + $profiles.set([ + { + has_env: false, + is_default: true, + model: null, + name: 'default', + path: '/profiles/default', + provider: null, + skill_count: 0 + }, + { + display_name: 'Research Bot', + has_env: false, + is_default: false, + model: null, + name: 'research', + path: '/profiles/research', + provider: null, + skill_count: 0 + } + ]) probePluginRepo.mockResolvedValue({ ok: true, agent: true, desktop: true, warnings: [] }) vi.stubGlobal('hermesDesktop', { probePluginRepo, installDesktopPlugin }) }) @@ -125,6 +147,28 @@ describe('Install from Git entry flow', () => { expect(installDesktopPlugin).not.toHaveBeenCalled() }) + it('installs a deep-linked agent plugin into the selected profile', async () => { + probePluginRepo.mockResolvedValue({ ok: true, agent: true, desktop: false, warnings: [] }) + requestGateway.mockImplementation(async method => + method === 'plugins.manage' ? { ok: true, plugin_name: 'plugin', plugins: [] } : { plugins: [] } + ) + renderFlow() + act(() => openPluginInstallRequest({ catalogName: 'plugin', repo: 'https://github.com/example/plugin' })) + + const profile = await screen.findByRole('combobox', { name: 'Install for profile' }) + + fireEvent.click(profile) + fireEvent.click(await screen.findByRole('option', { name: 'Research Bot' })) + fireEvent.click(screen.getByRole('button', { name: 'Install' })) + + await waitFor(() => + expect(requestGateway).toHaveBeenCalledWith( + 'plugins.manage', + expect.objectContaining({ action: 'install', catalog_name: 'plugin', profile: 'research' }) + ) + ) + }) + it('pins a custom install to a full commit SHA and refuses anything shorter', async () => { probePluginRepo.mockResolvedValue({ ok: true, agent: true, desktop: false, warnings: [] }) requestGateway.mockImplementation(async method => diff --git a/apps/desktop/src/app/settings/plugin-install-modal.tsx b/apps/desktop/src/app/settings/plugin-install-modal.tsx index ea111cdc79..355cf74269 100644 --- a/apps/desktop/src/app/settings/plugin-install-modal.tsx +++ b/apps/desktop/src/app/settings/plugin-install-modal.tsx @@ -16,6 +16,7 @@ import { preventCloseButtonAutoFocus } from '@/components/ui/dialog' import { Input } from '@/components/ui/input' +import { Select, SelectContent, SelectItem, SelectTrigger, SelectValue } from '@/components/ui/select' import { Switch } from '@/components/ui/switch' import { discoverRuntimePlugins } from '@/contrib/runtime-loader' import { useI18n } from '@/i18n' @@ -30,7 +31,7 @@ import { openPluginInstallRequest, type PluginInstallRequest } from '@/store/plugin-install-request' -import { $activeGatewayProfile, $profileScope } from '@/store/profile' +import { $activeGatewayProfile, $profiles, $profileScope, normalizeProfileKey, profileLabel } from '@/store/profile' import { $connection } from '@/store/session' import { runGatewayRestart } from '@/store/system-actions' @@ -48,9 +49,11 @@ export function PluginInstallModal() { const onSettings = location.pathname.startsWith(SETTINGS_ROUTE) const connection = useStore($connection) const activeProfile = useStore($activeGatewayProfile) + const profiles = useStore($profiles) const profileScope = useStore($profileScope) const [repoInput, setRepoInput] = useState('') + const [targetProfile, setTargetProfile] = useState('default') const [phase, setPhase] = useState('idle') const [probe, setProbe] = useState(null) const [installAgent, setInstallAgent] = useState(true) @@ -151,21 +154,23 @@ export function PluginInstallModal() { return } + setTargetProfile(normalizeProfileKey(request.profile || activeProfile || profileScope)) + if (request.repo) { void runProbe(request) } - }, [request, resetState, runProbe]) + }, [activeProfile, profileScope, request, resetState, runProbe]) - const profileLabel = request?.profile || activeProfile || profileScope || 'default' + const targetProfileInfo = profiles.find(profile => normalizeProfileKey(profile.name) === targetProfile) + const profileOptions = targetProfileInfo ? profiles : [...profiles, { name: targetProfile }] + const targetProfileLabel = profileLabel(targetProfileInfo ?? { name: targetProfile }) const agentTargetHint = connection?.mode === 'remote' - ? m.agentTargetRemote(profileLabel) + ? m.agentTargetRemote(targetProfileLabel) : m.agentTargetLocal( - profileLabel, - request?.profile && request.profile !== 'default' - ? `~/.hermes/profiles/${request.profile}/plugins/` - : '~/.hermes/plugins/' + targetProfileLabel, + targetProfile === 'default' ? '~/.hermes/plugins/' : `~/.hermes/profiles/${targetProfile}/plugins/` ) // A unified package installed into a local backend carries its own desktop @@ -211,7 +216,7 @@ export function PluginInstallModal() { enable: enableAgent, catalogName: request.catalogName, ref: pinRefTrimmed || undefined, - profile: request.profile + profile: targetProfile }) if (result.ok) { @@ -272,7 +277,7 @@ export function PluginInstallModal() { } } - await loadAgentPlugins(requestGateway) + await loadAgentPlugins(requestGateway, targetProfile) if (errors.length === 0) { for (const message of successes) { @@ -419,20 +424,39 @@ export function PluginInstallModal() { {probe.agent && ( -