-
Notifications
You must be signed in to change notification settings - Fork 4.7k
install: stop bun add -g from walking above the global install dir
#30659
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.
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
Open
robobun
wants to merge
8
commits into
main
Choose a base branch
from
farm/2aa053de/fix-global-install-walkup
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
8 commits
Select commit
Hold shift + click to select a range
529970e
install: stop `bun add -g` from walking above the global install dir
robobun c8c29ef
[autofix.ci] apply automated fixes
autofix-ci[bot] fdd9155
test: cover workspace-root hop case too (#28247)
robobun f778151
test: use tempDir harness fixture + assert non-zero exit
robobun 918b06c
[autofix.ci] apply automated fixes
autofix-ci[bot] 12e5d10
test: narrow workspace negative assertion to the dep name
robobun 822868e
ci: retrigger
robobun 11aece1
Port global-install walk-up guard to PackageManager.rs
robobun File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🟡 nit: as written, this test would still pass if only the
and !cli.globalguard (PackageManager.zig:696) were reverted —<global>/package.jsonis not pre-created so the line-638if (cli.global) break;handles the first pass, and on the retry passworkspaces: ["packages/*"]cannot match.bun/install/globalso 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.globalat PackageManager.zig:696). Tracing the fixture shows it does not isolate that guard — reverting onlyand !cli.globalwhile keeping the line-638if (cli.global) break;still lets this test pass.Step-by-step trace (with only
and !cli.globalreverted)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.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/globalwould not match thepackages/*glob, so the for-loop would still never setfound = 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): onmainthe first walk-up loop ascends from the empty global dir to<home>/package.json, pinsfs.top_level_dirthere, loads itsworkspace:*dep, and emitsWorkspace 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:
package.jsonso the firstinit()succeeds without relying on the line-638break+ retry path: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.globalguard 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.