Skip to content

fix(install): exclude .apm-pin marker from package content hash (unblocks v0.12.2 release)#1142

Merged
danielmeppiel merged 2 commits intomainfrom
fix/cache-pin-hash-exclusion
May 5, 2026
Merged

fix(install): exclude .apm-pin marker from package content hash (unblocks v0.12.2 release)#1142
danielmeppiel merged 2 commits intomainfrom
fix/cache-pin-hash-exclusion

Conversation

@danielmeppiel
Copy link
Copy Markdown
Collaborator

TL;DR

The v0.12.2 release tag (deleted) failed integration tests on every platform because re-installing any APM-managed package triggered a false supply-chain hash mismatch and rmtree'd the package directory. Root cause: the .apm-pin cache marker added by #1137 was being included in compute_package_hash despite being written AFTER hash recording. Fix: exclude .apm-pin from package hashing.

Problem (WHY)

After PR #1137 introduced the .apm-pin drift-replay cache marker, every second apm install of an already-installed package observed:

Content hash mismatch for github/awesome-copilot/instructions/code-review-generic.instructions.md:
expected sha256:31212689..., got sha256:4361e707...

…then safe_rmtree'd apm_modules/<owner>/<pkg>/ and sys.exit(1).

This blocked the v0.12.2 release -- tests/integration/test_guardrailing_hero_e2e.py::test_2_minute_guardrailing_flow failed on macOS-x86, macOS-arm, and Linux-arm in run 25369501023. The test calls run_command(show_output=True) which runs each apm install twice (once for display, once for capture) -- triggering the bug deterministically.

Approach (WHAT)

compute_package_hash already excluded .git/ and __pycache__/ directories. Add a parallel _EXCLUDED_FILES = {".apm-pin"} and skip matching files in the regular-files collection loop.

Implementation (HOW)

# src/apm_cli/utils/content_hash.py
_EXCLUDED_FILES = {".apm-pin"}
...
if item.is_file():
    if rel.name in _EXCLUDED_FILES:
        continue
    regular_files.append(rel)

Why .apm-pin is correct to exclude: it is install-process metadata (pin marker for drift replay), not package content. The marker is written by LockfileBuilder._sync_cache_pin_markers AFTER _compute_hash runs in FreshDependencySource.acquire, so including it can only make the on-disk hash diverge from the lockfile-recorded hash. Excluding it makes the hash stable across installs.

Validation

  • New regression test test_skips_apm_pin_marker in tests/unit/test_content_hash.py.
  • All 23 unit tests in test_content_hash.py pass locally.
  • Attestation against PyInstaller binary: rebuilt dist/apm-darwin-arm64/apm from this branch; pytest tests/integration/test_auto_install_e2e.py tests/integration/test_guardrailing_hero_e2e.py -- 5 passed in 134s, including the previously-failing test_2_minute_guardrailing_flow.
  • Lint mirror: uv run --extra dev ruff check src/ tests/ && uv run --extra dev ruff format --check src/ tests/ -- silent.

Follow-ups (out of scope)

  • Integration tests should run on PRs that touch src/apm_cli/install/ or src/apm_cli/deps/, not just on tag push -- this regression would have been caught at PR feat(audit): default-on integration drift detection #1137 time.
  • FreshDependencySource.acquire swallows exceptions at sources.py:632 and exits 0; worth surfacing as a non-zero exit. (Separate issue.)

Co-authored-by: Copilot 223556219+Copilot@users.noreply.github.com

Re-installing any package previously installed by APM 0.12.2 erased
its apm_modules/ directory: the .apm-pin cache marker (added by
#1137 for drift-replay) is written AFTER hash recording, so the
on-disk hash diverged from the lockfile hash on every subsequent
install, falsely tripping the supply-chain content-hash mismatch
check and rmtree'ing the cache.

Surfaced by tests/integration/test_guardrailing_hero_e2e.py, which
runs 'apm install <virtual-file>' twice via run_command(show_output=
True) and asserts apm_modules/github/<flattened-name> still exists --
the v0.12.2 release tag failed in CI on every platform on this test.

Fix: exclude .apm-pin from compute_package_hash via a new
_EXCLUDED_FILES set, mirroring _EXCLUDED_DIRS. Add regression test
test_skips_apm_pin_marker.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot AI review requested due to automatic review settings May 5, 2026 11:30
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR fixes a regression in APM's package-integrity hashing so reinstalling an already-cached package does not falsely fail the supply-chain content check and delete the installed package directory.

Changes:

  • Excludes .apm-pin from compute_package_hash so cache metadata does not perturb the recorded package hash.
  • Adds a unit regression test covering the reinstall/hash-stability scenario.
  • Adds a changelog entry documenting the reinstall fix for the pending release.
Show a summary per file
File Description
src/apm_cli/utils/content_hash.py Excludes .apm-pin during package hash computation.
tests/unit/test_content_hash.py Adds regression coverage for .apm-pin hash exclusion.
CHANGELOG.md Documents the reinstall/hash-mismatch fix.

Copilot's findings

  • Files reviewed: 3/3 changed files
  • Comments generated: 2

Comment thread src/apm_cli/utils/content_hash.py Outdated
Comment on lines +51 to +52
if rel.name in _EXCLUDED_FILES:
continue
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch -- applied in 9cf5452. Scoped the exclusion to root-level only (len(rel.parts) == 1 and rel.name in _EXCLUDED_ROOT_FILES) and extended the regression test to assert a nested subdir/.apm-pin is still hashed. The cache-pin marker is only ever written by cache_pin.write_marker(install_path, ...) to install_path / MARKER_FILENAME (always at the package root), so this is purely defense-in-depth -- but worth the tightening to keep the marker name from being a future blind spot in the integrity hash.

Comment thread CHANGELOG.md
### Fixed

- **Parallel subdir install race.** `apm install` no longer intermittently fails with `RuntimeError: Subdirectory '<path>' not found in repository` when multiple dependencies (including ADO sub-path deps) resolve to different subdirectories of the same `repo@ref`. The shared clone cache now stores subdir-agnostic bare clones and each consumer materializes its own working tree (mirrors the WS3 `GitCache` pattern). (#1135, fixes #1126, fixes #1140)
- **Re-installing the same package no longer rmtree's it.** `compute_package_hash` now excludes the `.apm-pin` cache marker (introduced by #1137) so the supply-chain content-hash check sees a stable hash across installs instead of falsely tripping and deleting `apm_modules/<owner>/<pkg>/`. (#1142, regression from #1137)
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not applicable in this case. The ## [0.12.2] section was added by the (already merged) release PR #1141, but the v0.12.2 git tag was deleted before any artifact was published -- v0.12.2 has never actually shipped. We're re-cycling the same version number with this fix included, so updating that section in place is correct. Moving the entry under ## [Unreleased] would imply a different version is coming next, which would itself drift from the release that's about to be tagged.

Address Copilot review feedback on PR #1142: a basename match would
exclude any .apm-pin file at any depth, so a malicious package could
hide bytes under subdir/.apm-pin to bypass the integrity hash. The
cache-pin marker is only ever written to the package root by
cache_pin.write_marker, so scope the exclusion accordingly. Extend
the regression test to assert nested .apm-pin files are still hashed.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@danielmeppiel danielmeppiel merged commit 2b1fb6b into main May 5, 2026
23 checks passed
@danielmeppiel danielmeppiel deleted the fix/cache-pin-hash-exclusion branch May 5, 2026 11:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants