Plugins loaded mid-run wire their platform handlers live; install surfaces report active-now vs next-session (#87770) - #119266
Conversation
૮ >ﻌ< ა ci reviewran on 8101b11 — fix: portable MCP server names in the activation summary are debug infoCI timingsCI timings · View report · View jobWall time 6m14s vs 5m21s (+16.5%). 10 job(s) slower, 4 faster,
|
cc54d0b to
23589b6
Compare
…87770) A plugin that finished loading after an adapter connected never got its platform handlers (slash commands, button callbacks, inbound transforms) registered until a gateway restart, silently. Three pieces, one seam shared by every surface: 1. Discovery listener: PluginManager.on_plugin_loaded(cb) fires from INSIDE discover_and_load for the plugins a sweep newly loaded (diff of the loaded set), with a per-plugin activation summary (hermes_cli/plugins_activation.py): activated_now {gateway_commands, gateway_transforms, hooks, callbacks} vs deferred {tools, prompt, mcp_servers}. Every mid-run load path now performs a real discover_plugins(force=True): CLI install/enable (via the gateway), Desktop/TUI plugins.manage install/toggle/update, dashboard REST install, tool-triggered force re-discovery, the new `reload-plugins` control-socket verb. A non-forced discover_plugins() short-circuits on _discovered, which is why reload.mcp after a mid-run install used to reload the OLD server set. 2. Idempotent re-wire: BasePlatformAdapter.rewire_plugin_handlers() runs only factories not yet wired on the live native client (keyed (plugin, qualname); a force reload hands back new function objects). Telegram hoists late handlers ahead of core's catch-all filters.COMMAND / CallbackQueryHandler (PTB dispatches the first match per group) and re-wires on the transient-init rebuild; Slack dedupes register_slack_action_handler per AsyncApp. The gateway runner subscribes per served profile and re-wires on the loop. 3. Scope limit + honest messaging: handlers only. Tools/prompt stay deferred to the next session (prompt-cache invariant), MCP servers to mcp.reload; the CLI hint and plugins.manage results (activation, gateway_reloaded, restart_required only when no gateway answered) say exactly that.
…json names (post-#119263)
23589b6 to
8101b11
Compare
andrexibiza
left a comment
There was a problem hiding this comment.
Exact-head review — live activation is strong; update and receipt semantics still need to close
The core shape is good. The discovery-listener seam is the right place to solve #87770, profile-scoped subscriptions are carried onto the gateway loop, Telegram preserves its first-match ordering, Slack gets per-app dedupe, and disable still honestly requires restart because there is no un-wire primitive. The branch is also based directly on current main (07610317671ca161c2e530cdb2858560375ba979, which includes #119263), so there is no stale-base ambiguity in this review.
Reviewed exact head 8101b11454ae5de3851ba870b69001b6a4b991a1: all 23 changed files/file-list, the behavior-critical loader/activation/adapter/runner/TUI paths, new tests, #87770 and its decision comment, merged hook siblings #118844/#118845, merged MCP-name predecessor #119263, open portable-MCP sibling #87350, and exact-head Actions. There was no substantive human review on this head when I checked.
Not ready to merge yet: two P1 correctness gaps remain in the contract this PR advertises. Both are about preserving the distinction between “registry reloaded” and “native handler is actually live.”
P1 — updating an already-loaded plugin cannot replace its live native handler, but the API reports no restart required
activate_plugin_now() is used for update as well as install/enable and force-runs discovery. But PluginManager.discover_and_load(force=True) snapshots the currently loaded keys before unload, then _notify_plugin_loaded() only emits summaries whose key was not in that pre-sweep set. An updated plugin keeps the same key, so no loaded event fires for it.
That is not just theoretical: the new test explicitly pins the behavior — a second discover_plugins(force=True) with the same plugin must produce no event. Separately, BasePlatformAdapter deliberately dedupes (plugin_name, factory.__qualname__); the new test also explicitly proves that a new factory object for the same plugin/qualname is skipped on the same native client. Slack does the analogous thing with (repr(action_id), plugin).
Those two invariants are correct for an unchanged force rescan, but together they make a changed plugin update impossible to activate live: the old PTB/Bolt/native callback remains registered, the replacement factory is not invoked, and activate_plugin_now() still returns gateway_reloaded=True, restart_required=False because the control socket answered.
Required repair: distinguish new plugin load / unchanged rescan / changed-plugin replacement. With no un-wire primitive, a changed plugin whose native handlers were already wired must either (a) be explicitly reported as restart-required/deferred for those handlers, or (b) use a real replacement mechanism that proves the old native registration was displaced. Do not clear the dedupe set and append a second handler; that recreates the duplicate/ordering failure the PR correctly avoids. Add update regressions for Telegram, Slack action handlers, and the base platform-handler path that change callback behavior under the same plugin key and qualname/action id.
P1 — reloaded=True is not a receipt that handlers were re-wired, so “active now” can be false
reload_plugins_verb() schedules _count_adapters() after the loaded callback and then returns reloaded=True unconditionally. But _count_adapters() only counts adapter objects; it does not report whether adapter.rewire_plugin_handlers() succeeded. If scheduling/waiting exceeds 5s, rewired becomes None and the verb still returns reloaded=True. Inside the adapter, a raising platform-handler factory is logged and then marked wired, so later rewires will not retry it on that native client.
activate_plugin_now() reduces all of that to bool(answer["reloaded"]) and therefore sets restart_required=False; activation_hint() can then say active in the running gateway now: callbacks even though no callback was installed. That is the same user-visible class #87770 is trying to eliminate: registration exists, but the running adapter does not actually have the handler.
Required repair: make the control-socket response carry a settled wiring result, not an adapter count. At minimum, distinguish success / partial failure / timeout and only set gateway_reloaded (or a separate handlers_active_now) when the requested profile's relevant adapter rewires completed without error. A factory exception should not be permanently converted into a successful live-activation receipt. Add negative tests for factory raise and loop timeout, and assert the user-facing result does not claim active-now/no-restart in those cases.
Interlocks / provenance
- Origin / decision: #87770 was opened by
paoloantinorias follow-up to #61576. The issue's design comment explicitly called out per-native-client idempotence, Telegram ordering, no un-wire, and a discovery listener; this PR implements that chosen direction rather than duplicating the earlier hook packs. - Merged siblings: #118844 and #118845 are complementary hook-policy / hook-surface work. #118845 explicitly deferred #87770 because it needed this discovery-listener + adapter-rewire design. This PR is the intended continuation, not a supersession of their hook semantics.
- Immediate base predecessor: #119263 by
alt-glitchis already merged as this PR's exact base and changed portable MCP server naming. The head's activation summary correctly follows that canonical server-name rule; preserve that lineage. - Open sibling #87350: it also touches
tui_gateway/methods_tools.py, but at themcp.servers.set_api_keypath while this PR changesplugins.managearound ~1537–1635. I found file-level overlap but not the same hunk; treat it as a rebase/integration watch, not a demonstrated semantic duplicate or hard merge-order dependency.
Verification / CI state
I could not run the repository suite in this environment; direct network checkout is unavailable here. I did run a tiny source-contract witness over the exact conditions above: existing-key force reload produced update_notifies=False; same (plugin, qualname) replacement produced replacement_factory_runs=False; and {reloaded: true, adapters_rewired: null} yields restart_required=False. The stronger evidence is already in the branch's own tests, which intentionally assert the first two behaviors independently.
Exact-head CI is still running, so there is no all-green merge receipt yet. At inspection time: attribution, common-ancestor, supply-chain scan, Python e2e, macOS-only, Ruff enforcement, Windows footguns, and Ruff+ty were green; Python full tests, Windows-only, JS/TS, docs, Docker, and Nix were still in progress. CI run: https://github.com/NousResearch/hermes-agent/actions/runs/35755982364 ; Docker: https://github.com/NousResearch/hermes-agent/actions/runs/35755981932 ; Nix: https://github.com/NousResearch/hermes-agent/actions/runs/35755981979 .
The narrow class invariant I would keep through the repair is: a plugin registry reload may be reported immediately, but “handler active now” requires proof that the live native registration for this plugin/version was installed; replacement without un-wire must fail closed to restart-required rather than silently retaining stale behavior.
| return | ||
| # ``on_plugin_loaded`` reports the plugins this sweep loads that the process did not have before | ||
| # (boot: everything; a mid-run install/enable: just the newcomer), keyed on the pre-sweep set. | ||
| loaded_before = frozenset(k for k, p in self._plugins.items() if not p.error and not p.deferred) |
There was a problem hiding this comment.
P1 — this newcomer-only edge excludes plugin updates. loaded_before contains every currently loaded key, then force=True unloads/reloads them, and _notify_plugin_loaded() filters the post-sweep summaries against this set. An updated plugin keeps the same key, so no event fires. The new gateway test explicitly asserts a second force discovery produces no event, while plugins.manage update calls activate_plugin_now() and later reports restart_required=False when the gateway answers. Combined with per-native (plugin, qualname) dedupe, the old native callback stays installed and the replacement cannot go live. Please distinguish newcomer / unchanged rescan / changed replacement; without an un-wire/replacement primitive, changed native handlers need an honest restart-required result rather than a live-activation claim.
| except Exception: | ||
| rewired = None | ||
| return {"reloaded": True, "home": str(requested), "plugins": names, "activations": activations, | ||
| "adapters_rewired": rewired} |
There was a problem hiding this comment.
P1 — reloaded=True is not a wiring receipt here. The scheduled coroutine only counts adapter objects; _rewire_plugin_handlers() failures are logged elsewhere and not reflected, and the 5s wait can set rewired=None. This still returns reloaded=True, which activate_plugin_now() converts to restart_required=False and can present as active in the running gateway now. Please propagate a settled success/partial/timeout result from the actual adapter rewires and gate the active-now/no-restart claim on that result. A raising factory is especially important because base wiring currently records the key as wired even after the exception, so the same native client will not retry it.
…P servers instead of a gateway restart (#119349) The success toast after a Desktop plugin install said "Restart the gateway for the plugin to take effect" with a button that restarts the messaging gateway. The Desktop chat talks to `hermes serve`, a different process, so the button did nothing for the plugin's MCP servers and the user relaunched the whole app to get them. Observed live on the x64 Windows desktop installing nvidia-app. `plugins.manage install` now returns `activation.deferred.mcp_servers` (the plugin's servers not yet connected) and `gateway_reloaded` (#119266), and the backend rescans plugins on the install path, so a follow-up `reload.mcp` connects them in place. The toast now branches on that: deferred.mcp_servers non-empty → "<name> installed. Its MCP server is not connected yet." + Connect now → reload.mcp {confirm: true, session_id?} → plugin list refresh gateway_reloaded → "<name> installed and active." (no button) otherwise → the existing restart toast, unchanged (native plugin the gateway missed) `installAgentPlugin` surfaces the two fields as `deferredMcpServers` / `gatewayReloaded`. New i18n keys in every locale. Two behaviour tests: deferred servers → Connect now fires reload.mcp with confirm:true; no activation → restart toast still fires. Verified on main a53b42d: install nvidia-app from the catalog, then `/reload-mcp now` (the same RPC the button calls) → `MCP server 'nvidia-app' (HTTP): registered 12 tool(s)` 0.5 s later, no relaunch.
…P servers instead of a gateway restart (NousResearch#119349) The success toast after a Desktop plugin install said "Restart the gateway for the plugin to take effect" with a button that restarts the messaging gateway. The Desktop chat talks to `hermes serve`, a different process, so the button did nothing for the plugin's MCP servers and the user relaunched the whole app to get them. Observed live on the x64 Windows desktop installing nvidia-app. `plugins.manage install` now returns `activation.deferred.mcp_servers` (the plugin's servers not yet connected) and `gateway_reloaded` (NousResearch#119266), and the backend rescans plugins on the install path, so a follow-up `reload.mcp` connects them in place. The toast now branches on that: deferred.mcp_servers non-empty → "<name> installed. Its MCP server is not connected yet." + Connect now → reload.mcp {confirm: true, session_id?} → plugin list refresh gateway_reloaded → "<name> installed and active." (no button) otherwise → the existing restart toast, unchanged (native plugin the gateway missed) `installAgentPlugin` surfaces the two fields as `deferredMcpServers` / `gatewayReloaded`. New i18n keys in every locale. Two behaviour tests: deferred servers → Connect now fires reload.mcp with confirm:true; no activation → restart toast still fires. Verified on main a53b42d: install nvidia-app from the catalog, then `/reload-mcp now` (the same RPC the button calls) → `MCP server 'nvidia-app' (HTTP): registered 12 tool(s)` 0.5 s later, no relaunch.
A plugin loaded after the gateway started now gets its platform handlers (slash commands, button callbacks, inbound transforms) wired live — no restart — and every install surface reports honestly what is active now vs deferred.
Fixes #87770.
Design (approved by @teknium1)
PluginManager.on_plugin_loaded(cb)(hermes_cli/plugins_loader.py) fires from insidediscover_and_loadfor the plugins a sweep newly loaded (diff of the loaded set before/after; never emitted by an RPC). Payload: one activation summary per plugin (hermes_cli/plugins_activation.py):{name, key, activated_now: {gateway_commands|gateway_transforms|hooks|callbacks: [names]}, deferred: {tools|prompt|mcp_servers: [names]}}—deferred.mcp_serverslists the plugin'smcp.jsonserver names (<namespace>__<server>) and is documented on thePluginActivationcontract for the Desktop reload card.BasePlatformAdapter.rewire_plugin_handlers()re-reads the factory registry and runs only factories not yet wired on the live native client, keyed(plugin, qualname)(a force reload hands back new function objects, so identity alone would double-register). Telegram hoists late handlers ahead of core's catch-allfilters.COMMAND/CallbackQueryHandler(PTB dispatches the first match per group) and re-wires on the transient-init rebuild; Slack dedupesregister_slack_action_handlerperAsyncApp. The gateway runner (gateway/run_plugin_rewire.py) subscribes per served profile at boot and re-wires every live adapter on the loop. Discord and every other adapter use the base method unchanged (their handler state has nothing that resists the dedupe)./skills install); portable MCP servers untilmcp.reload. No un-wire: disabling keeps wired handlers until restart, and surfaces say so.Every mid-run load path is a real rescan
Facts verified on
origin/main:discover_plugins()short-circuits on_discoveredunlessforce=True;tools/mcp_tool_config._portable_mcp_serverscalls it non-forced, soreload.mcpafter a mid-run install reloaded the OLD server set;tui_gateway/methods_tools._plugins_installreturned right afterdashboard_install_pluginwith no rescan. Nowactivate_plugin_now()runsdiscover_plugins(force=True)in the serving process (Desktop/TUIplugins.manage install/toggle/update, dashboard REST install) and nudges the running gateway through the newreload-pluginscontrol-socket verb (served homes only; the answer carriesplugins,activations,adapters_rewired).hermes plugins install/enable(separate process) nudge the gateway and print the split.Honest messaging (extends the existing restart-hint fields, no second message)
Gateway reloaded plugins — active in the running gateway now: callbacks; deferred: tools (next session)— or the old restart hint when no gateway answered.plugins.managetoggle/install/update result:activation+gateway_reloaded;restart_requiredis true only when no gateway answered. Contracts regenerated (scripts/gen_gateway_contracts.py).PluginsManageResult.python_dependencies, the Desktop "Installed. Connect its servers now" card wired tomcp.reload confirm=true, and the "not connected yet" pill copy. This PR only makesdeferred.mcp_serversavailable.Validation
PluginManager+ realTelegramAdapter+ real PTBApplication.process_update, offline; harness because a Telegram stub is impractical)/late -> ['core:_handle_command'], norewire_plugin_handlers. after:on_plugin_loadedfires once forlate_cmd(activated_now: {callbacks: [telegram]}), group0 =[CommandHandler, MessageHandler×4, CallbackQueryHandler, InlineQueryHandler],/late -> ['plugin:/late'](exactly ONE reply); after 2 more force loads + 3 re-wires still 1 CommandHandler, 1 replyAttributeError: no attribute 'on_plugin_loaded'/ collection)plugins.manage install→ real rescan → callback → later non-forced_portable_mcp_servers()sees the new plugin's MCP serversscripts/run_tests.sh tests/gateway tests/hermes_cli tests/tui_gateway tests/pluginsorigin/main(test_dashboard_auth_gate×4 port-held,test_local_runtime_recovery×1)git diff --checkExisting test changed:
test_plugins_cmd_activation_keysasserted the toggle result dict exactly; it now checks the same four keys plusgateway_reloaded is Falsebecause the enable also loads the plugin.Root cause in one sentence: adapters consumed
get_platform_handler_factories()only insideconnect()and nothing told them discovery ran again.Infographic