From 600b9c7e7e47e6e5f441ab62883c445614e30074 Mon Sep 17 00:00:00 2001 From: chelsealong Date: Sun, 23 Aug 2026 04:09:35 +0000 Subject: [PATCH] fix(gateway): scope force-reload hook re-registration to its own home MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- agent/outbound_webhooks.py | 23 +++++++++++ agent/shell_hooks.py | 11 +++++- hermes_cli/plugins.py | 27 +++++++++---- tests/agent/test_outbound_webhooks.py | 40 +++++++++++++++++++ tests/hermes_cli/test_plugins.py | 55 ++++++++++++++++++++++++++- 5 files changed, 146 insertions(+), 10 deletions(-) diff --git a/agent/outbound_webhooks.py b/agent/outbound_webhooks.py index dfdc4ccac3..0f29d5e50f 100644 --- a/agent/outbound_webhooks.py +++ b/agent/outbound_webhooks.py @@ -237,6 +237,29 @@ def flush(timeout: float = 5.0) -> bool: return _delivery_queue.unfinished_tasks == 0 +def re_register_config_hooks() -> None: + """Re-register outbound webhooks from config after a plugin force-reload. + + Mirrors ``agent.shell_hooks.re_register_config_hooks``: config-owned + outbound-webhook callbacks live in the same ``_hooks`` dict that + ``PluginManager.discover_and_load(force=True)`` clears via ``unload()``, + so without this the force-reloaded profile's outbound webhooks go + silently inert (#92682 review). Only the current home's idempotence + keys are cleared so a force-reload in one profile cannot invalidate + another profile's still-live registration. + """ + from hermes_cli.config import load_config + from hermes_constants import get_hermes_home + + home_key = str(get_hermes_home().expanduser().resolve()) + with _registered_lock: + _registered.difference_update( + {key for key in _registered if key[0] == home_key} + ) + + register_from_config(load_config()) + + def reset_for_tests() -> None: """Clear the idempotence set and drain the queue. Test-only helper.""" with _registered_lock: diff --git a/agent/shell_hooks.py b/agent/shell_hooks.py index 289cf39d04..49dfa9301e 100644 --- a/agent/shell_hooks.py +++ b/agent/shell_hooks.py @@ -354,11 +354,20 @@ def re_register_config_hooks() -> None: are wired again (#60036 / PR #60267; tracking #64178 — salvaged from PR #64188). + Only the idempotence keys for the *current* Hermes home are cleared — + ``discover_and_load(force=True)`` only unloads the manager scoped to + that one home, so clearing every home's keys would make a force-reload + in profile A drop profile B's still-live registration from the ledger + and duplicate it on B's next registration call (#92682 review). + Commands already allowlisted stay allowlisted, so this never re-prompts at a TTY for hooks the user previously approved. """ + home_key = str(get_hermes_home().expanduser().resolve()) with _registered_lock: - _registered.clear() + _registered.difference_update( + {key for key in _registered if key[0] == home_key} + ) from hermes_cli.config import load_config register_from_config(load_config()) diff --git a/hermes_cli/plugins.py b/hermes_cli/plugins.py index 9028e28530..3188526912 100644 --- a/hermes_cli/plugins.py +++ b/hermes_cli/plugins.py @@ -4262,18 +4262,21 @@ class PluginManager: # first process sees plugin backends (tracking #64177). self._refresh_secret_sources_after_discovery() if force: - # config.yaml shell hooks live in ``_hooks`` but are - # config-owned, not plugin-owned — the ledger-driven - # unload() above wiped them and cannot restore them. - # Re-register so force-reload is symmetric (#60036; - # tracking #64178 — salvaged from PR #64188). - self._re_register_shell_hooks_after_force() + # config.yaml shell hooks and outbound webhooks live in + # ``_hooks`` but are config-owned, not plugin-owned — + # the ledger-driven unload() above wiped them and + # cannot restore them. Re-register so force-reload is + # symmetric (#60036; tracking #64178 — salvaged from + # PR #64188; outbound webhooks added per #92682 review). + self._re_register_config_hooks_after_force() except BaseException: self._discovered = False raise - def _re_register_shell_hooks_after_force(self) -> None: - """Restore config.yaml shell hooks wiped by force-clear of ``_hooks``.""" + def _re_register_config_hooks_after_force(self) -> None: + """Restore config.yaml shell hooks/outbound webhooks wiped by + force-clear of ``_hooks``. Each re-register call is independently + guarded so one failing does not skip the other.""" try: from agent.shell_hooks import re_register_config_hooks @@ -4281,6 +4284,14 @@ class PluginManager: except Exception as exc: # Import cycle / missing module must not abort force reload. logger.debug("force-reload shell-hook re-register skipped: %s", exc) + try: + from agent.outbound_webhooks import ( + re_register_config_hooks as re_register_outbound_webhooks, + ) + + re_register_outbound_webhooks() + except Exception as exc: + logger.debug("force-reload outbound-webhook re-register skipped: %s", exc) def _refresh_secret_sources_after_discovery(self) -> None: """If any plugin secret source is enabled, reset cache and re-apply. diff --git a/tests/agent/test_outbound_webhooks.py b/tests/agent/test_outbound_webhooks.py index 29e05a4cdb..8b02970e50 100644 --- a/tests/agent/test_outbound_webhooks.py +++ b/tests/agent/test_outbound_webhooks.py @@ -345,6 +345,46 @@ class TestRegistration: assert len(http_server.captured) == 1 +class TestForceReloadHomeScoping: + """Force-reloading one profile's plugin manager must restore that + profile's own outbound webhook and leave it firing exactly once — + the mirror of the shell-hook force-reload symmetry fix (#92682 + review: outbound webhooks were the "same symptom class... after a + supported lifecycle transition instead of initial startup"). + """ + + def test_force_reload_restores_webhook_and_fires_once( + self, monkeypatch, http_server, + ): + from hermes_cli import plugins + + cfg = _cfg({"url": _url(http_server), "events": ["on_session_end"]}) + monkeypatch.setattr("hermes_cli.config.load_config", lambda: cfg) + + monkeypatch.setenv("HERMES_HOME", "/tmp/profile-b-webhook") + mgr_b = plugins.PluginManager() + plugins._plugin_manager = mgr_b + outbound_webhooks.register_from_config(cfg) + assert len(mgr_b._hooks.get("on_session_end", [])) == 1 + + # Force-reload: unload() wipes _hooks (config-owned webhook + # callbacks included, same as the ledger-driven plugin sweep), so + # without the fix the idempotence key alone would survive and a + # later register_from_config() call would see it and skip + # re-wiring — leaving the webhook silently inert. + mgr_b.unload() + assert mgr_b._hooks.get("on_session_end", []) == [] + + outbound_webhooks.re_register_config_hooks() + assert len(mgr_b._hooks.get("on_session_end", [])) == 1 + + plugins.get_plugin_manager().invoke_hook( + "on_session_end", session_id="s1", + ) + assert outbound_webhooks.flush() + assert len(http_server.captured) == 1 + + # ── E2E delivery against a real HTTP server ────────────────────────────── diff --git a/tests/hermes_cli/test_plugins.py b/tests/hermes_cli/test_plugins.py index 5ae4d5aee1..d44abafb4f 100644 --- a/tests/hermes_cli/test_plugins.py +++ b/tests/hermes_cli/test_plugins.py @@ -933,6 +933,13 @@ class TestDeliveryParity: class TestForceReloadSymmetry: """Force rediscovery restores non-plugin state it wiped (#64178).""" + @pytest.fixture(autouse=True) + def _cleanup_shell_hook_registry(self): + yield + import agent.shell_hooks as shell_hooks_mod + + shell_hooks_mod.reset_for_tests() + def test_force_reload_re_registers_shell_hooks(self, monkeypatch): """config.yaml shell hooks are re-wired after force=True (#60036).""" calls = [] @@ -992,6 +999,7 @@ class TestForceReloadSymmetry: def test_re_register_config_hooks_clears_idempotence_set(self, monkeypatch): import agent.shell_hooks as shell_hooks_mod + from hermes_constants import get_hermes_home recorded = {} monkeypatch.setattr( @@ -1002,8 +1010,9 @@ class TestForceReloadSymmetry: monkeypatch.setattr( "hermes_cli.config.load_config", lambda: {"hooks": {}} ) + home_key = str(get_hermes_home().expanduser().resolve()) with shell_hooks_mod._registered_lock: - shell_hooks_mod._registered.add(("post_llm_call", None, "echo hi")) + shell_hooks_mod._registered.add((home_key, "post_llm_call", None, "echo hi")) shell_hooks_mod.re_register_config_hooks() @@ -1219,6 +1228,50 @@ class TestForceReloadSymmetry: assert _PRE_TOOL_CALL_TIMEOUT_BLOCK_MESSAGE in result hold.set() + def test_force_reload_of_one_profile_does_not_orphan_another(self, monkeypatch): + """Real two-manager regression: force-reloading profile A's plugin + manager must leave profile B's shell hook registered exactly once — + not duplicated, not dropped (#92682 review). + """ + import hermes_cli.plugins as plugins_mod + import agent.shell_hooks as shell_hooks_mod + + cfg = {"hooks": {"on_session_start": [{"command": "/bin/true"}]}} + monkeypatch.setenv("HERMES_ACCEPT_HOOKS", "1") + monkeypatch.setattr("hermes_cli.config.load_config", lambda: cfg) + monkeypatch.setattr( + PluginManager, "_discover_and_load_inner", lambda self_inner: None, + ) + + monkeypatch.setenv("HERMES_HOME", "/tmp/profile-a") + mgr_a = PluginManager() + plugins_mod._plugin_manager = mgr_a + shell_hooks_mod.register_from_config(cfg, accept_hooks=True) + + monkeypatch.setenv("HERMES_HOME", "/tmp/profile-b") + mgr_b = PluginManager() + plugins_mod._plugin_manager = mgr_b + shell_hooks_mod.register_from_config(cfg, accept_hooks=True) + + assert len(mgr_a._hooks.get("on_session_start", [])) == 1 + assert len(mgr_b._hooks.get("on_session_start", [])) == 1 + + # Force-reload A. Its own manager's hook is wiped and restored; + # B's manager (and idempotence key) must be untouched. + mgr_a.discover_and_load(force=True) + + assert len(mgr_a._hooks.get("on_session_start", [])) == 1 + assert len(mgr_b._hooks.get("on_session_start", [])) == 1 + + # B's later adapter reconnect re-runs register_from_config(); its + # idempotence key must still be intact, so this must be a no-op + # rather than appending a second callback to B's live manager. + monkeypatch.setenv("HERMES_HOME", "/tmp/profile-b") + second = shell_hooks_mod.register_from_config(cfg, accept_hooks=True) + + assert second == [] + assert len(mgr_b._hooks.get("on_session_start", [])) == 1 + class TestPreToolCallBlocking: """Tests for the pre_tool_call block directive helper."""