fix(install): surface custom port in generic host clone/ls-remote error#804
Conversation
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 microsoft#798
edenfunf
requested review from
danielmeppiel and
sergio-sisternes-epam
as code owners
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
This PR fixes generic git-host error hints so they include custom ports (e.g., bitbucket.corp.com:7999) when clone or ls-remote fails, and adds regression tests to prevent future regressions.
Changes:
- Update generic-host error handling in
GitHubPackageDownloaderto useAuthResolver.classify_host(...).display_name(includes port). - Add unit tests covering both
_clone_with_fallbackandlist_remote_refsgeneric error branches for SSH/HTTPS custom ports and no-port control cases.
Show a summary per file
| File | Description |
|---|---|
src/apm_cli/deps/github_downloader.py |
Ensures generic-host private-repo hint renders host:port via HostInfo.display_name. |
tests/unit/test_generic_host_error_port.py |
Adds regression coverage to assert port rendering in both generic error paths. |
Copilot's findings
- Files reviewed: 2/2 changed files
- Comments generated: 4
…ort 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 microsoft#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. microsoft#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.
APM Review Panel VerdictDisposition: REQUEST_CHANGES -- one required action before merge (CHANGELOG entry); fix itself is clean and ready. Per-persona findingsPython Architect: No architectural concerns. The fix follows the exact pattern already used by the adjacent ADO branches ( CLI Logging Expert: Error message improvement is clean and consistent. Both DevX UX Expert: Real ergonomic win for Bitbucket Datacenter and any self-hosted server on a non-standard port. Before this fix, a user seeing Supply Chain Security Expert: No security surface changes. Auth Expert: OSS Growth Hacker: Bitbucket Datacenter adoption is a real enterprise wedge. Self-hosted Git servers on custom ports are ubiquitous in regulated industries (finance, healthcare, defense) -- the exact cohort APM's enterprise story needs. This fix removes a concrete papercut that would cause a support ticket or a dropped eval. No conversion surface changes needed; the fix speaks for itself in release notes. Side-channel to CEO: recommend leading with the "Bitbucket DC" framing in CHANGELOG and release notes -- it names a concrete product, which converts better than "custom port" in searches. CEO arbitrationThis is a clean, well-scoped bug fix that closes a real user-reported issue (#798) with appropriate test coverage (6 cases across both error branches: SSH custom port, HTTPS custom port, and no-port control for each). The implementation is consistent with the existing codebase pattern -- no new abstractions, no signature changes, no schema impact. The PR is merge-ready on the technical merits. The only gap is a missing Required actions before merge
Optional follow-ups
|
* chore: prepare v0.9.2 release Bumps version to 0.9.2 and finalizes CHANGELOG with one-line summaries for each PR merged since 0.9.1. Highlights: - ADO AAD bearer-token auth (#856) - Governance Guide + enterprise docs IA refactor (#851, #858) - Merge Gate orchestrator + single-authority aggregation (#865, #867) - Landing + first-package docs rewrite (#855, #866) - gh-aw imports migration (#864) - Custom-port surfacing fix (#804) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * ci: simplify merge-gate to single pull_request trigger The dual-trigger pattern (pull_request + pull_request_target with concurrency cancel-in-progress) shipped in #865 was over-engineered. It produced TWO 'gate' check-runs per SHA -- one SUCCESS, one CANCELLED -- and branch protection's status-check rollup treats CANCELLED as failure, so PRs were silently BLOCKED unless an admin overrode (which masked the bug on #867). GitHub Actions has no primitive for 'either of these events succeeded'. World-class OSS projects (kubernetes, rust, deno, next.js) accept this and use a single trigger. The cost: a dropped 'pull_request' webhook (rare; observed once on PR #856) requires manual recovery. Recovery paths now documented at top of file: - push empty commit - gh workflow run merge-gate.yml -f pr_number=NNN - close + reopen PR Replaces the dual-trigger + bootstrap-fetch dance with a clean two-job flow: resolve-sha (handles workflow_dispatch input or PR head) then gate (sparse checkout + run script). Same script, same exit codes, same EXPECTED_CHECKS env. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * ci: collapse merge-gate into a single job (one check-run in PR UI) The two-job split (resolve-sha + gate) created two visible check-runs. Inlining the SHA resolution as a step within the gate job leaves only one check-run -- 'Merge Gate / gate' -- on the PR. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Description
The two
is_genericerror branches ingithub_downloaderrender the bare host in the"For private repositories on {host}"hint, dropping any custom port. Users on Bitbucket Datacenter (or any self-hosted host on a non-default port) see a diagnostic that hides the very detail they need to verify their git credential helper against:instead of:
Both branches now route through
AuthResolver.classify_host(...).display_name, so the generic path shares port rendering with the adjacent ADO and auth branches (which already usedbuild_error_context→host_info.display_name). This also means any future refinement toHostInfo.display_name(e.g. default-port normalisation, tracked in #797) propagates to these two call sites for free.Scope-limited per the issue: no new signatures, no schema impact; only the two
host_name = dep_host or "..."lines change. The"the target host"fallback is kept for the (unreachable-today but defensive)dep_host=Nonecase, mirroring thedep_ref.port if dep_ref else Noneguard already used at the neighbouring auth call sites.Fixes #798
Type of change
Testing
New regression tests in
tests/unit/test_generic_host_error_port.pycover both error branches with three shapes each: ssh custom port (Bitbucket DC 7999), https custom port (Bitbucket DC 7990), and a no-port control that guards against spurious:suffixes. Temporarily reverting the fix turns the four port-bearing assertions red with the exact bare-host string described in the issue, then green again once the fix is reapplied -- the tests bite on the regression they're named after.Manual verification with
ssh://git@bitbucket.nonexistent-host.example:7999/team/repo.git:pytest tests/unit -q→ 4449 passed, 0 new failures. The three pre-existing failures intests/test_github_downloader.pyreproduce onorigin/mainwithout this patch and are unrelated.