Skip to content

fix: skip installed npm globals and prevent nvim lua typechange #1380

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
shunkakinoki merged 3 commits into from
Apr 6, 2026
Merged

fix: skip installed npm globals and prevent nvim lua typechange #1380

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
9ef3523
fix: skip installed npm globals and prevent nvim lua typechange
shunkakinoki Apr 6, 2026
013c624
fix: simplify version extraction for npm dependencies
shunkakinoki Apr 6, 2026
a123e8b
fix: update clipboard paste tests to include bash directory in PATH
shunkakinoki Apr 6, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
42 changes: 35 additions & 7 deletions home-manager/modules/npm-globals/install-npm-globals.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,21 +42,49 @@ if [ -n "$TRUSTED_DEPS" ]; then
done
fi

# Install global packages in batches (bun hangs when resolving too many at once)
DEPS=$(jq -r '.dependencies | keys[]' "$PACKAGE_JSON" 2>/dev/null || true)
# Build list of packages that need installing or updating
GLOBAL_MODULES="${HOME}/.bun/install/global/node_modules"
DEPS=$(jq -r '.dependencies | to_entries[] | "\(.key)=\(.value)"' "$PACKAGE_JSON" 2>/dev/null || true)
MISSING=()
Comment on lines +45 to +48

Copilot AI Apr 6, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

GLOBAL_MODULES is introduced here but the same variable is redefined later in the script for the override/dedup logic. Consider defining it once near the top (or making it readonly) to avoid accidental divergence if one path is changed later.

Copilot uses AI. Check for mistakes.
if [ -n "$DEPS" ]; then
BATCH_SIZE=10
while IFS= read -r entry; do
dep="${entry%%=*}"
wanted="${entry#*=}"
# Extract minimum version from semver spec (e.g. "^2.1.92" -> "2.1.92")
wanted_ver="${wanted//[^0-9.]/}"
installed_ver=""
pkg_json="${GLOBAL_MODULES}/${dep}/package.json"
if [ -f "$pkg_json" ]; then
installed_ver=$(jq -r '.version // empty' "$pkg_json" 2>/dev/null || true)
fi
if [ -n "$installed_ver" ] && [ -n "$wanted_ver" ] && [ "$installed_ver" = "$wanted_ver" ]; then
echo "$dep@$installed_ver already installed, skipping"
elif [ -n "$installed_ver" ]; then
echo "$dep@$installed_ver installed, want $wanted_ver, updating"
MISSING+=("$dep")
Comment on lines +53 to +64

Copilot AI Apr 6, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wanted_ver=$(echo "$wanted" | sed 's/[^0-9.]//g') is not a valid way to interpret semver specs. For common ranges like ^1.2.3, an installed 1.2.4 (which satisfies the range) will be treated as needing an update; for specs like *, latest, workspace:*, or prereleases (1.2.3-beta.1), this produces empty/incorrect versions and can lead to always-updating behavior and confusing logs. Use a comparison that checks whether the installed version satisfies the requested semver range (or avoid parsing and delegate to the package manager).

Copilot uses AI. Check for mistakes.
Comment on lines +53 to +64

@coderabbitai coderabbitai Bot Apr 6, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

鈿狅笍 Potential issue | 馃煚 Major

馃З Analysis chain

馃弫 Script executed:

#!/bin/bash
set -euo pipefail

# Verify which dependency specs are mangled by the current normalization logic.
python - <<'PY'
import json, re, pathlib
pkg = pathlib.Path("package.json")
deps = json.loads(pkg.read_text()).get("dependencies", {})
print("name\tspec\tcurrent_wanted_ver")
for name, spec in deps.items():
    wanted_ver = re.sub(r"[^0-9.]", "", spec)  # mirrors Line 54
    if wanted_ver != spec.lstrip("^~"):  # quick signal of lossy transform
        print(f"{name}\t{spec}\t{wanted_ver}")
PY

Repository: shunkakinoki/dotfiles

Length of output: 275

馃弫 Script executed:

