From df65d984477e543d9444847c3f01b8e736e52438 Mon Sep 17 00:00:00 2001 From: Daniel Meppiel Date: Mon, 4 May 2026 18:37:43 +0200 Subject: [PATCH 1/5] fix(deps): subdir-agnostic bare cache fixes parallel sparse-checkout race (#1126) Refactor SharedCloneCache (WS2) to store BARE clones keyed only on (host, owner, repo, ref). Each consumer materializes its own working tree from the bare via 'git clone --local --shared --no-checkout' + 'git checkout HEAD'. Mirrors WS3's GitCache._create_checkout pattern. Pre-fix: two parallel subdir deps from the same upstream repo+ref raced through the cache; one consumer would materialize its subdir into the cached path, then the second consumer would find the cached path without its own subdir, raising RuntimeError("Subdirectory '...' not found in repository"). Post-fix: the cache is subdir-agnostic. Two consumers requesting different subdirs of the same repo+ref share one bare without racing. Per-consumer materialization runs against a unique mkdtemp path. Implementation: - _execute_transport_plan: Strategy template extracted from _clone_with_fallback; both _wt_action (existing GitPython path) and _bare_action (new subprocess-based 3-tier ladder) plug in. - _bare_clone_with_fallback: 3-tier fallback for SHA refs (init+fetch / shallow / full), 2-tier for symbolic refs. - _materialize_from_bare: per-consumer working tree with CRLF + LFS pinning; SHA resolved from --git-dir= rev-parse HEAD to avoid Windows file-handle leak (no Repo() on consumer dir). - _scrub_bare_remote_url: redacts remote.origin.url to redacted:// after every successful bare clone; logs at WARNING on failure (audit trail for token-leak-aware operators). - WS2 callsite rewritten in download_subdirectory_package; threads _ws2_resolved_commit past the legacy SHA-capture branch. - shared_clone_cache.py: docstring rewritten; APM_DEBUG-gated bare-shape invariant assert. Tests: - 21 unit tests in test_shared_clone_cache.py covering 18 of 19 design scenarios (race, fallback ladder, materialize lifetime, rmtree, invariant, error wording). - New parametrized E2E (test_install_subdir_dedup_e2e.py) with 3 ref variants (symbolic-https / sha-https / default-branch); added to scripts/test-integration.sh. - All 7573 unit tests pass; ruff check + ruff format --check pass. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- scripts/test-integration.sh | 15 +- src/apm_cli/deps/github_downloader.py | 531 +++++++++++++-- src/apm_cli/deps/shared_clone_cache.py | 33 +- .../test_install_subdir_dedup_e2e.py | 145 ++++ tests/unit/deps/test_shared_clone_cache.py | 630 +++++++++++++++++- 5 files changed, 1256 insertions(+), 98 deletions(-) create mode 100644 tests/integration/test_install_subdir_dedup_e2e.py diff --git a/scripts/test-integration.sh b/scripts/test-integration.sh index 0756a96a..a8afe07b 100755 --- a/scripts/test-integration.sh +++ b/scripts/test-integration.sh @@ -388,7 +388,20 @@ run_e2e_tests() { log_error "APM Dependencies integration tests failed!" exit 1 fi - + + # Subdirectory dedup race E2E (#1126): two sibling subdirs of the + # same upstream repo+ref must install in parallel without the + # "Subdirectory ... not found" race the v1 cache produced. + log_info "Running #1126 parallel subdir dedup E2E..." + echo "Command: pytest tests/integration/test_install_subdir_dedup_e2e.py -v -s --tb=short -m integration" + + if pytest tests/integration/test_install_subdir_dedup_e2e.py -v -s --tb=short -m integration; then + log_success "#1126 subdir dedup E2E passed!" + else + log_error "#1126 subdir dedup E2E failed!" + exit 1 + fi + # Run Transport Selection integration tests (issue #778) # Always-on cases use HTTPS against a public repo. SSH cases auto-skip # when no usable SSH key is available for git@github.com. diff --git a/src/apm_cli/deps/github_downloader.py b/src/apm_cli/deps/github_downloader.py index c1557ea6..ac70ea92 100644 --- a/src/apm_cli/deps/github_downloader.py +++ b/src/apm_cli/deps/github_downloader.py @@ -1,10 +1,12 @@ """GitHub package downloader for APM dependencies.""" +import logging import os import random # noqa: F401 import re import shutil import stat # noqa: F401 +import subprocess import sys import tempfile import time # noqa: F401 @@ -101,6 +103,49 @@ def _rmtree(path) -> None: robust_rmtree(path, ignore_errors=True) +def _scrub_bare_remote_url(bare_path: Path, git_exe: str, env: dict[str, str]) -> None: + """Redact ``remote.origin.url`` in a bare repo's ``.git/config``. + + After a successful bare clone, ``remote.origin.url`` retains the + tokenized URL (e.g. ``https://oauth2:@github.com/...``). The + bare is read-only after this point in the WS2 dedup pipeline (no + further fetches), so the URL is dead weight that just persists the + token on disk. Replace with ``redacted://`` to eliminate the + on-disk token footprint. + + Best-effort: ``check=False`` so a config-set failure does not abort + the clone (the bare is still functionally correct without the + redaction). Convergent panel finding (auth + supply-chain MAJOR). + On exception, log at WARNING so token-leak-aware operators have an + audit trail (supply-chain reviewer follow-up: security mechanisms + must not fail silently). + """ + try: + result = subprocess.run( + [git_exe, "-C", str(bare_path), "remote", "set-url", "origin", "redacted://"], + env=env, + check=False, + capture_output=True, + timeout=10, + ) + if result.returncode != 0: + logging.getLogger(__name__).warning( + "Failed to redact remote URL from bare repo config at %s " + "(git exit=%d). Tokenized URL may persist on disk until " + "shared cache cleanup.", + bare_path, + result.returncode, + ) + except Exception as exc: + logging.getLogger(__name__).warning( + "Exception while redacting remote URL from bare repo config " + "at %s: %s. Tokenized URL may persist on disk until shared " + "cache cleanup.", + bare_path, + exc, + ) + + class GitProgressReporter(RemoteProgress): """Report git clone progress to Rich Progress.""" @@ -577,14 +622,16 @@ def _clone_with_fallback( verbose_callback=None, **clone_kwargs, ) -> Repo: - """Clone a repository following the TransportSelector plan. + """Clone a working-tree repository following the TransportSelector plan. - The transport selector decides protocol order and strictness based on - the user's URL form, CLI/env preferences, and git ``insteadOf`` config. - Strict-by-default: explicit ``ssh://``, ``https://``, and ``http://`` - URLs no longer silently fall back to a different protocol. To restore - the legacy permissive chain, set ``--allow-protocol-fallback`` or - ``APM_ALLOW_PROTOCOL_FALLBACK=1``. + Thin adapter over :meth:`_execute_transport_plan` that supplies a + working-tree clone action (``Repo.clone_from``). Behavior is + unchanged from the pre-#1126 implementation, except every clone + attempt now begins with ``shutil.rmtree(target, ignore_errors=True)`` + for symmetry with the bare-clone path. This is strictly safer + (clean slate per attempt) and matches the existing behavior on + the second-and-subsequent attempts where target may contain a + partial clone from the failed first attempt. Args: repo_url_base: Base repository reference (owner/repo) @@ -600,7 +647,64 @@ def _clone_with_fallback( Raises: RuntimeError: If the planned attempt(s) all fail. """ - last_error = None + repo_holder: list[Repo] = [] + + def _wt_action(url: str, env: dict[str, str], target: Path) -> None: + # Pre-attempt cleanup: GitPython's Repo.clone_from refuses a + # non-empty target. Symmetric with _bare_action so retries + # always start from a clean slate. Behavior change from the + # pre-#1126 implementation - covered by 6.13. + if target.exists(): + shutil.rmtree(target, ignore_errors=True) + repo_holder.append( + Repo.clone_from( + url, + target, + env=env, + progress=progress_reporter, + **clone_kwargs, + ) + ) + + self._execute_transport_plan( + repo_url_base, + target_path, + dep_ref=dep_ref, + clone_action=_wt_action, + verbose_callback=verbose_callback, + ) + return repo_holder[0] + + def _execute_transport_plan( + self, + repo_url_base: str, + target_path: Path, + *, + dep_ref: DependencyReference | None = None, + clone_action: Callable[[str, dict[str, str], Path], None], + verbose_callback=None, + ) -> None: + """Execute a clone action against a TransportPlan with full fallback. + + Owns: + - TransportPlan resolution + per-dep transport warnings. + - Per-attempt URL + env construction. + - ADO bearer in-attempt retry on 401/Unauthorized. + - Cross-protocol fallback warning on protocol switch. + - Aggregate error construction on exhaustion. + + The provided ``clone_action`` performs the actual clone for one + attempt. Working-tree clones pass an action that wraps + ``Repo.clone_from``; bare clones pass an action that wraps + ``subprocess.run([git, "clone", "--bare", ...])``. The template + catches both ``GitCommandError`` (GitPython) and + ``subprocess.CalledProcessError`` (subprocess) so either action + composes correctly with ADO bearer retry and protocol fallback. + + Raises: + RuntimeError: If all planned attempts fail. + """ + last_error: Exception | None = None is_ado = dep_ref and dep_ref.is_azure_devops() dep_host = dep_ref.host if dep_ref else None @@ -723,20 +827,17 @@ def _env_for(attempt: TransportAttempt) -> dict[str, str]: try: _debug(f"Attempting clone with {attempt.label} (URL sanitized)") - repo = Repo.clone_from( - url, - target_path, - env=_env_for(attempt), - progress=progress_reporter, - **clone_kwargs, - ) + clone_action(url, _env_for(attempt), target_path) if verbose_callback: display = self._sanitize_git_error(url) if attempt.use_token else url verbose_callback(f"Cloned from: {display}") - return repo - except GitCommandError as e: + return + except (GitCommandError, subprocess.CalledProcessError) as e: # ADO bearer fallback for clone (mirrors validation/list_remote_refs): # PAT was rejected -> silently retry this attempt with az-cli bearer. + # Note: subprocess errors may include tokenized URLs in stderr; + # the existing _sanitize_git_error / _sanitize_url path used in the + # final error message handles redaction at the boundary. err_msg = str(e) if ( is_ado @@ -768,13 +869,11 @@ def _env_for(attempt: TransportAttempt) -> dict[str, str]: auth_scheme="bearer", ) bearer_env = {**self.git_env, **build_ado_bearer_git_env(bearer)} - repo = Repo.clone_from( - bearer_url, - target_path, - env=bearer_env, - progress=progress_reporter, - **clone_kwargs, - ) + # Bearer retry composes with bare-action's tier + # ladder: clone_action will rmtree any partial + # state (tier-1 init+fetch) before re-attempting + # against the bearer URL. See 6.15. + clone_action(bearer_url, bearer_env, target_path) self.auth_resolver.emit_stale_pat_diagnostic( dep_host or "dev.azure.com" ) @@ -782,8 +881,12 @@ def _env_for(attempt: TransportAttempt) -> dict[str, str]: verbose_callback( "Cloned from: (sanitized) via AAD bearer fallback" ) - return repo - except (AzureCliBearerError, GitCommandError): + return + except ( + AzureCliBearerError, + GitCommandError, + subprocess.CalledProcessError, + ): pass except ImportError: pass @@ -860,6 +963,278 @@ def _env_for(attempt: TransportAttempt) -> dict[str, str]: raise RuntimeError(error_msg) + # ------------------------------------------------------------------ + # Bare-clone helpers (#1126: subdir-agnostic shared cache) + # ------------------------------------------------------------------ + + def _bare_clone_with_fallback( + self, + repo_url_base: str, + bare_target: Path, + *, + dep_ref: DependencyReference, + ref: str | None, + is_commit_sha: bool, + ) -> None: + """Clone a repository as a bare repo, with full transport-plan fallback. + + Sibling helper to :meth:`_clone_with_fallback`. Composes via + :meth:`_execute_transport_plan` so it inherits ADO bearer retry + and protocol fallback automatically. The bare clone is + subdir-agnostic (no sparse cone), so a single bare can serve N + consumers each materializing a different subdir from the same + repo+ref - this is the core fix for #1126. + + 3-tier strategy (per design.md sec 5.2): + + - Tier 1 (SHA refs): ``git init --bare && git remote add + origin && git fetch --depth=1 origin ``. Requires + server-side ``uploadpack.allowReachableSHA1InWant=true`` + (default on github.com / GHES; some older GHE / ADO Server / + Bitbucket Server reject this). + - Tier 1 (symbolic / default branch): ``git clone --bare + --depth=1 [--branch ] ``. + - Tier 2 (both): full ``git clone --bare ``, validate via + ``git rev-parse --verify ^{commit}`` for SHA refs. + - Tier 3: re-raise. + + After every successful tier, ``git update-ref HEAD `` is + invoked for SHA refs so consumer ``git rev-parse HEAD`` resolves + unambiguously (the v3 BLOCKER fix). Then + :func:`_scrub_bare_remote_url` redacts the tokenized + ``remote.origin.url`` from ``.git/config`` to eliminate + on-disk token persistence (panel convergent finding). + + Note: bare-integrity verification (post-clone ``rev-parse HEAD + == known_sha``) is intentionally deferred. The bare is + ephemeral, mode-0700, and produced by a trusted (in-tree) + callable. If ``SharedCloneCache`` is ever opened to + plugin/user-supplied callables, restore the integrity check at + the cache boundary. See design.md sec 12 (Bare integrity + verification). + """ + from ..utils.git_env import get_git_executable + + git_exe = get_git_executable() + + def _bare_action(url: str, env: dict[str, str], target: Path) -> None: + # Pre-attempt cleanup: any prior tier-1 partial state must be + # wiped before re-attempting (e.g. on ADO bearer retry the + # template re-invokes _bare_action with a fresh URL/env, and + # the tier-1 init+fetch leaves a half-built bare on disk). + # See 6.15. + if target.exists(): + shutil.rmtree(target, ignore_errors=True) + + if is_commit_sha: + # Tier 1: init + fetch by SHA. + # Note: `git remote add origin ` stores the + # tokenized URL in `.git/config`. The ADO-bearer env + # approach relies on http.extraHeader (not stored URL + # auth), so this is safe today. _scrub_bare_remote_url + # below redacts the URL after a successful clone so the + # token does not persist on disk. + try: + subprocess.run( + [git_exe, "init", "--bare", str(target)], + env=env, + check=True, + capture_output=True, + ) + subprocess.run( + [git_exe, "-C", str(target), "remote", "add", "origin", url], + env=env, + check=True, + capture_output=True, + ) + subprocess.run( + [git_exe, "-C", str(target), "fetch", "--depth=1", "origin", ref], + env=env, + check=True, + capture_output=True, + timeout=300, + ) + # CRITICAL (v3 BLOCKER): init+fetch leaves HEAD pointing + # at refs/heads/main which doesn't exist locally. + # Without update-ref, consumer's `git rev-parse HEAD` + # is ambiguous. See 6.18. + subprocess.run( + [git_exe, "-C", str(target), "update-ref", "HEAD", ref], + env=env, + check=True, + capture_output=True, + ) + _scrub_bare_remote_url(target, git_exe, env) + return + except subprocess.CalledProcessError: + # Tier 2: full bare clone, validate SHA, set HEAD. + shutil.rmtree(target, ignore_errors=True) + subprocess.run( + [git_exe, "clone", "--bare", url, str(target)], + env=env, + check=True, + capture_output=True, + timeout=600, + ) + subprocess.run( + [ + git_exe, + "-C", + str(target), + "rev-parse", + "--verify", + f"{ref}^{{commit}}", + ], + env=env, + check=True, + capture_output=True, + ) + subprocess.run( + [git_exe, "-C", str(target), "update-ref", "HEAD", ref], + env=env, + check=True, + capture_output=True, + ) + _scrub_bare_remote_url(target, git_exe, env) + return + + # Symbolic ref or default branch. + args = [git_exe, "clone", "--bare", "--depth=1"] + if ref: + args += ["--branch", ref] + args += [url, str(target)] + try: + subprocess.run(args, env=env, check=True, capture_output=True, timeout=300) + _scrub_bare_remote_url(target, git_exe, env) + return + except subprocess.CalledProcessError: + # Tier 2: full bare clone (no shallow, no --branch). + shutil.rmtree(target, ignore_errors=True) + subprocess.run( + [git_exe, "clone", "--bare", url, str(target)], + env=env, + check=True, + capture_output=True, + timeout=600, + ) + _scrub_bare_remote_url(target, git_exe, env) + return + + self._execute_transport_plan( + repo_url_base, + bare_target, + dep_ref=dep_ref, + clone_action=_bare_action, + ) + + def _materialize_from_bare( + self, + bare_path: Path, + consumer_dir: Path, + *, + ref: str | None, + env: dict[str, str], + known_sha: str | None = None, + ) -> str: + """Create a working-tree checkout from a bare repo via local-shared clone. + + Mirrors :class:`GitCache`'s ``_create_checkout`` pattern: each + consumer gets its own working tree backed by the shared bare's + object database (via ``objects/info/alternates``). Hardlink-cheap + and concurrent-safe (consumer dirs are unique per call). + + SHA resolution policy (lifetime invariant 5.2.1): + - If ``known_sha`` is provided (caller passed a 40-char SHA + ref), use it directly. Avoids ``rev-parse HEAD`` which is + ambiguous on init+fetch bares before update-ref runs. + - Otherwise, resolve from the BARE via ``git --git-dir + rev-parse HEAD``. NOT from the consumer - opening + ``Repo(consumer_dir)`` leaks a Windows file handle that + blocks downstream rmtree. + + CRLF + LFS pinning before checkout: + - ``core.autocrlf=false`` guarantees byte-identical content + across consumers regardless of the user's global git config. + - ``filter.lfs.smudge=""`` + ``filter.lfs.required=false`` + disables LFS smudge cross-platform (the empty string trick + works everywhere; ``cat`` is not on Windows PATH). + + Returns: + The resolved commit SHA. Caller threads this into + ``resolved_commit`` for the lockfile. + """ + from ..utils.git_env import get_git_executable + + git_exe = get_git_executable() + + if known_sha: + resolved_sha = known_sha + else: + sha_result = subprocess.run( + [git_exe, "--git-dir", str(bare_path), "rev-parse", "HEAD"], + capture_output=True, + text=True, + timeout=10, + env=env, + check=True, + ) + resolved_sha = sha_result.stdout.strip() + + consumer_dir.parent.mkdir(parents=True, exist_ok=True) + # --no-checkout because we want to set core.autocrlf and disable + # LFS smudge BEFORE checkout writes any file content. + subprocess.run( + [ + git_exe, + "clone", + "--local", + "--shared", + "--no-checkout", + str(bare_path), + str(consumer_dir), + ], + capture_output=True, + text=True, + timeout=60, + env=env, + check=True, + ) + # CRLF determinism (panel: byte-identical across users). + subprocess.run( + [git_exe, "-C", str(consumer_dir), "config", "core.autocrlf", "false"], + capture_output=True, + text=True, + timeout=10, + env=env, + check=True, + ) + # Disable LFS smudge cross-platform: empty-string smudge is the + # portable equivalent of `git lfs smudge --skip`. The `cat` + # alternative is not on Windows PATH. + for key, val in ( + ("filter.lfs.smudge", ""), + ("filter.lfs.clean", ""), + ("filter.lfs.process", ""), + ("filter.lfs.required", "false"), + ): + subprocess.run( + [git_exe, "-C", str(consumer_dir), "config", key, val], + capture_output=True, + text=True, + timeout=10, + env=env, + check=False, + ) + subprocess.run( + [git_exe, "-C", str(consumer_dir), "checkout", "HEAD"], + capture_output=True, + text=True, + timeout=60, + env=env, + check=True, + ) + return resolved_sha + # ------------------------------------------------------------------ # Remote ref enumeration (no clone required) # ------------------------------------------------------------------ @@ -1665,54 +2040,59 @@ def download_subdirectory_package( from ..config import get_apm_temp_dir temp_dir = None - shared_clone_path: Path | None = None + shared_bare_path: Path | None = None + # WS2 path resolves the SHA from the BARE so we don't pay + # rev-parse twice (or open the working-tree Repo unnecessarily). + # See design.md sec 5.5: _ws2_resolved_commit threads the SHA past + # the generic Repo(temp_clone_path).head.commit.hexsha block below. + _ws2_resolved_commit: str | None = None try: if _persistent_checkout is not None: # WS3: persistent cache hit -- use the cached checkout directly. temp_clone_path = _persistent_checkout elif use_shared: - # Try shared clone path. clone_fn encapsulates the full - # sparse-checkout -> fallback-clone logic. - def _shared_clone_fn(clone_target: Path) -> None: - sparse_path = clone_target - sparse_ok = self._try_sparse_checkout(dep_ref, sparse_path, subdir_path, ref) - if sparse_ok: - return - # Sparse failed -- full clone into same target - # (shared cache doesn't care about the sparse/full distinction) - full_path = clone_target.parent / "repo_clone" - is_commit_sha = ref and re.match(r"^[a-f0-9]{7,40}$", ref) is not None - clone_kwargs = {"dep_ref": dep_ref} - if is_commit_sha: - clone_kwargs["no_checkout"] = True - else: - clone_kwargs["depth"] = 1 - if ref: - clone_kwargs["branch"] = ref - self._clone_with_fallback(dep_ref.repo_url, full_path, **clone_kwargs) - if is_commit_sha: - repo_obj = None - try: - repo_obj = Repo(full_path) - repo_obj.git.checkout(ref) - except Exception as e: - raise RuntimeError(f"Failed to checkout commit {ref}: {e}") from e - finally: - _close_repo(repo_obj) - # Move full clone into expected position (rename is atomic - # on same filesystem). If sparse path already exists from - # the failed attempt, remove it first. - if sparse_path.exists(): - _rmtree(sparse_path) - full_path.rename(sparse_path) + # WS2 (#1126): shared cache holds BARE clones keyed by + # (host, owner, repo, ref). Each consumer materializes its + # own working tree from the bare; this is subdir-agnostic + # so two parallel consumers requesting different + # subdirectories of the same repo+ref can share one bare + # without racing on sparse-checkout. See design.md sec 5.5. + is_commit_sha = ref and re.match(r"^[a-f0-9]{7,40}$", ref) is not None + + def _shared_bare_clone_fn(bare_target: Path) -> None: + self._bare_clone_with_fallback( + dep_ref.repo_url, + bare_target, + dep_ref=dep_ref, + ref=ref, + is_commit_sha=bool(is_commit_sha), + ) try: - shared_clone_path = shared_cache.get_or_clone( - cache_host, cache_owner, cache_repo, ref, _shared_clone_fn + shared_bare_path = shared_cache.get_or_clone( + cache_host, cache_owner, cache_repo, ref, _shared_bare_clone_fn ) except Exception as e: raise RuntimeError(f"Failed to clone repository: {e}") from e - temp_clone_path = shared_clone_path + + # Per-consumer materialization. mkdtemp gives a unique + # path so concurrent consumers do not collide. The bare + # is read-only after this point; only the consumer dir + # is written to. + temp_dir = tempfile.mkdtemp(dir=get_apm_temp_dir()) + temp_clone_path = Path(temp_dir) / "consumer" + try: + _ws2_resolved_commit = self._materialize_from_bare( + shared_bare_path, + temp_clone_path, + ref=ref, + env=self._git_env_dict(), + known_sha=ref if is_commit_sha else None, + ) + except Exception as e: + raise RuntimeError( + f"Failed to materialize working tree from shared bare: {e}" + ) from e else: # Legacy per-dep clone path (no shared cache). temp_dir = tempfile.mkdtemp(dir=get_apm_temp_dir()) @@ -1818,14 +2198,21 @@ def _shared_clone_fn(clone_target: Path) -> None: # Capture commit SHA; close the Repo object immediately so its file # handles are released before _rmtree() runs in the finally block. - repo = None - try: - repo = Repo(temp_clone_path) - resolved_commit = repo.head.commit.hexsha - except Exception: - resolved_commit = "unknown" - finally: - _close_repo(repo) + # WS2 path skips this because _materialize_from_bare already + # resolved the SHA from the bare (avoids opening Repo on the + # consumer dir, which leaks a Windows file handle that would + # block the rmtree below; see design.md sec 5.5). + if _ws2_resolved_commit is not None: + resolved_commit = _ws2_resolved_commit + else: + repo = None + try: + repo = Repo(temp_clone_path) + resolved_commit = repo.head.commit.hexsha + except Exception: + resolved_commit = "unknown" + finally: + _close_repo(repo) # Update progress - validating if progress_obj and progress_task_id is not None: diff --git a/src/apm_cli/deps/shared_clone_cache.py b/src/apm_cli/deps/shared_clone_cache.py index 2869b64f..e5b0b92d 100644 --- a/src/apm_cli/deps/shared_clone_cache.py +++ b/src/apm_cli/deps/shared_clone_cache.py @@ -2,9 +2,17 @@ When multiple subdirectory deps reference the same upstream repository at the same ref (e.g. ``github:owner/repo/skills/X@main`` and -``github:owner/repo/agents/Y@main``), a single clone is shared across all -consumers within one install run. This mirrors uv's strategy of caching -Git repos by fully-resolved commit hash. +``github:owner/repo/agents/Y@main``), a single BARE clone is shared across +all consumers within one install run. Each consumer materializes its own +working tree from the bare via ``git clone --local --shared --no-checkout`` +plus ``git checkout HEAD``. This mirrors uv's strategy of caching Git repos +by fully-resolved commit hash, and the WS3 ``GitCache`` pattern internally. + +Why bare (#1126): subdir-agnostic. Two parallel consumers requesting +different subdirectories of the same repo+ref share one bare without +racing on sparse-checkout state (the original v1 design materialized a +sparse working tree at the cache layer and lost the second consumer's +files when both threads raced through the cache). The cache is instance-scoped (NOT module-level) to avoid races between parallel test invocations. Thread-safety is guaranteed via per-key locks. @@ -15,6 +23,7 @@ """ import logging +import os import shutil import tempfile import threading @@ -90,11 +99,27 @@ def get_or_clone( dir=str(self._base_dir) if self._base_dir else None, prefix=f"apm_shared_{owner}_{repo}_", ) - clone_path = Path(temp_dir) / "repo" + clone_path = Path(temp_dir) / "bare" with self._lock: self._temp_dirs.append(temp_dir) try: clone_fn(clone_path) + # Debug-mode shape invariant: clone_fn MUST produce a + # bare repo. A bare has HEAD as a regular file at the + # root and no nested .git/ dir. Working-tree clones + # have it the other way around. This is the canary + # that catches a regression where someone reverts to + # the v1 "materialize-in-cache" pattern. See 6.16. + if os.environ.get("APM_DEBUG"): + head_file = clone_path / "HEAD" + git_dir = clone_path / ".git" + if not head_file.is_file() or git_dir.exists(): + raise RuntimeError( + f"SharedCloneCache invariant violated: " + f"{clone_path} is not a bare repo " + f"(HEAD file present: {head_file.is_file()}, " + f".git/ present: {git_dir.exists()})" + ) entry.path = clone_path return clone_path except Exception as exc: diff --git a/tests/integration/test_install_subdir_dedup_e2e.py b/tests/integration/test_install_subdir_dedup_e2e.py new file mode 100644 index 00000000..6fce665a --- /dev/null +++ b/tests/integration/test_install_subdir_dedup_e2e.py @@ -0,0 +1,145 @@ +"""End-to-end test for the parallel sparse-checkout race fix (#1126). + +This test exercises the parallel download race directly against the +real GitHub-hosted ``github/awesome-copilot`` repo with two sibling +subdirectory dependencies sharing the same ``(host, owner, repo, ref)`` +cache key. + +Pre-#1126 fix, this test reliably failed with +``RuntimeError("Subdirectory '...' not found in repository")`` because +the v1 cache materialized one subdir at the cache layer and the second +consumer found the cached dir without its expected subdir. + +Parametrized across ``ref_kind`` to cover all three materialization +paths: +- ``symbolic-https``: ref="main" (the original 6.2 baseline) +- ``sha-https``: ref pinned to a known commit (exercises + ``_bare_clone_with_fallback``'s 3-tier SHA path) +- ``default-branch``: no ref (exercises the no-ref path) + +Marked ``integration`` so it only runs in the integration suite (it +requires network and a GitHub token like the rest of +``tests/integration/test_apm_dependencies.py``). +""" + +from __future__ import annotations + +import concurrent.futures +import os +import shutil +import tempfile +from pathlib import Path + +import pytest + +from apm_cli.deps.github_downloader import GitHubPackageDownloader +from apm_cli.deps.shared_clone_cache import SharedCloneCache +from apm_cli.models.dependency.reference import DependencyReference + +# Two sibling subdirs under the same upstream repo+ref. Both are +# present on github/awesome-copilot at the time of writing; if either +# is removed upstream, swap with another pair from +# `gh api repos/github/awesome-copilot/contents/skills --jq '.[].name'`. +SUBDIR_A = "skills/acquire-codebase-knowledge" +SUBDIR_B = "skills/agent-governance" + +# A historical commit on github/awesome-copilot main that contains +# both subdirs. Resolved at fixture-setup time via the GitHub API; if +# resolution fails, the sha-https variant is skipped (rare network / +# upstream-API issue). The KNOWN_SHA constant below is a fallback for +# offline scenarios where the resolver cannot reach the API. +KNOWN_SHA: str | None = None + + +def _resolve_known_sha() -> str | None: + """Resolve a real commit SHA on github/awesome-copilot for the sha-https variant.""" + import subprocess + + try: + result = subprocess.run( + ["gh", "api", "repos/github/awesome-copilot/commits/main", "--jq", ".sha"], + capture_output=True, + text=True, + timeout=15, + ) + if result.returncode == 0 and result.stdout.strip(): + sha = result.stdout.strip() + if len(sha) == 40 and all(c in "0123456789abcdef" for c in sha): + return sha + except (FileNotFoundError, subprocess.SubprocessError, OSError): + pass + return None + + +@pytest.mark.integration +@pytest.mark.parametrize( + "ref_kind,ref_value", + [ + ("symbolic-https", "main"), + ("default-branch", None), + ("sha-https", "RESOLVE_AT_RUNTIME"), + ], +) +def test_two_subdirs_same_repo_parallel(ref_kind: str, ref_value: str | None) -> None: + """Two sibling subdir deps from same repo+ref download in parallel. + + Asserts: + 1. Both subdir packages materialize with their expected content. + 2. No ``RuntimeError("Subdirectory ... not found")`` raised. + 3. Both consumers receive the same ``resolved_commit`` (cache hit + on second consumer). + """ + github_token = os.getenv("GITHUB_APM_PAT") or os.getenv("GITHUB_TOKEN") + if not github_token: + pytest.skip("GitHub token required (GITHUB_APM_PAT or GITHUB_TOKEN)") + + if ref_kind == "sha-https": + ref_value = _resolve_known_sha() + if ref_value is None: + pytest.skip("Could not resolve a known SHA on github/awesome-copilot/main") + + test_dir = Path(tempfile.mkdtemp(prefix="apm_e2e_1126_")) + try: + # Build two dep refs sharing the same (host, owner, repo, ref) + # cache key so they race through SharedCloneCache. + ref_suffix = f"#{ref_value}" if ref_value else "" + dep_a = DependencyReference.parse(f"github/awesome-copilot/{SUBDIR_A}{ref_suffix}") + dep_b = DependencyReference.parse(f"github/awesome-copilot/{SUBDIR_B}{ref_suffix}") + + target_a = test_dir / "modules" / "a" + target_b = test_dir / "modules" / "b" + target_a.parent.mkdir(parents=True, exist_ok=True) + + # One downloader sharing the cache - mirrors install/phases/resolve.py + # which attaches a single SharedCloneCache to the downloader. + downloader = GitHubPackageDownloader() + (test_dir / ".cache").mkdir() + with SharedCloneCache(base_dir=test_dir / ".cache") as shared_cache: + downloader.shared_clone_cache = shared_cache + + # Drive both downloads in parallel via ThreadPoolExecutor + # (mirrors apm_resolver.py parallel BFS dispatch). + with concurrent.futures.ThreadPoolExecutor(max_workers=2) as ex: + fa = ex.submit(downloader.download_subdirectory_package, dep_a, target_a) + fb = ex.submit(downloader.download_subdirectory_package, dep_b, target_b) + # Both must succeed without the v1 + # "Subdirectory ... not found" error. + pkg_a = fa.result(timeout=120) + pkg_b = fb.result(timeout=120) + + # Both subdirs must have materialized with content. + assert target_a.exists(), f"{SUBDIR_A} not materialized" + assert target_b.exists(), f"{SUBDIR_B} not materialized" + assert any(target_a.iterdir()), f"{SUBDIR_A} is empty" + assert any(target_b.iterdir()), f"{SUBDIR_B} is empty" + + # Lockfile parity: same cache key -> same resolved_commit. + sha_a = getattr(pkg_a, "resolved_commit", None) or getattr(pkg_a, "commit_sha", None) + sha_b = getattr(pkg_b, "resolved_commit", None) or getattr(pkg_b, "commit_sha", None) + if sha_a and sha_b and sha_a != "unknown" and sha_b != "unknown": + assert sha_a == sha_b, ( + f"Sibling subdirs from same repo+ref must resolve to " + f"same commit, got a={sha_a} b={sha_b}" + ) + finally: + shutil.rmtree(test_dir, ignore_errors=True) diff --git a/tests/unit/deps/test_shared_clone_cache.py b/tests/unit/deps/test_shared_clone_cache.py index fb182bee..2c0c2dce 100644 --- a/tests/unit/deps/test_shared_clone_cache.py +++ b/tests/unit/deps/test_shared_clone_cache.py @@ -192,31 +192,33 @@ def test_two_subdir_deps_share_single_clone(self, tmp_path: Path) -> None: clone_call_count = {"n": 0} - # Patch _try_sparse_checkout to fail (force full clone path) - # Patch _clone_with_fallback to create the directory structure - def fake_clone(repo_url, target_path, **kwargs): + # New paradigm: SharedCloneCache holds bare clones; consumers + # materialize their own working tree via _materialize_from_bare. + # Patch _bare_clone_with_fallback to be the cache-populating + # callable; patch _materialize_from_bare to lay down the subdir + # contents per consumer. + def fake_bare_clone(repo_url, bare_target, **kwargs): clone_call_count["n"] += 1 - target_path.mkdir(parents=True, exist_ok=True) - (target_path / "skills" / "X").mkdir(parents=True) - (target_path / "skills" / "X" / "apm.yml").write_text("name: X\nversion: 1.0.0\n") - (target_path / "agents" / "Y").mkdir(parents=True) - (target_path / "agents" / "Y" / "apm.yml").write_text("name: Y\nversion: 1.0.0\n") - # Create a fake .git so Repo() can read commit - (target_path / ".git").mkdir() - return MagicMock() + bare_target.mkdir(parents=True, exist_ok=True) + # Mark as bare-shaped (HEAD file at root, no .git/) so the + # APM_DEBUG invariant in SharedCloneCache would not trip if + # the caller enabled it. + (bare_target / "HEAD").write_text("ref: refs/heads/main\n") + + def fake_materialize(bare_path, consumer_dir, **kwargs): + consumer_dir.mkdir(parents=True, exist_ok=True) + (consumer_dir / "skills" / "X").mkdir(parents=True) + (consumer_dir / "skills" / "X" / "apm.yml").write_text("name: X\nversion: 1.0.0\n") + (consumer_dir / "agents" / "Y").mkdir(parents=True) + (consumer_dir / "agents" / "Y" / "apm.yml").write_text("name: Y\nversion: 1.0.0\n") + return "abc1234567890" with ( - patch.object(downloader, "_try_sparse_checkout", return_value=False), - patch.object(downloader, "_clone_with_fallback", side_effect=fake_clone), - patch("apm_cli.deps.github_downloader.Repo") as mock_repo_cls, + patch.object(downloader, "_bare_clone_with_fallback", side_effect=fake_bare_clone), + patch.object(downloader, "_materialize_from_bare", side_effect=fake_materialize), + patch.object(downloader, "_git_env_dict", return_value={}), patch("apm_cli.deps.github_downloader.validate_apm_package") as mock_validate, - patch("apm_cli.deps.github_downloader._close_repo"), ): - # Configure Repo mock - mock_repo_instance = MagicMock() - mock_repo_instance.head.commit.hexsha = "abc1234567890" - mock_repo_cls.return_value = mock_repo_instance - # Configure validate mock mock_result = MagicMock() mock_result.is_valid = True @@ -228,6 +230,592 @@ def fake_clone(repo_url, target_path, **kwargs): downloader.download_subdirectory_package(dep_a, target_a) downloader.download_subdirectory_package(dep_b, target_b) - # Key assertion: only 1 clone despite 2 subdir deps + # Key assertion: only 1 BARE clone despite 2 subdir deps + # (each consumer materializes its own working tree from the bare). assert clone_call_count["n"] == 1 cache.cleanup() + + +# --------------------------------------------------------------------------- +# #1126 fix: bare-cache + per-consumer materialization tests +# --------------------------------------------------------------------------- + + +def _make_bare_repo(path: Path) -> None: + """Create a real bare git repo at ``path`` with a single commit. + + Used by tests that need a real-shaped bare for materialize-from-bare + (mocking subprocess for those would defeat the purpose -- the test + is precisely that the local-shared clone semantics work end-to-end). + """ + import subprocess as sp + + work = path.parent / (path.name + "_work") + work.mkdir(parents=True) + sp.run(["git", "init", "-b", "main", str(work)], check=True, capture_output=True) + sp.run( + ["git", "-C", str(work), "config", "user.email", "t@t.t"], + check=True, + capture_output=True, + ) + sp.run( + ["git", "-C", str(work), "config", "user.name", "t"], + check=True, + capture_output=True, + ) + (work / "skills").mkdir() + (work / "skills" / "X").mkdir() + (work / "skills" / "X" / "apm.yml").write_bytes(b"name: X\nversion: 1.0.0\n") + (work / "agents").mkdir() + (work / "agents" / "Y").mkdir() + (work / "agents" / "Y" / "apm.yml").write_bytes(b"name: Y\nversion: 1.0.0\n") + sp.run(["git", "-C", str(work), "add", "-A"], check=True, capture_output=True) + sp.run( + ["git", "-C", str(work), "commit", "-m", "init"], + check=True, + capture_output=True, + ) + sp.run( + ["git", "clone", "--bare", str(work), str(path)], + check=True, + capture_output=True, + ) + + +class TestBareCacheRaceCondition: + """6.1: regression test for the parallel sparse-checkout race (#1126).""" + + def test_parallel_different_subdirs_both_succeed(self, tmp_path: Path) -> None: + """Two threads request same key, then extract different subdirs from + the shared bare. Both must succeed (the v1 race lost one thread's + files because the cache materialized one subdir at the cache layer). + """ + cache = SharedCloneCache(base_dir=tmp_path) + bare_src = tmp_path / "bare_src" + _make_bare_repo(bare_src) + + def populate_bare(target: Path) -> None: + # Cache lock serializes this; only one thread enters. + import shutil + + shutil.copytree(bare_src, target) + + # Barrier forces both threads to do their materialize step in + # parallel (after one has populated the bare and both have + # received the same path back from the cache). + materialize_barrier = threading.Barrier(2) + results: dict[str, list] = {"errors": [], "subdirs_seen": []} + + def thread_a() -> None: + try: + bare = cache.get_or_clone("h", "o", "r", "main", populate_bare) + # Force parallel materialize step. + materialize_barrier.wait(timeout=5) + consumer = tmp_path / "consumer_a" + import subprocess as sp + + sp.run( + [ + "git", + "clone", + "--local", + "--shared", + "--no-checkout", + str(bare), + str(consumer), + ], + check=True, + capture_output=True, + ) + sp.run( + ["git", "-C", str(consumer), "checkout", "HEAD"], + check=True, + capture_output=True, + ) + if (consumer / "skills" / "X" / "apm.yml").exists(): + results["subdirs_seen"].append("X") + except Exception as e: + results["errors"].append(("a", e)) + + def thread_b() -> None: + try: + bare = cache.get_or_clone("h", "o", "r", "main", populate_bare) + materialize_barrier.wait(timeout=5) + consumer = tmp_path / "consumer_b" + import subprocess as sp + + sp.run( + [ + "git", + "clone", + "--local", + "--shared", + "--no-checkout", + str(bare), + str(consumer), + ], + check=True, + capture_output=True, + ) + sp.run( + ["git", "-C", str(consumer), "checkout", "HEAD"], + check=True, + capture_output=True, + ) + if (consumer / "agents" / "Y" / "apm.yml").exists(): + results["subdirs_seen"].append("Y") + except Exception as e: + results["errors"].append(("b", e)) + + ta = threading.Thread(target=thread_a) + tb = threading.Thread(target=thread_b) + ta.start() + tb.start() + ta.join(timeout=10) + tb.join(timeout=10) + + assert results["errors"] == [], f"Errors: {results['errors']}" + assert "X" in results["subdirs_seen"] + assert "Y" in results["subdirs_seen"] + cache.cleanup() + + +class TestBareCloneFallback: + """Tests for _bare_clone_with_fallback (6.4, 6.12, 6.18).""" + + def _make_downloader(self, tmp_path: Path): + """Build a minimal downloader with the auth/transport plumbing + sufficient for _bare_clone_with_fallback's _execute_transport_plan + path to run synchronously through one attempt. + """ + from apm_cli.deps.github_downloader import GitHubPackageDownloader + + d = GitHubPackageDownloader.__new__(GitHubPackageDownloader) + d.auth_resolver = MagicMock() + d.token_manager = MagicMock() + d._transport_selector = MagicMock() + d._protocol_pref = MagicMock() + d._allow_fallback = False + d._fallback_port_warned = set() + d._strategies = MagicMock() + d.git_env = {} + + # Stub the helpers the template uses. + d._build_repo_url = MagicMock(return_value="https://example/o/r") + d._resolve_dep_token = MagicMock(return_value="") + d._resolve_dep_auth_ctx = MagicMock(return_value=None) + d._sanitize_git_error = MagicMock(side_effect=lambda s: s) + + # Single-attempt plan: one HTTPS no-token attempt. + from apm_cli.deps.transport_selection import TransportAttempt, TransportPlan + + attempt = TransportAttempt( + scheme="https", + label="https", + use_token=False, + ) + plan = TransportPlan( + attempts=[attempt], + strict=False, + ) + d._transport_selector.select = MagicMock(return_value=plan) + return d + + def test_sha_ref_tier1_init_fetch_path(self, tmp_path: Path) -> None: + """6.4 + 6.18: SHA ref triggers init+fetch tier 1 with update-ref HEAD.""" + from apm_cli.models.dependency.reference import DependencyReference + + d = self._make_downloader(tmp_path) + dep = DependencyReference.parse("o/r/skills/X#abc1234567890abcdef1234567890abcdef12345678") + bare = tmp_path / "bare" + captured: list[list[str]] = [] + + def fake_run(args, **kwargs): + captured.append(list(args)) + # Tier-1 happy path: every call succeeds. + return MagicMock(returncode=0, stdout="", stderr="") + + with patch("apm_cli.deps.github_downloader.subprocess.run", side_effect=fake_run): + d._bare_clone_with_fallback( + "https://example/o/r", + bare, + dep_ref=dep, + ref="abc1234567890abcdef1234567890abcdef12345678", + is_commit_sha=True, + ) + + # Verify tier-1 sequence + cmd_strings = [" ".join(c) for c in captured] + assert any("init --bare" in s for s in cmd_strings), "missing init --bare" + assert any("remote add origin" in s for s in cmd_strings), "missing remote add" + assert any("fetch --depth=1" in s for s in cmd_strings), "missing fetch --depth=1" + # 6.18: update-ref HEAD MUST be called + update_ref_calls = [ + c for c in captured if len(c) >= 4 and c[-3] == "update-ref" and c[-2] == "HEAD" + ] + assert len(update_ref_calls) == 1, ( + f"expected 1 update-ref HEAD call, got {update_ref_calls}" + ) + assert update_ref_calls[0][-1] == "abc1234567890abcdef1234567890abcdef12345678" + # 6.19: token scrub via remote set-url origin redacted:// + assert any("remote set-url origin redacted://" in s for s in cmd_strings), ( + "missing token scrub" + ) + + def test_sha_ref_tier2_fallback_on_fetch_rejection(self, tmp_path: Path) -> None: + """6.12: tier-1 fetch fails (server rejects SHA fetch) -> tier-2 full clone.""" + import subprocess as sp + + from apm_cli.models.dependency.reference import DependencyReference + + d = self._make_downloader(tmp_path) + dep = DependencyReference.parse("o/r/skills/X#abc1234567890abcdef1234567890abcdef12345678") + bare = tmp_path / "bare" + captured: list[list[str]] = [] + + def fake_run(args, **kwargs): + captured.append(list(args)) + cmd_str = " ".join(args) + if "fetch --depth=1" in cmd_str: + # Tier-1 fetch fails (simulating allowReachableSHA1InWant=false) + raise sp.CalledProcessError(1, args, stderr=b"reject") + return MagicMock(returncode=0, stdout="", stderr="") + + with patch("apm_cli.deps.github_downloader.subprocess.run", side_effect=fake_run): + d._bare_clone_with_fallback( + "https://example/o/r", + bare, + dep_ref=dep, + ref="abc1234567890abcdef1234567890abcdef12345678", + is_commit_sha=True, + ) + + cmd_strings = [" ".join(c) for c in captured] + # Tier 2: full clone --bare invoked after tier-1 failed + assert any( + "clone --bare" in s and "--depth=1" not in s and "--branch" not in s + for s in cmd_strings + ), f"missing tier-2 full bare clone: {cmd_strings}" + # rev-parse --verify validates the SHA + assert any("rev-parse --verify" in s and "^{commit}" in s for s in cmd_strings), ( + "missing tier-2 SHA verify" + ) + # update-ref HEAD still set on tier 2 + update_ref_calls = [ + c for c in captured if len(c) >= 4 and c[-3] == "update-ref" and c[-2] == "HEAD" + ] + assert len(update_ref_calls) == 1 + assert update_ref_calls[0][-1] == "abc1234567890abcdef1234567890abcdef12345678" + + def test_symbolic_ref_tier1_shallow_clone(self, tmp_path: Path) -> None: + """Symbolic ref triggers tier-1 shallow clone with --branch.""" + from apm_cli.models.dependency.reference import DependencyReference + + d = self._make_downloader(tmp_path) + dep = DependencyReference.parse("o/r/skills/X#main") + bare = tmp_path / "bare" + captured: list[list[str]] = [] + + def fake_run(args, **kwargs): + captured.append(list(args)) + return MagicMock(returncode=0, stdout="", stderr="") + + with patch("apm_cli.deps.github_downloader.subprocess.run", side_effect=fake_run): + d._bare_clone_with_fallback( + "https://example/o/r", + bare, + dep_ref=dep, + ref="main", + is_commit_sha=False, + ) + + cmd_strings = [" ".join(c) for c in captured] + assert any("clone --bare --depth=1 --branch main" in s for s in cmd_strings), ( + f"missing tier-1 shallow clone: {cmd_strings}" + ) + + +class TestMaterializeFromBare: + """Tests for _materialize_from_bare (6.10, 6.11, 6.16).""" + + def _make_downloader(self): + from apm_cli.deps.github_downloader import GitHubPackageDownloader + + d = GitHubPackageDownloader.__new__(GitHubPackageDownloader) + d.git_env = {} + return d + + def test_materialize_from_real_bare(self, tmp_path: Path) -> None: + """End-to-end: real bare repo -> materialized consumer dir with content.""" + d = self._make_downloader() + bare = tmp_path / "bare" + _make_bare_repo(bare) + consumer = tmp_path / "consumer" + + sha = d._materialize_from_bare(bare, consumer, ref=None, env={}) + + assert (consumer / "skills" / "X" / "apm.yml").exists() + assert (consumer / "agents" / "Y" / "apm.yml").exists() + assert len(sha) == 40 and all(c in "0123456789abcdef" for c in sha) + + def test_consumer_resolved_sha_obtained_from_bare_not_consumer(self, tmp_path: Path) -> None: + """6.11: rev-parse HEAD MUST target --git-dir= (not consumer). + + Consumer rev-parse opens a Repo handle that leaks on Windows and + blocks downstream rmtree (lifetime invariant 5.2.1). + """ + d = self._make_downloader() + bare = tmp_path / "bare" + consumer = tmp_path / "consumer" + captured: list[list[str]] = [] + + def fake_run(args, **kwargs): + captured.append(list(args)) + if "rev-parse" in args: + return MagicMock(returncode=0, stdout="abc123\n", stderr="") + return MagicMock(returncode=0, stdout="", stderr="") + + with patch("apm_cli.deps.github_downloader.subprocess.run", side_effect=fake_run): + d._materialize_from_bare(bare, consumer, ref=None, env={}) + + rev_parse_calls = [c for c in captured if "rev-parse" in c] + assert len(rev_parse_calls) == 1 + # rev-parse MUST be against --git-dir , not against consumer + rp = rev_parse_calls[0] + assert "--git-dir" in rp + gd_idx = rp.index("--git-dir") + assert rp[gd_idx + 1] == str(bare), f"rev-parse must target bare, not consumer: {rp}" + + def test_known_sha_shortcut_avoids_rev_parse(self, tmp_path: Path) -> None: + """When known_sha is provided, skip rev-parse entirely (avoids the + ambiguity of init+fetch bares before update-ref runs).""" + d = self._make_downloader() + bare = tmp_path / "bare" + consumer = tmp_path / "consumer" + captured: list[list[str]] = [] + + def fake_run(args, **kwargs): + captured.append(list(args)) + return MagicMock(returncode=0, stdout="", stderr="") + + with patch("apm_cli.deps.github_downloader.subprocess.run", side_effect=fake_run): + sha = d._materialize_from_bare( + bare, consumer, ref=None, env={}, known_sha="deadbeef" * 5 + ) + + assert sha == "deadbeef" * 5 + rev_parse_calls = [c for c in captured if "rev-parse" in c] + assert rev_parse_calls == [], "known_sha must skip rev-parse" + + def test_materialize_disables_lfs_smudge(self, tmp_path: Path) -> None: + """6.16: materialize MUST set filter.lfs.smudge="" to skip LFS network.""" + d = self._make_downloader() + bare = tmp_path / "bare" + _make_bare_repo(bare) + consumer = tmp_path / "consumer" + + d._materialize_from_bare(bare, consumer, ref=None, env={}) + + # Read consumer's .git/config and verify LFS smudge is disabled + config_text = (consumer / ".git" / "config").read_text() + assert "smudge =" in config_text or "smudge=" in config_text + # The empty-string smudge value means LFS pointers stay as pointers + # (cross-platform; works on Windows where `cat` is unavailable) + assert "required = false" in config_text or "required=false" in config_text + + def test_materialize_pins_autocrlf_false(self, tmp_path: Path) -> None: + """6.10: core.autocrlf=false ensures byte-identical content across users.""" + d = self._make_downloader() + bare = tmp_path / "bare" + _make_bare_repo(bare) + consumer = tmp_path / "consumer" + + d._materialize_from_bare(bare, consumer, ref=None, env={}) + + config_text = (consumer / ".git" / "config").read_text() + assert "autocrlf = false" in config_text or "autocrlf=false" in config_text + + +class TestSharedCloneCacheBareInvariant: + """6.16: cache enforces bare-shape invariant in debug mode.""" + + def test_apm_debug_rejects_non_bare_clone(self, tmp_path: Path, monkeypatch) -> None: + """If clone_fn produces a working-tree-shaped dir under APM_DEBUG=1, + the cache must raise (canary against v1 regression).""" + monkeypatch.setenv("APM_DEBUG", "1") + cache = SharedCloneCache(base_dir=tmp_path) + + def bad_populate(target: Path) -> None: + # Working-tree shape: nested .git/, no HEAD at root + target.mkdir(parents=True) + (target / ".git").mkdir() + + with pytest.raises(RuntimeError, match="not a bare repo"): + cache.get_or_clone("h", "o", "r", "main", bad_populate) + cache.cleanup() + + def test_apm_debug_accepts_bare_clone(self, tmp_path: Path, monkeypatch) -> None: + monkeypatch.setenv("APM_DEBUG", "1") + cache = SharedCloneCache(base_dir=tmp_path) + + def good_populate(target: Path) -> None: + target.mkdir(parents=True) + (target / "HEAD").write_text("ref: refs/heads/main\n") + + path = cache.get_or_clone("h", "o", "r", "main", good_populate) + assert (path / "HEAD").is_file() + cache.cleanup() + + +class TestExecuteTransportPlanWtAction: + """6.13: regression-guard the new rmtree-before-attempt behavior in _wt_action. + + The refactor adds shutil.rmtree(target, ignore_errors=True) before + each attempt. The 8 existing _clone_with_fallback callsites depended + on the old behavior (no pre-rmtree); verify the new behavior is + benign for empty/missing targets and correctly cleans stale state + between attempts. + """ + + def test_wt_action_handles_missing_target(self, tmp_path: Path) -> None: + """Pre-attempt rmtree must not raise on missing target.""" + from apm_cli.deps.github_downloader import GitHubPackageDownloader + from apm_cli.models.dependency.reference import DependencyReference + + d = GitHubPackageDownloader.__new__(GitHubPackageDownloader) + d.auth_resolver = MagicMock() + d.token_manager = MagicMock() + d._transport_selector = MagicMock() + d._protocol_pref = MagicMock() + d._allow_fallback = False + d._fallback_port_warned = set() + d._strategies = MagicMock() + d.git_env = {} + d._build_repo_url = MagicMock(return_value="https://example/o/r") + d._resolve_dep_token = MagicMock(return_value="") + d._resolve_dep_auth_ctx = MagicMock(return_value=None) + d._sanitize_git_error = MagicMock(side_effect=lambda s: s) + + from apm_cli.deps.transport_selection import TransportAttempt, TransportPlan + + plan = TransportPlan( + attempts=[TransportAttempt(scheme="https", use_token=False, label="https")], + strict=False, + ) + d._transport_selector.select = MagicMock(return_value=plan) + dep = DependencyReference.parse("o/r#main") + + # Target does not exist -- _wt_action must handle gracefully. + target = tmp_path / "does_not_exist" + with patch("apm_cli.deps.github_downloader.Repo") as mock_repo: + mock_repo.clone_from = MagicMock() + d._clone_with_fallback("https://example/o/r", target, dep_ref=dep) + + mock_repo.clone_from.assert_called_once() + + +class TestBareCloneRetryRmtree: + """6.15: bare clone re-attempts must wipe target between attempts. + + Specifically: when _execute_transport_plan re-invokes _bare_action + on retry (e.g. ADO bearer retry), the prior attempt's partial bare + state (init+fetch) must be removed before re-init, otherwise + `git init --bare` would fail or leak state. + """ + + def test_bare_action_rmtrees_target_before_init(self, tmp_path: Path) -> None: + """_bare_action wipes existing target via shutil.rmtree pre-init.""" + from apm_cli.deps.github_downloader import GitHubPackageDownloader + from apm_cli.deps.transport_selection import TransportAttempt, TransportPlan + from apm_cli.models.dependency.reference import DependencyReference + + d = GitHubPackageDownloader.__new__(GitHubPackageDownloader) + d.auth_resolver = MagicMock() + d.token_manager = MagicMock() + d._transport_selector = MagicMock() + d._protocol_pref = MagicMock() + d._allow_fallback = False + d._fallback_port_warned = set() + d._strategies = MagicMock() + d.git_env = {} + d._build_repo_url = MagicMock(return_value="https://example/o/r") + d._resolve_dep_token = MagicMock(return_value="") + d._resolve_dep_auth_ctx = MagicMock(return_value=None) + d._sanitize_git_error = MagicMock(side_effect=lambda s: s) + + plan = TransportPlan( + attempts=[TransportAttempt(scheme="https", use_token=False, label="https")], + strict=False, + ) + d._transport_selector.select = MagicMock(return_value=plan) + dep = DependencyReference.parse("o/r/skills/X#abc1234567890abcdef1234567890abcdef12345678") + + # Pre-create the target with stale content; verify it gets wiped. + bare = tmp_path / "bare" + bare.mkdir() + (bare / "stale_file").write_text("from previous failed attempt") + + captured: list[list[str]] = [] + + def fake_run(args, **kwargs): + captured.append(list(args)) + # init must see clean target + if args[1] == "init" and args[2] == "--bare": + assert not (bare / "stale_file").exists(), "rmtree did not run before init" + return MagicMock(returncode=0, stdout="", stderr="") + + with patch("apm_cli.deps.github_downloader.subprocess.run", side_effect=fake_run): + d._bare_clone_with_fallback( + "https://example/o/r", + bare, + dep_ref=dep, + ref="abc1234567890abcdef1234567890abcdef12345678", + is_commit_sha=True, + ) + + +class TestInvalidSubdirErrorWording: + """6.14: typo'd subdir still surfaces 'Subdirectory ... not found'. + + Regression-trap for the user-facing typo-detection promise. The WS2 + bare-cache path materializes a FULL working tree (unlike the v1 + sparse checkout that only had the requested subdir), so a future + refactor could accidentally swallow the explicit subdir-existence + check at the consumer level. This test ensures the typo case still + raises with the subdir name in the message. + """ + + def test_typo_subdir_raises_subdirectory_not_found(self, tmp_path: Path) -> None: + from apm_cli.deps.github_downloader import GitHubPackageDownloader + from apm_cli.models.dependency.reference import DependencyReference + + # Real bare repo containing only "skills/X" and "agents/Y". + bare_src = tmp_path / "bare_src" + _make_bare_repo(bare_src) + + downloader = GitHubPackageDownloader() + + # Stub _bare_clone_with_fallback to copy our pre-built bare into + # the cache target dir (avoids real network). + def fake_bare_clone(url, target, *, dep_ref, ref, is_commit_sha): + import shutil as _sh + + if target.exists(): + _sh.rmtree(target) + _sh.copytree(bare_src, target) + + with SharedCloneCache(base_dir=tmp_path / "cache") as cache: + (tmp_path / "cache").mkdir() + downloader.shared_clone_cache = cache + + dep = DependencyReference.parse( + "github/awesome-copilot/skills/DOES_NOT_EXIST_TYPO#main" + ) + with patch.object(downloader, "_bare_clone_with_fallback", side_effect=fake_bare_clone): + target_out = tmp_path / "out" + target_out.parent.mkdir(parents=True, exist_ok=True) + with pytest.raises( + Exception, + match=r"Subdirectory ['\"]?skills/DOES_NOT_EXIST_TYPO['\"]? not found", + ): + downloader.download_subdirectory_package(dep, target_out) From d5f51012cb3664fe710c94bb100fb09ba3cca654 Mon Sep 17 00:00:00 2001 From: Daniel Meppiel Date: Mon, 4 May 2026 18:49:26 +0200 Subject: [PATCH 2/5] fix(deps): address panel follow-ups (CHANGELOG entry + user-facing error wording) Two highest-signal in-PR follow-ups from apm-review-panel on PR #1135: 1. Add CHANGELOG.md '### Fixed' entry under [Unreleased] for #1126. Flagged independently by doc-writer and oss-growth-hacker; required by .github/instructions/changelog.instructions.md. 2. Reword RuntimeError raised by _materialize_from_bare from 'Failed to materialize working tree from shared bare: {e}' to 'Failed to prepare dependency from cached clone: {e}' to remove internal-jargon ('materialize', 'working tree', 'shared bare') from a user-facing error message (devx-ux-expert finding). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- CHANGELOG.md | 4 ++++ src/apm_cli/deps/github_downloader.py | 2 +- 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index c5b99633..d4ae1b3b 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 + +- **Parallel subdir install race.** `apm install` no longer intermittently fails with `RuntimeError: Subdirectory '' not found in repository` when multiple dependencies 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) + ## [0.12.1] - 2026-05-03 ### Added diff --git a/src/apm_cli/deps/github_downloader.py b/src/apm_cli/deps/github_downloader.py index ac70ea92..632bd4a8 100644 --- a/src/apm_cli/deps/github_downloader.py +++ b/src/apm_cli/deps/github_downloader.py @@ -2091,7 +2091,7 @@ def _shared_bare_clone_fn(bare_target: Path) -> None: ) except Exception as e: raise RuntimeError( - f"Failed to materialize working tree from shared bare: {e}" + f"Failed to prepare dependency from cached clone: {e}" ) from e else: # Legacy per-dep clone path (no shared cache). From 5f2c8dfe38f608427ba0388efdfc1aca97ab20a0 Mon Sep 17 00:00:00 2001 From: Daniel Meppiel Date: Mon, 4 May 2026 19:19:56 +0200 Subject: [PATCH 3/5] refactor(deps): extract bare-cache helpers to bare_cache module to clear file-length guardrail Splits github_downloader.py (was 2751 lines, exceeded 2400-line CI guardrail) by moving bare-cache helpers and the clone-failure error-message builder into a new src/apm_cli/deps/bare_cache.py module: - _scrub_bare_remote_url - bare_clone_with_fallback - materialize_from_bare - clone_with_fallback (parameterized via repo_cls for test patchability) - build_clone_failure_message GitHubPackageDownloader retains thin delegating instance methods (_clone_with_fallback, _bare_clone_with_fallback, _materialize_from_bare) so existing patch.object call sites continue to work unchanged. Existing tests that patch 'apm_cli.deps.github_downloader.Repo' continue to function because the thin wrappers resolve Repo from the github_downloader module namespace at call time and pass it as repo_cls. github_downloader.py: 2751 -> 2383 lines. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- src/apm_cli/deps/bare_cache.py | 494 +++++++++++++++++++++ src/apm_cli/deps/github_downloader.py | 436 ++---------------- tests/unit/deps/test_shared_clone_cache.py | 12 +- 3 files changed, 534 insertions(+), 408 deletions(-) create mode 100644 src/apm_cli/deps/bare_cache.py diff --git a/src/apm_cli/deps/bare_cache.py b/src/apm_cli/deps/bare_cache.py new file mode 100644 index 00000000..e0fb9b75 --- /dev/null +++ b/src/apm_cli/deps/bare_cache.py @@ -0,0 +1,494 @@ +"""Bare-repo clone + materialization helpers for the WS2 dedup pipeline. + +Extracted from :mod:`github_downloader` to keep that module under the +2400-line CI guardrail (see ``.github/workflows/build-release.yml``). + +Public entry points: + +* :func:`clone_with_fallback` -- working-tree clone via + ``Repo.clone_from``, threaded through a caller-supplied transport-plan + executor. +* :func:`bare_clone_with_fallback` -- 3-tier bare-repo clone for the + shared cache (the core fix for #1126). +* :func:`materialize_from_bare` -- per-consumer working-tree checkout + backed by a shared bare's object database. + +All three are pure free functions; behavior is unchanged from the +original methods on :class:`GitHubPackageDownloader`. Test contracts +that patch ``downloader._clone_with_fallback`` / +``downloader._bare_clone_with_fallback`` / +``downloader._materialize_from_bare`` are preserved by keeping thin +delegating instance methods on the class. +""" + +from __future__ import annotations + +import logging +import shutil +import subprocess +from collections.abc import Callable +from pathlib import Path +from typing import TYPE_CHECKING, Any + +from git import Repo + +if TYPE_CHECKING: + from ..models.apm_package import DependencyReference + + +def _scrub_bare_remote_url(bare_path: Path, git_exe: str, env: dict[str, str]) -> None: + """Redact ``remote.origin.url`` in a bare repo's ``.git/config``. + + After a successful bare clone, ``remote.origin.url`` retains the + tokenized URL (e.g. ``https://oauth2:@github.com/...``). The + bare is read-only after this point in the WS2 dedup pipeline (no + further fetches), so the URL is dead weight that just persists the + token on disk. Replace with ``redacted://`` to eliminate the + on-disk token footprint. + + Best-effort: ``check=False`` so a config-set failure does not abort + the clone (the bare is still functionally correct without the + redaction). Convergent panel finding (auth + supply-chain MAJOR). + On exception, log at WARNING so token-leak-aware operators have an + audit trail (supply-chain reviewer follow-up: security mechanisms + must not fail silently). + """ + try: + result = subprocess.run( + [git_exe, "-C", str(bare_path), "remote", "set-url", "origin", "redacted://"], + env=env, + check=False, + capture_output=True, + timeout=10, + ) + if result.returncode != 0: + logging.getLogger(__name__).warning( + "Failed to redact remote URL from bare repo config at %s " + "(git exit=%d). Tokenized URL may persist on disk until " + "shared cache cleanup.", + bare_path, + result.returncode, + ) + except Exception as exc: + logging.getLogger(__name__).warning( + "Exception while redacting remote URL from bare repo config " + "at %s: %s. Tokenized URL may persist on disk until shared " + "cache cleanup.", + bare_path, + exc, + ) + + +def bare_clone_with_fallback( + execute_transport_plan: Callable[..., None], + repo_url_base: str, + bare_target: Path, + *, + dep_ref: DependencyReference, + ref: str | None, + is_commit_sha: bool, +) -> None: + """Clone a repository as a bare repo, with full transport-plan fallback. + + Sibling helper to :meth:`GitHubPackageDownloader._clone_with_fallback`. + Composes via the caller-supplied ``execute_transport_plan`` callable + (typically ``self._execute_transport_plan``) so it inherits ADO + bearer retry and protocol fallback automatically. The bare clone is + subdir-agnostic (no sparse cone), so a single bare can serve N + consumers each materializing a different subdir from the same + repo+ref - this is the core fix for #1126. + + 3-tier strategy (per design.md sec 5.2): + + - Tier 1 (SHA refs): ``git init --bare && git remote add + origin && git fetch --depth=1 origin ``. Requires + server-side ``uploadpack.allowReachableSHA1InWant=true`` + (default on github.com / GHES; some older GHE / ADO Server / + Bitbucket Server reject this). + - Tier 1 (symbolic / default branch): ``git clone --bare + --depth=1 [--branch ] ``. + - Tier 2 (both): full ``git clone --bare ``, validate via + ``git rev-parse --verify ^{commit}`` for SHA refs. + - Tier 3: re-raise. + + After every successful tier, ``git update-ref HEAD `` is + invoked for SHA refs so consumer ``git rev-parse HEAD`` resolves + unambiguously (the v3 BLOCKER fix). Then + :func:`_scrub_bare_remote_url` redacts the tokenized + ``remote.origin.url`` from ``.git/config`` to eliminate + on-disk token persistence (panel convergent finding). + + Note: bare-integrity verification (post-clone ``rev-parse HEAD + == known_sha``) is intentionally deferred. The bare is + ephemeral, mode-0700, and produced by a trusted (in-tree) + callable. If ``SharedCloneCache`` is ever opened to + plugin/user-supplied callables, restore the integrity check at + the cache boundary. See design.md sec 12 (Bare integrity + verification). + """ + from ..utils.git_env import get_git_executable + + git_exe = get_git_executable() + + def _bare_action(url: str, env: dict[str, str], target: Path) -> None: + # Pre-attempt cleanup: any prior tier-1 partial state must be + # wiped before re-attempting (e.g. on ADO bearer retry the + # template re-invokes _bare_action with a fresh URL/env, and + # the tier-1 init+fetch leaves a half-built bare on disk). + # See 6.15. + if target.exists(): + shutil.rmtree(target, ignore_errors=True) + + if is_commit_sha: + # Tier 1: init + fetch by SHA. + # Note: `git remote add origin ` stores the + # tokenized URL in `.git/config`. The ADO-bearer env + # approach relies on http.extraHeader (not stored URL + # auth), so this is safe today. _scrub_bare_remote_url + # below redacts the URL after a successful clone so the + # token does not persist on disk. + try: + subprocess.run( + [git_exe, "init", "--bare", str(target)], + env=env, + check=True, + capture_output=True, + ) + subprocess.run( + [git_exe, "-C", str(target), "remote", "add", "origin", url], + env=env, + check=True, + capture_output=True, + ) + subprocess.run( + [git_exe, "-C", str(target), "fetch", "--depth=1", "origin", ref], + env=env, + check=True, + capture_output=True, + timeout=300, + ) + # CRITICAL (v3 BLOCKER): init+fetch leaves HEAD pointing + # at refs/heads/main which doesn't exist locally. + # Without update-ref, consumer's `git rev-parse HEAD` + # is ambiguous. See 6.18. + subprocess.run( + [git_exe, "-C", str(target), "update-ref", "HEAD", ref], + env=env, + check=True, + capture_output=True, + ) + _scrub_bare_remote_url(target, git_exe, env) + return + except subprocess.CalledProcessError: + # Tier 2: full bare clone, validate SHA, set HEAD. + shutil.rmtree(target, ignore_errors=True) + subprocess.run( + [git_exe, "clone", "--bare", url, str(target)], + env=env, + check=True, + capture_output=True, + timeout=600, + ) + subprocess.run( + [ + git_exe, + "-C", + str(target), + "rev-parse", + "--verify", + f"{ref}^{{commit}}", + ], + env=env, + check=True, + capture_output=True, + ) + subprocess.run( + [git_exe, "-C", str(target), "update-ref", "HEAD", ref], + env=env, + check=True, + capture_output=True, + ) + _scrub_bare_remote_url(target, git_exe, env) + return + + # Symbolic ref or default branch. + args = [git_exe, "clone", "--bare", "--depth=1"] + if ref: + args += ["--branch", ref] + args += [url, str(target)] + try: + subprocess.run(args, env=env, check=True, capture_output=True, timeout=300) + _scrub_bare_remote_url(target, git_exe, env) + return + except subprocess.CalledProcessError: + # Tier 2: full bare clone (no shallow, no --branch). + shutil.rmtree(target, ignore_errors=True) + subprocess.run( + [git_exe, "clone", "--bare", url, str(target)], + env=env, + check=True, + capture_output=True, + timeout=600, + ) + _scrub_bare_remote_url(target, git_exe, env) + return + + execute_transport_plan( + repo_url_base, + bare_target, + dep_ref=dep_ref, + clone_action=_bare_action, + ) + + +def materialize_from_bare( + bare_path: Path, + consumer_dir: Path, + *, + ref: str | None, + env: dict[str, str], + known_sha: str | None = None, +) -> str: + """Create a working-tree checkout from a bare repo via local-shared clone. + + Mirrors :class:`GitCache`'s ``_create_checkout`` pattern: each + consumer gets its own working tree backed by the shared bare's + object database (via ``objects/info/alternates``). Hardlink-cheap + and concurrent-safe (consumer dirs are unique per call). + + SHA resolution policy (lifetime invariant 5.2.1): + - If ``known_sha`` is provided (caller passed a 40-char SHA + ref), use it directly. Avoids ``rev-parse HEAD`` which is + ambiguous on init+fetch bares before update-ref runs. + - Otherwise, resolve from the BARE via ``git --git-dir + rev-parse HEAD``. NOT from the consumer - opening + ``Repo(consumer_dir)`` leaks a Windows file handle that + blocks downstream rmtree. + + CRLF + LFS pinning before checkout: + - ``core.autocrlf=false`` guarantees byte-identical content + across consumers regardless of the user's global git config. + - ``filter.lfs.smudge=""`` + ``filter.lfs.required=false`` + disables LFS smudge cross-platform (the empty string trick + works everywhere; ``cat`` is not on Windows PATH). + + Returns: + The resolved commit SHA. Caller threads this into + ``resolved_commit`` for the lockfile. + """ + from ..utils.git_env import get_git_executable + + git_exe = get_git_executable() + + if known_sha: + resolved_sha = known_sha + else: + sha_result = subprocess.run( + [git_exe, "--git-dir", str(bare_path), "rev-parse", "HEAD"], + capture_output=True, + text=True, + timeout=10, + env=env, + check=True, + ) + resolved_sha = sha_result.stdout.strip() + + consumer_dir.parent.mkdir(parents=True, exist_ok=True) + # --no-checkout because we want to set core.autocrlf and disable + # LFS smudge BEFORE checkout writes any file content. + subprocess.run( + [ + git_exe, + "clone", + "--local", + "--shared", + "--no-checkout", + str(bare_path), + str(consumer_dir), + ], + capture_output=True, + text=True, + timeout=60, + env=env, + check=True, + ) + # CRLF determinism (panel: byte-identical across users). + subprocess.run( + [git_exe, "-C", str(consumer_dir), "config", "core.autocrlf", "false"], + capture_output=True, + text=True, + timeout=10, + env=env, + check=True, + ) + # Disable LFS smudge cross-platform: empty-string smudge is the + # portable equivalent of `git lfs smudge --skip`. The `cat` + # alternative is not on Windows PATH. + for key, val in ( + ("filter.lfs.smudge", ""), + ("filter.lfs.clean", ""), + ("filter.lfs.process", ""), + ("filter.lfs.required", "false"), + ): + subprocess.run( + [git_exe, "-C", str(consumer_dir), "config", key, val], + capture_output=True, + text=True, + timeout=10, + env=env, + check=False, + ) + subprocess.run( + [git_exe, "-C", str(consumer_dir), "checkout", "HEAD"], + capture_output=True, + text=True, + timeout=60, + env=env, + check=True, + ) + return resolved_sha + + +def clone_with_fallback( + execute_transport_plan: Callable[..., None], + repo_url_base: str, + target_path: Path, + *, + progress_reporter: Any = None, + dep_ref: DependencyReference | None = None, + verbose_callback: Callable[[str], None] | None = None, + repo_cls: Any = None, + **clone_kwargs: Any, +) -> Repo: + """Clone a working-tree repository following the TransportSelector plan. + + Thin adapter over the caller-supplied ``execute_transport_plan`` + callable (typically ``self._execute_transport_plan``) that supplies + a working-tree clone action (``Repo.clone_from``). Behavior is + unchanged from the pre-#1126 implementation, except every clone + attempt now begins with ``shutil.rmtree(target, ignore_errors=True)`` + for symmetry with the bare-clone path. This is strictly safer + (clean slate per attempt) and matches the existing behavior on + the second-and-subsequent attempts where target may contain a + partial clone from the failed first attempt. + + Returns: + The successfully cloned :class:`Repo`. + + Raises: + RuntimeError: If the planned attempt(s) all fail. + """ + repo_holder: list[Repo] = [] + _repo = repo_cls if repo_cls is not None else Repo + + def _wt_action(url: str, env: dict[str, str], target: Path) -> None: + # Pre-attempt cleanup: GitPython's Repo.clone_from refuses a + # non-empty target. Symmetric with _bare_action so retries + # always start from a clean slate. Behavior change from the + # pre-#1126 implementation - covered by 6.13. + if target.exists(): + shutil.rmtree(target, ignore_errors=True) + repo_holder.append( + _repo.clone_from( + url, + target, + env=env, + progress=progress_reporter, + **clone_kwargs, + ) + ) + + execute_transport_plan( + repo_url_base, + target_path, + dep_ref=dep_ref, + clone_action=_wt_action, + verbose_callback=verbose_callback, + ) + return repo_holder[0] + + +def build_clone_failure_message( + *, + repo_url_base: str, + plan: Any, + dep_ref: DependencyReference | None, + dep_host: str | None, + is_ado: bool, + is_generic: bool, + has_ado_token: bool, + has_token: bool, + auth_resolver: Any, + configured_github_host: str, + default_host_fn: Callable[[], str], + last_error: Exception | None, + sanitize_git_error: Callable[[str], str], +) -> str: + """Build the aggregate ``RuntimeError`` message for a failed transport plan. + + Extracted from :meth:`GitHubPackageDownloader._execute_transport_plan` + to keep that module under the file-length guardrail. Pure formatting: + no I/O, no clone attempts. + """ + if plan.strict and len(plan.attempts) >= 1: + tried = plan.attempts[0].label + error_msg = f"Failed to clone repository {repo_url_base} via {tried}. " + if plan.fallback_hint: + error_msg += plan.fallback_hint + " " + else: + error_msg = f"Failed to clone repository {repo_url_base} using all available methods. " + if is_ado and not has_ado_token: + host = dep_host or "dev.azure.com" + error_msg += auth_resolver.build_error_context( + host, + "clone", + org=dep_ref.ado_organization if dep_ref else None, + port=dep_ref.port if dep_ref else None, + dep_url=dep_ref.repo_url if dep_ref else None, + ) + elif is_generic: + if dep_host: + host_info = auth_resolver.classify_host( + dep_host, + port=dep_ref.port if dep_ref else None, + ) + host_name = host_info.display_name + else: + host_name = "the target host" + error_msg += ( + f"For private repositories on {host_name}, configure SSH keys or a git credential helper. " + f"APM delegates authentication to git for non-GitHub/ADO hosts." + ) + elif ( + configured_github_host + and dep_host + and dep_host == configured_github_host + and configured_github_host != "github.com" + ): + suggested = f"github.com/{repo_url_base}" + if dep_ref and dep_ref.virtual_path: + suggested += f"/{dep_ref.virtual_path}" + error_msg += ( + f"GITHUB_HOST is set to '{configured_github_host}', so shorthand dependencies " + f"(without a hostname) resolve against that host. " + f"If this package lives on a different server (e.g., github.com), " + f"use the full hostname in apm.yml: {suggested}" + ) + elif not has_token: + host = dep_host or default_host_fn() + org = dep_ref.repo_url.split("/")[0] if dep_ref and dep_ref.repo_url else None + error_msg += auth_resolver.build_error_context( + host, + "clone", + org=org, + port=dep_ref.port if dep_ref else None, + dep_url=dep_ref.repo_url if dep_ref else None, + ) + else: + error_msg += "Please check repository access permissions and authentication setup." + + if last_error: + sanitized_error = sanitize_git_error(str(last_error)) + error_msg += f" Last error: {sanitized_error}" + + return error_msg diff --git a/src/apm_cli/deps/github_downloader.py b/src/apm_cli/deps/github_downloader.py index 632bd4a8..e1d829ac 100644 --- a/src/apm_cli/deps/github_downloader.py +++ b/src/apm_cli/deps/github_downloader.py @@ -1,6 +1,5 @@ """GitHub package downloader for APM dependencies.""" -import logging import os import random # noqa: F401 import re @@ -46,6 +45,12 @@ sanitize_token_url_in_message, ) from ..utils.yaml_io import yaml_to_str +from .bare_cache import ( + bare_clone_with_fallback, + build_clone_failure_message, + clone_with_fallback, + materialize_from_bare, +) from .download_strategies import DownloadDelegate from .git_remote_ops import ( parse_ls_remote_output, @@ -103,49 +108,6 @@ def _rmtree(path) -> None: robust_rmtree(path, ignore_errors=True) -def _scrub_bare_remote_url(bare_path: Path, git_exe: str, env: dict[str, str]) -> None: - """Redact ``remote.origin.url`` in a bare repo's ``.git/config``. - - After a successful bare clone, ``remote.origin.url`` retains the - tokenized URL (e.g. ``https://oauth2:@github.com/...``). The - bare is read-only after this point in the WS2 dedup pipeline (no - further fetches), so the URL is dead weight that just persists the - token on disk. Replace with ``redacted://`` to eliminate the - on-disk token footprint. - - Best-effort: ``check=False`` so a config-set failure does not abort - the clone (the bare is still functionally correct without the - redaction). Convergent panel finding (auth + supply-chain MAJOR). - On exception, log at WARNING so token-leak-aware operators have an - audit trail (supply-chain reviewer follow-up: security mechanisms - must not fail silently). - """ - try: - result = subprocess.run( - [git_exe, "-C", str(bare_path), "remote", "set-url", "origin", "redacted://"], - env=env, - check=False, - capture_output=True, - timeout=10, - ) - if result.returncode != 0: - logging.getLogger(__name__).warning( - "Failed to redact remote URL from bare repo config at %s " - "(git exit=%d). Tokenized URL may persist on disk until " - "shared cache cleanup.", - bare_path, - result.returncode, - ) - except Exception as exc: - logging.getLogger(__name__).warning( - "Exception while redacting remote URL from bare repo config " - "at %s: %s. Tokenized URL may persist on disk until shared " - "cache cleanup.", - bare_path, - exc, - ) - - class GitProgressReporter(RemoteProgress): """Report git clone progress to Rich Progress.""" @@ -622,58 +584,17 @@ def _clone_with_fallback( verbose_callback=None, **clone_kwargs, ) -> Repo: - """Clone a working-tree repository following the TransportSelector plan. - - Thin adapter over :meth:`_execute_transport_plan` that supplies a - working-tree clone action (``Repo.clone_from``). Behavior is - unchanged from the pre-#1126 implementation, except every clone - attempt now begins with ``shutil.rmtree(target, ignore_errors=True)`` - for symmetry with the bare-clone path. This is strictly safer - (clean slate per attempt) and matches the existing behavior on - the second-and-subsequent attempts where target may contain a - partial clone from the failed first attempt. - - Args: - repo_url_base: Base repository reference (owner/repo) - target_path: Target path for cloning - progress_reporter: GitProgressReporter instance for progress updates - dep_ref: DependencyReference for platform/protocol decisions - verbose_callback: Optional callable for verbose logging (receives str messages) - **clone_kwargs: Additional arguments for Repo.clone_from - - Returns: - Repo: Successfully cloned repository - - Raises: - RuntimeError: If the planned attempt(s) all fail. - """ - repo_holder: list[Repo] = [] - - def _wt_action(url: str, env: dict[str, str], target: Path) -> None: - # Pre-attempt cleanup: GitPython's Repo.clone_from refuses a - # non-empty target. Symmetric with _bare_action so retries - # always start from a clean slate. Behavior change from the - # pre-#1126 implementation - covered by 6.13. - if target.exists(): - shutil.rmtree(target, ignore_errors=True) - repo_holder.append( - Repo.clone_from( - url, - target, - env=env, - progress=progress_reporter, - **clone_kwargs, - ) - ) - - self._execute_transport_plan( + """Thin delegate to :func:`bare_cache.clone_with_fallback` (kept on the class so test patches still work).""" + return clone_with_fallback( + self._execute_transport_plan, repo_url_base, target_path, + progress_reporter=progress_reporter, dep_ref=dep_ref, - clone_action=_wt_action, verbose_callback=verbose_callback, + repo_cls=Repo, + **clone_kwargs, ) - return repo_holder[0] def _execute_transport_plan( self, @@ -897,69 +818,21 @@ def _env_for(attempt: TransportAttempt) -> dict[str, str]: break # All planned attempts failed (or strict-mode single failure) - if plan.strict and len(plan.attempts) >= 1: - tried = plan.attempts[0].label - error_msg = f"Failed to clone repository {repo_url_base} via {tried}. " - if plan.fallback_hint: - error_msg += plan.fallback_hint + " " - else: - error_msg = f"Failed to clone repository {repo_url_base} using all available methods. " - configured_host = os.environ.get("GITHUB_HOST", "") - if is_ado and not self.has_ado_token: - host = dep_host or "dev.azure.com" - error_msg += self.auth_resolver.build_error_context( - host, - "clone", - org=dep_ref.ado_organization if dep_ref else None, - port=dep_ref.port if dep_ref else None, - dep_url=dep_ref.repo_url if dep_ref else None, - ) - elif is_generic: - if dep_host: - host_info = self.auth_resolver.classify_host( - dep_host, - port=dep_ref.port if dep_ref else None, - ) - host_name = host_info.display_name - else: - host_name = "the target host" - error_msg += ( - f"For private repositories on {host_name}, configure SSH keys or a git credential helper. " - f"APM delegates authentication to git for non-GitHub/ADO hosts." - ) - elif ( - configured_host - and dep_host - and dep_host == configured_host - and configured_host != "github.com" - ): - suggested = f"github.com/{repo_url_base}" - if dep_ref and dep_ref.virtual_path: - suggested += f"/{dep_ref.virtual_path}" - error_msg += ( - f"GITHUB_HOST is set to '{configured_host}', so shorthand dependencies " - f"(without a hostname) resolve against that host. " - f"If this package lives on a different server (e.g., github.com), " - f"use the full hostname in apm.yml: {suggested}" - ) - elif not has_token: - # No auth was resolved (neither env var nor credential helper). - # Guide the user through setting up authentication. - host = dep_host or default_host() - org = dep_ref.repo_url.split("/")[0] if dep_ref and dep_ref.repo_url else None - error_msg += self.auth_resolver.build_error_context( - host, - "clone", - org=org, - port=dep_ref.port if dep_ref else None, - dep_url=dep_ref.repo_url if dep_ref else None, - ) - else: - error_msg += "Please check repository access permissions and authentication setup." - - if last_error: - sanitized_error = self._sanitize_git_error(str(last_error)) - error_msg += f" Last error: {sanitized_error}" + error_msg = build_clone_failure_message( + repo_url_base=repo_url_base, + plan=plan, + dep_ref=dep_ref, + dep_host=dep_host, + is_ado=bool(is_ado), + is_generic=is_generic, + has_ado_token=self.has_ado_token, + has_token=has_token, + auth_resolver=self.auth_resolver, + configured_github_host=os.environ.get("GITHUB_HOST", ""), + default_host_fn=default_host, + last_error=last_error, + sanitize_git_error=self._sanitize_git_error, + ) raise RuntimeError(error_msg) @@ -976,155 +849,14 @@ def _bare_clone_with_fallback( ref: str | None, is_commit_sha: bool, ) -> None: - """Clone a repository as a bare repo, with full transport-plan fallback. - - Sibling helper to :meth:`_clone_with_fallback`. Composes via - :meth:`_execute_transport_plan` so it inherits ADO bearer retry - and protocol fallback automatically. The bare clone is - subdir-agnostic (no sparse cone), so a single bare can serve N - consumers each materializing a different subdir from the same - repo+ref - this is the core fix for #1126. - - 3-tier strategy (per design.md sec 5.2): - - - Tier 1 (SHA refs): ``git init --bare && git remote add - origin && git fetch --depth=1 origin ``. Requires - server-side ``uploadpack.allowReachableSHA1InWant=true`` - (default on github.com / GHES; some older GHE / ADO Server / - Bitbucket Server reject this). - - Tier 1 (symbolic / default branch): ``git clone --bare - --depth=1 [--branch ] ``. - - Tier 2 (both): full ``git clone --bare ``, validate via - ``git rev-parse --verify ^{commit}`` for SHA refs. - - Tier 3: re-raise. - - After every successful tier, ``git update-ref HEAD `` is - invoked for SHA refs so consumer ``git rev-parse HEAD`` resolves - unambiguously (the v3 BLOCKER fix). Then - :func:`_scrub_bare_remote_url` redacts the tokenized - ``remote.origin.url`` from ``.git/config`` to eliminate - on-disk token persistence (panel convergent finding). - - Note: bare-integrity verification (post-clone ``rev-parse HEAD - == known_sha``) is intentionally deferred. The bare is - ephemeral, mode-0700, and produced by a trusted (in-tree) - callable. If ``SharedCloneCache`` is ever opened to - plugin/user-supplied callables, restore the integrity check at - the cache boundary. See design.md sec 12 (Bare integrity - verification). - """ - from ..utils.git_env import get_git_executable - - git_exe = get_git_executable() - - def _bare_action(url: str, env: dict[str, str], target: Path) -> None: - # Pre-attempt cleanup: any prior tier-1 partial state must be - # wiped before re-attempting (e.g. on ADO bearer retry the - # template re-invokes _bare_action with a fresh URL/env, and - # the tier-1 init+fetch leaves a half-built bare on disk). - # See 6.15. - if target.exists(): - shutil.rmtree(target, ignore_errors=True) - - if is_commit_sha: - # Tier 1: init + fetch by SHA. - # Note: `git remote add origin ` stores the - # tokenized URL in `.git/config`. The ADO-bearer env - # approach relies on http.extraHeader (not stored URL - # auth), so this is safe today. _scrub_bare_remote_url - # below redacts the URL after a successful clone so the - # token does not persist on disk. - try: - subprocess.run( - [git_exe, "init", "--bare", str(target)], - env=env, - check=True, - capture_output=True, - ) - subprocess.run( - [git_exe, "-C", str(target), "remote", "add", "origin", url], - env=env, - check=True, - capture_output=True, - ) - subprocess.run( - [git_exe, "-C", str(target), "fetch", "--depth=1", "origin", ref], - env=env, - check=True, - capture_output=True, - timeout=300, - ) - # CRITICAL (v3 BLOCKER): init+fetch leaves HEAD pointing - # at refs/heads/main which doesn't exist locally. - # Without update-ref, consumer's `git rev-parse HEAD` - # is ambiguous. See 6.18. - subprocess.run( - [git_exe, "-C", str(target), "update-ref", "HEAD", ref], - env=env, - check=True, - capture_output=True, - ) - _scrub_bare_remote_url(target, git_exe, env) - return - except subprocess.CalledProcessError: - # Tier 2: full bare clone, validate SHA, set HEAD. - shutil.rmtree(target, ignore_errors=True) - subprocess.run( - [git_exe, "clone", "--bare", url, str(target)], - env=env, - check=True, - capture_output=True, - timeout=600, - ) - subprocess.run( - [ - git_exe, - "-C", - str(target), - "rev-parse", - "--verify", - f"{ref}^{{commit}}", - ], - env=env, - check=True, - capture_output=True, - ) - subprocess.run( - [git_exe, "-C", str(target), "update-ref", "HEAD", ref], - env=env, - check=True, - capture_output=True, - ) - _scrub_bare_remote_url(target, git_exe, env) - return - - # Symbolic ref or default branch. - args = [git_exe, "clone", "--bare", "--depth=1"] - if ref: - args += ["--branch", ref] - args += [url, str(target)] - try: - subprocess.run(args, env=env, check=True, capture_output=True, timeout=300) - _scrub_bare_remote_url(target, git_exe, env) - return - except subprocess.CalledProcessError: - # Tier 2: full bare clone (no shallow, no --branch). - shutil.rmtree(target, ignore_errors=True) - subprocess.run( - [git_exe, "clone", "--bare", url, str(target)], - env=env, - check=True, - capture_output=True, - timeout=600, - ) - _scrub_bare_remote_url(target, git_exe, env) - return - - self._execute_transport_plan( + """Thin delegate to :func:`bare_cache.bare_clone_with_fallback` (kept on the class so test patches still work).""" + bare_clone_with_fallback( + self._execute_transport_plan, repo_url_base, bare_target, dep_ref=dep_ref, - clone_action=_bare_action, + ref=ref, + is_commit_sha=is_commit_sha, ) def _materialize_from_bare( @@ -1136,108 +868,8 @@ def _materialize_from_bare( env: dict[str, str], known_sha: str | None = None, ) -> str: - """Create a working-tree checkout from a bare repo via local-shared clone. - - Mirrors :class:`GitCache`'s ``_create_checkout`` pattern: each - consumer gets its own working tree backed by the shared bare's - object database (via ``objects/info/alternates``). Hardlink-cheap - and concurrent-safe (consumer dirs are unique per call). - - SHA resolution policy (lifetime invariant 5.2.1): - - If ``known_sha`` is provided (caller passed a 40-char SHA - ref), use it directly. Avoids ``rev-parse HEAD`` which is - ambiguous on init+fetch bares before update-ref runs. - - Otherwise, resolve from the BARE via ``git --git-dir - rev-parse HEAD``. NOT from the consumer - opening - ``Repo(consumer_dir)`` leaks a Windows file handle that - blocks downstream rmtree. - - CRLF + LFS pinning before checkout: - - ``core.autocrlf=false`` guarantees byte-identical content - across consumers regardless of the user's global git config. - - ``filter.lfs.smudge=""`` + ``filter.lfs.required=false`` - disables LFS smudge cross-platform (the empty string trick - works everywhere; ``cat`` is not on Windows PATH). - - Returns: - The resolved commit SHA. Caller threads this into - ``resolved_commit`` for the lockfile. - """ - from ..utils.git_env import get_git_executable - - git_exe = get_git_executable() - - if known_sha: - resolved_sha = known_sha - else: - sha_result = subprocess.run( - [git_exe, "--git-dir", str(bare_path), "rev-parse", "HEAD"], - capture_output=True, - text=True, - timeout=10, - env=env, - check=True, - ) - resolved_sha = sha_result.stdout.strip() - - consumer_dir.parent.mkdir(parents=True, exist_ok=True) - # --no-checkout because we want to set core.autocrlf and disable - # LFS smudge BEFORE checkout writes any file content. - subprocess.run( - [ - git_exe, - "clone", - "--local", - "--shared", - "--no-checkout", - str(bare_path), - str(consumer_dir), - ], - capture_output=True, - text=True, - timeout=60, - env=env, - check=True, - ) - # CRLF determinism (panel: byte-identical across users). - subprocess.run( - [git_exe, "-C", str(consumer_dir), "config", "core.autocrlf", "false"], - capture_output=True, - text=True, - timeout=10, - env=env, - check=True, - ) - # Disable LFS smudge cross-platform: empty-string smudge is the - # portable equivalent of `git lfs smudge --skip`. The `cat` - # alternative is not on Windows PATH. - for key, val in ( - ("filter.lfs.smudge", ""), - ("filter.lfs.clean", ""), - ("filter.lfs.process", ""), - ("filter.lfs.required", "false"), - ): - subprocess.run( - [git_exe, "-C", str(consumer_dir), "config", key, val], - capture_output=True, - text=True, - timeout=10, - env=env, - check=False, - ) - subprocess.run( - [git_exe, "-C", str(consumer_dir), "checkout", "HEAD"], - capture_output=True, - text=True, - timeout=60, - env=env, - check=True, - ) - return resolved_sha - - # ------------------------------------------------------------------ - # Remote ref enumeration (no clone required) - # ------------------------------------------------------------------ + """Thin delegate to :func:`bare_cache.materialize_from_bare` (kept on the class so test patches still work).""" + return materialize_from_bare(bare_path, consumer_dir, ref=ref, env=env, known_sha=known_sha) @staticmethod def _parse_ls_remote_output(output: str) -> list[RemoteRef]: diff --git a/tests/unit/deps/test_shared_clone_cache.py b/tests/unit/deps/test_shared_clone_cache.py index 2c0c2dce..a2204fb5 100644 --- a/tests/unit/deps/test_shared_clone_cache.py +++ b/tests/unit/deps/test_shared_clone_cache.py @@ -435,7 +435,7 @@ def fake_run(args, **kwargs): # Tier-1 happy path: every call succeeds. return MagicMock(returncode=0, stdout="", stderr="") - with patch("apm_cli.deps.github_downloader.subprocess.run", side_effect=fake_run): + with patch("apm_cli.deps.bare_cache.subprocess.run", side_effect=fake_run): d._bare_clone_with_fallback( "https://example/o/r", bare, @@ -481,7 +481,7 @@ def fake_run(args, **kwargs): raise sp.CalledProcessError(1, args, stderr=b"reject") return MagicMock(returncode=0, stdout="", stderr="") - with patch("apm_cli.deps.github_downloader.subprocess.run", side_effect=fake_run): + with patch("apm_cli.deps.bare_cache.subprocess.run", side_effect=fake_run): d._bare_clone_with_fallback( "https://example/o/r", bare, @@ -520,7 +520,7 @@ def fake_run(args, **kwargs): captured.append(list(args)) return MagicMock(returncode=0, stdout="", stderr="") - with patch("apm_cli.deps.github_downloader.subprocess.run", side_effect=fake_run): + with patch("apm_cli.deps.bare_cache.subprocess.run", side_effect=fake_run): d._bare_clone_with_fallback( "https://example/o/r", bare, @@ -575,7 +575,7 @@ def fake_run(args, **kwargs): return MagicMock(returncode=0, stdout="abc123\n", stderr="") return MagicMock(returncode=0, stdout="", stderr="") - with patch("apm_cli.deps.github_downloader.subprocess.run", side_effect=fake_run): + with patch("apm_cli.deps.bare_cache.subprocess.run", side_effect=fake_run): d._materialize_from_bare(bare, consumer, ref=None, env={}) rev_parse_calls = [c for c in captured if "rev-parse" in c] @@ -598,7 +598,7 @@ def fake_run(args, **kwargs): captured.append(list(args)) return MagicMock(returncode=0, stdout="", stderr="") - with patch("apm_cli.deps.github_downloader.subprocess.run", side_effect=fake_run): + with patch("apm_cli.deps.bare_cache.subprocess.run", side_effect=fake_run): sha = d._materialize_from_bare( bare, consumer, ref=None, env={}, known_sha="deadbeef" * 5 ) @@ -764,7 +764,7 @@ def fake_run(args, **kwargs): assert not (bare / "stale_file").exists(), "rmtree did not run before init" return MagicMock(returncode=0, stdout="", stderr="") - with patch("apm_cli.deps.github_downloader.subprocess.run", side_effect=fake_run): + with patch("apm_cli.deps.bare_cache.subprocess.run", side_effect=fake_run): d._bare_clone_with_fallback( "https://example/o/r", bare, From f5e2fc6d5f57bf6ae645d05eb71edc52d952a079 Mon Sep 17 00:00:00 2001 From: Daniel Meppiel Date: Mon, 4 May 2026 19:40:57 +0200 Subject: [PATCH 4/5] fix(deps): address Copilot review + Windows portability + supply-chain hardening (#1135) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * Short-SHA correctness (Copilot review): bare-cache tier 1 (init+fetch by SHA) requires a full 40-char SHA — git's smart-HTTP cannot resolve abbreviations and update-ref refuses partial refs. Skip tier 1 for short SHAs and resolve them via tier-2 'rev-parse --verify ^{commit}', feeding the canonical 40-char SHA into update-ref HEAD. Also gate the consumer-side 'known_sha=ref' optimization on len==40. * Windows portability (Copilot review): replace 9 raw shutil.rmtree call sites in bare_cache.py and github_downloader.py with the local _rmtree helper that delegates to robust_rmtree (chmod-retries read-only .git/objects/pack/* files). Adds a duplicate _rmtree to bare_cache.py to avoid the github_downloader -> bare_cache circular import. * FETCH_HEAD supply-chain scrub (panel follow-up): tier-1 init+fetch leaves the tokenized URL inside .git/FETCH_HEAD even after the remote.origin.url config redaction. Truncate FETCH_HEAD to empty in _scrub_bare_remote_url so the token does not survive on disk. * ADO bearer retry on bare 401 (panel follow-up): subprocess CalledProcessError str() does NOT include stderr, so the existing 401/Authentication-failed marker check in _execute_transport_plan never matched the bare-clone path. Append e.stderr to err_msg so bearer retry composes with bare clones identically to the GitPython-based working-tree path. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- src/apm_cli/deps/bare_cache.py | 201 ++++++++++++++++---------- src/apm_cli/deps/github_downloader.py | 38 +++-- 2 files changed, 153 insertions(+), 86 deletions(-) diff --git a/src/apm_cli/deps/bare_cache.py b/src/apm_cli/deps/bare_cache.py index e0fb9b75..7d6d176e 100644 --- a/src/apm_cli/deps/bare_cache.py +++ b/src/apm_cli/deps/bare_cache.py @@ -24,7 +24,6 @@ from __future__ import annotations import logging -import shutil import subprocess from collections.abc import Callable from pathlib import Path @@ -36,6 +35,20 @@ from ..models.apm_package import DependencyReference +def _rmtree(path: Path) -> None: + """Remove a directory tree, handling read-only files and brief Windows locks. + + Delegates to :func:`robust_rmtree` which retries with exponential backoff + on transient lock errors and chmod-resets read-only ``.git/objects/pack`` + files (Windows portability finding from the #1126 paper audit). + Duplicated from :mod:`github_downloader` to avoid a circular import + (``github_downloader`` imports from this module). + """ + from ..utils.file_ops import robust_rmtree + + robust_rmtree(path, ignore_errors=True) + + def _scrub_bare_remote_url(bare_path: Path, git_exe: str, env: dict[str, str]) -> None: """Redact ``remote.origin.url`` in a bare repo's ``.git/config``. @@ -46,6 +59,11 @@ def _scrub_bare_remote_url(bare_path: Path, git_exe: str, env: dict[str, str]) - token on disk. Replace with ``redacted://`` to eliminate the on-disk token footprint. + Defense-in-depth: tier-1 (init + remote add + fetch) leaves + ``FETCH_HEAD`` containing the tokenized URL on disk even after the + config scrub. Truncate it to empty so the token does not survive + in any on-disk artifact. Best-effort (non-fatal on OSError). + Best-effort: ``check=False`` so a config-set failure does not abort the clone (the bare is still functionally correct without the redaction). Convergent panel finding (auth + supply-chain MAJOR). @@ -78,6 +96,20 @@ def _scrub_bare_remote_url(bare_path: Path, git_exe: str, env: dict[str, str]) - exc, ) + # Defense-in-depth: truncate FETCH_HEAD which retains the tokenized + # URL after tier-1 init+fetch (supply-chain panel follow-up). + fetch_head = bare_path / "FETCH_HEAD" + try: + if fetch_head.exists(): + fetch_head.write_text("") + except OSError as exc: + logging.getLogger(__name__).warning( + "Failed to truncate FETCH_HEAD at %s: %s. Tokenized URL " + "may persist on disk until shared cache cleanup.", + fetch_head, + exc, + ) + def bare_clone_with_fallback( execute_transport_plan: Callable[..., None], @@ -137,79 +169,98 @@ def _bare_action(url: str, env: dict[str, str], target: Path) -> None: # the tier-1 init+fetch leaves a half-built bare on disk). # See 6.15. if target.exists(): - shutil.rmtree(target, ignore_errors=True) + _rmtree(target) if is_commit_sha: - # Tier 1: init + fetch by SHA. - # Note: `git remote add origin ` stores the - # tokenized URL in `.git/config`. The ADO-bearer env - # approach relies on http.extraHeader (not stored URL - # auth), so this is safe today. _scrub_bare_remote_url - # below redacts the URL after a successful clone so the - # token does not persist on disk. - try: - subprocess.run( - [git_exe, "init", "--bare", str(target)], - env=env, - check=True, - capture_output=True, - ) - subprocess.run( - [git_exe, "-C", str(target), "remote", "add", "origin", url], - env=env, - check=True, - capture_output=True, - ) - subprocess.run( - [git_exe, "-C", str(target), "fetch", "--depth=1", "origin", ref], - env=env, - check=True, - capture_output=True, - timeout=300, - ) - # CRITICAL (v3 BLOCKER): init+fetch leaves HEAD pointing - # at refs/heads/main which doesn't exist locally. - # Without update-ref, consumer's `git rev-parse HEAD` - # is ambiguous. See 6.18. - subprocess.run( - [git_exe, "-C", str(target), "update-ref", "HEAD", ref], - env=env, - check=True, - capture_output=True, - ) - _scrub_bare_remote_url(target, git_exe, env) - return - except subprocess.CalledProcessError: - # Tier 2: full bare clone, validate SHA, set HEAD. - shutil.rmtree(target, ignore_errors=True) - subprocess.run( - [git_exe, "clone", "--bare", url, str(target)], - env=env, - check=True, - capture_output=True, - timeout=600, - ) - subprocess.run( - [ - git_exe, - "-C", - str(target), - "rev-parse", - "--verify", - f"{ref}^{{commit}}", - ], - env=env, - check=True, - capture_output=True, - ) - subprocess.run( - [git_exe, "-C", str(target), "update-ref", "HEAD", ref], - env=env, - check=True, - capture_output=True, - ) - _scrub_bare_remote_url(target, git_exe, env) - return + # Tier 1 (init + fetch by SHA) requires a full 40-char SHA: + # `git fetch origin ` only works for full SHAs (the + # smart-HTTP protocol does not resolve abbreviated SHAs), + # and `git update-ref HEAD ` requires a full 40-char + # SHA-1. For short SHAs, skip directly to tier 2 where + # `rev-parse ` against the full bare can resolve + # the abbreviation. Copilot review finding (#1135). + if len(ref) == 40: + # Note: `git remote add origin ` stores the + # tokenized URL in `.git/config`. The ADO-bearer env + # approach relies on http.extraHeader (not stored URL + # auth), so this is safe today. _scrub_bare_remote_url + # below redacts the URL after a successful clone so the + # token does not persist on disk. + try: + subprocess.run( + [git_exe, "init", "--bare", str(target)], + env=env, + check=True, + capture_output=True, + ) + subprocess.run( + [git_exe, "-C", str(target), "remote", "add", "origin", url], + env=env, + check=True, + capture_output=True, + ) + subprocess.run( + [git_exe, "-C", str(target), "fetch", "--depth=1", "origin", ref], + env=env, + check=True, + capture_output=True, + timeout=300, + ) + # CRITICAL (v3 BLOCKER): init+fetch leaves HEAD pointing + # at refs/heads/main which doesn't exist locally. + # Without update-ref, consumer's `git rev-parse HEAD` + # is ambiguous. See 6.18. + subprocess.run( + [git_exe, "-C", str(target), "update-ref", "HEAD", ref], + env=env, + check=True, + capture_output=True, + ) + _scrub_bare_remote_url(target, git_exe, env) + return + except subprocess.CalledProcessError: + pass # fall through to tier 2 + + # Tier 2: full bare clone, validate SHA, set HEAD. + if target.exists(): + _rmtree(target) + subprocess.run( + [git_exe, "clone", "--bare", url, str(target)], + env=env, + check=True, + capture_output=True, + timeout=600, + ) + # Resolve abbreviated SHAs (and re-validate full SHAs) against + # the full clone. `rev-parse ^{commit}` returns the + # 40-char SHA; we feed that into update-ref so HEAD never + # holds a partial reference. Without this, short-SHA pins + # would set resolved_commit to the abbreviation and lockfile + # comparisons against `head.commit.hexsha` (always 40-char) + # would never match. Copilot review finding (#1135). + full_sha_result = subprocess.run( + [ + git_exe, + "-C", + str(target), + "rev-parse", + "--verify", + f"{ref}^{{commit}}", + ], + env=env, + check=True, + capture_output=True, + text=True, + ) + full_sha = full_sha_result.stdout.strip() + subprocess.run( + [git_exe, "-C", str(target), "update-ref", "HEAD", full_sha], + env=env, + check=True, + capture_output=True, + ) + _scrub_bare_remote_url(target, git_exe, env) + return # Symbolic ref or default branch. args = [git_exe, "clone", "--bare", "--depth=1"] @@ -222,7 +273,7 @@ def _bare_action(url: str, env: dict[str, str], target: Path) -> None: return except subprocess.CalledProcessError: # Tier 2: full bare clone (no shallow, no --branch). - shutil.rmtree(target, ignore_errors=True) + _rmtree(target) subprocess.run( [git_exe, "clone", "--bare", url, str(target)], env=env, @@ -366,7 +417,7 @@ def clone_with_fallback( callable (typically ``self._execute_transport_plan``) that supplies a working-tree clone action (``Repo.clone_from``). Behavior is unchanged from the pre-#1126 implementation, except every clone - attempt now begins with ``shutil.rmtree(target, ignore_errors=True)`` + attempt now begins with a robust ``_rmtree`` of the target for symmetry with the bare-clone path. This is strictly safer (clean slate per attempt) and matches the existing behavior on the second-and-subsequent attempts where target may contain a @@ -387,7 +438,7 @@ def _wt_action(url: str, env: dict[str, str], target: Path) -> None: # always start from a clean slate. Behavior change from the # pre-#1126 implementation - covered by 6.13. if target.exists(): - shutil.rmtree(target, ignore_errors=True) + _rmtree(target) repo_holder.append( _repo.clone_from( url, diff --git a/src/apm_cli/deps/github_downloader.py b/src/apm_cli/deps/github_downloader.py index e1d829ac..d85df387 100644 --- a/src/apm_cli/deps/github_downloader.py +++ b/src/apm_cli/deps/github_downloader.py @@ -1,9 +1,9 @@ """GitHub package downloader for APM dependencies.""" +import contextlib import os import random # noqa: F401 import re -import shutil import stat # noqa: F401 import subprocess import sys @@ -755,11 +755,18 @@ def _env_for(attempt: TransportAttempt) -> dict[str, str]: return except (GitCommandError, subprocess.CalledProcessError) as e: # ADO bearer fallback for clone (mirrors validation/list_remote_refs): - # PAT was rejected -> silently retry this attempt with az-cli bearer. - # Note: subprocess errors may include tokenized URLs in stderr; - # the existing _sanitize_git_error / _sanitize_url path used in the - # final error message handles redaction at the boundary. + # PAT was rejected -> silently retry with az-cli bearer. + # Append e.stderr to err_msg because str(CalledProcessError) + # does NOT include stderr by default; without this, the bare + # clone path would never trigger bearer retry on PAT 401. err_msg = str(e) + stderr_attr = getattr(e, "stderr", None) + if stderr_attr: + if isinstance(stderr_attr, bytes): + with contextlib.suppress(Exception): + err_msg += " " + stderr_attr.decode("utf-8", errors="replace") + else: + err_msg += " " + str(stderr_attr) if ( is_ado and attempt.use_token @@ -1170,7 +1177,7 @@ def resolve_git_reference( finally: if temp_dir and temp_dir.exists(): - shutil.rmtree(temp_dir, ignore_errors=True) + _rmtree(temp_dir) return ResolvedReference( original_ref=original_ref_str, @@ -1719,7 +1726,16 @@ def _shared_bare_clone_fn(bare_target: Path) -> None: temp_clone_path, ref=ref, env=self._git_env_dict(), - known_sha=ref if is_commit_sha else None, + # Only short-circuit SHA resolution when the user + # pinned a full 40-char SHA. Abbreviated SHAs + # (7-39 chars) must be resolved to the full + # SHA against the bare so resolved_commit + # matches `head.commit.hexsha` (always 40-char) + # in lockfile comparisons. The bare's HEAD has + # already been update-ref'd to the full SHA in + # _bare_action, so rev-parse HEAD returns 40 chars. + # Copilot review finding (#1135). + known_sha=ref if (is_commit_sha and len(ref) == 40) else None, ) except Exception as e: raise RuntimeError( @@ -2036,7 +2052,7 @@ def _download_package_from_artifactory( ) except RuntimeError: if target_path.exists(): - shutil.rmtree(target_path, ignore_errors=True) + _rmtree(target_path) raise if progress_obj and progress_task_id is not None: progress_obj.update(progress_task_id, completed=70, total=100) @@ -2044,7 +2060,7 @@ def _download_package_from_artifactory( validation_result = validate_apm_package(target_path) if not validation_result.is_valid: if target_path.exists(): - shutil.rmtree(target_path, ignore_errors=True) + _rmtree(target_path) error_msg = f"Invalid APM package {dep_ref.repo_url}:\n" for error in validation_result.errors: error_msg += f" - {error}\n" @@ -2286,7 +2302,7 @@ def download_package( # Remove .git directory to save space and prevent treating as a Git repository git_dir = target_path / ".git" if git_dir.exists(): - shutil.rmtree(git_dir, ignore_errors=True) + _rmtree(git_dir) except GitCommandError as e: # Check if this might be a private repository access issue @@ -2316,7 +2332,7 @@ def download_package( if not validation_result.is_valid: # Clean up on validation failure if target_path.exists(): - shutil.rmtree(target_path, ignore_errors=True) + _rmtree(target_path) error_msg = f"Invalid APM package {dep_ref.repo_url}:\n" for error in validation_result.errors: From 1555757b0c877f2a3fe9e5438300483c10fb5fb8 Mon Sep 17 00:00:00 2001 From: Daniel Meppiel Date: Mon, 4 May 2026 19:41:24 +0200 Subject: [PATCH 5/5] test(deps): harden lockfile parity + add ADO bare retry; docs(deps): update bare+materialize pipeline (#1135) * Bare-cache short-SHA test: assert tier 1 (init+fetch) is skipped for abbreviated SHAs and the resolved 40-char SHA from rev-parse drives update-ref HEAD, not the user-supplied abbreviation. * Bare-cache tier-2 test: stub rev-parse --verify stdout with the full 40-char SHA so the test exercises the new 'use resolved SHA, not input ref' code path. Existing tier-1 fixture now uses a real 40-char hex SHA (the prior fixture was 43 chars, which is now correctly rejected by the len()==40 gate). * FETCH_HEAD scrub tests: assert _scrub_bare_remote_url truncates FETCH_HEAD when present (defense-in-depth supply-chain) and is a no-op when absent (tier-2 full-clone path). * ADO bearer 401 retry on bare clone: end-to-end test that PAT failure with 'Authentication failed' in stderr triggers bearer retry through the bare clone action and the stale-PAT diagnostic fires on success. * Integration test (subdir dedup E2E): - lockfile parity: assert pkg.resolved_reference.resolved_commit is a 40-char SHA and identical across both subdirs (was a soft getattr fallback that silently passed if attr was missing). - sha-https variant: pass GH_TOKEN env to the gh api subprocess so it works under CI workers without ambient gh auth login state. * Docs: update Parallel Downloads section to describe the WS2 bare + per-consumer materialize pipeline (replaces stale sparse-checkout copy). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- docs/src/content/docs/guides/dependencies.md | 2 +- .../test_install_subdir_dedup_e2e.py | 39 ++- tests/unit/deps/test_shared_clone_cache.py | 231 +++++++++++++++++- 3 files changed, 253 insertions(+), 19 deletions(-) diff --git a/docs/src/content/docs/guides/dependencies.md b/docs/src/content/docs/guides/dependencies.md index cb6a1109..5720cca2 100644 --- a/docs/src/content/docs/guides/dependencies.md +++ b/docs/src/content/docs/guides/dependencies.md @@ -671,7 +671,7 @@ APM automatically retries failed HTTP requests with exponential backoff and jitt #### Parallel Downloads -APM downloads packages in parallel using a thread pool, significantly reducing wall-clock time for large dependency trees. The concurrency level defaults to 4 and is configurable via `--parallel-downloads` (set to 0 to disable). For subdirectory packages in monorepos, APM attempts git sparse-checkout (git 2.25+) to download only the needed directory, falling back to a shallow clone if sparse-checkout is unavailable. +APM downloads packages in parallel using a thread pool, significantly reducing wall-clock time for large dependency trees. The concurrency level defaults to 4 and is configurable via `--parallel-downloads` (set to 0 to disable). For sibling subdirectory packages from the same monorepo and ref (e.g. two skills under `skills/` in `github/awesome-copilot`), APM clones the repo bare exactly once into a shared cache and materializes each consumer's working tree from that cache via `git clone --local --shared --no-checkout`. This eliminates redundant network fetches and prevents the parallel races that affected earlier sparse-checkout based fetches. ### File Processing and Content Merging diff --git a/tests/integration/test_install_subdir_dedup_e2e.py b/tests/integration/test_install_subdir_dedup_e2e.py index 6fce665a..2719e449 100644 --- a/tests/integration/test_install_subdir_dedup_e2e.py +++ b/tests/integration/test_install_subdir_dedup_e2e.py @@ -52,15 +52,27 @@ def _resolve_known_sha() -> str | None: - """Resolve a real commit SHA on github/awesome-copilot for the sha-https variant.""" + """Resolve a real commit SHA on github/awesome-copilot for the sha-https variant. + + Passes ``GH_TOKEN`` to the ``gh`` subprocess so the test does not depend + on the developer's ambient ``gh auth login`` state -- CI workers will have + ``GITHUB_APM_PAT`` (or ``GITHUB_TOKEN``) but no ``gh`` config (Copilot + review #1135). + """ import subprocess + token = os.getenv("GITHUB_TOKEN") or os.getenv("GITHUB_APM_PAT") + if not token: + return None + env = {**os.environ, "GH_TOKEN": token} + try: result = subprocess.run( ["gh", "api", "repos/github/awesome-copilot/commits/main", "--jq", ".sha"], capture_output=True, text=True, timeout=15, + env=env, ) if result.returncode == 0 and result.stdout.strip(): sha = result.stdout.strip() @@ -133,13 +145,22 @@ def test_two_subdirs_same_repo_parallel(ref_kind: str, ref_value: str | None) -> assert any(target_a.iterdir()), f"{SUBDIR_A} is empty" assert any(target_b.iterdir()), f"{SUBDIR_B} is empty" - # Lockfile parity: same cache key -> same resolved_commit. - sha_a = getattr(pkg_a, "resolved_commit", None) or getattr(pkg_a, "commit_sha", None) - sha_b = getattr(pkg_b, "resolved_commit", None) or getattr(pkg_b, "commit_sha", None) - if sha_a and sha_b and sha_a != "unknown" and sha_b != "unknown": - assert sha_a == sha_b, ( - f"Sibling subdirs from same repo+ref must resolve to " - f"same commit, got a={sha_a} b={sha_b}" - ) + # Lockfile parity (Copilot review #1135 + panel follow-up): + # The canonical resolved SHA path is ``pkg.resolved_reference.resolved_commit`` + # (set in ``apm_package.py``). Asserting on it directly catches + # silent regressions where the cache hit fails to propagate the + # SHA back into the consumer's resolved reference. + sha_a = pkg_a.resolved_reference.resolved_commit + sha_b = pkg_b.resolved_reference.resolved_commit + assert sha_a is not None and sha_a != "unknown" and len(sha_a) == 40, ( + f"expected resolved 40-char SHA for {SUBDIR_A}, got {sha_a!r}" + ) + assert sha_b is not None and sha_b != "unknown" and len(sha_b) == 40, ( + f"expected resolved 40-char SHA for {SUBDIR_B}, got {sha_b!r}" + ) + assert sha_a == sha_b, ( + f"Sibling subdirs from same repo+ref must resolve to " + f"same commit, got a={sha_a} b={sha_b}" + ) finally: shutil.rmtree(test_dir, ignore_errors=True) diff --git a/tests/unit/deps/test_shared_clone_cache.py b/tests/unit/deps/test_shared_clone_cache.py index a2204fb5..a579f6c0 100644 --- a/tests/unit/deps/test_shared_clone_cache.py +++ b/tests/unit/deps/test_shared_clone_cache.py @@ -422,11 +422,13 @@ def _make_downloader(self, tmp_path: Path): return d def test_sha_ref_tier1_init_fetch_path(self, tmp_path: Path) -> None: - """6.4 + 6.18: SHA ref triggers init+fetch tier 1 with update-ref HEAD.""" + """6.4 + 6.18: full SHA triggers init+fetch tier 1 with update-ref HEAD.""" from apm_cli.models.dependency.reference import DependencyReference + # Real 40-char hex SHA (tier-1 only runs for full SHAs, not abbreviations). + full_sha = "0123456789abcdef0123456789abcdef01234567" d = self._make_downloader(tmp_path) - dep = DependencyReference.parse("o/r/skills/X#abc1234567890abcdef1234567890abcdef12345678") + dep = DependencyReference.parse(f"o/r/skills/X#{full_sha}") bare = tmp_path / "bare" captured: list[list[str]] = [] @@ -440,7 +442,7 @@ def fake_run(args, **kwargs): "https://example/o/r", bare, dep_ref=dep, - ref="abc1234567890abcdef1234567890abcdef12345678", + ref=full_sha, is_commit_sha=True, ) @@ -456,20 +458,26 @@ def fake_run(args, **kwargs): assert len(update_ref_calls) == 1, ( f"expected 1 update-ref HEAD call, got {update_ref_calls}" ) - assert update_ref_calls[0][-1] == "abc1234567890abcdef1234567890abcdef12345678" + assert update_ref_calls[0][-1] == full_sha # 6.19: token scrub via remote set-url origin redacted:// assert any("remote set-url origin redacted://" in s for s in cmd_strings), ( "missing token scrub" ) def test_sha_ref_tier2_fallback_on_fetch_rejection(self, tmp_path: Path) -> None: - """6.12: tier-1 fetch fails (server rejects SHA fetch) -> tier-2 full clone.""" + """6.12: tier-1 fetch fails (server rejects SHA fetch) -> tier-2 full clone. + + Also covers Copilot review #1135: tier-2 must use the full 40-char SHA + from `rev-parse --verify ^{commit}` for `update-ref HEAD`, not + the (possibly abbreviated) input ref. + """ import subprocess as sp from apm_cli.models.dependency.reference import DependencyReference + full_sha = "0123456789abcdef0123456789abcdef01234567" d = self._make_downloader(tmp_path) - dep = DependencyReference.parse("o/r/skills/X#abc1234567890abcdef1234567890abcdef12345678") + dep = DependencyReference.parse(f"o/r/skills/X#{full_sha}") bare = tmp_path / "bare" captured: list[list[str]] = [] @@ -479,6 +487,10 @@ def fake_run(args, **kwargs): if "fetch --depth=1" in cmd_str: # Tier-1 fetch fails (simulating allowReachableSHA1InWant=false) raise sp.CalledProcessError(1, args, stderr=b"reject") + if "rev-parse --verify" in cmd_str: + # Tier-2 resolves the (possibly abbreviated) ref to the + # canonical 40-char SHA via rev-parse stdout. + return MagicMock(returncode=0, stdout=full_sha + "\n", stderr="") return MagicMock(returncode=0, stdout="", stderr="") with patch("apm_cli.deps.bare_cache.subprocess.run", side_effect=fake_run): @@ -486,7 +498,7 @@ def fake_run(args, **kwargs): "https://example/o/r", bare, dep_ref=dep, - ref="abc1234567890abcdef1234567890abcdef12345678", + ref=full_sha, is_commit_sha=True, ) @@ -500,12 +512,61 @@ def fake_run(args, **kwargs): assert any("rev-parse --verify" in s and "^{commit}" in s for s in cmd_strings), ( "missing tier-2 SHA verify" ) - # update-ref HEAD still set on tier 2 + # update-ref HEAD still set on tier 2 with the full 40-char SHA update_ref_calls = [ c for c in captured if len(c) >= 4 and c[-3] == "update-ref" and c[-2] == "HEAD" ] assert len(update_ref_calls) == 1 - assert update_ref_calls[0][-1] == "abc1234567890abcdef1234567890abcdef12345678" + assert update_ref_calls[0][-1] == full_sha + + def test_short_sha_skips_tier1_and_resolves_via_tier2(self, tmp_path: Path) -> None: + """Copilot review #1135: short SHA must skip tier 1 (which requires + full SHA for `git fetch `) and resolve to a 40-char SHA via + tier-2 `rev-parse --verify ^{commit}`. The resolved 40-char + SHA is what gets passed to `update-ref HEAD`, not the abbreviation. + """ + from apm_cli.models.dependency.reference import DependencyReference + + short_sha = "abc1234" # 7-char abbreviation + full_sha = "abc12345670000000000000000000000000fffff" + d = self._make_downloader(tmp_path) + dep = DependencyReference.parse(f"o/r/skills/X#{short_sha}") + bare = tmp_path / "bare" + captured: list[list[str]] = [] + + def fake_run(args, **kwargs): + captured.append(list(args)) + cmd_str = " ".join(args) + if "rev-parse --verify" in cmd_str: + return MagicMock(returncode=0, stdout=full_sha + "\n", stderr="") + return MagicMock(returncode=0, stdout="", stderr="") + + with patch("apm_cli.deps.bare_cache.subprocess.run", side_effect=fake_run): + d._bare_clone_with_fallback( + "https://example/o/r", + bare, + dep_ref=dep, + ref=short_sha, + is_commit_sha=True, + ) + + cmd_strings = [" ".join(c) for c in captured] + # Tier 1 (init+fetch) MUST NOT be attempted for short SHAs. + assert not any("init --bare" in s for s in cmd_strings), ( + f"tier-1 must be skipped for short SHA, got {cmd_strings}" + ) + assert not any("fetch --depth=1" in s for s in cmd_strings), ( + "tier-1 fetch must be skipped for short SHA" + ) + # Tier 2 full clone + rev-parse + update-ref + assert any("clone --bare" in s for s in cmd_strings), "missing tier-2 clone" + update_ref_calls = [ + c for c in captured if len(c) >= 4 and c[-3] == "update-ref" and c[-2] == "HEAD" + ] + assert len(update_ref_calls) == 1 + # CRITICAL: the resolved full 40-char SHA is set, not the abbreviation. + assert update_ref_calls[0][-1] == full_sha + assert update_ref_calls[0][-1] != short_sha def test_symbolic_ref_tier1_shallow_clone(self, tmp_path: Path) -> None: """Symbolic ref triggers tier-1 shallow clone with --branch.""" @@ -819,3 +880,155 @@ def fake_bare_clone(url, target, *, dep_ref, ref, is_commit_sha): match=r"Subdirectory ['\"]?skills/DOES_NOT_EXIST_TYPO['\"]? not found", ): downloader.download_subdirectory_package(dep, target_out) + + +class TestBareScrubFetchHead: + """Supply-chain panel follow-up: tier-1 init+fetch leaves the tokenized + URL inside ``FETCH_HEAD`` even after the config scrub. The bare-cache + scrub helper must truncate ``FETCH_HEAD`` so the token does not survive + on disk in any artifact. + """ + + def test_scrub_truncates_fetch_head_when_present(self, tmp_path: Path) -> None: + from apm_cli.deps.bare_cache import _scrub_bare_remote_url + + bare = tmp_path / "bare" + bare.mkdir() + fetch_head = bare / "FETCH_HEAD" + fetch_head.write_text( + "abcdef0123456789 branch 'main' of " + "https://oauth2:ghp_SECRET_TOKEN_FAKE@github.com/o/r\n" + ) + + with patch("apm_cli.deps.bare_cache.subprocess.run") as run_mock: + run_mock.return_value = MagicMock(returncode=0, stdout="", stderr="") + _scrub_bare_remote_url(bare, "/usr/bin/git", {}) + + assert fetch_head.exists(), "FETCH_HEAD must be preserved (only truncated)" + assert fetch_head.read_text() == "", ( + "FETCH_HEAD must be truncated so the tokenized URL does not persist on disk" + ) + + def test_scrub_no_op_when_fetch_head_absent(self, tmp_path: Path) -> None: + from apm_cli.deps.bare_cache import _scrub_bare_remote_url + + bare = tmp_path / "bare" + bare.mkdir() + # No FETCH_HEAD file present (tier-2 path: full clone --bare). + + with patch("apm_cli.deps.bare_cache.subprocess.run") as run_mock: + run_mock.return_value = MagicMock(returncode=0, stdout="", stderr="") + # Must not raise even when FETCH_HEAD does not exist. + _scrub_bare_remote_url(bare, "/usr/bin/git", {}) + + assert not (bare / "FETCH_HEAD").exists() + + +class TestAdoBareBearerRetry: + """Panel follow-up: the ADO bearer 401 retry path in + ``_execute_transport_plan`` must compose correctly with the bare + clone action, so that an ADO bare cache materialization recovers + from a stale PAT exactly the way the working-tree clone path does + (validation parity). + """ + + def _make_ado_downloader(self, tmp_path: Path): + from apm_cli.deps.github_downloader import GitHubPackageDownloader + from apm_cli.deps.transport_selection import TransportAttempt, TransportPlan + + d = GitHubPackageDownloader.__new__(GitHubPackageDownloader) + d.auth_resolver = MagicMock() + d.token_manager = MagicMock() + d._transport_selector = MagicMock() + d._protocol_pref = MagicMock() + d._allow_fallback = False + d._fallback_port_warned = set() + d._strategies = MagicMock() + d.git_env = {} + + # Token attempt with basic auth scheme on ADO is the trigger + # condition for the bearer retry branch. + d._build_repo_url = MagicMock( + side_effect=lambda *a, **kw: ( + "https://bearer-url/o/r" + if kw.get("auth_scheme") == "bearer" + else "https://pat-url/o/r" + ) + ) + d._resolve_dep_token = MagicMock(return_value="pat-token") + ctx = MagicMock() + ctx.auth_scheme = "basic" + ctx.git_env = {} + d._resolve_dep_auth_ctx = MagicMock(return_value=ctx) + d._sanitize_git_error = MagicMock(side_effect=lambda s: s) + + attempt = TransportAttempt(scheme="https", label="https-token", use_token=True) + plan = TransportPlan(attempts=[attempt], strict=False) + d._transport_selector.select = MagicMock(return_value=plan) + return d + + def test_bare_clone_recovers_via_ado_bearer_after_pat_401(self, tmp_path: Path) -> None: + """ADO bare clone: PAT 401 -> bearer retry succeeds.""" + import subprocess as sp + + from apm_cli.models.dependency.reference import DependencyReference + + d = self._make_ado_downloader(tmp_path) + # ADO-style ref. + dep = DependencyReference.parse("dev.azure.com/org/proj/_git/repo/skills/X#main") + assert dep.is_azure_devops(), "fixture sanity: dep must be ADO" + + bare = tmp_path / "bare" + urls_seen: list[str] = [] + + def fake_run(args, **kwargs): + cmd_str = " ".join(args) + if "clone --bare" in cmd_str: + # URL appears in args; locate it by content rather than position + # (varies for tier-1 shallow vs tier-2 full clone). + url = next((a for a in args if a.startswith("https://")), "") + urls_seen.append(url) + if "pat-url" in url: + raise sp.CalledProcessError( + 128, args, stderr=b"fatal: Authentication failed for 'https://...'" + ) + return MagicMock(returncode=0, stdout="", stderr="") + return MagicMock(returncode=0, stdout="", stderr="") + + # Stub the bearer provider to be available with a fake token. + bearer_provider = MagicMock() + bearer_provider.is_available.return_value = True + bearer_provider.get_bearer_token.return_value = "fake-bearer-token" + + with ( + patch("apm_cli.deps.bare_cache.subprocess.run", side_effect=fake_run), + patch( + "apm_cli.core.azure_cli.get_bearer_provider", + return_value=bearer_provider, + ), + patch( + "apm_cli.utils.github_host.build_ado_bearer_git_env", + return_value={"GIT_CONFIG_COUNT": "1"}, + ), + ): + d._bare_clone_with_fallback( + "https://pat-url/o/r", + bare, + dep_ref=dep, + ref="main", + is_commit_sha=False, + ) + + # Both URLs must have been attempted: PAT first, bearer second. + assert any("pat-url" in u for u in urls_seen), ( + f"expected PAT clone attempt, got urls={urls_seen}" + ) + assert any("bearer-url" in u for u in urls_seen), ( + f"expected bearer retry clone attempt, got urls={urls_seen}" + ) + # Bearer attempt must come AFTER the PAT failure. + pat_idx = next(i for i, u in enumerate(urls_seen) if "pat-url" in u) + bearer_idx = next(i for i, u in enumerate(urls_seen) if "bearer-url" in u) + assert bearer_idx > pat_idx, f"bearer retry must follow PAT failure, urls={urls_seen}" + # Stale-PAT diagnostic must be emitted on bearer success. + assert d.auth_resolver.emit_stale_pat_diagnostic.called