fix(deps): use --git-dir for bare repos + pin fetched SHAs as refs (#1267)

[sergio-sisternes-epam]

fix(deps): use --git-dir for bare repos + pin fetched SHAs as refs

TL;DR

Two bugs discovered during E2E validation of the SHA-pin fix (PR #1259) on git 2.53.0: bare-cache git commands use git -C which is rejected by the new safe.bareRepository=explicit default, and fetched SHAs land in the object store without a ref so git clone --local --shared silently omits them. This PR fixes both.

Problem (WHY)

• Bug 1 -- git -C <bare> incompatible with git 2.53.0: Git 2.53.0 (Homebrew macOS) defaults safe.bareRepository=explicit, causing git -C <bare-path> to fail with exit 128. All 8 bare-repo subprocess.run calls in bare_cache.py use this form.
• Bug 2 -- fetched SHAs not ref-reachable: fetch_sha_into_bare inserts a SHA into the object store but creates no ref pointing to it. git clone --local --shared from a shallow bare falls back to upload-pack, which only transfers objects reachable from advertised refs -- the orphaned SHA is silently omitted, and git checkout <sha> fails downstream.

Both bugs were found during E2E validation with the test marketplace repo sergio-sisternes-epam/apm-marketplace-tests.

Approach (WHAT)

Change	What
git -C to --git-dir	Replace all 8 bare-repo subprocess calls to use git --git-dir <path>
_pin_sha_as_head_ref()	New closure in fetch_sha_into_bare that creates refs/heads/apm-pin-<sha12> after every successful fetch/discovery

Implementation (HOW)

File	Change
src/apm_cli/deps/bare_cache.py	8x "-C" to "--git-dir" replacements; new _pin_sha_as_head_ref() closure called at all 3 success paths in fetch_sha_into_bare (step 1 already-present, step 2 shallow fetch, step 3 broad fetch)
tests/unit/deps/test_shared_clone_cache.py	Updated 4 TestFetchShaIntoBare tests to expect the additional update-ref call from _pin_sha_as_head_ref
CHANGELOG.md	Entry under ## [Unreleased] / ### Fixed

Trade-offs

• refs/heads/apm-pin-* chosen over refs/apm-pins/*: git clone's default refspec only maps refs/heads/*; custom namespaces are silently ignored by upload-pack.
• Best-effort pinning: _pin_sha_as_head_ref never raises -- failure is logged at DEBUG. The fallback (fresh bare clone for the pinned package) remains functional.
• Lingering pin refs: These persist for the shared bare's lifetime (/tmp with random suffix, cleaned up on process exit). No disk or namespace concern.

Validation

Unit tests (40/40 pass)

40 passed in 1.59s

E2E validation

[*] Installed 2 APM dependencies in 5.5s.
=== Exit code: 0 ===

• composite-agent installed at HEAD (b30b10f6)
• base-skills installed at SHA pin (578096377c2a) -- not HEAD
• apm.lock.yaml records correct resolved_commit for both

How to test

1. Checkout this branch
2. uv run --extra dev pytest tests/unit/deps/test_shared_clone_cache.py -x -v -- all 40 pass
3. Optional E2E: apm install sergio-sisternes-epam/apm-marketplace-tests/packages/composite-agent --force --update -- should succeed with both packages installed

Closes #1267

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

[Copilot]

Pull request overview

This PR fixes two edge cases in the dependency shared-bare clone cache: (1) Git 2.53.0 rejecting git -C <bare> when safe.bareRepository=explicit is the default, and (2) ensuring SHA-fetched commits are ref-reachable so git clone --local --shared from a shallow bare actually transfers the pinned commit.

Changes:

• Replace bare-repo git invocations from git -C <bare> to git --git-dir <bare> for Git 2.53.0 compatibility.
• After a SHA is discovered/fetched into a bare, create a synthetic refs/heads/apm-pin-<sha-prefix> ref so upload-pack advertises the object.
• Update unit tests and add a changelog entry documenting the fix.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.

File	Description
src/apm_cli/deps/bare_cache.py	Switch bare git commands to --git-dir and add synthetic ref pinning after SHA fetch/discovery.
tests/unit/deps/test_shared_clone_cache.py	Update fetch_sha_into_bare tests to account for the new update-ref pin operation.
CHANGELOG.md	Add an Unreleased/Fixed entry describing the Git 2.53.0 bare behavior fix and SHA reachability pinning.

[github-actions]

APM Review Panel: ship_with_followups

Restores apm install on macOS git 2.53.0 (Homebrew): --git-dir replaces -C for safe.bareRepository=explicit, and SHA-pinned refs fix silent clone omissions.

cc @sergio-sisternes-epam @danielmeppiel -- a fresh advisory pass is ready for your review.

All eight panelists converge on a clean signal: zero blocking findings, zero required-tier findings. The two mechanical fixes in bare_cache.py are correct, independently motivated, and well-scoped. The --git-dir migration addresses a git 2.53.0 hardening of bare-repo semantics that would silently break apm install for any macOS user on a current Homebrew git -- an adoption-killer class regression. The _pin_sha_as_head_ref closure addresses the complementary issue that fetched SHAs are unreachable from clone --local --shared without a named ref; the best-effort pattern (no check=True, exception swallowed) is idiomatic for this codebase and the Python Architect confirms the pattern is coherent.

The panel surfaces three clusters of follow-up signal, all post-merge. First, the Python Architect flags that the 'pinned ...' DEBUG log fires unconditionally on the happy path regardless of update-ref's returncode -- a one-line guard before the log statement would close a silent-failure diagnostic gap. Second, the Supply Chain Security and Auth experts independently flag that _pin_sha_as_head_ref omits env=env; both confirm git update-ref is local-only with no credential path, so the risk is purely hygiene, but the inconsistency is a copy-paste hazard for future maintainers. Third, the Test Coverage Expert flags that all three _pin_sha_as_head_ref paths pass through mocked subprocess boundaries -- unit coverage is solid (40/40), but the install-pipeline tier-floor calls for at least one integration-with-fixtures test exercising a real local bare repo to create a regression-trap that would catch a silent -C reversion.

The CHANGELOG and doc surfaces are sound. The OSS Growth Hacker and Doc Writer both flag the same CHANGELOG sentence for a light editorial pass (lead with user outcome, not mechanism) -- the suggested rewrites from both panelists are compatible and either can be applied in-flight before merge.

Dissent. Supply Chain Security and Auth Expert agree on env=env hygiene but differ on the suggested fix: supply-chain recommends a comment documenting the intentional omission (mirroring _rev_parse_present); auth-expert recommends passing env=env unconditionally. Both are defensible; the comment-only approach has lower diff noise and is consistent with the existing _rev_parse_present pattern, so the CEO sides with supply-chain on this one. The Test Coverage Expert's integration-with-fixtures finding carries an evidence block with outcome: missing on a portability-by-manifest surface -- per panel weighting rules this ranks above any recommended opinion finding, and it does in the followups list below.

Aligned with: Portable by Manifest -- switching to --git-dir satisfies git 2.53.0's safe.bareRepository=explicit default, restoring portability across macOS Homebrew git versions without requiring user config workarounds. Secure by Default -- all network-touching subprocess calls retain env=env (GIT_TERMINAL_PROMPT=0, sanitized credential environment); the local-only update-ref omission is safe and a follow-up comment documents the intentional omission. Pragmatic as npm -- best-effort pin-ref creation with a silent fallback mirrors how npm handles advisory-tier install warnings: apm install succeeds even if the pin ref cannot be written, with a debug log for operator visibility.

Growth signal. The macOS git 2.53.0 failure is an adoption-killer: any new user installing APM on a current Homebrew git would hit a silent breakage with no actionable error. Shipping this fix cleanly -- with no config workaround required -- is a direct conversion-rate improvement for the macOS onboarding funnel. The SHA-pinned reproducibility story (fetched commits survive clone --local --shared intact) is a differentiator worth amplifying in the next release note: 'APM now works out of the box on macOS with the latest Homebrew git, and reproducible installs are preserved end-to-end through the bare-cache layer.'

Panel summary

Persona	B	R	N	Takeaway
Python Architect	0	0	1	Closure + best-effort pattern is coherent and consistent; one nit: success log fires on non-zero update-ref returncode.
CLI Logging Expert	0	0	0	New debug log messages follow existing patterns and are correctly scoped; no user-facing output changes. Ship.
DevX UX Expert	0	0	0	No CLI surface, flag, help text, or error-message changes; fix restores correct apm install behavior on git 2.53.0 -- ship.
Supply Chain Security Expert	0	0	2	No blocking supply-chain issues; missing env= in _pin_sha_as_head_ref is safe (local-only op), but worth a nit for consistency and future-proofing.
OSS Growth Hacker	0	1	0	Fixes silent install failures on git 2.53.0 macOS -- a direct adoption blocker; CHANGELOG entry is clear; no story surface gaps.
Auth Expert	0	0	1	Missing env=env in _pin_sha_as_head_ref is a hygiene inconsistency but not an auth risk; git update-ref is local-only and makes no network calls. Ship.
Doc Writer	0	0	1	CHANGELOG entry covers both fixes with correct voice; no user-facing docs or CLI surface changed; one nit on sentence structure.
Test Coverage Expert	0	2	1	Unit coverage is solid at all 3 pin-paths; install-pipeline floor (integration-with-fixtures) is unmet for fetch_sha_into_bare -- recommended follow-up, not a blocker.

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

Top 5 follow-ups

1. [Test Coverage Expert] Add tests/integration/test_bare_cache_integration.py: create a real local bare repo, call fetch_sha_into_bare, assert refs/heads/apm-pin-(sha12) exists via git for-each-ref. -- outcome: missing on a portability-by-manifest surface. Mocked unit tests cannot catch a silent reversion from --git-dir back to -C at the git level; this is the only automated guardrail that would survive a future refactor.
2. [Test Coverage Expert] In test_shallow_fetch_full_sha_succeeds and the broad-fetch path, add pin_argv assertions: assert 'update-ref' in pin_argv and assert f'refs/heads/apm-pin-{sha[:12]}' in pin_argv. -- The two sibling tests only assert call count; neither verifies the ref name string, so a typo in the ref format would pass undetected.
3. [Python Architect] Guard the 'pinned ...' DEBUG log behind if result.returncode == 0, with an else branch logging the non-zero exit code. -- As-is, the success log fires unconditionally; an operator debugging a missing pin ref would see 'pinned aabbcc...' while the ref was never created. One-line fix, zero behavior change.
4. [Supply Chain Security Expert] Add # no env= needed -- purely local git plumbing, no network access above the subprocess.run in both _rev_parse_present and _pin_sha_as_head_ref. -- Documents the intentional omission so future maintainers do not cargo-copy the pattern for a network-touching command without noticing the env= is absent.
5. [OSS Growth Hacker] Reframe CHANGELOG entry to lead with user outcome: 'apm install now works on macOS git 2.53.0 (Homebrew): ...' before the mechanism detail. -- CHANGELOG readers scanning for upgrade motivation need the user-visible impact up front; the current entry leads with --git-dir which is opaque to most consumers.

Architecture

classDiagram
    direction TB
    class bare_cache_module {
        <<Module / IOBoundary>>
        +clone_with_fallback()
        +bare_clone_with_fallback()
        +materialize_from_bare()
        +fetch_sha_into_bare(bare_path, sha, url, dep_ref) bool
        -_scrub_bare_remote_url()
        -_bare_action()
    }
    class fetch_sha_into_bare {
        <<FunctionScope / ClosureHost>>
        -bare_path Path
        -sha str
        -git_exe str
        +_rev_parse_present() bool
        +_pin_sha_as_head_ref() void
        +_scrub_fetch_head() void
        +_fetch_action_sha() void
        +_fetch_action_broad() void
    }
    class _pin_sha_as_head_ref {
        <<Closure / BestEffort>>
        captures: bare_path, sha, git_exe
        +__call__() void
    }
    class _rev_parse_present {
        <<Closure / Pure>>
        captures: bare_path, sha, git_exe
        +__call__() bool
    }
    class _scrub_fetch_head {
        <<Closure / IOBoundary>>
        captures: bare_path
        +__call__() void
    }
    note for _pin_sha_as_head_ref "Best-effort: catches Exception, logs DEBUG. Creates refs/heads/apm-pin-sha12 via git update-ref --git-dir"
    bare_cache_module *-- fetch_sha_into_bare : contains
    fetch_sha_into_bare *-- _pin_sha_as_head_ref : defines
    fetch_sha_into_bare *-- _rev_parse_present : defines
    fetch_sha_into_bare *-- _scrub_fetch_head : defines
    class _pin_sha_as_head_ref:::touched
    classDef touched fill:#fff3b0,stroke:#d47600

Loading

flowchart TD
    A(["fetch_sha_into_bare(bare_path, sha, url, dep_ref)"]) --> B
    B["_rev_parse_present() - git --git-dir rev-parse --verify sha"] -->|already present| C
    B -->|not present| D
    C["_pin_sha_as_head_ref() - git --git-dir update-ref refs/heads/apm-pin-sha12"] --> C2
    C2{"returncode==0?"} -->|yes| C3["DEBUG: pinned sha12"]
    C2 -->|no / exception| C4["DEBUG: could not create pin ref"]
    C3 --> R1(["return True"])
    C4 --> R1
    D{"len(sha)==40?"} -->|yes| E
    D -->|short SHA / tag| G
    E["Step 2: shallow fetch - git --git-dir fetch --depth=1 url sha"] --> E2
    E2["_scrub_fetch_head() - truncate FETCH_HEAD"] --> E3
    E3["_rev_parse_present()"] -->|present| E4
    E3 -->|still absent| G
    E4["_pin_sha_as_head_ref() - git --git-dir update-ref refs/heads/apm-pin-sha12"] --> R2(["return True"])
    G["Step 3: broad fetch - git --git-dir fetch --depth=broad_depth url"] --> G2
    G2["_scrub_fetch_head()"] --> G3
    G3["_rev_parse_present()"] -->|present| G4
    G3 -->|absent| RF(["return False"])
    G4["_pin_sha_as_head_ref() - git --git-dir update-ref refs/heads/apm-pin-sha12"] --> R3(["return True"])

Loading

Recommendation

The fix is correct, scoped, and urgently needed for macOS git 2.53.0 compatibility. The panel found zero blocking issues. The highest-signal follow-up to track post-merge is the integration-with-fixtures test for fetch_sha_into_bare (test-coverage-expert): it is the only automated guardrail that would catch a silent --git-dir reversion at the git level, and it belongs on the milestone immediately following this fix. The returncode guard for the pin-ref log (python-architect) and the env= comment (supply-chain-security) are good-housekeeping items suitable for the same or a subsequent patch. The CHANGELOG editorial pass (oss-growth-hacker / doc-writer) can be applied before merge with a single-line edit.

---

Full per-persona findings

Python Architect

• [nit] _pin_sha_as_head_ref logs 'pinned' even when update-ref exits non-zero at src/apm_cli/deps/bare_cache.py
  subprocess.run defaults to check=False. The try/except only catches raised exceptions (e.g. timeout, FileNotFoundError), not a non-zero returncode. The DEBUG 'pinned ...' message fires unconditionally on the happy path regardless of returncode, so an operator debugging the shallow-clone-omits-SHA scenario would see 'pinned aabbcc...' in the log while the ref was never actually created. A one-line returncode guard before the success log closes the gap without changing the best-effort contract.
  Suggested: result = subprocess.run(...); if result.returncode == 0: _log.debug('fetch_sha_into_bare: pinned %s as %s in %s', ...) else: _log.debug('fetch_sha_into_bare: update-ref exited %d for %s in %s', result.returncode, ...)

CLI Logging Expert

No findings.

DevX UX Expert

No findings.

Supply Chain Security Expert

• [nit] _pin_sha_as_head_ref omits env=env without a comment explaining the intentional omission at src/apm_cli/deps/bare_cache.py
  Every other subprocess.run in bare_cache.py that could touch credentials passes env=env. _pin_sha_as_head_ref does not. git update-ref is a local plumbing command with no network path, so there is no actual credential leakage here -- but the inconsistency is a maintenance hazard. Both _rev_parse_present and _pin_sha_as_head_ref should carry a comment: # no env= needed -- purely local git plumbing, no network access.
  Suggested: Add # no env= needed -- purely local git plumbing, no network access above the subprocess.run call in both _rev_parse_present and _pin_sha_as_head_ref to make the omission intentional and auditable.

• [nit] sha[:12] slice used in ref name without validating sha is hex-only at src/apm_cli/deps/bare_cache.py
  sha comes from the lockfile (user-supplied on disk). Passing sha[:12] into git update-ref via subprocess argv is safe (no shell expansion), and git will reject an invalid ref name independently. However, a brief re.fullmatch(r'[0-9a-f]{40}', sha) guard would make the invariant explicit and catch corrupt entries earlier.
  Suggested: Before ref_name = f'refs/heads/apm-pin-{sha[:12]}', add: if not re.fullmatch(r'[0-9a-f]{40}', sha): _log.debug('fetch_sha_into_bare: sha %r is not a valid 40-char hex SHA, skipping pin ref', sha); return

OSS Growth Hacker

• [recommended] CHANGELOG entry buries the user-visible impact behind implementation detail at CHANGELOG.md
  The current entry leads with the mechanism ('Bare-cache git commands now use --git-dir instead of -C') rather than the user promise ('apm install now works correctly on macOS with git 2.53.0 and SHA-pinned packages no longer silently fail'). New users scanning the CHANGELOG to decide whether to upgrade will not recognize why this matters to them.
  Suggested: Lead with the user outcome: 'apm install now works on macOS git 2.53.0 (Homebrew): bare-cache commands switch to --git-dir to satisfy the safe.bareRepository=explicit default, and fetched SHAs are pinned as synthetic refs so git clone --local --shared no longer silently omits them. (fix(deps): bare cache incompatible with git 2.53.0 safe.bareRepository + fetched SHAs not ref-reachable #1267)'

Auth Expert

• [nit] _pin_sha_as_head_ref omits env=env, breaking subprocess env hygiene parity at src/apm_cli/deps/bare_cache.py
  Every other subprocess.run call in bare_cache.py passes the sanitized env dict (GIT_TERMINAL_PROMPT=0, no token-embedded state). _pin_sha_as_head_ref inherits the raw parent-process environment. For git update-ref this is safe (local-only, no network calls, no token leakage) -- but the risk is hygiene: a future maintainer copying this pattern for a network-touching command would silently inherit live credentials.
  Suggested: Add env=env to the subprocess.run call in _pin_sha_as_head_ref for consistency with every other site in the file.

Doc Writer

• [nit] CHANGELOG entry is a single compound sentence that buries the second fix after a mid-clause parenthetical at CHANGELOG.md
  The parenthetical '(safe.bareRepository=explicit default)' interrupts the first clause, making the second fix harder to scan. A semicolon after the parenthetical matches the cadence of nearby multi-fix entries.
  Suggested: - Bare-cache git commands now use --git-dir instead of -C for compatibility with git 2.53.0 (safe.bareRepository=explicit default); fetched SHAs are pinned as synthetic refs so git clone --local --shared includes them in the object transfer. (#1267)

Test Coverage Expert

• [recommended] fetch_sha_into_bare has no integration-with-fixtures test exercising real git bare operations at tests/unit/deps/test_shared_clone_cache.py
  bare_cache.py is the install pipeline surface; the tier-floor matrix requires integration-with-fixtures coverage for install-pipeline changes (real subprocess, real bare repo, no mocked boundary). All tests for fetch_sha_into_bare patch subprocess.run, so they cannot catch a regression where --git-dir is silently reverted to -C or the pin ref is malformed at the git level. Grepped tests/integration/ for bare_cache, fetch_sha_into_bare, _pin_sha -- no match.
  Suggested: Add a test under tests/integration/ that: (1) calls fetch_sha_into_bare against a real local bare repo, (2) asserts refs/heads/apm-pin-(sha12) exists after the call, and (3) verifies --git-dir is used. Mark with @pytest.mark.requires_git.
  Proof (passed at unit): tests/unit/deps/test_shared_clone_cache.py::TestFetchShaIntoBare::test_sha_already_present_returns_true_without_fetch -- proves: fetch_sha_into_bare returns True without a network fetch and pins the ref when SHA is already present [portability-by-manifest, devx]

• [recommended] install-pipeline floor gap: no integration-with-fixtures evidence for fetch_sha_into_bare at tests/integration/
  Tier-floor compliance: unit tier passed above; floor tier is missing. Grepped tests/integration/ -- only test_install_subdir_dedup_e2e.py touches SharedCloneCache, but it does not call fetch_sha_into_bare or assert pin-ref creation.
  Suggested: New file tests/integration/test_bare_cache_integration.py with a test that creates a local bare repo, calls fetch_sha_into_bare, and reads refs/heads/apm-pin-(sha12) from the bare with git for-each-ref.
  Proof (missing at integration-with-fixtures): tests/integration/test_bare_cache_integration.py::test_fetch_sha_into_bare_pins_ref_in_real_bare_repo -- proves: fetch_sha_into_bare with real git uses --git-dir (not -C) and creates the apm-pin ref [portability-by-manifest, devx]

• [nit] Shallow and broad fetch paths do not assert the pin ref name, only the call count at tests/unit/deps/test_shared_clone_cache.py:1181
  test_sha_already_present_returns_true_without_fetch explicitly asserts refs/heads/apm-pin-{sha[:12]} is in the argv. The two sibling tests only add a MagicMock(returncode=0) for the extra call; neither asserts the ref name string.
  Suggested: Add pin_argv = mock_run.call_args_list[-1][0][0]; assert 'update-ref' in pin_argv; assert f'refs/heads/apm-pin-{sha[:12]}' in pin_argv to each sibling test.

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

Generated by PR Review Panel for issue #1268 · ● 2.2M · ◷

[Copilot]

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated 7 comments.