fix(install): clean up stale deployed files on rename/remove within a package (#666)

[Boubalou]

Summary

Fixes #666. After an apm install, files that were in a package's previous deployed_files but are no longer produced by the current integration are now removed from disk (and from apm.lock.yaml).

Before this change, the existing detect_orphans() cleaned up deployed files only when a package was removed from apm.yml. File-level renames or removals inside a still-present package left stale artifacts that users had to delete manually.

Approach

• New pure helper in src/apm_cli/drift.py: detect_stale_files(old_deployed, new_deployed) -> set. Pure set difference. Placed alongside detect_orphans and documented in the module-level "kinds of drift" list.
• New block in 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 iterates package_deployed_files, looks up the prior deployed_files from existing_lockfile, computes stale paths, and deletes them.

The block mirrors the patterns established by the orphan cleanup directly above it:

Concern	Pattern reused
Deletion safety gate	BaseIntegrator.validate_deploy_path
Post-deletion cleanup	BaseIntegrator.cleanup_empty_parents
Integration-error guard	diagnostics.count_for_package(dep_key, 'error') > 0 skip
Failure handling	diagnostics.error(...) + logger.verbose_detail(...), failed paths kept in deployed_files for retry
Summary log	logger.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 from detect_orphans, which no-ops for partial installs.

Scope notes

• No --dry-run handling. The current install() 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.
• Local packages already covered. The existing in-function stale cleanup for .apm/-sourced primitives at install.py:984-1033 (via local_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.
• No content-hash safety check before deletion. Consistent with the local-package pattern, deployed files are treated as APM-managed. Happy to add a hash check in a follow-up if preferred.
• Black/isort not run on the touched files in this PR. The repo's main already has pre-existing black/isort drift, and CI does not enforce those tools (only a custom Lint - no raw str(relative_to) patterns grep). 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

• Unit — tests/unit/test_stale_file_detection.py (6 tests): empty sets, identity, rename, removal, addition, order/duplicates.
• Integration — tests/integration/test_diff_aware_install_e2e.py::TestFileRenamedWithinPackage (2 tests), using a throwaway local-path package fixture (no network, no token):
  • renamed file cleanup on full install (asserts disk + lockfile)
  • renamed file cleanup on partial install --only=apm (asserts disk + lockfile)

The file-level pytestmark skip was moved into each existing class so the new tests can run without GITHUB_APM_PAT.

Local results:

• tests/unit tests/test_console.py: 3881 passed
• TestFileRenamedWithinPackage: 2 passed
• detect_stale_files: 6 passed
• Manual end-to-end smoke test on a local package with rename: passes cleanly; verbose output shows "Removed 1 stale file(s) from <dep_key>".

Closes #666

[Boubalou]

@microsoft-github-policy-service agree company="Mirego"

[Copilot]

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 in src/apm_cli/drift.py to compute per-package stale deployed paths via set difference.
• Extended apm install to 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

[Boubalou]

Addressed the review. Two new commits on the branch:

c17ef3a - encoding fixes

Replaced U+2014 (em dash) and U+2500 (box drawing light horizontal) with ASCII equivalents (--) in every line this PR adds — in src/apm_cli/drift.py, src/apm_cli/commands/install.py, tests/unit/test_stale_file_detection.py, and tests/integration/test_diff_aware_install_e2e.py. Verified by scanning the full diff for codepoints > U+007E — zero remaining in additions. Pre-existing violations on main (e.g. in the existing drift.py module docstring and in the other test classes) are left untouched so this PR stays scoped to the feature.

8030653 - docs update for Starlight

Added a bullet under the "Diff-Aware Installation (manifest as source of truth)" section in docs/src/content/docs/reference/cli-commands.md, documenting that files from a still-present package no longer produced on the next install are removed from disk and from apm.lock.yaml. Slotted right after the existing "packages removed from apm.yml" bullet for parity.

On packages/apm-guide/.apm/skills/apm-usage/commands.md

Not updated. That file is a one-liner table (apm install [PKGS...] | description | flags). This PR doesn't add a flag or change the command shape — the behaviour change is already covered by the existing "Install packages" summary. Let me know if you want a line added anyway and I'll push it.

[danielmeppiel]

Removing this file level block and adding it three times below does not seem appropriate