Skip to content

fix: skip already-installed npm globals to prevent bun hang #1379

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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
shunkakinoki wants to merge 1 commit into from
Closed

fix: skip already-installed npm globals to prevent bun hang #1379

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
28 changes: 22 additions & 6 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,37 @@ if [ -n "$TRUSTED_DEPS" ]; then
done
fi

# Install global packages in batches (bun hangs when resolving too many at once)
# Build list of packages that are not yet installed
GLOBAL_MODULES="${HOME}/.bun/install/global/node_modules"

@gemini-code-assist gemini-code-assist Bot Apr 5, 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

The GLOBAL_MODULES path is hardcoded to $HOME/.bun. It is better to respect the BUN_INSTALL environment variable if it is set, as this variable is explicitly configured in the Nix module and ensures consistency across different environments.

Suggested change
GLOBAL_MODULES="${HOME}/.bun/install/global/node_modules"
GLOBAL_MODULES="${BUN_INSTALL:-${HOME}/.bun}/install/global/node_modules"
References
  1. Maintain consistency with established patterns for writing scripts that are extracted from Nix expressions, ensuring that environment variables configured in Nix are respected.

DEPS=$(jq -r '.dependencies | keys[]' "$PACKAGE_JSON" 2>/dev/null || true)
Comment on lines +45 to 47

Copilot AI Apr 5, 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 hard-coded to $HOME/.bun/... here and then redefined later in the script. Since BUN_INSTALL is already exported by the Home Manager module, consider deriving this path from ${BUN_INSTALL} and defining it once to avoid duplication/drift.

Copilot uses AI. Check for mistakes.
MISSING=()
if [ -n "$DEPS" ]; then
BATCH_SIZE=10
BATCH=()
while IFS= read -r dep; do
if [ ! -d "${GLOBAL_MODULES}/${dep}" ]; then

@gemini-code-assist gemini-code-assist Bot Apr 5, 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.

high

Checking only for the existence of the directory will cause the script to skip version updates. If a package version is updated in package.json, the script will see that the directory already exists and skip the installation, leaving the outdated version installed. This changes the script's behavior from ensuring the correct environment state to merely ensuring packages are present.

MISSING+=("$dep")
else
echo "$dep already installed, skipping"
fi
done <<< "$DEPS"
Comment on lines 47 to +56

Copilot AI Apr 5, 2026

Copy link

Choose a reason for hiding this comment

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

The skip logic only checks for an existing node_modules directory and ignores the version/range specified in dotfiles/package.json. If a dependency’s version changes, this will now skip it and leave the global install pinned to the old version. Consider deriving desired specifiers from package.json (e.g., name@version) and comparing installed versions (via bun pm -g ls/lockfile) or re-running bun add --global for all deps (but with safer batching/timeout) so upgrades still happen.

Copilot uses AI. Check for mistakes.
fi
Comment on lines +45 to +57

@coderabbitai coderabbitai Bot Apr 5, 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

PACKAGE_JSON="$HOME/dotfiles/package.json"
GLOBAL_PKG="$HOME/.bun/install/global/package.json"

if [[ ! -f "$PACKAGE_JSON" || ! -f "$GLOBAL_PKG" ]]; then
  echo "Missing $PACKAGE_JSON or $GLOBAL_PKG; cannot compare desired vs installed specs."
  exit 0
fi

