Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 19 additions & 0 deletions src/apm_cli/integration/hook_integrator.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down
126 changes: 126 additions & 0 deletions tests/unit/integration/test_hook_integrator.py
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down