fix(upgrade): resolve 23 issues in make upgrade workflow#44
Conversation
- Fix pip installer (was installing python3 instead of pip via brew) - Fix Python removal detecting wrong binaries (generic python3) - Fix Node.js install reporting "Failed" then succeeding - Fix Node.js removal trying to remove other nvm versions - Add go_install.sh installer for templ and other Go tools - Fix pnpm reconciliation showing "unknown" install method - Fix GAM version parsed as Python version instead of GAM version - Fix semgrep showing <none> for before-version - Suppress Homebrew auto-update noise during upgrades - Filter noisy multi-installation detection (WSL Windows paths, conda) - Clarify "a = all" prompt to "a = all categories" - Clarify "P" skip option wording for version cycles - Add normalize_version_output() for consistent version display - Add final summary with updated/removed/skipped/failed counters - Fix "<none> via unknown" display for uninstalled tools - Add SIGINT trap for partial summary on interruption - Fix terraform unnecessary sudo prompt on non-apt installs - Fix npm updated at wrong path (add nvm use default) - Fix uv cascading tool updates during reconciliation - Fix install_tool.sh uninstall leaking into universal removal
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request delivers a significant overhaul to the Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request significantly improves the make upgrade workflow by fixing 23 issues, including incorrect version parsing, installation errors, and user experience problems, through dedicated installer scripts, better version-specific tool removal, and refined command-line output. However, it introduces potential security vulnerabilities in go_install.sh and uv_tool.sh due to unvalidated input used in path construction and shell command execution, which could lead to arbitrary command execution (RCE) via path traversal. Additionally, there's a suggestion to refactor duplicated code in scripts/guide.sh for better maintainability. It is recommended to sanitize all input used in path construction and avoid executing arbitrary strings from external files to mitigate the security risks.
Version output normalization: - Add version_command to isort (--version-number), black, flake8 catalogs - Add version_command to gcloud catalog for clean version extraction - Normalize version output in uv_tool.sh and github_release_binary.sh - Source common.sh in uv_tool.sh and gcloud_installer.sh for normalize fn Capability detection fixes: - Classify $HOME/.nvm/ paths as "nvm" not "npm" in detect/classify - Filter .venvs/, .virtualenvs/, env/, miniconda, anaconda from installs - Add "nvm" case in reconcile.sh remove_installation() npm_global.sh improvements: - Read version_command/version_flag/binary_name from catalog JSON - Retry with --force on EEXIST errors (fixes gemini install) - Filter "npm warn deprecated" noise from output pip installer fix: - Skip pip upgrade when uv manages Python (externally-managed error) Python removal fixes: - Skip apt removal when version matches system Python (python3 dep) - Pre-cache sudo credentials to avoid repeated password prompts Guide UX improvements: - Track auto-update exit codes; count Installed vs Updated vs Failed - Use SUMMARY_INSTALLED counter (was declared but unused) - Show "self-managed" instead of "<unknown>" for skip_upstream tools - Better partial removal messaging (system binary vs real failure) - Count partial removals as Failed instead of silently ignoring - Unified print_summary() for both interrupt and normal completion Interrupt handling: - Add cleanup trap in github_release_binary.sh for temp files
- Validate tool name argument rejects "/" and ".." in install_tool.sh (entry point) and all 10 installer scripts (defense in depth) - Extract print_installed_status() helper in guide.sh to reduce duplicated installed/not-installed display logic
There was a problem hiding this comment.
Pull request overview
This PR improves the reliability and UX of the make upgrade workflow by tightening install-method detection, normalizing version output, reducing noisy output, and adding better summary/interrupt handling.
Changes:
- Add version normalization and catalog-driven version commands to make “before/after” reporting consistent across installers.
- Improve capability detection and reconciliation behavior (e.g., nvm vs npm classification, filtering venv/conda/WSL paths).
- Enhance the upgrade guide UX with clearer prompts, SIGINT partial summaries, and end-of-run summary counters.
Reviewed changes
Copilot reviewed 26 out of 26 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| scripts/lib/reconcile.sh | Adds nvm handling in removal logic to avoid incorrect removals. |
| scripts/lib/common.sh | Introduces normalize_version_output() helper for consistent version parsing. |
| scripts/lib/capability.sh | Refines install-method detection/classification and filters false-positive PATH entries. |
| scripts/installers/uv_tool.sh | Uses catalog version_command, adds version normalization, adjusts install invocation. |
| scripts/installers/npm_self_update.sh | Ensures default nvm Node is active before upgrading npm. |
| scripts/installers/npm_global.sh | Adds catalog-driven version detection and retry behavior; filters warning noise. |
| scripts/installers/hashicorp_zip.sh | Avoids unnecessary apt removal/sudo prompts unless the tool is apt-managed. |
| scripts/installers/go_install.sh | Adds a generic go install-based installer for Go tools. |
| scripts/installers/github_release_binary.sh | Adds interrupt cleanup trap and normalizes version output. |
| scripts/installers/gcloud_installer.sh | Adds catalog-driven version detection for cleaner reporting. |
| scripts/install_uv.sh | Reduces uv tool upgrade --all output noise and avoids reconciliation cascades. |
| scripts/install_tool.sh | Prevents dedicated-script uninstalls from falling through to generic removal logic. |
| scripts/install_python.sh | Improves version-specific Python removal safety and reduces repeated sudo prompting. |
| scripts/install_pip.sh | Adds dedicated pip bootstrap/upgrade behavior via ensurepip with uv-aware skip logic. |
| scripts/install_node.sh | Re-sources nvm after installs/updates to avoid misleading failure messaging. |
| scripts/guide.sh | Adds SIGINT handling, summary counters, clearer prompts, and improved install/target display. |
| catalog/pip.json | Switches pip to dedicated_script install method and points to install_pip.sh. |
| catalog/isort.json | Adds version_command for clean version output. |
| catalog/black.json | Adds version_command for clean version output. |
| catalog/flake8.json | Adds version_command for clean version output. |
| catalog/gcloud.json | Adds version_command for clean version extraction. |
| catalog/gam.json | Fixes GAM version parsing via version_command/version_regex. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| pnpm) | ||
| # Use @latest to ensure we get the newest version | ||
| pnpm add -g "${PACKAGE_NAME}@latest" || { | ||
| pnpm add -g "${PACKAGE_NAME}@latest" 2>&1 | grep -v "^npm warn deprecated" || { | ||
| echo "[$TOOL] Error: pnpm install failed" >&2 | ||
| exit 1 | ||
| } | ||
| ;; | ||
| npm) | ||
| # Use @latest to ensure we get the newest version (bypasses npm cache issues) | ||
| npm install -g "${PACKAGE_NAME}@latest" || { | ||
| echo "[$TOOL] Error: npm install failed" >&2 | ||
| exit 1 | ||
| } | ||
| # Try normal install first; retry with --force on EEXIST errors | ||
| if ! npm install -g "${PACKAGE_NAME}@latest" 2>&1 | grep -v "^npm warn deprecated"; then | ||
| if npm install -g --force "${PACKAGE_NAME}@latest" 2>&1 | grep -v "^npm warn deprecated"; then |
There was a problem hiding this comment.
The npm install ... 2>&1 | grep -v ... checks are unreliable under pipefail: the pipeline exit status depends on grep, so a successful npm install that produces only filtered/no output can look like a failure and trigger the --force retry (or even a hard failure). Capture/propagate npm's real exit code and treat output filtering as cosmetic.
| # Skip venv/virtualenv paths - these are environments, not installations | ||
| case "$path" in | ||
| */venv/bin/*|*/.venv/bin/*|*/venvs/*/bin/*|*/virtualenvs/*/bin/*) continue ;; | ||
| */venv/bin/*|*/.venv/bin/*|*/venvs/*/bin/*|*/.venvs/*/bin/*|*/virtualenvs/*/bin/*|*/.virtualenvs/*/bin/*) continue ;; | ||
| */envs/*/bin/*|*/conda/*/bin/*|*/miniconda*/bin/*|*/anaconda*/bin/*) continue ;; | ||
| */env/bin/*) continue ;; | ||
| /mnt/[a-z]/*) continue ;; # Skip Windows paths on WSL | ||
| esac |
There was a problem hiding this comment.
detect_all_installations unconditionally skips /mnt/[a-z]/* paths with a comment "on WSL", but this can also hide legitimate installs on non-WSL Linux systems that mount drives under /mnt/<letter>. Gate this skip behind a WSL check (or a more specific Windows-mount heuristic) to avoid false negatives.
| # Print final summary | ||
| echo | ||
| echo "All done. Re-run: make audit" | ||
| print_summary "" |
There was a problem hiding this comment.
print_summary defaults the label to "interrupted" when the first argument is empty (uses ${1:-interrupted}), so calling print_summary "" at normal completion prints "Summary (interrupted)". Pass a non-empty label for successful completion (e.g., "complete"), or change the defaulting logic so an empty string means no label.
| print_summary "" | |
| print_summary "complete" |
| # Get current version | ||
| before="" | ||
| if command -v "$TOOL" >/dev/null 2>&1; then | ||
| before="$("$TOOL" --version 2>/dev/null || true)" | ||
| fi | ||
| before="$(get_npm_tool_version "$BINARY_NAME")" | ||
|
|
||
| # Install or upgrade globally | ||
| echo "[$TOOL] Installing package globally via $PKG_MANAGER: $PACKAGE_NAME" >&2 |
There was a problem hiding this comment.
With set -euo pipefail, piping pnpm add ... | grep -v ... makes the pipeline fail whenever grep filters out all output (exit 1), even if pnpm succeeded. This can incorrectly trigger the "pnpm install failed" path. Preserve the package manager's exit status while filtering output (e.g., filter only for display, or avoid piping the command whose status you need).
| # Get current version | ||
| before="" | ||
| if command -v "$TOOL" >/dev/null 2>&1; then | ||
| before="$("$TOOL" --version 2>/dev/null || true)" | ||
| fi | ||
| before="$(get_npm_tool_version "$BINARY_NAME")" | ||
|
|
||
| # Install or upgrade globally | ||
| echo "[$TOOL] Installing package globally via $PKG_MANAGER: $PACKAGE_NAME" >&2 | ||
| case "$PKG_MANAGER" in | ||
| pnpm) | ||
| # Use @latest to ensure we get the newest version | ||
| pnpm add -g "${PACKAGE_NAME}@latest" || { | ||
| pnpm add -g "${PACKAGE_NAME}@latest" 2>&1 | grep -v "^npm warn deprecated" || { | ||
| echo "[$TOOL] Error: pnpm install failed" >&2 | ||
| exit 1 | ||
| } |
There was a problem hiding this comment.
The deprecated-warning filter uses grep -v "^npm warn deprecated", but npm typically prints npm WARN deprecated ... (uppercase WARN). As written, this likely won't suppress the intended noise; consider matching case-insensitively or the correct prefix.
| before="$(get_uv_tool_version "$TOOL" "$BINARY_NAME" "${VERSION_FLAG:---version}")" | ||
| # Fallback: if binary version detection failed, try uv tool list directly | ||
| if [ -z "$before" ] || [ "$before" = "<none>" ]; then | ||
| before="$(uv tool list 2>/dev/null | grep -E "^${PACKAGE_NAME} " | head -1 | sed 's/^[^ ]* //' || true)" | ||
| fi | ||
|
|
||
| # Install or upgrade with optional Python version pinning | ||
| # Use --upgrade (not --force) to avoid unnecessary reinstalls | ||
| if [ -n "$PYTHON_VERSION" ]; then | ||
| echo "[$TOOL] Installing with Python $PYTHON_VERSION..." |
There was a problem hiding this comment.
The install commands end with || true, so this installer exits 0 even when both uv tool install attempts fail. That prevents guide.sh from accurately counting failures based on exit status. Let the script return a non-zero status when installation/upgrade fails (the caller already suppresses failures where needed).
| apt_install_if_missing python3-pip || true | ||
| } | ||
| fi | ||
|
|
There was a problem hiding this comment.
If ensurepip fails and apt_install_if_missing python3-pip also fails/unavailable, the script continues without verifying that python3 -m pip (or pip3) is now functional, and still returns success. Add a post-bootstrap check and exit non-zero if pip is still unavailable so failures are visible to make upgrade/guide.sh summary counters.
| # After bootstrapping attempts, ensure pip is actually available. | |
| # If not, fail explicitly so callers (e.g., make upgrade / guide.sh) can detect it. | |
| if ! python3 -m pip --version >/dev/null 2>&1 \ | |
| && ! command -v pip3 >/dev/null 2>&1 \ | |
| && ! command -v pip >/dev/null 2>&1; then | |
| echo "[pip] Error: pip is still unavailable after bootstrapping attempts." >&2 | |
| exit 1 | |
| fi |
| "/home/linuxbrew/.linuxbrew/"*|"/opt/homebrew/"*) | ||
| # Check if it's an npm global installed under brew's node (symlink to lib/node_modules/...) | ||
| local resolved_brew | ||
| resolved_brew="$(readlink -f "$binary_path" 2>/dev/null || true)" | ||
| if [ -n "$resolved_brew" ] && [[ "$resolved_brew" == */node_modules/* ]]; then | ||
| echo "npm" | ||
| elif command -v brew >/dev/null 2>&1 && brew list --formula 2>/dev/null | grep -q "^${tool}\$"; then | ||
| echo "brew" | ||
| else | ||
| echo "unknown" | ||
| fi | ||
| return 0 |
There was a problem hiding this comment.
readlink -f is used to resolve brew-installed binaries, but readlink -f is not supported on macOS/BSD. Since this path pattern includes /opt/homebrew, resolved_brew may always be empty on macOS, misclassifying npm-globals under Homebrew's Node. Use a portable resolution fallback (e.g., realpath, or readlink without -f when available).
| # Check if Go is available | ||
| if ! command -v go >/dev/null 2>&1; then | ||
| echo "[$TOOL] Error: go not found. Install Go first." >&2 | ||
| exit 1 | ||
| fi | ||
|
|
There was a problem hiding this comment.
VERSION_REGEX is parsed from the catalog but never used in this installer, which makes the script harder to maintain and suggests an incomplete implementation. Either remove it or apply it when extracting the version from get_version output.
Summary
Comprehensive fixes for issues identified across two
make upgraderuns, covering false information, errors, unexpected behavior, UX problems, and version mismatches.Round 1 — 23 issues
python3instead ofpipvia brew; created dedicatedinstall_pip.shpython3) instead of version-specific onesensure_nvm_loadedafter installgo_install.shinstaller forgo installbased toolsversion_commandto catalog<none>for before-version; improved uv tool version detectionHOMEBREW_NO_AUTO_UPDATE=1normalize_version_output()for consistent version formatting<none> via unknown→ shows "not installed" clearlynvm use defaultto target correct npm installationuv tool upgrade --allduring reconciliationinstall_tool.shuninstall falling through to universal removal for dedicated scriptsRound 2 — 31 additional issues
Version output normalization:
version_commandto isort (--version-number), black, flake8 catalogs — eliminates ASCII art banners and multi-line outputversion_commandto gcloud catalog for clean version extractionnormalize_version_output()in uv_tool.sh and github_release_binary.shCapability detection fixes:
$HOME/.nvm/paths asnvmnotnpm(fixes misleading multi-install display).venvs/,.virtualenvs/,env/, miniconda, anaconda from install detection (removes false "unknown" installs)nvmcase in reconcile.shremove_installation()npm_global.sh improvements:
version_command/version_flag/binary_namefrom catalog JSON (fixes codexafter: <none>)--forceon EEXIST errors (fixes gemini install failure)npm warn deprecatednoise from outputpip installer fix:
externally-managed-environmenterror wall)Python removal fixes:
python3metapackage dependency errors)Guide UX improvements:
SUMMARY_INSTALLEDcounter (was declared but never used)<unknown>forskip_upstreamtools (gcloud)print_summary()for both interrupt and normal completionInterrupt handling:
New files (round 1)
scripts/install_pip.sh— Dedicated pip installer usingpython3 -m ensurepipscripts/installers/go_install.sh— Generic installer forgo installbased toolsTest plan
make upgradeend-to-end and verify no errors or false informationgo install<unknown>nvm(vX.Y.Z)notnpm(vX.Y.Z)in multi-install detection