Skip to content

fix(ps): drop ssh probe + correct exit-code check in Invoke-GhRepoClone#250

Closed
szymonos wants to merge 2 commits into
mainfrom
fix/git-clone-protocol-and-exitcode
Closed

fix(ps): drop ssh probe + correct exit-code check in Invoke-GhRepoClone#250
szymonos wants to merge 2 commits into
mainfrom
fix/git-clone-protocol-and-exitcode

Conversation

@szymonos

@szymonos szymonos commented May 2, 2026

Copy link
Copy Markdown
Owner

Summary

Two follow-up fixes for code-review feedback on the SSH-first path introduced in #246 (`modules/InstallUtils/Functions/git.ps1`).

1. Drop the `ssh -T git@github.com` probe

The probe ran on every `Invoke-GhRepoClone` call to decide between SSH and HTTPS. Three problems:

  • Network round-trip per call. Every PowerShell module install during `linux_setup.sh` paid this cost.
  • Can block. Host-key prompt (no entry in `~/.ssh/known_hosts`), slow DNS, firewall holes, encrypted-key passphrase prompts.
  • Redundant. The catch block already retries SSH failures over HTTPS, so the probe was defensive over-engineering.

Protocol selection reverts to deriving from the parent repo's remote URL via `getOrigin` (the original pre-#246 behavior). SSH-configured parent repos still trigger SSH-first cloning, and the HTTPS retry remains as fallback.

The `Get-Command ssh` guard added in #247 (for Windows hosts without OpenSSH) is now obsolete — there's no bare `ssh` invocation to guard.

2. Replace pipeline `$?` check with `$LASTEXITCODE`

```powershell
git clone $cloneUrl "$destPath" --quiet 2>&1 | ForEach-Object { $cloneErr += "$_`n" }
if (-not $?) { ... } # WRONG: $? reflects ForEach-Object (always true)
```

The pipeline made `$?` reflect the tail (`ForEach-Object`, which always succeeds), so failed clones were treated as successful: the HTTPS fallback never ran and callers received `status=1` (cloned) when nothing was actually fetched. Now `$LASTEXITCODE` is consulted directly and captured stdout+stderr is rendered via `Out-String` for the warning text.

```powershell
$cloneOut = git clone $cloneUrl "$destPath" --quiet 2>&1
$cloneOk = $LASTEXITCODE -eq 0
if (-not $cloneOk -and $gitProtocol -eq 'git@github.com:') {
# ... HTTPS retry ...
}
```

Test plan

  • `pwsh` parse-check on `modules/InstallUtils/Functions/git.ps1`
  • `make lint` clean (gremlins / shellcheck / shfmt / pester / bats all pass)
  • Manual: run `linux_setup.sh` on a fresh distro with `-Scope shell` to validate the ps-modules clone still works (HTTPS path)
  • Manual: same on a distro with SSH key configured to validate the SSH path still triggers when parent repo origin is SSH-form

🤖 Generated with Claude Code

szymonos added 2 commits May 2, 2026 15:43
Two follow-up fixes for code-review feedback on the SSH-first path
introduced in #246:

1. Remove the `ssh -T git@github.com` probe in the begin block.
   It adds a network round-trip before every clone attempt, can block
   on host-key prompts (no entry in ~/.ssh/known_hosts), slow DNS, or
   firewall holes, and prompts for passphrase on encrypted keys. The
   catch block already retries SSH failures over HTTPS, so the probe
   was redundant defensive-ness. Protocol selection reverts to deriving
   from the parent repo's remote URL via `getOrigin` - SSH-configured
   parent repos still trigger SSH-first cloning, with the HTTPS retry
   as fallback.

   The Get-Command ssh guard added in #247 (for Windows hosts without
   OpenSSH) is no longer needed since there's no `ssh` invocation here.

2. Replace `git clone ... | ForEach-Object { ... }` + `if (-not $?)`
   with non-pipeline capture + `$LASTEXITCODE`. The pipeline made $?
   reflect ForEach-Object's success (always true), so failed clones
   were treated as successful: the HTTPS fallback never ran and
   callers received status=1 (cloned) when nothing was actually
   fetched. Now $LASTEXITCODE is consulted directly and the captured
   stdout+stderr is rendered into the warning via Out-String.
@szymonos szymonos closed this Jun 14, 2026
@szymonos
szymonos deleted the fix/git-clone-protocol-and-exitcode branch June 14, 2026 13:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant