Skip to content

Commit ac7a840

Browse files
committed
fix(hooks): only strip prior-owned entries once per event per run
Review feedback on #709: the per-hook-file loop was filtering owned entries on every iteration, so a package contributing to the same event from multiple hook files would lose earlier files' freshly-appended entries when the next file's iteration re-ran the filter. Track cleared `(event_name)` in a set and skip filtering on subsequent iterations. Add a regression test with two hook files targeting `Stop`, verifying both entries survive install and repeated re-installs. Also add a defensive `parent.mkdir(parents=True, exist_ok=True)` in the heal test before seeding settings.json, per review feedback.
1 parent d332581 commit ac7a840

2 files changed

Lines changed: 62 additions & 4 deletions

File tree

src/apm_cli/integration/hook_integrator.py

Lines changed: 15 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -491,6 +491,11 @@ def _integrate_merged_hooks(
491491
hooks_integrated = 0
492492
scripts_copied = 0
493493
target_paths: List[Path] = []
494+
# Events whose prior-owned entries have already been cleared on
495+
# this install run. Packages can contribute to the same event
496+
# from multiple hook files — we must only strip once so earlier
497+
# files' fresh entries aren't wiped by later iterations.
498+
cleared_events: set = set()
494499

495500
# Read existing JSON config
496501
json_path = target_dir / config.config_filename
@@ -535,10 +540,16 @@ def _integrate_merged_hooks(
535540
# package before appending fresh ones. Without this, every
536541
# `apm install` re-run duplicates the package's hooks
537542
# because `.extend()` is unconditional. See microsoft/apm#708.
538-
json_config["hooks"][event_name] = [
539-
e for e in json_config["hooks"][event_name]
540-
if not (isinstance(e, dict) and e.get("_apm_source") == package_name)
541-
]
543+
# Only strip once per event per install run — a package
544+
# with multiple hook files targeting the same event
545+
# contributes each file's entries in turn, and stripping
546+
# on every iteration would erase earlier files' work.
547+
if event_name not in cleared_events:
548+
json_config["hooks"][event_name] = [
549+
e for e in json_config["hooks"][event_name]
550+
if not (isinstance(e, dict) and e.get("_apm_source") == package_name)
551+
]
552+
cleared_events.add(event_name)
542553
json_config["hooks"][event_name].extend(entries)
543554

544555
hooks_integrated += 1

tests/unit/integration/test_hook_integrator.py

Lines changed: 47 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -646,6 +646,7 @@ def test_reinstall_heals_preexisting_duplicates(self, temp_project):
646646
"_apm_source": "ralph-loop",
647647
}
648648
settings_path = temp_project / ".claude" / "settings.json"
649+
settings_path.parent.mkdir(parents=True, exist_ok=True)
649650
settings_path.write_text(json.dumps({
650651
"hooks": {
651652
"Stop": [
@@ -674,6 +675,52 @@ def test_reinstall_heals_preexisting_duplicates(self, temp_project):
674675
assert len(user_entries) == 1
675676
assert user_entries[0]["hooks"][0]["command"] == "user-owned"
676677

678+
def test_reinstall_preserves_multiple_hook_files_same_event(self, temp_project):
679+
"""A package can contribute to one event from several hook files.
680+
681+
The idempotent-upsert must only strip prior-owned entries once per
682+
event per install run — otherwise the second hook file's iteration
683+
wipes the first file's fresh entries before extending. Also verifies
684+
the combined output is stable across re-runs.
685+
"""
686+
pkg_dir = temp_project / "multi-stop-pkg"
687+
hooks_dir = pkg_dir / "hooks"
688+
hooks_dir.mkdir(parents=True, exist_ok=True)
689+
690+
def stop_hook(script: str) -> dict:
691+
return {"hooks": {"Stop": [{"hooks": [
692+
{"type": "command", "command": f"${{CLAUDE_PLUGIN_ROOT}}/hooks/{script}"}
693+
]}]}}
694+
695+
(hooks_dir / "hooks-a.json").write_text(json.dumps(stop_hook("stop-a.sh")))
696+
(hooks_dir / "hooks-b.json").write_text(json.dumps(stop_hook("stop-b.sh")))
697+
(hooks_dir / "stop-a.sh").write_text("#!/bin/bash\nexit 0")
698+
(hooks_dir / "stop-b.sh").write_text("#!/bin/bash\nexit 0")
699+
700+
pkg_info = _make_package_info(pkg_dir, "multi-stop-pkg")
701+
integrator = HookIntegrator()
702+
integrator.integrate_package_hooks_claude(pkg_info, temp_project)
703+
704+
settings_path = temp_project / ".claude" / "settings.json"
705+
first = settings_path.read_text()
706+
707+
def extract_commands(text: str) -> set:
708+
stop = json.loads(text)["hooks"]["Stop"]
709+
assert all(e["_apm_source"] == "multi-stop-pkg" for e in stop)
710+
return {h["command"] for entry in stop for h in entry["hooks"]}
711+
712+
assert extract_commands(first) == {
713+
".claude/hooks/multi-stop-pkg/hooks/stop-a.sh",
714+
".claude/hooks/multi-stop-pkg/hooks/stop-b.sh",
715+
}
716+
717+
# Re-run twice more — both entries must survive and the file must
718+
# be byte-identical each time (idempotent across hook files too).
719+
for _ in range(2):
720+
integrator.integrate_package_hooks_claude(pkg_info, temp_project)
721+
722+
assert settings_path.read_text() == first
723+
677724
def test_no_hooks_returns_empty_result(self, temp_project):
678725
"""Test Claude integration with no hook files returns empty result."""
679726
pkg_dir = temp_project / "pkg"

0 commit comments

Comments
 (0)