refactor(install): modularize install.py into engine package

CHANGELOG.md

@@ -16,6 +16,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 ### Fixed
 
 - Harden `apm install` stale-file cleanup to prevent unsafe lockfile deletions, preserve user-edited files via per-file SHA-256 provenance, and improve cleanup reporting during install and `--dry-run` (#666, #762)
+- Local `.apm/` stale-cleanup now uses pre-install content hashes for provenance verification. Previously the lockfile was re-read after regeneration, which always yielded empty hashes, causing the user-edit safety gate to be silently skipped for project-local files (#764)
 - Fix `apm marketplace add` silently failing for private repos by using credentials when probing `marketplace.json` (#701)
 - Harden marketplace plugin normalization to enforce that manifest-declared `agents`/`skills`/`commands`/`hooks` paths resolve inside the plugin root (#760)
 - Stop `test_auto_detect_through_proxy` from making real `api.github.com` calls by passing a mock `auth_resolver`, fixing flaky macOS CI rate-limit failures (#759)
@@ -26,7 +27,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ### Changed
 
+- Refactor `apm install` internals to apply real design patterns: introduce a `DependencySource` Strategy hierarchy with shared `run_integration_template()` Template Method (kills ~300 LOC duplication across local/cached/fresh dep handlers), add `services.py` DI seam to eliminate `_install_mod` indirection, and wrap the pipeline in a typed `InstallService` Application Service consuming a frozen `InstallRequest`. `install/phases/integrate.py` shrinks from 1013 to ~400 LOC; the public `apm install` behaviour and CLI surface are unchanged. Backward-compatible: `_install_apm_dependencies` re-export and 55 healthy test patches keep working (#764)
 - `apm marketplace browse/search/add/update` now route through the registry proxy when `PROXY_REGISTRY_URL` is set; `PROXY_REGISTRY_ONLY=1` blocks direct GitHub API calls (#506)
+- Refactor `apm install` into a modular engine package (`apm_cli/install/`) with discrete phases (resolve, targets, download, integrate, cleanup, lockfile, finalize, post-deps local), preserving behaviour and the `#762` cleanup chokepoint (#764)
 
 ## [0.8.11] - 2026-04-06
 

src/apm_cli/install/__init__.py

@@ -0,0 +1,23 @@
+"""APM install engine.
+
+This package implements the install pipeline that the
+`apm_cli.commands.install` Click command delegates to.
+
+Architecture:
+
+    pipeline.py     orchestrator that calls each phase in order
+    context.py      InstallContext dataclass (state passed between phases)
+    request.py      InstallRequest dataclass (typed CLI inputs)
+    service.py      InstallService Application Service (entry point)
+    services.py     DI seam re-exporting integration helpers
+    sources.py      DependencySource Strategy hierarchy
+    template.py     run_integration_template() Template Method
+    validation.py   manifest validation (dependency syntax, existence checks)
+
+    phases/         one module per pipeline phase
+    helpers/        cross-cutting helpers (security scan, gitignore)
+    presentation/   dry-run preview + final result rendering
+
+The engine is import-safe (no Click decorators at top level) so phase modules
+can be unit-tested directly without invoking the CLI.
+"""

src/apm_cli/install/context.py

@@ -0,0 +1,114 @@
+"""Mutable state passed between install pipeline phases.
+
+Each phase is a function ``def run(ctx: InstallContext) -> None`` that reads
+the inputs already populated by earlier phases and writes its own outputs to
+the context.  Keeping shared state on a single typed object turns implicit
+shared lexical scope (the legacy 1444-line ``_install_apm_dependencies``)
+into explicit data flow that is easy to audit and to test phase-by-phase.
+
+Fields are added to this dataclass incrementally as phases are extracted from
+the legacy entry point.  A field belongs here if and only if it is read or
+written by more than one phase.  Phase-local state should stay local.
+"""
+
+from __future__ import annotations
+
+from dataclasses import dataclass, field
+from pathlib import Path
+from typing import Any, Dict, List, Optional, Set, Tuple
+
+
+@dataclass
+class InstallContext:
+    """State shared across install pipeline phases.
+
+    Required-on-construction fields go above the ``field(default=...)``
+    barrier; outputs accumulated by phases use ``field(default_factory=...)``.
+
+    Fields are grouped by the phase that first populates them.  A trailing
+    comment ``# <phase>`` marks the originating phase for auditability.
+    """
+
+    # ------------------------------------------------------------------
+    # Required on construction (caller supplies before any phase runs)
+    # ------------------------------------------------------------------
+    project_root: Path
+    apm_dir: Path
+
+    # ------------------------------------------------------------------
+    # Inputs: populated by the caller from CLI args / APMPackage
+    # ------------------------------------------------------------------
+    apm_package: Any = None  # APMPackage
+    update_refs: bool = False
+    scope: Any = None  # InstallScope (defaults to PROJECT)
+    auth_resolver: Any = None  # AuthResolver
+    marketplace_provenance: Optional[Dict[str, Any]] = None
+    parallel_downloads: int = 4
+    logger: Any = None  # InstallLogger
+    target_override: Optional[str] = None  # CLI --target value
+
+    dry_run: bool = False
+    force: bool = False
+    verbose: bool = False
+    dev: bool = False
+    only_packages: Optional[List[str]] = None
+
+    # ------------------------------------------------------------------
+    # Resolve phase outputs
+    # ------------------------------------------------------------------
+    all_apm_deps: List[Any] = field(default_factory=list)  # resolve
+    root_has_local_primitives: bool = False  # resolve
+    deps_to_install: List[Any] = field(default_factory=list)  # resolve
+    dependency_graph: Any = None  # resolve
+    existing_lockfile: Any = None  # resolve
+    lockfile_path: Optional[Path] = None  # resolve
+    apm_modules_dir: Optional[Path] = None  # resolve
+    downloader: Any = None  # resolve (GitHubPackageDownloader)
+    callback_downloaded: Dict[str, Any] = field(default_factory=dict)  # resolve
+    callback_failures: Set[str] = field(default_factory=set)  # resolve
+    transitive_failures: List[Tuple[str, str]] = field(default_factory=list)  # resolve
+
+    # ------------------------------------------------------------------
+    # Targets phase outputs
+    # ------------------------------------------------------------------
+    targets: List[Any] = field(default_factory=list)  # targets
+    integrators: Dict[str, Any] = field(default_factory=dict)  # targets
+
+    # ------------------------------------------------------------------
+    # Download phase outputs
+    # ------------------------------------------------------------------
+    pre_download_results: Dict[str, Any] = field(default_factory=dict)  # download
+    pre_downloaded_keys: Set[str] = field(default_factory=set)  # download
+
+    # ------------------------------------------------------------------
+    # Pre-integrate inputs (populated by caller before integrate phase)
+    # ------------------------------------------------------------------
+    diagnostics: Any = None  # DiagnosticCollector
+    registry_config: Any = None  # RegistryConfig
+    managed_files: Set[str] = field(default_factory=set)
+
+    # ------------------------------------------------------------------
+    # Integrate phase outputs (written by integrate, read by cleanup/lockfile/summary)
+    # ------------------------------------------------------------------
+    intended_dep_keys: Set[str] = field(default_factory=set)
+    package_deployed_files: Dict[str, List[str]] = field(default_factory=dict)
+    package_types: Dict[str, str] = field(default_factory=dict)
+    package_hashes: Dict[str, str] = field(default_factory=dict)
+    installed_count: int = 0  # integrate
+    unpinned_count: int = 0  # integrate
+    installed_packages: List[Any] = field(default_factory=list)  # integrate
+    total_prompts_integrated: int = 0  # integrate
+    total_agents_integrated: int = 0  # integrate
+    total_skills_integrated: int = 0  # integrate
+    total_sub_skills_promoted: int = 0  # integrate
+    total_instructions_integrated: int = 0  # integrate
+    total_commands_integrated: int = 0  # integrate
+    total_hooks_integrated: int = 0  # integrate
+    total_links_resolved: int = 0  # integrate
+
+    # ------------------------------------------------------------------
+    # Post-deps local content tracking (F3)
+    # ------------------------------------------------------------------
+    old_local_deployed: List[str] = field(default_factory=list)  # pipeline setup
+    local_deployed_files: List[str] = field(default_factory=list)  # integrate (root)
+    local_content_errors_before: int = 0  # integrate (pre-root)

src/apm_cli/install/helpers/__init__.py

@@ -0,0 +1 @@
+"""Cross-cutting install helpers (security scan, gitignore)."""

src/apm_cli/install/helpers/security_scan.py

@@ -0,0 +1,51 @@
+"""Pre-deploy security scan that runs before any file is written to the project tree.
+
+Wraps the :class:`~apm_cli.security.gate.SecurityGate` scanner used by the
+install pipeline. The scan detects hidden characters (zero-width joiners,
+bidirectional overrides, etc.) that could be used to smuggle malicious payloads
+into prompts, skills, or agent definitions.
+"""
+
+from pathlib import Path
+
+from apm_cli.utils.diagnostics import DiagnosticCollector
+
+
+def _pre_deploy_security_scan(
+    install_path: Path,
+    diagnostics: DiagnosticCollector,
+    package_name: str = "",
+    force: bool = False,
+    logger=None,
+) -> bool:
+    """Scan package source files for hidden characters BEFORE deployment.
+
+    Delegates to :class:`SecurityGate` for the scan->classify->decide pipeline.
+    Inline CLI feedback (error/info lines) is kept here because it is
+    install-specific formatting.
+
+    Returns:
+        True if deployment should proceed, False to block.
+    """
+    from apm_cli.security.gate import BLOCK_POLICY, SecurityGate
+
+    verdict = SecurityGate.scan_files(
+        install_path, policy=BLOCK_POLICY, force=force
+    )
+    if not verdict.has_findings:
+        return True
+
+    # Record into diagnostics (consistent messaging via gate)
+    SecurityGate.report(verdict, diagnostics, package=package_name, force=force)
+
+    if verdict.should_block:
+        if logger:
+            logger.error(
+                f"  Blocked: {package_name or 'package'} contains "
+                f"critical hidden character(s)"
+            )
+            logger.tree_item(f"  |-- Inspect source: {install_path}")
+            logger.tree_item("  |-- Use --force to deploy anyway")
+        return False
+
+    return True

src/apm_cli/install/phases/__init__.py

@@ -0,0 +1 @@
+"""Install pipeline phases."""

src/apm_cli/install/phases/cleanup.py

@@ -0,0 +1,153 @@
+"""Cleanup orchestrator phase -- orphan and stale-file removal.
+
+Routes **all** file-system deletions through the canonical security chokepoint
+``apm_cli.integration.cleanup.remove_stale_deployed_files`` (PR #762) which
+enforces three safety gates: ``validate_deploy_path``, directory rejection,
+and fail-closed content-hash provenance.
+
+Two distinct cleanup passes run in sequence:
+
+**Block A -- Orphan cleanup**
+    For every dependency in the *previous* lockfile whose key is NOT in
+    ``ctx.intended_dep_keys``, all deployed files are removed.  ``targets=None``
+    is passed deliberately so the helper validates against *all*
+    ``KNOWN_TARGETS``, not just the active install's target set.
+
+**Block B -- Intra-package stale-file cleanup**
+    For every dependency still in the manifest, files present in the old
+    lockfile but absent from the fresh integration output are removed.
+    Failed deletions are re-inserted into ``ctx.package_deployed_files`` so
+    the downstream lockfile phase records the retained paths.
+
+This module is a faithful extraction from ``commands/install.py`` --
+no behavioural changes.
+"""
+
+from __future__ import annotations
+
+from typing import TYPE_CHECKING
+
+from apm_cli.drift import detect_stale_files
+from apm_cli.integration.base_integrator import BaseIntegrator
+from apm_cli.integration.cleanup import remove_stale_deployed_files
+
+if TYPE_CHECKING:
+    from apm_cli.install.context import InstallContext
+
+
+def run(ctx: InstallContext) -> None:
+    """Execute orphan cleanup and intra-package stale-file cleanup.
+
+    Reads ``ctx.existing_lockfile``, ``ctx.intended_dep_keys``,
+    ``ctx.package_deployed_files`` (mutated), ``ctx.diagnostics``,
+    ``ctx.targets``, ``ctx.logger``, ``ctx.project_root``,
+    ``ctx.only_packages``.
+    """
+    existing_lockfile = ctx.existing_lockfile
+    only_packages = ctx.only_packages
+    intended_dep_keys = ctx.intended_dep_keys
+    project_root = ctx.project_root
+    _targets = ctx.targets
+    diagnostics = ctx.diagnostics
+    logger = ctx.logger
+    package_deployed_files = ctx.package_deployed_files
+
+    # ------------------------------------------------------------------
+    # Orphan cleanup: remove deployed files for packages that were
+    # removed from the manifest. This happens on every full install
+    # (no only_packages), making apm install idempotent with the manifest.
+    # Routed through remove_stale_deployed_files() so the same safety
+    # gates -- including per-file content-hash provenance -- apply
+    # uniformly with the intra-package stale path below.
+    # ------------------------------------------------------------------
+    if existing_lockfile and not only_packages:
+        # Use intended_dep_keys (manifest intent, computed at ~line 1707) --
+        # NOT package_deployed_files.keys() (integration outcome). A transient
+        # integration failure for a still-declared package would leave its key
+        # absent from package_deployed_files; deriving orphans from the outcome
+        # set would then misclassify it as removed and delete its previously
+        # deployed files even though it is still in apm.yml.
+        _orphan_total_deleted = 0
+        _orphan_deleted_targets: list = []
+        for _orphan_key, _orphan_dep in existing_lockfile.dependencies.items():
+            if _orphan_key in intended_dep_keys:
+                continue  # still in manifest -- handled by stale-cleanup below
+            if not _orphan_dep.deployed_files:
+                continue
+            _orphan_result = remove_stale_deployed_files(
+                _orphan_dep.deployed_files,
+                project_root,
+                dep_key=_orphan_key,
+                # targets=None -> validate against all KNOWN_TARGETS, not
+                # just the active install's targets. An orphan can have
+                # files under a target the user is not currently running
+                # (e.g. switched runtime since the dep was installed,
+                # or scope mismatch). Restricting to _targets here would
+                # leave those files behind. Pre-PR code handled this by
+                # explicitly merging KNOWN_TARGETS; targets=None is the
+                # cleaner equivalent.
+                targets=None,
+                diagnostics=diagnostics,
+                recorded_hashes=dict(_orphan_dep.deployed_file_hashes),
+                failed_path_retained=False,
+            )
+            _orphan_total_deleted += len(_orphan_result.deleted)
+            _orphan_deleted_targets.extend(_orphan_result.deleted_targets)
+            for _skipped in _orphan_result.skipped_user_edit:
+                if logger:
+                    logger.cleanup_skipped_user_edit(_skipped, _orphan_key)
+        if _orphan_deleted_targets:
+            BaseIntegrator.cleanup_empty_parents(
+                _orphan_deleted_targets, project_root
+            )
+        if logger:
+            logger.orphan_cleanup(_orphan_total_deleted)
+
+    # ------------------------------------------------------------------
+    # Stale-file cleanup: within each package still present in the
+    # manifest, remove files that were in the previous lockfile's
+    # deployed_files but are not in the fresh integration output.
+    # Handles renames and intra-package file removals (issue #666).
+    # Complements the package-level orphan cleanup above, which handles
+    # packages that left the manifest entirely.
+    # ------------------------------------------------------------------
+    if existing_lockfile and package_deployed_files:
+        for dep_key, new_deployed in package_deployed_files.items():
+            # Skip packages whose integration reported errors this run --
+            # a file that failed to re-deploy would look stale and get
+            # wrongly deleted.
+            if diagnostics.count_for_package(dep_key, "error") > 0:
+                continue
+
+            prev_dep = existing_lockfile.get_dependency(dep_key)
+            if not prev_dep:
+                continue  # new package this install -- nothing stale yet
+            stale = detect_stale_files(prev_dep.deployed_files, new_deployed)
+            if not stale:
+                continue
+
+            cleanup_result = remove_stale_deployed_files(
+                stale, project_root,
+                dep_key=dep_key,
+                # `_targets or None` mirrors the pre-refactor behavior: when
+                # no targets were resolved (e.g. unknown runtime), pass None
+                # so the cleanup helper falls back to scanning all
+                # KNOWN_TARGETS rather than skipping cleanup entirely.
+                # The chokepoint at integration/cleanup.py applies its own
+                # path-safety gates regardless of which target set is used.
+                targets=_targets or None,
+                diagnostics=diagnostics,
+                recorded_hashes=dict(prev_dep.deployed_file_hashes),
+            )
+            # Re-insert failed paths so the lockfile retains them for
+            # retry on the next install.
+            new_deployed.extend(cleanup_result.failed)
+            if cleanup_result.deleted_targets:
+                BaseIntegrator.cleanup_empty_parents(
+                    cleanup_result.deleted_targets, project_root
+                )
+            for _skipped in cleanup_result.skipped_user_edit:
+                if logger:
+                    logger.cleanup_skipped_user_edit(_skipped, dep_key)
+            if logger:
+                logger.stale_cleanup(dep_key, len(cleanup_result.deleted))