fix(update): re-check transitive dependencies' own semver ranges during apm update

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)
+
 ### Added
 
 - Corporate proxy and internal-CA users can now use Python-based APM HTTPS paths

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,29 @@ 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 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 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
+            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 +1027,9 @@ 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 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)
                 # Avoid re-downloading the same logical (dep_ref, anchor) pair

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 ``<anchor>/apm.yml``. Preserve the original

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