fix(update): re-check transitive dependencies' own semver ranges during apm update#2053
Open
nadav-y wants to merge 6 commits into
Open
fix(update): re-check transitive dependencies' own semver ranges during apm update#2053nadav-y wants to merge 6 commits into
nadav-y wants to merge 6 commits into
Conversation
…ed/dry-run apm update cannot leave apm_modules/ ahead of apm.lock.yaml download_callback materializes a re-resolved semver dep's new content to apm_modules/ during the resolve phase, before apm update's plan-confirmation gate runs. When that gate does not commit -- the user declines, the shell is non-interactive (no TTY, no --yes), or --dry-run is passed -- the new content was already written to disk while apm.lock.yaml stayed on the old version, silently leaving the project in a torn state. Reproduced live for both registry-sourced and git-sourced dependencies. _purge_cached_semver_paths_for_update (moved to the new update_backup.py, which also keeps install/phases/resolve.py under its LOC budget) now moves a semver dep's existing install path aside instead of deleting it outright, but only when apm update's plan_callback is present -- apm install --update has no decline path and keeps the original delete-and-redownload behavior unchanged. Once the plan-confirmation gate resolves, pipeline.py's new try/finally calls restore_update_backups: on commit, backups for successfully re-downloaded deps are discarded; otherwise every purged dep is reverted to its original content, and any freshly-downloaded dep with no prior backup (a new add swept into the same resolve pass) is removed outright. The finally block runs even when the non-interactive path raises SystemExit, so the rollback cannot be skipped by that abort. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…ups on resolve-phase failure, fix collision-prone naming
Four real issues from the automated Copilot review, all confirmed by reading
the actual code paths before fixing:
- restore_update_backups was only ever invoked from the plan-confirmation
gate in pipeline.py. If resolver.resolve_dependencies() itself raises
(e.g. a network error on a *different* dep, or a bad transitive manifest)
after purge_cached_semver_paths_for_update already moved some dep aside,
nothing restored it -- the backup was orphaned in .apm-update-backup/
with the dep's install path left missing. Added a try/except around the
resolve phase call in pipeline.py that restores staged backups before
re-raising, with two new pipeline tests covering both the failure path
and the (still correct) no-op when no backups were staged.
- restore_update_backups looked up dep objects via ctx.deps_to_install,
which is only populated after resolve_dependencies() returns
successfully -- exactly the failure window above. Switched to
ctx.all_apm_deps (populated before resolve starts, and guaranteed to
contain every dep purge_cached_semver_paths_for_update could have
purged) merged with ctx.deps_to_install (extends coverage to transitive
adds when resolution did complete).
- _sanitize_backup_name collapsed distinct dep keys to the same backup
directory name (e.g. "owner/repo" and "owner_repo" both became
"owner_repo"), risking one dep's backup silently overwriting another's.
Appended a short hash of the original key. New unit tests assert
distinct keys never collide.
- purge_cached_semver_paths_for_update logged "cleared cached install
path" unconditionally even when the rename/rmtree inside the
suppress(Exception) block actually failed, misleadingly implying
semver re-resolution would occur when it might not. Now only logs on
confirmed success.
Also added type hints (per repo convention) and fixed a stale docstring
reference to the old (now-public, no-underscore) function name.
The isinstance-based coercion this required in restore_update_backups
(rather than a plain `... or {}` fallback) is defensive against any
caller whose ctx doesn't populate these fields as real dicts/lists --
irrelevant in production (InstallContext's dataclass defaults guarantee
this), but it also happens to make the function robust against loosely-
mocked ctx objects in unrelated tests that now incidentally reach this
code path via the new pipeline.py exception handler.
Full unit suite: 18162 passed, 6 pre-existing failures (confirmed
identical on main, unrelated to this change), 0 new failures. ruff clean.
Re-verified the live repro (registry + git-sourced, decline/non-interactive/
dry-run/happy-path) end-to-end after every change in this commit.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
The Mode B detector flags this PR's changes under src/apm_cli/install/ as net-new normative behaviour requiring a spec anchor + manifest row + Appendix C entry + conformance test. It isn't: apm update's plan- confirmation gate already documents (in its own code comment) that confirmation happens "before downloads begin so a 'no' answer cancels cleanly without touching the cache." This PR fixes a bug where that promise wasn't actually kept for semver re-resolution -- it restores the existing contract, it doesn't define a new one. apm-spec-waiver: bug fix restoring existing update-confirm-gate no-side-effect contract
…ng apm update APMDependencyResolver only invoked download_callback when a dependency's install path didn't already exist on disk. The prior fix for the plan-confirmation torn-state bug (microsoft#2050) forced this recheck for direct dependencies only (a pre-purge pass before BFS resolution starts), so any transitive dependency's own semver range was never re-evaluated against the remote, no matter how many newer matching versions had been published -- e.g. pkg1 -> pkg2 -> pkg3, all pinned to ^1.0.0: publishing pkg3 1.0.4 was silently ignored by `apm update` in pkg1, since pkg2 (and therefore pkg3) already existed locally. Replace the direct-only pre-purge with a per-node resolver gate (APMDependencyResolver._should_force_recheck, threaded via a new update_refs constructor flag) that widens the existing-path check for any non-local, non-artifactory-proxied, semver-ranged dependency -- covering every depth reached during BFS, not just direct deps. Move the backup-before-overwrite staging inline into download_callback (update_backup.backup_before_overwrite), since a transitive dependency's existence isn't known until its parent's manifest is read -- a pre-pass can't know what to back up ahead of time. ctx.update_backups now stores (dep_ref, backup_path) tuples so a transitive dep's backup can be restored even if it never appears in ctx.all_apm_deps or ctx.deps_to_install (e.g. resolution fails before those populate). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
nadav-y
requested review from
danielmeppiel and
sergio-sisternes-epam
as code owners
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR fixes apm update so that semver-ranged dependencies are re-evaluated against the remote at any depth (including transitives), and extends the staged-backup/rollback mechanism so update-plan confirmation can safely decline/abort without leaving apm_modules/ ahead of apm.lock.yaml.
Changes:
- Widen dependency resolution so existing install paths can still invoke
download_callbackwhenupdate_refs=Trueand the dep is a non-local, non-proxied semver ref. - Move update rollback staging to an inline
backup_before_overwrite()call insidedownload_callback, and reconcile staged backups at the plan-confirmation gate (plus early restore on resolve-phase exceptions). - Add/refresh unit tests for the new recheck predicate and the inline backup/restore behavior; add changelog entries for #2050 and this fix.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
src/apm_cli/deps/apm_resolver.py |
Adds update_refs + _should_force_recheck() and widens the existing-path gate so transitive semver deps can be rechecked. |
src/apm_cli/install/phases/resolve.py |
Removes direct-only pre-purge and stages backups inline before overwrites; threads update_refs into the resolver. |
src/apm_cli/install/phases/update_backup.py |
Introduces the inline backup + restore reconciliation helpers for update-plan gating. |
src/apm_cli/install/pipeline.py |
Plumbs plan_callback into context and ensures backups are reconciled on plan decision (and restored on resolve exceptions when staged). |
src/apm_cli/install/context.py |
Documents and adds plan_callback and the new update_backups shape on InstallContext. |
tests/unit/deps/test_apm_resolver_edge_cases.py |
Adds coverage for _should_force_recheck() and for callback invocation on existing paths when updating. |
tests/unit/install/phases/test_update_backup.py |
New unit tests for sanitization + inline staging + restore behavior (including a transitive-only restore case). |
tests/unit/test_install_pipeline_orchestration.py |
Adds coverage ensuring resolve-phase failures trigger restore when backups were staged. |
CHANGELOG.md |
Adds Unreleased fixes entries for #2050 and #2053. |
Addresses Copilot review on microsoft#2053: backup_before_overwrite suppressed every exception during staging and returned False, so a rename/mkdir failure on an existing install path let the caller proceed straight to overwriting it with no rollback point staged. A later declined/aborted update would then see that dep in ctx.callback_downloaded but absent from ctx.update_backups -- indistinguishable from a fresh add -- and delete it outright, permanently losing the original content. Now raises once staging is actually required (plan_callback set and install_path exists), instead of swallowing the error. The resolve-phase exception handler already added in pipeline.py (for microsoft#2050's Copilot review) restores any backups staged for other deps this run before re-raising, so the whole apm update aborts cleanly -- recoverable, unlike a silent, undetected loss of the original content. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
This was referenced Jul 7, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This branch's diff contains three bugs in
apm update's dependency resolution and rollback safety. Bug 1 was already fixed by #2050 and is only present here because this branch stacks on it. Bugs 2 and 3 are new in this PR.Bug 1 (fixed in #2050): torn state on declined/aborted update. Resolving a semver range downloads the candidate version to disk before the user confirms the update plan. A declined confirmation, a non-interactive abort, or
--dry-runused to leaveapm_modules/already advanced whileapm.lock.yamlstayed on the old version.Bug 2 (this PR): transitive dependencies never get re-checked.
apm updatewas not picking up a new version of a transitive dependency, even when the intermediate package's manifest still allowed it:Publishing a new
org/pkg3matching^1.0.0was silently ignored byapm updaterun fromorg/pkg1, even thoughorg/pkg2's manifest hadn't changed. Root cause:APMDependencyResolver._try_load_dependency_packageonly callsdownload_callback(which re-resolves a semver range against the remote) when the install path doesn't already exist:#2050 forced this recheck for direct dependencies only (a pre-purge pass that ran before BFS resolution started, since transitive deps aren't known until their parent's manifest is read). Once
org/pkg2exists locally,org/pkg3's own range is never re-evaluated -- the walk sees its path already exists and stops.Bug 3 (this PR, caught in Copilot review): a failed backup silently allowed an unprotected overwrite.
backup_before_overwriteswallowed every exception during staging and returnedFalse. If staging failed (e.g. a rename error) on an existing install path, the caller proceeded to overwrite it anyway with no rollback point. A later declined/aborted update would then see that dep as downloaded but with no backup entry -- indistinguishable from a fresh add -- and delete it outright, permanently losing the original content.Fix
apm_resolver.py: AddedAPMDependencyResolver._should_force_recheck(dep_ref), gated on a newupdate_refsflag. Widens the existing-path check toif not install_path.exists() or self._should_force_recheck(dep_ref):, sodownload_callbackruns for any non-local, non-artifactory-proxied, semver-ranged dependency at any depth. (fixes Bug 2)resolve.py: Removed the direct-only pre-purge pass (no longer needed); threadsupdate_refsinto the resolver.update_backup.py: Moved backup staging inline intodownload_callback, called immediately before a dependency's install path is overwritten, for direct and transitive deps alike -- sidesteps the chicken-and-egg problem of not knowing transitive deps ahead of time.ctx.update_backupsnow storesdep_key -> (dep_ref, backup_path)tuples so a transitive dep's backup survives even if it never reachesctx.all_apm_deps/ctx.deps_to_install. (fixes Bug 2's backup coverage)update_backup.py:backup_before_overwriteno longer suppresses staging exceptions -- it now raises when a backup is actually required, so the caller never proceeds to overwrite without one. The existing resolve-phase exception handler inpipeline.pyrestores any backups already staged for other deps this run before re-raising, so the whole update aborts cleanly instead of silently losing content. (fixes Bug 3)context.py/pipeline.py: Comment updates only, no functional change.A regression surfaced during development: widening only the resolver gate (without widening backup coverage) caused
restore_update_backupsto treat newly-reachable transitive deps as fresh adds with no backup, deleting them outright on decline. Caught via live repro before landing the inline-backup fix.Test plan
test_apm_resolver_edge_cases.py(TestShouldForceRecheck,TestTryLoadDependencyPackageForceRecheck) covering the recheck predicate and gate across semver/literal ref kinds, local deps, artifactory-proxied deps, andupdate_refson/off.test_update_backup.pyfor the inline, tuple-based API, including a transitive-dep-not-in-all_apm_deps regression test and a staging-failure test asserting it raises with the original content untouched.mcp_integrator/global_mcp_scope/lifecycle_executor), 0 new failures.^1.0.0: update reaches depth 3 across successive publishes.2.0.0against^1.0.0): correctly ignored.pkg1pinspkg2exactly,pkg2rangespkg3): innermost dep still updates through the exact-pinned intermediate.-y) andapm install --update(no confirm gate): both apply cleanly.Branch note
Stacked on #2050 (
fix/update-plan-gate-torn-state) -- sameupdate_backup.pymechanism, so the diff includes #2050's commits until it merges. Please review/merge #2050 first; commits unique to this PR start after2683e8ea.