[FOLLOW-UP #788] Normalise default-scheme ports (443/80/22) on DependencyReference / HostInfo

[edenfunf]

Follow-up from the #788 review panel — devx-ux-expert finding #2.

Problem

`HostInfo.display_name` (and the corresponding `[i]` hint in `build_error_context`) renders `host:port` whenever `port is not None`. But the parser at `DependencyReference.parse` happily stores the explicit port from URLs like `https://github.com:443/owner/repo\` or `ssh://git@gitlab.com:22/owner/repo.git` — both are default-scheme ports that carry no semantic difference from the bare URL.

Result: a user who writes `https://github.com:443/owner/repo\` would see error text saying `Authentication failed for clone on github.com:443.` and an `[i] Host 'github.com:443' -- verify your credential helper stores per-port entries` hint that is irrelevant for that user (github.com on port 443 is the default).

The truthy → `is not None` change in #788 did not address this — `port=443` is non-`None`, so both forms render the same way.

Proposed fix

Two equally valid layers:

Option A — normalise at parse time (`DependencyReference.parse` / `_parse_ssh_protocol_url` / `_parse_standard_url`):

```python
DEFAULT_PORTS = {"https": 443, "http": 80, "ssh": 22}
if port == DEFAULT_PORTS.get(scheme):
port = None
```

Option B — normalise at `HostInfo.post_init`:

A frozen-dataclass-friendly equivalent of A, scoped to whichever scheme `HostInfo` is constructed for. Slightly trickier because `HostInfo` doesn't currently know the scheme.

Option A is more direct (the port comes off the URL, the URL has a scheme right there) and keeps `HostInfo` scheme-agnostic. Recommend A.

Required tests

A unit test pinning the contract for each transport:

```python
def test_default_https_port_normalised_to_none():
dep = DependencyReference.parse("https://github.com:443/owner/repo\")
assert dep.port is None

def test_default_ssh_port_normalised_to_none():
dep = DependencyReference.parse("ssh://git@gitlab.com:22/owner/repo.git")
assert dep.port is None

def test_non_default_port_preserved():
dep = DependencyReference.parse("https://bitbucket.corp.com:7990/team/repo\")
assert dep.port == 7990
```

And the existing `test_https_url_with_port_capture` style tests stay green.

Refs

• Follow-up to [FEATURE] Thread dep_ref.port into credential resolution (git credential fill) #785 / fix(auth): thread dep_ref.port into credential resolution (#785) #788 (auth port threading).
• Panel review: fix(auth): thread dep_ref.port into credential resolution (#785) #788 review by @danielmeppiel — "[!] Default-port normalization risk".

[edenfunf]

@danielmeppiel -- picking a direction before I queue this PR, since I flagged two options in the body and wanted to sanity-check my lean with you.

I was tentatively preferring Option A (normalise at DependencyReference.parse) for one specific reason beyond what the body captures: HostInfo doesn't currently know the URL scheme -- it only knows host + kind -- so doing the normalisation in HostInfo.__post_init__ (Option B) would either require threading scheme through classify_host, or baking a host-kind-to-default-port map into HostInfo. The #788 architect note asked to keep classify_host transport-agnostic, which nudges me away from B.

Option A concretely, at the end of DependencyReference.parse() after port assignment:

DEFAULT_PORTS = {"ssh": 22, "http": 80, "https": 443}
if port is not None and port == DEFAULT_PORTS.get(explicit_scheme):
    port = None

One location, one constant; every parse path (_parse_ssh_protocol_url, _parse_standard_url, future) aggregates into parse(), so this catches them all uniformly. Downstream gets a clean contract: port is not None always means "non-default port."

Tests I was planning: :443 → None, :22 → None, :7990 preserved, plus one to_github_url() round-trip to confirm dropping explicit :443 doesn't break URL construction (most callers re-add port only when non-None, so this should be fine, but worth pinning).

Does that read right to you, or is there a reason to prefer B (or a third shape I haven't considered)? Happy to re-think -- you'll have better context on whether anyone downstream actually depends on explicit-default ports round-tripping.

[github-actions]

Triage decision

accept

Proposed labels

theme/security
area/lockfile
area/cli
type/bug
status/accepted
priority/high

Milestone

0.9.4

Suggested next action

Add a _normalize_port(scheme, port) utility in DependencyReference.parse() that strips default ports (443 for https, 80 for http, 22 for ssh/git), and include a lockfile migration note for any existing entry that stored an explicit default port.

Suggested issue comment

Thanks for the follow-up -- this is a genuine correctness bug with a latent lockfile identity implication.

The fix is clear: normalize default-scheme ports to `None` at parse time in `DependencyReference.parse()`. After normalization, `https://github.com:443/owner/repo` and `https://github.com/owner/repo` produce identical `HostInfo` and identical lockfile keys, which is the correct behavior.

Ports to normalize:
- `https` -> strip port 443
- `http`  -> strip port 80
- `ssh` / `git+ssh` / `git` -> strip port 22

A small note on migration: any `apm.lock.yaml` that was written with an explicit default port in the key (e.g. `github.com:443/owner/repo`) will silently become stale after the fix, since re-resolving the same dep will now produce a key without the port. The migration path is to treat the old and new key as aliases during lock-read and rewrite the lock on the next `apm install`. Please document this in the PR description and CHANGELOG.

`HostInfo.display_name` should also be updated to never render a default port in the `[i]` hint, consistent with the parse-time normalization.

Per-lens notes (collapsed)

DevX UX Expert -- User-Need Reviewer

The user mental model (cargo, npm, pip) is that default ports are transparent -- https://github.com:443 is indistinguishable from https://github.com. Currently APM preserves the explicit port, causing misleading display (host:443 in error messages) and inconsistent lockfile keys. The fix (normalize at parse time) aligns the behavior with every other dependency manager the user is familiar with.

Supply Chain Security Expert -- Risk-Surface Reviewer

This is a supply-chain-adjacent lockfile correctness issue. If https://github.com:443/owner/repo and https://github.com/owner/repo produce different lockfile keys, a dependency declared one way may not match the lock entry for the other, causing silent re-resolution or, in an adversarial scenario, substitution with a different host's package that shares the un-normalized key. theme/security and area/lockfile are required. The lockfile migration concern (old keys with explicit ports) must be handled to avoid silent stale-lock situations.

OSS Growth Hacker -- Contributor-Tone Reviewer

Not activated -- edenfunf is a returning contributor (CONTRIBUTOR association) with meaningful prior project interactions; warmth tuning for a first-time contributor is not needed.

Python Architect -- Architecture Reviewer

The change is localized to DependencyReference.parse() (add _normalize_port()) and HostInfo.display_name (skip rendering default ports). The migration concern is the main risk: existing lockfiles with explicit default ports will drift. A backward-compat alias lookup during lock-read (try old key -> try new key) plus a rewrite-on-install strategy is the standard APM pattern. status/accepted is appropriate -- the design is fully specified.

Doc Writer -- Documentation Reviewer

Not activated -- the fix is a transparent behavior correction with no user-facing documentation change needed beyond a CHANGELOG entry and a migration note in the PR description.

APM CEO -- Triage Arbiter

Lockfile identity correctness with a supply-chain-adjacent angle. Accept at priority/high for 0.9.4: the implicit lockfile key collision is a real risk for anyone using explicit default ports in dependency URLs, and the fix is well-specified. The migration concern must be explicitly addressed in the PR.

{
  "decision": "accept",
  "decision_detail": "",
  "theme": "theme/security",
  "areas": ["area/lockfile", "area/cli"],
  "type": "type/bug",
  "status": "status/accepted",
  "priority": "priority/high",
  "preserved_labels": [],
  "milestone": "0.9.4",
  "next_action": "Add _normalize_port() to DependencyReference.parse() for default scheme ports (443/80/22) and handle lockfile migration for existing entries with explicit default ports.",
  "comment_markdown": "Thanks for the follow-up -- this is a genuine correctness bug with a latent lockfile identity implication.\n\nThe fix is clear: normalize default-scheme ports to `None` at parse time in `DependencyReference.parse()`. After normalization, `https://github.com:443/owner/repo` and `https://github.com/owner/repo` produce identical `HostInfo` and identical lockfile keys, which is the correct behavior.\n\nPorts to normalize:\n- `https` -> strip port 443\n- `http`  -> strip port 80\n- `ssh` / `git+ssh` / `git` -> strip port 22\n\nA small note on migration: any `apm.lock.yaml` that was written with an explicit default port in the key (e.g. `github.com:443/owner/repo`) will silently become stale after the fix, since re-resolving the same dep will now produce a key without the port. The migration path is to treat the old and new key as aliases during lock-read and rewrite the lock on the next `apm install`. Please document this in the PR description and CHANGELOG.\n\n`HostInfo.display_name` should also be updated to never render a default port in the `[i]` hint, consistent with the parse-time normalization."
}

---

Triage status: agentic proposal pending human ratification.
Silence is approval. Maintainers can:

• Override any label or milestone above by editing it directly --
  human edits are authoritative and will not be reverted on
  subsequent runs.
• Re-trigger triage by applying the status/needs-triage label, or
  by removing status/triaged to enroll the issue in the next
  daily sweep.

Posted by the Triage Panel workflow. See .apm/skills/apm-triage-panel for the panel skill.

Generated by Triage Panel · ● 2.3M · ◷