From df1d34bf7877cdeb9b2d39e283f398722377d42b Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Tue, 31 Mar 2026 04:21:51 +0000 Subject: [PATCH 1/5] Initial plan From 0f55310e3e1f22fa7248c2866fac7f3a54cdb944 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Tue, 31 Mar 2026 04:28:22 +0000 Subject: [PATCH 2/5] Initial plan for perf optimization Agent-Logs-Url: https://github.com/microsoft/apm/sessions/e100ba84-ca0e-4cbe-8ade-067d46ace7bf Co-authored-by: danielmeppiel <51440732+danielmeppiel@users.noreply.github.com> --- uv.lock | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/uv.lock b/uv.lock index 3246ec7c4..c35c123c1 100644 --- a/uv.lock +++ b/uv.lock @@ -179,7 +179,7 @@ wheels = [ [[package]] name = "apm-cli" -version = "0.8.6" +version = "0.8.7" source = { editable = "." } dependencies = [ { name = "click" }, From 9ebef37edd8b426092c14a401080a05d1074138d Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Tue, 31 Mar 2026 04:32:28 +0000 Subject: [PATCH 3/5] perf: skip download when resolved SHA matches lockfile in update mode When `apm deps update` resolves refs, compare the resolved SHA with the lockfile SHA before downloading. If they match, skip the download and integration entirely. This makes the common case (nothing changed) near-instant. Three choke points in install.py were gating lockfile access behind `not update_refs`: 1. Lockfile loading (line 1056) - now loads unconditionally 2. Locked dep lookup for SHA comparison - now loads unconditionally 3. Lockfile match check - now supports update_refs path via resolved SHA vs lockfile SHA comparison Also adds pre-download optimization: packages where the local HEAD matches the lockfile SHA are deferred to the sequential loop, which resolves the remote ref and compares before deciding to download. Agent-Logs-Url: https://github.com/microsoft/apm/sessions/e100ba84-ca0e-4cbe-8ade-067d46ace7bf Co-authored-by: danielmeppiel <51440732+danielmeppiel@users.noreply.github.com> --- CHANGELOG.md | 4 + .../content/docs/reference/lockfile-spec.md | 6 +- src/apm_cli/commands/install.py | 75 +++++-- tests/unit/test_install_update.py | 187 +++++++++++++++++- 4 files changed, 247 insertions(+), 25 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 27711e79a..6787971bf 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Changed + +- `apm deps update` now skips download and integration for packages whose resolved SHA matches the lockfile SHA, making the common "nothing changed" case near-instant (#495) + ## [0.8.7] - 2026-03-30 ### Fixed diff --git a/docs/src/content/docs/reference/lockfile-spec.md b/docs/src/content/docs/reference/lockfile-spec.md index 36962cd60..c428024df 100644 --- a/docs/src/content/docs/reference/lockfile-spec.md +++ b/docs/src/content/docs/reference/lockfile-spec.md @@ -215,8 +215,10 @@ The dependency resolver interacts with the lock file as follows: 1. **First install** — resolve all refs to commits, write `apm.lock.yaml`. 2. **Subsequent installs** — read `apm.lock.yaml`, reuse locked commits. Only newly added dependencies trigger resolution. -3. **Update** (`--update` flag or `apm deps update`) — re-resolve all refs, - overwrite the lock file with fresh commits. +3. **Update** (`--update` flag or `apm deps update`) — re-resolve all refs + to their latest commits. If a resolved commit matches the existing lock + file entry and the local checkout is intact, the download is skipped. + Otherwise, the package is re-fetched. The lock file is always refreshed. When a locked commit is no longer reachable (force-pushed branch, deleted tag), APM MUST report an error and refuse to install until the lock file is updated. diff --git a/src/apm_cli/commands/install.py b/src/apm_cli/commands/install.py index 9772f70a7..b8c4c83c5 100644 --- a/src/apm_cli/commands/install.py +++ b/src/apm_cli/commands/install.py @@ -1053,12 +1053,15 @@ def _install_apm_dependencies( lockfile_path = get_lockfile_path(project_root) existing_lockfile = None lockfile_count = 0 - if lockfile_path.exists() and not update_refs: + if lockfile_path.exists(): existing_lockfile = LockFile.read(lockfile_path) if existing_lockfile and existing_lockfile.dependencies: lockfile_count = len(existing_lockfile.dependencies) if logger: - logger.verbose_detail(f"Using apm.lock.yaml ({lockfile_count} locked dependencies)") + if update_refs: + logger.verbose_detail(f"Loaded apm.lock.yaml for SHA comparison ({lockfile_count} dependencies)") + else: + logger.verbose_detail(f"Using apm.lock.yaml ({lockfile_count} locked dependencies)") if logger.verbose: for locked_dep in existing_lockfile.get_all_dependencies(): _sha = locked_dep.resolved_commit[:8] if locked_dep.resolved_commit else "" @@ -1383,7 +1386,7 @@ def _collect_descendants(node, visited=None): # detect_ref_change() handles all transitions including None→ref. _pd_locked_chk = ( existing_lockfile.get_dependency(_pd_key) - if existing_lockfile and not update_refs + if existing_lockfile else None ) _pd_ref_changed = detect_ref_change( @@ -1400,6 +1403,19 @@ def _collect_descendants(node, visited=None): continue except Exception: pass + # Perf: when update_refs=True, skip pre-download for packages where + # the local HEAD matches the lockfile SHA. The sequential loop will + # resolve the remote ref and compare it to the lockfile SHA. If the + # remote is unchanged the download is skipped entirely; if it changed, + # it falls back to sequential download (rare case). + if update_refs and _pd_path.exists() and _pd_locked_chk: + if _pd_locked_chk.resolved_commit and _pd_locked_chk.resolved_commit != "cached": + try: + from git import Repo as _PDGitRepo + if _PDGitRepo(_pd_path).head.commit.hexsha == _pd_locked_chk.resolved_commit: + continue + except Exception: + pass # Build download ref (use locked commit for reproducibility). # build_download_ref() uses the manifest ref when ref_changed is True. _pd_dlref = build_download_ref( @@ -1612,11 +1628,14 @@ def _collect_descendants(node, visited=None): from apm_cli.models.apm_package import GitReferenceType resolved_ref = None - if dep_ref.reference and dep_ref.get_unique_key() not in _pre_downloaded_keys: - try: - resolved_ref = downloader.resolve_git_reference(dep_ref) - except Exception: - pass # If resolution fails, skip cache (fetch latest) + if dep_ref.get_unique_key() not in _pre_downloaded_keys: + # Resolve when there is an explicit ref, OR when update_refs + # is True (need the latest SHA to compare against lockfile). + if dep_ref.reference or update_refs: + try: + resolved_ref = downloader.resolve_git_reference(dep_ref) + except Exception: + pass # If resolution fails, skip cache (fetch latest) # Use cache only for tags and commits (not branches) is_cacheable = resolved_ref and resolved_ref.ref_type in [ @@ -1629,7 +1648,7 @@ def _collect_descendants(node, visited=None): # detect_ref_change() handles all transitions including None→ref. _dep_locked_chk = ( existing_lockfile.get_dependency(dep_ref.get_unique_key()) - if existing_lockfile and not update_refs + if existing_lockfile else None ) ref_changed = detect_ref_change( @@ -1638,16 +1657,24 @@ def _collect_descendants(node, visited=None): # Phase 5 (#171): Also skip when lockfile SHA matches local HEAD # — but not when the manifest ref has changed (user wants different version). lockfile_match = False - if install_path.exists() and existing_lockfile and not update_refs and not ref_changed: + if install_path.exists() and existing_lockfile: 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": - try: - from git import Repo as GitRepo - local_repo = GitRepo(install_path) - if local_repo.head.commit.hexsha == locked_dep.resolved_commit: + if update_refs: + # Update mode: compare resolved remote SHA with lockfile SHA. + # If the remote ref still resolves to the same commit, + # the package content is unchanged — skip download. + if resolved_ref and resolved_ref.resolved_commit == locked_dep.resolved_commit: lockfile_match = True - except Exception: - pass # Not a git repo or invalid — fall through to download + elif not ref_changed: + # Normal mode: compare local HEAD with lockfile SHA. + try: + from git import Repo as GitRepo + local_repo = GitRepo(install_path) + if local_repo.head.commit.hexsha == locked_dep.resolved_commit: + lockfile_match = True + 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 ) @@ -1713,8 +1740,10 @@ def _collect_descendants(node, visited=None): source=dep_ref.repo_url, ) - # Create basic resolved reference for cached packages - resolved_ref = ResolvedReference( + # Use resolved reference from ref resolution if available + # (e.g. when update_refs matched the lockfile SHA), + # otherwise create a placeholder for cached packages. + cached_resolved_ref = resolved_ref if resolved_ref else ResolvedReference( original_ref=dep_ref.reference or "default", ref_type=GitReferenceType.BRANCH, resolved_commit="cached", # Mark as cached since we don't know exact commit @@ -1724,7 +1753,7 @@ def _collect_descendants(node, visited=None): cached_package_info = PackageInfo( package=cached_package, install_path=install_path, - resolved_reference=resolved_ref, + resolved_reference=cached_resolved_ref, installed_at=datetime.now().isoformat(), dependency_ref=dep_ref, # Store for canonical dependency string ) @@ -1740,9 +1769,13 @@ def _collect_descendants(node, visited=None): depth = node.depth if node else 1 resolved_by = node.parent.dependency_ref.repo_url if node and node.parent else None _is_dev = node.is_dev if node else False - # Get commit SHA: callback capture > existing lockfile > explicit reference + # Get commit SHA: resolved ref > callback capture > existing lockfile > explicit reference dep_key = dep_ref.get_unique_key() - cached_commit = callback_downloaded.get(dep_key) + cached_commit = None + if resolved_ref and resolved_ref.resolved_commit and resolved_ref.resolved_commit != "cached": + cached_commit = resolved_ref.resolved_commit + if not cached_commit: + cached_commit = callback_downloaded.get(dep_key) if not cached_commit and existing_lockfile: locked_dep = existing_lockfile.get_dependency(dep_key) if locked_dep: diff --git a/tests/unit/test_install_update.py b/tests/unit/test_install_update.py index d260b3916..b7d2fdab1 100644 --- a/tests/unit/test_install_update.py +++ b/tests/unit/test_install_update.py @@ -77,8 +77,8 @@ def test_cacheable_does_not_skip_with_update(self): ) 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`).""" + """lockfile_match should always skip (including under update_refs) because + the lockfile_match check now handles both normal and update_refs modes.""" assert self._build_skip_download( install_path_exists=True, is_cacheable=False, @@ -511,3 +511,186 @@ def test_multiple_entries_partial_drift(self): "changed": {"url": "http://old.com"}, } assert detect_config_drift(current_configs, stored) == {"changed"} + + +class TestUpdateRefsShaComparison: + """Tests for the perf optimization: skip download when resolved SHA matches lockfile. + + When ``update_refs=True``, the engine resolves refs to get the latest SHA, + then compares against the lockfile SHA. If they match, the download is skipped. + This avoids re-downloading packages that are already at their latest version. + """ + + @staticmethod + def _build_lockfile_match_update(*, resolved_sha, lockfile_sha, install_exists): + """Reproduce the update_refs lockfile_match check from install.py. + + In update mode, lockfile_match is True when the resolved remote SHA + matches the lockfile SHA (meaning the remote hasn't changed). + """ + resolved_ref = Mock() if resolved_sha else None + if resolved_ref: + resolved_ref.resolved_commit = resolved_sha + + locked_dep = Mock() if lockfile_sha else None + if locked_dep: + locked_dep.resolved_commit = lockfile_sha + + lockfile_match = False + if install_exists and locked_dep: + if locked_dep.resolved_commit and locked_dep.resolved_commit != "cached": + # Update mode: compare resolved SHA with lockfile SHA + if resolved_ref and resolved_ref.resolved_commit == locked_dep.resolved_commit: + lockfile_match = True + return lockfile_match + + def test_matching_sha_skips_download(self): + """When resolved SHA matches lockfile SHA, lockfile_match is True.""" + assert self._build_lockfile_match_update( + resolved_sha="abc123def456", + lockfile_sha="abc123def456", + install_exists=True, + ) is True + + def test_different_sha_does_not_skip(self): + """When resolved SHA differs, lockfile_match is False (download needed).""" + assert self._build_lockfile_match_update( + resolved_sha="new_sha_789", + lockfile_sha="abc123def456", + install_exists=True, + ) is False + + def test_no_resolved_ref_does_not_skip(self): + """When resolution failed (None), lockfile_match is False.""" + assert self._build_lockfile_match_update( + resolved_sha=None, + lockfile_sha="abc123def456", + install_exists=True, + ) is False + + def test_no_lockfile_entry_does_not_skip(self): + """When no lockfile entry exists (new package), lockfile_match is False.""" + assert self._build_lockfile_match_update( + resolved_sha="abc123def456", + lockfile_sha=None, + install_exists=True, + ) is False + + def test_cached_lockfile_entry_does_not_skip(self): + """When lockfile has 'cached' placeholder, lockfile_match is False.""" + assert self._build_lockfile_match_update( + resolved_sha="abc123def456", + lockfile_sha="cached", + install_exists=True, + ) is False + + def test_no_install_path_does_not_skip(self): + """When install path doesn't exist, lockfile_match is False.""" + assert self._build_lockfile_match_update( + resolved_sha="abc123def456", + lockfile_sha="abc123def456", + install_exists=False, + ) is False + + def test_skip_download_with_update_lockfile_match(self): + """End-to-end: skip_download is True when update_refs lockfile_match is True.""" + install_path_exists = True + is_cacheable = False + update_refs = True + already_resolved = False + lockfile_match = True # From the update_refs SHA comparison + + skip_download = install_path_exists and ( + (is_cacheable and not update_refs) or already_resolved or lockfile_match + ) + assert skip_download is True + + def test_no_skip_when_sha_changed_during_update(self): + """When remote SHA changed, skip_download is False (package must be fetched).""" + install_path_exists = True + is_cacheable = False + update_refs = True + already_resolved = False + lockfile_match = False # SHA comparison failed (remote changed) + + skip_download = install_path_exists and ( + (is_cacheable and not update_refs) or already_resolved or lockfile_match + ) + assert skip_download is False + + +class TestPreDownloadUpdateRefsSkip: + """Tests for the pre-download skip optimization when update_refs=True. + + When update_refs=True and the local HEAD matches the lockfile SHA, + the pre-download section skips the package (defers to sequential + resolution for remote SHA comparison). + """ + + @staticmethod + def _should_skip_pre_download(*, update_refs, path_exists, lockfile_sha, + local_head_sha): + """Reproduce the pre-download skip condition for update_refs.""" + locked_dep = Mock() if lockfile_sha else None + if locked_dep: + locked_dep.resolved_commit = lockfile_sha + + if update_refs and path_exists and locked_dep: + if locked_dep.resolved_commit and locked_dep.resolved_commit != "cached": + if local_head_sha == locked_dep.resolved_commit: + return True + return False + + def test_skip_when_head_matches_lockfile(self): + """Skip pre-download when local HEAD matches lockfile SHA.""" + assert self._should_skip_pre_download( + update_refs=True, + path_exists=True, + lockfile_sha="abc123", + local_head_sha="abc123", + ) is True + + def test_no_skip_when_head_differs(self): + """Don't skip pre-download when local HEAD differs (corrupted install).""" + assert self._should_skip_pre_download( + update_refs=True, + path_exists=True, + lockfile_sha="abc123", + local_head_sha="different", + ) is False + + def test_no_skip_when_not_update_mode(self): + """Don't use this optimization in normal install mode.""" + assert self._should_skip_pre_download( + update_refs=False, + path_exists=True, + lockfile_sha="abc123", + local_head_sha="abc123", + ) is False + + def test_no_skip_when_path_missing(self): + """Don't skip when install path doesn't exist.""" + assert self._should_skip_pre_download( + update_refs=True, + path_exists=False, + lockfile_sha="abc123", + local_head_sha="abc123", + ) is False + + def test_no_skip_when_no_lockfile_entry(self): + """Don't skip when there's no lockfile entry (new package).""" + assert self._should_skip_pre_download( + update_refs=True, + path_exists=True, + lockfile_sha=None, + local_head_sha="abc123", + ) is False + + def test_no_skip_when_lockfile_sha_is_cached(self): + """Don't skip when lockfile has 'cached' placeholder.""" + assert self._should_skip_pre_download( + update_refs=True, + path_exists=True, + lockfile_sha="cached", + local_head_sha="cached", + ) is False From 76f347c05f109fbad218ab4eca7e79d7bcaf73a5 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Tue, 31 Mar 2026 04:35:24 +0000 Subject: [PATCH 4/5] refactor: consolidate pre-download lockfile checks and improve variable naming Address code review feedback: - Merge the normal-mode and update-mode lockfile HEAD checks in the pre-download section into a single unified condition - Rename cached_resolved_ref to resolved_or_cached_ref for clarity Agent-Logs-Url: https://github.com/microsoft/apm/sessions/e100ba84-ca0e-4cbe-8ade-067d46ace7bf Co-authored-by: danielmeppiel <51440732+danielmeppiel@users.noreply.github.com> --- src/apm_cli/commands/install.py | 43 +++++++++++++-------------------- 1 file changed, 17 insertions(+), 26 deletions(-) diff --git a/src/apm_cli/commands/install.py b/src/apm_cli/commands/install.py index b8c4c83c5..be9fcd3f3 100644 --- a/src/apm_cli/commands/install.py +++ b/src/apm_cli/commands/install.py @@ -1392,30 +1392,21 @@ def _collect_descendants(node, visited=None): _pd_ref_changed = detect_ref_change( _pd_ref, _pd_locked_chk, update_refs=update_refs ) - # Skip if lockfile SHA matches local HEAD (Phase 5 check) - # — but only if the ref itself has not changed in the manifest. - if _pd_path.exists() and existing_lockfile and not update_refs and not _pd_ref_changed: - _pd_locked = existing_lockfile.get_dependency(_pd_key) - if _pd_locked and _pd_locked.resolved_commit and _pd_locked.resolved_commit != "cached": - try: - from git import Repo as _PDGitRepo - if _PDGitRepo(_pd_path).head.commit.hexsha == _pd_locked.resolved_commit: - continue - except Exception: - pass - # Perf: when update_refs=True, skip pre-download for packages where - # the local HEAD matches the lockfile SHA. The sequential loop will - # resolve the remote ref and compare it to the lockfile SHA. If the - # remote is unchanged the download is skipped entirely; if it changed, - # it falls back to sequential download (rare case). - if update_refs and _pd_path.exists() and _pd_locked_chk: - if _pd_locked_chk.resolved_commit and _pd_locked_chk.resolved_commit != "cached": - try: - from git import Repo as _PDGitRepo - if _PDGitRepo(_pd_path).head.commit.hexsha == _pd_locked_chk.resolved_commit: - continue - except Exception: - pass + # Skip if lockfile SHA matches local HEAD. + # Normal mode: only when the ref hasn't changed in the manifest. + # Update mode: defer to the sequential loop which resolves the + # remote ref and compares — if unchanged, the download is skipped + # entirely; if changed, it falls back to sequential download. + if (_pd_path.exists() and _pd_locked_chk + and _pd_locked_chk.resolved_commit + and _pd_locked_chk.resolved_commit != "cached" + and (update_refs or not _pd_ref_changed)): + try: + from git import Repo as _PDGitRepo + if _PDGitRepo(_pd_path).head.commit.hexsha == _pd_locked_chk.resolved_commit: + continue + except Exception: + pass # Build download ref (use locked commit for reproducibility). # build_download_ref() uses the manifest ref when ref_changed is True. _pd_dlref = build_download_ref( @@ -1743,7 +1734,7 @@ def _collect_descendants(node, visited=None): # Use resolved reference from ref resolution if available # (e.g. when update_refs matched the lockfile SHA), # otherwise create a placeholder for cached packages. - cached_resolved_ref = resolved_ref if resolved_ref else ResolvedReference( + resolved_or_cached_ref = resolved_ref if resolved_ref else ResolvedReference( original_ref=dep_ref.reference or "default", ref_type=GitReferenceType.BRANCH, resolved_commit="cached", # Mark as cached since we don't know exact commit @@ -1753,7 +1744,7 @@ def _collect_descendants(node, visited=None): cached_package_info = PackageInfo( package=cached_package, install_path=install_path, - resolved_reference=cached_resolved_ref, + resolved_reference=resolved_or_cached_ref, installed_at=datetime.now().isoformat(), dependency_ref=dep_ref, # Store for canonical dependency string ) From 212857a7b65099695c286b0b6b07b90a46383605 Mon Sep 17 00:00:00 2001 From: danielmeppiel Date: Tue, 31 Mar 2026 07:06:27 +0200 Subject: [PATCH 5/5] fix: address PR review comments -- encoding, local HEAD check, gated resolution - Replace Unicode em dashes with ASCII '--' in PR-touched code and docs - Add local HEAD verification in update_refs lockfile_match to guard against corrupted local checkouts (Comment 1) - Gate ref resolution on having a non-cached lockfile entry to avoid wasted resolve_git_reference calls for new packages (Comment 3) - Add test for corrupted local checkout edge case Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../content/docs/reference/lockfile-spec.md | 2 +- src/apm_cli/commands/install.py | 30 ++++++++++++++----- tests/unit/test_install_update.py | 26 +++++++++++++--- 3 files changed, 46 insertions(+), 12 deletions(-) diff --git a/docs/src/content/docs/reference/lockfile-spec.md b/docs/src/content/docs/reference/lockfile-spec.md index c428024df..bd1a228d0 100644 --- a/docs/src/content/docs/reference/lockfile-spec.md +++ b/docs/src/content/docs/reference/lockfile-spec.md @@ -215,7 +215,7 @@ The dependency resolver interacts with the lock file as follows: 1. **First install** — resolve all refs to commits, write `apm.lock.yaml`. 2. **Subsequent installs** — read `apm.lock.yaml`, reuse locked commits. Only newly added dependencies trigger resolution. -3. **Update** (`--update` flag or `apm deps update`) — re-resolve all refs +3. **Update** (`--update` flag or `apm deps update`) -- re-resolve all refs to their latest commits. If a resolved commit matches the existing lock file entry and the local checkout is intact, the download is skipped. Otherwise, the package is re-fetched. The lock file is always refreshed. diff --git a/src/apm_cli/commands/install.py b/src/apm_cli/commands/install.py index 1e6384d39..674a71a2a 100644 --- a/src/apm_cli/commands/install.py +++ b/src/apm_cli/commands/install.py @@ -1447,7 +1447,7 @@ def _collect_descendants(node, visited=None): # Skip if lockfile SHA matches local HEAD. # Normal mode: only when the ref hasn't changed in the manifest. # Update mode: defer to the sequential loop which resolves the - # remote ref and compares — if unchanged, the download is skipped + # remote ref and compares -- if unchanged, the download is skipped # entirely; if changed, it falls back to sequential download. if (_pd_path.exists() and _pd_locked_chk and _pd_locked_chk.resolved_commit @@ -1677,8 +1677,16 @@ def _collect_descendants(node, visited=None): resolved_ref = None if dep_ref.get_unique_key() not in _pre_downloaded_keys: # Resolve when there is an explicit ref, OR when update_refs - # is True (need the latest SHA to compare against lockfile). - if dep_ref.reference or update_refs: + # is True AND we have a non-cached lockfile entry to compare + # against (otherwise resolution is wasted work -- the package + # will be downloaded regardless). + _has_lockfile_sha = False + if update_refs and existing_lockfile: + _lck = existing_lockfile.get_dependency(dep_ref.get_unique_key()) + _has_lockfile_sha = bool( + _lck and _lck.resolved_commit and _lck.resolved_commit != "cached" + ) + if dep_ref.reference or (update_refs and _has_lockfile_sha): try: resolved_ref = downloader.resolve_git_reference(dep_ref) except Exception: @@ -1702,7 +1710,7 @@ def _collect_descendants(node, visited=None): dep_ref, _dep_locked_chk, update_refs=update_refs ) # Phase 5 (#171): Also skip when lockfile SHA matches local HEAD - # — but not when the manifest ref has changed (user wants different version). + # -- but not when the manifest ref has changed (user wants different version). lockfile_match = False if install_path.exists() and existing_lockfile: locked_dep = existing_lockfile.get_dependency(dep_ref.get_unique_key()) @@ -1710,9 +1718,17 @@ def _collect_descendants(node, visited=None): if update_refs: # Update mode: compare resolved remote SHA with lockfile SHA. # If the remote ref still resolves to the same commit, - # the package content is unchanged — skip download. + # the package content is unchanged -- skip download. + # Also verify local checkout matches to guard against + # corrupted installs that bypassed pre-download checks. if resolved_ref and resolved_ref.resolved_commit == locked_dep.resolved_commit: - lockfile_match = True + try: + from git import Repo as GitRepo + local_repo = GitRepo(install_path) + if local_repo.head.commit.hexsha == locked_dep.resolved_commit: + lockfile_match = True + except Exception: + pass # Local checkout invalid -- fall through to download elif not ref_changed: # Normal mode: compare local HEAD with lockfile SHA. try: @@ -1721,7 +1737,7 @@ def _collect_descendants(node, visited=None): if local_repo.head.commit.hexsha == locked_dep.resolved_commit: lockfile_match = True except Exception: - pass # Not a git repo or invalid — fall through to download + 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 ) diff --git a/tests/unit/test_install_update.py b/tests/unit/test_install_update.py index 20f226970..05bac905d 100644 --- a/tests/unit/test_install_update.py +++ b/tests/unit/test_install_update.py @@ -524,11 +524,13 @@ class TestUpdateRefsShaComparison: """ @staticmethod - def _build_lockfile_match_update(*, resolved_sha, lockfile_sha, install_exists): + def _build_lockfile_match_update(*, resolved_sha, lockfile_sha, install_exists, + local_head_sha=None): """Reproduce the update_refs lockfile_match check from install.py. In update mode, lockfile_match is True when the resolved remote SHA - matches the lockfile SHA (meaning the remote hasn't changed). + matches the lockfile SHA AND local HEAD matches (guarding against + corrupted local checkouts). """ resolved_ref = Mock() if resolved_sha else None if resolved_ref: @@ -538,12 +540,18 @@ def _build_lockfile_match_update(*, resolved_sha, lockfile_sha, install_exists): if locked_dep: locked_dep.resolved_commit = lockfile_sha + # Default: local HEAD matches lockfile when not explicitly set + if local_head_sha is None: + local_head_sha = lockfile_sha + lockfile_match = False if install_exists and locked_dep: if locked_dep.resolved_commit and locked_dep.resolved_commit != "cached": - # Update mode: compare resolved SHA with lockfile SHA + # Update mode: compare resolved SHA with lockfile SHA, + # then verify local checkout matches. if resolved_ref and resolved_ref.resolved_commit == locked_dep.resolved_commit: - lockfile_match = True + if local_head_sha == locked_dep.resolved_commit: + lockfile_match = True return lockfile_match def test_matching_sha_skips_download(self): @@ -594,6 +602,16 @@ def test_no_install_path_does_not_skip(self): install_exists=False, ) is False + def test_corrupted_local_checkout_does_not_skip(self): + """When local HEAD differs from lockfile SHA, lockfile_match is False + even if resolved remote matches lockfile (guards against corrupt installs).""" + assert self._build_lockfile_match_update( + resolved_sha="abc123def456", + lockfile_sha="abc123def456", + install_exists=True, + local_head_sha="corrupted_different_sha", + ) is False + def test_skip_download_with_update_lockfile_match(self): """End-to-end: skip_download is True when update_refs lockfile_match is True.""" install_path_exists = True