fix(hooks): stabilize root-local hook source ids#1330
Conversation
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Stabilizes root-local hook source IDs so re-running hook installation/sync remains idempotent across different checkout directory names, and adds targeted healing to remove stale same-content root entries in merged hook targets.
Changes:
- Derive root-local hook “source” from
apm.ymlname (sanitized) and namespace merged-target ownership as_local/<name>. - Heal stale merged-hook entries for root-local installs by removing prior same-content entries with outdated
_apm_sourcevalues while preserving dependency- and user-owned entries. - Add regression tests for Claude/Codex merged-hook stale-source healing and adjust an integration test cleanup block.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| src/apm_cli/integration/hook_integrator.py | Implements stable root-local source markers and stale-entry healing in merged hook targets |
| tests/unit/integration/test_hook_integrator.py | Adds unit coverage for root-local stable naming + stale-source healing across merged targets |
| tests/integration/test_registry.py | Replaces try/except cleanup with contextlib.suppress (no behavior change intended) |
| CHANGELOG.md | Documents the root hook source drift + stale-entry healing fix |
| def _dependency_hook_sources(project_root: Path) -> set[str]: | ||
| """Return source markers that correspond to installed dependency dirs.""" | ||
| apm_modules = project_root / "apm_modules" | ||
| if not apm_modules.is_dir(): | ||
| return set() | ||
| sources: set[str] = set() | ||
| for path in apm_modules.rglob("*"): | ||
| if not path.is_dir(): | ||
| continue | ||
| if ( | ||
| (path / "hooks").is_dir() | ||
| or (path / ".apm" / "hooks").is_dir() | ||
| or (path / "apm.yml").is_file() | ||
| or (path / "SKILL.md").is_file() | ||
| ): | ||
| sources.add(path.name) | ||
| return sources |
| if not heal_stale_root_source or not source or source in dependency_sources: | ||
| return False | ||
| return self._hook_entry_content_key(entry) in fresh_content_keys |
APM Review Panel:
|
| Persona | B | R | N | Takeaway |
|---|---|---|---|---|
| Python Architect | 0 | 2 | 3 | Solid fix; one recommended refactor (RemovalContext dataclass) and one I/O scope concern (rglob). No blocking issues. |
| CLI Logging Expert | 0 | 2 | 1 | No new user-visible output (correct). Two debug-log gaps matter for verbose/agent mode diagnostics. |
| DevX UX Expert | 0 | 2 | 1 | Idempotency contract well-addressed. Two recommended items: silent healing visibility and bare _local fallback edge case. |
| Supply Chain Security Expert | 0 | 0 | 2 | No blocking security issues. Two nits: symlink traversal in rglob, and a comment suggestion on _safe_source_name. |
| OSS Growth Hacker | 0 | 0 | 1 | Strong trust-signal fix. CHANGELOG wording leads with implementation detail rather than user outcome. |
| Doc Writer | 0 | 1 | 1 | CHANGELOG entry leaks internal symbols. No docs drift found elsewhere. |
| Test Coverage Expert | 0 | 2 | 0 | Six targeted scenario tests defend the core user promise. Two gaps: no integration-tier test; private helper edge cases uncovered. |
B = blocking-severity findings, R = recommended, N = nits.
Counts are signal strength, not gates. The maintainer ships.
Top 5 follow-ups
- [Doc Writer] Rewrite CHANGELOG entry to user-observable outcome before merge. -- Current entry exposes
_apm_sourceand 'heal stale same-content merged hook entries' -- internal vocabulary obscuring the user benefit. doc-writer and oss-growth-hacker converge on a user-first rewrite. CHANGELOG is the public release surface; this is a one-line fix with zero risk. - [Test Coverage Expert] Add integration-tier test: seed stale
.claude/settings.json, runapm installvia subprocess, assert stale entry replaced by_local/{name}. -- All six new tests call HookIntegrator directly. The full user promise -- thatapm installheals stale entries -- is untested at the CLI-entry-point tier. Evidence: outcome=missing attests/integration/. - [CLI Logging Expert] Add
_log.debug()calls for silent OSError/YAMLError swallows and for the stale-entry healing count. -- Unexpected_localfallbacks and healed-entry counts are invisible in verbose mode. Two targeted debug log lines would make agent-assisted debugging meaningfully more informative. - [Python Architect] Bound
_dependency_hook_sourcesto depth-1/2 iteration instead ofrglob('*')across the fullapm_modulestree. --rglob('*')is O(total files in all deps) and follows symlinks. A depth-bounded walk also closes the supply-chain-security-expert's symlink-cycle concern in one change. - [Test Coverage Expert] Add unit-tier coverage for private helper edge cases:
_is_root_local_packagewith None path,_get_root_local_package_namewithname: nullorname: ''in apm.yml. -- The bare_localfallback produces a source marker that differs from_local/{name}, meaning a future add ofname:to apm.yml triggers a one-time re-heal. No test currently asserts this fallback path.
Architecture
classDiagram
direction LR
class BaseIntegrator {
<<AbstractBase>>
+check_collision()
+is_content_identical_to_source()
+find_hook_files()
}
class HookIntegrator {
<<ConcreteIntegrator>>
+integrate_package_hooks()
+integrate_package_hooks_merged_target()
+_get_package_name()
+_get_hook_source_marker()
+_is_root_local_package()
+_safe_source_name()
+_get_root_local_package_name()
+_hook_entry_content_key()
+_dependency_hook_sources()
+_should_remove_prior_merged_entry()
}
class HookIntegrationResult {
<<ValueObject>>
+hooks_integrated int
}
class IntegrationResult {
<<ValueObject>>
+files_integrated int
+files_updated int
+files_skipped int
}
class _MergeHookConfig {
<<ValueObject>>
+config_filename str
+target_key str
}
BaseIntegrator <|-- HookIntegrator : extends
IntegrationResult <|-- HookIntegrationResult : extends
HookIntegrator ..> _MergeHookConfig : reads
HookIntegrator ..> HookIntegrationResult : produces
class HookIntegrator:::touched
classDef touched fill:#fff3b0,stroke:#d47600
flowchart TD
A([apm install -- root package]) --> B[integrate_package_hooks_merged_target]
B --> C{_is_root_local_package?}
C -- No --> D[source_marker = install_path.name]
C -- Yes --> E[_get_root_local_package_name]
E --> E1[read apm.yml via load_yaml]
E1 -- success --> E2[_safe_source_name from manifest name]
E1 -- OSError/YAMLError --> E3[fallback to package.name or _local]
E2 --> F[source_marker = _local/name]
E3 --> F
D --> G[heal_stale_root_source = False]
F --> H[heal_stale_root_source = True]
H --> I[_dependency_hook_sources: scan apm_modules/]
G --> J[dependency_sources = empty set]
I --> J2[dependency_sources = set of dep dir names]
J --> K[per hook_file loop]
J2 --> K
K --> L[build fresh_content_keys via _hook_entry_content_key]
L --> M{remove_current_source OR heal_stale_root_source?}
M -- Yes --> N[_should_remove_prior_merged_entry filter]
N --> O{owns current source?}
O -- Yes --> REMOVE[remove entry]
O -- No --> P{heal AND not dep source AND content matches?}
P -- Yes --> REMOVE
P -- No --> KEEP[keep entry]
M -- No --> KEEP
KEEP --> Q[extend with new entries]
REMOVE --> Q
Q --> R[write merged JSON config file]
Recommendation
Ship after the CHANGELOG entry is rewritten to user-observable language (follow-up #1 above -- one line, zero risk, should land in this PR). All other follow-ups are post-merge: the integration-tier test gap is the highest-signal item to track on the milestone. The fix is correct, safe, and well-tested at unit tier; blocking it for the integration test would penalize a solid contributor fix for a gap that predates this PR.
Full per-persona findings
Python Architect
-
[recommended]
_should_remove_prior_merged_entryhas 5 keyword-only parameters; extract a RemovalContext frozen dataclass atsrc/apm_cli/integration/hook_integrator.py:603
The method signature is repeated verbatim at two call-sites. A frozen_RemovalContextdataclass encapsulates call-state once, aligns with the BaseIntegrator value-object pattern, and makes future flag additions non-invasive.
Suggested:@dataclass(frozen=True) class _RemovalContext: source_marker: str; fresh_content_keys: set[str]; heal_stale_root_source: bool; dependency_sources: set[str]; remove_current_source: bool -
[recommended]
_dependency_hook_sourcesrglobs the entireapm_modulestree on every root-package install atsrc/apm_cli/integration/hook_integrator.py:585
O(total files in all deps) and unbounded I/O on the install path. A depth-1/2 bounded walk correctly identifies installed package roots without traversing deep script trees.
Suggested: Replacerglob('*')with explicititerdir()at depth 1 and 2. -
[nit]
sort_keys=Truein_hook_entry_content_keyis redundant aftersorted(entry.items())atsrc/apm_cli/integration/hook_integrator.py:581
Dropsort_keys=True. -
[nit]
import yamlat module level solely foryaml.YAMLErroratsrc/apm_cli/integration/hook_integrator.py:54
Replace withfrom yaml import YAMLError. -
[nit]
_is_root_local_packagecalled 3x in the hot path atsrc/apm_cli/integration/hook_integrator.py:768
Compute once and thread through.
CLI Logging Expert
-
[recommended] Silent swallow of
OSError/YAMLErrorin_get_root_local_package_nameshould emit a debug log atsrc/apm_cli/integration/hook_integrator.py:543
Without it, unexpected_localfallbacks are invisible in verbose/agent mode. Add_log.debug("apm.yml name read failed (%s); falling back to package name", e)before thepass. -
[recommended] Stale-entry healing is silent at
src/apm_cli/integration/hook_integrator.py:619
Users and agents cannot confirm old checkout-derived entries were cleaned up. Add_log.debug("Healed %d stale root-source entries for event '%s'", removed, event_name)after the filter step whenheal_stale_root_sourceis True andremoved > 0. -
[nit] Silent JSON decode failure when reading existing hook config is not logged at
src/apm_cli/integration/hook_integrator.py:791
Pre-existing pattern; new healing logic makes a debug warning more relevant.
DevX UX Expert
-
[recommended] Silent healing provides no user-visible signal when stale entries are removed at
src/apm_cli/integration/hook_integrator.py:619
Users inspecting.claude/settings.jsonpost-reinstall and seeing missing entries have no explanation. A--verbose-gated line would satisfy the discoverability contract. -
[recommended] When
apm.ymlhas nonamefield, source marker degrades to bare_localatsrc/apm_cli/integration/hook_integrator.py:550
Weakens the namespace convention. When user later addsname:, the source ID changes and triggers a one-time re-heal. A doc comment or debug log noting this edge case prevents future confusion. -
[nit]
_hook_entry_content_keyexcluding_apm_sourcemeans a byte-identical user-written entry could silently get APM's source marker atsrc/apm_cli/integration/hook_integrator.py:580
Low probability; worth a code comment since the new healing path increases exposure.
Supply Chain Security Expert
No blocking findings.
-
[nit]
apm_modules.rglob('*')follows symlinks by default atsrc/apm_cli/integration/hook_integrator.py:591
A malicious package could plant a symlink cycle insideapm_modules, causing unbounded traversal. Addif path.is_symlink(): continuebefore theis_dir()check. -
[nit]
_safe_source_namepermits interior dots after stripping atsrc/apm_cli/integration/hook_integrator.py:526
No file-system risk (value is only used as a JSON string), but a comment clarifying it must never be used as a path component without containment would prevent future misuse.
OSS Growth Hacker
- [nit] CHANGELOG entry leads with internal implementation detail at
CHANGELOG.md
Current: "...use a stable local hook source id and heal stale same-content merged hook entries left behind by older checkout-derived_apm_sourcevalues..." Suggested: "Hooks installed from root.apm/hooks/*.jsonare now idempotent across different checkout directories -- duplicate hook entries in Claude/Codex/Cursor/Gemini/Windsurf config files no longer accumulate when the same repo is cloned to a different path."
Auth Expert -- inactive
No auth-relevant files changed; hook_integrator.py does not touch AuthResolver, token management, credential resolution, or host classification.
Doc Writer
-
[recommended] CHANGELOG entry exposes internal implementation vocabulary at
CHANGELOG.md:12
_apm_sourceand "heal stale same-content merged hook entries left behind by older checkout-derived_apm_sourcevalues" mean nothing to users. The user-observable behavior is simply: re-runningapm installno longer creates duplicate hook entries.
Suggested: "Root.apm/hooks/*.jsoninstalls now use a stable source identifier, making hook entries in Claude/Codex/Cursor/Gemini/Windsurf configs idempotent across re-installs; stale duplicate entries from older installs are removed automatically. ([BUG] Root .apm hooks can duplicate when _apm_source changes with checkout directory #1329)" -
[nit] "keeping ... idempotent" uses gerund where adjacent entries use declarative present tense at
CHANGELOG.md:12
Restructure to match the section's tense pattern.
Test Coverage Expert
-
[recommended] No integration-tier test for end-to-end "install heals stale root source" flow
All 6 new tests callHookIntegratordirectly (unit tier). The full user promise only manifests when the realapm installCLI entry point drives the integrator. Greppedtests/integration/*.pyfor_apm_source,stale,root_local,heal-- zero matches in hook-healing context.
Proof (missing at integration-with-fixtures):tests/integration/test_local_install.py-- proves: Runningapm installheals stale checkout-name entries in.claude/settings.json[portability-by-manifest, devx]
Suggested: Add a fixture-based integration test that seeds a stale.claude/settings.json, runsapm installvia subprocess, and asserts the stale entry is replaced by_local/{name}. Mark@pytest.mark.integration. -
[recommended] Private helper edge cases uncovered at any tier
_is_root_local_package(pkg_info, None)-> False;path.resolve()raising OSError -> False;_get_root_local_package_namewithname: ""orname: nullin apm.yml falling back to_local(not_local/_local). Grepped for direct calls to private methods -- zero matches.
Proof (missing at unit):tests/unit/integration/test_hook_integrator.py::TestIssue1007Fixes::test_root_local_source_marker_when_apm_yml_name_is_empty-- proves: When apm.yml has name: '' or null, source marker degrades gracefully to '_local' rather than crashing [devx]
This panel is advisory. It does not block merge. Re-apply the
panel-review label after addressing feedback to re-run.
Note
🔒 Integrity filter blocked 2 items
The following items were blocked because they don't meet the GitHub integrity level.
- #1330
pull_request_read: has lower integrity than agent requires. The agent cannot read data with integrity below "approved". - fix(hooks): stabilize root-local hook source ids #1330
pull_request_read: has lower integrity than agent requires. The agent cannot read data with integrity below "approved".
To allow these resources, lower min-integrity in your GitHub frontmatter:
tools:
github:
min-integrity: approved # merged | approved | unapproved | noneGenerated by PR Review Panel for issue #1330 · ● 2.9M · ◷
github-actions
Bot
removed
the
panel-review
|
Maintainer sweep [2026-05-15]: tried to land your rebase from a maintainer terminal but my OAuth scope refuses workflow-file pushes (today's main contains a Trivial rebase, two conflicts, both safe to resolve as below: Conflict 1 -- Conflict 2 -- I verified the rebase locally and the panel verdict ( Thanks @imk1t! |
Description
TL;DR
Root-local
.apm/hooks/*.jsoninstalls now use a checkout-stable source marker derived fromapm.ymlinstead of the worktree basename. Merged hook targets heal stale same-content root hook entries whose old_apm_sourcecame from a previous checkout name, so re-running install does not append duplicates. User-authored hooks and dependency package hooks remain untouched unless they are the current source being replaced.Note
Closes #1329. The array-dedup healer is intentionally scoped to merged hook targets; Copilot's individual-file hook model is not part of this duplicate-array failure mode.
Problem (WHY)
_apm_sourcechanges between installs..apmcontent between worktrees could create install drift.Why these matter: install idempotency is a DevX contract, and hook emission is a multi-harness surface. The issue explicitly asks for root
.apm_apm_sourceto be "stable across checkout directories" and lists Claude, Codex, Gemini, Cursor, and Windsurf as affected merged-hook targets.Approach (WHAT)
install_path.name; use safeapm.ymlnamewith_localfallback._local/<name>so root content cannot collide with dependency source names._apm_source, while preserving user hooks and dependency sources.install_path.namemarker so package aliases and cross-package installs do not collapse.Implementation (HOW)
src/apm_cli/integration/hook_integrator.py-- adds root-local package detection, safe manifest-name normalization,_local/<name>merged-source markers, and content-key based stale-source healing. Current-source cleanup still runs once per event, while stale-source healing runs per root hook file so multiple files targeting the same event are handled.tests/unit/integration/test_hook_integrator.py-- adds regression coverage for Claude and Codex stale-source healing, stable root source naming, user-hook preservation, dependency-hook preservation, dependency-name collision avoidance, and multiple root hook files sharing one event.tests/integration/test_registry.py-- keeps Ruff green by replacing a narrow cleanuptry/except/passblock withcontextlib.suppress; no registry behavior change.CHANGELOG.md-- documents the root hook source-drift fix underUnreleased / Fixed.Diagrams
Legend: root hook content now gets a stable local marker before merged-target cleanup, while dependency and user-owned entries stay outside the stale-root healer.
flowchart LR subgraph Source[Root hooks] H1[".apm/hooks/*.json"] H2["apm.yml name"] end subgraph Integrator[HookIntegrator] I1["stable root package name"]:::new I2["merged source marker _local/name"]:::new I3["same-content stale root healer"]:::new end subgraph Outputs[Merged hook targets] O1[".claude/settings.json"] O2[".codex/hooks.json"] O3["Cursor Gemini Windsurf"] end D1["dependency package sources"] U1["user hooks without _apm_source"] H2 --> I1 H1 --> I2 I1 --> I2 I2 --> I3 I3 --> O1 I3 --> O2 I3 --> O3 D1 --> O1 U1 --> O1 classDef new stroke-dasharray: 5 5; class I1,I2,I3 new;Trade-offs
_local/<safe manifest name>for merged hook ownership; rejected using the raw manifest name because a dependency package can legitimately have the same source id._localwhenapm.ymlcannot provide a safe name; rejected failing install because this path is cleanup/idempotency support, not manifest validation.Benefits
.apmhooks over an old checkout-derived_apm_sourceleaves one managed semantic hook, not two._local/<apm.yml name>across checkout directory names._apm_sourceremain in the generated merged config.Closes #1329
Type of change
Testing
uv run pytest tests/unit/integration/test_hook_integrator.py -x:uv run pytest tests/unit tests/test_console.py -x:uv run --extra dev ruff check src/ tests/:uv run --extra dev ruff format --check src/ tests/:git diff --check:npx --yes -p @mermaid-js/mermaid-cli mmdc -i /private/tmp/apm-pr-diag-1329.mmd -o /private/tmp/apm-pr-diag-1329.svg --quiet:Scenario Evidence
tests/unit/integration/test_hook_integrator.py::test_root_local_heals_stale_source_in_claude_settings(regression-trap for #1329)tests/unit/integration/test_hook_integrator.py::test_root_local_heals_stale_source_in_codex_hooks(regression-trap for #1329).apmhook ownership stays stable when the checkout directory basename changes.tests/unit/integration/test_hook_integrator.py::test_root_local_source_uses_manifest_name_apm_sourcesurvive root hook healing.tests/unit/integration/test_hook_integrator.py::test_root_local_heals_stale_source_in_claude_settingstests/unit/integration/test_hook_integrator.py::test_root_local_healer_preserves_dependency_source_entriestests/unit/integration/test_hook_integrator.py::test_content_dedup_preserves_cross_packagetests/unit/integration/test_hook_integrator.py::test_root_local_source_marker_does_not_collide_with_dependency_nametests/unit/integration/test_hook_integrator.py::test_root_local_heals_stale_source_for_multiple_hook_files_same_eventHow to test
.claude/settings.jsonwith a root hook tagged by an old_apm_source, then run root-local hook integration; expect one managed_local/<name>entry plus any user-owned entries..codex/hooks.json; expect exactly one managed root hook entry._local/<name>sources to remain.uv run pytest tests/unit/integration/test_hook_integrator.py -x; expect all hook-integrator tests to pass.