fix(install): lowercase host in fallback-port-warned dedup key

[edenfunf]

Description

Follow-up A from the PR #789 review panel: the _fallback_port_warned dedup set keys on (dep_host, repo_url_base, dep_port), but DNS hostnames are case-insensitive per RFC 4343. Two deps that differ only in host casing should collapse to one cross-protocol fallback warning, not two.

Normalise the host axis on insertion, matching the host.lower() if host else host idiom already used by AuthResolver._cache at core/auth.py:224. This keeps cache-identity semantics uniform across the module.

The inline NOTE at the construction site was a placeholder for this follow-up and is removed with the fix.

Trigger surface note

While writing the regression test I noticed the issue's manifest-based repro is actually shielded by the URL parser: DependencyReference.parse lowercases the host on URL-string inputs, so two apm.yml entries that differ only in host casing collapse to one identity before they ever reach the dedup key. The casing leak only survives when a DependencyReference is built via the dataclass constructor (bypassing the parser), which is the surface the new test exercises by constructing its deps that way.

The fix is still worth landing as defensive alignment with the AuthResolver._cache convention -- and it becomes the only line of defence if any future refactor loosens the parser-side normalisation or adds a non-parser construction path.

Fixes #800

Type of change

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

Testing

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

Two regression tests in tests/unit/test_protocol_fallback_warning.py:

• test_warning_dedup_normalises_hostname_casing -- two deps with bitbucket.example.com / Bitbucket.Example.com, same port + repo, built via the dataclass constructor. Asserts exactly one [!] warning. Temporarily reverting the fix turns this red with got 2: naming both casings verbatim, then green once reapplied -- the test bites on the regression it is named after.
• test_host_normalisation_does_not_collapse_distinct_repos -- guards against over-normalisation: same host+port but different repo paths still warn twice even when host casing differs. Stays green in both directions -- it is a boundary test, not a regression test.

pytest tests/unit -q -> 4518 passed, zero new failures.

Manual verification with the same two-casings scenario via _clone_with_fallback in a REPL: without the fix, two [!] Custom port ... lines; with the fix, one.

[Copilot]

Pull request overview

Note

Copilot was unable to run its full agentic suite in this review.

This PR fixes protocol-fallback warning deduplication by normalizing dependency hostnames to lowercase (DNS hostnames are case-insensitive), and adds regression/boundary tests to prevent the warning from duplicating when host casing differs.

Changes:

• Lowercase dep_host when forming _fallback_port_warned dedup keys in the fallback warning path.
• Remove the old inline NOTE (placeholder) and codify the behavior via unit tests.
• Add a boundary test ensuring repo-path differences still produce distinct warnings.

Show a summary per file

File	Description
tests/unit/test_protocol_fallback_warning.py	Adds regression + boundary tests for host-casing normalization in fallback warning deduping.
src/apm_cli/deps/github_downloader.py	Normalizes host casing in _fallback_port_warned keys to prevent duplicate warnings.

Copilot's findings

• Files reviewed: 2/2 changed files
• Comments generated: 3

[github-actions]

APM Review Panel Verdict

Disposition: APPROVE (with one required action before merge)

---

Per-persona findings

Python Architect: Clean, minimal fix -- 5 lines of production code, 114 lines of tests. The ternary dep_host.lower() if dep_host else dep_host is idiomatic and handles None safely. Pattern alignment with AuthResolver._cache (core/auth.py:224) is correct: dedup-key semantics should be uniform across the module. The two-test design is well-reasoned: one test proves dedup fires on casing variants, the other guards against over-normalisation (same host+port, distinct repos still warn twice). No structural concerns. Minor observation: import shutil appears inline inside finally blocks in both new tests (lines 322 and 375) -- this is a pre-existing style pattern in the file, not introduced by this PR, so no action needed from the author.

CLI Logging Expert: No changes to output routing, CommandLogger lifecycle, or DiagnosticCollector. The fix correctly reduces duplicate [!] Custom port warnings when two deps share a host that differs only in casing. host_display at line 841 still uses the original (un-normalized) dep_host in the warning text -- this is correct: the first-seen casing is displayed, dedup prevents the second. Signal-to-noise improves. No concerns.

DevX UX Expert: No command surface, flag, or help-text changes. Users on Bitbucket Datacenter setups (the primary real-world trigger) will now see exactly one port-mismatch warning when the same server appears with mixed casing across deps -- less noise, same information. No docs update needed; this is internal dedup behavior not surfaced in the CLI reference. The PR description is thorough and explains the parser-path shield clearly -- if someone files a doc issue, the existing fallback-warning docs are accurate.

Supply Chain Security Expert: The normalization is applied ONLY to the dedup key used for warning emission. It has no effect on auth resolution (AuthResolver._cache normalizes independently), git clone URL construction, token scoping, or path safety. dep_host is still passed unnormalized into all security-sensitive paths. The repo_url_base axis of the dedup key is deliberately NOT normalized -- repo path case-sensitivity is a separate concern and outside this PR's scope. No new attack surface. Fails closed (a false miss on dedup means an extra warning, not a skipped security check). No concerns.

Auth Expert: The PR description correctly observes the alignment with AuthResolver._cache convention and explicitly calls out that the normalization is defensive -- the parser-side lowercasing in DependencyReference.parse already closes the gap for URL-string inputs. The fix defends the dataclass-constructor path (used internally and potentially by future refactors). Auth resolution is unaffected. No concerns.

OSS Growth Hacker: No conversion-surface impact. This PR is a positive signal: it demonstrates the review-panel loop working end-to-end (#789 panel raised it, #815 closes it). That story angle is worth a one-liner in the release note when 0.9.x ships ("Responsive follow-up to community review: ..."). WIP/growth-strategy.md does not need an update for a bug fix of this scope.

---

CEO arbitration

This is a textbook follow-up PR: small, well-tested, clearly motivated by a documented prior finding, and zero risk of regression. All seven personas are aligned -- no disagreements to arbitrate. The one gap is a missing CHANGELOG.md entry. Per project convention (Keep a Changelog, every merged PR that changes code must have an entry), a ### Fixed line is required under [Unreleased]. Everything else is merge-ready. Approve pending that addition.

---

Required actions before merge

1. Add CHANGELOG entry. Under ## [Unreleased], add a ### Fixed section (or append to an existing one) with a line like:

   - Lowercase the host axis of the `_fallback_port_warned` dedup key so deps that differ only in hostname casing collapse to one cross-protocol fallback warning, matching the `AuthResolver._cache` convention (RFC 4343). Fixes #800 (#815)
   

Optional follow-ups

• Move import shutil to the module-level imports in test_protocol_fallback_warning.py (pre-existing pattern, low priority, can be batched with a future housekeeping PR).
• Consider normalizing repo_url_base in the dedup key as a separate issue if case-insensitive repo resolution ever becomes a requirement.

Generated by PR Review Panel for issue #815 · ● 625.5K · ◷

[danielmeppiel]

Per agentic panel review