perf(resolve): honour locked commits without re-resolving refs over the network

[srobroek]

Description

With a complete apm.lock, direct semver deps already replay the locked tag without a network call. But deps whose lockfile entry has a resolved_commit without a resolved_tag — notably branch pins (e.g. #main) and transitive deps tracking a branch — fall through the semver-replay path and re-resolve their ref over the network on every install, even though the lockfile already holds the exact commit.

This PR:

1. Seeds the TieredRefResolver's L0 per-run cache from the lockfile (resolved_ref -> resolved_commit) at the start of the resolve phase, before any download runs. Any later resolve_git_reference() for a locked ref then hits L0 and never fires the commits-API tier or a clone. Skipped under --update/--refresh (those intentionally re-resolve). Safe: the seeded SHA is the lockfile's own trust anchor, and drift detection still runs against the manifest ref so a changed pin is re-resolved.

2. Counts an already-concrete SHA ref as a distinct sha_passthrough stat instead of commits_api. The commits-API tier short-circuits on a SHA-regex match and returns with no HTTP call, but still incremented the commits-API counter — so a warm install that made zero network calls reported e.g. commits_api=40, misleading anyone profiling install cost. Verbose tier stats now reflect real round-trips.

Measured: on a machine with a committed lockfile but cold cache, a 1-direct + 1-transitive (branch-pinned) install made 2 real commits-API calls before, 0 after (call-level instrumentation).

The seeding step lives in install/helpers/ref_seed.py so phases/resolve.py stays within its per-module LOC budget (test_architecture_invariants).

Fixes # (issue)

Type of change

• Bug fix
• New feature
• Documentation
• Maintenance / refactor

Testing

• Tested locally
• All existing tests pass
• Added tests for new functionality (if applicable)

Added unit tests for TieredRefResolver.seed(), the sha_passthrough counting, and the resolve-phase seeding wiring (update/refresh guards, entry filtering, case-normalization). Full tests/unit/install + tests/unit/deps suites pass (2882 passed).

Spec conformance (OpenAPM v0.1)

• N/A -- this PR does not change OpenAPM-observable behaviour.

[srobroek]

@microsoft-github-policy-service agree

[Copilot]

Pull request overview

This PR optimizes install-time git ref resolution by seeding the tiered ref resolver’s per-run (L0) cache from apm.lock so branch-pinned/tagless locked dependencies can reuse resolved_commit without re-resolving refs over the network. It also improves resolver diagnostics by tracking already-concrete SHA refs as sha_passthrough rather than inflating commits_api stats.

Changes:

• Seed TieredRefResolver L0 cache from the existing lockfile early in the resolve phase (skipped under --update / --refresh).
• Add sha_passthrough stats path for SHA refs to keep tier counters aligned with real network I/O.
• Add unit tests covering seeding behavior and SHA passthrough stats.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
tests/unit/install/phases/test_resolve_seed_from_lockfile.py	Adds resolve-phase unit tests for lockfile-to-resolver seeding behavior and guardrails.
tests/unit/deps/test_tiered_ref_resolver.py	Adds unit tests for TieredRefResolver.seed() and sha_passthrough stats behavior.
src/apm_cli/install/phases/resolve.py	Wires the lockfile seeding step into the resolve phase execution flow.
src/apm_cli/install/helpers/ref_seed.py	Introduces helper that extracts lockfile entries and seeds the resolver cache (with update/refresh guards).
src/apm_cli/deps/tiered_ref_resolver.py	Implements seed() and sha_passthrough stats handling in the tiered resolver.

[srobroek]

Note on the commit trail: the middle commit ("Potential fix for pull request finding") is a Copilot Autofix suggestion that landed automatically on the review findings. The final commit (test(resolve): address PR review on lockfile ref-seeding) is my complete fix and supersedes it — it rewrites the seed() docstring to clarify ref is the resolved_ref (branch or tag name), and makes the test-double guard actually validate a 40-char hex SHA (was length-only) plus adds a non-hex rejection case. Squash-merge will flatten all three into one entry.

[danielmeppiel]

APM Review Panel: ship_with_followups

Community perf win: lockfile-seeded L0 cache eliminates redundant network RTTs. Structurally sound, no blocking findings. Ship once CI greens and trilogy order is confirmed.

cc @srobroek -- a fresh advisory pass is ready for your review.

All eight active panelists converge: this is a correct, well-scoped optimization that removes 1 real HTTP round-trip per unique (repo, branch) locked dependency during the integrate phase. The python-architect confirms sequencing and extraction are clean; the supply-chain-security-expert confirms the trust boundary is unchanged (lockfile commits are the same anchor the download phase already trusts); the performance-expert validates the mechanism eliminates the claimed calls. No panelist raised a REQUIRED finding -- the entire findings set is recommended-tier or nit-tier.

The performance evidence gate is satisfied at the mechanism level: the author's "2 calls before, 0 after" claim is reproducible via the unit test test_seed_populates_l0_and_avoids_network_tier which asserts network_tier.try_resolve.assert_not_called() and stats['commits_api']==0 after seeding. This is not a wall-clock benchmark on a 500-dep monorepo, but for an optimization whose entire value proposition is "skip one HTTP call per locked dep," call-count instrumentation IS the appropriate measurement -- latency varies by network; call elimination does not. The absence of degradation is proven structurally: test_skips_when_update_refs, test_skips_when_refresh, and test_noop_without_lockfile_or_resolver confirm the optimization correctly disengages when freshness is requested.

The sole hard blocker is operational, not technical: CI has never run on this fork branch (GitHub shows action_required). The maintainer must approve the workflow run before merge. Additionally, this is PR 1 of 3 in the perf trilogy (1973 -> 1974 -> 1975); the performance-expert's cache-key invariant note and the test-coverage-expert's integration-test recommendation are worth tracking as follow-ups but do not gate this PR.

Dissent. Minor symbol disagreement ([*] vs [i]) surfaced identically by cli-logging-expert and devx-ux-expert -- both agree it is a nit (already folded in this run). The performance-expert's "recommended: document cache-key invariant" and test-coverage-expert's "recommended: integration test" overlap in intent (detect future bypass regression); a single integration test covers both. Neither is blocking.

Aligned with: Pragmatic as npm (eliminates unnecessary network calls on locked installs -- matches the "fast by default when lock exists" expectation users bring from npm ci / pip install); OSS community driven (community contributor's first trilogy PR; fast, constructive review cycle reinforces that external perf contributions land without bureaucratic drag).

Growth signal. The oss-growth-hacker correctly identifies a quotable narrative: "locked installs now skip N API calls entirely." When the trilogy lands, the release note should lead with the contributor shout-out and the concrete "2->0 calls per dep" metric. This is the kind of "show your work" story that converts perf-sensitive teams evaluating APM against incumbents.

Reservations carried from strategic-alignment:

• MAINTAINER ACCEPTANCE BAR (hard gate): net win on ALL angles AND empirically proven. -- ADDRESSED: all nine panelists find no blocking issues; perf evidence is credible (call-count instrumentation + unit test assertion).
• PERFORMANCE-EVIDENCE GATE: ready-to-merge REQUIRES (a) profiling/benchmark on a REAL scenario AND (b) no degradation demonstrated. -- ADDRESSED: (a) author measured 2->0 API calls on real locked install (PR body); unit test test_seed_populates_l0_and_avoids_network_tier provides reproducible mock-level proof. (b) guard tests for --update/--refresh/no-lockfile prove non-degradation. REMAINING: CI has not run (action_required); maintainer must approve the workflow run.
• PERF TRILOGY: PRs 1973/1974/1975 share install/phases/resolve.py + git_cache.py; note stacking/merge-order. -- NOTE: This PR (1973) is self-contained (adds ref_seed.py, touches resolve.py at 2 lines, extends tiered_ref_resolver.py). Recommend merging in trilogy order (1973 -> 1974 -> 1975) to avoid conflicts on the shared files.

Panel summary

Persona	B	R	N	Takeaway
Python Architect	0	0	2	Clean perf optimization: lockfile-seeded L0 cache + SHA passthrough short-circuit. Correct sequencing, appropriate extraction, thread-safe.
CLI Logging Expert	0	0	2	Verbose-detail level correct; [*] symbol changed to [i] (folded); sha_passthrough already rendered by tier_summary.
DevX UX Expert	0	0	2	Solid perf UX: no default-output change, verbose messages informative, stats more honest.
Supply Chain Security Expert	0	0	2	Lockfile seeding is safe -- caches same trust anchor download phase already uses; content-hash integrity unaffected.
OSS Growth Hacker	0	1	1	High-signal community perf PR targeting CI/CD locked installs; quotable "2->0 API calls" story deserves a release beat.
Auth Expert	--	--	--	INACTIVE: no auth surface touched (no auth.py, token_manager.py, azure_cli.py, github_downloader.py, or marketplace client.py changed).
Doc Writer	--	--	--	INACTIVE: no user-facing behavior change; no doc update required.
Test Coverage Expert	0	1	1	Unit coverage thorough (10 tests); recommend one integration-level test asserting seeding avoids network calls end-to-end.
Performance Expert	0	2	2	Correct high-leverage optimization: eliminates 1 real HTTP RTT per unique (repo, branch) locked dep; sha_passthrough saves misleading stat inflation.

B = blocking-severity findings, R = recommended, N = nits.
Counts are signal strength, not gates. The maintainer ships.

Top 4 follow-ups

1. [performance-expert] Document cache-key invariant in ref_seed.py -- folded: comment added documenting that locked.repo_url must match dep_ref.repo_url at all resolve() call sites; future URL canonicalisation must be symmetric.
2. [test-coverage-expert] Add integration test asserting lockfile-seeded resolve avoids network tier end-to-end -- deferred: would require a new integration test fixture; the existing unit test test_seed_populates_l0_and_avoids_network_tier provides mock-level proof; a follow-up integration test is recommended but non-gating.
3. [cli-logging-expert] Change [*] status symbol to [i] for setup/info messages -- folded: symbol updated in this run.
4. [oss-growth-hacker] Amplify trilogy in release notes with contributor attribution and before/after metric -- CHANGELOG entry added in this run crediting @srobroek with "2 calls -> 0" metric.

Folded in this run

• cache-key invariant comment added to src/apm_cli/install/helpers/ref_seed.py (commit 8f3bf53)
• [*] symbol changed to [i] in verbose seeding message (commit 8f3bf53)
• docstring corrected: "lockfile-verified" -> "lockfile-recorded" in src/apm_cli/deps/tiered_ref_resolver.py (commit 8f3bf53)
• CHANGELOG [Unreleased] Performance entry added (commit 8f3bf53)
• Merged origin/main to resolve CHANGELOG conflict (commit 1194141)

Copilot signals reviewed

• ID 4606125113 (review body, 2 inline comments generated):
  • Comment 1 (docstring resolved_tag vs resolved_ref): LEGIT -- valid docstring inconsistency. RESOLVED in commit 581109e by author before this run.
  • Comment 2 (test double guard length-only check): LEGIT -- valid test double accuracy issue. RESOLVED in commit 581109e by author before this run.

Lint evidence

ruff check src/ tests/: all checks passed (exit 0)
ruff format --check src/ tests/: 1388 files already formatted (exit 0)

CI evidence

CI check status: action_required for external fork workflows (CI, Merge Gate, CodeQL, Spec conformance). These require maintainer approval to run. The most recent PR event (2026-07-04) triggered Docs Sync Advisory and PR Review Panel workflows -- both completed SKIPPED. license/cla: SUCCESS. The main test suite (CI workflow) has NOT run; awaiting maintainer approval of the workflow run for this external fork.

Mergeability

#PR	SHA	CEO stance	Iterations	Folds	Deferrals	Copilot rounds	CI	Mergeable	MergeState	Notes
#1973	1194141	ship_with_followups	1	4	1	1	blocked	MERGEABLE	BLOCKED	awaiting CI approval + maintainer review; trilogy order: 1973->1974->1975

Full per-persona findings

Python Architect

• nit: Stats dict mutations are not thread-safe (pre-existing pattern; diagnostic-only counters; not introduced by this PR)
• nit: getattr duck-typing is pragmatic at one call site; if a second consumer of seed() appears, consider a Seedable protocol

CLI Logging Expert

• nit: [*] symbol changed to [i] for seeding message (FOLDED)
• nit: sha_passthrough stat rendered automatically by tier_summary (no extra output needed)

DevX UX Expert

• nit: sha_passthrough could be renamed lock_hit for user-friendliness in verbose output (consistent with existing naming, not blocking)
• nit: [*] vs [i] prefix (FOLDED)

Supply Chain Security Expert

• nit: Docstring corrected: "lockfile-verified" -> "lockfile-recorded" (FOLDED)
• nit: repo_url not normalized in seed() -- a key mismatch degrades gracefully to the pre-PR resolve path (not a security issue)

OSS Growth Hacker

• recommended: Amplify trilogy in release notes with contributor attribution (CHANGELOG entry added, FOLDED)
• nit: Before/after timing snippet in CHANGELOG if wall-clock data available (non-blocking)

Test Coverage Expert

• recommended: Add integration test asserting lockfile-seeded resolve avoids network tier end-to-end (DEFERRED -- scope boundary: existing unit tests prove mechanism; e2e integration test is a follow-up)
• nit: Parametrize duplicate-seed/overwrite-key scenario in unit tests

Performance Expert

• recommended: Document cache-key invariant (FOLDED -- comment added)
• recommended: Add integration-level stat assertion to detect future bypass regression (DEFERRED -- same as test-coverage-expert integration test recommendation)
• nit: sha_passthrough saves ~5-10us per dep (quantified, not blocking)
• nit: Seed iterates local deps that always no-op (minimal overhead, not worth fixing)

[danielmeppiel]

APM Spec Guardian: fold_and_ship

Scope: editorial-patch (spec amendment req-rs-015); shocked-meter avg: 7.8/10. All six fold-now items have been applied in 0c0efc075; this comment records the panel outcome for the review trail.

The four spec-ecosystem panelists converged on ship: the req-rs-015 amendment codifies genuinely new, specifiable behavior (network-free replay of commit-pinned lockfile entries) rather than papering over an internal optimization, and it binds correctly across anchor / manifest / Appendix C / pytest. No panelist raised a blocker. The recommended folds sharpened scope and cross-references; all are now in the pushed commit.

Convergence

Panel	Verdict	Shocked	New B	New R	New N
Swagger / OpenAPI Editor	ship_with_followups	8/10	0	1	2
OCI Distribution Editor	ship_with_followups	8/10	0	2	1
Pkgmgr Registry-Contract Editor	ship_with_followups	7/10	0	3	1
W3C TAG Architect	ship_with_followups	8/10	0	2	1

B = new blocking findings, R = new recommended, N = new nits.
Counts are signal strength, not gates. The maintainer ships.

Convergent themes (flagged by 2+ panels)

• T1 -- drift predicate underspecified (supporting: sw-rec-r1-1, pkg-rec-r1-2)
• T2 -- wrong anchor cross-reference (req-lk-006 vs recording anchor) (supporting: sw-nit-r1-2, oci-rec-r1-1, pkg-nit-r1-1)
• T3 -- normative text mixed with rationale (supporting: oci-nit-r1-1, tag-rec-r1-3)
• T4 -- "no clone" over-broad vs content fetch (supporting: oci-rec-r1-2, tag-rec-r1-1)

Fold now (6 item(s)) -- APPLIED in 0c0efc075

1. [F1 / over-scope] Section 7.5 req-rs-015 -- scope the replay trigger to entries recording a resolved_commit WITHOUT a resolved_tag (git-literal and untagged-branch entries per req-rs-003), so semver-entry freshness checks under req-rs-004 are not wrongly forbidden.
   Success criterion: req-rs-015 names the resolved_commit-without-resolved_tag scope
2. [F2 / T1] Section 7.5 req-rs-015 -- replace bare "(drift)" with a local definition: manifest ref not character-equal to lockfile resolved_ref, or entry removed; semver-range drift governed by req-rs-004.
   Success criterion: drift is defined in-place, not left to the reader
3. [F3 / T2] Section 7.5 req-rs-015 -- correct cross-ref: resolved_commit is the resolution anchor (req-lk-003), not a trust anchor (req-lk-006 is frozen-mode); content integrity remains under tree_sha256 (req-lk-015) and resolved_hash (req-lk-013).
   Success criterion: no reference to req-lk-006 as the recording anchor
4. [F4 / T4] Section 7.5 req-rs-015 -- scope "no clone" to ref discovery (illustrative, not exhaustive) and state object fetch to materialise already-resolved content is not constrained.
   Success criterion: content fetch at the resolved commit is explicitly out of scope
5. [F5 / T3] Section 7.5 req-rs-015 -- move the trailing rationale sentence into a NOTE (non-normative) block, preserving "network-free at the resolution step".
   Success criterion: normative body contains only the MUST + its conditions
6. [F6 / hygiene] Section 7.5 req-rs-015 -- lowercase "provided" (not an RFC 2119 keyword).
   Success criterion: no non-keyword all-caps in req-rs-015

Defer to v0.1.1

• [F7 / conformance-fixture] tests/fixtures/spec-conformance -- add a behavioral fixture exercising the network-free replay path end-to-end. Impl-level behavioral tests already exist in this PR; the text-pinning marker follows the req-rs-cluster convention, so this is a convention upgrade, not a gap.

Rejected findings

• pkg-rec-r1-3 -- split req-rs-015 into two MUSTs (replay + re-resolve): the pkgmgr-registry lens is deprioritized for this amendment and the split would force a renumber of the req-rs cluster for no conformance gain; the single MUST with explicit conditions is testable as-is.
• tag-nit-r1-1 -- abstract the CLI verbs ("or equivalent"): req-rs-011/012 already carry the CLI-abstraction caveat; the parenthetical here aids implementers without over-constraining.

Linter handoff: orphan_check 96 aligned; conformance suite 129 passed; Mode-B short-circuits green (spec-concurrent). CONFORMANCE.{json,md} unchanged (prose-only folds). The three conformance-pinned phrases ("non-update install", "WITHOUT issuing a network ref-resolution", "network-free at the resolution step") survive the rewrite contiguous on single lines.

_{This panel is advisory. It does not block merge. Re-apply the spec-review label after addressing feedback to re-run.}

[danielmeppiel]

Approving as maintainer: triaged via apm-issue-autopilot, driven to green CI, advisory review panels clean. Batch-merge authorized.