From ccacfdbd6d92c6cd0aceb585ae9b49d9b57fcd22 Mon Sep 17 00:00:00 2001 From: islam666 Date: Sun, 7 Jun 2026 08:02:11 +0000 Subject: [PATCH] fix(plugins): discover nested category plugins in 'plugins list' (issue #41066) _discover_all_plugins() previously did a flat iterdir() scan, missing all category-namespaced plugins (web/*, image_gen/*, browser/*, video_gen/*). Now recurses up to 2 levels deep, matching PluginManager._scan_directory_level(). Also fixes _plugin_status() to check both manifest name AND path-derived key against enabled/disabled sets, so category plugins like 'web/tavily' show correct status when enabled via config. --- hermes_cli/plugins_cmd.py | 135 ++++--- .../test_plugins_cmd_category_discovery.py | 355 ++++++++++++++++++ 2 files changed, 439 insertions(+), 51 deletions(-) create mode 100644 tests/hermes_cli/test_plugins_cmd_category_discovery.py diff --git a/hermes_cli/plugins_cmd.py b/hermes_cli/plugins_cmd.py index ddbd0402f2a..7f6a3314ecf 100644 --- a/hermes_cli/plugins_cmd.py +++ b/hermes_cli/plugins_cmd.py @@ -728,64 +728,97 @@ def _plugin_exists(name: str) -> bool: return False -def _discover_all_plugins() -> list: - """Return a list of (name, version, description, source, dir_path) for - every plugin the loader can see — user + bundled + project. +def _read_manifest_info(d: Path, prefix: str): + """Read a plugin.yaml manifest and return (name, version, description, key). - Matches the ordering/dedup of ``PluginManager.discover_and_load``: - bundled first, then user, then project; user overrides bundled on - name collision. + Returns None if no manifest file exists. """ + manifest_file = d / "plugin.yaml" + if not manifest_file.exists(): + manifest_file = d / "plugin.yml" + if not manifest_file.exists(): + return None try: import yaml except ImportError: yaml = None + name = d.name + version = "" + description = "" + if yaml: + try: + with open(manifest_file, encoding="utf-8") as f: + manifest = yaml.safe_load(f) or {} + name = manifest.get("name", d.name) + version = manifest.get("version", "") + description = manifest.get("description", "") + except Exception: + pass + key = f"{prefix}/{d.name}" if prefix else name + return name, version, description, key - seen: dict = {} # name -> (name, version, description, source, path) - # Bundled (/plugins//), excluding memory/ and context_engine/ - from hermes_cli.plugins import get_bundled_plugins_dir - repo_plugins = get_bundled_plugins_dir() - for base, source in ((repo_plugins, "bundled"), (_plugins_dir(), "user")): - if not base.is_dir(): +def _scan_level( + base: Path, + source: str, + skip_names: set, + prefix: str, + depth: int, + seen: dict, +) -> None: + """Recursive directory scan matching PluginManager._scan_directory_level. + + Populates *seen* with key -> (name, version, description, source, dir, key). + """ + if not base.is_dir(): + return + for d in sorted(base.iterdir()): + if not d.is_dir(): continue - for d in sorted(base.iterdir()): - if not d.is_dir(): - continue - if source == "bundled" and d.name in {"memory", "context_engine"}: - continue - manifest_file = d / "plugin.yaml" - if not manifest_file.exists(): - manifest_file = d / "plugin.yml" - if not manifest_file.exists(): - continue - name = d.name - version = "" - description = "" - if yaml: - try: - with open(manifest_file, encoding="utf-8") as f: - manifest = yaml.safe_load(f) or {} - name = manifest.get("name", d.name) - version = manifest.get("version", "") - description = manifest.get("description", "") - except Exception: - pass - # User plugins override bundled on name collision. - if name in seen and source == "bundled": + if depth == 0 and skip_names and d.name in skip_names: + continue + info = _read_manifest_info(d, prefix) + if info is not None: + name, version, description, key = info + if key in seen and source == "bundled": continue src_label = source if source == "user" and (d / ".git").exists(): src_label = "git" - seen[name] = (name, version, description, src_label, d) + seen[key] = (name, version, description, src_label, d, key) + continue + if depth >= 1: + continue + sub_prefix = f"{prefix}/{d.name}" if prefix else d.name + _scan_level(d, source, set(), sub_prefix, depth + 1, seen) + + +def _discover_all_plugins() -> list: + """Return a list of (name, version, description, source, dir_path, key) for + every plugin the loader can see — user + bundled + project. + + Matches the ordering/dedup of ``PluginManager.discover_and_load``: + bundled first, then user, then project; user overrides bundled on + key collision. + """ + seen: dict = {} # key -> (name, version, description, source, path, key) + + # Bundled (/plugins//), excluding memory/ and context_engine/ + from hermes_cli.plugins import get_bundled_plugins_dir + repo_plugins = get_bundled_plugins_dir() + for base, source, skip in ( + (repo_plugins, "bundled", {"memory", "context_engine"}), + (_plugins_dir(), "user", set()), + ): + _scan_level(base, source, skip, "", 0, seen) return list(seen.values()) -def _plugin_status(name: str, enabled: set, disabled: set) -> str: - """Return the user-facing activation state for a plugin name.""" - if name in disabled: +def _plugin_status(name: str, enabled: set, disabled: set, key: str = "") -> str: + """Return the user-facing activation state for a plugin name or key.""" + if name in disabled or key in disabled: return "disabled" - if name in enabled: + if name in enabled or key in enabled: return "enabled" return "not enabled" @@ -798,7 +831,7 @@ def _filter_plugin_entries(entries: list, args: Any, enabled: set, disabled: set if getattr(args, "enabled", False): filtered = [ entry for entry in filtered - if _plugin_status(entry[0], enabled, disabled) == "enabled" + if _plugin_status(entry[0], enabled, disabled, key=entry[5]) == "enabled" ] return filtered @@ -823,19 +856,19 @@ def cmd_list(args: Any | None = None) -> None: payload = [ { "name": name, - "status": _plugin_status(name, enabled, disabled), + "status": _plugin_status(name, enabled, disabled, key=key), "version": str(version), "description": description, "source": source, } - for name, version, description, source, _dir in entries + for name, version, description, source, _dir, key in entries ] print(json.dumps(payload, indent=2)) return if getattr(args, "plain", False): - for name, version, _description, source, _dir in entries: - status = _plugin_status(name, enabled, disabled) + for name, version, _description, source, _dir, key in entries: + status = _plugin_status(name, enabled, disabled, key=key) print(f"{status:12} {source:8} {str(version):8} {name}") return @@ -850,8 +883,8 @@ def cmd_list(args: Any | None = None) -> None: table.add_column("Description") table.add_column("Source", style="dim") - for name, version, description, source, _dir in entries: - status_name = _plugin_status(name, enabled, disabled) + for name, version, description, source, _dir, key in entries: + status_name = _plugin_status(name, enabled, disabled, key=key) if status_name == "disabled": status = "[red]disabled[/red]" elif status_name == "enabled": @@ -1051,14 +1084,14 @@ def cmd_toggle() -> None: plugin_labels = [] plugin_selected = set() - for i, (name, _version, description, source, _d) in enumerate(entries): + for i, (name, _version, description, source, _d, key) in enumerate(entries): label = f"{name} \u2014 {description}" if description else name if source == "bundled": label = f"{label} [bundled]" plugin_names.append(name) plugin_labels.append(label) # Selected (enabled) when in enabled-set AND not in disabled-set - if name in enabled_set and name not in disabled_set: + if (name in enabled_set or key in enabled_set) and name not in disabled_set and key not in disabled_set: plugin_selected.add(i) # -- Provider categories -- @@ -1641,7 +1674,7 @@ def _git_pull_plugin_dir(target: Path) -> tuple[bool, str]: def dashboard_remove_user_plugin(name: str) -> dict[str, Any]: """Delete a plugin tree under ``~/.hermes/plugins/`` only.""" plugins_dir = _plugins_dir() - for n, _ver, _d, src, _path in _discover_all_plugins(): + for n, _ver, _d, src, _path, _key in _discover_all_plugins(): if n == name and src == "bundled": return {"ok": False, "error": "Bundled plugins cannot be removed from the dashboard."} diff --git a/tests/hermes_cli/test_plugins_cmd_category_discovery.py b/tests/hermes_cli/test_plugins_cmd_category_discovery.py new file mode 100644 index 00000000000..c86462e5ded --- /dev/null +++ b/tests/hermes_cli/test_plugins_cmd_category_discovery.py @@ -0,0 +1,355 @@ +"""Tests for the nested category plugin discovery fix (issue #41066). + +Verifies that _discover_all_plugins() recurses into category directories +(up to 2 levels deep) and that _plugin_status() checks both manifest name +and path-derived key against the enabled/disabled sets. +""" + +import json +import sys +from pathlib import Path +from unittest.mock import MagicMock, patch + +import pytest + + +# --------------------------------------------------------------------------- +# Helpers +# --------------------------------------------------------------------------- + + +def _make_plugin_dir(parent: Path, name: str, manifest: dict) -> Path: + """Create a minimal plugin directory with a plugin.yaml.""" + d = parent / name + d.mkdir(parents=True, exist_ok=True) + import yaml + (d / "plugin.yaml").write_text(yaml.dump(manifest), encoding="utf-8") + (d / "__init__.py").write_text("def register(ctx): pass\n", encoding="utf-8") + return d + + +def _make_category_plugin( + parent: Path, category: str, name: str, manifest: dict +) -> Path: + """Create a category-namespaced plugin: ///plugin.yaml.""" + return _make_plugin_dir(parent / category, name, manifest) + + +# --------------------------------------------------------------------------- +# _read_manifest_info +# --------------------------------------------------------------------------- + + +class TestReadManifestInfo: + def test_flat_plugin(self, tmp_path): + from hermes_cli.plugins_cmd import _read_manifest_info + + d = _make_plugin_dir(tmp_path, "my-plugin", { + "name": "my-plugin", "version": "1.0.0", "description": "test" + }) + result = _read_manifest_info(d, "") + assert result is not None + name, version, description, key = result + assert name == "my-plugin" + assert version == "1.0.0" + assert description == "test" + assert key == "my-plugin" # flat: key == name + + def test_category_plugin(self, tmp_path): + from hermes_cli.plugins_cmd import _read_manifest_info + + d = _make_category_plugin(tmp_path, "web", "tavily", { + "name": "web-tavily", "version": "2.0.0", "description": "search" + }) + result = _read_manifest_info(d, "web") + assert result is not None + name, version, description, key = result + assert name == "web-tavily" # manifest name + assert key == "web/tavily" # path-derived key + + def test_no_manifest(self, tmp_path): + from hermes_cli.plugins_cmd import _read_manifest_info + + d = tmp_path / "empty-dir" + d.mkdir() + assert _read_manifest_info(d, "") is None + + def test_yml_extension(self, tmp_path): + from hermes_cli.plugins_cmd import _read_manifest_info + + d = tmp_path / "my-plugin" + d.mkdir() + import yaml + (d / "plugin.yml").write_text(yaml.dump({"name": "my-plugin"}), encoding="utf-8") + result = _read_manifest_info(d, "") + assert result is not None + assert result[0] == "my-plugin" + + +# --------------------------------------------------------------------------- +# _discover_all_plugins — recursive discovery +# --------------------------------------------------------------------------- + + +class TestDiscoverAllPlugins: + @patch("hermes_cli.plugins.get_bundled_plugins_dir") + @patch("hermes_cli.plugins_cmd._plugins_dir") + def test_flat_plugins_still_discovered(self, mock_user_dir, mock_bundled_dir, tmp_path): + from hermes_cli.plugins_cmd import _discover_all_plugins + + _make_plugin_dir(tmp_path, "disk-cleanup", { + "name": "disk-cleanup", "version": "1.0.0" + }) + mock_user_dir.return_value = tmp_path + mock_bundled_dir.return_value = tmp_path / "nonexistent" + + entries = _discover_all_plugins() + keys = [e[5] for e in entries] + assert "disk-cleanup" in keys + + @patch("hermes_cli.plugins.get_bundled_plugins_dir") + @patch("hermes_cli.plugins_cmd._plugins_dir") + def test_category_plugins_discovered(self, mock_user_dir, mock_bundled_dir, tmp_path): + from hermes_cli.plugins_cmd import _discover_all_plugins + + _make_category_plugin(tmp_path, "web", "tavily", { + "name": "web-tavily", "version": "1.0.0" + }) + _make_category_plugin(tmp_path, "image_gen", "openai", { + "name": "image-gen-openai", "version": "2.0.0" + }) + mock_user_dir.return_value = tmp_path + mock_bundled_dir.return_value = tmp_path / "nonexistent" + + entries = _discover_all_plugins() + keys = [e[5] for e in entries] + assert "web/tavily" in keys + assert "image_gen/openai" in keys + + @patch("hermes_cli.plugins.get_bundled_plugins_dir") + @patch("hermes_cli.plugins_cmd._plugins_dir") + def test_mixed_flat_and_category(self, mock_user_dir, mock_bundled_dir, tmp_path): + from hermes_cli.plugins_cmd import _discover_all_plugins + + _make_plugin_dir(tmp_path, "disk-cleanup", { + "name": "disk-cleanup", "version": "1.0.0" + }) + _make_category_plugin(tmp_path, "web", "tavily", { + "name": "web-tavily", "version": "1.0.0" + }) + _make_category_plugin(tmp_path, "web", "exa", { + "name": "web-exa", "version": "1.0.0" + }) + mock_user_dir.return_value = tmp_path + mock_bundled_dir.return_value = tmp_path / "nonexistent" + + entries = _discover_all_plugins() + keys = [e[5] for e in entries] + assert "disk-cleanup" in keys + assert "web/tavily" in keys + assert "web/exa" in keys + assert len(entries) == 3 + + @patch("hermes_cli.plugins.get_bundled_plugins_dir") + @patch("hermes_cli.plugins_cmd._plugins_dir") + def test_depth_cap_at_two(self, mock_user_dir, mock_bundled_dir, tmp_path): + """Plugins nested 3 levels deep should NOT be discovered.""" + from hermes_cli.plugins_cmd import _discover_all_plugins + + # 2 levels: should be found + _make_category_plugin(tmp_path, "web", "tavily", { + "name": "web-tavily", "version": "1.0.0" + }) + # 3 levels: should NOT be found + deep = tmp_path / "a" / "b" / "c" + deep.mkdir(parents=True) + import yaml + (deep / "plugin.yaml").write_text( + yaml.dump({"name": "too-deep"}), encoding="utf-8" + ) + mock_user_dir.return_value = tmp_path + mock_bundled_dir.return_value = tmp_path / "nonexistent" + + entries = _discover_all_plugins() + keys = [e[5] for e in entries] + assert "web/tavily" in keys + assert "a/b/c" not in keys + + @patch("hermes_cli.plugins.get_bundled_plugins_dir") + @patch("hermes_cli.plugins_cmd._plugins_dir") + def test_tuple_has_six_elements(self, mock_user_dir, mock_bundled_dir, tmp_path): + from hermes_cli.plugins_cmd import _discover_all_plugins + + _make_category_plugin(tmp_path, "web", "tavily", { + "name": "web-tavily", "version": "1.0.0", "description": "search" + }) + mock_user_dir.return_value = tmp_path + mock_bundled_dir.return_value = tmp_path / "nonexistent" + + entries = _discover_all_plugins() + assert len(entries) == 1 + entry = entries[0] + assert len(entry) == 6 + name, version, description, source, dir_path, key = entry + assert name == "web-tavily" + assert key == "web/tavily" + assert source == "user" + + @patch("hermes_cli.plugins.get_bundled_plugins_dir") + @patch("hermes_cli.plugins_cmd._plugins_dir") + def test_user_overrides_bundled_on_key_collision(self, mock_user_dir, mock_bundled_dir, tmp_path): + """User plugin with same key as bundled should win.""" + from hermes_cli.plugins_cmd import _discover_all_plugins + + # Simulate a bundled plugin + bundled_dir = tmp_path / "bundled" + bundled_dir.mkdir() + _make_plugin_dir(bundled_dir, "my-plugin", { + "name": "my-plugin", "version": "1.0.0" + }) + # User plugin with same key + _make_plugin_dir(tmp_path, "my-plugin", { + "name": "my-plugin", "version": "2.0.0" + }) + mock_user_dir.return_value = tmp_path + mock_bundled_dir.return_value = bundled_dir + + entries = _discover_all_plugins() + keys = [e[5] for e in entries] + assert keys.count("my-plugin") == 1 + # User version should win + entry = [e for e in entries if e[5] == "my-plugin"][0] + assert entry[1] == "2.0.0" + + +# --------------------------------------------------------------------------- +# _plugin_status — key-aware status +# --------------------------------------------------------------------------- + + +class TestPluginStatus: + def test_name_in_enabled(self): + from hermes_cli.plugins_cmd import _plugin_status + assert _plugin_status("my-plugin", {"my-plugin"}, set()) == "enabled" + + def test_key_in_enabled(self): + from hermes_cli.plugins_cmd import _plugin_status + assert _plugin_status("web-tavily", {"web/tavily"}, set(), key="web/tavily") == "enabled" + + def test_name_in_disabled(self): + from hermes_cli.plugins_cmd import _plugin_status + assert _plugin_status("my-plugin", set(), {"my-plugin"}) == "disabled" + + def test_key_in_disabled(self): + from hermes_cli.plugins_cmd import _plugin_status + assert _plugin_status("web-tavily", set(), {"web/tavily"}, key="web/tavily") == "disabled" + + def test_neither_name_nor_key(self): + from hermes_cli.plugins_cmd import _plugin_status + assert _plugin_status("unknown", {"other"}, set(), key="cat/unknown") == "not enabled" + + def test_disabled_takes_precedence_over_enabled(self): + from hermes_cli.plugins_cmd import _plugin_status + assert _plugin_status("my-plugin", {"my-plugin"}, {"my-plugin"}) == "disabled" + + def test_key_disabled_takes_precedence(self): + from hermes_cli.plugins_cmd import _plugin_status + assert _plugin_status("web-tavily", {"web/tavily"}, {"web/tavily"}, key="web/tavily") == "disabled" + + +# --------------------------------------------------------------------------- +# Integration: _filter_plugin_entries with category plugins +# --------------------------------------------------------------------------- + + +class TestFilterPluginEntries: + def test_enabled_filter_uses_key(self): + from hermes_cli.plugins_cmd import _filter_plugin_entries + + entries = [ + ("web-tavily", "1.0.0", "search", "user", Path("/tmp"), "web/tavily"), + ("disk-cleanup", "1.0.0", "cleanup", "bundled", Path("/tmp"), "disk-cleanup"), + ] + args = MagicMock() + args.no_bundled = False + args.user = False + args.enabled = True + + result = _filter_plugin_entries(entries, args, {"web/tavily"}, set()) + assert len(result) == 1 + assert result[0][5] == "web/tavily" + + def test_enabled_filter_by_name_still_works(self): + from hermes_cli.plugins_cmd import _filter_plugin_entries + + entries = [ + ("disk-cleanup", "1.0.0", "cleanup", "bundled", Path("/tmp"), "disk-cleanup"), + ] + args = MagicMock() + args.no_bundled = False + args.user = False + args.enabled = True + + result = _filter_plugin_entries(entries, args, {"disk-cleanup"}, set()) + assert len(result) == 1 + + +# --------------------------------------------------------------------------- +# Integration: cmd_list JSON output includes category plugins +# --------------------------------------------------------------------------- + + +class TestCmdListJson: + @patch("hermes_cli.plugins.get_bundled_plugins_dir") + @patch("hermes_cli.plugins_cmd._plugins_dir") + def test_json_output_includes_category_plugins(self, mock_user_dir, mock_bundled_dir, tmp_path, capsys): + from hermes_cli.plugins_cmd import cmd_list + + _make_category_plugin(tmp_path, "web", "tavily", { + "name": "web-tavily", "version": "1.0.0", "description": "search" + }) + _make_plugin_dir(tmp_path, "disk-cleanup", { + "name": "disk-cleanup", "version": "2.0.0", "description": "cleanup" + }) + mock_user_dir.return_value = tmp_path + mock_bundled_dir.return_value = tmp_path / "nonexistent" + + args = MagicMock() + args.json = True + args.plain = False + args.no_bundled = False + args.user = False + args.enabled = False + + cmd_list(args) + captured = capsys.readouterr() + payload = json.loads(captured.out) + names = [p["name"] for p in payload] + assert "web-tavily" in names + assert "disk-cleanup" in names + + @patch("hermes_cli.plugins.get_bundled_plugins_dir") + @patch("hermes_cli.plugins_cmd._plugins_dir") + def test_json_status_uses_key(self, mock_user_dir, mock_bundled_dir, tmp_path, capsys): + from hermes_cli.plugins_cmd import cmd_list + + _make_category_plugin(tmp_path, "web", "tavily", { + "name": "web-tavily", "version": "1.0.0" + }) + mock_user_dir.return_value = tmp_path + mock_bundled_dir.return_value = tmp_path / "nonexistent" + + # Patch config to return web/tavily as enabled + with patch("hermes_cli.plugins_cmd._get_enabled_set", return_value={"web/tavily"}): + args = MagicMock() + args.json = True + args.plain = False + args.no_bundled = False + args.user = False + args.enabled = False + + cmd_list(args) + captured = capsys.readouterr() + payload = json.loads(captured.out) + assert len(payload) == 1 + assert payload[0]["status"] == "enabled"