diff --git a/src/apm_cli/integration/hook_integrator.py b/src/apm_cli/integration/hook_integrator.py index 7b200a22..8f0ca235 100644 --- a/src/apm_cli/integration/hook_integrator.py +++ b/src/apm_cli/integration/hook_integrator.py @@ -491,6 +491,11 @@ def _integrate_merged_hooks( hooks_integrated = 0 scripts_copied = 0 target_paths: List[Path] = [] + # Events whose prior-owned entries have already been cleared on + # this install run. Packages can contribute to the same event + # from multiple hook files — we must only strip once so earlier + # files' fresh entries aren't wiped by later iterations. + cleared_events: set = set() # Read existing JSON config json_path = target_dir / config.config_filename @@ -531,6 +536,20 @@ def _integrate_merged_hooks( if isinstance(entry, dict): entry["_apm_source"] = package_name + # Idempotent upsert: drop any prior entries owned by this + # package before appending fresh ones. Without this, every + # `apm install` re-run duplicates the package's hooks + # because `.extend()` is unconditional. See microsoft/apm#708. + # Only strip once per event per install run — a package + # with multiple hook files targeting the same event + # contributes each file's entries in turn, and stripping + # on every iteration would erase earlier files' work. + if event_name not in cleared_events: + json_config["hooks"][event_name] = [ + e for e in json_config["hooks"][event_name] + if not (isinstance(e, dict) and e.get("_apm_source") == package_name) + ] + cleared_events.add(event_name) json_config["hooks"][event_name].extend(entries) hooks_integrated += 1 diff --git a/tests/unit/integration/test_hook_integrator.py b/tests/unit/integration/test_hook_integrator.py index 1708c15f..8a7a63ac 100644 --- a/tests/unit/integration/test_hook_integrator.py +++ b/tests/unit/integration/test_hook_integrator.py @@ -595,6 +595,132 @@ def test_additive_merge_same_event(self, temp_project): # Both Stop hooks should be present (additive) assert len(settings["hooks"]["Stop"]) == 2 + def test_reinstall_is_idempotent(self, temp_project): + """Re-running integration for the same package must not duplicate its entries. + + Regression test for microsoft/apm#708: before the fix, each subsequent + `apm install` appended another copy of every hook owned by an + already-integrated package. + """ + # `_get_package_name` derives the package name from install_path.name, + # so the directory name is what ends up as `_apm_source`. + pkg_dir = temp_project / "ralph-loop" + hooks_dir = pkg_dir / "hooks" + hooks_dir.mkdir(parents=True, exist_ok=True) + (hooks_dir / "hooks.json").write_text(json.dumps(RALPH_LOOP_HOOKS_JSON)) + (hooks_dir / "stop-hook.sh").write_text("#!/bin/bash\nexit 0") + pkg_info = _make_package_info(pkg_dir, "ralph-loop") + integrator = HookIntegrator() + + integrator.integrate_package_hooks_claude(pkg_info, temp_project) + first = (temp_project / ".claude" / "settings.json").read_text() + + # Re-run twice more — the file should be byte-identical each time. + for _ in range(2): + integrator.integrate_package_hooks_claude(pkg_info, temp_project) + + settings = json.loads((temp_project / ".claude" / "settings.json").read_text()) + assert len(settings["hooks"]["Stop"]) == 1 + assert settings["hooks"]["Stop"][0]["_apm_source"] == "ralph-loop" + assert (temp_project / ".claude" / "settings.json").read_text() == first + + def test_reinstall_heals_preexisting_duplicates(self, temp_project): + """Existing duplicate entries for a package get collapsed on re-integration. + + Upgrades from a pre-#708 apm can leave a settings.json with multiple + identical `_apm_source` entries; the next install should clean them up. + """ + pkg_dir = temp_project / "ralph-loop" + hooks_dir = pkg_dir / "hooks" + hooks_dir.mkdir(parents=True, exist_ok=True) + (hooks_dir / "hooks.json").write_text(json.dumps(RALPH_LOOP_HOOKS_JSON)) + (hooks_dir / "stop-hook.sh").write_text("#!/bin/bash\nexit 0") + pkg_info = _make_package_info(pkg_dir, "ralph-loop") + integrator = HookIntegrator() + + # Seed a settings.json with three duplicate ralph-loop Stop entries + # plus one unrelated user hook that must survive. + dup_entry = { + "matcher": "", + "hooks": [{"type": "command", "command": "stale"}], + "_apm_source": "ralph-loop", + } + settings_path = temp_project / ".claude" / "settings.json" + settings_path.parent.mkdir(parents=True, exist_ok=True) + settings_path.write_text(json.dumps({ + "hooks": { + "Stop": [ + {"hooks": [{"type": "command", "command": "user-owned"}]}, + dup_entry, + dup_entry, + dup_entry, + ] + } + })) + + integrator.integrate_package_hooks_claude(pkg_info, temp_project) + + settings = json.loads(settings_path.read_text()) + apm_entries = [ + e for e in settings["hooks"]["Stop"] + if isinstance(e, dict) and e.get("_apm_source") == "ralph-loop" + ] + user_entries = [ + e for e in settings["hooks"]["Stop"] + if not (isinstance(e, dict) and "_apm_source" in e) + ] + assert len(apm_entries) == 1 + # Stale command replaced with the freshly rewritten one. + assert "stop-hook.sh" in apm_entries[0]["hooks"][0]["command"] + assert len(user_entries) == 1 + assert user_entries[0]["hooks"][0]["command"] == "user-owned" + + def test_reinstall_preserves_multiple_hook_files_same_event(self, temp_project): + """A package can contribute to one event from several hook files. + + The idempotent-upsert must only strip prior-owned entries once per + event per install run — otherwise the second hook file's iteration + wipes the first file's fresh entries before extending. Also verifies + the combined output is stable across re-runs. + """ + pkg_dir = temp_project / "multi-stop-pkg" + hooks_dir = pkg_dir / "hooks" + hooks_dir.mkdir(parents=True, exist_ok=True) + + def stop_hook(script: str) -> dict: + return {"hooks": {"Stop": [{"hooks": [ + {"type": "command", "command": f"${{CLAUDE_PLUGIN_ROOT}}/hooks/{script}"} + ]}]}} + + (hooks_dir / "hooks-a.json").write_text(json.dumps(stop_hook("stop-a.sh"))) + (hooks_dir / "hooks-b.json").write_text(json.dumps(stop_hook("stop-b.sh"))) + (hooks_dir / "stop-a.sh").write_text("#!/bin/bash\nexit 0") + (hooks_dir / "stop-b.sh").write_text("#!/bin/bash\nexit 0") + + pkg_info = _make_package_info(pkg_dir, "multi-stop-pkg") + integrator = HookIntegrator() + integrator.integrate_package_hooks_claude(pkg_info, temp_project) + + settings_path = temp_project / ".claude" / "settings.json" + first = settings_path.read_text() + + def extract_commands(text: str) -> set: + stop = json.loads(text)["hooks"]["Stop"] + assert all(e["_apm_source"] == "multi-stop-pkg" for e in stop) + return {h["command"] for entry in stop for h in entry["hooks"]} + + assert extract_commands(first) == { + ".claude/hooks/multi-stop-pkg/hooks/stop-a.sh", + ".claude/hooks/multi-stop-pkg/hooks/stop-b.sh", + } + + # Re-run twice more — both entries must survive and the file must + # be byte-identical each time (idempotent across hook files too). + for _ in range(2): + integrator.integrate_package_hooks_claude(pkg_info, temp_project) + + assert settings_path.read_text() == first + def test_no_hooks_returns_empty_result(self, temp_project): """Test Claude integration with no hook files returns empty result.""" pkg_dir = temp_project / "pkg"