Repository navigation
Make --linker=isolated the default for bun install - #23311
Conversation
WalkthroughAdds automatic Changes
Possibly related PRs
Suggested reviewers
Pre-merge checks❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: ASSERTIVE Plan: Pro Disabled knowledge base sources:
📒 Files selected for processing (1)
🧰 Additional context used📓 Path-based instructions (6)test/**📄 CodeRabbit inference engine (.cursor/rules/writing-tests.mdc)
Files:
test/cli/**/*.{js,ts}📄 CodeRabbit inference engine (.cursor/rules/writing-tests.mdc)
Files:
test/**/*.{js,ts}📄 CodeRabbit inference engine (.cursor/rules/writing-tests.mdc)
Files:
test/**/*.test.ts📄 CodeRabbit inference engine (test/CLAUDE.md)
Files:
test/**/*.test.{ts,tsx}📄 CodeRabbit inference engine (CLAUDE.md)
Files:
test/cli/**📄 CodeRabbit inference engine (CLAUDE.md)
Files:
🧠 Learnings (2)📚 Learning: 2025-08-30T00:12:56.803ZApplied to files:
📚 Learning: 2025-08-30T00:12:56.803ZApplied to files:
🧬 Code graph analysis (1)test/cli/install/isolated-install.test.ts (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
🔇 Additional comments (1)
Comment |
|
so you will break |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (6)
src/install/PackageManager/patchPackage.zig(2 hunks)src/install/install.zig(1 hunks)src/install/isolated_install.zig(1 hunks)src/install/isolated_install/FileCopier.zig(1 hunks)src/install/isolated_install/Installer.zig(7 hunks)src/install/patch_install.zig(2 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
src/**/*.zig
📄 CodeRabbit inference engine (.cursor/rules/building-bun.mdc)
Implement debug logs in Zig using
const log = bun.Output.scoped(.${SCOPE}, false);and invokinglog("...", .{})In Zig code, manage memory carefully and use defer for cleanup of allocations/resources
src/**/*.zig: Use the # prefix to declare private fields in Zig structs (e.g., struct { #foo: u32 })
Prefer decl literals when initializing values in Zig (e.g., const decl: Decl = .{ .binding = 0, .value = 0 })
Place @import directives at the bottom of Zig files
Use @import("bun") instead of @import("root").bun
Files:
src/install/install.zigsrc/install/isolated_install.zigsrc/install/PackageManager/patchPackage.zigsrc/install/isolated_install/Installer.zigsrc/install/patch_install.zigsrc/install/isolated_install/FileCopier.zig
**/*.zig
📄 CodeRabbit inference engine (.cursor/rules/javascriptcore-class.mdc)
**/*.zig: Declare the extern C symbol in Zig and export a Zig-friendly alias for use
Wrap the Bun____toJS extern in a Zig method that takes a JSGlobalObject and returns JSC.JSValue
Files:
src/install/install.zigsrc/install/isolated_install.zigsrc/install/PackageManager/patchPackage.zigsrc/install/isolated_install/Installer.zigsrc/install/patch_install.zigsrc/install/isolated_install/FileCopier.zig
🧠 Learnings (1)
📚 Learning: 2025-10-07T03:19:53.024Z
Learnt from: CR
PR: oven-sh/bun#0
File: src/CLAUDE.md:0-0
Timestamp: 2025-10-07T03:19:53.024Z
Learning: Applies to src/**/*.zig : Use import("bun") instead of import("root").bun
Applied to files:
src/install/isolated_install/Installer.zig
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Format
🔇 Additional comments (2)
src/install/PackageManager/patchPackage.zig (2)
711-711: Call site correctly updated to match new function signature.The updated call correctly passes the required parameters without the manager reference.
750-764: InspectAbsPath.init()to ensure valid base path on non-Windows
Confirm thatbun.AbsPath.init()produces a usable prefix (e.g.,".") so thatthis.src_path.append(entry.path)andbun.copyFile()resolve correctly on POSIX.
Run:#!/bin/bash rg -n "struct AbsPath" -C5 src/paths rg -n "pub fn init" -C5 src/paths
| switch (bun.sys.lstat(dest_subpath.sliceZ())) { | ||
| .err => {}, | ||
| .result => |stat| { | ||
| if (bun.S.ISLNK(stat.mode)) { | ||
| bun.sys.unlink(dest_subpath.sliceZ()).unwrap() catch {}; | ||
| bun.sys.mkdir(dest_subpath.sliceZ(), 0o755).unwrap() catch {}; | ||
| } | ||
| }, | ||
| } |
There was a problem hiding this comment.
Avoid silently ignoring errors that could indicate real problems.
Both .unwrap() catch {} on lines 743 and 744 suppress all errors, including permission denials or filesystem issues. While ignoring a "not found" error for unlink might be acceptable, suppressing mkdir failures could hide real problems that would cause the subsequent copy operation to fail in confusing ways.
Consider handling specific expected errors while propagating unexpected ones:
{
switch (bun.sys.lstat(dest_subpath.sliceZ())) {
.err => {},
.result => |stat| {
if (bun.S.ISLNK(stat.mode)) {
- bun.sys.unlink(dest_subpath.sliceZ()).unwrap() catch {};
- bun.sys.mkdir(dest_subpath.sliceZ(), 0o755).unwrap() catch {};
+ switch (bun.sys.unlink(dest_subpath.sliceZ())) {
+ .err => |e| if (e.getErrno() != .NOENT) return e,
+ .result => {},
+ }
+ try bun.sys.mkdir(dest_subpath.sliceZ(), 0o755).unwrap();
}
},
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| switch (bun.sys.lstat(dest_subpath.sliceZ())) { | |
| .err => {}, | |
| .result => |stat| { | |
| if (bun.S.ISLNK(stat.mode)) { | |
| bun.sys.unlink(dest_subpath.sliceZ()).unwrap() catch {}; | |
| bun.sys.mkdir(dest_subpath.sliceZ(), 0o755).unwrap() catch {}; | |
| } | |
| }, | |
| } | |
| switch (bun.sys.lstat(dest_subpath.sliceZ())) { | |
| .err => {}, | |
| .result => |stat| { | |
| if (bun.S.ISLNK(stat.mode)) { | |
| switch (bun.sys.unlink(dest_subpath.sliceZ())) { | |
| .err => |e| if (e.getErrno() != .NOENT) return e, | |
| .result => {}, | |
| } | |
| try bun.sys.mkdir(dest_subpath.sliceZ(), 0o755).unwrap(); | |
| } | |
| }, | |
| } |
🤖 Prompt for AI Agents
In src/install/PackageManager/patchPackage.zig around lines 739–747, the current
.unwrap() catch {} calls on unlink and mkdir silently swallow all errors; change
this to handle expected "not found" for unlink by ignoring only that error and
for mkdir explicitly check and propagate/log any unexpected errors (e.g.,
permission, ENOENT on parent, filesystem errors) instead of suppressing them so
failures surface to the caller; use explicit error matching on the syscall
result and return or bubble up non-allowed errors with context.
| const cached_package_folder = try cache_dir.openDir(cache_dir_subpath, .{ .iterate = true }); | ||
|
|
||
| return real_file_count; | ||
| } | ||
| const ignore_directories: []const bun.OSPathSlice = &.{ | ||
| comptime bun.OSPathLiteral("node_modules"), | ||
| comptime bun.OSPathLiteral(".git"), | ||
| comptime bun.OSPathLiteral("CMakeFiles"), | ||
| }; | ||
|
|
||
| var pkg_in_cache_dir = try cache_dir.openDir(cache_dir_subpath, .{ .iterate = true }); | ||
| defer pkg_in_cache_dir.close(); | ||
| var walker = bun.handleOom(Walker.walk(.fromStdDir(pkg_in_cache_dir), manager.allocator, &.{}, IGNORED_PATHS)); | ||
| defer walker.deinit(); | ||
|
|
||
| var buf1: if (bun.Environment.isWindows) bun.WPathBuffer else void = undefined; | ||
| var buf2: if (bun.Environment.isWindows) bun.WPathBuffer else void = undefined; | ||
| var in_dir: if (bun.Environment.isWindows) []const u16 else void = undefined; | ||
| var out_dir: if (bun.Environment.isWindows) []const u16 else void = undefined; | ||
|
|
||
| if (comptime bun.Environment.isWindows) { | ||
| const inlen = bun.windows.GetFinalPathNameByHandleW(pkg_in_cache_dir.fd, &buf1, buf1.len, 0); | ||
| if (inlen == 0) { | ||
| const e = bun.windows.Win32Error.get(); | ||
| const err = if (e.toSystemErrno()) |sys_err| bun.errnoToZigErr(sys_err) else error.Unexpected; | ||
| Output.prettyError("<r><red>error<r>: copying file {}", .{err}); | ||
| Global.crash(); | ||
| } | ||
| in_dir = buf1[0..inlen]; | ||
| const outlen = bun.windows.GetFinalPathNameByHandleW(node_modules_folder.fd, &buf2, buf2.len, 0); | ||
| if (outlen == 0) { | ||
| const e = bun.windows.Win32Error.get(); | ||
| const err = if (e.toSystemErrno()) |sys_err| bun.errnoToZigErr(sys_err) else error.Unexpected; | ||
| Output.prettyError("<r><red>error<r>: copying file {}", .{err}); | ||
| Global.crash(); | ||
| } | ||
| out_dir = buf2[0..outlen]; | ||
| var tmpbuf: [1024]u8 = undefined; | ||
| const tmpname = bun.span(bun.fs.FileSystem.instance.tmpname("tffbp", tmpbuf[0..], bun.fastRandom()) catch |e| { | ||
| Output.prettyError("<r><red>error<r>: copying file {s}", .{@errorName(e)}); | ||
| Global.crash(); | ||
| }); | ||
| const temp_folder_in_node_modules = try node_modules_folder.makeOpenPath(tmpname, .{}); | ||
| defer { | ||
| node_modules_folder.deleteTree(tmpname) catch {}; | ||
| } | ||
| _ = try FileCopier.copy( | ||
| node_modules_folder, | ||
| &walker, | ||
| in_dir, | ||
| out_dir, | ||
| &buf1, | ||
| &buf2, | ||
| temp_folder_in_node_modules, | ||
| ); | ||
| } else if (Environment.isPosix) { | ||
| _ = try FileCopier.copy( | ||
| node_modules_folder, | ||
| &walker, | ||
| {}, | ||
| {}, | ||
| {}, | ||
| {}, | ||
| {}, | ||
| ); | ||
| } | ||
| var copier: bun.install.FileCopier = try .init( | ||
| .fromStdDir(cached_package_folder), | ||
| src_path, | ||
| dest_subpath, | ||
| ignore_directories, | ||
| ); | ||
| defer copier.deinit(); | ||
|
|
||
| try copier.copy().unwrap(); |
There was a problem hiding this comment.
Resource leak if FileCopier initialization fails.
The cached_package_folder directory opened on line 766 is not closed if FileCopier.init fails on line 774. The defer copier.deinit() on line 780 only executes if init succeeds, leaving the directory handle leaked on the error path.
Apply this diff to ensure cleanup:
const cached_package_folder = try cache_dir.openDir(cache_dir_subpath, .{ .iterate = true });
+defer cached_package_folder.close();
const ignore_directories: []const bun.OSPathSlice = &.{
comptime bun.OSPathLiteral("node_modules"),
comptime bun.OSPathLiteral(".git"),
comptime bun.OSPathLiteral("CMakeFiles"),
};
var copier: bun.install.FileCopier = try .init(
.fromStdDir(cached_package_folder),
src_path,
dest_subpath,
ignore_directories,
);
defer copier.deinit();
try copier.copy().unwrap();Note: Verify that FileCopier.init does not take ownership of the directory handle. If it does transfer ownership, the explicit close() may cause a double-free. In that case, you'll need to conditionally close it only on init failure.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const cached_package_folder = try cache_dir.openDir(cache_dir_subpath, .{ .iterate = true }); | |
| return real_file_count; | |
| } | |
| const ignore_directories: []const bun.OSPathSlice = &.{ | |
| comptime bun.OSPathLiteral("node_modules"), | |
| comptime bun.OSPathLiteral(".git"), | |
| comptime bun.OSPathLiteral("CMakeFiles"), | |
| }; | |
| var pkg_in_cache_dir = try cache_dir.openDir(cache_dir_subpath, .{ .iterate = true }); | |
| defer pkg_in_cache_dir.close(); | |
| var walker = bun.handleOom(Walker.walk(.fromStdDir(pkg_in_cache_dir), manager.allocator, &.{}, IGNORED_PATHS)); | |
| defer walker.deinit(); | |
| var buf1: if (bun.Environment.isWindows) bun.WPathBuffer else void = undefined; | |
| var buf2: if (bun.Environment.isWindows) bun.WPathBuffer else void = undefined; | |
| var in_dir: if (bun.Environment.isWindows) []const u16 else void = undefined; | |
| var out_dir: if (bun.Environment.isWindows) []const u16 else void = undefined; | |
| if (comptime bun.Environment.isWindows) { | |
| const inlen = bun.windows.GetFinalPathNameByHandleW(pkg_in_cache_dir.fd, &buf1, buf1.len, 0); | |
| if (inlen == 0) { | |
| const e = bun.windows.Win32Error.get(); | |
| const err = if (e.toSystemErrno()) |sys_err| bun.errnoToZigErr(sys_err) else error.Unexpected; | |
| Output.prettyError("<r><red>error<r>: copying file {}", .{err}); | |
| Global.crash(); | |
| } | |
| in_dir = buf1[0..inlen]; | |
| const outlen = bun.windows.GetFinalPathNameByHandleW(node_modules_folder.fd, &buf2, buf2.len, 0); | |
| if (outlen == 0) { | |
| const e = bun.windows.Win32Error.get(); | |
| const err = if (e.toSystemErrno()) |sys_err| bun.errnoToZigErr(sys_err) else error.Unexpected; | |
| Output.prettyError("<r><red>error<r>: copying file {}", .{err}); | |
| Global.crash(); | |
| } | |
| out_dir = buf2[0..outlen]; | |
| var tmpbuf: [1024]u8 = undefined; | |
| const tmpname = bun.span(bun.fs.FileSystem.instance.tmpname("tffbp", tmpbuf[0..], bun.fastRandom()) catch |e| { | |
| Output.prettyError("<r><red>error<r>: copying file {s}", .{@errorName(e)}); | |
| Global.crash(); | |
| }); | |
| const temp_folder_in_node_modules = try node_modules_folder.makeOpenPath(tmpname, .{}); | |
| defer { | |
| node_modules_folder.deleteTree(tmpname) catch {}; | |
| } | |
| _ = try FileCopier.copy( | |
| node_modules_folder, | |
| &walker, | |
| in_dir, | |
| out_dir, | |
| &buf1, | |
| &buf2, | |
| temp_folder_in_node_modules, | |
| ); | |
| } else if (Environment.isPosix) { | |
| _ = try FileCopier.copy( | |
| node_modules_folder, | |
| &walker, | |
| {}, | |
| {}, | |
| {}, | |
| {}, | |
| {}, | |
| ); | |
| } | |
| var copier: bun.install.FileCopier = try .init( | |
| .fromStdDir(cached_package_folder), | |
| src_path, | |
| dest_subpath, | |
| ignore_directories, | |
| ); | |
| defer copier.deinit(); | |
| try copier.copy().unwrap(); | |
| const cached_package_folder = try cache_dir.openDir(cache_dir_subpath, .{ .iterate = true }); | |
| defer cached_package_folder.close(); | |
| const ignore_directories: []const bun.OSPathSlice = &.{ | |
| comptime bun.OSPathLiteral("node_modules"), | |
| comptime bun.OSPathLiteral(".git"), | |
| comptime bun.OSPathLiteral("CMakeFiles"), | |
| }; | |
| var copier: bun.install.FileCopier = try .init( | |
| .fromStdDir(cached_package_folder), | |
| src_path, | |
| dest_subpath, | |
| ignore_directories, | |
| ); | |
| defer copier.deinit(); | |
| try copier.copy().unwrap(); |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/install/patch_install.zig (1)
132-139: Avoid double-deinit of logger in runFromMainThreadApplyLogger is deinitialized here and again in PatchTask.deinit() for .apply; this will double-free on the error path.
- if (this.callback.apply.logger.errors > 0) { - defer this.callback.apply.logger.deinit(); + if (this.callback.apply.logger.errors > 0) { Output.errGeneric("failed to apply patchfile ({s})", .{this.callback.apply.patchfilepath}); this.callback.apply.logger.print(Output.errorWriter()) catch {}; }
♻️ Duplicate comments (3)
src/install/PackageManager/patchPackage.zig (2)
739-747: Don’t swallow unlink/mkdir errors; handle expected cases explicitlyBoth unwrap() catch {} lines suppress real errors (e.g., permission, parent missing), making failures hard to diagnose.
switch (bun.sys.lstat(dest_subpath.sliceZ())) { .err => {}, .result => |stat| { if (bun.S.ISLNK(stat.mode)) { - bun.sys.unlink(dest_subpath.sliceZ()).unwrap() catch {}; - bun.sys.mkdir(dest_subpath.sliceZ(), 0o755).unwrap() catch {}; + switch (bun.sys.unlink(dest_subpath.sliceZ())) { + .err => |e| if (e.getErrno() != .NOENT) return e, + .result => {}, + } + try bun.sys.mkdir(dest_subpath.sliceZ(), 0o755).unwrap(); } }, }
766-767: Close directory on init failure to avoid handle leakIf FileCopier.init fails, cached_package_folder leaks. Close it unconditionally unless FileCopier takes ownership.
- const cached_package_folder = try cache_dir.openDir(cache_dir_subpath, .{ .iterate = true }); + const cached_package_folder = try cache_dir.openDir(cache_dir_subpath, .{ .iterate = true }); + defer cached_package_folder.close(); // If FileCopier takes ownership, remove this and close only on init failure.If FileCopier.init transfers ownership of the dir handle, adjust by closing only on the error path to prevent double-close.
src/install/isolated_install/Installer.zig (1)
85-99: Surface patch apply failures (don’t rely solely on internal logger)apply() records errors in logger but returns success; check the logger and return a failure so onTaskFail triggers.
const patch_task = install.PatchTask.newApplyPatchHash( this.manager, pkg_id, patch.contents_hash, patch.name_and_version_hash, ); defer patch_task.deinit(); - try patch_task.apply(); + const log = &patch_task.callback.apply.logger; + try patch_task.apply(); + if (log.errors > 0) { + log.print(Output.errorWriter()) catch {}; + return error.PatchApplyFailed; + }
🧹 Nitpick comments (1)
src/install/PackageManager/patchPackage.zig (1)
774-783: Propagate copy errors consistently (align with Installer changes)Use explicit switch on copy result and bubble the error; this matches the updated call sites elsewhere.
- var copier: bun.install.FileCopier = try .init( + var copier: bun.install.FileCopier = try .init( .fromStdDir(cached_package_folder), src_path, dest_subpath, ignore_directories, ); defer copier.deinit(); - - try copier.copy().unwrap(); + switch (copier.copy()) { + .result => {}, + .err => |err| return err, + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
src/install/PackageManager/patchPackage.zig(2 hunks)src/install/isolated_install/Installer.zig(7 hunks)src/install/patch_install.zig(2 hunks)test/internal/ban-limits.json(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- test/internal/ban-limits.json
🧰 Additional context used
📓 Path-based instructions (2)
src/**/*.zig
📄 CodeRabbit inference engine (.cursor/rules/building-bun.mdc)
Implement debug logs in Zig using
const log = bun.Output.scoped(.${SCOPE}, false);and invokinglog("...", .{})In Zig code, manage memory carefully and use defer for cleanup of allocations/resources
src/**/*.zig: Use the # prefix to declare private fields in Zig structs (e.g., struct { #foo: u32 })
Prefer decl literals when initializing values in Zig (e.g., const decl: Decl = .{ .binding = 0, .value = 0 })
Place @import directives at the bottom of Zig files
Use @import("bun") instead of @import("root").bun
Files:
src/install/PackageManager/patchPackage.zigsrc/install/isolated_install/Installer.zigsrc/install/patch_install.zig
**/*.zig
📄 CodeRabbit inference engine (.cursor/rules/javascriptcore-class.mdc)
**/*.zig: Declare the extern C symbol in Zig and export a Zig-friendly alias for use
Wrap the Bun____toJS extern in a Zig method that takes a JSGlobalObject and returns JSC.JSValue
Files:
src/install/PackageManager/patchPackage.zigsrc/install/isolated_install/Installer.zigsrc/install/patch_install.zig
🧠 Learnings (1)
📚 Learning: 2025-10-07T03:19:53.024Z
Learnt from: CR
PR: oven-sh/bun#0
File: src/CLAUDE.md:0-0
Timestamp: 2025-10-07T03:19:53.024Z
Learning: Applies to src/**/*.zig : Use import("bun") instead of import("root").bun
Applied to files:
src/install/PackageManager/patchPackage.zigsrc/install/isolated_install/Installer.zig
🔇 Additional comments (4)
src/install/patch_install.zig (1)
150-156: LGTM: clearer calcHash error reportingSimplified reporting is fine and keeps silent mode behavior intact.
src/install/PackageManager/patchPackage.zig (1)
711-718: LGTM: overwrite call updated to new signatureCall-site change matches the new overwrite function parameters.
src/install/isolated_install/Installer.zig (2)
46-79: LGTM: pre-apply patch and gate task startResolving patch info and applying before starting the task is correct for isolated installs; failure paths route to onTaskFail.
575-596: LGTM: explicit switch on FileCopier.copy resultsSwitching to handle result/err explicitly improves clarity and avoids throwing on Maybe wrappers.
Also applies to: 768-789
| bun.handleOom(log.addErrorFmt(null, Loc.Empty, this.manager.allocator, "Couldn't find patch file: '{s}'\n\nTo create a new patch file run:\n\n <cyan>bun patch {s}<r>", .{ | ||
| this.callback.calc_hash.patchfile_path, | ||
| this.manager.lockfile.patched_dependencies.get(this.callback.calc_hash.name_and_version_hash).?.path.slice(this.manager.lockfile.buffers.string_bytes.items), | ||
| }; | ||
| bun.handleOom(log.addErrorFmt(null, Loc.Empty, this.manager.allocator, fmt, args)); | ||
| })); |
There was a problem hiding this comment.
Wrong message and severity on stat error (non-NOENT); inconsistent OOM handling
For errors other than NOENT, this logs a warning that the patchfile is empty, which is incorrect. Also prefer consistent OOM handling style.
- .err => |e| {
- if (e.getErrno() == .NOENT) {
- bun.handleOom(log.addErrorFmt(null, Loc.Empty, this.manager.allocator, "Couldn't find patch file: '{s}'\n\nTo create a new patch file run:\n\n <cyan>bun patch {s}<r>", .{
- this.callback.calc_hash.patchfile_path,
- this.manager.lockfile.patched_dependencies.get(this.callback.calc_hash.name_and_version_hash).?.path.slice(this.manager.lockfile.buffers.string_bytes.items),
- }));
- return null;
- }
- log.addWarningFmt(
- null,
- Loc.Empty,
- this.manager.allocator,
- "patchfile <b>{s}<r> is empty, please restore or delete it.",
- .{absolute_patchfile_path},
- ) catch |err| bun.handleOom(err);
- return null;
- },
+ .err => |e| {
+ if (e.getErrno() == .NOENT) {
+ log.addErrorFmt(
+ null,
+ Loc.Empty,
+ this.manager.allocator,
+ "Couldn't find patch file: '{s}'\n\nTo create a new patch file run:\n\n <cyan>bun patch {s}<r>",
+ .{
+ this.callback.calc_hash.patchfile_path,
+ this.manager.lockfile.patched_dependencies.get(this.callback.calc_hash.name_and_version_hash).?.path.slice(this.manager.lockfile.buffers.string_bytes.items),
+ },
+ ) catch |oom| bun.handleOom(oom);
+ return null;
+ }
+ log.addErrorFmt(
+ null,
+ Loc.Empty,
+ this.manager.allocator,
+ "failed to stat patch file: {} ({s})",
+ .{ e, absolute_patchfile_path },
+ ) catch |oom| bun.handleOom(oom);
+ return null;
+ },Also applies to: 429-437
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
test/cli/install/bun-install-patch.test.ts(3 hunks)
🧰 Additional context used
📓 Path-based instructions (6)
test/**
📄 CodeRabbit inference engine (.cursor/rules/writing-tests.mdc)
Place all tests under the test/ directory
Files:
test/cli/install/bun-install-patch.test.ts
test/cli/**/*.{js,ts}
📄 CodeRabbit inference engine (.cursor/rules/writing-tests.mdc)
test/cli/**/*.{js,ts}: Place CLI command tests (e.g., bun install, bun init) under test/cli/
When testing Bun as a CLI, use spawn with bunExe() and bunEnv from harness, and capture stdout/stderr via pipes
Files:
test/cli/install/bun-install-patch.test.ts
test/**/*.{js,ts}
📄 CodeRabbit inference engine (.cursor/rules/writing-tests.mdc)
test/**/*.{js,ts}: Write tests in JavaScript or TypeScript using Bun’s Jest-style APIs (test, describe, expect) and run with bun test
Prefer data-driven tests (e.g., test.each) to reduce boilerplate
Use shared utilities from test/harness.ts where applicable
Files:
test/cli/install/bun-install-patch.test.ts
test/**/*.test.ts
📄 CodeRabbit inference engine (test/CLAUDE.md)
test/**/*.test.ts: Name test files*.test.tsand usebun:test
Do not write flaky tests: never wait for arbitrary time; wait for conditions instead
Never hardcode port numbers in tests; useport: 0to get a random port
When spawning Bun in tests, usebunExe()andbunEnvfromharness
Preferasync/awaitin tests; for a single callback, usePromise.withResolvers()
Do not set explicit test timeouts; rely on Bun’s built-in timeouts
UsetempDir/tempDirWithFilesfromharnessfor temporary files and directories in tests
For large/repetitive strings in tests, preferBuffer.alloc(count, fill).toString()over"A".repeat(count)
Import common test utilities fromharness(e.g.,bunExe,bunEnv,tempDirWithFiles,tmpdirSync, platform checks, GC helpers)
In error tests, assert non-zero exit codes for failing processes and usetoThrowfor synchronous errors
Usedescribeblocks for grouping,describe.eachfor parameterized tests, snapshots withtoMatchSnapshot, and lifecycle hooks (beforeAll,beforeEach,afterEach); track resources for cleanup inafterEach
Useusing/await usingwith Bun resources (e.g., Bun.listen/connect/spawn/serve) to ensure cleanup in tests
Files:
test/cli/install/bun-install-patch.test.ts
test/**/*.test.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
test/**/*.test.{ts,tsx}: Test files must live under test/ and end with .test.ts or .test.tsx
In tests, always use port: 0; do not hardcode ports or roll your own random port
Prefer normalizeBunSnapshot for snapshotting test output instead of asserting raw strings
Do not write tests that assert absence of crashes (e.g., 'no panic' or 'no uncaught exception')
Use Bun’s Jest-compatible runner (import { test, expect } from "bun:test") for tests
Avoid shell commands like find or grep in tests; use Bun’s Glob and built-in tools instead
Prefer running tests via bun bd test and use provided harness utilities (bunEnv, bunExe, tempDir)
Use Bun.spawn with proper stdio handling and await proc.exited in process-spawning tests
Files:
test/cli/install/bun-install-patch.test.ts
test/cli/**
📄 CodeRabbit inference engine (CLAUDE.md)
Place CLI command tests under test/cli/
Files:
test/cli/install/bun-install-patch.test.ts
🧠 Learnings (1)
📚 Learning: 2025-08-30T00:12:56.803Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/writing-tests.mdc:0-0
Timestamp: 2025-08-30T00:12:56.803Z
Learning: Applies to test/js/bun/**/*.{js,ts} : Place Bun API tests under test/js/bun/, separated by category (e.g., test/js/bun/glob/)
Applied to files:
test/cli/install/bun-install-patch.test.ts
🧬 Code graph analysis (1)
test/cli/install/bun-install-patch.test.ts (2)
test/harness.ts (2)
bunExe(101-104)tempDirWithFiles(258-265)scripts/utils.mjs (1)
$(143-161)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Format
| const { stderr: commitStderr } = await $`${bunExe()} patch --commit '${relativePatchPath}'` | ||
| .env(bunEnv) | ||
| .cwd(filedir); | ||
|
|
There was a problem hiding this comment.
Remove quotes around relativePatchPath in `$`` templates
Line 610 and its siblings wrap ${relativePatchPath} in single quotes. With Bun’s $ helper each quote becomes its own argv entry, so the process receives --commit, ', <path>, '. Bun then tries to commit a folder literally named ' and the test blows up. Please drop the quotes everywhere you call patch --commit in this block.
- const { stderr: commitStderr } = await $`${bunExe()} patch --commit '${relativePatchPath}'`
+ const { stderr: commitStderr } = await $`${bunExe()} patch --commit ${relativePatchPath}`
@@
- const { stderr: commitStderr } = await $`${bunExe()} patch --commit '${relativePatchPath}'`
+ const { stderr: commitStderr } = await $`${bunExe()} patch --commit ${relativePatchPath}`
@@
- const { stderr: commitStderr } = await $`${bunExe()} patch --commit '${relativePatchPath}'`
+ const { stderr: commitStderr } = await $`${bunExe()} patch --commit ${relativePatchPath}`
@@
- const { stderr: commitStderr } = await $`${bunExe()} patch --commit '${relativePatchPath}'`
+ const { stderr: commitStderr } = await $`${bunExe()} patch --commit ${relativePatchPath}`
@@
- await $`${bunExe()} patch --commit '${relativePatchPath}'`.env(bunEnv).cwd(filedir);
+ await $`${bunExe()} patch --commit ${relativePatchPath}`.env(bunEnv).cwd(filedir);
@@
- const { stderr: commitStderr1 } = await $`${bunExe()} patch --commit '${relativePatchPath1}'`
+ const { stderr: commitStderr1 } = await $`${bunExe()} patch --commit ${relativePatchPath1}`
@@
- const { stderr: commitStderr2 } = await $`${bunExe()} patch --commit '${relativePatchPath2}'`
+ const { stderr: commitStderr2 } = await $`${bunExe()} patch --commit ${relativePatchPath2}`Also applies to: 673-676, 729-732, 792-795, 847-848, 898-901, 923-926
🤖 Prompt for AI Agents
In test/cli/install/bun-install-patch.test.ts around lines 610-613 (and
similarly at 673-676, 729-732, 792-795, 847-848, 898-901, 923-926), the `$``
template calls wrap ${relativePatchPath} in single quotes which causes Bun’s `$`
helper to treat the quotes as separate argv entries; remove the surrounding
single quotes so the template receives --commit and the path as two arguments
(replace "'${relativePatchPath}'" with ${relativePatchPath} in each affected `$`
invocation).
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
test/cli/install/bun-install-patch.test.ts (1)
611-614: Remove quotes around${relativePatchPath}in `$`` templates.This is the same issue previously flagged: wrapping
${relativePatchPath}in single quotes causes Bun's$helper to treat the quotes as separate argv entries. The process receives--commit,',<path>,'instead of--commitand<path>.Apply this diff to remove the quotes:
- const { stderr: commitStderr } = await $`${bunExe()} patch --commit '${relativePatchPath}'` + const { stderr: commitStderr } = await $`${bunExe()} patch --commit ${relativePatchPath}`Apply the same fix to all affected lines: 674, 728, 791, 846, 897, 922.
Also applies to: 673-676, 728-732, 791-795, 846-848, 897-901, 922-926
🧹 Nitpick comments (1)
test/cli/install/bun-install-patch.test.ts (1)
118-118: Consider whether external source testing should be preserved.The version array was simplified from multiple sources (including GitHub and SSH URLs) to a single local version
[["1.0.0"]]. While this reduces test execution time and potential flakiness, it also eliminates coverage for patch scenarios with external package sources. If patch behavior differs based on source type, this could be a gap.If external sources were removed due to test reliability issues, consider documenting why. Otherwise, evaluate whether at least one external source test case should be retained.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
test/cli/install/bun-install-patch.test.ts(4 hunks)
🧰 Additional context used
📓 Path-based instructions (6)
test/**
📄 CodeRabbit inference engine (.cursor/rules/writing-tests.mdc)
Place all tests under the test/ directory
Files:
test/cli/install/bun-install-patch.test.ts
test/cli/**/*.{js,ts}
📄 CodeRabbit inference engine (.cursor/rules/writing-tests.mdc)
test/cli/**/*.{js,ts}: Place CLI command tests (e.g., bun install, bun init) under test/cli/
When testing Bun as a CLI, use spawn with bunExe() and bunEnv from harness, and capture stdout/stderr via pipes
Files:
test/cli/install/bun-install-patch.test.ts
test/**/*.{js,ts}
📄 CodeRabbit inference engine (.cursor/rules/writing-tests.mdc)
test/**/*.{js,ts}: Write tests in JavaScript or TypeScript using Bun’s Jest-style APIs (test, describe, expect) and run with bun test
Prefer data-driven tests (e.g., test.each) to reduce boilerplate
Use shared utilities from test/harness.ts where applicable
Files:
test/cli/install/bun-install-patch.test.ts
test/**/*.test.ts
📄 CodeRabbit inference engine (test/CLAUDE.md)
test/**/*.test.ts: Name test files*.test.tsand usebun:test
Do not write flaky tests: never wait for arbitrary time; wait for conditions instead
Never hardcode port numbers in tests; useport: 0to get a random port
When spawning Bun in tests, usebunExe()andbunEnvfromharness
Preferasync/awaitin tests; for a single callback, usePromise.withResolvers()
Do not set explicit test timeouts; rely on Bun’s built-in timeouts
UsetempDir/tempDirWithFilesfromharnessfor temporary files and directories in tests
For large/repetitive strings in tests, preferBuffer.alloc(count, fill).toString()over"A".repeat(count)
Import common test utilities fromharness(e.g.,bunExe,bunEnv,tempDirWithFiles,tmpdirSync, platform checks, GC helpers)
In error tests, assert non-zero exit codes for failing processes and usetoThrowfor synchronous errors
Usedescribeblocks for grouping,describe.eachfor parameterized tests, snapshots withtoMatchSnapshot, and lifecycle hooks (beforeAll,beforeEach,afterEach); track resources for cleanup inafterEach
Useusing/await usingwith Bun resources (e.g., Bun.listen/connect/spawn/serve) to ensure cleanup in tests
Files:
test/cli/install/bun-install-patch.test.ts
test/**/*.test.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
test/**/*.test.{ts,tsx}: Test files must live under test/ and end with .test.ts or .test.tsx
In tests, always use port: 0; do not hardcode ports or roll your own random port
Prefer normalizeBunSnapshot for snapshotting test output instead of asserting raw strings
Do not write tests that assert absence of crashes (e.g., 'no panic' or 'no uncaught exception')
Use Bun’s Jest-compatible runner (import { test, expect } from "bun:test") for tests
Avoid shell commands like find or grep in tests; use Bun’s Glob and built-in tools instead
Prefer running tests via bun bd test and use provided harness utilities (bunEnv, bunExe, tempDir)
Use Bun.spawn with proper stdio handling and await proc.exited in process-spawning tests
Files:
test/cli/install/bun-install-patch.test.ts
test/cli/**
📄 CodeRabbit inference engine (CLAUDE.md)
Place CLI command tests under test/cli/
Files:
test/cli/install/bun-install-patch.test.ts
🧠 Learnings (10)
📚 Learning: 2025-09-03T17:10:13.486Z
Learnt from: CR
PR: oven-sh/bun#0
File: test/CLAUDE.md:0-0
Timestamp: 2025-09-03T17:10:13.486Z
Learning: Applies to test/**/*.test.ts : When spawning Bun in tests, use `bunExe()` and `bunEnv` from `harness`
Applied to files:
test/cli/install/bun-install-patch.test.ts
📚 Learning: 2025-10-04T09:51:30.294Z
Learnt from: CR
PR: oven-sh/bun#0
File: CLAUDE.md:0-0
Timestamp: 2025-10-04T09:51:30.294Z
Learning: Applies to test/**/*.test.{ts,tsx} : Prefer running tests via bun bd test <file> and use provided harness utilities (bunEnv, bunExe, tempDir)
Applied to files:
test/cli/install/bun-install-patch.test.ts
📚 Learning: 2025-08-30T00:12:56.803Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/writing-tests.mdc:0-0
Timestamp: 2025-08-30T00:12:56.803Z
Learning: Applies to test/cli/**/*.{js,ts} : When testing Bun as a CLI, use spawn with bunExe() and bunEnv from harness, and capture stdout/stderr via pipes
Applied to files:
test/cli/install/bun-install-patch.test.ts
📚 Learning: 2025-09-03T17:10:13.486Z
Learnt from: CR
PR: oven-sh/bun#0
File: test/CLAUDE.md:0-0
Timestamp: 2025-09-03T17:10:13.486Z
Learning: Applies to test/**/*.test.ts : Import common test utilities from `harness` (e.g., `bunExe`, `bunEnv`, `tempDirWithFiles`, `tmpdirSync`, platform checks, GC helpers)
Applied to files:
test/cli/install/bun-install-patch.test.ts
📚 Learning: 2025-10-04T09:51:30.294Z
Learnt from: CR
PR: oven-sh/bun#0
File: CLAUDE.md:0-0
Timestamp: 2025-10-04T09:51:30.294Z
Learning: Applies to test/**/*.test.{ts,tsx} : Use Bun’s Jest-compatible runner (import { test, expect } from "bun:test") for tests
Applied to files:
test/cli/install/bun-install-patch.test.ts
📚 Learning: 2025-08-30T00:12:56.803Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/writing-tests.mdc:0-0
Timestamp: 2025-08-30T00:12:56.803Z
Learning: Applies to test/**/*.{js,ts} : Write tests in JavaScript or TypeScript using Bun’s Jest-style APIs (test, describe, expect) and run with bun test
Applied to files:
test/cli/install/bun-install-patch.test.ts
📚 Learning: 2025-08-30T00:12:56.803Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/writing-tests.mdc:0-0
Timestamp: 2025-08-30T00:12:56.803Z
Learning: Applies to test/js/bun/**/*.{js,ts} : Place Bun API tests under test/js/bun/, separated by category (e.g., test/js/bun/glob/)
Applied to files:
test/cli/install/bun-install-patch.test.ts
📚 Learning: 2025-10-04T09:51:30.294Z
Learnt from: CR
PR: oven-sh/bun#0
File: CLAUDE.md:0-0
Timestamp: 2025-10-04T09:51:30.294Z
Learning: Applies to test/js/bun/** : Place Bun-specific API tests under test/js/bun/
Applied to files:
test/cli/install/bun-install-patch.test.ts
📚 Learning: 2025-10-04T09:51:30.294Z
Learnt from: CR
PR: oven-sh/bun#0
File: CLAUDE.md:0-0
Timestamp: 2025-10-04T09:51:30.294Z
Learning: Applies to test/**/*.test.{ts,tsx} : Use Bun.spawn with proper stdio handling and await proc.exited in process-spawning tests
Applied to files:
test/cli/install/bun-install-patch.test.ts
📚 Learning: 2025-08-30T00:09:39.100Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/dev-server-tests.mdc:0-0
Timestamp: 2025-08-30T00:09:39.100Z
Learning: Applies to test/bake/bake-harness.ts : Provide and maintain shared test utilities: devTest, prodTest, devAndProductionTest, Dev, Client, and helpers in the harness
Applied to files:
test/cli/install/bun-install-patch.test.ts
🧬 Code graph analysis (1)
test/cli/install/bun-install-patch.test.ts (1)
test/harness.ts (2)
bunExe(101-104)tempDirWithFiles(258-265)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Format
🔇 Additional comments (3)
test/cli/install/bun-install-patch.test.ts (3)
3-3: LGTM!The
rmSyncimport is correctly added for cleanup in the reinstall test scenario.
556-556: LGTM!Adding
--linker=hoistedensures this test continues to verify the intended failure scenario with the hoisted linker, since the PR changes the default to isolated for workspaces.
575-943: Well-structured test suite for isolated linker patch scenarios.The new test suite comprehensively covers patch functionality with the isolated linker:
- Basic patch creation and commit flow
- Transitive dependencies
- Scoped packages
- Workspace scenarios
- Reinstall preservation
- Multiple patches
The tests follow good patterns: consistent setup, clear verification of both patch metadata and runtime behavior, and appropriate error checking.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/install/patch_install.zig (1)
420-437: Wrong log message for stat failures.For stat errors other than
ENOENTwe now warn that the patchfile is empty, which is factually wrong and masks the real failure. Restore the prior behavior (log the underlying errno) so users see the actual issue.
♻️ Duplicate comments (1)
src/install/PackageManager/patchPackage.zig (1)
737-751: Stop swallowing filesystem errors here.
unlinkat(...).unwrap() catch { ... }andmkdir(...).unwrap() catch {}suppress all failures—even permission errors or missing parents—so a broken workspace silently proceeds and later copy will explode elsewhere. Handle only the expectedENOENTfor unlink and propagate/log anything else; likewise, propagate mkdir failures. This mirrors the feedback from the earlier review.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
src/install/PackageManager/patchPackage.zig(2 hunks)src/install/isolated_install.zig(1 hunks)src/install/isolated_install/Installer.zig(7 hunks)src/install/patch_install.zig(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/install/isolated_install.zig
🧰 Additional context used
📓 Path-based instructions (2)
src/**/*.zig
📄 CodeRabbit inference engine (.cursor/rules/building-bun.mdc)
Implement debug logs in Zig using
const log = bun.Output.scoped(.${SCOPE}, false);and invokinglog("...", .{})In Zig code, manage memory carefully and use defer for cleanup of allocations/resources
src/**/*.zig: Use the # prefix to declare private fields in Zig structs (e.g., struct { #foo: u32 })
Prefer decl literals when initializing values in Zig (e.g., const decl: Decl = .{ .binding = 0, .value = 0 })
Place @import directives at the bottom of Zig files
Use @import("bun") instead of @import("root").bun
Files:
src/install/patch_install.zigsrc/install/isolated_install/Installer.zigsrc/install/PackageManager/patchPackage.zig
**/*.zig
📄 CodeRabbit inference engine (.cursor/rules/javascriptcore-class.mdc)
**/*.zig: Declare the extern C symbol in Zig and export a Zig-friendly alias for use
Wrap the Bun____toJS extern in a Zig method that takes a JSGlobalObject and returns JSC.JSValue
Files:
src/install/patch_install.zigsrc/install/isolated_install/Installer.zigsrc/install/PackageManager/patchPackage.zig
🧠 Learnings (1)
📚 Learning: 2025-10-07T03:19:53.024Z
Learnt from: CR
PR: oven-sh/bun#0
File: src/CLAUDE.md:0-0
Timestamp: 2025-10-07T03:19:53.024Z
Learning: Applies to src/**/*.zig : Use import("bun") instead of import("root").bun
Applied to files:
src/install/isolated_install/Installer.zigsrc/install/PackageManager/patchPackage.zig
- Added detection logic to determine if using isolated or hoisted installs - Created pkgInfoForNameAndVersionIsolated() for isolated install lookups - Added findPackageInIsolatedStore() to locate packages in .bun directory - Fixed scoped package naming (@scope/name -> @scope+name in store paths) - Added Output.flush() calls to ensure error messages are printed - Updated test expectations for workspace tests to use isolated paths The patch command now correctly handles: - Hoisted installs: packages in node_modules/{name} - Isolated installs: packages in node_modules/.bun/{name}@{version}/node_modules/{name} Test results: 19/26 tests pass. The 6 failing workspace tests have a separate issue with patchfile parsing (bad_file_mode error) during the commit phase, which is unrelated to the prepare phase changes here. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
The test was calling bun patch is-odd twice - once without capturing output (redundant) and then with version is-odd@0.1.2 (the actual test). Removed the first redundant call.
…ce path handling This fixes the remaining test failures by: 1. **Add isolated install support to doPatchCommit**: The commit command now detects isolated vs hoisted mode (same logic as preparePatch) and uses pkgInfoForNameAndVersionIsolated for isolated installs. This fixes the issue where 'bun patch --commit @types/ws@8.5.4' would fail to find the package. 2. **Fix workspace package path handling**: When running patch commands from within a workspace package (not the root), paths starting with 'node_modules/.bun/' are now correctly recognized as workspace-root-relative and not adjusted. Previously, the code would incorrectly prepend the workspace package path, causing ENOENT errors. 3. **Add Output.flush() calls**: Added flush calls before Global.crash() to ensure error messages are printed before the process exits. All 26 tests in bun-patch.test.ts now pass.
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
src/install/PackageManager/patchPackage.zig(9 hunks)src/install/isolated_install/Installer.zig(7 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
src/**/*.zig
📄 CodeRabbit inference engine (.cursor/rules/building-bun.mdc)
Implement debug logs in Zig using
const log = bun.Output.scoped(.${SCOPE}, false);and invokinglog("...", .{})In Zig code, manage memory carefully and use defer for cleanup of allocations/resources
src/**/*.zig: Use the # prefix to declare private fields in Zig structs (e.g., struct { #foo: u32 })
Prefer decl literals when initializing values in Zig (e.g., const decl: Decl = .{ .binding = 0, .value = 0 })
Place @import directives at the bottom of Zig files
Use @import("bun") instead of @import("root").bun
Files:
src/install/PackageManager/patchPackage.zigsrc/install/isolated_install/Installer.zig
**/*.zig
📄 CodeRabbit inference engine (.cursor/rules/javascriptcore-class.mdc)
**/*.zig: Declare the extern C symbol in Zig and export a Zig-friendly alias for use
Wrap the Bun____toJS extern in a Zig method that takes a JSGlobalObject and returns JSC.JSValue
Files:
src/install/PackageManager/patchPackage.zigsrc/install/isolated_install/Installer.zig
🧠 Learnings (1)
📚 Learning: 2025-10-07T03:19:53.024Z
Learnt from: CR
PR: oven-sh/bun#0
File: src/CLAUDE.md:0-0
Timestamp: 2025-10-07T03:19:53.024Z
Learning: Applies to src/**/*.zig : Use import("bun") instead of import("root").bun
Applied to files:
src/install/PackageManager/patchPackage.zigsrc/install/isolated_install/Installer.zig
…eer dependencies Addresses two critical CodeRabbit review comments: 1. **Deep-clone patch logs to prevent use-after-free**: The Error.clone() method now properly deep-clones the patching log using Log.init() and cloneToWithRecycled(). Previously, it just copied the log by value, causing the clone and original to share the same backing buffer. When the worker-side instance was deinit'd, it left the queued copy pointing at freed memory. 2. **Support packages with peer dependencies in isolated store lookup**: The findPackageInIsolatedStore() function now searches the .bun directory for matching packages instead of constructing the path manually. This handles packages with peer dependency hashes like 'react-dom@18.2.0_react@18.2.0' which the previous implementation would miss. Changes: - src/install/isolated_install/Installer.zig: Deep-clone log with new buffer - src/install/PackageManager/patchPackage.zig: Search .bun directory for packages with peer hash suffixes (_peerHash or +peerHash) All 26 tests in bun-patch.test.ts continue to pass.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
src/install/PackageManager/patchPackage.zig (2)
703-711: Memory leak:relative_pathis never freed here too.Same issue as in
doPatchCommit: the allocatedinfo.relative_pathis never freed.
779-795: Error suppression on mkdir could hide real failures.Lines 787-792 suppress all errors from
unlinkat,rmdirat, andmkdiroperations. While ignoring "already exists" or "not found" errors is reasonable, permission denials or filesystem issues should be surfaced to avoid silent failures in the subsequent copy operation.Consider handling expected errors explicitly:
bun.sys.unlinkat(FD.cwd(), dest_subpath.sliceZ()).unwrap() catch { if (comptime Environment.isWindows) { bun.sys.rmdirat(FD.cwd(), dest_subpath.sliceZ()).unwrap() catch {}; } }; - bun.sys.mkdir(dest_subpath.sliceZ(), 0o755).unwrap() catch {}; + switch (bun.sys.mkdir(dest_subpath.sliceZ(), 0o755)) { + .err => |e| if (e.getErrno() != .EXIST) return e, + .result => {}, + } },
🧹 Nitpick comments (3)
src/install/PackageManager/patchPackage.zig (2)
855-921: Isolated store search improved but incomplete.This implementation addresses the previous concern about hard-coded paths by searching for peer dependency variants. However, there are limitations:
Only searches root level: The search is limited to
node_modules/.bunand doesn't check workspace-specific isolated stores, despite workspaces being the primary use case for isolated installs.Silent error handling: Returning
nullon any error (lines 887, 872) hides issues like permission problems or filesystem errors that might help diagnose failures.Performance: Iterating the entire
.bundirectory on every lookup could be slow for large projects with many packages.Consider:
- Checking workspace-specific locations (e.g.,
packages/foo/node_modules/.bun/)- Logging diagnostic information when searches fail
- Caching the directory listing if this is called frequently
1029-1034: Error message could be more diagnostic.When a package isn't found in the isolated store, the error message doesn't indicate which locations were searched, making it harder for users to diagnose the issue.
Consider enhancing the error message:
Output.prettyError( - "<r><red>error<r>: could not find the folder for <b>{s}<r> in node_modules<r>\n<r>", + "<r><red>error<r>: could not find the folder for <b>{s}<r> in isolated store (searched node_modules/.bun/)<r>\n<r>", .{pkg_maybe_version_to_patch}, );src/install/isolated_install/Installer.zig (1)
89-107: Optional: return a status instead of using an out‑param logReturning a boolean success flag simplifies the contract and reduces foot‑guns with logger ownership. Callers can still clone errors into their own log when needed.
- pub fn applyPackagePatch(this: *Installer, entry_id: Store.Entry.Id, patch: PatchInfo.Patch, log: *bun.logger.Log) void { + pub fn applyPackagePatch(this: *Installer, entry_id: Store.Entry.Id, patch: PatchInfo.Patch, log: *bun.logger.Log) bool { @@ - bun.handleOom(patch_task.apply()); + bun.handleOom(patch_task.apply()); @@ - if (patch_task.callback.apply.logger.hasErrors()) { - bun.handleOom(patch_task.callback.apply.logger.cloneToWithRecycled(log, true)); - } + if (patch_task.callback.apply.logger.hasErrors()) { + bun.handleOom(patch_task.callback.apply.logger.cloneToWithRecycled(log, true)); + return true; // had errors + } + return false; // success }Call site:
- this.applyPackagePatch(entry_id, patch_info.patch, &log); - if (log.hasErrors()) { + if (this.applyPackagePatch(entry_id, patch_info.patch, &log)) { entry_steps[entry_id.get()].store(.done, .monotonic); this.onTaskFail(entry_id, .{ .patching = log }); continue; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
src/install/PackageManager/patchPackage.zig(9 hunks)src/install/isolated_install/Installer.zig(7 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
src/**/*.zig
📄 CodeRabbit inference engine (.cursor/rules/building-bun.mdc)
Implement debug logs in Zig using
const log = bun.Output.scoped(.${SCOPE}, false);and invokinglog("...", .{})In Zig code, manage memory carefully and use defer for cleanup of allocations/resources
src/**/*.zig: Use the # prefix to declare private fields in Zig structs (e.g., struct { #foo: u32 })
Prefer decl literals when initializing values in Zig (e.g., const decl: Decl = .{ .binding = 0, .value = 0 })
Place @import directives at the bottom of Zig files
Use @import("bun") instead of @import("root").bun
Files:
src/install/PackageManager/patchPackage.zigsrc/install/isolated_install/Installer.zig
**/*.zig
📄 CodeRabbit inference engine (.cursor/rules/javascriptcore-class.mdc)
**/*.zig: Declare the extern C symbol in Zig and export a Zig-friendly alias for use
Wrap the Bun____toJS extern in a Zig method that takes a JSGlobalObject and returns JSC.JSValue
Files:
src/install/PackageManager/patchPackage.zigsrc/install/isolated_install/Installer.zig
🧠 Learnings (1)
📚 Learning: 2025-10-07T03:19:53.024Z
Learnt from: CR
PR: oven-sh/bun#0
File: src/CLAUDE.md:0-0
Timestamp: 2025-10-07T03:19:53.024Z
Learning: Applies to src/**/*.zig : Use import("bun") instead of import("root").bun
Applied to files:
src/install/PackageManager/patchPackage.zigsrc/install/isolated_install/Installer.zig
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Format
🔇 Additional comments (13)
src/install/PackageManager/patchPackage.zig (6)
60-65: LGTM! Isolated path detection is correct.The logic properly identifies paths that are already relative to the isolated store structure and should not be adjusted, with appropriate Windows path handling.
774-778: Signature simplification looks good.Removing the
managerparameter is appropriate since it's not needed by the function body.
816-817: Resource leak fix confirmed.The
defer cached_package_folder.close()immediately after opening addresses the resource leak concern from previous reviews. The directory will be properly closed even ifFileCopier.initfails.Based on previous review comments.
825-833: FileCopier usage looks correct.The
.unwrap()converts the syscall result to an error union, andtryproperly propagates any errors to the caller.
617-617: Good addition of output flushing.Adding
Output.flush()beforeGlobal.crash()ensures error messages are visible to the user.
769-769: Output flushing improves UX.Flushing output after printing instructions ensures the user sees the complete message.
src/install/isolated_install/Installer.zig (7)
46-82: Pre‑apply patch flow is solidApplies patch before task start, fail‑fast sets step to done and routes through onTaskFail; success path deinitializes the log. Looks correct and memory‑safe.
104-106: Confirm recycled‑clone + deinit interactioncloneToWithRecycled(log, true) implies transferring buffers out of the source logger. Please confirm PatchTask.deinit() will not also free or reuse that logger’s buffers post‑transfer (to avoid double‑free/UAF).
142-150: Failure logging for patching is correctPrints context, prints log, and frees it via defer. No leak; clear UX.
596-617: Explicit switch on FileCopier.copy() resultsDropping try in favor of switch improves error reporting and consistency across both paths. Good.
Also applies to: 789-810
358-358: Deep‑clone of patch log in Error.clone()Allocates a fresh logger and copies into it, preventing shared‑buffer lifetime/double‑free bugs across threads. Good safeguard.
Also applies to: 366-376
1165-1177: PatchInfo variants and accessors look correctremove/patch variants added; contentsHash/nameAndVersionHash return null for remove and the correct hash for patch. Aligns with cache key usage.
Also applies to: 1178-1191
1461-1461: Import path for FileCopierUsing bun.install.FileCopier matches the new re‑export. Consistent.
Addresses CodeRabbit review comments: 1. **Fix memory leak in doPatchCommit**: The relative_path from pkgInfoForNameAndVersionIsolated was allocated but never freed. Now we copy it into the pathbuf buffer (same lifetime as hoisted paths) and free the original allocation immediately. 2. **Fix memory leak in preparePatch**: Same issue - relative_path was allocated but never freed. Now we copy it into a local isolated_path_buf buffer and free the original allocation. 3. **Update misleading comment**: The searchIsolatedStore function comment claimed it handles workspace-specific node_modules, but it only searches the root .bun directory. Updated the comment to reflect current behavior and added a TODO for future workspace support. All 26 tests in bun-patch.test.ts continue to pass.
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
test/cli/install/bun-add.test.ts(6 hunks)test/harness.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (6)
test/**
📄 CodeRabbit inference engine (.cursor/rules/writing-tests.mdc)
Place all tests under the test/ directory
Files:
test/cli/install/bun-add.test.tstest/harness.ts
test/cli/**/*.{js,ts}
📄 CodeRabbit inference engine (.cursor/rules/writing-tests.mdc)
test/cli/**/*.{js,ts}: Place CLI command tests (e.g., bun install, bun init) under test/cli/
When testing Bun as a CLI, use spawn with bunExe() and bunEnv from harness, and capture stdout/stderr via pipes
Files:
test/cli/install/bun-add.test.ts
test/**/*.{js,ts}
📄 CodeRabbit inference engine (.cursor/rules/writing-tests.mdc)
test/**/*.{js,ts}: Write tests in JavaScript or TypeScript using Bun’s Jest-style APIs (test, describe, expect) and run with bun test
Prefer data-driven tests (e.g., test.each) to reduce boilerplate
Use shared utilities from test/harness.ts where applicable
Files:
test/cli/install/bun-add.test.tstest/harness.ts
test/**/*.test.ts
📄 CodeRabbit inference engine (test/CLAUDE.md)
test/**/*.test.ts: Name test files*.test.tsand usebun:test
Do not write flaky tests: never wait for arbitrary time; wait for conditions instead
Never hardcode port numbers in tests; useport: 0to get a random port
When spawning Bun in tests, usebunExe()andbunEnvfromharness
Preferasync/awaitin tests; for a single callback, usePromise.withResolvers()
Do not set explicit test timeouts; rely on Bun’s built-in timeouts
UsetempDir/tempDirWithFilesfromharnessfor temporary files and directories in tests
For large/repetitive strings in tests, preferBuffer.alloc(count, fill).toString()over"A".repeat(count)
Import common test utilities fromharness(e.g.,bunExe,bunEnv,tempDirWithFiles,tmpdirSync, platform checks, GC helpers)
In error tests, assert non-zero exit codes for failing processes and usetoThrowfor synchronous errors
Usedescribeblocks for grouping,describe.eachfor parameterized tests, snapshots withtoMatchSnapshot, and lifecycle hooks (beforeAll,beforeEach,afterEach); track resources for cleanup inafterEach
Useusing/await usingwith Bun resources (e.g., Bun.listen/connect/spawn/serve) to ensure cleanup in tests
Files:
test/cli/install/bun-add.test.ts
test/**/*.test.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
test/**/*.test.{ts,tsx}: Test files must live under test/ and end with .test.ts or .test.tsx
In tests, always use port: 0; do not hardcode ports or roll your own random port
Prefer normalizeBunSnapshot for snapshotting test output instead of asserting raw strings
Do not write tests that assert absence of crashes (e.g., 'no panic' or 'no uncaught exception')
Use Bun’s Jest-compatible runner (import { test, expect } from "bun:test") for tests
Avoid shell commands like find or grep in tests; use Bun’s Glob and built-in tools instead
Prefer running tests via bun bd test and use provided harness utilities (bunEnv, bunExe, tempDir)
Use Bun.spawn with proper stdio handling and await proc.exited in process-spawning tests
Files:
test/cli/install/bun-add.test.ts
test/cli/**
📄 CodeRabbit inference engine (CLAUDE.md)
Place CLI command tests under test/cli/
Files:
test/cli/install/bun-add.test.ts
🧠 Learnings (12)
📚 Learning: 2025-08-30T00:12:56.803Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/writing-tests.mdc:0-0
Timestamp: 2025-08-30T00:12:56.803Z
Learning: Applies to test/js/bun/**/*.{js,ts} : Place Bun API tests under test/js/bun/, separated by category (e.g., test/js/bun/glob/)
Applied to files:
test/cli/install/bun-add.test.ts
📚 Learning: 2025-08-30T00:12:56.803Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/writing-tests.mdc:0-0
Timestamp: 2025-08-30T00:12:56.803Z
Learning: Applies to test/cli/**/*.{js,ts} : When testing Bun as a CLI, use spawn with bunExe() and bunEnv from harness, and capture stdout/stderr via pipes
Applied to files:
test/cli/install/bun-add.test.ts
📚 Learning: 2025-09-03T17:10:13.486Z
Learnt from: CR
PR: oven-sh/bun#0
File: test/CLAUDE.md:0-0
Timestamp: 2025-09-03T17:10:13.486Z
Learning: Applies to test/**/*.test.ts : When spawning Bun in tests, use `bunExe()` and `bunEnv` from `harness`
Applied to files:
test/cli/install/bun-add.test.ts
📚 Learning: 2025-10-04T09:51:30.294Z
Learnt from: CR
PR: oven-sh/bun#0
File: CLAUDE.md:0-0
Timestamp: 2025-10-04T09:51:30.294Z
Learning: Applies to test/**/*.test.{ts,tsx} : Prefer running tests via bun bd test <file> and use provided harness utilities (bunEnv, bunExe, tempDir)
Applied to files:
test/cli/install/bun-add.test.ts
📚 Learning: 2025-10-04T09:51:30.294Z
Learnt from: CR
PR: oven-sh/bun#0
File: CLAUDE.md:0-0
Timestamp: 2025-10-04T09:51:30.294Z
Learning: Applies to test/**/*.test.{ts,tsx} : Use Bun’s Jest-compatible runner (import { test, expect } from "bun:test") for tests
Applied to files:
test/cli/install/bun-add.test.ts
📚 Learning: 2025-09-03T17:10:13.486Z
Learnt from: CR
PR: oven-sh/bun#0
File: test/CLAUDE.md:0-0
Timestamp: 2025-09-03T17:10:13.486Z
Learning: Applies to test/**/*.test.ts : Name test files `*.test.ts` and use `bun:test`
Applied to files:
test/cli/install/bun-add.test.ts
📚 Learning: 2025-10-04T09:51:30.294Z
Learnt from: CR
PR: oven-sh/bun#0
File: CLAUDE.md:0-0
Timestamp: 2025-10-04T09:51:30.294Z
Learning: Applies to test/**/*.test.{ts,tsx} : Use Bun.spawn with proper stdio handling and await proc.exited in process-spawning tests
Applied to files:
test/cli/install/bun-add.test.ts
📚 Learning: 2025-09-03T17:10:13.486Z
Learnt from: CR
PR: oven-sh/bun#0
File: test/CLAUDE.md:0-0
Timestamp: 2025-09-03T17:10:13.486Z
Learning: Applies to test/**/*.test.ts : Import common test utilities from `harness` (e.g., `bunExe`, `bunEnv`, `tempDirWithFiles`, `tmpdirSync`, platform checks, GC helpers)
Applied to files:
test/cli/install/bun-add.test.ts
📚 Learning: 2025-08-30T00:12:56.803Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/writing-tests.mdc:0-0
Timestamp: 2025-08-30T00:12:56.803Z
Learning: Applies to test/**/*.{js,ts} : Write tests in JavaScript or TypeScript using Bun’s Jest-style APIs (test, describe, expect) and run with bun test
Applied to files:
test/cli/install/bun-add.test.ts
📚 Learning: 2025-10-04T09:51:30.294Z
Learnt from: CR
PR: oven-sh/bun#0
File: CLAUDE.md:0-0
Timestamp: 2025-10-04T09:51:30.294Z
Learning: Applies to test/**/*.test.{ts,tsx} : Avoid shell commands like find or grep in tests; use Bun’s Glob and built-in tools instead
Applied to files:
test/cli/install/bun-add.test.ts
📚 Learning: 2025-08-30T00:09:39.100Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/dev-server-tests.mdc:0-0
Timestamp: 2025-08-30T00:09:39.100Z
Learning: Applies to test/bake/{dev/*.test.ts,dev-and-prod.ts} : Import testing utilities (devTest, prodTest, devAndProductionTest, Dev, Client) from test/bake/bake-harness.ts
Applied to files:
test/cli/install/bun-add.test.ts
📚 Learning: 2025-09-03T17:10:13.486Z
Learnt from: CR
PR: oven-sh/bun#0
File: test/CLAUDE.md:0-0
Timestamp: 2025-09-03T17:10:13.486Z
Learning: Applies to test/**/*.test.ts : Use `using`/`await using` with Bun resources (e.g., Bun.listen/connect/spawn/serve) to ensure cleanup in tests
Applied to files:
test/cli/install/bun-add.test.ts
🧬 Code graph analysis (2)
test/cli/install/bun-add.test.ts (1)
test/harness.ts (1)
readdirSorted(1767-1771)
test/harness.ts (1)
src/node-fallbacks/path.js (1)
isAbsolute(168-171)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Format
🔇 Additional comments (2)
test/cli/install/bun-add.test.ts (2)
1068-1070: Workspace dependency wiring looks correct.The addition of explicit workspace dependency
"bar": "workspace:*"in the test setup (lines 1068-1070) with corresponding assertion (line 1124) properly validates workspace dependency resolution in the context ofbun add.Also applies to: 1124-1124
2132-2132: Windows-specific validation change looks appropriate.The Windows branch now reads and validates the
package.jsoncontent directly instead of usingreadlink, which is more reliable for verifying that workspace dependencies are correctly installed on Windows.
| expect(urls.sort()).toEqual([`${root_url}/baz`, `${root_url}/baz-0.0.3.tgz`]); | ||
| expect(requested).toBe(2); | ||
| expect(await readdirSorted(join(package_dir, "node_modules"))).toEqual([".bin", ".cache", "bar", "baz"]); | ||
| expect(await readdirSorted(join(package_dir, "node_modules"))).toEqual([".bin", ".bun", ".cache", "bar", "baz"]); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
Verify .bun directory expectations align with isolated install.
The tests now expect a .bun directory in node_modules (lines 1104, 2143), which appears to be related to the isolated install strategy becoming the default.
Confirm that:
- The
.bundirectory is consistently present with--linker=isolated - Its absence with
--linker=hoistedwon't break these tests (or that the tests properly account for linker mode)
Run the following to check how .bun directory usage relates to linker configuration:
Also applies to: 2143-2143
🏁 Script executed:
#!/bin/bash
# Check for .bun directory expectations across install tests
rg -n '\.bun' test/cli/install/*.test.ts | head -20
# Verify if tests set linker mode explicitly
rg -n 'linker.*isolated' test/cli/install/bun-add.test.tsLength of output: 2241
🏁 Script executed:
#!/bin/bash
echo "Lines 1080-1150 in test/cli/install/bun-add.test.ts"
sed -n '1080,1150p' test/cli/install/bun-add.test.ts
echo "All .bun references in bun-add.test.ts"
rg -n '\.bun' test/cli/install/bun-add.test.ts
echo "Linker flags or 'isolated' mentions in bun-add.test.ts"
rg -n '--linker' -n 'isolated' test/cli/install/bun-add.test.tsLength of output: 3153
Make tests explicit about isolated linker mode
In test/cli/install/bun-add.test.ts (lines 1104, 2143), the assertions expect a .bun folder in node_modules—which only appears under the isolated linker. Update the spawn({ cmd: [...] }) calls for bun add to include --linker=isolated so the tests don’t rely on the default linker mode.
🤖 Prompt for AI Agents
In test/cli/install/bun-add.test.ts around lines 1104 and 2143, the assertions
expect a .bun folder in node_modules which only exists when using the isolated
linker; update the corresponding spawn({ cmd: [...] }) calls that invoke `bun
add` to include the flag `--linker=isolated` (add it alongside existing args) so
the test explicitly runs in isolated linker mode and no longer depends on
default linker behavior; ensure both occurrences are modified.
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
src/install/isolated_install.zig(2 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.zig
📄 CodeRabbit inference engine (.cursor/rules/javascriptcore-class.mdc)
**/*.zig: Declare the extern C symbol in Zig and export a Zig-friendly alias for use
Wrap the Bun____toJS extern in a Zig method that takes a JSGlobalObject and returns JSC.JSValue
Files:
src/install/isolated_install.zig
src/**/*.zig
📄 CodeRabbit inference engine (CLAUDE.md)
In Zig code, manage memory carefully and use defer for cleanup of allocations/resources
src/**/*.zig: Use the # prefix to declare private fields in Zig structs (e.g., struct { #foo: u32 })
Prefer decl literals when initializing values in Zig (e.g., const decl: Decl = .{ .binding = 0, .value = 0 })
Place @import directives at the bottom of Zig files
Use @import("bun") instead of @import("root").bunWhen adding debug logs in Zig, create a scoped logger and log via Bun APIs:
const log = bun.Output.scoped(.${SCOPE}, .hidden);thenlog("...", .{})
Files:
src/install/isolated_install.zig
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Format
| const is_new_bun_modules = is_new_bun_modules: { | ||
| const node_modules_path = bun.OSPathLiteral("node_modules"); | ||
| const bun_modules_path = bun.OSPathLiteral("node_modules/" ++ Store.modules_dir_name); | ||
|
|
||
| sys.mkdirat(cwd, node_modules_path, 0o755).unwrap() catch { | ||
| sys.mkdirat(cwd, bun_modules_path, 0o755).unwrap() catch { | ||
| break :is_new_bun_modules false; | ||
| }; | ||
|
|
||
| // existing node_modules and missing node_modules/.bun folder. move and delete all | ||
| // node_modules to prevent hoisted package pollution | ||
|
|
||
| var delete_path: bun.AutoRelPath = .init(); | ||
| defer delete_path.deinit(); | ||
|
|
||
| delete_path.append("node_modules"); | ||
|
|
||
| { | ||
| const node_modules = bun.openDirForIterationOSPath(cwd, node_modules_path).unwrap() catch |err| { | ||
| Output.err(err, "failed to open './node_modules'", .{}); | ||
| Global.exit(1); | ||
| }; | ||
| defer node_modules.close(); | ||
|
|
||
| var node_modules_iter = bun.DirIterator.iterate(node_modules, .u8); | ||
| while (node_modules_iter.next().unwrap() catch |err| { | ||
| Output.err(err, "failed to read './node_modules'", .{}); | ||
| Global.exit(1); | ||
| }) |entry| { | ||
| if (bun.strings.startsWithChar(entry.name.slice(), '.')) { | ||
| continue; | ||
| } | ||
|
|
||
| var delete_path_save = delete_path.save(); | ||
| defer delete_path_save.restore(); | ||
| delete_path.append(entry.name.slice()); | ||
|
|
||
| installer.startDeleteTask(delete_path.sliceZ()); | ||
| } | ||
| } | ||
|
|
||
| for (lockfile.workspace_paths.values()) |workspace_path| { | ||
| delete_path.clear(); | ||
| delete_path.append(workspace_path.slice(lockfile.buffers.string_bytes.items)); | ||
| delete_path.append("node_modules"); | ||
|
|
||
| installer.startDeleteTask(delete_path.sliceZ()); | ||
| } | ||
|
|
||
| break :is_new_bun_modules true; | ||
| }; | ||
|
|
||
| sys.mkdirat(cwd, bun_modules_path, 0o755).unwrap() catch |err| { | ||
| Output.err(err, "failed to create './node_modules/.bun' directory", .{}); | ||
| Global.exit(1); | ||
| }; | ||
|
|
||
| break :is_new_bun_modules true; | ||
| }; |
There was a problem hiding this comment.
🧩 Analysis chain
Cleanup preserves .bin and other hidden dirs; delete everything except .bun (and “.”/“..”).
Skipping all dot-entries leaves stale hoisted artifacts (e.g., node_modules/.bin), defeating the isolation cleanup. Only .bun should be preserved.
Apply this focused change:
- if (bun.strings.startsWithChar(entry.name.slice(), '.')) {
- continue;
- }
+ const name = entry.name.slice();
+ // Keep only the .bun directory; delete everything else
+ if (std.mem.eql(u8, name, Store.modules_dir_name)) {
+ continue;
+ }
+ // Be defensive if iterator yields "." or ".."
+ if ((name.len == 1 and name[0] == '.') or (name.len == 2 and name[0] == '.' and name[1] == '.')) {
+ continue;
+ }Also verify:
- startDeleteTask copies the provided path; delete_path is reused and mutated after scheduling, which will dangle if the task holds the slice as-is.
- mkdirat error handling: in the “node_modules exists” branch, non-EEXIST failures to create
.buncurrently fall through as “not new.” Consider treating non-EEXIST as fatal (log+exit), consistent with the other branch.
🏁 Script executed:
#!/bin/bash
echo ">> startDeleteTask definition:"
rg -n 'fn startDeleteTask' -n src
echo
echo ">> mkdirat declaration:"
rg -n 'pub fn mkdirat' -n srcLength of output: 495
🏁 Script executed:
#!/bin/bash
echo ">> startDeleteTask implementation (20 lines around):"
rg -n 'pub fn startDeleteTask' -n -C10 src/install/isolated_install/Installer.zigLength of output: 821
🏁 Script executed:
#!/bin/bash
echo ">> startDeleteTask (+100 lines):"
rg -n 'pub fn startDeleteTask' -n -A100 src/install/isolated_install/Installer.zig
echo
echo ">> mkdiratZ implementation (+20 lines):"
rg -n 'pub fn mkdiratZ' -n -C20 src/sys.zigLength of output: 6148
Tighten cleanup filter and enforce fatal errors
-
In src/install/isolated_install.zig’s
is_new_bun_modulesblock, replace the “skip all dot-entries” check with logic that preserves only the.bundirectory (and skips “.”/“..”):- if (bun.strings.startsWithChar(entry.name.slice(), '.')) { - continue; - } + const name = entry.name.slice(); + // Keep only the .bun directory (and skip "."/"..") + if (std.mem.eql(u8, name, Store.modules_dir_name) + or (name.len == 1 and name[0] == '.') + or (name.len == 2 and name[0] == '.' and name[1] == '.')) { + continue; + }
-
Only treat EEXIST as “already present” in both
mkdirat(cwd, "node_modules")andmkdirat(cwd, "node_modules/.bun"); on any other error, log viaOutput.errandGlobal.exit(1). -
After a successful
applyPackagePatch, adddefer log.deinit();(or calllog.deinit()) to free thebun.logger.Logon success.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const is_new_bun_modules = is_new_bun_modules: { | |
| const node_modules_path = bun.OSPathLiteral("node_modules"); | |
| const bun_modules_path = bun.OSPathLiteral("node_modules/" ++ Store.modules_dir_name); | |
| sys.mkdirat(cwd, node_modules_path, 0o755).unwrap() catch { | |
| sys.mkdirat(cwd, bun_modules_path, 0o755).unwrap() catch { | |
| break :is_new_bun_modules false; | |
| }; | |
| // existing node_modules and missing node_modules/.bun folder. move and delete all | |
| // node_modules to prevent hoisted package pollution | |
| var delete_path: bun.AutoRelPath = .init(); | |
| defer delete_path.deinit(); | |
| delete_path.append("node_modules"); | |
| { | |
| const node_modules = bun.openDirForIterationOSPath(cwd, node_modules_path).unwrap() catch |err| { | |
| Output.err(err, "failed to open './node_modules'", .{}); | |
| Global.exit(1); | |
| }; | |
| defer node_modules.close(); | |
| var node_modules_iter = bun.DirIterator.iterate(node_modules, .u8); | |
| while (node_modules_iter.next().unwrap() catch |err| { | |
| Output.err(err, "failed to read './node_modules'", .{}); | |
| Global.exit(1); | |
| }) |entry| { | |
| if (bun.strings.startsWithChar(entry.name.slice(), '.')) { | |
| continue; | |
| } | |
| var delete_path_save = delete_path.save(); | |
| defer delete_path_save.restore(); | |
| delete_path.append(entry.name.slice()); | |
| installer.startDeleteTask(delete_path.sliceZ()); | |
| } | |
| } | |
| for (lockfile.workspace_paths.values()) |workspace_path| { | |
| delete_path.clear(); | |
| delete_path.append(workspace_path.slice(lockfile.buffers.string_bytes.items)); | |
| delete_path.append("node_modules"); | |
| installer.startDeleteTask(delete_path.sliceZ()); | |
| } | |
| break :is_new_bun_modules true; | |
| }; | |
| sys.mkdirat(cwd, bun_modules_path, 0o755).unwrap() catch |err| { | |
| Output.err(err, "failed to create './node_modules/.bun' directory", .{}); | |
| Global.exit(1); | |
| }; | |
| break :is_new_bun_modules true; | |
| }; | |
| var node_modules_iter = bun.DirIterator.iterate(node_modules, .u8); | |
| while (node_modules_iter.next().unwrap() catch |err| { | |
| Output.err(err, "failed to read './node_modules'", .{}); | |
| Global.exit(1); | |
| }) |entry| { | |
| const name = entry.name.slice(); | |
| // Keep only the .bun directory (and skip "."/"..") | |
| if (std.mem.eql(u8, name, Store.modules_dir_name) | |
| or (name.len == 1 and name[0] == '.') | |
| or (name.len == 2 and name[0] == '.' and name[1] == '.')) { | |
| continue; | |
| } | |
| var delete_path_save = delete_path.save(); | |
| defer delete_path_save.restore(); | |
| delete_path.append(entry.name.slice()); | |
| installer.startDeleteTask(delete_path.sliceZ()); | |
| } |
🤖 Prompt for AI Agents
In src/install/isolated_install.zig around lines 636-694, tighten the
directory-cleanup filter and make mkdir errors fatal: replace the current "skip
all dot-entries" check when iterating node_modules with logic that only
preserves the ".bun" directory while still skipping "." and ".." (i.e., skip
entries that are "." or "..", and skip non-".bun" dot-prefixed names only if
they are not ".bun"); change both sys.mkdirat calls (for "node_modules" and
"node_modules/.bun") to treat only EEXIST as a non-fatal condition and on any
other error call Output.err(...) and Global.exit(1); and after a successful
applyPackagePatch ensure the bun.logger.Log is freed by adding a defer
log.deinit() (or calling log.deinit()) on the success path.
| if (patch_info == .patch) { | ||
| var patch_log: bun.logger.Log = .init(manager.allocator); | ||
| installer.applyPackagePatch(entry_id, patch_info.patch, &patch_log); | ||
| if (patch_log.hasErrors()) { | ||
| // monotonic is okay because we haven't started the task yet (it isn't running | ||
| // on another thread) | ||
| entry_steps[entry_id.get()].store(.done, .monotonic); | ||
| installer.onTaskFail(entry_id, .{ .patching = patch_log }); | ||
| continue; | ||
| } | ||
| } |
There was a problem hiding this comment.
patch_log not freed on success (memory leak).
Deinit the log when no errors to avoid leaking allocations.
- var patch_log: bun.logger.Log = .init(manager.allocator);
- installer.applyPackagePatch(entry_id, patch_info.patch, &patch_log);
- if (patch_log.hasErrors()) {
+ var patch_log: bun.logger.Log = .init(manager.allocator);
+ installer.applyPackagePatch(entry_id, patch_info.patch, &patch_log);
+ if (patch_log.hasErrors()) {
// monotonic is okay because we haven't started the task yet (it isn't running
// on another thread)
entry_steps[entry_id.get()].store(.done, .monotonic);
installer.onTaskFail(entry_id, .{ .patching = patch_log });
continue;
- }
+ } else {
+ patch_log.deinit();
+ }Additionally, confirm ownership: if onTaskFail assumes ownership of patch_log, ensure no deinit happens on the error path elsewhere.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (patch_info == .patch) { | |
| var patch_log: bun.logger.Log = .init(manager.allocator); | |
| installer.applyPackagePatch(entry_id, patch_info.patch, &patch_log); | |
| if (patch_log.hasErrors()) { | |
| // monotonic is okay because we haven't started the task yet (it isn't running | |
| // on another thread) | |
| entry_steps[entry_id.get()].store(.done, .monotonic); | |
| installer.onTaskFail(entry_id, .{ .patching = patch_log }); | |
| continue; | |
| } | |
| } | |
| if (patch_info == .patch) { | |
| var patch_log: bun.logger.Log = .init(manager.allocator); | |
| installer.applyPackagePatch(entry_id, patch_info.patch, &patch_log); | |
| if (patch_log.hasErrors()) { | |
| // monotonic is okay because we haven't started the task yet (it isn't running | |
| // on another thread) | |
| entry_steps[entry_id.get()].store(.done, .monotonic); | |
| installer.onTaskFail(entry_id, .{ .patching = patch_log }); | |
| continue; | |
| } else { | |
| patch_log.deinit(); | |
| } | |
| } |
🤖 Prompt for AI Agents
In src/install/isolated_install.zig around lines 841-851, the temporary
patch_log created for patch application is not deinitialized on the success path
causing a memory leak; free its resources by calling patch_log.deinit() after
the conditional branch when patch_log.hasErrors() is false (i.e., after continue
is not taken), and ensure you do NOT deinit it on the error path because
installer.onTaskFail(entry_id, .{ .patching = patch_log }) must take ownership
of the log—if onTaskFail currently expects ownership, leave the error path
as-is; otherwise adjust onTaskFail signature/usage so ownership is clear before
adding the deinit.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/bun.js/RuntimeTranspilerCache.zig (1)
165-172: Bug: passing extension with leading “.” yields double-dot filenamesstd.fs.path.extension returns the extension including the leading dot. With tmpname formatting ".{…}-{…}.{s}", this produces names like "..pile". Strip the dot before calling tmpname.
Apply this fix:
- const tmpfilename = try bun.fs.FileSystem.tmpname(std.fs.path.extension(destination_path.slice()), &tmpname_buf, input_hash); + const ext_with_dot = std.fs.path.extension(destination_path.slice()); + const ext = if (ext_with_dot.len > 0 and ext_with_dot[0] == '.') ext_with_dot[1..] else ext_with_dot; + const tmpfilename = try bun.fs.FileSystem.tmpname(ext, &tmpname_buf, input_hash);src/install/PackageManager.zig (1)
290-296: Allocator mismatch: resolution_label allocated with default_allocator, freed with manager.allocatorThis will corrupt memory or leak under the wrong allocator. Allocate with manager.allocator or free with bun.default_allocator to match.
Apply one of these diffs (prefer A):
- break :brk .{ bun.handleOom(std.fmt.allocPrint(bun.default_allocator, "{}", .{resolution.fmt(this.manager.lockfile.buffers.string_bytes.items, .posix)})), resolution.tag }; + break :brk .{ bun.handleOom(std.fmt.allocPrint(this.manager.allocator, "{}", .{resolution.fmt(this.manager.lockfile.buffers.string_bytes.items, .posix)})), resolution.tag };or
- defer this.manager.allocator.free(resolution_label); + defer bun.default_allocator.free(resolution_label);src/install/extract_tarball.zig (1)
349-355: Guard against buffer overrun when appending "tmp"We write 4 bytes past tmpname_len. While PathBuffer is large, add a quick bound check/assert to prevent accidental overflow if tmpname were ever at capacity.
Suggested guard:
- var tmpname_bytes = std.mem.asBytes(&tmpname_buf); - const tmpname_len = tmpname.len; + var tmpname_bytes = std.mem.asBytes(&tmpname_buf); + const tmpname_len = tmpname.len; + if (comptime bun.Environment.allow_assert) { + bun.assert(tmpname_len + 4 <= tmpname_bytes.len); + }
♻️ Duplicate comments (3)
src/install/patch_install.zig (1)
419-437: Non-NOENT stat error handling still incorrect (duplicate of prior review)On errors other than NOENT, this still logs “patchfile is empty,” which is wrong. Use an explicit “failed to stat patch file” error for non-NOENT and reserve the “empty” message for size==0.
Consider applying the previously suggested fix.
src/install/isolated_install.zig (2)
638-641: Make mkdir failures explicit; only treat EEXIST as non-fatal.Lines 638–639 silently suppress all mkdir errors and break to
false, which can hide real filesystem problems (permissions, disk full, etc.). Later mkdir calls at lines 661, 710, and 718 correctly treat non-EEXIST errors as fatal. Apply the same pattern here for consistency.Apply this diff:
- sys.mkdirat(FD.cwd(), node_modules_path, 0o755).unwrap() catch { - sys.mkdirat(FD.cwd(), bun_modules_path, 0o755).unwrap() catch { - break :is_new_bun_modules false; - }; + sys.mkdirat(FD.cwd(), node_modules_path, 0o755).unwrap() catch |err| { + if (err != bun.sys.Error.EEXIST) { + Output.err(err, "failed to create './node_modules'", .{}); + Global.exit(1); + } + sys.mkdirat(FD.cwd(), bun_modules_path, 0o755).unwrap() catch |err2| { + if (err2 != bun.sys.Error.EEXIST) { + Output.err(err2, "failed to create './node_modules/.bun'", .{}); + Global.exit(1); + } + break :is_new_bun_modules false; + };Based on past review comments.
871-881: Free patch_log on success to prevent memory leak.The
patch_logis initialized on line 872 but only deinitialized implicitly when passed toonTaskFailon error (line 878). On the success path, the log's allocations are leaked.Apply this diff:
if (patch_info == .patch) { var patch_log: bun.logger.Log = .init(manager.allocator); installer.applyPackagePatch(entry_id, patch_info.patch, &patch_log); if (patch_log.hasErrors()) { // monotonic is okay because we haven't started the task yet (it isn't running // on another thread) entry_steps[entry_id.get()].store(.done, .monotonic); installer.onTaskFail(entry_id, .{ .patching = patch_log }); continue; + } else { + patch_log.deinit(); } }Verify that
onTaskFailtakes ownership ofpatch_logon the error path so no deinit is needed there. Based on past review comments.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (14)
src/StandaloneModuleGraph.zig(1 hunks)src/bun.js/ModuleLoader.zig(1 hunks)src/bun.js/RuntimeTranspilerCache.zig(1 hunks)src/compile_target.zig(1 hunks)src/fs.zig(1 hunks)src/install/PackageManager.zig(1 hunks)src/install/PackageManager/PackageManagerDirectories.zig(1 hunks)src/install/PackageManager/patchPackage.zig(5 hunks)src/install/extract_tarball.zig(5 hunks)src/install/isolated_install.zig(2 hunks)src/install/patch_install.zig(4 hunks)src/paths/Path.zig(0 hunks)test/cli/install/bun-add.test.ts(5 hunks)test/cli/install/isolated-install.test.ts(1 hunks)
💤 Files with no reviewable changes (1)
- src/paths/Path.zig
🧰 Additional context used
📓 Path-based instructions (9)
**/*.zig
📄 CodeRabbit inference engine (.cursor/rules/javascriptcore-class.mdc)
**/*.zig: Declare the extern C symbol in Zig and export a Zig-friendly alias for use
Wrap the Bun____toJS extern in a Zig method that takes a JSGlobalObject and returns JSC.JSValue
Files:
src/install/PackageManager/PackageManagerDirectories.zigsrc/bun.js/RuntimeTranspilerCache.zigsrc/install/PackageManager.zigsrc/fs.zigsrc/StandaloneModuleGraph.zigsrc/install/patch_install.zigsrc/compile_target.zigsrc/install/extract_tarball.zigsrc/install/isolated_install.zigsrc/bun.js/ModuleLoader.zigsrc/install/PackageManager/patchPackage.zig
src/**/*.zig
📄 CodeRabbit inference engine (CLAUDE.md)
In Zig code, manage memory carefully and use defer for cleanup of allocations/resources
src/**/*.zig: Use the # prefix to declare private fields in Zig structs (e.g., struct { #foo: u32 })
Prefer decl literals when initializing values in Zig (e.g., const decl: Decl = .{ .binding = 0, .value = 0 })
Place @import directives at the bottom of Zig files
Use @import("bun") instead of @import("root").bunWhen adding debug logs in Zig, create a scoped logger and log via Bun APIs:
const log = bun.Output.scoped(.${SCOPE}, .hidden);thenlog("...", .{})
Files:
src/install/PackageManager/PackageManagerDirectories.zigsrc/bun.js/RuntimeTranspilerCache.zigsrc/install/PackageManager.zigsrc/fs.zigsrc/StandaloneModuleGraph.zigsrc/install/patch_install.zigsrc/compile_target.zigsrc/install/extract_tarball.zigsrc/install/isolated_install.zigsrc/bun.js/ModuleLoader.zigsrc/install/PackageManager/patchPackage.zig
src/bun.js/**/*.zig
📄 CodeRabbit inference engine (.cursor/rules/zig-javascriptcore-classes.mdc)
src/bun.js/**/*.zig: In Zig binding structs, expose generated bindings via pub const js = JSC.Codegen.JS and re-export toJS/fromJS/fromJSDirect
Constructors and prototype methods should return bun.JSError!JSC.JSValue to integrate Zig error handling with JS exceptions
Use parameter name globalObject (not ctx) and accept (*JSC.JSGlobalObject, *JSC.CallFrame) in binding methods/constructors
Implement getters as get(this, globalObject) returning JSC.JSValue and matching the .classes.ts interface
Provide deinit() for resource cleanup and finalize() that calls deinit(); use bun.destroy(this) or appropriate destroy pattern
Access JS call data via CallFrame (argument(i), argumentCount(), thisValue()) and throw errors with globalObject.throw(...)
For properties marked cache: true, use the generated Zig accessors (NameSetCached/GetCached) to work with GC-owned values
In finalize() for objects holding JS references, release them using .deref() before destroy
Files:
src/bun.js/RuntimeTranspilerCache.zigsrc/bun.js/ModuleLoader.zig
test/**
📄 CodeRabbit inference engine (.cursor/rules/writing-tests.mdc)
Place all tests under the test/ directory
Files:
test/cli/install/isolated-install.test.tstest/cli/install/bun-add.test.ts
test/cli/**/*.{js,ts}
📄 CodeRabbit inference engine (.cursor/rules/writing-tests.mdc)
test/cli/**/*.{js,ts}: Place CLI command tests (e.g., bun install, bun init) under test/cli/
When testing Bun as a CLI, use spawn with bunExe() and bunEnv from harness, and capture stdout/stderr via pipes
Files:
test/cli/install/isolated-install.test.tstest/cli/install/bun-add.test.ts
test/**/*.{js,ts}
📄 CodeRabbit inference engine (.cursor/rules/writing-tests.mdc)
test/**/*.{js,ts}: Write tests in JavaScript or TypeScript using Bun’s Jest-style APIs (test, describe, expect) and run with bun test
Prefer data-driven tests (e.g., test.each) to reduce boilerplate
Use shared utilities from test/harness.ts where applicable
Files:
test/cli/install/isolated-install.test.tstest/cli/install/bun-add.test.ts
test/**/*.test.ts
📄 CodeRabbit inference engine (test/CLAUDE.md)
test/**/*.test.ts: Name test files*.test.tsand usebun:test
Do not write flaky tests: never wait for arbitrary time; wait for conditions instead
Never hardcode port numbers in tests; useport: 0to get a random port
When spawning Bun in tests, usebunExe()andbunEnvfromharness
Preferasync/awaitin tests; for a single callback, usePromise.withResolvers()
Do not set explicit test timeouts; rely on Bun’s built-in timeouts
UsetempDir/tempDirWithFilesfromharnessfor temporary files and directories in tests
For large/repetitive strings in tests, preferBuffer.alloc(count, fill).toString()over"A".repeat(count)
Import common test utilities fromharness(e.g.,bunExe,bunEnv,tempDirWithFiles,tmpdirSync, platform checks, GC helpers)
In error tests, assert non-zero exit codes for failing processes and usetoThrowfor synchronous errors
Usedescribeblocks for grouping,describe.eachfor parameterized tests, snapshots withtoMatchSnapshot, and lifecycle hooks (beforeAll,beforeEach,afterEach); track resources for cleanup inafterEach
Useusing/await usingwith Bun resources (e.g., Bun.listen/connect/spawn/serve) to ensure cleanup in tests
Files:
test/cli/install/isolated-install.test.tstest/cli/install/bun-add.test.ts
test/**/*.test.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
test/**/*.test.{ts,tsx}: Test files must live under test/ and end with .test.ts or .test.tsx
In tests, always use port: 0; do not hardcode ports or roll your own random port
Prefer normalizeBunSnapshot for snapshotting test output instead of asserting raw strings
Do not write tests that assert absence of crashes (e.g., 'no panic' or 'no uncaught exception')
Use Bun’s Jest-compatible runner (import { test, expect } from "bun:test") for tests
Avoid shell commands like find or grep in tests; use Bun’s Glob and built-in tools instead
Prefer running tests via bun bd test and use provided harness utilities (bunEnv, bunExe, tempDir)
Use Bun.spawn with proper stdio handling and await proc.exited in process-spawning tests
Files:
test/cli/install/isolated-install.test.tstest/cli/install/bun-add.test.ts
test/cli/**
📄 CodeRabbit inference engine (CLAUDE.md)
Place CLI command tests under test/cli/
Files:
test/cli/install/isolated-install.test.tstest/cli/install/bun-add.test.ts
🧠 Learnings (7)
📚 Learning: 2025-08-30T00:13:36.815Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/zig-javascriptcore-classes.mdc:0-0
Timestamp: 2025-08-30T00:13:36.815Z
Learning: Applies to src/bun.js/**/*.zig : For properties marked cache: true, use the generated Zig accessors (<Prop>NameSetCached/GetCached) to work with GC-owned values
Applied to files:
src/bun.js/RuntimeTranspilerCache.zig
📚 Learning: 2025-08-30T00:12:56.803Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/writing-tests.mdc:0-0
Timestamp: 2025-08-30T00:12:56.803Z
Learning: Applies to test/js/bun/**/*.{js,ts} : Place Bun API tests under test/js/bun/, separated by category (e.g., test/js/bun/glob/)
Applied to files:
test/cli/install/bun-add.test.ts
📚 Learning: 2025-09-03T17:10:13.486Z
Learnt from: CR
PR: oven-sh/bun#0
File: test/CLAUDE.md:0-0
Timestamp: 2025-09-03T17:10:13.486Z
Learning: Applies to test/**/*.test.ts : When spawning Bun in tests, use `bunExe()` and `bunEnv` from `harness`
Applied to files:
test/cli/install/bun-add.test.ts
📚 Learning: 2025-08-30T00:12:56.803Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/writing-tests.mdc:0-0
Timestamp: 2025-08-30T00:12:56.803Z
Learning: Applies to test/cli/**/*.{js,ts} : When testing Bun as a CLI, use spawn with bunExe() and bunEnv from harness, and capture stdout/stderr via pipes
Applied to files:
test/cli/install/bun-add.test.ts
📚 Learning: 2025-10-04T09:51:30.294Z
Learnt from: CR
PR: oven-sh/bun#0
File: CLAUDE.md:0-0
Timestamp: 2025-10-04T09:51:30.294Z
Learning: Applies to test/**/*.test.{ts,tsx} : Prefer running tests via bun bd test <file> and use provided harness utilities (bunEnv, bunExe, tempDir)
Applied to files:
test/cli/install/bun-add.test.ts
📚 Learning: 2025-10-07T03:19:53.031Z
Learnt from: CR
PR: oven-sh/bun#0
File: src/CLAUDE.md:0-0
Timestamp: 2025-10-07T03:19:53.031Z
Learning: Applies to src/**/*.zig : Use import("bun") instead of import("root").bun
Applied to files:
src/bun.js/ModuleLoader.zigsrc/install/PackageManager/patchPackage.zig
📚 Learning: 2025-10-04T09:51:30.294Z
Learnt from: CR
PR: oven-sh/bun#0
File: CLAUDE.md:0-0
Timestamp: 2025-10-04T09:51:30.294Z
Learning: Applies to src/js/bun/** : Organize Bun-specific built-in modules under src/js/bun/ (e.g., bun:ffi, bun:sqlite)
Applied to files:
src/bun.js/ModuleLoader.zig
🧬 Code graph analysis (2)
test/cli/install/isolated-install.test.ts (1)
test/harness.ts (3)
bunExe(102-105)bunEnv(49-64)readdirSorted(1768-1772)
test/cli/install/bun-add.test.ts (1)
test/harness.ts (1)
readdirSorted(1768-1772)
🔇 Additional comments (13)
src/fs.zig (1)
50-58: Static tmpname API + precise error type LGTMSignature change and bufPrintZ usage look correct; atomic counter ensures uniqueness.
src/StandaloneModuleGraph.zig (1)
543-546: Correct migration to new tmpname APIDirect try on FileSystem.tmpname with clear error path is good.
test/cli/install/isolated-install.test.ts (1)
535-582: Good coverage for .bun bootstrap/migrationAssertions are stable (sorted, stringContaining), and use bunExe/bunEnv harness.
src/compile_target.zig (1)
249-253: tmpname refactor usage LGTMUsing FileSystem.tmpname("tmp", …, fastRandom/hash) is consistent and safe.
test/cli/install/bun-add.test.ts (2)
1067-1069: Aligns tests with isolated-by-default for workspacesAdding the workspace dep and expecting .bun/.old_modules is appropriate.
Also applies to: 1103-1110
2149-2153: Expectation update for migration artifacts LGTMUsing stringContaining(".old_modules-") with sorted directory listing is robust.
src/install/PackageManager/PackageManagerDirectories.zig (1)
44-44: tmpname refactor looks good; confirm Z-slice across platformsSwitch to Fs.FileSystem.tmpname is consistent. Please confirm it returns a zero-terminated slice on all platforms, since createFileZ/renameatZ expect Z strings.
src/bun.js/ModuleLoader.zig (1)
41-41: LGTM: use static FileSystem.tmpnameDirect call removes unnecessary slicing; matches repo-wide refactor.
src/install/PackageManager.zig (1)
457-457: LGTM: tmpname API usageUsing Fs.FileSystem.tmpname here aligns with the new API.
src/install/extract_tarball.zig (1)
148-152: tmpname + makeOpenPath change is correctPassing basename (capped) into FileSystem.tmpname and creating the temp dir via MakePath is consistent with the new API.
src/install/patch_install.zig (3)
150-156: OK to reduce noisy output on hash failureConditional logging under non-silent mode is reasonable.
274-279: LGTM: use FileSystem.tmpname for patch tempdirThe temp dir name generation now matches the repo-wide tmpname API.
296-312: Verify no aliasing hazards with destination_dir_subpath_bufdestination_dir_subpath and destination_dir_subpath_buf point to the same underlying buffer. Ensure install() doesn’t mutate beyond the intended slice or require an independent scratch buffer.
| Output.prettyError("<r><red>error<r>: copying file {}", .{e}); | ||
| Global.crash(); | ||
| } | ||
| FD.cwd().deleteTree(node_modules_folder_path) catch {}; |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Avoid silently suppressing deleteTree errors.
Line 736 swallows all errors from deleteTree, which could hide real filesystem issues like permission denials or I/O failures. While ignoring "not found" might be acceptable, suppressing all errors makes debugging harder.
Consider handling expected errors explicitly:
- FD.cwd().deleteTree(node_modules_folder_path) catch {};
+ FD.cwd().deleteTree(node_modules_folder_path) catch |err| {
+ // Ignore if directory doesn't exist
+ switch (err) {
+ error.FileNotFound => {},
+ else => return err,
+ }
+ };📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| FD.cwd().deleteTree(node_modules_folder_path) catch {}; | |
| FD.cwd().deleteTree(node_modules_folder_path) catch |err| { | |
| // Ignore if directory doesn't exist | |
| switch (err) { | |
| error.FileNotFound => {}, | |
| else => return err, | |
| } | |
| }; |
🤖 Prompt for AI Agents
In src/install/PackageManager/patchPackage.zig around line 736, the call to
FD.cwd().deleteTree(node_modules_folder_path) currently swallows all errors;
instead catch the error and explicitly ignore only the "not found" case (or
other benign EOF/not-exist errors) while logging and/or returning any other
errors so real filesystem problems (permissions, I/O) are surfaced; update the
catch to pattern-match on the error, quietly continue for expected missing-path
errors, and otherwise propagate or log the unexpected error with context.
| expect(await file(join(package_dir, "moo", "node_modules", "foo", "package.json")).json()).toEqual(fooPackage); | ||
| } else { | ||
| expect(await file(join(package_dir, "moo", "node_modules", "foo", "package.json")).json()).toEqual(fooPackage); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Remove redundant platform conditional
Both branches assert the same thing; simplify to a single expect.
- if (process.platform === "win32") {
- expect(await file(join(packageDir, "moo", "node_modules", "foo", "package.json")).json()).toEqual(fooPackage);
- } else {
- expect(await file(join(packageDir, "moo", "node_modules", "foo", "package.json")).json()).toEqual(fooPackage);
- }
+ expect(await file(join(packageDir, "moo", "node_modules", "foo", "package.json")).json()).toEqual(fooPackage);Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In test/cli/install/bun-add.test.ts around lines 2138 to 2141, the platform
conditional is redundant because both branches perform the same assertion;
remove the if/else block and replace it with a single expect that awaits and
asserts file(join(package_dir, "moo", "node_modules", "foo",
"package.json")).json() equals fooPackage.
What does this PR do?
Makes isolated installs the default install strategy for projects with workspaces in Bun v1.3.
When the isolated linker is used and
./node_modules/.bundoesn't exist,bun installwill automatically move./node_modulesand each workspace node_modules into./node_modules/.old_modules-{hex}. This prevents previously hoisted packages from polluting your node_modules.Also fixes creating patches with
bun patchand--linker isolatedFixes #22693
How did you verify your code works?
Added tests for node_modules renaming
bun patchwith isolated install.