Conversation
|
Warning Review limit reached
Next review available in: 17 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
WalkthroughThe isolated linker computes a per-package global bin flag for ChangesGlobal bin linking for isolated linker
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Found 3 issues this PR may fix:
🤖 Generated with Claude Code |
|
Added
|
There was a problem hiding this comment.
The change looks correct and tightly mirrors the hoisted installer's logic, but I'll defer to a human since this touches the package installer's bin-linking path and CI is currently red on fea1458 (the build-cpp failures alongside build-zig suggest an infra issue rather than this diff, but worth confirming the autofix commit goes green).
Extended reasoning...
Overview
This PR fixes bun add -g --linker=isolated <pkg> not linking binaries into $BUN_INSTALL/bin/. The change is ~20 lines in src/install/isolated_install/Installer.zig:linkDependencyBins: it computes a per-dependency global flag (true only when options.global, the parent is the root entry, and the dep's pkg_id matches an entry in manager.update_requests) and passes it to bin_linker.link() instead of hardcoded false. Two regression tests are added in test/cli/install/isolated-install.test.ts.
Security risks
None. The change only affects which directory bin symlinks are written into during a global install the user explicitly requested. No new inputs are parsed, no path construction changes, and Bin.Linker.link(true) is the same code path the hoisted installer already exercises (PackageInstaller.zig:316-328).
Level of scrutiny
Medium. The diff is small and exactly mirrors proven logic from the hoisted installer, and it's gated behind options.global and parent_entry_id == .root so non-global installs (the overwhelming majority) short-circuit to the old false behavior. That said, src/install/ is a production-critical subsystem that affects every bun install user, so a human sanity-check is reasonable.
Other factors
- CI is red on
fea1458: bothbuild-zigandbuild-cppfailed across every platform. Since this diff touches no C++, the cpp failures strongly suggest an infrastructure/runner issue rather than a code error, but the follow-up autofix commit (36349f2, test-file formatting only) hasn't reported status yet. - Thread safety:
linkDependencyBinsruns on a task thread, butmanager.update_requestsandmanager.optionsare read-only during the install phase (mutations inrunTasks.zighappen during resolution, before install). Only the root entry's task hits the new branch, so there's no concurrent write to$BUN_INSTALL/bin/. - No CODEOWNERS cover
src/install/. - Tests cover both the positive case (requested package's bin lands in
BUN_INSTALL/bin) and the negative guard (transitive bins do not).
|
Diff is green — the two red Windows lanes are both infra flakes unrelated to this PR:
Used my one retrigger (477d2d6). Needs a maintainer to re-run or merge. |
There was a problem hiding this comment.
LGTM — small, well-gated fix that mirrors the hoisted installer's existing global-bin logic.
Extended reasoning...
Overview
This PR fixes bun add -g --linker isolated <pkg> failing to link bins into $BUN_INSTALL/bin/. The change is ~16 lines in src/install/isolated_install/Installer.zig:linkDependencyBins plus two new regression tests in test/cli/install/isolated-install.test.ts. It computes a per-package global flag (true iff options.global && parent_entry_id == .root && pkg_id ∈ update_requests) and passes it to bin_linker.link() instead of a hardcoded false.
Security risks
None. This writes symlinks/shims into $BUN_INSTALL/bin/ for explicitly user-requested packages — the same behavior the hoisted installer already has. No auth, crypto, permissions, or untrusted-input parsing is touched.
Level of scrutiny
Low-to-medium. The new logic is a near-verbatim port of src/install/PackageInstaller.zig:316-328 (verified). It's gated behind options.global and parent_entry_id == .root, so for all non-global installs link_into_global_bin is false and the behavior is byte-identical to before — zero blast radius on the common path. For global installs, only packages matching an explicit update_requests entry get the new behavior, which is exactly the user expectation and matches the hoisted linker.
Other factors
- The one inline nit (test 2 doesn't precisely pin the
update_requestsfilter becausedep-pkgis transitive and already gated byparent_entry_id != .root) is a valid coverage observation but non-blocking — the production code is correct by construction (mirrors the hoisted path), and test 1 is the load-bearing regression gate. - No CODEOWNERS apply to these files.
- No outstanding human reviewer comments.
- Both
bin_linker.link()call sites (initial + retry-without-native-binlink) are updated consistently.
477d2d6 to
5a819f8
Compare
There was a problem hiding this comment.
LGTM — the Rust port mirrors the hoisted installer's update_requests gate exactly and both bin_linker.link() sites are covered.
Extended reasoning...
Overview
This PR fixes bun add -g --linker=isolated <pkg> not linking the requested package's bins into $BUN_INSTALL/bin/. The change is ~25 lines in src/install/isolated_install/Installer.rs (link_dependency_bins): it computes a per-dep global flag — options.global && parent_entry_id == ROOT && pkg_id ∈ update_requests — and passes it to bin_linker.link() at both the initial call and the retry-without-native-binlink path. Two regression tests are added in test/cli/install/isolated-install.test.ts.
Security risks
None. The change only affects where bin symlinks/shims are written during a global install the user explicitly invoked. The target directory (options.bin_path) was already wired into the bin::Linker struct; the only behavioral change is passing true instead of a hardcoded false for the narrow root-entry/update-request case. No new path computation, no new user-controlled input.
Level of scrutiny
Low–medium. The new logic is a verbatim copy of the established pattern in the hoisted installer (src/install/PackageInstaller.rs:603-614) — same labeled-block loop over manager.update_requests comparing package_id. The isolated installer's parent_entry_id == StoreEntryId::ROOT is the structural equivalent of the hoisted tree_id == 0 gate. Since the Rust port has been rebased onto current main, I re-verified the surrounding code: global_bin_path is already populated, StoreEntryId::ROOT is the established root sentinel used elsewhere in the file, and the retry branch correctly reuses the same global value.
Other factors
- Test 1 directly gates the bug (fails on main, passes with fix). My prior inline nit about test 2 not specifically pinning the
update_requestsfilter still stands but was explicitly non-blocking — the production code is correct regardless, and test 2 does guard against the broader "link everything globally" regression. - No CODEOWNERS for these paths.
- Bug-hunting system found no issues this round.
- CI was green on the prior revision modulo known Windows infra flakes; the latest commit (5a819f8) is the Rust-port rebase with no logic change beyond language translation.
5a819f8 to
223874d
Compare
|
CI is red only on Every other lane is green and the |
34ea2a3 to
c39ab3a
Compare
c39ab3a to
5bc7fc6
Compare
`bun add -g --linker isolated <pkg>` used to leave the installed bin in `$BUN_INSTALL/install/global/node_modules/.bin/` instead of symlinking into `$BUN_INSTALL/bin/`, so the command never made it onto PATH. The isolated installer's `link_dependency_bins` always passed `false` to `bin::Linker::link()`; the hoisted installer computes `global` per-package from `options.global` + `update_requests`. Match that logic: for the root entry's direct dependencies, link into `options.bin_path` when the package matches a top-level update request. Transitive deps and non-root entries stay in the parent's local `.bin/`, which is correct. Fixes #30450 Fixes #28597
5bc7fc6 to
92ac61a
Compare
There was a problem hiding this comment.
LGTM — the earlier review feedback has been addressed in 92ac61a (test comment rewritten to match the non-global fixture, Rust comment trimmed).
What was reviewed:
- The new
globalcomputation inlink_dependency_binsmirrors the hoisted installer's gate atPackageInstaller.rs:685-696byte-for-byte, and bothbin_linker.link()call sites (initial + retry-without-native-binlink) are updated. StoreEntryId::ROOTisSelf(0), matching the hoisted installer'stree_id != 0check; the comparison pattern is already used elsewhere in this file.- Tests use file-path deps (no registry), spread
bunEnv, drain pipes concurrently, and handle the Windows.exe/.bunxshim naming.
Extended reasoning...
Overview
Two-file change: src/install/isolated_install/Installer.rs gains a per-dep global boolean in link_dependency_bins — true iff options.global && parent_entry_id == ROOT and the dep's pkg_id matches one of manager.update_requests — passed to both bin_linker.link() calls (initial + retry). test/cli/install/isolated-install.test.ts adds a two-test describe("global install") block: one asserting bun add -g --linker=isolated links the bin into $BUN_INSTALL/bin/ (the load-bearing regression), one asserting a non-global bun install links only into the project's node_modules/.bin/.
Security risks
None. This changes where symlinks are written for globally-installed package bins under a temp BUN_INSTALL. No untrusted-input parsing, no auth/crypto, no path-traversal surface introduced — the global_bin_path field was already populated on the bin::Linker struct; this just flips the flag that consults it.
Level of scrutiny
Low-to-medium. The fix is a direct port of existing, proven logic from the hoisted installer (PackageInstaller.rs:685-696) — same labeled-block shape, same update_requests iteration, same package_id comparison. StoreEntryId::ROOT is defined as Self(0) in Store.rs:74, so parent_entry_id == ROOT is the isolated-installer equivalent of the hoisted tree_id == 0. The retry path is updated symmetrically. No new state, no lifecycle changes, no allocation.
Other factors
I left two prior review comments on this PR (both resolved). The first (2026-05-10) noted test 2's original fixture didn't exercise the update_requests filter — the author reworked it to guard the options.global half instead, and updated the PR description to match. The second (2026-06-05) flagged a stale comment left over from that rewrite — fixed in 92ac61a. The comment-cop bot's flag on the Rust comment was addressed (trimmed to two lines) and the author's rationale for keeping it is sound: it documents a non-obvious gate, not a workaround. Tests follow harness conventions (tempDir, bunEnv spread, concurrent pipe drain, exitCode asserted last, cross-platform bin naming). All imports used by the new tests are already present at the top of the file.
|
Final CI state for build 96259 (head 92ac61a): 177/177 executed jobs passed, zero test failures. The build is marked failed only because the two darwin-14-aarch64 test-bun retries expired waiting for an agent (capacity on that lane), plus the manual 👀 block step. Nothing red is related to this diff. Needs a maintainer to re-run the darwin shards or merge. |
|
Cross-reference: #43384 changes the hoisted rule that this PR mirrors. A bare |
Fixes #30450.
Fixes #28597.
Repro
With the hoisted linker the same command symlinks
cowsayandcowthinkinto
$BUN_INSTALL/bin/correctly. The reporter hit this vialinker = "isolated"in~/.bunfig.tomlon Windows — same code path.Cause
src/install/isolated_install/Installer.rs—link_dependency_binsiterates the parent entry's direct deps and calls
bin::Linker::link(false)unconditionally. With
global=false, the linker writes intonode_modules/.bin/(the staged store entry) and never consultsoptions.bin_path($BUN_INSTALL/bin/).Compare the hoisted installer (
src/install/PackageInstaller.rs), whichcomputes per-package:
and passes it to
bin_linker.link(global). The isolated installer had noequivalent.
Fix
For the root entry's direct deps, set
global = trueiffoptions.globalis true and the dep's
pkg_idmatches amanager.update_requests[*].This mirrors the hoisted logic. Non-root entries and transitive deps keep
global = false— their bins belong in the parent's local.bin/.Both call sites are updated (the initial call + the retry-without-native-
binlink path).
Verification
test/cli/install/isolated-install.test.ts— two new tests under adescribe("global install", …)block:bun add -g --linker=isolated <pkg-with-bin>symlinks the bin into$BUN_INSTALL/bin/. Fails onmain(bin absent), passes with the fix —this is the load-bearing regression gate.
bun installwith the isolated linker links the bin intothe project's
node_modules/.bin/, never into$BUN_INSTALL/bin/—guards the
options.globalhalf of the gate.File-path deps are used so the tests don't depend on the verdaccio harness.
Rebase notes
Rebased from the pre-Rust-port tree onto current
main(Bun is now Rust).The fix lives in
src/install/isolated_install/Installer.rs. Conflicts weretrivial: an adjacent append in the test file (main's new alias-traversal
test) and a reworded comment next to the new block in
Installer.rs— bothkept alongside each other.
no test proof · iteration 8 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/cli/install/isolated-install.test.ts