diff --git a/CHANGELOG.md b/CHANGELOG.md index e77c0d7435..927bea792b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,12 +7,18 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Changed + +- **BREAKING:** `apm install` now exits `1` whenever the diagnostic summary reports `Installation failed with N error(s)`. Previously the command exited `0` even after reporting errors, so CI could not detect failure via exit code. `--force` continues to bypass only the security scan's critical-finding block; it does **not** suppress general install errors (matches `npm` / `pip` / `cargo`). Callers that asserted `exit_code == 0` while errors were reported must update. + ### Added +- `ref:` on git-source dependencies now accepts semver ranges (`^1.2.0`, `~1.4`, `>=2.0 <3`, `1.5.x`). `apm install` runs `git ls-remote`, picks the highest tag matching the range, and pins the resolved tag, commit SHA, version, and original constraint in `apm.lock.yaml`. Subsequent installs replay the lockfile without network; use `apm install --update` to re-resolve against current remote tags. Two tag patterns are tried in order (`v{version}`, `{name}--v{version}`) with a bare `{version}` fallback. (closes #1488) - `apm deps why ` explains why a transitive dependency is installed by walking the lockfile's `resolved_by` chain back to the user's direct declaration in `apm.yml`. Supports `--global` for user-scope lockfiles and `--json` for scriptable output (JSON to stdout, all logs to stderr; analogue of `npm why` / `yarn why`). Exits `0` on success, `1` when the package isn't installed or the query is ambiguous, `2` when no lockfile exists. (#1490) ### Fixed +- `apm install --update` now re-resolves direct git-source semver dependencies. Previously, when the dependency's install path already existed on disk, the BFS resolver short-circuited and `--update` was a silent no-op for git-semver refs; the lockfile kept the previously-resolved tag. - `policy.dependencies.require_pinned_constraint: true` no longer misclassifies the npm- and cargo-style explicit-equality form `=1.2.3` as `BARE_BRANCH`. Both `1.2.3` and `=1.2.3` are now recognized as pinned constraints; the pip-style `==1.2.3` form is still rejected (not part of node-semver). Follow-up to #1494 / #1505. ## [0.15.0] - 2026-05-27 diff --git a/docs/src/content/docs/consumer/manage-dependencies.md b/docs/src/content/docs/consumer/manage-dependencies.md index 88c02ab793..54d3dd6896 100644 --- a/docs/src/content/docs/consumer/manage-dependencies.md +++ b/docs/src/content/docs/consumer/manage-dependencies.md @@ -155,6 +155,28 @@ SHAs. The lockfile pins the resolved commit either way, so two clones running `apm install` get the same bytes -- but a branch ref will resolve to a new SHA on the next `apm update`. +### Pin a semver range + +For git-source dependencies you can also pin a semver range as the ref. +APM resolves the range against the remote's tags at install time and +records the concrete tag in the lockfile: + +```yaml +dependencies: + apm: + - acme/widget#^1.2.0 # any 1.x >= 1.2.0 + - acme/widget#~1.4 # any 1.4.x + - acme/widget#>=2.0 <3 # explicit range + - acme/widget#1.5.x # wildcard +``` + +APM matches tags against `v{version}` and `{name}--v{version}` patterns +(with `{version}` as a bare-tag fallback) and picks the highest tag that +satisfies the range. The original constraint is preserved in the +lockfile alongside the resolved tag, so `apm install` on a fresh clone +replays the same tag deterministically. Only `apm install --update` or a +manifest change re-resolves to a newer tag. + ## Remove a dependency 1. Delete the entry from `apm.yml`. diff --git a/docs/src/content/docs/reference/cli/install.md b/docs/src/content/docs/reference/cli/install.md index b807bc8dbc..b24ae96cf4 100644 --- a/docs/src/content/docs/reference/cli/install.md +++ b/docs/src/content/docs/reference/cli/install.md @@ -32,7 +32,7 @@ With no arguments it installs everything from `apm.yml`. With one or more `PACKA | `--update` | off | Re-resolve dependencies to the latest Git ref allowed by `apm.yml` and rewrite `apm.lock.yaml`. Mutually exclusive with `--frozen`. Prefer the dedicated [`apm update`](../update/) command for the consent-gated workflow. | | `--frozen` | off | Lockfile-only install: refuse to resolve anything new and fail if `apm.yml` and `apm.lock.yaml` have drifted. Mirrors `npm ci`. Mutually exclusive with `--update`. | | `--dry-run` | off | Print the install plan without touching the filesystem. | -| `--force` | off | Overwrite locally-authored files on collision **and** bypass the security scan's critical-finding block. Does **not** refresh remote refs -- use `apm update` for that. Use only after independent verification. | +| `--force` | off | Overwrite locally-authored files on collision **and** bypass the security scan's critical-finding block. Does **not** suppress general install errors (any reported error still exits `1`, matching npm / pip / cargo). Does **not** refresh remote refs -- use `apm update` for that. Use only after independent verification. | | `--verbose`, `-v` | off | Show per-file paths and full error context in the diagnostic summary. | | `--dev` | off | Add new packages to `devDependencies`. Dev deps install locally but are excluded from `apm pack` output. | @@ -91,6 +91,7 @@ Transport env vars: `APM_GIT_PROTOCOL` (`ssh` or `https`) sets the default initi - **Auto-bootstrap.** `apm install ` with no `apm.yml` creates a minimal one. Bare `apm install` with no `apm.yml` exits with a hint to run `apm init` or `apm install `. - **Diff-aware.** Packages whose ref or version changed in `apm.yml` are re-downloaded automatically; `--update` is only needed to pull a newer ref under a floating constraint. MCP servers with matching config are skipped (`already configured`); changed config is re-applied (`updated`). +- **Semver ranges on git deps.** `ref:` accepts semver ranges (`^1.2.0`, `~1.4`, `>=2.0 <3`, `1.5.x`) for git-source deps. APM runs `git ls-remote` against the dep, picks the highest tag matching the range, and pins the resolved tag plus commit SHA, version, and original constraint in `apm.lock.yaml`. Subsequent installs replay the lockfile without network; use `--update` (or change the manifest constraint) to re-resolve. See [manage dependencies](../../../consumer/manage-dependencies/#pin-a-semver-range) for the supported syntax. - **No-op nudge.** When the lockfile is already satisfied and nothing needs deploying, install prints `[i] Run 'apm update' to check for newer versions.` so you know the silent success was not a missed refresh. - **Frozen mode.** With `--frozen`, install resolves only what is in `apm.lock.yaml`. A direct dependency missing from the lockfile, or a missing lockfile entirely, exits `1`. Orphan lockfile entries (locked but no longer in `apm.yml`) are tolerated; local-path deps are skipped. This is a structural check, not a content check -- run `apm audit --ci` for hash verification. - **Local `.apm/` deployment.** After dependencies are integrated, primitives in the project's own `.apm/` directory are deployed to the same targets. Local files win on collision. Skipped at `--global` and with `--only mcp`. @@ -172,12 +173,12 @@ apm install owner/skill-bundle --skill '*' # reset to all skills | Code | Meaning | |---|---| | `0` | Success. All requested dependencies and local content deployed. | -| `1` | Install failure: security scan blocked a critical finding, auth error, manifest write error, dependency resolution error, `--frozen` with a missing lockfile or a direct dependency absent from `apm.lock.yaml`, or unhandled exception. The diagnostic summary names the cause. | +| `1` | Install failure: security scan blocked a critical finding, auth error, manifest write error, dependency resolution error, `--frozen` with a missing lockfile or a direct dependency absent from `apm.lock.yaml`, any reported install error (the diagnostic summary closes with `Installation failed with N error(s)`), or unhandled exception. `--force` does **not** suppress general install errors. The diagnostic summary names the cause. | | `2` | Usage error: no deployment target detectable (no `--target`, no `targets:` in `apm.yml`, no harness signal in the project), `--ssh` and `--https` both passed, `--frozen` and `--update` both passed, or a Click flag conflict. | ## Notes -- **`--force` is dual-purpose.** It overwrites locally-authored files on collision **and** disables the critical-finding block from the built-in security scan. It does **not** refresh remote refs -- for routine ref updates, run [`apm update`](../update/). To remediate findings, prefer `apm audit --strip`. See [Drift and secure by default](../../../consumer/drift-and-secure-by-default/). +- **`--force` is dual-purpose.** It overwrites locally-authored files on collision **and** disables the critical-finding block from the built-in security scan. It does **not** suppress general install errors -- any error reported in the diagnostic summary still exits `1` (matches `npm` / `pip` / `cargo`). It does **not** refresh remote refs -- for routine ref updates, run [`apm update`](../update/). To remediate findings, prefer `apm audit --strip`. See [Drift and secure by default](../../../consumer/drift-and-secure-by-default/). - **Claude target prompt rewrite.** When deploying to `.claude/commands/`, prompt files with an `input:` front-matter key are rewritten to Claude's `arguments:` shape and `${input:name}` placeholders become `$name`. Argument names must match `^[A-Za-z][\w-]{0,63}$`; rejected names are dropped with a warning. - **Copilot CLI env-var passthrough.** When deploying MCP entries to `~/.copilot/mcp-config.json`, `${env:VAR}` and `` placeholders are translated to `${VAR}` so Copilot CLI resolves them at server-start. Plaintext secrets are never written to disk. Other targets currently resolve placeholders at install time. @@ -208,12 +209,12 @@ See [Private registries](../../../guides/private-registries/) for the full setup | Code | Meaning | |---|---| | `0` | Success. All requested dependencies and local content deployed. | -| `1` | Install failure: security scan blocked a critical finding, auth error, manifest write error, dependency resolution error, `--frozen` with a missing lockfile or a direct dependency absent from `apm.lock.yaml`, or unhandled exception. The diagnostic summary names the cause. | +| `1` | Install failure: security scan blocked a critical finding, auth error, manifest write error, dependency resolution error, `--frozen` with a missing lockfile or a direct dependency absent from `apm.lock.yaml`, any reported install error (the diagnostic summary closes with `Installation failed with N error(s)`), or unhandled exception. `--force` does **not** suppress general install errors. The diagnostic summary names the cause. | | `2` | Usage error: no deployment target detectable (no `--target`, no `targets:` in `apm.yml`, no harness signal in the project), `--ssh` and `--https` both passed, `--frozen` and `--update` both passed, or a Click flag conflict. | ## Notes -- **`--force` is dual-purpose.** It overwrites locally-authored files on collision **and** disables the critical-finding block from the built-in security scan. It does **not** refresh remote refs -- for routine ref updates, run [`apm update`](../update/). To remediate findings, prefer `apm audit --strip`. See [Drift and secure by default](../../../consumer/drift-and-secure-by-default/). +- **`--force` is dual-purpose.** It overwrites locally-authored files on collision **and** disables the critical-finding block from the built-in security scan. It does **not** suppress general install errors -- any error reported in the diagnostic summary still exits `1` (matches `npm` / `pip` / `cargo`). It does **not** refresh remote refs -- for routine ref updates, run [`apm update`](../update/). To remediate findings, prefer `apm audit --strip`. See [Drift and secure by default](../../../consumer/drift-and-secure-by-default/). - **Claude target prompt rewrite.** When deploying to `.claude/commands/`, prompt files with an `input:` front-matter key are rewritten to Claude's `arguments:` shape and `${input:name}` placeholders become `$name`. Argument names must match `^[A-Za-z][\w-]{0,63}$`; rejected names are dropped with a warning. - **Copilot CLI env-var passthrough.** When deploying MCP entries to `~/.copilot/mcp-config.json`, `${env:VAR}` and `` placeholders are translated to `${VAR}` so Copilot CLI resolves them at server-start. Plaintext secrets are never written to disk. Other targets currently resolve placeholders at install time. diff --git a/docs/src/content/docs/reference/lockfile-spec.md b/docs/src/content/docs/reference/lockfile-spec.md index 1093075487..3796eb5a64 100644 --- a/docs/src/content/docs/reference/lockfile-spec.md +++ b/docs/src/content/docs/reference/lockfile-spec.md @@ -130,6 +130,9 @@ Each item in `dependencies` describes one resolved package. | `marketplace_plugin_name` | string | no | Plugin name as listed in that marketplace. | | `is_insecure` | bool | no | `true` when the source URL was `http://`. | | `allow_insecure` | bool | no | `true` when the manifest explicitly opted in to the insecure source. | +| `constraint` | string | git-source semver only | The original semver range from `apm.yml` (`^1.2.0`, `~1.4`). Present when `ref:` was a range; used by drift detection so a manifest range vs. a locked tag (`v1.5.3`) is not a false positive, and by lockfile replay to pin the resolved tag deterministically across installs. | +| `resolved_tag` | string | git-source semver only | The concrete git tag (`v1.5.3`, `widget--v1.5.3`) that satisfied `constraint`. | +| `resolved_at` | string | git-source semver only | RFC 3339 timestamp of the resolution. Surfaces "how stale is this pin?" in `apm why`. | Fields are emitted only when set. A minimal entry is just `repo_url` plus `resolved_commit`. diff --git a/docs/src/content/docs/reference/manifest-schema.md b/docs/src/content/docs/reference/manifest-schema.md index ac25da5794..91e84af649 100644 --- a/docs/src/content/docs/reference/manifest-schema.md +++ b/docs/src/content/docs/reference/manifest-schema.md @@ -376,6 +376,10 @@ Remote dependency (git URL plus sub-path): alias: acme-sec ``` +`ref:` accepts either a literal git ref (`main`, `v2.0`, a 40-char commit SHA) or a **semver range** (`^1.2.0`, `~1.4`, `>=2.0 <3`, `1.5.x`). When `ref:` is a semver range, APM resolves it against the remote's tags at install time, matching against `v{version}` and `{name}--v{version}` patterns (with `{version}` as a bare-tag fallback) and selecting the highest tag that satisfies the range. + +The lockfile records the original `constraint`, the `resolved_tag`, the resolved `version`, the `resolved_commit`, and a `resolved_at` timestamp so subsequent installs replay the same tag deterministically -- only `apm install --update` or a manifest change re-resolves. When no remote tag satisfies the range, APM surfaces `NoMatchingTagError` with the inspected patterns. + Local path dependency (development only): ```yaml diff --git a/packages/apm-guide/.apm/skills/apm-usage/dependencies.md b/packages/apm-guide/.apm/skills/apm-usage/dependencies.md index 92b33c816f..11ad4f2d02 100644 --- a/packages/apm-guide/.apm/skills/apm-usage/dependencies.md +++ b/packages/apm-guide/.apm/skills/apm-usage/dependencies.md @@ -204,11 +204,22 @@ dependencies: | Strategy | Syntax | When to use | |----------|--------|-------------| | Tag | `owner/repo#v1.0.0` | Production -- immutable reference | +| Semver range | `owner/repo#^1.2.0` | Track patch/minor updates within a range; APM lists remote tags and pins the highest match in the lockfile | | Branch | `owner/repo#main` | Development -- tracks latest | | Commit SHA | `owner/repo#abc123d` | Maximum reproducibility | | No ref | `owner/repo` | Resolves default branch at install time | | Marketplace ref | `plugin@marketplace#ref` | Override marketplace source ref | +Semver ranges accept `^1.2.0`, `~1.4`, `>=2.0 <3`, or `1.5.x`. At +install time APM runs `git ls-remote` against the dep and picks the +highest tag matching the range; the resolved tag, commit SHA, version, +and original constraint are pinned in the lockfile. Subsequent +`apm install` runs replay the lockfile without network. Use +`apm install --update` (or change the manifest constraint) to +re-resolve against current remote tags. Two tag patterns are tried in +order: `v{version}` and `{name}--v{version}`, then a bare `{version}` +fallback. + ## Marketplace ref override When installing from a marketplace, the `#` suffix overrides the `source.ref` from the marketplace entry: diff --git a/src/apm_cli/deps/git_semver_resolver.py b/src/apm_cli/deps/git_semver_resolver.py new file mode 100644 index 0000000000..559ad3ee63 --- /dev/null +++ b/src/apm_cli/deps/git_semver_resolver.py @@ -0,0 +1,348 @@ +"""Resolve semver ranges on git-source dependencies against repo tags. + +This module is the git-source counterpart of the registry/marketplace +semver resolvers. When an author writes ``acme/foo#^1.2.0`` (string +shorthand) or ``ref: ^1.2.0`` (object form) in ``apm.yml``, the install +pipeline calls :class:`GitSemverResolver` to map the constraint to a +concrete tag on the remote. + +Resolution algorithm +-------------------- +1. Call :meth:`apm_cli.marketplace.ref_resolver.RefResolver.list_remote_refs` + to enumerate the remote's refs (cached for 5 minutes). +2. Filter tag refs through each tag pattern in order + (``DEFAULT_TAG_PATTERNS``: ``v{version}`` then ``{name}--v{version}``, + followed by a bare ``{version}`` fallback). +3. Discard pre-release versions unless ``include_prerelease=True``. +4. Of all remaining candidates across all patterns, pick the highest + :class:`~apm_cli.marketplace.semver.SemVer` that satisfies the + constraint. + +The module purposefully avoids any new transport or cache code -- it +composes ``RefResolver`` (transport + cache), ``tag_pattern.build_tag_regex`` +(pattern matching) and ``marketplace.semver`` (version arithmetic). +""" + +from __future__ import annotations + +from collections.abc import Sequence +from dataclasses import dataclass +from datetime import datetime, timezone + +from ..marketplace.ref_resolver import RefResolver, RemoteRef +from ..marketplace.semver import SemVer, parse_semver, satisfies_range +from ..marketplace.tag_pattern import build_tag_regex + +__all__ = [ + "DEFAULT_TAG_PATTERNS", + "FALLBACK_BARE_PATTERN", + "GitSemverResolution", + "GitSemverResolver", + "NoMatchingTagError", + "iter_semver_tags", + "utc_now_iso", +] + +# Default tag-pattern fallback order. +# +# The two-pattern default mirrors the conventions APM already commits to +# elsewhere: +# +# * ``v{version}`` -- universal lockstep convention (most projects). +# * ``{name}--v{version}`` -- Claude Code / PR #1422 per-package convention +# used by multi-marketplace repos. Double dash is intentional and +# matches Claude's published convention; single-dash variants are NOT +# tried by default to avoid silent collisions with branch names like +# ``my-skills-v1`` (a hand-cut release branch). +DEFAULT_TAG_PATTERNS: tuple[str, ...] = ("v{version}", "{name}--v{version}") + +# Bare-version fallback (``1.2.3`` literal, no prefix). +# +# Only consulted when neither default pattern produced a match. Without +# this fallback, an author who tags releases as ``1.2.3`` (no leading +# ``v``) and writes ``ref: 1.2.3`` would see ``NoMatchingTagError`` +# despite the tag existing -- because ``1.2.3`` parses as a semver +# *range* (exact-version match) and would never round-trip through the +# branch/literal-tag path. Tested by +# ``resolve_bare_version_tag_falls_through_to_third_pattern``. +FALLBACK_BARE_PATTERN: str = "{version}" + +_REFS_TAGS_PREFIX = "refs/tags/" + + +def utc_now_iso() -> str: + """Return the current UTC time as an ISO-8601 string (no microseconds). + + Centralised so :class:`GitSemverResolution` and tests share a single + spelling. Kept out of :mod:`datetime` import sites to make the + timestamp easy to stub in tests. + """ + return datetime.now(timezone.utc).replace(microsecond=0).isoformat() + + +class NoMatchingTagError(Exception): + """Raised when no tag on the remote satisfies the requested constraint. + + Carries an actionable hint -- the resolver lists the patterns that + were tried and (when available) the highest version it did find so + authors can widen the range or pin a literal tag. + """ + + def __init__(self, summary: str, hint: str) -> None: + super().__init__(f"{summary}\n{hint}" if hint else summary) + self.summary = summary + self.hint = hint + + +@dataclass(frozen=True) +class GitSemverResolution: + """One git-source semver resolution result. + + Attributes + ---------- + constraint: + The original spec the author wrote (e.g. ``"^1.2.0"``). Preserved + verbatim into the lockfile so ``apm install`` is reproducible + and ``apm update`` knows what to widen. + resolved_version: + Concrete version string the constraint resolved to (e.g. ``"1.5.3"``). + resolved_tag: + Concrete tag name on the remote (e.g. ``"v1.5.3"``). + resolved_sha: + 40-char hex SHA the tag points to. + matched_pattern: + The tag pattern that produced the winning tag (one of + ``DEFAULT_TAG_PATTERNS`` or ``FALLBACK_BARE_PATTERN``). + resolved_at: + ISO-8601 UTC timestamp captured at resolution time. Surfaced in + the lockfile so audits can answer "when was this tag picked?" + without re-running ``git ls-remote``. + """ + + constraint: str + resolved_version: str + resolved_tag: str + resolved_sha: str + matched_pattern: str + resolved_at: str + + +def iter_semver_tags( + refs: Sequence[RemoteRef], + *, + package_name: str, + patterns: Sequence[str], +) -> list[tuple[SemVer, str, str, str]]: + """Yield ``(version, tag_name, sha, pattern)`` for tags matching any pattern. + + Each tag may match multiple patterns; in that case it appears once + per matching pattern. Callers downstream pick the highest version + across all matches, so duplicates resolve to the same concrete tag + even when patterns overlap. + + ``{name}`` placeholders in *patterns* are expanded with the literal + *package_name* (regex-escaped) before regex compilation, so that + a pattern like ``{name}--v{version}`` matches only tags scoped to + the expected package (e.g. ``some-skills--v1.2.0``) and does not + accept tags belonging to other packages (e.g. ``otherpkg--v9.9.9``) + in repositories that publish multiple ``{name}--v{version}`` tag + families. See issue #1488 review thread. + + Non-tag refs (``refs/heads/*``) and peeled-tag refs are skipped. + """ + expanded = [pat.replace("{name}", package_name) for pat in patterns] + compiled = [(pat, build_tag_regex(exp)) for pat, exp in zip(patterns, expanded, strict=True)] + out: list[tuple[SemVer, str, str, str]] = [] + for ref in refs: + if not ref.name.startswith(_REFS_TAGS_PREFIX): + continue + tag_name = ref.name[len(_REFS_TAGS_PREFIX) :] + for pattern, rx in compiled: + # ``build_tag_regex`` receives a pattern where any ``{name}`` + # has already been substituted with the literal package_name, + # so the resulting regex is scoped to this package's tag + # family and never accepts tags belonging to a sibling + # package in the same repository. + m = rx.match(tag_name) + if not m: + continue + try: + version_str = m.group("version") + except (IndexError, KeyError): + continue + v = parse_semver(version_str) + if v is None: + continue + out.append((v, tag_name, ref.sha, pattern)) + # Allow same tag to match multiple patterns; the picker + # de-duplicates by (version, sha). + return out + + +class GitSemverResolver: + """Resolve a semver constraint against a remote's git tags. + + Composition only: holds a :class:`RefResolver` for transport + cache + and applies the pattern + semver logic on top. Stateless besides + the underlying ref cache. + + Parameters + ---------- + ref_resolver: + The :class:`RefResolver` to use for ``git ls-remote`` calls. + include_prerelease: + When ``True``, prerelease versions are eligible. Defaults to + ``False`` (mirrors the marketplace resolver's default). + """ + + def __init__( + self, + ref_resolver: RefResolver, + *, + include_prerelease: bool = False, + ) -> None: + self._ref_resolver = ref_resolver + self._include_prerelease = include_prerelease + + @property + def include_prerelease(self) -> bool: + """Whether prerelease versions are eligible for resolution.""" + return self._include_prerelease + + def resolve( + self, + *, + owner_repo: str, + package_name: str, + constraint: str, + tag_patterns: Sequence[str] = DEFAULT_TAG_PATTERNS, + now_iso: str | None = None, + ) -> GitSemverResolution: + """Resolve *constraint* to a concrete tag on ``owner_repo``. + + Parameters + ---------- + owner_repo: + ``"owner/repo"`` string (no host, no ``.git`` suffix). + package_name: + Package name used to expand ``{name}`` placeholders in + patterns. Conventionally the trailing path segment of + ``owner_repo``. + constraint: + Semver range (e.g. ``"^1.2.0"``, ``"~2.1"``, ``">=1.0 <2.0"``) + or exact version (``"1.2.3"``). + tag_patterns: + Ordered tag patterns to try. Defaults to + :data:`DEFAULT_TAG_PATTERNS`. A bare-version fallback + (:data:`FALLBACK_BARE_PATTERN`) is appended automatically + only when no candidate matches the user-supplied patterns + (see the "Risks" discussion in issue #1488). + now_iso: + Override for the ISO-8601 ``resolved_at`` field; tests use + this to keep assertions deterministic. + + Returns + ------- + GitSemverResolution + Constraint + winning tag + version + SHA + matched pattern. + + Raises + ------ + NoMatchingTagError + When no tag on the remote satisfies the constraint after + trying the default patterns and the bare-version fallback. + """ + refs = self._ref_resolver.list_remote_refs(owner_repo) + primary_patterns = tuple(tag_patterns) + # Two-pass: try the author-supplied patterns first; only if they + # find zero candidates do we widen to the bare-version fallback. + # That preserves the "literal 1.2.3 tag" edge case without + # silently promoting bare numeric tags to first-class status. + winner = self._pick_best(refs, package_name, primary_patterns, constraint) + if winner is None: + fallback_patterns = (FALLBACK_BARE_PATTERN,) + winner = self._pick_best(refs, package_name, fallback_patterns, constraint) + if winner is None: + raise NoMatchingTagError( + summary=self._format_no_match_summary( + owner_repo=owner_repo, + constraint=constraint, + refs=refs, + ), + hint=self._format_no_match_hint(constraint=constraint), + ) + version, tag_name, sha, pattern = winner + return GitSemverResolution( + constraint=constraint, + resolved_version=self._render_version(version), + resolved_tag=tag_name, + resolved_sha=sha, + matched_pattern=pattern, + resolved_at=now_iso or utc_now_iso(), + ) + + # ------------------------------------------------------------------ + # Internals + # ------------------------------------------------------------------ + + def _pick_best( + self, + refs: Sequence[RemoteRef], + package_name: str, + patterns: Sequence[str], + constraint: str, + ) -> tuple[SemVer, str, str, str] | None: + """Return the highest-version candidate or ``None``.""" + candidates = iter_semver_tags(refs, package_name=package_name, patterns=patterns) + if not candidates: + return None + filtered: list[tuple[SemVer, str, str, str]] = [] + for cand in candidates: + version = cand[0] + if version.is_prerelease and not self._include_prerelease: + continue + if not satisfies_range(version, constraint): + continue + filtered.append(cand) + if not filtered: + return None + # Sort by SemVer ordering; the last entry is the winner. + filtered.sort(key=lambda t: t[0]) + return filtered[-1] + + @staticmethod + def _render_version(version: SemVer) -> str: + """Render a :class:`SemVer` back into its canonical string form.""" + base = f"{version.major}.{version.minor}.{version.patch}" + if version.prerelease: + base = f"{base}-{version.prerelease}" + if version.build_meta: + base = f"{base}+{version.build_meta}" + return base + + @staticmethod + def _format_no_match_summary( + *, + owner_repo: str, + constraint: str, + refs: Sequence[RemoteRef], + ) -> str: + tags = [ + r.name[len(_REFS_TAGS_PREFIX) :] for r in refs if r.name.startswith(_REFS_TAGS_PREFIX) + ] + if not tags: + return f"No tags on {owner_repo} satisfy {constraint!r}. The remote has no tag refs." + sample = ", ".join(sorted(tags)[:5]) + return ( + f"No tags on {owner_repo} satisfy {constraint!r}. " + f"Tags considered: {sample}" + f"{' (and more)' if len(tags) > 5 else ''}." + ) + + @staticmethod + def _format_no_match_hint(*, constraint: str) -> str: + return ( + "Hint: widen the range, pin a literal tag with " + f"'ref: ' instead of '{constraint}', or fall back to " + "a branch / SHA ref." + ) diff --git a/src/apm_cli/deps/installed_package.py b/src/apm_cli/deps/installed_package.py index 26be464b96..9a28ff5ebe 100644 --- a/src/apm_cli/deps/installed_package.py +++ b/src/apm_cli/deps/installed_package.py @@ -12,6 +12,7 @@ from typing import TYPE_CHECKING if TYPE_CHECKING: + from apm_cli.deps.git_semver_resolver import GitSemverResolution from apm_cli.deps.registry.resolver import RegistryResolution from apm_cli.deps.registry_proxy import RegistryConfig from apm_cli.models.dependency.reference import DependencyReference @@ -52,6 +53,11 @@ class InstalledPackage: ``resolved_hash`` / ``version`` from it so re-installs verify against the same content (design §6.1). Distinct concept from ``registry_config`` (Artifactory VCS proxy). + git_semver_resolution: + The :class:`~apm_cli.deps.git_semver_resolver.GitSemverResolution` produced + when a git-source dep used a semver range as ``ref:`` (issue #1488). + When present the lockfile records ``constraint`` / ``resolved_tag`` / + ``resolved_at`` and ``resolved_ref`` is set to the concrete tag. """ dep_ref: DependencyReference @@ -61,3 +67,4 @@ class InstalledPackage: is_dev: bool = False registry_config: RegistryConfig | None = None registry_resolution: RegistryResolution | None = None + git_semver_resolution: GitSemverResolution | None = None diff --git a/src/apm_cli/deps/lockfile.py b/src/apm_cli/deps/lockfile.py index 7e067a7b0a..5196a863c3 100644 --- a/src/apm_cli/deps/lockfile.py +++ b/src/apm_cli/deps/lockfile.py @@ -55,6 +55,21 @@ class LockedDependency: resolved_url: str | None = None resolved_hash: str | None = None + # Git-source semver resolution fields (issue #1488). + # Populated when a git-source dependency carried a semver range + # (e.g. ``^1.2.0``) that the install pipeline resolved against the + # remote's tags. Lockfile version stays at "2" -- these fields are + # purely additive and forward-compatible with old readers (which + # ignore unknown keys via the explicit ``from_dict`` allowlist). + constraint: str | None = None + resolved_tag: str | None = None + resolved_at: str | None = None + # Forward-compat carrier: keys we don't recognise are preserved + # through a from_dict / to_dict round-trip so an older APM build + # reading a lockfile written by a newer build doesn't silently drop + # fields when it re-emits. + _unknown_fields: dict[str, Any] = field(default_factory=dict) + def get_unique_key(self) -> str: """Returns unique key for this dependency.""" if self.source == "local" and self.local_path: @@ -114,6 +129,16 @@ def to_dict(self) -> dict[str, Any]: result["resolved_url"] = self.resolved_url if self.resolved_hash: result["resolved_hash"] = self.resolved_hash + if self.constraint: + result["constraint"] = self.constraint + if self.resolved_tag: + result["resolved_tag"] = self.resolved_tag + if self.resolved_at: + result["resolved_at"] = self.resolved_at + # Replay forward-compat unknown fields LAST so they never shadow a + # known field that this build understands. + for k, v in self._unknown_fields.items(): + result.setdefault(k, v) return result @classmethod @@ -144,6 +169,44 @@ def from_dict(cls, data: dict[str, Any]) -> LockedDependency: if _p_int is not None and 1 <= _p_int <= 65535: port = _p_int + # Recognised keys this build knows about. Anything else is captured + # as ``_unknown_fields`` so a re-emit preserves forward-introduced + # fields rather than silently dropping them. ``deployed_skills`` is + # the explicit legacy key handled above; do NOT consider it unknown. + _known_keys = { + "repo_url", + "host", + "port", + "registry_prefix", + "resolved_commit", + "resolved_ref", + "version", + "virtual_path", + "is_virtual", + "depth", + "resolved_by", + "package_type", + "deployed_files", + "deployed_file_hashes", + "source", + "local_path", + "content_hash", + "is_dev", + "discovered_via", + "marketplace_plugin_name", + "is_insecure", + "allow_insecure", + "skill_subset", + "resolved_url", + "resolved_hash", + "constraint", + "resolved_tag", + "resolved_at", + # legacy migration key handled above + "deployed_skills", + } + unknown_fields = {k: v for k, v in data.items() if k not in _known_keys} + return cls( repo_url=data["repo_url"], host=data.get("host"), @@ -170,6 +233,10 @@ def from_dict(cls, data: dict[str, Any]) -> LockedDependency: skill_subset=list(data.get("skill_subset") or []), resolved_url=data.get("resolved_url"), resolved_hash=data.get("resolved_hash"), + constraint=data.get("constraint"), + resolved_tag=data.get("resolved_tag"), + resolved_at=data.get("resolved_at"), + _unknown_fields=unknown_fields, ) @classmethod @@ -182,6 +249,7 @@ def from_dependency_ref( is_dev: bool = False, registry_config=None, registry_resolution=None, + git_semver_resolution=None, ) -> LockedDependency: """Create from a DependencyReference with resolution info. @@ -201,7 +269,30 @@ def from_dependency_ref( ``source`` is set to ``"registry"`` and ``resolved_url`` / ``resolved_hash`` / ``version`` are populated from it (the trust anchor for re-installs per design §6.1). + git_semver_resolution: Optional + :class:`~apm_cli.deps.git_semver_resolver.GitSemverResolution` + produced when a git-source dep had a semver range as ``ref:``. + When provided, ``constraint`` / ``resolved_tag`` / + ``resolved_at`` are populated and ``resolved_ref`` is set + to the concrete tag (issue #1488). Mutually exclusive with + ``registry_resolution``. + + Raises: + ValueError: When both ``registry_resolution`` and + ``git_semver_resolution`` are provided. The two resolution + paths are mutually exclusive: a dependency is either + registry-sourced (carries ``resolved_url`` / ``resolved_hash``) + or git-source with a semver range (carries ``constraint`` / + ``resolved_tag`` / ``resolved_at``). Combining both would + produce an inconsistent lockfile entry (e.g. ``source=registry`` + while ``resolved_ref`` is overridden to a git tag). """ + if registry_resolution is not None and git_semver_resolution is not None: + raise ValueError( + "registry_resolution and git_semver_resolution are mutually " + "exclusive: a dependency is either registry-sourced or a " + "git-source semver resolution, not both." + ) if registry_config is not None: host = registry_config.host registry_prefix = registry_config.prefix @@ -218,14 +309,31 @@ def from_dependency_ref( else: source = None + # When a git-semver resolution is present, prefer the concrete + # resolved tag for ``resolved_ref`` (so subsequent installs see a + # literal tag, not the original range). The original constraint + # is preserved in the dedicated ``constraint`` field. + if git_semver_resolution is not None: + resolved_ref_val: str | None = git_semver_resolution.resolved_tag + else: + resolved_ref_val = dep_ref.reference + return cls( repo_url=dep_ref.repo_url, host=host, port=dep_ref.port, registry_prefix=registry_prefix, resolved_commit=resolved_commit, - resolved_ref=dep_ref.reference, - version=(registry_resolution.version if registry_resolution is not None else None), + resolved_ref=resolved_ref_val, + version=( + registry_resolution.version + if registry_resolution is not None + else ( + git_semver_resolution.resolved_version + if git_semver_resolution is not None + else None + ) + ), virtual_path=dep_ref.virtual_path, is_virtual=dep_ref.is_virtual, depth=depth, @@ -244,6 +352,15 @@ def from_dependency_ref( resolved_hash=( registry_resolution.resolved_hash if registry_resolution is not None else None ), + constraint=( + git_semver_resolution.constraint if git_semver_resolution is not None else None + ), + resolved_tag=( + git_semver_resolution.resolved_tag if git_semver_resolution is not None else None + ), + resolved_at=( + git_semver_resolution.resolved_at if git_semver_resolution is not None else None + ), ) def to_dependency_ref(self) -> DependencyReference: @@ -292,12 +409,15 @@ class LockFile: def add_dependency(self, dep: LockedDependency) -> None: """Add a dependency to the lock file. - Adding a registry-sourced dep promotes ``lockfile_version`` to - ``"2"`` immediately, keeping the in-memory state consistent with - what ``to_yaml()`` would emit (design §6.1). + Adding a registry-sourced dep or a git-source dep with semver + resolution fields promotes ``lockfile_version`` to ``"2"`` eagerly, + keeping the in-memory state consistent with what ``to_yaml()`` + would emit (design section 6.1; issue #1488). """ self.dependencies[dep.get_unique_key()] = dep - if dep.source == "registry" and self.lockfile_version == "1": + if self.lockfile_version == "1" and ( + dep.source == "registry" or dep.constraint or dep.resolved_tag or dep.resolved_at + ): self.lockfile_version = "2" def get_dependency(self, key: str) -> LockedDependency | None: @@ -319,12 +439,19 @@ def get_package_dependencies(self) -> list[LockedDependency]: def _needs_v2(self) -> bool: """Whether the resolved graph requires lockfile schema v2. - Per design §6.1 (and invariant §2.1.4): bump opportunistically — only - when at least one dep is sourced from a dedicated registry. A project - that never opts into the registry keeps v1 forever, even on a newer - client. + Per design section 6.1 (and invariant 2.1.4): bump opportunistically -- + only when at least one dep is sourced from a dedicated registry, OR + when at least one dep carries git-source semver resolution fields + (``constraint`` / ``resolved_tag`` / ``resolved_at`` -- issue #1488). + A project that uses neither feature keeps v1 forever, even on a + newer client. """ - return any(d.source == "registry" for d in self.dependencies.values()) + for d in self.dependencies.values(): + if d.source == "registry": + return True + if d.constraint or d.resolved_tag or d.resolved_at: + return True + return False def to_yaml(self) -> str: """Serialize to YAML string.""" @@ -332,8 +459,8 @@ def to_yaml(self) -> str: # the lockfile_version field always reflects current content at # emit time. ``add_dependency`` bumps to "2" eagerly, but callers # that mutate ``self.dependencies`` directly or remove the last - # registry dep need the field re-derived here so the on-disk - # version is correct in both directions. + # registry / git-semver dep need the field re-derived here so the + # on-disk version is correct in both directions. self.lockfile_version = "2" if self._needs_v2() else "1" emit_version = self.lockfile_version # The synthesized self-entry (key ".") is an in-memory normalization @@ -449,6 +576,7 @@ def from_installed_packages( for entry in installed_packages: registry_resolution = None + git_semver_resolution = None if isinstance(entry, InstalledPackage): dep_ref = entry.dep_ref resolved_commit = entry.resolved_commit @@ -457,6 +585,7 @@ def from_installed_packages( is_dev = entry.is_dev registry_config = getattr(entry, "registry_config", None) registry_resolution = getattr(entry, "registry_resolution", None) + git_semver_resolution = getattr(entry, "git_semver_resolution", None) elif len(entry) >= 5: dep_ref, resolved_commit, depth, resolved_by, is_dev = entry[:5] registry_config = None @@ -473,6 +602,7 @@ def from_installed_packages( is_dev=is_dev, registry_config=registry_config, registry_resolution=registry_resolution, + git_semver_resolution=git_semver_resolution, ) lock.add_dependency(locked_dep) diff --git a/src/apm_cli/drift.py b/src/apm_cli/drift.py index 6809dc36f7..4cdc69f3cb 100644 --- a/src/apm_cli/drift.py +++ b/src/apm_cli/drift.py @@ -144,6 +144,16 @@ def detect_ref_change( locked_dep.version, ) + # Git-source semver-range deps (issue #1488): the manifest carries + # a semver range (``^1.2.0``) while the lockfile records the + # resolved tag (``v1.5.3``) plus the original constraint. Direct + # comparison of the range against the tag is a false positive -- + # instead, treat the dep as unchanged when the lockfile already + # stores the same constraint (and we therefore trust its resolution + # until the user runs ``--update``). + if getattr(dep_ref, "ref_kind", None) == "semver": + return dep_ref.reference != locked_dep.constraint + # Git/local deps: direct ref comparison. Handles None→value, value→None, # and value→value. No truthiness guard on locked_dep.resolved_ref — # None != "v1.0.0" is True. diff --git a/src/apm_cli/install/context.py b/src/apm_cli/install/context.py index dfd539047f..37f322820f 100644 --- a/src/apm_cli/install/context.py +++ b/src/apm_cli/install/context.py @@ -98,6 +98,12 @@ class InstallContext: diagnostics: Any = None # DiagnosticCollector registry_config: Any = None # RegistryConfig (proxy registry; pre-existing) registry_resolver: Any = None # RegistryPackageResolver -- dedicated registry resolver + # Per-dep git-source semver resolutions (issue #1488). Keyed by + # dep_key (DependencyReference.get_unique_key()), populated by the + # BFS download_callback when a git-source dep has ref_kind == "semver", + # consumed by install/sources.py to plumb the resolution into the + # lockfile via InstalledPackage.git_semver_resolution. + git_semver_resolutions: dict[str, Any] = field(default_factory=dict) managed_files: set[str] = field(default_factory=set) # ------------------------------------------------------------------ diff --git a/src/apm_cli/install/phases/resolve.py b/src/apm_cli/install/phases/resolve.py index 56eb0a04fa..17d2d9c54f 100644 --- a/src/apm_cli/install/phases/resolve.py +++ b/src/apm_cli/install/phases/resolve.py @@ -59,6 +59,163 @@ def _require_package_registry_feature_if_needed(registries_map, existing_lockfil return needs_registry +def _maybe_resolve_git_semver( + *, + dep_ref, + existing_lockfile, + update_refs: bool, + auth_resolver=None, +): + """Resolve a git-source semver-range ``ref:`` to a concrete tag. + + Returns the :class:`~apm_cli.deps.git_semver_resolver.GitSemverResolution` + when resolution ran (and the caller should rewrite ``dep_ref.reference``); + returns ``None`` for any dep that should NOT route through the + git-semver resolver (local, registry-sourced, proxy-sourced, literal + ref, or a lockfile-pinned reinstall without ``--update``). + + Lockfile replay + --------------- + When a lockfile entry already records ``constraint == dep_ref.reference`` + and the locked tag still satisfies it, this function rebuilds the + :class:`GitSemverResolution` from the lockfile WITHOUT touching the + network. This is the npm-style "honour the lock" path -- the locked + tag is canonical until the manifest range changes or the user passes + ``--update`` / ``--refresh``. + + Auth + ---- + When ``auth_resolver`` is supplied, the per-dep ``AuthContext`` is + resolved before constructing :class:`RefResolver` and its token is + embedded in the ``https://`` URL used by ``git ls-remote``. This + mirrors the auth path used by the clone step downstream, so a + private-repo semver dep that clones successfully also enumerates + its tags successfully in CI environments where ``GITHUB_APM_PAT`` / + ``ADO_APM_PAT`` are the only credential source (no system + credential helper available). Passing ``auth_resolver=None`` (the + legacy path) preserves the previous unauthenticated behaviour for + public repos and for callers that intentionally skip auth. + """ + # Only git-source deps with a semver-range reference are eligible. + if dep_ref.is_local: + return None + if getattr(dep_ref, "source", None) == "registry": + return None + if getattr(dep_ref, "artifactory_prefix", None): + return None + if dep_ref.ref_kind != "semver": + return None + + constraint = dep_ref.reference + owner_repo = dep_ref.repo_url + package_name = owner_repo.rsplit("/", 1)[-1] + + # Lockfile replay (npm semantics): if the lockfile already records a + # resolution for this constraint, return it directly. Saves a + # ls-remote call and keeps installs deterministic across machines. + if not update_refs and existing_lockfile is not None: + locked = existing_lockfile.get_dependency(dep_ref.get_unique_key()) + if ( + locked is not None + and locked.constraint == constraint + and locked.resolved_tag + and locked.resolved_commit + and locked.version + ): + from apm_cli.deps.git_semver_resolver import GitSemverResolution + + return GitSemverResolution( + constraint=locked.constraint, + resolved_version=locked.version, + resolved_tag=locked.resolved_tag, + resolved_sha=locked.resolved_commit, + # The pattern that produced the locked tag is not + # persisted (it would just be informational); the empty + # string here means "unknown / from lockfile". + matched_pattern="", + resolved_at=locked.resolved_at or "", + ) + + # Fresh resolution: call git ls-remote and pick the highest matching tag. + from apm_cli.deps.git_semver_resolver import GitSemverResolver + from apm_cli.marketplace.ref_resolver import RefResolver + + # Resolve the per-dep token via AuthResolver so ls-remote uses the + # same credential source the downstream clone will use. Without this + # threading, ls-remote on a private repo would rely on the host's + # git credential helper (present on dev laptops, absent in CI). + token: str | None = None + if auth_resolver is not None: + try: + auth_ctx = auth_resolver.resolve_for_dep(dep_ref) + token = auth_ctx.token if auth_ctx is not None else None + except Exception: + # Auth lookup is best-effort here: if it fails the unauth path + # remains, the downstream clone will surface the real auth + # error with its own actionable diagnostic. + token = None + + ref_resolver = RefResolver(host=dep_ref.host, token=token) + resolver = GitSemverResolver(ref_resolver) + return resolver.resolve( + owner_repo=owner_repo, + package_name=package_name, + constraint=constraint, + ) + + +def _purge_cached_semver_paths_for_update( + *, + all_apm_deps, + apm_modules_dir, + logger, +) -> None: + """Pre-purge on-disk install paths for direct git-source semver deps + when ``--update`` / ``--refresh`` is set. + + Bug 1 fix (#1496): the BFS resolver short-circuits at + ``install_path.exists()`` and never invokes ``download_callback``, + which is where ``_maybe_resolve_git_semver`` lives. For git-source + semver direct deps we therefore pre-purge the install path so the + resolver is forced through the callback, re-runs ``git ls-remote``, + and rewrites the lockfile with the latest matching tag. Matches + npm / cargo / bundler: ``--update`` is the explicit re-resolve + trigger and must not be swallowed by the on-disk cache. Scoped to + direct deps to avoid disturbing transitive cached content; the + resolver re-walks transitives naturally once a direct dep's + callback rewrites its ref. Local, registry, and proxy deps are + excluded -- their semver semantics (if any) belong to a different + resolver path. + """ + from contextlib import suppress + + from apm_cli.utils.file_ops import robust_rmtree as _rrm + + for _dep in all_apm_deps: + if getattr(_dep, "ref_kind", None) != "semver": + continue + if _dep.is_local: + continue + if getattr(_dep, "source", None) == "registry": + continue + if getattr(_dep, "artifactory_prefix", None): + continue + try: + _ip = _dep.get_install_path(apm_modules_dir) + except Exception: # noqa: S112 + # Path computation failure (e.g. malformed dep) is non-fatal + # here -- the resolver will surface a real error downstream. + continue + if _ip.exists(): + with suppress(Exception): + _rrm(_ip, ignore_errors=True) + if logger: + logger.verbose_detail( + f"[*] --update: cleared cached install path for " + f"{_dep.get_unique_key()} to force semver re-resolution" + ) + + def run(ctx: InstallContext) -> None: """Execute the resolve phase. @@ -266,8 +423,27 @@ def download_callback(dep_ref, modules_dir, parent_chain="", parent_pkg=None): package's directory rather than the root consumer (#857). """ install_path = dep_ref.get_install_path(modules_dir) + # Cache short-circuit: skip the rest of the callback when the + # install path already exists. Exception: for git-source semver + # deps under ``--update`` / ``--refresh`` (``update_refs=True``), + # fall through so ``_maybe_resolve_git_semver`` re-runs + # ``git ls-remote`` and the lockfile gets rewritten with the + # latest matching tag. Matches npm/cargo/bundler: ``--update`` + # is the explicit re-resolve trigger and must not be swallowed + # by the on-disk cache (Bug 1 fix on #1496). The downstream + # ``downloader.download_package`` rmtrees and re-clones the + # install path when the resolved tag changes, so refetching is + # safe. if install_path.exists(): - return install_path + _force_semver_resolve = ( + update_refs + and not dep_ref.is_local + and getattr(dep_ref, "source", None) != "registry" + and not getattr(dep_ref, "artifactory_prefix", None) + and getattr(dep_ref, "ref_kind", None) == "semver" + ) + if not _force_semver_resolve: + return install_path # F1 (#1116): surface a heartbeat BEFORE the network/copy work so # users see the install advancing past silent transitive lookups. # Under F7's parallel BFS this callback may run on a worker @@ -388,6 +564,29 @@ def download_callback(dep_ref, modules_dir, parent_chain="", parent_pkg=None): _tui.task_failed(dep_ref.get_unique_key()) return None + # --- Git-source semver range resolution (issue #1488) --- + # When the manifest carries a semver range as ``ref:`` and + # the dep is non-local, non-registry, and non-proxy, resolve + # it to a concrete tag BEFORE any git operation. The result + # is stashed on ctx so install/sources.py can plumb it into + # the lockfile, and the dep_ref's ``reference`` is replaced + # with the concrete tag so build_download_ref / clone use a + # literal git ref. + _semver_resolution = _maybe_resolve_git_semver( + dep_ref=dep_ref, + existing_lockfile=existing_lockfile, + update_refs=update_refs, + auth_resolver=ctx.auth_resolver, + ) + if _semver_resolution is not None: + with callback_lock: + ctx.git_semver_resolutions[dep_ref.get_unique_key()] = _semver_resolution + # Rewrite the dep_ref's ref to the concrete tag so the + # rest of the pipeline (drift detection, download, etc.) + # operates on a literal git ref. The original constraint + # is preserved in the resolution dataclass. + dep_ref.reference = _semver_resolution.resolved_tag + # T5: Use locked commit for reproducibility, unless the manifest # ref has drifted from what the lockfile recorded (spec drift). _locked_dep = ( @@ -440,9 +639,25 @@ def download_callback(dep_ref, modules_dir, parent_chain="", parent_pkg=None): dep_key = dep_ref.get_unique_key() is_direct = dep_key in direct_dep_keys + # Distinguish resolution failures (git-semver no-match) from + # download failures: the dep_ref was rewritten to a concrete + # tag BEFORE clone, so a NoMatchingTagError means we never + # got to the download step. Using "download" as the verb + # would mislead users who are debugging an unsatisfied + # constraint -- nothing was downloaded yet. + from apm_cli.deps.git_semver_resolver import NoMatchingTagError + + if isinstance(e, NoMatchingTagError): + if is_direct: + fail_msg = f"No matching tag for {dep_ref.repo_url}: {e}" + else: + chain_hint = f" (via {parent_chain})" if parent_chain else "" + fail_msg = ( + f"No matching tag for transitive dep {dep_ref.repo_url}{chain_hint}: {e}" + ) # Distinguish direct vs transitive failure messages so users # don't see a misleading "transitive dep" label for top-level deps. - if is_direct: + elif is_direct: fail_msg = f"Failed to download dependency {dep_ref.repo_url}: {e}" else: chain_hint = f" (via {parent_chain})" if parent_chain else "" @@ -467,6 +682,13 @@ def download_callback(dep_ref, modules_dir, parent_chain="", parent_pkg=None): # ------------------------------------------------------------------ # 6. Resolver creation + dependency resolution # ------------------------------------------------------------------ + if update_refs: + _purge_cached_semver_paths_for_update( + all_apm_deps=ctx.all_apm_deps, + apm_modules_dir=apm_modules_dir, + logger=ctx.logger, + ) + resolver = APMDependencyResolver( apm_modules_dir=apm_modules_dir, download_callback=download_callback, diff --git a/src/apm_cli/install/sources.py b/src/apm_cli/install/sources.py index be91683258..cab2ea3379 100644 --- a/src/apm_cli/install/sources.py +++ b/src/apm_cli/install/sources.py @@ -67,6 +67,40 @@ def _format_package_type_label(pkg_type) -> str | None: }.get(pkg_type) +def _rebuild_cached_semver_resolution(dep_locked_chk: Any) -> Any: + """Rebuild a ``GitSemverResolution`` from a cached lockfile entry. + + Returns ``None`` unless ALL required fields are present on + *dep_locked_chk*: ``constraint``, ``version``, ``resolved_tag``, + and ``resolved_commit``. Per PR #1496 review thread: gating on + just ``constraint`` and back-filling missing fields with empty + strings risks propagating an incomplete semver resolution into + ``InstalledPackage`` and rewriting the lockfile with empty/missing + fields (and an empty ``resolved_ref``). When the lockfile cache is + incomplete we prefer to leave the resolution as ``None`` so the + caller falls back to the literal-ref path. + """ + if dep_locked_chk is None: + return None + if not ( + dep_locked_chk.constraint + and dep_locked_chk.version + and dep_locked_chk.resolved_tag + and dep_locked_chk.resolved_commit + ): + return None + from apm_cli.deps.git_semver_resolver import GitSemverResolution + + return GitSemverResolution( + constraint=dep_locked_chk.constraint, + resolved_version=dep_locked_chk.version, + resolved_tag=dep_locked_chk.resolved_tag, + resolved_sha=dep_locked_chk.resolved_commit, + matched_pattern="", + resolved_at=dep_locked_chk.resolved_at or "", + ) + + @dataclass class Materialization: """Outcome of ``DependencySource.acquire()``. @@ -464,6 +498,17 @@ def acquire(self) -> Materialization | None: ctx, dep_ref, dep_key, dep_locked_chk ) + # Cached git-source semver dep (#1488): replay the resolution from + # either ctx (we resolved earlier in this same run) or the lockfile + # so re-writing the lockfile from cache preserves constraint / + # resolved_tag / resolved_at instead of dropping them. The + # lockfile-backed reconstruction is gated on ALL required fields + # being present (see ``_rebuild_cached_semver_resolution`` and the + # PR #1496 review thread). + _cached_semver = ctx.git_semver_resolutions.get(dep_key) + if _cached_semver is None: + _cached_semver = _rebuild_cached_semver_resolution(dep_locked_chk) + ctx.installed_packages.append( InstalledPackage( dep_ref=dep_ref, @@ -473,6 +518,7 @@ def acquire(self) -> Materialization | None: is_dev=_is_dev, registry_config=_cached_registry, registry_resolution=_cached_resolution, + git_semver_resolution=_cached_semver, ) ) if install_path.is_dir(): @@ -688,6 +734,9 @@ def acquire(self) -> Materialization | None: if dep_ref.source == "registry" else None ) + # Git-source semver-range deps (#1488): the resolution was + # captured by the BFS download_callback in phases/resolve.py. + _git_semver_resolution = ctx.git_semver_resolutions.get(dep_key) ctx.installed_packages.append( InstalledPackage( dep_ref=dep_ref, @@ -697,6 +746,7 @@ def acquire(self) -> Materialization | None: is_dev=_is_dev, registry_config=(ctx.registry_config if not dep_ref.is_local else None), registry_resolution=_registry_resolution, + git_semver_resolution=_git_semver_resolution, ) ) if install_path.is_dir(): diff --git a/src/apm_cli/install/summary.py b/src/apm_cli/install/summary.py index 1964c4daac..0a701d7a06 100644 --- a/src/apm_cli/install/summary.py +++ b/src/apm_cli/install/summary.py @@ -71,3 +71,12 @@ def render_post_install_summary( # (consistent with ``apm unpack``). ``--force`` overrides. if not force and apm_diagnostics and apm_diagnostics.has_critical_security: sys.exit(1) + + # Hard-fail when ANY per-dep install error was reported. Matches + # the npm / pip / cargo convention: any install failure -> non-zero + # exit so CI scripts can detect failure without parsing stderr. + # ``--force`` covers critical-security overrides only; it does NOT + # suppress this hard-fail (Bug 2 fix on #1496, where the CLI used + # to exit 0 even after printing "Installation failed with N error(s)"). + if error_count > 0: + sys.exit(1) diff --git a/src/apm_cli/models/dependency/reference.py b/src/apm_cli/models/dependency/reference.py index c4afa6da1c..427453df82 100644 --- a/src/apm_cli/models/dependency/reference.py +++ b/src/apm_cli/models/dependency/reference.py @@ -115,6 +115,41 @@ class DependencyReference: source: str | None = None registry_name: str | None = None + @property + def ref_kind(self) -> str | None: + """Classify ``reference`` for routing purposes. + + Returns one of: + + * ``"semver"`` -- ``reference`` parses as a valid semver range + (``^1.2.0``, ``~2.1``, ``>=1.0 <2.0``, ``1.2.x``, exact ``1.2.3``). + The install pipeline resolves it against the remote's tags via + :class:`~apm_cli.deps.git_semver_resolver.GitSemverResolver`. + * ``"literal"`` -- ``reference`` is a non-empty string that does + NOT parse as semver (branch name, tag name with prefix, SHA). + * ``None`` -- ``reference`` is unset; downstream uses the remote's + default branch. + + Semver routing is opt-in by syntax: any ``ref:`` value that + survives the literal-branch / literal-tag / SHA parse intact + bypasses the semver resolver, so existing dependencies on + ``ref: v1.2.3`` (literal tag with ``v`` prefix) keep their + existing behaviour. + + Note: ``"1.2.3"`` (no ``v`` prefix) parses as a semver exact-version + constraint, NOT a literal tag. The git-semver resolver's bare- + version fallback pattern covers the "literal ``1.2.3`` tag on the + remote" case without breaking semver routing for the same input. + """ + if not self.reference: + return None + # ``v1.2.3``, ``main``, SHAs, anything-with-prefix is literal. + # Only inputs that parse as a *standalone* semver range are + # routed through the git-semver resolver. + if _is_valid_registry_semver_range(self.reference): + return "semver" + return "literal" + # Supported file extensions for virtual packages VIRTUAL_FILE_EXTENSIONS = ( ".prompt.md", diff --git a/tests/integration/test_git_semver_install_e2e.py b/tests/integration/test_git_semver_install_e2e.py new file mode 100644 index 0000000000..b8744e6376 --- /dev/null +++ b/tests/integration/test_git_semver_install_e2e.py @@ -0,0 +1,719 @@ +"""End-to-end ``apm install`` coverage for git-source semver range refs (#1488). + +The unit-tier suite (``tests/unit/deps/test_git_semver_resolver.py``, +``tests/unit/install/test_git_semver_wiring.py``) covers each helper in +isolation; this file pairs that work with the full +``apm install`` -> resolve phase -> lockfile write -> lockfile replay +pipeline and asserts on the user-observable artifacts (exit code, +``apm.lock.yaml`` contents, network-call counts). + +Fidelity strategy +----------------- +Two seams are stubbed -- everything else runs through the real install +pipeline: + +* ``RefResolver.list_remote_refs`` returns canned ``RemoteRef`` lists per + ``owner/repo`` (the "git ls-remote" output a private fixture repo + would produce). +* ``GitHubPackageDownloader.download_package`` writes a minimal + ``apm.yml`` to the install path and returns a ``PackageInfo`` whose + ``resolved_commit`` matches the SHA the resolver picked. Validation, + integration, and lockfile writes then run against real disk content. + +Both stubs record their call counts so tests can assert "lockfile replay +did not touch the network" without relying on subprocess sentinels. +""" + +from __future__ import annotations + +from datetime import datetime +from pathlib import Path +from unittest.mock import patch + +import pytest +import yaml +from click.testing import CliRunner + +from apm_cli.cli import cli +from apm_cli.marketplace.ref_resolver import RemoteRef +from apm_cli.models.apm_package import ( + APMPackage, + PackageInfo, + clear_apm_yml_cache, +) +from apm_cli.models.dependency.types import GitReferenceType, ResolvedReference + +_PATCH_UPDATES = "apm_cli.commands._helpers.check_for_updates" + + +# --------------------------------------------------------------------------- +# Canned remote ref sets +# --------------------------------------------------------------------------- + + +def _refs_v_prefixed() -> list[RemoteRef]: + """Standard ``v{version}`` tag fixture: v1.0.0, v1.2.3, v1.5.0, v2.0.0.""" + return [ + RemoteRef(name="refs/heads/main", sha="0" * 40), + RemoteRef(name="refs/tags/v1.0.0", sha="1" * 40), + RemoteRef(name="refs/tags/v1.2.3", sha="2" * 40), + RemoteRef(name="refs/tags/v1.5.0", sha="3" * 40), + RemoteRef(name="refs/tags/v2.0.0", sha="4" * 40), + ] + + +def _refs_only_name_dashv() -> list[RemoteRef]: + """Repo where ONLY the ``{name}--v{version}`` pattern matches. + + Mirrors a multi-package repo (PR #1422 convention) where each + package's tags are scoped by package name. + """ + return [ + RemoteRef(name="refs/heads/main", sha="0" * 40), + RemoteRef(name="refs/tags/widget--v1.0.0", sha="a" * 40), + RemoteRef(name="refs/tags/widget--v1.3.0", sha="b" * 40), + RemoteRef(name="refs/tags/otherpkg--v9.9.9", sha="c" * 40), + ] + + +def _refs_only_bare() -> list[RemoteRef]: + """Repo that tags as bare ``{version}`` -- triggers third-pattern fallback.""" + return [ + RemoteRef(name="refs/heads/main", sha="0" * 40), + RemoteRef(name="refs/tags/1.0.0", sha="d" * 40), + RemoteRef(name="refs/tags/1.4.2", sha="e" * 40), + ] + + +def _refs_no_match() -> list[RemoteRef]: + """No tag in any pattern satisfies a ^1.2.0 constraint.""" + return [ + RemoteRef(name="refs/heads/main", sha="0" * 40), + RemoteRef(name="refs/tags/v0.9.0", sha="9" * 40), + ] + + +# --------------------------------------------------------------------------- +# Stubs +# --------------------------------------------------------------------------- + + +class _RefResolverCallRecorder: + """Records ``list_remote_refs`` calls and serves canned refs.""" + + def __init__(self, refs_by_repo: dict[str, list[RemoteRef]]) -> None: + self.refs_by_repo = refs_by_repo + self.calls: list[str] = [] + self.init_kwargs: list[dict] = [] + + def install(self, monkeypatch: pytest.MonkeyPatch) -> None: + recorder = self + + original_init = None + from apm_cli.marketplace import ref_resolver as _rr_mod + + original_init = _rr_mod.RefResolver.__init__ + + def _capture_init(self, *args, **kwargs): + recorder.init_kwargs.append(dict(kwargs)) + return original_init(self, *args, **kwargs) + + def _fake_list_remote_refs(self, owner_repo: str) -> list[RemoteRef]: + recorder.calls.append(owner_repo) + refs = recorder.refs_by_repo.get(owner_repo) + if refs is None: + raise AssertionError( + f"Unexpected list_remote_refs call for {owner_repo!r}; " + f"fixture has: {sorted(recorder.refs_by_repo)}" + ) + return list(refs) + + monkeypatch.setattr(_rr_mod.RefResolver, "__init__", _capture_init) + monkeypatch.setattr(_rr_mod.RefResolver, "list_remote_refs", _fake_list_remote_refs) + + +class _DownloaderStub: + """Stubs ``GitHubPackageDownloader.download_package`` to write a + minimal valid apm package at the install path and return a + ``PackageInfo`` whose ``resolved_commit`` reflects the SHA the + resolver picked (read off ``dep_ref.reference`` after the resolve + phase has rewritten it to the concrete tag). + """ + + def __init__(self, sha_by_tag: dict[str, str]) -> None: + self.sha_by_tag = sha_by_tag + self.calls: list[tuple[str, str]] = [] # (owner_repo, reference) + + def install(self, monkeypatch: pytest.MonkeyPatch) -> None: + recorder = self + + def _fake_download(self, repo_ref, target_path, *args, **kwargs): + from apm_cli.models.apm_package import DependencyReference + + if isinstance(repo_ref, DependencyReference): + dep_ref = repo_ref + else: + dep_ref = DependencyReference.parse(str(repo_ref)) + + ref_value = dep_ref.reference or "main" + recorder.calls.append((dep_ref.repo_url, ref_value)) + + sha = recorder.sha_by_tag.get(ref_value, "f" * 40) + target_path = Path(target_path) + target_path.mkdir(parents=True, exist_ok=True) + + package_name = dep_ref.repo_url.rsplit("/", 1)[-1] + (target_path / "apm.yml").write_text( + yaml.safe_dump( + { + "name": package_name, + "version": "0.0.0", + "description": "test fixture package", + } + ), + encoding="utf-8", + ) + + package = APMPackage.from_apm_yml(target_path / "apm.yml") + return PackageInfo( + package=package, + install_path=target_path, + installed_at=datetime.now().isoformat(), + dependency_ref=dep_ref, + resolved_reference=ResolvedReference( + original_ref=ref_value, + ref_type=GitReferenceType.TAG, + resolved_commit=sha, + ref_name=ref_value, + ), + ) + + from apm_cli.deps import github_downloader as _ghd + + monkeypatch.setattr(_ghd.GitHubPackageDownloader, "download_package", _fake_download) + + +# --------------------------------------------------------------------------- +# Fixtures +# --------------------------------------------------------------------------- + + +@pytest.fixture +def runner() -> CliRunner: + return CliRunner() + + +@pytest.fixture(autouse=True) +def _clear_cache() -> None: + clear_apm_yml_cache() + yield + clear_apm_yml_cache() + + +def _write_apm_yml(project: Path, deps: list, name: str = "consumer-pkg") -> None: + project.mkdir(parents=True, exist_ok=True) + (project / "apm.yml").write_text( + yaml.safe_dump( + { + "name": name, + "version": "1.0.0", + "target": "copilot", + "dependencies": {"apm": deps, "mcp": []}, + } + ), + encoding="utf-8", + ) + (project / ".github").mkdir(exist_ok=True) + (project / ".github" / "copilot-instructions.md").write_text("# Project\n", encoding="utf-8") + + +def _read_lockfile(project: Path) -> dict | None: + path = project / "apm.lock.yaml" + if not path.exists(): + return None + return yaml.safe_load(path.read_text(encoding="utf-8")) + + +def _find_locked(lockfile: dict, repo_url: str) -> dict | None: + deps = lockfile.get("dependencies") if lockfile else None + if not deps: + return None + if isinstance(deps, dict): + return deps.get(repo_url) + for entry in deps: + if entry.get("repo_url") == repo_url: + return entry + return None + + +def _run_install( + runner: CliRunner, + project: Path, + monkeypatch: pytest.MonkeyPatch, + args: list[str] | None = None, +): + monkeypatch.chdir(project) + with patch(_PATCH_UPDATES, return_value=None): + return runner.invoke(cli, ["install", *(args or [])], catch_exceptions=False) + + +# --------------------------------------------------------------------------- +# Promise A: highest matching tag wins +# Promise B: lockfile records all four semver fields +# --------------------------------------------------------------------------- + + +class TestSemverRangeResolves: + def test_caret_range_resolves_to_highest_matching_tag_and_lockfile_records_fields( + self, + runner: CliRunner, + tmp_path: Path, + monkeypatch: pytest.MonkeyPatch, + ) -> None: + """``acme/widget#^1.2.0`` against {v1.0.0, v1.2.3, v1.5.0, v2.0.0} picks v1.5.0. + + Asserts the lockfile records ``constraint``, ``resolved_tag``, + ``resolved_commit``, ``version``, and ``resolved_at`` so future + replays are deterministic. + """ + project = tmp_path / "promise-ab" + _write_apm_yml(project, ["acme/widget#^1.2.0"]) + + rr = _RefResolverCallRecorder({"acme/widget": _refs_v_prefixed()}) + dl = _DownloaderStub({"v1.5.0": "3" * 40}) + rr.install(monkeypatch) + dl.install(monkeypatch) + + result = _run_install(runner, project, monkeypatch) + assert result.exit_code == 0, f"install failed:\n{result.output}" + + lockfile = _read_lockfile(project) + assert lockfile is not None, "apm.lock.yaml was not written" + + locked = _find_locked(lockfile, "acme/widget") + assert locked is not None, f"acme/widget missing from lockfile: {lockfile}" + + assert locked.get("constraint") == "^1.2.0" + assert locked.get("resolved_tag") == "v1.5.0" + assert locked.get("version") == "1.5.0" + assert locked.get("resolved_commit") == "3" * 40 + assert locked.get("resolved_at"), "resolved_at timestamp missing" + + +# --------------------------------------------------------------------------- +# Promise C: second install is offline (lockfile replay) +# --------------------------------------------------------------------------- + + +class TestLockfileReplayIsOffline: + def test_reinstall_with_unchanged_manifest_does_not_call_ref_resolver( + self, + runner: CliRunner, + tmp_path: Path, + monkeypatch: pytest.MonkeyPatch, + ) -> None: + """First install hits ls-remote; second install replays from lockfile. + + The recorder asserts ``list_remote_refs`` was called exactly once + (during the first install) and never during the second install. + """ + project = tmp_path / "promise-c" + _write_apm_yml(project, ["acme/widget#^1.2.0"]) + + rr = _RefResolverCallRecorder({"acme/widget": _refs_v_prefixed()}) + dl = _DownloaderStub({"v1.5.0": "3" * 40}) + rr.install(monkeypatch) + dl.install(monkeypatch) + + first = _run_install(runner, project, monkeypatch) + assert first.exit_code == 0, first.output + assert rr.calls == ["acme/widget"], f"first install should ls-remote once, got: {rr.calls}" + + second = _run_install(runner, project, monkeypatch) + assert second.exit_code == 0, second.output + assert rr.calls == ["acme/widget"], ( + f"second install must replay from lockfile (no new ls-remote); " + f"calls after second install: {rr.calls}" + ) + + +# --------------------------------------------------------------------------- +# Promise D: tag-pattern fallback order +# --------------------------------------------------------------------------- + + +class TestTagPatternFallback: + def test_name_dashv_pattern_matches_when_v_pattern_has_no_candidates( + self, + runner: CliRunner, + tmp_path: Path, + monkeypatch: pytest.MonkeyPatch, + ) -> None: + """Repo with only ``widget--v1.x.y`` tags resolves via second pattern. + + The default ``v{version}`` pattern finds no candidates; the + ``{name}--v{version}`` pattern scopes to this package only and + picks ``widget--v1.3.0`` (ignoring ``otherpkg--v9.9.9``). + """ + project = tmp_path / "promise-d" + _write_apm_yml(project, ["acme/widget#^1.0.0"]) + + rr = _RefResolverCallRecorder({"acme/widget": _refs_only_name_dashv()}) + dl = _DownloaderStub({"widget--v1.3.0": "b" * 40}) + rr.install(monkeypatch) + dl.install(monkeypatch) + + result = _run_install(runner, project, monkeypatch) + assert result.exit_code == 0, result.output + + locked = _find_locked(_read_lockfile(project), "acme/widget") + assert locked is not None + assert locked.get("resolved_tag") == "widget--v1.3.0" + assert locked.get("version") == "1.3.0" + # Critical: the other package's tag must not leak into this resolution. + assert locked.get("version") != "9.9.9" + + def test_bare_version_pattern_is_third_pattern_fallback( + self, + runner: CliRunner, + tmp_path: Path, + monkeypatch: pytest.MonkeyPatch, + ) -> None: + """When neither default pattern matches, bare ``{version}`` is tried.""" + project = tmp_path / "promise-d-bare" + _write_apm_yml(project, ["acme/widget#^1.0.0"]) + + rr = _RefResolverCallRecorder({"acme/widget": _refs_only_bare()}) + dl = _DownloaderStub({"1.4.2": "e" * 40}) + rr.install(monkeypatch) + dl.install(monkeypatch) + + result = _run_install(runner, project, monkeypatch) + assert result.exit_code == 0, result.output + + locked = _find_locked(_read_lockfile(project), "acme/widget") + assert locked is not None + assert locked.get("resolved_tag") == "1.4.2" + assert locked.get("version") == "1.4.2" + + +# --------------------------------------------------------------------------- +# Promise E: constraint change re-resolves +# Promise F: drift between locked constraint and manifest constraint +# --------------------------------------------------------------------------- + + +class TestConstraintChangeReResolves: + def test_lockfile_constraint_change_with_stale_install_path_re_resolves( + self, + runner: CliRunner, + tmp_path: Path, + monkeypatch: pytest.MonkeyPatch, + ) -> None: + """Drift sub-case (Promise F): when the install path is missing + (cache pruned, ``apm_modules`` deleted) and the lockfile constraint + differs from the manifest, the resolver re-runs without ``--update``. + + Exercises the ``_maybe_resolve_git_semver`` branch where + ``locked.constraint != constraint`` skips the lockfile replay and + falls through to ``GitSemverResolver.resolve``. + """ + import shutil + + project = tmp_path / "promise-f" + _write_apm_yml(project, ["acme/widget#^1.2.0"]) + + rr = _RefResolverCallRecorder({"acme/widget": _refs_v_prefixed()}) + dl = _DownloaderStub({"v1.5.0": "3" * 40, "v2.0.0": "4" * 40}) + rr.install(monkeypatch) + dl.install(monkeypatch) + + first = _run_install(runner, project, monkeypatch) + assert first.exit_code == 0, first.output + + # Simulate a cache-pruned environment: drop the materialised dep + # but keep the lockfile, then bump the constraint. + shutil.rmtree(project / "apm_modules", ignore_errors=True) + _write_apm_yml(project, ["acme/widget#^2.0.0"]) + clear_apm_yml_cache() + + second = _run_install(runner, project, monkeypatch) + assert second.exit_code == 0, second.output + + # The drift branch in _maybe_resolve_git_semver fired -- ls-remote + # was called and the lockfile records the new constraint. + assert rr.calls.count("acme/widget") == 2, ( + f"expected 2 ls-remote calls (initial + drift), got: {rr.calls}" + ) + locked = _find_locked(_read_lockfile(project), "acme/widget") + assert locked["constraint"] == "^2.0.0" + assert locked["resolved_tag"] == "v2.0.0" + + +# --------------------------------------------------------------------------- +# Promise G: literal ref bypasses the semver resolver +# --------------------------------------------------------------------------- + + +class TestLiteralRefUnchanged: + def test_literal_tag_ref_does_not_invoke_semver_resolver( + self, + runner: CliRunner, + tmp_path: Path, + monkeypatch: pytest.MonkeyPatch, + ) -> None: + """``ref: v1.2.3`` (literal tag) keeps existing behaviour. + + No ``list_remote_refs`` call, no ``constraint``/``resolved_tag`` + fields in the lockfile entry -- these are reserved for the semver + path. + """ + project = tmp_path / "promise-g" + _write_apm_yml(project, ["acme/widget#v1.2.3"]) + + rr = _RefResolverCallRecorder({"acme/widget": _refs_v_prefixed()}) + dl = _DownloaderStub({"v1.2.3": "2" * 40}) + rr.install(monkeypatch) + dl.install(monkeypatch) + + result = _run_install(runner, project, monkeypatch) + assert result.exit_code == 0, result.output + + # The literal-ref path must not touch the semver resolver. + assert rr.calls == [], f"literal ref must not invoke list_remote_refs; got: {rr.calls}" + + locked = _find_locked(_read_lockfile(project), "acme/widget") + assert locked is not None + # Semver-specific fields stay absent for literal refs. + assert "constraint" not in locked or locked.get("constraint") is None + assert "resolved_tag" not in locked or locked.get("resolved_tag") is None + # The literal ref is still pinned through the normal resolved_ref field. + assert locked.get("resolved_ref") == "v1.2.3" + + +# --------------------------------------------------------------------------- +# Promise H: AuthResolver token threads into RefResolver for private repos +# --------------------------------------------------------------------------- + + +class TestAuthTokenThreadedToLsRemote: + def test_github_apm_pat_reaches_ref_resolver_for_semver_dep( + self, + runner: CliRunner, + tmp_path: Path, + monkeypatch: pytest.MonkeyPatch, + ) -> None: + """Regression-trap for the auth-blocking panel finding on PR #1496. + + With ``GITHUB_APM_PAT`` set, an ``apm install`` of a semver-range + git-source dep must pass that token to the ``RefResolver`` that + runs ``git ls-remote`` -- otherwise private repos fail in CI + environments without a system git credential helper. + """ + monkeypatch.setenv("GITHUB_APM_PAT", "ghp_e2e_token_abc123") + monkeypatch.delenv("GITHUB_TOKEN", raising=False) + + project = tmp_path / "promise-h" + _write_apm_yml(project, ["acme/widget#^1.2.0"]) + + rr = _RefResolverCallRecorder({"acme/widget": _refs_v_prefixed()}) + dl = _DownloaderStub({"v1.5.0": "3" * 40}) + rr.install(monkeypatch) + dl.install(monkeypatch) + + result = _run_install(runner, project, monkeypatch) + assert result.exit_code == 0, result.output + + # At least one RefResolver instance must have been constructed + # with the configured PAT. + tokens_seen = [kw.get("token") for kw in rr.init_kwargs] + assert "ghp_e2e_token_abc123" in tokens_seen, ( + "AuthResolver did not thread GITHUB_APM_PAT into the " + f"RefResolver used for ls-remote. token kwargs seen: {tokens_seen}" + ) + + +# --------------------------------------------------------------------------- +# Promise I: no matching tag -> clear, actionable error +# --------------------------------------------------------------------------- + + +class TestNoMatchingTagError: + def test_no_matching_tag_exits_nonzero_with_actionable_message( + self, + runner: CliRunner, + tmp_path: Path, + monkeypatch: pytest.MonkeyPatch, + ) -> None: + """``^1.2.0`` against a repo with only ``v0.9.0`` fails with a + message that names the constraint, the repo, and the tags considered.""" + project = tmp_path / "promise-i" + _write_apm_yml(project, ["acme/widget#^1.2.0"]) + + rr = _RefResolverCallRecorder({"acme/widget": _refs_no_match()}) + dl = _DownloaderStub({}) + rr.install(monkeypatch) + dl.install(monkeypatch) + + result = _run_install(runner, project, monkeypatch) + + combined = (result.output or "") + (result.stderr or "") + # npm/pip/cargo convention: ANY reported install failure exits + # non-zero so CI and scripts can detect failure without parsing + # stderr. Regression trap for Bug 2 (#1496 e2e wave): the CLI + # used to exit 0 even when "Installation failed with N error(s)" + # was printed. + assert result.exit_code != 0, ( + f"install with no-matching-tag must exit non-zero; got 0\n{combined}" + ) + assert "failed" in combined.lower(), ( + f"expected an explicit failure marker in output:\n{combined}" + ) + + # The diagnostic must name (a) the constraint, (b) the repo, and + # (c) at least one tag considered so the user can widen the range. + assert "^1.2.0" in combined, f"constraint not surfaced:\n{combined}" + assert "acme/widget" in combined, f"repo not surfaced:\n{combined}" + assert "v0.9.0" in combined, f"available tags not surfaced:\n{combined}" + + # No lockfile entry for the failed dep. + lockfile = _read_lockfile(project) + locked = _find_locked(lockfile, "acme/widget") if lockfile else None + assert locked is None or not locked.get("resolved_commit"), ( + "failed semver resolution must not write a half-populated lockfile entry" + ) + + +# --------------------------------------------------------------------------- +# Bug 1 (#1496 e2e wave): apm install --update must re-resolve git-semver +# constraints against the latest remote tags even when the install path +# already exists on disk. npm/cargo/bundler precedent: --update is the +# explicit re-resolve trigger; the install-path cache short-circuit must +# not swallow it. +# --------------------------------------------------------------------------- + + +class TestUpdateReResolvesGitSemver: + def test_update_flag_re_resolves_when_install_path_exists_and_new_tag_published( + self, + runner: CliRunner, + tmp_path: Path, + monkeypatch: pytest.MonkeyPatch, + ) -> None: + """First install pins v1.2.3; new tag v1.5.0 published upstream; + ``apm install --update`` must call ls-remote again and the lockfile + must record v1.5.0. + + Regression trap for the silent no-op surfaced in the e2e wave on + PR #1496: ``download_callback`` returned early on + ``install_path.exists()`` before ``_maybe_resolve_git_semver`` + could run, so ``--update`` never re-resolved the constraint. + """ + project = tmp_path / "bug1-update" + _write_apm_yml(project, ["acme/widget#^1.2.0"]) + + # Initial remote: tags up through v1.2.3 only. + initial_refs = [ + RemoteRef(name="refs/heads/main", sha="0" * 40), + RemoteRef(name="refs/tags/v1.0.0", sha="1" * 40), + RemoteRef(name="refs/tags/v1.2.3", sha="2" * 40), + ] + rr = _RefResolverCallRecorder({"acme/widget": initial_refs}) + dl = _DownloaderStub({"v1.2.3": "2" * 40, "v1.5.0": "3" * 40}) + rr.install(monkeypatch) + dl.install(monkeypatch) + + first = _run_install(runner, project, monkeypatch) + assert first.exit_code == 0, first.output + + # Clear the module-level apm.yml parse cache so the second invocation + # re-parses apm.yml from disk. In production each CLI invocation is a + # fresh process (empty cache); under CliRunner both invocations share + # one Python session, so without this clear the cached APMPackage + # instance (whose DependencyReference.reference was mutated to + # ``v1.2.3`` by the first run's semver resolver) leaks into the + # second run and disguises the cache-pre-purge gate as ineffective. + from apm_cli.models.apm_package import clear_apm_yml_cache as _clear_yml + + _clear_yml() + + locked = _find_locked(_read_lockfile(project), "acme/widget") + assert locked is not None and locked.get("resolved_tag") == "v1.2.3", ( + f"first install must lock v1.2.3, got: {locked}" + ) + assert (project / "apm_modules" / "acme" / "widget").exists(), ( + "first install must materialise the dep so the cache short-circuit " + "fires on the second invocation" + ) + assert rr.calls.count("acme/widget") == 1, ( + f"first install should ls-remote once, got: {rr.calls}" + ) + + # Upstream publishes v1.5.0. The install path still exists from + # the first run -- this is the surface that hid the bug. + rr.refs_by_repo["acme/widget"] = [ + RemoteRef(name="refs/heads/main", sha="0" * 40), + RemoteRef(name="refs/tags/v1.0.0", sha="1" * 40), + RemoteRef(name="refs/tags/v1.2.3", sha="2" * 40), + RemoteRef(name="refs/tags/v1.5.0", sha="3" * 40), + ] + + second = _run_install(runner, project, monkeypatch, args=["--update"]) + assert second.exit_code == 0, second.output + + # --update must trigger a second ls-remote (the silent-no-op bug + # would leave this at 1). + assert rr.calls.count("acme/widget") == 2, ( + f"--update must re-resolve via ls-remote, got calls: {rr.calls}" + ) + + # Lockfile must now record the newly-published highest tag. + locked_after = _find_locked(_read_lockfile(project), "acme/widget") + assert locked_after is not None + assert locked_after.get("resolved_tag") == "v1.5.0", ( + f"--update must update resolved_tag to v1.5.0, got: {locked_after}" + ) + assert locked_after.get("version") == "1.5.0", ( + f"--update must update version to 1.5.0, got: {locked_after}" + ) + assert locked_after.get("resolved_commit") == "3" * 40, ( + f"--update must update resolved_commit, got: {locked_after}" + ) + + +# --------------------------------------------------------------------------- +# Bug 2 (#1496 e2e wave): apm install must exit non-zero whenever +# "Installation failed with N error(s)" is reported. Matches npm / pip / +# cargo: ANY install failure -> non-zero exit so CI scripts can detect it. +# The TestNoMatchingTagError class above also pins the exit-code assertion +# for the per-dep failure path; this class isolates the contract at the +# summary level. +# --------------------------------------------------------------------------- + + +class TestInstallExitCodeOnReportedErrors: + def test_install_with_unsatisfiable_semver_exits_nonzero( + self, + runner: CliRunner, + tmp_path: Path, + monkeypatch: pytest.MonkeyPatch, + ) -> None: + """A direct dep with an unsatisfiable semver constraint produces + a reported error -> exit code MUST be non-zero. + + Regression trap for Bug 2 (#1496 e2e wave). + """ + project = tmp_path / "bug2-exit" + _write_apm_yml(project, ["acme/widget#^9.9.0"]) + + rr = _RefResolverCallRecorder({"acme/widget": _refs_v_prefixed()}) + dl = _DownloaderStub({}) + rr.install(monkeypatch) + dl.install(monkeypatch) + + result = _run_install(runner, project, monkeypatch) + + combined = (result.output or "") + (result.stderr or "") + assert result.exit_code != 0, ( + f"install with reported errors must exit non-zero; got 0\n{combined}" + ) diff --git a/tests/integration/test_global_mcp_lockfile_e2e.py b/tests/integration/test_global_mcp_lockfile_e2e.py index 6910d717e8..18da1fdc55 100644 --- a/tests/integration/test_global_mcp_lockfile_e2e.py +++ b/tests/integration/test_global_mcp_lockfile_e2e.py @@ -11,7 +11,7 @@ import os from pathlib import Path -from unittest.mock import patch +from unittest.mock import MagicMock, patch import pytest import yaml @@ -19,6 +19,26 @@ from apm_cli.deps.lockfile import LockedDependency, LockFile + +def _stub_downloader_for_lockfile(mock_dl_cls) -> None: + """Configure a patched ``GitHubPackageDownloader`` class mock so the + install pipeline's lockfile writer can serialize the downloader's + return value. Without string-typed ``resolved_commit`` / + ``package_type.value``, pyyaml raises and the install reports an + error, which under Bug 2 (#1496) exits non-zero. These tests only + care about lockfile MCP-server bookkeeping, not downloader internals. + """ + instance = mock_dl_cls.return_value + pkg_info = MagicMock() + pkg_info.resolved_reference.resolved_commit = "0" * 40 + pkg_info.resolved_reference.ref_name = "main" + pkg_info.resolved_reference.is_branch = True + pkg_info.resolved_reference.is_tag = False + pkg_info.resolved_reference.is_sha = False + pkg_info.package_type.value = "apm_package" + instance.download_package.return_value = pkg_info + + # --------------------------------------------------------------------------- # Helpers # --------------------------------------------------------------------------- @@ -124,6 +144,7 @@ def test_global_install_writes_mcp_servers_to_global_lockfile( global_env, ): """MCP server entries must land in ~/.apm/apm.lock.yaml, not cwd.""" + _stub_downloader_for_lockfile(mock_dl_cls) fake_home, work_dir, runner = global_env apm_dir = fake_home / ".apm" apm_modules = apm_dir / "apm_modules" @@ -207,6 +228,7 @@ def test_global_install_no_mcp_clears_servers_in_global_lockfile( global_env, ): """When the manifest has no MCP deps, global lockfile mcp_servers is cleared.""" + _stub_downloader_for_lockfile(mock_dl_cls) fake_home, work_dir, runner = global_env apm_dir = fake_home / ".apm" apm_modules = apm_dir / "apm_modules" diff --git a/tests/integration/test_selective_install_mcp.py b/tests/integration/test_selective_install_mcp.py index 91705e0776..18159e1e03 100644 --- a/tests/integration/test_selective_install_mcp.py +++ b/tests/integration/test_selective_install_mcp.py @@ -12,7 +12,7 @@ import json import os from pathlib import Path -from unittest.mock import MagicMock, patch # noqa: F401 +from unittest.mock import MagicMock, patch import pytest import yaml @@ -20,6 +20,28 @@ from apm_cli.deps.lockfile import LockedDependency, LockFile + +def _stub_downloader_for_lockfile(mock_dl_cls) -> None: + """Configure a patched ``GitHubPackageDownloader`` class mock so the + install pipeline's lockfile writer can serialize the downloader's + return value. Without this, ``resolved_reference.resolved_commit`` + is an auto-generated ``MagicMock`` that pyyaml cannot represent, + which produces a diagnostic error -> non-zero exit code under the + Bug 2 contract (#1496). These tests only care about lockfile + MCP-server bookkeeping, not the downloader's wire-format, so the + stub is intentionally minimal. + """ + instance = mock_dl_cls.return_value + pkg_info = MagicMock() + pkg_info.resolved_reference.resolved_commit = "0" * 40 + pkg_info.resolved_reference.ref_name = "main" + pkg_info.resolved_reference.is_branch = True + pkg_info.resolved_reference.is_tag = False + pkg_info.resolved_reference.is_sha = False + pkg_info.package_type.value = "apm_package" + instance.download_package.return_value = pkg_info + + # --------------------------------------------------------------------------- # Helpers # --------------------------------------------------------------------------- @@ -155,6 +177,7 @@ class TestSelectiveInstallTransitiveMCPIntegration: def test_lockfile_records_transitive_mcp_servers( self, mock_dl_cls, mock_mcp_install, mock_validate, mock_updates, cli_env ): + _stub_downloader_for_lockfile(mock_dl_cls) tmp_path, runner = cli_env from apm_cli.cli import cli @@ -191,6 +214,7 @@ def test_install_mcp_receives_transitive_deps( self, mock_dl_cls, mock_mcp_install, mock_validate, mock_updates, cli_env ): """_install_mcp_dependencies must be called with transitive deps.""" + _stub_downloader_for_lockfile(mock_dl_cls) tmp_path, runner = cli_env # noqa: RUF059 from apm_cli.cli import cli @@ -222,6 +246,7 @@ class TestDeepChainIntegration: def test_deep_chain_mcp_in_lockfile( self, mock_dl_cls, mock_mcp_install, mock_validate, mock_updates, tmp_path ): + _stub_downloader_for_lockfile(mock_dl_cls) orig_cwd = os.getcwd() os.chdir(tmp_path) try: @@ -295,6 +320,7 @@ class TestDiamondDependencyIntegration: def test_diamond_mcp_in_lockfile( self, mock_dl_cls, mock_mcp_install, mock_validate, mock_updates, tmp_path ): + _stub_downloader_for_lockfile(mock_dl_cls) orig_cwd = os.getcwd() os.chdir(tmp_path) try: @@ -375,6 +401,7 @@ class TestMultiPackageSelectiveInstallIntegration: def test_multiple_packages_mcp_merged( self, mock_dl_cls, mock_mcp_install, mock_validate, mock_updates, tmp_path ): + _stub_downloader_for_lockfile(mock_dl_cls) orig_cwd = os.getcwd() os.chdir(tmp_path) try: @@ -449,6 +476,7 @@ class TestFullInstallTransitiveMCPIntegration: def test_full_install_collects_transitive_mcp( self, mock_dl_cls, mock_mcp_install, mock_updates, cli_env ): + _stub_downloader_for_lockfile(mock_dl_cls) tmp_path, runner = cli_env from apm_cli.cli import cli @@ -474,6 +502,7 @@ class TestStaleRemovalAfterUpdate: def test_stale_mcp_removed_on_update( self, mock_dl_cls, mock_mcp_install, mock_updates, tmp_path ): + _stub_downloader_for_lockfile(mock_dl_cls) orig_cwd = os.getcwd() os.chdir(tmp_path) try: @@ -558,6 +587,7 @@ class TestNoMCPWhenOnlyAPM: @patch("apm_cli.commands._helpers.check_for_updates", return_value=None) @patch("apm_cli.deps.github_downloader.GitHubPackageDownloader") def test_only_apm_preserves_mcp_servers(self, mock_dl_cls, mock_updates, cli_env): + _stub_downloader_for_lockfile(mock_dl_cls) tmp_path, runner = cli_env # Seed lockfile with existing MCP servers diff --git a/tests/unit/deps/test_git_semver_resolver.py b/tests/unit/deps/test_git_semver_resolver.py new file mode 100644 index 0000000000..136fb726bf --- /dev/null +++ b/tests/unit/deps/test_git_semver_resolver.py @@ -0,0 +1,337 @@ +"""Tests for ``apm_cli.deps.git_semver_resolver``.""" + +from __future__ import annotations + +from unittest.mock import MagicMock + +import pytest + +from apm_cli.deps.git_semver_resolver import ( + DEFAULT_TAG_PATTERNS, + GitSemverResolution, + GitSemverResolver, + NoMatchingTagError, + iter_semver_tags, +) +from apm_cli.marketplace.ref_resolver import RemoteRef + +# --------------------------------------------------------------------------- +# Test fixtures +# --------------------------------------------------------------------------- + + +def _refs_from(*tags_with_sha: tuple[str, str]) -> list[RemoteRef]: + """Build ``RemoteRef`` objects from ``(tag_name, sha)`` tuples.""" + return [RemoteRef(name=f"refs/tags/{name}", sha=sha) for name, sha in tags_with_sha] + + +def _ref_resolver_returning(refs: list[RemoteRef]) -> MagicMock: + """Build a stub ``RefResolver`` whose ``list_remote_refs`` returns *refs*.""" + rr = MagicMock() + rr.list_remote_refs.return_value = list(refs) + return rr + + +_FROZEN_NOW = "2024-06-15T12:00:00+00:00" + + +# --------------------------------------------------------------------------- +# GitSemverResolver.resolve() +# --------------------------------------------------------------------------- + + +class TestResolveCaretRange: + """Caret range resolves to the highest matching tag.""" + + def test_resolve_caret_range_picks_highest_matching_tag(self) -> None: + refs = _refs_from( + ("v1.0.0", "a" * 40), + ("v1.2.0", "b" * 40), + ("v1.5.3", "c" * 40), + ("v2.0.0", "d" * 40), + ) + resolver = GitSemverResolver(_ref_resolver_returning(refs)) + + result = resolver.resolve( + owner_repo="acme/some-skills", + package_name="some-skills", + constraint="^1.2.0", + now_iso=_FROZEN_NOW, + ) + + assert isinstance(result, GitSemverResolution) + assert result.constraint == "^1.2.0" + assert result.resolved_version == "1.5.3" + assert result.resolved_tag == "v1.5.3" + assert result.resolved_sha == "c" * 40 + assert result.matched_pattern == "v{version}" + assert result.resolved_at == _FROZEN_NOW + + +class TestPatternFallback: + """Pattern-list fallthrough behaviour.""" + + def test_resolve_falls_through_to_secondary_pattern_when_primary_misses(self) -> None: + # Only the {name}--v{version} convention is used; primary v{version} produces nothing. + refs = _refs_from( + ("some-skills--v1.2.0", "a" * 40), + ("some-skills--v1.4.0", "b" * 40), + ("unrelated-tag", "c" * 40), + ) + resolver = GitSemverResolver(_ref_resolver_returning(refs)) + + result = resolver.resolve( + owner_repo="acme/some-skills", + package_name="some-skills", + constraint="^1.0.0", + ) + + assert result.resolved_version == "1.4.0" + assert result.resolved_tag == "some-skills--v1.4.0" + assert result.matched_pattern == "{name}--v{version}" + + def test_resolve_prefers_higher_version_when_both_patterns_match(self) -> None: + # Both v{version} and {name}--v{version} match different versions. + # The picker chooses the highest version regardless of pattern order. + refs = _refs_from( + ("v1.2.0", "a" * 40), + ("some-skills--v1.5.3", "b" * 40), + ) + resolver = GitSemverResolver(_ref_resolver_returning(refs)) + + result = resolver.resolve( + owner_repo="acme/some-skills", + package_name="some-skills", + constraint="^1.0.0", + ) + + assert result.resolved_version == "1.5.3" + assert result.resolved_tag == "some-skills--v1.5.3" + assert result.matched_pattern == "{name}--v{version}" + + +class TestPrereleaseHandling: + """Prerelease tags are excluded by default and opt-in via flag.""" + + def test_resolve_excludes_prereleases_by_default(self) -> None: + refs = _refs_from( + ("v1.5.3", "a" * 40), + ("v2.0.0-beta.1", "b" * 40), + ) + resolver = GitSemverResolver(_ref_resolver_returning(refs)) + + result = resolver.resolve( + owner_repo="acme/foo", + package_name="foo", + constraint=">=1.0.0", + ) + + assert result.resolved_version == "1.5.3" + assert result.resolved_tag == "v1.5.3" + + def test_resolve_includes_prereleases_when_flag_set(self) -> None: + refs = _refs_from( + ("v1.5.3", "a" * 40), + ("v2.0.0-beta.1", "b" * 40), + ) + resolver = GitSemverResolver( + _ref_resolver_returning(refs), + include_prerelease=True, + ) + + result = resolver.resolve( + owner_repo="acme/foo", + package_name="foo", + constraint=">=1.0.0", + ) + + assert result.resolved_version == "2.0.0-beta.1" + assert result.resolved_tag == "v2.0.0-beta.1" + + +class TestNoMatch: + """Error path: no matching tag.""" + + def test_resolve_no_matching_tag_raises_with_actionable_hint(self) -> None: + refs = _refs_from( + ("v0.9.0", "a" * 40), + ("v1.0.0", "b" * 40), + ) + resolver = GitSemverResolver(_ref_resolver_returning(refs)) + + with pytest.raises(NoMatchingTagError) as excinfo: + resolver.resolve( + owner_repo="acme/foo", + package_name="foo", + constraint="^2.0.0", + ) + + err = excinfo.value + # The exception carries both a summary (what we looked at) and a hint + # (what the user can do). Both must be present and actionable. + assert "acme/foo" in err.summary + assert "^2.0.0" in err.summary + assert "v0.9.0" in err.summary or "v1.0.0" in err.summary + assert err.hint + assert "pin" in err.hint.lower() or "widen" in err.hint.lower() + + def test_resolve_no_tags_at_all_raises_with_empty_remote_hint(self) -> None: + resolver = GitSemverResolver(_ref_resolver_returning([])) + + with pytest.raises(NoMatchingTagError) as excinfo: + resolver.resolve( + owner_repo="acme/foo", + package_name="foo", + constraint="^1.0.0", + ) + + # When zero tag refs are present we still surface a usable message. + assert "no tag refs" in excinfo.value.summary.lower() + + +class TestRefResolverCaching: + """Two resolves against the same remote reuse the underlying cache.""" + + def test_resolve_uses_ref_resolver_cache_on_second_call(self) -> None: + refs = _refs_from( + ("v1.0.0", "a" * 40), + ("v1.2.0", "b" * 40), + ) + rr = _ref_resolver_returning(refs) + # Resolver delegates caching to the ref_resolver. Verify the resolver + # never reaches past list_remote_refs (i.e. we don't shortcircuit + # the cache contract by calling resolve_ref_sha directly). + resolver = GitSemverResolver(rr) + + resolver.resolve( + owner_repo="acme/foo", + package_name="foo", + constraint="^1.0.0", + ) + resolver.resolve( + owner_repo="acme/foo", + package_name="foo", + constraint="^1.0.0", + ) + + # list_remote_refs is the only network-ish call we make. + # The real RefResolver caches it; here we just assert we + # consistently route through that single entry point. + assert rr.list_remote_refs.call_count == 2 + rr.list_remote_refs.assert_called_with("acme/foo") + # Crucially we do NOT call resolve_ref_sha — that would bypass + # the cache for our concrete-tag lookup. + rr.resolve_ref_sha.assert_not_called() + + +class TestBareVersionTagFallback: + """Regression trap: literal ``1.2.3`` tag with no ``v`` prefix is resolvable.""" + + def test_resolve_bare_version_tag_falls_through_to_third_pattern(self) -> None: + # The only tag uses the bare-version convention. Default patterns + # (``v{version}`` and ``{name}--v{version}``) would miss; the + # bare-version fallback must catch it. + refs = _refs_from( + ("1.2.3", "a" * 40), + ("1.5.0", "b" * 40), + ) + resolver = GitSemverResolver(_ref_resolver_returning(refs)) + + result = resolver.resolve( + owner_repo="acme/bare", + package_name="bare", + constraint="^1.0.0", + ) + + assert result.resolved_version == "1.5.0" + assert result.resolved_tag == "1.5.0" + assert result.matched_pattern == "{version}" + + def test_resolve_v_prefixed_wins_over_bare_when_both_present(self) -> None: + # When both conventions exist, the primary v{version} pattern + # matches first and the bare fallback is never consulted. + # This is intentional: bare-version is a *fallback*, not a + # competing default. + refs = _refs_from( + ("v1.5.3", "a" * 40), + ("1.5.3", "b" * 40), + ) + resolver = GitSemverResolver(_ref_resolver_returning(refs)) + + result = resolver.resolve( + owner_repo="acme/mixed", + package_name="mixed", + constraint="^1.0.0", + ) + + assert result.resolved_tag == "v1.5.3" + assert result.matched_pattern == "v{version}" + + +# --------------------------------------------------------------------------- +# iter_semver_tags (internal but worth its own surface) +# --------------------------------------------------------------------------- + + +class TestIterSemverTags: + """The internal tag iterator skips non-tag refs and invalid versions.""" + + def test_skips_branch_refs(self) -> None: + refs = [ + RemoteRef(name="refs/heads/main", sha="a" * 40), + RemoteRef(name="refs/tags/v1.0.0", sha="b" * 40), + ] + out = iter_semver_tags(refs, package_name="foo", patterns=DEFAULT_TAG_PATTERNS) + assert len(out) == 1 + assert out[0][1] == "v1.0.0" + + def test_skips_non_semver_tags(self) -> None: + refs = [ + RemoteRef(name="refs/tags/release-candidate", sha="a" * 40), + RemoteRef(name="refs/tags/v1.0.0", sha="b" * 40), + ] + out = iter_semver_tags(refs, package_name="foo", patterns=DEFAULT_TAG_PATTERNS) + tags = [t[1] for t in out] + assert tags == ["v1.0.0"] + + def test_name_placeholder_scoped_to_package_name(self) -> None: + """``{name}--v{version}`` must only match tags for the requested package. + + Regression-trap for PR #1496 review thread: previously + ``iter_semver_tags`` accepted ``package_name`` but never used it, + leaving ``{name}`` as a wildcard (``[^/]+``). In a repo that + publishes multiple ``{name}--v{version}`` tag families, the + resolver could then accept a sibling package's tag (e.g. + ``otherpkg--v9.9.9``) when asked to resolve ``mypkg``. + """ + refs = _refs_from( + ("mypkg--v1.0.0", "a" * 40), + ("mypkg--v1.2.0", "b" * 40), + ("otherpkg--v9.9.9", "c" * 40), + ) + out = iter_semver_tags( + refs, + package_name="mypkg", + patterns=("{name}--v{version}",), + ) + tags = sorted(t[1] for t in out) + assert tags == ["mypkg--v1.0.0", "mypkg--v1.2.0"] + assert "otherpkg--v9.9.9" not in tags + + def test_resolver_does_not_pick_sibling_package_tag(self) -> None: + """Highest-version picker must ignore tags belonging to other packages.""" + refs = _refs_from( + ("mypkg--v1.0.0", "a" * 40), + ("otherpkg--v9.9.9", "c" * 40), + ) + resolver = GitSemverResolver(_ref_resolver_returning(refs)) + + result = resolver.resolve( + owner_repo="acme/mypkg", + package_name="mypkg", + constraint=">=1.0.0", + tag_patterns=("{name}--v{version}",), + ) + + assert result.resolved_tag == "mypkg--v1.0.0" + assert result.resolved_version == "1.0.0" diff --git a/tests/unit/deps/test_lockfile_git_semver.py b/tests/unit/deps/test_lockfile_git_semver.py new file mode 100644 index 0000000000..475e21d8b1 --- /dev/null +++ b/tests/unit/deps/test_lockfile_git_semver.py @@ -0,0 +1,233 @@ +"""Tests for git-semver-resolution fields on ``LockedDependency``.""" + +from __future__ import annotations + +from apm_cli.deps.git_semver_resolver import GitSemverResolution +from apm_cli.deps.installed_package import InstalledPackage +from apm_cli.deps.lockfile import LockedDependency, LockFile +from apm_cli.models.dependency.reference import DependencyReference + + +def _make_dep_ref(repo_url: str = "acme/some-skills", reference: str = "^1.2.0"): + return DependencyReference( + repo_url=repo_url, + host="github.com", + reference=reference, + source="git", + ) + + +def _make_resolution(constraint: str = "^1.2.0") -> GitSemverResolution: + return GitSemverResolution( + constraint=constraint, + resolved_version="1.5.3", + resolved_tag="v1.5.3", + resolved_sha="c" * 40, + matched_pattern="v{version}", + resolved_at="2024-06-15T12:00:00+00:00", + ) + + +class TestLockedDependencySerialization: + """Resolution fields round-trip through to_dict / from_dict.""" + + def test_git_semver_dep_writes_constraint_and_resolved_tag_fields(self) -> None: + dep_ref = _make_dep_ref() + resolution = _make_resolution() + + locked = LockedDependency.from_dependency_ref( + dep_ref=dep_ref, + resolved_commit=resolution.resolved_sha, + depth=1, + resolved_by=None, + git_semver_resolution=resolution, + ) + + # All three new fields are populated on the dataclass. + assert locked.constraint == "^1.2.0" + assert locked.resolved_tag == "v1.5.3" + assert locked.resolved_at == "2024-06-15T12:00:00+00:00" + # The concrete tag becomes ``resolved_ref`` so re-installs route through + # the literal-tag git path, NOT the original semver range. + assert locked.resolved_ref == "v1.5.3" + # Version is recorded too (so audits can answer "what version is locked?"). + assert locked.version == "1.5.3" + + d = locked.to_dict() + assert d["constraint"] == "^1.2.0" + assert d["resolved_tag"] == "v1.5.3" + assert d["resolved_at"] == "2024-06-15T12:00:00+00:00" + assert d["resolved_ref"] == "v1.5.3" + assert d["version"] == "1.5.3" + + def test_git_semver_lockfile_roundtrips_through_to_dict_from_dict(self) -> None: + dep_ref = _make_dep_ref() + resolution = _make_resolution() + locked = LockedDependency.from_dependency_ref( + dep_ref=dep_ref, + resolved_commit=resolution.resolved_sha, + depth=1, + resolved_by=None, + git_semver_resolution=resolution, + ) + + rebuilt = LockedDependency.from_dict(locked.to_dict()) + + assert rebuilt.constraint == locked.constraint + assert rebuilt.resolved_tag == locked.resolved_tag + assert rebuilt.resolved_at == locked.resolved_at + assert rebuilt.resolved_ref == locked.resolved_ref + assert rebuilt.version == locked.version + # Going back through to_dict produces the identical mapping. + assert rebuilt.to_dict() == locked.to_dict() + + def test_dep_with_no_resolution_omits_git_semver_fields(self) -> None: + # Plain ref (branch / literal tag / SHA) must not introduce + # empty ``constraint`` keys -- those would dirty existing lockfiles. + dep_ref = _make_dep_ref(reference="main") + locked = LockedDependency.from_dependency_ref( + dep_ref=dep_ref, + resolved_commit="a" * 40, + depth=1, + resolved_by=None, + ) + + d = locked.to_dict() + assert "constraint" not in d + assert "resolved_tag" not in d + assert "resolved_at" not in d + # The legacy resolved_ref still tracks the manifest ref. + assert d["resolved_ref"] == "main" + + +class TestLockfileVersion: + """Lockfile version remains v2 after git-semver resolution.""" + + def test_lockfile_version_remains_v2_after_git_semver_resolution(self) -> None: + dep_ref = _make_dep_ref() + resolution = _make_resolution() + lock = LockFile() + locked = LockedDependency.from_dependency_ref( + dep_ref=dep_ref, + resolved_commit=resolution.resolved_sha, + depth=1, + resolved_by=None, + git_semver_resolution=resolution, + ) + lock.add_dependency(locked) + + yaml_str = lock.to_yaml() + # Bumping the schema for an optional, forward-compatible field + # would force a "lockfile version mismatch" warning across every + # existing lockfile in the wild. v2 stays. + assert "lockfile_version: '2'" in yaml_str or 'lockfile_version: "2"' in yaml_str + + rebuilt = LockFile.from_yaml(yaml_str) + assert rebuilt.lockfile_version == "2" + + def test_lockfile_version_stays_v1_when_only_git_deps_have_no_semver(self) -> None: + # Resolution fields are forward-compat additions; their *presence* + # alone must not trigger a v2 bump for projects that only use + # plain git deps with literal refs. + dep_ref = _make_dep_ref(reference="v1.0.0") + lock = LockFile() + locked = LockedDependency.from_dependency_ref( + dep_ref=dep_ref, + resolved_commit="a" * 40, + depth=1, + resolved_by=None, + ) + lock.add_dependency(locked) + + yaml_str = lock.to_yaml() + assert "lockfile_version: '1'" in yaml_str or 'lockfile_version: "1"' in yaml_str + + +class TestForwardCompatUnknownKeys: + """Forward-compat trap: unknown keys survive a round-trip.""" + + def test_lockfile_preserves_unknown_keys_on_roundtrip(self) -> None: + # Simulate an older APM build reading a lockfile written by a + # newer build with future fields. The fields must NOT be silently + # dropped on re-emit. + future_payload = { + "repo_url": "acme/some-skills", + "host": "github.com", + "resolved_commit": "c" * 40, + "resolved_ref": "v1.5.3", + "constraint": "^1.2.0", + "resolved_tag": "v1.5.3", + "future_field_we_dont_know": "some-value", + "another_future_dict": {"nested": True}, + } + + locked = LockedDependency.from_dict(future_payload) + emitted = locked.to_dict() + + assert emitted["future_field_we_dont_know"] == "some-value" + assert emitted["another_future_dict"] == {"nested": True} + # Known fields still serialize correctly. + assert emitted["constraint"] == "^1.2.0" + assert emitted["resolved_tag"] == "v1.5.3" + + +class TestInstalledPackagePlumbing: + """``from_installed_packages`` propagates the resolution through.""" + + def test_installed_package_carries_resolution_into_lockfile(self) -> None: + dep_ref = _make_dep_ref() + resolution = _make_resolution() + pkg = InstalledPackage( + dep_ref=dep_ref, + resolved_commit=resolution.resolved_sha, + depth=1, + resolved_by=None, + git_semver_resolution=resolution, + ) + + lock = LockFile.from_installed_packages( + installed_packages=[pkg], + dependency_graph=None, # unused by from_installed_packages + ) + + deps = lock.get_all_dependencies() + assert len(deps) == 1 + ld = deps[0] + assert ld.constraint == "^1.2.0" + assert ld.resolved_tag == "v1.5.3" + assert ld.resolved_ref == "v1.5.3" + assert ld.version == "1.5.3" + + +class TestMutualExclusivity: + """``from_dependency_ref`` enforces resolution-source mutual exclusivity. + + Regression-trap for PR #1496 review thread: the docstring promises + ``git_semver_resolution`` is mutually exclusive with + ``registry_resolution``, but the constructor previously combined + fields from both (e.g. ``source="registry"`` while also setting + ``constraint``/``resolved_tag`` and overriding ``resolved_ref``). + """ + + def test_passing_both_resolution_sources_raises_value_error(self) -> None: + from apm_cli.deps.registry.resolver import RegistryResolution + + dep_ref = _make_dep_ref() + git_res = _make_resolution() + reg_res = RegistryResolution( + resolved_url="https://registry.example/pkg/1.0.0.tgz", + resolved_hash="sha256-abc", + version="1.0.0", + ) + + import pytest + + with pytest.raises(ValueError, match=r"mutually exclusive"): + LockedDependency.from_dependency_ref( + dep_ref=dep_ref, + resolved_commit="d" * 40, + depth=1, + resolved_by=None, + registry_resolution=reg_res, + git_semver_resolution=git_res, + ) diff --git a/tests/unit/install/test_cached_semver_rebuild.py b/tests/unit/install/test_cached_semver_rebuild.py new file mode 100644 index 0000000000..f4103a5191 --- /dev/null +++ b/tests/unit/install/test_cached_semver_rebuild.py @@ -0,0 +1,93 @@ +"""Tests for the cached git-semver resolution rebuild gate (PR #1496). + +Regression-trap for the PR #1496 Copilot review thread: previously the +``CachedDependencySource.acquire`` path only checked +``dep_locked_chk.constraint`` and back-filled the other fields with +empty strings, which would propagate an incomplete +``GitSemverResolution`` into ``InstalledPackage`` and cause the +lockfile to be rewritten with empty ``version`` / ``resolved_tag`` / +``resolved_commit`` (and an empty ``resolved_ref``). + +The fix lives in ``_rebuild_cached_semver_resolution``: it returns +``None`` unless ALL required fields (constraint, version, +resolved_tag, resolved_commit) are present. +""" + +from __future__ import annotations + +import pytest + +from apm_cli.deps.git_semver_resolver import GitSemverResolution +from apm_cli.deps.lockfile import LockedDependency +from apm_cli.install.sources import _rebuild_cached_semver_resolution + + +def _make_locked( + *, + constraint: str | None = "^1.2.0", + version: str | None = "1.5.3", + resolved_tag: str | None = "v1.5.3", + resolved_commit: str | None = "c" * 40, + resolved_at: str | None = "2024-06-15T12:00:00+00:00", +) -> LockedDependency: + return LockedDependency( + repo_url="acme/some-skills", + host="github.com", + port=None, + registry_prefix=None, + resolved_commit=resolved_commit, + resolved_ref=resolved_tag, + version=version, + local_path="apm_modules/some-skills", + depth=1, + resolved_by=None, + is_dev=False, + constraint=constraint, + resolved_tag=resolved_tag, + resolved_at=resolved_at, + ) + + +class TestRebuildCachedSemverResolution: + def test_returns_resolution_when_all_required_fields_present(self) -> None: + locked = _make_locked() + result = _rebuild_cached_semver_resolution(locked) + assert isinstance(result, GitSemverResolution) + assert result.constraint == "^1.2.0" + assert result.resolved_version == "1.5.3" + assert result.resolved_tag == "v1.5.3" + assert result.resolved_sha == "c" * 40 + + def test_returns_none_when_dep_is_none(self) -> None: + assert _rebuild_cached_semver_resolution(None) is None + + def test_returns_none_when_constraint_missing(self) -> None: + assert _rebuild_cached_semver_resolution(_make_locked(constraint=None)) is None + + @pytest.mark.parametrize( + "missing_field", + ["version", "resolved_tag", "resolved_commit"], + ) + def test_returns_none_when_any_required_field_missing(self, missing_field: str) -> None: + """Mutation-trap: if any required field is missing, rebuild aborts. + + If the gate is loosened (e.g. back to ``and dep_locked_chk.constraint`` + only), each of these cases would return a ``GitSemverResolution`` + with empty strings -- which is exactly the bug the PR #1496 review + thread called out. + """ + kwargs: dict[str, str | None] = {missing_field: None} + locked = _make_locked(**kwargs) + assert _rebuild_cached_semver_resolution(locked) is None + + def test_returns_none_when_required_field_is_empty_string(self) -> None: + """Empty strings are as harmful as ``None`` -- truthiness check guards both.""" + locked = _make_locked(resolved_tag="") + assert _rebuild_cached_semver_resolution(locked) is None + + def test_missing_resolved_at_is_tolerated(self) -> None: + """``resolved_at`` is not required: it's an audit field, not a trust anchor.""" + locked = _make_locked(resolved_at=None) + result = _rebuild_cached_semver_resolution(locked) + assert isinstance(result, GitSemverResolution) + assert result.resolved_at == "" diff --git a/tests/unit/install/test_git_semver_wiring.py b/tests/unit/install/test_git_semver_wiring.py new file mode 100644 index 0000000000..a779c6ebf8 --- /dev/null +++ b/tests/unit/install/test_git_semver_wiring.py @@ -0,0 +1,329 @@ +"""Tests for git-semver wiring in the install resolve phase and drift detection. + +Covers issue #1488: + +- ``_maybe_resolve_git_semver`` correctly routes git-source semver-range deps + through ``GitSemverResolver`` and falls back to lockfile replay when the + constraint is unchanged. +- ``drift.detect_ref_change`` does not report drift when the manifest + carries a semver range and the lockfile holds the resolved tag, as long + as the constraint matches the locked constraint. +""" + +from __future__ import annotations + +from unittest.mock import MagicMock, patch + +import pytest # noqa: F401 + +from apm_cli.deps.git_semver_resolver import GitSemverResolution +from apm_cli.deps.lockfile import LockedDependency, LockFile +from apm_cli.drift import detect_ref_change +from apm_cli.install.phases.resolve import _maybe_resolve_git_semver +from apm_cli.models.dependency.reference import DependencyReference + + +def _make_dep_ref(*, reference="^1.2.0", source="github", is_local=False, artifactory_prefix=None): + """Build a minimal git-source DependencyReference for tests.""" + return DependencyReference( + host="github.com", + repo_url="acme/widget", + reference=reference, + source=source, + is_local=is_local, + artifactory_prefix=artifactory_prefix, + ) + + +class TestMaybeResolveGitSemver: + def test_returns_none_for_local_dep(self): + dep = _make_dep_ref(is_local=True) + assert ( + _maybe_resolve_git_semver(dep_ref=dep, existing_lockfile=None, update_refs=False) + is None + ) + + def test_returns_none_for_registry_dep(self): + dep = _make_dep_ref(source="registry") + assert ( + _maybe_resolve_git_semver(dep_ref=dep, existing_lockfile=None, update_refs=False) + is None + ) + + def test_returns_none_for_proxy_dep(self): + dep = _make_dep_ref(artifactory_prefix="my-prefix") + assert ( + _maybe_resolve_git_semver(dep_ref=dep, existing_lockfile=None, update_refs=False) + is None + ) + + def test_returns_none_for_literal_ref(self): + dep = _make_dep_ref(reference="v1.2.3") + assert ( + _maybe_resolve_git_semver(dep_ref=dep, existing_lockfile=None, update_refs=False) + is None + ) + + def test_returns_none_for_none_ref(self): + dep = _make_dep_ref(reference=None) + assert ( + _maybe_resolve_git_semver(dep_ref=dep, existing_lockfile=None, update_refs=False) + is None + ) + + def test_lockfile_replay_on_unchanged_constraint(self): + """When lockfile already records the same constraint, replay without network.""" + dep = _make_dep_ref(reference="^1.2.0") + lockfile = LockFile() + lockfile.dependencies[dep.get_unique_key()] = LockedDependency( + host="github.com", + repo_url="acme/widget", + source="github", + resolved_ref="v1.5.3", + resolved_commit="a" * 40, + version="1.5.3", + constraint="^1.2.0", + resolved_tag="v1.5.3", + resolved_at="2025-01-15T12:00:00Z", + ) + + # Patch RefResolver / GitSemverResolver so any accidental network + # call would blow up the test loudly. + with patch("apm_cli.marketplace.ref_resolver.RefResolver") as rr_mock: + resolution = _maybe_resolve_git_semver( + dep_ref=dep, existing_lockfile=lockfile, update_refs=False + ) + assert rr_mock.called is False + + assert isinstance(resolution, GitSemverResolution) + assert resolution.constraint == "^1.2.0" + assert resolution.resolved_tag == "v1.5.3" + assert resolution.resolved_version == "1.5.3" + assert resolution.resolved_sha == "a" * 40 + + def test_fresh_resolution_when_update_refs(self): + """With --update, ignore lockfile and call out to RefResolver.""" + dep = _make_dep_ref(reference="^1.2.0") + lockfile = LockFile() + lockfile.dependencies[dep.get_unique_key()] = LockedDependency( + host="github.com", + repo_url="acme/widget", + source="github", + resolved_ref="v1.5.3", + resolved_commit="a" * 40, + version="1.5.3", + constraint="^1.2.0", + resolved_tag="v1.5.3", + resolved_at="2025-01-15T12:00:00Z", + ) + + fresh = GitSemverResolution( + constraint="^1.2.0", + resolved_version="1.6.0", + resolved_tag="v1.6.0", + resolved_sha="b" * 40, + matched_pattern="v{version}", + resolved_at="2025-02-01T00:00:00Z", + ) + with patch("apm_cli.deps.git_semver_resolver.GitSemverResolver") as resolver_cls: + instance = MagicMock() + instance.resolve.return_value = fresh + resolver_cls.return_value = instance + + resolution = _maybe_resolve_git_semver( + dep_ref=dep, existing_lockfile=lockfile, update_refs=True + ) + + assert resolution is fresh + + def test_lockfile_replay_skipped_when_constraint_changed(self): + """If the manifest constraint differs from the locked constraint, + replay is skipped and a fresh resolution kicks in.""" + dep = _make_dep_ref(reference="^2.0.0") # manifest bumped + lockfile = LockFile() + lockfile.dependencies[dep.get_unique_key()] = LockedDependency( + host="github.com", + repo_url="acme/widget", + source="github", + resolved_ref="v1.5.3", + resolved_commit="a" * 40, + version="1.5.3", + constraint="^1.2.0", # stale + resolved_tag="v1.5.3", + resolved_at="2025-01-15T12:00:00Z", + ) + + fresh = GitSemverResolution( + constraint="^2.0.0", + resolved_version="2.1.0", + resolved_tag="v2.1.0", + resolved_sha="c" * 40, + matched_pattern="v{version}", + resolved_at="2025-02-01T00:00:00Z", + ) + with patch("apm_cli.deps.git_semver_resolver.GitSemverResolver") as resolver_cls: + instance = MagicMock() + instance.resolve.return_value = fresh + resolver_cls.return_value = instance + + resolution = _maybe_resolve_git_semver( + dep_ref=dep, existing_lockfile=lockfile, update_refs=False + ) + + assert resolution is fresh + + +class TestDriftDetectRefChangeForSemver: + """``detect_ref_change`` must not report drift when the manifest carries + a semver range and the lockfile's recorded constraint is identical -- + even though ``dep_ref.reference`` (``^1.2.0``) differs from the locked + ``resolved_ref`` (``v1.5.3``).""" + + def _locked(self, *, constraint, resolved_ref="v1.5.3"): + return LockedDependency( + host="github.com", + repo_url="acme/widget", + source="github", + resolved_ref=resolved_ref, + resolved_commit="a" * 40, + version="1.5.3", + constraint=constraint, + resolved_tag=resolved_ref, + resolved_at="2025-01-15T12:00:00Z", + ) + + def test_no_drift_when_constraint_unchanged(self): + dep = _make_dep_ref(reference="^1.2.0") + locked = self._locked(constraint="^1.2.0") + assert detect_ref_change(dep, locked, update_refs=False) is False + + def test_drift_when_constraint_changed(self): + dep = _make_dep_ref(reference="^2.0.0") + locked = self._locked(constraint="^1.2.0") + assert detect_ref_change(dep, locked, update_refs=False) is True + + def test_no_false_drift_against_literal_locked_tag(self): + """The classic bug: ``^1.2.0`` != ``v1.5.3`` substring-comparison + used to trip a false drift. With the new branch, equal constraint + wins regardless of resolved_ref.""" + dep = _make_dep_ref(reference="^1.2.0") + locked = self._locked(constraint="^1.2.0", resolved_ref="v1.5.3") + assert detect_ref_change(dep, locked, update_refs=False) is False + + +class TestMaybeResolveGitSemverAuthThreading: + """Regression-trap for the auth-blocking panel finding on PR #1496. + + The git-semver resolution path runs ``git ls-remote`` against the + dep's remote BEFORE the clone step. Without these tests, the + ls-remote call would silently bypass ``AuthResolver`` and rely on + the host's git credential helper -- which is absent in CI + environments (GitHub Actions, ADO pipelines, containers) where + ``GITHUB_APM_PAT`` / ``ADO_APM_PAT`` are the only token source. + Private-repo semver-range deps would then fail with a cryptic + ``repository not found`` instead of using the configured token. + """ + + def test_token_threaded_into_ref_resolver_when_auth_resolver_supplied(self): + """When an auth_resolver is passed, its per-dep token must reach RefResolver.""" + dep = _make_dep_ref(reference="^1.2.0") + + # Mock auth_resolver that returns a known token for this dep. + auth_ctx = MagicMock() + auth_ctx.token = "ghp_testtoken_abc123" + auth_resolver = MagicMock() + auth_resolver.resolve_for_dep.return_value = auth_ctx + + # Capture the kwargs RefResolver receives. + with ( + patch("apm_cli.marketplace.ref_resolver.RefResolver") as rr_cls, + patch("apm_cli.deps.git_semver_resolver.GitSemverResolver"), + ): + _maybe_resolve_git_semver( + dep_ref=dep, + existing_lockfile=None, + update_refs=False, + auth_resolver=auth_resolver, + ) + + auth_resolver.resolve_for_dep.assert_called_once_with(dep) + rr_cls.assert_called_once() + _, kwargs = rr_cls.call_args + assert kwargs.get("token") == "ghp_testtoken_abc123", ( + "RefResolver must receive the token resolved from AuthResolver " + "so ls-remote against private repos uses the configured PAT " + "instead of relying on the system git credential helper." + ) + assert kwargs.get("host") == "github.com" + + def test_no_auth_resolver_passes_none_token(self): + """Backward-compat: callers that don't supply auth_resolver still work.""" + dep = _make_dep_ref(reference="^1.2.0") + + with ( + patch("apm_cli.marketplace.ref_resolver.RefResolver") as rr_cls, + patch("apm_cli.deps.git_semver_resolver.GitSemverResolver"), + ): + _maybe_resolve_git_semver( + dep_ref=dep, + existing_lockfile=None, + update_refs=False, + ) + + rr_cls.assert_called_once() + _, kwargs = rr_cls.call_args + assert kwargs.get("token") is None + + def test_auth_resolver_exception_falls_back_to_unauth(self): + """If AuthResolver raises, ls-remote falls back to unauth path. + + The downstream clone will surface the real auth error with its + own actionable diagnostic, so swallowing the exception here is + safe and avoids double-reporting. + """ + dep = _make_dep_ref(reference="^1.2.0") + + auth_resolver = MagicMock() + auth_resolver.resolve_for_dep.side_effect = RuntimeError("auth lookup failed") + + with ( + patch("apm_cli.marketplace.ref_resolver.RefResolver") as rr_cls, + patch("apm_cli.deps.git_semver_resolver.GitSemverResolver"), + ): + _maybe_resolve_git_semver( + dep_ref=dep, + existing_lockfile=None, + update_refs=False, + auth_resolver=auth_resolver, + ) + + rr_cls.assert_called_once() + _, kwargs = rr_cls.call_args + assert kwargs.get("token") is None + + def test_lockfile_replay_path_skips_auth_resolution(self): + """Replay path must not call auth_resolver -- no network, no token needed.""" + dep = _make_dep_ref(reference="^1.2.0") + lockfile = LockFile() + lockfile.dependencies[dep.get_unique_key()] = LockedDependency( + host="github.com", + repo_url="acme/widget", + source="github", + resolved_ref="v1.5.3", + resolved_commit="a" * 40, + version="1.5.3", + constraint="^1.2.0", + resolved_tag="v1.5.3", + resolved_at="2025-01-15T12:00:00Z", + ) + + auth_resolver = MagicMock() + resolution = _maybe_resolve_git_semver( + dep_ref=dep, + existing_lockfile=lockfile, + update_refs=False, + auth_resolver=auth_resolver, + ) + + assert isinstance(resolution, GitSemverResolution) + auth_resolver.resolve_for_dep.assert_not_called() diff --git a/tests/unit/install/test_install_pkg_policy_rollback.py b/tests/unit/install/test_install_pkg_policy_rollback.py index 2a13c158fa..e66efdab0d 100644 --- a/tests/unit/install/test_install_pkg_policy_rollback.py +++ b/tests/unit/install/test_install_pkg_policy_rollback.py @@ -78,7 +78,7 @@ class PolicyViolationError(RuntimeError): def _successful_install_result() -> InstallResult: - diag = MagicMock(has_diagnostics=False, has_critical_security=False) + diag = MagicMock(has_diagnostics=False, has_critical_security=False, error_count=0) return InstallResult(diagnostics=diag) diff --git a/tests/unit/install/test_no_policy_flag.py b/tests/unit/install/test_no_policy_flag.py index c70177ea58..a38efebe88 100644 --- a/tests/unit/install/test_no_policy_flag.py +++ b/tests/unit/install/test_no_policy_flag.py @@ -48,7 +48,7 @@ def _successful_install_result() -> InstallResult: - diag = MagicMock(has_diagnostics=False, has_critical_security=False) + diag = MagicMock(has_diagnostics=False, has_critical_security=False, error_count=0) return InstallResult(diagnostics=diag) diff --git a/tests/unit/install/test_summary.py b/tests/unit/install/test_summary.py index d9897bb0b7..5fabf1455f 100644 --- a/tests/unit/install/test_summary.py +++ b/tests/unit/install/test_summary.py @@ -101,7 +101,13 @@ def test_rich_blank_line_when_apm_diagnostics_is_none(self) -> None: def test_error_count_forwarded_to_install_summary(self) -> None: logger = _make_logger() diag = _make_diag(has_diagnostics=True, error_count=2) - with patch("apm_cli.install.summary._rich_blank_line"): + # error_count > 0 hard-fails with exit 1 (npm/pip/cargo convention, + # Bug 2 fix on #1496). The install_summary call still fires before + # the SystemExit, so the forwarded counter assertion still holds. + with ( + patch("apm_cli.install.summary._rich_blank_line"), + pytest.raises(SystemExit) as exc_info, + ): render_post_install_summary( logger=logger, apm_count=1, @@ -109,9 +115,52 @@ def test_error_count_forwarded_to_install_summary(self) -> None: apm_diagnostics=diag, force=False, ) + assert exc_info.value.code == 1 call_kwargs = logger.install_summary.call_args[1] assert call_kwargs["errors"] == 2 + def test_hard_fail_on_reported_errors_without_critical_security(self) -> None: + """Bug 2 (#1496): ``Installation failed with N error(s)`` must exit 1. + + Mirrors npm/pip/cargo: any per-dep install failure -> non-zero exit + so CI scripts can detect failure without parsing stderr. The + ``--force`` flag covers critical-security overrides only; it does + NOT suppress the hard-fail on reported errors. + """ + logger = _make_logger() + diag = _make_diag(has_diagnostics=True, error_count=1, has_critical_security=False) + with ( + patch("apm_cli.install.summary._rich_blank_line"), + pytest.raises(SystemExit) as exc_info, + ): + render_post_install_summary( + logger=logger, + apm_count=0, + mcp_count=0, + apm_diagnostics=diag, + force=False, + ) + assert exc_info.value.code == 1 + + def test_force_does_not_suppress_reported_errors(self) -> None: + """``--force`` overrides only the critical-security hard-fail; a + non-zero ``error_count`` must still exit 1 so scripted installers + cannot mask a "failed to download dep" by passing ``--force``.""" + logger = _make_logger() + diag = _make_diag(has_diagnostics=True, error_count=1, has_critical_security=False) + with ( + patch("apm_cli.install.summary._rich_blank_line"), + pytest.raises(SystemExit) as exc_info, + ): + render_post_install_summary( + logger=logger, + apm_count=0, + mcp_count=0, + apm_diagnostics=diag, + force=True, + ) + assert exc_info.value.code == 1 + def test_elapsed_seconds_forwarded(self) -> None: logger = _make_logger() with patch("apm_cli.install.summary._rich_blank_line"): diff --git a/tests/unit/models/__init__.py b/tests/unit/models/__init__.py new file mode 100644 index 0000000000..e69de29bb2 diff --git a/tests/unit/models/test_dependency_reference_semver_routing.py b/tests/unit/models/test_dependency_reference_semver_routing.py new file mode 100644 index 0000000000..a71f468031 --- /dev/null +++ b/tests/unit/models/test_dependency_reference_semver_routing.py @@ -0,0 +1,91 @@ +"""Semver-routing detection on ``DependencyReference``. + +The ``ref_kind`` property classifies ``reference`` values so the install +pipeline can route to the git-semver resolver only when the author wrote +a semver range. Literal tags, branch names, and SHAs MUST keep their +existing literal-routing behaviour. +""" + +from __future__ import annotations + +from apm_cli.models.dependency.reference import DependencyReference + + +class TestStringShorthandRouting: + """Routing from the ``owner/repo#`` shorthand.""" + + def test_string_shorthand_with_caret_range_routes_to_semver(self) -> None: + dep = DependencyReference.parse("acme/some-skills#^1.2.0") + assert dep.reference == "^1.2.0" + assert dep.ref_kind == "semver" + + def test_string_shorthand_with_tilde_range_routes_to_semver(self) -> None: + dep = DependencyReference.parse("acme/some-skills#~2.1.0") + assert dep.ref_kind == "semver" + + def test_string_shorthand_with_wildcard_routes_to_semver(self) -> None: + dep = DependencyReference.parse("acme/some-skills#1.2.x") + assert dep.ref_kind == "semver" + + def test_string_shorthand_with_branch_name_does_not_route_to_semver(self) -> None: + dep = DependencyReference.parse("acme/some-skills#main") + assert dep.reference == "main" + assert dep.ref_kind == "literal" + + def test_string_shorthand_with_sha_does_not_route_to_semver(self) -> None: + dep = DependencyReference.parse("acme/some-skills#abc1234") + assert dep.ref_kind == "literal" + + def test_no_reference_returns_none(self) -> None: + dep = DependencyReference.parse("acme/some-skills") + assert dep.reference is None + assert dep.ref_kind is None + + +class TestObjectFormRouting: + """Routing from the ``{git: ..., ref: ...}`` object form.""" + + def test_object_form_caret_range_routes_to_semver(self) -> None: + dep = DependencyReference.parse_from_dict( + { + "git": "https://github.com/acme/some-skills.git", + "ref": "^1.2.0", + } + ) + assert dep.reference == "^1.2.0" + assert dep.ref_kind == "semver" + + def test_object_form_branch_name_does_not_route_to_semver(self) -> None: + dep = DependencyReference.parse_from_dict( + { + "git": "https://github.com/acme/some-skills.git", + "ref": "main", + } + ) + assert dep.ref_kind == "literal" + + +class TestLiteralTagRegressionTrap: + """Regression trap: literal ``v1.2.3`` tag MUST NOT route to semver. + + ``v1.2.3`` is a valid git tag literal; before #1488, authors who + wrote ``ref: v1.2.3`` got an exact-tag clone. Routing this through + the semver resolver would be a behaviour break. + """ + + def test_literal_tag_v1_2_3_does_not_route_to_semver(self) -> None: + dep = DependencyReference.parse("acme/some-skills#v1.2.3") + assert dep.reference == "v1.2.3" + # ``v1.2.3`` does NOT parse as a semver range (leading 'v' is not + # an operator); the literal path takes it. The git-semver resolver + # is never invoked, preserving pre-#1488 behaviour. + assert dep.ref_kind == "literal" + + def test_bare_version_1_2_3_routes_to_semver(self) -> None: + # The mirror case: ``1.2.3`` with no prefix parses as an exact- + # version semver constraint, so it DOES route through the + # resolver. The resolver's bare-version fallback pattern covers + # the case where the remote tag is also literally ``1.2.3``. + dep = DependencyReference.parse("acme/some-skills#1.2.3") + assert dep.reference == "1.2.3" + assert dep.ref_kind == "semver" diff --git a/tests/unit/test_dev_dependencies.py b/tests/unit/test_dev_dependencies.py index b969227445..386209cf86 100644 --- a/tests/unit/test_dev_dependencies.py +++ b/tests/unit/test_dev_dependencies.py @@ -318,7 +318,9 @@ def test_dev_flag_writes_to_dev_dependencies( mock_apm_package.from_apm_yml.return_value = mock_pkg mock_install_apm.return_value = InstallResult( - diagnostics=MagicMock(has_diagnostics=False, has_critical_security=False) + diagnostics=MagicMock( + has_diagnostics=False, has_critical_security=False, error_count=0 + ) ) result = self.runner.invoke(cli, ["install", "--dev", "test/dev-pkg"]) @@ -362,7 +364,9 @@ def test_no_dev_flag_writes_to_dependencies( mock_apm_package.from_apm_yml.return_value = mock_pkg mock_install_apm.return_value = InstallResult( - diagnostics=MagicMock(has_diagnostics=False, has_critical_security=False) + diagnostics=MagicMock( + has_diagnostics=False, has_critical_security=False, error_count=0 + ) ) result = self.runner.invoke(cli, ["install", "test/prod-pkg"]) diff --git a/tests/unit/test_install_command.py b/tests/unit/test_install_command.py index fbc03db257..6582b06454 100644 --- a/tests/unit/test_install_command.py +++ b/tests/unit/test_install_command.py @@ -82,7 +82,9 @@ def test_install_no_apm_yml_with_packages_creates_minimal_apm_yml( # Mock the install function to avoid actual installation mock_install_apm.return_value = InstallResult( - diagnostics=MagicMock(has_diagnostics=False, has_critical_security=False) + diagnostics=MagicMock( + has_diagnostics=False, has_critical_security=False, error_count=0 + ) ) result = self.runner.invoke(cli, ["install", "test/package"]) @@ -120,7 +122,9 @@ def test_install_no_apm_yml_with_multiple_packages( mock_apm_package.from_apm_yml.return_value = mock_pkg_instance mock_install_apm.return_value = InstallResult( - diagnostics=MagicMock(has_diagnostics=False, has_critical_security=False) + diagnostics=MagicMock( + has_diagnostics=False, has_critical_security=False, error_count=0 + ) ) result = self.runner.invoke(cli, ["install", "org1/pkg1", "org2/pkg2"]) @@ -160,7 +164,9 @@ def test_install_existing_apm_yml_preserves_behavior(self, mock_install_apm, moc mock_apm_package.from_apm_yml.return_value = mock_pkg_instance mock_install_apm.return_value = InstallResult( - diagnostics=MagicMock(has_diagnostics=False, has_critical_security=False) + diagnostics=MagicMock( + has_diagnostics=False, has_critical_security=False, error_count=0 + ) ) result = self.runner.invoke(cli, ["install"]) @@ -200,7 +206,9 @@ def test_install_auto_created_apm_yml_has_correct_metadata( mock_apm_package.from_apm_yml.return_value = mock_pkg_instance mock_install_apm.return_value = InstallResult( - diagnostics=MagicMock(has_diagnostics=False, has_critical_security=False) + diagnostics=MagicMock( + has_diagnostics=False, has_critical_security=False, error_count=0 + ) ) result = self.runner.invoke(cli, ["install", "test/package"]) @@ -863,7 +871,9 @@ def test_global_creates_user_apm_yml(self, mock_install_apm, mock_apm_package, m mock_pkg.target = None mock_apm_package.from_apm_yml.return_value = mock_pkg mock_install_apm.return_value = InstallResult( - diagnostics=MagicMock(has_diagnostics=False, has_critical_security=False) + diagnostics=MagicMock( + has_diagnostics=False, has_critical_security=False, error_count=0 + ) ) with patch.object(Path, "home", return_value=fake_home): @@ -1289,7 +1299,9 @@ def test_http_dep_addition_passes_with_allow_insecure_flag( mock_pkg_instance.get_mcp_dependencies.return_value = [] mock_apm_package.from_apm_yml.return_value = mock_pkg_instance mock_install_apm.return_value = InstallResult( - diagnostics=MagicMock(has_diagnostics=False, has_critical_security=False) + diagnostics=MagicMock( + has_diagnostics=False, has_critical_security=False, error_count=0 + ) ) result = self.runner.invoke( @@ -1328,7 +1340,9 @@ def test_allow_insecure_host_is_passed_to_install_engine( mock_pkg_instance.get_dev_apm_dependencies.return_value = [] mock_apm_package.from_apm_yml.return_value = mock_pkg_instance mock_install_apm.return_value = InstallResult( - diagnostics=MagicMock(has_diagnostics=False, has_critical_security=False) + diagnostics=MagicMock( + has_diagnostics=False, has_critical_security=False, error_count=0 + ) ) result = self.runner.invoke(