From 88b31637422cc8bbbda67d86b46fda2b345dcf0a Mon Sep 17 00:00:00 2001 From: Konstantin Khlopkov Date: Thu, 17 Sep 2026 15:18:38 +0300 Subject: [PATCH] fix(plugins): one kill-list resolution per plugins hub rebuild and CLI listing --- hermes_cli/plugin_catalog.py | 60 ++++- hermes_cli/plugins_cmd.py | 14 +- hermes_cli/plugins_cmd_catalog.py | 36 ++- hermes_cli/web_server_dashboard.py | 15 +- .../test_plugins_hub_live_catalog.py | 251 ++++++++++++++++++ 5 files changed, 363 insertions(+), 13 deletions(-) create mode 100644 tests/hermes_cli/test_plugins_hub_live_catalog.py diff --git a/hermes_cli/plugin_catalog.py b/hermes_cli/plugin_catalog.py index ee57b843d1..dd6c41af14 100644 --- a/hermes_cli/plugin_catalog.py +++ b/hermes_cli/plugin_catalog.py @@ -32,6 +32,7 @@ CATALOG_TIERS = ("official", "community") CATALOG_CATEGORIES = ("desktop", "memory", "platform", "web", "tools", "voice", "automation", "models", "general") LIVE_CATALOG_URL = "https://hermes-agent.nousresearch.com/docs/api/plugin-catalog.json" LIVE_CATALOG_TTL_SECONDS = 6 * 60 * 60 +LIVE_CATALOG_FAILURE_TTL_SECONDS = 60.0 _REQUEST_TIMEOUT = 5.0 _MAX_LIVE_BYTES = 2 * 1024 * 1024 @@ -235,10 +236,31 @@ def find_removed(name_or_repo: str, catalog_dir: Optional[Path] = None) -> Optio """ if not name_or_repo: return None - candidate = name_or_repo.strip() - candidate_repo = _normalize_repo(candidate) - for entry in load_removed_list(catalog_dir) + (live_removed_list() if catalog_dir is None else []): - if candidate == entry.name or (entry.repo and candidate_repo == _normalize_repo(entry.repo)): + entries = load_removed_list(catalog_dir) + if catalog_dir is None: + entries = entries + live_removed_list() + return match_removed(name_or_repo, entries) + + +def resolved_removed_entries() -> List[RemovedEntry]: + """The full kill list (in-tree UNION live) in one resolution. Callers that match many candidates + — e.g. a plugins-hub rebuild annotating every installed plugin — resolve the list once instead + of paying a live-catalog fetch per candidate.""" + return load_removed_list() + live_removed_list() + + +def match_removed( + candidate: str, entries: List[RemovedEntry] +) -> Optional[RemovedEntry]: + """One candidate against a pre-resolved kill list: exact name or normalized repo URL match.""" + if not candidate: + return None + text = candidate.strip() + text_repo = _normalize_repo(text) + for entry in entries: + if text == entry.name or ( + entry.repo and text_repo == _normalize_repo(entry.repo) + ): return entry return None @@ -250,10 +272,37 @@ def _live_cache_path() -> Path: return get_hermes_home() / "cache" / "plugin-catalog.json" +# Last failed live fetch: without it, a dead catalog host costs one full request +# timeout PER CALLER (the plugins hub alone asks once per installed plugin), so +# the dashboard event loop stalls for minutes. A remembered failure keeps those +# callers on the in-tree copy until the TTL lets one fresh attempt through. +_live_fetch_failed_until: Dict[str, float] = {} + + +def _live_fetch_failure_recent() -> bool: + failed_until = _live_fetch_failed_until.get(LIVE_CATALOG_URL) + return failed_until is not None and time.time() < failed_until + + +def _remember_live_fetch_failure() -> None: + failed_until = time.time() + LIVE_CATALOG_FAILURE_TTL_SECONDS + _live_fetch_failed_until[LIVE_CATALOG_URL] = failed_until + + def fetch_live_catalog(*, force: bool = False) -> Optional[Dict[str, Any]]: """The published ``plugin-catalog.json`` (``{"entries": [...], "removed": [...]}``), cached under ``HERMES_HOME/cache`` for :data:`LIVE_CATALOG_TTL_SECONDS`. ``None`` on ANY failure — callers fall - back to the in-tree catalog.""" + back to the in-tree catalog. A failed network attempt is remembered for + :data:`LIVE_CATALOG_FAILURE_TTL_SECONDS` so a dead host costs one timeout per TTL window, not one + per caller (``force`` bypasses both caches).""" + if not force and _live_fetch_failure_recent(): + try: # stale cache still beats the in-tree copy when the network is down + cache = _live_cache_path() + if cache.is_file(): + return json.loads(cache.read_text(encoding="utf-8")) + except Exception: + pass + return None cache = _live_cache_path() try: if not force and cache.is_file() and time.time() - cache.stat().st_mtime < LIVE_CATALOG_TTL_SECONDS: @@ -276,6 +325,7 @@ def fetch_live_catalog(*, force: bool = False) -> Optional[Dict[str, Any]]: return data except Exception as exc: logger.debug("Plugin catalog: live fetch failed: %s", exc) + _remember_live_fetch_failure() try: # stale cache still beats the in-tree copy when the network is down return json.loads(cache.read_text(encoding="utf-8")) if cache.is_file() else None except Exception: diff --git a/hermes_cli/plugins_cmd.py b/hermes_cli/plugins_cmd.py index 769c03f8ac..58e6d07c85 100644 --- a/hermes_cli/plugins_cmd.py +++ b/hermes_cli/plugins_cmd.py @@ -1432,10 +1432,18 @@ def cmd_list(args: Any | None = None) -> None: # Source shows catalog provenance (``catalog:@``) or a ``--ref`` pin # (``git pinned@``) so a team can eyeball that everyone runs the same commit. pins = _read_install_metadata() + # One kill-list resolution for the whole listing: resolving per row costs a live-catalog + # fetch per installed plugin when the catalog host is slow or unreachable. + resolved_removed = catalog.resolved_removed_entries() rows = [ - (name, _plugin_status(name, enabled, disabled, key=key), str(version), description, - catalog.catalog_annotation(_dir) or _pin_annotation(name, pins) or source, - catalog.removed_annotation(name, _dir)) + ( + name, + _plugin_status(name, enabled, disabled, key=key), + str(version), + description, + catalog.catalog_annotation(_dir) or _pin_annotation(name, pins) or source, + catalog.removed_annotation(name, _dir, removed_entries=resolved_removed), + ) for name, version, description, source, _dir, key in entries ] diff --git a/hermes_cli/plugins_cmd_catalog.py b/hermes_cli/plugins_cmd_catalog.py index 67c9497ec8..c29c719326 100644 --- a/hermes_cli/plugins_cmd_catalog.py +++ b/hermes_cli/plugins_cmd_catalog.py @@ -18,6 +18,7 @@ from hermes_cli.plugin_catalog import ( PluginCatalogEntry, entry_capability_summary, filter_entries, find_removed, get_live_catalog_entry, load_catalog_live, load_removed_list, _NAME_RE, ) +from hermes_cli.plugin_catalog import match_removed, resolved_removed_entries logger = logging.getLogger(__name__) @@ -93,8 +94,14 @@ def catalog_annotation(dir_path) -> Optional[str]: return f"catalog:{sidecar.get('tier') or 'community'}@{str(sidecar.get('sha') or '')[:8]}" -def removed_annotation(name: str, dir_path) -> Optional[str]: - """Kill-list reason when an INSTALLED plugin matches by name, catalog name or repo, else ``None``.""" +def removed_annotation(name: str, dir_path, *, removed_entries=None) -> Optional[str]: + """Kill-list reason when an INSTALLED plugin matches by name, catalog name or repo, else ``None``. + + ``removed_entries`` reuses one pre-resolved kill list (``resolved_removed_entries()``) across + many rows; resolving per row costs a live-catalog fetch per installed plugin. + """ + if removed_entries is not None: + return removed_annotation_batch([(name, dir_path)], removed_entries).get(name) sidecar = read_catalog_sidecar(dir_path) or {} for candidate in (name, sidecar.get("catalog_name"), sidecar.get("repo")): removed = find_removed(str(candidate)) if candidate else None @@ -103,6 +110,31 @@ def removed_annotation(name: str, dir_path) -> Optional[str]: return None +def removed_annotation_batch( + names_and_dirs, removed_entries +) -> Dict[str, Optional[str]]: + """``removed_annotation`` for many plugins against ONE pre-resolved kill list. + + The dashboard plugins hub annotates every installed plugin on every rebuild; resolving the kill + list per plugin turns the hub into one live-catalog fetch per row. Callers resolve the list once + (:func:`plugin_catalog.resolved_removed_entries`) and pass it here. + """ + annotations: Dict[str, Optional[str]] = {} + for name, dir_path in names_and_dirs: + sidecar = read_catalog_sidecar(dir_path) or {} + candidates = (name, sidecar.get("catalog_name"), sidecar.get("repo")) + for candidate in candidates: + if not candidate: + continue + removed = match_removed(str(candidate), removed_entries) + if removed is not None: + annotations[name] = removed.reason or "no reason recorded" + break + else: + annotations[name] = None + return annotations + + # ── Catalog-aware install / update ─────────────────────────────────────────── def install_catalog_entry(entry: PluginCatalogEntry, *, force: bool, ref: Optional[str] = None, diff --git a/hermes_cli/web_server_dashboard.py b/hermes_cli/web_server_dashboard.py index e99ef1c773..702a295b30 100644 --- a/hermes_cli/web_server_dashboard.py +++ b/hermes_cli/web_server_dashboard.py @@ -674,7 +674,8 @@ def _merged_plugins_hub(force_refresh: bool = False) -> Dict[str, Any]: _get_enabled_set, _read_manifest as _read_plugin_manifest_at, ) - from hermes_cli.plugins_cmd_catalog import removed_annotation + from hermes_cli.plugins_cmd_catalog import removed_annotation_batch + from hermes_cli.plugin_catalog import resolved_removed_entries dashboard_list = _get_dashboard_plugins() dash_by_name = {str(p["name"]): p for p in dashboard_list} @@ -684,7 +685,15 @@ def _merged_plugins_hub(force_refresh: bool = False) -> Dict[str, Any]: plugins_root_resolved = (get_hermes_home() / "plugins").resolve() rows: List[Dict[str, Any]] = [] - for name, version, description, source, dir_str, key in _discover_all_plugins(): + discovered = _discover_all_plugins() + # One kill-list resolution for the whole rebuild: resolving per row costs a live-catalog + # fetch per installed plugin when the catalog host is slow or unreachable. + removed_annotations = removed_annotation_batch( + ((name, dir_str) for name, _v, _d, _s, dir_str, _k in discovered), + resolved_removed_entries(), + ) + + for name, version, description, source, dir_str, key in discovered: # Both the path-derived key (nested category plugins) and the bare manifest name # count for enabled/disabled state, matching the runtime loader's back-compat lookup. aliases = {name, key} if key else {name} @@ -716,7 +725,7 @@ def _merged_plugins_hub(force_refresh: bool = False) -> Dict[str, Any]: "auth_required": auth_required, "auth_command": auth_command, "user_hidden": name in hidden_plugins, - "removed_reason": removed_annotation(name, dir_str), + "removed_reason": removed_annotations.get(name), }) agent_names = {r["name"] for r in rows} diff --git a/tests/hermes_cli/test_plugins_hub_live_catalog.py b/tests/hermes_cli/test_plugins_hub_live_catalog.py new file mode 100644 index 0000000000..94885b11a4 --- /dev/null +++ b/tests/hermes_cli/test_plugins_hub_live_catalog.py @@ -0,0 +1,251 @@ +"""Plugins hub must not multiply live-catalog fetches (issue #113677 fix contract). + +A hub rebuild annotates every installed plugin with the kill-list reason; resolving the kill list +per row cost one synchronous catalog HTTPS request per candidate, so a slow/unreachable catalog +host stalled the dashboard event loop for minutes. These tests pin the two halves of the fix: +one kill-list resolution per rebuild, and a remembered failed fetch within a short TTL window. +""" + +from __future__ import annotations + +import json +import threading +from pathlib import Path +from types import SimpleNamespace + +import pytest + +from hermes_cli import plugin_catalog as pc +from hermes_cli import plugins_cmd_catalog as pcc +from hermes_cli import web_server +import hermes_cli.config as _cfg_mod +import hermes_cli.web_server_dashboard as _web_server_dashboard +import hermes_cli.web_server_memory as _web_server_memory +from hermes_cli import plugins_cmd +from tools import registry as tools_registry + + +_PLUGIN_ROWS = [ + ("demo", "1.0.0", "demo plugin", "user", "/tmp/demo-plugin", "demo"), + ("second", "0.2.0", "second plugin", "user", "/tmp/second-plugin", "second"), +] + + +@pytest.fixture(autouse=True) +def _reset_live_fetch_state(monkeypatch, tmp_path): + monkeypatch.setattr(pc, "_live_fetch_failed_until", {}) + monkeypatch.setattr( + pc, "_live_cache_path", lambda: tmp_path / "cache" / "plugin-catalog.json" + ) + tools_registry.invalidate_check_fn_cache() + _web_server_dashboard._invalidate_plugins_hub_cache() + + +class _UnreachableCatalog: + """Counts network attempts; every attempt blocks like a real timeout, then fails.""" + + def __init__(self): + self.attempts = 0 + + def __call__(self, *args, **kwargs): + self.attempts += 1 + raise OSError("catalog host unreachable") + + +def _patch_hub_dependencies(monkeypatch, *, rows=None): + rows = rows if rows is not None else list(_PLUGIN_ROWS) + monkeypatch.setattr( + web_server, "_get_dashboard_plugins", lambda force_rescan=False: [] + ) + monkeypatch.setattr( + _web_server_memory, "_discover_memory_provider_statuses", lambda: [] + ) + monkeypatch.setattr(_cfg_mod, "get_hermes_home", lambda: Path("/tmp/hermes-home")) + monkeypatch.setattr( + _cfg_mod, "load_config", lambda: {"dashboard": {"hidden_plugins": []}} + ) + monkeypatch.setattr(plugins_cmd, "_discover_all_plugins", lambda: rows) + monkeypatch.setattr( + plugins_cmd, "_get_current_context_engine", lambda: "compressor" + ) + monkeypatch.setattr(plugins_cmd, "_get_current_memory_provider", lambda: "") + monkeypatch.setattr(plugins_cmd, "_discover_context_engines", lambda: []) + monkeypatch.setattr(plugins_cmd, "_get_disabled_set", lambda: set()) + monkeypatch.setattr(plugins_cmd, "_get_enabled_set", lambda: {"demo"}) + monkeypatch.setattr( + plugins_cmd, "_read_manifest", lambda _path: {"provides_tools": []} + ) + monkeypatch.setattr( + tools_registry.registry, + "get_entry", + lambda _name: SimpleNamespace(check_fn=None), + ) + + +def test_hub_rebuild_issues_one_network_attempt_then_none(monkeypatch): + """An unreachable catalog host costs ONE timeout per rebuild (not one per installed plugin), + and rebuilds inside the failure window cost none at all.""" + unreachable = _UnreachableCatalog() + monkeypatch.setattr("httpx.get", unreachable) + + _patch_hub_dependencies(monkeypatch) + + payload = _web_server_dashboard._merged_plugins_hub(force_refresh=True) + assert len(payload["plugins"]) == len(_PLUGIN_ROWS) + assert all(row["removed_reason"] is None for row in payload["plugins"]) + assert unreachable.attempts == 1 + + _web_server_dashboard._invalidate_plugins_hub_cache() + _web_server_dashboard._merged_plugins_hub(force_refresh=True) + assert unreachable.attempts == 1 # remembered failure: no second timeout + + +def test_failed_fetch_is_remembered_only_within_ttl(monkeypatch): + clock = {"now": 1_000_000.0} + attempts = {"count": 0} + + def fake_get(url, **kwargs): + attempts["count"] += 1 + raise OSError("catalog host unreachable") + + monkeypatch.setattr(pc.time, "time", lambda: clock["now"]) + monkeypatch.setattr("httpx.get", fake_get) + + assert pc.fetch_live_catalog() is None + pc.fetch_live_catalog() # remembered failure: served without a second attempt + assert attempts["count"] == 1 + + clock["now"] += pc.LIVE_CATALOG_FAILURE_TTL_SECONDS + 1 + assert ( + pc.fetch_live_catalog() is None + ) # TTL expired: one fresh attempt reaches the network + assert attempts["count"] == 2 + + +def test_failure_window_prefers_stale_cache_over_in_tree(monkeypatch, tmp_path): + """Inside the failure window a previously fetched on-disk cache still answers — removals + published before the outage keep blocking without any network traffic.""" + cache = tmp_path / "cache" / "plugin-catalog.json" + cache.parent.mkdir(parents=True) + cache.write_text( + json.dumps({ + "entries": [], + "removed": [{"name": "pulled-live", "reason": "cve"}], + }) + ) + monkeypatch.setattr("httpx.get", _UnreachableCatalog()) + + data = pc.fetch_live_catalog() + assert data is not None and data["removed"][0]["name"] == "pulled-live" + assert pc.fetch_live_catalog() is not None # still served from the stale cache + + +def test_batch_annotation_matches_per_plugin_lookup(monkeypatch, tmp_path): + """The batch resolver returns exactly what per-plugin ``removed_annotation`` returns, across + name matches, sidecar catalog names and repo URLs.""" + kill_list = [ + pc.RemovedEntry( + name="evil", repo="https://github.com/x/evil.git", reason="malware" + ), + pc.RemovedEntry(name="pulled-live", reason="cve"), + ] + monkeypatch.setattr(pc, "load_removed_list", lambda *a, **k: kill_list) + monkeypatch.setattr(pc, "live_removed_list", lambda: []) + + sidecar_dir = tmp_path / "installed-from-catalog" + sidecar_dir.mkdir() + (sidecar_dir / pcc.CATALOG_SIDECAR).write_text( + json.dumps({ + "catalog_name": "pulled-live", + "repo": "https://github.com/x/other", + "sha": "0" * 40, + "tier": "community", + }) + ) + repo_dir = tmp_path / "installed-from-repo" + repo_dir.mkdir() + (repo_dir / pcc.CATALOG_SIDECAR).write_text( + json.dumps({ + "catalog_name": "unknown", + "repo": "https://github.com/x/EVIL/", + "sha": "0" * 40, + "tier": "community", + }) + ) + plain_dir = tmp_path / "plain" + plain_dir.mkdir() + + plugins = [ + ("evil", plain_dir), + ("catalog-install", sidecar_dir), + ("repo-install", repo_dir), + ("fine", plain_dir), + ] + batch = pcc.removed_annotation_batch(plugins, pc.resolved_removed_entries()) + + for name, dir_path in plugins: + assert batch[name] == pcc.removed_annotation(name, dir_path) + assert batch["evil"] == "malware" + assert batch["catalog-install"] == "cve" + assert ( + batch["repo-install"] == "malware" + ) # repo match is .git/trailing-slash insensitive + assert batch["fine"] is None + + +def test_hub_rebuild_annotates_from_preloaded_kill_list(monkeypatch, tmp_path): + """The hub surfaces removal reasons from ONE resolved kill list even when the live catalog + is unreachable — an installed removed plugin is still reported, at hub-rebuild speed.""" + monkeypatch.setattr("httpx.get", _UnreachableCatalog()) + monkeypatch.setattr(pc, "get_catalog_dir", lambda: tmp_path) + (tmp_path / "removed.yaml").write_text( + "removed:\n- name: demo\n reason: exfiltrated env vars\n" + ) + + _patch_hub_dependencies(monkeypatch) + payload = _web_server_dashboard._merged_plugins_hub(force_refresh=True) + + by_name = {row["name"]: row["removed_reason"] for row in payload["plugins"]} + assert by_name["demo"] == "exfiltrated env vars" + assert by_name["second"] is None + + +def test_cmd_list_resolves_kill_list_once(monkeypatch, tmp_path, capsys): + """``hermes plugins list`` resolves the kill list ONCE per listing, not per row: a slow or + unreachable live catalog must cost one resolution (one network attempt at most) regardless + of how many plugins are installed.""" + import argparse + + calls = {"load_removed_list": 0, "live_removed_list": 0} + kill_list = [pc.RemovedEntry(name="pulled-plugin", reason="security review")] + + def counted_load_removed(*args, **kwargs): + calls["load_removed_list"] += 1 + return kill_list + + def counted_live_removed(): + calls["live_removed_list"] += 1 + return [] + + monkeypatch.setattr(pc, "load_removed_list", counted_load_removed) + monkeypatch.setattr(pc, "live_removed_list", counted_live_removed) + monkeypatch.setattr( + plugins_cmd, "_discover_all_plugins", + lambda: [(f"plugin-{i}", "1.0", "", "user", tmp_path / str(i), f"plugin-{i}") + for i in range(3)] + + [("pulled-plugin", "1.0", "", "user", tmp_path / "pulled", "pulled-plugin")], + ) + monkeypatch.setattr(plugins_cmd, "_get_enabled_set", lambda: {"plugin-0"}) + monkeypatch.setattr(plugins_cmd, "_get_disabled_set", lambda: set()) + monkeypatch.setattr(plugins_cmd, "_read_install_metadata", lambda: {}) + monkeypatch.setattr(pc, "_live_cache_path", lambda: tmp_path / "cache" / "plugin-catalog.json") + + plugins_cmd.cmd_list(argparse.Namespace( + enabled=False, user=False, no_bundled=False, plain=False, json=True)) + + assert calls["load_removed_list"] == 1 + assert calls["live_removed_list"] == 1 + rows = json.loads(capsys.readouterr().out) + by_name = {row["name"]: row["removed"] for row in rows} + assert by_name["pulled-plugin"] == "security review" + assert by_name["plugin-0"] is None