From 7fb9eab4f7009019903e12c9f3fea2b769e85f0c Mon Sep 17 00:00:00 2001 From: abhishekacharya200 Date: Wed, 28 Oct 2026 16:20:34 +0545 Subject: [PATCH 1/2] fix(install): keep the locked commit on a cached source pin 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. --- CHANGELOG.md | 4 ++ src/apm_cli/install/sources.py | 25 +++++++++--- .../install/test_sources_materialisation.py | 39 +++++++++++++++++-- 3 files changed, 59 insertions(+), 9 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 82960dc779..15b880418e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Fixed + +- A warm-cache `apm install --frozen` keeps the commit a dependency was resolved from as the Codex source pin, instead of the `cached` sentinel that native admission rejected as an invalid pin. (#3388) + ## [0.35.0] - 2026-10-22 ### Added diff --git a/src/apm_cli/install/sources.py b/src/apm_cli/install/sources.py index 572ccf3289..9210834fcf 100644 --- a/src/apm_cli/install/sources.py +++ b/src/apm_cli/install/sources.py @@ -26,6 +26,7 @@ from __future__ import annotations +import re from abc import ABC, abstractmethod from dataclasses import dataclass, field from datetime import datetime @@ -63,6 +64,10 @@ from apm_cli.install.context import InstallContext from apm_cli.models.apm_package import PackageInfo +# 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})") + @dataclass class Materialization: @@ -504,13 +509,26 @@ def acquire(self) -> Materialization | None: if not cached_package.source: cached_package.source = dep_ref.repo_url + # Determine commit SHA for the cached path. See _resolve_cached_commit + # for the invariant ("recorded SHA must match disk identity") and the + # priority rules (PR #1158 -- branch-ref drift fix). + cached_commit = self._resolve_cached_commit() + + # A cache hit without a live resolved reference still knows the commit + # the bytes came from, and downstream consumers read it from here: the + # Codex native path treats a resolved commit as a source pin, so the + # ``cached`` sentinel made a locked SHA look like an invalid pin (#3388). 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", + resolved_commit=( + cached_commit + if isinstance(cached_commit, str) and _FULL_COMMIT_RE.fullmatch(cached_commit) + else "cached" + ), ref_name=dep_ref.reference or "default", ) ) @@ -530,11 +548,6 @@ def acquire(self) -> Materialization | None: resolved_by = locked_parent_identity(ctx, node) _is_dev = node.is_dev if node else False - # Determine commit SHA for the cached path. See _resolve_cached_commit - # for the invariant ("recorded SHA must match disk identity") and the - # priority rules (PR #1158 -- branch-ref drift fix). - cached_commit = self._resolve_cached_commit() - # Determine if cached package came from registry _cached_registry = None if (dep_locked_chk and dep_locked_chk.registry_prefix) or ( diff --git a/tests/unit/install/test_sources_materialisation.py b/tests/unit/install/test_sources_materialisation.py index 0923e6058d..8546283419 100644 --- a/tests/unit/install/test_sources_materialisation.py +++ b/tests/unit/install/test_sources_materialisation.py @@ -621,7 +621,8 @@ def test_with_targets_no_apm_yml(self, tmp_path: Path) -> None: assert result is not None def test_resolved_ref_none_creates_fallback_resolved_reference(self, tmp_path: Path) -> None: - """resolved_ref=None → a synthetic ResolvedReference is built.""" + """resolved_ref=None → the fallback carries the locked commit, not 'cached'.""" + locked_sha = "f1b2ba49d18a3de11f87d8e6dc672dfdaadbff85" ctx = _make_ctx(targets=["copilot"]) dep_ref = _make_dep_ref(is_virtual=False, reference="feature-branch") install_path = tmp_path / "pkg" @@ -637,14 +638,46 @@ def test_resolved_ref_none_creates_fallback_resolved_reference(self, tmp_path: P patch("apm_cli.utils.content_hash.compute_package_hash", return_value="h"), patch( "apm_cli.install.sources.CachedDependencySource._resolve_cached_commit", - return_value="c", + return_value=locked_sha, ), ): source = self._make_source(ctx, dep_ref, install_path, resolved_ref=None) result = source.acquire() assert result is not None - # resolved_ref was None; the code builds a fallback - we just confirm no crash + assert result.package_info is not None + # A locked SHA must survive as the source pin; the Codex native path + # rejects the 'cached' sentinel as an invalid pin (#3388). + assert result.package_info.resolved_reference.resolved_commit == locked_sha + + def test_resolved_ref_none_keeps_cached_sentinel_without_a_full_sha( + self, tmp_path: Path + ) -> None: + """A branch name is not a commit identity, so the sentinel stays.""" + ctx = _make_ctx(targets=["copilot"]) + dep_ref = _make_dep_ref(is_virtual=False, reference="main") + install_path = tmp_path / "pkg" + install_path.mkdir() + (install_path / "apm.yml").write_text("name: test\nversion: 0.0.1\n") + + mock_pkg = MagicMock() + mock_pkg.source = "owner/repo" + + with ( + patch("apm_cli.models.apm_package.APMPackage.from_apm_yml", return_value=mock_pkg), + patch("apm_cli.models.validation.detect_package_type", return_value=(None, None)), + patch("apm_cli.utils.content_hash.compute_package_hash", return_value="h"), + patch( + "apm_cli.install.sources.CachedDependencySource._resolve_cached_commit", + return_value="main", + ), + ): + source = self._make_source(ctx, dep_ref, install_path, resolved_ref=None) + result = source.acquire() + + assert result is not None + assert result.package_info is not None + assert result.package_info.resolved_reference.resolved_commit == "cached" def test_unpinned_dep_sets_delta(self, tmp_path: Path) -> None: """dep_ref.reference=None → deltas['unpinned']=1.""" From 049a7e9f1135dee6125803cdef5fb7d6fa3aeb72 Mon Sep 17 00:00:00 2001 From: abhishekacharya200 Date: Sat, 31 Oct 2026 17:00:10 +1100 Subject: [PATCH 2/2] refactor(install): share the Codex source pin predicate 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. --- .../marketplace_package_and_registration.py | 2 ++ src/apm_cli/codex_plugins/registrar.py | 14 ++++++---- src/apm_cli/install/sources.py | 14 +++------- .../test_architecture_owner_rule_mutations.py | 5 ++++ tests/unit/test_codex_native_registrar.py | 27 +++++++++++++++++++ 5 files changed, 47 insertions(+), 15 deletions(-) diff --git a/scripts/architecture_linter/checks/marketplace_package_and_registration.py b/scripts/architecture_linter/checks/marketplace_package_and_registration.py index 34ba91b906..cf5078a3d2 100644 --- a/scripts/architecture_linter/checks/marketplace_package_and_registration.py +++ b/scripts/architecture_linter/checks/marketplace_package_and_registration.py @@ -472,6 +472,7 @@ def _check_codex_lifecycle(provider: FactsProvider) -> tuple[Violation, ...]: ("compute_native_bundle_hash", "src/apm_cli/utils/content_hash.py"), ("owns_codex_dependency", _CODEX_PREFIX + "registrar.py"), ("verify_codex_source_identity", _CODEX_PREFIX + "registrar.py"), + ("is_codex_source_pin", _CODEX_PREFIX + "registrar.py"), ): count = _count_across( provider, @@ -532,6 +533,7 @@ def _check_codex_lifecycle(provider: FactsProvider) -> tuple[Violation, ...]: "ctx.package_deployed_files.get(key)", ), "src/apm_cli/install/sources.py": ( + "is_codex_source_pin(cached_commit)", "is_codex_compatibility_manifest(install_path, detect_package_type(install_path)[1])", "native_validation = validate_apm_package(install_path, source_path=install_path)", ), diff --git a/src/apm_cli/codex_plugins/registrar.py b/src/apm_cli/codex_plugins/registrar.py index 73c11c869d..2e47004f8a 100644 --- a/src/apm_cli/codex_plugins/registrar.py +++ b/src/apm_cli/codex_plugins/registrar.py @@ -31,6 +31,13 @@ from apm_cli.utils.path_security import ensure_path_within, has_symlink_component, safe_rmtree +def is_codex_source_pin(value: object) -> bool: + """Return whether a value is a full commit identity, the only valid source pin.""" + return ( + isinstance(value, str) and re.fullmatch(r"(?:[0-9a-f]{40}|[0-9a-f]{64})", value) is not None + ) + + def marketplace_name(modules_dir: Path, dependency_key: str) -> str: """Namespace an owned marketplace by APM home and canonical dependency identity.""" identity = f"{modules_dir.resolve().as_posix()}\0{dependency_key}".encode() @@ -111,10 +118,7 @@ def read(cls, data: Any, root: Path) -> _Record: or not isinstance(data.get("pending"), bool) or ( data.get("source_commit") is not None - and ( - not isinstance(data["source_commit"], str) - or re.fullmatch(r"(?:[0-9a-f]{40}|[0-9a-f]{64})", data["source_commit"]) is None - ) + and (not is_codex_source_pin(data["source_commit"])) ) or any( data.get(key) is not None and not isinstance(data[key], str) @@ -180,7 +184,7 @@ def verify_codex_source_identity( records = _read_records(ledger_path_for(modules_dir)) record = records.get(dependency_key) if source_commit is not None: - if re.fullmatch(r"(?:[0-9a-f]{40}|[0-9a-f]{64})", source_commit) is None: + if not is_codex_source_pin(source_commit): raise CodexNativeError( f"Invalid original Codex source pin for {dependency_key}; " "restore the pinned dependency before retrying." diff --git a/src/apm_cli/install/sources.py b/src/apm_cli/install/sources.py index 9210834fcf..ebc6f85db7 100644 --- a/src/apm_cli/install/sources.py +++ b/src/apm_cli/install/sources.py @@ -26,7 +26,6 @@ from __future__ import annotations -import re from abc import ABC, abstractmethod from dataclasses import dataclass, field from datetime import datetime @@ -64,10 +63,6 @@ from apm_cli.install.context import InstallContext from apm_cli.models.apm_package import PackageInfo -# 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})") - @dataclass class Materialization: @@ -514,6 +509,8 @@ def acquire(self) -> Materialization | None: # priority rules (PR #1158 -- branch-ref drift fix). cached_commit = self._resolve_cached_commit() + from apm_cli.codex_plugins.registrar import is_codex_source_pin + # A cache hit without a live resolved reference still knows the commit # the bytes came from, and downstream consumers read it from here: the # Codex native path treats a resolved commit as a source pin, so the @@ -524,11 +521,8 @@ def acquire(self) -> Materialization | None: else ResolvedReference( original_ref=dep_ref.reference or "default", ref_type=GitReferenceType.BRANCH, - resolved_commit=( - cached_commit - if isinstance(cached_commit, str) and _FULL_COMMIT_RE.fullmatch(cached_commit) - else "cached" - ), + # Short SHAs and branch names stay on the ``cached`` sentinel. + resolved_commit=cached_commit if is_codex_source_pin(cached_commit) else "cached", ref_name=dep_ref.reference or "default", ) ) diff --git a/tests/integration/test_architecture_owner_rule_mutations.py b/tests/integration/test_architecture_owner_rule_mutations.py index 9b336131c3..3cfe357778 100644 --- a/tests/integration/test_architecture_owner_rule_mutations.py +++ b/tests/integration/test_architecture_owner_rule_mutations.py @@ -170,6 +170,11 @@ def test_native_typed_error_guard_retains_legacy_and_rejects_broad_handlers( "record.source_commit == source_commit", "False", ), + ( + "src/apm_cli/install/sources.py", + "is_codex_source_pin(cached_commit)", + "bool(cached_commit)", + ), ( "src/apm_cli/install/drift.py", "codex_records = verify_codex_source_identity(", diff --git a/tests/unit/test_codex_native_registrar.py b/tests/unit/test_codex_native_registrar.py index 01ffad53be..85537fc88b 100644 --- a/tests/unit/test_codex_native_registrar.py +++ b/tests/unit/test_codex_native_registrar.py @@ -16,6 +16,7 @@ _preflight, _Record, _write_records, + is_codex_source_pin, marketplace_name, synchronize_codex_plugins, verify_codex_source_identity, @@ -332,3 +333,29 @@ def test_source_identity_records_are_cached_only_for_one_operation(tmp_path, mon ) assert deployment.plugin.dependency_key in verify_codex_source_identity(modules, **identity) assert reader.call_count == 2 + + +@pytest.mark.parametrize( + ("value", "expected"), + [ + ("a" * 40, True), + ("a" * 64, True), + ("a" * 7, False), + ("A" * 40, False), + ("main", False), + ("cached", False), + (None, False), + ], +) +def test_source_pin_predicate_is_shared_by_ledger_and_identity_checks( + tmp_path: Path, value: object, expected: bool +) -> None: + assert is_codex_source_pin(value) is expected + if isinstance(value, str) and not expected: + with pytest.raises(CodexNativeError, match="Invalid original Codex source pin"): + verify_codex_source_identity( + tmp_path / "modules", + dependency_key="owner/repo", + source_hash="sha256:" + "0" * 64, + source_commit=value, + )