fix: preserve protocol (e.g. ssh:// or https://) and port in dependency URL

[edenfunf]

Fixes #661.

Root Cause

When APM parses a dependency URL that starts with ssh://, _normalize_ssh_protocol_url converts it to the git@host:path format and silently drops any custom port:

ssh://git@bitbucket.domain.ext:7999/project/repo.git
→ git@bitbucket.domain.ext:project/repo.git   (port 7999 gone)

_clone_with_fallback then tries SSH on the reconstructed URL (default port 22, wrong) which fails, and falls back to:

https://bitbucket.domain.ext/project/repo   ← not what the user asked for

This affects Bitbucket Datacenter and any other self-hosted git server that uses a non-standard SSH port.

Fix

Add an original_ssh_url field to DependencyReference. When parse_from_str encounters a URL that begins with ssh://, the verbatim string is stored in this field before the normalisation step.

In _clone_with_fallback, Method 2 (SSH) now uses dep_ref.original_ssh_url directly when it is set, instead of rebuilding the URL from host + repo_ref (which loses the port):

if dep_ref and dep_ref.original_ssh_url:
    ssh_url = dep_ref.original_ssh_url      # exact user-supplied URL
else:
    ssh_url = self._build_repo_url(...)     # existing behaviour

Before

ssh://git@bitbucket.domain.ext:7999/project/repo.git
→ git clone ... https://bitbucket.domain.ext/project/repo   ❌

After

ssh://git@bitbucket.domain.ext:7999/project/repo.git
→ git clone ... ssh://git@bitbucket.domain.ext:7999/project/repo.git   ✅

Changes

File	Change
src/apm_cli/models/dependency/reference.py	Add original_ssh_url dataclass field; capture it in parse_from_str before normalisation; pass to constructor
src/apm_cli/deps/github_downloader.py	Use dep_ref.original_ssh_url verbatim in SSH clone attempt when present
tests/unit/test_generic_git_urls.py	New TestBitbucketDatacenterSSH class with 5 regression tests

Regression Protection

Added TestBitbucketDatacenterSSH covering:

• ssh:// with custom port stores the original URL in original_ssh_url
• Host and repo_url fields are still correctly parsed
• ssh:// without a port also preserves the original URL
• HTTPS URLs do not set original_ssh_url
• git@ shorthand URLs do not set original_ssh_url

All 3 763 existing unit tests continue to pass.

[sergio-sisternes-epam]

Clean, focused fix — the "preserve original URL, don't fix normalization" approach is the right call. Minimal risk, no side effects on existing URL handling. The PR description is excellent with clear root cause analysis and before/after examples.

The test coverage is thorough: custom port, standard ssh, https exclusion, git@ exclusion, and clone URL ordering all covered.

One request before merge: please add a CHANGELOG entry under ## [Unreleased]:

### Fixed
- Preserve `ssh://` dependency URLs with custom ports for Bitbucket Datacenter repositories instead of silently falling back to HTTPS (#661)

Thanks for another solid contribution!

[Copilot]

Pull request overview

This PR fixes cloning failures for self-hosted Git servers (notably Bitbucket Datacenter) when dependencies are specified with an explicit ssh://...:PORT/... URL by preserving the original SSH URL and using it for the SSH clone attempt.

Changes:

• Add DependencyReference.original_ssh_url and populate it when parsing ssh:// dependency strings.
• Update _clone_with_fallback to prefer original_ssh_url for the SSH clone method.
• Add regression tests covering parsing and clone URL selection for ssh:// URLs with custom ports.

Show a summary per file

File	Description
src/apm_cli/models/dependency/reference.py	Adds original_ssh_url and captures it during parsing for ssh:// inputs.
src/apm_cli/deps/github_downloader.py	Uses original_ssh_url directly for the SSH clone attempt to preserve custom ports.
tests/unit/test_generic_git_urls.py	Adds parsing regression tests for Bitbucket Datacenter-style ssh:// URLs with ports.
tests/unit/test_auth_scoping.py	Adds downloader regression tests to assert the SSH clone attempt uses the original ssh:// URL verbatim.

Copilot's findings

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

[sergio-sisternes-epam]

@edenfunf looks like we got a conflict now. We will need that solved before we can merge the PR. Can you review this please?

[edenfunf]

@sergio-sisternes-epam Conflict has been resolved — merged the latest main into the branch. Ready for review.

[danielmeppiel]

@edenfunf the lockfile needs to track provenance URL with the port too in this case. We need to test this also works.

[edenfunf]

@danielmeppiel Thanks for catching this — you're right, the lockfile was silently dropping the custom port.

Added original_ssh_url to LockedDependency so the verbatim ssh:// URL is now written to apm.lock.yaml and survives a to_dict/from_dict round-trip. Also added three regression tests covering the lockfile path. Will keep provenance completeness in mind for future changes involving URL normalization.

[danielmeppiel]

I'm not sure the lockfile should track this as a separate field. In essence this is the actual repo url. There is a repo url field already. We may be cluttering the lockfile and overall logic (including download logic) unnecessarily.

I'm not sure the lockfile should track this as a separate field. In essence this is the actual repo url. There is a repo url field already. We may be cluttering the lockfile and overall logic (including download logic) unnecessarily.

[edenfunf]

I'm not sure the lockfile should track this as a separate field. In essence this is the actual repo url. There is a repo url field already. We may be cluttering the lockfile and overall logic (including download logic) unnecessarily.
Fair point — I think the key question is whether this is actually a separate piece of data or just a representation issue.

Right now, the problem is that the existing repo_url path drops the port during normalization, so the lockfile ends up not reflecting the exact source that was used. My initial approach was to preserve the original SSH URL separately to avoid losing that information.

That said, I agree this may be duplicating what should conceptually be a single source of truth. A cleaner approach would be to fix the existing repo_url / host handling so the port is preserved, instead of introducing a new field.

Also worth noting: the download path already re-parses apm.yml, so this doesn't affect install correctness — it's mainly about lockfile fidelity / auditability.

I'm happy to switch to fixing repo_url normalization (e.g. preserving port in host like bitbucket.domain.ext:7999) and drop original_ssh_url if that aligns better with the intended design.

[danielmeppiel]

@edenfunf thank you - we already have a user reporting a related problem with an https url on a specific port #731

So I'd rather solve this at the root rather than patching just for ssh. The lockfile should stay as simple as possible and the repo_url should reflect the actual url including the port from where the package was retrieved for maximum auditability

[danielmeppiel]

@edenfunf I changed the title of the PR to reflect the scope (preserving protocol as well as port in repo url)

[danielmeppiel]

@edenfunf im thinking of a scenario here that may need a bit more consideration:

• a developer who prefers ssh to authenticate to GitHub installs a dependency using a ssh:// address

• other developers in the team who may not have configured ssh keys and prefer https are now forced by the lockfile to use ssh (if we preserve the protocol)

The other way around (https vs ssh) is also true.

This is why I originally decided to normalize ssh into https. We probably need to preserve protocol yet fall back to the other protocol (ssh vs https and viceversa). This may need careful consideration / analysis.

This is not the case for ports as those are always to be preserved.

[danielmeppiel]

Additional thoughts:

We may have git@github.com:user/repo.git which is not a standard URL. It’s a Git-specific shorthand (called SCP-like syntax) that:

• resembles user@host:path
• does not include a scheme like ssh://
• is parsed specially by Git, not by general URL parsers

We may also have true SSH URL format such as ssh://git@github.com/user/repo.git which is is a valid URL, because:

• it uses the ssh:// scheme
• it follows standard URL structure

Git accepts both interchangeably. But:

• tools outside Git (browsers, APIs, validators) usually only recognize the ssh:// version as a URL
• the SCP-like syntax is more of a Git convenience format

I'm leaning towards respecting the users ssh url and keeping it in lockfile (in true URL format only, not SCP) regardless of SCP or URL notation provided as input - I.e. normalizing SCP to SSH URL.

Falling back to HTTPS may be better for UX in developer teams. I need a second thought from @sergio-sisternes-epam

[sergio-sisternes-epam]

@danielmeppiel additional comments from my side:

1. Normalise SCP → ssh:// URL in lockfile? Agree 100%. SCP syntax (git@host:path) isn't a real URL — it's a Git convenience. Normalising to ssh://git@host/path gives us a proper URL that standard parsers can
   handle, while Git still accepts it. No information loss.

2. Protocol fallback for team UX? I think we should fall back. The scenario you described is real — one dev installs with SSH, another teammate doesn't have SSH keys configured, and they're stuck.
   _clone_with_fallback already does this at install time, so extending that behaviour to lockfile-driven installs is consistent.

The lockfile should record what was used (respecting the installer's choice), but clone time should be forgiving: try the lockfile protocol first, fall back to the alternative if it fails. This way:

• The lockfile is honest (auditability preserved)
• Teammates aren't blocked (UX preserved)
• Git's own insteadOf config still works for users who want to force a protocol globally

The only trade-off is slightly noisier lockfile diffs/conflicts when two devs install the same package with different protocols, but that's minor compared to broken installs. We might want to update the documentation and recommend teams to use the same clone method to avoid this inconvenience.

[sergio-sisternes-epam]

@danielmeppiel Weighing in on your two questions:

1. Normalise SCP to ssh:// URL in lockfile? Agree 100%. SCP syntax (git@host:path) isn't a real URL -- it's a Git convenience. Normalising to ssh://git@host/path gives us a proper URL that standard parsers can handle, while Git still accepts it. No information loss.

2. Protocol fallback for team UX? I think we should fall back. The scenario you described is real -- one dev installs with SSH, another teammate doesn't have SSH keys configured, and they're stuck. _clone_with_fallback already does this at install time, so extending that behaviour to lockfile-driven installs is consistent.

The lockfile should record what was used (respecting the installer's choice), but clone time should be forgiving: try the lockfile protocol first, fall back to the alternative if it fails. This way:

• The lockfile is honest (auditability preserved)
• Teammates aren't blocked (UX preserved)
• Git's own insteadOf config still works for users who want to force a protocol globally

The only trade-off is slightly noisier lockfile diffs when two devs install the same package with different protocols, but that's minor compared to broken installs.

[edenfunf]

Thanks @danielmeppiel and @sergio-sisternes-epam for the thoughtful discussion. Here's my planned approach before I start reworking the PR:

Lockfile format

• Normalize SCP syntax (git@host:path) to true SSH URL (ssh://git@host/path.git) at parse time
• Store the normalized URL honestly in the lockfile — including the user's chosen protocol and any custom port
• Drop the original_ssh_url workaround; a single source_url field carries protocol + host + port

Clone-time protocol fallback

• Extend the existing _clone_with_fallback behavior to lockfile-driven installs
• Try the protocol declared in the lockfile first (respects the author's choice)
• Fall back to the alternative protocol on failure, so teammates without SSH keys aren't blocked when the author committed an ssh:// URL
• Ports are protocol-specific, so on fallback we drop the source-protocol port and use the target protocol's default (or a known alternative)

Field changes

• DependencyReference / LockedDependency: add source_url (normalized) + protocol; host stays port-free for identity matching
• build_ssh_url goes back to emitting a clean ssh:// URL — no more stuffing host:port into the host field
• A small normalize_to_ssh_url() helper handles the SCP → URL conversion

Tests

• SCP normalization round-trips
• Port preservation for ssh://host:7999 and https://host:8443
• Protocol fallback in both directions (SSH → HTTPS and vice versa)
• Identity matching across the same repo reached via different ports

Does this match what you both had in mind? Happy to adjust before I start — otherwise I'll push the rework shortly.

[danielmeppiel]

Maintainer verdict (port preservation design)

@edenfunf @sergio-sisternes-epam thanks for the thoughtful back-and-forth. I ran this through our python-architect persona and a security review. Here's where I land — and I'd like to redirect the rework before you push v3.

TL;DR

Reject both v2 (original_ssh_url parallel field) and v3 (source_url + protocol + fallback). The right fix is smaller and more direct: add a single port: Optional[int] field, thread it through the URL builders, and stop normalizing ssh:// to SCP form during parsing. ~4 files, ~50 lines, zero schema breakage.

Why not v2

original_ssh_url is a duplicate of repo_url/host for one transport. It only fixes SSH and ignores #731 (HTTPS https://host:8443/... has the same bug). Cluttering both DependencyReference and LockedDependency with a field that exists only because the parser drops a number is the wrong abstraction.

Why not v3

It's over-engineered for what is a port-preservation bug. Specifically:

• Adding source_url + protocol to both dataclasses re-creates the same duplication problem on a bigger surface — these fields are derivable from host + port + repo_url if we treat the port as a first-class field.
• The protocol-fallback-with-port-drop has a real security risk. ssh://internal-host:7999/... failing and silently retrying as https://internal-host/... (default 443) routes to a potentially different service on a corporate proxy. Worse: an attacker who can poison the HTTPS path but not SSH (or vice versa) gets a free transport swap. Our content_hash only catches this on re-install, not first install.
• _clone_with_fallback already runs for lockfile-driven installs — we don't need to extend it. The right fix is making sure the URL builders carry the port; the existing chain then does the right thing.

What to build instead

1. Single new field

# src/apm_cli/models/dependency/reference.py
port: Optional[int] = None  # Non-standard SSH/HTTPS port

# src/apm_cli/deps/lockfile.py — LockedDependency
port: Optional[int] = None

2. Stop normalizing ssh:// to SCP

The current chain _normalize_ssh_protocol_url (reference.py:380) -> SCP -> _parse_ssh_url is what drops the port (SCP git@host:path cannot represent a port — the : is a path separator).

Replace it with a real ssh:// parser that mirrors how we already handle https:// URLs in _parse_standard_url (reference.py:711):

@staticmethod
def _parse_ssh_protocol_url(url: str):
    if not url.startswith("ssh://"):
        return None
    parsed = urllib.parse.urlparse(url)
    return parsed.hostname, parsed.port, repo_url, reference, alias

This also addresses Copilot inline comments #1/#2 (#ref/@alias stripping) cleanly via urlparse — fragment is separate from path.

3. URL builders accept port

# src/apm_cli/utils/github_host.py
def build_ssh_url(host, repo_ref, port=None):
    if port:
        return f"ssh://git@{host}:{port}/{repo_ref}.git"  # ssh:// form (SCP can't carry port)
    return f"git@{host}:{repo_ref}.git"  # SCP shorthand for default port

def build_https_clone_url(host, repo_ref, token=None, port=None):
    netloc = f"{host}:{port}" if port else host
    ...

When port is set, builders emit ssh:// form; without port they keep emitting SCP shorthand (no behavioural change for the 99% case).

4. Same port across protocols on fallback

_clone_with_fallback doesn't change structurally. SSH attempt uses ssh://host:7999, HTTPS attempt uses https://host:7999. If 7999 doesn't speak HTTPS, fail loudly — better than silently hitting a different service.

5. Identity/dedup unchanged

get_unique_key() stays repo_url (bare). get_identity() stays host/repo_url (bare). Port is a transport detail, not an identity component — two devs referencing the same logical repo via different protocols/ports should still dedup to the same package.

6. Lockfile validation (security)

In LockedDependency.from_dict, validate the port:

_p = data.get("port")
port = int(_p) if _p is not None and 1 <= int(_p) <= 65535 else None

We already do validate_path_segments on repo_url; the port deserves the same defensive cast against lockfile tampering.

Tests to add

• ssh://host:7999/path.git parses with port=7999 (no SCP intermediate)
• https://host:8443/path parses with port=8443 (covers Using dependencies within non-default ports #731)
• SCP shorthand git@host:path still parses with port=None (no behaviour change)
• Lockfile round-trip preserves port
• Lockfile rejects garbage port (port: 'x', port: 99999, port: -1)
• Same logical repo on two ports still dedups via get_unique_key()
• _clone_with_fallback builds both SSH and HTTPS URLs with the port when set

Why I'm not asking for protocol fallback in this PR

The team-UX scenario (one dev installs via SSH, teammate has no SSH keys) is real but separate from this bug and bigger than it looks — it changes lockfile semantics across the whole codebase. Let's land port preservation cleanly first, then open a dedicated discussion for cross-protocol fallback design.

Scope summary

File	Change
src/apm_cli/models/dependency/reference.py	Add port, replace _normalize_ssh_protocol_url with a real ssh:// parser via urlparse
src/apm_cli/utils/github_host.py	build_ssh_url / build_https_clone_url accept port
src/apm_cli/deps/lockfile.py	Add port, serialize when non-default, validate on deserialize
src/apm_cli/deps/github_downloader.py	Thread dep_ref.port through _build_repo_url (no fallback chain changes)

Closes the same root cause for both #661 (SSH) and #731 (HTTPS).

@edenfunf — happy to discuss any specific tradeoff, but this is the direction I'd like the PR to take. Let me know if anything is unclear before you start.

[danielmeppiel]

@edenfunf here's a concrete checklist to land this. Sing out if any item is unclear and I'll expand.

Code

• src/apm_cli/models/dependency/reference.py
  • Add port: Optional[int] = None to the dataclass (right after host)
  • Replace _normalize_ssh_protocol_url (line ~380) with _parse_ssh_protocol_url that returns (host, port, repo_url, reference, alias) using urllib.parse.urlparse
  • Update parse() to try the new ssh:// parser before the SCP _parse_ssh_url regex
  • Update _parse_standard_url (line ~711) to capture parsed_url.port for https:///http://
  • In to_canonical()/get_identity(): if port is set, render as host:port/repo_url for display only — get_unique_key() stays repo_url bare
• src/apm_cli/utils/github_host.py
  • build_ssh_url(host, repo_ref, port=None) — emit ssh://git@host:port/repo.git when port is set, keep SCP shorthand otherwise
  • build_https_clone_url(host, repo_ref, token=None, port=None) — embed port in netloc
• src/apm_cli/deps/lockfile.py
  • Add port: Optional[int] = None to LockedDependency
  • to_dict: emit port only when non-None (backward compat)
  • from_dict: defensive cast — int(p) only if 1 <= int(p) <= 65535, else None
  • from_dependency_ref: pass port=dep_ref.port
• src/apm_cli/deps/github_downloader.py
  • In _build_repo_url (line ~571), thread dep_ref.port to the URL builders
  • Drop the original_ssh_url branch in _clone_with_fallback (lines 697-701) — once port is in the URL builders, the existing fallback chain produces the right URLs naturally

Tests

Two existing test files are the right home — please add to them rather than creating a new file:

• tests/unit/test_generic_git_urls.py (parsing)
  • ssh://host:7999/path.git -> host=..., port=7999, repo_url=path
  • https://host:8443/path -> port=8443 (covers Using dependencies within non-default ports #731)
  • git@host:path (SCP) -> port=None (no behaviour change)
  • ssh://host/path.git (no port) -> port=None
  • #ref and @alias correctly extracted from ssh://host:7999/path.git#main@alias
• tests/unit/test_lockfile.py (or wherever LockedDependency round-trip lives)
  • port round-trips via to_dict/from_dict
  • Lockfile without port field still loads cleanly
  • Garbage port values rejected: 'x', 99999, -1, 0 -> None
  • Same logical repo_url with two different ports still collides under get_unique_key() (dedup invariant)
• tests/unit/test_auth_scoping.py (clone behaviour — you already added cases here)
  • _clone_with_fallback builds ssh://host:7999/... for the SSH attempt when dep_ref.port=7999
  • Same builds https://host:7999/... for the HTTPS attempt — port preserved across protocols
  • Replace the except Exception swallow with except (RuntimeError, GitCommandError) (Copilot inline Will there be MCP coverage? #3)
• Drop the existing TestBitbucketDatacenterSSH class — it's testing original_ssh_url which goes away

Docs

These need updating in the same PR (our convention is code + docs land together):

• docs/src/content/docs/guides/dependencies.md
  • Around line 124 ("SSH URL" bullet) add a sub-bullet: ssh://git@host:PORT/owner/repo.git -- explicit ssh:// form is required when using a non-default SSH port (SCP shorthand cannot carry ports)
  • Around line 139 (the git@bitbucket.org:team/rules.git example) add a Bitbucket Datacenter / self-hosted example with custom port
• docs/src/content/docs/reference/manifest-schema.md
  • Line ~177 (URL grammar): note that ssh:// and https:// URLs may include :PORT; git@ SCP shorthand may not
  • Line ~206 (examples block): add a ssh://git@bitbucket.example.com:7999/team/repo.git example
  • If there's a lockfile schema section, add the new port field
• docs/src/content/docs/guides/private-packages.md (line ~39 self-hosted section) — one-line note that custom SSH/HTTPS ports are supported via the URL form

CHANGELOG

• Update the existing entry to cover both protocols and reference both issues:
  - Preserve custom port in dependency URLs for self-hosted git servers (\ssh://host:7999`, `https://host:8443\`) instead of silently dropping it ([BUG] Ignores ssh://, tries https (Bitbucket Datacenter) #661, Using dependencies within non-default ports #731) (fix: preserve protocol (e.g. ssh:// or https://) and port in dependency URL #665)`

Out of scope (separate follow-up)

• Cross-protocol fallback for team UX (one dev installs SSH, teammate has no SSH keys). Real concern, but it changes lockfile semantics and deserves its own discussion. Please don't bundle it here.
• Refactoring the v2 TestExplicitTargetDirCreation-style tests into CliRunner-driven integration tests — that's a broader pattern we'll address in Test gap: TestExplicitTargetDirCreation and TestContentHashFallback are logic-replays #768.

Validation before pushing

uv run pytest tests/unit/test_generic_git_urls.py tests/unit/test_lockfile.py tests/unit/test_auth_scoping.py -x
uv run pytest tests/unit tests/test_console.py -x   # full unit suite, must stay green

Once all of the above is in, I'll do another review pass and we can ship. Thanks for sticking with this — the original bug report is going to make a lot of self-hosted users happy.