From 28b7a30332edf4296e02cd1ea0d57a79213a085b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E8=A8=B1=E5=85=83=E8=B1=AA?= <146086744+edenfunf@users.noreply.github.com> Date: Tue, 21 Apr 2026 15:52:07 +0800 Subject: [PATCH 1/2] fix(install): surface custom port in generic host clone/ls-remote error The two is_generic error branches in github_downloader rendered the bare host, dropping ``:{port}`` from the ``"For private repositories on {host}"`` hint. Users on Bitbucket Datacenter (or any self-hosted host using a non-default port) saw a diagnostic that hid the very detail they needed to verify their git credential helper against. Route both branches through ``AuthResolver.classify_host(...).display_name`` so the generic path shares port rendering with the adjacent ADO and auth branches (which already used ``build_error_context`` -> ``host_info.display_name``). Keep the ``"the target host"`` fallback for the (unreachable-today but defensive) ``dep_host=None`` case, and mirror the ``dep_ref.port if dep_ref else None`` guard used at the neighbouring call sites. Scope-limited: no new signatures, no schema impact; only the two ``host_name = dep_host or "..."`` lines change. Regression tests in tests/unit/test_generic_host_error_port.py cover both call sites with ssh/https custom port and a no-port control. Closes #798 --- src/apm_cli/deps/github_downloader.py | 16 ++- tests/unit/test_generic_host_error_port.py | 157 +++++++++++++++++++++ 2 files changed, 171 insertions(+), 2 deletions(-) create mode 100644 tests/unit/test_generic_host_error_port.py diff --git a/src/apm_cli/deps/github_downloader.py b/src/apm_cli/deps/github_downloader.py index 65cd05bbc..2ed9f0099 100644 --- a/src/apm_cli/deps/github_downloader.py +++ b/src/apm_cli/deps/github_downloader.py @@ -861,7 +861,13 @@ def _env_for(use_token_attempt: bool) -> Dict[str, str]: port=dep_ref.port if dep_ref else None, ) elif is_generic: - host_name = dep_host or "the target host" + if dep_host: + host_info = self.auth_resolver.classify_host( + dep_host, port=dep_ref.port if dep_ref else None, + ) + host_name = host_info.display_name + else: + host_name = "the target host" error_msg += ( f"For private repositories on {host_name}, configure SSH keys or a git credential helper. " f"APM delegates authentication to git for non-GitHub/ADO hosts." @@ -1032,7 +1038,13 @@ def list_remote_refs(self, dep_ref: DependencyReference) -> List[RemoteRef]: error_msg = f"Failed to list remote refs for {repo_url_base}. " if is_generic: - host_name = dep_host or "the target host" + if dep_host: + host_info = self.auth_resolver.classify_host( + dep_host, port=dep_ref.port if dep_ref else None, + ) + host_name = host_info.display_name + else: + host_name = "the target host" error_msg += ( f"For private repositories on {host_name}, configure SSH keys " f"or a git credential helper. " diff --git a/tests/unit/test_generic_host_error_port.py b/tests/unit/test_generic_host_error_port.py new file mode 100644 index 000000000..5f7764f7c --- /dev/null +++ b/tests/unit/test_generic_host_error_port.py @@ -0,0 +1,157 @@ +"""Regression tests for issue #798 -- is_generic error paths must render host:port. + +When a non-GitHub/non-ADO host fails during ``_clone_with_fallback`` or +``list_remote_refs``, the diagnostic hint ``"For private repositories on +{host}, ..."`` previously rendered the bare host and dropped the port. +Users on Bitbucket Datacenter (custom port 7999) or any self-hosted host +on a non-default port saw a message that hid the very detail they +needed to verify their credentials against. + +The fix routes both call sites through +``AuthResolver.classify_host(...).display_name``, so the generic branch +shares port rendering with the adjacent ADO / auth branches (which +already used ``build_error_context`` -> ``host_info.display_name``). +""" + +import os +import shutil +import tempfile +from pathlib import Path +from unittest.mock import MagicMock, patch + +import pytest +from git.exc import GitCommandError + +from apm_cli.deps.github_downloader import GitHubPackageDownloader +from apm_cli.models.apm_package import DependencyReference + + +def _make_downloader(): + """Build a GitHubPackageDownloader with a real AuthResolver. + + The real resolver is required here: the regression is specifically + about ``classify_host`` feeding ``HostInfo.display_name`` into the + rendered error message. A mocked resolver would bypass the exact + integration under test. + """ + with patch.dict(os.environ, {}, clear=True), patch( + "apm_cli.core.token_manager.GitHubTokenManager.resolve_credential_from_git", + return_value=None, + ): + dl = GitHubPackageDownloader() + dl.auth_resolver._cache.clear() + dl._resolve_dep_token = MagicMock(return_value=None) + return dl + + +def _diagnostic_prefix(msg: str) -> str: + """Strip the `` Last error: ...`` tail so assertions only see our hint. + + The sanitized git-command text can echo arbitrary hostnames; we + don't want a bare-host regression hiding behind that noise. + """ + return msg.split(" Last error:", 1)[0] + + +class TestGenericHostCloneErrorPort: + """``_clone_with_fallback`` generic branch (github_downloader.py ~L864).""" + + def _clone_error(self, dep) -> str: + dl = _make_downloader() + + def _fake_clone(*_args, **_kwargs): + raise GitCommandError("clone", "failed") + + with patch.dict(os.environ, {}, clear=True), patch( + "apm_cli.core.token_manager.GitHubTokenManager.resolve_credential_from_git", + return_value=None, + ), patch("apm_cli.deps.github_downloader.Repo") as MockRepo: + MockRepo.clone_from.side_effect = _fake_clone + target = Path(tempfile.mkdtemp()) + try: + with pytest.raises(RuntimeError) as exc_info: + dl._clone_with_fallback(dep.repo_url, target, dep_ref=dep) + return str(exc_info.value) + finally: + shutil.rmtree(target, ignore_errors=True) + + def test_ssh_custom_port_surfaces_in_error(self): + """Bitbucket-DC-style ssh://host:7999/... -> hint names host:7999.""" + dep = DependencyReference.parse( + "ssh://git@bitbucket.example.com:7999/project/repo.git" + ) + assert dep.port == 7999 + + prefix = _diagnostic_prefix(self._clone_error(dep)) + assert "For private repositories on bitbucket.example.com:7999," in prefix + + def test_https_custom_port_surfaces_in_error(self): + """https://host:7990/... -> hint names host:7990.""" + dep = DependencyReference.parse( + "https://bitbucket.example.com:7990/project/repo.git" + ) + assert dep.port == 7990 + + prefix = _diagnostic_prefix(self._clone_error(dep)) + assert "For private repositories on bitbucket.example.com:7990," in prefix + + def test_no_port_renders_bare_host(self): + """Default-port dep has no port suffix -- no regression for common case.""" + dep = DependencyReference.parse( + "https://gitlab.example.com/team/repo.git" + ) + assert dep.port is None + + prefix = _diagnostic_prefix(self._clone_error(dep)) + assert "For private repositories on gitlab.example.com," in prefix + assert "gitlab.example.com:" not in prefix, ( + f"bare-host case must not synthesise a stray ':' suffix: {prefix!r}" + ) + + +class TestGenericHostLsRemoteErrorPort: + """``list_remote_refs`` generic branch (github_downloader.py ~L1035).""" + + def _ls_remote_error(self, dep) -> str: + dl = _make_downloader() + with patch.dict(os.environ, {}, clear=True), patch( + "apm_cli.core.token_manager.GitHubTokenManager.resolve_credential_from_git", + return_value=None, + ), patch( + "apm_cli.deps.github_downloader.git.cmd.Git" + ) as MockGitCmd: + mock_git = MockGitCmd.return_value + mock_git.ls_remote.side_effect = GitCommandError("ls-remote", 128) + with pytest.raises(RuntimeError) as exc_info: + dl.list_remote_refs(dep) + return str(exc_info.value) + + def test_ssh_custom_port_surfaces_in_error(self): + dep = DependencyReference.parse( + "ssh://git@bitbucket.example.com:7999/project/repo.git" + ) + assert dep.port == 7999 + + prefix = _diagnostic_prefix(self._ls_remote_error(dep)) + assert "For private repositories on bitbucket.example.com:7999," in prefix + + def test_https_custom_port_surfaces_in_error(self): + dep = DependencyReference.parse( + "https://bitbucket.example.com:7990/project/repo.git" + ) + assert dep.port == 7990 + + prefix = _diagnostic_prefix(self._ls_remote_error(dep)) + assert "For private repositories on bitbucket.example.com:7990," in prefix + + def test_no_port_renders_bare_host(self): + dep = DependencyReference.parse( + "https://gitlab.example.com/team/repo.git" + ) + assert dep.port is None + + prefix = _diagnostic_prefix(self._ls_remote_error(dep)) + assert "For private repositories on gitlab.example.com," in prefix + assert "gitlab.example.com:" not in prefix, ( + f"bare-host case must not synthesise a stray ':' suffix: {prefix!r}" + ) From 4a88c8e2cbe307b723b23139fc0624146d780b81 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E8=A8=B1=E5=85=83=E8=B1=AA?= <146086744+edenfunf@users.noreply.github.com> Date: Wed, 22 Apr 2026 19:42:52 +0800 Subject: [PATCH 2/2] fix(install): address review feedback on #798 generic-host port fix + CHANGELOG Code changes: - Simplify port guard in ``list_remote_refs``' is_generic branch: ``port=dep_ref.port if dep_ref else None`` -> ``port=dep_ref.port``. ``list_remote_refs(self, dep_ref: DependencyReference)`` has no Optional on ``dep_ref`` and L1026 already dereferences it unconditionally, so the guard implied a nullability contract that does not exist on this code path. The clone-path ternary at L866 stays -- ``_clone_with_fallback``'s L715 does treat ``dep_ref`` as optional, so the guard is load-bearing there. - Use integer exit code 128 instead of the string "failed" in the clone test fixture. Matches the adjacent ls-remote fixture (L124) and ``test_list_remote_refs.py``; GitPython's ``GitCommandError`` status argument is a shell exit code, so an int models real failures more faithfully than a placeholder string. Deliberately not adopted (rationale recorded for the next reviewer): - No ``_generic_host_display_name(dep_host, dep_ref)`` helper: the single source of truth for port rendering already lives on ``HostInfo.display_name``; a private helper would only relocate the two-line ``classify_host(...).display_name`` call without adding defence against drift. Future refinements (e.g. the default-port normalisation tracked in #797, if it lands on ``HostInfo.__post_init__``) propagate to both call sites through ``display_name`` regardless of whether the call sites share a wrapper. #798 was scoped explicitly as a two-line change on each branch. - No ``@pytest.mark.parametrize`` over the six regression cases: each test has a self-documenting name (``test_ssh_custom_port_surfaces_in_error``, ``test_no_port_renders_bare_host``, etc.) which keeps failure triage grep-friendly. Clone and ls-remote branches use different mocks (``Repo.clone_from`` vs ``git.cmd.Git``), and the no-port variant asserts a negative property, so the six-case matrix does not compress cleanly into a single parametrize table without either bifurcating the parametrize or reducing assertion clarity. CHANGELOG: add a ``### Fixed`` entry under ``[Unreleased]`` documenting the ``host:port`` surfacing in the generic clone / ls-remote error branches. --- CHANGELOG.md | 1 + src/apm_cli/deps/github_downloader.py | 2 +- tests/unit/test_generic_host_error_port.py | 2 +- 3 files changed, 3 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 18dda5afe..cf5eea814 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -47,6 +47,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - `apm mcp search`, `apm mcp list`, and `apm mcp show` now honour the `MCP_REGISTRY_URL` environment variable (previously hardcoded to the public registry), bringing them in line with `apm install --mcp`. When the variable is set, the discovery commands print a one-line `Registry: ` diagnostic and surface the configured URL in network-error messages so misconfigured enterprise registries are obvious (#813) - `apm install --mcp ... --dry-run` now validates the would-be entry through the same chokepoint the real install uses, so dry-run never previews "success" for an entry `apm install` would reject (empty / whitespace-only / over-128-char / embedded `..` names, invalid transport-conflict combinations) (#810) - `SimpleRegistryClient` now applies a `(connect=10s, read=30s)` timeout on every registry HTTP call, removing the unbounded-hang failure mode when a registry is slow or unreachable. Operators can tune via `MCP_REGISTRY_CONNECT_TIMEOUT` / `MCP_REGISTRY_READ_TIMEOUT` env vars; invalid values silently fall back to defaults (#810) +- Surface the custom port in the `apm install` diagnostic hint when clone or `git ls-remote` fails against a non-GitHub/ADO host: the `is_generic` error branches now route through `HostInfo.display_name` so Bitbucket Datacenter users (SSH 7999) and other self-hosted-on-custom-port deployments see `host:port` instead of a bare hostname, aligning with the adjacent ADO / auth branches that already used `AuthResolver.build_error_context`. Closes #798 (#804) ### Security diff --git a/src/apm_cli/deps/github_downloader.py b/src/apm_cli/deps/github_downloader.py index 2ed9f0099..6bbcf4561 100644 --- a/src/apm_cli/deps/github_downloader.py +++ b/src/apm_cli/deps/github_downloader.py @@ -1040,7 +1040,7 @@ def list_remote_refs(self, dep_ref: DependencyReference) -> List[RemoteRef]: if is_generic: if dep_host: host_info = self.auth_resolver.classify_host( - dep_host, port=dep_ref.port if dep_ref else None, + dep_host, port=dep_ref.port, ) host_name = host_info.display_name else: diff --git a/tests/unit/test_generic_host_error_port.py b/tests/unit/test_generic_host_error_port.py index 5f7764f7c..24f3da70e 100644 --- a/tests/unit/test_generic_host_error_port.py +++ b/tests/unit/test_generic_host_error_port.py @@ -60,7 +60,7 @@ def _clone_error(self, dep) -> str: dl = _make_downloader() def _fake_clone(*_args, **_kwargs): - raise GitCommandError("clone", "failed") + raise GitCommandError("clone", 128) with patch.dict(os.environ, {}, clear=True), patch( "apm_cli.core.token_manager.GitHubTokenManager.resolve_credential_from_git",