fix(auth): thread dep_ref.port into credential resolution (#785)

[edenfunf]

Description

Closes #785. Follow-up from the #665 review panel's auth-expert finding and the design discussion on the issue thread.

DependencyReference.port was parsed and stored by #665 but never reached the authentication path. Same-host multi-port setups (e.g. Bitbucket Datacenter with distinct PATs on 7990 and 7991) therefore collapsed onto a single credential entry via the AuthResolver cache, causing APM's pre-resolved token path (Method 1: authenticated HTTPS) to return the wrong credential.

The fix threads port through every layer that identifies a credential target: HostInfo, both in-process caches, git credential fill stdin, and user-facing error output.

What changed

• HostInfo gains a port: Optional[int] = None field and a display_name property ("host:port" if port else host). Classification stays transport-agnostic -- classify_host carries the port through but never uses it to decide kind.
• AuthResolver._cache key widens to (host.lower(), port, org.lower() if org else ""). port=None preserves the pre-fix key for the >99% common case; multi-port setups get per-port entries. Lock semantics unchanged.
• TokenManager._credential_cache is keyed by (host, port) tuple. Both caches widen together -- a half-widened cache would return the wrong cached credential at the collapsed layer.
• git credential fill receives host=host:port per gitcredentials(7). The credential protocol has no standalone port= attribute; sending one would be silently ignored by every helper. The _format_credential_host helper and an anti-pattern test guard against this regression.
• resolve_for_dep threads dep_ref.port; github_downloader, validation, and sources call sites forward the port argument.
• build_error_context uses host_info.display_name in the failure summary and, when a port is set, appends one [i] hint pointing users at credential helpers that key by hostname only.
• Docs -- docs/getting-started/authentication.md gains a per-helper port-awareness table under the "Git credential helper not found" section.
• CHANGELOG entry under [Unreleased] / Fixed.

Lockfile identity is unaffected -- DependencyReference.get_unique_key() still excludes port by design; the widened cache keys are purely in-process memoisation.

Type of change

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

Testing

• Tested locally
• All existing tests pass (4315 unit tests; 4 pre-existing failures unrelated to this change)
• Added tests for new functionality

New test coverage (23 cases)

• TestHostInfoPort -- field, display_name, classify_host port attachment + transport-agnostic invariant.
• TestResolvePortDiscrimination -- same host / different port → separate entries; same (host, port, org) hits cache; port=None vs port=<int> are distinct; resolve_for_dep threads port end-to-end for both ssh:// and https:// URLs; HostInfo carries port on resolve.
• TestBuildErrorContextWithPort -- display_name in error; [i] hint appears when port is set; no hint when port is absent.
• TestTryWithFallbackWithPort -- port flows into the credential-fill fallback path.
• TestCredentialFillPortEmbedding -- host=host:port stdin format; no standalone port= line (anti-pattern guard); backward-compat with bare host when port=None.
• TestGetTokenWithCredentialFallback gains same-host-different-port + same-host-same-port cache tests.

Manual end-to-end verification

Ran a scripted check covering: ssh/https port capture, resolve_for_dep propagation, cache-hit behaviour, git credential fill stdin format, TokenManager cache segmentation, and error-context hint presence/absence. All six scenarios pass.

Notes for reviewers

• Per the panel feedback, proposal item Add ARM64 Linux support to CI/CD pipeline #4 in the original issue (a standalone port= line in git credential fill stdin) is not implemented -- the port is embedded into the host field instead, per gitcredentials(7). An anti-pattern regression test asserts no \nport= ever appears in the stdin.
• classify_host accepts port as a kwarg and attaches it to the returned HostInfo. Classification logic itself remains transport-agnostic as per the architect's note; the kwarg is purely metadata.
• Port-unaware call sites elsewhere in the codebase (policy/discovery.py, marketplace/client.py, registry_proxy.py) are intentionally left alone -- they either do not parse a dep_ref (policy discovery) or only use host_info.kind/api_base for classification, where port is irrelevant.

Refs: #661, #665, #784

[edenfunf]

@danielmeppiel — ready for review when you have a cycle. Scope is the 9-item PR spec from your review-panel comment on #785 (standalone PR, both caches widened, host=host:port per gitcredentials(7), display_name used at the three call sites, [i] hint, docs table, CHANGELOG). Manual e2e + 23 new unit tests all green. Thanks!

[danielmeppiel]

APM Review Panel — PR #788

[+] Verdict: APPROVE — merge as-is, file two follow-ups

@edenfunf delivered a faithful 9-for-9 execution on the panel ask from #785. Four specialists (python-architect, auth-expert, cli-logging-expert, devx-ux-expert) converge on mergeable. The only "blocker" surfaced (architect flag on validation.py:326/335) was verified as a false positive — those line numbers fall in the parse-failure fallback path where port is genuinely unavailable and the assumed host is github.com (which doesn't use custom ports).

What's excellent

• Faithful scope discipline. All 9 panel asks landed; zero scope creep, zero drive-by refactors.
• Test quality. 23 behavior-targeted tests with the anti-pattern guard assert "\nport=" not in sent — exactly the right shape for a gitcredentials(7) protocol contract.
• Surface design. display_name as a @property with truthy-port guard keeps the happy path invisible and the error path materially better for Bitbucket DC users.

Findings

1. [i] Style nit — auth.py:309 (auth-expert). _try_credential_fallback uses closure host instead of host_info.host. Behaviorally identical today, drift risk tomorrow. Prefer host=host_info.host, port=host_info.port for symmetry with _resolve_token at line 462. Non-blocking — fix in this PR if trivial, else follow-up.
2. [!] Default-port normalization risk (devx-ux-expert). display_name truthy-checks self.port, so HostInfo(host="github.com", port=443) would render github.com:443 and incorrectly trigger the [i] hint. Confirm the dep_ref parser stores None for default-scheme ports (443/80/22), or normalize in HostInfo.__post_init__. Add a unit test pinning the contract. Follow-up issue — low likelihood in practice given current parser, but cheap to lock down.
3. [i] Pre-existing gap — github_downloader.py:808-811 (cli-logging-expert). is_generic error path renders bare host, losing port for Bitbucket DC users. Not introduced by this PR — file as follow-up.
4. [i] [i] hint actionability ~70% (devx-ux-expert). "Verify your credential helper stores per-port entries" names the suspect but not the next action. Follow-up — add a concrete git credential-manager get snippet or doc anchor in a polish pass.
5. [i] Cosmetic — cache-key None → "" (auth-expert). Behaviorally identical. One-line normalization comment would help future readers. Optional, non-blocking.

Merge call

Ship it. Open follow-ups for #2, #3, #4. @edenfunf — this is the standard for contributor work on a multi-part ask: read the full thread, execute every item, write the tests that prove the contract. Thank you for the precision. [+]

---

Growth angle

• Enterprise/Bitbucket-DC segment is self-organizing. @edenfunf's chain (review fix: preserve protocol (e.g. ssh:// or https://) and port in dependency URL #665 → PRs [BUG] SCP shorthand with numeric port silently misparsed as repo path #784/fix: detect port-like segment in SCP shorthand and suggest ssh:// form #787 → issue [FEATURE] Thread dep_ref.port into credential resolution (git credential fill) #785 → PR fix(auth): thread dep_ref.port into credential resolution (#785) #788) is the signature of a real on-prem user with sustained pain, not a drive-by. A repeat external contributor specifically hardening Bitbucket Data Center auth means APM is being adopted inside firewalled enterprises faster than our docs acknowledge — worth a dedicated "APM on Bitbucket DC" guide and probing whether a repeatable persona exists behind them.
• Story-shaped CHANGELOG line. "External contributor diagnosed and fixed an enterprise auth bug end-to-end — issue, fix, anti-regression tests, docs, self-credited" is exactly the narrative that recruits the next contributor. Frame it in release notes as "community-shipped" with @edenfunf credited in the headline, not just the trailer.
• Flag for next maintainer comms. Three+ merged PRs and a faithful 9-item panel execution clears the bar for a public shoutout (release notes highlight + contributor-spotlight post) and an internal nudge: invite to the contributors channel, consider triage permissions, watch for the fourth PR.

_{Reviewed via the apm-review-panel skill: python-architect, auth-expert, cli-logging-expert, devx-ux-expert; synthesized by apm-ceo with oss-growth-hacker side-channel.}

[Copilot]

Pull request overview

Threads DependencyReference.port through APM’s auth/credential resolution so same-host multi-port setups don’t collide in in-process caches or git credential fill lookups.

Changes:

• Add port to HostInfo, widen AuthResolver cache keys, and propagate port through resolve/fallback/error-context paths.
• Embed port into git credential fill as host=host:port and segregate GitHubTokenManager credential cache by (host, port).
• Add/adjust unit tests, update auth docs, and add a changelog entry.

Show a summary per file

File	Description
src/apm_cli/core/auth.py	Adds port-aware HostInfo, port-threading through auth resolution, cache key widening, and port-aware error messaging.
src/apm_cli/core/token_manager.py	Embeds port into git-credential host field and keys credential cache by (host, port).
src/apm_cli/deps/github_downloader.py	Threads dep_ref.port into auth error-context generation and GitHub file auth resolution.
src/apm_cli/install/validation.py	Threads dep_ref.port into auth classification, resolution, fallback, and verbose diagnostics.
src/apm_cli/install/sources.py	Threads dep_ref.port into auth resolution used for package auth logging.
tests/unit/test_auth.py	Adds coverage for port propagation, cache discrimination, error context display/hints, and fallback behavior.
tests/test_token_manager.py	Updates existing assertions and adds tests for (host, port) credential cache + credential-fill stdin format.
tests/test_github_downloader.py	Adjusts mocks for new resolve_credential_from_git(host, port=...) signature.
docs/src/content/docs/getting-started/authentication.md	Documents port-in-host behavior and helper port-awareness caveats.
CHANGELOG.md	Adds an Unreleased “Fixed” entry for port-discriminated token resolution.

Copilot's findings

• Files reviewed: 10/10 changed files
• Comments generated: 5

[danielmeppiel]

@edenfunf check Copilot Review and CodeQL scan alert and I will approve after that's done, thanks!

[edenfunf]

@danielmeppiel — pushed f5fdb07 addressing the review feedback. Summary of what changed and what was deliberately deferred:

In this push

• dani Why do we need a GitHub token? #1 (auth.py:309): _try_credential_fallback now reads host_info.host / host_info.port instead of the closure variables. Behaviourally identical today, but matches _resolve_token's symmetry and removes a future drift hazard.
• CodeQL alert (high): anchored the build_error_context substring assertion in tests/unit/test_auth.py with surrounding " on " / "." tokens. The previous bare <host:port> in <message> form was flagged by py/incomplete-url-substring-sanitization (false positive in test context, but the anchored form is also strictly more precise).
• Copilot Integrate copilot runtime #2 / Will there be MCP coverage? #3: tightened HostInfo.display_name and _format_credential_host to use port is not None instead of truthy checks. None is the sentinel everywhere else (build_error_context already used this form), so this removes the only inconsistency. Functionally equivalent for valid TCP ports; tightens the contract against any future port=0 misuse.
• Copilot Add ARM64 Linux support to CI/CD pipeline #4: mirrored the new "Custom-port hosts and per-port credentials" section into packages/apm-guide/.apm/skills/apm-usage/authentication.md so the in-repo APM usage skill stays consistent with the public docs.

Replies to remaining review threads

• Copilot Why do we need a GitHub token? #1 (cache key org=None → ""): matches the panel spec verbatim (org.lower() if org else ""). Behaviourally equivalent — no caller in the codebase passes org="", so the only None/"" distinction is internal to the resolver. PR description language could be tighter; I'll fold that wording into a CHANGELOG/PR pass before merge if you'd like, otherwise non-blocking.
• dani Integrate copilot runtime #2 (default-port 443/80/22 normalisation): leaving for a follow-up issue per your "follow-up — low likelihood in practice but cheap to lock down" guidance. The proper fix lives at the parser or HostInfo.__post_init__ layer (truthy → is not None alone doesn't address port=443), and should ship with a contract test pinning the parser behaviour. Will open the issue and link from this PR.
• dani Will there be MCP coverage? #3 (pre-existing is_generic error path at github_downloader.py:808-811 losing port): confirmed pre-existing — separate follow-up issue.
• dani Add ARM64 Linux support to CI/CD pipeline #4 ([i] hint actionability — concrete git credential-manager get snippet): separate follow-up polish, will batch with a docs pass.

Verification

• tests/unit/test_auth.py tests/unit/test_auth_scoping.py tests/test_token_manager.py tests/integration/test_auth_resolver.py — 159 passed (1 pre-existing Windows-only GIT_ASKPASS skip).
• Broader sweep: 4316 unit + auth tests pass with no new regressions.
• Manual e2e (six scenarios from the original implementation) re-verified after the changes.

[edenfunf]

Follow-up issue for dani #2 (default-port normalisation): #797. Will batch dani #3 (is_generic error path port loss) and #4 ([i] hint actionability) into separate issues if they are not already filed.

[danielmeppiel]

APM Review Panel -- PR #788 (re-review)

[+] APPROVE. All in-PR items from my prior approve-with-fixes are addressed in f5fdb07; deferred items have follow-up issues.

Verified in f5fdb07

• dani #1 (auth.py:309): _try_credential_fallback now reads host_info.host / host_info.port, matching _resolve_token's symmetry. Drift hazard removed.
• CodeQL high alert: build_error_context substring assertion anchored with " on " / "." tokens. Strictly more precise than the bare form, beyond just silencing the false positive.
• Copilot #2/#3: HostInfo.display_name and _format_credential_host switched to port is not None -- matches the sentinel convention used everywhere else and tightens the contract against any future port=0 misuse.
• Copilot #4: packages/apm-guide/.apm/skills/apm-usage/authentication.md mirrored with the new "Custom-port hosts and per-port credentials" section, keeping the in-repo APM usage skill in sync with public docs (per the doc-sync rule in cli.instructions.md).

Follow-ups filed

• #797 -- default-port (443/80/22) normalization in parser/HostInfo.__post_init__ + contract test (dani #2).
• Pending issues for is_generic error path port loss (dani #3) and [i] hint actionability snippet (dani #4) -- batched for a docs polish pass.

Accepted as-is

• Copilot #1 (cache-key org=None -> ""): matches panel spec verbatim; no caller passes org="". Behaviourally identical, internal to the resolver.

Nice work, @edenfunf -- this is the cleanest 9-for-9 panel execution we've seen, with contract tests and an anti-pattern guard for gitcredentials(7). Thanks for the precision.

_{Re-reviewed via apm-review-panel skill: prior approve-with-fixes items verified individually against f5fdb07 diff.}