fix: normalise default-scheme ports (443/80/22) on DependencyReference / HostInfo

[Sergio Sisternes (sergio-sisternes-epam)]

Description

Normalise default-scheme ports (443 for HTTPS, 80 for HTTP, 22 for SSH) to None at parse time in DependencyReference.parse(), so that https://github.com:443/owner/repo and https://github.com/owner/repo produce identical HostInfo and lockfile keys.

Also updates HostInfo.display_name to never render a default port in the [i] hint.

Fixes#797

Type of change

• Bug fix
• New feature
• Documentation
• Maintenance / refactor

Testing

• Tested locally
• All existing tests pass
• Added tests for new functionality (if applicable)

[Copilot]

Pull request overview

Normalizes default ports during dependency parsing so URLs like https://github.com:443/... and https://github.com/... produce identical DependencyReference/lockfile identity, and adjusts host display rendering to avoid showing redundant default ports in user-facing output.

Changes:

• Normalize scheme-default ports to None during parsing (https=443, http=80, ssh=22) in DependencyReference URL parsing.
• Update HostInfo.display_name to suppress rendering of default ports.
• Add unit tests covering default-port normalization and display_name behavior.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.

File	Description
src/apm_cli/models/dependency/reference.py	Normalizes default-scheme ports to None during URL parsing to stabilize canonical identity/lockfile keys.
src/apm_cli/core/auth.py	Updates HostInfo.display_name logic to avoid displaying default ports in user-facing text.
tests/unit/test_generic_git_urls.py	Adds tests verifying default-port normalization across HTTPS/HTTP/SSH and canonicalization consistency.
tests/unit/test_auth.py	Adds tests asserting HostInfo.display_name suppression of default ports.

[github-actions]

APM Review Panel: ship_with_followups

Port normalisation fix (443/80/22->None at parse time) eliminates lockfile drift for enterprise users with explicit-port URLs; auth-safe, RFC 3986 correct, two recommended follow-ups before next release.

cc Sergio Sisternes (@sergio-sisternes-epam)@danielmeppiel -- a fresh advisory pass is ready for your review.

