fix(install): clean up stale deployed files on rename/remove within a package (#666)#750
Conversation
|
@microsoft-github-policy-service agree company="Mirego" |
There was a problem hiding this comment.
Pull request overview
This PR fixes install-time drift handling by removing stale deployed files when a package remains installed but its integrated output changes (e.g., file rename/removal), keeping both disk and apm.lock.yaml consistent with the current integration result.
Changes:
- Added
detect_stale_files()helper insrc/apm_cli/drift.pyto compute per-package stale deployed paths via set difference. - Extended
apm installto delete stale deployed paths for packages processed in the current run, with safety validation and diagnostics. - Added unit + integration coverage for stale-file detection and rename cleanup (including partial installs), and adjusted integration test token skipping to allow local-only tests to run without GitHub auth.
Show a summary per file
| File | Description |
|---|---|
src/apm_cli/drift.py |
Documents and implements file-level stale drift detection via a new pure helper. |
src/apm_cli/commands/install.py |
Adds post-integration stale-file cleanup to remove previously deployed paths that are no longer produced. |
tests/unit/test_stale_file_detection.py |
Adds focused unit tests for detect_stale_files() semantics (empty/identity/rename/removal/etc.). |
tests/integration/test_diff_aware_install_e2e.py |
Adds E2E coverage for rename cleanup using a local-path dependency; moves token skips to only the networked test classes. |
Copilot's findings
Comments suppressed due to low confidence (4)
src/apm_cli/commands/install.py:2648
- This inline comment uses a Unicode em dash ("—"), but repo policy requires printable ASCII only in source files. Please replace it with ASCII punctuation (e.g., "--").
prev_dep = existing_lockfile.get_dependency(dep_key)
if not prev_dep:
continue # new package this install — nothing stale yet
old_deployed = builtins.list(prev_dep.deployed_files)
tests/integration/test_diff_aware_install_e2e.py:416
- This assertion message includes a Unicode em dash ("—"), but repo policy requires printable ASCII only in source files and CLI/test output strings. Please replace it with ASCII punctuation.
assert deployed_before, "No deployed files found — cannot verify cleanup"
tests/integration/test_diff_aware_install_e2e.py:453
- This docstring uses a Unicode em dash ("—"), but repo policy requires printable ASCII only in source files. Please replace it with ASCII punctuation (e.g., "--").
"""`apm install --only=apm` on a package with a renamed file still cleans up.
Verifies that partial installs clean files for the packages they touch
— a deliberate departure from detect_orphans (package-level), which
no-ops on partial installs."""
tests/integration/test_diff_aware_install_e2e.py:403
- These newly added step comments use Unicode box-drawing characters ("──"), but repo policy requires printable ASCII only in source files. Please replace with ASCII equivalents (e.g., "-- Step 1: ... --").
"""Rename a source primitive, re-install, assert old files gone and
lockfile deployed_files no longer lists the stale paths."""
# ── Step 1: initial install ──
_write_apm_yml_local(temp_project, local_pkg_root)
result1 = _run_apm(apm_command, ["install"], temp_project)
- Files reviewed: 4/4 changed files
- Comments generated: 5
|
Addressed the review. Two new commits on the branch:
Replaced U+2014 (em dash) and U+2500 (box drawing light horizontal) with ASCII equivalents (
Added a bullet under the "Diff-Aware Installation (manifest as source of truth)" section in On Not updated. That file is a one-liner table ( |
| # Skip all tests if no GitHub token is available | ||
| pytestmark = pytest.mark.skipif( | ||
| not os.environ.get("GITHUB_APM_PAT") and not os.environ.get("GITHUB_TOKEN"), | ||
| reason="GITHUB_APM_PAT or GITHUB_TOKEN required for GitHub API access", | ||
| ) | ||
|
|
||
|
|
There was a problem hiding this comment.
Removing this file level block and adding it three times below does not seem appropriate
There was a problem hiding this comment.
You're right -- that was the wrong call. Fixed in 9c44e05 by splitting: the new tests live in tests/integration/test_intra_package_cleanup.py, and tests/integration/test_diff_aware_install_e2e.py is now restored byte-for-byte to main (module-level skipif untouched, no per-class copies). Root cause: the new tests don't need GITHUB_APM_PAT, so appending them to the token-gated file forced the skipif duplication. Splitting removes the constraint entirely.
…anup Pure set-difference helper that identifies deployed files no longer produced by the current install, enabling file-level cleanup to complement the existing package-level detect_orphans. Refs: microsoft#666
After each package's integration, diff the previous deployed_files (from the lockfile) against the fresh integration output and delete files that were renamed away or removed inside the package. Runs once per install, after the package-level orphan cleanup, mirroring the existing safety patterns (validate_deploy_path, cleanup_empty_parents, diagnostics error guard, verbose logging). Respects --dry-run. Closes microsoft#666
Two integration tests using a local-path package fixture: - renamed file cleanup on full install - renamed file cleanup on partial install (--only=apm) Refs: microsoft#666
Per .github/instructions/encoding.instructions.md, source files must stay within printable ASCII for Windows cp1252 compatibility. Replace U+2014 (em dash) and U+2500 (box drawing light horizontal) in the lines added by this PR. Pre-existing violations on main are left untouched. Refs: microsoft#666
…tion Adds a bullet to the Diff-Aware Installation list in the apm install reference, documenting that files from a still-present package that are no longer produced (rename/remove) are cleaned up on the next install. Complements the existing package-level orphan cleanup bullet. Refs: microsoft#666
Addresses review feedback (@danielmeppiel): the previous approach moved the module-level GITHUB_APM_PAT skipif into each existing class so the new TestFileRenamedWithinPackage could run without a token, which meant duplicating the skipif three times. The cleaner split: keep test_diff_aware_install_e2e.py untouched (restored bit-for-bit from upstream/main, module-level skipif intact) and move the new local-path-based tests to tests/integration/test_intra_package_cleanup.py -- which has no module-level skipif because these tests do not use GitHub at all. No change to the tested behaviour: same fixture, same two tests. Refs: microsoft#666
9c44e05 to
e9916dc
Compare
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>
…ovenance (#666 follow-up) (#762) * Harden stale-file cleanup on apm install (#666 follow-up) 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> * Address review-team feedback on stale-cleanup follow-up 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> * fix(install): orphan loop must use manifest intent, not integration outcome 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> * fix: address PR #762 review feedback 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> --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Summary
Fixes #666. After an
apm install, files that were in a package's previousdeployed_filesbut are no longer produced by the current integration are now removed from disk (and fromapm.lock.yaml).Before this change, the existing
detect_orphans()cleaned up deployed files only when a package was removed fromapm.yml. File-level renames or removals inside a still-present package left stale artifacts that users had to delete manually.Approach
src/apm_cli/drift.py:detect_stale_files(old_deployed, new_deployed) -> set. Pure set difference. Placed alongsidedetect_orphansand documented in the module-level "kinds of drift" list.src/apm_cli/commands/install.py: a single post-loop cleanup placed immediately after the existing package-level orphan cleanup and before lockfile generation. It iteratespackage_deployed_files, looks up the priordeployed_filesfromexisting_lockfile, computes stale paths, and deletes them.The block mirrors the patterns established by the orphan cleanup directly above it:
BaseIntegrator.validate_deploy_pathBaseIntegrator.cleanup_empty_parentsdiagnostics.count_for_package(dep_key, 'error') > 0skipdiagnostics.error(...)+logger.verbose_detail(...), failed paths kept indeployed_filesfor retrylogger.verbose_detail(f"Removed N stale file(s) from {dep_key}")The block scopes naturally to packages touched by the current install, so partial installs (
apm install <pkg>) clean their own stale files — a deliberate, useful departure fromdetect_orphans, which no-ops for partial installs.Scope notes
--dry-runhandling. The currentinstall()command short-circuits at line 796 for dry-run (prints what would be installed and returns) without invoking_install_apm_dependencies. So neither the existing orphan cleanup nor this new stale cleanup runs in dry-run mode. Extending dry-run to simulate the full install is a separate change..apm/-sourced primitives atinstall.py:984-1033(vialocal_deployed_files) is left untouched. Unifying the two via a shared helper would be a welcome follow-up if maintainers want it — I kept them separate here to keep the diff reviewable.mainalready has pre-existing black/isort drift, and CI does not enforce those tools (only a customLint - no raw str(relative_to) patternsgrep). Running the formatters would spread cosmetic noise across files we otherwise leave untouched. The diff is kept surgical. Happy to add a formatting-only follow-up commit if you prefer.Testing
tests/unit/test_stale_file_detection.py(6 tests): empty sets, identity, rename, removal, addition, order/duplicates.tests/integration/test_diff_aware_install_e2e.py::TestFileRenamedWithinPackage(2 tests), using a throwaway local-path package fixture (no network, no token):--only=apm(asserts disk + lockfile)The file-level
pytestmarkskip was moved into each existing class so the new tests can run withoutGITHUB_APM_PAT.Local results:
tests/unit tests/test_console.py: 3881 passedTestFileRenamedWithinPackage: 2 passeddetect_stale_files: 6 passedCloses #666