install: stop bun add -g from walking above the global install dir#30659
install: stop bun add -g from walking above the global install dir#30659robobun wants to merge 8 commits into
bun add -g from walking above the global install dir#30659Conversation
|
Updated 11:00 AM PT - May 14th, 2026
❌ @robobun, your commit 11aece1 has 1 failures in
🧪 To try this PR locally: bunx bun-pr 30659That installs a local version of the PR into your bun-30659 --bun |
|
Found 1 issue this PR may fix:
🤖 Generated with Claude Code |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughPackageManager.init no longer climbs past the current directory for package.json or workspace/root discovery when performing global installs (cli.global). Two regression tests verify ChangesGlobal Install Package Discovery Isolation
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/cli/install/bun-add.test.ts`:
- Around line 2509-2513: The test currently awaits the spawned process via the
awaited variable exited but never asserts its exit code, letting a regression
slip by; after the existing stdout/stderr assertions add an explicit check that
the process exit code (from the promise/variable exited) is non-zero (e.g.,
assert/expect that exited.code or the resolved exit value is !== 0) so the test
fails if the process succeeds; keep the stdout/stderr checks first to preserve
useful failure messages and then assert non-zero exit using the exited result.
- Line 2475: The test creates a temp directory with tmpdirSync (const home =
tmpdirSync()) which bypasses the test harness cleanup; replace this by using the
test harness tempDir fixture instead (call tempDir(...) / await tempDir() per
harness API) and assign its returned path to the same variable (home), making
the test async if needed so you can await the fixture; this ensures automatic
cleanup/disposal and follows the coding guideline to avoid tmpdirSync or
fs.mkdtempSync.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: f47a7b01-eaf3-4524-a1a2-730acd2413ca
📒 Files selected for processing (2)
src/install/PackageManager.zigtest/cli/install/bun-add.test.ts
There was a problem hiding this comment.
♻️ Duplicate comments (1)
test/cli/install/bun-add.test.ts (1)
2475-2475: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winReplace
tmpdirSync()with thetempDirharness fixture.Line 2475 is using
tmpdirSync(), which bypasses the test harness cleanup contract in this repo.Suggested patch
-import { bunExe, bunEnv as env, readdirSorted, tmpdirSync, toBeValidBin, toBeWorkspaceLink, toHaveBins } from "harness"; +import { bunExe, bunEnv as env, readdirSorted, tempDir, tmpdirSync, toBeValidBin, toBeWorkspaceLink, toHaveBins } from "harness"; @@ - const home = tmpdirSync(); + using home = tempDir("bun-add-global-parent-lockfile", {});As per coding guidelines:
Use tempDir from harness to create temporary directories; do not use tmpdirSync or fs.mkdtempSync.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/cli/install/bun-add.test.ts` at line 2475, The test is creating a temporary directory with tmpdirSync() which bypasses the test harness cleanup; replace the tmpdirSync() call with the harness-provided tempDir fixture and assign its result to the same variable (home) so the test uses tempDir(...) (or the harness's tempDir helper) instead of tmpdirSync(); this ensures the harness manages cleanup and the rest of the test that references home continues to work unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@test/cli/install/bun-add.test.ts`:
- Line 2475: The test is creating a temporary directory with tmpdirSync() which
bypasses the test harness cleanup; replace the tmpdirSync() call with the
harness-provided tempDir fixture and assign its result to the same variable
(home) so the test uses tempDir(...) (or the harness's tempDir helper) instead
of tmpdirSync(); this ensures the harness manages cleanup and the rest of the
test that references home continues to work unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: ce22ab13-8319-4c2d-8194-9354bfc220ec
📒 Files selected for processing (1)
test/cli/install/bun-add.test.ts
|
Good catch — the workspace-root hop guard does fix #28247 too. Verified locally (parent |
There was a problem hiding this comment.
♻️ Duplicate comments (2)
test/cli/install/bun-add.test.ts (2)
2475-2475:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUse
tempDir(...)instead oftmpdirSync()for these new test homes.These new tests bypass harness-managed disposal by using
tmpdirSync(). Switch tousing home = tempDir(name, {})for automatic cleanup.Suggested patch
-import { bunExe, bunEnv as env, readdirSorted, tmpdirSync, toBeValidBin, toBeWorkspaceLink, toHaveBins } from "harness"; +import { bunExe, bunEnv as env, readdirSorted, tempDir, tmpdirSync, toBeValidBin, toBeWorkspaceLink, toHaveBins } from "harness"; @@ - const home = tmpdirSync(); + using home = tempDir("bun-add-global-parent-lockfile", {}); @@ - const home = tmpdirSync(); + using home = tempDir("bun-add-global-parent-workspaces", {});As per coding guidelines:
Use tempDir from harness to create temporary directories; do not use tmpdirSync or fs.mkdtempSync. Based on learnings:TempDir must be invoked with two arguments in test harness code.Also applies to: 2523-2523
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/cli/install/bun-add.test.ts` at line 2475, The test creates temporary directories using tmpdirSync() (const home = tmpdirSync()) which bypasses harness-managed cleanup; replace these with the harness helper by declaring a managed temp dir using the tempDir helper (e.g., using home = tempDir(name, {})) so the directory is auto-disposed by the test harness; update each occurrence (including the one at the symbol 'home' in this file and the similar occurrence around line ~2523) to call tempDir with two arguments (a name and an options object) instead of tmpdirSync().
2506-2510:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAssert a non-zero exit code so these regressions can’t pass vacuously.
Both tests intentionally use an unreachable registry; assert that the process fails after the stderr checks.
Suggested patch
- await exited; + expect(await exited).not.toBe(0); @@ - await exited; + expect(await exited).not.toBe(0);As per coding guidelines:
When spawning processes in tests, check stdout expectations before exit code expectations to provide more useful error messages on test failure.Also applies to: 2550-2552
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/cli/install/bun-add.test.ts` around lines 2506 - 2510, The tests currently await the child-process completion via the exited promise but don't assert a non-zero exit code, so add an assertion that the exit code is non-zero after the existing stderr/stdout expectations: after awaiting exited, check exited.code (or the process result object you receive) !== 0. Ensure you keep stdout/stderr checks before the exit-code assertion to follow the guideline; apply the same change for the second occurrence referenced near the other test (the other exited usage around the nearby block). Use the existing variable name exited to locate where to add the assertion.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@test/cli/install/bun-add.test.ts`:
- Line 2475: The test creates temporary directories using tmpdirSync() (const
home = tmpdirSync()) which bypasses harness-managed cleanup; replace these with
the harness helper by declaring a managed temp dir using the tempDir helper
(e.g., using home = tempDir(name, {})) so the directory is auto-disposed by the
test harness; update each occurrence (including the one at the symbol 'home' in
this file and the similar occurrence around line ~2523) to call tempDir with two
arguments (a name and an options object) instead of tmpdirSync().
- Around line 2506-2510: The tests currently await the child-process completion
via the exited promise but don't assert a non-zero exit code, so add an
assertion that the exit code is non-zero after the existing stderr/stdout
expectations: after awaiting exited, check exited.code (or the process result
object you receive) !== 0. Ensure you keep stdout/stderr checks before the
exit-code assertion to follow the guideline; apply the same change for the
second occurrence referenced near the other test (the other exited usage around
the nearby block). Use the existing variable name exited to locate where to add
the assertion.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 3e85ca97-45d2-48be-9a02-a829ea372555
📒 Files selected for processing (1)
test/cli/install/bun-add.test.ts
Addresses coderabbit review feedback on PR #30659: tempDir gives us the Symbol.dispose cleanup contract, and asserting exit != 0 keeps the test from passing vacuously if the spawn fails to run at all.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/cli/install/bun-add.test.ts`:
- Around line 2553-2555: The negative stderr assertion
expect(err).not.toContain("failed to resolve") is too broad; update the test
assertions around expect(err).not.toContain("Workspace dependency") and
expect(err).not.toContain("workspace:*") to instead narrowly assert against
workspace-specific messaging (e.g., replace the generic
expect(err).not.toContain("failed to resolve") with a check such as
expect(err).not.toContain("failed to resolve workspace") or
expect(err).not.toMatch(/failed to resolve.*workspace/) so only
workspace-related "failed to resolve" text is disallowed.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 27b44422-9bbe-49f2-8551-e0314d72c963
📒 Files selected for processing (1)
test/cli/install/bun-add.test.ts
|
CI is green on everything I changed. The one red lane ( Prior run hit a Zig ICE in Needs a maintainer to merge. |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| using home = tempDir("bun-add-global-parent-workspaces", { | ||
| // parent package.json declares workspaces + a workspace-protocol dep | ||
| // that doesn't actually exist on disk — same shape as the reports. | ||
| "package.json": JSON.stringify({ | ||
| name: "stray-root", | ||
| version: "1.0.0", | ||
| workspaces: ["packages/*"], | ||
| dependencies: { "@scope/nonexistent": "workspace:*" }, | ||
| }), | ||
| ".bun": { install: { global: {} } }, | ||
| }); |
There was a problem hiding this comment.
🟡 nit: as written, this test would still pass if only the and !cli.global guard (PackageManager.zig:696) were reverted — <global>/package.json is not pre-created so the line-638 if (cli.global) break; handles the first pass, and on the retry pass workspaces: ["packages/*"] cannot match .bun/install/global so the hop never happens regardless. It is still a valid #28247 regression test (fails on main), so fine to leave as-is; but if you want it to independently pin the second guard, pre-create ".bun/install/global/package.json" in the fixture and use a glob that captures the global dir (e.g. [".bun/install/*"]).
Extended reasoning...
What this is
This is a test-coverage observation, not a correctness bug. The PR adds two Zig guards and two regression tests; the commit test: cover workspace-root hop case too (#28247) and the author's comment "added test coverage for that case too" suggest this second test is meant to pin the second guard (subcommand.shouldChdirToRoot() and !cli.global at PackageManager.zig:696). Tracing the fixture shows it does not isolate that guard — reverting only and !cli.global while keeping the line-638 if (cli.global) break; still lets this test pass.
Step-by-step trace (with only and !cli.global reverted)
- First
init()(subcommand =.add,cli.global = true): chdirs to<home>/.bun/install/global, tries to openpackage.json→FileNotFound. The fixture only creates".bun": { install: { global: {} } }— an empty dir — so the first guardif (cli.global) break;fires. Sincesubcommand != .install, the function returnserror.MissingPackageJSON. updatePackageJSONAndInstall.zig:528-530catches it, callsattemptToCreatePackageJSON()(which writespackage.jsoninto the current cwd, i.e. the global dir, sinceinit()already chdir'd there), then callsinit()again.- Second
init(): opens the freshly-created<global>/package.jsonon the first iteration.created_package_json = false(it's a freshinit()call). The workspace-hop block now runs (since we reverted!cli.global). It walks up to<home>, opens<home>/package.json, findsworkspaces: ["packages/*"]. processNamesArrayglobspackages/*relative to<home>. The fixture creates no<home>/packages/directory, soworkspace_namesis empty. Thefor (workspace_names.keys(), …)loop iterates zero times and falls through tobreak;.foundstaysfalse.fs.top_level_diris set tochild_cwd(the global dir).<home>/package.jsonis never used as the root,@scope/nonexistentnever enters resolution, and the assertionsexpect(err).not.toContain("Workspace dependency")/not.toContain("@scope/nonexistent")pass.
Even if a <home>/packages/ dir existed, .bun/install/global would not match the packages/* glob, so the for-loop would still never set found = true.
Why this still has value as-is
To be clear — and addressing the counter-argument directly — this test does fail on main (where neither guard exists): on main the first walk-up loop ascends from the empty global dir to <home>/package.json, pins fs.top_level_dir there, loads its workspace:* dep, and emits Workspace dependency "@scope/nonexistent" not found. So it is a perfectly valid black-box regression test for #28247-as-reported, and the two tests do cover two distinct user-visible symptoms (lockfile-migration error vs. workspace-dep error) of the same walk-up bug. That is reasonable regression-test practice and I am not suggesting the test is wrong.
Why it's still worth mentioning
The PR makes two separate code changes and the second one (and !cli.global) currently has no test that would fail if it alone were removed. Given the commit message explicitly frames this test as covering "the workspace-root hop case", it seems worth flagging that the fixture as written never actually reaches a state where that guard is the deciding factor. If a future refactor accidentally drops !cli.global, this test will not catch it.
How to make it independently exercise the second guard (optional)
Two small fixture tweaks, both needed:
- Pre-create the global
package.jsonso the firstinit()succeeds without relying on the line-638break+ retry path:".bun": { install: { global: { "package.json": JSON.stringify({ name: "g" }) } } },
- Use a workspace glob that actually captures the global dir, e.g.
workspaces: [".bun/install/*"](or["**"]), so that without!cli.globalthe for-loop matcheschild_cwd, setsfound = true, hops to<home>as root, and surfaces@scope/nonexistentin stderr.
The second tweak is admittedly not a "realistic" workspace glob, which is why this is filed as a nit rather than a blocker — the !cli.global guard is largely defensive belt-and-suspenders on top of the first fix, and it's reasonable to decide that contrived white-box coverage isn't worth it. Either choice (tighten the fixture, or leave it and accept the second guard is covered only by code review) is fine.
`bun add -g` chdirs to <BUN_INSTALL>/install/global then walks up looking for a package.json. If one exists in a parent directory (typically $HOME on Windows) with an old npm v1 package-lock.json next to it, bun treated that as the root project and aborted with 'Please upgrade package-lock.json to lockfileVersion 2 or 3'. Global installs are self-contained. Stop the walk — and the workspace-root hop that follows it — at the global install dir when `cli.global` is set. Fixes #30658.
Addresses coderabbit review feedback on PR #30659: tempDir gives us the Symbol.dispose cleanup contract, and asserting exit != 0 keeps the test from passing vacuously if the spawn fails to run at all.
'failed to resolve' is generic enough that a registry/network error could trip it; checking the workspace dep name is the strong signal that bun touched the parent workspace at all.
windows-x64-baseline-build-zig hit a Zig ICE in std/array_hash_map.zig (unrelated to this diff — the other *-build-zig lanes all compile fine).
robobun
force-pushed
the
farm/2aa053de/fix-global-install-walkup
branch
from
0f4a180 to
11aece1
Compare
|
Rebased onto main (post-Rust-rewrite) and ported both hunks to
Gate verified: without the |
|
New CI failure on build 54323 is also flake: Still green on everything my diff touches. Needs a maintainer to merge. |
1 participant
Repro
Cause
PackageManager.initchdirs to<BUN_INSTALL>/install/globalfor global installs and then walks up the filesystem looking for apackage.json. If one is found in$HOME(or any ancestor),fs.top_level_dirgets pinned there, the process chdirs to that directory, andLockfile.loadFromCwdends up trying to migrate the straypackage-lock.jsonnext to it. npm v1 lockfiles fail migration with the user-facing error.On Windows,
<BUN_INSTALL>defaults to%USERPROFILE%\.bun, so the walk always passes through%USERPROFILE%— that's why the bug is so consistent on Windows.The same walk-up is also why
bun update -g/bun upgrade -g --latestfail withWorkspace dependency "<name>" not foundon setups where$HOME/package.jsonhas aworkspacesfield (#28247): once the walk hops onto that parent, the global install inherits its workspace definitions and then can't resolve the workspace-scoped deps.Fix
Global installs are self-contained.
src/install/PackageManager.zig:cli.globalis set andFileNotFoundis returned forpackage.jsonin the current dir, break out of the walk-up loop instead of ascending into the parent.bun add <pkg>then falls through to the existingattemptToCreatePackageJSONAndOpenpath that creates<global>/package.jsonon demand.cli.globalis set — a parent directorypackage.jsonwithworkspacesshouldn't capture the global install either.Verification
Added ````bun add -g
ignores package.json/package-lock.json above the global dir``` totest/cli/install/bun-add.test.ts`. It sets `BUN_INSTALL` to a sandbox dir containing stray `package.json` + npm v1 `package-lock.json` at the sandbox root, runs `bun add -g chalk --registry=http://127.0.0.1:1/`, and asserts stderr never contains the migration error. The test fails on current main and passes with this change.Manually verified the workspace-root hop case locally: a
$HOME/package.jsonwithworkspacesand aworkspace:*dependency makesbun update -g/bun add -g/bun upgrade -g --latestfail on main withWorkspace dependency "…" not found, and succeed after this change.Fixes #30658.
Fixes #28247.