All seven active panelists converge: this fix is correct, auth-safe, and carries no supply-chain risk. The normalisation happens at the right architectural layer (DependencyReference.parse()), auth token selection is unaffected (no path keys on port), and the lockfile deduplication improvement is the intended outcome. The RFC 3986 rationale is sound and the fix aligns with prior port-related work (#815, #665).

Two findings are recommended-tier and should be tracked as follow-ups before the next release. First, the missing CHANGELOG entry is flagged independently by both oss-growth-hacker and doc-writer: this is a user-visible lockfile behaviour change that enterprise evaluators will care about, and the pattern of covering port-related fixes in the changelog is already established. Second, the python-architect identifies a defence-in-depth gap: callers that construct HostInfo(port=443) directly bypass parse-time normalisation, and the port is not None branch in auth.py:648 will still fire a spurious credential-helper warning for those callers. Adding HostInfo.__post_init__ self-normalisation closes this gap cleanly and is the highest-value structural change in this PR's wake.

The highest-frequency nit across the panel (4 panelists: cli-logging, devx-ux, auth, python-architect) is the inline _well_known_default_ports = {443, 80, 22} set literal in display_name. This should be hoisted to a module-level frozenset and derived from _DEFAULT_SCHEME_PORTS.values() to guarantee the two sets cannot silently diverge.

Dissent. No substantive disagreement between panelists. The supply-chain-expert raises a cosmetic display-layer concern (scheme-agnostic port suppression could silently drop :22 from an HTTPS-on-22 display string), but explicitly confirms no auth bypass and no real-world supply-chain risk.

Aligned with: Portable by Manifest -- apm.yml files with explicit default ports produce identical lockfile keys to bare-host equivalents, enabling reproducible installs across environments. Secure by Default -- auth token selection is unaffected; cache deduplication improves; spurious credential-helper warnings for default ports are correctly suppressed. Multi-Harness / Multi-Host -- HostInfo.display_name correctly suppresses default ports while retaining non-default port rendering for GHE and custom-port endpoints.

Growth signal. Enterprise proxy and firewall environments frequently inject explicit ports (443, 80) into URLs via tooling -- this fix silently removes a real friction point that causes lockfile churn and confuses reproducibility-conscious teams. Grouping this with #815 and #788 under an 'enterprise network hardening' beat in the next release post would land well with the enterprise evaluator segment.

Panel summary

Persona	B	R	N	Takeaway
Python Architect	0	2	1	Correct fix at the right layer; one structural gap (port-is-not-None guards in AuthResolver/diagnostic paths not covered by defence-in-depth) and one maintainability smell (two independent port-set literals that can drift).
CLI Logging Expert	0	1	1	Suppressing default ports in display_name is the right UX call; no CLI output regressions found.
DevX UX Expert	0	0	2	Good friction-reducer: copy-pasted URLs with explicit default ports now work identically to bare-host URLs, making lockfile keys and error messages consistent -- no DevX regressions found.
Supply Chain Security Expert	0	0	1	Port normalisation is semantically correct per RFC 3986 and introduces no supply-chain or auth-bypass risk; one cosmetic display-layer scheme/port mismatch worth noting.
OSS Growth Hacker	0	1	0	Silent lockfile churn on explicit default ports is a real enterprise friction point -- this fix helps adoption, but no CHANGELOG entry means no one will know it shipped.
Auth Expert	0	0	2	Port normalisation is auth-safe: no token-selection path keys on port, cache deduplication improves, and the spurious credential-helper warning for default ports is correctly suppressed.
Doc Writer	0	1	1	CHANGELOG has no entry for this fix; dependencies.md is consistent but could explicitly state the default-port normalisation behaviour.
Test Coverage Expert	0	1	1	Unit tests for parse-time port normalisation are solid; lockfile-key equivalence for :443 vs bare URL lacks an integration-tier regression trap, and (redacted) port 9418 is untested in DependencyReference.

B = blocking-severity findings, R = recommended, N = nits.
Counts are signal strength, not gates. The maintainer ships.

Top 5 follow-ups

1. [Test Coverage Expert] Add integration test: https://github.com:443/owner/repo and https://github.com/owner/repo produce identical lockfile entries -- Evidence-backed missing test (outcome: missing) on a portability-by-manifest surface. The PR's core claim is only defended at unit level; the lockfile YAML round-trip path is unexercised. This is the strongest gap -- a missing automated guardrail on the precise promise this PR is making.
2. [OSS Growth Hacker] Add CHANGELOG entry to [Unreleased] Fixed section for default-port normalisation -- User-visible lockfile behaviour change; prior port-related fixes (fix(install): lowercase host in fallback-port-warned dedup key #815, fix: preserve protocol (e.g. ssh:// or https://) and port in dependency URL #665) set the expectation. Two panelists flagged this independently.
3. [Python Architect] Add HostInfo.__post_init__ to self-normalise port=443/80/22 to None regardless of call site -- Closes the defence-in-depth gap: callers that construct HostInfo directly bypass parse-time normalisation and will still trigger the spurious credential-helper warning at auth.py:648. Makes the invariant hold at the data-class boundary.
4. [Python Architect] Extract shared _DEFAULT_PORTS frozenset; derive display_name's port-suppression set from it -- Two independent port-set literals encoding the same invariant will silently diverge when a new scheme is added. Four panelists flagged the inline set; the structural fix (single source of truth) subsumes the nit.
5. [CLI Logging Expert] Update display_name docstring to explicitly state custom non-default ports are still rendered -- Auth-chain log lines rely on display_name to distinguish GHE/custom-port targets; the updated docstring drops the rationale for preserving non-default ports.

Architecture

classDiagram
direction LR
class HostInfo {
<<ValueObject>>
+host: str
+kind: str
+port: int | None
+has_public_repos: bool
+api_base: str
+display_name: str
}
class DependencyReference {
<<ValueObject>>
+host: str
+port: int | None
+repo_url: str
+parse(s) DependencyReference$
+to_canonical() str
-_parse_ssh_protocol_url(url) tuple$
-_parse_standard_url(url) tuple$
}
class AuthResolver {
<<Strategy>>
+resolve(host, org, port) AuthContext
+resolve_for_dep(dep) AuthContext
+classify_host(host, port) HostInfo
}
class AuthContext {
<<ValueObject>>
+token: str | None
+source: str
+host_info: HostInfo
+git_env: dict
}
DependencyReference ..> AuthResolver : feeds via resolve_for_dep
AuthResolver ..> HostInfo : constructs and reads port
AuthContext *-- HostInfo : contains
class DependencyReference:::touched
class HostInfo:::touched
classDef touched fill:#fff3b0,stroke:#d47600

Loading

flowchart TD
A(["User: apm install https://github.com:443/owner/repo"]) --> B
B["DependencyReference.parse()"] --> C
C{"(redacted) URL?"} -->|yes| D
C -->|no| F
D["_parse_ssh_protocol_url()\nif port == _DEFAULT_SCHEME_PORTS.get('ssh'): port = None"]
D --> G
F["_parse_standard_url()\nscheme = parsed.scheme\nif port == _DEFAULT_SCHEME_PORTS.get(scheme): port = None"]
F --> G
G["DependencyReference\nport = None (normalised)"] --> H
H["AuthResolver.resolve_for_dep(dep)\npasses dep.port=None to resolve()"] --> I
I["AuthResolver.classify_host(host, port=None)\nHostInfo(port=None)"] --> J
J["host_info.port is not None?\nauth.py:648"] -->|False -- no spurious warning| K
K["HostInfo.display_name\nport not in well_known -- returns bare host"] --> L(["Output: github.com"])
style D fill:#fff3b0,stroke:#d47600
style F fill:#fff3b0,stroke:#d47600
style K fill:#fff3b0,stroke:#d47600

Loading

Recommendation

Ship this PR. The fix is correct, auth-safe, RFC 3986 compliant, and addresses a real enterprise pain point. No panelist found a blocking issue. The two recommended-tier gaps (CHANGELOG entry, integration regression trap) are lightweight and can land as a fast-follow in the same release cycle -- the CHANGELOG entry in particular should be added before the next tag. The HostInfo self-normalisation and shared port-set consolidation are good hygiene items that belong in a follow-up PR, not a merge gate.

---

Full per-persona findings

Python Architect

• [recommended] Two independent port-set literals can silently drift at src/apm_cli/core/auth.py:76
reference.py defines _DEFAULT_SCHEME_PORTS and auth.py:display_name defines inline _well_known_default_ports = {443, 80, 22}. They encode the same invariant in two different modules with no shared source of truth. If a future scheme (e.g. (redacted) port 9418) is added to one set, the other will silently diverge. *Suggested:* Extract _DEFAULT_PORTS: frozenset[int] = frozenset({443, 80, 22})at module level inauth.py(or sharedcore/ports.py), then import and use it in both reference.pyanddisplay_name`.

• [recommended]HostInfo.port is not None guards in AuthResolver diagnostic path not covered by defence-in-depth at src/apm_cli/core/auth.py:648
auth.py:648 fires a user-visible credential-helper warning whenever host_info.port is not None. Callers that construct HostInfo(port=443, ...) directly bypass parse-time normalisation and will still trigger a spurious warning for a standard HTTPS host.
  Suggested: Add __post_init__ to HostInfo that normalises port to None when it is a known default port. Makes HostInfo a self-normalising value object.

• [nit]_well_known_default_ports is reallocated on every display_name access at src/apm_cli/core/auth.py:76
  Set literal {443, 80, 22} inside display_name is constructed on each property invocation.
  Suggested: Move to module level: _WELL_KNOWN_PORTS: frozenset[int] = frozenset({443, 80, 22}).

CLI Logging Expert

• [recommended] Docstring no longer states that custom non-default ports are still rendered at src/apm_cli/core/auth.py:75
  The updated docstring drops the rationale that 'port differentiates the target'. For enterprise GHE on custom ports (e.g. :8443, :2222), the old behaviour is preserved but no longer explicit. Auth-chain log lines rely on display_name to distinguish targets.
  Suggested: Add one sentence: 'Custom non-default ports (e.g. :8443) are still rendered to distinguish targets.'

• [nit] Inline _well_known_default_ports set should be hoisted to module-level constant at src/apm_cli/core/auth.py:68
  The set is reconstructed on every call to display_name.

DevX UX Expert

• [nit] Inline constant allocation on every display_name access at src/apm_cli/core/auth.py:69
_well_known_default_ports = {443, 80, 22} is re-created on every call.
  Suggested: Add _WELL_KNOWN_DEFAULT_PORTS: frozenset[int] = frozenset({443, 80, 22}) at module level; derive from _DEFAULT_SCHEME_PORTS.values() to keep in sync.

• [nit]display_name docstring has implementation-internal language at src/apm_cli/core/auth.py:69
  'defence-in-depth against callers that construct a HostInfo without prior normalisation' is implementation-internal and noisy.
  Suggested: Trim to: 'Returns host:port for non-default ports, bare host otherwise. Well-known ports (443, 80, 22) are always suppressed.'

Supply Chain Security Expert

• [nit]display_name port-suppression set is scheme-agnostic; unusual HTTPS-on-22 endpoint would silently drop port from display at src/apm_cli/core/auth.py:78
  The hardcoded set {443, 80, 22} is not conditioned on URL scheme. If a caller constructs HostInfo for an HTTPS-on-port-22 endpoint, display_name silently drops :22. No auth bypass -- only a cosmetic discrepancy between what the user sees in log output and the port actually dialled.
  Suggested: Add an inline comment acknowledging this limitation, or extend HostInfo with an optional scheme field.

OSS Growth Hacker

• [recommended] No CHANGELOG entry for this fix at CHANGELOG.md
  User-visible lockfile behaviour change that enterprise evaluators will care about. Prior port-related fixes (fix(install): lowercase host in fallback-port-warned dedup key #815, fix: preserve protocol (e.g. ssh:// or https://) and port in dependency URL #665) set the expectation. Without a CHANGELOG entry it ships invisibly.
  Suggested: Add to [Unreleased] Fixed: 'Default-scheme ports (443 for HTTPS, 80 for HTTP, 22 for SSH) are now normalised to None at parse time, so https://github.com:443/owner/repo and https://github.com/owner/repo produce identical lockfile keys. Closes [FOLLOW-UP #788] Normalise default-scheme ports (443/80/22) on DependencyReference / HostInfo #797 (fix: normalise default-scheme ports (443/80/22) on DependencyReference / HostInfo #1237)'

Auth Expert

• [nit]_well_known_default_ports rebuilt on every display_name call at src/apm_cli/core/auth.py:78
  Set literal inside property body is allocated and GC'd on every call.
  Suggested: Add _WELL_KNOWN_DEFAULT_PORTS: frozenset[int] = frozenset({443, 80, 22}) at module level.

• [nit]HostInfo.port field comment says 'Non-standard' but semantics now include 'None for default ports' at src/apm_cli/core/auth.py:65
  The field comment reads 'Non-standard git port (e.g. 7999 for Bitbucket DC)'. After this PR, callers that previously stored 443 explicitly will now store None.
  Suggested: Update inline comment: 'Explicit non-default port; None for standard scheme ports (443/80/22) or when port is unspecified.'

Doc Writer

• [recommended] Missing CHANGELOG entry for default-port normalisation fix at CHANGELOG.md
  The [Unreleased] Fixed section has no entry for this user-visible behaviour change affecting lockfile reproducibility.
  Suggested: Add to ## [Unreleased] / ### Fixed: 'Default-scheme ports (443 for HTTPS, 80 for HTTP, 22 for SSH) are now stripped at parse time in DependencyReference.parse(), so https://github.com:443/owner/repo and https://github.com/owner/repo produce the same lockfile key. Closes [FOLLOW-UP #788] Normalise default-scheme ports (443/80/22) on DependencyReference / HostInfo #797'

• [nit]dependencies.md implies but does not state the default-port normalisation rule at packages/apm-guide/.apm/skills/apm-usage/dependencies.md
  The text says 'Non-default git ports are preserved' but does not explicitly cover normalisation of user-spelled explicit default ports.
  Suggested: Append: 'Default ports (443 for HTTPS, 80 for HTTP, 22 for SSH) are stripped at parse time -- https://github.com:443/owner/repo and https://github.com/owner/repo are identical in the lockfile.'

Test Coverage Expert

• [recommended] No integration test asserts that :443 and bare URL produce identical lockfile entries at tests/integration/test_generic_git_url_install.py
  The PR's core claim (lockfile key consistency) is only defended at unit level. The lockfile YAML round-trip path is unexercised. Probed: grep'd tests/integration/ for '443', 'port.*lock', 'canonical.*443' -- no match.
  Proof (missing at integration-with-fixtures):tests/integration/test_generic_git_url_install.py::test_port_443_url_produces_same_lockfile_entry_as_bare_url -- proves: A user who writes https://github.com:443/owner/repo in apm.yml gets an identical lockfile entry to one who writes https://github.com/owner/repo. [portability-by-manifest, devx]

• [nit](redacted) port 9418 normalisation not tested via DependencyReference.parse()attests/unit/test_generic_git_urls.pyurl_normalize.pylists 9418 as default (redacted) port but the new normalisation table inreference.pyomits 'git'. Probed: grep -r '9418' tests/ -- zero hits. Low real-world impact. *Proof (missing at unit):*tests/unit/test_generic_git_urls.py::test_default_git_port_9418_normalised_to_none` -- proves: A (redacted) URL with the default port 9418 is normalised identically to the bare (redacted) URL. [portability-by-manifest]

_{This panel is advisory. It does not block merge. Re-apply the
panel-review label after addressing feedback to re-run.}

Generated by PR Review Panel for issue #1237 · ● 2M · ◷