Fix cross-package file protection in stale cleanup (#1831)#1856
Conversation
Add cross-package ownership check in the cleanup phase: before deleting a stale file, verify no other currently-installed package claims it in its deployed_files. This prevents incorrect deletion when two packages share a path and one is updated. Also add regression test for legacy lockfiles with empty deployed_files confirming the graceful-skip behaviour (no stale removal attempted, new deployed_files recorded for future diffs). Closes #1831 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
sergio-sisternes-epam
added
the
panel-review
There was a problem hiding this comment.
Pull request overview
This PR addresses a correctness gap in the install/update stale-file cleanup phase: when updating one package removes a file that still exists because another installed package also deploys it, cleanup must not delete the shared path.
Changes:
- Adds cross-package ownership filtering in
cleanup.run()to exclude paths still deployed by other packages from the stale deletion set. - Adds unit tests covering shared-path protection and the “legacy lockfile has empty deployed_files” skip behavior.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
src/apm_cli/install/phases/cleanup.py |
Introduces cross-package deployed-file union filtering before calling remove_stale_deployed_files. |
tests/unit/install/phases/test_cleanup_phase.py |
Adds regression tests for cross-package protection and legacy lockfile empty-deployed_files skip behavior. |
| other_deployed: set = set() | ||
| for other_key, other_files in package_deployed_files.items(): | ||
| if other_key != dep_key: | ||
| other_deployed.update(other_files) | ||
| stale = stale - other_deployed |
| ctx = _make_ctx( | ||
| existing_lockfile=lf, | ||
| intended_dep_keys={"pkg-a", "pkg-b"}, | ||
| package_deployed_files={ | ||
| "pkg-a": [], |
phillipschandler19-web
left a comment
phillipschandler19-web
left a comment
There was a problem hiding this comment.
- name: Download a Build Artifact
uses: actions/download-artifact@v3.1.0
with:
# Artifact name
name: # optional
# Destination path
path: # optional
dependencies:
apm:
# 1. Shorthand, github.com is implicit
- acme/standards#v1.2.0
# 2. Shorthand with explicit host (GHE Cloud, GHES, GitLab)
- acme.ghe.com/acme/standards#v1.2.0
- gitlab.com/acme/standards#v1.2.0
# 3. Azure DevOps shorthand: org/project/repo
- dev.azure.com/acme-org/platform/standards#v1.2.0
# 4. Object form (any git URL, any port, custom protocol)
- git: ssh://git@bitbucket.acme.com:7999/team/standards.git
ref: v1.2.0
# 5. Local path (file:// equivalent for unpacked bundles)
- ./vendor/standardshttps://opencode.ai/installgitlab.com/acme/standards#v1.2.0export GITHUB_APM_PAT=ghp_your_token
apm install
phillipschandler19-web
left a comment
phillipschandler19-web
left a comment
There was a problem hiding this comment.
sergio-sisternes-epam-fix-1831-stale-file-cleanup-on-update
| prev_dep = existing_lockfile.get_dependency(dep_key) | ||
| if not prev_dep: | ||
| continue # new package this install -- nothing stale yet | ||
| stale = detect_stale_files(prev_dep.deployed_files, new_deployed) |
There was a problem hiding this comment.
gitlab.com/acme/standards#v1.2.0https://opencode.ai/installgithub.event.client_payload.versions
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
Precompute deployed-file ownership, add verbose breadcrumbs for protected stale paths, and add an integration regression for the shared-file update scenario. Addresses panel follow-ups from python-architect, cli-logging-expert, supply-chain-security-expert, performance-expert, and test-coverage-expert. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Update the cleanup phase module docstring so it no longer claims the file is an unchanged extraction after the cross-package stale-file protection work. Addresses python-architect panel follow-up. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
APM Review Panel:
|
| Persona | B | R | N | Takeaway |
|---|---|---|---|---|
| Python Architect | 0 | 0 | 0 | Final cleanup flow is simple: precompute ownership once, filter stale paths, then delegate deletion to the canonical cleanup helper. |
| CLI Logging Expert | 0 | 0 | 0 | Default output stays quiet for non-deletions; verbose mode now explains protected stale paths. |
| DevX UX Expert | 0 | 0 | 0 | Shared-file updates now match package-manager user expectations: do not delete files still owned by another dependency. |
| Supply Chain Security Expert | 0 | 0 | 0 | The guard is reliability-oriented and explicitly does not replace content-hash deletion provenance. |
| OSS Growth Hacker | 0 | 0 | 0 | Strong community bug fix with concrete regression coverage. |
| Test Coverage Expert | 0 | 0 | 0 | Unit and integration-with-fixtures tests cover the regression; mutation-break proved the guard is load-bearing. |
| Performance Expert | 0 | 0 | 0 | Ownership union is precomputed once; overhead is bounded and negligible. |
Recommendation
Ship from the maintainer queue. No in-scope follow-ups remain from this shepherd pass.
Full per-persona findings
No remaining findings after the folded commits:
0c3fcddb486c8b282f27786e009650a96d3b2b02folded precomputation, verbose breadcrumbs, integration-tier coverage, and trust-boundary documentation.04e74e394765332414cbe7c1b10b62baba5887bcrefreshed the cleanup phase module context after the behavior change.
Folded in this run
- (panel) Precompute cross-package deployed-file ownership instead of rebuilding it per dependency -- resolved in
0c3fcddb486c8b282f27786e009650a96d3b2b02. - (panel) Add verbose-only diagnostics for stale paths kept because another package still deploys them -- resolved in
0c3fcddb486c8b282f27786e009650a96d3b2b02. - (panel) Document that
package_deployed_filesis integration outcome, not a security boundary -- resolved in0c3fcddb486c8b282f27786e009650a96d3b2b02. - (panel) Add integration-with-fixtures coverage for the two-local-package shared-file update scenario -- resolved in
0c3fcddb486c8b282f27786e009650a96d3b2b02. - (panel) Refresh the cleanup phase module docstring after the behavior change -- resolved in
04e74e394765332414cbe7c1b10b62baba5887bc.
Copilot signals reviewed
pull review 4534610106-- NOT-LEGIT: overview-only Copilot review; no actionable inline finding was present after two fetch rounds.
Regression-trap evidence (mutation-break gate)
tests/unit/install/phases/test_cleanup_phase.py::TestCrossPackageProtection::test_shared_file_not_removed_when_other_package_deploys_it-- deletedstale = stale - other_deployed; test FAILED as expected; guard restored.
Lint contract
uv run --extra dev ruff check src/ tests/ and
uv run --extra dev ruff format --check src/ tests/ both passed on the latest pushed head.
CI
gh pr checks 1856 --repo microsoft/apm --watch observed all required checks green on 04e74e394765332414cbe7c1b10b62baba5887bc after 0 CI fix iteration(s). CI run: https://github.com/microsoft/apm/actions/runs/27879757167
Mergeability status
Captured from gh pr view 1856 --json mergeable,mergeStateStatus,statusCheckRollup immediately after the last push of this run.
| PR | head SHA | CEO stance | iters | folds | defers | Copilot rounds | CI | mergeable | mergeStateStatus | notes |
|---|---|---|---|---|---|---|---|---|---|---|
| #1856 | 04e74e3 |
ship_now | 2 | 5 | 0 | 2 | green | MERGEABLE | BLOCKED | awaiting maintainer review |
Convergence
2 outer iteration(s); 2 Copilot round(s). Final panel recommendation: ship_now.
Ready for maintainer review.
This panel is advisory. It does not block merge. Re-apply the panel-review label after addressing feedback to re-run.
danielmeppiel
removed
the
panel-review
danielmeppiel
deleted the
sergio-sisternes-epam-fix-1831-stale-file-cleanup-on-update
branch
…#1871) Add the user-facing PRs merged after the v0.21.0 tag that were missing from the changelog (#1830 Added; #1854, #1856 Fixed), add the external- contributor credit on #1855, and move #1853 out of the released [0.21.0] section into [Unreleased] since it merged after the v0.21.0 tag. No version bump -- everything stays under [Unreleased]. Co-authored-by: danielmeppiel <danielmeppiel@users.noreply.github.com> Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Description
When
apm updatefetches a new version of a package where upstream deleted a file, the stale-file cleanup phase correctly removes the orphaned local copy. However, if another currently-installed package also deploys a file at the same path, that file could be incorrectly deleted because there was no cross-package ownership check.This PR adds a cross-package ownership guard to the cleanup phase: before passing stale files to deletion, it builds a union of all other packages'
deployed_filesand excludes any shared paths from the stale set. It also adds regression coverage for the legacy-lockfile graceful-skip behaviour (emptydeployed_filesfrom old APM versions).Fixes #1831
Type of change
Testing
New tests in
TestCrossPackageProtection:New tests in
TestLegacyLockfileGracefulSkip:deployed_files(legacy lockfile) gracefully skips stale cleanupSpec conformance (OpenAPM v0.1)