feat: Implement low-level os.zig abstractions#23912
Conversation
WalkthroughAdds a cross-platform OS module exposing home-dir and sys-temp queries, wires libuv/C interop for homedir, updates call sites to use the new APIs, expands Windows syscall/build surface, and adds memory and pointer utility changes. Changes
Possibly related PRs
Suggested reviewers
Pre-merge checks❌ Failed checks (1 inconclusive)
✅ Passed checks (1 passed)
Comment |
markovejnovic
force-pushed
the
user/markovejnovic/system-abstractions-2.0
branch
from
4837db0 to
5ab5dd6
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/cli/upgrade_command.zig (1)
560-588: Fix PowerShell path join and clarify error messageThe code currently has two bugs:
- Duplicate path segment: Line 582 passes
system_roottwice—once as the base parameter and again in the segments array, resulting in a malformed path (e.g.,C:\Windows\C:\Windows\System32\...).- Misleading error message: Line 586 incorrectly blames "PowerShell not being installed" when the actual failure is Windows directory resolution.
Apply the suggested diff to fix both issues:
- const system_root = switch (bun.os.win32.queryWinDir()) { + const system_root = switch (bun.os.win32.queryWinDir()) { .err => { - Output.prettyErrorln("<r><red>error:<r> Failed to unzip {s} due to PowerShell not being installed.", .{tmpname}); + Output.prettyErrorln("<r><red>error:<r> Failed to locate Windows directory; cannot resolve PowerShell path to unzip {s}.", .{tmpname}); Global.exit(1); }, .result => |v| v, }; - const hardcoded_system_powershell = bun.path.joinAbsStringBuf(system_root, &buf, &.{ system_root, "System32\\WindowsPowerShell\\v1.0\\powershell.exe" }, .windows); + const hardcoded_system_powershell = bun.path.joinAbsStringBuf( + system_root, + &buf, + &.{ "System32\\WindowsPowerShell\\v1.0\\powershell.exe" }, + .windows, + );
📜 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.
📥 Commits
Reviewing files that changed from the base of the PR and between 7662de9 and a215342249bbbcb7e03b0ce0a7a89043fcd29c70.
📒 Files selected for processing (22)
cmake/targets/BuildBun.cmake(2 hunks)src/bun.js/bindings/libuv/generate_uv_posix_stubs_constants.ts(1 hunks)src/bun.js/bindings/uv-posix-stubs.c(1 hunks)src/bun.js/node/node_os.zig(1 hunks)src/bun.zig(1 hunks)src/c-headers-for-zig.h(1 hunks)src/cli/install_completions_command.zig(7 hunks)src/cli/upgrade_command.zig(2 hunks)src/fs.zig(1 hunks)src/install/PackageManager.zig(1 hunks)src/install/PackageManager/PackageManagerOptions.zig(1 hunks)src/install/repository.zig(1 hunks)src/linker.lds(1 hunks)src/os.zig(1 hunks)src/os/posix.zig(1 hunks)src/os/uvinterop.zig(1 hunks)src/os/win32.zig(1 hunks)src/sys.zig(3 hunks)src/threading.zig(1 hunks)src/threading/AtomicMutex.zig(1 hunks)src/threading/guarded.zig(1 hunks)src/windows.zig(1 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
**/*.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.zigsrc/install/PackageManager/PackageManagerOptions.zigsrc/threading/AtomicMutex.zigsrc/cli/upgrade_command.zigsrc/windows.zigsrc/bun.zigsrc/bun.js/node/node_os.zigsrc/os.zigsrc/threading/guarded.zigsrc/cli/install_completions_command.zigsrc/install/repository.zigsrc/threading.zigsrc/fs.zigsrc/os/posix.zigsrc/os/uvinterop.zigsrc/os/win32.zigsrc/sys.zig
src/**/*.zig
📄 CodeRabbit inference engine (.cursor/rules/building-bun.mdc)
When adding debug logs in Zig, create a scoped logger and log via Bun APIs:
const log = bun.Output.scoped(.${SCOPE}, .hidden);thenlog("...", .{})
src/**/*.zig: Use private fields in Zig with the#prefix (e.g.,struct { #foo: u32 };)
Prefer decl literals in Zig (e.g.,const decl: Decl = .{ .binding = 0, .value = 0 };)
Prefer placing@importstatements at the bottom of the Zig file (formatter may reorder automatically)
Prefer@import("bun")rather than@import("root").bunor@import("../bun.zig")In Zig code, be careful with allocators and use defer for cleanup
Files:
src/install/PackageManager.zigsrc/install/PackageManager/PackageManagerOptions.zigsrc/threading/AtomicMutex.zigsrc/cli/upgrade_command.zigsrc/windows.zigsrc/bun.zigsrc/bun.js/node/node_os.zigsrc/os.zigsrc/threading/guarded.zigsrc/cli/install_completions_command.zigsrc/install/repository.zigsrc/threading.zigsrc/fs.zigsrc/os/posix.zigsrc/os/uvinterop.zigsrc/os/win32.zigsrc/sys.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/node/node_os.zig
**/*.{cpp,h}
📄 CodeRabbit inference engine (.cursor/rules/javascriptcore-class.mdc)
**/*.{cpp,h}: When exposing a JS class with public Constructor and Prototype, define three C++ types: class Foo : public JSC::DestructibleObject (if it has C++ fields), class FooPrototype : public JSC::JSNonFinalObject, and class FooConstructor : public JSC::InternalFunction
If the class has C++ data members, inherit from JSC::DestructibleObject and provide proper destruction; if it has no C++ fields (only JS properties), avoid a class and use JSC::constructEmptyObject(vm, structure) with putDirectOffset
Prefer placing the subspaceFor implementation in the .cpp file rather than the header when possible
Files:
src/c-headers-for-zig.h
🧠 Learnings (4)
📚 Learning: 2025-10-14T23:34:06.388Z
Learnt from: CR
PR: oven-sh/bun#0
File: src/CLAUDE.md:0-0
Timestamp: 2025-10-14T23:34:06.388Z
Learning: Applies to src/**/*.zig : Prefer `import("bun")` rather than `import("root").bun` or `import("../bun.zig")`
Applied to files:
src/bun.zig
📚 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 : In Zig binding structs, expose generated bindings via pub const js = JSC.Codegen.JS<ClassName> and re-export toJS/fromJS/fromJSDirect
Applied to files:
src/bun.zig
📚 Learning: 2025-08-30T09:09:18.384Z
Learnt from: Jarred-Sumner
PR: oven-sh/bun#22231
File: src/bundler/bundle_v2.zig:48-48
Timestamp: 2025-08-30T09:09:18.384Z
Learning: In Zig, when a module exports a top-level struct, import("./Module.zig") directly returns that struct type and can be used as a type alias without needing to access a field within the module. This is a common pattern in the Bun codebase.
Applied to files:
src/bun.zig
📚 Learning: 2025-09-12T18:16:50.754Z
Learnt from: RiskyMH
PR: oven-sh/bun#22606
File: src/glob/GlobWalker.zig:449-452
Timestamp: 2025-09-12T18:16:50.754Z
Learning: For Bun codebase: prefer using `std.fs.path.sep` over manual platform separator detection, and use `bun.strings.lastIndexOfChar` instead of `std.mem.lastIndexOfScalar` for string operations.
Applied to files:
src/bun.js/node/node_os.zig
🔇 Additional comments (13)
src/linker.lds (1)
324-326: Linker script whitespace change is benign.The modification is purely formatting—a blank line adjustment around the extern "C++" boundary with no functional impact on symbol visibility or linking behavior.
However, note that the PR reports build failures on multiple platforms (
aarch64,x64,x64-asan) forscripts/build.mjs - build-zig. This linker script change is unlikely to be the cause, but the underlying build issues should be investigated before merge.src/c-headers-for-zig.h (1)
26-28: LGTM: Standard POSIX headers added for OS abstraction support.The addition of
errno.handlimits.hheaders is appropriate for the new OS abstraction layer, providing necessary error handling and system limit definitions for the POSIX interop code.src/bun.js/bindings/uv-posix-stubs.c (1)
1132-1136: LGTM: Correct delegation pattern to Bun's OS abstraction.The implementation correctly bridges the libuv
uv_os_homedirAPI to Bun's internalbunuv__os_homedirfunction, replacing the previous "not implemented" stub. The extern declaration and delegation pattern are appropriate.src/bun.js/bindings/libuv/generate_uv_posix_stubs_constants.ts (1)
3-3: LGTM: Correctly skips now-implemented function.Since
uv_os_homediris now implemented (delegating tobunuv__os_homedir), it should be excluded from stub generation testing. This change is consistent with the implementation inuv-posix-stubs.c.src/install/PackageManager/PackageManagerOptions.zig (1)
227-235: LGTM: Consistent home directory resolution with proper error handling.The change replaces
bun.DotEnv.home_envwithbun.os.queryHomeDir().asValue(), consistent with the centralized OS abstraction pattern. The error path at line 237 properly handles the case when bothXDG_CACHE_HOMEandqueryHomeDir()return null, returning a clear error message.cmake/targets/BuildBun.cmake (1)
1263-1263: The advapi32 library linkage is correct and required.The code explicitly declares
OpenProcessTokenfrom advapi32 insrc/windows.zig, and this function is used insrc/os/win32.zigto obtain a process token before callingGetUserProfileDirectoryWto resolve the user's home directory. This is a standard Windows pattern for accessing user profile information and confirms the new OS abstraction code legitimately requires the advapi32 library.src/install/PackageManager.zig (1)
794-796: No issues found - null handling is correct.The change properly handles all scenarios:
- When
queryHomeDir()returns an error,asValue()extractsnull- The null-coalescing expression
(bun.getenvZ("XDG_CONFIG_HOME") orelse home_dir)correctly attemptsXDG_CONFIG_HOMEfirst, then falls back tohome_dir- When both sources are
null, the condition isfalseand the else branch (lines 813-820) executes, which properly handles this case by creating a fallback installation structure without requiring adata_dirThe implementation safely handles all three cases: successful home directory retrieval, missing environment variables, and the fallback path.
src/sys.zig (2)
278-299: <!-- [scratchpad_start]
[task final_verification_complete_1/1]
[observations]
- isWindows() function (line 301): uses numeric comparison
@intFromEnum(this) > @intFromEnum(Tag.WriteFile)as boundary- New tags all appended after WriteFile, so numeric enum values are higher
- strings map (line 304):
std.EnumMap(Tag, jsc.C.JSStringRef).initFull(null)— uses Zig's EnumMap which automatically adjusts to enum changes- No sys.strings or Tag.strings access found in codebase
- No exhaustive switches over sys.Tag found (rg returned no results)
- No dynamic iteration over Tag variants that assumes count or layout
- No compile-time checks that would fail
[analysis]- Review concern about "string mapping usage" is addressed: EnumMap.initFull(null) is defensive and automatically includes new variants
- isWindows() boundary logic correctly handles new tags
- Absence of exhaustive switches means no downstream code will break
- Tag additions are structurally sound
[conclusion]
All verification checks pass. The code changes are safe. No issues found that require action.
[scratchpad_end]-->Enum additions are safe; no downstream issues detected
After verification of the src/sys.zig Tag enum modifications:
isWindows() logic preserved: The
isWindows()function at line 301 uses numeric comparison against theWriteFileboundary. New tags appended after WriteFile will have higher enum values and correctly evaluate totrue.No exhaustive switches to update: Search found zero exhaustive switch statements over
sys.Tagvalues, so adding new variants causes no compile failures elsewhere.String mapping is defensive: The
stringsEnumMap at line 304 initializes withinitFull(null), which automatically adjusts to accommodate all enum variants. No runtime access to unmapped tags discovered in the codebase.No invariant violations: No code iterates over Tag variants, assumes a fixed count, or relies on layout assumptions that would be broken by the additions.
4268-4301: Incorrect review premise — std.c._errno() is portable to POSIXThe review claims std.c._errno() is "Windows/MSVCRT-specific" and "won't compile on Linux/macOS," but this is false. The codebase already uses std.c._errno() successfully in src/errno/darwin_errno.zig and src/errno/linux_errno.zig on POSIX platforms. std.c._errno in Zig is a thin binding to the C library's errno accessor for the target platform; its exact implementation is platform-specific (e.g. glibc, musl, Darwin), and platform modules in std.c provide the correct errno accessor for that platform.
While std.posix.errno or the std.os/std.posix wrappers are recommended for portable code, the current code is not a build breaker and the proposed diff's rationale is incorrect.
Likely an incorrect or invalid review comment.
src/windows.zig (1)
89-113: Externs look correct; ensure userenv is linked and lifetime rules are clear.
- Please verify the build links userenv.lib in addition to advapi32 for GetUserProfileDirectoryW.
- Confirm TokenHandle is closed on all paths where OpenProcessToken succeeds (callers do defer CloseHandle).
src/threading.zig (1)
1-1: Public threading exports LGTM.Re-exports match downstream usage in os modules.
Please run a quick search to ensure all prior imports of guarded.AtomicallyGuarded switch to the new alias where appropriate.
Also applies to: 6-6
src/os/uvinterop.zig (1)
18-43: uv_os_homedir bridge looks good; buffer contract is correct.
- Returns 0 on success, negative UV_* on error.
- Sets size to required bytes (including NUL) on ENOBUFS. Good.
If libuv ever returns EINVAL for null args, this is aligned. Consider documenting that size is in bytes (UTF‑8) to avoid confusion.
src/os/win32.zig (1)
451-468: TMP/TEMP fallback looks good. Small consistency nit.You correctly register atexit for the managed concat result here. Consider doing the same pattern in the other functions (above fixes).
markovejnovic
force-pushed
the
user/markovejnovic/system-abstractions-2.0
branch
from
a215342 to
eff27c4
Compare
There was a problem hiding this comment.
Actionable comments posted: 7
♻️ Duplicate comments (4)
src/os/posix.zig (3)
35-38: Verify thatpw_diris null-terminated.
std.mem.lenrequires a null-terminated pointer[*:0]u8. Ifbun.c.struct_passwd.pw_diris not explicitly typed as sentinel-terminated, this will fail at compile time.If
pw_diris typed as[*c]u8rather than[*:0]u8, use a Z-aware helper:- self._memo.pw_dir_len = std.mem.len(self.entry_ptr.pw_dir); + self._memo.pw_dir_len = bun.strings.lengthZ(self.entry_ptr.pw_dir);Run this script to verify the field type:
#!/bin/bash # Check the type definition of struct_passwd.pw_dir in bun C bindings ast-grep --pattern $'struct_passwd { $$$ pw_dir: $_ $$$ }'
27-29: Compile blocker: Cannot call.init()on integer type.
std.c.uid_tis a plain integer type (typicallyu32), not a struct. Calling.init()on it will cause a compilation error.Apply this diff to fix:
pub fn pwUid(self: *const Self) std.c.uid_t { - return .init(self.pwUidC()); + return self.pwUidC(); }
65-74: Integer overflow risk:sysconfcan return -1.When
sysconfreturns -1 (indeterminate value), the direct@intCasttousizewill wrap the negative value into a very large unsigned integer, potentially causing enormous allocations.Apply this diff to guard against negative results:
- var buffer_size: usize = Memo.pwbuf_size.load(.monotonic); - if (buffer_size == 0) { - // We know that we haven't initialized it yet. Let's try to do so now. - const initial_buf_size: usize = @intCast( - bun.sys.sysconf(bun.c._SC_GETPW_R_SIZE_MAX) catch default_buf_size, - ); - - buffer_size = initial_buf_size; - } + var buffer_size: usize = Memo.pwbuf_size.load(.monotonic); + if (buffer_size == 0) { + const sc = bun.sys.sysconf(bun.c._SC_GETPW_R_SIZE_MAX) catch default_buf_size; + // sysconf returns -1 for indeterminate; guard against wrapping + const initial_buf_size: usize = if (sc <= 0) default_buf_size else @intCast(sc); + buffer_size = initial_buf_size; + }src/os/win32.zig (1)
339-352: Same leak issue: MissingINTENTIONALLY_LEAKafter successfultryStore.See the comment on lines 209-225. The environment variable path stored at line 345 needs an intentional leak annotation when
tryStoresucceeds.const hpath = bun.handleOom(allocator.create([]const u8)); hpath.* = t; if (Store.path.tryStore(.init(hpath))) |v| { allocator.destroy(hpath); return .initResult(AtomicMaybeManagedPath.forceAsSlice(v)); } + bun.memory.INTENTIONALLY_LEAK( + allocator, + @ptrCast(hpath), + "temp directory path considered singleton", + ); return .initResult(t);
📜 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.
📥 Commits
Reviewing files that changed from the base of the PR and between a215342249bbbcb7e03b0ce0a7a89043fcd29c70 and eff27c4a3fcd58b7d6e6905d13b18f80d8e4e601.
📒 Files selected for processing (4)
src/memory.zig(1 hunks)src/os/posix.zig(1 hunks)src/os/win32.zig(1 hunks)src/ptr/tagged_pointer.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/os/posix.zigsrc/ptr/tagged_pointer.zigsrc/memory.zigsrc/os/win32.zig
src/**/*.zig
📄 CodeRabbit inference engine (.cursor/rules/building-bun.mdc)
When adding debug logs in Zig, create a scoped logger and log via Bun APIs:
const log = bun.Output.scoped(.${SCOPE}, .hidden);thenlog("...", .{})
src/**/*.zig: Use private fields in Zig with the#prefix (e.g.,struct { #foo: u32 };)
Prefer decl literals in Zig (e.g.,const decl: Decl = .{ .binding = 0, .value = 0 };)
Prefer placing@importstatements at the bottom of the Zig file (formatter may reorder automatically)
Prefer@import("bun")rather than@import("root").bunor@import("../bun.zig")In Zig code, be careful with allocators and use defer for cleanup
Files:
src/os/posix.zigsrc/ptr/tagged_pointer.zigsrc/memory.zigsrc/os/win32.zig
🧠 Learnings (1)
📚 Learning: 2025-09-26T01:09:19.196Z
Learnt from: nektro
PR: oven-sh/bun#22781
File: src/sql/shared/SQLDataCell.zig:144-168
Timestamp: 2025-09-26T01:09:19.196Z
Learning: In Zig, packed structs with a specified backing integer type (e.g., `packed struct(u32)`) are ABI-identical to their underlying integer type and can be safely passed to C functions expecting the corresponding integer type without any casting or bitcasting.
Applied to files:
src/ptr/tagged_pointer.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 (5)
src/ptr/tagged_pointer.zig (1)
139-139: Fix verified: enum field naming consistency is correct.The change properly aligns the
case()method lookup with how enum fields are actually created. Enum fields are created at line 66 using the full@typeName(field), andcase()at line 139 now looks them up using the same@typeName(Type). This matches the pattern used throughout the rest of the struct (lines 130, 143, 155, 160). The previous code withtypeBaseName()would have failed to locate fields due to a name mismatch. All existing call sites use type names that will work correctly with fully qualified naming.src/os/posix.zig (2)
147-245: LGTM: Thread-safe home directory resolution.The atomic compare-exchange pattern correctly handles race conditions, and the intentional leak is appropriate for singleton lifetime management. The fallback chain (HOME env → passwd entry) follows POSIX conventions.
249-260: LGTM: Follows POSIX temporary directory conventions.The implementation correctly checks
TMPDIRand defaults to/tmpas recommended by IEEE Std 1003.1-2017.src/os/win32.zig (2)
5-30: LGTM: Clean atomic caching abstraction.The
TaggedPointerUnionapproach elegantly handles both managed ([]u8) and unmanaged ([]const u8) paths with thread-safe compare-and-swap semantics.
298-393: LGTM: Windows temp directory fallback chain is correct.The implementation matches the documented behavior of
GetTempPath2(TMP → TEMP → USERPROFILE → Windows\Temp) and includes a sensible deviation by appending\Tempto avoid polluting user/system directories. The comments clearly explain the reasoning.
markovejnovic
force-pushed
the
user/markovejnovic/system-abstractions-2.0
branch
2 times, most recently
from
8dfe8ac to
233b915
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
src/ptr/tagged_pointer.zig (1)
91-91: Specify backing integer type forpacked structto maintain ABI clarity.This issue was already flagged in a previous review. The packed struct at line 91 still lacks an explicit backing integer type specification (
packed struct(u64)), which is inconsistent with the patterns established throughout this file and necessary for ABI compatibility guarantees.Apply this diff:
- return packed struct { + return packed struct(u64) {
📜 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.
📥 Commits
Reviewing files that changed from the base of the PR and between eff27c4a3fcd58b7d6e6905d13b18f80d8e4e601 and ac42d9eef0240a1f83cd9b5647b3542d23fb9f8b.
📒 Files selected for processing (7)
src/install/PackageManager.zig(1 hunks)src/os.zig(1 hunks)src/os/posix.zig(1 hunks)src/os/win32.zig(1 hunks)src/ptr/tagged_pointer.zig(2 hunks)src/sys.zig(3 hunks)src/threading.zig(1 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/ptr/tagged_pointer.zigsrc/os/posix.zigsrc/install/PackageManager.zigsrc/os/win32.zigsrc/os.zigsrc/threading.zigsrc/sys.zig
src/**/*.zig
📄 CodeRabbit inference engine (.cursor/rules/building-bun.mdc)
When adding debug logs in Zig, create a scoped logger and log via Bun APIs:
const log = bun.Output.scoped(.${SCOPE}, .hidden);thenlog("...", .{})
src/**/*.zig: Use private fields in Zig with the#prefix (e.g.,struct { #foo: u32 };)
Prefer decl literals in Zig (e.g.,const decl: Decl = .{ .binding = 0, .value = 0 };)
Prefer placing@importstatements at the bottom of the Zig file (formatter may reorder automatically)
Prefer@import("bun")rather than@import("root").bunor@import("../bun.zig")In Zig code, be careful with allocators and use defer for cleanup
Files:
src/ptr/tagged_pointer.zigsrc/os/posix.zigsrc/install/PackageManager.zigsrc/os/win32.zigsrc/os.zigsrc/threading.zigsrc/sys.zig
🧠 Learnings (3)
📚 Learning: 2025-09-26T01:09:19.196Z
Learnt from: nektro
PR: oven-sh/bun#22781
File: src/sql/shared/SQLDataCell.zig:144-168
Timestamp: 2025-09-26T01:09:19.196Z
Learning: In Zig, packed structs with a specified backing integer type (e.g., `packed struct(u32)`) are ABI-identical to their underlying integer type and can be safely passed to C functions expecting the corresponding integer type without any casting or bitcasting.
Applied to files:
src/ptr/tagged_pointer.zig
📚 Learning: 2025-10-14T23:34:06.388Z
Learnt from: CR
PR: oven-sh/bun#0
File: src/CLAUDE.md:0-0
Timestamp: 2025-10-14T23:34:06.388Z
Learning: Applies to src/**/*.zig : Prefer `import("bun")` rather than `import("root").bun` or `import("../bun.zig")`
Applied to files:
src/ptr/tagged_pointer.zig
📚 Learning: 2025-10-16T17:32:03.074Z
Learnt from: markovejnovic
PR: oven-sh/bun#23710
File: src/install/PackageManager/PackageManagerOptions.zig:187-193
Timestamp: 2025-10-16T17:32:03.074Z
Learning: In Bun's codebase (particularly in files like src/install/PackageManager/PackageManagerOptions.zig), mixing bun.EnvVar.*.get() and bun.EnvVar.*.platformGet() for environment variable lookups is intentional and safe. The code is protected by compile-time platform checks (Environment.isWindows, etc.), and compilation will fail if the wrong function is used on the wrong platform. This pattern should not be flagged as a consistency issue.
Applied to files:
src/os.zig
🔇 Additional comments (4)
src/os.zig (2)
1-41: Well-structured cross-platform OS abstraction layer.The module provides a clean, platform-agnostic API surface for OS-level queries. The conditional imports (lines 29-30) correctly prevent cross-compilation issues, and the comptime linking block (lines 32-39) appropriately gates POSIX-specific dependencies. Import placement at the bottom follows the coding guidelines.
The TODO comment at line 33 about moving
uvinteroplinking is acknowledged and can be addressed in a follow-up.
21-27: ****The
posix.getSysTmpDir()function returns[]const u8(not aMaybetype) and is infallible—it always returns either the TMPDIR environment variable or defaults to "/tmp". The use of.initResult()at line 26 is the correct pattern for wrapping this infallible result into theMaybereturn type. No changes needed.src/ptr/tagged_pointer.zig (1)
138-140: Type name change is correct and consistent with the codebase.The verification shows extensive usage of
@field(Tag, @typeName(...))throughout the codebase—across Sink.zig, Task.zig (30+ instances), process.zig, SavedSourceMap.zig, and posix_event_loop.zig. This confirms that the Tag enum fields are constructed using full type names, making the change frombun.meta.typeBaseName(@typeName(Type))to@typeName(Type)the correct alignment. The case() function on line 139 now matches how all existing code accesses Tag enum fields.src/install/PackageManager.zig (1)
794-795: Error handling semantics verified—code is correct.The
Maybe.asValue()implementation at src/bun.js/node.zig:150-152 confirms it returns?ReturnType(optional), not panicking. On error, the union tag is.errandasValue()returnsnull, allowing theorelsechain to gracefully fall through. All 9 usages across the codebase (PackageManager.zig, PackageManagerOptions.zig, install_completions_command.zig) follow this pattern consistently.
markovejnovic
force-pushed
the
user/markovejnovic/system-abstractions-2.0
branch
2 times, most recently
from
14304f3 to
64bb78a
Compare
markovejnovic
requested review from
a team,
Jarred-Sumner,
RiskyMH,
alii,
cirospaciari,
dylan-conway,
nektro,
pfgithub,
sosukesuzuki,
taylordotfish and
zackradisic
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 (2)
src/cli/install_completions_command.zig (1)
28-33: Hoist home-dir lookup to avoid repeated syscallsFetch bun.os.queryHomeDir().asValue() once per exec() and reuse; reduces duplication and avoids inconsistent fallbacks if env changes mid-run.
Also applies to: 37-42, 232-239, 290-308, 342-361, 452-468
src/cli/upgrade_command.zig (1)
571-589: Windows PowerShell path join repeats system_root; results in invalid pathThe bug is confirmed.
joinAbsStringBuf(system_root, &buf, &.{ system_root, ... })duplicates system_root: once as the basecwdparameter and again in thepartsarray. This produces an incorrect path likesystem_root/system_root/System32/....Apply this diff:
- const hardcoded_system_powershell = bun.path.joinAbsStringBuf(system_root, &buf, &.{ system_root, "System32\\WindowsPowerShell\\v1.0\\powershell.exe" }, .windows); + const hardcoded_system_powershell = bun.path.joinAbsStringBuf( + system_root, + &buf, + &.{ "System32\\WindowsPowerShell\\v1.0\\powershell.exe" }, + .windows, + );
♻️ Duplicate comments (2)
src/sys.zig (1)
4268-4301: sysconf: still truncates c_long; fix return type and align docs
- Good: now accepts 0 as success (ret >= 0).
- Not good: returning u32 can truncate valid c_long values (e.g., large limits). Return usize instead to preserve width. Docstring also mentions a “default” parameter that doesn’t exist.
Apply:
-/// Matches the POSIX sysconf function, except returns rich error information. +/// Matches the POSIX sysconf(3) function, returning rich error information. @@ -/// Providing a default will cause that value to be returned instead of an error in the case of -/// error. You can use this to provide fallback values for systems that don't support certain -/// sysconf names. -/// -/// On platforms which do not support sysconf, this will return the default if provided, or a -/// compile-time error if not. +/// Note: callers must handle error.IndeterminateLimit when the limit is validly indeterminate +/// (errno remains 0 and -1 is returned by sysconf). @@ -pub fn sysconf(name: c_int) !u32 { +pub fn sysconf(name: c_int) !usize { @@ - if (ret >= 0) { - return @intCast(ret); - } + if (ret >= 0) { + return @as(usize, @intCast(ret)); + }src/memory.zig (1)
205-223: Avoid hard dependency on __lsan_ignore_object to prevent link errors when LSAN isn’t linkedAs previously noted, a strong extern here can break debug/non-LSAN builds (Windows/macOS x64). Guard via an optional extern and call only if present.
Apply this diff:
-extern "C" fn __lsan_ignore_object(object: *const anyopaque) callconv(.C) void; +// Optional (weak) LSAN hook: present only when LeakSanitizer is linked +const __lsan_ignore_object = + @extern(?*const fn (*const anyopaque) callconv(.C) void, .{ .name = "__lsan_ignore_object" }); /// Mark the given object as intentionally leaked, so that leak sanitizers do not report it. pub fn INTENTIONALLY_LEAK( comptime alloc: anytype, object: *anyopaque, comptime why: []const u8, ) void { _ = why; if (comptime bun.Environment.isDebug) { if (comptime bun.allocators.isDefault(alloc)) { - __lsan_ignore_object(object); + if (__lsan_ignore_object) |f| f(object); } else { @compileError("INTENTIONALLY_LEAK is not yet implemented for non-default allocators"); } } }Run to ensure no remaining strong references to LSAN symbols:
#!/bin/bash rg -nP --type=zig '\b__lsan_(ignore_object|is_turned_off)\b' -C2
📜 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.
📥 Commits
Reviewing files that changed from the base of the PR and between ac42d9eef0240a1f83cd9b5647b3542d23fb9f8b and 3b4f51ad8b5bd5ab14d5e58da517d6c2ad870254.
📒 Files selected for processing (15)
src/bun.js/node/node_os.zig(1 hunks)src/cli/install_completions_command.zig(7 hunks)src/cli/upgrade_command.zig(2 hunks)src/fs.zig(1 hunks)src/install/PackageManager.zig(1 hunks)src/install/PackageManager/PackageManagerOptions.zig(1 hunks)src/install/repository.zig(1 hunks)src/memory.zig(1 hunks)src/os.zig(1 hunks)src/os/posix.zig(1 hunks)src/os/uvinterop.zig(1 hunks)src/os/win32.zig(1 hunks)src/ptr/tagged_pointer.zig(2 hunks)src/sys.zig(3 hunks)src/windows.zig(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.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/PackageManagerOptions.zigsrc/bun.js/node/node_os.zigsrc/install/repository.zigsrc/install/PackageManager.zigsrc/cli/upgrade_command.zigsrc/memory.zigsrc/os/posix.zigsrc/os/win32.zigsrc/fs.zigsrc/os.zigsrc/os/uvinterop.zigsrc/windows.zigsrc/cli/install_completions_command.zigsrc/sys.zigsrc/ptr/tagged_pointer.zig
src/**/*.zig
📄 CodeRabbit inference engine (.cursor/rules/building-bun.mdc)
When adding debug logs in Zig, create a scoped logger and log via Bun APIs:
const log = bun.Output.scoped(.${SCOPE}, .hidden);thenlog("...", .{})
src/**/*.zig: Use private fields in Zig with the#prefix (e.g.,struct { #foo: u32 };)
Prefer decl literals in Zig (e.g.,const decl: Decl = .{ .binding = 0, .value = 0 };)
Prefer placing@importstatements at the bottom of the Zig file (formatter may reorder automatically)
Prefer@import("bun")rather than@import("root").bunor@import("../bun.zig")In Zig code, be careful with allocators and use defer for cleanup
Files:
src/install/PackageManager/PackageManagerOptions.zigsrc/bun.js/node/node_os.zigsrc/install/repository.zigsrc/install/PackageManager.zigsrc/cli/upgrade_command.zigsrc/memory.zigsrc/os/posix.zigsrc/os/win32.zigsrc/fs.zigsrc/os.zigsrc/os/uvinterop.zigsrc/windows.zigsrc/cli/install_completions_command.zigsrc/sys.zigsrc/ptr/tagged_pointer.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/node/node_os.zig
🧠 Learnings (3)
📚 Learning: 2025-10-16T17:32:03.074Z
Learnt from: markovejnovic
PR: oven-sh/bun#23710
File: src/install/PackageManager/PackageManagerOptions.zig:187-193
Timestamp: 2025-10-16T17:32:03.074Z
Learning: In Bun's codebase (particularly in files like src/install/PackageManager/PackageManagerOptions.zig), mixing bun.EnvVar.*.get() and bun.EnvVar.*.platformGet() for environment variable lookups is intentional and safe. The code is protected by compile-time platform checks (Environment.isWindows, etc.), and compilation will fail if the wrong function is used on the wrong platform. This pattern should not be flagged as a consistency issue.
Applied to files:
src/os.zig
📚 Learning: 2025-09-26T01:09:19.196Z
Learnt from: nektro
PR: oven-sh/bun#22781
File: src/sql/shared/SQLDataCell.zig:144-168
Timestamp: 2025-09-26T01:09:19.196Z
Learning: In Zig, packed structs with a specified backing integer type (e.g., `packed struct(u32)`) are ABI-identical to their underlying integer type and can be safely passed to C functions expecting the corresponding integer type without any casting or bitcasting.
Applied to files:
src/ptr/tagged_pointer.zig
📚 Learning: 2025-10-14T23:34:06.388Z
Learnt from: CR
PR: oven-sh/bun#0
File: src/CLAUDE.md:0-0
Timestamp: 2025-10-14T23:34:06.388Z
Learning: Applies to src/**/*.zig : Prefer `import("bun")` rather than `import("root").bun` or `import("../bun.zig")`
Applied to files:
src/ptr/tagged_pointer.zig
🔇 Additional comments (7)
src/ptr/tagged_pointer.zig (1)
138-140: Bug fix verified—no issues found.The change correctly fixes an inconsistency in tag field lookups. The script results confirm that all existing
.case()usages across the codebase (insrc/os/win32.zig,src/os/posix.zig,src/bun.js/api/server.zig, etc.) pass straightforward types that will work correctly with the new full-name lookup approach.Since enum fields are created using
@typeName(field)directly at line 65, and the newcase()implementation also uses@typeName(Type)directly, the names are now guaranteed to match consistently—fixing the previous bug where namespaced types would fail to resolve.src/sys.zig (1)
278-279: Review comment is incorrect; GetTempPath2 is an error tag label, not a function call requiring fallback.The original concern conflates GetTempPath2 (a Tag enum member for error reporting in sys.zig:298) with an actual function that needs down-level Windows availability checking. In reality:
- GetTempPath2 appears only in error reporting via .fromCode() calls (src/os/win32.zig:356, 377)—it's an enum label, not a function
- The actual Windows functions used are GetUserProfileDirectoryW and GetSystemWindowsDirectoryW, both properly declared as extern in src/windows.zig:100 and 107
- Tag enum overflow is not a concern (106 members ≪ 256 limit for enum(u8))
- No Tag.strings mapping is needed; Zig's @TagName handles enum serialization automatically
No action required. The code is correct as written.
Likely an incorrect or invalid review comment.
src/bun.js/node/node_os.zig (1)
305-312: Homedir resolution refactor looks goodUnified through bun.os.queryHomeDir() with proper error-to-JS propagation. No further issues spotted.
src/install/PackageManager/PackageManagerOptions.zig (1)
227-235: LGTM: consistent global bin resolutionUses XDG_CACHE_HOME first, otherwise home via bun.os.queryHomeDir(); non-Z join variant avoids sentinel pitfalls.
src/fs.zig (1)
534-541: Temp dir via os abstraction looks goodSwitch to bun.os.querySysTmpDir() centralizes platform logic; panic on error is acceptable for this low-level fallback path.
src/install/PackageManager.zig (1)
795-819: ****The review comment's analysis is based on an incorrect assumption about
joinAbsStringBufZ's signature. The actual signature ispub fn joinAbsStringBufZ(cwd: []const u8, buf: []u8, _parts: anytype, comptime platform: Platform) [:0]const u8, which accepts[]const u8for thecwdparameter—not[:0]const u8.In the reviewed code,
data_dircomes frombun.getenvZ("XDG_CONFIG_HOME") orelse bun.os.queryHomeDir().asValue(). ThequeryHomeDir()function returnsbun.sys.Maybe([]const u8), so both branches ([:0]const u8fromgetenvZand[]const u8fromasValue) are compatible with the[]const u8parameter. The code is type-correct as-is, and the suggested refactoring is unnecessary.Likely an incorrect or invalid review comment.
src/install/repository.zig (1)
19-22: ****The review comment misinterprets the
joinAbsStringBufZsignature. The function parametercwdis declared as[]const u8(not[:0]const u8), meaning it accepts non-Z-terminated slices. The return type is[:0]const u8(Z-terminated output), which the "Z" suffix refers to. The current code at lines 19–21 is correct:queryHomeDir().unwrap()returns[]const u8, which matches the expected parameter type. No allocation or conversion is necessary.Likely an incorrect or invalid review comment.
markovejnovic
force-pushed
the
user/markovejnovic/system-abstractions-2.0
branch
from
3b4f51a to
1757d41
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
src/cli/install_completions_command.zig (1)
28-33: Deduplicate home directory lookups; compute once per scopequeryHomeDir() is invoked multiple times in the same flow. Cache it once per function/branch to avoid repeated syscalls and keep behavior consistent within a run.
Apply (example in installBunxSymlinkPosix):
- // if that fails, try $HOME/.bun/bin - outer: { - if (bun.os.queryHomeDir().asValue()) |home_dir| { + // if that fails, try $HOME/.bun/bin + const home_dir_opt = bun.os.queryHomeDir().asValue(); + outer: { + if (home_dir_opt) |home_dir| { target = std.fmt.bufPrint(&target_buf, "{s}/.bun/bin/" ++ bunx_name, .{home_dir}) catch unreachable; std.posix.symlink(exe, target) catch break :outer; return; } }Repeat the same pattern for the other sites (HOME/.local/bin, fish/zsh/bash paths, .zshrc/.zshenv opens).
Also applies to: 37-42, 232-239, 290-308, 342-361, 452-468
src/cli/upgrade_command.zig (1)
575-588: Simplify PowerShell path join (avoid duplicated base argument)joinAbsStringBuf is given system_root as both base and first path element. Not harmful but redundant; pass empty base.
Apply:
- const hardcoded_system_powershell = bun.path.joinAbsStringBuf(system_root, &buf, &.{ system_root, "System32\\WindowsPowerShell\\v1.0\\powershell.exe" }, .windows); + const hardcoded_system_powershell = bun.path.joinAbsStringBuf("", &buf, &.{ system_root, "System32\\WindowsPowerShell\\v1.0\\powershell.exe" }, .windows);src/windows.zig (2)
3109-3113: Correct signature: GetTempPathW’s output buffer must be writable (LPWSTR).The param is declared as LPCWSTR but the API writes into the buffer.
Apply this diff:
pub extern "kernel32" fn GetTempPathW( nBufferLength: DWORD, // [in] - lpBuffer: LPCWSTR, // [out] + lpBuffer: LPWSTR, // [out] ) DWORD;
3292-3304: Avoid logging PII (username) in userUniqueId.This logs the Windows username unconditionally. Treat as sensitive; remove or hide behind debug and don’t print the actual name.
- const name = buf[0..size]; - bun.Output.scoped(.windowsUserUniqueId, .visible)("username: {}", .{bun.fmt.utf16(name)}); - return bun.hash32(std.mem.sliceAsBytes(name)); + const name = buf[0..size]; + if (comptime bun.Environment.enable_logs) { + const log = bun.Output.scoped(.windowsUserUniqueId, .hidden); + log("computed userUniqueId", .{}); + } + return bun.hash32(std.mem.sliceAsBytes(name));As per coding guidelines.
♻️ Duplicate comments (6)
src/ptr/tagged_pointer.zig (1)
91-91: Specify backing integer type for packed struct.This issue has already been flagged in a previous review. The packed struct should explicitly specify its backing integer type (e.g.,
packed struct(u64)) to maintain ABI clarity and consistency with other packed structs in this file, especially given the PR's focus on FFI/C interop work.src/memory.zig (1)
205-221: Guard__lsan_ignore_objectfor builds without LeakSanitizerLine 205
INTENTIONALLY_LEAKis compiled into every debug build, but many of those (Windows, macOS x64, most CI presets) still ship without LeakSanitizer linked in. Calling an unguardedextern "C" fn __lsan_ignore_objecthere will therefore fail the final link step on those targets. Please keep the symbol optional (weak extern + availability check) before invoking it. This is blocking because it prevents debug builds from linking successfully.Apply this diff to guard the symbol:
-extern "C" fn __lsan_ignore_object(object: *const anyopaque) callconv(.C) void; +const lsan_ignore_object = @extern( + ?*const fn (*const anyopaque) callconv(.C) void, + .{ .name = "__lsan_ignore_object", .linkage = .Weak }, +); @@ - __lsan_ignore_object(object); + if (lsan_ignore_object) |ignore| { + ignore(@ptrCast(*const anyopaque, object)); + }src/sys.zig (1)
4268-4302: sysconf: avoid truncation; return full-width type (usize) and keep zero as successCurrent signature returns u32; POSIX sysconf returns long and values can exceed 32 bits. Truncation corrupts results (e.g., _SC_PHYS_PAGES). Keep ret>=0, but return usize. This complements the earlier feedback; zero handling is fixed, width is not.
Apply:
-pub fn sysconf(name: c_int) !u32 { +pub fn sysconf(name: c_int) !usize { @@ - if (ret >= 0) { - return @intCast(ret); - } + if (ret >= 0) { + return @as(usize, @intCast(ret)); + }src/os.zig (1)
26-27: Minor: unify Maybe init style (use .success for consistency)Elsewhere you use .success/.err helpers; here .initResult is used. Consider aligning for consistency.
Apply:
- return .initResult(posix.getSysTmpDir()); + return .success(posix.getSysTmpDir());src/os/win32.zig (1)
328-341: Consistency: mark env-var tmp path as intentionally leaked when stored.Matches queryWinDir/queryUserProfile patterns and aids leak tracking.
if (Static.path.tryStore(.init(hpath))) |v| { allocator.destroy(hpath); return .initResult(AtomicMaybeManagedPath.forceAsSlice(v)); } - return .initResult(t); + bun.memory.INTENTIONALLY_LEAK( + allocator, + @ptrCast(hpath), + "tmp directory considered singleton", + ); + return .initResult(t);Based on learnings.
src/os/posix.zig (1)
76-105: Fix leak: errdefer won’t run for bun.sys.Maybe; guard with defer + flag.All .initErr returns leak self.buffer. Use a boolean to free on error and keep on success.
var self: PasswdEntry = .{ .allocator = alloc, .buffer = alloc.alloc(u8, buffer_size) catch { return .initErr(.fromCode(.NOMEM, .getpwuid_r)); }, .entry = undefined, .entry_ptr = undefined, }; - errdefer self.allocator.free(self.buffer); + var keep_buffer = false; + defer if (!keep_buffer) self.allocator.free(self.buffer); ... if (rc == 0) { // Since we found the correct buffer size, let's store it for future use. Memo.pwbuf_size.store(buffer_size, .monotonic); if (result) |r| { self.entry_ptr = r; - return .initResult(self); + keep_buffer = true; + return .initResult(self); } return .initErr(.fromCode(.NOENT, .getpwuid_r)); }
📜 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.
📥 Commits
Reviewing files that changed from the base of the PR and between 3b4f51ad8b5bd5ab14d5e58da517d6c2ad870254 and 1757d41542a3419af496016bac2a54d8dc632713.
📒 Files selected for processing (15)
src/bun.js/node/node_os.zig(1 hunks)src/cli/install_completions_command.zig(7 hunks)src/cli/upgrade_command.zig(2 hunks)src/fs.zig(1 hunks)src/install/PackageManager.zig(1 hunks)src/install/PackageManager/PackageManagerOptions.zig(1 hunks)src/install/repository.zig(1 hunks)src/memory.zig(1 hunks)src/os.zig(1 hunks)src/os/posix.zig(1 hunks)src/os/uvinterop.zig(1 hunks)src/os/win32.zig(1 hunks)src/ptr/tagged_pointer.zig(2 hunks)src/sys.zig(3 hunks)src/windows.zig(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.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/PackageManagerOptions.zigsrc/cli/install_completions_command.zigsrc/os.zigsrc/fs.zigsrc/install/repository.zigsrc/os/uvinterop.zigsrc/install/PackageManager.zigsrc/memory.zigsrc/os/win32.zigsrc/bun.js/node/node_os.zigsrc/ptr/tagged_pointer.zigsrc/windows.zigsrc/cli/upgrade_command.zigsrc/os/posix.zigsrc/sys.zig
src/**/*.zig
📄 CodeRabbit inference engine (.cursor/rules/building-bun.mdc)
When adding debug logs in Zig, create a scoped logger and log via Bun APIs:
const log = bun.Output.scoped(.${SCOPE}, .hidden);thenlog("...", .{})
src/**/*.zig: Use private fields in Zig with the#prefix (e.g.,struct { #foo: u32 };)
Prefer decl literals in Zig (e.g.,const decl: Decl = .{ .binding = 0, .value = 0 };)
Prefer placing@importstatements at the bottom of the Zig file (formatter may reorder automatically)
Prefer@import("bun")rather than@import("root").bunor@import("../bun.zig")In Zig code, be careful with allocators and use defer for cleanup
Files:
src/install/PackageManager/PackageManagerOptions.zigsrc/cli/install_completions_command.zigsrc/os.zigsrc/fs.zigsrc/install/repository.zigsrc/os/uvinterop.zigsrc/install/PackageManager.zigsrc/memory.zigsrc/os/win32.zigsrc/bun.js/node/node_os.zigsrc/ptr/tagged_pointer.zigsrc/windows.zigsrc/cli/upgrade_command.zigsrc/os/posix.zigsrc/sys.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/node/node_os.zig
🧠 Learnings (4)
📚 Learning: 2025-10-16T17:32:03.074Z
Learnt from: markovejnovic
PR: oven-sh/bun#23710
File: src/install/PackageManager/PackageManagerOptions.zig:187-193
Timestamp: 2025-10-16T17:32:03.074Z
Learning: In Bun's codebase (particularly in files like src/install/PackageManager/PackageManagerOptions.zig), mixing bun.EnvVar.*.get() and bun.EnvVar.*.platformGet() for environment variable lookups is intentional and safe. The code is protected by compile-time platform checks (Environment.isWindows, etc.), and compilation will fail if the wrong function is used on the wrong platform. This pattern should not be flagged as a consistency issue.
Applied to files:
src/os.zig
📚 Learning: 2025-10-20T04:22:55.575Z
Learnt from: CR
PR: oven-sh/bun#0
File: CLAUDE.md:0-0
Timestamp: 2025-10-20T04:22:55.575Z
Learning: Applies to src/**/*.zig : In Zig code, be careful with allocators and use defer for cleanup
Applied to files:
src/os/win32.zig
📚 Learning: 2025-09-26T01:09:19.196Z
Learnt from: nektro
PR: oven-sh/bun#22781
File: src/sql/shared/SQLDataCell.zig:144-168
Timestamp: 2025-09-26T01:09:19.196Z
Learning: In Zig, packed structs with a specified backing integer type (e.g., `packed struct(u32)`) are ABI-identical to their underlying integer type and can be safely passed to C functions expecting the corresponding integer type without any casting or bitcasting.
Applied to files:
src/ptr/tagged_pointer.zig
📚 Learning: 2025-10-14T23:34:06.388Z
Learnt from: CR
PR: oven-sh/bun#0
File: src/CLAUDE.md:0-0
Timestamp: 2025-10-14T23:34:06.388Z
Learning: Applies to src/**/*.zig : Prefer `import("bun")` rather than `import("root").bun` or `import("../bun.zig")`
Applied to files:
src/ptr/tagged_pointer.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 (5)
src/ptr/tagged_pointer.zig (1)
138-140: Now I need to inspect the actual code to understand the issue properly:Let me directly inspect the tagged_pointer.zig file to understand the enum creation:
Based on my analysis of the codebase and the .case() calls found throughout the repository, I now have enough information to generate the rewritten review comment. The script output showed actual usage patterns of the case() method with different types, and the web search confirmed that bun.meta.typeBaseName extracts base names from full type names. The critical issue is whether the enum field names match the @typename output.
Verify type name consistency across enum field creation and case() method lookups.
The change from
bun.meta.typeBaseName(@typeName(Type))to@typeName(Type)in thecase()method affects how types are resolved to Tag enum field names.If the Tag enum at lines 64-71 also uses full
@typeName()results for field names (e.g.,[]const u8,PasswdEntry,HTTPServer), the change is consistent and correct. However, if those enum fields were created usingtypeBaseNameto extract only base names (e.g., stripping module prefixes), this change creates a mismatch where lookups will fail becausecase()now returns qualified names while enum fields are named with base names.The script output confirms
.case()is called with types like[]const u8,PasswdEntry,HTTPServer,HTTPSServer, etc. acrosssrc/os/posix.zig,src/os/win32.zig, andsrc/bun.js/api/server.zig. Ensure the enum field creation logic at lines 64-71 uses the same naming convention ascase()now expects.src/sys.zig (2)
278-279: LGTM: POSIX tag addedgetpwuid_r Tag addition is consistent with existing Tag layout.
295-299: LGTM: Windows Tag entries placed after boundaryNew Windows API tags are correctly placed below the WriteFile boundary used by Tag.isWindows().
src/os.zig (2)
29-31: LGTM: gated platform imports prevent cross-target build breakswin32/posix imports are correctly compile-time gated.
5-11: No changes needed—requested tests already exist in the codebase.The test file
test/js/node/test/parallel/test-os-homedir-no-envvar.jsalready covers both scenarios requested in the review:
- POSIX without HOME set: Test removes
HOMEenv var and verifiesos.homedir()returns a valid string (relies ongetpwuid_r()fallback implemented insrc/os/posix.zig)- Windows with USERPROFILE unset: Test removes
USERPROFILEenv var and verifiesos.homedir()returns a valid string (relies onGetUserProfileDirectoryW()fallback implemented insrc/os/win32.zig)Both implementations have proper fallback mechanisms:
- POSIX: Falls back to passwd database via
getpwuid_r()with robust error handling- Windows: Falls back to Win32 API
GetUserProfileDirectoryW()with proper error handlingThe API surface and test coverage are complete. The review comment appears to be outdated; the suggested tests are already in place and passing.
markovejnovic
force-pushed
the
user/markovejnovic/system-abstractions-2.0
branch
2 times, most recently
from
a91cd4a to
33033b9
Compare
markovejnovic
force-pushed
the
user/markovejnovic/system-abstractions-2.0
branch
from
33033b9 to
6e79a73
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (2)
src/ptr/tagged_pointer.zig (1)
91-91: Specify backing integer type for packed struct (already flagged).This issue was already raised in a previous review. The packed struct at line 91 should explicitly specify
(u64)as its backing integer type to ensure ABI clarity and maintain consistency with other packed structs in this file (e.g., line 3).src/sys.zig (1)
4268-4281: Preserve the full-widthsysconfresult
std.c.sysconfreturns ac_long. On 64‑bit platforms that’s 64 bits, and values such as_SC_PHYS_PAGESor_SC_CLK_TCKfrequently exceedu32::max. Casting every successful result tou32silently wraps those large answers, breaking consumers that rely on the real limit. Please keep the return type wide (e.g.usize) and only cast once to that width. citehttps://pubs.opengroup.org/onlinepubs/9699919799/functions/sysconf.html-pub fn sysconf(name: c_int) !u32 { +pub fn sysconf(name: c_int) !usize { @@ - if (ret >= 0) { - return @intCast(ret); + if (ret >= 0) { + return @as(usize, @intCast(ret));
📜 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.
📥 Commits
Reviewing files that changed from the base of the PR and between 1757d41542a3419af496016bac2a54d8dc632713 and 6e79a73.
📒 Files selected for processing (15)
src/bun.js/node/node_os.zig(1 hunks)src/cli/install_completions_command.zig(7 hunks)src/cli/upgrade_command.zig(2 hunks)src/fs.zig(1 hunks)src/install/PackageManager.zig(1 hunks)src/install/PackageManager/PackageManagerOptions.zig(1 hunks)src/install/repository.zig(1 hunks)src/memory.zig(1 hunks)src/os.zig(1 hunks)src/os/posix.zig(1 hunks)src/os/uvinterop.zig(1 hunks)src/os/win32.zig(1 hunks)src/ptr/tagged_pointer.zig(2 hunks)src/sys.zig(3 hunks)src/windows.zig(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.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/PackageManagerOptions.zigsrc/install/PackageManager.zigsrc/cli/install_completions_command.zigsrc/install/repository.zigsrc/cli/upgrade_command.zigsrc/os.zigsrc/ptr/tagged_pointer.zigsrc/bun.js/node/node_os.zigsrc/os/posix.zigsrc/os/uvinterop.zigsrc/os/win32.zigsrc/windows.zigsrc/sys.zigsrc/memory.zigsrc/fs.zig
src/**/*.zig
📄 CodeRabbit inference engine (.cursor/rules/building-bun.mdc)
When adding debug logs in Zig, create a scoped logger and log via Bun APIs:
const log = bun.Output.scoped(.${SCOPE}, .hidden);thenlog("...", .{})
src/**/*.zig: Use private fields in Zig with the#prefix (e.g.,struct { #foo: u32 };)
Prefer decl literals in Zig (e.g.,const decl: Decl = .{ .binding = 0, .value = 0 };)
Prefer placing@importstatements at the bottom of the Zig file (formatter may reorder automatically)
Prefer@import("bun")rather than@import("root").bunor@import("../bun.zig")In Zig code, be careful with allocators and use defer for cleanup
Files:
src/install/PackageManager/PackageManagerOptions.zigsrc/install/PackageManager.zigsrc/cli/install_completions_command.zigsrc/install/repository.zigsrc/cli/upgrade_command.zigsrc/os.zigsrc/ptr/tagged_pointer.zigsrc/bun.js/node/node_os.zigsrc/os/posix.zigsrc/os/uvinterop.zigsrc/os/win32.zigsrc/windows.zigsrc/sys.zigsrc/memory.zigsrc/fs.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/node/node_os.zig
🧠 Learnings (4)
📚 Learning: 2025-10-16T17:32:03.074Z
Learnt from: markovejnovic
PR: oven-sh/bun#23710
File: src/install/PackageManager/PackageManagerOptions.zig:187-193
Timestamp: 2025-10-16T17:32:03.074Z
Learning: In Bun's codebase (particularly in files like src/install/PackageManager/PackageManagerOptions.zig), mixing bun.EnvVar.*.get() and bun.EnvVar.*.platformGet() for environment variable lookups is intentional and safe. The code is protected by compile-time platform checks (Environment.isWindows, etc.), and compilation will fail if the wrong function is used on the wrong platform. This pattern should not be flagged as a consistency issue.
Applied to files:
src/os.zig
📚 Learning: 2025-09-26T01:09:19.196Z
Learnt from: nektro
PR: oven-sh/bun#22781
File: src/sql/shared/SQLDataCell.zig:144-168
Timestamp: 2025-09-26T01:09:19.196Z
Learning: In Zig, packed structs with a specified backing integer type (e.g., `packed struct(u32)`) are ABI-identical to their underlying integer type and can be safely passed to C functions expecting the corresponding integer type without any casting or bitcasting.
Applied to files:
src/ptr/tagged_pointer.zig
📚 Learning: 2025-10-14T23:34:06.388Z
Learnt from: CR
PR: oven-sh/bun#0
File: src/CLAUDE.md:0-0
Timestamp: 2025-10-14T23:34:06.388Z
Learning: Applies to src/**/*.zig : Prefer `import("bun")` rather than `import("root").bun` or `import("../bun.zig")`
Applied to files:
src/ptr/tagged_pointer.zig
📚 Learning: 2025-10-20T04:22:55.575Z
Learnt from: CR
PR: oven-sh/bun#0
File: CLAUDE.md:0-0
Timestamp: 2025-10-20T04:22:55.575Z
Learning: Applies to src/**/*.zig : In Zig code, be careful with allocators and use defer for cleanup
Applied to files:
src/os/win32.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 (4)
src/ptr/tagged_pointer.zig (1)
139-139: Correct fix: aligns type name lookup with type map storage.This change correctly removes the
bun.meta.typeBaseName()wrapper and uses@typeName(Type)directly, ensuring consistency with how type names are stored in the type map (line 65). The enum field names are created using@typeName(field)(lines 66-67), so thecase()function must use the same naming convention for correct field lookup.src/memory.zig (1)
207-222: Good Zig-friendly wrapper with clear constraints.The implementation correctly wraps the C API and restricts usage to the default allocator with a clear compile-time error. The debug-only behavior is appropriate for leak tracking utilities.
Minor observation: The
whyparameter is accepted but unused (line 213). As per coding guidelines, you could create a scoped logger and log the reason in debug builds:const log = bun.Output.scoped(.memory_leak, .hidden); if (comptime bun.Environment.isDebug) { if (comptime bun.allocators.isDefault(alloc)) { log("Intentionally leaking object: {s}", .{why}); __lsan_ignore_object(object); } else { @compileError("INTENTIONALLY_LEAK is not yet implemented for non-default allocators"); } }This would provide useful diagnostics while maintaining the current behavior.
src/windows.zig (2)
107-112: LGTM!The
GetSystemWindowsDirectoryWextern declaration andGetCurrentProcessalias are correctly implemented and follow Windows API conventions.
99-104: Userenv linkage verified—no action required.The
GetUserProfileDirectoryWextern declaration insrc/windows.zigis correctly paired with the build system configuration. userenv is already included in the CMake build configuration as a PRIVATE link library for Windows builds, as evidenced by its presence in theif(WIN32)block ofcmake/targets/BuildBun.cmake.
| return allocator.dupe(Child, slice); | ||
| } | ||
|
|
||
| extern "C" fn __lsan_ignore_object(object: *const anyopaque) callconv(.C) void; |
There was a problem hiding this comment.
🧩 Analysis chain
Verify LSan symbol availability across build configurations.
The extern declaration for __lsan_ignore_object will cause linker errors if LeakSanitizer is not available or enabled. Even though the symbol is only called in debug builds (line 215), the extern declaration itself must resolve at link time in all configurations.
Consider adding a conditional compilation guard or verifying that LSan is always linked in your build system. Run the following script to check how the build system handles sanitizer flags:
🏁 Script executed:
#!/bin/bash
# Check build configurations and sanitizer flags
echo "=== Checking build scripts for sanitizer configuration ==="
rg -n -C3 'sanitize|lsan|LSAN' --type-add 'build:*.{zig,cmake,make,sh,json}' --type build
echo -e "\n=== Checking if __lsan_ignore_object is used elsewhere ==="
ast-grep --pattern '__lsan_ignore_object'Length of output: 16242
Based on my verification analysis, the original review comment raises a legitimate portability concern. Here's what I found:
- AddressSanitizer integrates LeakSanitizer and enables it by default on supported platforms, but LeakSanitizer is supported on x86_64 Linux and macOS.
- The build system installs
liblsan0and conditionally enables ASAN with-fsanitize=address, but the new__lsan_ignore_objectextern declaration in line 205 is unconditional. - Even though the call to
__lsan_ignore_objectat line 217 is guarded by debug checks, the extern declaration must resolve at link time regardless of runtime conditions. - The codebase already demonstrates the correct pattern in
src/asan.zigwhere extern symbols are conditionally declared based onbun.Environment.enable_asan.
The extern declaration should be conditionally compiled to avoid linker errors on builds where:
- LSan is not explicitly enabled
- Non-LLVM/Clang compilers are used
- Unsupported platforms are targeted
- Builds disable sanitizers
The approach followed in src/asan.zig should be mirrored here to ensure the symbol is only declared when available.
🤖 Prompt for AI Agents
In src/memory.zig around line 205, the extern declaration for
__lsan_ignore_object is unconditional which can cause linker errors on
platforms/builds without LeakSanitizer; change it to follow the pattern in
src/asan.zig by wrapping the extern declaration in a comptime conditional (e.g.,
bun.Environment.enable_asan or the same compile-time predicate used in asan.zig)
so the symbol is only declared when ASAN/LSan is enabled and supported, and
remove the unconditional extern.
| const initial_buf_size: usize = @intCast( | ||
| bun.sys.sysconf(bun.c._SC_GETPW_R_SIZE_MAX) catch default_buf_size, | ||
| ); | ||
|
|
||
| buffer_size = initial_buf_size; | ||
| } | ||
|
|
||
| var result: ?*bun.c.struct_passwd = undefined; | ||
| var self: PasswdEntry = .{ | ||
| .allocator = alloc, | ||
| .buffer = alloc.alloc(u8, buffer_size) catch { | ||
| return .initErr(.fromCode(.NOMEM, .getpwuid_r)); | ||
| }, | ||
| .entry = undefined, | ||
| .entry_ptr = undefined, | ||
| }; | ||
| var deallocate_buffer: bool = true; | ||
| defer { | ||
| if (deallocate_buffer) { | ||
| self.allocator.free(self.buffer); | ||
| } | ||
| } | ||
|
|
||
| while (buffer_size <= max_buf_size) { | ||
| const rc = bun.c.getpwuid_r( | ||
| user, | ||
| &self.entry, | ||
| self.buffer.ptr, | ||
| self.buffer.len, | ||
| @ptrCast(&result), | ||
| ); | ||
| if (rc == 0) { | ||
| // Since we found the correct buffer size, let's store it for future use. | ||
| Memo.pwbuf_size.store(buffer_size, .monotonic); | ||
|
|
||
| if (result) |r| { | ||
| self.entry_ptr = r; | ||
| deallocate_buffer = false; | ||
| return .initResult(self); | ||
| } | ||
|
|
||
| return .initErr(.fromCode(.NOENT, .getpwuid_r)); | ||
| } | ||
|
|
||
| const err = std.posix.errno(rc); | ||
| switch (err) { | ||
| .NOMEM, .RANGE => { | ||
| // ENOMEM -- Insufficient memory to allocate passwd structure. | ||
| // ERANGE -- Insufficient buffer space supplied. | ||
| buffer_size *= buf_size_gain; | ||
| self.buffer = alloc.realloc(self.buffer, buffer_size) catch { | ||
| return .initErr(.fromCode(.NOMEM, .getpwuid_r)); | ||
| }; | ||
| continue; | ||
| }, | ||
| .INTR => { |
There was a problem hiding this comment.
Prevent the ERANGE spin when the initial buffer is zero.
If _SC_GETPW_R_SIZE_MAX yields 0 (allowed when the required size is indeterminate), we allocate a zero-length buffer. getpwuid_r then returns ERANGE, but the retry path multiplies buffer_size (0 * 2) and we loop forever. Clamp the initial and retry values to a positive minimum before entering the loop.
- const initial_buf_size: usize = @intCast(
- bun.sys.sysconf(bun.c._SC_GETPW_R_SIZE_MAX) catch default_buf_size,
- );
-
- buffer_size = initial_buf_size;
+ const sc = bun.sys.sysconf(bun.c._SC_GETPW_R_SIZE_MAX) catch default_buf_size;
+ const initial_buf_size: usize = if (@as(isize, sc) <= 0)
+ default_buf_size
+ else
+ @intCast(sc);
+
+ buffer_size = initial_buf_size;
}
var result: ?*bun.c.struct_passwd = undefined;
var self: PasswdEntry = .{
@@
.NOMEM, .RANGE => {
// ENOMEM -- Insufficient memory to allocate passwd structure.
// ERANGE -- Insufficient buffer space supplied.
- buffer_size *= buf_size_gain;
+ buffer_size = if (buffer_size == 0)
+ default_buf_size
+ else
+ buffer_size * buf_size_gain;
self.buffer = alloc.realloc(self.buffer, buffer_size) catch {
return .initErr(.fromCode(.NOMEM, .getpwuid_r));
};🤖 Prompt for AI Agents
In src/os/posix.zig around lines 69 to 124, the code can spin forever if
_SC_GETPW_R_SIZE_MAX returns 0 because you allocate a zero-length buffer and
ERANGE causes buffer_size *= buf_size_gain to stay zero; fix this by clamping
the initial_buf_size to a positive minimum (e.g. max(initial_buf_size, 1) or use
default_buf_size if zero) before assigning buffer_size, and also ensure the
retry growth path clamps a zero buffer_size to that same positive minimum before
multiplication (so on ERANGE set buffer_size = max(buffer_size * buf_size_gain,
min_buf_size) or if buffer_size == 0 set buffer_size = min_buf_size) so realloc
gets a nonzero size and the loop can make progress.
| @ptrCast(wchar_buf.ptr), | ||
| &path_size, | ||
| ); | ||
|
|
||
| if (rc == bun.windows.FALSE and | ||
| path_size <= @as(bun.windows.DWORD, @intCast(wchar_buf.len))) | ||
| { | ||
| // Otherwise we found ourselves some really weird error. | ||
| const err = bun.windows.GetLastError(); | ||
| // TODO(markovejnovic): This conversion between Win32Error -> SystemErrno -> E | ||
| // feels very wrong. | ||
| return .initErr( | ||
| .fromCode(@enumFromInt(@intFromEnum(err)), .GetUserProfileDirectoryW), | ||
| ); | ||
| } | ||
|
|
||
| // Either the buffer was too small (in which case path_size contains the required | ||
| // size), or we succeeded (in which case path_size contains the actual size). | ||
| return .initResult(@intCast(path_size)); | ||
| } | ||
| }).doSyscall, |
There was a problem hiding this comment.
Drop the trailing NUL from GetUserProfileDirectoryW results
On success GetUserProfileDirectoryW writes path_size including the terminating NUL. Returning that value straight to runWin32CallWithResizingBuffer means the slice we hand to toUTF8Alloc still contains the trailing \0, so the cached UTF‑8 path ends with an embedded NUL. Callers like querySysTmpDir then build strings such as C:\Users\me\0Temp, and the path is truncated at the NUL when passed back into Win32. Please only reuse the full path_size when we need to grow the buffer, and subtract one character on success before handing it back. citehttps://learn.microsoft.com/windows/win32/api/userenv/nf-userenv-getuserprofiledirectoryw
- if (rc == bun.windows.FALSE and
- path_size <= @as(bun.windows.DWORD, @intCast(wchar_buf.len)))
- {
- // Otherwise we found ourselves some really weird error.
- const err = bun.windows.GetLastError();
- return .initErr(
- .fromCode(@enumFromInt(@intFromEnum(err)), .GetUserProfileDirectoryW),
- );
- }
-
- // Either the buffer was too small (in which case path_size contains the required
- // size), or we succeeded (in which case path_size contains the actual size).
- return .initResult(@intCast(path_size));
+ if (rc == bun.windows.FALSE) {
+ if (path_size <= @as(bun.windows.DWORD, @intCast(wchar_buf.len))) {
+ const err = bun.windows.GetLastError();
+ return .initErr(
+ .fromCode(@enumFromInt(@intFromEnum(err)), .GetUserProfileDirectoryW),
+ );
+ }
+ // Buffer was too small; path_size now holds the required capacity incl. NUL.
+ return .initResult(@intCast(path_size));
+ }
+
+ bun.debug.assert(path_size > 0);
+ // Successful call: subtract the trailing NUL before sharing the slice.
+ return .initResult(@intCast(path_size - 1));🤖 Prompt for AI Agents
In src/os/win32.zig around lines 238 to 258, the code returns path_size directly
from GetUserProfileDirectoryW which includes the terminating NUL, causing
subsequent UTF‑8 conversions to embed a trailing NUL in cached paths; when the
call succeeded (buffer was large enough / rc == bun.windows.TRUE) return
path_size - 1 (drop the terminating wchar) but when the buffer was too small (rc
== bun.windows.FALSE and path_size > buffer length) still use the full path_size
to resize the buffer; adjust the return logic accordingly so callers receive the
correct character count without the trailing NUL.
| /// learn.microsoft.com/en-us/windows/win32/secauthz/access-rights-for-access-token-objects | ||
| pub const TOKEN_READ: DWORD = 0x20008; | ||
|
|
||
| /// learn.microsoft.com/en-us/windows/win32/api/processthreadsapi/nf-processthreadsapi-openprocesstoken | ||
| pub extern "advapi32" fn OpenProcessToken( | ||
| ProcessHandle: HANDLE, | ||
| DesiredAccess: DWORD, | ||
| TokenHandle: *HANDLE, | ||
| ) callconv(windows.WINAPI) BOOL; |
There was a problem hiding this comment.
🧩 Analysis chain
Verify advapi32 library linkage.
The OpenProcessToken function is declared as extern "advapi32", which requires linking against advapi32.lib. Please ensure the build system (cmake/targets/BuildBun.cmake) includes this library for the Bun target.
Run the following script to verify advapi32 linkage:
🏁 Script executed:
#!/bin/bash
# Description: Verify advapi32 is linked in CMake build configuration
# Search for advapi32 linkage in CMake files
rg -nC3 'advapi32' cmake/Length of output: 331
---
🏁 Script executed:
```shell
#!/bin/bash
# Description: Retrieve full context of windows.zig to verify doc comment consistency
# Get lines 89-112 to see all declarations and doc comment styles
sed -n '89,112p' src/windows.zig
Length of output: 918
🏁 Script executed:
#!/bin/bash
# Description: Verify userenv library linkage for GetUserProfileDirectoryW
# Search for userenv linkage in CMake files
rg -nC3 'userenv' cmake/Length of output: 374
🏁 Script executed:
#!/bin/bash
# Description: Check if there's a build.zig file that might have Windows library configuration
fd -type f -name 'build.zig' -o -name 'build.zig.zon' | head -5Length of output: 170
Fix doc comment style inconsistency at line 106.
The declaration of GetSystemWindowsDirectoryW at line 106 uses a single-line comment (//) instead of a doc comment (///), inconsistent with the other API declarations (lines 89, 92, 99). Change // to /// to maintain consistency.
Additionally, advapi32 and userenv libraries are properly linked in the build system (cmake/targets/BuildBun.cmake lines 1263, 1267), so the extern declarations will resolve correctly.
🤖 Prompt for AI Agents
In src/windows.zig around lines 89 to 97 (specifically line 106), the doc
comment for GetSystemWindowsDirectoryW uses a single-line comment ("//") which
is inconsistent with surrounding API declarations that use doc comments ("///");
change that single-line comment to a doc comment ("///") so the declaration uses
the same /// style as the other extern declarations (no code changes to linking
needed since advapi32/userenv are already linked in the build system).
| lpcchSize: *DWORD, | ||
| ) callconv(windows.WINAPI) BOOL; | ||
|
|
||
| // learn.microsoft.com/en-us/windows/win32/api/sysinfoapi/nf-sysinfoapi-getsystemwindowsdirectoryw |
There was a problem hiding this comment.
Fix doc comment style inconsistency.
The documentation comment should use /// (three slashes) instead of // (two slashes) to match the style used on lines 89, 92, and 99.
Apply this diff:
-// learn.microsoft.com/en-us/windows/win32/api/sysinfoapi/nf-sysinfoapi-getsystemwindowsdirectoryw
+/// learn.microsoft.com/en-us/windows/win32/api/sysinfoapi/nf-sysinfoapi-getsystemwindowsdirectoryw📝 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.
| // learn.microsoft.com/en-us/windows/win32/api/sysinfoapi/nf-sysinfoapi-getsystemwindowsdirectoryw | |
| /// learn.microsoft.com/en-us/windows/win32/api/sysinfoapi/nf-sysinfoapi-getsystemwindowsdirectoryw |
🤖 Prompt for AI Agents
In src/windows.zig around line 106, the documentation comment uses '//' instead
of the project-standard '///'; change the leading comment marker to '///' so it
matches the doc-comment style used on lines 89, 92, and 99, ensuring consistent
documentation formatting across the file.
2 participants
What does this PR do?
See #23878. @Jarred-Sumner voiced two concerns:
We've had a long back and forth in DMs regarding this PR and I am now trying to implement the changes that @Jarred-Sumner requested.
I ultimately think this implementation is more complex, but the API is better.
How did you verify your code works?
CI + tests coming soon. Waiting to get approvals on this API before I implement tests.