Repository navigation
Conversation
`C:\` is the root of drive C. Without the separator it is `C:`, the
current directory of the drive. `C:` is not an absolute path.
`PackageManager::init` removed the separator from a cwd of `C:\`. It then
used `C:` as the project directory. `read_directory("C:")` opens the name
`C:` below the cwd handle and returns ENOENT. The same happened for a cwd
below the root when the package.json is in the root.
- `init` keeps the root separator in the cwd and in each parent it walks
to. It joins `package.json` onto the directory, so a root gets no
second separator.
- The package.json path helpers of add, update, audit fix and the hoisted
installer pass `top_level_dir` to the join as it is. The join asserts an
absolute cwd in debug builds.
- `bun pm pack`, `bun publish` and `bun link` keep the root separator in
the package directory. They could not open `C:`, ran scripts in `C:\C:`,
and could not create the junction.
Fixes #29273
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: oven-sh/bun/.coderabbit.yaml Review profile: ASSERTIVE Plan: Essentials Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan provides up to 10 included reviews per hour; 0 remain after this review. WalkthroughThe change updates Windows drive-root path construction, workspace discovery, and CLI path handling. It adds substituted-drive support and regression tests for install, run, link, pack, and publish operations. ChangesWindows drive-root paths
Suggested reviewers: Priority: ➖ Normal Severity of issue fixed: Medium 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Status Reproduced on Windows x64 with canary 1.4.3 (26e7a4b) and with a debug build of main. It needs a throwaway machine, because it writes to [System.IO.File]::WriteAllText('C:\package.json', '{"name":"fsroot","version":"1.0.0"}')
cd C:\
bun install --ignore-scripts
# ENOENT: Bun could not find a file, and the code that produces this error is missing a better error.The same happens at the root of a mkdir C:\tmp\root; [System.IO.File]::WriteAllText('C:\tmp\root\package.json', '{"name":"fsroot","version":"1.0.0"}')
subst X: C:\tmp\root; cd X:\; bun install; cd C:\; subst X: /dTests: CI (build 118300, head 8e7602c): the diff is green. On both Windows lanes (2019 x64 and 11 aarch64) the five changed test files ran at this commit with 0 failures: One job is red, and it does not touch this change:
Four other tests failed once and passed on a retry or alone: PR: #43437 |
|
Status
Open PRs in the same function (
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Beyond the inline findings, I also checked: without_trailing_slash_windows_path on UNC roots (\\server\share\) falls through to the plain strip, which yields a still-valid \\server\share, so push_package_json and the parent walk are unaffected there; the pack/publish abs_workspace_path derivation at POSIX / (now /package.json minus suffix) resolves to /, matching the old value; and the substDrive helper's letter probe, where subst refuses an already-mapped letter, so concurrent tests fall through to the next free one rather than sharing a root.
Extended reasoning...
Five verified findings are being posted inline (POSIX / path-spelling mismatch, INIT_CWD still drive-relative for lifecycle scripts, the same defect left in run_command.rs, the junction/subst workspace-root case, and the unfixed shared top_level_dir_without_trailing_slash helper), so approval is off the table. This note only records what else was examined and ruled out in the diff: UNC-root handling of the new strip helper, the POSIX / derivation in publish_command.rs/pack_command.rs, and the drive-letter probe in test/harness.ts. All new tests are skipIf(!isWindows), so the Windows-only behaviour is exercised only on Windows CI; a human should weigh the sibling call sites the inline comments name.
Additional findings (outside the current diff — GitHub can't attach inline comments there):
-
🔴
src/install/PackageManager.rs— Lifecycle scripts run by an install at a Windows drive root now get INIT_CWD="C:" (drive-relative) instead of "C:\", a case the base never reached because install failed earlier. configure_env_for_scripts_run at src/install/PackageManager.rs:1096 still uses strings::without_trailing_slash on top_level_dir, which strips the root separator that this PR preserves everywhere else. Fix: build INIT_CWD with the same root-preserving strip (without_trailing_slash_windows_path) so a drive root stays "C:\" while other directories keep losing their trailing separator. The PR keeps the generic strip only at sites that append a separator; this site stores the bare value. [also at: src/install/PackageManager.rs:1097 - nit: lifecycle scripts of a project at a Windows drive root now run, but getINIT_CWD=C:(drive-relative) instead ofC:\, which npm and the Bun runtime report.]Extended reasoning...
The PR makes bun install succeed with cwd C:\ (or a subst drive X:). top_level_dir is then C:\ (src/install/PackageManager.rs:1834 set_top_level_dir(child_cwd), child_cwd is the C:\ prefix). Before running lifecycle scripts, configure_env_for_scripts_run at src/install/PackageManager.rs:1093-1100 inserts INIT_CWD when not already set, with value strings::without_trailing_slash(top_level_dir). bun_core::strings::without_trailing_slash (src/bun_core/lib.rs:2309) strips any trailing / or \ while len > 1, so C:\ becomes C:. npm and the rest of this PR treat C:\ as the root; C: is the current directory of drive C, so a postinstall or prepare script that does path.join(process.env.INIT_CWD, 'file') builds C:file, and a script that changes drive first resolves it against another directory. On the base branch this env value is never produced at a drive root because init fails with ENOENT before scripts run, so this is a new output of the merged code. The three other remaining…
Verification: normal — triggers when
bun installruns at a Windows drive root (cwdC:\or asubstdrive) and a lifecycle script readsINIT_CWD; a path this PR newly makes reachable (the base failed with ENOENT before any script ran). Mechanism verified in the code: -src/install/PackageManager.rs:1520now computestop_level_dir_no_trailing_slash =… | normal — triggers whenbun install/add`/etc.… -
🟣
src/runtime/cli/run_command.rs— pre-existing: a user runningbun run <script>in a project whose package.json sits at a Windows drive root still gets a shell failure, the same one this PR fixes forbun pm packandbun publish. run_command.rs:2438 buildspackage_json_dirwithstrings::without_trailing_slash(without_suffix_comptime(path.text, "package.json")), which turnsC:\into the drive-relativeC:, and passes it as the script cwd. Fix: keep the root separator in every script-cwd derivation, i.e. usewithout_trailing_slash_windows_pathhere as at pack_command.rs:2136 and publish_command.rs:674, which covers the 3 sites. Same pattern at 3 sites (run_command.rs:2438, pack_command.rs:2136, publish_command.rs:674).Extended reasoning...
The PR description reports that with a cwd of
Z:the shell printsbunsh: No such file or directory: Z:\Z:for the prepack script, and fixes that by switching pack_command.rs:2136 and publish_command.rs:674 towithout_trailing_slash_windows_path. It lists other left-alone sites but not this one. The resolver builds the package.json path withr_fs.abs(&[input_path, b"package.json"])(src/resolver/package_json.rs:384), so for a project atC:\package_json.source.path.textisC:\package.json. run_command.rs:2437-2441 stripspackage.jsongivingC:\, thenstrings::without_trailing_slash(bun_core/lib.rs:2309, no drive-root guard) givesC:. That value is passed ascwdtorun_package_script_foreground_with_shell_path(run_command.rs:2456-2466 and the following call for the main script), which hands it toMiniEventLoop::init_global(.., Some(cwd))andInterpreter::init_and_run_from_source(.., Some(cwd))at run_command.rs:326-341. This is the identical functionpack_command.rs:2965 run_lifecycle_scriptuses, so the sameZ:\Z:failure occurs. Trigger:…Verification: pre-existing (the base fails the same way; the PR does not touch run_command.rs, but it fixes the identical pattern in two sibling sites and REVIEW.md asks for the whole class). Trigger:
bun run <script>(Bun shell, the default) when the enclosing package.json is at a Windows drive root (C:\package.json). Mechanism verified: /home/claude/bun/src/runtime/cli/run_command.rs:2437-2441 computes… -
🟣
src/install/PackageManager.rs— Windows users whose workspace root is reached through a junction, symlink or subst drive get the member installed as a standalone project instead of the workspace root, with no error in release builds. PackageManager.rs:1707 takes the root package.json path from get_fd_path, which returns the real drive path, while child_cwd at :1675 keeps the aliased path; relative_normalized at :1794 then yields a..-prefixed path that never equals a workspace key, so :1814 never runs. Fix: compare against a canonical form of child_cwd (realpath of the cwd fd) or compare the dirname of the opened root package.json against the walk's ownparent.Extended reasoning...
The author defers this to #39361 and says a debug assertion fires. In release the debug_assert in relative_normalized is compiled out, so nothing fires; the loop at :1786 finds no match,
breakat :1826 exits, and :1835 sets top_level_dir to child_cwd, the member directory. The install then runs with the member's package.json as the root: its own node_modules, its own lockfile, no hoisting, no catalog resolution. The new drive-root tests use subst, which is exactly this alias shape, and the PR notes workspaces at a subst root abort, so the code path is reached in this PR's own fixture. Population: Windows developers with projects undersubstdrives, junctions to another volume, or Dev Drive mount points, and everybun installrun from a member directory. The base had the same mismatch but could not reach it from a drive root; this PR opens the drive-root path and leaves the mismatch in place. Remedy: resolve child_cwd through the same get_fd_path canonicalization before relative_normalized.Verification: pre-existing — triggered when the workspace root is reached through a junction/symlink/subst alias so that the aliased cwd and the handle's real path differ. Mechanism verified in /home/claude/bun/src/install/PackageManager.rs:
json_pathcomes frombun_sys::get_fd_path(json_file.handle, ...)(:1707-1711, real/final path), whilechild_cwdis a slice of the getcwd-derived buffer (:1675,…
…ipts - `bun add`, `remove`, `update` and `link <pkg>` edit the package.json cache entry keyed by `original_package_json_path`. They then install from the entry keyed by a path that updatePackageJSONAndInstall.rs builds. With the project in `/` the first was `/package.json` and the second `//package.json`, so the install did not see the edit. Both paths now come from `push_package_json`. The runtime manager uses it too. - `INIT_CWD` for lifecycle scripts keeps the separator of a drive root. - `bun run <script>` keeps the separator of a drive root for the cwd of the script. It failed with `bunsh: No such file or directory: C:\C:`.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
Replies to the findings of the review that have no inline thread:
|
There was a problem hiding this comment.
I re-reviewed the two new commits and found no bugs; since this rewrites the cwd/package.json spelling in PackageManager::init and the new tests only run on Windows, a human look is still worthwhile.
What was reviewed:
- The POSIX
/cache-key mismatch from my earlier comment:init,init_with_runtime_onceandupdatePackageJSONAndInstall.rsnow all go throughpush_package_json, andthis_cwdstays a prefix of the buffer across thetruncate+ re-push, so the keys match by construction. - The parent-directory walks:
bun_core::dirnamereturnsNoneatC:\and/, so the loops terminate;join_abs_string_buf_z::<Auto>tolerates a cwd with or without a trailing separator, so the six call sites that dropped the strip produce the same bytes as before except at a drive root. - The detached
ZStr::from_rawborrow ininit: theVeccapacity still covers the longestpush_package_jsonresult, so no reallocation invalidates it. substDrive:substfails on an in-use letter, so concurrent tests fall through to the next letter; drive disposal is registered before the assertions.
Extended reasoning...
Overview
The diff replaces strings::without_trailing_slash(top_level_dir()) with bun_paths::string_paths::without_trailing_slash_windows_path (keeps C:\) or passes top_level_dir() straight into join_abs_string_buf at eleven Rust sites across src/install/ and src/runtime/cli/. PackageManager.rs gains a push_package_json helper used by init, init_with_runtime_once and updatePackageJSONAndInstall.rs, and its two parent-directory walks now use join_abs_string_buf_z::<platform::Auto> into a pooled buffer instead of manual byte copies. Tests add a Windows-only substDrive harness helper and eight Windows-gated tests across install, link, pack, publish and run.
Security risks
None specific to this change: the paths come from getcwd and the resolver's DirnameStore, not from user-controlled package data. The one unsafe (the detached ZStr::from_raw over original_package_json_path_buf) pre-exists; I checked that the Vec::with_capacity(len + "/package.json".len() + 1) still covers the largest push_package_json result (separator + 12 bytes + NUL), so no reallocation happens before the pointer is reassigned at the truncate site, and original_cwd is not read after the buffer's tail is rewritten (only original_cwd_clone is).
Level of scrutiny
Moderate-to-high. PackageManager::init is the entry point for every install subcommand, the cache-key coincidence between original_package_json_path and the root path in updatePackageJSONAndInstall.rs was already broken once in the first revision, and the tests that prove the fix are skipIf(!isWindows) so Linux CI cannot exercise them. I verified from source that bun_core::dirname returns None for C:\ and / (so the walks terminate), that without_trailing_slash_windows_path degrades to without_trailing_slash on unix (so POSIX behaviour is unchanged except //package.json becoming /package.json, now consistently at every site), that the POSIX join body inserts a separator only when the cwd does not already end in one, and that the Windows join's debug_assert!(is_absolute_windows(cwd)) is now satisfied by C:\ where C: would have tripped it. The .last().is_some_and(...) in push_package_json does not match any pattern in the byte-search source lint. Module aliases (path::string_paths, strings::paths) resolve in the files that use them.
Other factors
The commit after my previous review (246d037) addresses the POSIX root cache-key mismatch I raised; the pre-existing FileSystem::top_level_dir_without_trailing_slash still returning C: for init/repl/patch is explicitly deferred to a follow-up in the PR description. Remaining sibling sites (hoisted_install.rs:376/429, PackageInstaller.rs:750) append SEP themselves, so a bare C: is the right prefix there. The bug hunt exited on dry_streak with no findings. I could not run the Windows tests from this Linux checkout, which is the main reason for deferring rather than approving.
This branch has not been deployed
1 participant
Fixes #29273
Problem
bun installfails when the project is the root of a drive (C:\package.json):ENOENT: Bun could not find a file, and the code that produces this error is missing a better error.The cwd can beC:\or a directory below it with no package.json.PackageManager::init(src/install/PackageManager.rs:1509) strips the trailing separator of the cwd.C:\becomesC:, andread_directory("C:")gets ENOENT.bun run <script>,bun pm packandbun publish(bunsh: No such file or directory: C:\C:), andbun link(EINVAL: Invalid argument (symlink())).Fix
initstrips withwithout_trailing_slash_windows_path, which keepsC:\.top_level_dir, the original cwd andINIT_CWDare now absolute.push_package_jsonspells every root package.json path. The path is a key of the package.json cache, sobun addneeds one spelling.run,pack,publishandlinkkeep the root separator. Six join helpers taketop_level_dirwithout the strip.//package.jsonchanges, to/package.json. Verified: 8 new Windows tests intest/cli/install/fail on main and on canary, and pass on this branch (debug build and CI).Background
C:andC:fooare relative to it. OnlyC:\names the root.top_level_diris the project root thatbun installchanges into. Each install path is joined onto it.subst X: <dir>maps a directory to a drive letter, so a test can run bun inX:\. The newsubstDrivehelper intest/harness.tsdoes this.Notes
Which syscall fails
open_dir("C:")goes tonormalize_path_windows.C:is not absolute, and it has no separator and no dot, so the name goes toNtCreateFileunchanged with the cwd handle asRootDirectory. NT looks for an entryC:in the cwd.bun_sys::chdir("C:")just before it succeeds, becauseto_w_dir_pathappends a backslash.Each edit is needed (Windows x64 debug builds,
substroot)bun run hello:bunsh: No such file or directory: Z:\Z:. The others: the ENOENT aboveinitchangebun add:panic: assertion failed: crate::is_absolute_windows(cwd)inroot_package_json_path(add_remove_with_filter.rs:43).bun pm pack:ENOENT: failed to open root directory: Z:, and with aprepackscriptbunsh: No such file or directory: Z:\Z:publish_command.rschangepublishscript fails:bunsh: No such file or directory: Z:\Z:link_command.rschangefailed to create junction to node_modules in global dir due to error EINVAL: Invalid argument (symlink())run_command.rsandINIT_CWDchangesbun run hellofails as on main. Thepostinstallscript printsINIT_CWD=Z:A release build has no such assertion. There
join_abs_string_buf_windowsturns a cwd ofC:intoC:\, so the six join helpers give the same output before and after. The change toselect_targetsalso keeps the--filtersubjects consistent with the original cwd, which is nowC:\.The POSIX root
The first revision of this PR made
initspell the root path/package.jsonand left//package.jsoninupdatePackageJSONAndInstall.rs. The package.json cache is keyed by the raw path on POSIX (Windows converts the separators of the key first). Sobun add ./dirin/edited one entry and installed from the other:No packages! Deleted empty lockfile, and package.json got"./dir": "./dir". The review caught it. Nowinit, the runtime manager andupdatePackageJSONAndInstall.rsall callpush_package_json, so the bytes are equal by construction. A test cannot write to/. By hand, in a container with a writable/(debug build of this branch):bun install,bun installfrom/dir/sub,bun add ./dir,bun remove,bun update,bun run <script>(cwd/),bun pm packwith aprepackscript, andINIT_CWD=/. All correct, and the same as the release build of main for the commands that worked there.By hand, at the root of the real drive
C:(debug build of this branch)bun install(hoisted and isolated, registry andfile:dependencies, apostinstallscript, bins),bun add,bun add ./dir,bun remove,bun update,bun update <name>,bun install --frozen-lockfile,bun installfromC:\dir\sub,bun addwith no package.json,bun pm ls,bun pm bin,bun pm hash,bun why,bun outdated,bun patch,bun pm pack,bun pm pack --destination,bun linkwith a consumer,bun unlink,bun init -y: all exit 0 with the expected files. Together with #43404 the same holds withworkspacesinC:\package.json, includingbun add --filter,bun install --filterandbun update -r.Suites
bun-install(261 pass),bun-run(408 pass),bun-workspaces,bun-add,bun-remove,bun-update,bun-add-filter,bun-add-catalog,bun-pm,bun-pack,bun-publish,bun-link,bun-patch,bun-install-patch: 0 fail.bun-audit,bun-lock,bad-workspace: 0 fail, except tests that need the public network (bitbucket, gitlab) andbun-link"should link dependency without crashing", which compares stdout to a fixed list and gets the stack trace that a debug build prints for the expected failure. Both fail the same way without this change.Sites with the same pattern that this PR leaves alone
strings::without_trailing_slashitself. Many callers append a separator and a name to its result (hoisted_install.rs:376,PackageInstaller.rs:751). For themC:is the correct prefix.add_remove_with_filter.rs:364andfilter_arg.rs:182takeresolve_path::dirnameofC:\package.json, which isC:. I found no failure:bun add --filter <root> ./dirspells the path./dir, andbun run --filterand--workspacesgive the same output atC:\as in a normal directory.FileSystem::top_level_dir_without_trailing_slash()still returnsC:.bun init,bun repland a message ofbun patchread it.bun init -yinU:\writes"name": "u:". That has its own follow-up, because the fix changes what those three commands print.substdrive abort withassertion failed: !bun_paths::is_absolute(path.slice(buf)), also with this PR and install: resolve the workspaces of a package.json in the filesystem root #43404. The package.json path comes fromGetFinalPathNameByHandleand is on the real drive. Fix bun install through subst drives, cross-drive junctions, and symlinked package.json #39361 covers that.#26192 is a different bug
The duplicate bot closed #29273 against #26192 (
R:\code> bun installon an ImDisk RAM disk). I installed ImDisk and ran canary 1.4.3 and this branch inR:\code: both printerror: An internal error occurred (EBADF).GetFinalPathNameByHandleWfails with Win32 error 1 on that volume, andget_fd_pathininitreturns EBADF. This PR does not change that.no test proof · iteration 0 · platform-specific test(s) that do not run on this machine, deferring to CI, which covers all platforms: test/cli/install/bun-run.test.ts, test/cli/install/bun-publish.test.ts, test/cli/install/bun-pack.test.ts, test/cli/install/bun-link.test.ts, test/cli/install/bun-install.test.ts