fix: apm install --update fetches stale package content (lockfile SHA override)#199
Conversation
When --update is used, the download ref was still being overridden with the stale commit SHA from apm.lock. The existing_lockfile variable was unconditionally re-read for collision detection, shadowing the earlier None assignment. Add 'and not update_refs' guard at both download-ref construction sites (pre-download and sequential paths).
There was a problem hiding this comment.
Pull request overview
Fixes apm install --update incorrectly re-downloading packages at the lockfile-pinned commit SHA by preventing lockfile SHA overrides when update_refs=True (Bug #198).
Changes:
- Guard lockfile-based download-ref overrides behind
and not update_refsin both the parallel pre-download path and sequential download path. - Add a new unit test module intended to validate
--updatebehavior around skip/download-ref construction.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
src/apm_cli/cli.py |
Prevents lockfile SHA overrides from applying during --update installs. |
tests/unit/test_install_update.py |
Adds tests intended to cover --update semantics for skip logic and lockfile SHA overrides. |
Comments suppressed due to low confidence (2)
src/apm_cli/cli.py:1812
- When overriding the pre-download ref with a locked commit,
_pd_baseis built from_pd_ref.repo_urland (optionally)virtual_path, but it drops the dependency host. For non-default hosts (e.g., gitlab.com / GHE), this will rewrite ahost/owner/repodep intoowner/repo#<sha>and can fetch from the wrong host. Build the base ref the same waydownload_callbackdoes (includedep_ref.hostwhen it’s not the default) before appendingvirtual_path/#resolved_commit.
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
if _pd_ref.virtual_path:
src/apm_cli/cli.py:2174
- Same issue in the sequential download path:
base_ref = dep_ref.repo_urldropsdep_ref.hostwhen constructing the locked-commit download ref. This can mis-download packages from non-default git hosts when a lockfile is present. Include the host (as done earlier indownload_callback) before appendingvirtual_path/#resolved_commit.
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
base_ref = dep_ref.repo_url
You can also share your feedback on Copilot code review. Take the survey.
| @staticmethod | ||
| def _build_download_ref(dep_ref, existing_lockfile, update_refs): | ||
| """Reproduce the download_ref construction logic from cli.py (sequential 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 |
There was a problem hiding this comment.
These tests currently re-implement the production boolean/ref-building logic inside the test module (e.g., _build_download_ref / _build_pre_download_ref). Because the expected behavior is encoded in the test helpers themselves, the suite can still pass even if the real CLI code regresses. Prefer asserting on the actual behavior by invoking the install flow (e.g., via CliRunner + patching LockFile.read and GitHubPackageDownloader.download_package) and verifying the repo_ref passed to download_package does not include the lockfile SHA when --update is used.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Description
apm install --updatewas not fetching the latest version of packages from the server. It re-downloaded the same stale commit pinned inapm.lock.The root cause is a variable shadowing issue:
existing_lockfileis correctly set toNonewhenupdate_refs=True(line ~1597), but is unconditionally re-read for collision detection (line ~1762), overwritingNone. The downstream download-ref construction then uses the locked (old) commit SHA without checkingupdate_refs.This adds
and not update_refsguards at both download-ref construction sites (pre-download parallel path and sequential download path).Fixes #198
Type of change
Testing