diff --git a/CHANGELOG.md b/CHANGELOG.md index 6672b8593..a023d9755 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -33,6 +33,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Retry the `apm mcp search` and `apm mcp show` integration tests on the documented "Could not reach MCP registry" transient (with backoff and a final skip) so a brief `api.mcp.github.com` outage no longer red-marks the Windows integration job. (#1274) - Also wrap `Path.expanduser()` in the root test conftest so the `windows-2025-vs2026` runner cannot raise `RuntimeError("Could not determine home directory.")` from `ntpath.expanduser` when production code (e.g. `install.package_resolution.user_scope_rejection_reason`) calls `Path("~/pkg").expanduser()`. Falls back to the hermetic tmp dir; assertions about `~/pkg` being absolute still hold. (#1276) - `apm install` from marketplaces registered on `*.ghe.com` (GHE Cloud) hosts now routes auth at the registered enterprise host instead of silently defaulting to `github.com` and failing with 401; the marketplace resolver backfills the enterprise host onto the canonical so downstream `DependencyReference.parse` recovers it, and the resulting `apm.yml` entry records the correct enterprise `git:` URL instead of `https://github.com/...`. (#1292) +- `apm install` from `*.ghe.com` (GHE Cloud) marketplaces now surfaces a warning-level hint when a cross-repo dict `type: github` plugin source with a bare `repo` field fails validation, naming the marketplace's enterprise host and the exact host-qualified `repo` value to set in `marketplace.json`; the resolver attaches a typed misconfig sentinel that the install command consults at the validation-failure boundary, so the legitimate cross-host path (validation succeeds) emits no hint and the suggestion does not pollute working configs. (#1319) - `apm view --help` and the `view` row in `apm --help` now render in release binaries; PyInstaller's `optimize=2` was stripping `__doc__` from every Click command, and `view` was the only command that relied on its docstring instead of the explicit `help=` kwarg every other command defensively sets. Lowered the spec to `optimize=1` so asserts are still removed but docstrings survive, restoring Click's documented help-from-docstring fallback for all current and future commands. (#1298) ## [0.13.0] - 2026-05-11 diff --git a/src/apm_cli/commands/install.py b/src/apm_cli/commands/install.py index ab0d7f5d5..e63da3e15 100644 --- a/src/apm_cli/commands/install.py +++ b/src/apm_cli/commands/install.py @@ -349,6 +349,13 @@ def _resolve_package_references( invalid_outcomes = [] # (package, reason) tuples _marketplace_provenance = {} # canonical -> {discovered_via, marketplace_plugin_name} _apm_yml_entries = {} # canonical -> apm.yml entry (str or dict for HTTP deps) + # #1305: canonical -> (marketplace_name, plugin_name, CrossRepoMisconfigRisk) + # for cross-repo dict ``type: github`` sources on enterprise marketplaces + # whose bare ``repo`` would mis-route auth at ``github.com``. Recorded + # before validation runs so the validation-fail branch can emit an + # actionable hint -- ``_marketplace_provenance`` is only written on + # validation success and cannot be relied on at the failure boundary. + _misconfig_risks = {} validated_packages = [] dependencies_changed = False @@ -403,6 +410,13 @@ def warning_handler(msg): } package = canonical_str marketplace_dep_ref = getattr(resolution, "dependency_reference", None) + _risk = getattr(resolution, "cross_repo_misconfig_risk", None) + if _risk is not None: + _misconfig_risks[canonical_str] = ( + marketplace_name, + plugin_name, + _risk, + ) except Exception as mkt_err: reason = str(mkt_err) invalid_outcomes.append((package, reason)) @@ -521,6 +535,34 @@ def warning_handler(msg): invalid_outcomes.append((package, reason)) if logger: logger.validation_fail(package, reason) + # #1305: when a cross-repo dict ``type: github`` source on an + # enterprise marketplace fails validation, the failure is most + # likely the silent auth mis-route (bare canonical fell back to + # ``github.com``). Surface the host-qualify hint inline so the + # operator can correct ``marketplace.json`` without rerunning + # under ``--verbose`` to decode the auth trace. ``logger.warning`` + # is used (not ``info``) per the PR #1292 panel review's explicit + # guidance for this exact follow-up: a misconfiguration that + # voids ``apm install`` should be at warning level, not buried + # in info-level ambient output. The second clause acknowledges + # the legitimate cross-host alternative so operators whose + # github.com dep failed for a transient reason (rate limit, + # network, expired PAT) are not misdirected into adding an + # enterprise host prefix that would break a working config. + _risk_entry = _misconfig_risks.get(package) + if _risk_entry is not None and logger: + _mp_name, _plugin_name, _risk = _risk_entry + logger.warning( + f"'{_plugin_name}@{_mp_name}' is registered on " + f"'{_risk.marketplace_host}' but the plugin's bare " + f"`repo: {_risk.bare_repo_field}` resolved to " + "'github.com'. If you meant the enterprise host, set " + "the plugin's `repo` field to " + f"'{_risk.suggested_qualified_repo}' in marketplace.json. " + "If this is intentionally a github.com dependency, " + "verify your github.com credentials and that the " + "repository is accessible." + ) return ( valid_outcomes, diff --git a/src/apm_cli/marketplace/resolver.py b/src/apm_cli/marketplace/resolver.py index f62dbe4fc..6aae5b04d 100644 --- a/src/apm_cli/marketplace/resolver.py +++ b/src/apm_cli/marketplace/resolver.py @@ -47,6 +47,29 @@ _SEMVER_RANGE_CHARS = re.compile(r"[~^<>=!]") +@dataclass(frozen=True) +class CrossRepoMisconfigRisk: + """Signal that a cross-repo dict ``type: github`` source on an enterprise + GitHub-family marketplace resolved to a bare canonical (#1305). + + Attached to :class:`MarketplacePluginResolution` when the marketplace is on + ``*.ghe.com`` and the plugin's dict source declares a bare ``owner/repo`` + that does not match the marketplace project. The resolver deliberately + leaves these canonicals bare (PR #1292 scoped its host backfill to + in-marketplace sources), so ``DependencyReference.parse`` defaults the host + to ``github.com``. Two intents share this syntax -- a legitimate cross-host + ``github.com`` open-source dep, or a misconfigured same-host entry that + should have been ``corp.ghe.com/owner/repo`` -- and the resolver cannot + distinguish them. The install command consults this sentinel when the + package fails validation so an actionable hint surfaces only at the + failure boundary, never on the legitimate path. + """ + + marketplace_host: str + bare_repo_field: str + suggested_qualified_repo: str + + @dataclass class MarketplacePluginResolution: """Outcome of :func:`resolve_marketplace_plugin`. @@ -57,11 +80,15 @@ class MarketplacePluginResolution: subdirectory plugins), install logic should prefer it over :meth:`~apm_cli.models.dependency.reference.DependencyReference.parse` on :attr:`canonical` to avoid mis-parsing nested paths as GitLab project segments. + :attr:`cross_repo_misconfig_risk` is non-``None`` only for the #1305 + cross-repo bare-on-enterprise pattern; consumers emit it as a hint when the + package subsequently fails validation. """ canonical: str plugin: MarketplacePlugin dependency_reference: DependencyReference | None = None + cross_repo_misconfig_risk: CrossRepoMisconfigRisk | None = None def __iter__(self) -> Iterator[str | MarketplacePlugin]: yield self.canonical @@ -221,6 +248,55 @@ def _needs_canonical_host_prefix(canonical: str, host: str) -> bool: return first_segment.lower() != h.lower() +def _compute_cross_repo_misconfig_risk( + plugin: MarketplacePlugin, + source: MarketplaceSource, + canonical: str, + dep_ref: DependencyReference | None, +) -> CrossRepoMisconfigRisk | None: + """Identify the #1305 misconfiguration: cross-repo dict ``type: github`` + source with bare ``repo`` on an enterprise GitHub-family marketplace. + + Returns a :class:`CrossRepoMisconfigRisk` when **all** of: + + - ``dep_ref`` is ``None`` (GitHub-family virtual-shorthand path; GitLab and + self-managed FQDNs build a structured ref upstream and sidestep the bug) + - ``plugin.source`` is a dict whose normalized type is ``github`` (other + dict types -- ``gitlab``, ``git-subdir`` -- hit the same auth-routing + bug but the "host-qualify with marketplace host" remediation only + matches operator intent for the GitHub family) + - the source is **not** an in-marketplace reference (PR #1292 already + backfills the host for those) + - ``_needs_canonical_host_prefix`` agrees the canonical is bare and the + host is GitHub-family enterprise (``*.ghe.com``; idempotent against + already host-qualified, URL, and SSH forms) + - the ``repo`` field is a non-empty ``owner/repo`` shorthand + + Otherwise returns ``None``. Pure -- no logging, no side effects. + """ + if dep_ref is not None: + return None + if not isinstance(plugin.source, dict): + return None + if _coerce_dict_plugin_type(plugin.source) != "github": + return None + if _is_in_marketplace_source(plugin, source): + return None + if not _needs_canonical_host_prefix(canonical, source.host): + return None + repo_field = plugin.source.get("repo", "") + if not isinstance(repo_field, str): + return None + bare = repo_field.strip().lstrip("/") + if "/" not in bare: + return None + return CrossRepoMisconfigRisk( + marketplace_host=source.host, + bare_repo_field=bare, + suggested_qualified_repo=f"{source.host}/{bare}", + ) + + def _marketplace_https_git_url(source: MarketplaceSource) -> str: """HTTPS clone URL for the registered marketplace project (same project as ``marketplace.json``).""" segments = [p for p in f"{source.owner}/{source.repo}".split("/") if p] @@ -607,6 +683,19 @@ def _emit_warning(msg: str) -> None: marketplace_name, ) + # ---- Cross-repo misconfig sentinel (#1305) ---- + # PR #1292's host backfill only covers in-marketplace sources. A cross-repo + # dict ``type: github`` source with a bare ``repo`` on an enterprise + # marketplace cannot be safely backfilled here -- the bare syntax also + # legitimately means "a github.com open-source dep from this enterprise + # marketplace" -- so the canonical stays bare and downstream auth routes at + # github.com. Attach a sentinel so the install command can emit an + # actionable hint ONLY when the package subsequently fails validation; the + # legitimate cross-host path validates fine and never sees the hint. + cross_repo_misconfig_risk = _compute_cross_repo_misconfig_risk( + plugin, source, canonical, dep_ref + ) + # ---- Raw ref override ---- # When version_spec is provided it is treated as a raw git ref that # overrides whatever ref came from the marketplace source field. @@ -674,5 +763,8 @@ def _emit_warning(msg: str) -> None: logger.debug("Shadow detection failed", exc_info=True) return MarketplacePluginResolution( - canonical=canonical, plugin=plugin, dependency_reference=dep_ref + canonical=canonical, + plugin=plugin, + dependency_reference=dep_ref, + cross_repo_misconfig_risk=cross_repo_misconfig_risk, ) diff --git a/tests/integration/test_ghe_marketplace_install_e2e.py b/tests/integration/test_ghe_marketplace_install_e2e.py index 34cdaf9bd..26aba648b 100644 --- a/tests/integration/test_ghe_marketplace_install_e2e.py +++ b/tests/integration/test_ghe_marketplace_install_e2e.py @@ -205,14 +205,20 @@ def test_github_com_marketplace_keeps_github_default(self): assert ctx.host_info.kind == "github" def test_cross_repo_locks_known_silent_misroute(self): - """Regression trap for the cross-repo bug class tracked separately in #1305. - - A ``*.ghe.com`` marketplace with a cross-repo dict source bears the same - symptoms as #1285 -- canonical emerges bare, parse defaults to ``github.com``. - This is intentionally out of scope of PR #1292; #1305 tracks the fix - (which belongs in the install-time error handler, not the resolver). - The test locks the current behaviour so the future #1305 fix has an - explicit before/after diff to assert against. + """Regression trap for the cross-repo routing semantics + #1305 sentinel. + + A ``*.ghe.com`` marketplace with a cross-repo dict source bears the + same superficial symptoms as #1285 -- canonical emerges bare, parse + defaults to ``github.com``. The #1305 fix deliberately preserves + that resolver-level routing (a bare cross-repo ``repo`` also + legitimately means "a github.com open-source dep from this enterprise + marketplace") and instead attaches a + :class:`~apm_cli.marketplace.resolver.CrossRepoMisconfigRisk` sentinel + that the install command consults at the validation-failure boundary + to emit an actionable host-qualify hint. This test locks both halves: + the routing preservation (so the legitimate path is not regressed) + and the sentinel attachment (so the hint emission has the metadata + it needs). """ plugin = MarketplacePlugin( name="cross-repo", @@ -234,12 +240,143 @@ def test_cross_repo_locks_known_silent_misroute(self): ): result = resolve_marketplace_plugin("cross-repo", _REPO) - # Pre-existing behaviour: no host prefix for cross-repo (#1305 to fix). + # Routing preservation: cross-repo canonical stays bare; parse still + # falls back to ``github.com``. This is intentional -- the legitimate + # cross-host path validates successfully and never needs to recover + # the enterprise host. #1305 surfaces the diagnostic at install time + # when the misconfigured path subsequently fails validation. assert result.canonical == "anotherorg/anothertool/plugins/x" dep_ref = DependencyReference.parse(result.canonical) auth = AuthResolver() ctx = auth.resolve_for_dep(dep_ref) - assert ctx.host_info.host == "github.com", ( - "If this assertion fails, the cross-repo silent mis-route bug from " - "#1305 has been fixed -- update this test to reflect the new behaviour." + assert ctx.host_info.host == "github.com" + + # #1305: sentinel must attach so the install command's + # validation-fail branch has the metadata to emit the hint. + risk = result.cross_repo_misconfig_risk + assert risk is not None + assert risk.marketplace_host == _GHE_HOST + assert risk.bare_repo_field == "anotherorg/anothertool" + assert risk.suggested_qualified_repo == f"{_GHE_HOST}/anotherorg/anothertool" + + +@pytest.mark.integration +class TestCrossRepoMisconfigHintIntegration: + """End-to-end: the #1305 hint surfaces when a cross-repo bare entry on + a ``*.ghe.com`` marketplace fails validation. + + Unit tests in ``tests/unit/commands/`` mock ``DependencyReference`` and + ``resolve_marketplace_plugin``; this integration trap walks the real + ``_resolve_package_references`` + real ``InstallLogger`` and asserts on + the actual stdout the operator would see. Required by the PR review + panel (test-coverage-expert: ``outcome: missing`` on a secure-by-default + surface) and matches the e2e-integration convention PR #1292 established + with ``test_ghe_marketplace_backfills_host_on_bare_canonical`` above. + + Stubs at one seam only: + + - ``_validate_package_exists``: forces the failure outcome that triggers + the hint. The real validate path makes outbound HTTP calls; this stub + keeps the test deterministic. Everything between the resolver sentinel + and the logger render is the real code path. + """ + + @pytest.fixture(autouse=True) + def _isolate_github_host_env(self, monkeypatch): + monkeypatch.delenv("GITHUB_HOST", raising=False) + + def test_cross_repo_hint_emitted_on_validation_failure(self, capsys): + """The canonical misconfiguration scenario from #1305 surfaces a + warning-level hint identifying the marketplace host and the exact + host-qualified ``repo`` value to use as a fix.""" + from apm_cli.commands.install import _resolve_package_references + from apm_cli.core.command_logger import InstallLogger + + plugin = MarketplacePlugin( + name="shared-tool", + source={ + "type": "github", + "repo": "platform-team/shared-tool", + "path": "plugins/shared", + }, + ) + + with ( + patch( + "apm_cli.marketplace.resolver.get_marketplace_by_name", + return_value=_make_source(_GHE_HOST), + ), + patch( + "apm_cli.marketplace.resolver.fetch_or_cache", + return_value=_make_manifest(plugin), + ), + patch( + "apm_cli.commands.install._validate_package_exists", + return_value=False, + ), + ): + _resolve_package_references( + ["shared-tool@my-marketplace"], + [], + set(), + logger=InstallLogger(verbose=False), + ) + + captured = capsys.readouterr() + emitted = captured.out + # Hint identifies the plugin@marketplace + assert "'shared-tool@my-marketplace'" in emitted + # Marketplace host is named in the "registered on" clause + # (anchored substring sidesteps CodeQL bare-host pattern recognizers) + assert f"registered on '{_GHE_HOST}'" in emitted + # The bare repo from marketplace.json is echoed back + assert "`repo: platform-team/shared-tool`" in emitted + # Concrete remediation value the operator can copy-paste + assert f"'{_GHE_HOST}/platform-team/shared-tool'" in emitted + # Auth-expert clause acknowledges the legitimate-cross-host + # alternative so transient failures of real github.com deps are + # not misdirected into adding an enterprise host prefix. + assert "intentionally a github.com dependency" in emitted + + def test_legitimate_cross_host_validation_passes_no_hint(self, capsys): + """The legitimate cross-host case (validation passes) emits no hint. + + This is the entire reason the diagnostic lives at the + validation-failure boundary instead of resolver time.""" + from apm_cli.commands.install import _resolve_package_references + from apm_cli.core.command_logger import InstallLogger + + plugin = MarketplacePlugin( + name="shared-tool", + source={ + "type": "github", + "repo": "platform-team/shared-tool", + "path": "plugins/shared", + }, ) + + with ( + patch( + "apm_cli.marketplace.resolver.get_marketplace_by_name", + return_value=_make_source(_GHE_HOST), + ), + patch( + "apm_cli.marketplace.resolver.fetch_or_cache", + return_value=_make_manifest(plugin), + ), + patch( + "apm_cli.commands.install._validate_package_exists", + return_value=True, + ), + ): + _resolve_package_references( + ["shared-tool@my-marketplace"], + [], + set(), + logger=InstallLogger(verbose=False), + ) + + emitted = capsys.readouterr().out + # No hint substrings on the successful path. + assert "intentionally a github.com dependency" not in emitted + assert "If you meant the enterprise host" not in emitted diff --git a/tests/unit/commands/test_install_resolve_refs.py b/tests/unit/commands/test_install_resolve_refs.py index 60dcdf182..ac6900dad 100644 --- a/tests/unit/commands/test_install_resolve_refs.py +++ b/tests/unit/commands/test_install_resolve_refs.py @@ -280,3 +280,214 @@ def test_generated_entry_round_trips_via_from_apm_yml(self, tmp_path): assert ref.repo_url == "epm-ease/apm-registry" assert ref.virtual_path == "agents/ai-run-ba-flow" assert ref.is_virtual is True + + +# --------------------------------------------------------------------------- +# #1305 -- cross-repo bare-on-enterprise hint surfaces on validation failure +# --------------------------------------------------------------------------- + + +class TestResolvePackageReferencesCrossRepoMisconfigHint: + """When a marketplace resolution attaches a ``CrossRepoMisconfigRisk`` + sentinel and the package later fails validation, an actionable hint must + be emitted via the logger. The legitimate cross-host path validates + successfully and never reaches the hint branch.""" + + @staticmethod + def _resolution_with_risk(): + from apm_cli.marketplace.models import MarketplacePlugin + from apm_cli.marketplace.resolver import ( + CrossRepoMisconfigRisk, + MarketplacePluginResolution, + ) + + plugin = MarketplacePlugin( + name="shared-tool", + source={ + "type": "github", + "repo": "platform-team/shared-tool", + "path": "plugins/shared", + }, + ) + return MarketplacePluginResolution( + canonical="platform-team/shared-tool/plugins/shared", + plugin=plugin, + dependency_reference=None, + cross_repo_misconfig_risk=CrossRepoMisconfigRisk( + marketplace_host="corp.ghe.com", + bare_repo_field="platform-team/shared-tool", + suggested_qualified_repo=("corp.ghe.com/platform-team/shared-tool"), + ), + ) + + @staticmethod + def _resolution_without_risk(): + from apm_cli.marketplace.models import MarketplacePlugin + from apm_cli.marketplace.resolver import MarketplacePluginResolution + + plugin = MarketplacePlugin( + name="shared-tool", + source="./plugins/shared", + ) + return MarketplacePluginResolution( + canonical="myorg/my-marketplace/plugins/shared", + plugin=plugin, + dependency_reference=None, + cross_repo_misconfig_risk=None, + ) + + @patch("apm_cli.commands.install._validate_package_exists", return_value=False) + @patch("apm_cli.marketplace.resolver.resolve_marketplace_plugin") + @patch("apm_cli.marketplace.resolver.parse_marketplace_ref") + @patch("apm_cli.commands.install.DependencyReference") + def test_hint_emitted_on_validation_failure_with_risk( + self, + mock_dep_cls, + mock_parse_ref, + mock_resolve_mkt, + mock_validate, + ): + """Risk-bearing resolution that fails validation emits the hint.""" + mock_parse_ref.return_value = ("shared-tool", "my-marketplace", None) + mock_resolve_mkt.return_value = self._resolution_with_risk() + ref = _make_dep_ref( + "platform-team/shared-tool/plugins/shared", + "github.com/platform-team/shared-tool/plugins/shared", + ) + mock_dep_cls.parse.return_value = ref + mock_dep_cls.is_local_path.return_value = False + _disable_gitlab_direct_probe(mock_dep_cls) + + logger = MagicMock() + logger.verbose = False + + _resolve_package_references( + ["shared-tool@my-marketplace"], + [], + set(), + logger=logger, + ) + + # The hint must be emitted exactly once via ``logger.warning`` + # (PR #1292 panel review's explicit guidance for this follow-up). + # Assertions are anchored to the surrounding prose so a CodeQL + # "incomplete URL substring sanitization" pattern recognizer does + # not flag a bare hostname substring check in this test file. + warn_calls = list(logger.warning.call_args_list) + assert len(warn_calls) == 1 + # ``info`` must NOT be used for the hint (the original PR shipped + # ``logger.info`` and was caught by the 3-persona panel convergence). + assert logger.info.call_args_list == [] + emitted = warn_calls[0].args[0] + # Hint identifies the plugin@marketplace and both intent branches. + assert "'shared-tool@my-marketplace'" in emitted + assert "registered on 'corp.ghe.com'" in emitted + assert "`repo: platform-team/shared-tool`" in emitted + assert "'corp.ghe.com/platform-team/shared-tool'" in emitted + # Second clause acknowledges the legitimate-cross-host path so a + # transient-failure on a real github.com dep is not misdirected. + assert "intentionally a github.com dependency" in emitted + # Stale ``Hint:`` prefix from the original PR must not return; the + # warning symbol carries the advisory signal on its own. + assert "Hint:" not in emitted + + @patch("apm_cli.commands.install._validate_package_exists", return_value=True) + @patch("apm_cli.marketplace.resolver.resolve_marketplace_plugin") + @patch("apm_cli.marketplace.resolver.parse_marketplace_ref") + @patch("apm_cli.commands.install.DependencyReference") + def test_hint_not_emitted_when_validation_passes_even_with_risk( + self, + mock_dep_cls, + mock_parse_ref, + mock_resolve_mkt, + mock_validate, + ): + """The legitimate cross-host path (validation succeeds) must NOT + emit the hint -- this is the entire reason the hint lives at the + failure boundary instead of at resolver time.""" + mock_parse_ref.return_value = ("shared-tool", "my-marketplace", None) + mock_resolve_mkt.return_value = self._resolution_with_risk() + ref = _make_dep_ref( + "platform-team/shared-tool/plugins/shared", + "github.com/platform-team/shared-tool/plugins/shared", + ) + mock_dep_cls.parse.return_value = ref + mock_dep_cls.is_local_path.return_value = False + _disable_gitlab_direct_probe(mock_dep_cls) + + logger = MagicMock() + logger.verbose = False + + _resolve_package_references( + ["shared-tool@my-marketplace"], + [], + set(), + logger=logger, + ) + + assert logger.warning.call_args_list == [] + + @patch("apm_cli.commands.install._validate_package_exists", return_value=False) + @patch("apm_cli.marketplace.resolver.resolve_marketplace_plugin") + @patch("apm_cli.marketplace.resolver.parse_marketplace_ref") + @patch("apm_cli.commands.install.DependencyReference") + def test_no_hint_when_resolution_has_no_risk( + self, + mock_dep_cls, + mock_parse_ref, + mock_resolve_mkt, + mock_validate, + ): + """In-marketplace / non-enterprise resolutions carry no risk + sentinel; validation-fail must NOT print a hint.""" + mock_parse_ref.return_value = ("shared-tool", "my-marketplace", None) + mock_resolve_mkt.return_value = self._resolution_without_risk() + ref = _make_dep_ref( + "myorg/my-marketplace/plugins/shared", + "github.com/myorg/my-marketplace/plugins/shared", + ) + mock_dep_cls.parse.return_value = ref + mock_dep_cls.is_local_path.return_value = False + _disable_gitlab_direct_probe(mock_dep_cls) + + logger = MagicMock() + logger.verbose = False + + _resolve_package_references( + ["shared-tool@my-marketplace"], + [], + set(), + logger=logger, + ) + + assert logger.warning.call_args_list == [] + + @patch("apm_cli.commands.install._validate_package_exists", return_value=False) + @patch("apm_cli.commands.install.DependencyReference") + def test_no_hint_for_plain_owner_repo_failure( + self, + mock_dep_cls, + mock_validate, + ): + """A bare ``owner/repo`` (no marketplace) that fails validation + must NOT trigger the hint -- the risk map is only populated by + marketplace resolutions.""" + ref = _make_dep_ref( + "platform-team/shared-tool", + "github.com/platform-team/shared-tool", + ) + mock_dep_cls.parse.return_value = ref + mock_dep_cls.is_local_path.return_value = False + _disable_gitlab_direct_probe(mock_dep_cls) + + logger = MagicMock() + logger.verbose = False + + _resolve_package_references( + ["platform-team/shared-tool"], + [], + set(), + logger=logger, + ) + + assert logger.warning.call_args_list == [] diff --git a/tests/unit/marketplace/test_marketplace_resolver.py b/tests/unit/marketplace/test_marketplace_resolver.py index bdf8640e2..59824343f 100644 --- a/tests/unit/marketplace/test_marketplace_resolver.py +++ b/tests/unit/marketplace/test_marketplace_resolver.py @@ -1019,6 +1019,318 @@ def test_github_com_canonical_remains_bare(self, mock_get, mock_fetch): assert result.dependency_reference is None +class TestCrossRepoMisconfigRisk: + """``MarketplacePluginResolution.cross_repo_misconfig_risk`` for #1305. + + PR #1292 narrowly scoped its ``*.ghe.com`` host backfill to in-marketplace + sources because cross-repo dict ``repo`` syntax legitimately serves two + intents on an enterprise marketplace (open-source ``github.com`` dep vs + misconfigured same-host entry). The resolver cannot distinguish them, but + it can flag the signature so the install command surfaces an actionable + hint when validation later fails. These tests lock that signature shape. + """ + + @pytest.fixture + def ghe_marketplace_source(self) -> MarketplaceSource: + return MarketplaceSource( + name="my-marketplace", + owner="myorg", + repo="my-marketplace", + host="corp.ghe.com", + branch="main", + ) + + @staticmethod + def _manifest_with_plugin(plugin: MarketplacePlugin) -> MarketplaceManifest: + return MarketplaceManifest( + name="my-marketplace", + plugins=(plugin,), + plugin_root="", + ) + + @patch("apm_cli.marketplace.resolver.fetch_or_cache") + @patch("apm_cli.marketplace.resolver.get_marketplace_by_name") + def test_cross_repo_bare_attaches_risk(self, mock_get, mock_fetch, ghe_marketplace_source): + """Textbook #1305: ``type: github`` + bare cross-repo ``repo`` on ``*.ghe.com``.""" + plugin = MarketplacePlugin( + name="shared-tool", + source={ + "type": "github", + "repo": "platform-team/shared-tool", + "path": "plugins/shared", + }, + ) + mock_get.return_value = ghe_marketplace_source + mock_fetch.return_value = self._manifest_with_plugin(plugin) + + result = resolve_marketplace_plugin("shared-tool", "my-marketplace") + risk = result.cross_repo_misconfig_risk + assert risk is not None + assert risk.marketplace_host == "corp.ghe.com" + assert risk.bare_repo_field == "platform-team/shared-tool" + assert risk.suggested_qualified_repo == "corp.ghe.com/platform-team/shared-tool" + # Resolver leaves canonical bare -- behavior contract unchanged + assert result.canonical == "platform-team/shared-tool/plugins/shared" + 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_cross_repo_inferred_github_via_path_attaches_risk( + self, mock_get, mock_fetch, ghe_marketplace_source + ): + """Dict source with no ``type`` but with ``path`` is inferred ``github``.""" + plugin = MarketplacePlugin( + name="inferred", + source={ + "repo": "platform-team/shared-tool", + "path": "plugins/inferred", + }, + ) + mock_get.return_value = ghe_marketplace_source + mock_fetch.return_value = self._manifest_with_plugin(plugin) + + result = resolve_marketplace_plugin("inferred", "my-marketplace") + assert result.cross_repo_misconfig_risk is not None + + @patch("apm_cli.marketplace.resolver.fetch_or_cache") + @patch("apm_cli.marketplace.resolver.get_marketplace_by_name") + def test_cross_repo_kind_github_attaches_risk( + self, mock_get, mock_fetch, ghe_marketplace_source + ): + """``kind: github`` (Claude-style) routes through the same path.""" + plugin = MarketplacePlugin( + name="kind-style", + source={ + "kind": "github", + "repo": "platform-team/shared-tool", + "path": "plugins/kind-style", + }, + ) + mock_get.return_value = ghe_marketplace_source + mock_fetch.return_value = self._manifest_with_plugin(plugin) + + result = resolve_marketplace_plugin("kind-style", "my-marketplace") + assert result.cross_repo_misconfig_risk is not None + + @patch("apm_cli.marketplace.resolver.fetch_or_cache") + @patch("apm_cli.marketplace.resolver.get_marketplace_by_name") + def test_cross_repo_uppercase_type_attaches_risk( + self, mock_get, mock_fetch, ghe_marketplace_source + ): + """``type: GitHub`` (mixed case) is normalized to ``github``.""" + plugin = MarketplacePlugin( + name="upper", + source={ + "type": "GitHub", + "repo": "platform-team/shared-tool", + "path": "plugins/upper", + }, + ) + mock_get.return_value = ghe_marketplace_source + mock_fetch.return_value = self._manifest_with_plugin(plugin) + + result = resolve_marketplace_plugin("upper", "my-marketplace") + assert result.cross_repo_misconfig_risk is not None + + @patch("apm_cli.marketplace.resolver.fetch_or_cache") + @patch("apm_cli.marketplace.resolver.get_marketplace_by_name") + def test_cross_repo_host_qualified_no_risk(self, mock_get, mock_fetch, ghe_marketplace_source): + """``repo: corp.ghe.com/owner/repo`` already routes; no hint needed.""" + plugin = MarketplacePlugin( + name="qualified", + source={ + "type": "github", + "repo": "corp.ghe.com/platform-team/shared-tool", + "path": "plugins/qualified", + }, + ) + mock_get.return_value = ghe_marketplace_source + mock_fetch.return_value = self._manifest_with_plugin(plugin) + + result = resolve_marketplace_plugin("qualified", "my-marketplace") + assert result.cross_repo_misconfig_risk is None + + @patch("apm_cli.marketplace.resolver.fetch_or_cache") + @patch("apm_cli.marketplace.resolver.get_marketplace_by_name") + def test_cross_repo_url_form_no_risk(self, mock_get, mock_fetch, ghe_marketplace_source): + """Full ``https://`` URL carries its own host; hint inapplicable.""" + plugin = MarketplacePlugin( + name="url", + source={ + "type": "github", + "repo": "https://corp.ghe.com/platform-team/shared-tool", + "path": "plugins/url", + }, + ) + mock_get.return_value = ghe_marketplace_source + mock_fetch.return_value = self._manifest_with_plugin(plugin) + + result = resolve_marketplace_plugin("url", "my-marketplace") + assert result.cross_repo_misconfig_risk is None + + @patch("apm_cli.marketplace.resolver.fetch_or_cache") + @patch("apm_cli.marketplace.resolver.get_marketplace_by_name") + def test_cross_repo_ssh_form_no_risk(self, mock_get, mock_fetch, ghe_marketplace_source): + """SSH SCP shorthand carries its own host; hint inapplicable.""" + plugin = MarketplacePlugin( + name="ssh", + source={ + "type": "github", + "repo": "git@corp.ghe.com:platform-team/shared-tool", + "path": "plugins/ssh", + }, + ) + mock_get.return_value = ghe_marketplace_source + mock_fetch.return_value = self._manifest_with_plugin(plugin) + + result = resolve_marketplace_plugin("ssh", "my-marketplace") + assert result.cross_repo_misconfig_risk is None + + @patch("apm_cli.marketplace.resolver.fetch_or_cache") + @patch("apm_cli.marketplace.resolver.get_marketplace_by_name") + def test_cross_repo_gitlab_type_no_risk(self, mock_get, mock_fetch, ghe_marketplace_source): + """``type: gitlab`` cross-repo hits the same routing bug but the + "host-qualify with marketplace host" remediation does not match the + operator's intent (they meant gitlab.com, not corp.ghe.com).""" + plugin = MarketplacePlugin( + name="gl", + source={ + "type": "gitlab", + "repo": "platform-team/shared-tool", + "path": "plugins/gl", + }, + ) + mock_get.return_value = ghe_marketplace_source + mock_fetch.return_value = self._manifest_with_plugin(plugin) + + result = resolve_marketplace_plugin("gl", "my-marketplace") + assert result.cross_repo_misconfig_risk is None + + @patch("apm_cli.marketplace.resolver.fetch_or_cache") + @patch("apm_cli.marketplace.resolver.get_marketplace_by_name") + def test_cross_repo_git_subdir_type_no_risk(self, mock_get, mock_fetch, ghe_marketplace_source): + """``type: git-subdir`` cross-repo: same wording mismatch as gitlab.""" + plugin = MarketplacePlugin( + name="gs", + source={ + "type": "git-subdir", + "repo": "platform-team/shared-tool", + "subdir": "plugins/gs", + }, + ) + mock_get.return_value = ghe_marketplace_source + mock_fetch.return_value = self._manifest_with_plugin(plugin) + + result = resolve_marketplace_plugin("gs", "my-marketplace") + assert result.cross_repo_misconfig_risk is None + + @patch("apm_cli.marketplace.resolver.fetch_or_cache") + @patch("apm_cli.marketplace.resolver.get_marketplace_by_name") + def test_in_marketplace_dict_source_no_risk(self, mock_get, mock_fetch, ghe_marketplace_source): + """In-marketplace dict source (PR #1292's domain) does not get a risk flag.""" + plugin = MarketplacePlugin( + name="in-mkt", + source={ + "type": "github", + "repo": "myorg/my-marketplace", + "path": "plugins/in-mkt", + }, + ) + mock_get.return_value = ghe_marketplace_source + mock_fetch.return_value = self._manifest_with_plugin(plugin) + + result = resolve_marketplace_plugin("in-mkt", "my-marketplace") + assert result.cross_repo_misconfig_risk is None + + @patch("apm_cli.marketplace.resolver.fetch_or_cache") + @patch("apm_cli.marketplace.resolver.get_marketplace_by_name") + def test_in_marketplace_string_source_no_risk( + self, mock_get, mock_fetch, ghe_marketplace_source + ): + """Relative string source is always in-marketplace; no risk flag.""" + plugin = MarketplacePlugin(name="rel", source="./plugins/rel") + mock_get.return_value = ghe_marketplace_source + mock_fetch.return_value = self._manifest_with_plugin(plugin) + + result = resolve_marketplace_plugin("rel", "my-marketplace") + assert result.cross_repo_misconfig_risk is None + + @patch("apm_cli.marketplace.resolver.fetch_or_cache") + @patch("apm_cli.marketplace.resolver.get_marketplace_by_name") + def test_github_com_marketplace_cross_repo_no_risk(self, mock_get, mock_fetch): + """Cross-repo on a plain ``github.com`` marketplace: bare canonical + is correct (parse default matches the marketplace host) so no hint.""" + gh_source = MarketplaceSource( + name="my-marketplace", + owner="myorg", + repo="my-marketplace", + host="github.com", + branch="main", + ) + plugin = MarketplacePlugin( + name="cross", + source={ + "type": "github", + "repo": "platform-team/shared-tool", + "path": "plugins/cross", + }, + ) + mock_get.return_value = gh_source + mock_fetch.return_value = self._manifest_with_plugin(plugin) + + result = resolve_marketplace_plugin("cross", "my-marketplace") + assert result.cross_repo_misconfig_risk is None + + @patch("apm_cli.marketplace.resolver.fetch_or_cache") + @patch("apm_cli.marketplace.resolver.get_marketplace_by_name") + def test_cross_repo_source_field_synonym_attaches_risk( + self, mock_get, mock_fetch, ghe_marketplace_source + ): + """``source: github`` synonym (third leg of the ``type``/``kind``/``source`` trio).""" + plugin = MarketplacePlugin( + name="src-key", + source={ + "source": "github", + "repo": "platform-team/shared-tool", + "path": "plugins/src-key", + }, + ) + mock_get.return_value = ghe_marketplace_source + mock_fetch.return_value = self._manifest_with_plugin(plugin) + + result = resolve_marketplace_plugin("src-key", "my-marketplace") + assert result.cross_repo_misconfig_risk is not None + + def test_compute_returns_none_on_no_slash_repo_field(self): + """Defensive guard inside the helper: ``repo`` without ``/`` is + rejected by ``_resolve_github_source`` upstream, but if a future + refactor ever bypasses that we must not synthesize a nonsense + ``host/no-slash`` suggestion. Calls the helper directly because + no real resolver flow lets us reach it with this input.""" + from apm_cli.marketplace.resolver import _compute_cross_repo_misconfig_risk + + plugin = MarketplacePlugin( + name="bad", + source={ + "type": "github", + "repo": "no-slash", + "path": "plugins/bad", + }, + ) + source = MarketplaceSource( + name="my-marketplace", + owner="myorg", + repo="my-marketplace", + host="corp.ghe.com", + branch="main", + ) + # Hand-build a plausible (if malformed) canonical the way + # ``_resolve_github_source`` would have if its guard were removed. + canonical = "no-slash/plugins/bad" + risk = _compute_cross_repo_misconfig_risk(plugin, source, canonical, None) + assert risk is None + + class TestGitLabShorthandParseVsStructuredRef: """``DependencyReference.parse`` on a long FQDN does not split monorepo paths on GitLab hosts."""