fix(hooks): stabilize root .apm hook source-ids across renames/worktrees (supersedes #1330, closes #1329)

[danielmeppiel]

Supersedes #1330 — same author content, plus the three follow-ups noted in the shepherd review at #1330 (comment). PR #1330 cannot be force-pushed from a maintainer environment because the rebase onto current main pulls in .github/workflows/* changes that exceed the OAuth scope on the fork; this branch lives on the canonical repo so CI can complete and we can ship the fix for #1329.

What this lands

Closes #1329.

Fixes root .apm hook duplication after renaming a project directory or using git worktrees. _get_root_local_package_name now reads apm.yml's name field and emits _local/<name>, decoupling the hook source-id from install_path.name. A bounded healer removes stale same-content merged entries on subsequent apm install runs (scoped strictly to root-local installs; lockfile-resolved dependency sources are excluded). Affects Claude, Codex, Cursor, Gemini, Windsurf.

Original commits by @imk1t preserved with authorship attribution.

Follow-ups landed on top

1. Subprocess-tier integration test (tests/integration/test_hook_root_source_drift_e2e.py) — runs apm install end-to-end after seeding a stale .claude/settings.json + apm-hooks.json sidecar with _apm_source: "old-checkout-name"; asserts entries heal to _local/myapp and a user-owned hook survives. Closes the integration-tier coverage gap noted by the shepherd; the six unit tests defended the fix at integrator level only.
2. CHANGELOG polish — replaces internal vocabulary (_apm_source, "heal stale same-content merged hook entries") with one-line user outcome: "Root .apm hooks no longer duplicate after renaming the project directory or using git worktrees; Claude/Codex/Cursor/Gemini/Windsurf hook configs stay idempotent across checkouts."
3. Verbose debug logs — two _log.debug(...) lines in src/apm_cli/integration/hook_integrator.py: one for the manifest-parse fallback in _get_root_local_package_name, one for the heal count when stale entries are removed. Surface under --verbose.

Also updates the PR's unit tests to read _apm_source from the Claude sidecar (post-#1359 schema-strict layout), since #1359 landed after #1330 was opened.

Validation evidence

• Lint contract silent: uv run --extra dev ruff check src/ tests/ && uv run --extra dev ruff format --check src/ tests/ → clean
• Unit tier: uv run --extra dev pytest tests/unit/integration/test_hook_integrator.py → 156 passed
• Integration tier (new): uv run --extra dev pytest tests/integration/test_hook_root_source_drift_e2e.py → 1 passed
• CI will validate the full suite.

[Copilot]

Pull request overview

Stabilizes the _apm_source marker for root .apm hooks so reinstalls after directory renames or worktree moves do not leave duplicate entries in merged hook configs (Claude/Codex/Cursor/Gemini/Windsurf). Root hook ownership is now derived from apm.yml's name and namespaced as _local/<name>; a bounded, scoped healer removes stale same-content entries on subsequent installs while leaving user hooks and dependency-owned hooks intact.

Changes:

• Add _get_root_local_package_name / _get_hook_source_marker / _dependency_hook_sources in HookIntegrator, plus content-key based stale-source healing in _integrate_merged_hooks.
• Extensive unit coverage in tests/unit/integration/test_hook_integrator.py for stable root naming, stale healing across Claude/Codex, dependency-source preservation, lockfile and bounded fallback discovery (including symlink rejection), and multi-hook-file events.
• New subprocess-tier E2E in tests/integration/test_hook_root_source_drift_e2e.py and a CHANGELOG entry under Unreleased/Fixed.

Show a summary per file

File	Description
src/apm_cli/integration/hook_integrator.py	Stable root source marker, dependency-source discovery, and stale-entry healer
tests/unit/integration/test_hook_integrator.py	New unit tests covering source naming, healing, dependency preservation, and bounded scans
tests/integration/test_hook_root_source_drift_e2e.py	New subprocess-tier regression for #1329
CHANGELOG.md	User-facing entry under Unreleased/Fixed

Copilot's findings

• Files reviewed: 4/4 changed files
• Comments generated: 0

[github-actions]

APM Review Panel: ship_with_followups

Decouples root .apm hook source-ids from install-path names, ending duplicate-hook drift on project renames and git worktrees across all five major AI harnesses.

cc @danielmeppiel @sergio-sisternes-epam -- a fresh advisory pass is ready for your review.

The panel converges on a sound, well-scoped fix with no blocking findings. The core mechanic -- reading apm.yml name to emit _local/<name> as the hook source-id, plus a bounded healer on subsequent apm install runs -- is architecturally correct and the right level of intervention. Supply-chain-security confirms the source-name sanitizer is sound for today's usage; the embedded double-dot edge case is a hardening recommendation, not a current vulnerability. Auth is inactive (correctly). CLI logging and DevX UX both clear the PR with nit-level log-line improvements only.

Three recommended-tier gaps warrant attention before or immediately after merge. First, both doc-writer and oss-growth-hacker independently confirm CHANGELOG.md is absent from the diff despite the PR body advertising it as a changed file -- a five-harness reliability fix without a changelog entry is a missed adoption signal and a maintenance liability. Second, the test-coverage-expert finds that the heal path for Cursor, Gemini, and Windsurf has no integration-tier regression trap; only Claude is exercised in the e2e suite, meaning a silent predicate drift in _should_remove_prior_merged_entry for those three targets would reach users undetected. The e2e test is well-structured and parametrizing it over the remaining harnesses is a low-effort, high-value guard. Third, python-architect flags a God-Class trajectory in HookIntegrator at 1558 lines; extracting DependencySourceScanner is a clean modularization that does not need to land in this PR but should not slip past the next meaningful touch to hook_integrator.py.

The test-coverage-expert's S7 probe returned outcome: unknown because the review sandbox lacked the uv/pytest environment. Under the panel's evidence-weighting rules, unknown carries no arbitration weight; however, the missing Cursor/Gemini/Windsurf integration coverage (outcome: missing on a multi-harness-support surface) inherits near-blocking weight by policy and is the highest-priority follow-up.

Dissent. No substantive dissent between panelists. The doc-writer and oss-growth-hacker convergence on the CHANGELOG gap (both independently confirmed from the diff) strengthens the recommended-tier weight of that finding; no specialist argued against addressing it in-PR.

Aligned with: Portable by manifest -- source-id now derived from apm.yml name field, hook identity travels with the manifest not the filesystem path. Secure by default -- _safe_source_name strips traversal characters; one additional consecutive-dot collapse recommended. Multi-harness + multi-host -- fix covers Claude, Codex, Cursor, Gemini, and Windsurf; Copilot correctly excluded. Pragmatic as npm -- silent auto-heal on apm install matches the npm install idempotency contract.

Growth signal. The oss-growth-hacker identifies this fix as a latent launch beat: "rename your project directory and apm install auto-heals your AI assistant hooks" is a one-sentence proof point for the vendor-neutral, zero-configuration positioning story. It resonates most with monorepo and worktree users -- a cohort that skews toward OSS contributors and power users who generate secondary reach. Recommend shipping the CHANGELOG entry with user-outcome framing and including this fix in the next release post under a reliability section. A short social post naming all five harnesses explicitly reinforces the multi-harness positioning without requiring a feature launch.

Panel summary

Persona	B	R	N	Takeaway
Python Architect	0	1	3	HookIntegrator has grown to 1558 lines with 14 new helpers forming two orthogonal concern clusters; extract DependencySourceScanner before the class becomes unnavigable.
CLI Logging Expert	0	0	2	Two well-formed DEBUG lines only; no user-visible output added. Logging surface is clean. Ship.
DevX UX Expert	0	0	1	No CLI surface regressions; silent healing is correct UX. One nit on verbose fallback legibility.
Supply Chain Security Expert	0	1	1	Path guards applied correctly; source-name sanitizer is sound; one recommended hardening on yaml import coupling; no blocking findings.
OSS Growth Hacker	0	1	1	Strong power-user fix with a compelling story; CHANGELOG entry is absent from the diff and the release narrative undersells the 5-harness impact and the "it just works after rename" hook.
Doc Writer	0	2	1	CHANGELOG.md is absent from the PR diff despite the PR body advertising CHANGELOG polish as follow-up #2; user-facing outcome sentence was never committed. One recommended doc gap in hooks-and-commands.md.
Test Coverage Expert	0	2	0	Unit tier is dense and well-targeted; e2e heal test covers Claude only -- Gemini/Windsurf/Cursor healing has no integration-tier regression trap.

B = blocking-severity findings, R = recommended, N = nits.
Counts are signal strength, not gates. The maintainer ships.

Top 5 follow-ups

1. [Test Coverage Expert] Parametrize the e2e heal test over cursor, gemini, and windsurf targets in addition to claude. -- outcome: missing on a multi-harness-support surface. A silent regression in _should_remove_prior_merged_entry for these three harnesses would not be caught before users see duplicate hook entries after a project rename. Highest-signal gap in the PR.
2. [Doc Writer] Add CHANGELOG.md [Unreleased] > Fixed entry before merge; add Pitfalls note to hooks-and-commands.md post-merge. -- Both doc-writer and oss-growth-hacker independently confirm CHANGELOG.md is absent from the diff. A five-harness reliability fix without a changelog entry is a missed adoption signal and leaves the fix undiscoverable for users tracking changes.
3. [Supply Chain Security Expert] Collapse consecutive dots in _safe_source_name after the re.sub call: re.sub(r'\.{2,}', '.', safe).strip('.-_'). -- Embedded double-dots (e.g. foo..bar) pass the current guard. No traversal risk today since the marker is JSON metadata only, but hardens against any future caller that passes the name into a Path join.
4. [Python Architect] Extract DependencySourceScanner (or a free function) from HookIntegrator before the next meaningful touch to hook_integrator.py. -- HookIntegrator is at 1558 lines with 14 new helpers forming two orthogonal concern clusters. Group B (dependency-directory scanning) has zero coupling to instance state. Extraction cuts ~200 lines and makes that surface independently testable.
5. [Doc Writer] Add parenthetical in CHANGELOG noting Copilot is unaffected and why. -- PR body covers Claude/Codex/Cursor/Gemini/Windsurf but not Copilot. A one-line explanation (per-file namespacing) prevents confusion and reinforces the multi-harness positioning.

Architecture

classDiagram
    direction LR
    class BaseIntegrator {
      <<Abstract>>
      +integrate_package_hooks()
    }
    class HookIntegrator {
      <<Facade>>
      +integrate_package_hooks(package_info, project_root)
      +_integrate_merged_hooks(package_info, project_root, ...)
      +_get_package_name(package_info, project_root) str
      +_get_root_local_package_name(package_info, project_root) str
      +_get_hook_source_marker(package_info, project_root, name) str
      +_is_root_local_package(package_info, project_root) bool
      +_safe_source_name(value, fallback) str
      +_hook_entry_content_key(entry) str
      +_should_remove_prior_merged_entry(entry, ...) bool
      +_dependency_hook_sources(project_root) set
      +_lockfile_dependency_paths(project_root) tuple
      +_safe_dependency_path(apm_modules, rel_path) Path
      +_has_symlink_component(apm_modules, path) bool
      +_is_dependency_package_dir(path) bool
      +_add_dependency_source(sources, path) bool
      +_child_dependency_dirs(path) list
      +_bounded_dependency_hook_sources(apm_modules) set
    }
    class LockFile {
      <<ValueObject>>
      +read(path) LockFile
      +get_installed_paths(base) list
    }
    class PathTraversalError {
      <<Exception>>
    }
    class DependencySourceScanner {
      <<PragmaticExtract>>
    }
    BaseIntegrator <|-- HookIntegrator : extends
    HookIntegrator ..> LockFile : reads lockfile
    HookIntegrator ..> PathTraversalError : guards
    HookIntegrator ..> DependencySourceScanner : candidate extract
    class HookIntegrator:::touched
    classDef touched fill:#fff3b0,stroke:#d47600

Loading

flowchart TD
    A(["apm install"])
    B["integrate_package_hooks\nhook_integrator.py:898"]
    C{"_is_root_local_package\n:593"}
    D["_get_root_local_package_name\n:612"]
    E["[I/O] read apm.yml\nname field"]
    F["fallback: install_path.name"]
    G["_get_hook_source_marker\n:651"]
    H["source_marker = _local/name"]
    I{"heal_stale_root_source?"}
    J["_dependency_hook_sources\n:671"]
    K{"lockfile readable?"}
    L["[I/O] LockFile.read\nlockfile path"]
    M["_safe_dependency_path\n:710 (traversal guard)"]
    N["_bounded_dependency_hook_sources\n:830 (fallback scan)"]
    O["_integrate_merged_hooks\n:971"]
    P["build fresh_content_keys\n_hook_entry_content_key :665"]
    Q["_should_remove_prior_merged_entry\n:842"]
    R{"remove_current_source\nor heal_stale?"}
    S["keep entry if source in dependency_sources"]
    T["heal: log debug stale count\n_log.debug :1130"]
    U["[FS] write updated hook JSON\nclaude/codex/cursor/gemini/windsurf"]

    A --> B
    B --> C
    C -- root local --> D
    D --> E
    E -- name found --> G
    E -- unreadable --> F
    F --> G
    G --> H
    C -- not root local --> O
    H --> I
    I -- yes --> J
    J --> K
    K -- yes --> L
    L --> M
    M --> O
    K -- no --> N
    N --> O
    I -- no --> O
    O --> P
    P --> R
    R -- yes --> Q
    Q --> S
    S --> T
    T --> U
    R -- no --> U

Loading

sequenceDiagram
    participant User
    participant CLI as apm install
    participant HI as HookIntegrator
    participant FS as Filesystem
    participant LF as LockFile

    User->>CLI: apm install (after dir rename or worktree)
    CLI->>HI: integrate_package_hooks(package_info, project_root)
    HI->>FS: read apm.yml name field
    FS-->>HI: name = myapp
    HI->>HI: source_marker = _local/myapp
    HI->>LF: LockFile.read(apm.lock.yaml)
    LF-->>HI: installed_paths = [dep-a, dep-b]
    HI->>FS: read existing hook JSON (e.g. .claude/settings.json)
    FS-->>HI: entries with _apm_source = old-checkout-name
    HI->>HI: content_key matches fresh entry? yes -> stale
    HI->>HI: source not in dependency_sources -> heal
    HI->>FS: write hook JSON: _apm_source = _local/myapp, user entries preserved
    HI-->>CLI: hooks_integrated count
    CLI-->>User: install complete (no duplicate hook blocks)

Loading

Recommendation

Merge after adding the CHANGELOG [Unreleased] > Fixed entry in-PR (the doc-writer/oss-growth-hacker convergence makes this the clearest in-PR action). The multi-harness e2e parametrization (Cursor/Gemini/Windsurf heal coverage) is the highest-signal post-merge follow-up and should be tracked as a fast-follow before the next release cut. The double-dot source-name hardening and DependencySourceScanner extraction are clean improvements for the next hook_integrator.py touch cycle.

---

Full per-persona findings

Python Architect

• [recommended] HookIntegrator accumulates two orthogonal concern clusters that should be extracted at src/apm_cli/integration/hook_integrator.py
  The 14 new methods fall into two clean groups: (A) source-marker derivation and (B) dependency-directory scanning. Group B has no coupling to HookIntegrator state -- every method in it is @staticmethod. At 1558 lines HookIntegrator is approaching God-Class territory. Extracting group B into a module-level DependencySourceScanner (or a free function returning set[str]) would cut the class by ~200 lines, make _dependency_hook_sources independently testable, and respect the APM principle: one class per primary abstraction.
  Suggested: Create src/apm_cli/integration/dependency_source_scanner.py with a single public function scan_dependency_sources(project_root: Path) -> set[str]. HookIntegrator._dependency_hook_sources becomes a one-liner.
• [nit] _get_root_local_package_name, _get_hook_source_marker, _should_remove_prior_merged_entry are instance methods that touch no instance state at src/apm_cli/integration/hook_integrator.py
  All three methods call only @staticmethod siblings. Should be @staticmethod to allow direct class-level calls in tests without constructing a HookIntegrator.
  Suggested: Add @staticmethod to these three methods. Update call sites: self.X(...) -> HookIntegrator.X(...).
• [nit] Healing-count calculation uses O(n^2) list membership check at src/apm_cli/integration/hook_integrator.py:1120
  The healed-count block checks e not in kept_entries where kept_entries is a list. O(k*m). Convert kept_entries to a set of ids before the counting loop.
  Suggested: kept_ids = {id(e) for e in kept_entries}; healed = sum(1 for e in prior_entries if isinstance(e, dict) and e.get('_apm_source') and e.get('_apm_source') != source_marker and e.get('_apm_source') not in dependency_sources and id(e) not in kept_ids)
• [nit] _get_package_name docstring Args section does not document the new project_root parameter at src/apm_cli/integration/hook_integrator.py:638
  The Args block was not updated when project_root was added to the signature.
  Suggested: Add project_root (Path | None): When provided and the package is the project root, reads apm.yml name for a stable source marker.

CLI Logging Expert

• [nit] Fallback debug log omits the exception message, only logs the class name at src/apm_cli/integration/hook_integrator.py
  The message logs exc.class.name but drops the exception value (str(exc)). For YAML parse errors the class name alone rarely tells what went wrong.
  Suggested: _log.debug("Hook integrator: apm.yml manifest unreadable for %s (%s: %s), falling back to install_path basename", project_root, exc.__class__.__name__, exc,)
• [nit] Heal-count log fires per event_name loop iteration, not once as a post-heal summary at src/apm_cli/integration/hook_integrator.py
  If a project has N hook events the heal log emits N separate lines. A single aggregated line after the loop is less noisy and easier to parse.

DevX UX Expert

• [nit] Verbose fallback log emits bare '_local' with no project path hint, making it hard to correlate to a specific project in multi-worktree debugging sessions at src/apm_cli/integration/hook_integrator.py
  When apm.yml is unreadable and the fallback fires, the debug line logs '_local' with no disambiguating project root. npm and cargo always include the package root path in fallback warnings.
  Suggested: Include project_root in the fallback debug log: 'Hook integrator: apm.yml unreadable at %s (%s), falling back; source will be _local'

Supply Chain Security Expert

• [recommended] _safe_source_name allows embedded double-dots (e.g. 'foo..bar') in the source marker at src/apm_cli/integration/hook_integrator.py
  The allowlist regex [^A-Za-z0-9._-]+ permits consecutive dots. 'foo..bar' passes the final guard (only rejects entire string '.' or '..'). No traversal risk today (marker is JSON metadata only), but hardens against any future Path join use.
  Suggested: After the re.sub call, add: safe = re.sub(r'\.{2,}', '.', safe).strip('.-_')
• [nit] Module-level 'import yaml' added only to name yaml.YAMLError in an except clause at src/apm_cli/integration/hook_integrator.py
  Introduces a direct PyYAML coupling just to reference yaml.YAMLError. Prefer catching (OSError, ValueError, Exception) or importing yaml locally inside the try block.

OSS Growth Hacker

• [recommended] CHANGELOG entry is missing from the diff despite being listed as a changed file at CHANGELOG.md
  The PR body claims CHANGELOG.md is a changed file, but the diff contains only hook_integrator.py and two test files. A fix that silently heals duplicate entries across Claude, Codex, Cursor, Gemini, and Windsurf on next apm install belongs in the changelog.
  Suggested: Add a Fixed entry under [Unreleased]: Hook source-ids for root .apm content are now derived from apm.yml 'name:' instead of the install directory name, so renaming your project directory or using git worktrees no longer creates duplicate hook entries in Claude, Codex, Cursor, Gemini, and Windsurf; stale entries are auto-healed on the next apm install. (#1329, #1392)
• [nit] The 'auto-healed on next apm install' detail is the strongest UX hook but is buried in the PR body
  For release notes and social copy, 'rename your project, run apm install, hooks just work' is a one-sentence proof point for monorepo and worktree users.
  Suggested: Lead the CHANGELOG bullet with the user outcome.

Auth Expert -- inactive

No auth surface touched; none of the changed files touch auth.py, token_manager.py, or any credential-resolution, token-management, host-classification, or HTTP-authorization surface.

Doc Writer

• [recommended] CHANGELOG.md is not in the PR diff; the promised user-facing entry was never committed at CHANGELOG.md
  The PR body explicitly lists 'CHANGELOG polish' as follow-up Integrate copilot runtime #2. The GitHub API reports changed_files: 3 (hook_integrator.py, test_hook_root_source_drift_e2e.py, test_hook_integrator.py). CHANGELOG.md is not among them and the current CHANGELOG has no [BUG] Root .apm hooks can duplicate when _apm_source changes with checkout directory #1329 entry.
  Suggested: Add under [Unreleased] > Fixed: - Root '.apm' hooks no longer duplicate after renaming the project directory or using git worktrees; Claude, Codex, Cursor, Gemini, and Windsurf hook configs stay idempotent across checkouts. (#1392, closes #1329)
• [recommended] hooks-and-commands.md Pitfalls section is silent on worktree and directory-rename behavior at docs/src/content/docs/producer/author-primitives/hooks-and-commands.md
  The fix corrects a non-obvious failure mode. The Pitfalls section already lists six pitfalls; omitting this one leaves users with no documented signal that the pattern was historically broken and is now stable.
  Suggested: Add to Pitfalls: **Renaming the project directory or using git worktrees.** Hook source-ids are derived from 'apm.yml' 'name' field, not the directory name. Renaming the checkout or switching worktrees does not produce duplicate hook entries; a bounded healer removes any stale entries on the next 'apm install'.
• [nit] Proposed CHANGELOG entry omits Copilot; omission is correct but unexplained at CHANGELOG.md
  The PR body lists Claude/Codex/Cursor/Gemini/Windsurf but not Copilot (per-file namespacing, unaffected). Readers may notice the gap.
  Suggested: Add parenthetical: '(Copilot uses per-file namespacing and is unaffected)'

Test Coverage Expert

• [recommended] Heal path for Gemini, Windsurf, and Cursor targets has no integration-tier test at tests/integration/test_hook_root_source_drift_e2e.py
  PR body states fix affects Claude, Codex, Cursor, Gemini, and Windsurf. The new e2e test exercises only the Claude sidecar path. grep of test_hook_integrator.py for gemini|windsurf|cursor combined with heal|stale|root_local returns zero hits. Hooks surface floor is integration-with-fixtures per the tier matrix.
  Suggested: Parametrize the e2e test over ['claude', 'cursor', 'gemini', 'windsurf'] by adjusting the settings path written before 'apm install', asserting stale source is replaced with '_local/(name)' in each harness's hook file.
  Proof (missing at integration-with-fixtures): tests/integration/test_hook_root_source_drift_e2e.py::test_root_hook_source_drift_heals_on_reinstall[cursor] -- proves: After renaming a project directory, apm install removes the stale hook source-id for Cursor, Gemini, and Windsurf as well as Claude. [multi-harness-support, devx]
  assert sidecar_sources == ['_local/myapp'], f'Expected single _local/myapp in cursor rules.json, got {sidecar_sources}'
• [recommended] S7 probe: e2e integration test exists but could not be run in this review environment at tests/integration/test_hook_root_source_drift_e2e.py
  uv and the project's pytest install are not available in the review sandbox. The test reads well and confidence is high, but outcome cannot be certified as irrefutable under the S7 probe rule.
  Suggested: No code change needed. CI run evidence (pytest invocation + pass line + duration) should be recorded in the PR Scenario Evidence table per the rubric.
  Proof (unknown at e2e): tests/integration/test_hook_root_source_drift_e2e.py::test_root_hook_source_drift_heals_on_reinstall -- proves: After a simulated project rename, apm install produces exactly one '_local/(manifest-name)' sidecar entry and preserves user-owned hooks. [portability-by-manifest, devx, multi-harness-support]
  assert sidecar_sources == ['_local/myapp']

_{This panel is advisory. It does not block merge. Re-apply the
panel-review label after addressing feedback to re-run.}

Generated by PR Review Panel for issue #1392 · ● 3.8M · ◷