fix: enhance git remote validation by trying SSH URLs for generic hosts#584
fix: enhance git remote validation by trying SSH URLs for generic hosts#584zzoubian wants to merge 8 commits intomicrosoft:mainfrom
Conversation
@microsoft-github-policy-service agree |
Review FeedbackThanks @zzoubian for tackling this! The SSH-first approach for generic hosts is sound. A few things need addressing before merge: 1. CLA not signed (blocker)Please reply to the CLA bot comment with 2. Missing
|
|
Ok added unit tests and replied to CLA with |
sergio-sisternes-epam
left a comment
There was a problem hiding this comment.
Clean fix that aligns validation with the existing download behavior in _clone_with_fallback(). SSH-first for generic hosts, HTTPS fallback — well tested with 5 cases covering the happy path, fallback, and negative scenarios. All prior review feedback addressed.
CodeQL note: The CodeQL failure is a false positive on test URL strings — not a code issue.
There was a problem hiding this comment.
Pull request overview
Fixes pre-install validation failures for repositories on generic/self-hosted Git servers that are accessible via SSH but may require authentication over HTTPS, aligning validation behavior with the existing clone fallback strategy.
Changes:
- Update
_validate_package_exists()to trygit ls-remoteover SSH first for generic (non-GitHub/non-ADO) hosts, then fall back to HTTPS. - Add unit tests covering SSH-first probing, HTTPS fallback, and host-type-specific behavior (GitHub.com vs GHES).
Show a summary per file
| File | Description |
|---|---|
| src/apm_cli/commands/install.py | Implements SSH-first git ls-remote probing for generic hosts with HTTPS fallback. |
| tests/unit/test_install_command.py | Adds unit tests to validate SSH-first probing behavior and ensure GitHub/GHES paths do not take the generic SSH-first route. |
Copilot's findings
- Files reviewed: 2/2 changed files
- Comments generated: 2
| for probe_url in urls_to_try: | ||
| cmd = ["git", "ls-remote", "--heads", "--exit-code", probe_url] | ||
| result = subprocess.run( | ||
| cmd, | ||
| capture_output=True, | ||
| text=True, | ||
| encoding="utf-8", | ||
| timeout=30, | ||
| env=validate_env, | ||
| ) |
There was a problem hiding this comment.
In the generic-host path, validate_env intentionally omits GIT_ASKPASS / GIT_CONFIG_GLOBAL (see above). Later in this function, the verbose stderr sanitization does stderr_snippet.replace(validate_env.get(env_var, ""), "***"); when the key is missing this replaces the empty string, producing garbled output. Guard the replacement so it only runs when the env var value is a non-empty string (or use token/url sanitizers instead of env var substitution).
|
|
||
| @patch("subprocess.run") | ||
| def test_ghes_host_skips_ssh_attempt(self, mock_run): | ||
| """A GHES host is treated as GitHub, not generic SSH probe is skipped.""" |
There was a problem hiding this comment.
Docstring grammar: "A GHES host is treated as GitHub, not generic SSH probe is skipped." reads like a run-on sentence and is hard to parse. Reword to clearly state that GHES hosts are treated as GitHub and therefore skip the generic SSH-first probe.
| """A GHES host is treated as GitHub, not generic SSH probe is skipped.""" | |
| """GHES hosts are treated as GitHub hosts and skip the generic SSH-first probe.""" |
Description
Fixes apm install fails the pre-install validation step when targeting a self-hosted Git server
Fixes #583
Type of change
Testing