jq -n --argfile desired "$PACKAGE_JSON" --argfile global "$GLOBAL_PKG" '
  ($desired.dependencies // {}) as $d |
  ($global.dependencies // {}) as $g |
  [$d | to_entries[] |
    select(($g[.key] // "") != .value) |
    {package: .key, desired: .value, installed: ($g[.key] // "<missing>")}
  ]
'

Repository: shunkakinoki/dotfiles

Length of output: 204

🏁 Script executed:

find . -type f -name "install-npm-globals.sh" | head -20

Repository: shunkakinoki/dotfiles

Length of output: 122

🏁 Script executed:

fd -t f "install-npm-globals.sh"

Repository: shunkakinoki/dotfiles

Length of output: 120

🏁 Script executed:

git ls-files | grep -E "(npm-globals|package\.json)" | head -20

Repository: shunkakinoki/dotfiles

Length of output: 178

🏁 Script executed:

cat -n ./home-manager/modules/npm-globals/install-npm-globals.sh

Repository: shunkakinoki/dotfiles

Length of output: 4393

🏁 Script executed:

cat -n ./package.json

Repository: shunkakinoki/dotfiles

Length of output: 4404

🏁 Script executed:

cat -n ./home-manager/modules/npm-globals/default.nix

Repository: shunkakinoki/dotfiles

Length of output: 1850

🌐 Web query:

Bun global package installation update version changes behavior

💡 Result:

Bun supports global package installation via bun install -g or bun add -g . Global packages are stored in ~/.bun/install/global, which acts like a project directory with its own package.json and bun.lockb. Binaries are symlinked to ~/.bun/bin, which must be in your PATH (add export PATH="$HOME/.bun/bin:$PATH" to ~/.zshrc or equivalent if needed). Unlike npm's npm update -g, Bun initially did not fully support bun update -g—it often failed to update all packages or crashed (e.g., issues in 2025 like #25585). The recommended method was cd /.bun/install/global && bun update. Recent changes (2026): - PR #28252 (Mar 2026): Fixed lockfile handling for global installs during bun update -g --latest by skipping workspace-specific preprocessing, preventing assertion failures. - Official docs confirm bun update supports -g/--global flag alongside other options like --interactive, --latest. Current behavior (as of 2026-04-05): bun update -g works to update global packages to latest compatible versions. Use bun update -g --latest for any latest version. List globals with cd /.bun/install/global && bun pm ls (note: bun pm ls -g buggy per #13019/#1863). To update all: bun update -g Interactive: bun update -g --interactive Check outdated: bun outdated -g (inferred from update support). Configurable via bunfig.toml: [install] globalDir = "/.bun/install/global" globalBinDir = "/.bun/bin". Sources confirm evolution from manual directory navigation to supported CLI flags with fixes in recent versions.

Citations:

Directory-only install check misses required version/range changes.

At line 51, the script checks only whether ${GLOBAL_MODULES}/${dep} exists. If package.json updates a dependency version (e.g., "^2.1.92" → "^3.0.0"), the directory persists from the prior install, and the package is skipped (line 54), leaving the global installation stale. When bun add --global is run for missing packages (lines 67, 72), it cannot update an existing global package to a new version range—only initial installs or explicit bun update -g will apply the change.

To fix this, compare the desired version spec in package.json against the installed spec in ~/.bun/install/global/package.json and mark packages for reinstall if specs differ:

Suggested fix 
 GLOBAL_MODULES="${HOME}/.bun/install/global/node_modules"
+GLOBAL_PKG="${HOME}/.bun/install/global/package.json"
 DEPS=$(jq -r '.dependencies | keys[]' "$PACKAGE_JSON" 2>/dev/null || true)
 MISSING=()
 if [ -n "$DEPS" ]; then
   while IFS= read -r dep; do
-    if [ ! -d "${GLOBAL_MODULES}/${dep}" ]; then
-      MISSING+=("$dep")
+    desired=$(jq -r --arg dep "$dep" '.dependencies[$dep] // empty' "$PACKAGE_JSON")
+    installed=$(jq -r --arg dep "$dep" '.dependencies[$dep] // empty' "$GLOBAL_PKG" 2>/dev/null || true)
+    if [ ! -d "${GLOBAL_MODULES}/${dep}" ] || [ "$installed" != "$desired" ]; then
+      MISSING+=("$dep")
     else
       echo "$dep already installed, skipping"
     fi
   done <<< "$DEPS"
 fi
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
# Build list of packages that are not yet installed
GLOBAL_MODULES="${HOME}/.bun/install/global/node_modules"
DEPS=$(jq -r '.dependencies | keys[]' "$PACKAGE_JSON" 2>/dev/null || true)
MISSING=()
if [ -n "$DEPS" ]; then
BATCH_SIZE=10
BATCH=()
while IFS= read -r dep; do
if [ ! -d "${GLOBAL_MODULES}/${dep}" ]; then
MISSING+=("$dep")
else
echo "$dep already installed, skipping"
fi
done <<< "$DEPS"
fi
# Build list of packages that are not yet installed
GLOBAL_MODULES="${HOME}/.bun/install/global/node_modules"
GLOBAL_PKG="${HOME}/.bun/install/global/package.json"
DEPS=$(jq -r '.dependencies | keys[]' "$PACKAGE_JSON" 2>/dev/null || true)
MISSING=()
if [ -n "$DEPS" ]; then
while IFS= read -r dep; do
desired=$(jq -r --arg dep "$dep" '.dependencies[$dep] // empty' "$PACKAGE_JSON")
installed=$(jq -r --arg dep "$dep" '.dependencies[$dep] // empty' "$GLOBAL_PKG" 2>/dev/null || true)
if [ ! -d "${GLOBAL_MODULES}/${dep}" ] || [ "$installed" != "$desired" ]; then
MISSING+=("$dep")
else
echo "$dep already installed, skipping"
fi
done <<< "$DEPS"
fi
🤖 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 45 -
57, The install check currently only tests for the presence of
GLOBAL_MODULES/${dep} and misses version/range changes; update the loop that
builds MISSING to read the desired spec from PACKAGE_JSON (DEPS loop) and
compare it to the installed spec in the global package manifest (e.g.,
GLOBAL_MODULES/../package.json or GLOBAL_MODULES/<dep>/package.json) and if they
differ push the dep onto MISSING; keep the existing echo for already up-to-date
packages but change the condition from a simple directory existence check to a
version-spec comparison so that code paths using MISSING later (the bun add
--global logic) will reinstall when the desired version/range has changed.


# Install missing packages in small batches with a timeout
if [ "${#MISSING[@]}" -gt 0 ]; then
echo "Installing ${#MISSING[@]} missing packages..."
BATCH_SIZE=5
BATCH=()
for dep in "${MISSING[@]}"; do
Comment on lines +45 to +64

Copilot AI Apr 5, 2026

Copy link

Choose a reason for hiding this comment

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

New behavior (building a MISSING list and skipping already-installed packages) isn’t covered by the existing ShellSpec tests for this script (they currently only grep for strings). Consider adding a behavior-level test that sets HOME to a temp dir, stubs bun/jq, pre-creates a fake global node_modules tree, and asserts that already-present packages are skipped while missing ones are installed.

Copilot uses AI. Check for mistakes.
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 5, 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

Suppressing stderr with 2>/dev/null makes it difficult to diagnose why a batch installation failed. Since the failure is already caught and logged with a custom message, it is better to allow the original error output to be visible for troubleshooting.

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

BATCH=()
Comment on lines +59 to 68

Copilot AI Apr 5, 2026

Copy link

Choose a reason for hiding this comment

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

The comment says installs are done “with a timeout”, but the bun add --global calls are not wrapped in timeout. Since the PR goal is to prevent bun hangs, consider applying a timeout to each batch install and treating timeout as a failure case that’s logged (and possibly retried).

Copilot uses AI. Check for mistakes.
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 5, 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 batch installation above, suppressing stderr here hides potential error messages that would be useful for identifying the cause of a failure.

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

fi
Comment on lines +67 to 73

Copilot AI Apr 5, 2026

Copy link

Choose a reason for hiding this comment

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

bun add --global ... 2>/dev/null discards stderr, so when a batch fails the log message won’t include the underlying resolver error. Consider preserving stderr (or capturing it and printing it only on failure) to make failures actionable.

Copilot uses AI. Check for mistakes.
Comment on lines +67 to 73

@coderabbitai coderabbitai Bot Apr 5, 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

# 1) Confirm the masking pattern exists in the script under review.
rg -n 'bun add --global .*\\|\\| echo "Batch install failed' home-manager/modules/npm-globals/install-npm-globals.sh

# 2) Demonstrate Bash behavior with set -e + `||`.
bash -lc 'set -euo pipefail; false || echo "fallback ran"; echo "script continued with success"'

Repository: shunkakinoki/dotfiles

Length of output: 525

🏁 Script executed:

# Check the beginning of the script for error handling
head -20 home-manager/modules/npm-globals/install-npm-globals.sh

# Check the default.nix file to see how exit status is used
cat -n home-manager/modules/npm-globals/default.nix | head -50

Repository: shunkakinoki/dotfiles

Length of output: 2376

🏁 Script executed:

# Check for other similar patterns in the script
rg -n '\|\|' home-manager/modules/npm-globals/install-npm-globals.sh

# Read the full context around lines 67-73 and 60-75
sed -n '55,80p' home-manager/modules/npm-globals/install-npm-globals.sh | cat -n

Repository: shunkakinoki/dotfiles

Length of output: 1648

Batch install failures are silently masked as success.

At lines 67 and 72, bun add --global ... || echo ... masks non-zero exits from bun even under set -e, allowing the script to exit with code 0 after failed installations. The callers in default.nix (lines 16 and 33) execute this script directly and rely on its exit status; installation failures remain invisible to both the systemd service and home-manager activation.

The other || true patterns in the script (lines 37, 41, 47, 80, 86) are intentional and correct, but these two || echo clauses must exit on failure.

Suggested fix 
 if [ "${`#MISSING`[@]}" -gt 0 ]; then
   echo "Installing ${`#MISSING`[@]}" missing packages..."
   BATCH_SIZE=5
   BATCH=()
+  FAILED_BATCHES=()
   for dep in "${MISSING[@]}"; do
     BATCH+=("$dep")
     if [ "${`#BATCH`[@]}" -ge "$BATCH_SIZE" ]; then
-      bun add --global "${BATCH[@]}" 2>/dev/null || echo "Batch install failed: ${BATCH[*]}"
+      if ! bun add --global "${BATCH[@]}"; then
+        FAILED_BATCHES+=("${BATCH[*]}")
+      fi
       BATCH=()
     fi
   done
   if [ "${`#BATCH`[@]}" -gt 0 ]; then
-    bun add --global "${BATCH[@]}" 2>/dev/null || echo "Batch install failed: ${BATCH[*]}"
+    if ! bun add --global "${BATCH[@]}"; then
+      FAILED_BATCHES+=("${BATCH[*]}")
+    fi
+  fi
+  if [ "${`#FAILED_BATCHES`[@]}" -gt 0 ]; then
+    printf 'Batch install failed: %s\n' "${FAILED_BATCHES[@]}" >&2
+    exit 1
   fi
 else
   echo "All npm global packages already installed"
 fi
🤖 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 67 -
73, The two bun batch install invocations using the BATCH array (the commands
that read `bun add --global "${BATCH[@]}" || echo "Batch install failed:
${BATCH[*]}"`) should not mask failures; change them so that a non-zero exit
from `bun add` causes the script to fail (e.g., remove the `|| echo` and let
`set -e` propagate, or replace the `|| echo` with a failing handler that logs
the error to stderr and `exit 1`). Update both occurrences that reference the
BATCH array so failures are propagated to callers (use the same failing behavior
in the blocks where BATCH is flushed and when finishing the loop). Ensure the
logged message (if any) goes to stderr and the process exits non-zero on error.

else
echo "All npm global packages already installed"
fi

# Apply dependency overrides to the global install
Expand Down
Loading