Skip to content

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

Closed
shunkakinoki wants to merge 1 commit into
mainfrom
fix/npm-globals-skip-installed
Closed

fix: skip already-installed npm globals to prevent bun hang#1379
shunkakinoki wants to merge 1 commit into
mainfrom
fix/npm-globals-skip-installed

Conversation

@shunkakinoki

@shunkakinoki shunkakinoki commented Apr 5, 2026

Copy link
Copy Markdown
Owner

Summary

  • Skip packages that already exist in ~/.bun/install/global/node_modules instead of re-adding them
  • Reduce batch size from 10 to 5 to lower resolution pressure on bun
  • Log which packages are skipped and which batches fail

Test plan

  • Run bash home-manager/modules/npm-globals/install-npm-globals.sh with all packages installed - should skip all
  • Remove one global package and re-run - should only install the missing one

Summary by cubic

Skip already-installed npm global packages and install only missing ones to prevent bun from hanging during resolution. Also reduces batch size and adds clearer logs.

  • Bug Fixes
    • Skip packages already present in ~/.bun/install/global/node_modules.
    • Reduce batch size from 10 to 5 to ease bun resolution.
    • Log skipped packages and report failed batch installs.

Written for commit b3531a3. Summary will update on new commits.

Copilot AI review requested due to automatic review settings April 5, 2026 16:24
@mesa-dot-dev

mesa-dot-dev Bot commented Apr 5, 2026

Copy link
Copy Markdown

You do not have enough credits to review this pull request. Please purchase more credits to continue.

@coderabbitai

coderabbitai Bot commented Apr 5, 2026

Copy link
Copy Markdown
📝 Walkthrough

Summary by CodeRabbit

  • Performance

    • npm global packages installation now detects and skips already-installed packages, reducing setup time.
  • Bug Fixes

    • Installation errors are now properly reported instead of being silently ignored.
    • Optimized batch processing to focus on missing packages only.

Walkthrough

The npm globals installation script now checks for existing packages in the bun installation directory before installing, builds a missing packages list, skips already-installed packages, installs only missing packages in smaller batches of 5, and reports batch installation failures with error messages.

Changes

Cohort / File(s) Summary
NPM Globals Installation Logic
home-manager/modules/npm-globals/install-npm-globals.sh
Added package existence checking against ${HOME}/.bun/install/global/node_modules, filters to only missing packages, reduced batch size from 10 to 5, added error reporting for failed batches, and skip installation if all packages already exist.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

Poem

🐰 Hopping through dependencies with care and delight,
Checking what's there before install takes flight,
In batches of five, no wasteful repeat—
Your npm globals stay tidy and neat!

🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately and concisely describes the main change: skipping already-installed npm globals to prevent bun hangs.
Description check ✅ Passed The description is directly related to the changeset, detailing the specific modifications made to the npm globals installation script.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/npm-globals-skip-installed

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@mesa-dot-dev

mesa-dot-dev Bot commented Apr 5, 2026

Copy link
Copy Markdown

Mesa Description

TL;DR

Fixes bun hanging during npm global package installation by skipping already-installed packages and reducing the installation batch size.

What changed?

  • Implemented logic to skip npm global packages that already exist in ~/.bun/install/global/node_modules.
  • Reduced the installation batch size from 10 to 5 to alleviate resolution pressure on bun.
  • Added logging for packages that are skipped and for batches that fail during installation.

Description generated by Mesa. Update settings

@gemini-code-assist gemini-code-assist Bot left a comment

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.

Code Review

This pull request optimizes the global npm package installation script by checking for existing packages and installing only missing ones in smaller batches. Review feedback identifies a significant issue where checking only for directory existence prevents package version updates. Additionally, suggestions were made to respect the BUN_INSTALL environment variable for consistency and to stop suppressing error output during installation to facilitate easier troubleshooting.

BATCH_SIZE=10
BATCH=()
while IFS= read -r dep; do
if [ ! -d "${GLOBAL_MODULES}/${dep}" ]; then

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.


# 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"

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.

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[*]}"

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[*]}"

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[*]}"

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[*]}"

Copilot AI left a comment

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.

Pull request overview

Updates the npm-globals Home Manager install script to reduce bun resolver hangs by avoiding redundant installs and lowering install batch size.

Changes:

  • Detect already-installed global packages under ~/.bun/install/global/node_modules and skip them.
  • Reduce bun add --global batch size from 10 to 5 and add logging for skipped packages / failed batches.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines 47 to +56
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"

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.
Comment on lines +59 to 68
# 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
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[*]}"
BATCH=()

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.
Comment on lines +67 to 73
bun add --global "${BATCH[@]}" 2>/dev/null || echo "Batch install failed: ${BATCH[*]}"
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[*]}"
fi

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 +45 to 47
# 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)

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.
Comment on lines +45 to +64
# 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

# 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

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.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Actionable comments posted: 2

🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@home-manager/modules/npm-globals/install-npm-globals.sh`:
- Around line 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.
- Around line 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.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: e512fc19-723c-4d00-b029-d988c843519e

📥 Commits

Reviewing files that changed from the base of the PR and between 5b19bf3 and b3531a3.

📒 Files selected for processing (1)
  • home-manager/modules/npm-globals/install-npm-globals.sh

Comment on lines +45 to +57
# 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

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.

Comment on lines +67 to 73
bun add --global "${BATCH[@]}" 2>/dev/null || echo "Batch install failed: ${BATCH[*]}"
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[*]}"
fi

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.

@cubic-dev-ai cubic-dev-ai Bot left a comment

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.

No issues found across 1 file

shunkakinoki added a commit that referenced this pull request Apr 6, 2026
- Skip already-installed npm packages by comparing versions from
  node_modules package.json, preventing bun hang on resolution
- Reduce batch size from 10 to 5 for more reliable installs
- Remove recursive=true from nvim lua home.file to prevent
  home-manager from replacing repo files with Nix store symlinks
- Add tests for version checking and skip-installed logic

Closes #1379

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
shunkakinoki added a commit that referenced this pull request Apr 6, 2026
* fix: skip installed npm globals and prevent nvim lua typechange

- Skip already-installed npm packages by comparing versions from
  node_modules package.json, preventing bun hang on resolution
- Reduce batch size from 10 to 5 for more reliable installs
- Remove recursive=true from nvim lua home.file to prevent
  home-manager from replacing repo files with Nix store symlinks
- Add tests for version checking and skip-installed logic

Closes #1379

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* fix: simplify version extraction for npm dependencies

* fix: update clipboard paste tests to include bash directory in PATH

---------

Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@shunkakinoki
shunkakinoki deleted the fix/npm-globals-skip-installed branch April 6, 2026 04:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants