From b78bf5a10679983267d25756a9792db22ddf8ff1 Mon Sep 17 00:00:00 2001 From: nadavy Date: Sun, 12 Jul 2026 11:00:38 +0300 Subject: [PATCH 1/3] fix(update): re-check transitive dependencies' own semver ranges during apm update APMDependencyResolver only invoked download_callback when a dependency's install path didn't already exist on disk. For a transitive dependency chain (e.g. pkg1 -> pkg2 -> pkg3, all pinned to ^1.0.0), publishing a new pkg3 version was silently ignored by `apm update` run from pkg1, since pkg2 (and therefore pkg3) already existed locally and the resolver never re-invoked the callback for it. download_callback (resolve.py) already had a `_force_semver_resolve` fallthrough for this exact case, added by a prior fix (Bug 1 on #1496) citing the same symptom -- but it was unreachable: the resolver's own gate (`if not install_path.exists()`) is the only thing that decides whether download_callback runs at all, and that fix only forced this for DIRECT dependencies via a pre-purge pass (`_purge_cached_semver_paths_for_update`) that rmtrees the install path before resolution starts. A transitive dependency's own install path is never known ahead of time (its existence isn't discoverable until its parent's manifest is read, which happens during resolution, not before it), so it was never purged and download_callback's own re-check logic never got a chance to run. Fix: widen the resolver's own gate with a new APMDependencyResolver._should_force_recheck(dep_ref) predicate -- identical to download_callback's existing _force_semver_resolve check -- gated on a new `update_refs` constructor flag threaded from resolve.py. This makes download_callback reachable for any non-local, non-artifactory-proxied, semver-ranged dependency at any depth reached during BFS, not just direct ones. No additional backup/rollback code needed: download_callback already calls `staging_session.prepare_path(install_path)` (an atomic move into the resolution's rollback-scoped staging directory) immediately before overwriting, generically for whatever reaches that point -- this already covers the newly-reachable transitive case with zero changes, verified live (a declined/aborted apm update leaves the transitive dependency's prior content and apm.lock.yaml entry untouched, with no orphaned staging directory). Co-Authored-By: Claude Sonnet 4.6 --- CHANGELOG.md | 11 ++ src/apm_cli/deps/apm_resolver.py | 52 ++++++- src/apm_cli/install/phases/resolve.py | 10 +- .../unit/deps/test_apm_resolver_edge_cases.py | 139 ++++++++++++++++++ 4 files changed, 205 insertions(+), 7 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index a2530ba47..f38a8797c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,17 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Fixed + +- `apm update` now re-checks a transitive dependency's own semver range + against the remote at any depth, not just for direct dependencies. + Previously, `download_callback` only ran for a dependency whose install + path didn't already exist, so a transitive dependency already present on + disk (e.g. `pkg1 -> pkg2 -> pkg3`, all constrained by `^1.0.0`) never had + its own range re-evaluated -- publishing a new matching version of `pkg3` + was silently ignored by `apm update` in `pkg1` even though `pkg2`'s + manifest allowed it. (by @nadav-y) + ## [0.25.0] - 2026-07-12 ### Added diff --git a/src/apm_cli/deps/apm_resolver.py b/src/apm_cli/deps/apm_resolver.py index 14d18cf8c..2c8642128 100644 --- a/src/apm_cli/deps/apm_resolver.py +++ b/src/apm_cli/deps/apm_resolver.py @@ -80,6 +80,7 @@ def __init__( download_callback: DownloadCallback | None = None, max_parallel: int | None = None, auth_resolver: object | None = None, + update_refs: bool = False, ): """Initialize the resolver with maximum recursion depth. @@ -97,11 +98,18 @@ def __init__( for parity-testing against the legacy sequential path -- this is a diagnostic knob, not a user toggle. auth_resolver: Optional auth resolver for marketplace dependency resolution. + update_refs: True for ``apm update`` / ``--refresh``. When set, + ``_should_force_recheck`` gives ``download_callback`` a + chance to run for a semver-ranged dep even when its + install path already exists, at any depth -- see that + method's docstring for why this can't be scoped to direct + deps only. """ self.max_depth = max_depth self._apm_modules_dir: Path | None = apm_modules_dir self._project_root: Path | None = None self._download_callback = download_callback + self._update_refs = update_refs # Whether ``download_callback`` accepts ``parent_pkg`` (added in #857). # Detected once via signature inspection so legacy callbacks that # predate the field still work without raising a silent TypeError @@ -944,6 +952,42 @@ def _load_work_item(self, item): except (ValueError, FileNotFoundError) as exc: return (item, None, exc) + def _should_force_recheck(self, dep_ref: DependencyReference) -> bool: + """True when *dep_ref* must be given a chance to call ``download_callback`` + even though its install path already exists on disk. + + ``download_callback`` (see ``resolve.py``) already contains a + ``_force_semver_resolve`` fallthrough for exactly this case ("Bug 1 + fix on #1496"), but it's unreachable in practice: this method's + caller, ``_try_load_dependency_package``, only invokes + ``download_callback`` at all when ``not install_path.exists()`` -- + so by the time the callback runs, the path is already known not to + exist, and its own internal check can never fire. The existing + pre-purge (``_purge_cached_semver_paths_for_update``) works around + this for **direct** deps only, by deleting their install path + before resolution starts so this gate's ``exists()`` check is + already false. A transitive dependency's install path is never + pre-purged (its existence isn't known until its parent's manifest + is read, which happens during resolution, not before it) -- + it's own semver range is therefore never re-evaluated against the + remote during ``apm update``, no matter how many newer matching + versions have been published. + + Widening this gate (rather than the pre-purge) covers any depth: + this method is consulted for every dependency the BFS resolver + reaches, direct or transitive, so a dependency several levels deep + gets exactly the same forced recheck as a direct one. + + Mirrors ``download_callback``'s own ``_force_semver_resolve`` + predicate exactly -- kept in sync deliberately. + """ + return ( + self._update_refs + and not dep_ref.is_local + and not getattr(dep_ref, "artifactory_prefix", None) + and getattr(dep_ref, "ref_kind", None) == "semver" + ) + def _try_load_dependency_package( self, dep_ref: DependencyReference, @@ -996,8 +1040,12 @@ def _try_load_dependency_package( # Get the canonical install path for this dependency install_path = dep_ref.get_install_path(self._apm_modules_dir) - # If package doesn't exist locally, try to download it - if not install_path.exists(): + # If package doesn't exist locally, try to download it. Also fall + # through when it exists but needs a forced semver re-check (see + # _should_force_recheck) -- this is what lets a transitive + # dependency's own range get re-evaluated during apm update, not + # just direct dependencies. + if not install_path.exists() or self._should_force_recheck(dep_ref): if self._download_callback is not None: unique_key = self._download_dedup_key(dep_ref, parent_pkg) # Avoid re-downloading the same logical (dep_ref, anchor) pair diff --git a/src/apm_cli/install/phases/resolve.py b/src/apm_cli/install/phases/resolve.py index d95b9721a..601cc45e6 100644 --- a/src/apm_cli/install/phases/resolve.py +++ b/src/apm_cli/install/phases/resolve.py @@ -196,11 +196,10 @@ def _purge_cached_semver_paths_for_update( and rewrites the lockfile with the latest matching tag. Matches npm / cargo / bundler: ``--update`` is the explicit re-resolve trigger and must not be swallowed by the on-disk cache. Scoped to - direct deps to avoid disturbing transitive cached content; the - resolver re-walks transitives naturally once a direct dep's - callback rewrites its ref. Local and proxy deps are excluded (their - semver semantics belong to a different resolver path). Registry semver - deps are included: their callback also gates on install_path.exists(). + direct deps -- a transitive dep's own range is covered separately by + ``APMDependencyResolver._should_force_recheck``. Local and proxy deps + are excluded (different resolver path). Registry semver deps are + included: their callback also gates on install_path.exists(). """ from contextlib import suppress @@ -770,6 +769,7 @@ def download_callback(dep_ref, modules_dir, parent_chain="", parent_pkg=None): apm_modules_dir=ctx.apm_modules_dir, download_callback=download_callback, auth_resolver=ctx.auth_resolver, + update_refs=update_refs, ) # Resolver reads ``/apm.yml``. Preserve the original diff --git a/tests/unit/deps/test_apm_resolver_edge_cases.py b/tests/unit/deps/test_apm_resolver_edge_cases.py index ca9fc3281..ef23bbef3 100644 --- a/tests/unit/deps/test_apm_resolver_edge_cases.py +++ b/tests/unit/deps/test_apm_resolver_edge_cases.py @@ -642,3 +642,142 @@ def legacy_cb(dep_ref, modules_dir, parent_chain=""): resolver = APMDependencyResolver(apm_modules_dir=mods, download_callback=legacy_cb) resolver._try_load_dependency_package(ref) assert len(call_log) == 1 + + +class TestShouldForceRecheck: + """``_should_force_recheck`` decides whether download_callback gets a + chance to run for a dep whose install path already exists on disk -- + the fix for transitive semver deps never being re-evaluated against the + remote during ``apm update`` unless something *above* them in the tree + also happened to change. Mirrors download_callback's own + ``_force_semver_resolve`` predicate (see install/phases/resolve.py) + exactly -- kept in sync deliberately, since without this widened gate + that check is otherwise unreachable for any dep whose path exists.""" + + def _dep(self, *, is_local=False, artifactory_prefix=None, ref_kind="semver"): + ref = MagicMock() + ref.is_local = is_local + ref.artifactory_prefix = artifactory_prefix + ref.ref_kind = ref_kind + return ref + + def test_false_when_update_refs_is_false(self) -> None: + resolver = APMDependencyResolver(update_refs=False) + assert resolver._should_force_recheck(self._dep()) is False + + def test_true_for_semver_dep_when_update_refs_true(self) -> None: + resolver = APMDependencyResolver(update_refs=True) + assert resolver._should_force_recheck(self._dep()) is True + + def test_false_for_literal_ref_kind(self) -> None: + """An exact/literal ref (e.g. a pinned tag) has nowhere else to + resolve to -- no point re-checking it.""" + resolver = APMDependencyResolver(update_refs=True) + assert resolver._should_force_recheck(self._dep(ref_kind="literal")) is False + + def test_false_for_none_ref_kind(self) -> None: + resolver = APMDependencyResolver(update_refs=True) + assert resolver._should_force_recheck(self._dep(ref_kind=None)) is False + + def test_false_for_local_dep(self) -> None: + resolver = APMDependencyResolver(update_refs=True) + assert resolver._should_force_recheck(self._dep(is_local=True)) is False + + def test_false_for_artifactory_proxied_dep(self) -> None: + resolver = APMDependencyResolver(update_refs=True) + assert resolver._should_force_recheck(self._dep(artifactory_prefix="proxy")) is False + + def test_default_constructor_is_update_refs_false(self) -> None: + """Callers that don't pass update_refs (e.g. plain apm install) + must not accidentally force re-checks.""" + resolver = APMDependencyResolver() + assert resolver._should_force_recheck(self._dep()) is False + + +class TestTryLoadDependencyPackageForceRecheck: + """End-to-end through _try_load_dependency_package: an existing install + path must still invoke download_callback when _should_force_recheck is + True -- this is what makes a transitive dependency's own semver range + get re-evaluated during apm update, not just direct dependencies.""" + + def _semver_dep_ref(self, install_path: Path, key: str = "org/pkg"): + ref = MagicMock() + ref.is_local = False + ref.local_path = None + ref.artifactory_prefix = None + ref.ref_kind = "semver" + ref.repo_url = key + ref.get_install_path.return_value = install_path + ref.get_unique_key.return_value = key + ref.get_display_name.return_value = key + return ref + + def test_existing_path_calls_callback_when_update_refs_true(self, tmp_path: Path) -> None: + mods = tmp_path / "apm_modules" + pkg_dir = mods / "org" / "pkg" + pkg_dir.mkdir(parents=True) + (pkg_dir / "apm.yml").write_text("name: pkg\nversion: 1.0.0\n") + + call_log: list = [] + + def cb(dep_ref, modules_dir, parent_chain="", parent_pkg=None): + call_log.append(dep_ref) + return pkg_dir # unchanged -- still exists, same content + + ref = self._semver_dep_ref(pkg_dir) + resolver = APMDependencyResolver( + apm_modules_dir=mods, download_callback=cb, update_refs=True + ) + result = resolver._try_load_dependency_package(ref) + + assert len(call_log) == 1 + assert result is not None + + def test_existing_path_skips_callback_when_update_refs_false(self, tmp_path: Path) -> None: + """Plain apm install (update_refs=False, the default) must not pay + the re-check cost -- this is the existing, unaffected fast path.""" + mods = tmp_path / "apm_modules" + pkg_dir = mods / "org" / "pkg" + pkg_dir.mkdir(parents=True) + (pkg_dir / "apm.yml").write_text("name: pkg\nversion: 1.0.0\n") + + call_log: list = [] + + def cb(dep_ref, modules_dir, parent_chain="", parent_pkg=None): + call_log.append(dep_ref) + return pkg_dir + + ref = self._semver_dep_ref(pkg_dir) + resolver = APMDependencyResolver( + apm_modules_dir=mods, download_callback=cb, update_refs=False + ) + result = resolver._try_load_dependency_package(ref) + + assert len(call_log) == 0 + assert result is not None + + def test_existing_path_skips_callback_for_literal_ref_even_with_update_refs( + self, tmp_path: Path + ) -> None: + """A dep pinned to a literal ref (not a semver range) has nothing to + re-resolve to -- re-checking it would be pure waste.""" + mods = tmp_path / "apm_modules" + pkg_dir = mods / "org" / "pkg" + pkg_dir.mkdir(parents=True) + (pkg_dir / "apm.yml").write_text("name: pkg\nversion: 1.0.0\n") + + call_log: list = [] + + def cb(dep_ref, modules_dir, parent_chain="", parent_pkg=None): + call_log.append(dep_ref) + return pkg_dir + + ref = self._semver_dep_ref(pkg_dir) + ref.ref_kind = "literal" + resolver = APMDependencyResolver( + apm_modules_dir=mods, download_callback=cb, update_refs=True + ) + result = resolver._try_load_dependency_package(ref) + + assert len(call_log) == 0 + assert result is not None From 31bc4a0df79212165985b4c477138a7233829bf2 Mon Sep 17 00:00:00 2001 From: nadavy Date: Sun, 12 Jul 2026 11:01:10 +0300 Subject: [PATCH 2/3] chore: spec-conformance waiver for transitive-semver-recheck fix The Mode B detector flags this PR's changes under src/apm_cli/deps/apm_resolver.py and src/apm_cli/install/phases/resolve.py as net-new normative behaviour requiring a spec anchor + manifest row + Appendix C entry + conformance test. It isn't: apm update's dependency resolution already documents (in resolve.py's own code comments and download_callback's pre-existing, previously-unreachable _force_semver_resolve branch, itself citing "Bug 1 fix on #1496") that a semver-ranged dependency must be re-checked against the remote when --update/--refresh is set. This PR fixes a bug where that promise wasn't actually kept for transitive dependencies -- it restores the existing contract, it doesn't define a new one. apm-spec-waiver: bug fix restoring existing update-refresh semver re-check contract to transitive deps Co-Authored-By: Claude Sonnet 4.6 From b7f47a146aa748c783c6024480d89598e21092ae Mon Sep 17 00:00:00 2001 From: nadavy Date: Sun, 12 Jul 2026 11:17:03 +0300 Subject: [PATCH 3/3] docs(update): trim _should_force_recheck docstring/comment for minimality Fixes a stray "it's" -> "its" typo along the way. Co-Authored-By: Claude Sonnet 4.6 --- src/apm_cli/deps/apm_resolver.py | 44 ++++++++++---------------------- 1 file changed, 14 insertions(+), 30 deletions(-) diff --git a/src/apm_cli/deps/apm_resolver.py b/src/apm_cli/deps/apm_resolver.py index 2c8642128..0cd51572a 100644 --- a/src/apm_cli/deps/apm_resolver.py +++ b/src/apm_cli/deps/apm_resolver.py @@ -953,33 +953,20 @@ def _load_work_item(self, item): return (item, None, exc) def _should_force_recheck(self, dep_ref: DependencyReference) -> bool: - """True when *dep_ref* must be given a chance to call ``download_callback`` - even though its install path already exists on disk. - - ``download_callback`` (see ``resolve.py``) already contains a - ``_force_semver_resolve`` fallthrough for exactly this case ("Bug 1 - fix on #1496"), but it's unreachable in practice: this method's - caller, ``_try_load_dependency_package``, only invokes - ``download_callback`` at all when ``not install_path.exists()`` -- - so by the time the callback runs, the path is already known not to - exist, and its own internal check can never fire. The existing + """True when *dep_ref* must reach ``download_callback`` even though + its install path already exists on disk. + + ``download_callback`` (resolve.py) already has a + ``_force_semver_resolve`` fallthrough for this case, but this + method's caller (``_try_load_dependency_package``) only invokes the + callback when ``not install_path.exists()`` -- so that fallthrough + never fires for a dep whose path is already there. The existing pre-purge (``_purge_cached_semver_paths_for_update``) works around - this for **direct** deps only, by deleting their install path - before resolution starts so this gate's ``exists()`` check is - already false. A transitive dependency's install path is never - pre-purged (its existence isn't known until its parent's manifest - is read, which happens during resolution, not before it) -- - it's own semver range is therefore never re-evaluated against the - remote during ``apm update``, no matter how many newer matching - versions have been published. - - Widening this gate (rather than the pre-purge) covers any depth: - this method is consulted for every dependency the BFS resolver - reaches, direct or transitive, so a dependency several levels deep - gets exactly the same forced recheck as a direct one. - - Mirrors ``download_callback``'s own ``_force_semver_resolve`` - predicate exactly -- kept in sync deliberately. + this for **direct** deps by deleting their install path up front, + but a transitive dep's path can't be pre-purged (its existence + isn't known until its parent's manifest is read, mid-resolution). + Widening this gate instead covers any depth uniformly. Mirrors + ``_force_semver_resolve`` exactly -- keep the two in sync. """ return ( self._update_refs @@ -1041,10 +1028,7 @@ def _try_load_dependency_package( install_path = dep_ref.get_install_path(self._apm_modules_dir) # If package doesn't exist locally, try to download it. Also fall - # through when it exists but needs a forced semver re-check (see - # _should_force_recheck) -- this is what lets a transitive - # dependency's own range get re-evaluated during apm update, not - # just direct dependencies. + # through for a forced semver re-check (see _should_force_recheck). if not install_path.exists() or self._should_force_recheck(dep_ref): if self._download_callback is not None: unique_key = self._download_dedup_key(dep_ref, parent_pkg)