fix(hooks): make apm install idempotent for hook entries#709
fix(hooks): make apm install idempotent for hook entries#709srid wants to merge 3 commits intomicrosoft:mainfrom
Conversation
Re-running `apm install` appended a duplicate hook entry per package on every run because `_integrate_merged_hooks` extended the per-event list unconditionally. Upsert by `_apm_source` before appending — the cleanup code path already treats that marker as the ownership key; the install path now matches. Fixes microsoft#708.
Adds two regression tests for microsoft#708 under TestClaudeIntegration: * test_reinstall_is_idempotent — three consecutive integrate_package_hooks_claude() calls produce a byte-identical settings.json with a single Stop entry. * test_reinstall_heals_preexisting_duplicates — a settings.json seeded with three duplicate entries for one _apm_source plus an unrelated user entry collapses to one APM entry and preserves the user entry. Both tests fail against the pre-fix commit and pass against the upsert-before-extend implementation in _integrate_merged_hooks, so they lock in the behaviour for all merge-style hook targets (Claude, Cursor, Codex) that share the same code path.
srid
requested review from
danielmeppiel and
sergio-sisternes-epam
as code owners
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
This PR fixes duplicate hook entries when re-running apm install by making merged-hook integration idempotent via _apm_source-based upsert semantics (regression fix for #708).
Changes:
- Add unit tests covering reinstall idempotency and healing of preexisting duplicates in Claude settings.
- Update
_integrate_merged_hooksto remove previously integrated entries for the same package before appending new ones.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| tests/unit/integration/test_hook_integrator.py | Adds regression tests ensuring reinstall doesn’t duplicate hooks and collapses legacy duplicates. |
| src/apm_cli/integration/hook_integrator.py | Implements _apm_source-based “upsert” behavior during merged-hook integration to avoid duplicates. |
**Bare `apm install` duplicates hook entries in `.claude/settings.json` on every re-run**, so kolu's `apm` recipe papers over it by wiping `.claude/` before each install and `apm-update` does `rm -f settings.json` for the same reason. The real bug is [microsoft/apm#708](microsoft/apm#708) — the merge integrator `.extend()`s the per-event hook list unconditionally instead of upserting by `_apm_source`. Pin `apm_cmd` to microsoft/apm#709, which filters existing entries owned by the package before appending fresh ones. Fix is eight lines in `hook_integrator.py` plus two regression tests covering idempotent re-integration and healing of pre-existing duplicates. _Both workarounds in `ai.just` — the wholesale `.claude/` wipe and the settings.json `rm -f` — are dropped; `apm install` is now safe to run over a live tree._ The workflow instructions also pick up a small fix: `/do` now knows to invoke the `/test` and `/ci` skills for the test and CI pipeline steps (previously only `just check` / `just fmt` were documented, leaving those steps without a verified execution path). > Revert `apm_cmd` back to `microsoft/apm` once #708 lands upstream. Branch comment in `ai.just` flags this.
Review feedback on microsoft#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.
|
Thanks for the review @copilot — all three addressed in ac7a840: Multi-hook-file regression ( Multi-file test ( Defensive |
Description
Re-running
apm installappended a duplicate hook entry per package on every run because_integrate_merged_hooksextended the per-event list unconditionally.Upsert by
_apm_sourcebefore appending — the cleanup code path already treats that marker as the ownership key; the install path now matches.Fixes #708
Type of change
Testing