fix(install): harden stale-file cleanup with per-file content-hash provenance (#666 follow-up) by danielmeppiel · Pull Request #762 · microsoft/apm

danielmeppiel · 2026-04-19T08:23:34Z

Summary

Hardening follow-up to #750 (issue #666). Three commits, addressing every blocker and reasonable polish surfaced by an internal review team (security expert, Python architect, CLI logging UX expert) across three iteration rounds.

What this PR does

1. Per-file content-hash provenance for stale-file deletion

#750 cleans up files that left a package between installs. This PR makes that cleanup safe under user edits:

LockedDependency.deployed_file_hashes: Dict[str, str] records sha256:<hex> for every file APM deploys.
On the next install, before deleting a "stale" file, APM compares the current on-disk hash to the recorded hash. Mismatch -> file kept, treated as a user edit, attributed in diagnostics.
Missing recorded hash (legacy lockfiles) falls through to delete (back-compat).

2. Single safety chokepoint: `apm_cli/integration/cleanup.py`

New remove_stale_deployed_files(...) helper is the only path that deletes lockfile-tracked files on install. Three gates, in order:

BaseIntegrator.validate_deploy_path() -- rejects unmanaged prefixes.
Directory rejection -- defeats poisoned-lockfile rmtree.
Recorded-hash comparison -- defeats user-edit clobber.

The orphan-cleanup path (packages removed from apm.yml entirely) and the intra-package stale path both route through this helper. Pinned by test_helper_signature_does_not_accept_logger (SoC: helper pushes to DiagnosticCollector, never logs directly).

3. Logger surface for cleanup

Three new CommandLogger methods:

stale_cleanup(n, package=) / orphan_cleanup(n) -- [i] symbol (informational, not progress; cleanup is not a success in itself).
cleanup_skipped_user_edit(path, package) -- yellow inline warning per skipped file (verbose mode).
install_summary(stale_cleaned=) -- final summary appends (N stale files cleaned). inside the sentence.

4. Orphan-cleanup correctness regression fix (commit 3)

The first refactor introduced a subtle bug caught by security re-review: the orphan loop derived its "still-declared" set from package_deployed_files.keys() (integration outcome). A transient integration error for a still-declared package would leave its key absent from that dict, and the loop would silently delete the package's files. Fix: derive from intended_dep_keys (manifest intent). Pinned by test_orphan_loop_uses_manifest_intent_not_integration_outcome (source-level guard that strips comments before checking).

Review iteration

Round	Security	Architecture	UX
1	REQUEST-CHANGES (orphan path bypassed hash gate)	APPROVE-WITH-SUGGESTIONS (5 polish items)	APPROVE-WITH-SUGGESTIONS (1 wiring blocker + 6 polish)
2	REQUEST-CHANGES (orphan loop misclassification on integration failure)	APPROVE	APPROVE
3	APPROVE	-- (no code in scope)	-- (no code in scope)

Deferred suggestions (tracked, non-blocking):

Validate sha256:<hex> format on recorded hashes (fail-safe today).
Plan a deprecation path for legacy lockfiles missing hashes.
Apply safety gates to dry-run preview (preview-vs-actual divergence is UX, not security).

Tests

New: 14 unit tests in tests/unit/integration/test_cleanup_helper.py (gates, orphan path, SoC pin, regression guard).
New: 9 unit tests in tests/unit/test_command_logger.py (cleanup methods + summary punctuation).
New: 4 unit tests in tests/test_lockfile.py (deployed_file_hashes round-trip).
Full suite: 3968 unit + 87 install/orphan integration tests pass.

Files changed

New: src/apm_cli/integration/cleanup.py, src/apm_cli/utils/content_hash.py
Modified: src/apm_cli/commands/install.py, src/apm_cli/deps/lockfile.py, src/apm_cli/core/command_logger.py
Tests: tests/unit/integration/test_cleanup_helper.py (new), tests/unit/test_command_logger.py, tests/test_lockfile.py
Docs: docs/src/content/docs/reference/cli-commands.md (stale-cleanup section + safety contract), CHANGELOG.md ([Unreleased] -> Fixed)

Follow-up review of #750 surfaced 3 blockers and 6 design improvements. This change addresses all 9. Safety fixes (blockers): - Refuse to delete directory entries from the lockfile -- closes a poisoned-lockfile rmtree vector (an entry like ".github/instructions" passed validate_deploy_path and would have rmtree'd a user-managed subtree). - Add per-file SHA-256 provenance: APM records a deployed_file_hashes map on each LockedDependency at install time and verifies it before deletion. Mismatched hashes are treated as user-edited and skipped with an actionable warning. - Skip stale cleanup for any package whose integration reported an error (avoids deleting a file that just failed to redeploy). Architecture: - Extract apm_cli.integration.cleanup.remove_stale_deployed_files() helper with a CleanupResult dataclass. Both the local-package cleanup at install.py:1011 and the remote-package cleanup at install.py:2706 now go through it; new safety gates apply uniformly. - Drop dead getattr(BaseIntegrator, "KNOWN_TARGETS", None) merges in three sites -- KNOWN_TARGETS lives in apm_cli.integration.targets, not on BaseIntegrator; validate_deploy_path already falls back to it. - Drop defensive `if logger:` guards inside _install_apm_dependencies (logger is always passed in production). Logging UX: - New InstallLogger.stale_cleanup() / orphan_cleanup() methods render destructive actions at default verbosity (filesystem deletions in a user-tracked workspace must not be hidden behind --verbose). - install_summary() grows a stale_cleaned parameter; total is tracked on the logger and surfaced as "Installed N APM deps (M stale files cleaned)." - Recoverable cleanup failures move from diagnostics.error() to .warn() with retry-on-next-install guidance ("So What?" test). - Dry-run now previews package-level orphan cleanup (computable from lockfile + manifest alone) and explicitly notes that intra-package stale cleanup is not previewed. Schema (additive, backward-compatible): - LockedDependency.deployed_file_hashes: Dict[str, str] - LockFile.local_deployed_file_hashes: Dict[str, str] - New compute_file_hash() helper in utils.content_hash. - Both fields omitted from YAML when empty; legacy lockfiles load cleanly. Tests: - 10 new unit tests for the cleanup helper covering: happy path, path traversal, unmanaged prefix, directory rejection, missing-file, hash mismatch (skip), hash match (delete), no-hash legacy fallback, unlink failure retry path. - 9 new logger tests covering stale_cleanup/orphan_cleanup visibility, total accumulation, install_summary suffix, user-edit warning. - 4 new lockfile tests covering deployed_file_hashes round-trip, local_deployed_file_hashes round-trip, omit-when-empty, default-empty-on-load. - Full unit suite (3964 tests) green. Docs: - Promote stale-file cleanup to its own subsection in cli-commands.md with the safety contract explicitly documented. - CHANGELOG entry under [Unreleased]. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Three reviewers (security, python-architect, cli-logging-ux) flagged issues on commit 449ad7a. This addresses every blocker and the non-trivial polish items. Security blocker -- orphan cleanup bypassed the per-file hash gate: Refactor the orphan cleanup loop in install.py to route through remove_stale_deployed_files() instead of reimplementing the gates inline. Pass each orphaned package's deployed_file_hashes so the same provenance check applied to intra-package stale deletions now also protects user-edited files in packages removed from apm.yml. The helper grew a `failed_path_retained: bool` parameter so the failure diagnostic stays accurate: orphan failures can NOT be retained in the lockfile (the owning package is being dropped), so they say "delete the file manually" instead of "will retry on next install". Drops the old inline orphan loop and the now-unused detect_orphans() call site that fed it. detect_orphans is still used for the dry-run preview. UX blocker -- cleanup_skipped_user_edit was dead code: The InstallLogger method existed and was tested but no caller iterated CleanupResult.skipped_user_edit to invoke it -- so the inline yellow "Kept user-edited file ... delete manually" warning never rendered. Wire it at all three call sites (local stale, remote stale, orphan). Architecture cleanups: - Remove dead `package_deployed_file_hashes` dict and its three population sites: the lockfile-build path recomputes hashes via _hash_deployed(dep_files) anyway, so the dict was wasted state. - Remove dead `import shutil` (cleanup.py) and dead `import shutil as _shutil` (install.py orphan block) -- both paths refuse directory entries now and only call .unlink(). - Drop unused `logger` parameter from remove_stale_deployed_files. The helper communicates exclusively through CleanupResult and diagnostics; rendering policy belongs to the caller. SoC pinned by a new test. UX polish: - install_summary punctuation: move the period after the parenthetical -- "Installed N deps (M stale files cleaned)." instead of "Installed N deps. (M stale files cleaned)". - stale_cleanup / orphan_cleanup use symbol="info" ([i]) instead of "gear" ([*]) so cleanup actions don't visually masquerade as success messages. - Dry-run notice about stale cleanup not being previewed only emits when there are APM deps to have stale files for. Tests added: - test_orphan_failure_message_does_not_promise_retry: verifies the new failed_path_retained=False mode does NOT say "will retry on next install". - test_orphan_path_honours_hash_gate: regression guard for the security blocker -- orphan cleanup must skip user-edited files just like intra-package stale cleanup. - test_helper_signature_does_not_accept_logger: pins the SoC decision so a future contributor can't accidentally re-add direct logger calls inside the helper. - install_summary punctuation: explicit assertion that the period ends the sentence and ". (" never appears. All 3967 unit tests + 77 install/cleanup integration tests green. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

…utcome Security re-review (round 2) caught a regression introduced by the round-1 refactor: the orphan-cleanup loop derived its "still-declared" set from package_deployed_files.keys() (populated INSIDE the integration try block at install.py:2406 and 2601). A transient integration failure for a still-declared package would therefore leave its key absent from that dict, the orphan loop would classify it as removed, and the per-file content-hash gate (which only defends user-edited files) would let APM-deployed files matching the recorded hash be deleted -- silently corrupting a package that is still in apm.yml. Fix: derive the membership test from intended_dep_keys (manifest intent, already computed at install.py:1707), not from the integration outcome. Add a source-level regression guard in tests/unit/integration/test_cleanup_helper.py (test_orphan_loop_uses_manifest_intent_not_integration_outcome) -- same spirit as test_helper_signature_does_not_accept_logger. The test extracts the orphan block, strips comments, and asserts intended_dep_keys is read AND package_deployed_files.keys() is not. Future refactors that re-introduce the regression will fail this test loudly. Tests: 3968/3968 unit + 87/87 install/orphan integration green. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot

Pull request overview

This PR hardens apm install stale/orphan deployed-file cleanup by adding per-file SHA-256 provenance to the lockfile and routing all install-time deletions through a shared safety-gated helper, with updated CLI logging and documentation.

Changes:

Add deployed_file_hashes / local_deployed_file_hashes to lockfile schema and record per-file hashes for provenance checks.
Introduce remove_stale_deployed_files() helper with path validation, directory-entry refusal, and content-hash mismatch skipping.
Update install logging, dry-run orphan preview, tests, docs, and changelog entry.

Show a summary per file

File	Description
`src/apm_cli/commands/install.py`	Routes orphan + stale cleanup through shared helper; records per-file hashes; adds dry-run orphan preview; updates summary to include cleaned count.
`src/apm_cli/integration/cleanup.py`	New centralized deletion helper with safety gates and diagnostics reporting.
`src/apm_cli/utils/content_hash.py`	Adds `compute_file_hash()` for per-file provenance hashing.
`src/apm_cli/deps/lockfile.py`	Adds new hash fields to lockfile models + YAML serialization/deserialization.
`src/apm_cli/core/command_logger.py`	Adds cleanup-phase logger methods and summary suffix reporting.
`tests/unit/integration/test_cleanup_helper.py`	New unit tests pinning helper safety gates and regressions.
`tests/unit/test_command_logger.py`	New unit tests for cleanup logger methods and summary punctuation.
`tests/test_lockfile.py`	Tests for new hash-field round-tripping and omission when empty.
`docs/src/content/docs/reference/cli-commands.md`	Documents stale-file cleanup behavior and safety contract.
`CHANGELOG.md`	Adds an Unreleased Fixed entry for this change.

Copilot's findings

Files reviewed: 10/10 changed files
Comments generated: 6

Six findings from copilot-pull-request-reviewer, all addressed: 1. NameError on local .apm/ persist (install.py:1088, CRITICAL) `_hash_deployed` was an inner closure of `_install_apm_dependencies` but referenced from `_integrate_local_content` (sibling module-level function). Would raise `NameError` at runtime whenever local content was deployed. Promoted to a module-level helper with an explicit `(rel_paths, project_root)` signature; removed the closure; updated the lockfile-build call site to pass `project_root`. Pinned by `test_hash_deployed_is_module_level_and_works`. 2. Provenance gate failed open on hash-read errors (cleanup.py:155-157) A `compute_file_hash` exception (PermissionError, etc.) was swallowed into `actual_hash = None`, allowing deletion to proceed despite a recorded hash existing. Now fails CLOSED: any verification failure skips the deletion, records `skipped_user_edit`, and emits a "could not verify file content" warning telling the user to inspect and delete manually if needed. Pinned by `test_hash_read_failure_fails_closed`. 3. Orphan cleanup restricted to active targets only (install.py:2720) Pre-PR code explicitly merged `BaseIntegrator.KNOWN_TARGETS` into the validation set so legacy targets (runtime switched, scope mismatch) could still be cleaned up. The refactor passed `targets=_targets or None` which excluded those paths. Now passes `targets=None`, which `validate_deploy_path` treats as "all KNOWN_TARGETS" -- the cleaner equivalent of the explicit merge. Comment explains the trap. 4. Stale module docstring (cleanup.py:25-27) Mentioned a `logger` parameter the helper no longer accepts (and that `test_helper_signature_does_not_accept_logger` actively forbids). Rewrote to describe the actual SoC: helper records to diagnostics, callers own logging. 5. CHANGELOG entry missing PR number Added (#762) to the entry; trimmed to a single concise sentence per `.github/instructions/changelog.instructions.md`. 6. cleanup_skipped_user_edit verbosity claim The PR description claimed verbose-only; the implementation always emits. The implementation is correct (this is a safety message -- the user MUST see when APM kept their edit) and the docstring on the method already documents "default verbosity". The PR description was misleading; addressing in the PR body, no code change. Tests: 3970/3970 unit + 87/87 install/orphan integration green. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

danielmeppiel · 2026-04-19T08:38:49Z

Thanks for the careful review. All six findings addressed in 4356c63. Summary:

Critical (NameError) -- _hash_deployed was an inner closure of _install_apm_dependencies but referenced from the sibling _integrate_local_content. Confirmed the call would NameError at runtime on the local .apm/ persist path. Promoted to a module-level helper with explicit (rel_paths, project_root) signature; updated all three call sites; pinned by test_hash_deployed_is_module_level_and_works. Good catch.

Security (fail-open on hash-read error) -- Confirmed and fixed. The provenance gate now fails CLOSED: a compute_file_hash exception with a recorded hash present skips the deletion, records skipped_user_edit, and emits a "could not verify file content" warning. Pinned by test_hash_read_failure_fails_closed.

Correctness (orphan target scope) -- Confirmed against the pre-PR code, which explicitly merged BaseIntegrator.KNOWN_TARGETS for exactly this reason. Switched the orphan call site to targets=None, which validate_deploy_path treats as "all KNOWN_TARGETS" -- the cleaner equivalent. Added a comment explaining the trap.

Docstring -- Updated to reflect the current SoC (helper records to diagnostics, callers own logging). The mismatch was real -- the test test_helper_signature_does_not_accept_logger actively forbids the parameter the docstring still mentioned.

CHANGELOG -- Added (#762) and trimmed to a single concise sentence per the instructions file.

cleanup_skipped_user_edit verbosity -- The implementation (always emit) is intentional and correct: this is the visible safety guarantee surfacing -- the user must know when APM kept their edit, otherwise the per-file provenance check is invisible. The method's own docstring already says "Yellow inline at default verbosity". The PR description was misleading; that's a description-only correction, no code change.

Tests: 3970 unit + 87 install/orphan integration green.

…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>

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>

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>

@patch

* refactor(install): P0 -- create install engine package skeleton 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> * refactor(install): P1.L1 -- extract validation.py from install.py 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> * refactor(install): P1.L2 -- extract local_content.py phase module 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> * refactor(install): P1.L3 -- introduce LockfileBuilder + relocate _hash_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> * refactor(install): P1.L4 -- extract pre-deploy security scan helper 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> * refactor(install): P2.S1 -- extract resolve & targets phases 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 * refactor(install): P2.S2 -- extract parallel download phase 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> * refactor(install): P2.S3 -- extract sequential integration phase 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> * refactor(install): P2.S4 -- extract cleanup orchestrator phase 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> * refactor(install): P2.S5 -- extract dry-run presentation 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> * refactor(install): P2.S6 -- fold lockfile assembly into LockfileBuilder + 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> * refactor(install): P3.R2 -- activate architectural invariant tests 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> * refactor(install): P3.R1 -- harden logging UX in extracted phase modules - 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> * refactor(install): F5 -- document _targets fallback in cleanup phase 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> * refactor(install): F4 -- UX polish (logger plumbing, tree-item helper, 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> * refactor(install): F1 -- decompose integrate.py per-package paths 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> * refactor(install): F2 -- extract pipeline.py orchestrator 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> * refactor(install): F3 -- fold local-content integration into pipeline 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> * refactor(install): F4b -- guard logger calls in cleanup phase + thread 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> * docs(changelog): record install-modularization refactor and F3 stale-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> * refactor(install): P1.1 -- move integration template to install/services.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> * refactor(install): P1.2 -- replace _install_mod indirection with direct 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> * refactor(install): P2 -- introduce DependencySource Strategy + integration 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> * refactor(install): P3 -- introduce InstallService Application Service 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> * refactor(install): P4 -- address architect-review nits 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> * fix(install): address PR review nits from #764 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> * Update CHANGELOG.md Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> * refactor(install): address PR review nits (unused locals, type hints, 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> --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Co-authored-by: copilot-chat <copilot-chat@github.com> Co-authored-by: GitHub Copilot <copilot@github.com> Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>

Bump version to 0.9.0 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), 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>

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>

* 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>

danielmeppiel and others added 3 commits April 19, 2026 10:05

danielmeppiel requested a review from sergio-sisternes-epam as a code owner April 19, 2026 08:23

Copilot AI review requested due to automatic review settings April 19, 2026 08:23

Copilot started reviewing on behalf of danielmeppiel April 19, 2026 08:24 View session

Copilot AI reviewed Apr 19, 2026

View reviewed changes

Comment thread src/apm_cli/commands/install.py

Comment thread src/apm_cli/integration/cleanup.py

Comment thread src/apm_cli/integration/cleanup.py Outdated

Comment thread src/apm_cli/commands/install.py

Comment thread src/apm_cli/core/command_logger.py

Comment thread CHANGELOG.md Outdated

danielmeppiel merged commit 522c246 into main Apr 19, 2026
8 checks passed

danielmeppiel deleted the harden/stale-cleanup-followup-666 branch April 19, 2026 08:44

danielmeppiel mentioned this pull request Apr 19, 2026

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

Merged

Copilot AI mentioned this pull request Apr 19, 2026

test(integration): close install/uninstall/update CLI coverage gaps surfaced by #764 review #767

Merged

danielmeppiel mentioned this pull request Apr 19, 2026

chore: prepare v0.8.12 release #774

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(install): harden stale-file cleanup with per-file content-hash provenance (#666 follow-up)#762

fix(install): harden stale-file cleanup with per-file content-hash provenance (#666 follow-up)#762
danielmeppiel merged 4 commits intomainfrom
harden/stale-cleanup-followup-666

danielmeppiel commented Apr 19, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

danielmeppiel commented Apr 19, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

danielmeppiel commented Apr 19, 2026

Summary

What this PR does

1. Per-file content-hash provenance for stale-file deletion

2. Single safety chokepoint: apm_cli/integration/cleanup.py

3. Logger surface for cleanup

4. Orphan-cleanup correctness regression fix (commit 3)

Review iteration

Tests

Files changed

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Copilot's findings

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

danielmeppiel commented Apr 19, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

2. Single safety chokepoint: `apm_cli/integration/cleanup.py`