From 4eb56ebec61ebb0bcf0fdce178ebe3b0bce3f192 Mon Sep 17 00:00:00 2001 From: Molecule AI Triage Operator Date: Fri, 17 Apr 2026 05:22:00 +0000 Subject: [PATCH] fix(plugins_registry): deduplicate handlers in _deep_merge_hooks() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Unconditional list.extend() on repeated plugin install caused every hook handler to be appended on each reinstall, leading to 3-4x duplicate firings per event (PreToolUse, PostToolUse, Stop, etc.). Fix: before appending each incoming handler, compute a fingerprint of (matcher, frozenset-of-commands). Skip append if the fingerprint is already present in the merged list. First-time installs are unaffected — new handlers still land correctly. Adds 7 unit tests covering: first install, double install, triple install, different-matcher co-existence, different-command co-existence, existing user hook preservation, and top-level key merge semantics. Closes #566 Co-Authored-By: Claude Sonnet 4.6 --- .../plugins_registry/builtins.py | 24 ++++- .../tests/test_plugins_builtins.py | 88 +++++++++++++++++++ 2 files changed, 108 insertions(+), 4 deletions(-) diff --git a/workspace-template/plugins_registry/builtins.py b/workspace-template/plugins_registry/builtins.py index 634d5fb1..9816ee85 100644 --- a/workspace-template/plugins_registry/builtins.py +++ b/workspace-template/plugins_registry/builtins.py @@ -319,9 +319,25 @@ def _deep_merge_hooks(existing: dict, fragment: dict) -> dict: out.setdefault("hooks", {}) for event, handlers in fragment.get("hooks", {}).items(): out["hooks"].setdefault(event, []) - out["hooks"][event].extend(handlers) - for key, val in fragment.items(): - if key == "hooks": + # Build a set of already-present handler fingerprints so that + # re-installing the same plugin fragment does not append duplicates. + # Key: (matcher, frozenset-of-commands) — same logic the issue spec + # describes. Two handlers are considered identical when they watch the + # same matcher pattern and invoke exactly the same set of commands. + seen: set[tuple[str, frozenset[str]]] = { + (h.get("matcher", ""), frozenset(c.get("command", "") for c in h.get("hooks", []))) + for h in out["hooks"][event] + } + for handler in handlers: + hkey = ( + handler.get("matcher", ""), + frozenset(c.get("command", "") for c in handler.get("hooks", [])), + ) + if hkey not in seen: + seen.add(hkey) + out["hooks"][event].append(handler) + for top_key, val in fragment.items(): + if top_key == "hooks": continue - out.setdefault(key, val) + out.setdefault(top_key, val) return out diff --git a/workspace-template/tests/test_plugins_builtins.py b/workspace-template/tests/test_plugins_builtins.py index f34e6d4a..31d14cae 100644 --- a/workspace-template/tests/test_plugins_builtins.py +++ b/workspace-template/tests/test_plugins_builtins.py @@ -7,6 +7,7 @@ Covers: - Empty rules directory doesn't write an empty block - README.md / CHANGELOG.md are skipped at the root (not treated as fragments) - Uninstall is safe on a plugin that was never installed + - _deep_merge_hooks deduplication (issue #566) """ from __future__ import annotations @@ -393,3 +394,90 @@ async def test_setup_sh_absent_no_warning(tmp_path: Path): result = await AgentskillsAdaptor("p", "claude_code").install(_make_ctx(configs, plugin)) assert result.warnings == [] + + +# --------------------------------------------------------------------------- +# _deep_merge_hooks deduplication — issue #566 +# --------------------------------------------------------------------------- + +from plugins_registry.builtins import _deep_merge_hooks # noqa: E402 + + +def _make_fragment(event: str, matcher: str, command: str) -> dict: + """Build a minimal settings-fragment dict for one hook handler.""" + return { + "hooks": { + event: [ + { + "matcher": matcher, + "hooks": [{"type": "command", "command": command}], + } + ] + } + } + + +def test_deep_merge_hooks_first_install_adds_handler(): + """Merging into an empty dict adds the handler exactly once.""" + result = _deep_merge_hooks({}, _make_fragment("PreToolUse", "Bash", "/hooks/lint.sh")) + handlers = result["hooks"]["PreToolUse"] + assert len(handlers) == 1 + assert handlers[0]["matcher"] == "Bash" + + +def test_deep_merge_hooks_dedup_on_reinstall(): + """Merging the same fragment twice must not duplicate the handler.""" + fragment = _make_fragment("PreToolUse", "Bash", "/hooks/lint.sh") + once = _deep_merge_hooks({}, fragment) + twice = _deep_merge_hooks(once, fragment) + assert len(twice["hooks"]["PreToolUse"]) == 1, ( + "Re-installing the same fragment must not append a duplicate handler" + ) + + +def test_deep_merge_hooks_dedup_three_reinstalls(): + """Issue #566 reported 3–4× duplication — verify three installs still yield one entry.""" + fragment = _make_fragment("PostToolUse", "Write", "/hooks/format.sh") + state = {} + for _ in range(3): + state = _deep_merge_hooks(state, fragment) + assert len(state["hooks"]["PostToolUse"]) == 1 + + +def test_deep_merge_hooks_different_matchers_both_kept(): + """Two handlers with different matchers must co-exist — dedup must not over-filter.""" + state = _deep_merge_hooks({}, _make_fragment("PreToolUse", "Bash", "/hooks/lint.sh")) + state = _deep_merge_hooks(state, _make_fragment("PreToolUse", "Edit", "/hooks/lint.sh")) + assert len(state["hooks"]["PreToolUse"]) == 2 + + +def test_deep_merge_hooks_different_commands_both_kept(): + """Same matcher but different commands → both handlers must be kept.""" + state = _deep_merge_hooks({}, _make_fragment("PreToolUse", "Bash", "/hooks/lint.sh")) + state = _deep_merge_hooks(state, _make_fragment("PreToolUse", "Bash", "/hooks/security.sh")) + assert len(state["hooks"]["PreToolUse"]) == 2 + + +def test_deep_merge_hooks_existing_user_hooks_preserved(): + """Existing hooks in settings.json that don't match the fragment must survive.""" + existing = { + "hooks": { + "PreToolUse": [ + {"matcher": "Bash", "hooks": [{"type": "command", "command": "/user/custom.sh"}]} + ] + } + } + fragment = _make_fragment("PreToolUse", "Edit", "/hooks/lint.sh") + result = _deep_merge_hooks(existing, fragment) + matchers = {h["matcher"] for h in result["hooks"]["PreToolUse"]} + assert matchers == {"Bash", "Edit"} + + +def test_deep_merge_hooks_top_level_keys_merged(): + """Non-hook top-level keys in the fragment are merged into the output.""" + existing = {"someKey": "old"} + fragment = {"someKey": "new", "anotherKey": "value", "hooks": {}} + result = _deep_merge_hooks(existing, fragment) + # setdefault semantics: existing keys win, new keys are added + assert result["someKey"] == "old" + assert result["anotherKey"] == "value"