fix(plugins_registry): deduplicate handlers in _deep_merge_hooks()
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 <noreply@anthropic.com>
This commit is contained in:
parent
b9dbfda68b
commit
4eb56ebec6
@ -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
|
||||
|
||||
@ -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"
|
||||
|
||||
Loading…
Reference in New Issue
Block a user