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/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/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/bare_cache.py b/src/apm_cli/deps/bare_cache.py new file mode 100644 index 00000000..7d6d176e --- /dev/null +++ b/src/apm_cli/deps/bare_cache.py @@ -0,0 +1,545 @@ +"""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 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 _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``. + + 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. + + 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). + 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, + ) + + # 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], + 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(): + _rmtree(target) + + if is_commit_sha: + # 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"] + 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). + _rmtree(target) + 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 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 + 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(): + _rmtree(target) + 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 c1557ea6..d85df387 100644 --- a/src/apm_cli/deps/github_downloader.py +++ b/src/apm_cli/deps/github_downloader.py @@ -1,10 +1,11 @@ """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 import tempfile import time # noqa: F401 @@ -44,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, @@ -577,30 +584,48 @@ def _clone_with_fallback( verbose_callback=None, **clone_kwargs, ) -> Repo: - """Clone a 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``. - - 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 + """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, + verbose_callback=verbose_callback, + repo_cls=Repo, + **clone_kwargs, + ) - Returns: - Repo: Successfully cloned repository + 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 the planned attempt(s) all fail. + RuntimeError: If all planned attempts fail. """ - last_error = None + 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,21 +748,25 @@ 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. + # 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 @@ -768,13 +797,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 +809,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 @@ -794,76 +825,59 @@ 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) # ------------------------------------------------------------------ - # Remote ref enumeration (no clone required) + # 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: + """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, + ref=ref, + is_commit_sha=is_commit_sha, + ) + + def _materialize_from_bare( + self, + bare_path: Path, + consumer_dir: Path, + *, + ref: str | None, + env: dict[str, str], + known_sha: str | None = None, + ) -> str: + """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]: """Backward-compat stub -- delegates to git_remote_ops.""" @@ -1163,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, @@ -1665,54 +1679,68 @@ 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(), + # 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( + f"Failed to prepare dependency from cached clone: {e}" + ) from e else: # Legacy per-dep clone path (no shared cache). temp_dir = tempfile.mkdtemp(dir=get_apm_temp_dir()) @@ -1818,14 +1846,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: @@ -2017,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) @@ -2025,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" @@ -2267,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 @@ -2297,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: 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..2719e449 --- /dev/null +++ b/tests/integration/test_install_subdir_dedup_e2e.py @@ -0,0 +1,166 @@ +"""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. + + 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() + 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 (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 fb182bee..a579f6c0 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,805 @@ 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: 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(f"o/r/skills/X#{full_sha}") + 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.bare_cache.subprocess.run", side_effect=fake_run): + d._bare_clone_with_fallback( + "https://example/o/r", + bare, + dep_ref=dep, + ref=full_sha, + 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] == 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. + + 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(f"o/r/skills/X#{full_sha}") + 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") + 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): + d._bare_clone_with_fallback( + "https://example/o/r", + bare, + dep_ref=dep, + ref=full_sha, + 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 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] == 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.""" + 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.bare_cache.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.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] + 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.bare_cache.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.bare_cache.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) + + +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