Fix/installer - #21
Conversation
There was a problem hiding this comment.
Pull request overview
This pull request refactors the Windows installation scripts to improve flexibility and reliability. The main change is removing the automatic installation of uv and instead supporting multiple package managers (uv, pipx, pip3, pip) with a fallback chain. The install.cmd script now also supports downloading the PowerShell script from GitHub if a local copy isn't available.
Changes:
- Modified
Select-Installerto detect available package managers instead of auto-installing uv - Enhanced
Install-Pichuto support multiple installers (uv, pipx, pip3, pip) with appropriate command syntax for each - Added defensive property checks in
Get-CommandPathto handle different command object types - Updated install.cmd to support both local and remote script execution with automatic fallback
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 10 comments.
| File | Description |
|---|---|
| install.ps1 | Refactored installer selection to support multiple package managers; added defensive checks for command properties; added exit code validation |
| install.cmd | Added logic to download and execute remote install.ps1 if local copy doesn't exist; improved error handling and temp file cleanup |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
|
@copilot open a new pull request to apply changes based on the comments in this thread |
|
@copilot open a new pull request to apply changes based on the comments in this thread |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 6 changed files in this pull request and generated 9 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| ## [0.1.3] - 2026-02-21 | ||
|
|
||
| ### Security | ||
| - fixed one line installers security issues. |
There was a problem hiding this comment.
Changelog entry is grammatically incorrect and inconsistent with the style of the rest of the file. Consider: "Fixed one-line installer security issues." (capitalization + hyphenation, and no trailing period if following the existing bullet style).
| - fixed one line installers security issues. | |
| - Fixed one-line installer security issues. |
|
|
||
| Ensure-PathEntry -Dir $pathTargetDir -Skip:$NoModifyPath | ||
| Ensure-AliasEntry $Alias | ||
| Set-PathEntry -Dir $pathTargetDir -Skip:($NoModifyPath.IsPresent) |
There was a problem hiding this comment.
This call uses $NoModifyPath.IsPresent, but $NoModifyPath may have been reassigned from the env var to a Boolean earlier, which would make .IsPresent throw at runtime. Pass a boolean explicitly (e.g., -Skip:$NoModifyPath or -Skip:$skip) instead of dereferencing .IsPresent here.
| Set-PathEntry -Dir $pathTargetDir -Skip:($NoModifyPath.IsPresent) | |
| Set-PathEntry -Dir $pathTargetDir -Skip:$NoModifyPath |
| if [ "$SHELL_KIND" = "fish" ]; then | ||
| printf 'fish_add_path "%s"' "$escaped_path" | ||
| printf "fish_add_path '%s'" "$safe_dir" | ||
| else | ||
| printf 'export PATH="%s:$PATH"' "$escaped_path" | ||
| printf "export PATH='%s:\$PATH'" "$safe_dir" | ||
| fi |
There was a problem hiding this comment.
get_path_command emits export PATH='...:$PATH' using single quotes, so $PATH will not expand when sourced and PATH will be set to a literal string containing $PATH. Use quoting that preserves expansion of the existing PATH (e.g., double quotes or concatenated quoting).
| printf '%s\n' "$path_cmd" | ||
| printf '# <<< pichu path <<<\n' | ||
| } >>"$PROFILE_FILE" | ||
| } > "$tmp" |
There was a problem hiding this comment.
With set -o noclobber enabled, this } > "$tmp" write will fail because mktemp already created the file. Use a clobber-override redirect (>|) or temporarily disable noclobber for this write.
| [Net.ServicePointManager]::SecurityProtocol = [Net.SecurityProtocolType]::Tls12 | ||
|
|
||
| # ── Honour env override for NoModifyPath ───────────────────────────────────── | ||
| if ($env:PICHU_NO_MODIFY_PATH -eq '1') { $NoModifyPath = $true } |
There was a problem hiding this comment.
$NoModifyPath is set to $true based on the PICHU_NO_MODIFY_PATH env var. Since PowerShell variables are not type-fixed, this can turn $NoModifyPath into a plain Boolean, which later breaks usages of $NoModifyPath.IsPresent. Prefer computing a separate $skip Boolean (or keep $NoModifyPath as a SwitchParameter) and avoid relying on .IsPresent after reassignment.
| if ($env:PICHU_NO_MODIFY_PATH -eq '1') { $NoModifyPath = $true } | |
| if ($env:PICHU_NO_MODIFY_PATH -eq '1') { [System.Management.Automation.SwitchParameter]$NoModifyPath = $true } |
| powershell.exe -NoProfile -NonInteractive -ExecutionPolicy Bypass ^ | ||
| -File "%REMOTE_SCRIPT%" |
There was a problem hiding this comment.
The downloaded install.ps1 invocation does not forward %*, so CMD arguments are dropped. Append %* after the -File invocation to preserve passed arguments.
| printf 'end\n' | ||
| printf '# <<< pichu alias <<<\n' | ||
| } >>"$PROFILE_FILE" | ||
| } > "$tmp" |
There was a problem hiding this comment.
Same issue as above: mktemp creates $tmp, and set -o noclobber makes > "$tmp" fail. Use >| or disable noclobber around this redirect.
| printf "alias %s='%s'\n" "$REQUESTED_ALIAS" "$safe_target" | ||
| printf '# <<< pichu alias <<<\n' | ||
| } >>"$PROFILE_FILE" | ||
| } > "$tmp" |
There was a problem hiding this comment.
Same issue as above: mktemp creates $tmp, and set -o noclobber makes > "$tmp" fail. Use >| or disable noclobber around this redirect.
| if exist "%LOCAL_SCRIPT%" ( | ||
| powershell.exe -NoProfile -NonInteractive -ExecutionPolicy Bypass ^ | ||
| -File "%LOCAL_SCRIPT%" | ||
| exit /b %errorlevel% |
There was a problem hiding this comment.
The local install.ps1 invocation does not forward %*, so CMD arguments are dropped (regression vs previous version). Append %* after the -File invocation so users can pass through PowerShell parameters.
Summary
fixing the one installer