fix(gateway): register secondary profiles' shell hooks and outbound webhooks - #92682
chelsealong wants to merge 2 commits into
Conversation
…ebhooks Multiplex gateway startup only ever calls agent.shell_hooks/ outbound_webhooks register_from_config() once, against the root/default profile's config, before any profile scope exists. _start_one_profile_adapters() discovers Python plugins per profile but never registered that profile's own declarative `hooks:` block, so a secondary profile's shell hooks (e.g. a deny-writes gate) and outbound webhooks silently never fire. Load and register each profile's own config inside its _profile_runtime_scope, and key the module-level idempotence sets in shell_hooks.py/outbound_webhooks.py by resolved Hermes home so two profiles configuring an identical hook/webhook both register on their own plugin manager instead of the second being dropped as a duplicate of the first. Fixes NousResearch#92672
andrexibiza
left a comment
There was a problem hiding this comment.
Blocking review on exact head f828c27457bf8e1f2df296463e2104e9a2c48359.
The primary fix is pointed at the right boundary: secondary profiles now load their own config under _profile_runtime_scope, and adding the resolved home to the shell/outbound idempotence key is necessary so identical config in two profile-local PluginManagers does not shadow. Exact-head CI/Docker/Nix are green, and the branch is a clean one-commit child of current main fd760435c6688a2b6c6b7436dde30e267237baef.
There is still one lifecycle/ownership blocker before this can safely become the profile-scoped registration contract: the idempotence ledger is now per-home, but its reset/reload lifecycle is still process-global and asymmetric.
For shell hooks, agent.shell_hooks.re_register_config_hooks() still does _registered.clear() for the entire process and then re-registers only the current home. PluginManager.discover_and_load(force=True) runs under _plugin_home_scope(self.home_path), calls self.unload() (which clears only that manager's _hooks), then invokes _re_register_shell_hooks_after_force(). In a multiplex process with homes A and B:
- A and B each register the same shell hook once; both managers correctly hold one callback and
_registeredhas A/B keys. - A force-reloads plugins. A's manager clears its hooks;
re_register_config_hooks()clears all A/B idempotence keys, then restores only A. - B's existing callback was never removed, but B's idempotence key is now gone.
- The next B adapter restart/reconnect runs this PR's new
register_from_config()call; it sees no B key and appends a second callback to B's still-live manager.
That makes a profile-local force reload in A mutate B's registration authority and can cause B's policy script to execute twice (including duplicate approval prompts / side effects). The new home dimension therefore is not actually owned end-to-end yet.
Outbound webhooks expose the mirror failure in the other direction. PluginManager.unload() clears the current manager's _hooks, but the force-reload path only re-registers shell hooks. agent.outbound_webhooks._registered is not cleared or replayed. After B force-reloads, its outbound callback is gone while (home_B,event,url) remains marked registered; a later _start_one_profile_adapters() call hits that stale key and skips re-wiring, so the webhook remains silently inert. That is the same symptom class #92672 is trying to close, just after a supported lifecycle transition instead of initial startup.
Please make config-owned hook registration have profile-local lifecycle ownership rather than patching only the startup key:
- reset/re-register shell-hook idempotence only for the manager/home being force-reloaded, never all homes;
- make outbound webhook callbacks participate in the same symmetric force-reload restoration, with stale idempotence state impossible;
- preferably centralize current-home config-hook registration/teardown so startup, profile adapter recovery, and plugin force reload all use one authority instead of separate module-global ledgers;
- add a real two-home regression: A+B identical shell hook, force-reload A, re-run B registration/reconnect, and assert B still has exactly one callback;
- add B outbound webhook -> force-reload B -> callback still present and fires exactly once;
- extend the current test beyond mocked
register_from_configcalls to invoke through the actual profile-scoped manager, so the per-home key and dispatch behavior are what is proven.
Topology/provenance:
- #92672 by @vszgdcn8cj-ctrl owns the concrete multiplex-profile failure; #92682 by @chelsealong is the right delivery object for that axis.
- #92655 by @LiTerBo is complementary, not duplicate: it fixes the separate
hermes serve/ Desktop startup surface where config shell hooks are never registered at all. - #60036 / open #60267 by @webtecnica is the prior force-reload shell-hook defect and the source of the current
re_register_config_hooks()shape; current-main comments explicitly preserve that lineage via the #64188 salvage work. This PR now needs to generalize that older single-home repair to multi-home ownership rather than invalidating another profile's ledger. - #64178 / closed #64188 by @Bartok9 is the broader hook-delivery parity lineage and explicitly called force-reload symmetry only partial. This is exactly the remaining other side of that shape.
No separate code finding beyond this lifecycle authority issue: the initial per-profile registration call and home-scoped dedupe direction are sound. Hosted evidence on this exact SHA is genuinely green: CI 32616082267, Docker 32616081794, Nix 32616081787. Those tests do not exercise force reload across two live profile managers, which is why they do not close the blocker.
GitHub does not permit this reviewer identity to submit formal REQUEST_CHANGES, so this blocking disposition is recorded as COMMENTED.
re_register_config_hooks() cleared the entire process-global idempotence set on every force-reload, so a profile-local plugin force-reload dropped another live profile's ledger key without touching its still-registered callback — the next registration call for that profile then appended a duplicate. Scope the clear to the reloading profile's own home, and give outbound webhooks the same force-reload restoration shell hooks already had, since unload() wipes both from the shared _hooks dict.
|
Fixed the lifecycle-authority gap in c435c0a:
Full run: |
andrexibiza
left a comment
There was a problem hiding this comment.
Exact-head rereview — lifecycle blocker closed; acceptance now blocked only on fresh CI
Reviewed exact head c435c0ac809de13b8735167b31e6bc4b4d066c53 against my prior f828c27457bf8e1f2df296463e2104e9a2c48359 lifecycle-authority finding.
The code-level blocker is closed.
- Shell-hook idempotence teardown/replay is now current-home scoped instead of clearing the process-global ledger.
- Outbound webhooks now participate in the same force-reload restoration path, so callback state and idempotence state remain symmetric.
PluginManager.discover_and_load(force=True)executes under_plugin_home_scope(self.home_path), so the "current home" used by both re-registration helpers is the manager actually being reloaded.get_plugin_manager()is already cached per resolved Hermes home, so manager ownership and the new home-qualified ledgers now align on the same identity dimension.- The new two-manager shell-hook regression exercises force-reload A followed by B recovery and proves B remains wired exactly once; the outbound-webhook regression proves the reloaded callback is restored and fires once.
I do not see a remaining code-level blocker in this seven-file shape.
Exact-head Docker 32617192952 and Nix 32617192947 are green. CI 32617193278 is red only in e2e job 97139723243, where tests/e2e/test_platform_commands.py::TestSlashCommands::test_plaintext_restart_gateway_in_group_stays_plain_text[telegram] fails because its mocked send was called zero times instead of once. This PR does not touch that test or its command-routing implementation; the normal Python suite, lints, OSV/supply-chain checks, Windows-only and macOS-only lanes are green. That makes the remaining boundary repository acceptance, not a demonstrated hook-lifecycle regression.
I attempted to rerun the exact failed e2e job, but GitHub rejected the mutation with 403 Resource not accessible by integration, so I cannot manufacture the required fresh green exact-head receipt from this identity.
Disposition: the previous multiplex hook/webhook lifecycle-authority blocker is resolved. Keep the PR blocked only on exact-head CI acceptance; no further code change is requested by this review.
|
The red Same test, same assertion (
#92130 explains why the message carries no information: Noted on #92909 today: no other open PR could be found running Python e2e to compare against. This run is that comparison — same job, same test, same assertion, on a change confined to hook and webhook registration. |
|
Thanks @chelsealong — this landed. Merged via #101255 (bd81bf0) on current main. Your commits were cherry-picked onto the salvage branch with your git authorship preserved. Closing this PR as merged-via-salvage. |
Fixes #92672.
Root cause
With
gateway.multiplex_profiles: true,GatewayRunner.start()registers declarative shell hooks and outbound webhooks exactly once, at process startup, against the root/default profile's config (gateway/run.py, theregister_from_configcall site right beforeself.hooks.discover_and_load()). That call runs before any profile scope exists.Secondary profiles are started later, in
_start_one_profile_adapters(), which runs each profile's turns under its own_profile_runtime_scope(its ownHERMES_HOMEoverride) and already callsdiscover_plugins()there so per-profile plugins work — but it never calledagent.shell_hooks.register_from_config()/agent.outbound_webhooks.register_from_config()for that profile's ownconfig.yaml. Becausehermes_cli.plugins.get_plugin_manager()caches onePluginManagerper resolved home, a secondary profile's turns dispatch through its own manager, which never received the root profile's (or its own) hook callbacks. Result: a secondary profile'shooks.pre_tool_call/hooks.outboundblock is silently inert — no error, no webhook fires, and a security gate (e.g. a deny-writes hook) never runs.Additional wrinkle noted in the issue:
agent/shell_hooks.py's_registeredidempotence set (and the analogous one inagent/outbound_webhooks.py) was keyed only by(event, matcher, command)/(event, url), with no home/profile scoping. Even after wiring the registration call into the secondary-profile startup path, two profiles configuring an identical hook would have the second call see the triple already in_registeredand skip wiring it onto its own (different) plugin manager.Fix
gateway/run.py: inside_start_one_profile_adapters(), within the profile's_profile_runtime_scope, load that profile's own config and callagent.shell_hooks.register_from_config()/agent.outbound_webhooks.register_from_config()against it (mirrors the exact call shape used at root startup, includingaccept_hooks=Falsesohooks_auto_acceptin the profile's own config is honored). Failures are logged and never block startup, matching the root startup call site's behavior.agent/shell_hooks.py/agent/outbound_webhooks.py: the module-global idempotence sets are now keyed by resolved Hermes home in addition to the existing fields, so identical hook/webhook configuration in two different profiles registers independently on each profile's own plugin manager instead of the second profile's attempt being dropped as a duplicate.Test plan
Added
TestSecondaryProfileHookRegistrationtotests/gateway/test_multiplex_adapter_registry.py, asserting that_start_one_profile_adapters()calls bothagent.shell_hooks.register_from_configandagent.outbound_webhooks.register_from_configwith the secondary profile's own config.Confirmed the new test fails without the fix:
And passes with it, alongside the full surrounding suites:
Notes
Authored with AI assistance; the diff, reproduction, and test were verified manually (real test runs, real ruff run, and a stash-based before/after check of the new regression test) before pushing.