Conversation
Summary of ChangesHello @fortes, 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 focuses on enhancing the reliability and efficiency of various installation scripts within the repository. By introducing idempotency checks for GitHub and Node.js package installations, and strengthening the Highlights
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 introduces several useful fixes, primarily adding idempotency checks to installation scripts to avoid re-installing software that is already present. The changes are generally good, but I've found a few critical and high-severity issues in the implementation of these checks that could cause them to fail or behave incorrectly. My review includes suggestions to fix a critical bug in the stow script that would prevent it from working, and to correct faulty logic in the package-checking mechanisms in install_github_packages and install_node_packages.
script/stow
Outdated
| local packages=() | ||
| while IFS= read -r -d '' package; do | ||
| packages+=("${package}") | ||
| done < <(find "${stowed_files_path}" -mindepth 1 -maxdepth 1 -type d -print0) |
There was a problem hiding this comment.
The find command currently returns the full path for each package directory. However, stow expects only the package names (i.e., the directory basenames) relative to the directory specified with --dir. Passing full paths will cause the stow command to fail with an error like stow: ERROR: The stow directory ... does not contain package ....
You can use find's -printf '%f\0' action to print only the filename, which is more efficient than calling basename in a loop.
done < <(find "${stowed_files_path}" -mindepth 1 -maxdepth 1 -type d -printf '%f\0')
script/install_github_packages
Outdated
| local install_location="$HOME/.local/share/lua-language-server" | ||
|
|
||
| # Check if already installed | ||
| if [[ -d "${install_location}" ]] && [[ -n "$(find "${install_location}" -type f -name "bin/*" 2>/dev/null)" ]]; then |
There was a problem hiding this comment.
The find command used to check if lua-language-server is already installed is incorrect. The -name "bin/*" argument will cause find to look for a file literally named bin/*, which is unlikely to exist. This means the check will always fail, and the script will attempt to reinstall the package on every run.
A more correct and efficient approach would be to check for the specific executable, as its path within the extracted archive is predictable.
if [[ -x "${install_location}/bin/lua-language-server" ]]; then
| local missing_packages=() | ||
| for package in "${PACKAGES[@]}"; do | ||
| # Extract package name (remove @scope/ if present) | ||
| local package_name="${package#*/}" | ||
| package_name="${package_name%%@*}" # Remove version if present | ||
|
|
||
| if ! pnpm list --global | grep -q "${package_name}"; then | ||
| missing_packages+=("${package}") | ||
| fi | ||
| done |
There was a problem hiding this comment.
The current logic for checking installed node packages is inefficient and can be unreliable.
- Inefficiency:
pnpm list --globalis executed inside theforloop, which is inefficient for a large number of packages. - Incorrect Package Name: Stripping the scope from packages (e.g.,
@foo/barbecomesbar) withlocal package_name="${package#*/}"is incorrect and can lead to name collisions. - False Positives: The
grepcommand can match substrings, leading to false positives. For example, checking forromecould incorrectly match an installed package namedchrome.
A more robust approach is to fetch the list of packages once, not strip the scope, and use a more precise grep to match the full package name at the beginning of a line.
local missing_packages=()
# Get a flat list of globally installed packages once to be efficient.
# Redirect stderr to avoid errors if no packages are installed, and use `|| true` to prevent script exit.
local installed_list
installed_list="$(pnpm list --global --depth=0 2>/dev/null || true)"
for package in "${PACKAGES[@]}"; do
# Extract package name, removing only the version specifier.
local package_name_no_version="${package%%@*}"
# Check for an exact match at the beginning of a line, followed by a space.
if ! echo "${installed_list}" | grep -q -E "^${package_name_no_version} "; then
missing_packages+=("${package}")
fi
done
There was a problem hiding this comment.
Pull request overview
This PR adds robustness improvements and idempotency checks to installation scripts, allowing them to skip already-installed packages and handle error conditions more gracefully.
- Enhanced validation with checks for required commands and directories
- Added skip logic to avoid reinstalling already-present packages
- Improved error handling with clearer error messages and early returns
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| script/stow | Adds stow command availability check, converts warning to error for missing stowed-files directory, validates target directory, and uses find to build package list instead of ls |
| script/install_node_packages | Adds check to skip Node LTS installation if already present, implements package-by-package checking to only install missing global packages |
| script/install_github_packages | Adds existence checks to skip installation in install_bin, install_tarball, install_zip, install_lua_language_server, install_neovim_stable, and install_yazi functions |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
script/install_node_packages
Outdated
| if [[ "$(uname -m)" == "aarch64" ]]; then | ||
| fnm install --lts --arch arm64 | ||
| # Check if node LTS is already installed | ||
| if fnm list | grep -q "lts/latest"; then |
There was a problem hiding this comment.
The grep pattern "lts/latest" doesn't match the actual output format of fnm list. The command fnm list typically shows versions like v20.11.0 lts-latest or v20.11.0 (lts/iron), not lts/latest.
Consider using a more robust check:
if fnm list | grep -q "lts-latest"; thenOr better yet, check if any LTS version is installed:
if fnm list | grep -qE '\(lts/|lts-latest'; then| if fnm list | grep -q "lts/latest"; then | |
| if fnm list | grep -qE "\(lts/|lts-latest"; then |
script/install_node_packages
Outdated
| # Extract package name (remove @scope/ if present) | ||
| local package_name="${package#*/}" | ||
| package_name="${package_name%%@*}" # Remove version if present |
There was a problem hiding this comment.
The package name extraction logic is incorrect for scoped packages. For a package like @anthropic-ai/claude-code:
- Line 83
${package#*/}removes everything before the first/, givingclaude-code - But for unscoped packages like
neovim, it returnsneovimunchanged (correct) - This breaks the grep check on line 86 for unscoped packages
The logic should handle both scoped and unscoped packages:
local package_name="${package%%@*}" # Remove version if present
if [[ "${package_name}" == *"/"* ]]; then
package_name="${package_name#*/}" # Remove scope for scoped packages
fi| # Extract package name (remove @scope/ if present) | |
| local package_name="${package#*/}" | |
| package_name="${package_name%%@*}" # Remove version if present | |
| # Extract package name (handle scoped/unscoped, remove version if present) | |
| local package_name="${package%%@*}" # Remove version if present | |
| if [[ "${package_name}" == *"/"* ]]; then | |
| package_name="${package_name#*/}" # Remove scope for scoped packages | |
| fi |
script/install_github_packages
Outdated
| local install_location="$HOME/.local/share/lua-language-server" | ||
|
|
||
| # Check if already installed | ||
| if [[ -d "${install_location}" ]] && [[ -n "$(find "${install_location}" -type f -name "bin/*" 2>/dev/null)" ]]; then |
There was a problem hiding this comment.
The find command has an incorrect -name pattern. The pattern "bin/*" will never match because -name only matches the basename of files, not paths. This means the check will always fail to detect an existing installation.
The pattern should be:
if [[ -d "${install_location}" ]] && [[ -f "${install_location}/bin/lua-language-server" ]]; thenAlternatively, if you want to check for any files in the bin directory:
if [[ -d "${install_location}/bin" ]] && [[ -n "$(find "${install_location}/bin" -type f 2>/dev/null)" ]]; then| if [[ -d "${install_location}" ]] && [[ -n "$(find "${install_location}" -type f -name "bin/*" 2>/dev/null)" ]]; then | |
| if [[ -d "${install_location}/bin" ]] && [[ -n "$(find "${install_location}/bin" -type f 2>/dev/null)" ]]; then |
| # Get list of packages to stow | ||
| local packages=() | ||
| while IFS= read -r -d '' package; do | ||
| packages+=("${package}") |
There was a problem hiding this comment.
The find command returns full paths (e.g., /path/to/stowed-files/package1), but stow expects package names (basenames) relative to the --dir argument. This will cause stow to fail because it won't be able to find packages with full paths.
Fix by extracting the basename:
while IFS= read -r -d '' package; do
packages+=("$(basename "${package}")")
done < <(find "${stowed_files_path}" -mindepth 1 -maxdepth 1 -type d -print0)| packages+=("${package}") | |
| packages+=("$(basename "${package}")") |
1 participant
No description provided.