# Check file existence and line count
wc -l "home-manager/modules/npm-globals/install-npm-globals.sh"

Repository: shunkakinoki/dotfiles

Length of output: 124

馃弫 Script executed:

# Read lines 45-90 to see the context and verify the code snippet
sed -n '45,90p' "home-manager/modules/npm-globals/install-npm-globals.sh"

Repository: shunkakinoki/dotfiles

Length of output: 1708

馃弫 Script executed:

# Check if file exists at the stated path
ls -la "home-manager/modules/npm-globals/install-npm-globals.sh"

Repository: shunkakinoki/dotfiles

Length of output: 169

Semver parsing loses prerelease and dev version information, causing unnecessary package updates.

Line 54's sed pattern s/[^0-9.]//g strips all non-numeric characters, corrupting dependency specs like ^0.1.0-alpha.1743007075 (becomes 0.1.0.1743007075), ^0.0.1775390627-g406649 (becomes 0.0.1775390627406649), and ^7.0.0-dev.20260405.1 (becomes 7.0.0.20260405.1). Line 60's strict equality check then compares the installed version against these corrupted values, causing packages with prerelease or dev versions to be incorrectly identified as needing updates.

Preserve the full version spec without lossy character stripping to ensure accurate version comparison.

馃 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@home-manager/modules/npm-globals/install-npm-globals.sh` around lines 53 -
64, The code strips prerelease/build metadata by using wanted_ver=$(echo
"$wanted" | sed 's/[^0-9.]//g'); instead, preserve the full semver (including
prerelease/dev identifiers) by only removing leading range operators and
optional leading "v". Replace the wanted_ver assignment with a safe
normalization that strips only leading ^ ~ > < = and whitespace (e.g.
wanted_ver=$(echo "$wanted" | sed 's/^[\^~><=[:space:]]*//; s/^v//')) and ensure
you apply the same normalization to installed_ver (from package.json) before the
equality check in the install-npm-globals.sh logic so comparisons like in the
if/elif that reference wanted_ver and installed_ver correctly detect
prerelease/dev versions.

else
MISSING+=("$dep")
fi
done <<<"$DEPS"
fi

# Install missing packages in small batches
if [ "${#MISSING[@]}" -gt 0 ]; then
echo "Installing ${#MISSING[@]} missing packages..."
BATCH_SIZE=5
BATCH=()
while IFS= read -r dep; do
for dep in "${MISSING[@]}"; do
BATCH+=("$dep")
if [ "${#BATCH[@]}" -ge "$BATCH_SIZE" ]; then
bun add --global "${BATCH[@]}" 2>/dev/null || true
bun add --global "${BATCH[@]}" 2>/dev/null || echo "Batch install failed: ${BATCH[*]}"

@gemini-code-assist gemini-code-assist Bot Apr 6, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Using echo for error reporting in a script that uses set -e (via set -euo pipefail) is good, but consider redirecting the error message to stderr using >&2 to ensure it is captured correctly by logging systems or when the output is redirected.

Suggested change
bun add --global "${BATCH[@]}" 2>/dev/null || echo "Batch install failed: ${BATCH[*]}"
bun add --global "${BATCH[@]}" 2>/dev/null || echo "Batch install failed: ${BATCH[*]}" >&2

BATCH=()
fi
done <<<"$DEPS"
done
if [ "${#BATCH[@]}" -gt 0 ]; then
bun add --global "${BATCH[@]}" 2>/dev/null || true
bun add --global "${BATCH[@]}" 2>/dev/null || echo "Batch install failed: ${BATCH[*]}"

@gemini-code-assist gemini-code-assist Bot Apr 6, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Similar to the previous batch install, redirect the error message to stderr for consistency.

Suggested change
bun add --global "${BATCH[@]}" 2>/dev/null || echo "Batch install failed: ${BATCH[*]}"
bun add --global "${BATCH[@]}" 2>/dev/null || echo "Batch install failed: ${BATCH[*]}" >&2

fi
Comment on lines +47 to 85

Copilot AI Apr 6, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The install step ignores the requested version/range from package.json: MISSING stores only dep, and bun add --global "${BATCH[@]}" installs latest. That makes the earlier version check ineffective and can cause repeated installs/updates even when the installed version already satisfies the requested spec. Consider carrying the original spec into the install list (e.g., install name@<spec>), and base the skip/update decision on the same spec you pass to bun.

Copilot uses AI. Check for mistakes.
Comment on lines +79 to 85

@coderabbitai coderabbitai Bot Apr 6, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

鈿狅笍 Potential issue | 馃煚 Major

馃З Analysis chain

馃弫 Script executed:

fd -t f "install-npm-globals.sh" --exec head -100 {}

Repository: shunkakinoki/dotfiles

Length of output: 3494

馃弫 Script executed:

sed -n '80,120p' home-manager/modules/npm-globals/install-npm-globals.sh

Repository: shunkakinoki/dotfiles

Length of output: 1653

Batch install errors are silently suppressed, leaving packages half-installed.

The bun add ... || echo ... pattern bypasses set -e. When bun add fails, the || catches it and executes echo, which returns 0鈥攕o the overall expression succeeds and the script continues without detecting the failure. No subsequent code validates installation success, so downstream operations proceed on incomplete state.

Apply the proposed fix: track failures with a FAILED variable, and exit non-zero at the end of the installation block if any batch failed.

馃 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@home-manager/modules/npm-globals/install-npm-globals.sh` around lines 79 -
85, The bun batch-install blocks currently swallow errors via "bun add ... ||
echo ...", so introduce a FAILED flag (e.g., FAILED=0) before the loop, replace
each "|| echo ..." with logic that sets FAILED=1 on failure (so the failure is
recorded) while still printing the error message, and after the final batch
check examine FAILED and exit non-zero (exit 1) if any batch failed; update both
occurrences that run "bun add --global \"${BATCH[@]}\"" and ensure the BATCH
reset (BATCH=()) remains unchanged.

