-
Notifications
You must be signed in to change notification settings - Fork 46.7k
fix(toolsets): platform plugins without a static bundle silently get zero tools; config set stores list literals as strings #57063
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -810,6 +810,57 @@ def _set_plugin_entry_flag(plugin_id: str, key: str, value: bool) -> None: | |
| save_config(config) | ||
|
|
||
|
|
||
| # Plugin kinds that the general loader does NOT gate on plugins.enabled / | ||
| # plugins.disabled: model providers register through providers/__init__.py's | ||
| # own discovery (selected via `hermes model` / model.provider), and exclusive | ||
| # category plugins (memory providers) activate via `<category>.provider`. | ||
| # `plugins enable`/`disable` used to accept these and print a success message | ||
| # while having zero effect — the flag was written but nothing ever read it, | ||
| # which misled users into thinking they had switched something on or off. | ||
| _PASSIVE_PLUGIN_KINDS = {"model-provider", "exclusive"} | ||
|
|
||
|
|
||
| def _plugin_kind(key: str) -> str: | ||
| """Manifest ``kind`` for a discovered plugin key (default: ``standalone``).""" | ||
| for entry in _discover_all_plugins(): | ||
| # entry = (name, version, description, source, dir_path, key) | ||
| if entry[5] == key: | ||
| d = Path(entry[4]) | ||
| for fname in ("plugin.yaml", "plugin.yml"): | ||
| mf = d / fname | ||
| if mf.exists(): | ||
| try: | ||
| import yaml | ||
|
|
||
| data = yaml.safe_load(mf.read_text(encoding="utf-8")) or {} | ||
| return str(data.get("kind", "standalone")) | ||
| except Exception: | ||
| return "standalone" | ||
| return "standalone" | ||
|
|
||
|
|
||
| def _print_passive_kind_hint(console, key: str, kind: str) -> None: | ||
| """Explain how a passive-kind plugin is actually controlled.""" | ||
| if kind == "model-provider": | ||
| console.print( | ||
| f"[yellow]![/yellow] [bold]{key}[/bold] is a model provider — it is " | ||
| "not controlled by plugins.enabled/disabled (providers register " | ||
| "automatically at startup).\n" | ||
| " To use it: run [bold]hermes model[/bold] and pick it, or set " | ||
| "[dim]model.provider[/dim] in config.yaml.\n" | ||
| " To stop using it: select a different provider; remove its API key " | ||
| "from ~/.hermes/.env to make it unselectable.\n" | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Use |
||
| "Nothing was changed." | ||
| ) | ||
| else: # exclusive | ||
| console.print( | ||
| f"[yellow]![/yellow] [bold]{key}[/bold] is an exclusive category " | ||
| "plugin — it is activated by its category's provider key (e.g. " | ||
| "[dim]memory.provider[/dim]), not by plugins.enabled/disabled.\n" | ||
| "Nothing was changed." | ||
| ) | ||
|
|
||
|
|
||
| def cmd_enable(name: str, allow_tool_override: Optional[bool] = None) -> None: | ||
| """Add a plugin to the enabled allow-list (and remove it from disabled). | ||
|
|
||
|
|
@@ -831,6 +882,11 @@ def cmd_enable(name: str, allow_tool_override: Optional[bool] = None) -> None: | |
| sys.exit(1) | ||
| key, source = resolved | ||
|
|
||
| kind = _plugin_kind(key) | ||
| if kind in _PASSIVE_PLUGIN_KINDS: | ||
| _print_passive_kind_hint(console, key, kind) | ||
| return | ||
|
|
||
| enabled = _get_enabled_set() | ||
| disabled = _get_disabled_set() | ||
|
|
||
|
|
@@ -910,6 +966,11 @@ def cmd_disable(name: str) -> None: | |
| console.print(f"[red]Plugin '{name}' is not installed or bundled.[/red]") | ||
| sys.exit(1) | ||
|
|
||
| kind = _plugin_kind(key) | ||
| if kind in _PASSIVE_PLUGIN_KINDS: | ||
| _print_passive_kind_hint(console, key, kind) | ||
| return | ||
|
|
||
| enabled = _get_enabled_set() | ||
| disabled = _get_disabled_set() | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,71 @@ | ||
| """``hermes config set`` must parse list/mapping literals, not store them as strings. | ||
|
|
||
| Before this fix, ``hermes config set platform_toolsets.discord '["file","web"]'`` | ||
| stored the value as a raw STRING. Every reader that gates on | ||
| ``isinstance(..., list)`` — ``_get_platform_tools``, ``_get_enabled_set``, | ||
| ``_get_disabled_set`` — then silently ignored it and fell back to its default, | ||
| so the setting looked saved but never took effect (observed in the wild as a | ||
| platform running on the wrong toolset bundle for weeks). | ||
| """ | ||
| import pytest | ||
|
|
||
|
|
||
| @pytest.fixture | ||
| def user_home(tmp_path, monkeypatch): | ||
| home = tmp_path / "home" | ||
| home.mkdir() | ||
| monkeypatch.setenv("HERMES_HOME", str(home)) | ||
| monkeypatch.delenv("HERMES_MANAGED_DIR", raising=False) | ||
| import hermes_cli.config as cfg | ||
| from hermes_cli import managed_scope | ||
|
|
||
| cfg._LOAD_CONFIG_CACHE.clear() | ||
| cfg._RAW_CONFIG_CACHE.clear() | ||
| managed_scope.invalidate_managed_cache() | ||
| return home | ||
|
|
||
|
|
||
| def test_list_literal_is_parsed_to_list(user_home): | ||
| from hermes_cli.config import set_config_value, read_raw_config | ||
|
|
||
| set_config_value("platform_toolsets.line", '["clarify", "file", "web"]') | ||
| raw = read_raw_config() | ||
| assert raw["platform_toolsets"]["line"] == ["clarify", "file", "web"] | ||
|
|
||
|
|
||
| def test_mapping_literal_is_parsed_to_dict(user_home): | ||
| from hermes_cli.config import set_config_value, read_raw_config | ||
|
|
||
| set_config_value("display.tool_progress_overrides", '{"terminal": "off"}') | ||
| raw = read_raw_config() | ||
| assert raw["display"]["tool_progress_overrides"] == {"terminal": "off"} | ||
|
|
||
|
|
||
| def test_yaml_flow_list_is_parsed(user_home): | ||
| from hermes_cli.config import set_config_value, read_raw_config | ||
|
|
||
| set_config_value("plugins.enabled", "[model-providers/gemini]") | ||
| raw = read_raw_config() | ||
| assert raw["plugins"]["enabled"] == ["model-providers/gemini"] | ||
|
|
||
|
|
||
| def test_invalid_list_literal_warns_and_stores_string(user_home, capsys): | ||
| from hermes_cli.config import set_config_value, read_raw_config | ||
|
|
||
| set_config_value("platform_toolsets.line", '["unclosed') | ||
| captured = capsys.readouterr() | ||
| assert "not valid" in captured.err.lower() or "warning" in captured.err.lower() | ||
| raw = read_raw_config() | ||
| assert raw["platform_toolsets"]["line"] == '["unclosed' | ||
|
|
||
|
|
||
| def test_scalar_values_unaffected(user_home): | ||
| from hermes_cli.config import set_config_value, read_raw_config | ||
|
|
||
| set_config_value("agent.max_turns", "300") | ||
| set_config_value("display.compact", "true") | ||
| set_config_value("tts.provider", "edge") | ||
| raw = read_raw_config() | ||
| assert raw["agent"]["max_turns"] == 300 | ||
| assert raw["display"]["compact"] is True | ||
| assert raw["tts"]["provider"] == "edge" |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,91 @@ | ||
| """``hermes plugins enable/disable`` must not silently no-op on passive kinds. | ||
|
|
||
| Model providers (``kind: model-provider``) register through | ||
| ``providers/__init__.py``'s own discovery and are selected via ``hermes model`` | ||
| / ``model.provider``; exclusive plugins (``kind: exclusive``, e.g. memory | ||
| providers) activate via ``<category>.provider``. The general plugin loader | ||
| skips both kinds, so a ``plugins.enabled``/``disabled`` entry for them is dead | ||
| config. Before this fix, ``hermes plugins enable gemini`` printed a green | ||
| success message while changing nothing that any loader would ever read. | ||
| """ | ||
| import pytest | ||
|
|
||
|
|
||
| @pytest.fixture | ||
| def fake_plugins(tmp_path, monkeypatch): | ||
| """A discovery view with one plugin per kind, backed by real manifests.""" | ||
| import hermes_cli.plugins_cmd as pc | ||
|
|
||
| entries = [] | ||
| for name, kind in [ | ||
| ("gemini", "model-provider"), | ||
| ("honcho", "exclusive"), | ||
| ("nemo_relay", "standalone"), | ||
| ]: | ||
| d = tmp_path / name | ||
| d.mkdir() | ||
| (d / "plugin.yaml").write_text( | ||
| f"name: {name}\nkind: {kind}\nversion: 1.0.0\n", encoding="utf-8" | ||
| ) | ||
| key = f"model-providers/{name}" if kind == "model-provider" else name | ||
| entries.append((name, "1.0.0", "desc", "bundled", str(d), key)) | ||
|
|
||
| monkeypatch.setattr(pc, "_discover_all_plugins", lambda: entries) | ||
| # config writes must land in a scratch HERMES_HOME | ||
| home = tmp_path / "home" | ||
| home.mkdir() | ||
| monkeypatch.setenv("HERMES_HOME", str(home)) | ||
| import hermes_cli.config as cfg | ||
|
|
||
| cfg._LOAD_CONFIG_CACHE.clear() | ||
| cfg._RAW_CONFIG_CACHE.clear() | ||
| return pc | ||
|
|
||
|
|
||
| def _enabled_disabled(pc): | ||
| return pc._get_enabled_set(), pc._get_disabled_set() | ||
|
|
||
|
|
||
| def test_enable_model_provider_is_refused_with_hint(fake_plugins, capsys): | ||
| pc = fake_plugins | ||
| pc.cmd_enable("gemini") | ||
| out = capsys.readouterr().out | ||
| assert "model provider" in out | ||
| assert "hermes model" in out | ||
| assert "Nothing was changed" in out | ||
| enabled, disabled = _enabled_disabled(pc) | ||
| assert "model-providers/gemini" not in enabled | ||
| assert "model-providers/gemini" not in disabled | ||
|
|
||
|
|
||
| def test_disable_model_provider_is_refused_with_hint(fake_plugins, capsys): | ||
| pc = fake_plugins | ||
| pc.cmd_disable("gemini") | ||
| out = capsys.readouterr().out | ||
| assert "model provider" in out | ||
| enabled, disabled = _enabled_disabled(pc) | ||
| assert "model-providers/gemini" not in disabled | ||
|
|
||
|
|
||
| def test_enable_exclusive_is_refused_with_hint(fake_plugins, capsys): | ||
| pc = fake_plugins | ||
| pc.cmd_enable("honcho") | ||
| out = capsys.readouterr().out | ||
| assert "exclusive" in out | ||
| assert "provider" in out | ||
| enabled, _ = _enabled_disabled(pc) | ||
| assert "honcho" not in enabled | ||
|
|
||
|
|
||
| def test_enable_standalone_still_works(fake_plugins, capsys): | ||
| pc = fake_plugins | ||
| pc.cmd_enable("nemo_relay", allow_tool_override=False) | ||
| out = capsys.readouterr().out | ||
| assert "enabled" in out | ||
| enabled, _ = _enabled_disabled(pc) | ||
| assert "nemo_relay" in enabled | ||
|
|
||
|
|
||
| def test_plugin_kind_defaults_to_standalone(fake_plugins): | ||
| pc = fake_plugins | ||
| assert pc._plugin_kind("no-such-key") == "standalone" |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -291,3 +291,70 @@ def test_all_alias_accepts_include_registry(self): | |
|
|
||
| def test_registry_only_toolset_static_view_is_empty(self): | ||
| assert resolve_toolset("__definitely_not_a_real_toolset__", include_registry=False) == [] | ||
|
|
||
|
|
||
| class TestBundledPlatformBundles: | ||
| """Every bundled platform plugin must ship a static ``hermes-<platform>`` bundle. | ||
|
|
||
| ``_get_platform_tools`` falls back to ``hermes-<platform>`` for any platform | ||
| missing from config's ``platform_toolsets``, and ``resolve_toolset`` returns | ||
| ``[]`` for an unknown name. The runtime auto-generate path only fires once | ||
| ``gateway.platform_registry`` has the platform registered, so outside the | ||
| gateway process (cron, kanban dispatch, ``hermes tools``, doctor) — and in | ||
| the explicit-config composite expansion, which skips names absent from | ||
| ``TOOLSETS`` — a bundled platform plugin without a static bundle silently | ||
| degrades to ZERO tools. LINE shipped this way (#23197): every platform | ||
| listed in ``plugins/platforms/`` must have a matching static bundle. | ||
| """ | ||
|
|
||
| def test_every_bundled_platform_plugin_has_a_static_bundle(self): | ||
| import pathlib | ||
|
|
||
| platforms_dir = ( | ||
| pathlib.Path(__file__).resolve().parent.parent / "plugins" / "platforms" | ||
| ) | ||
| missing = [] | ||
| for child in sorted(platforms_dir.iterdir()): | ||
| if not child.is_dir() or not (child / "plugin.yaml").exists(): | ||
| continue | ||
| bundle = f"hermes-{child.name}" | ||
| if bundle not in TOOLSETS or not resolve_toolset( | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This permanently requires static core bundles for every platform plugin, but current dynamic-platform support was introduced specifically to avoid toolsets.py entries (commit 52d9e57; current |
||
| bundle, include_registry=False | ||
| ): | ||
| missing.append(bundle) | ||
| assert not missing, ( | ||
| f"Bundled platform plugins without a static toolset bundle: {missing}. " | ||
| f"Add a 'hermes-<platform>' entry to TOOLSETS in toolsets.py — without " | ||
| f"it the platform silently resolves to zero tools (see #38798 shape)." | ||
| ) | ||
|
|
||
| def test_new_platform_bundles_resolve_to_core_tools(self): | ||
| for platform in ( | ||
| "line", | ||
| "google_chat", | ||
| "teams", | ||
| "irc", | ||
| "ntfy", | ||
| "photon", | ||
| "simplex", | ||
| "raft", | ||
| ): | ||
| tools = resolve_toolset(f"hermes-{platform}", include_registry=False) | ||
| for core_tool in ("terminal", "read_file", "memory", "web_search", "todo"): | ||
| assert core_tool in tools, ( | ||
| f"hermes-{platform} is missing core tool {core_tool!r}" | ||
| ) | ||
|
|
||
| def test_gateway_composite_includes_all_platform_bundles(self): | ||
| gateway_includes = set(TOOLSETS["hermes-gateway"]["includes"]) | ||
| for platform in ( | ||
| "line", | ||
| "google_chat", | ||
| "teams", | ||
| "irc", | ||
| "ntfy", | ||
| "photon", | ||
| "simplex", | ||
| "raft", | ||
| ): | ||
| assert f"hermes-{platform}" in gateway_includes | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This raw manifest read disagrees with the loader:
PluginManagernormalizeskindand heuristically classifies kind-less memory/model providers (hermes_cli/plugins.py:1583-1627). Such a provider will still bypass this guard and receive the misleading success path. Reuse the canonical classification or duplicate its normalization and heuristic with coverage.