From 5ce07b462b9e41719906e34d7eb3af692136d706 Mon Sep 17 00:00:00 2001 From: sergio-sisternes-epam Date: Fri, 6 Mar 2026 23:13:39 +0000 Subject: [PATCH 1/2] fix: --update flag now bypasses lockfile SHA to fetch latest content (#190) Gate lockfile SHA overrides behind `not update_refs` so that `apm install --update` actually re-fetches the latest content instead of re-downloading the stale pinned commit. Three conditions were fixed in _install_apm_dependencies(): - Pre-download ref construction: skip lockfile override when updating - Sequential download ref construction: same guard added - skip_download condition: already_resolved no longer bypasses --update Adds 14 unit tests covering the corrected logic. --- src/apm_cli/cli.py | 6 +- tests/unit/test_install_update.py | 236 ++++++++++++++++++++++++++++++ 2 files changed, 239 insertions(+), 3 deletions(-) create mode 100644 tests/unit/test_install_update.py diff --git a/src/apm_cli/cli.py b/src/apm_cli/cli.py index 7fa71762..469b2aaa 100644 --- a/src/apm_cli/cli.py +++ b/src/apm_cli/cli.py @@ -1805,7 +1805,7 @@ def download_callback(dep_ref, modules_dir): pass # Build download ref (use locked commit for reproducibility) _pd_dlref = str(_pd_ref) - if existing_lockfile: + if existing_lockfile and not update_refs: _pd_locked = existing_lockfile.get_dependency(_pd_key) if _pd_locked and _pd_locked.resolved_commit and _pd_locked.resolved_commit != "cached": _pd_base = _pd_ref.repo_url @@ -1902,7 +1902,7 @@ def download_callback(dep_ref, modules_dir): except Exception: pass # Not a git repo or invalid — fall through to download skip_download = install_path.exists() and ( - (is_cacheable and not update_refs) or already_resolved or lockfile_match + (is_cacheable and not update_refs) or (already_resolved and not update_refs) or lockfile_match ) if skip_download: @@ -2167,7 +2167,7 @@ def download_callback(dep_ref, modules_dir): # T5: Build download ref - use locked commit if available download_ref = str(dep_ref) - if existing_lockfile: + if existing_lockfile and not update_refs: locked_dep = existing_lockfile.get_dependency(dep_ref.get_unique_key()) if locked_dep and locked_dep.resolved_commit and locked_dep.resolved_commit != "cached": # Override with locked commit for reproducible install diff --git a/tests/unit/test_install_update.py b/tests/unit/test_install_update.py new file mode 100644 index 00000000..57694fd0 --- /dev/null +++ b/tests/unit/test_install_update.py @@ -0,0 +1,236 @@ +"""Tests for --update flag behavior in install command (Bug #190). + +Verifies that `apm install --update` bypasses lockfile-pinned SHAs +and re-fetches the latest content, especially for subdirectory packages. +""" + +import pytest +from pathlib import Path +from unittest.mock import Mock, patch, MagicMock + +from apm_cli.models.apm_package import ( + DependencyReference, + GitReferenceType, +) + + +class TestSkipDownloadWithUpdateFlag: + """Test that skip_download respects --update (update_refs=True). + + The skip_download condition must NOT skip when update_refs is True, + even if the package was already resolved by the BFS callback. + """ + + def _build_skip_download(self, *, install_path_exists, is_cacheable, update_refs, + already_resolved, lockfile_match): + """Reproduce the skip_download condition from cli.py.""" + return install_path_exists and ( + (is_cacheable and not update_refs) or (already_resolved and not update_refs) or lockfile_match + ) + + def test_already_resolved_skips_without_update(self): + """Without --update, already_resolved packages should be skipped.""" + assert self._build_skip_download( + install_path_exists=True, + is_cacheable=False, + update_refs=False, + already_resolved=True, + lockfile_match=False, + ) is True + + def test_already_resolved_does_not_skip_with_update(self): + """With --update, already_resolved packages must NOT be skipped.""" + assert self._build_skip_download( + install_path_exists=True, + is_cacheable=False, + update_refs=True, + already_resolved=True, + lockfile_match=False, + ) is False + + def test_cacheable_skips_without_update(self): + """Without --update, cacheable (tag/commit) packages should be skipped.""" + assert self._build_skip_download( + install_path_exists=True, + is_cacheable=True, + update_refs=False, + already_resolved=False, + lockfile_match=False, + ) is True + + def test_cacheable_does_not_skip_with_update(self): + """With --update, cacheable packages must NOT be skipped.""" + assert self._build_skip_download( + install_path_exists=True, + is_cacheable=True, + update_refs=True, + already_resolved=False, + lockfile_match=False, + ) is False + + def test_lockfile_match_always_skips(self): + """lockfile_match should always skip (not gated by update_refs because + the lockfile_match check itself is already gated by `not update_refs`).""" + assert self._build_skip_download( + install_path_exists=True, + is_cacheable=False, + update_refs=True, + already_resolved=False, + lockfile_match=True, + ) is True + + def test_no_install_path_never_skips(self): + """If install path doesn't exist, never skip regardless of other flags.""" + assert self._build_skip_download( + install_path_exists=False, + is_cacheable=True, + update_refs=False, + already_resolved=True, + lockfile_match=True, + ) is False + + +class TestDownloadRefLockfileOverride: + """Test that lockfile SHA override is gated behind `not update_refs`. + + When --update is used, the download ref should NOT be overridden with + the lockfile's pinned SHA. The package should be fetched at its + original reference (or default branch). + """ + + @staticmethod + def _build_download_ref(dep_ref, existing_lockfile, update_refs): + """Reproduce the download_ref construction logic from cli.py. + + This mirrors the sequential download path. The same logic applies + to the parallel pre-download path. + """ + download_ref = str(dep_ref) + if existing_lockfile and not update_refs: + locked_dep = existing_lockfile.get_dependency(dep_ref.get_unique_key()) + if locked_dep and locked_dep.resolved_commit and locked_dep.resolved_commit != "cached": + base_ref = dep_ref.repo_url + if dep_ref.virtual_path: + base_ref = f"{base_ref}/{dep_ref.virtual_path}" + download_ref = f"{base_ref}#{locked_dep.resolved_commit}" + return download_ref + + def _make_subdirectory_dep(self): + return DependencyReference( + repo_url="owner/monorepo", + host="github.com", + reference=None, + virtual_path="packages/my-skill", + is_virtual=True, + ) + + def _make_regular_dep(self): + return DependencyReference( + repo_url="owner/repo", + host="github.com", + reference="main", + ) + + def _mock_lockfile(self, dep_ref, resolved_commit="abc123def456"): + lockfile = Mock() + locked_dep = Mock() + locked_dep.resolved_commit = resolved_commit + lockfile.get_dependency = Mock(return_value=locked_dep) + return lockfile + + def test_subdirectory_lockfile_override_without_update(self): + """Without --update, subdirectory download ref uses locked SHA.""" + dep = self._make_subdirectory_dep() + lockfile = self._mock_lockfile(dep) + + ref = self._build_download_ref(dep, lockfile, update_refs=False) + assert "#abc123def456" in ref + assert ref == "owner/monorepo/packages/my-skill#abc123def456" + + def test_subdirectory_no_lockfile_override_with_update(self): + """With --update, subdirectory download ref must NOT use locked SHA.""" + dep = self._make_subdirectory_dep() + lockfile = self._mock_lockfile(dep) + + ref = self._build_download_ref(dep, lockfile, update_refs=True) + assert "#abc123def456" not in ref + assert ref == str(dep) + + def test_regular_lockfile_override_without_update(self): + """Without --update, regular package download ref uses locked SHA.""" + dep = self._make_regular_dep() + lockfile = self._mock_lockfile(dep) + + ref = self._build_download_ref(dep, lockfile, update_refs=False) + assert "#abc123def456" in ref + + def test_regular_no_lockfile_override_with_update(self): + """With --update, regular package download ref must NOT use locked SHA.""" + dep = self._make_regular_dep() + lockfile = self._mock_lockfile(dep) + + ref = self._build_download_ref(dep, lockfile, update_refs=True) + assert "#abc123def456" not in ref + + def test_no_lockfile_returns_original_ref(self): + """Without a lockfile, download ref is the original dependency string.""" + dep = self._make_subdirectory_dep() + ref = self._build_download_ref(dep, existing_lockfile=None, update_refs=False) + assert ref == str(dep) + + def test_cached_lockfile_entry_not_overridden(self): + """Lockfile entries with resolved_commit='cached' should not override.""" + dep = self._make_subdirectory_dep() + lockfile = self._mock_lockfile(dep, resolved_commit="cached") + + ref = self._build_download_ref(dep, lockfile, update_refs=False) + assert ref == str(dep) + + +class TestPreDownloadRefLockfileOverride: + """Same as TestDownloadRefLockfileOverride but for the parallel pre-download path.""" + + @staticmethod + def _build_pre_download_ref(dep_ref, existing_lockfile, update_refs): + """Reproduce the _pd_dlref construction logic from cli.py's pre-download loop.""" + _pd_dlref = str(dep_ref) + if existing_lockfile and not update_refs: + _pd_locked = existing_lockfile.get_dependency(dep_ref.get_unique_key()) + if _pd_locked and _pd_locked.resolved_commit and _pd_locked.resolved_commit != "cached": + _pd_base = dep_ref.repo_url + if dep_ref.virtual_path: + _pd_base = f"{_pd_base}/{dep_ref.virtual_path}" + _pd_dlref = f"{_pd_base}#{_pd_locked.resolved_commit}" + return _pd_dlref + + def _make_subdirectory_dep(self): + return DependencyReference( + repo_url="owner/monorepo", + host="github.com", + reference=None, + virtual_path="packages/my-skill", + is_virtual=True, + ) + + def _mock_lockfile(self, dep_ref, resolved_commit="abc123def456"): + lockfile = Mock() + locked_dep = Mock() + locked_dep.resolved_commit = resolved_commit + lockfile.get_dependency = Mock(return_value=locked_dep) + return lockfile + + def test_pre_download_no_lockfile_override_with_update(self): + """With --update, pre-download ref must NOT use locked SHA.""" + dep = self._make_subdirectory_dep() + lockfile = self._mock_lockfile(dep) + + ref = self._build_pre_download_ref(dep, lockfile, update_refs=True) + assert "#abc123def456" not in ref + + def test_pre_download_lockfile_override_without_update(self): + """Without --update, pre-download ref uses locked SHA.""" + dep = self._make_subdirectory_dep() + lockfile = self._mock_lockfile(dep) + + ref = self._build_pre_download_ref(dep, lockfile, update_refs=False) + assert "#abc123def456" in ref From e5e1659f8fa05110b8bc0b2adb0bb1c9b0e796d9 Mon Sep 17 00:00:00 2001 From: sergio-sisternes-epam Date: Fri, 6 Mar 2026 23:36:08 +0000 Subject: [PATCH 2/2] =?UTF-8?q?refactor:=20address=20Copilot=20review=20?= =?UTF-8?q?=E2=80=94=20revert=20already=5Fresolved=20gating,=20clean=20imp?= =?UTF-8?q?orts?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Revert already_resolved skip: the BFS callback already fetches fresh content when update_refs=True, so re-downloading is redundant. - Remove unused imports from test file (pytest, Path, patch, MagicMock, GitReferenceType). --- src/apm_cli/cli.py | 2 +- tests/unit/test_install_update.py | 26 ++++++++++++++------------ 2 files changed, 15 insertions(+), 13 deletions(-) diff --git a/src/apm_cli/cli.py b/src/apm_cli/cli.py index 469b2aaa..75ef6537 100644 --- a/src/apm_cli/cli.py +++ b/src/apm_cli/cli.py @@ -1902,7 +1902,7 @@ def download_callback(dep_ref, modules_dir): except Exception: pass # Not a git repo or invalid — fall through to download skip_download = install_path.exists() and ( - (is_cacheable and not update_refs) or (already_resolved and not update_refs) or lockfile_match + (is_cacheable and not update_refs) or already_resolved or lockfile_match ) if skip_download: diff --git a/tests/unit/test_install_update.py b/tests/unit/test_install_update.py index 57694fd0..f7934a72 100644 --- a/tests/unit/test_install_update.py +++ b/tests/unit/test_install_update.py @@ -4,14 +4,9 @@ and re-fetches the latest content, especially for subdirectory packages. """ -import pytest -from pathlib import Path -from unittest.mock import Mock, patch, MagicMock +from unittest.mock import Mock -from apm_cli.models.apm_package import ( - DependencyReference, - GitReferenceType, -) +from apm_cli.models.apm_package import DependencyReference class TestSkipDownloadWithUpdateFlag: @@ -23,9 +18,15 @@ class TestSkipDownloadWithUpdateFlag: def _build_skip_download(self, *, install_path_exists, is_cacheable, update_refs, already_resolved, lockfile_match): - """Reproduce the skip_download condition from cli.py.""" + """Reproduce the skip_download condition from cli.py. + + Note: ``already_resolved`` is intentionally NOT gated by ``update_refs``. + When the BFS resolver callback downloads a package during this run it is + always a fresh fetch (the callback itself skips lockfile overrides when + ``update_refs=True``), so re-downloading would be redundant. + """ return install_path_exists and ( - (is_cacheable and not update_refs) or (already_resolved and not update_refs) or lockfile_match + (is_cacheable and not update_refs) or already_resolved or lockfile_match ) def test_already_resolved_skips_without_update(self): @@ -38,15 +39,16 @@ def test_already_resolved_skips_without_update(self): lockfile_match=False, ) is True - def test_already_resolved_does_not_skip_with_update(self): - """With --update, already_resolved packages must NOT be skipped.""" + def test_already_resolved_still_skips_with_update(self): + """With --update, already_resolved packages are still skipped because + the BFS callback already fetched them fresh in this run.""" assert self._build_skip_download( install_path_exists=True, is_cacheable=False, update_refs=True, already_resolved=True, lockfile_match=False, - ) is False + ) is True def test_cacheable_skips_without_update(self): """Without --update, cacheable (tag/commit) packages should be skipped."""