else
echo "All npm global packages already installed"
fi

# Apply dependency overrides to the global install
Expand Down
1 change: 0 additions & 1 deletion home-manager/programs/neovim/default.nix
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,6 @@ in

home.file.".config/nvim/lua" = {
source = ./lua;
recursive = true;
force = true;
};

Expand Down
15 changes: 12 additions & 3 deletions spec/clipboard_paste_spec.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,13 +36,15 @@ setup() {
MOCK_BIN="$(mktemp -d)"
MOCK_ORIGINAL_PATH="${PATH:-}"
export MOCK_BIN MOCK_ORIGINAL_PATH
export PATH="$MOCK_BIN:$MOCK_ORIGINAL_PATH"
export WAYLAND_DISPLAY=wayland-0
cat >"$MOCK_BIN/wl-paste" <<'EOF'
#!/usr/bin/env bash
printf 'hello'
EOF
chmod +x "$MOCK_BIN/wl-paste"
local bash_dir
bash_dir="$(dirname "$(readlink -f "$(command -v bash)")")"
export PATH="$MOCK_BIN:$bash_dir"
}
cleanup() {
export PATH="$MOCK_ORIGINAL_PATH"
Expand All @@ -61,16 +63,23 @@ End

Describe 'when xclip is available'
setup() {
mock_bin_setup xclip
MOCK_BIN="$(mktemp -d)"
MOCK_ORIGINAL_PATH="${PATH:-}"
export MOCK_BIN MOCK_ORIGINAL_PATH
unset WAYLAND_DISPLAY
cat >"$MOCK_BIN/xclip" <<'EOF'
#!/usr/bin/env bash
printf 'hello'
EOF
chmod +x "$MOCK_BIN/xclip"
local bash_dir
bash_dir="$(dirname "$(readlink -f "$(command -v bash)")")"
export PATH="$MOCK_BIN:$bash_dir"
}
cleanup() {
mock_bin_cleanup
export PATH="$MOCK_ORIGINAL_PATH"
rm -rf "$MOCK_BIN"
unset MOCK_BIN MOCK_ORIGINAL_PATH
}
Before 'setup'
After 'cleanup'
Expand Down
58 changes: 51 additions & 7 deletions spec/npm_globals_spec.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,25 +68,69 @@ The output should include 'exit 0'
End
End

Describe 'package installation'
Describe 'trusted dependencies'
It 'reads trustedDependencies from package.json'
When run bash -c "grep 'trustedDependencies' '$SCRIPT'"
The output should include 'trustedDependencies'
End

It 'trusts postinstall scripts before installing'
When run bash -c "grep 'bun pm -g trust' '$SCRIPT'"
The output should include 'bun pm -g trust'
End
End

Describe 'skip already installed'
It 'checks global node_modules for installed packages'
When run bash -c "grep 'GLOBAL_MODULES=' '$SCRIPT' | head -1"
The output should include '.bun/install/global/node_modules'
End

It 'reads installed version from package.json'
When run bash -c "grep 'installed_ver' '$SCRIPT'"
The output should include 'installed_ver'
End

It 'extracts wanted version from semver spec'
When run bash -c "grep 'wanted_ver' '$SCRIPT'"
The output should include 'wanted_ver'
End

It 'skips when installed version matches wanted'
When run bash -c "grep 'already installed, skipping' '$SCRIPT'"
The output should include 'already installed, skipping'
End

It 'detects when update is needed'
When run bash -c "grep 'updating' '$SCRIPT'"
The output should include 'updating'
End

It 'builds a MISSING array of packages to install'
When run bash -c "grep 'MISSING' '$SCRIPT'"
The output should include 'MISSING'
End
End

Describe 'batch installation'
It 'uses batch size of 5'
When run bash -c "grep 'BATCH_SIZE=5' '$SCRIPT'"
The output should include 'BATCH_SIZE=5'
End

It 'uses bun add --global in batches'
When run bash -c "grep 'bun add --global' '$SCRIPT'"
The output should include 'bun add --global'
End

It 'batches packages to avoid bun resolution hangs'
When run bash -c "grep 'BATCH_SIZE' '$SCRIPT'"
The output should include 'BATCH_SIZE'
It 'reports batch failures'
When run bash -c "grep 'Batch install failed' '$SCRIPT'"
The output should include 'Batch install failed'
End

It 'parses dependencies with jq'
When run bash -c "grep 'dependencies' '$SCRIPT'"
The output should include 'dependencies'
It 'reports when all packages are already installed'
When run bash -c "grep 'All npm global packages already installed' '$SCRIPT'"
The output should include 'All npm global packages already installed'
End
Comment on lines +83 to 134

@coderabbitai coderabbitai Bot Apr 6, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

鈿狅笍 Potential issue | 馃煚 Major

These new cases validate strings, not behavior, so skip/update logic is effectively untested.

The added specs (installed_ver, wanted_ver, MISSING, updating, All npm global packages already installed) only assert text exists in the script. They do not execute install-npm-globals.sh with controlled fixtures (HOME, fake global node_modules, fixture package.json), so regressions in actual decision logic will still pass.

If you want, I can draft a fixture-based ShellSpec block that covers:

  1. all deps preinstalled => zero installs, skip message;
  2. one dep removed => exactly one dep installed.
馃 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@spec/npm_globals_spec.sh` around lines 83 - 134, The new ShellSpec tests only
grep for literal strings (e.g., 'installed_ver', 'wanted_ver', 'MISSING',
'updating', 'All npm global packages already installed') and thus don't verify
actual behavior of install-npm-globals.sh; update the spec to run the script
with controlled fixtures instead: create a temporary HOME and a fake global
node_modules structure and fixture package.json, then execute
install-npm-globals.sh and assert real outcomes (zero installs and "All npm
global packages already installed" when all deps present; exactly one install
and appropriate "updating"/batch install output when one dep is missing). Keep
the existing string checks for BATCH_SIZE=5 and 'bun add --global' but replace
the greps for internal variable names with executable ShellSpec examples that
source or run install-npm-globals.sh against the fixtures to validate
skip/update logic.

End

Expand Down
Loading