Skip to content

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

Merged
danielmeppiel merged 28 commits intomainfrom
refactor/install-modularization
Apr 19, 2026
Merged

refactor(install): modularize install.py into engine package#764
danielmeppiel merged 28 commits intomainfrom
refactor/install-modularization

Conversation

@danielmeppiel
Copy link
Copy Markdown
Collaborator

Summary

Decompose commands/install.py from a 2905-line module (containing one 1444-line god-function) into a coherent apm_cli/install/ engine package. 56% LOC reduction in commands/install.py (2905 → 1268), zero behavioral changes, zero test regressions.

Builds on #762 (the security/UX hardening); preserves the cleanup chokepoint integration/cleanup.py.remove_stale_deployed_files invariant.

Why

install.py had become the largest and most error-prone module in the codebase. The single _install_apm_dependencies function held ~40 implicit shared locals across resolution, target detection, downloading, integration, cleanup, lockfile assembly, and result rendering, making targeted changes (like the recent #762 cleanup hardening) high-risk and hard to review.

What changed

New engine package (src/apm_cli/install/)

apm_cli/install/
  context.py             InstallContext dataclass (data backbone)
  validation.py          _validate_and_add_packages_to_apm_yml
  helpers/security_scan.py
  phases/
    resolve.py           lockfile load + auth + BFS resolution + --only filtering
    targets.py           target detection + scope + integrator init
    download.py          ThreadPoolExecutor parallel pre-fetch
    integrate.py         per-package sequential integration loop
    cleanup.py           orphan + intra-package stale (routes through #762 chokepoint)
    lockfile.py          LockfileBuilder (7 cohesive methods) + compute_deployed_hashes
    local_content.py     root primitives + local-path copy helpers
    finalize.py          stats + result construction
  presentation/dry_run.py

Thinned adapter

commands/install.py: 2905 → 1268 LOC. Now a Click adapter that re-exports engine symbols (preserving the @patch("apm_cli.commands.install.X") contract used by ~30 test sites) and orchestrates InstallContext through the phases.

Architectural invariant guard

New tests/unit/install/test_architecture_invariants.py enforces a 1000 LOC budget per module under apm_cli/install/ (with documented exceptions: 900 for phases/integrate.py, 1500 for legacy commands/install.py). Prevents future re-bloat.

How (key design decisions)

  1. Engine sibling, not commands/install/ package. Turning commands/install.py into a package would name-collide and force rewriting all @patch("apm_cli.commands.install.X") sites. Keeping the Click adapter as a single .py that re-exports from apm_cli/install/ preserves all test patches verbatim.

  2. Module-attribute access pattern (_install_mod.X). When a phase needs to call code that tests patch via apm_cli.commands.install.X, it imports the module (from apm_cli.commands import install as _install_mod) and references _install_mod.X so monkeypatching at the patched namespace flows through. Used for _integrate_package_primitives, _rich_success/_rich_error, _copy_local_package, _pre_deploy_security_scan, GitHubPackageDownloader. AST-verified zero bare-name calls to patched symbols in phases/integrate.py.

  3. InstallContext is the data backbone. Each phase extends it with the fields it owns. Mutable dict/list fields (package_deployed_files, etc.) carry mutations across phase boundaries (cleanup mutates lists that LockfileBuilder reads). All defaults via field(default_factory=...) to prevent shared-default bugs.

  4. fix(install): harden stale-file cleanup with per-file content-hash provenance (#666 follow-up) #762 invariant preserved. All cleanup paths still flow through integration/cleanup.py.remove_stale_deployed_files (3 safety gates intact). Security review explicitly confirmed; 16/16 cleanup helper tests + 32 cleanup/orphan integration tests green.

Phased execution (13 commits)

  • P0 — engine package skeleton + InstallContext stub
  • P1.L1-L4 — leaf extractions (validation, local_content, lockfile/_hash_deployed, security_scan)
  • P2.S1-S6 — sequential extractions of resolve+targets, download, integrate (largest, 715 LOC moved), cleanup, dry-run, lockfile builder + finalize
  • P3.R1 — cli-logging-ux hardening (fixed 2 BLOCKERS: Unicode box-drawing chars in security_scan.py and resolve.py that would crash Windows cp1252 terminals; 3 other ASCII fixes)
  • P3.R2 — activated architectural invariant tests with realistic budgets
  • P3.R3 — 3-agent parallel review (architect + cli-logging-ux + security)

Review verdict

Final 3-agent parallel review:

  • Python architect: APPROVE WITH FOLLOW-UPS — clean layering, cohesive phases, faithful extraction (spot-checked _hash_deployed, _copy_local_package, integration loop), test-patch contract airtight
  • CLI logging UX: APPROVE WITH FOLLOW-UPS — 5 non-blocking polish items, all 2 BLOCKERS fixed in P3.R1
  • Security: APPROVE WITH FOLLOW-UPS — fix(install): harden stale-file cleanup with per-file content-hash provenance (#666 follow-up) #762 chokepoint preserved, no token leakage, no path-safety regressions

Latent bugs surfaced (preserved per faithful-extraction contract)

  1. download.py:81-86 — HEAD-skip path swallows GitPython ImportError/corruption silently (perf-only, preserved verbatim from original)
  2. dry_run.pyonly_packages NameError in dry-run orphan preview when lockfile exists — fixed at extraction time (helper now defaults to None)

To document in CHANGELOG before merge.

Documented technical debt

  • phases/integrate.py at 861 LOC — contains 4 per-package code paths (alias / pre-downloaded / fresh download / local) that share fall-through state. Listed in KNOWN_LARGE_MODULES with 900 LOC budget. Decomposition into private _integrate_local_dep / _integrate_cached_dep / _integrate_fresh_dep helpers deferred to follow-up PR.

Follow-up PRs identified by reviewers

Item Source Priority
F1 — Decompose integrate.py per-package paths architect HIGH
F2 — Extract pipeline.py orchestrator from _install_apm_dependencies architect MEDIUM
F3 — Fold local-content integration in Click handler into engine pipeline architect LOW
F4 — UX polish (5 items: elif verbose fallbacks, security_scan tree-item method, validation/local_content logger threading, comment ASCII cleanup) cli-logging-ux LOW
F5 — cleanup.py:130 _targets or None widening review security LOW

Test results

  • 3974/3974 unit tests green
  • 102 targeted install/cleanup tests green
  • 32 cleanup/orphan integration tests green
  • 4 pre-existing failures on main confirmed unrelated

Test commands

uv run pytest tests/unit tests/test_console.py -x --no-header -q
uv run pytest tests/integration/test_intra_package_cleanup.py tests/unit/test_prune_command.py tests/unit/test_orphan_detection.py -x --no-header -q

Co-authored-by: Copilot 223556219+Copilot@users.noreply.github.com

danielmeppiel and others added 13 commits April 19, 2026 10:54
Sets up the structural foundation for decomposing
`commands/install.py` (2905 LOC, with one 1444-line function) into a
proper engine package with explicit phase boundaries and a typed shared
context.

Adds:
- `src/apm_cli/install/` engine package with phases/, helpers/,
  presentation/ subpackages.
- `InstallContext` dataclass stub. Fields are added incrementally as
  phases are extracted, turning the legacy implicit lexical scope into
  explicit, audit-friendly data flow.
- `tests/unit/install/test_architecture_invariants.py` to pin the
  structural contract. The 500-LOC budget guard is staged (skipped)
  until extraction completes in P3.R2.

Why a sibling package instead of `commands/install/`: the existing
module is heavily monkeypatched (~30 `@patch("apm_cli.commands.install.X")`
sites). Turning it into a package would create a name collision and
force test rewrites or late-lookup gymnastics. The Click adapter at
`commands/install.py` will stay a single module that re-exports engine
symbols, preserving every existing test patch verbatim.

This commit is import-only -- no behaviour change. Targeted install
tests remain green (173 tests).

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Moves three leaf validation helpers (package-existence checks, local-path
diagnostics) into the install engine package as
`apm_cli/install/validation.py`.  These functions had zero coupling to
the rest of install.py -- they take packages and return booleans or
strings -- so this is a pure relocation.

Functions moved:
- _validate_package_exists
- _local_path_failure_reason
- _local_path_no_markers_hint

`_validate_and_add_packages_to_apm_yml` remains in commands/install.py
because it calls _validate_package_exists and _local_path_failure_reason
via module-level name lookup, and 30+ tests patch
`apm_cli.commands.install._validate_package_exists` to intercept those
calls.  Keeping the orchestrator co-located with the re-exported names
preserves all @patch targets without any test modifications.

`commands/install.py` re-exports the three moved names so existing test
patches keep working verbatim.  Targeted install tests (175) + full unit
suite (3972) green.

install.py LOC: 2905 -> 2630.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Moves local-content leaf helpers (root project's .apm/ as implicit
local package per #714, local-path dependencies from apm.yml) into
`apm_cli/install/phases/local_content.py`.  Three of the four planned
functions form a coherent feature with no coupling to the integration
pipeline -- they take a project root and return booleans or copy files.

Functions moved:
- _project_has_root_primitives
- _has_local_apm_content
- _copy_local_package

Function KEPT in commands/install.py:
- _integrate_local_content  (calls _integrate_package_primitives via
  bare-name lookup; 5 tests patch
  apm_cli.commands.install._integrate_package_primitives to intercept
  that call -- same L1 lesson as _validate_and_add_packages_to_apm_yml)

`commands/install.py` re-exports the three moved names so existing
callers (_install_apm_dependencies) and any future @patch targets
keep working.  Targeted install tests (196) + full unit suite (3972)
green.

install.py LOC: 2630 -> 2562.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…h_deployed

Moves the per-file content-hash helper (added in #762) into the
install engine package as `apm_cli/install/phases/lockfile.py`,
alongside a `LockfileBuilder` class skeleton. The bulk of lockfile
assembly currently lives inline inside `_install_apm_dependencies`;
P2.S6 will fold that into LockfileBuilder. This commit relocates
only the leaf helper to keep the change small and the test patches
stable.

`commands/install.py` aliases the new function back to
`_hash_deployed` so the regression test pinned in #762
(`test_hash_deployed_is_module_level_and_works`) keeps working
without modification.

install.py LOC: 2562 -> 2547.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Moves `_pre_deploy_security_scan` into the install engine package as
`apm_cli/install/helpers/security_scan.py`. The helper has a clean
single responsibility (run the MCP scanner before any deploy) and
no coupling to the rest of install.py.

Note: the plan also called for a gitignore helper extraction, but
`_update_gitignore_for_apm_modules` already lives in
`commands/_helpers.py`. L4 reduces to security_scan only.

`commands/install.py` re-exports the moved name so
`tests/unit/test_install_scanning.py`'s
`from apm_cli.commands.install import _pre_deploy_security_scan`
keeps working without modification.

install.py LOC: 2547 -> 2513. P1 (leaf extractions) complete.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Extract the resolve and targets phases from _install_apm_dependencies
into dedicated modules under src/apm_cli/install/phases/.

InstallContext extended with all fields for resolve + targets outputs.
The mega-function now delegates to phase.run(ctx) and reads results
back at the seam so the remaining ~1100 lines are untouched.

install.py body: 2513 → 2282 LOC (−231)
New: phases/resolve.py 313 LOC, phases/targets.py 100 LOC

Rubber-duck findings (1 critical, 1 medium, 1 low):

  CRITICAL – Fixed: GitHubPackageDownloader test patch bypass.
  resolve.py imported the class by name (from apm_cli.deps.github_downloader
  import GitHubPackageDownloader), so tests patching
  apm_cli.commands.install.GitHubPackageDownloader were silent no-ops.
  Fix: resolve.py now uses module-attribute access (_ghd_mod.GitHubPackageDownloader);
  10 test patches updated to canonical path apm_cli.deps.github_downloader.

  MEDIUM – Noted: try/except boundary shift. Resolution errors now propagate
  raw instead of being wrapped in 'Failed to resolve APM dependencies: ...'.
  No test asserts on the wrapped message. External callers may need updating.

  LOW – Preserved: callback_failures latent bug (dict assignment on set at
  original line 1182). Faithfully extracted; filed for follow-up.

Verification gates (all green):
  1. Import check: phases load cleanly
  2. Targeted suite: 90 passed
  3. Full unit suite: 4528 passed (5 pre-existing failures excluded)
  4. Integration suite: 87 passed
Extract the Phase 4 (#171) parallel pre-download block from
_install_apm_dependencies into src/apm_cli/install/phases/download.py.

The phase reads ctx.deps_to_install, ctx.existing_lockfile, ctx.update_refs,
ctx.parallel_downloads, ctx.apm_modules_dir, ctx.downloader, and
ctx.callback_downloaded (all already on InstallContext from S1).

New InstallContext fields added:
  - pre_download_results: Dict[str, Any]  (dep_key -> PackageInfo)
  - pre_downloaded_keys: Set[str]

install.py retains bridge aliases (_pre_download_results,
_pre_downloaded_keys) that read from ctx so downstream code at the
sequential integration loop (~lines 1483, 1767-1768) is untouched.

concurrent.futures import removed from install.py (no longer used there).
rich.progress imports in download.py are local to the if-block.

install.py LOC: 2282 -> 2211 (-71)
New: phases/download.py 132 LOC

Rubber-duck findings:
  CLEAN -- No test patches invalidated (zero patches target download symbols
  at apm_cli.commands.install.X; detect_ref_change/build_download_ref are
  imported from apm_cli.drift in both old and new code).
  CLEAN -- Progress UI lifecycle preserved (transient context manager).
  CLEAN -- Error-swallowing semantic preserved (except Exception: silent).
  CLEAN -- parallel_downloads=0 skip path works correctly.
  LATENT BUG (preserved, not fixed) -- HEAD-skip path silently swallows
  GitPython ImportError/corruption, causing unnecessary re-downloads.

Verification gates (all green):
  1. Import check: download.py + install.py load cleanly
  2. Targeted suite: 102 passed, 1 skipped
  3. Full unit suite: 3972 passed, 1 skipped
  4. Cleanup/prune/orphan suite: 32 passed

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Seam: lines 1290-2004 of install.py -> install/phases/integrate.py

Extract the ~700 LOC sequential integration loop into
src/apm_cli/install/phases/integrate.py as a single run(ctx) function.
This covers per-dependency integration (local copy, cached, fresh download)
and root-project primitives integration (#714).

install.py shrinks from 2211 to 1511 lines (-700).

New InstallContext fields (context.py):
  Pre-integrate inputs:  diagnostics, registry_config, managed_files,
                         installed_packages
  Integrate outputs:     installed_count, unpinned_count,
                         total_prompts_integrated, total_agents_integrated,
                         total_skills_integrated, total_sub_skills_promoted,
                         total_instructions_integrated,
                         total_commands_integrated, total_hooks_integrated,
                         total_links_resolved

Test-patch contract preserved:
  _integrate_package_primitives (4 call sites), _rich_success,
  _rich_error, _copy_local_package, _pre_deploy_security_scan
  all accessed via _install_mod.X indirection.

Rubber-duck review: all 8 questions CLEAN -- no test-patch bypass,
no NameError, exception handling preserved, int counter write-back
correct, dependency_graph read-only, lazy imports preserved.

Test gates: 3972 unit + 32 integration passed.

Co-authored-by: Daniel Meppiel <dmeppiel@microsoft.com>
Seam: lines 1310-1399 of install.py -> install/phases/cleanup.py

Extract the orphan-cleanup and intra-package stale-file cleanup
blocks into src/apm_cli/install/phases/cleanup.py as a single
run(ctx) function. This is a faithful extraction -- no behavioural
changes.

Block A (orphan cleanup): iterates existing_lockfile.dependencies,
for each key NOT in intended_dep_keys calls
remove_stale_deployed_files(targets=None, failed_path_retained=False).
Then cleanup_empty_parents and logger.orphan_cleanup.

Block B (stale-file cleanup): iterates package_deployed_files.items(),
compares prev vs new deployed, calls remove_stale_deployed_files(
targets=_targets or None). Re-inserts failed paths back into
new_deployed (mutation persists to ctx for lockfile assembly).

PR #762 security chokepoint invariant PRESERVED:
  - Every file-system deletion flows through
    apm_cli.integration.cleanup.remove_stale_deployed_files
  - All kwargs preserved exactly (targets, recorded_hashes defensive
    copies, failed_path_retained=False for orphans, omitted for
    intra-package)
  - No cleanup logic inlined, simplified, or bypassed

install.py shrinks from 1511 to 1426 lines (-85).
cleanup.py: 143 lines.
No new InstallContext fields required (all already existed from S3).

Rubber-duck review: 10/10 PASS -- all call sites, kwargs, defensive
copies, mutation semantics, guard conditions, and test-patch contract
verified clean.

Test update: test_orphan_loop_uses_manifest_intent_not_integration_outcome
now inspects apm_cli.install.phases.cleanup (where the code moved)
instead of apm_cli.commands.install. Structural assertion unchanged.

Test gates: 3972 unit + 2 integration passed, 1 skipped.

Co-authored-by: Daniel Meppiel <dmeppiel@microsoft.com>
Seam: lines 525-581 of install.py -> install/presentation/dry_run.py

Extract the dry-run preview block into a standalone render_and_exit()
function in src/apm_cli/install/presentation/dry_run.py. This is a
faithful extraction -- no behavioural changes.

The block renders:
  - APM dependency list with install/update action labels
  - MCP dependency list
  - "No dependencies found" fallback
  - Orphan preview via LockFile.read + detect_orphans
  - Per-package stale-file cleanup caveat (dry_run_notice)
  - Final "Dry run complete" success message

The function does NOT return/exit -- the caller is responsible for
the `return` after calling render_and_exit(), keeping responsibility
separation clean.

Latent bug fix (surfaced by extraction): the original block referenced
`only_packages` which was never defined in the install() Click handler
scope. This would have caused a NameError if a lockfile existed during
dry-run. The helper defaults only_packages=None, which is the correct
value for dry-run context (no partial-install filtering).

builtins.set() preserved for safety -- matches the original extraction
site where set may be shadowed in the enclosing scope.

install.py shrinks from 1426 to 1384 lines (-42).
dry_run.py: 93 lines.
No test patches affected (verified via grep).

Rubber-duck review: all inputs verified, orphan preview try/except
preserved, dry_run_notice condition preserved, function correctly
does not return.

Test gates: 3972 unit passed, 1 skipped.

Co-authored-by: Daniel Meppiel <dmeppiel@microsoft.com>
…er + extract finalize

Seam A: lines 1274-1347 of install.py -> LockfileBuilder.build_and_save()
Seam B: lines 1349-1377 of install.py -> install/phases/finalize.py

LockfileBuilder (src/apm_cli/install/phases/lockfile.py):
  The skeleton class from P1.L3 now has a full build_and_save() entry
  point that decomposes the 70-line inline lockfile assembly block into
  7 private methods:
    _attach_deployed_files   - per-dep deployed_files + on-disk hashes
    _attach_package_types    - per-dep package_type metadata
    _attach_content_hashes   - sha256 captured at download/verify time
    _attach_marketplace_provenance - discovered_via + plugin_name
    _merge_existing          - selective merge from prior lockfile
    _maybe_merge_partial     - partial-install merge (apm install <pkg>)
    _write_if_changed        - semantic-equivalence guard + save

  All logic is verbatim from the original block; only the variable
  access pattern changed (self.ctx.X instead of bare locals).

  Rubber-duck findings:
    - _attach_* order preserved (deployed_files before hashing -- OK)
    - existing_lockfile RE-READ in _write_if_changed is a FRESH disk
      read via a local variable, NOT ctx.existing_lockfile (the snapshot
      from resolve phase) -- intentional and preserved
    - try/except wraps build_and_save(); _handle_failure() preserves
      the non-fatal diagnostics.error + logger.error pattern

finalize.py (src/apm_cli/install/phases/finalize.py):
  55-line module with run(ctx) -> InstallResult.  Preserves:
    - 4 separate "if X > 0" verbose stat blocks (no refactor)
    - "if not logger" bare-success fallback via _install_mod indirection
    - unpinned-dependency warning
    - InstallResult(4 positional args) constructor call

  _rich_success routed through _install_mod so test patches at
  apm_cli.commands.install._rich_success remain effective.

Bridge locals removed: the 17 reassignment lines that bridged integrate
phase outputs back into bare locals are now dead (all downstream code
reads from ctx directly) and have been deleted.

install.py: 1384 -> 1268 LOC (-116)
lockfile.py: 62 -> 182 LOC (+120)
finalize.py: 0 -> 55 LOC (new)

Co-authored-by: copilot-chat <copilot-chat@github.com>
Replace the skipped 500-LOC budget test with two active guards:

- test_no_install_module_exceeds_loc_budget: 1000 LOC default per-file
  budget under apm_cli/install/, with a per-file override of 900 LOC for
  phases/integrate.py. Integrate.py's 4 per-package code paths are clear
  natural seams for a follow-up decomposition; this test will tighten
  once those land. The KNOWN_LARGE_MODULES dict makes the technical debt
  explicit so it doesn't silently grow.

- test_install_py_under_legacy_budget: commands/install.py budget at
  1500 LOC (started this refactor at 2905, ended P2 at 1268). New logic
  must go into apm_cli/install/ phase modules, not back into the legacy
  seam.

Both tests pass at the current actuals.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- security_scan.py: replace Unicode box-drawing chars (U+2514, U+2500) in
  logger.progress() string literals with ASCII "|--" tree connectors
- resolve.py: replace \u2514\u2500 Python escape sequences in _rich_error()
  f-string with ASCII "|--" (runtime output was Unicode on cp1252 terminals)
- download.py: replace \u2014 (em dash) and \u2192 (right arrow) literal
  escape text in comments with ASCII "--" and "->"
- local_content.py: replace Unicode right arrow (U+2192) in comment with
  ASCII "->"
- validation.py: replace Unicode em dash (U+2014) in comment with ASCII "--"

All changes are encoding-only (ASCII compliance per encoding.instructions.md).
No behavioural or output-semantic changes.

Co-authored-by: GitHub Copilot <copilot@github.com>
Copilot AI review requested due to automatic review settings April 19, 2026 10:53
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Refactors the apm install implementation by extracting the install pipeline out of src/apm_cli/commands/install.py into a new src/apm_cli/install/ engine package with phase modules and a shared InstallContext.

Changes:

  • Introduces apm_cli/install/ engine package (context + phases for resolve/targets/download/integrate/cleanup/lockfile/finalize).
  • Thins commands/install.py into an adapter that re-exports key helpers and orchestrates the engine phases.
  • Updates tests to align with the refactor (including new architecture invariants test and updated patch targets).
Show a summary per file
File Description
tests/unit/test_install_command.py Updates downloader patch target used by unit tests.
tests/unit/integration/test_cleanup_helper.py Points invariant/marker test at new cleanup phase module source.
tests/unit/install/test_architecture_invariants.py Adds LOC-budget invariant tests for the new install engine modules.
tests/unit/install/init.py Adds unit test package marker for install tests.
tests/integration/test_selective_install_mcp.py Updates downloader patch target used by integration tests.
src/apm_cli/install/init.py Adds top-level documentation for the new install engine package.
src/apm_cli/install/context.py Introduces InstallContext dataclass shared across phases.
src/apm_cli/install/validation.py Extracts manifest/package validation helpers from the legacy install module.
src/apm_cli/install/helpers/init.py Declares install helpers package.
src/apm_cli/install/helpers/security_scan.py Extracts pre-deploy security scan helper.
src/apm_cli/install/presentation/init.py Declares install presentation package.
src/apm_cli/install/presentation/dry_run.py Extracts --dry-run rendering logic into a presentation module.
src/apm_cli/install/phases/init.py Declares phases package.
src/apm_cli/install/phases/resolve.py Implements lockfile loading + auth + dependency resolution + --only filtering.
src/apm_cli/install/phases/targets.py Implements target detection and integrator initialization.
src/apm_cli/install/phases/download.py Implements parallel pre-download via ThreadPoolExecutor.
src/apm_cli/install/phases/integrate.py Implements sequential per-package integration loop and root .apm/ integration.
src/apm_cli/install/phases/cleanup.py Implements orphan + stale cleanup routed through the security chokepoint helper.
src/apm_cli/install/phases/lockfile.py Implements lockfile building/writing and deployed-file hash computation.
src/apm_cli/install/phases/finalize.py Implements final stats rendering + fallback success output + result construction.
src/apm_cli/commands/install.py Re-exports extracted helpers and orchestrates the new install engine phases.

Copilot's findings

  • Files reviewed: 21/22 changed files
  • Comments generated: 6

Comment thread src/apm_cli/install/phases/resolve.py Outdated
Comment thread src/apm_cli/install/context.py Outdated
Comment thread src/apm_cli/commands/install.py
Comment thread src/apm_cli/install/phases/cleanup.py Outdated
Comment thread src/apm_cli/install/validation.py Outdated
Comment thread tests/unit/test_install_command.py
danielmeppiel and others added 7 commits April 19, 2026 12:59
Per architect+security review of #764: the `_targets or None` widening at
phases/cleanup.py:130 mirrors pre-refactor behavior. Add a comment so future
maintainers don't 'fix' it. Behavior unchanged.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…, ASCII cleanup)

F4a: Remove redundant elif-verbose fallback in resolve.py download_callback;
     logger is the single output path, deferred diagnostics cover the rest.
F4b: Use logger.tree_item() instead of logger.progress() for security scan
     sub-items -- correct semantic: continuation lines, not progress events.
F4c: Thread optional logger param through validation.py so
     _validate_package_exists and _local_path_no_markers_hint route through
     CommandLogger when available, falling back to raw _rich_* calls.
F4d: Thread optional logger param through local_content._copy_local_package
     so error messages route through CommandLogger. Updated call sites in
     resolve.py and integrate.py.
F4e: Replace 8 non-ASCII characters (em dashes U+2014, arrows U+2192) in
     source-code comments with ASCII equivalents (-- and ->).

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Extract the 3 per-package code paths and root-project integration from
the monolithic run() function into private module-level helpers:

- _resolve_download_strategy(): preamble computing cache/download decision
- _integrate_local_dep(): local filesystem package integration
- _integrate_cached_dep(): cached/pre-downloaded package integration
- _integrate_fresh_dep(): fresh download + integration
- _integrate_root_project(): root project .apm/ primitives (#714)

run() is now a slim orchestrator (~143 LOC) that iterates deps_to_install,
dispatches to the correct helper, and accumulates counters.

File total: 978 LOC (under 1000 standard budget).
KNOWN_LARGE_MODULES exception for phases/integrate.py removed.

All helpers honour the _install_mod.X test-patch contract.
Mutable ctx containers are shared by reference (no local copies).
Int counters use delta-dict return pattern.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Move _install_apm_dependencies orchestration (~237 LOC) from
commands/install.py into apm_cli/install/pipeline.py as
run_install_pipeline().  The Click command module drops from
1268 LOC to 1072 LOC.

A thin wrapper _install_apm_dependencies() remains in
commands/install.py to preserve the patch path used by 15+
test files.  All interstitial code (DiagnosticCollector setup,
registry config, managed_files initialization) moves with the
orchestration into the pipeline module.

Phase modules continue accessing patchable symbols through the
existing _install_mod indirection pattern -- no changes needed.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Move the ~140 LOC "local .apm/ content integration" block from the Click
install() handler into the install pipeline, eliminating the duplicate
target resolution and integrator initialization identified in PR #764.

Changes:
- Rewrite _integrate_root_project in phases/integrate.py to delegate to
  _integrate_local_content (preserves test-patch contract and correct
  PackageType.APM_PACKAGE for root SKILL.md handling)
- Create phases/post_deps_local.py for stale cleanup + lockfile
  persistence of local_deployed_files (routes through
  integration/cleanup.py per #762)
- Extend InstallContext with old_local_deployed, local_deployed_files,
  and local_content_errors_before fields
- Extend pipeline early-exit to consider old_local_deployed (stale
  cleanup must run even when .apm/ is removed)
- Remove the 140-line inline block from commands/install.py Click handler

commands/install.py: 1072 -> 933 LOC (-139)

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…d logger to local copy

UX re-review found 2 logger-pattern inconsistencies introduced by F1/F3:
- phases/cleanup.py: 4 unguarded logger.X() calls (orphan_cleanup,
  cleanup_skipped_user_edit, stale_cleanup) -- every other phase guards
  with 'if logger:' since ctx.logger defaults to None.
- phases/integrate.py:204: _copy_local_package called without logger= in
  the local-dep helper, breaking F4d's single-output-path invariant for
  one of the two callsites.

Both produce correct output today (pipeline always passes a logger; the
fallback path uses _rich_*) but the inconsistency is fragile for future
callers and tests. Aligning all phases on the same guard pattern.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…cleanup hash bug-fix

Per architect re-review of #764: F3 closes a latent bug where local .apm/
stale-cleanup was reading the lockfile after regeneration, losing
local_deployed_file_hashes and silently skipping the user-edit gate. Worth
calling out distinctly from the broader refactor entry.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@danielmeppiel
Copy link
Copy Markdown
Collaborator Author

Follow-ups F1-F5 added (commits 2f82f2b..0e649ff)

All five architectural follow-ups identified by the reviewers have been addressed in this PR rather than deferred:

ID What Result
F1 Decompose integrate.py per-package paths 5 helpers extracted (_resolve_download_strategy, _integrate_local_dep, _integrate_cached_dep, _integrate_fresh_dep, _integrate_root_project); run() reduced from 824 to 143 LOC
F2 Extract pipeline.py orchestrator New apm_cli/install/pipeline.py (306 LOC); commands/install.py 1268 -> 1072
F3 Fold local-content integration into pipeline New phases/post_deps_local.py (122 LOC); eliminated duplicate target resolution + integrator init from Click handler; commands/install.py 1072 -> 933
F4 UX polish (5 items: logger plumbing, tree-item helper, ASCII cleanup) All 5 sub-items landed
F5 Document _targets or None fallback in cleanup phase Comment added

Final size

commands/install.py: 2905 -> 933 LOC (68% reduction)

Bonus bug-fix surfaced by F3

F3 fixed a latent bug in local .apm/ stale-cleanup: the lockfile was being read after regeneration (which doesn't write local_deployed_file_hashes), so the user-edit safety gate received empty hashes and was silently skipped. Now reads ctx.existing_lockfile (pre-install snapshot). Documented in CHANGELOG.

Re-review (3 agents in parallel)

  • Architect: APPROVE WITH FOLLOW-UPS (CHANGELOG entry added; further integrate.py decomposition deferred since it's now back at 1013/1020 budget after F3 added _integrate_root_project rewrite)
  • CLI logging UX: APPROVE WITH FOLLOW-UPS (2 logger-guard inconsistencies found and fixed in commit 3914f69)
  • Security: APPROVE — chokepoint preserved across all new commits, F3 closes a real security gap

Tests

  • 3974/3974 unit tests pass
  • 32/32 integration tests pass
  • 4/4 architectural invariant tests pass
  • Zero non-ASCII characters across src/apm_cli/install/ and commands/install.py

danielmeppiel and others added 5 commits April 19, 2026 14:21
…ces.py

Move `_integrate_package_primitives` and `_integrate_local_content` from
the Click command module to a new `apm_cli/install/services.py` so the
install engine package owns its own integration template.

`commands/install` keeps both name forms re-exported for backward
compatibility with external callers and the 55 healthy `@patch` sites.
The 5 `@patch("apm_cli.commands.install._integrate_package_primitives")`
sites in `test_local_content_install.py` now patch the canonical
`apm_cli.install.services.integrate_package_primitives` directly,
because services.`_integrate_local_content` calls services.`integrate_package_primitives`
by bare name -- a re-export aliased on `commands/install` would not be
intercepted from inside services.

This is the first commit of the design-patterns refactor (Strategy + DI +
Application Service) following the install-modularization PR.  Behaviour
preserved.  3974 unit tests green.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…ct imports

Eliminates the legacy module-attribute indirection
`from apm_cli.commands import install as _install_mod` inside
`install/phases/integrate.py`.  Collaborators are now imported directly
from their canonical locations:

- integrate_package_primitives, integrate_local_content
  from apm_cli.install.services
- _pre_deploy_security_scan from apm_cli.install.helpers.security_scan
- _copy_local_package from apm_cli.install.phases.local_content
- _rich_success, _rich_error from apm_cli.utils.console

The four per-source helpers (_integrate_local_dep, _integrate_cached_dep,
_integrate_fresh_dep, _integrate_root_project) no longer take an
`_install_mod: Any` parameter, and the four matching call sites in
`run()` are simplified accordingly.  The 12 `_install_mod.X`
references are replaced by direct names.

This is the DI seam that subsequent phases (Strategy + Application
Service) will consume.  The 5 healthy test patches that previously
needed `_install_mod` indirection now point at
`apm_cli.install.services.integrate_package_primitives`
(updated in P1.1).  All 3974 unit tests pass.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…ation Template Method

Replaces the three per-source helpers in `install/phases/integrate.py`
(`_integrate_local_dep`, `_integrate_cached_dep`, `_integrate_fresh_dep`,
~600 LOC of overlapping code) with two new modules:

- `install/sources.py` -- `DependencySource` ABC + three concrete
  Strategy implementations (`LocalDependencySource`,
  `CachedDependencySource`, `FreshDependencySource`) plus a
  `make_dependency_source(...)` factory.  Each source encapsulates
  acquisition (copy / cache reuse / network download with supply-chain
  hash verification) and returns a `Materialization` describing the
  prepared package.
- `install/template.py` -- `run_integration_template(source)` Template
  Method.  After `acquire()`, every source funnels through the same
  flow: pre-deploy security scan, primitive integration, deployed-files
  tracking, per-package verbose diagnostics.

Root-project integration (`<project_root>/.apm/`) remains a sibling
helper (`_integrate_root_project`) because its shape is structurally
distinct (no `PackageInfo`, dedicated `ctx.local_deployed_files`
tracking, different downstream cleanup semantics).

`integrate.py:run()` now reads as the orchestration it always was:
build the right source, run the template, accumulate counters.  Module
LOC drops from 1001 to 402, and the `KNOWN_LARGE_MODULES` exception is
removed -- integrate.py is now well under the default 1000-LOC budget.

All 3974 unit tests pass; no behavioural changes.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
… with typed InstallRequest

Adds the Application Service pattern as the typed entry point for the
install pipeline.  Adapters (the Click handler today; programmatic /
API callers tomorrow) build a frozen InstallRequest and call
InstallService.run(request) -> InstallResult.

* New install/request.py: frozen dataclass replacing the 11-kwarg ad-hoc
  parameter list with a typed, immutable record.
* New install/service.py: InstallService class wrapping the pipeline.
  Stateless and reusable; documented as the future DI seam for
  collaborator injection (downloader, scanner, integrator factory).
* commands/install.py: _install_apm_dependencies re-export now builds an
  InstallRequest and delegates through InstallService.  All 55 existing
  test patches against the re-export keep working unchanged.
* tests/unit/install/test_service.py: 6 direct-invocation tests
  exercising the typed Request -> Result contract without CliRunner.

3980 unit tests pass (3974 prior + 6 new).

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Cheap polish from the post-P3 architect review (APPROVE_WITH_NITS):

* Strategy pattern leak fix: replace isinstance() switch in template.py
  exception handler with a per-source INTEGRATE_ERROR_PREFIX class
  attribute on DependencySource and overrides on Local/Cached.  The
  template now reads source.INTEGRATE_ERROR_PREFIX -- no more type
  switches in the supposedly-polymorphic dispatch.
* Remove redundant ctx.package_deployed_files write in
  CachedDependencySource.acquire() no-targets branch -- the template
  is the single source of truth for that bookkeeping.
* Drop vestigial 'if TYPE_CHECKING: pass' block in template.py.
* Drop unused 'field' import in install/request.py.
* Tighten test_service.py: use FrozenInstanceError instead of bare
  Exception; cover only_packages and marketplace_provenance round-trip;
  document the shallow-immutability gotcha with an explicit test.

3981 unit tests pass.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@danielmeppiel
Copy link
Copy Markdown
Collaborator Author

danielmeppiel commented Apr 19, 2026

Architect's Notes -- Full Refactor: main vs this PR

This comment supersedes the earlier one. The PR is bigger than just the design-patterns layer -- it ships a complete restructuring of the install subsystem: a 2905-LOC monolith on main becomes a 23-module engine package on the PR, with the design patterns built on top of that scaffolding.


main today

flowchart TD
    Cli["apm install (Click CLI)"] --> Mono["commands/install.py<br/>2905 LOC monolith"]
    Mono --> A["arg validation<br/>(inlined)"]
    Mono --> B["scope resolution<br/>(inlined)"]
    Mono --> C["dependency resolve loop<br/>(inlined)"]
    Mono --> D["target detection<br/>(inlined)"]
    Mono --> E["parallel download<br/>(inlined)"]
    Mono --> F["per-package integrate<br/>3 copy-pasted blocks"]
    Mono --> G["cleanup orchestration<br/>(inlined)"]
    Mono --> H["lockfile assembly<br/>(inlined)"]
    Mono --> I["dry-run rendering<br/>(inlined)"]
    Mono --> J["finalize stats<br/>(inlined)"]
    Mono --> K["security scan helper<br/>(inlined)"]
    style Mono fill:#fdd,stroke:#a00
Loading

Symptoms on main:

  • One file = one giant scope. Any change requires loading the whole file mentally.
  • 11 phases inlined as code blocks rather than callable units, so they cannot be tested independently of CliRunner.
  • 3 near-identical per-source integration blocks (~200 LOC each = ~600 LOC of duplication).
  • Module exceeds the 1000-LOC architectural-invariant budget; an explicit exception lives in KNOWN_LARGE_MODULES.
  • No typed contract between adapters (the Click handler) and the orchestrator.

This PR ships

flowchart TD
    Cli["apm install (Click CLI)"]
    Cli --> Thin["commands/install.py<br/>761 LOC<br/>(arg parsing + rendering only)"]
    Thin --> Req["InstallRequest<br/>(frozen dataclass)"]
    Thin --> Svc["InstallService.run(request)"]
    Svc --> Pipe["install/pipeline.py<br/>orchestrator"]

    subgraph Engine["install/ (engine package, 23 modules)"]
        Pipe
        Pipe --> Ctx["context.py<br/>InstallContext"]
        Pipe --> Val["validation.py"]
        Pipe --> Phases
        Pipe --> Pres["presentation/<br/>dry_run.py"]

        subgraph Phases["phases/ (7 phase modules)"]
            P1["resolve.py"]
            P2["targets.py"]
            P3["download.py"]
            P4["integrate.py<br/>402 LOC"]
            P5["local_content.py"]
            P6["post_deps_local.py"]
            P7["cleanup.py"]
            P8["lockfile.py"]
            P9["finalize.py"]
        end

        P4 --> Factory["make_dependency_source()"]
        Factory --> SrcStrat["sources.py<br/>(Strategy hierarchy)"]
        SrcStrat --> Tmpl["template.py<br/>(Template Method)"]
        Tmpl --> Helpers["helpers/<br/>security_scan.py"]
        Tmpl --> Svcs["services.py<br/>(DI seam)"]
    end

    Svc -->|returns| Res["InstallResult"]
    style Thin fill:#dfd,stroke:#070
    style Engine fill:#eef,stroke:#447
    style SrcStrat fill:#dfd,stroke:#070
    style Tmpl fill:#dfd,stroke:#070
    style Svc fill:#dfd,stroke:#070
Loading

What changed, by responsibility

Concern On main This PR
CLI handler size 2905 LOC monolith 761 LOC (arg parsing + rendering only)
Pipeline orchestration Inlined in handler install/pipeline.py (306 LOC)
Phases 9 inlined code blocks 9 modules under install/phases/
Per-source acquisition 3 copy-pasted blocks (~600 LOC) install/sources.py Strategy hierarchy (579 LOC, 0 duplication)
Post-acquire flow (security gate + integrate + diagnostics) Inlined 3x install/template.py Template Method (126 LOC, single source of truth)
Integration helper location On Click module; reached via _install_mod.X indirection install/services.py (DI seam, no reach-backs)
Pipeline entry contract 11-arg free function Frozen InstallRequest -> InstallService.run() -> InstallResult
Dry-run rendering Inlined in handler install/presentation/dry_run.py
Lockfile assembly Inlined install/phases/lockfile.py (LockfileBuilder)
Pre-deploy security scan Inlined helper install/helpers/security_scan.py
Validation (apm.yml bootstrap, package add) Inlined install/validation.py
KNOWN_LARGE_MODULES exception Required for commands/install.py Empty -- 1000-LOC budget enforced everywhere

How the PR was structured (commit phases)

Six layers, applied in a strict bottom-up order so each layer rests on solid ground:

flowchart LR
    P0["P0<br/>Skeleton"] --> PL["P1.L*<br/>Low-risk extracts<br/>(validation,<br/>local_content,<br/>LockfileBuilder,<br/>security scan)"]
    PL --> PS["P2.S*<br/>Phase-by-phase<br/>extraction<br/>(resolve, targets,<br/>download, integrate,<br/>cleanup, dry-run,<br/>finalize)"]
    PS --> PR["P3.R*<br/>Hardening<br/>(logging UX,<br/>arch invariants)"]
    PR --> F["F1-F5<br/>Follow-ups<br/>(integrate.py decompose,<br/>pipeline.py extract,<br/>local-content fold-in,<br/>UX polish)"]
    F --> DP["Design Patterns<br/>(this session)<br/>P1: DI seam<br/>P2: Strategy + Template<br/>P3: Application Service<br/>P4: nit fixes"]
Loading

The first four layers (P0 through F5) did the mechanical decomposition -- they took the monolith apart and laid the modules out by responsibility, but the resulting phases/integrate.py still had the 3-copy duplication and _install_mod.X indirection.

The final layer (the 5 commits at the head of this branch) introduced the named patterns that earned the structure: Strategy + Template Method killed the duplication; the DI seam killed the indirection; the Application Service gave the pipeline a typed contract.


Design patterns introduced (the top of the stack)

Strategy + Template Method (install/sources.py + install/template.py)

Replaces the 3 copy-pasted per-source helpers in the old phases/integrate.py.

classDiagram
    class DependencySource {
        <<abstract>>
        +ctx, dep_ref, install_path, dep_key
        +INTEGRATE_ERROR_PREFIX: str
        +acquire()* Materialization
    }
    class LocalDependencySource {
        +acquire() copies from filesystem
    }
    class CachedDependencySource {
        +acquire() loads cached APMPackage
    }
    class FreshDependencySource {
        +acquire() downloads + supply-chain hash check
    }
    class Materialization {
        <<dataclass>>
        +package_info, install_path, dep_key, deltas
    }
    DependencySource <|-- LocalDependencySource
    DependencySource <|-- CachedDependencySource
    DependencySource <|-- FreshDependencySource
    DependencySource ..> Materialization : returns
Loading
sequenceDiagram
    participant I as integrate.run()
    participant F as make_dependency_source
    participant S as Strategy
    participant T as run_integration_template
    participant SG as security_scan
    participant IP as integrate_package_primitives

    I->>F: factory(dep_ref, flags)
    F-->>I: concrete Strategy
    I->>T: run_integration_template(source)
    T->>S: source.acquire()
    S-->>T: Materialization
    T->>SG: _pre_deploy_security_scan
    SG-->>T: pass / block
    T->>IP: integrate_package_primitives
    IP-->>T: deltas + deployed_files
    T-->>I: deltas dict
Loading

Per-source error wording is dispatched via an INTEGRATE_ERROR_PREFIX class attribute on each subclass -- no isinstance switches in the template.

Application Service (install/service.py + install/request.py)

Replaces the 11-arg pipeline entry point with a typed Request -> Result contract.

flowchart LR
    Caller["Adapter<br/>(Click handler today,<br/>SDK / MCP tomorrow)"]
    Caller -->|builds| Req["InstallRequest<br/>(frozen dataclass)"]
    Caller -->|invokes| Svc["InstallService.run(request)"]
    Req --> Svc
    Svc -->|returns| Res["InstallResult<br/>(typed)"]
    Svc -.delegates to.-> Pipeline["run_install_pipeline()"]
Loading

The class is intentionally lean today (a thin delegator) but exists as the documented seam for future collaborator injection (downloader / scanner / integrator factories) without changing every call site.


Patterns at a glance

Pattern Module What it replaces Primary benefit
Pipeline / Phases install/pipeline.py + install/phases/* 9 inlined code blocks in commands/install.py Each phase is independently testable and replaceable
DI seam install/services.py _install_mod.X reach-backs Single-direction dependencies; no circular-import workarounds
Strategy install/sources.py 3 copy-pasted source handlers (~600 LOC) New source types are local additions; 0 duplication
Template Method install/template.py Inlined post-acquire blocks repeated 3x Single source of truth for security gate + integrate + diagnostics
Application Service install/service.py 11-arg free function Typed Request -> Result contract; Click-free testability
Value Object install/request.py, install/context.py, Materialization Untyped kwargs / tuples Immutable inputs; explicit module-boundary contracts
Simple Factory make_dependency_source() in sources.py if/elif source-type ladder Centralised construction; subclass leaf chooses defaults
Builder LockfileBuilder in phases/lockfile.py Inlined dict-assembly logic Lockfile assembly is reusable and testable in isolation

Numbers

Metric main This PR Delta
commands/install.py LOC 2905 761 -74%
Files in install/ package 0 (does not exist) 23 new
Largest module under install/ n/a sources.py 579 under 1000 budget
KNOWN_LARGE_MODULES exceptions 1 (commands/install.py) 0 budget enforced
Duplicated source-handler LOC ~600 0 eliminated
_install_mod.X reach-backs from phases 12+ 0 eliminated
Pipeline entry-point arg count 11 positional/keyword 1 typed InstallRequest typed
Unit tests 3974 3981 +7 (service tests)

Reviewer reading order

  1. install/__init__.py -- public surface of the engine package
  2. install/context.py -- the InstallContext value object that flows through phases
  3. install/pipeline.py -- top-level orchestration (read this to understand the phase order)
  4. install/phases/*.py -- one phase per module (read in pipeline order: resolve, targets, download, integrate, cleanup, lockfile, finalize)
  5. install/sources.py + install/template.py -- Strategy + Template Method (the design-patterns layer)
  6. install/services.py -- DI seam
  7. install/request.py + install/service.py -- Application Service entry point
  8. commands/install.py -- now thin: arg parsing, dry-run dispatch, InstallService call, result rendering, exit codes

Safety invariants verified by the team review

  • git diff main -- src/apm_cli/integration/cleanup.py is empty -- the fix(install): harden stale-file cleanup with per-file content-hash provenance (#666 follow-up) #762 hardened cleanup chokepoint is untouched
  • Pre-deploy security gate still runs before integrate_package_primitives for every source
  • Critical-security hard-fail (sys.exit(1)) preserved
  • Auth resolver propagates end-to-end (InstallRequest -> InstallService -> pipeline -> GitHubPackageDownloader)
  • ASCII-only source / output (Windows cp1252 safe)
  • 3981 unit tests pass; architectural invariants pass

Four targeted fixes from copilot-pull-request-reviewer feedback on the
modularization PR. Each is a real defect inherited from main but
surfaced by the new module boundaries; all four sit inside files we
own in this PR so they're appropriate to fix here.

1. resolve.py: callback_failures TypeError on local + user-scope
   callback_failures is initialized as a set (line 105) and used with
   .add() everywhere except the local-package + user-scope branch,
   which used dict-style assignment.  Would raise TypeError at runtime
   when a local dep was encountered under --global.  Switch to .add().
   The discarded message string was never consumed downstream
   (ctx.callback_failures is only iterated for counting).

2. context.py: incorrect Dict[str, Dict[...]] type hints
   package_types and package_hashes are typed as nested dicts but
   used as Dict[str, str] at all 6 write sites in sources.py.  Fix
   the annotations to match actual usage.  No runtime impact.

3. install.py: dry-run dropped only_packages filtering
   The dry-run renderer accepts only_packages= and forwards it to
   detect_orphans() for accurate orphan-preview filtering, but the
   call site never passed it.  Hoist the canonical only_pkgs
   computation before the dry-run branch and thread it through.  The
   actual install path now reuses the same hoisted variable.

4. validation.py: PAT leak in verbose ls-remote stderr
   The verbose-mode stderr scrub only replaced two env-var values, but
   git almost always echoes the failing URL in error messages, and the
   URL we built embeds _url_token (a per-dep PAT for GHES/ADO).
   Apply _sanitize_git_error() (the same scrubber the downloader
   uses for clone errors) before the env-value redaction.

All 3981 unit tests pass.  No behaviour change for the happy path;
fixes manifest only on rare branches that were latent bugs on main.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot's findings

  • Files reviewed: 30/32 changed files
  • Comments generated: 9

Comment thread src/apm_cli/install/pipeline.py Outdated
# Future S-phases will fold them into the context one by one.
# --------------------------------------------------------------
transitive_failures = ctx.transitive_failures
apm_modules_dir = ctx.apm_modules_dir
Copy link

Copilot AI Apr 19, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

apm_modules_dir = ctx.apm_modules_dir is assigned but never used. Drop the unused local (or use it below) to avoid implying later phases still depend on bare-name locals here.

Suggested change
apm_modules_dir = ctx.apm_modules_dir

Copilot uses AI. Check for mistakes.
Comment thread src/apm_cli/install/pipeline.py Outdated
Comment on lines +186 to +188
package_deployed_files: builtins.dict = {} # dep_key -> list of relative deployed paths
package_types: builtins.dict = {} # dep_key -> package type string
_package_hashes: builtins.dict = {} # dep_key -> sha256 hash
Copy link

Copilot AI Apr 19, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These locals (package_deployed_files, package_types, _package_hashes) are initialized but never read or written back into ctx. Since later phases use ctx.package_* fields, this looks like leftover legacy scaffolding; remove them or explicitly bind them onto ctx if they are intended to be the shared containers.

Suggested change
package_deployed_files: builtins.dict = {} # dep_key -> list of relative deployed paths
package_types: builtins.dict = {} # dep_key -> package type string
_package_hashes: builtins.dict = {} # dep_key -> sha256 hash
ctx.package_deployed_files = {} # dep_key -> list of relative deployed paths
ctx.package_types = {} # dep_key -> package type string
ctx.package_hashes = {} # dep_key -> sha256 hash

Copilot uses AI. Check for mistakes.
Comment thread src/apm_cli/install/sources.py Outdated
Comment on lines +51 to +55
package_info: "PackageInfo"
install_path: Path
dep_key: str
deltas: Dict[str, int] = field(default_factory=lambda: {"installed": 1})

Copy link

Copilot AI Apr 19, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Materialization.package_info is annotated as PackageInfo, but multiple sources construct Materialization(package_info=None, ...) (see cached/fresh paths). Make this field Optional[PackageInfo] so the type contract matches actual usage and you can remove the type: ignore[arg-type] call sites.

Copilot uses AI. Check for mistakes.
Comment thread src/apm_cli/install/__init__.py Outdated

pipeline.py orchestrator that calls each phase in order
context.py InstallContext dataclass (state passed between phases)
options.py InstallOptions dataclass (parsed CLI options)
Copy link

Copilot AI Apr 19, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The module docstring lists options.py / InstallOptions, but there is no options.py under apm_cli/install/ in this PR. Update the architecture list to reflect the actual files (or add the missing module) so new contributors aren't sent to a dead reference.

Suggested change
options.py InstallOptions dataclass (parsed CLI options)

Copilot uses AI. Check for mistakes.
Comment thread CHANGELOG.md Outdated

### 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
Copy link

Copilot AI Apr 19, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This changelog entry does not follow the repo's Keep-a-Changelog convention of ending each bullet with a PR reference like (#764). Add the PR number (and consider splitting this into shorter, single-focus bullets) so the Unreleased section stays consistent.

Suggested change
- 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
- 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)

Copilot uses AI. Check for mistakes.
Comment thread CHANGELOG.md Outdated
Comment on lines +15 to +20
The orchestrator ``_integrate_local_content`` remains in
``apm_cli.commands.install`` because it calls ``_integrate_package_primitives``
via bare-name lookup, and tests patch
``apm_cli.commands.install._integrate_package_primitives`` to intercept that
call. Keeping the orchestrator co-located with the re-exported name preserves
``@patch`` compatibility without any test modifications.
Copy link

Copilot AI Apr 19, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The module docstring says _integrate_local_content "remains in apm_cli.commands.install", but in this refactor the implementation lives in apm_cli.install.services (with re-exports from commands/install.py). Please update/remove this note so it reflects the current structure and patching story.

Suggested change
The orchestrator ``_integrate_local_content`` remains in
``apm_cli.commands.install`` because it calls ``_integrate_package_primitives``
via bare-name lookup, and tests patch
``apm_cli.commands.install._integrate_package_primitives`` to intercept that
call. Keeping the orchestrator co-located with the re-exported name preserves
``@patch`` compatibility without any test modifications.
The orchestration entrypoint for local-content integration now lives in
``apm_cli.install.services``. ``apm_cli.commands.install`` may re-export
that functionality for compatibility, so tests should patch the symbol at the
import path used by the code under test rather than assuming the
implementation lives in the commands module.

Copilot uses AI. Check for mistakes.
Comment thread src/apm_cli/install/pipeline.py Outdated
from ..deps.lockfile import LockFile, get_lockfile_path
from ..deps.installed_package import InstalledPackage
from ..deps.registry_proxy import RegistryConfig
from ..utils.content_hash import compute_package_hash as _compute_hash
Copy link

Copilot AI Apr 19, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

compute_package_hash as _compute_hash is imported but not used in this module. Removing the unused import will reduce noise in the pipeline seam block (and avoid suggesting hashes are still managed here).

Suggested change
from ..utils.content_hash import compute_package_hash as _compute_hash

Copilot uses AI. Check for mistakes.
Comment thread src/apm_cli/install/validation.py Outdated
Comment on lines +53 to +55
def _local_path_no_markers_hint(local_dir, verbose_log=None, logger=None):
"""Scan two levels for sub-packages and print a hint if any are found."""
from apm_cli.utils.helpers import find_plugin_json
Copy link

Copilot AI Apr 19, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

verbose_log is accepted by _local_path_no_markers_hint(...) but never used. Either remove the parameter (and the argument passed from _validate_package_exists) or use it for consistent verbose output to avoid a misleading/unused API surface.

Copilot uses AI. Check for mistakes.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
@danielmeppiel
Copy link
Copy Markdown
Collaborator Author

@copilot address the PR review comments as needed

… docstrings)

Agent-Logs-Url: https://github.com/microsoft/apm/sessions/130893b0-cd40-40a5-9cfe-156b8035fac5

Co-authored-by: danielmeppiel <51440732+danielmeppiel@users.noreply.github.com>
Copy link
Copy Markdown
Contributor

Copilot AI commented Apr 19, 2026

@copilot address the PR review comments as needed

Addressed in 4656c0b:

  • pipeline.py: dropped unused apm_modules_dir, package_deployed_files/package_types/_package_hashes locals, and the unused compute_package_hash as _compute_hash import
  • sources.py: Materialization.package_info is now Optional[PackageInfo]; both # type: ignore[arg-type] markers removed
  • install/__init__.py: refreshed the architecture docstring to match actual files (dropped dead options.py reference, added request.py/service.py/services.py/sources.py/template.py)
  • phases/local_content.py: corrected docstring -- the orchestrator now lives in apm_cli.install.services and is re-exported from commands/install.py
  • validation.py: removed unused verbose_log parameter from _local_path_no_markers_hint and updated the call site
  • CHANGELOG.md: added (#764) to the design-patterns entry

Skipped the CHANGELOG:32 suggestion -- it's already resolved (and that bullet covers a distinct concern from the design-patterns one).

3981/3981 unit tests pass.

danielmeppiel added a commit that referenced this pull request Apr 19, 2026
Covers gap G2: presentation/dry_run.py (extracted in PR #764) had zero
binary-level tests. Three cases lock the contract end-to-end:

- Plain --dry-run lists APM deps, prints banner/footer, makes no changes
  on disk (no lockfile, no apm_modules/, no .github/ artifacts).
- --dry-run --only=apm correctly suppresses MCP dependency lines.
- --dry-run after manifest edit previews orphan removals (Files that
  would be removed: N) without actually deleting anything. Guards the
  NameError regression on the orphan-preview path.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
danielmeppiel added a commit that referenced this pull request Apr 19, 2026
Covers gap G4: PR #764 fixed a PAT leak in install/validation.py:218-231
by piping git ls-remote stderr through ado_downloader._sanitize_git_error.
Without an integration test, that fix could regress silently.

Two cases inject a recognizable canary token via GITHUB_TOKEN /
GITHUB_APM_PAT, run apm install --verbose against nonexistent repos,
and assert the canary substring is fully absent from stdout/stderr:

- 404 repo via shorthand (org/repo)
- 404 repo via explicit git+https URL

Allows redacted forms (***, [REDACTED]) but never the literal token.
Verified the canary is fully scrubbed; no leak detected.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
danielmeppiel added a commit that referenced this pull request Apr 19, 2026
Builds a real 3-level dependency chain using local-path APM deps so the
test is fast and network-free:

  consumer -> pkg-a -> pkg-b -> pkg-c (leaf)

Two cases exercise the post-#764 install/resolve walker end-to-end:
- Resolution: all 3 packages materialize in apm_modules/_local/, lockfile
  records depth (1/2/3) and resolved_by chain, instructions deploy from
  every level into .github/instructions/.
- Uninstall cascade: uninstall ../pkg-a removes pkg-b and pkg-c from
  disk, lockfile, and .github/instructions/ via _cleanup_transitive_orphans.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@danielmeppiel danielmeppiel merged commit 90a49da into main Apr 19, 2026
5 checks passed
@danielmeppiel danielmeppiel deleted the refactor/install-modularization branch April 19, 2026 14:48
danielmeppiel added a commit that referenced this pull request Apr 19, 2026
Covers gap G2: presentation/dry_run.py (extracted in PR #764) had zero
binary-level tests. Three cases lock the contract end-to-end:

- Plain --dry-run lists APM deps, prints banner/footer, makes no changes
  on disk (no lockfile, no apm_modules/, no .github/ artifacts).
- --dry-run --only=apm correctly suppresses MCP dependency lines.
- --dry-run after manifest edit previews orphan removals (Files that
  would be removed: N) without actually deleting anything. Guards the
  NameError regression on the orphan-preview path.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
danielmeppiel added a commit that referenced this pull request Apr 19, 2026
Covers gap G4: PR #764 fixed a PAT leak in install/validation.py:218-231
by piping git ls-remote stderr through ado_downloader._sanitize_git_error.
Without an integration test, that fix could regress silently.

Two cases inject a recognizable canary token via GITHUB_TOKEN /
GITHUB_APM_PAT, run apm install --verbose against nonexistent repos,
and assert the canary substring is fully absent from stdout/stderr:

- 404 repo via shorthand (org/repo)
- 404 repo via explicit git+https URL

Allows redacted forms (***, [REDACTED]) but never the literal token.
Verified the canary is fully scrubbed; no leak detected.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
danielmeppiel added a commit that referenced this pull request Apr 19, 2026
Builds a real 3-level dependency chain using local-path APM deps so the
test is fast and network-free:

  consumer -> pkg-a -> pkg-b -> pkg-c (leaf)

Two cases exercise the post-#764 install/resolve walker end-to-end:
- Resolution: all 3 packages materialize in apm_modules/_local/, lockfile
  records depth (1/2/3) and resolved_by chain, instructions deploy from
  every level into .github/instructions/.
- Uninstall cascade: uninstall ../pkg-a removes pkg-b and pkg-c from
  disk, lockfile, and .github/instructions/ via _cleanup_transitive_orphans.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
danielmeppiel added a commit that referenced this pull request Apr 19, 2026
…urfaced by #764 review (#767)

* test(integration): add --global install/uninstall E2E coverage (G1+U1)

Covers gap G1 (apm install -g real package + content verification) and
U1 (apm uninstall pkg -g removes files from ~/.apm/). Existing global
scope tests only validated directory plumbing and error paths; no test
actually deployed a real package under user scope.

Three cases:
- install -g deploys microsoft/apm-sample-package; lockfile + primitive
  files appear under fake_home; cwd remains untouched.
- uninstall -g removes lockfile entry, manifest entry, and all deployed
  primitive files.
- Global + project installs of the same package coexist without collision.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

* test(integration): add apm install --dry-run E2E coverage (G2)

Covers gap G2: presentation/dry_run.py (extracted in PR #764) had zero
binary-level tests. Three cases lock the contract end-to-end:

- Plain --dry-run lists APM deps, prints banner/footer, makes no changes
  on disk (no lockfile, no apm_modules/, no .github/ artifacts).
- --dry-run --only=apm correctly suppresses MCP dependency lines.
- --dry-run after manifest edit previews orphan removals (Files that
  would be removed: N) without actually deleting anything. Guards the
  NameError regression on the orphan-preview path.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

* test(integration): add apm deps update E2E coverage (Up1+Up2+Up3+G3)

The canonical update command had zero CLI-level integration coverage;
the closest existing test (test_apm_dependencies::test_dependency_update_workflow)
called GitHubPackageDownloader directly rather than the binary.

Four cases:
- Up1: deps update (all packages) with real ref change bumps lockfile SHA
  and re-deploys files (pinned commit -> main).
- Up2: deps update <pkg> with two installed packages updates only the
  named one.
- Up3: deps update -g respects user scope. Lockfile lands at
  ~/.apm/apm.lock.yaml; cwd remains clean (regression guard against the
  historical silent-deploy-to-project bug).
- G3: deps update unknown-pkg exits non-zero with a clear error.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

* test(integration): add verbose install token redaction guard (G4)

Covers gap G4: PR #764 fixed a PAT leak in install/validation.py:218-231
by piping git ls-remote stderr through ado_downloader._sanitize_git_error.
Without an integration test, that fix could regress silently.

Two cases inject a recognizable canary token via GITHUB_TOKEN /
GITHUB_APM_PAT, run apm install --verbose against nonexistent repos,
and assert the canary substring is fully absent from stdout/stderr:

- 404 repo via shorthand (org/repo)
- 404 repo via explicit git+https URL

Allows redacted forms (***, [REDACTED]) but never the literal token.
Verified the canary is fully scrubbed; no leak detected.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

* test(integration): add marketplace CLI E2E coverage (G3.5, partial)

The marketplace CLI flow had ~1604 LOC of unit coverage but zero
binary-level integration tests. This adds three config-side cases plus
one documented skip:

- list shows seeded ~/.apm/marketplaces.json entries
- add rejects invalid OWNER/REPO format before hitting network
- remove clears the entry from disk and from list output
- install plugin@marketplace deploy is skipped (needs a stable public
  marketplace.json fixture; documented in test for follow-up)

Uses fake_home isolation pattern; no real network required for the three
active tests.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

* test(integration): add apm uninstall --dry-run E2E coverage (U2)

Two cases:
- Real install + uninstall --dry-run leaves all files, manifest, and
  lockfile untouched while emitting the dry-run preview.
- uninstall --dry-run with an unknown package emits a warning and makes
  no mutations (locks current behavior: exit 0 with warning, NOT failure
  -- engine.py:60-101).

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

* test(integration): add multi-package uninstall E2E coverage (U3)

Two cases using two real public APM packages:
- uninstall PKG_A PKG_B in a single command removes both from manifest,
  lockfile, and disk in one operation.
- uninstall known unknown/pkg removes the known one and emits a warning
  for the unknown (exit 0, behavior locked).

Packages: microsoft/apm-sample-package and github/awesome-copilot/skills/aspire
(both used elsewhere in the integration suite).

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

* test(integration): add 3-level transitive chain E2E coverage (G5)

Builds a real 3-level dependency chain using local-path APM deps so the
test is fast and network-free:

  consumer -> pkg-a -> pkg-b -> pkg-c (leaf)

Two cases exercise the post-#764 install/resolve walker end-to-end:
- Resolution: all 3 packages materialize in apm_modules/_local/, lockfile
  records depth (1/2/3) and resolved_by chain, instructions deploy from
  every level into .github/instructions/.
- Uninstall cascade: uninstall ../pkg-a removes pkg-b and pkg-c from
  disk, lockfile, and .github/instructions/ via _cleanup_transitive_orphans.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

---------

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
danielmeppiel added a commit that referenced this pull request Apr 19, 2026
- Consolidate two #764 bullets into one (same PR, two bullets violated 'one line per PR' rule)
- Merge inert CI stub bullet into the merge-queue (#770, #771) line since the stub came from direct pushes (no PR number)
- Fold 'Tests' and 'Dependencies' headings into 'Changed' to match Keep-a-Changelog conventions used by the project (Added/Changed/Deprecated/Removed/Fixed/Security only)

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
danielmeppiel added a commit that referenced this pull request Apr 19, 2026
* chore: prepare v0.8.12 release

Bump version to 0.8.12 and backfill CHANGELOG entries for all PRs merged since v0.8.11. Includes feature additions (project-local .apm/ install, configurable temp-dir, root-project primitives, marketplace proxy), an install-engine refactor + design-pattern cleanup, the #666/#762 stale-cleanup safety hardening, multiple Windows-environment fixes (CP950 encoding, hook backslash paths, init path validation), and the GitHub Merge Queue tiered-CI rollout.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

* review: address Copilot feedback on CHANGELOG structure

- Consolidate two #764 bullets into one (same PR, two bullets violated 'one line per PR' rule)
- Merge inert CI stub bullet into the merge-queue (#770, #771) line since the stub came from direct pushes (no PR number)
- Fold 'Tests' and 'Dependencies' headings into 'Changed' to match Keep-a-Changelog conventions used by the project (Added/Changed/Deprecated/Removed/Fixed/Security only)

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

---------

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants