Repository navigation
fix(install): keep the locked commit as the cached source pin - #3407
Abhishek Acharya (abhishekacharya200) wants to merge 2 commits into
Conversation
A cache hit without a live resolved reference filled its fallback ResolvedReference with the literal resolved_commit="cached". Codex native admission forwards that field as the source pin, so a warm-cache `apm install --frozen` on a dependency with a locked SHA failed with "Invalid original Codex source pin" even though the lockfile and .apm-pin both recorded the commit. Build the fallback with the commit the cache was resolved from when it is a full SHA, and keep the sentinel when no commit identity is available.
There was a problem hiding this comment.
🟡 Changes recommended
Source-pin validation is duplicated outside its designated owner, risking divergence from native admission and requiring a shared predicate backed by a regression test and registered architecture guard.
1 open issue
What changed in this PR
Fixes #3388 by preserving a cached dependency's full locked commit in the fallback reference used by Codex source-pin validation.
Changes:
- Resolves the cached commit once and preserves full 40- or 64-character lowercase hexadecimal identities.
- Keeps the
cachedsentinel for other values and adds regression coverage. - Documents the warm-cache installation fix in the changelog.
| File | Description |
|---|---|
tests/unit/install/test_sources_materialisation.py |
Tests locked-SHA preservation and branch-name fallback. |
src/apm_cli/install/sources.py |
Carries full cached commit identities into fallback references. |
CHANGELOG.md |
Records the cached Codex source-pin fix. |
Review effort: Balanced
|
|
||
| # A full Git commit identity, the only shape a resolved reference may carry as a | ||
| # source pin. Short SHAs and branch names stay on the ``cached`` sentinel. | ||
| _FULL_COMMIT_RE = re.compile(r"(?:[0-9a-f]{40}|[0-9a-f]{64})") |
There was a problem hiding this comment.
Fixed in 049a7e9. is_codex_source_pin now lives in codex_plugins/registrar.py, and the ledger reader, the identity check and cache materialization all use it. The Codex lifecycle guard requires one definition and the call in install/sources.py, with a mutation case, and a new test covers the predicate and the rejection path.
Cache materialization, the ownership ledger and the identity check now use is_codex_source_pin from the registrar, the owner of source pin integrity, and the architecture guard requires it.
|
Thank you for contributing this pull request. The linked issue #3388 is open but has not been accepted by a maintainer. APM starts with an issue, not an implementation (https://github.com/microsoft/apm/blob/main/CONTRIBUTING.md). Please wait for a maintainer to review and accept the scope in #3388 before proceeding. This PR is labelled status/deferred until then. Generated by autopilot-pr-triage-worker. This comment is AI-generated and may contain errors. |
Sergio Sisternes (sergio-sisternes-epam)
added
area/distribution
Fixes #3388
Problem
A cache hit with no live resolved reference built its fallback
ResolvedReferencewith the literalresolved_commit="cached". Codex native admission readspackage_info.resolved_reference.resolved_commitas the source pin, so a warm-cacheapm install --global --frozenon a dependency whose lockfile and.apm-pinboth record a Git SHA fails with:Change
CachedDependencySource.acquire()now resolves the cached commit once, before the fallback reference is built, and uses it when it is a full commit identity (40or64lowercase hex). Branch names and short SHAs keep thecachedsentinel, so nothing downstream that treats the sentinel as "no recorded identity" changes behavior. The later_resolve_cached_commit()call was folded into the earlier one.Validation
test_resolved_ref_none_creates_fallback_resolved_referencenow asserts the locked SHA survives on the fallback reference. It fails onmainand passes with the change.test_resolved_ref_none_keeps_cached_sentinel_without_a_full_shapins the branch-name case to the sentinel.tests/unit/installplus the Codex registrar/admission suites: failure set identical tomainapart from the new test (74 vs 75 failures, all pre-existing in this Windows checkout — symlink tests and fixtures outside the sparse clone).ruff checkandruff format --checkclean on the touched files.