diff --git a/hermes_cli/plugins_cmd.py b/hermes_cli/plugins_cmd.py index 7f6a3314ecf..08e052353ec 100644 --- a/hermes_cli/plugins_cmd.py +++ b/hermes_cli/plugins_cmd.py @@ -649,29 +649,62 @@ def _save_enabled_set(enabled: set) -> None: save_config(config) +def _resolve_plugin_key(name: str) -> Optional[str]: + """Resolve a user-supplied plugin identifier to its canonical registry key. + + Accepts either the bare manifest name (``nemo_relay``), the directory + name, or the full path-derived key (``observability/nemo_relay``) and + returns the canonical key the loader gates on (``manifest.key`` or, for a + flat plugin, the bare name). Returns ``None`` when no plugin matches. + + This is the single normalization point so ``hermes plugins enable`` / + ``disable`` write the same key that ``PluginManager`` matches against — + nested category plugins (e.g. ``observability/nemo_relay``) included. + """ + entries = _discover_all_plugins() + # 1. Exact match on canonical key or manifest name — always unambiguous. + for entry in entries: + # entry = (name, version, description, source, dir_path, key) + if name == entry[5] or name == entry[0]: + return entry[5] + # 2. Fall back to a bare leaf-name match (e.g. "nemo_relay" -> + # "observability/nemo_relay"), but only when it resolves to exactly one + # plugin so we never silently pick the wrong same-named nested plugin. + leaf_matches = [entry[5] for entry in entries if name == entry[5].split("/")[-1]] + if len(leaf_matches) == 1: + return leaf_matches[0] + return None + + def cmd_enable(name: str) -> None: """Add a plugin to the enabled allow-list (and remove it from disabled).""" from rich.console import Console console = Console() - # Discover the plugin — check installed (user) AND bundled. - if not _plugin_exists(name): + # Discover the plugin — check installed (user) AND bundled, including + # nested category plugins — and normalize to its canonical registry key. + key = _resolve_plugin_key(name) + if key is None: console.print(f"[red]Plugin '{name}' is not installed or bundled.[/red]") sys.exit(1) enabled = _get_enabled_set() disabled = _get_disabled_set() - if name in enabled and name not in disabled: - console.print(f"[dim]Plugin '{name}' is already enabled.[/dim]") + if key in enabled and key not in disabled: + console.print(f"[dim]Plugin '{key}' is already enabled.[/dim]") return - enabled.add(name) - disabled.discard(name) + enabled.add(key) + disabled.discard(key) + # Drop any legacy bare-name entry so the two don't drift out of sync. + bare = key.split("/")[-1] + if bare != key: + disabled.discard(bare) _save_enabled_set(enabled) _save_disabled_set(disabled) console.print( - f"[green]✓[/green] Plugin [bold]{name}[/bold] enabled. " + f"[green]✓[/green] Plugin [bold]{key}[/bold] enabled. " "Takes effect on next session." ) @@ -681,51 +714,36 @@ def cmd_disable(name: str) -> None: from rich.console import Console console = Console() - if not _plugin_exists(name): + key = _resolve_plugin_key(name) + if key is None: console.print(f"[red]Plugin '{name}' is not installed or bundled.[/red]") sys.exit(1) enabled = _get_enabled_set() disabled = _get_disabled_set() - if name not in enabled and name in disabled: - console.print(f"[dim]Plugin '{name}' is already disabled.[/dim]") + if key not in enabled and key in disabled: + console.print(f"[dim]Plugin '{key}' is already disabled.[/dim]") return - enabled.discard(name) - disabled.add(name) + enabled.discard(key) + # Drop any legacy bare-name entry from the allow-list too, so a stale + # bare name can't keep a nested plugin loading after an explicit disable. + bare = key.split("/")[-1] + if bare != key: + enabled.discard(bare) + disabled.add(key) _save_enabled_set(enabled) _save_disabled_set(disabled) console.print( - f"[yellow]\u2298[/yellow] Plugin [bold]{name}[/bold] disabled. " + f"[yellow]\u2298[/yellow] Plugin [bold]{key}[/bold] disabled. " "Takes effect on next session." ) def _plugin_exists(name: str) -> bool: - """Return True if a plugin with *name* is installed (user) or bundled.""" - # Installed: directory name or manifest name match in user plugins dir - user_dir = _plugins_dir() - if user_dir.is_dir(): - if (user_dir / name).is_dir(): - return True - for child in user_dir.iterdir(): - if not child.is_dir(): - continue - manifest = _read_manifest(child) - if manifest.get("name") == name: - return True - # Bundled: /plugins// (or HERMES_BUNDLED_PLUGINS on Nix). - from hermes_cli.plugins import get_bundled_plugins_dir - repo_plugins = get_bundled_plugins_dir() - if repo_plugins.is_dir(): - candidate = repo_plugins / name - if candidate.is_dir() and ( - (candidate / "plugin.yaml").exists() - or (candidate / "plugin.yml").exists() - ): - return True - return False + """Return True if a plugin with *name* (bare name or key) exists.""" + return _resolve_plugin_key(name) is not None def _read_manifest_info(d: Path, prefix: str): diff --git a/hermes_cli/web_server.py b/hermes_cli/web_server.py index 95cfd34fc14..7d8c99f75ae 100644 --- a/hermes_cli/web_server.py +++ b/hermes_cli/web_server.py @@ -9308,10 +9308,16 @@ def _merged_plugins_hub() -> Dict[str, Any]: plugins_root_resolved = (get_hermes_home() / "plugins").resolve() rows: List[Dict[str, Any]] = [] - for name, version, description, source, dir_str in _discover_all_plugins(): - if name in disabled_set: + for name, version, description, source, dir_str, key in _discover_all_plugins(): + # 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} + if key: + aliases.add(key) + if aliases & disabled_set: runtime_status = "disabled" - elif name in enabled_set: + elif aliases & enabled_set: runtime_status = "enabled" else: runtime_status = "inactive" diff --git a/tests/hermes_cli/test_plugins_cmd_enable_disable_nested.py b/tests/hermes_cli/test_plugins_cmd_enable_disable_nested.py new file mode 100644 index 00000000000..427647095aa --- /dev/null +++ b/tests/hermes_cli/test_plugins_cmd_enable_disable_nested.py @@ -0,0 +1,193 @@ +"""Tests for nested/alias-normalized enable & disable flows. + +Companion to test_plugins_cmd_category_discovery.py. That file covers the +*listing* side of nested category plugins (issue #41066). These tests cover +the *mutation* side: `hermes plugins enable/disable` must resolve a bare name +OR a full path-derived key (e.g. `observability/nemo_relay`) to the canonical +registry key and write THAT — the same string PluginManager gates on — so a +nested bundled plugin can actually be toggled. +""" + +import sys # noqa: F401 +from pathlib import Path +from unittest.mock import patch + +import pytest + + +def _make_plugin_dir(parent: Path, name: str, manifest: dict) -> Path: + 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: + return _make_plugin_dir(parent / category, name, manifest) + + +@pytest.fixture +def nested_plugin_env(tmp_path): + """A user-plugins dir containing one nested and one flat plugin, with the + bundled dir pointed at an empty path. Returns the tmp_path.""" + _make_category_plugin(tmp_path, "observability", "nemo_relay", { + "name": "nemo_relay", "version": "1.0.0", "description": "relay obs" + }) + _make_plugin_dir(tmp_path, "disk-cleanup", { + "name": "disk-cleanup", "version": "1.0.0" + }) + return tmp_path + + +# --------------------------------------------------------------------------- +# _resolve_plugin_key +# --------------------------------------------------------------------------- + + +class TestResolvePluginKey: + @patch("hermes_cli.plugins.get_bundled_plugins_dir") + @patch("hermes_cli.plugins_cmd._plugins_dir") + def test_full_key_resolves_to_itself(self, mock_user, mock_bundled, nested_plugin_env): + from hermes_cli.plugins_cmd import _resolve_plugin_key + mock_user.return_value = nested_plugin_env + mock_bundled.return_value = nested_plugin_env / "nonexistent" + assert _resolve_plugin_key("observability/nemo_relay") == "observability/nemo_relay" + + @patch("hermes_cli.plugins.get_bundled_plugins_dir") + @patch("hermes_cli.plugins_cmd._plugins_dir") + def test_bare_leaf_name_resolves_to_key(self, mock_user, mock_bundled, nested_plugin_env): + from hermes_cli.plugins_cmd import _resolve_plugin_key + mock_user.return_value = nested_plugin_env + mock_bundled.return_value = nested_plugin_env / "nonexistent" + # "nemo_relay" (bare) must normalize to the path-derived key. + assert _resolve_plugin_key("nemo_relay") == "observability/nemo_relay" + + @patch("hermes_cli.plugins.get_bundled_plugins_dir") + @patch("hermes_cli.plugins_cmd._plugins_dir") + def test_flat_plugin_resolves_to_name(self, mock_user, mock_bundled, nested_plugin_env): + from hermes_cli.plugins_cmd import _resolve_plugin_key + mock_user.return_value = nested_plugin_env + mock_bundled.return_value = nested_plugin_env / "nonexistent" + assert _resolve_plugin_key("disk-cleanup") == "disk-cleanup" + + @patch("hermes_cli.plugins.get_bundled_plugins_dir") + @patch("hermes_cli.plugins_cmd._plugins_dir") + def test_unknown_returns_none(self, mock_user, mock_bundled, nested_plugin_env): + from hermes_cli.plugins_cmd import _resolve_plugin_key + mock_user.return_value = nested_plugin_env + mock_bundled.return_value = nested_plugin_env / "nonexistent" + assert _resolve_plugin_key("does-not-exist") is None + + @patch("hermes_cli.plugins.get_bundled_plugins_dir") + @patch("hermes_cli.plugins_cmd._plugins_dir") + def test_ambiguous_leaf_name_returns_none(self, mock_user, mock_bundled, tmp_path): + """Same leaf name under two categories must NOT silently pick one.""" + from hermes_cli.plugins_cmd import _resolve_plugin_key + _make_category_plugin(tmp_path, "image_gen", "openai", {"name": "image-gen-openai"}) + _make_category_plugin(tmp_path, "model-providers", "openai", {"name": "mp-openai"}) + mock_user.return_value = tmp_path + mock_bundled.return_value = tmp_path / "nonexistent" + # Bare "openai" is ambiguous -> None; the full key still resolves. + assert _resolve_plugin_key("openai") is None + assert _resolve_plugin_key("image_gen/openai") == "image_gen/openai" + + +# --------------------------------------------------------------------------- +# cmd_enable / cmd_disable — write the canonical key +# --------------------------------------------------------------------------- + + +class TestEnableDisableNested: + @patch("hermes_cli.plugins.get_bundled_plugins_dir") + @patch("hermes_cli.plugins_cmd._plugins_dir") + @patch("hermes_cli.plugins_cmd._save_disabled_set") + @patch("hermes_cli.plugins_cmd._save_enabled_set") + @patch("hermes_cli.plugins_cmd._get_disabled_set", return_value=set()) + @patch("hermes_cli.plugins_cmd._get_enabled_set", return_value=set()) + def test_enable_bare_name_writes_key( + self, mock_en, mock_dis, mock_save_en, mock_save_dis, + mock_user, mock_bundled, nested_plugin_env, + ): + from hermes_cli.plugins_cmd import cmd_enable + mock_user.return_value = nested_plugin_env + mock_bundled.return_value = nested_plugin_env / "nonexistent" + + cmd_enable("nemo_relay") # bare name + + saved = mock_save_en.call_args[0][0] + # The canonical key — NOT the bare name — must be persisted, because + # that is what PluginManager matches when deciding to load. + assert "observability/nemo_relay" in saved + assert "nemo_relay" not in saved or "observability/nemo_relay" in saved + + @patch("hermes_cli.plugins.get_bundled_plugins_dir") + @patch("hermes_cli.plugins_cmd._plugins_dir") + @patch("hermes_cli.plugins_cmd._save_disabled_set") + @patch("hermes_cli.plugins_cmd._save_enabled_set") + @patch("hermes_cli.plugins_cmd._get_disabled_set", return_value=set()) + @patch("hermes_cli.plugins_cmd._get_enabled_set", return_value=set()) + def test_enable_full_key_writes_key( + self, mock_en, mock_dis, mock_save_en, mock_save_dis, + mock_user, mock_bundled, nested_plugin_env, + ): + from hermes_cli.plugins_cmd import cmd_enable + mock_user.return_value = nested_plugin_env + mock_bundled.return_value = nested_plugin_env / "nonexistent" + + cmd_enable("observability/nemo_relay") + saved = mock_save_en.call_args[0][0] + assert "observability/nemo_relay" in saved + + @patch("hermes_cli.plugins.get_bundled_plugins_dir") + @patch("hermes_cli.plugins_cmd._plugins_dir") + @patch("hermes_cli.plugins_cmd._save_disabled_set") + @patch("hermes_cli.plugins_cmd._save_enabled_set") + @patch("hermes_cli.plugins_cmd._get_disabled_set", return_value=set()) + @patch("hermes_cli.plugins_cmd._get_enabled_set", return_value=set()) + def test_disable_bare_name_writes_key_and_clears_alias( + self, mock_en, mock_dis, mock_save_en, mock_save_dis, + mock_user, mock_bundled, nested_plugin_env, + ): + from hermes_cli.plugins_cmd import cmd_disable + mock_user.return_value = nested_plugin_env + mock_bundled.return_value = nested_plugin_env / "nonexistent" + # Simulate an existing config where the plugin was enabled under the + # legacy bare name — disabling must clear that too, or the plugin would + # keep loading (PluginManager accepts the bare name as well). + mock_en.return_value = {"nemo_relay"} + + cmd_disable("nemo_relay") + saved_dis = mock_save_dis.call_args[0][0] + saved_en = mock_save_en.call_args[0][0] + assert "observability/nemo_relay" in saved_dis + assert "nemo_relay" not in saved_en # stale bare alias dropped + + @patch("hermes_cli.plugins.get_bundled_plugins_dir") + @patch("hermes_cli.plugins_cmd._plugins_dir") + def test_enable_unknown_plugin_exits(self, mock_user, mock_bundled, nested_plugin_env): + from hermes_cli.plugins_cmd import cmd_enable + mock_user.return_value = nested_plugin_env + mock_bundled.return_value = nested_plugin_env / "nonexistent" + with pytest.raises(SystemExit): + cmd_enable("does-not-exist") + + @patch("hermes_cli.plugins.get_bundled_plugins_dir") + @patch("hermes_cli.plugins_cmd._plugins_dir") + @patch("hermes_cli.plugins_cmd._save_disabled_set") + @patch("hermes_cli.plugins_cmd._save_enabled_set") + @patch("hermes_cli.plugins_cmd._get_disabled_set", return_value=set()) + @patch("hermes_cli.plugins_cmd._get_enabled_set", return_value=set()) + def test_enable_flat_plugin_unchanged( + self, mock_en, mock_dis, mock_save_en, mock_save_dis, + mock_user, mock_bundled, nested_plugin_env, + ): + """Flat plugins keep writing their bare name (key == name) — no regression.""" + from hermes_cli.plugins_cmd import cmd_enable + mock_user.return_value = nested_plugin_env + mock_bundled.return_value = nested_plugin_env / "nonexistent" + + cmd_enable("disk-cleanup") + saved = mock_save_en.call_args[0][0] + assert "disk-cleanup" in saved diff --git a/tests/hermes_cli/test_plugins_cmd_list.py b/tests/hermes_cli/test_plugins_cmd_list.py index 1d9051c2822..5e8c061dab4 100644 --- a/tests/hermes_cli/test_plugins_cmd_list.py +++ b/tests/hermes_cli/test_plugins_cmd_list.py @@ -18,9 +18,9 @@ def _args(**kwargs): def test_filter_plugin_entries_enabled_only(): entries = [ - ("disk-cleanup", "2.0.0", "Bundled", "bundled", None), - ("web-search-plus", "2.2.0", "Search", "git", None), - ("old-plugin", "1.0.0", "Old", "user", None), + ("disk-cleanup", "2.0.0", "Bundled", "bundled", None, "disk-cleanup"), + ("web-search-plus", "2.2.0", "Search", "git", None, "web-search-plus"), + ("old-plugin", "1.0.0", "Old", "user", None, "old-plugin"), ] filtered = plugins_cmd._filter_plugin_entries( @@ -35,9 +35,9 @@ def test_filter_plugin_entries_enabled_only(): def test_filter_plugin_entries_no_bundled(): entries = [ - ("disk-cleanup", "2.0.0", "Bundled", "bundled", None), - ("drawthings-grpc", "0.3.0", "Draw Things", "user", None), - ("web-search-plus", "2.2.0", "Search", "git", None), + ("disk-cleanup", "2.0.0", "Bundled", "bundled", None, "disk-cleanup"), + ("drawthings-grpc", "0.3.0", "Draw Things", "user", None, "drawthings-grpc"), + ("web-search-plus", "2.2.0", "Search", "git", None, "web-search-plus"), ] filtered = plugins_cmd._filter_plugin_entries( @@ -52,8 +52,8 @@ def test_filter_plugin_entries_no_bundled(): def test_cmd_list_plain_compact_output(monkeypatch, capsys): entries = [ - ("disk-cleanup", "2.0.0", "Bundled", "bundled", None), - ("web-search-plus", "2.2.0", "Search", "git", None), + ("disk-cleanup", "2.0.0", "Bundled", "bundled", None, "disk-cleanup"), + ("web-search-plus", "2.2.0", "Search", "git", None, "web-search-plus"), ] monkeypatch.setattr(plugins_cmd, "_discover_all_plugins", lambda: entries) monkeypatch.setattr(plugins_cmd, "_get_enabled_set", lambda: {"web-search-plus"}) @@ -69,7 +69,7 @@ def test_cmd_list_plain_compact_output(monkeypatch, capsys): def test_cmd_list_json_output(monkeypatch, capsys): - entries = [("web-search-plus", "2.2.0", "Search", "git", None)] + entries = [("web-search-plus", "2.2.0", "Search", "git", None, "web-search-plus")] monkeypatch.setattr(plugins_cmd, "_discover_all_plugins", lambda: entries) monkeypatch.setattr(plugins_cmd, "_get_enabled_set", lambda: {"web-search-plus"}) monkeypatch.setattr(plugins_cmd, "_get_disabled_set", lambda: set())