fix(npm-globals): install transitive native binaries dropped by bun - #2027
Conversation
|
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
shunkakinoki
added
the
bug
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughSummary by CodeRabbit
WalkthroughThis PR modifies install-npm-globals.sh to detect and repair missing platform-native optionalDependencies within the same activation run, using new helper functions to scan wrapper package.json files and directly reinstall the correct native binary rather than reinstalling the whole wrapper. Corresponding test cases are added/updated. ChangesNative optionalDependency repair
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant InstallScript
participant missing_native_optional_dep
participant repair_native_optional_dep
participant Bun
InstallScript->>missing_native_optional_dep: check wrapper for missing native dep
missing_native_optional_dep-->>InstallScript: native dep missing
InstallScript->>repair_native_optional_dep: attempt repair(dep)
repair_native_optional_dep->>Bun: bun add --global native-pkg@version
repair_native_optional_dep->>repair_native_optional_dep: purge bun shim
repair_native_optional_dep-->>InstallScript: repaired or failed
alt repair succeeded
InstallScript->>InstallScript: continue (skip reinstall)
else repair failed
InstallScript->>InstallScript: remove wrapper, queue reinstall
end
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. Comment |
![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 improves the installation of npm global packages by adding support for resolving and repairing missing platform-native optional dependencies that are nested under a wrapper package (such as the tokscale pattern). It introduces helper functions to traverse candidate package definitions and directly install the missing native binaries. The feedback suggests adding a defensive guard in missing_native_from_pkg to handle empty platform variables and recommends moving the definition of the global variable GLOBAL_MODULES to the top of the script to prevent potential runtime issues.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| local pj="$1" | ||
| [ -f "$pj" ] || return 0 | ||
| local opt_deps name decl_ver |
There was a problem hiding this comment.
To prevent unexpected behavior or incorrect pattern matching when PLATFORM_OS or PLATFORM_CPU are empty (e.g., on unsupported platforms), we should add a defensive guard at the beginning of missing_native_from_pkg to return early.
| local pj="$1" | |
| [ -f "$pj" ] || return 0 | |
| local opt_deps name decl_ver | |
| local pj="$1" | |
| [ -f "$pj" ] || return 0 | |
| [ -n "$PLATFORM_OS" ] && [ -n "$PLATFORM_CPU" ] || return 0 | |
| local opt_deps name decl_ver |
| local dep="$1" | ||
| local pj="${GLOBAL_MODULES}/${dep}/package.json" | ||
| printf '%s\n' "$pj" |
There was a problem hiding this comment.
The variable GLOBAL_MODULES is used in several helper functions (such as native_candidate_pkgs, missing_native_optional_dep, and repair_native_optional_dep), but it is defined globally much later in the script (on line 255) and redundantly redefined on line 468.
This is fragile and can lead to bugs if any of these functions are called earlier in the script. It is highly recommended to define GLOBAL_MODULES at the top of the script (e.g., right after PACKAGE_JSON on line 20) and remove the redundant definitions.
![mesa-dot-dev[bot]](https://avatars.githubusercontent.com/in/1050077?s=60&v=4){kind=small}
There was a problem hiding this comment.
Performed full review of 9913f36...570fd96
Analysis
• The fixed one-level indirection depth is a pragmatic constraint but lacks explicit documentation as a design boundary (e.g., MAX_NATIVE_SCAN_DEPTH=1). Deeper nesting scenarios will fail silently without clear diagnostics, potentially leaving installations in a broken state without alerting maintainers.
• Logging and observability are adequate but production debugging requires more actionable diagnostics around repair failures—specifically the exact native package name, declaring version, and attempted repair command—to triage why the fallback reinstall was triggered.
• Version extraction and matching logic couples wrapper/binary compatibility tightly to package.json declarations. Any mismatch in how Bun or npm resolves versions compared to your extraction logic could cause silent version skew between the wrapper and binary.
Tip
Help
Slash Commands:
/review- Request a full code review/review latest- Review only changes since the last review/describe- Generate PR description. This will update the PR body or issue comment depending on your configuration/help- Get help with Mesa commands and configuration options
0 files reviewed | 3 comments | Edit Agent Settings • Read Docs
| done < <(native_candidate_pkgs "$dep") | ||
| [ -n "$found" ] || return 1 | ||
|
|
||
| native="${found%% *}" |
There was a problem hiding this comment.
The word-splitting logic here assumes missing_native_from_pkg output format is stable. If decl_ver contains spaces or the native package name contains spaces, this parsing could break. Consider using a more robust delimiter (e.g., tab \t or null byte) or explicitly validate that neither field is empty before splitting to avoid silent failures.
Prompt for Agent
Task: Address review feedback left on GitHub.
Repository: shunkakinoki/dotfiles#2027
File: home-manager/modules/npm-globals/install-npm-globals.sh#L158
Action: Open this file location in your editor, inspect the highlighted code, and resolve the issue described below.
Feedback:
The word-splitting logic here assumes `missing_native_from_pkg` output format is stable. If `decl_ver` contains spaces or the native package name contains spaces, this parsing could break. Consider using a more robust delimiter (e.g., tab `\t` or null byte) or explicitly validate that neither field is empty before splitting to avoid silent failures.
| spec="$native" | ||
| [ -n "$decl_ver" ] && spec="${native}@${decl_ver}" | ||
| echo "Installing missing native binary: $spec" | ||
| timeout 600 bun add --global "$spec" --minimum-release-age 0 2>/dev/null || |
There was a problem hiding this comment.
The bun add --global command output is redirected to /dev/null, which may hide useful diagnostic information when the install fails. Consider capturing stderr to a variable or logging it for debugging purposes, especially since this is a repair path that may fail due to network issues, version conflicts, or package availability.
Prompt for Agent
Task: Address review feedback left on GitHub.
Repository: shunkakinoki/dotfiles#2027
File: home-manager/modules/npm-globals/install-npm-globals.sh#L163
Action: Open this file location in your editor, inspect the highlighted code, and resolve the issue described below.
Feedback:
The `bun add --global` command output is redirected to `/dev/null`, which may hide useful diagnostic information when the install fails. Consider capturing stderr to a variable or logging it for debugging purposes, especially since this is a repair path that may fail due to network issues, version conflicts, or package availability.
| # Heal in the same run: bun drops transitive platform binaries on fresh | ||
| # global installs, so repair immediately instead of waiting for next activation. | ||
| if missing_native_optional_dep "$dep"; then | ||
| repair_native_optional_dep "$dep" || echo "Native binary repair failed: $dep" >&2 |
There was a problem hiding this comment.
The repair happens after run_postinstall_if_needed completes. If the postinstall script depends on the native binary being present, it would fail before the repair runs. Consider moving the repair check before the postinstall step, or add error handling in postinstall to gracefully handle missing binaries during this window.
Prompt for Agent
Task: Address review feedback left on GitHub.
Repository: shunkakinoki/dotfiles#2027
File: home-manager/modules/npm-globals/install-npm-globals.sh#L310
Action: Open this file location in your editor, inspect the highlighted code, and resolve the issue described below.
Feedback:
The repair happens after `run_postinstall_if_needed` completes. If the postinstall script depends on the native binary being present, it would fail before the repair runs. Consider moving the repair check before the postinstall step, or add error handling in postinstall to gracefully handle missing binaries during this window.
![indent-zero[bot]](https://avatars.githubusercontent.com/in/1426746?s=60&v=4){kind=small}
| spec="$native" | ||
| [ -n "$decl_ver" ] && spec="${native}@${decl_ver}" | ||
| echo "Installing missing native binary: $spec" | ||
| timeout 600 bun add --global "$spec" --minimum-release-age 0 2>/dev/null || |
There was a problem hiding this comment.
Missing the documented bun add -g no-op workaround. The aliased-native block at lines 425-431 explicitly strips the target from $GLOBAL_PKG's optionalDependencies before bun add --global, with the comment:
Bun's
bun add -g <pkg>is a no-op if is already in the global package.json's optionalDependencies (it never materializes the dir). Strip from optionalDependencies first so the add becomes a real install.
This repair path skips that mitigation. Transitive natives aren't normally promoted into the global optionalDependencies map, so the common case is safe — but if the entry ever ends up there (e.g. a prior aliased-native run for the same package, or a stale entry left from user-declared optionalDependencies), bun add --global <native>@<ver> will silently no-op. The presence check at line 166 will then fail, the caller logs "native binary repair failed, reinstalling wrapper", rm -rfs the wrapper and reinstalls it — which just re-triggers the same bun transitive drop. That's a permanent loop across activations for the exact scenario this function is meant to fix.
Suggest applying the same strip-before-add pattern here, or hoisting it into a shared helper so both call sites stay consistent.
| [ -n "$opt_deps" ] || return 1 | ||
|
|
||
| [ -n "$opt_deps" ] || return 0 | ||
| decl_ver=$(jq -r '.version // empty' "$pj" 2>/dev/null || true) |
There was a problem hiding this comment.
decl_ver reads the wrapper's own .version, not the pinned native version. The emitted "<name> <decl_ver>" is later concatenated into bun add --global <name>@<decl_ver>. That's only correct under the convention that a wrapper pins its native optionalDeps at its own version.
For wrappers that pin at a different version (e.g. republished natives, or a shared native versioned independently), or that pin at a semver range like ^1.0.0, we'll install a mismatched or nonexistent version. The new spec at spec/npm_globals_spec.sh:381-387 sets wrapper and native both to 1.0.0 so this case is not exercised.
Consider reading the actual pin with jq -r --arg n "$name" '.optionalDependencies[$n] // empty' "$pj" and falling back to .version only when that yields empty or a non-concrete range.
| timeout 600 bun add --global "$spec" --minimum-release-age 0 2>/dev/null || | ||
| echo "Install failed: $spec" >&2 | ||
| purge_bun_npm_shim | ||
| [ -d "${GLOBAL_MODULES}/${native}" ] |
There was a problem hiding this comment.
Presence check accepts phantom-empty native dir. This function returns success purely on directory existence, but the rest of the script already recognizes that as insufficient for natives:
- Line 421-423 explicitly treats a directory without a
package.jsonas a phantom and reinstalls it. - Line 435-438 warns
"$dep still missing after install ($spec)"whenpackage.jsonis absent after a bun add.
Bun has been observed to create empty native dirs during transitive drops — exactly the scenario this function is invoked for. When that happens, this returns 0, the caller logs "$dep native binary repaired in place", and the destructive wrapper-reinstall fallback is skipped, leaving the user with a broken CLI and a misleading success message.
Suggest matching the aliased-native check at line 414: verify ${GLOBAL_MODULES}/${native}/package.json exists (and optionally that .version matches $decl_ver).
| set -euo pipefail | ||
| printf 'bun %s\n' "$*" >>"$MOCK_LOG" | ||
| EOF | ||
| chmod +x "$MOCK_BIN/bun" |
There was a problem hiding this comment.
Mock bun only logs argv — doesn't exercise the presence check. The bun mock at lines 400-405 just appends to MOCK_LOG and creates no files, and the assertion at line 417 only checks that the correct argv was invoked.
That means repair_native_optional_dep's trailing [ -d "${GLOBAL_MODULES}/${native}" ] always returns false, so the spec doesn't distinguish "repair succeeded" from "bun ran but no dir was materialized" (the phantom-empty case and the documented bun add -g no-op case). Extending the mock to mkdir -p "$GM/@wrap/cli-${os_tok}-${cpu_tok}" (and optionally writing a stub package.json inside) when the invoked argv matches would meaningfully validate the end-to-end heal.
Mesa DescriptionTL;DRFixes runtime "binary not found" errors by detecting and repairing transitive platform-native optional dependencies silently dropped by Bun during global installations. What changed?
Description generated by Mesa. Update settings |
1 participant
Follow-up to #2023.
tokscalewas broken: the wrapper ran but errored withtokscale binary not found.Root cause
tokscaleis a JS wrapper that execs a Rust binary shipped as a transitive platform optionalDependency (tokscale->@tokscale/cli->@tokscale/cli-darwin-arm64). Bun silently drops transitive platform optional deps during global installs, so the binary was never on disk.The existing
missing_native_optional_depguard only inspected a dep's ownoptionalDependencies, so it never fired for wrappers that hide the binary one level down.Changes
missing_native_from_pkg/native_candidate_pkgs: follow one level of wrapper indirection (dep + its direct dependencies) to locate the dropped native binary.repair_native_optional_dep: install the specific missing binary directly at its declaring version, instead of reinstalling the wrapper (which just re-triggers the bun drop).bun add --global, so it heals in the same activation run.Testing
shellspec spec/npm_globals_spec.sh-> 61 examples, 0 failures.shellcheckclean.tokscale --version->tokscale 4.0.7.Summary by cubic
Fixes broken global CLIs by detecting and installing transitive platform-native binaries that
bundrops during global installs, so wrappers liketokscalework again. Handles cases where the real binary is an optionalDependency one level down (e.g.,tokscale->@tokscale/cli-> platform package).missing_native_from_pkg,native_candidate_pkgs).repair_native_optional_dep) during version-match checks and immediately afterbun add --global, avoiding wrapper reinstalls.Written for commit 570fd96. Summary will update on new commits.