diff --git a/.apm/instructions/architecture.instructions.md b/.apm/instructions/architecture.instructions.md index af2549aa9..fb8ad35e2 100644 --- a/.apm/instructions/architecture.instructions.md +++ b/.apm/instructions/architecture.instructions.md @@ -59,6 +59,7 @@ semicolon-delimited, and specific to the file(s) that own the fact. | File-level deploy / sync / cleanup | BaseIntegrator (see integrators.instructions.md) | `src/apm_cli/integration/base_integrator.py` | | Windows stable executable path | install.ps1 ($currentDir / $currentExe) | `install.ps1` | | Git repository cache-key normalization | cache/url_normalize.py (normalize_repo_url / cache_shard_key) | `src/apm_cli/cache/url_normalize.py` | +| Sparse-cone setup, dangling-symlink repair, and materialized symlink validation | utils/git_sparse.py | `src/apm_cli/utils/git_sparse.py` | | Self-update release -> installer ref + VERSION | commands/self_update.py (_ResolvedSelfUpdateRelease) | `src/apm_cli/commands/self_update.py` | | Dependency comparison identity vs display-cased materialization path | models/dependency/identity.py + materialization.py + DependencyReference | `src/apm_cli/models/dependency/identity.py`; `src/apm_cli/models/dependency/materialization.py`; `src/apm_cli/models/dependency/reference.py` | | Cached policy shape | policy/discovery.py (_policy_to_dict via _serialize_policy; ADO_POLICY_PROJECT; ADO_POLICY_REPOSITORY) | `src/apm_cli/policy/discovery.py` | diff --git a/.github/instructions/architecture.instructions.md b/.github/instructions/architecture.instructions.md index af2549aa9..fb8ad35e2 100644 --- a/.github/instructions/architecture.instructions.md +++ b/.github/instructions/architecture.instructions.md @@ -59,6 +59,7 @@ semicolon-delimited, and specific to the file(s) that own the fact. | File-level deploy / sync / cleanup | BaseIntegrator (see integrators.instructions.md) | `src/apm_cli/integration/base_integrator.py` | | Windows stable executable path | install.ps1 ($currentDir / $currentExe) | `install.ps1` | | Git repository cache-key normalization | cache/url_normalize.py (normalize_repo_url / cache_shard_key) | `src/apm_cli/cache/url_normalize.py` | +| Sparse-cone setup, dangling-symlink repair, and materialized symlink validation | utils/git_sparse.py | `src/apm_cli/utils/git_sparse.py` | | Self-update release -> installer ref + VERSION | commands/self_update.py (_ResolvedSelfUpdateRelease) | `src/apm_cli/commands/self_update.py` | | Dependency comparison identity vs display-cased materialization path | models/dependency/identity.py + materialization.py + DependencyReference | `src/apm_cli/models/dependency/identity.py`; `src/apm_cli/models/dependency/materialization.py`; `src/apm_cli/models/dependency/reference.py` | | Cached policy shape | policy/discovery.py (_policy_to_dict via _serialize_policy; ADO_POLICY_PROJECT; ADO_POLICY_REPOSITORY) | `src/apm_cli/policy/discovery.py` | diff --git a/CHANGELOG.md b/CHANGELOG.md index 0d20b6e20..6f70b75a3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -34,6 +34,12 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 under either `executables` or the deprecated `allowExecutables` key as an actionable informational warning instead of omitting the check. (#2719) +- Git subdirectory dependencies with symlinks to files elsewhere in the same + repository now install successfully where Git materializes symlinks; APM + widens the checkout only when needed. On Windows, Git defaults to + `core.symlinks=false` and checks these entries out as plain files, which is + outside #2707's scope. (by @MohammedAlkindi, closes #2707, #2710) + ## [0.29.0] - 2026-08-30 ### Added diff --git a/apm.lock.yaml b/apm.lock.yaml index e9f3ddbc1..e66145761 100644 --- a/apm.lock.yaml +++ b/apm.lock.yaml @@ -2783,7 +2783,7 @@ deployments: owners: - . active_owner: . - content_hash: sha256:7ab9944e010603517123e62c171be8eb1bc674c05e0ec8370d737a8890acb07f + content_hash: sha256:3445ddcf51a14f5a730cddb7f3cc5ce2bb11b5078951521a4903a098250f6f86 - kind: project-relative target: copilot value: .github/instructions/changelog.instructions.md @@ -3290,7 +3290,7 @@ local_deployed_file_hashes: .github/agents/spec-tag-architect.agent.md: sha256:82907265c5e7cf1ac61ad96866fa7c5683b69c8f09b7a4c5f3cc241acc9568ca .github/agents/supply-chain-security-expert.agent.md: sha256:8fb8cc426d6af17ba084a28b3f026c2b475b62e3ca63ed2f88b83bd823f877af .github/agents/test-coverage-expert.agent.md: sha256:48c2172d1f18a394fa83ef9dc2be0b9b921a4e51e976498165250fed66369711 - .github/instructions/architecture.instructions.md: sha256:7ab9944e010603517123e62c171be8eb1bc674c05e0ec8370d737a8890acb07f + .github/instructions/architecture.instructions.md: sha256:3445ddcf51a14f5a730cddb7f3cc5ce2bb11b5078951521a4903a098250f6f86 .github/instructions/changelog.instructions.md: sha256:1e51ec4c74e847967962bd279dc4c6e582c5d3578490b3c28d5f3acd3e05f73e .github/instructions/cicd.instructions.md: sha256:33201cb88ea2f34b4950a9b52f87dc8dfb682796aaf53068ba7ae406c0c5e2c2 .github/instructions/cli.instructions.md: sha256:8e39e8d5047ce88575cb02f87c2bcede584dfef258bd86f7466c7badf136541a diff --git a/docs/src/content/docs/enterprise/security.md b/docs/src/content/docs/enterprise/security.md index 4cc3e6e7b..f4651a5ba 100644 --- a/docs/src/content/docs/enterprise/security.md +++ b/docs/src/content/docs/enterprise/security.md @@ -329,9 +329,8 @@ Trust boundaries: ### Symlink handling -Symlinks are rejected in most APM operations; the only context where in-package -symlinks are followed is local-path install, under a per-symlink containment -check (see below): +Symlinks are rejected in most APM operations. They are followed only +during contained package materialization: - **Primitive discovery** (instructions, agents, prompts, contexts, skills) rejects symlinked files during glob-based file enumeration. Symlinks are silently skipped. - **Prompt resolution** (`apm preview`, `apm run`) rejects symlinked `.prompt.md` files with an explicit error message. @@ -342,6 +341,12 @@ check (see below): - **Manifest integrity** -- a malformed `apm.yml` (invalid YAML or non-mapping content) triggers a failing `manifest-parse` audit check. Policy and baseline CI checks never silently pass when the manifest cannot be parsed. If this check fires, fix the YAML syntax error in your `apm.yml` and re-run the audit. - **Archive creation** -- `apm pack` excludes symlinks from bundled archives. Packaged artifacts contain no symbolic links, preventing symlink-based escape attacks in distributed bundles. +Remote Git subdirectory installs can dereference a symlink whose target is a +tracked file in the same checked-out commit. If sparse checkout excluded that +target, APM widens the checkout before copying the package. Generated Git +metadata, targets outside the repository, and links that remain broken after +widening hard-fail the install. + #### Local-install symlink dereference and containment guarantee When installing a local-path dependency (`apm install /path/to/pkg`), APM diff --git a/docs/src/content/docs/reference/cli/cache.md b/docs/src/content/docs/reference/cli/cache.md index 9a5b98a16..2b2a1ba1c 100644 --- a/docs/src/content/docs/reference/cli/cache.md +++ b/docs/src/content/docs/reference/cli/cache.md @@ -129,7 +129,10 @@ Inside the cache root: # for sparse-checkout consumers checkouts_v1/ # per-SHA worktree checkouts, variant-keyed # //full/ -- full tree - # //sparse-/ -- sparse cone + # //sparse-/ -- sparse cone, or a + # full tree when a + # symlink target lies + # outside the cone # ( = first # 16 hex of # sha256(paths)) @@ -140,7 +143,9 @@ The `full/` and `sparse-/` subdirs let two consumers of the same commit share storage when they want the same subdirs, and keep distinct shards when they do not -- without the variant suffix the sparse checkout would clobber the full tree for any other consumer -of that SHA. +of that SHA. A sparse variant widens to the full tree when a package +symlink targets a tracked file excluded from the sparse cone, so that +variant can consume more disk than its name suggests. The cache root is created with mode `0700` and validated to be absolute with no NUL bytes before use. diff --git a/scripts/lint-architecture-boundaries.sh b/scripts/lint-architecture-boundaries.sh index ceef89e7f..fedf01cba 100755 --- a/scripts/lint-architecture-boundaries.sh +++ b/scripts/lint-architecture-boundaries.sh @@ -1112,6 +1112,34 @@ check_pattern \ 'to_repository_cache_url' \ src/apm_cli +echo "[*] AC11a: sparse-cone materialization authority" +sparse_cone_owner="src/apm_cli/utils/git_sparse.py" +sparse_cone_raw_set_hits=$( + grep -rEn --include='*.py' \ + '"sparse-checkout",[[:space:]]*"set"' src/apm_cli \ + | grep -v "^${sparse_cone_owner}:" \ + | grep -v '^src/apm_cli/deps/git_file_transport.py:' \ + | grep -v 'architecture-authority-exempt:' \ + || true +) +if [ "$(grep -Ec '^def apply_sparse_cone\(' "$sparse_cone_owner")" -ne 1 ] \ + || [ "$(grep -Ec '^def repair_dangling_cone_symlinks\(' "$sparse_cone_owner")" -ne 1 ] \ + || [ "$(grep -Ec '^def _literal_pathspec\(' "$sparse_cone_owner")" -ne 1 ] \ + || [ "$(grep -Fc '"ls-tree",' "$sparse_cone_owner")" -ne 2 ] \ + || [ "$(grep -Fc '_literal_pathspec(path)' "$sparse_cone_owner")" -ne 2 ] \ + || [ "$(grep -Ec '^ def _finalize_sparse_checkout\(' src/apm_cli/cache/git_cache.py)" -ne 1 ] \ + || [ "$(grep -Fc 'self._finalize_sparse_checkout(' src/apm_cli/cache/git_cache.py)" -ne 3 ] \ + || [ "$(grep -Fc 'repair_dangling_cone_symlinks(' src/apm_cli/cache/git_cache.py)" -ne 1 ] \ + || [ "$(grep -Fc 'repair_dangling_cone_symlinks(' src/apm_cli/deps/bare_cache.py)" -ne 1 ] \ + || [ "$(grep -Fc 'repair_dangling_cone_symlinks(' src/apm_cli/deps/github_downloader.py)" -ne 1 ] \ + || ! grep -Fq 'return _repair(setup_env)' src/apm_cli/deps/github_downloader.py \ + || ! grep -Fq 'return _repair(env)' src/apm_cli/deps/github_downloader.py \ + || [ -n "$sparse_cone_raw_set_hits" ]; then + echo "[x] Sparse-cone materialization must route through utils/git_sparse.py" + [ -n "$sparse_cone_raw_set_hits" ] && echo "$sparse_cone_raw_set_hits" + violations=$((violations + 1)) +fi + echo "[*] AC12: diagnostic printable-ASCII authority" diagnostic_ascii_output=$(python3 scripts/check_diagnostic_ascii_owner.py --root "$ROOT" 2>&1) diagnostic_ascii_status=$? diff --git a/src/apm_cli/cache/git_cache.py b/src/apm_cli/cache/git_cache.py index e3949ec5d..e49c14bd7 100644 --- a/src/apm_cli/cache/git_cache.py +++ b/src/apm_cli/cache/git_cache.py @@ -33,7 +33,7 @@ import subprocess from pathlib import Path -from ..utils.git_sparse import apply_sparse_cone +from ..utils.git_sparse import apply_sparse_cone, repair_dangling_cone_symlinks from ..utils.path_security import ensure_path_within from .integrity import verify_checkout_sha from .locking import atomic_land, cleanup_incomplete, shard_lock, stage_path @@ -192,7 +192,12 @@ def get_checkout( if not self._refresh and checkout_dir.is_dir(): if verify_checkout_sha(checkout_dir, sha): _log.debug("Cache HIT: %s @ %s [%s]", url, sha[:12], variant) - return checkout_dir + with shard_lock(checkout_dir): + return self._finalize_sparse_checkout( + checkout_dir, + sparse_paths, + env=env, + ) else: # Integrity failure -- evict _log.warning( @@ -218,6 +223,36 @@ def get_checkout( promisor_url=url if use_partial else None, ) + def _finalize_sparse_checkout( + self, + checkout_dir: Path, + sparse_paths: list[str] | None, + *, + env: dict[str, str] | None, + ) -> Path: + """Repair and validate a sparse checkout before any cache return.""" + if not sparse_paths: + return checkout_dir + from ..utils.git_env import get_git_executable, git_subprocess_env + + git_exe = get_git_executable() + subprocess_env = env if env is not None else git_subprocess_env() + dangling = repair_dangling_cone_symlinks( + git_exe, + checkout_dir, + list(sparse_paths), + env=subprocess_env, + extra_git_args=_safe_git_args(), + ) + if dangling is not None: + _log.info( + "Sparse-cone checkout of %s left a dangling symlink at %s; " + "widened to a full checkout so it resolves (#2707).", + checkout_dir, + dangling, + ) + return checkout_dir + def _resolve_sha( self, url: str, @@ -552,7 +587,11 @@ def _create_checkout( sha[:12], variant, ) - return final_dir + return self._finalize_sparse_checkout( + final_dir, + sparse_paths, + env=env, + ) staged = stage_path(final_dir) ensure_path_within(staged, self._checkouts_root) @@ -645,6 +684,22 @@ def _create_checkout( env=subprocess_env, check=True, ) + if sparse_paths: + # Correctness repair, not a failure fallback (#2707): + # if the cone left a dangling symlink (target outside + # the requested paths), widen to a full checkout so + # it resolves. Only fires when the narrow cone would + # otherwise ship a broken checkout. + self._finalize_sparse_checkout( + staged, + sparse_paths, + env=env, + ) + except (RuntimeError, ValueError): + from ..utils.file_ops import robust_rmtree + + robust_rmtree(staged, ignore_errors=True) + raise except (subprocess.CalledProcessError, subprocess.TimeoutExpired, OSError) as exc: from ..utils.file_ops import robust_rmtree diff --git a/src/apm_cli/deps/bare_cache.py b/src/apm_cli/deps/bare_cache.py index b3e156d9b..9ce6a8657 100644 --- a/src/apm_cli/deps/bare_cache.py +++ b/src/apm_cli/deps/bare_cache.py @@ -33,7 +33,7 @@ from git import Repo -from ..utils.git_sparse import apply_sparse_cone +from ..utils.git_sparse import apply_sparse_cone, repair_dangling_cone_symlinks if TYPE_CHECKING: from ..models.apm_package import DependencyReference @@ -579,6 +579,11 @@ def materialize_from_bare( - Sparse-checkout failures are RAISED (not silently fallen back) because a silent fallback would re-introduce the 78 MB bloat this parameter exists to avoid. + - After checkout, if the cone left a dangling symlink (target + outside the requested paths -- #2707), falls back to + ``git sparse-checkout disable`` so the target resolves. This is + a correctness repair, not a failure fallback: it only fires + when the narrow cone would otherwise ship a broken checkout. Returns: The resolved commit SHA. Caller threads this into @@ -657,6 +662,16 @@ def materialize_from_bare( env=env, check=True, ) + if sparse_paths: + dangling = repair_dangling_cone_symlinks(git_exe, consumer_dir, list(sparse_paths), env=env) + if dangling is not None: + _log.info( + "Sparse-cone checkout of %s left a dangling symlink at %s " + "(target outside the requested cone); widened to a full " + "checkout so it resolves (#2707).", + consumer_dir, + dangling, + ) return resolved_sha diff --git a/src/apm_cli/deps/github_downloader.py b/src/apm_cli/deps/github_downloader.py index 242d33c99..306701231 100644 --- a/src/apm_cli/deps/github_downloader.py +++ b/src/apm_cli/deps/github_downloader.py @@ -1,6 +1,7 @@ """GitHub package downloader for APM dependencies.""" import contextlib +import logging import os import re import subprocess @@ -32,6 +33,12 @@ from ..utils.console import ( _rich_warning, # noqa: F401 -- re-exported; tests patch github_downloader._rich_warning ) +from ..utils.git_sparse import ( + apply_sparse_cone, + repair_dangling_cone_symlinks, + sparse_checkout_active, + validate_materialized_symlinks, +) from ..utils.github_host import ( default_host, is_github_hostname, @@ -56,6 +63,8 @@ TransportSelector, ) +_log = logging.getLogger(__name__) + # Public docs anchor for the cross-protocol fallback caveat surfaced by the # #786 warning. Lives under the dependencies guide, next to the canonical # `--allow-protocol-fallback` section (Starlight site defined in @@ -1167,6 +1176,20 @@ def _try_sparse_checkout( try: temp_clone_path.mkdir(parents=True, exist_ok=True) + def _repair(repo_env: dict[str, str]) -> bool: + dangling = repair_dangling_cone_symlinks( + "git", + temp_clone_path, + [subdir_path], + env=repo_env, + ) + if dangling is not None: + _log.info( + "Sparse checkout widened to repair dangling symlink '%s' (#2707).", + dangling.relative_to(temp_clone_path), + ) + return True + public_github_anonymous_first = ( not dep_ref.is_insecure and self.auth_resolver.uses_public_github_anonymous_first( @@ -1189,8 +1212,6 @@ def _try_sparse_checkout( setup_cmds = [ ["git", "init"], ["git", "remote", "add", "origin", anonymous_url], - ["git", "sparse-checkout", "init", "--cone"], - ["git", "sparse-checkout", "set", subdir_path], ] for cmd in setup_cmds: result = subprocess.run( @@ -1204,6 +1225,13 @@ def _try_sparse_checkout( ) if result.returncode != 0: return False + apply_sparse_cone( + "git", + temp_clone_path, + [subdir_path], + env=setup_env, + timeout=120, + ) def _fetch(token: str | None, git_env: dict[str, str]) -> None: if token is not None: @@ -1266,7 +1294,9 @@ def _fetch(token: str | None, git_env: dict[str, str]) -> None: encoding="utf-8", timeout=120, ) - return checkout_result.returncode == 0 + if checkout_result.returncode != 0: + return False + return _repair(setup_env) # Resolve per-dependency auth via AuthResolver. dep_auth_ctx = self._resolve_dep_auth_ctx(dep_ref) @@ -1292,16 +1322,35 @@ def _fetch(token: str | None, git_env: dict[str, str]) -> None: cmds = [ ["git", "init"], ["git", "remote", "add", "origin", auth_url], - ["git", "sparse-checkout", "init", "--cone"], - ["git", "sparse-checkout", "set", subdir_path], ] + for cmd in cmds: + result = subprocess.run( + cmd, + cwd=str(temp_clone_path), + env=env, + capture_output=True, + text=True, + encoding="utf-8", + timeout=120, + ) + if result.returncode != 0: + _debug( + f"Sparse-checkout step failed ({' '.join(cmd)}): {result.stderr.strip()}" + ) + return False + apply_sparse_cone( + "git", + temp_clone_path, + [subdir_path], + env=env, + timeout=120, + ) fetch_cmd = ["git", "fetch", "origin"] fetch_cmd.append(ref or "HEAD") fetch_cmd.append("--depth=1") - cmds.append(fetch_cmd) - cmds.append(["git", "checkout", "FETCH_HEAD"]) + checkout_cmds = [fetch_cmd, ["git", "checkout", "FETCH_HEAD"]] - for cmd in cmds: + for cmd in checkout_cmds: result = subprocess.run( cmd, cwd=str(temp_clone_path), @@ -1317,7 +1366,7 @@ def _fetch(token: str | None, git_env: dict[str, str]) -> None: ) return False - return True + return _repair(env) except Exception as e: _debug(f"Sparse-checkout failed: {e}") return False @@ -1422,7 +1471,7 @@ def download_subdirectory_package( if _persistent_checkout is not None: # WS3: persistent cache hit -- use the cached checkout directly. temp_clone_path = _persistent_checkout - if _perf_logger is not None: + if _perf_logger is not None and getattr(_perf_logger, "verbose", False): _sha_short = ( (ref or "")[:12] if ref and re.match(r"^[a-f0-9]{7,40}$", ref) else "" ) @@ -1433,7 +1482,11 @@ def download_subdirectory_package( sparse_paths=[subdir_path], ) _perf_logger.materialize_result( - sparse_applied=True, + sparse_applied=sparse_checkout_active( + "git", + _persistent_checkout, + env=self._git_env_dict(), + ), consumer_size_bytes=_dir_size_bytes(_persistent_checkout), ) elif use_shared: @@ -1523,9 +1576,13 @@ def _shared_bare_fetch_fn(existing_bare: Path, ref_or_sha: str) -> bool: raise RuntimeError( f"Failed to prepare dependency from cached clone: {e}" ) from e - if _perf_logger is not None: + if _perf_logger is not None and getattr(_perf_logger, "verbose", False): _perf_logger.materialize_result( - sparse_applied=True, + sparse_applied=sparse_checkout_active( + "git", + temp_clone_path, + env=self._git_env_dict(), + ), consumer_size_bytes=_dir_size_bytes(temp_clone_path), ) else: @@ -1610,6 +1667,12 @@ def _shared_bare_fetch_fn(existing_bare: Path, ref_or_sha: str) -> bool: if not source_subdir.is_dir(): raise RuntimeError(f"Path '{subdir_path}' is not a directory") + validate_materialized_symlinks( + "git", + temp_clone_path, + [subdir_path], + env=self._git_env_dict(), + ) # Create target directory target_path.mkdir(parents=True, exist_ok=True) diff --git a/src/apm_cli/utils/git_sparse.py b/src/apm_cli/utils/git_sparse.py index 19288b0d2..118087c5c 100644 --- a/src/apm_cli/utils/git_sparse.py +++ b/src/apm_cli/utils/git_sparse.py @@ -9,9 +9,250 @@ from __future__ import annotations +import os import subprocess from pathlib import Path +from .path_security import PathTraversalError, ensure_path_within + +FULL_CHECKOUT_TIMEOUT_SECONDS = 300 + + +def _literal_pathspec(path: str) -> str: + """Return a Git pathspec that treats every character in *path* literally.""" + return f":(literal){path}" + + +def _tracked_symlinks( + git_exe: str, + repo_dir: Path, + paths: list[str], + *, + env: dict[str, str] | None, + timeout: int, + extra_git_args: list[str] | None, +) -> list[Path]: + """Return materialized tracked symlinks under *paths*. + + Git's index identifies mode-120000 entries without walking every file + in the cone. The filesystem check excludes platforms where Git writes + symlink entries as plain files because ``core.symlinks`` is disabled. + """ + if not paths: + return [] + head = [git_exe, *(extra_git_args or [])] + result = subprocess.run( + [ + *head, + "-C", + str(repo_dir), + "ls-tree", + "-r", + "-z", + "HEAD", + "--", + *(_literal_pathspec(path) for path in paths), + ], + capture_output=True, + text=True, + timeout=timeout, + env=env, + check=True, + ) + symlinks: list[Path] = [] + for record in result.stdout.split("\0"): + metadata, separator, relative = record.partition("\t") + if separator and metadata.split(maxsplit=1)[0] == "120000": + candidate = repo_dir / relative + if os.path.islink(candidate): + symlinks.append(candidate) + return symlinks + + +def _first_dangling_tracked_symlink( + git_exe: str, + repo_dir: Path, + paths: list[str], + *, + env: dict[str, str] | None, + timeout: int, + extra_git_args: list[str] | None, +) -> Path | None: + """Validate tracked symlink containment and return the first broken link.""" + symlinks = _tracked_symlinks( + git_exe, + repo_dir, + paths, + env=env, + timeout=timeout, + extra_git_args=extra_git_args, + ) + targets: list[tuple[Path, Path]] = [] + for link in symlinks: + try: + raw_target = Path(os.readlink(link)) + except OSError: + if not os.path.exists(link): + return link + continue + target = raw_target if raw_target.is_absolute() else link.parent / raw_target + resolved_target = ensure_path_within(target, repo_dir) + targets.append((link, resolved_target)) + + if targets: + head = [git_exe, *(extra_git_args or [])] + relative_targets = [ + target.relative_to(repo_dir.resolve()).as_posix() for _, target in targets + ] + result = subprocess.run( + [ + *head, + "-C", + str(repo_dir), + "ls-tree", + "-r", + "-z", + "HEAD", + "--", + *(_literal_pathspec(path) for path in relative_targets), + ], + capture_output=True, + text=True, + timeout=timeout, + env=env, + check=True, + ) + tracked_files = { + relative + for record in result.stdout.split("\0") + if record + for _, separator, relative in (record.partition("\t"),) + if separator + } + else: + tracked_files = set() + + for link, resolved_target in targets: + relative_target = resolved_target.relative_to(repo_dir.resolve()).as_posix() + if relative_target not in tracked_files: + relative_link = link.relative_to(repo_dir) + raise PathTraversalError( + f"Symlink '{relative_link}' targets '{relative_target}', which is not " + "a tracked file in the checked-out commit." + ) + if not os.path.exists(link): + return link + return None + + +def validate_materialized_symlinks( + git_exe: str, + repo_dir: Path, + paths: list[str], + *, + env: dict[str, str] | None, + timeout: int = 30, + extra_git_args: list[str] | None = None, +) -> None: + """Reject broken or checkout-escaping symlinks before package copy.""" + dangling = _first_dangling_tracked_symlink( + git_exe, + repo_dir, + paths, + env=env, + timeout=timeout, + extra_git_args=extra_git_args, + ) + if dangling is not None: + relative = dangling.relative_to(repo_dir) + raise RuntimeError( + f"Symlink '{relative}' is unresolved after materializing the repository; " + "repair its target in the package repository." + ) + + +def sparse_checkout_active( + git_exe: str, + repo_dir: Path, + *, + env: dict[str, str] | None, + timeout: int = 10, + extra_git_args: list[str] | None = None, +) -> bool: + """Return whether Git still considers *repo_dir* a sparse checkout.""" + head = [git_exe, *(extra_git_args or [])] + result = subprocess.run( + [*head, "-C", str(repo_dir), "config", "--bool", "core.sparseCheckout"], + capture_output=True, + text=True, + timeout=timeout, + env=env, + check=False, + ) + return result.returncode == 0 and result.stdout.strip().lower() == "true" + + +def repair_dangling_cone_symlinks( + git_exe: str, + repo_dir: Path, + paths: list[str], + *, + env: dict[str, str] | None, + timeout: int = FULL_CHECKOUT_TIMEOUT_SECONDS, + extra_git_args: list[str] | None = None, +) -> Path | None: + """Widen a cone checkout to a full tree if it left a dangling symlink. + + Call AFTER the cone checkout (``apply_sparse_cone`` + ``git + checkout``) completes. Queries the Git index for tracked symlinks in + the requested ``paths`` and checks whether a target was excluded by + the cone. If one is found, + falls back to ``git sparse-checkout disable`` so every symlink + target that exists anywhere in the tree resolves (#2707). In a plain + clone the full tree repopulates from objects already fetched; in a + partial clone (``--filter=blob:none`` promisor remotes) the disable + fetches the missing blobs from the remote at repair time. + + This trades the perf-#1433 disk savings for correctness on the repos + that need it -- a dependency whose payload is mostly symlinks into + the repo root loses the sparse win on every install. The common case + (no cross-cone symlinks) checks only mode-120000 index entries and + never disables sparse-checkout. + + Returns: + The first dangling symlink found (repo-relative resolution + already applied by the caller's ``repo_dir``), or ``None`` if + the cone had no dangling symlinks and no repair was needed. + """ + dangling = _first_dangling_tracked_symlink( + git_exe, + repo_dir, + paths, + env=env, + timeout=timeout, + extra_git_args=extra_git_args, + ) + if dangling is None: + return None + head = [git_exe, *(extra_git_args or [])] + subprocess.run( + [*head, "-C", str(repo_dir), "sparse-checkout", "disable"], + capture_output=True, + text=True, + timeout=timeout, + env=env, + check=True, + ) + validate_materialized_symlinks( + git_exe, + repo_dir, + paths, + env=env, + timeout=timeout, + extra_git_args=extra_git_args, + ) + return dangling + def apply_sparse_cone( git_exe: str, diff --git a/tests/integration/test_architecture_authorities.py b/tests/integration/test_architecture_authorities.py index e76b84a22..3c282d6e0 100644 --- a/tests/integration/test_architecture_authorities.py +++ b/tests/integration/test_architecture_authorities.py @@ -53,6 +53,78 @@ def test_resolution_replacement_activation_has_one_owner(tmp_path: Path) -> None assert "duplicates owner methods: prepare_replacement" in result.stdout +def test_sparse_cone_materialization_has_single_owner() -> None: + """Every cone checkout must route through the shared repair policy.""" + root = Path(__file__).parents[2] + owner = (root / "src/apm_cli/utils/git_sparse.py").read_text(encoding="utf-8") + git_cache = (root / "src/apm_cli/cache/git_cache.py").read_text(encoding="utf-8") + bare_cache = (root / "src/apm_cli/deps/bare_cache.py").read_text(encoding="utf-8") + downloader = (root / "src/apm_cli/deps/github_downloader.py").read_text(encoding="utf-8") + guard = (root / "scripts/lint-architecture-boundaries.sh").read_text(encoding="utf-8") + owner_table = (root / ".apm/instructions/architecture.instructions.md").read_text( + encoding="utf-8" + ) + + assert owner.count("def apply_sparse_cone(") == 1 + assert owner.count("def repair_dangling_cone_symlinks(") == 1 + assert owner.count("def _literal_pathspec(") == 1 + assert owner.count('"ls-tree",') == 2 + assert owner.count("_literal_pathspec(path)") == 2 + assert git_cache.count("repair_dangling_cone_symlinks(") == 1 + assert git_cache.count("def _finalize_sparse_checkout(") == 1 + assert git_cache.count("self._finalize_sparse_checkout(") == 3 + assert bare_cache.count("repair_dangling_cone_symlinks(") == 1 + assert downloader.count("repair_dangling_cone_symlinks(") == 1 + assert "return _repair(setup_env)" in downloader + assert "return _repair(env)" in downloader + assert '"sparse-checkout", "set"' not in downloader + assert "Sparse-cone materialization must route through utils/git_sparse.py" in guard + assert ( + "| Sparse-cone setup, dangling-symlink repair, and materialized symlink validation " + "| utils/git_sparse.py | `src/apm_cli/utils/git_sparse.py` |" + ) in owner_table + + +def test_sparse_cone_materialization_guard_rejects_bypass(tmp_path: Path) -> None: + """The boundary lint rejects a consumer that skips the repair owner.""" + root = Path(__file__).parents[2] + sandbox = tmp_path / "repo" + shutil.copytree( + root, + sandbox, + ignore=shutil.ignore_patterns( + ".git", + ".venv", + ".pytest_cache", + "__pycache__", + "build", + "dist", + "node_modules", + ), + ) + consumer = sandbox / "src/apm_cli/deps/github_downloader.py" + consumer.write_text( + consumer.read_text(encoding="utf-8").replace( + " return _repair(env)\n", + " return True\n", + 1, + ), + encoding="utf-8", + ) + + result = subprocess.run( + ("bash", "scripts/lint-architecture-boundaries.sh"), + cwd=sandbox, + capture_output=True, + text=True, + check=False, + timeout=300, + ) + + assert result.returncode == 1 + assert "Sparse-cone materialization must route through utils/git_sparse.py" in result.stdout + + def test_generated_bundle_text_writes_are_lf_deterministic() -> None: """Generated bundle text must route through the checked LF boundary.""" root = Path(__file__).parents[2] diff --git a/tests/integration/test_sparse_cone_symlink_repair.py b/tests/integration/test_sparse_cone_symlink_repair.py new file mode 100644 index 000000000..46ba05b2a --- /dev/null +++ b/tests/integration/test_sparse_cone_symlink_repair.py @@ -0,0 +1,243 @@ +"""Real-Git regression coverage for sparse-cone symlink repair.""" + +from __future__ import annotations + +import os +import subprocess +from pathlib import Path +from unittest.mock import MagicMock + +import pytest + +from apm_cli.cache.git_cache import GitCache, _variant_key +from apm_cli.cache.url_normalize import cache_shard_key +from apm_cli.deps.github_downloader import GitHubPackageDownloader +from apm_cli.models.apm_package import DependencyReference +from apm_cli.utils.git_sparse import ( + apply_sparse_cone, + repair_dangling_cone_symlinks, + validate_materialized_symlinks, +) +from apm_cli.utils.path_security import PathTraversalError + +pytestmark = pytest.mark.component + + +def _commit_symlink_repo( + tmp_path: Path, + target: str, + *, + package_path: str = "packages/tool", +) -> Path: + """Create a bare repo with a tracked symlink inside the package cone.""" + work = tmp_path / "work" + package = work / package_path + shared = work / "shared" + package.mkdir(parents=True) + shared.mkdir() + (package / "apm.yml").write_text("name: tool\nversion: 1.0.0\n", encoding="utf-8") + (shared / "reference.md").write_text("shared content\n", encoding="utf-8") + (package / "reference.md").symlink_to(target) + subprocess.run(["git", "init", "-q", "-b", "main", str(work)], check=True) + subprocess.run(["git", "-C", str(work), "config", "user.email", "test@example.com"], check=True) + subprocess.run(["git", "-C", str(work), "config", "user.name", "APM Test"], check=True) + subprocess.run(["git", "-C", str(work), "add", "."], check=True) + subprocess.run(["git", "-C", str(work), "commit", "-q", "-m", "fixture"], check=True) + bare = tmp_path / "origin.git" + subprocess.run(["git", "clone", "-q", "--bare", str(work), str(bare)], check=True) + return bare + + +def _checkout_sparse(tmp_path: Path, bare: Path, consumer: Path | None = None) -> Path: + consumer = consumer or tmp_path / "consumer" + consumer.parent.mkdir(parents=True, exist_ok=True) + subprocess.run( + ["git", "clone", "-q", "--no-checkout", str(bare), str(consumer)], + check=True, + ) + apply_sparse_cone("git", consumer, ["packages/tool"], env=os.environ.copy()) + subprocess.run(["git", "-C", str(consumer), "checkout", "-q", "HEAD"], check=True) + return consumer + + +@pytest.mark.skipif(os.name == "nt", reason="Git materializes plain files by default on Windows") +def test_legacy_downloader_repairs_real_out_of_cone_symlink(tmp_path: Path) -> None: + """The no-cache downloader path must widen and return a live package link.""" + bare = _commit_symlink_repo(tmp_path, "../../shared/reference.md") + checkout = tmp_path / "legacy" + downloader = object.__new__(GitHubPackageDownloader) + downloader.git_env = {} + downloader.github_token = None + downloader.auth_resolver = MagicMock() + downloader.auth_resolver.uses_public_github_anonymous_first.return_value = False + downloader._resolve_dep_auth_ctx = lambda dep: None + downloader._build_repo_url = lambda *args, **kwargs: str(bare) + dep = DependencyReference(repo_url="owner/repo", reference="main") + + assert downloader._try_sparse_checkout(dep, checkout, "packages/tool", "main") is True + installed_link = checkout / "packages" / "tool" / "reference.md" + assert installed_link.is_symlink() + assert installed_link.resolve().read_text(encoding="utf-8") == "shared content\n" + assert (checkout / "shared" / "reference.md").is_file() + + +@pytest.mark.skipif(os.name == "nt", reason="Git materializes plain files by default on Windows") +def test_repair_rejects_link_that_remains_broken(tmp_path: Path) -> None: + """Full-tree fallback must explain a link whose target is absent from Git.""" + bare = _commit_symlink_repo(tmp_path, "../../missing/reference.md") + consumer = _checkout_sparse(tmp_path, bare) + + with pytest.raises(PathTraversalError, match="not a tracked file in the checked-out commit"): + repair_dangling_cone_symlinks( + "git", + consumer, + ["packages/tool"], + env=os.environ.copy(), + ) + + +@pytest.mark.skipif(os.name == "nt", reason="Git materializes plain files by default on Windows") +def test_materialization_rejects_symlink_outside_checkout(tmp_path: Path) -> None: + """Remote package copies must remain within the pinned checkout.""" + outside = tmp_path / "outside.txt" + outside.write_text("outside\n", encoding="utf-8") + bare = _commit_symlink_repo(tmp_path, str(outside)) + consumer = _checkout_sparse(tmp_path, bare) + + with pytest.raises(PathTraversalError, match="outside the allowed base directory"): + validate_materialized_symlinks( + "git", + consumer, + ["packages/tool"], + env=os.environ.copy(), + ) + + +@pytest.mark.skipif(os.name == "nt", reason="Git materializes plain files by default on Windows") +def test_persistent_cache_hit_repairs_preexisting_dangling_shard(tmp_path: Path) -> None: + """A cache variant created before #2707 must be repaired when reused.""" + bare = _commit_symlink_repo(tmp_path, "../../shared/reference.md") + sha = subprocess.run( + ["git", "--git-dir", str(bare), "rev-parse", "HEAD"], + check=True, + capture_output=True, + text=True, + ).stdout.strip() + cache_root = tmp_path / "cache" + cache = GitCache(cache_root) + url = bare.as_uri() + checkout = ( + cache_root + / "git" + / "checkouts_v1" + / cache_shard_key(url) + / sha + / _variant_key(["packages/tool"]) + ) + _checkout_sparse(tmp_path, bare, checkout) + link = checkout / "packages" / "tool" / "reference.md" + assert link.is_symlink() + assert not link.exists() + + result = cache.get_checkout( + url, + "main", + locked_sha=sha, + env=os.environ.copy(), + sparse_paths=["packages/tool"], + ) + + assert result == checkout + assert link.resolve().read_text(encoding="utf-8") == "shared content\n" + assert (checkout / "shared" / "reference.md").is_file() + + +@pytest.mark.skipif(os.name == "nt", reason="Git materializes plain files by default on Windows") +def test_invalid_symlink_cleans_persistent_cache_staging(tmp_path: Path) -> None: + """Validation failure must not leave nested incomplete cache shards.""" + outside = tmp_path / "outside.txt" + outside.write_text("outside\n", encoding="utf-8") + bare = _commit_symlink_repo(tmp_path, str(outside)) + sha = subprocess.run( + ["git", "--git-dir", str(bare), "rev-parse", "HEAD"], + check=True, + capture_output=True, + text=True, + ).stdout.strip() + cache_root = tmp_path / "cache" + cache = GitCache(cache_root) + + with pytest.raises(PathTraversalError, match="outside the allowed base directory"): + cache.get_checkout( + bare.as_uri(), + "main", + locked_sha=sha, + env=os.environ.copy(), + sparse_paths=["packages/tool"], + ) + + checkout_root = cache_root / "git" / "checkouts_v1" + assert list(checkout_root.rglob("*.inc.*")) == [] + + +@pytest.mark.skipif(os.name == "nt", reason="Git materializes plain files by default on Windows") +def test_downloader_rejects_real_symlink_into_git_metadata(tmp_path: Path) -> None: + """The user-facing downloader must reject generated Git metadata targets.""" + bare = _commit_symlink_repo(tmp_path, "../../.git/config") + consumer = _checkout_sparse(tmp_path, bare) + downloader = object.__new__(GitHubPackageDownloader) + downloader.install_logger = None + downloader.shared_clone_cache = None + downloader.auth_resolver = MagicMock() + downloader.auth_resolver.uses_public_github_anonymous_first.return_value = False + downloader.persistent_git_cache = MagicMock() + downloader.persistent_git_cache.get_checkout.return_value = consumer + downloader.resolve_git_reference = lambda dep: MagicMock(resolved_commit="a" * 40) + downloader._cache_git_env = lambda dep: os.environ.copy() + downloader._git_env_dict = lambda: os.environ.copy() + dep = DependencyReference( + repo_url="owner/repo", + reference="main", + is_virtual=True, + virtual_path="packages/tool", + ) + target = tmp_path / "installed" + + with pytest.raises(PathTraversalError, match="not a tracked file in the checked-out commit"): + downloader.download_subdirectory_package(dep, target) + + assert not target.exists() + + +@pytest.mark.skipif(os.name == "nt", reason="Git materializes plain files by default on Windows") +def test_downloader_treats_colon_prefixed_package_path_literally(tmp_path: Path) -> None: + """Git pathspec magic must not hide an external package symlink.""" + outside = tmp_path / "outside.txt" + outside.write_text("outside\n", encoding="utf-8") + package_path = ":(literal)pkg" + bare = _commit_symlink_repo( + tmp_path, + str(outside), + package_path=package_path, + ) + consumer = tmp_path / "consumer" + subprocess.run(["git", "clone", "-q", str(bare), str(consumer)], check=True) + downloader = object.__new__(GitHubPackageDownloader) + downloader.install_logger = None + downloader.shared_clone_cache = None + downloader.auth_resolver = MagicMock() + downloader.auth_resolver.uses_public_github_anonymous_first.return_value = False + downloader.persistent_git_cache = MagicMock() + downloader.persistent_git_cache.get_checkout.return_value = consumer + downloader.resolve_git_reference = lambda dep: MagicMock(resolved_commit="a" * 40) + downloader._cache_git_env = lambda dep: os.environ.copy() + downloader._git_env_dict = lambda: os.environ.copy() + dep = DependencyReference( + repo_url="owner/repo", + reference="main", + is_virtual=True, + virtual_path=package_path, + ) + + with pytest.raises(PathTraversalError, match="outside the allowed base directory"): + downloader.download_subdirectory_package(dep, tmp_path / "installed") diff --git a/tests/unit/cache/test_git_cache_sparse.py b/tests/unit/cache/test_git_cache_sparse.py index e122beadc..dfcef2dbe 100644 --- a/tests/unit/cache/test_git_cache_sparse.py +++ b/tests/unit/cache/test_git_cache_sparse.py @@ -2,6 +2,7 @@ from __future__ import annotations +import os import subprocess from pathlib import Path from urllib.parse import urlparse @@ -387,3 +388,104 @@ def reject_auth(cmd, *args, **kwargs): assert len(clone_commands) == 1 assert "--filter=blob:none" in clone_commands[0] + + +def _build_repo_with_out_of_cone_symlink_target(tmp_path: Path) -> tuple[Path, str]: + """Repro shape for #2707: a symlink inside the cone, target outside it.""" + work = tmp_path / "work" + work.mkdir() + subprocess.run(["git", "init", "-q", "-b", "main", str(work)], check=True) + subprocess.run(["git", "-C", str(work), "config", "user.email", "t@e"], check=True) + subprocess.run(["git", "-C", str(work), "config", "user.name", "t"], check=True) + + cone_dir = work / "alpha" / "skill" + cone_dir.mkdir(parents=True) + (cone_dir / "ref.md").write_text("stand-in for a symlink entry\n") + + shared_dir = work / "shared" + shared_dir.mkdir() + (shared_dir / "ref.md").write_text("the real target content\n") + + subprocess.run(["git", "-C", str(work), "add", "."], check=True) + subprocess.run( + ["git", "-C", str(work), "commit", "-q", "-m", "test: init fixture repo"], check=True + ) + sha = subprocess.run( + ["git", "-C", str(work), "rev-parse", "HEAD"], + check=True, + capture_output=True, + text=True, + ).stdout.strip() + + bare = tmp_path / "bare-symlink.git" + subprocess.run(["git", "clone", "-q", "--bare", str(work), str(bare)], check=True) + return bare, sha + + +class TestDanglingSymlinkRepair: + """#2707: GitCache's sparse-cone checkout must not ship a dangling symlink.""" + + def test_dangling_symlink_in_cone_is_repaired(self, tmp_path: Path, monkeypatch): + """A symlink whose target sits outside the requested cone must + still resolve after ``get_checkout`` returns. + + This box can't create a real symlink (WinError 1314) and a real + checkout here writes a mode-120000 entry as a plain file + (``core.symlinks`` defaults to false on this filesystem), so the + ``ref.md`` stand-in is monkeypatched to LOOK dangling the way a + real symlink pointing at ``../../shared/ref.md`` would on a + filesystem that honors ``core.symlinks`` -- same technique as + tests/unit/utils/test_git_sparse.py and + tests/unit/deps/test_bare_cache_sparse.py. + """ + bare, sha = _build_repo_with_out_of_cone_symlink_target(tmp_path) + cache_root = tmp_path / "cache" + cache = GitCache(cache_root) + + checkout_dir = cache_root / "git" / "checkouts_v1" + url = bare.as_uri() + + # The variant shard path isn't known until after get_checkout + # resolves the sha/variant key, but the fake symlink's parent + # (alpha/skill/ref.md) is deterministic once we know the shard + # root, so match on the relative suffix instead. + real_islink = os.path.islink + rel_suffix = Path("alpha") / "skill" / "ref.md" + + def fake_islink(path): + p = Path(path) + return True if p.parts[-3:] == rel_suffix.parts else real_islink(path) + + def fake_exists(path): + p = Path(path) + if p.parts[-3:] == rel_suffix.parts: + return (p.parents[2] / "shared" / "ref.md").exists() + return os.path.lexists(path) + + monkeypatch.setattr(os.path, "islink", fake_islink) + monkeypatch.setattr(os.path, "exists", fake_exists) + monkeypatch.setattr( + "apm_cli.utils.git_sparse._tracked_symlinks", + lambda *args, **kwargs: [Path(args[1]) / "alpha" / "skill" / "ref.md"], + ) + + result = cache.get_checkout(url, "main", locked_sha=sha, sparse_paths=["alpha/skill"]) + + assert checkout_dir in result.parents + # The repair must have actually widened the tree: the previously + # cone-excluded sibling holding the symlink's target now exists. + assert (result / "shared" / "ref.md").is_file() + assert (result / "shared" / "ref.md").read_text() == "the real target content\n" + + def test_no_dangling_symlink_cone_stays_narrow(self, tmp_path: Path): + bare, sha = _build_repo_with_out_of_cone_symlink_target(tmp_path) + cache_root = tmp_path / "cache" + cache = GitCache(cache_root) + + url = bare.as_uri() + result = cache.get_checkout(url, "main", locked_sha=sha, sparse_paths=["alpha/skill"]) + + assert (result / "alpha" / "skill" / "ref.md").is_file() + # No dangling symlink was ever reported (the stand-in is a plain + # file), so the cone must stay narrow -- no repair should fire. + assert not (result / "shared").exists() diff --git a/tests/unit/core/test_public_github_anonymous_first.py b/tests/unit/core/test_public_github_anonymous_first.py index 58c4b709e..9652d89c4 100644 --- a/tests/unit/core/test_public_github_anonymous_first.py +++ b/tests/unit/core/test_public_github_anonymous_first.py @@ -741,6 +741,20 @@ def test_private_github_subdirectory_cache_retries_with_scoped_credential( package_dir = cached_checkout / dep_ref.virtual_path package_dir.mkdir(parents=True) (package_dir / "apm.yml").write_text("name: my-pkg\nversion: 1.0.0\n") + subprocess.run(["git", "init", "-q", "-b", "main", str(cached_checkout)], check=True) + subprocess.run( + ["git", "-C", str(cached_checkout), "config", "user.email", "test@example.com"], + check=True, + ) + subprocess.run( + ["git", "-C", str(cached_checkout), "config", "user.name", "APM Test"], + check=True, + ) + subprocess.run(["git", "-C", str(cached_checkout), "add", "."], check=True) + subprocess.run( + ["git", "-C", str(cached_checkout), "commit", "-q", "-m", "fixture"], + check=True, + ) cache_calls: list[tuple[str, dict[str, object]]] = [] def cache_checkout(url: str, _ref: str, **kwargs: object) -> Path: diff --git a/tests/unit/deps/test_bare_cache_sparse.py b/tests/unit/deps/test_bare_cache_sparse.py index 9b57fa125..ff0b927ca 100644 --- a/tests/unit/deps/test_bare_cache_sparse.py +++ b/tests/unit/deps/test_bare_cache_sparse.py @@ -102,3 +102,88 @@ def test_nonexistent_sparse_subdir_fails_loud_or_empty(tmp_path: Path): assert not (consumer / "plugins").exists() assert not (consumer / "tools").exists() assert not (consumer / "docs").exists() + + +def _build_repo_with_out_of_cone_symlink_target(tmp_path: Path) -> tuple[Path, str]: + """Repro shape for #2707: a symlink inside the cone, target outside it. + + ``plugins/skill/ref.md`` stands in for a symlink whose target lives + in the sibling ``shared/`` dir, which the ``plugins`` cone excludes. + """ + work = tmp_path / "work" + work.mkdir() + subprocess.run(["git", "init", "-q", "-b", "main", str(work)], check=True) + subprocess.run(["git", "-C", str(work), "config", "user.email", "t@e"], check=True) + subprocess.run(["git", "-C", str(work), "config", "user.name", "t"], check=True) + + cone_dir = work / "plugins" / "skill" + cone_dir.mkdir(parents=True) + (cone_dir / "ref.md").write_text("stand-in for a symlink entry\n") + + shared_dir = work / "shared" + shared_dir.mkdir() + (shared_dir / "ref.md").write_text("the real target content\n") + + subprocess.run(["git", "-C", str(work), "add", "."], check=True) + subprocess.run( + ["git", "-C", str(work), "commit", "-q", "-m", "test: init fixture repo"], check=True + ) + sha = subprocess.run( + ["git", "-C", str(work), "rev-parse", "HEAD"], + check=True, + capture_output=True, + text=True, + ).stdout.strip() + + bare = tmp_path / "bare-symlink.git" + subprocess.run(["git", "clone", "-q", "--bare", str(work), str(bare)], check=True) + return bare, sha + + +def test_dangling_symlink_in_cone_is_repaired(tmp_path: Path, monkeypatch): + """#2707: a symlink whose target sits outside the requested cone must + still resolve after ``materialize_from_bare`` returns. + + This box can't create a real symlink (WinError 1314) and a real + checkout here writes a mode-120000 entry as a plain file (verified: + ``core.symlinks`` defaults to false on this filesystem), so the + ``ref.md`` stand-in above is monkeypatched to LOOK dangling the way + a real symlink pointing at ``../../shared/ref.md`` would on a + filesystem that honors ``core.symlinks`` -- see + tests/unit/utils/test_git_sparse.py for the same technique applied + to the detection helper in isolation. + """ + bare, sha = _build_repo_with_out_of_cone_symlink_target(tmp_path) + consumer = tmp_path / "consumer" + + fake_link = consumer / "plugins" / "skill" / "ref.md" + real_islink = os.path.islink + + def fake_islink(path): + return True if Path(path) == fake_link else real_islink(path) + + def fake_exists(path): + if Path(path) == fake_link: + return (consumer / "shared" / "ref.md").exists() + return os.path.lexists(path) + + monkeypatch.setattr(os.path, "islink", fake_islink) + monkeypatch.setattr(os.path, "exists", fake_exists) + monkeypatch.setattr( + "apm_cli.utils.git_sparse._tracked_symlinks", + lambda *args, **kwargs: [fake_link], + ) + + materialize_from_bare( + bare, + consumer, + ref=None, + env=os.environ.copy(), + known_sha=sha, + sparse_paths=["plugins/skill"], + ) + + # The would-be symlink's target must be reachable: the fallback + # widened the tree instead of leaving it dangling. + assert (consumer / "shared" / "ref.md").is_file() + assert (consumer / "shared" / "ref.md").read_text() == "the real target content\n" diff --git a/tests/unit/deps/test_github_downloader_error_handling.py b/tests/unit/deps/test_github_downloader_error_handling.py index 974f114b7..9988a0edb 100644 --- a/tests/unit/deps/test_github_downloader_error_handling.py +++ b/tests/unit/deps/test_github_downloader_error_handling.py @@ -903,6 +903,7 @@ def fake_materialize(bare: Path, consumer: Path, **kwargs: Any) -> str: patch("apm_cli.deps.github_downloader.tempfile.mkdtemp", return_value=str(tmp_path)), patch("apm_cli.deps.github_downloader._rmtree"), patch("apm_cli.utils.path_security.ensure_path_within"), + patch("apm_cli.deps.github_downloader.validate_materialized_symlinks"), patch("apm_cli.deps.github_downloader.validate_apm_package", return_value=validation), patch("apm_cli.deps.package_validator.stamp_plugin_version"), patch("apm_cli.utils.file_ops.robust_copytree"), diff --git a/tests/unit/deps/test_github_downloader_phase3.py b/tests/unit/deps/test_github_downloader_phase3.py index 9050f3d49..c281c1414 100644 --- a/tests/unit/deps/test_github_downloader_phase3.py +++ b/tests/unit/deps/test_github_downloader_phase3.py @@ -904,6 +904,7 @@ def fake_materialize(bare: Path, consumer: Path, **kwargs: Any) -> str: patch("apm_cli.deps.github_downloader.tempfile.mkdtemp", return_value=str(tmp_path)), patch("apm_cli.deps.github_downloader._rmtree"), patch("apm_cli.utils.path_security.ensure_path_within"), + patch("apm_cli.deps.github_downloader.validate_materialized_symlinks"), patch("apm_cli.deps.github_downloader.validate_apm_package", return_value=validation), patch("apm_cli.deps.package_validator.stamp_plugin_version"), patch("apm_cli.utils.file_ops.robust_copytree"), @@ -974,6 +975,38 @@ def test_returns_true_on_success( result = downloader._try_sparse_checkout(dep, tmp_path / "sparse", "skills/foo", "main") assert result is True + def test_repair_logs_via_verbose_channel( + self, downloader: GitHubPackageDownloader, tmp_path: Path + ) -> None: + """Successful widening must use logging configured by --verbose.""" + dep = _make_dep() + downloader._strategies.build_repo_url = MagicMock(return_value="https://github.com/o/r") + downloader.git_env = {} + ctx = MagicMock() + ctx.auth_scheme = "basic" + ctx.git_env = {} + downloader.auth_resolver.resolve_for_dep.return_value = ctx + downloader.auth_resolver.git_env_for_context.side_effect = lambda auth_ctx, *, base_env: { + **base_env, + **auth_ctx.git_env, + } + clone_path = tmp_path / "sparse" + dangling = clone_path / "skills" / "foo" / "reference.md" + ok_result = MagicMock(returncode=0) + + with ( + patch("apm_cli.deps.github_downloader.subprocess.run", return_value=ok_result), + patch( + "apm_cli.deps.github_downloader.repair_dangling_cone_symlinks", + return_value=dangling, + ), + patch("apm_cli.deps.github_downloader._log.info") as log_info, + ): + result = downloader._try_sparse_checkout(dep, clone_path, "skills/foo", "main") + + assert result is True + log_info.assert_called_once() + def test_bearer_auth_scheme_uses_dep_auth_ctx_git_env( self, downloader: GitHubPackageDownloader, tmp_path: Path ) -> None: diff --git a/tests/unit/deps/test_shared_clone_cache.py b/tests/unit/deps/test_shared_clone_cache.py index 94a7f016e..2c51bc260 100644 --- a/tests/unit/deps/test_shared_clone_cache.py +++ b/tests/unit/deps/test_shared_clone_cache.py @@ -313,6 +313,7 @@ def fake_materialize(bare_path, consumer_dir, **kwargs): 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_materialized_symlinks"), patch("apm_cli.deps.github_downloader.validate_apm_package") as mock_validate, ): mock_result = MagicMock() @@ -385,6 +386,7 @@ def fake_materialize(bare_path, consumer_dir, **kwargs): 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_materialized_symlinks"), patch("apm_cli.deps.github_downloader.validate_apm_package") as mock_validate, ): # Configure validate mock diff --git a/tests/unit/utils/test_git_sparse.py b/tests/unit/utils/test_git_sparse.py new file mode 100644 index 000000000..d2bf59c58 --- /dev/null +++ b/tests/unit/utils/test_git_sparse.py @@ -0,0 +1,126 @@ +"""Tests for the sparse-cone dangling-symlink repair helper (#2707). + +Sparse-cone checkout (perf #1433) only materializes the requested +top-level paths. A repo whose payload contains a symlink pointing +OUTSIDE those paths ends up with a dangling symlink once checked +out on a filesystem that honors ``core.symlinks`` -- the entry itself +is inside the cone (so it gets checked out) but its target is not, so +any code that later dereferences it (a plain ``open()``, +``shutil.copytree`` without ``symlinks=True``) fails with +``FileNotFoundError``. + +The platform-neutral unit tests use a stand-in entry so they also run +where Git has ``core.symlinks=false``. The real-symlink consumer path is +covered in ``tests/integration/test_sparse_cone_symlink_repair.py``. +""" + +from __future__ import annotations + +import os +import subprocess +from pathlib import Path + +from apm_cli.utils.git_sparse import ( + apply_sparse_cone, + repair_dangling_cone_symlinks, +) + + +def _build_local_bare_repo(tmp_path: Path) -> tuple[Path, str]: + """Repo shaped like the #2707 repro: a cone dir plus an outside sibling.""" + work = tmp_path / "work" + work.mkdir() + subprocess.run(["git", "init", "-q", "-b", "main", str(work)], check=True) + subprocess.run(["git", "-C", str(work), "config", "user.email", "t@e"], check=True) + subprocess.run(["git", "-C", str(work), "config", "user.name", "t"], check=True) + + cone_dir = work / "skills" / "better-writing" / "references" + cone_dir.mkdir(parents=True) + (cone_dir / "genre-tells.md").write_text("stand-in for a symlink entry\n") + (work / "skills" / "better-writing" / "SKILL.md").write_text("skill\n") + + outside_dir = work / "references" + outside_dir.mkdir() + (outside_dir / "genre-tells.md").write_text("the real target content\n") + + subprocess.run(["git", "-C", str(work), "add", "."], check=True) + subprocess.run( + ["git", "-C", str(work), "commit", "-q", "-m", "test: init fixture repo"], check=True + ) + sha = subprocess.run( + ["git", "-C", str(work), "rev-parse", "HEAD"], + check=True, + capture_output=True, + text=True, + ).stdout.strip() + + bare = tmp_path / "bare.git" + subprocess.run(["git", "clone", "-q", "--bare", str(work), str(bare)], check=True) + return bare, sha + + +def _checkout_cone(tmp_path: Path, bare: Path, consumer_name: str) -> Path: + consumer = tmp_path / consumer_name + subprocess.run( + ["git", "clone", "-q", "--local", "--shared", "--no-checkout", str(bare), str(consumer)], + check=True, + ) + apply_sparse_cone("git", consumer, ["skills/better-writing"], env=os.environ.copy()) + subprocess.run(["git", "-C", str(consumer), "checkout", "-q", "HEAD"], check=True) + return consumer + + +class TestRepairDanglingConeSymlinks: + def test_no_dangling_symlink_leaves_cone_untouched(self, tmp_path: Path): + bare, _sha = _build_local_bare_repo(tmp_path) + consumer = _checkout_cone(tmp_path, bare, "consumer-clean") + + result = repair_dangling_cone_symlinks( + "git", consumer, ["skills/better-writing"], env=os.environ.copy() + ) + + assert result is None + # Cone stays narrow: the outside sibling was never materialized. + assert not (consumer / "references").exists() + + def test_dangling_symlink_falls_back_to_full_checkout(self, tmp_path: Path, monkeypatch): + bare, _sha = _build_local_bare_repo(tmp_path) + consumer = _checkout_cone(tmp_path, bare, "consumer-dangling") + + fake_link = consumer / "skills" / "better-writing" / "references" / "genre-tells.md" + assert fake_link.is_file() # the stand-in checked out fine + + real_islink = os.path.islink + + def fake_islink(path): + return True if Path(path) == fake_link else real_islink(path) + + def fake_exists(path): + if Path(path) == fake_link: + return (consumer / "references" / "genre-tells.md").exists() + return os.path.lexists(path) + + monkeypatch.setattr(os.path, "islink", fake_islink) + monkeypatch.setattr(os.path, "exists", fake_exists) + monkeypatch.setattr( + "apm_cli.utils.git_sparse._tracked_symlinks", + lambda *args, **kwargs: [fake_link], + ) + + result = repair_dangling_cone_symlinks( + "git", consumer, ["skills/better-writing"], env=os.environ.copy() + ) + + assert result == fake_link + # The repair must have actually widened the tree, not just + # reported the problem: the previously cone-excluded sibling + # directory holding the symlink's target now exists. + assert (consumer / "references" / "genre-tells.md").is_file() + assert ( + consumer / "references" / "genre-tells.md" + ).read_text() == "the real target content\n" + + def test_empty_paths_is_a_noop(self, tmp_path: Path): + bare, _sha = _build_local_bare_repo(tmp_path) + consumer = _checkout_cone(tmp_path, bare, "consumer-empty-paths") + assert repair_dangling_cone_symlinks("git", consumer, [], env=os.environ.copy()) is None