From 131d4a46347d98af4f4b621e997cbb8fe514a4e4 Mon Sep 17 00:00:00 2001 From: kshitijk4poor <82637225+kshitijk4poor@users.noreply.github.com> Date: Fri, 25 Sep 2026 15:29:27 +0530 Subject: [PATCH] fix(plugins): picker tick purges every alias of the disable; report real flips Follow-up to the ported picker fix (#121623): - Ticking a row re-enables it even when plugins.disabled holds the manifest name (e.g. telegram-platform, written by pickers before #40190). The save only dropped the key and its bare leaf, so the gate still matched the manifest name and the plugin stayed off. It now purges every alias like `hermes plugins enable/disable` (_apply_activation, shared with _set_plugin_enabled), with one discovery scan per save. - The success line counts the rows actually turned on/off instead of treating every unticked row as "disabled"; the unused new_enabled return is gone. - _entry_status shares the per-row status call between the picker preselection and `hermes plugins list --enabled`. --- hermes_cli/plugins_cmd.py | 20 +++++---- hermes_cli/plugins_cmd_listing.py | 12 +++--- hermes_cli/plugins_cmd_toggle.py | 69 ++++++++++++------------------- 3 files changed, 46 insertions(+), 55 deletions(-) diff --git a/hermes_cli/plugins_cmd.py b/hermes_cli/plugins_cmd.py index a9894698eb..8c41b49491 100644 --- a/hermes_cli/plugins_cmd.py +++ b/hermes_cli/plugins_cmd.py @@ -501,12 +501,13 @@ def _discard_key_and_leaf(names: set, key: str) -> None: names.discard(key.split("/")[-1]) -def _plugin_aliases(key: str) -> set: +def _plugin_aliases(key: str, entries: Optional[list] = None) -> set: """Every spelling a config list may hold for *key*: the key, its bare leaf and the manifest name. The loader matches BOTH the canonical key (``web/firecrawl``) and the manifest name - (``web-firecrawl``), so a stale entry under any form vetoes an enable ("explicit disable wins").""" + (``web-firecrawl``), so a stale entry under any form vetoes an enable ("explicit disable wins"). + Pass *entries* to reuse one :func:`_discover_all_plugins` scan across several keys.""" names = {key, key.split("/")[-1]} - names.update(e[0] for e in _discover_all_plugins() if e[5] == key) + names.update(e[0] for e in (_discover_all_plugins() if entries is None else entries) if e[5] == key) return names @@ -566,15 +567,20 @@ def _set_plugin_enabled(name: str, *, enable: bool, aliases=(), console=None) -> plugins = config.get("plugins") or {} enabled = set(plugins.get("enabled") or ()) disabled = set(plugins.get("disabled") or ()) - removed = disabled if enable else enabled - _discard_key_and_leaf(removed, name) - removed.difference_update(aliases) - (enabled if enable else disabled).add(name) + _apply_activation(enabled, disabled, name, aliases, enable=enable) _admit_and_save_plugin_sets(enabled, disabled, console=console, action=f"{'Enable' if enable else 'Disable'} '{name}'", expected_config=expected_config, plugin=name if enable else None) +def _apply_activation(enabled: set, disabled: set, key: str, aliases, *, enable: bool) -> None: + """Add canonical *key* to the target list and purge it, its bare leaf and *aliases* from the other.""" + removed = disabled if enable else enabled + _discard_key_and_leaf(removed, key) + removed.difference_update(aliases) + (enabled if enable else disabled).add(key) + + def _resolve_plugin_key(name: str) -> Optional[str]: """Canonical registry key for a manifest name / directory name / path key, or ``None``. The single normalization point so enable/disable write the key ``PluginManager`` gates on.""" diff --git a/hermes_cli/plugins_cmd_listing.py b/hermes_cli/plugins_cmd_listing.py index b1c29e10a7..c89f5b5c81 100644 --- a/hermes_cli/plugins_cmd_listing.py +++ b/hermes_cli/plugins_cmd_listing.py @@ -24,14 +24,16 @@ def _filter_plugin_entries(entries: list, args: Any, enabled: set, disabled: set filtered = [entry for entry in filtered if entry[3] != "bundled"] if getattr(args, "enabled", False): active = _pc()._category_active_names() - filtered = [ - entry for entry in filtered - if _pc()._plugin_status(entry[0], enabled, disabled, key=entry[5], source=entry[3], dir_path=entry[4], - active=active) == "enabled" - ] + filtered = [entry for entry in filtered if _entry_status(entry, enabled, disabled, active) == "enabled"] return filtered +def _entry_status(entry: tuple, enabled: set, disabled: set, active: set) -> str: + """``_plugin_status`` for one ``_discover_all_plugins`` row.""" + name, _version, _description, source, dir_path, key = entry + return _pc()._plugin_status(name, enabled, disabled, key, source=source, dir_path=dir_path, active=active) + + _STATUS_MARKUP = {"disabled": "[red]disabled[/red]", "enabled": "[green]enabled[/green]"} diff --git a/hermes_cli/plugins_cmd_toggle.py b/hermes_cli/plugins_cmd_toggle.py index 02e9e613e0..4c0a426136 100644 --- a/hermes_cli/plugins_cmd_toggle.py +++ b/hermes_cli/plugins_cmd_toggle.py @@ -117,52 +117,36 @@ def cmd_toggle() -> None: def _effective_plugin_selection(entries, enabled: set, disabled: set) -> set: - """Row indices ticked when the picker opens: every plugin that is ACTIVE right now, by the same - rule ``gate_manifest`` applies at load (``_plugin_status``), not merely the ones listed in - ``plugins.enabled``. Bundled platforms, backends and model providers are on without a list - entry, so listing-based preselection showed them unticked and a no-change exit disabled every - messaging adapter on the next gateway restart.""" + """Row indices ticked on open: the load-time rule, not list membership — bundled platforms, + backends and model providers are active without a ``plugins.enabled`` entry.""" + from hermes_cli.plugins_cmd_listing import _entry_status active = _pc()._category_active_names() - return { - i for i, (name, _v, _desc, source, dir_path, key) in enumerate(entries) - if _pc()._plugin_status(name, enabled, disabled, key, source=source, dir_path=dir_path, - active=active) == "enabled" - } + return {i for i, entry in enumerate(entries) if _entry_status(entry, enabled, disabled, active) == "enabled"} -def _persist_plugin_selection(plugin_keys, chosen, disabled, initial, *, expected_config=None) -> tuple[bool, set]: - """Save the composite UI's checkbox state; returns ``(changed, new_enabled)``. +def _persist_plugin_selection(plugin_keys, chosen, disabled, initial, *, expected_config=None) -> tuple[list, list]: + """Write only the rows flipped this session; returns the ``(turned_on, turned_off)`` keys. - Only rows the user actually flipped are written: a plugin unticked in this session goes to - the disabled-list (so it stays off even if something auto-enables it) under the canonical key - ONLY, so the list can't drift from what ``cmd_enable`` clears; a plugin ticked in this session - is added to ``plugins.enabled`` and any stale legacy bare-leaf disable is dropped. Rows left - as they were are not persisted, so "never ticked" is never mistaken for "explicitly unticked" - and list entries for plugins not shown in the picker survive. + Untouched rows are never persisted, so a never-ticked row is not mistaken for an explicit untick + and list entries for plugins the picker didn't show survive. Canonical key only, with every alias + purged from the opposing list like ``hermes plugins enable/disable`` (#40190). """ - # Persist by canonical key only — never the bare manifest name — so the disabled-list stays aligned with - # cmd_enable / PluginManager (#40190). - if expected_config is None: - expected_config = _pc()._plugin_selection_version() - new_enabled: set = set(_pc()._get_enabled_set()) - new_disabled: set = set(disabled) turned_on = [plugin_keys[i] for i in sorted(chosen - initial)] turned_off = [plugin_keys[i] for i in sorted(initial - chosen)] - for key in turned_on: - new_enabled.add(key) - _pc()._discard_key_and_leaf(new_disabled, key) - for key in turned_off: - _pc()._discard_key_and_leaf(new_enabled, key) - new_disabled.add(key) - - changed = bool(turned_on or turned_off) - if changed: - # C13: the composite UI's candidate goes through the ONE admission - # authority — refusal raises AdmissionRefused BEFORE any config - # write; the caller surfaces it and the selection stays unsaved. - _pc()._admit_and_save_plugin_sets(new_enabled, new_disabled, action="Save plugin selection", expected_config=expected_config) - logger.info("plugins picker: enabled %s; disabled %s", turned_on or "none", turned_off or "none") - return changed, new_enabled + if not (turned_on or turned_off): + return [], [] + if expected_config is None: + expected_config = _pc()._plugin_selection_version() + new_enabled, new_disabled = set(_pc()._get_enabled_set()), set(disabled) + entries = _pc()._discover_all_plugins() + for keys, enable in ((turned_on, True), (turned_off, False)): + for key in keys: + _pc()._apply_activation(new_enabled, new_disabled, key, _pc()._plugin_aliases(key, entries), enable=enable) + # C13: the composite UI's candidate goes through the ONE admission authority — refusal raises + # AdmissionRefused BEFORE any config write; the caller surfaces it and the selection stays unsaved. + _pc()._admit_and_save_plugin_sets(new_enabled, new_disabled, action="Save plugin selection", expected_config=expected_config) + logger.info("plugins picker: enabled %s; disabled %s", turned_on or "none", turned_off or "none") + return turned_on, turned_off def _run_composite_ui(curses, plugin_keys, plugin_labels, plugin_selected, disabled, categories, console, *, expected_config=None): @@ -278,7 +262,7 @@ def _run_composite_ui(curses, plugin_keys, plugin_labels, plugin_selected, disab from hermes_cli.plugins_admission import AdmissionRefused try: - changed, _new_enabled = _persist_plugin_selection(plugin_keys, chosen, disabled, plugin_selected, + turned_on, turned_off = _persist_plugin_selection(plugin_keys, chosen, disabled, plugin_selected, expected_config=expected_config) except AdmissionRefused as exc: console.print(f"[red]✗[/red] Plugin selection refused, not saved: {exc}") @@ -287,10 +271,9 @@ def _run_composite_ui(curses, plugin_keys, plugin_labels, plugin_selected, disab "Run `hermes pm install` to resolve, then retry.[/dim]" ) return - if changed: + if turned_on or turned_off: console.print( - f"\n[green]\u2713[/green] General plugins: {len(chosen)} enabled, " - f"{len(plugin_keys) - len(chosen)} disabled.") + f"\n[green]\u2713[/green] General plugins: {len(turned_on)} turned on, {len(turned_off)} turned off.") elif n_plugins > 0: console.print("\n[dim]General plugins unchanged.[/dim]") if providers_changed: