Conversation
There was a problem hiding this comment.
Pull request overview
This PR enhances Windows installation support by adding a native PowerShell installer while also hardening the existing shell-based installer for better cross-platform compatibility. It addresses Windows Schannel certificate revocation errors that previously prevented installations on Windows 10, and improves bin-directory detection to prefer existing PATH locations.
Changes:
- Add new
scripts/install.ps1PowerShell installer for native Windows installations - Add Windows Schannel certificate revocation error handling to
install.shwith automatic fallback flags - Add GitHub API fallback for version detection when redirect-based method fails
- Implement intelligent bin-directory selection preferring already-present PATH locations
- Update documentation to recommend PowerShell for native Windows installations
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| scripts/install.ps1 | New PowerShell installer with complete Windows support including architecture detection, checksum verification, and PATH management |
| scripts/install.sh | Enhanced shell installer with Schannel fallback handling, API fallback for version detection, and smart bin-dir selection |
| scripts/ensure-basecamp.sh | Updated with same enhancements as install.sh for consistency |
| README.md | Updated Quick Start and installation methods to include PowerShell option and Windows Schannel troubleshooting note |
| install.md | Updated installation guide with separate PowerShell instructions and Windows-specific guidance |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 5 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="scripts/ensure-basecamp.sh">
<violation number="1" location="scripts/ensure-basecamp.sh:117">
P0: `curl_run` reads `$?` from the `if` statement instead of the failed `curl`, causing curl failures to be treated as success and skipping fallback logic.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
|
Fixed — cubic found that |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
robzolkos
force-pushed
the
windows-installers
branch
from
6a15753 to
ccc26c3
Compare
robzolkos
force-pushed
the
windows-installers
branch
2 times, most recently
from
4913838 to
9c380bc
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
robzolkos
force-pushed
the
windows-installers
branch
from
9c380bc to
44dbf02
Compare
robzolkos
force-pushed
the
windows-installers
branch
from
44dbf02 to
6874dff
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
When the chosen bin dir wasn't already on PATH, install_basecamp printed the persistent-add instructions but didn't update PATH for the running script. The follow-up check_basecamp call then failed because `command -v basecamp` couldn't find it, so --install exited 1 on a genuinely successful install. Export BIN_DIR into PATH after the existing instructions block so the in-script check passes; the persistent-add guidance still prints because it inspects the original PATH.
Bring install.ps1 closer to install.sh's behavior on three security and reliability fronts, plus an actionable error for the most common Windows re-install failure. - Cosign signature verification when cosign.exe is on PATH, mirroring install.sh. Check $LASTEXITCODE explicitly because Windows PowerShell 5.1 doesn't surface native exits via $ErrorActionPreference=Stop, so a silent verify failure would otherwise false-green. - Surface an actionable error when basecamp.exe is locked. Windows holds an exclusive lock on running PE files; -Force doesn't help. Wrap the copy in a generic catch (typed catches miss the ActionPreferenceStopException wrapper) and include the original exception text so unrelated failures aren't masked. - Try the GitHub releases/latest redirect before the API to avoid unauthenticated rate limits, falling back to the API on miss. Read Location off the response in both the success and 302-as-error paths so it works on Windows PowerShell 5.1 and PowerShell Core. - Prepend BIN_DIR to the persisted user PATH so a stale basecamp.exe earlier in PATH doesn't shadow the freshly-installed one in new shells.
|
Just pushed two commits addressing the blockers from my review:
Heads-up: I cannot validate
A few smaller notes I'd be happy for the author to take or leave — none are blockers: 5. 6. Persisted PATH change isn't broadcast. Other open shells/Explorer don't see the new PATH until restart (no 7. 8. 9. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
1 issue found across 2 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="scripts/install.ps1">
<violation number="1" location="scripts/install.ps1:243">
P2: The new error handling always reports `basecamp.exe` as "in use" even when `Copy-Item` fails for other reasons, which can mislead troubleshooting.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
- Mention Git Bash alongside macOS/Linux/WSL2 for the shell installer in README so it matches install.md and reflects this PR's Schannel hardening. - Document the PATH-aware BASECAMP_BIN_DIR default in both install.sh and ensure-basecamp.sh — default_bin_dir() prefers ~/bin or ~/.local/bin if already on PATH before falling back to a platform default. - Soften the install.ps1 Copy-Item failure message: lead with the generic install failure, suggest closing running processes if the file is in use. The original exception text is still appended so disk-full or permission failures aren't masked.
Bash parameter expansion `${api_json#*\"tag_name\":\"v}` only matched the
exact `\"tag_name\":\"v...\"` shape. GitHub currently returns minified JSON
under `Accept: application/vnd.github+json`, so the parse worked, but a
future format change or alternate endpoint that pretty-printed would have
silently fallen through (the trailing semver guard would catch the garbage,
but the fallback would be unusable).
Replace with a bash regex that tolerates spaces, tabs, and newlines around
the colon and makes the leading `v` optional. The regex is anchored on the
unescaped quotes around `tag_name`, so a release body containing the
substring `\"tag_name\":\"...\"` (which gets backslash-escaped in JSON)
can't false-match. Stays in pure bash so it works on macOS' shipped
/bin/bash 3.2 and Git Bash without depending on GNU-awk's 3-arg match().
|
You're iterating quickly on this pull request. To help protect your rate limits, cubic has paused automatic reviews on new pushes for now—when you're ready for another review, comment |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- curl_run: force --show-error inside the helper. All current callers pass -fsSL (which already includes -S), so behavior is unchanged today, but encoding the flag in the helper makes the Schannel revocation fallback invariant — "errors must reach stderr so we can grep for CRYPT_E_NO_REVOCATION_CHECK" — robust to a future caller that uses -s alone. - install.ps1 Download-File: pass -UseBasicParsing -ErrorAction Stop. On Windows PowerShell 5.1 the cmdlet initializes IE's MSHTML parser even for -OutFile downloads, which fails on Server Core / locked-down installs. -UseBasicParsing is a no-op on PowerShell 6+. -ErrorAction Stop doubles up on the global $ErrorActionPreference for clarity and defense if a future function locally relaxes it. - install.ps1 Get-LatestVersion API fallback: same -ErrorAction Stop hardening on Invoke-RestMethod for consistency. - install.md: align Step 1 heading with README — "WSL2 / Git Bash" instead of "WSL / Git Bash". The PR description and README both say WSL2.
Summary
Windows users were being funneled into a Unix-first
curl | bashflow that can fail before the installer even runs, especially on Windows 10 with Schannel revocation errors likeCRYPT_E_NO_REVOCATION_CHECK. This PR makes Windows installation first-class by adding a native PowerShell installer, while also hardening the existing shell installer for Git Bash and other bash-based environments on Windows. The result is a more reliable install experience across native Windows, Git Bash, and WSL2 without regressing Unix users.Ref: https://3.basecamp.com/2914079/buckets/46292715/card_tables/cards/9751372396
Changes
scripts/install.ps1install.shas the installer for macOS, Linux, WSL2, and bash-based Windows environmentsinstall.shto better handle Windows Schannel revocation-check failures by retrying with supported curl flags~/binor~/.local/bin) before falling back to platform defaultsscripts/ensure-basecamp.shwith the same smarter path and Windows-friendly behaviorNotes
Summary by cubic
Adds a native Windows PowerShell installer and toughens the bash installers across Windows/WSL2/macOS/Linux. Improves latest-version detection (incl. prereleases), SHA‑256 with optional
cosign, PATH‑aware bin‑dir defaults, Schannel‑awarecurl, and non‑interactive behavior; docs add OS‑specific quick starts, Git Bash callouts, and Windows Schannel guidance.New Features
scripts/install.ps1: latest‑version via redirect with GitHub API fallback; SHA‑256 and optionalcosignverification; installs to a PATH‑aware bin dir and prepends the user PATH; supports prereleases; clearer non‑interactive flow; actionable install‑failure message ifbasecamp.exeis in use; hardened downloads with-UseBasicParsing -ErrorAction Stop.scripts/install.sh,scripts/ensure-basecamp.sh): Schannel‑awarecurlwrapper that forces--show-errorand retries onCRYPT_E_NO_REVOCATION_CHECK; smarter bin‑dir selection using current PATH (prefers~/binor~/.local/bin; defaults to~/binon Windows) with exact PATH hints; optionalcosign.Bug Fixes
install.shandensure-basecamp.shto avoid breaks on JSON formatting changes.scripts/ensure-basecamp.sh: after installing to a bin dir not on PATH, export that dir into the current PATH so the post‑install self‑check succeeds without a new shell.Written for commit cd75c3b. Summary will update on new commits. Review in cubic