Fix/installer: harden install.ps1 and install.cmd based on review feedback#22
Conversation
…rovements Co-authored-by: yeabwang <122813658+yeabwang@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR hardens the Windows installer entrypoints by improving installer selection/upgrade behavior in install.ps1 and making the CMD bootstrapper more robust for downloading and executing the PowerShell installer.
Changes:
- Hardened
Get-CommandPathnull/whitespace handling and improved installer selection error messaging ininstall.ps1. - Added reinstall/upgrade flags for
pipx/pip3/pipinstalls and tightened install failure detection ininstall.ps1. - Added branch override support and revamped temp-script creation/execution/cleanup flow in
install.cmd.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| install.ps1 | Improves robustness of command path resolution, installer selection messaging, and reinstall/upgrade behavior. |
| install.cmd | Adds configurable download branch and changes execution model intended to ensure temp script cleanup. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| powershell -NoProfile -ExecutionPolicy Bypass -Command "& { param([string]$scriptPath, [string[]]$scriptArgs) try { & $scriptPath @scriptArgs; exit $LASTEXITCODE } finally { Remove-Item -LiteralPath $scriptPath -ErrorAction SilentlyContinue } }" "%REMOTE_SCRIPT%" %* | ||
| set "EXIT_CODE=%errorlevel%" |
There was a problem hiding this comment.
The -Command wrapper runs the downloaded script in-process (& $scriptPath ...). Because install.ps1 uses exit (e.g., exit 0 for -Help and exit 1 on failure), an exit inside the script will terminate the PowerShell process before this wrapper’s finally executes, so the temp file may not be cleaned up (and the exit code handling becomes brittle). Consider invoking the script in a child PowerShell process inside the try (e.g., powershell -File $scriptPath ...) so the outer finally always runs, then propagate the child process exit code.
| powershell -NoProfile -ExecutionPolicy Bypass -Command "& { param([string]$scriptPath, [string[]]$scriptArgs) try { & $scriptPath @scriptArgs; exit $LASTEXITCODE } finally { Remove-Item -LiteralPath $scriptPath -ErrorAction SilentlyContinue } }" "%REMOTE_SCRIPT%" %* | |
| set "EXIT_CODE=%errorlevel%" | |
| powershell -NoProfile -ExecutionPolicy Bypass -File "%REMOTE_SCRIPT%" %* | |
| set "EXIT_CODE=%errorlevel%" | |
| del /Q "%REMOTE_SCRIPT%" >nul 2>&1 |
| set "REMOTE_SCRIPT=%TEMP%\pichu-install-%RANDOM%%RANDOM%.ps1" | ||
| curl -fsSL "https://raw.githubusercontent.com/yeabwang/pichu/main/install.ps1" -o "%REMOTE_SCRIPT%" | ||
| rem Allow overriding the branch used to download install.ps1 (default: main) | ||
| if "%PICHU_INSTALL_BRANCH%"=="" set "PICHU_INSTALL_BRANCH=main" |
There was a problem hiding this comment.
PICHU_INSTALL_BRANCH is interpolated directly into a quoted URL. If it contains a double-quote or other metacharacters, it can break out of the quoted string and change the curl invocation (command injection / unexpected options). Please validate/sanitize PICHU_INSTALL_BRANCH to a safe branch/ref pattern (e.g., alphanumerics plus ._/-) before using it, or avoid direct interpolation by mapping known-safe values.
| if "%PICHU_INSTALL_BRANCH%"=="" set "PICHU_INSTALL_BRANCH=main" | |
| if "%PICHU_INSTALL_BRANCH%"=="" ( | |
| set "PICHU_INSTALL_BRANCH=main" | |
| ) else ( | |
| rem Validate PICHU_INSTALL_BRANCH to avoid command injection via curl URL | |
| rem Allow only: A-Z, a-z, 0-9, dot (.), underscore (_), slash (/), hyphen (-) | |
| rem Additionally, disallow a leading '-' to prevent it from being interpreted as an option | |
| set "PICHU_BRANCH_CAND=%PICHU_INSTALL_BRANCH%" | |
| echo(%PICHU_BRANCH_CAND%| findstr /R /I "^[A-Za-z0-9._/][A-Za-z0-9._/-]*$" >nul | |
| if errorlevel 1 ( | |
| echo Invalid PICHU_INSTALL_BRANCH value "%PICHU_INSTALL_BRANCH%". | |
| echo Allowed characters: A-Z, a-z, 0-9, dot (.), underscore (_), slash (/), hyphen (-), and it must not start with '-'. | |
| exit /b 1 | |
| ) | |
| ) |
Addresses a set of robustness and correctness issues in the Windows installer scripts surfaced during code review.
Summary
Fixes missing reinstall/upgrade flags, a null-deref risk in path resolution, misleading error messaging, and fragile temp-file handling in the CMD fallback.
install.ps1
Get-CommandPath: guard$command.Definitionagainst null/whitespace before passing toTest-PathSelect-Installererror: updated to list all supported managers (uv,pipx,pip3,pip) instead of onlyuvpipx install: added--forceto allow reinstall/repairpip3/pip install: added--upgrade --userto allow updates when already installed$LASTEXITCODE -ne 0 -or -not $?to catch both native-exe and PowerShell-level failuresinstall.cmd
PICHU_INSTALL_BRANCHenv var (defaults tomain), enabling pinning for CI or fork testing%RANDOM%tokens instead of two + locale-sensitive%TIME%substring-File+ post-hocdelwith a PowerShell-Commandtry-finallyblock so the temp file is removed even on abnormal exit (Ctrl+C, crash)Type of Change
Validation
Related Issues
Checklist
✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.