diff --git a/CHANGELOG.md b/CHANGELOG.md index 1a517122c..28966b82f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,14 +9,32 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Added +- Deterministic Artifactory boundary probe: at install time, `_resolve_artifactory_boundary` HEAD-probes the candidate archive URLs and rebuilds the dependency reference at the proxy-verified split. Covers both explicit-FQDN (`host/artifactory/key/owner/repo/...`) and bare-shorthand-under-proxy deps. Distinguishes "missing repo" from auth (401/403) errors; uses `allow_redirects=False` so the bearer token can't leak cross-host. Mirrors the native GitLab probing pattern but without a separate metadata API. (#1472) +- `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) - Org-wide policy discovery now cascades through candidate repo names (`.github`, then `.apm`, then `_apm`) and speaks the Azure DevOps Items API, so Azure DevOps organizations -- which forbid repo names that begin or end with `.` -- can host an APM governance policy repo for the first time. (by @sergio-sisternes-epam; closes #1813) (#1830) +### 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. +- Parse-time Artifactory boundary detection no longer uses directory-marker heuristics (`skills/`, `prompts/`, `agents/`, `collections/`, `instructions/`). The install-time resolver is authoritative for the (owner, repo, virtual_path) split. Parse-time defaults differ by mode: + - **Explicit FQDN** (`host/artifactory/key/owner/repo/...`): `parse_artifactory_path` stays intentionally shallow -- `owner` = first segment after the prefix, `repo` = next segment, remainder = `virtual_path`. + - **Bare shorthand under `PROXY_REGISTRY_ONLY`**: `_bare_shorthand_repo_segment_count` defaults to all-as-repo with a structural file-extension rule on the last segment (a path ending in `.prompt.md`/`.instructions.md`/`.chatmode.md`/`.agent.md` is by shape a virtual file; everything before it is the repo). + + Both modes converge at install time: `_resolve_artifactory_boundary` HEAD-probes candidate splits and rebuilds the dependency reference at the proxy-verified split. The `_VIRTUAL_PATH_ROOT_SEGMENTS`, `_ARTIFACTORY_VIRTUAL_MARKERS`, and `_ARTIFACTORY_VIRTUAL_FILE_EXTENSIONS` constants are removed. (#1472) + ### Fixed +- `apm install` through a registry proxy now supports nested-group repos (3+ path segments, e.g. `group/subgroup/project`). Previously every trailing segment past `owner/repo` was treated as an in-repo virtual sub-path, so the downloader requested the wrong archive URL and the install failed with HTTP 404 from the proxy. Behavior is gated on `PROXY_REGISTRY_ONLY` so direct (non-proxy) installs keep the legacy two-segment shape. Affects `parse_artifactory_path`, `build_artifactory_archive_url`, `_detect_virtual_package`, the shorthand resolvers in `DependencyReference`, and `ArtifactoryOrchestrator._split_owner_repo` / `download_subdirectory`. (#1472) +- URL-form Artifactory deps no longer round-trip with the `artifactory/` prefix folded into `repo_url`. The duplicated prefix caused the downloader to construct double-prefixed archive URLs (`/artifactory/key/artifactory/key/owner/repo/...`) and 404. `_validate_url_repo_path` now strips the Artifactory VCS prefix before returning the bare `owner/repo` slug; the prefix is still recovered separately via `_extract_artifactory_prefix`. (#1472) +- `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. +- `apm install plugin@marketplace` now correctly fetches from the marketplace's registered `--ref` branch instead of silently falling back to the repository's default branch. Root cause: `resolve_marketplace_plugin` did not propagate `source.ref` to downstream resolution calls. Covers both GitHub-family hosts (ref appended to canonical as `#ref`) and GitLab-hosted marketplaces (ref injected into `DependencyReference`). Guards prevent double-injection when a plugin's own dict source already carries an explicit `ref`, and skip `main`/`HEAD` as implicit defaults. (mirrors [microsoft/apm#1824](https://github.com/microsoft/apm/pull/1824)) +- GitLab archive URLs with slash-containing branch names (e.g. `feat/my-feature`) now produce correctly-formed Artifactory-proxyable filenames. The slash is preserved in the path segment (as the GitLab archive API expects) but replaced with `-` in the archive filename, matching GitLab's own naming convention. Previously, `PROXY_REGISTRY_ONLY=1` installs from such branches returned HTTP 404 from the proxy because the generated filename contained a literal slash. This is the proxy scenario not covered by [microsoft/apm#1824](https://github.com/microsoft/apm/pull/1824). - `apm install @` now preserves GitLab and other non-GitHub hosts from url-type marketplace plugin sources, so auth resolution no longer falls back to `github.com` for those installs. diff --git a/src/apm_cli/install/artifactory_resolver.py b/src/apm_cli/install/artifactory_resolver.py index c87ee223e..30cfbc9c7 100644 --- a/src/apm_cli/install/artifactory_resolver.py +++ b/src/apm_cli/install/artifactory_resolver.py @@ -93,6 +93,7 @@ def _candidate_archive_status( """ urls = build_artifactory_archive_url(host, prefix, owner, repo, ref, scheme=scheme) saw_auth = False + saw_non_auth_4xx = False saw_any_response = False last_exc: BaseException | None = None for url in urls: @@ -111,9 +112,14 @@ def _candidate_archive_status( return _CandidateStatus.EXISTS, None if r.status_code in (401, 403): saw_auth = True + else: + saw_non_auth_4xx = True if not saw_any_response: return _CandidateStatus.INCONCLUSIVE, last_exc - if saw_auth: + # AUTH only when *every* response was 401/403 -- no non-auth 4xx seen. + # A mixed 401+404 means at least one URL shape was definitively absent; + # report MISSING so the user-facing error stays accurate (per module spec). + if saw_auth and not saw_non_auth_4xx: return _CandidateStatus.AUTH, None return _CandidateStatus.MISSING, None diff --git a/src/apm_cli/utils/github_host.py b/src/apm_cli/utils/github_host.py index 0845d02ba..ee807ca2f 100644 --- a/src/apm_cli/utils/github_host.py +++ b/src/apm_cli/utils/github_host.py @@ -775,7 +775,10 @@ def build_artifactory_archive_url( # GitHub-style: /archive/refs/heads/{ref}.zip f"{base}/archive/refs/heads/{ref}.zip", # GitLab-style: /-/archive/{ref}/{basename}-{ref}.zip - f"{base}/-/archive/{ref}/{repo_basename}-{ref}.zip", + # GitLab keeps the raw ref (including slashes) as a path segment but replaces + # slashes with dashes in the archive *filename*. E.g. branch + # ``feat/my-feature`` -> ``.../-/archive/feat/my-feature/repo-feat-my-feature.zip``. + f"{base}/-/archive/{ref}/{repo_basename}-{ref.replace('/', '-')}.zip", # GitHub-style tags fallback f"{base}/archive/refs/tags/{ref}.zip", # codeload.github.com-style: /zip/refs/heads/{ref} diff --git a/tests/unit/marketplace/test_marketplace_resolver.py b/tests/unit/marketplace/test_marketplace_resolver.py index 5736af1fd..d222bb967 100644 --- a/tests/unit/marketplace/test_marketplace_resolver.py +++ b/tests/unit/marketplace/test_marketplace_resolver.py @@ -1507,102 +1507,145 @@ def test_fqdn_shorthand_without_git_path_misclassifies(self, monkeypatch): assert good.virtual_path == "agents/reverse-architect" -class TestMarketplaceRefPropagation: - """Registered --ref must propagate to relative string plugin sources (#1811).""" +class TestMarketplaceRegisteredRefPropagation: + """Registered marketplace ``--ref`` must propagate to plugin canonicals. - @staticmethod - def _manifest_with_plugin(plugin: MarketplacePlugin) -> MarketplaceManifest: - return MarketplaceManifest(name="mkt", plugins=(plugin,), plugin_root="") + When a marketplace is added with ``apm marketplace add --ref feat/x owner/repo``, + subsequent ``apm install plugin@marketplace`` must download the plugin from + ``feat/x`` -- not from the repo's default branch. This applies to both: - @patch("apm_cli.marketplace.resolver.fetch_or_cache") - @patch("apm_cli.marketplace.resolver.get_marketplace_by_name") - def test_github_relative_source_propagates_registered_ref(self, mock_get, mock_fetch): - """A GitHub marketplace registered with --ref feat/xxx appends #feat/xxx.""" - source = MarketplaceSource( - name="mkt", + - GitHub-family marketplaces (no ``dependency_reference``; ref embedded in canonical) + - GitLab / non-GitHub marketplaces (ref embedded in ``dependency_reference``) + """ + + @staticmethod + def _github_source(ref: str = "main") -> MarketplaceSource: + return MarketplaceSource( + name="my-marketplace", owner="acme", - repo="catalog", + repo="plugins", host="github.com", - ref="feat/my-feature", + ref=ref, ) + + @staticmethod + def _gitlab_source(ref: str = "main") -> MarketplaceSource: + return MarketplaceSource( + name="my-marketplace", + owner="acme", + repo="plugins", + host="gitlab.com", + ref=ref, + ) + + @staticmethod + def _manifest(plugin: MarketplacePlugin) -> MarketplaceManifest: + return MarketplaceManifest(name="my-marketplace", plugins=(plugin,), plugin_root="") + + # -- GitHub-family: ref embedded in canonical --------------------------------- + + @patch("apm_cli.marketplace.resolver.fetch_or_cache") + @patch("apm_cli.marketplace.resolver.get_marketplace_by_name") + def test_github_string_source_registered_ref_appended(self, mock_get, mock_fetch): + """Non-main marketplace ref must appear as ``#ref`` suffix in the canonical.""" plugin = MarketplacePlugin(name="my-plugin", source="./plugins/my-plugin") - mock_get.return_value = source - mock_fetch.return_value = self._manifest_with_plugin(plugin) + mock_get.return_value = self._github_source(ref="feat/my-feature") + mock_fetch.return_value = self._manifest(plugin) + + result = resolve_marketplace_plugin("my-plugin", "my-marketplace") - result = resolve_marketplace_plugin("my-plugin", "mkt") - assert result.canonical == "acme/catalog/plugins/my-plugin#feat/my-feature" + assert result.canonical == "acme/plugins/plugins/my-plugin#feat/my-feature" assert result.dependency_reference is None @patch("apm_cli.marketplace.resolver.fetch_or_cache") @patch("apm_cli.marketplace.resolver.get_marketplace_by_name") - def test_github_relative_source_main_ref_not_appended(self, mock_get, mock_fetch): - """ref='main' is the default -- do not append #main to the canonical.""" - source = MarketplaceSource( - name="mkt", - owner="acme", - repo="catalog", - host="github.com", - ref="main", - ) + def test_github_string_source_main_ref_not_appended(self, mock_get, mock_fetch): + """Default ``main`` ref must NOT be embedded (implicit; avoids cluttering canonicals).""" plugin = MarketplacePlugin(name="my-plugin", source="./plugins/my-plugin") - mock_get.return_value = source - mock_fetch.return_value = self._manifest_with_plugin(plugin) + mock_get.return_value = self._github_source(ref="main") + mock_fetch.return_value = self._manifest(plugin) - result = resolve_marketplace_plugin("my-plugin", "mkt") - assert result.canonical == "acme/catalog/plugins/my-plugin" + result = resolve_marketplace_plugin("my-plugin", "my-marketplace") + + assert "#" not in result.canonical @patch("apm_cli.marketplace.resolver.fetch_or_cache") @patch("apm_cli.marketplace.resolver.get_marketplace_by_name") - def test_version_spec_overrides_registered_ref(self, mock_get, mock_fetch): - """Explicit version_spec takes precedence over registered ref.""" - source = MarketplaceSource( - name="mkt", - owner="acme", - repo="catalog", - host="github.com", - ref="feat/my-feature", - ) + def test_github_string_source_head_ref_not_appended(self, mock_get, mock_fetch): + """``HEAD`` ref, like ``main``, must not be embedded.""" plugin = MarketplacePlugin(name="my-plugin", source="./plugins/my-plugin") - mock_get.return_value = source - mock_fetch.return_value = self._manifest_with_plugin(plugin) + mock_get.return_value = self._github_source(ref="HEAD") + mock_fetch.return_value = self._manifest(plugin) - result = resolve_marketplace_plugin("my-plugin", "mkt", version_spec="v2.0.0") - assert result.canonical == "acme/catalog/plugins/my-plugin#v2.0.0" + result = resolve_marketplace_plugin("my-plugin", "my-marketplace") + + assert "#" not in result.canonical @patch("apm_cli.marketplace.resolver.fetch_or_cache") @patch("apm_cli.marketplace.resolver.get_marketplace_by_name") - def test_gitlab_relative_source_propagates_registered_ref(self, mock_get, mock_fetch): - """A GitLab marketplace registered with --ref feat/xxx sets ref in dep_ref.""" - source = MarketplaceSource( - name="mkt", - owner="acme", - repo="catalog", - host="gitlab.com", - ref="feat/my-feature", + def test_github_string_source_version_spec_overrides_registered_ref( + self, mock_get, mock_fetch + ): + """Explicit ``version_spec`` (``apm install plugin@mkt#v2``) must win over source.ref.""" + plugin = MarketplacePlugin(name="my-plugin", source="./plugins/my-plugin") + mock_get.return_value = self._github_source(ref="feat/my-feature") + mock_fetch.return_value = self._manifest(plugin) + + result = resolve_marketplace_plugin( + "my-plugin", "my-marketplace", version_spec="v2.0.0" ) - plugin = MarketplacePlugin(name="my-plugin", source="registry/my-plugin") - mock_get.return_value = source - mock_fetch.return_value = self._manifest_with_plugin(plugin) - result = resolve_marketplace_plugin("my-plugin", "mkt") - assert result.dependency_reference is not None - assert result.dependency_reference.reference == "feat/my-feature" + assert result.canonical.endswith("#v2.0.0") + assert "feat/my-feature" not in result.canonical @patch("apm_cli.marketplace.resolver.fetch_or_cache") @patch("apm_cli.marketplace.resolver.get_marketplace_by_name") - def test_gitlab_relative_source_main_ref_not_propagated(self, mock_get, mock_fetch): - """ref='main' should not be propagated to the GitLab dep_ref.""" - source = MarketplaceSource( - name="mkt", - owner="acme", - repo="catalog", - host="gitlab.com", - ref="main", + def test_github_dict_source_registered_ref_not_propagated(self, mock_get, mock_fetch): + """Dict sources with an explicit ``ref`` must not be overwritten by source.ref.""" + plugin = MarketplacePlugin( + name="dict-plugin", + source={"type": "github", "repo": "acme/plugins", "path": "p", "ref": "v1.0.0"}, + ) + mock_get.return_value = self._github_source(ref="feat/my-feature") + mock_fetch.return_value = self._manifest(plugin) + + result = resolve_marketplace_plugin("dict-plugin", "my-marketplace") + + assert result.canonical.endswith("#v1.0.0"), ( + "Explicit plugin ref must survive; marketplace ref must not overwrite it" ) + + # -- GitLab: ref embedded in dependency_reference ---------------------------- + + @patch("apm_cli.marketplace.resolver.fetch_or_cache") + @patch("apm_cli.marketplace.resolver.get_marketplace_by_name") + def test_gitlab_string_source_registered_ref_propagated(self, mock_get, mock_fetch): + """GitLab monorepo: marketplace ref must reach dep_ref when path_ref is None.""" plugin = MarketplacePlugin(name="my-plugin", source="registry/my-plugin") - mock_get.return_value = source - mock_fetch.return_value = self._manifest_with_plugin(plugin) + mock_get.return_value = self._gitlab_source(ref="feat/my-feature") + mock_fetch.return_value = self._manifest(plugin) + + result = resolve_marketplace_plugin("my-plugin", "my-marketplace") - result = resolve_marketplace_plugin("my-plugin", "mkt") assert result.dependency_reference is not None - assert result.dependency_reference.reference is None + dep = result.dependency_reference + assert dep.reference == "feat/my-feature", ( + "Registered marketplace ref must be threaded into the DependencyReference" + ) + assert dep.virtual_path == "registry/my-plugin" + + @patch("apm_cli.marketplace.resolver.fetch_or_cache") + @patch("apm_cli.marketplace.resolver.get_marketplace_by_name") + def test_gitlab_string_source_main_ref_not_propagated(self, mock_get, mock_fetch): + """Default ``main`` ref on a GitLab marketplace must not produce a dep_ref ref field.""" + plugin = MarketplacePlugin(name="my-plugin", source="registry/my-plugin") + mock_get.return_value = self._gitlab_source(ref="main") + mock_fetch.return_value = self._manifest(plugin) + + result = resolve_marketplace_plugin("my-plugin", "my-marketplace") + + dep = result.dependency_reference + assert dep is not None + assert dep.reference is None, ( + "Default 'main' ref must not be embedded in DependencyReference" + ) diff --git a/tests/unit/test_artifactory_support.py b/tests/unit/test_artifactory_support.py index c96c7c024..d660b92c3 100644 --- a/tests/unit/test_artifactory_support.py +++ b/tests/unit/test_artifactory_support.py @@ -335,6 +335,47 @@ def test_gitlab_nested_subgroup_archive_uses_repo_basename(self): "GitLab archive filename must use the repo basename, not the full slug" ) + def test_gitlab_slash_ref_filename_normalised(self): + """GitLab archive filename must replace slashes with dashes for slash-containing refs. + + A branch like ``feat/my-slash-branch`` must produce the URL + ``/-/archive/feat/my-slash-branch/repo-feat-my-slash-branch.zip`` + -- the slash stays in the *path segment* (how GitLab addresses the archive) but is + replaced with a dash in the *filename* (how GitLab names the zip). + + Without this fix the filename would contain a literal slash + (``repo-feat/my-slash-branch.zip``) which is not a valid filename, causing + Artifactory to return 404 when ``PROXY_REGISTRY_ONLY=1`` suppresses the direct-API + fallback. + """ + ref = "feat/my-slash-branch" + urls = build_artifactory_archive_url( + "art.example.com", "artifactory/gitlab", "owner", "repo", ref=ref + ) + # The GitLab path segment must retain the slash (so Artifactory can proxy it). + assert any(f"/-/archive/{ref}/" in u for u in urls), ( + "GitLab path segment must preserve the raw ref including the slash" + ) + # The archive filename must have the slash replaced with a dash. + assert any(u.endswith("/-/archive/feat/my-slash-branch/repo-feat-my-slash-branch.zip") for u in urls), ( + "GitLab archive filename must replace '/' with '-' in the ref portion" + ) + # The GitHub-style candidates must be unchanged (slashes are valid there). + assert any("/archive/refs/heads/feat/my-slash-branch.zip" in u for u in urls), ( + "GitHub-style /archive/refs/heads/{ref}.zip must be unchanged for slash refs" + ) + + def test_non_slash_ref_gitlab_filename_unchanged(self): + """Single-component refs (no slash) must produce the same GitLab filename as before.""" + for ref in ("main", "v1.2.3", "my-feature"): + urls = build_artifactory_archive_url( + "art.example.com", "artifactory/github", "owner", "repo", ref=ref + ) + safe_ref = ref.replace("/", "-") + assert any(f"/-/archive/{ref}/repo-{safe_ref}.zip" in u for u in urls), ( + f"Non-slash ref '{ref}' must produce GitLab filename repo-{safe_ref}.zip" + ) + # ── apm_package.py: DependencyReference Artifactory parsing ── @@ -772,7 +813,7 @@ def test_split_owner_repo_nested_subgroup_after_probe(self): assert repo == "shared-modules/pkg-utils" -# ── token_manager.py: Artifactory token support ── +# -- token_manager.py: Artifactory token support -- class TestArtifactoryTokenManager: