Add PowerShell installer path for gh-aw and cover it in Windows CI#44953
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
Thanks for the detailed PR — the Windows installer path and CI coverage are clearly relevant to gh-aw. However, this repository's contribution process requires pull requests to be opened by core team members (MEMBER/OWNER/COLLABORATOR). This PR is authored by a non-core contributor account ( Please move the proposal into an issue or hand it off to a core team member to open and implement via the project's agentic workflow. The PR description and test coverage are helpful and can be reused in that issue.
|
There was a problem hiding this comment.
Pull request overview
Adds native PowerShell installation support and routes Windows setup-action usage through it.
Changes:
- Adds and synchronizes the PowerShell installer.
- Adds Windows-specific action routing and CI coverage.
- Updates installer documentation and integration checks.
Show a summary per file
| File | Description |
|---|---|
install-gh-aw.ps1 |
Implements the canonical PowerShell installer. |
actions/setup-cli/install.ps1 |
Generated installer copy for the action. |
actions/setup-cli/action.yml |
Selects PowerShell on Windows. |
.github/workflows/install.yml |
Adds Windows installer CI coverage. |
pkg/cli/setup_cli_action_integration_test.go |
Checks PowerShell script wiring and synchronization. |
Makefile |
Synchronizes both installer scripts. |
.gitattributes |
Marks the generated PowerShell script. |
docs/src/content/docs/setup/cli.md |
Documents PowerShell installation. |
actions/setup-cli/README.md |
Documents generated installer development. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 9/9 changed files
- Comments generated: 10
- Review effort level: Medium
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #44953 does not have the 'implementation' label and has ≤100 new lines of code in business logic directories (60 additions, threshold 100). |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
✅ Test Quality Sentinel completed test quality analysis. |
🧪 Test Quality Sentinel Report
📊 Metrics (6 tests)
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: Add PowerShell installer for gh-aw
The overall approach is solid — native PowerShell installer with feature parity to the bash script, proper platform detection, checksum verification, and Windows-specific timeout handling.
Blocking Issue
$output -notmatch on an array is not a boolean check (.github/workflows/install.yml, dry-run test step).
When & ./install-gh-aw.ps1 v999.999.999 2>&1 is assigned to $output, the variable is a PowerShell array (strings + ErrorRecord objects). Applying -notmatch to an array filters elements rather than returning a boolean. Since many output lines will not contain "Detected OS:", the filter returns a non-empty array — which is always truthy — so the throw fires unconditionally even when detection succeeds. This makes the dry-run test permanently broken.
Fix: join $output into a single string before the -notmatch checks:
$outputStr = ($output | Out-String)
if ($outputStr -notmatch "Detected OS:") { throw "OS detection failed" }
if ($outputStr -notmatch "Detected architecture:") { throw "Architecture detection failed" }
if ($outputStr -notmatch "Platform:") { throw "Platform string construction failed" }Non-blocking Notes
- The PowerShell script itself (
install-gh-aw.ps1) is well-structured with good error handling, retry logic, checksum verification, and Windows Defender timeout awareness. - The
action.ymlrouting (runner.os == 'Windows'→ PowerShell, otherwise Bash) is correct. - The Go integration tests for
install.ps1are reasonable sanity checks.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · 69.5 AIC · ⌖ 4.55 AIC · ⊞ 4.8K
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /tdd, /codebase-design, and /grill-with-docs — requesting changes on 4 issues.
📋 Key Findings & Highlights
Issues to Address
-
installed_versionreports"latest"verbatim (install-gh-aw.ps1, end of file) — the gh-extension fast-path correctly emits the resolved semver; the manual-download fallback does not. Consumers of the action output will receive the literal string"latest"instead of a tag likev0.4.2. -
Asymmetric
TryGhInstallactivation (install-gh-aw.ps1, line ~20) —INPUT_VERSIONauto-enables the gh-extension fast path; a positional version argument does not. Direct callers miss the fast path entirely. -
PowerShell lines inside a
bashfence (docs/src/content/docs/setup/cli.md, lines 56–57) — incorrect syntax highlighting, misleads copy-pasters. -
Duplicated timeout helper in CI (.github/workflows/install.yml, lines 75–97) —
Invoke-WithTimeoutduplicatesInvoke-ProcessWithTimeoutfrom the installer, creating a maintenance burden.
Positive Highlights
- ✅ Checksum verification with graceful degradation
- ✅ Retry with exponential backoff
- ✅ Thoughtful Windows Defender timeout handling
- ✅ Integration tests verify sync, feature presence, and action wiring
- ✅
make sync-action-scriptskeeps canonical and action copies consistent
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 63 AIC · ⌖ 4.87 AIC · ⊞ 6.6K
Comment /matt to run again
Comments that could not be inline-anchored
install-gh-aw.ps1:430
[/tdd] installed_version outputs the string "latest" instead of the resolved semver tag when no version is pinned — breaking consumers that parse the action output.
<details>
<summary>💡 Fix: resolve the tag before writing the output</summary>
The gh-extension fast-path already resolves the actual version and writes $installedVersion. The manual-download path should do the same. $LatestTag is already fetched from the API earlier in the script:
if ($env:GITHUB_OUTPUT…
</details>
<details><summary>install-gh-aw.ps1:20</summary>
**[/tdd]** When `INPUT_VERSION` is set, `TryGhInstall` is silently forced to `true`, but passing a version as a positional argument does not enable the gh-extension fast path. This asymmetry means the behaviour of `./install-gh-aw.ps1 v1.2.3` differs from `INPUT_VERSION=v1.2.3 ./install-gh-aw.ps1`, which can surprise users calling the standalone script directly.
<details>
<summary>💡 Suggestion</summary>
Either always set `TryGhInstall = $true` when any version is specified (positional or env…
</details>
<details><summary>docs/src/content/docs/setup/cli.md:57</summary>
**[/grill-with-docs]** PowerShell commands are placed inside a `bash wrap` code fence — they will be syntax-highlighted and linted as Bash, confusing readers and tooling.
<details>
<summary>💡 Suggestion: split into separate fences by language</summary>
```markdown
```bash
curl -sL https://raw.githubusercontent.com/github/gh-aw/main/install-gh-aw.sh | bashInvoke-WebRequest https://raw.githubusercontent.com/github/gh-aw/main/install-gh-aw.ps1 -OutFile install-gh-aw.ps1
pwsh…
</details>
<details><summary>.github/workflows/install.yml:75</summary>
**[/codebase-design]** `Invoke-WithTimeout` in the CI step is a near-copy of `Invoke-ProcessWithTimeout` inside the installer. Duplicating the function means any bug fix (e.g. in `Kill($true)`) must be applied in two places.
<details>
<summary>💡 Suggestion</summary>
The CI verification step can simply call the installed binary directly — since the timeout function already lives inside the installer script, a thin wrapper at the CI level is sufficient:
```yaml
- name: Verify installed binary…
</details>
There was a problem hiding this comment.
REQUEST_CHANGES — Multiple blocking issues across the installer, the composite action output wiring, and the CI verification logic.
Blocking themes (must fix before merge)
Critical / High
-
Set-StrictModecrash (install-gh-aw.ps1line 419):($null + $null).Trim()throwsPropertyNotFoundExceptionwheneverghextension install produces no stdout/stderr. Crashes the installer in its own error-handling path. -
CI dry-run is broken (
install.ymlline 45):Write-Hostemits to PS stream 6;2>&1does not capture it.$outputis always empty, so every-notmatchguard always fires — the dry-run step is permanently broken. -
installed-versionaction output is always empty (action.ymlline 190): composite action outputs require avalue:mapping; neither install step has anidnor maps its output. The documentedinstalled-versionoutput resolves to empty string on all platforms. -
No HTTP timeout (
install-gh-aw.ps1line 377):Invoke-WebRequest/Invoke-RestMethodcalls have no-TimeoutSec, so stalled connections bypass all retry logic and hang indefinitely. -
Checksum failure silently skips verification (
install-gh-aw.ps1line 536): a failedchecksums.txtdownload is treated as a warning and the binary is installed unverified. An attacker can suppresschecksums.txtto bypass integrity checks. -
GITHUB_OUTPUTwrites literal"latest"(install-gh-aw.ps1line 632): when API tag resolution fails, the manual-install path writesinstalled_version=latestinstead of a real semver tag.
Medium
- Kill/read race (
install-gh-aw.ps1line 344): redirect files read before killed process fully exits — partial output risk on Windows. - CI accepts timeout as success (
install.ymlline 100): exit code 124 (timeout) is treated equivalently to exit 0, making binary verification vacuous.
Low
- PowerShell commands in a Bash fence (
docsline 641): wrong syntax highlighting.
🔎 Code quality review by PR Code Quality Reviewer · 101.6 AIC · ⌖ 5.65 AIC · ⊞ 5.4K
Comment /review to run again
Comments that could not be inline-anchored
install-gh-aw.ps1:419
NullReferenceException crash under Set-StrictMode when gh produces no output: .Trim() is called on the result of $null + $null, which throws under Set-StrictMode -Version Latest (set at line 6), crashing the installer whenever gh runs silently.
<details>
<summary>💡 Suggested fix</summary>
Get-Content -Raw on an empty redirect file returns $null, not "". Under Set-StrictMode -Version Latest, calling .Trim() on $null throws PropertyNotFoundException. The same patter…
install-gh-aw.ps1:45
CI dry-run check will always fail: Write-Host emits to PowerShell's information stream (stream 6), but $output = & ./install-gh-aw.ps1 v999.999.999 2>&1 only redirects stdout (stream 1) and stderr (stream 2). The $output variable will be empty, making every -notmatch check evaluate against $null, and the if ($output -notmatch "Detected OS:") guard will always throw even when detection works.
<details>
<summary>💡 Suggested fix</summary>
Capture the information stream by redire…
actions/setup-cli/action.yml:190
installed-version output is never populated on Windows: the composite action declares installed-version as an output but neither step sets value:, so the output is always empty. The install script writes to $GITHUB_OUTPUT, but composite action outputs require an explicit value: mapping to expose a step's output to callers.
<details>
<summary>💡 Suggested fix</summary>
Give each install step an id and map the action output:
steps:
- name: Install gh-aw CLI (PowerShel…
</details>
<details><summary>install-gh-aw.ps1:377</summary>
**No HTTP timeout on `Invoke-WebRequest` — installer can hang indefinitely**: binary and checksum downloads use `Invoke-WebRequest` without `-TimeoutSec`, so a stalled connection blocks the installer forever with no recovery path.
<details>
<summary>💡 Suggested fix</summary>
The bash installer bounds downloads to 120 seconds (`--max-time 120`); match that here. Add `-TimeoutSec` to every `Invoke-WebRequest` call — in `Invoke-DownloadWithRetry` (line 377) and the checksums loop (line 530), an…
</details>
<details><summary>install-gh-aw.ps1:536</summary>
**Checksum download failure silently skips integrity verification**: if all 3 checksum download attempts fail, the installer emits a warning and continues installing the binary without verification — this is a security hole, not a recoverable warning.
<details>
<summary>💡 Suggested fix</summary>
A network attacker or CDN error could make `checksums.txt` unreachable while delivering a tampered binary. The current behavior lets that attack succeed silently.
Unless `--skip-checksum` was explic…
</details>
<details><summary>install-gh-aw.ps1:632</summary>
**`GITHUB_OUTPUT` writes literal string `"latest"` instead of resolved version**: when no version is specified and the API tag resolution fails (line 474), `$Version` remains `"latest"`. At install completion, `installed_version=latest` is written to the output — any caller checking the output to pin or verify a version will get a useless sentinel value.
<details>
<summary>💡 Suggested fix</summary>
Resolve the actual installed version from the binary before writing to output, falling back to…
</details>
<details><summary>install-gh-aw.ps1:344</summary>
**Race condition: redirect files read immediately after `Kill()` without waiting for process exit**: `Process.Kill($true)` initiates termination asynchronously on Windows; the OS may not have flushed buffered I/O to the redirect files before `Get-Content` reads them, yielding partial or empty output.
<details>
<summary>💡 Suggested fix</summary>
After calling `Kill()`, wait for process exit before reading the redirect files:
```powershell
try {
$process.Kill($true)
$process.WaitForEx…
</details>
<details><summary>.github/workflows/install.yml:100</summary>
**CI test accepts timeout (exit 124) as a successful install verification**: `Invoke-WithTimeout` returning 124 means the binary timed out — not that it passed. Treating it as a non-failure makes it impossible for this test to distinguish a working binary from a hanging or crashing one.
<details>
<summary>💡 Suggested fix</summary>
At minimum, mark a timeout as a distinct warning rather than letting the step silently pass:
```powershell
$versionExit = Invoke-WithTimeout -FilePath $binaryPath…
</details>
<details><summary>docs/src/content/docs/setup/cli.md:641</summary>
**PowerShell commands in a Bash code fence**: the two new `Invoke-WebRequest` lines are inside a ```bash``` fenced block, so rendered documentation applies Bash syntax highlighting and copy-as-bash context to PowerShell syntax.
<details>
<summary>💡 Suggested fix</summary>
Split into separate, language-tagged fences:
```markdown
```bash
curl -sL https://raw.githubusercontent.com/github/gh-aw/main/install-gh-aw.sh | bash # Latest
curl -sL https://raw.githubusercontent.com/githu…
</details>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Fixed in commit CI failure (dry-run step): The step was using action.yml outputs: Added Installer timeouts: Added Process.Kill async: Added |
|
@copilot please run the
|
…l-script-install-gh-aw Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Branch updated with latest main (merged |
|
|
Branch is up to date with main. All 11 review threads remain resolved. Local validation passes ( |
|
Run: https://github.com/github/gh-aw/actions/runs/29180928196
|
Branch is up to date with main. All 11 review threads are resolved. Local validation passes ( |
🤖 PR Triage
Rationale: Non-draft, adds Windows PowerShell installer path + CI coverage. Good value, moderate scope (9 files). Batch with other medium-risk non-urgent PRs. Windows CI in_progress. Triage run §29183606049
|
|
|
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Branch is up to date with main. Fixed the remaining CI failure in the "Test PowerShell installer on Windows" job. Root cause: The "Verify composite-action binary and installed-version output" step asserted the binary existed at Fix (commit All 11 review threads remain resolved. Local validation passes ( |
🤖 PR Triage
PowerShell installer + Windows CI. +1042/-10 — large addition but isolated to install/CI path. Batch review.
|
|
@copilot please run the
|
Branch is up to date with main. All CI checks on the latest HEAD are now passing — including "Test PowerShell installer on Windows" (fixed in commit |
|
@copilot please run the Run: https://github.com/github/gh-aw/actions/runs/29196572530
|
Branch is up to date with main. All CI checks are passing (run |
Windows installation relied on the bash installer path even though the project already ships Windows binaries. This change adds a native PowerShell installer, routes the setup action through it on Windows, and exercises that path in CI.
Installer
install-gh-aw.ps1as the PowerShell equivalent ofinstall-gh-aw.shgh extension installfast path, checksum verification, latest-release fallback, and post-install verificationactions/setup-cli/install.ps1duringmake buildSetup action
actions/setup-cli/action.ymlto use:install.ps1on Windows runnersinstall.shon non-Windows runnersCI coverage
.github/workflows/install.ymlwith a dedicated Windows PowerShell install jobRepo wiring
actions/setup-cli/install.ps1as generated in.gitattributesExample PowerShell usage: