Repository navigation
Add postinstall optimizer with native binlink support and script skipping - #24283
Conversation
…ping This introduces a new postinstall optimization system that allows packages to: 1. Skip running lifecycle scripts for specific dependencies 2. Use platform-specific optional dependencies for binaries instead of postinstall scripts ## Native Binlink Optimization When a package like esbuild has platform-specific optional dependencies (@esbuild/darwin-arm64, @esbuild/linux-x64, etc.), we now: - Detect when the package uses the native binlink pattern (enabled by default for esbuild) - Find the matching platform-specific dependency based on target CPU/OS - Link binaries directly from the platform-specific package - Fall back to the main package if the platform-specific package isn't found or linking fails This eliminates the need to run postinstall scripts that just copy binaries from optional dependencies. ## Script Ignoring Packages can now skip lifecycle scripts for specific dependencies (enabled by default for sharp). This prevents running heavy postinstall scripts that may: - Download large native binaries - Compile native code unnecessarily - Fail in environments where they're not needed ## Configuration Both features support configuration via package.json: - `nativeDependencies`: Array of package names to enable native binlink optimization - `ignoreScripts`: Array of package names to skip lifecycle scripts Set these to empty arrays to disable the defaults. Feature flags are also available: - BUN_FEATURE_FLAG_DISABLE_NATIVE_DEPENDENCY_LINKER - BUN_FEATURE_FLAG_DISABLE_IGNORE_SCRIPTS ## Implementation Details - Added PostinstallOptimizer enum and List types to track optimization strategies per package - Modified Bin.Linker to support separate target paths and package names for binlinks - Updated PackageInstaller and isolated Installer to check optimizer before running scripts - Fallback logic retries without optimization if linking fails - Updated esbuild to 0.25.11 to test with latest version 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
WalkthroughThreads a new PostinstallOptimizer through installer and linker flows to enable platform-aware native binlink replacement and lifecycle-script ignoring; Linker gains target path/name and a skipped flag; installers gain replacement helpers, retry/fallback linking, feature flags, tests, and registry fixtures. Changes
Sequence Diagram(s)sequenceDiagram
participant Installer as Installer / PackageInstaller
participant Optimizer as PostinstallOptimizer
participant Linker as Bin.Linker
participant Scripts as Lifecycle Scripts
Installer->>Optimizer: maybeReplaceNodeModulesPath(name_hash, resolutions, metas, target)
Optimizer-->>Installer: ?replacementPackageID
alt replacement found
Installer->>Linker: set target_package_name & target_node_modules_path
Linker-->>Installer: link result (success / skipped_due_to_missing_bin / error)
alt skipped or error and retry allowed
Installer->>Installer: reset target_* -> original
Note right of Installer: verbose log "retrying without native binlink"
Installer->>Linker: retry link (original package_name/path)
Linker-->>Installer: final result
end
else no replacement
Installer->>Linker: link with original package_name/path
Linker-->>Installer: result
end
Installer->>Optimizer: shouldIgnoreLifecycleScripts(name_hash,resolutions,metas,target,tree_id)
alt true
Note right of Installer: skip enqueueing lifecycle scripts
else false
Installer->>Scripts: enqueueLifecycleScripts()
Scripts-->>Installer: scripts enqueued / trusted deps updated
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (2 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Disabled knowledge base sources:
📒 Files selected for processing (2)
🧰 Additional context used📓 Path-based instructions (2)**/*.zig📄 CodeRabbit inference engine (.cursor/rules/javascriptcore-class.mdc)
Files:
src/**/*.zig📄 CodeRabbit inference engine (.cursor/rules/building-bun.mdc)
Files:
🧠 Learnings (19)📓 Common learnings📚 Learning: 2025-09-08T00:41:12.052ZApplied to files:
📚 Learning: 2025-10-26T05:04:50.692ZApplied to files:
📚 Learning: 2025-10-26T05:04:50.692ZApplied to files:
📚 Learning: 2025-10-25T22:53:31.261ZApplied to files:
📚 Learning: 2025-08-30T00:13:36.815ZApplied to files:
📚 Learning: 2025-08-30T00:13:36.815ZApplied to files:
📚 Learning: 2025-08-30T00:11:57.076ZApplied to files:
📚 Learning: 2025-08-30T00:11:57.076ZApplied to files:
📚 Learning: 2025-10-25T22:53:31.261ZApplied to files:
📚 Learning: 2025-10-24T10:43:09.398ZApplied to files:
📚 Learning: 2025-09-06T03:37:41.154ZApplied to files:
📚 Learning: 2025-10-16T21:24:52.779ZApplied to files:
📚 Learning: 2025-09-02T17:09:17.391ZApplied to files:
📚 Learning: 2025-10-16T17:32:03.074ZApplied to files:
📚 Learning: 2025-11-03T20:43:06.996ZApplied to files:
📚 Learning: 2025-10-26T05:04:50.692ZApplied to files:
📚 Learning: 2025-10-19T23:27:43.240ZApplied to files:
📚 Learning: 2025-10-08T03:11:45.286ZApplied to files:
🔇 Additional comments (13)
Comment |
|
Updated 6:53 PM PT - Nov 3rd, 2025
❌ @dylan-conway, your commit d8fed87 has 3 failures in
🧪 To try this PR locally: bunx bun-pr 24283That installs a local version of the PR into your bun-24283 --bun |
![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: Path: .coderabbit.yaml
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 (11)
src/cli/link_command.zig(1 hunks)src/cli/unlink_command.zig(1 hunks)src/env_var.zig(1 hunks)src/install/PackageInstaller.zig(7 hunks)src/install/PackageManager.zig(1 hunks)src/install/bin.zig(3 hunks)src/install/install.zig(1 hunks)src/install/isolated_install/Installer.zig(6 hunks)src/install/lockfile.zig(2 hunks)src/install/lockfile/Package.zig(1 hunks)src/install/postinstall_optimizer.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/install/lockfile.zigsrc/env_var.zigsrc/cli/link_command.zigsrc/install/PackageManager.zigsrc/install/bin.zigsrc/install/PackageInstaller.zigsrc/install/isolated_install/Installer.zigsrc/install/postinstall_optimizer.zigsrc/install/install.zigsrc/cli/unlink_command.zigsrc/install/lockfile/Package.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 Zig private fields with the # prefix for encapsulation (e.g., struct { #foo: u32 })
Prefer Decl literals for initialization (e.g., const decl: Decl = .{ .binding = 0, .value = 0 };)
Place @import statements at the bottom of the file (formatter will handle ordering)
src/**/*.zig: In Zig code, manage memory carefully: use appropriate allocators and defer for cleanup
Cache JavaScriptCore class structures in ZigGlobalObject when adding new classes
Files:
src/install/lockfile.zigsrc/env_var.zigsrc/cli/link_command.zigsrc/install/PackageManager.zigsrc/install/bin.zigsrc/install/PackageInstaller.zigsrc/install/isolated_install/Installer.zigsrc/install/postinstall_optimizer.zigsrc/install/install.zigsrc/cli/unlink_command.zigsrc/install/lockfile/Package.zig
🧠 Learnings (20)
📚 Learning: 2025-10-16T02:17:35.237Z
Learnt from: markovejnovic
Repo: oven-sh/bun PR: 23710
File: src/analytics.zig:15-21
Timestamp: 2025-10-16T02:17:35.237Z
Learning: In src/analytics.zig and similar files using bun.EnvVar boolean environment variables: the new EnvVar API for boolean flags (e.g., bun.EnvVar.do_not_track.get(), bun.EnvVar.ci.get()) is designed to parse and return boolean values from environment variables, not just check for their presence. This is an intentional design change from the previous presence-based checks using bun.getenvZ().
Applied to files:
src/env_var.zig
📚 Learning: 2025-10-15T22:03:50.832Z
Learnt from: markovejnovic
Repo: oven-sh/bun PR: 23710
File: src/envvars.zig:135-144
Timestamp: 2025-10-15T22:03:50.832Z
Learning: In src/envvars.zig, the boolean feature flag cache uses a single atomic enum and should remain monotonic. Only the string cache (which uses two atomics: ptr and len) requires acquire/release ordering to prevent torn reads.
Applied to files:
src/env_var.zig
📚 Learning: 2025-09-08T00:41:12.052Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: src/bun.js/bindings/v8/CLAUDE.md:0-0
Timestamp: 2025-09-08T00:41:12.052Z
Learning: Applies to src/bun.js/bindings/v8/src/napi/napi.zig : Add new V8 API method mangled symbols to the V8API struct in src/napi/napi.zig for both GCC/Clang and MSVC
Applied to files:
src/env_var.zigsrc/cli/link_command.zigsrc/install/PackageManager.zigsrc/install/bin.zigsrc/install/postinstall_optimizer.zig
📚 Learning: 2025-08-30T00:13:36.815Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: .cursor/rules/zig-javascriptcore-classes.mdc:0-0
Timestamp: 2025-08-30T00:13:36.815Z
Learning: Applies to src/bun.js/bindings/generated_classes_list.zig : Update src/bun.js/bindings/generated_classes_list.zig to include new classes
Applied to files:
src/env_var.zigsrc/cli/link_command.zigsrc/install/PackageManager.zigsrc/install/bin.zigsrc/install/postinstall_optimizer.zig
📚 Learning: 2025-10-16T17:32:03.074Z
Learnt from: markovejnovic
Repo: oven-sh/bun PR: 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/env_var.zig
📚 Learning: 2025-09-02T18:25:27.976Z
Learnt from: taylordotfish
Repo: oven-sh/bun PR: 22227
File: src/allocators/allocation_scope.zig:284-314
Timestamp: 2025-09-02T18:25:27.976Z
Learning: In bun's custom Zig implementation, the `#` prefix for private fields is valid syntax and should not be flagged as invalid. The syntax `#fieldname` creates private fields that cannot be accessed from outside the defining struct, and usage like `self.#fieldname` is correct within the same struct. This applies to fields like `#parent`, `#state`, `#allocator`, `#trace`, etc. throughout the codebase.
Applied to files:
src/env_var.zig
📚 Learning: 2025-09-02T17:14:46.924Z
Learnt from: taylordotfish
Repo: oven-sh/bun PR: 22227
File: src/safety/alloc.zig:93-95
Timestamp: 2025-09-02T17:14:46.924Z
Learning: In bun's Zig codebase, they use a custom extension of Zig that supports private field syntax with the `#` prefix (e.g., `#allocator`, `#trace`). This is not standard Zig syntax but is valid in their custom implementation. Fields prefixed with `#` are private fields that cannot be accessed from outside the defining struct.
Applied to files:
src/env_var.zig
📚 Learning: 2025-10-16T21:24:52.779Z
Learnt from: markovejnovic
Repo: oven-sh/bun PR: 23710
File: src/crash_handler.zig:1415-1423
Timestamp: 2025-10-16T21:24:52.779Z
Learning: When a boolean EnvVar in src/envvars.zig is defined with a default value (e.g., `.default = false`), the `get()` method returns `bool` instead of `?bool`. This means you cannot distinguish between "environment variable not set" and "environment variable explicitly set to the default value". For opt-out scenarios where detection of explicit setting is needed (like `BUN_ENABLE_CRASH_REPORTING` on platforms where crash reporting defaults to enabled), either: (1) don't provide a default value so `get()` returns `?bool`, or (2) use the returned boolean directly instead of only checking if it's true.
Applied to files:
src/env_var.zig
📚 Learning: 2025-08-30T00:11:57.076Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: .cursor/rules/registering-bun-modules.mdc:0-0
Timestamp: 2025-08-30T00:11:57.076Z
Learning: Implement the core feature in Zig under its own directory within src/<feature>/
Applied to files:
src/env_var.zig
📚 Learning: 2025-10-30T21:52:04.707Z
Learnt from: pfgithub
Repo: oven-sh/bun PR: 24212
File: src/cli/publish_command.zig:782-788
Timestamp: 2025-10-30T21:52:04.707Z
Learning: In the Bun codebase (oven-sh/bun), `enable_ansi_colors` flags are used to gate both ANSI color codes and Unicode box-drawing characters/emoji. This is the established pattern across the codebase.
Applied to files:
src/env_var.zig
📚 Learning: 2025-08-30T00:13:36.815Z
Learnt from: CR
Repo: oven-sh/bun PR: 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/install/PackageManager.zigsrc/install/bin.zigsrc/install/postinstall_optimizer.zigsrc/install/install.zig
📚 Learning: 2025-08-30T00:11:00.890Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: .cursor/rules/javascriptcore-class.mdc:0-0
Timestamp: 2025-08-30T00:11:00.890Z
Learning: Applies to **/*.zig : Declare the extern C symbol in Zig and export a Zig-friendly alias for use
Applied to files:
src/install/PackageManager.zigsrc/install/install.zig
📚 Learning: 2025-08-30T09:09:18.384Z
Learnt from: Jarred-Sumner
Repo: oven-sh/bun PR: 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/install/PackageManager.zig
📚 Learning: 2025-10-19T02:44:46.354Z
Learnt from: theshadow27
Repo: oven-sh/bun PR: 23798
File: packages/bun-otel/context-propagation.test.ts:1-1
Timestamp: 2025-10-19T02:44:46.354Z
Learning: In the Bun repository, standalone packages under packages/ (e.g., bun-vscode, bun-inspector-protocol, bun-plugin-yaml, bun-plugin-svelte, bun-debug-adapter-protocol, bun-otel) co-locate their tests with package source code using *.test.ts files. This follows standard npm/monorepo patterns. The test/ directory hierarchy (test/js/bun/, test/cli/, test/js/node/) is reserved for testing Bun's core runtime APIs and built-in functionality, not standalone packages.
Applied to files:
src/install/PackageManager.zig
📚 Learning: 2025-10-25T22:53:31.261Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: src/CLAUDE.md:0-0
Timestamp: 2025-10-25T22:53:31.261Z
Learning: Applies to src/**/*.zig : Use Zig private fields with the # prefix for encapsulation (e.g., struct { #foo: u32 })
Applied to files:
src/install/bin.zig
📚 Learning: 2025-10-26T05:04:50.692Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-10-26T05:04:50.692Z
Learning: Applies to src/**/*.zig : Cache JavaScriptCore class structures in ZigGlobalObject when adding new classes
Applied to files:
src/install/bin.zigsrc/install/postinstall_optimizer.zig
📚 Learning: 2025-10-26T05:04:50.692Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-10-26T05:04:50.692Z
Learning: Run bun run zig:check-all for cross-platform Zig checks when making platform-specific changes
Applied to files:
src/install/bin.zig
📚 Learning: 2025-09-06T03:37:41.154Z
Learnt from: taylordotfish
Repo: oven-sh/bun PR: 22229
File: src/bundler/LinkerGraph.zig:0-0
Timestamp: 2025-09-06T03:37:41.154Z
Learning: In Bun's codebase, when checking import record source indices in src/bundler/LinkerGraph.zig, prefer using `if (import_index >= self.import_records.len)` bounds checking over `isValid()` checks, as the bounds check is more robust and `isValid()` is a strict subset of this condition.
Applied to files:
src/install/isolated_install/Installer.zig
📚 Learning: 2025-10-25T22:53:31.261Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: src/CLAUDE.md:0-0
Timestamp: 2025-10-25T22:53:31.261Z
Learning: Applies to src/**/*.zig : Place import statements at the bottom of the file (formatter will handle ordering)
Applied to files:
src/install/postinstall_optimizer.zigsrc/install/install.zig
📚 Learning: 2025-08-30T00:11:57.076Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: .cursor/rules/registering-bun-modules.mdc:0-0
Timestamp: 2025-08-30T00:11:57.076Z
Learning: Applies to src/**/js_*.zig : Implement JavaScript bindings in a Zig file named with a js_ prefix (e.g., js_smtp.zig, js_your_feature.zig)
Applied to files:
src/install/postinstall_optimizer.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 (6)
src/install/bin.zig (2)
848-865: LGTM: Target path redirection correctly implemented.The
buildTargetPackageDirmethod now correctly usestarget_node_modules_pathandtarget_package_nameinstead of the original fields, enabling the native binlink optimization to redirect package resolution to platform-specific dependencies.
507-513: All Bin.Linker instantiations properly initialized.Verification confirms that all 5 instantiation sites across the codebase have been updated to initialize the new
target_node_modules_pathandtarget_package_namefields:
- src/cli/link_command.zig:140
- src/cli/unlink_command.zig:103
- src/install/PackageInstaller.zig:322
- src/install/isolated_install/Installer.zig:1021 and 1387
Initialization logic is appropriate, including conditional assignments in Installer.zig for native binlink optimization paths. The change is complete and consistent across the codebase.
src/install/PackageManager.zig (1)
1324-1324: LGTM: Public alias correctly exposes PostinstallOptimizer.This alias enables external access to the PostinstallOptimizer type through the PackageManager module, consistent with the broader API exposure pattern across the install subsystem.
src/cli/unlink_command.zig (1)
104-105: LGTM: New Linker fields correctly initialized for unlink flow.The target fields are appropriately set to the local values (
node_modules_pathandname) for the unlink operation, which doesn't require path redirection.src/install/install.zig (1)
260-260: LGTM: PostinstallOptimizer correctly exported as public API.The export follows the established pattern in this file for exposing install subsystem types. This enables external modules to reference
install.PostinstallOptimizerfor postinstall optimization configuration.src/install/lockfile/Package.zig (1)
1586-1587: No action required—PostinstallOptimizer.fromPackageJSON already handles malformed or missing configuration gracefully.The implementation correctly manages all edge cases without failing the install for configuration issues:
- Missing fields (
nativeDependencies,ignoreScripts) are silently ignored via optionalexpr.get()- Non-array field values gracefully return
falseviaasArray() orelse return false- Empty arrays, non-string entries, and empty strings are all skipped without errors
- The only error propagated is memory allocation failure in
list.dynamic.put(), which is appropriateThe code meets the review concern—malformed or missing optimizer configuration is handled gracefully, allowing the install to proceed with default behavior rather than failing.
| postinstall_optimizer: PostinstallOptimizer.List = .{}, | ||
|
|
There was a problem hiding this comment.
Preserve postinstall optimizer data in load/clone paths.
The new postinstall_optimizer list is introduced here, but the lifecycle of the list never gets wired up: loadFromBytes doesn’t reset it before repopulating, cleanWithLogger/Cloner never copy it into the new lockfile, and deinit never frees it. In practice that leaves stale entries hanging around between loads, drops optimizer decisions whenever we rebuild the lockfile, and leaks allocator state. Because this list governs which packages Bun skips or trusts for lifecycle scripts, losing it means the lockfile forgets those security/perf decisions the next time we save. (bun.com)
Please zero the list before loading (this.postinstall_optimizer = .{};), copy it when we create a trimmed lockfile (either clone or rebuild the list on new), and release it in deinit. Without those pieces the postinstall optimizer configuration can’t be persisted safely.
🤖 Prompt for AI Agents
In src/install/lockfile.zig around lines 30-31, the newly-added
postinstall_optimizer list is not initialized/reset when loading, not copied
when creating trimmed/clone lockfiles, and not freed on deinit; as a result
stale entries persist between loads, optimizer decisions are lost on
rebuild/clone, and allocator state leaks. Fix by (1) zeroing/resetting
this.postinstall_optimizer at the start of loadFromBytes (e.g.
this.postinstall_optimizer = .{} before repopulating), (2) ensuring
cleanWithLogger/Cloner (or the Lockfile::new used for trimmed copies) copies or
rebuilds the postinstall_optimizer contents into the new Lockfile instance so
optimizer decisions are preserved, and (3) releasing/freeing the list in deinit
so its allocator state is cleaned up.
| for (resolutions) |resolution| { | ||
| if (resolution > metas.len) continue; | ||
| const meta: *const Meta = &metas[resolution]; | ||
| if (meta.arch == .all or meta.os == .all) continue; | ||
| if (meta.arch.isMatch(target_cpu) and meta.os.isMatch(target_os)) { | ||
| return resolution; | ||
| } | ||
| } |
There was a problem hiding this comment.
Prevent out-of-bounds access before reading meta.
Line 49 currently checks if (resolution > metas.len) and then immediately indexes metas[resolution]. When resolution == metas.len, this condition passes, but the subsequent index is still out of bounds and will panic. Please switch the comparison to >= so we never read past the end of metas.
Apply this diff:
- if (resolution > metas.len) continue;
+ if (resolution >= metas.len) continue;📝 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.
| for (resolutions) |resolution| { | |
| if (resolution > metas.len) continue; | |
| const meta: *const Meta = &metas[resolution]; | |
| if (meta.arch == .all or meta.os == .all) continue; | |
| if (meta.arch.isMatch(target_cpu) and meta.os.isMatch(target_os)) { | |
| return resolution; | |
| } | |
| } | |
| for (resolutions) |resolution| { | |
| if (resolution >= metas.len) continue; | |
| const meta: *const Meta = &metas[resolution]; | |
| if (meta.arch == .all or meta.os == .all) continue; | |
| if (meta.arch.isMatch(target_cpu) and meta.os.isMatch(target_os)) { | |
| return resolution; | |
| } | |
| } |
🤖 Prompt for AI Agents
In src/install/postinstall_optimizer.zig around lines 48 to 55, the loop checks
`if (resolution > metas.len)` before indexing `metas[resolution]`, which still
allows an out-of-bounds access when `resolution == metas.len`; change the
comparison to `>=` so the guard prevents any attempt to read past the end of the
metas array, i.e., replace the `>` check with `>=` and keep the subsequent logic
unchanged.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
src/install/isolated_install/Installer.zig (3)
1020-1052: Minor ordering inconsistency in retry logic.The retry logic is correct and properly implements fallback behavior when native binlink optimization fails. However, there's a minor inconsistency: line 1040 sets
bin_linker.err = nullbefore the cleanup logic, while the equivalent code inlinkDependencyBins(line 1411) does this after cleanup.Consider making the ordering consistent between both retry blocks:
if (bin_linker.err) |err| { if (can_retry_without_native_binlink_optimization) { - bin_linker.err = null; debug("retrying without native bin link", .{}); can_retry_without_native_binlink_optimization = false; target_node_modules_path.?.deinit(); target_node_modules_path = null; target_package_name = strings.StringOrTinyString.init(dep_name); + bin_linker.err = null; continue; }Note: This is purely for consistency—both orderings work correctly since
bin_linkeris a local variable that's recreated each iteration.
1276-1321: Implementation looks correct.The
maybeReplaceNodeModulesPathfunction properly implements the native binlink replacement logic with correct memory management using the out-parameter pattern. The debug logging follows the coding guidelines (scoped logger).Optional performance consideration: The linear search through
entry_node_ids(lines 1305-1313) is O(n) relative to the number of entries. However, this is acceptable since it only executes when:
- Native binlink optimization is enabled
- A replacement package is configured for this dependency
- A platform-specific replacement is found
These conditions make the search path rare enough that the performance impact should be negligible.
1385-1419: Retry logic correctly mirrors the binaries step implementation.The retry mechanism properly handles fallback when native binlink optimization fails. The logic is consistent with the binaries step (lines 1020-1052).
Optional refactor suggestion: There's substantial code duplication between this retry logic and the one in the binaries step (lines 1020-1052). Consider extracting the common retry pattern into a helper function. However, given that one executes in a task thread and the other on the main thread, keeping them separate may be intentional for clarity.
📜 Review details
Configuration used: Path: .coderabbit.yaml
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 (3)
src/install/isolated_install/Installer.zig(7 hunks)src/install/lockfile.zig(2 hunks)src/install/postinstall_optimizer.zig(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/install/lockfile.zig
- src/install/postinstall_optimizer.zig
🧰 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/Installer.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 Zig private fields with the # prefix for encapsulation (e.g., struct { #foo: u32 })
Prefer Decl literals for initialization (e.g., const decl: Decl = .{ .binding = 0, .value = 0 };)
Place @import statements at the bottom of the file (formatter will handle ordering)
src/**/*.zig: In Zig code, manage memory carefully: use appropriate allocators and defer for cleanup
Cache JavaScriptCore class structures in ZigGlobalObject when adding new classes
Files:
src/install/isolated_install/Installer.zig
🧠 Learnings (2)
📓 Common learnings
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-10-26T05:04:50.692Z
Learning: Run bun run zig:check-all for cross-platform Zig checks when making platform-specific changes
📚 Learning: 2025-10-08T03:11:45.286Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: .cursor/rules/building-bun.mdc:0-0
Timestamp: 2025-10-08T03:11:45.286Z
Learning: Applies to src/**/*.zig : When adding debug logs in Zig, create a scoped logger and log via Bun APIs: `const log = bun.Output.scoped(.${SCOPE}, .hidden);` then `log("...", .{})`
Applied to files:
src/install/isolated_install/Installer.zig
🔇 Additional comments (3)
src/install/isolated_install/Installer.zig (3)
438-438: LGTM!The additional data gathering for
pkg_resolutions_listsis correctly integrated and used by the new native binlink optimization logic.
1551-1551: LGTM!The debug scope declaration and
PostinstallOptimizerimport follow the coding guidelines correctly:
- Scoped logger created with
.hiddendefault visibility- Import placed at the bottom of the file
- Debug logging used appropriately throughout the new logic
Based on coding guidelines.
Also applies to: 1581-1581
1001-1052: Ensure comprehensive test coverage for the new optimization paths.The implementation correctly handles the native binlink optimization with proper fallback behavior. According to the PR objectives, the following tests are pending:
- esbuild without postinstall scripts
- sharp with scripts skipped
- custom package.json configuration
- platform-specific fallback (the retry logic implemented here)
- feature flag overrides
Please ensure these tests are added to verify both the optimization path and the fallback behavior, especially the retry logic when the platform-specific package is not found.
Also applies to: 1276-1321, 1323-1421
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
src/install/postinstall_optimizer.zig (2)
48-53: Fix the off-by-one before reading Meta.
resolution == metas.lenstill passes the guard, so the very next line dereferences past the end ofmetas, crashing the installer. Switch the comparison to>=so we never index out of bounds.- for (resolutions) |resolution| { - if (resolution > metas.len) continue; + for (resolutions) |resolution| { + if (resolution >= metas.len) continue; const meta: *const Meta = &metas[resolution];
67-79: Respect nativeDependencies opt-out.When the user sets
nativeDependencies: [], we markdisable_default_native_binlinks = true, but this function still returnstrue, so we keep attempting native binlinking against their explicit request. Flip the logic so the opt-out actually disables the feature unless there are explicit overrides and still honor the feature flag.- pub fn isNativeBinlinkEnabled(this: *const @This()) bool { - if (this.dynamic.count() == 0) { - if (this.disable_default_native_binlinks) { - return true; - } - } - - if (bun.env_var.feature_flag.BUN_FEATURE_FLAG_DISABLE_NATIVE_DEPENDENCY_LINKER.get()) { - return false; - } - - return true; - } + pub fn isNativeBinlinkEnabled(this: *const @This()) bool { + if (bun.env_var.feature_flag.BUN_FEATURE_FLAG_DISABLE_NATIVE_DEPENDENCY_LINKER.get()) { + return false; + } + + if (this.dynamic.count() == 0 and this.disable_default_native_binlinks) { + return false; + } + + return true; + }
📜 Review details
Configuration used: Path: .coderabbit.yaml
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/isolated_install/Installer.zig(7 hunks)src/install/postinstall_optimizer.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/install/isolated_install/Installer.zigsrc/install/postinstall_optimizer.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 Zig private fields with the # prefix for encapsulation (e.g., struct { #foo: u32 })
Prefer Decl literals for initialization (e.g., const decl: Decl = .{ .binding = 0, .value = 0 };)
Place @import statements at the bottom of the file (formatter will handle ordering)
src/**/*.zig: In Zig code, manage memory carefully: use appropriate allocators and defer for cleanup
Cache JavaScriptCore class structures in ZigGlobalObject when adding new classes
Files:
src/install/isolated_install/Installer.zigsrc/install/postinstall_optimizer.zig
🧠 Learnings (16)
📚 Learning: 2025-10-08T03:11:45.286Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: .cursor/rules/building-bun.mdc:0-0
Timestamp: 2025-10-08T03:11:45.286Z
Learning: Applies to src/**/*.zig : When adding debug logs in Zig, create a scoped logger and log via Bun APIs: `const log = bun.Output.scoped(.${SCOPE}, .hidden);` then `log("...", .{})`
Applied to files:
src/install/isolated_install/Installer.zig
📚 Learning: 2025-09-08T00:41:12.052Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: src/bun.js/bindings/v8/CLAUDE.md:0-0
Timestamp: 2025-09-08T00:41:12.052Z
Learning: Applies to src/bun.js/bindings/v8/src/napi/napi.zig : Add new V8 API method mangled symbols to the V8API struct in src/napi/napi.zig for both GCC/Clang and MSVC
Applied to files:
src/install/postinstall_optimizer.zig
📚 Learning: 2025-10-26T05:04:50.692Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-10-26T05:04:50.692Z
Learning: Applies to src/**/*.zig : Cache JavaScriptCore class structures in ZigGlobalObject when adding new classes
Applied to files:
src/install/postinstall_optimizer.zig
📚 Learning: 2025-08-30T00:13:36.815Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: .cursor/rules/zig-javascriptcore-classes.mdc:0-0
Timestamp: 2025-08-30T00:13:36.815Z
Learning: Applies to src/bun.js/bindings/generated_classes_list.zig : Update src/bun.js/bindings/generated_classes_list.zig to include new classes
Applied to files:
src/install/postinstall_optimizer.zig
📚 Learning: 2025-10-25T22:53:31.261Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: src/CLAUDE.md:0-0
Timestamp: 2025-10-25T22:53:31.261Z
Learning: Applies to src/**/*.zig : Place import statements at the bottom of the file (formatter will handle ordering)
Applied to files:
src/install/postinstall_optimizer.zig
📚 Learning: 2025-08-30T00:11:57.076Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: .cursor/rules/registering-bun-modules.mdc:0-0
Timestamp: 2025-08-30T00:11:57.076Z
Learning: Applies to src/**/js_*.zig : Implement JavaScript bindings in a Zig file named with a js_ prefix (e.g., js_smtp.zig, js_your_feature.zig)
Applied to files:
src/install/postinstall_optimizer.zig
📚 Learning: 2025-08-30T00:13:36.815Z
Learnt from: CR
Repo: oven-sh/bun PR: 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/install/postinstall_optimizer.zig
📚 Learning: 2025-10-26T05:04:50.692Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-10-26T05:04:50.692Z
Learning: Run bun run zig:check-all for cross-platform Zig checks when making platform-specific changes
Applied to files:
src/install/postinstall_optimizer.zig
📚 Learning: 2025-08-30T00:11:57.076Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: .cursor/rules/registering-bun-modules.mdc:0-0
Timestamp: 2025-08-30T00:11:57.076Z
Learning: Implement the core feature in Zig under its own directory within src/<feature>/
Applied to files:
src/install/postinstall_optimizer.zig
📚 Learning: 2025-08-30T00:11:00.890Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: .cursor/rules/javascriptcore-class.mdc:0-0
Timestamp: 2025-08-30T00:11:00.890Z
Learning: Applies to **/*.zig : Declare the extern C symbol in Zig and export a Zig-friendly alias for use
Applied to files:
src/install/postinstall_optimizer.zig
📚 Learning: 2025-10-04T09:52:49.414Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: src/js/CLAUDE.md:0-0
Timestamp: 2025-10-04T09:52:49.414Z
Learning: Applies to src/js/{builtins,node,bun,thirdparty,internal}/**/*.{ts,js} : Use process.platform and process.arch for platform detection (rely on inlining/dead-code elimination)
Applied to files:
src/install/postinstall_optimizer.zig
📚 Learning: 2025-10-24T10:43:09.398Z
Learnt from: fmguerreiro
Repo: oven-sh/bun PR: 23774
File: src/install/PackageManager/updatePackageJSONAndInstall.zig:548-548
Timestamp: 2025-10-24T10:43:09.398Z
Learning: In Bun's Zig codebase, the `as(usize, intCast(...))` cast pattern triggers a Zig compiler bug that causes compilation to hang indefinitely when used in complex control flow contexts (loops + short-circuit operators + optional unwrapping). Avoid this pattern and use simpler alternatives like just `intCast(...)` if type casting is necessary.
Applied to files:
src/install/postinstall_optimizer.zig
📚 Learning: 2025-09-06T03:37:41.154Z
Learnt from: taylordotfish
Repo: oven-sh/bun PR: 22229
File: src/bundler/LinkerGraph.zig:0-0
Timestamp: 2025-09-06T03:37:41.154Z
Learning: In Bun's codebase, when checking import record source indices in src/bundler/LinkerGraph.zig, prefer using `if (import_index >= self.import_records.len)` bounds checking over `isValid()` checks, as the bounds check is more robust and `isValid()` is a strict subset of this condition.
Applied to files:
src/install/postinstall_optimizer.zig
📚 Learning: 2025-10-16T21:24:52.779Z
Learnt from: markovejnovic
Repo: oven-sh/bun PR: 23710
File: src/crash_handler.zig:1415-1423
Timestamp: 2025-10-16T21:24:52.779Z
Learning: When a boolean EnvVar in src/envvars.zig is defined with a default value (e.g., `.default = false`), the `get()` method returns `bool` instead of `?bool`. This means you cannot distinguish between "environment variable not set" and "environment variable explicitly set to the default value". For opt-out scenarios where detection of explicit setting is needed (like `BUN_ENABLE_CRASH_REPORTING` on platforms where crash reporting defaults to enabled), either: (1) don't provide a default value so `get()` returns `?bool`, or (2) use the returned boolean directly instead of only checking if it's true.
Applied to files:
src/install/postinstall_optimizer.zig
📚 Learning: 2025-09-02T17:09:17.391Z
Learnt from: taylordotfish
Repo: oven-sh/bun PR: 22227
File: src/allocators.zig:778-782
Timestamp: 2025-09-02T17:09:17.391Z
Learning: In Zig's bun codebase, the `isDefault` function in `src/allocators.zig` only needs to compare `allocator.vtable == c_allocator.vtable` rather than also checking the `ptr` field, because: (1) the codebase never creates multiple allocators that use `c_allocator.vtable` but have different `ptr`s, and (2) the default allocator vtable ignores the `ptr` field anyway, so any allocators sharing the same vtable would function identically.
Applied to files:
src/install/postinstall_optimizer.zig
📚 Learning: 2025-10-16T17:32:03.074Z
Learnt from: markovejnovic
Repo: oven-sh/bun PR: 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/install/postinstall_optimizer.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
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/install/bin.zig (1)
960-971: Signal “missing dir” as a skip to trigger fallback tooIf bin points to a directory that doesn’t exist, we return early without setting skipped_due_to_missing_bin, so the retry path won’t trigger. Set the flag for consistency.
- var target_dir = bun.openDirAbsolute(abs_target_dir) catch |err| { - if (err == error.ENOENT) { - // avoid erroring when the directory does not exist - return; - } + var target_dir = bun.openDirAbsolute(abs_target_dir) catch |err| { + if (err == error.ENOENT) { + // Directory missing; allow caller to retry without native binlink + this.skipped_due_to_missing_bin = true; + return; + }
🧹 Nitpick comments (2)
src/install/isolated_install/Installer.zig (1)
1281-1327: Replacement lookup integrates well; add a quick short-circuit for no-op casesThe resolver scans entries to locate the replacement’s entry_id and computes its node_modules path. Consider early-returning if replacement_pkg_id == pkg_id to avoid redundant work.
- )) |replacement_pkg_id| { + )) |replacement_pkg_id| { + if (replacement_pkg_id == pkg_id) return null;src/install/PackageInstaller.zig (1)
1157-1184: Lifecycle script skipping: gating is correct; add testsThe shouldIgnoreLifecycleScripts gate avoids enqueue; please add CLI tests to cover:
- ignoreScripts default for sharp,
- overrides via BUN_FEATURE_FLAG_DISABLE_IGNORE_SCRIPTS=1,
- per‑project configuration in package.json.
I can draft data-driven tests under test/cli/install/ covering the three cases.
Also applies to: 1339-1369
📜 Review details
Configuration used: Path: .coderabbit.yaml
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 (9)
src/install/PackageInstaller.zig(7 hunks)src/install/PackageManager.zig(2 hunks)src/install/bin.zig(5 hunks)src/install/isolated_install/Installer.zig(7 hunks)src/install/lockfile/Package.zig(1 hunks)test/cli/install/bun-install-native-binlink.test.ts(1 hunks)test/cli/install/registry/packages/create-native-binlink-packages.ts(1 hunks)test/cli/install/registry/packages/test-native-binlink-target/package.json(1 hunks)test/cli/install/registry/packages/test-native-binlink/package.json(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/install/lockfile/Package.zig
🧰 Additional context used
📓 Path-based instructions (9)
test/**
📄 CodeRabbit inference engine (.cursor/rules/writing-tests.mdc)
Place all tests under the test/ directory
Files:
test/cli/install/registry/packages/create-native-binlink-packages.tstest/cli/install/bun-install-native-binlink.test.tstest/cli/install/registry/packages/test-native-binlink-target/package.jsontest/cli/install/registry/packages/test-native-binlink/package.json
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/registry/packages/create-native-binlink-packages.tstest/cli/install/bun-install-native-binlink.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/registry/packages/create-native-binlink-packages.tstest/cli/install/bun-install-native-binlink.test.ts
test/**/*.{ts,tsx,js,jsx,mjs,cjs}
📄 CodeRabbit inference engine (test/CLAUDE.md)
For large/repetitive strings, use
Buffer.alloc(count, fill).toString()instead of"A".repeat(count)
Files:
test/cli/install/registry/packages/create-native-binlink-packages.tstest/cli/install/bun-install-native-binlink.test.ts
test/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Avoid shell commands in tests (e.g., find, grep); use Bun's Glob and built-in tools
Files:
test/cli/install/registry/packages/create-native-binlink-packages.tstest/cli/install/bun-install-native-binlink.test.ts
test/**/*.test.{ts,js,jsx,tsx,mjs,cjs}
📄 CodeRabbit inference engine (test/CLAUDE.md)
test/**/*.test.{ts,js,jsx,tsx,mjs,cjs}: Usebun:testfor files ending with*.test.{ts,js,jsx,tsx,mjs,cjs}
Prefer concurrent tests (test.concurrent/describe.concurrent) over sequential when feasible
Organize tests withdescribeblocks to group related tests
Use utilities likedescribe.each,toMatchSnapshot, and lifecycle hooks (beforeAll,beforeEach,afterEach) and track resources for cleanup
Files:
test/cli/install/bun-install-native-binlink.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 invent random port functions
Prefer snapshot assertions and use normalizeBunSnapshot for snapshot output in tests
Never write tests that assert absence of crashes (e.g., no "panic" or "uncaught exception") in output
Use tempDir from "harness" for temporary directories; do not use tmpdirSync or fs.mkdtempSync in tests
When spawning processes in tests, assert on stdout before asserting exitCode
Do not use setTimeout in tests; await conditions instead to avoid flakiness
Files:
test/cli/install/bun-install-native-binlink.test.ts
**/*.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/Installer.zigsrc/install/bin.zigsrc/install/PackageInstaller.zigsrc/install/PackageManager.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 Zig private fields with the # prefix for encapsulation (e.g., struct { #foo: u32 })
Prefer Decl literals for initialization (e.g., const decl: Decl = .{ .binding = 0, .value = 0 };)
Place @import statements at the bottom of the file (formatter will handle ordering)
src/**/*.zig: In Zig code, manage memory carefully: use appropriate allocators and defer for cleanup
Cache JavaScriptCore class structures in ZigGlobalObject when adding new classes
Files:
src/install/isolated_install/Installer.zigsrc/install/bin.zigsrc/install/PackageInstaller.zigsrc/install/PackageManager.zig
🧠 Learnings (26)
📓 Common learnings
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-10-26T05:04:50.692Z
Learning: Run bun run zig:check-all for cross-platform Zig checks when making platform-specific changes
📚 Learning: 2025-10-19T02:44:46.354Z
Learnt from: theshadow27
Repo: oven-sh/bun PR: 23798
File: packages/bun-otel/context-propagation.test.ts:1-1
Timestamp: 2025-10-19T02:44:46.354Z
Learning: In the Bun repository, standalone packages under packages/ (e.g., bun-vscode, bun-inspector-protocol, bun-plugin-yaml, bun-plugin-svelte, bun-debug-adapter-protocol, bun-otel) co-locate their tests with package source code using *.test.ts files. This follows standard npm/monorepo patterns. The test/ directory hierarchy (test/js/bun/, test/cli/, test/js/node/) is reserved for testing Bun's core runtime APIs and built-in functionality, not standalone packages.
Applied to files:
test/cli/install/registry/packages/create-native-binlink-packages.tstest/cli/install/bun-install-native-binlink.test.tstest/cli/install/registry/packages/test-native-binlink/package.json
📚 Learning: 2025-10-04T09:52:49.414Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: src/js/CLAUDE.md:0-0
Timestamp: 2025-10-04T09:52:49.414Z
Learning: Applies to src/js/{builtins,node,bun,thirdparty,internal}/**/*.{ts,js} : Use process.platform and process.arch for platform detection (rely on inlining/dead-code elimination)
Applied to files:
test/cli/install/registry/packages/create-native-binlink-packages.ts
📚 Learning: 2025-08-30T00:12:56.803Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: .cursor/rules/writing-tests.mdc:0-0
Timestamp: 2025-08-30T00:12:56.803Z
Learning: Applies to test/js/third_party/**/*.{js,ts} : Place third-party npm package tests under test/js/third_party/
Applied to files:
test/cli/install/registry/packages/create-native-binlink-packages.ts
📚 Learning: 2025-08-30T00:12:56.803Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: .cursor/rules/writing-tests.mdc:0-0
Timestamp: 2025-08-30T00:12:56.803Z
Learning: Applies to test/cli/**/*.{js,ts} : Place CLI command tests (e.g., bun install, bun init) under test/cli/
Applied to files:
test/cli/install/registry/packages/create-native-binlink-packages.tstest/cli/install/bun-install-native-binlink.test.ts
📚 Learning: 2025-10-19T02:52:37.412Z
Learnt from: theshadow27
Repo: oven-sh/bun PR: 23798
File: packages/bun-otel/tsconfig.json:1-15
Timestamp: 2025-10-19T02:52:37.412Z
Learning: In the Bun repository, packages under packages/ (e.g., bun-otel) can follow a TypeScript-first pattern where package.json exports point directly to .ts files (not compiled .js files). Bun natively runs TypeScript, so consumers import .ts sources directly and receive full type information without needing compiled .d.ts declaration files. For such packages, adding "declaration": true or "outDir" in tsconfig.json is unnecessary and would break the export structure.
<!-- [remove_learning]
ceedde95-980e-4898-a2c6-40ff73913664
Applied to files:
test/cli/install/registry/packages/create-native-binlink-packages.ts
📚 Learning: 2025-10-12T02:22:34.373Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: test/CLAUDE.md:0-0
Timestamp: 2025-10-12T02:22:34.373Z
Learning: Applies to test/**/*.test.{ts,js,jsx,tsx,mjs,cjs} : Use `bun:test` for files ending with `*.test.{ts,js,jsx,tsx,mjs,cjs}`
Applied to files:
test/cli/install/registry/packages/create-native-binlink-packages.tstest/cli/install/bun-install-native-binlink.test.ts
📚 Learning: 2025-10-12T02:22:34.373Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: test/CLAUDE.md:0-0
Timestamp: 2025-10-12T02:22:34.373Z
Learning: Applies to test/{test/**/*.test.{ts,js,jsx,tsx,mjs,cjs},test/js/node/test/{parallel,sequential}/*.js} : When spawning Bun in tests, use `bunExe()` and `bunEnv` from `harness`
Applied to files:
test/cli/install/registry/packages/create-native-binlink-packages.tstest/cli/install/bun-install-native-binlink.test.ts
📚 Learning: 2025-08-30T00:12:56.803Z
Learnt from: CR
Repo: oven-sh/bun PR: 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/registry/packages/create-native-binlink-packages.tstest/cli/install/bun-install-native-binlink.test.ts
📚 Learning: 2025-08-30T00:12:56.803Z
Learnt from: CR
Repo: oven-sh/bun PR: 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/registry/packages/create-native-binlink-packages.tstest/cli/install/bun-install-native-binlink.test.ts
📚 Learning: 2025-10-12T02:22:34.373Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: test/CLAUDE.md:0-0
Timestamp: 2025-10-12T02:22:34.373Z
Learning: Applies to test/js/node/test/{parallel,sequential}/*.js : For `test/js/node/test/{parallel,sequential}/*.js` (no .test extension), run with `bun bd <file>` instead of `bun bd test <file>`
Applied to files:
test/cli/install/registry/packages/create-native-binlink-packages.tstest/cli/install/bun-install-native-binlink.test.ts
📚 Learning: 2025-08-30T00:09:39.100Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: .cursor/rules/dev-server-tests.mdc:0-0
Timestamp: 2025-08-30T00:09:39.100Z
Learning: Applies to test/bake/dev/bundle.test.ts : bundle.test.ts should contain DevServer-specific bundling tests
Applied to files:
test/cli/install/registry/packages/create-native-binlink-packages.ts
📚 Learning: 2025-10-26T05:04:50.692Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-10-26T05:04:50.692Z
Learning: Applies to test/**/*.{ts,tsx} : Avoid shell commands in tests (e.g., find, grep); use Bun's Glob and built-in tools
Applied to files:
test/cli/install/registry/packages/create-native-binlink-packages.ts
📚 Learning: 2025-08-30T00:12:56.803Z
Learnt from: CR
Repo: oven-sh/bun PR: 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/registry/packages/create-native-binlink-packages.tstest/cli/install/bun-install-native-binlink.test.ts
📚 Learning: 2025-10-26T01:32:04.844Z
Learnt from: Jarred-Sumner
Repo: oven-sh/bun PR: 24082
File: test/cli/test/coverage.test.ts:60-112
Timestamp: 2025-10-26T01:32:04.844Z
Learning: In the Bun repository test files (test/cli/test/*.test.ts), when spawning Bun CLI commands with Bun.spawnSync for testing, prefer using stdio: ["inherit", "inherit", "inherit"] to inherit stdio streams rather than piping them.
Applied to files:
test/cli/install/bun-install-native-binlink.test.ts
📚 Learning: 2025-10-12T02:22:34.373Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: test/CLAUDE.md:0-0
Timestamp: 2025-10-12T02:22:34.373Z
Learning: Applies to test/js/{bun,node}/** : Organize unit tests by module under `/test/js/bun/` and `/test/js/node/`
Applied to files:
test/cli/install/bun-install-native-binlink.test.ts
📚 Learning: 2025-10-12T02:22:34.373Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: test/CLAUDE.md:0-0
Timestamp: 2025-10-12T02:22:34.373Z
Learning: Applies to test/{test/**/*.test.{ts,js,jsx,tsx,mjs,cjs},test/js/node/test/{parallel,sequential}/*.js,test/**/*-fixture.ts} : Use `using`/`await using` for resource cleanup with Bun APIs (e.g., `Bun.spawn`, `Bun.listen`, `Bun.serve`)
Applied to files:
test/cli/install/bun-install-native-binlink.test.ts
📚 Learning: 2025-10-12T02:22:34.373Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: test/CLAUDE.md:0-0
Timestamp: 2025-10-12T02:22:34.373Z
Learning: Applies to test/{test/**/*.test.{ts,js,jsx,tsx,mjs,cjs},test/js/node/test/{parallel,sequential}/*.js} : Import common utilities from `harness` (e.g., `bunExe`, `bunEnv`, `tempDirWithFiles`, platform helpers, GC helpers)
Applied to files:
test/cli/install/bun-install-native-binlink.test.ts
📚 Learning: 2025-10-12T02:22:34.373Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: test/CLAUDE.md:0-0
Timestamp: 2025-10-12T02:22:34.373Z
Learning: Applies to test/**/*.test.{ts,js,jsx,tsx,mjs,cjs} : Prefer concurrent tests (`test.concurrent`/`describe.concurrent`) over sequential when feasible
Applied to files:
test/cli/install/bun-install-native-binlink.test.ts
📚 Learning: 2025-10-08T03:11:45.286Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: .cursor/rules/building-bun.mdc:0-0
Timestamp: 2025-10-08T03:11:45.286Z
Learning: Applies to src/**/*.zig : When adding debug logs in Zig, create a scoped logger and log via Bun APIs: `const log = bun.Output.scoped(.${SCOPE}, .hidden);` then `log("...", .{})`
Applied to files:
src/install/isolated_install/Installer.zig
📚 Learning: 2025-08-30T00:13:36.815Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: .cursor/rules/zig-javascriptcore-classes.mdc:0-0
Timestamp: 2025-08-30T00:13:36.815Z
Learning: Applies to src/bun.js/bindings/generated_classes_list.zig : Update src/bun.js/bindings/generated_classes_list.zig to include new classes
Applied to files:
src/install/bin.zig
📚 Learning: 2025-09-08T00:41:12.052Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: src/bun.js/bindings/v8/CLAUDE.md:0-0
Timestamp: 2025-09-08T00:41:12.052Z
Learning: Applies to src/bun.js/bindings/v8/src/napi/napi.zig : Add new V8 API method mangled symbols to the V8API struct in src/napi/napi.zig for both GCC/Clang and MSVC
Applied to files:
src/install/bin.zig
📚 Learning: 2025-10-26T05:04:50.692Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-10-26T05:04:50.692Z
Learning: Run bun run zig:check-all for cross-platform Zig checks when making platform-specific changes
Applied to files:
src/install/bin.zig
📚 Learning: 2025-10-25T22:53:31.261Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: src/CLAUDE.md:0-0
Timestamp: 2025-10-25T22:53:31.261Z
Learning: Applies to src/**/*.zig : Use Zig private fields with the # prefix for encapsulation (e.g., struct { #foo: u32 })
Applied to files:
src/install/bin.zig
📚 Learning: 2025-08-30T00:13:36.815Z
Learnt from: CR
Repo: oven-sh/bun PR: 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/install/bin.zig
📚 Learning: 2025-09-12T18:16:50.754Z
Learnt from: RiskyMH
Repo: oven-sh/bun PR: 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/install/bin.zig
🧬 Code graph analysis (1)
test/cli/install/bun-install-native-binlink.test.ts (1)
test/harness.ts (2)
VerdaccioRegistry(1631-1767)bunExe(102-105)
⏰ 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 (9)
test/cli/install/registry/packages/test-native-binlink-target/package.json (1)
1-28: Clarify the test fixture design for platform-specific package resolution testing.The test package declares support for multiple platforms (darwin, linux, win32) and architectures (arm64, x64) simultaneously. This is unusual for a platform-specific package fixture, which typically represents a single os+cpu combination (e.g., @esbuild/darwin-arm64).
Additionally, the package name doesn't follow the platform-specific naming convention used by real packages (e.g., @scope/package-os-arch).
Please clarify:
- How is this package used in the native binlink resolution test? Is it filtered by the installer based on the target platform, or are separate platform-specific variants expected?
- Should this be renamed or split into separate platform-specific packages to better mirror real-world scenarios (e.g., test-native-binlink-target-darwin-arm64, test-native-binlink-target-linux-x64)?
- Does the current fixture properly exercise the platform/architecture resolution logic in the optimizer?
src/install/bin.zig (2)
507-515: New Linker targeting metadata and skip flag look solidtarget_node_modules_path/target_package_name enable source redirection; skipped_due_to_missing_bin is the right signal for fallback.
Also applies to: 533-535
643-699: Shebang normalization: preserve permissions and perform atomic swap correctlyThe atomic replace logic and mode preservation look correct and guarded by best-effort I/O; no changes needed.
src/install/isolated_install/Installer.zig (3)
1005-1057: Retry logic is correct; ensure target_node_modules_path lifetime is managedThe loop retries without optimization if the target is missing or linking fails; and you deinit the optional AbsPath before nulling—good. No change needed.
1375-1427: Consistent fallback when linking dependency binsSymmetric to task-time path; logs are gated by verbose flag. Looks good.
1559-1590: Add scoped logger: good practiceUsing Output.scoped(.IsolatedInstaller, .hidden) aligns with repo logging guidelines.
src/install/PackageInstaller.zig (2)
259-306: Native binlink targeting and retry are integrated correctlyReads metas/resolutions and swaps target_package_name when replacement is present; falls back cleanly.
338-365: Surface bin-link errors without aborting; respects fail_early flagGood balance: logs when not silent, crashes only if configured.
src/install/PackageManager.zig (1)
92-93: PostinstallOptimizer field initialization verifiedThe field is correctly populated during init via
fromPackageJSONat src/install/lockfile/Package.zig:1587, which processes package.json data and feature flags as expected. Usage throughout PackageInstaller.zig and Installer.zig confirms proper integration with methods likeisNativeBinlinkEnabled()andshouldIgnoreLifecycleScripts(). No manual deinit required based on the List initialization pattern.
| beforeAll(async () => { | ||
| setDefaultTimeout(1000 * 60 * 5); | ||
| verdaccio = new VerdaccioRegistry(); | ||
| await verdaccio.start(); | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
Generate registry fixtures with the actual registry URL before starting tests
Without regenerating the package metadata/tarballs, the hardcoded 4873 tarball host may cause fetch failures. Generate fixtures up front using the current Verdaccio port.
@@
-import { spawn } from "bun";
+import { $, spawn } from "bun";
@@
beforeAll(async () => {
setDefaultTimeout(1000 * 60 * 5);
- verdaccio = new VerdaccioRegistry();
- await verdaccio.start();
+ verdaccio = new VerdaccioRegistry();
+ await verdaccio.start();
+ // Regenerate native-binlink test packages with this registry's URL
+ const genScript = join(import.meta.dir, "registry", "packages", "create-native-binlink-packages.ts");
+ process.env.VERDACCIO_REGISTRY_URL = verdaccio.registryUrl();
+ await $`bun ${genScript}`;
});Run the test locally after this change to confirm green:
🏁 Script executed:
#!/bin/bash
bun test test/cli/install/bun-install-native-binlink.test.tsLength of output: 41
🏁 Script executed:
#!/bin/bash
# Examine the test file
cat test/cli/install/bun-install-native-binlink.test.tsLength of output: 4877
🏁 Script executed:
#!/bin/bash
# Check for VerdaccioRegistry implementation
find test -name "*.ts" -o -name "*.js" | xargs grep -l "class VerdaccioRegistry" | head -5Length of output: 972
🏁 Script executed:
#!/bin/bash
# Search for the fixture generation script mentioned in the review
find test -path "*registry*" -name "*create-native-binlink*" -o -path "*registry*" -name "*packages*"Length of output: 103
🏁 Script executed:
#!/bin/bash
# Search for hardcoded port 4873 references in test fixtures
rg "4873" test/cli/install/ -C2Length of output: 174762
🏁 Script executed:
#!/bin/bash
# Check VerdaccioRegistry implementation and createTestDir method
cat test/harness.ts | grep -A 50 "class VerdaccioRegistry"Length of output: 1701
🏁 Script executed:
#!/bin/bash
# Check if createTestDir method regenerates packages
cat test/harness.ts | grep -A 30 "createTestDir"Length of output: 1260
🏁 Script executed:
#!/bin/bash
# Look for registryUrl method implementation
cat test/harness.ts | grep -A 10 "registryUrl"Length of output: 1313
Address hardcoded registry port in native-binlink test fixtures
The review comment is correct. Verdaccio starts on a random port via randomPort(), and registryUrl() returns the correct dynamic URL for bunfig.toml. However, the static fixture package.json files in registry/packages/ contain hardcoded http://localhost:4873/ URLs for tarballs. When Verdaccio runs on a different port, these tarball URLs won't match, causing fetch failures.
The test should regenerate fixtures before startup to embed the actual registry URL:
@@
-import { spawn } from "bun";
+import { $, spawn } from "bun";
@@
beforeAll(async () => {
setDefaultTimeout(1000 * 60 * 5);
verdaccio = new VerdaccioRegistry();
await verdaccio.start();
+ // Regenerate native-binlink test packages with this registry's URL
+ const genScript = join(import.meta.dir, "registry", "packages", "create-native-binlink-packages.ts");
+ process.env.VERDACCIO_REGISTRY_URL = verdaccio.registryUrl();
+ await $`bun ${genScript}`;
});Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In test/cli/install/bun-install-native-binlink.test.ts around lines 9-13, the
fixture package.json files in registry/packages contain hardcoded tarball URLs
(http://localhost:4873) that don't match Verdaccio's dynamic port; after
starting Verdaccio, call a fixture-regeneration step (or implement a small
script) that reads all registry/packages/*.json and rewrites tarball URLs to use
verdaccio.registryUrl() (or the returned host+port), then save the files before
running the rest of the tests so tarball URLs match the running registry.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
src/install/postinstall_optimizer.zig (2)
48-55: Fix the out-of-bounds check.Line 49 checks
if (resolution > metas.len)before accessingmetas[resolution]on line 50. Whenresolution == metas.len, the condition passes but the array access is out of bounds. Change the comparison to>=.Apply this diff:
- if (resolution > metas.len) continue; + if (resolution >= metas.len) continue;
67-79: Fix the inverted logic when defaults are disabled.When
disable_default_native_binlinksistrue(set bynativeDependencies: []in package.json), the function returnstrueon line 70, which enables native binlink despite the user's explicit request to disable it. The logic is inverted.Apply this diff:
pub fn isNativeBinlinkEnabled(this: *const @This()) bool { - if (this.dynamic.count() == 0) { - if (this.disable_default_native_binlinks) { - return true; - } - } - if (bun.env_var.feature_flag.BUN_FEATURE_FLAG_DISABLE_NATIVE_DEPENDENCY_LINKER.get()) { return false; } + if (this.dynamic.count() == 0 and this.disable_default_native_binlinks) { + return false; + } + return true; }
🧹 Nitpick comments (2)
test/integration/esbuild/esbuild.test.ts (2)
13-19: Consider simplifying the tempDir usage pattern.The pattern
using dir = tempDir(...); const packageDir = dir + ""works correctly but the empty string concatenation is unnecessary. SincetempDiralready returns a string (that's also disposable), you can use it directly:-using dir = tempDir("esbuild-test", { +using packageDir = tempDir("esbuild-test", { "package.json": JSON.stringify({ name: "bun-esbuild-test", version: "1.0.0", }), }); -const packageDir = dir + "";The same applies to the second test at lines 53-59.
Also applies to: 53-59
21-21: Use consistent variable declaration keywords.Line 21 uses
varwhile line 61 usesletfor the same pattern. Preferletconsistently for reassignable bindings.Apply this diff:
-var { stdout, stderr, exited } = spawn({ +let { stdout, stderr, exited } = spawn({Also applies to: 61-61
📜 Review details
Configuration used: Path: .coderabbit.yaml
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 (3)
src/install/PackageInstaller.zig(7 hunks)src/install/postinstall_optimizer.zig(1 hunks)test/integration/esbuild/esbuild.test.ts(9 hunks)
🧰 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/postinstall_optimizer.zigsrc/install/PackageInstaller.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 Zig private fields with the # prefix for encapsulation (e.g., struct { #foo: u32 })
Prefer Decl literals for initialization (e.g., const decl: Decl = .{ .binding = 0, .value = 0 };)
Place @import statements at the bottom of the file (formatter will handle ordering)
src/**/*.zig: In Zig code, manage memory carefully: use appropriate allocators and defer for cleanup
Cache JavaScriptCore class structures in ZigGlobalObject when adding new classes
Files:
src/install/postinstall_optimizer.zigsrc/install/PackageInstaller.zig
test/**
📄 CodeRabbit inference engine (.cursor/rules/writing-tests.mdc)
Place all tests under the test/ directory
Files:
test/integration/esbuild/esbuild.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/integration/esbuild/esbuild.test.ts
test/**/*.test.{ts,js,jsx,tsx,mjs,cjs}
📄 CodeRabbit inference engine (test/CLAUDE.md)
test/**/*.test.{ts,js,jsx,tsx,mjs,cjs}: Usebun:testfor files ending with*.test.{ts,js,jsx,tsx,mjs,cjs}
Prefer concurrent tests (test.concurrent/describe.concurrent) over sequential when feasible
Organize tests withdescribeblocks to group related tests
Use utilities likedescribe.each,toMatchSnapshot, and lifecycle hooks (beforeAll,beforeEach,afterEach) and track resources for cleanup
Files:
test/integration/esbuild/esbuild.test.ts
test/**/*.{ts,tsx,js,jsx,mjs,cjs}
📄 CodeRabbit inference engine (test/CLAUDE.md)
For large/repetitive strings, use
Buffer.alloc(count, fill).toString()instead of"A".repeat(count)
Files:
test/integration/esbuild/esbuild.test.ts
test/integration/**
📄 CodeRabbit inference engine (test/CLAUDE.md)
Place integration tests under
/test/integration/
Files:
test/integration/esbuild/esbuild.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 invent random port functions
Prefer snapshot assertions and use normalizeBunSnapshot for snapshot output in tests
Never write tests that assert absence of crashes (e.g., no "panic" or "uncaught exception") in output
Use tempDir from "harness" for temporary directories; do not use tmpdirSync or fs.mkdtempSync in tests
When spawning processes in tests, assert on stdout before asserting exitCode
Do not use setTimeout in tests; await conditions instead to avoid flakiness
Files:
test/integration/esbuild/esbuild.test.ts
test/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Avoid shell commands in tests (e.g., find, grep); use Bun's Glob and built-in tools
Files:
test/integration/esbuild/esbuild.test.ts
🧠 Learnings (40)
📓 Common learnings
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-10-26T05:04:50.692Z
Learning: Run bun run zig:check-all for cross-platform Zig checks when making platform-specific changes
📚 Learning: 2025-09-08T00:41:12.052Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: src/bun.js/bindings/v8/CLAUDE.md:0-0
Timestamp: 2025-09-08T00:41:12.052Z
Learning: Applies to src/bun.js/bindings/v8/src/napi/napi.zig : Add new V8 API method mangled symbols to the V8API struct in src/napi/napi.zig for both GCC/Clang and MSVC
Applied to files:
src/install/postinstall_optimizer.zig
📚 Learning: 2025-10-26T05:04:50.692Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-10-26T05:04:50.692Z
Learning: Applies to src/**/*.zig : Cache JavaScriptCore class structures in ZigGlobalObject when adding new classes
Applied to files:
src/install/postinstall_optimizer.zig
📚 Learning: 2025-08-30T00:13:36.815Z
Learnt from: CR
Repo: oven-sh/bun PR: 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/install/postinstall_optimizer.zig
📚 Learning: 2025-08-30T00:13:36.815Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: .cursor/rules/zig-javascriptcore-classes.mdc:0-0
Timestamp: 2025-08-30T00:13:36.815Z
Learning: Applies to src/bun.js/bindings/generated_classes_list.zig : Update src/bun.js/bindings/generated_classes_list.zig to include new classes
Applied to files:
src/install/postinstall_optimizer.zig
📚 Learning: 2025-10-26T05:04:50.692Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-10-26T05:04:50.692Z
Learning: Run bun run zig:check-all for cross-platform Zig checks when making platform-specific changes
Applied to files:
src/install/postinstall_optimizer.zig
📚 Learning: 2025-08-30T00:11:57.076Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: .cursor/rules/registering-bun-modules.mdc:0-0
Timestamp: 2025-08-30T00:11:57.076Z
Learning: Applies to src/**/js_*.zig : Implement JavaScript bindings in a Zig file named with a js_ prefix (e.g., js_smtp.zig, js_your_feature.zig)
Applied to files:
src/install/postinstall_optimizer.zig
📚 Learning: 2025-10-04T09:52:49.414Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: src/js/CLAUDE.md:0-0
Timestamp: 2025-10-04T09:52:49.414Z
Learning: Applies to src/js/{builtins,node,bun,thirdparty,internal}/**/*.{ts,js} : Use process.platform and process.arch for platform detection (rely on inlining/dead-code elimination)
Applied to files:
src/install/postinstall_optimizer.zig
📚 Learning: 2025-10-25T22:53:31.261Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: src/CLAUDE.md:0-0
Timestamp: 2025-10-25T22:53:31.261Z
Learning: Applies to src/**/*.zig : Place import statements at the bottom of the file (formatter will handle ordering)
Applied to files:
src/install/postinstall_optimizer.zig
📚 Learning: 2025-08-30T00:11:57.076Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: .cursor/rules/registering-bun-modules.mdc:0-0
Timestamp: 2025-08-30T00:11:57.076Z
Learning: Implement the core feature in Zig under its own directory within src/<feature>/
Applied to files:
src/install/postinstall_optimizer.zig
📚 Learning: 2025-10-25T22:53:31.261Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: src/CLAUDE.md:0-0
Timestamp: 2025-10-25T22:53:31.261Z
Learning: Applies to src/**/*.zig : Prefer Decl literals for initialization (e.g., const decl: Decl = .{ .binding = 0, .value = 0 };)
Applied to files:
src/install/postinstall_optimizer.zig
📚 Learning: 2025-10-24T10:43:09.398Z
Learnt from: fmguerreiro
Repo: oven-sh/bun PR: 23774
File: src/install/PackageManager/updatePackageJSONAndInstall.zig:548-548
Timestamp: 2025-10-24T10:43:09.398Z
Learning: In Bun's Zig codebase, the `as(usize, intCast(...))` cast pattern triggers a Zig compiler bug that causes compilation to hang indefinitely when used in complex control flow contexts (loops + short-circuit operators + optional unwrapping). Avoid this pattern and use simpler alternatives like just `intCast(...)` if type casting is necessary.
Applied to files:
src/install/postinstall_optimizer.zig
📚 Learning: 2025-09-06T03:37:41.154Z
Learnt from: taylordotfish
Repo: oven-sh/bun PR: 22229
File: src/bundler/LinkerGraph.zig:0-0
Timestamp: 2025-09-06T03:37:41.154Z
Learning: In Bun's codebase, when checking import record source indices in src/bundler/LinkerGraph.zig, prefer using `if (import_index >= self.import_records.len)` bounds checking over `isValid()` checks, as the bounds check is more robust and `isValid()` is a strict subset of this condition.
Applied to files:
src/install/postinstall_optimizer.zig
📚 Learning: 2025-10-16T21:24:52.779Z
Learnt from: markovejnovic
Repo: oven-sh/bun PR: 23710
File: src/crash_handler.zig:1415-1423
Timestamp: 2025-10-16T21:24:52.779Z
Learning: When a boolean EnvVar in src/envvars.zig is defined with a default value (e.g., `.default = false`), the `get()` method returns `bool` instead of `?bool`. This means you cannot distinguish between "environment variable not set" and "environment variable explicitly set to the default value". For opt-out scenarios where detection of explicit setting is needed (like `BUN_ENABLE_CRASH_REPORTING` on platforms where crash reporting defaults to enabled), either: (1) don't provide a default value so `get()` returns `?bool`, or (2) use the returned boolean directly instead of only checking if it's true.
Applied to files:
src/install/postinstall_optimizer.zig
📚 Learning: 2025-09-02T17:09:17.391Z
Learnt from: taylordotfish
Repo: oven-sh/bun PR: 22227
File: src/allocators.zig:778-782
Timestamp: 2025-09-02T17:09:17.391Z
Learning: In Zig's bun codebase, the `isDefault` function in `src/allocators.zig` only needs to compare `allocator.vtable == c_allocator.vtable` rather than also checking the `ptr` field, because: (1) the codebase never creates multiple allocators that use `c_allocator.vtable` but have different `ptr`s, and (2) the default allocator vtable ignores the `ptr` field anyway, so any allocators sharing the same vtable would function identically.
Applied to files:
src/install/postinstall_optimizer.zig
📚 Learning: 2025-10-16T17:32:03.074Z
Learnt from: markovejnovic
Repo: oven-sh/bun PR: 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/install/postinstall_optimizer.zig
📚 Learning: 2025-10-12T02:22:34.373Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: test/CLAUDE.md:0-0
Timestamp: 2025-10-12T02:22:34.373Z
Learning: Applies to test/{test/**/*.test.{ts,js,jsx,tsx,mjs,cjs},test/js/node/test/{parallel,sequential}/*.js} : Use `tempDirWithFiles` (or `tempDir`) from `harness` for temporary directories/files
Applied to files:
test/integration/esbuild/esbuild.test.ts
📚 Learning: 2025-10-26T05:04:50.692Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-10-26T05:04:50.692Z
Learning: Applies to test/**/*.test.{ts,tsx} : Use tempDir from "harness" for temporary directories; do not use tmpdirSync or fs.mkdtempSync in tests
Applied to files:
test/integration/esbuild/esbuild.test.ts
📚 Learning: 2025-08-30T00:09:39.100Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: .cursor/rules/dev-server-tests.mdc:0-0
Timestamp: 2025-08-30T00:09:39.100Z
Learning: Applies to test/bake/dev/esm.test.ts : esm.test.ts should cover ESM feature behavior in development mode
Applied to files:
test/integration/esbuild/esbuild.test.ts
📚 Learning: 2025-10-12T02:22:34.373Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: test/CLAUDE.md:0-0
Timestamp: 2025-10-12T02:22:34.373Z
Learning: Applies to test/**/*.test.{ts,js,jsx,tsx,mjs,cjs} : Prefer concurrent tests (`test.concurrent`/`describe.concurrent`) over sequential when feasible
Applied to files:
test/integration/esbuild/esbuild.test.ts
📚 Learning: 2025-08-30T00:09:39.100Z
Learnt from: CR
Repo: oven-sh/bun PR: 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} : Do not use node:fs APIs in tests; mutate files via dev.write, dev.patch, and dev.delete
Applied to files:
test/integration/esbuild/esbuild.test.ts
📚 Learning: 2025-08-30T00:09:39.100Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: .cursor/rules/dev-server-tests.mdc:0-0
Timestamp: 2025-08-30T00:09:39.100Z
Learning: Applies to test/bake/dev/ecosystem.test.ts : ecosystem.test.ts should focus on concrete library integration bugs rather than whole-package coverage
Applied to files:
test/integration/esbuild/esbuild.test.ts
📚 Learning: 2025-08-30T00:12:56.803Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: .cursor/rules/writing-tests.mdc:0-0
Timestamp: 2025-08-30T00:12:56.803Z
Learning: Applies to test/js/third_party/**/*.{js,ts} : Place third-party npm package tests under test/js/third_party/
Applied to files:
test/integration/esbuild/esbuild.test.ts
📚 Learning: 2025-10-12T02:22:34.373Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: test/CLAUDE.md:0-0
Timestamp: 2025-10-12T02:22:34.373Z
Learning: Applies to test/{test/**/*.test.{ts,js,jsx,tsx,mjs,cjs},test/js/node/test/{parallel,sequential}/*.js} : Always check exit codes and error scenarios in tests (e.g., spawned processes should assert non-zero on failure)
Applied to files:
test/integration/esbuild/esbuild.test.ts
📚 Learning: 2025-10-12T02:22:34.373Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: test/CLAUDE.md:0-0
Timestamp: 2025-10-12T02:22:34.373Z
Learning: Applies to test/**/*.test.{ts,js,jsx,tsx,mjs,cjs} : Use utilities like `describe.each`, `toMatchSnapshot`, and lifecycle hooks (`beforeAll`, `beforeEach`, `afterEach`) and track resources for cleanup
Applied to files:
test/integration/esbuild/esbuild.test.ts
📚 Learning: 2025-10-26T05:04:50.692Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-10-26T05:04:50.692Z
Learning: Applies to test/**/*.{ts,tsx} : Avoid shell commands in tests (e.g., find, grep); use Bun's Glob and built-in tools
Applied to files:
test/integration/esbuild/esbuild.test.ts
📚 Learning: 2025-10-12T02:22:34.373Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: test/CLAUDE.md:0-0
Timestamp: 2025-10-12T02:22:34.373Z
Learning: Applies to test/{test/**/*.test.{ts,js,jsx,tsx,mjs,cjs},test/js/node/test/{parallel,sequential}/*.js} : When spawning Bun in tests, use `bunExe()` and `bunEnv` from `harness`
Applied to files:
test/integration/esbuild/esbuild.test.ts
📚 Learning: 2025-08-30T00:12:56.803Z
Learnt from: CR
Repo: oven-sh/bun PR: 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/integration/esbuild/esbuild.test.ts
📚 Learning: 2025-10-12T02:22:34.373Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: test/CLAUDE.md:0-0
Timestamp: 2025-10-12T02:22:34.373Z
Learning: Applies to test/{test/**/*.test.{ts,js,jsx,tsx,mjs,cjs},test/js/node/test/{parallel,sequential}/*.js,test/**/*-fixture.ts} : Use `using`/`await using` for resource cleanup with Bun APIs (e.g., `Bun.spawn`, `Bun.listen`, `Bun.serve`)
Applied to files:
test/integration/esbuild/esbuild.test.ts
📚 Learning: 2025-08-30T00:12:56.803Z
Learnt from: CR
Repo: oven-sh/bun PR: 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/integration/esbuild/esbuild.test.ts
📚 Learning: 2025-10-12T02:22:34.373Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: test/CLAUDE.md:0-0
Timestamp: 2025-10-12T02:22:34.373Z
Learning: Applies to test/{test/**/*.test.{ts,js,jsx,tsx,mjs,cjs},test/js/node/test/{parallel,sequential}/*.js} : Import common utilities from `harness` (e.g., `bunExe`, `bunEnv`, `tempDirWithFiles`, platform helpers, GC helpers)
Applied to files:
test/integration/esbuild/esbuild.test.ts
📚 Learning: 2025-10-19T02:44:46.354Z
Learnt from: theshadow27
Repo: oven-sh/bun PR: 23798
File: packages/bun-otel/context-propagation.test.ts:1-1
Timestamp: 2025-10-19T02:44:46.354Z
Learning: In the Bun repository, standalone packages under packages/ (e.g., bun-vscode, bun-inspector-protocol, bun-plugin-yaml, bun-plugin-svelte, bun-debug-adapter-protocol, bun-otel) co-locate their tests with package source code using *.test.ts files. This follows standard npm/monorepo patterns. The test/ directory hierarchy (test/js/bun/, test/cli/, test/js/node/) is reserved for testing Bun's core runtime APIs and built-in functionality, not standalone packages.
Applied to files:
test/integration/esbuild/esbuild.test.ts
📚 Learning: 2025-10-12T02:22:34.373Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: test/CLAUDE.md:0-0
Timestamp: 2025-10-12T02:22:34.373Z
Learning: Applies to test/**/*.test.{ts,js,jsx,tsx,mjs,cjs} : Use `bun:test` for files ending with `*.test.{ts,js,jsx,tsx,mjs,cjs}`
Applied to files:
test/integration/esbuild/esbuild.test.ts
📚 Learning: 2025-10-26T01:32:04.844Z
Learnt from: Jarred-Sumner
Repo: oven-sh/bun PR: 24082
File: test/cli/test/coverage.test.ts:60-112
Timestamp: 2025-10-26T01:32:04.844Z
Learning: In the Bun repository test files (test/cli/test/*.test.ts), when spawning Bun CLI commands with Bun.spawnSync for testing, prefer using stdio: ["inherit", "inherit", "inherit"] to inherit stdio streams rather than piping them.
Applied to files:
test/integration/esbuild/esbuild.test.ts
📚 Learning: 2025-10-12T02:22:34.373Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: test/CLAUDE.md:0-0
Timestamp: 2025-10-12T02:22:34.373Z
Learning: Applies to test/{test/**/*.test.{ts,js,jsx,tsx,mjs,cjs},test/js/node/test/{parallel,sequential}/*.js} : Do not set explicit test timeouts; Bun already has timeouts
Applied to files:
test/integration/esbuild/esbuild.test.ts
📚 Learning: 2025-10-26T05:04:50.692Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-10-26T05:04:50.692Z
Learning: Applies to test/**/*.test.{ts,tsx} : When spawning processes in tests, assert on stdout before asserting exitCode
Applied to files:
test/integration/esbuild/esbuild.test.ts
📚 Learning: 2025-10-12T02:22:34.373Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: test/CLAUDE.md:0-0
Timestamp: 2025-10-12T02:22:34.373Z
Learning: Applies to test/{test/**/*.test.{ts,js,jsx,tsx,mjs,cjs},test/js/node/test/{parallel,sequential}/*.js} : Prefer async/await; for single callbacks, use `Promise.withResolvers()`
Applied to files:
test/integration/esbuild/esbuild.test.ts
📚 Learning: 2025-10-26T05:04:50.692Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-10-26T05:04:50.692Z
Learning: Applies to test/bundler/**/*.test.{ts,tsx} : Bundler/transpiler tests must use the itBundled helper
Applied to files:
test/integration/esbuild/esbuild.test.ts
📚 Learning: 2025-08-30T00:09:39.100Z
Learnt from: CR
Repo: oven-sh/bun PR: 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} : Explicitly assert client console logs with c.expectMessage; unasserted logs should not appear
Applied to files:
test/integration/esbuild/esbuild.test.ts
📚 Learning: 2025-10-08T13:48:02.430Z
Learnt from: Jarred-Sumner
Repo: oven-sh/bun PR: 23373
File: test/js/bun/tarball/extract.test.ts:107-111
Timestamp: 2025-10-08T13:48:02.430Z
Learning: In Bun's test runner, use `expect(async () => { await ... }).toThrow()` to assert async rejections. Unlike Jest/Vitest, Bun does not require `await expect(...).rejects.toThrow()` - the async function wrapper with `.toThrow()` is the correct pattern for async error assertions in Bun tests.
Applied to files:
test/integration/esbuild/esbuild.test.ts
🧬 Code graph analysis (1)
test/integration/esbuild/esbuild.test.ts (1)
test/harness.ts (2)
tempDir(277-284)bunExe(102-105)
⏰ 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 (14)
test/integration/esbuild/esbuild.test.ts (2)
73-76: LGTM: Promise.all pattern is efficient and correctly ordered.The refactor to use
Promise.allfor collecting process outputs in parallel is clean and efficient. The assertion order (checking stdout/stderr before exitCode) correctly follows the testing guidelines.Also applies to: 87-90, 103-104, 130-134, 145-148, 159-162, 173-176, 187-190
11-191: Consider adding explicit test coverage for postinstall optimizer behavior.While these tests verify that esbuild installation and execution work correctly, they don't explicitly validate the new postinstall optimizer feature introduced in this PR. Based on the PR summary, tests for "esbuild install without postinstall scripts" are pending.
Consider adding test cases that verify:
- Postinstall scripts are skipped for configured packages (esbuild)
- Native binlinks work correctly for platform-specific packages
- Fallback behavior when platform-specific package is not found
- Feature flag overrides (BUN_FEATURE_FLAG_DISABLE_NATIVE_DEPENDENCY_LINKER)
The existing tests provide a baseline that the optimizer doesn't break normal esbuild usage, which is valuable. However, explicit coverage of the new optimization paths would increase confidence in the feature.
src/install/postinstall_optimizer.zig (7)
1-11: LGTM!The enum definition and default package hashes are well-structured and align with the PR objectives (esbuild for native binlink, sharp for ignore).
13-29: LGTM!The string array parsing logic correctly handles empty arrays (which disable defaults) and filters/hashes valid package names into the dynamic map.
31-38: LGTM!The package.json parsing correctly sets disable flags when empty arrays are provided, aligning with the PR requirement that empty arrays disable defaults.
81-106: LGTM!The lifecycle script skipping logic is correct. For
native_binlinkpackages, it only skips scripts when a platform-specific replacement package is found. Forignorepackages, it always skips scripts. This ensures fallback postinstall scripts can run when native binlink replacement fails.
108-120: LGTM!The default package lookup correctly maps esbuild to
native_binlinkand sharp toignoreas specified in the PR objectives.
122-141: LGTM!The
getmethod correctly prioritizes user-configured packages over defaults and respects the disable flags when returning default optimizers.
145-156: LGTM!Imports follow the Zig coding guideline of placing them at the bottom of the file, and all imported types are used in the implementation.
src/install/PackageInstaller.zig (5)
259-305: LGTM!The native binlink optimization logic correctly captures package metadata, checks for platform-specific replacements, and sets up retry state when a replacement is found. The
.ignorecase appropriately does nothing in the linking phase since it only affects lifecycle scripts.
322-368: LGTM!The retry loop correctly implements a one-time fallback when native binlink replacement fails. The loop prevents infinite retries by resetting the flag, logs the retry when verbose mode is enabled, and properly handles errors with fail-early support.
1156-1190: LGTM!The lifecycle script handling correctly consults the postinstall optimizer before enqueueing scripts. It passes platform-specific parameters to ensure native binlink packages only skip scripts when a replacement exists, and properly logs when scripts are ignored.
1345-1381: LGTM!The lifecycle script handling for cached packages mirrors the fresh install logic with appropriate differences in trusted dependency handling. While there is some code duplication with the earlier block (lines 1156-1190), the different trust propagation logic justifies the separate implementations.
1514-1514: LGTM!The PostinstallOptimizer import follows the existing pattern for importing types from the install module.
|
|
||
| var err = await stderr.text(); | ||
| var out = await stdout.text(); | ||
| [err, out, exitCode] = await Promise.all([new Response(stderr).text(), new Response(stdout).text(), exited]); |
There was a problem hiding this comment.
Remove unnecessary Response wrapper for consistency.
Line 73 wraps stderr and stdout with new Response(...) before calling .text(), but all other instances in this file (lines 87, 103, 130, 145, 159, 173, 187) call .text() directly on the streams. The wrapper is unnecessary since Bun's spawn streams already have a .text() method.
Apply this diff:
-[err, out, exitCode] = await Promise.all([new Response(stderr).text(), new Response(stdout).text(), exited]);
+[err, out, exitCode] = await Promise.all([stderr.text(), stdout.text(), exited]);📝 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.
| [err, out, exitCode] = await Promise.all([new Response(stderr).text(), new Response(stdout).text(), exited]); | |
| [err, out, exitCode] = await Promise.all([stderr.text(), stdout.text(), exited]); |
🤖 Prompt for AI Agents
In test/integration/esbuild/esbuild.test.ts around line 73, the code wraps
stderr and stdout with new Response(...) before calling .text(), which is
inconsistent with other instances and unnecessary because Bun's spawn streams
already expose .text(); remove the Response wrapper and call .text() directly on
stderr and stdout (i.e., replace new Response(stderr).text() and new
Response(stdout).text() with stderr.text() and stdout.text()) so the Promise.all
call matches the rest of the file.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
bench/install/README.md (1)
23-23: Replace informal language for consistency with documentation standards.The phrase "Check out these awesome tutorials" uses informal language. Consider replacing "awesome" with a more professional alternative such as "recommended," "excellent," or "comprehensive."
- [Learn the T3 Stack](https://create.t3.gg/en/faq#what-learning-resources-are-currently-available) — Check out these awesome tutorials + [Learn the T3 Stack](https://create.t3.gg/en/faq#what-learning-resources-are-currently-available) — Check out these recommended tutorialsbench/install/next.config.js (1)
7-10: Consider adding Next.js optimizations for the benchmark.The empty configuration uses Next.js defaults, which is valid. However, for a benchmark/install testing setup, consider adding configurations to optimize build performance or enable experimental features relevant to the PR's postinstall optimization work.
Examples of useful additions:
/** @type {import("next").NextConfig} */ const config = { // Enable static exports if this is just for testing // output: 'export', // Optimize for faster builds in CI/benchmark environments swcMinify: true, // Reduce bundle size for testing reactStrictMode: true, };bench/install/package.json (1)
6-19: Consider adding a test script.The scripts section is well-organized for Next.js development, but lacks a test script. For a benchmark/install testing package, adding a test script would be beneficial for automated verification.
Example:
{ "scripts": { "build": "next build", "test": "bun test", ... } }
📜 Review details
Configuration used: Path: .coderabbit.yaml
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 ignored due to path filters (2)
bench/install/bun.lockis excluded by!**/*.lockbench/install/public/favicon.icois excluded by!**/*.ico
📒 Files selected for processing (13)
bench/install/README.md(1 hunks)bench/install/app/entry.client.tsx(0 hunks)bench/install/app/entry.server.tsx(0 hunks)bench/install/app/root.tsx(0 hunks)bench/install/app/routes/_index.tsx(0 hunks)bench/install/next-env.d.ts(1 hunks)bench/install/next.config.js(1 hunks)bench/install/package.json(1 hunks)bench/install/remix.config.js(0 hunks)bench/install/remix.env.d.ts(0 hunks)bench/install/tsconfig.json(0 hunks)src/install/PackageInstaller.zig(7 hunks)src/install/isolated_install/Installer.zig(8 hunks)
💤 Files with no reviewable changes (7)
- bench/install/app/entry.client.tsx
- bench/install/remix.config.js
- bench/install/app/entry.server.tsx
- bench/install/app/root.tsx
- bench/install/tsconfig.json
- bench/install/remix.env.d.ts
- bench/install/app/routes/_index.tsx
✅ Files skipped from review due to trivial changes (1)
- bench/install/next-env.d.ts
🧰 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/Installer.zigsrc/install/PackageInstaller.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 Zig private fields with the # prefix for encapsulation (e.g., struct { #foo: u32 })
Prefer Decl literals for initialization (e.g., const decl: Decl = .{ .binding = 0, .value = 0 };)
Place @import statements at the bottom of the file (formatter will handle ordering)
src/**/*.zig: In Zig code, manage memory carefully: use appropriate allocators and defer for cleanup
Cache JavaScriptCore class structures in ZigGlobalObject when adding new classes
Files:
src/install/isolated_install/Installer.zigsrc/install/PackageInstaller.zig
🧠 Learnings (3)
📓 Common learnings
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: test/CLAUDE.md:0-0
Timestamp: 2025-10-12T02:22:34.373Z
Learning: Applies to test/{test/**/*.test.{ts,js,jsx,tsx,mjs,cjs},test/js/node/test/{parallel,sequential}/*.js} : When spawning Bun in tests, use `bunExe()` and `bunEnv` from `harness`
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: src/js/CLAUDE.md:0-0
Timestamp: 2025-10-04T09:52:49.414Z
Learning: Applies to src/js/{builtins,node,bun,thirdparty,internal}/**/*.{ts,js} : Use process.platform and process.arch for platform detection (rely on inlining/dead-code elimination)
📚 Learning: 2025-10-19T02:44:46.354Z
Learnt from: theshadow27
Repo: oven-sh/bun PR: 23798
File: packages/bun-otel/context-propagation.test.ts:1-1
Timestamp: 2025-10-19T02:44:46.354Z
Learning: In the Bun repository, standalone packages under packages/ (e.g., bun-vscode, bun-inspector-protocol, bun-plugin-yaml, bun-plugin-svelte, bun-debug-adapter-protocol, bun-otel) co-locate their tests with package source code using *.test.ts files. This follows standard npm/monorepo patterns. The test/ directory hierarchy (test/js/bun/, test/cli/, test/js/node/) is reserved for testing Bun's core runtime APIs and built-in functionality, not standalone packages.
Applied to files:
bench/install/README.md
📚 Learning: 2025-10-08T03:11:45.286Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: .cursor/rules/building-bun.mdc:0-0
Timestamp: 2025-10-08T03:11:45.286Z
Learning: Applies to src/**/*.zig : When adding debug logs in Zig, create a scoped logger and log via Bun APIs: `const log = bun.Output.scoped(.${SCOPE}, .hidden);` then `log("...", .{})`
Applied to files:
src/install/isolated_install/Installer.zig
🪛 LanguageTool
bench/install/README.md
[style] ~23-~23: Consider using a more formal and expressive alternative to ‘awesome’.
Context: ...-currently-available) — Check out these awesome tutorials You can check out the [creat...
(AWESOME)
🔇 Additional comments (6)
bench/install/README.md (1)
1-29: Verify: Complete content replacement appears misaligned with file purpose and PR objectives.This file in the
bench/install/directory has been entirely replaced with generic T3 Stack boilerplate documentation. However:
- The original content (per AI summary) contained specific benchmarking instructions (hyperfine usage, online/offline modes, sample commands), which are now completely removed.
- The new content focuses on Next.js, Prisma, Drizzle, etc.—unrelated to the postinstall optimizer feature described in the PR objectives.
- The AI summary mentions "Next.js-based bench setup," but this is ambiguous without seeing the broader context.
This appears to be either:
- An unintended file modification (wrong file updated during a refactor)
- A merge conflict resolved incorrectly
- Part of a larger architectural change to the benchmarking infrastructure (in which case the original instructions should be migrated elsewhere)
Please clarify:
- Is this change intentional as part of a restructuring of the bench/install directory?
- Where should developers now find the original benchmarking instructions?
- Does this align with the PR's goal of adding the postinstall optimizer feature?
If this is indeed intentional, ensure the original benchmarking documentation is available elsewhere or is updated in a companion PR.
bench/install/package.json (5)
2-5: LGTM!The package metadata is correctly configured with ESM support (
"type": "module"), which aligns with Next.js 15 and modern tooling requirements.
49-51: LGTM!The T3 stack metadata correctly tracks the initialization version for future migrations and upgrades.
47-47: No action required.TypeScript 5.8.2 was released February 28, 2025, making it a stable, established version as of this PR's date. Next.js 15 officially supports TypeScript and is generally compatible with recent TypeScript releases. The specified @types packages (react 19.0.0, react-dom 19.0.0) are appropriately aligned. No compatibility issues exist.
45-46: PostCSS and Tailwind CSS v4 compatibility verified—no issues found.The
package.jsoncorrectly includes both required dependencies:@tailwindcss/postcss(^4.0.15) andpostcss(^8.5.3). PostCSS 8.5.3 satisfies the 8.x requirement for Tailwind v4, and the separate PostCSS plugin package is present. The configuration is compatible.
30-30: next-auth beta version may introduce instability if benchmark is completed.The package uses
next-auth@5.0.0-beta.25with Next.js15.2.3, which fall within a documented problematic compatibility range. Web searches confirm real issues: route handler export failures, middlewareauth()null returns, client bundle bloat, and login state bugs affecting Next.js 15.2.x + next-auth 5.0.0-beta.25.However,
bench/installis a T3 Stack template scaffold with no application code (nosrc/directory, no route handlers, no middleware, no client-side auth hooks). It's not actively executed as a benchmark (noinstallscript in parentpackage.json). The documented bugs won't trigger in the current state, but if this template is completed and deployed, these issues could surface.
| can_retry_without_native_binlink_optimization = false; | ||
| target_node_modules_path.?.deinit(); | ||
| target_node_modules_path = null; | ||
| if (PackageManager.verbose_install) { |
There was a problem hiding this comment.
| if (PackageManager.verbose_install) { | |
| if (this.manager.options.log_level.isVerbose()) { |
| .global_bin_path = this.manager.options.bin_path, | ||
| .package_name = package_name, | ||
| .string_buf = string_buf, | ||
| .extern_string_buf = extern_string_buf, | ||
| .seen = &seen, | ||
| .node_modules_path = &node_modules_path, | ||
| .target_node_modules_path = if (target_node_modules_path) |*path| path else &node_modules_path, | ||
| .target_package_name = if (target_node_modules_path != null) target_package_name else package_name, | ||
| .abs_target_buf = link_target_buf, | ||
| .abs_dest_buf = link_dest_buf, | ||
| .rel_buf = link_rel_buf, | ||
| }; | ||
|
|
||
| bin_linker.link(false); | ||
| bin_linker.link(false); | ||
|
|
||
| if (can_retry_without_native_binlink_optimization and (bin_linker.skipped_due_to_missing_bin or bin_linker.err != null)) { | ||
| can_retry_without_native_binlink_optimization = false; | ||
| target_node_modules_path.?.deinit(); | ||
| target_node_modules_path = null; | ||
| if (PackageManager.verbose_install) { | ||
| Output.prettyErrorln("<d>[Bin Linker]<r> {s} -> {s} retrying without native bin link", .{ | ||
| package_name.slice(), | ||
| target_package_name.slice(), | ||
| }); | ||
| } | ||
| continue; | ||
| } | ||
|
|
||
| if (bin_linker.err) |err| { | ||
| return err; | ||
| if (bin_linker.err) |err| { | ||
| return err; | ||
| } | ||
|
|
||
| break; |
There was a problem hiding this comment.
this should be more simple. i'd delete can_retry_without_native_binlink_optimization and the while(true). after the first link do the second link:
if (target_node_modules_path != null and (bin_linker.skipped_due_to_missing_bin or bin_linker.err != null)) {
target_node_modules_path.?.deinit();
target_node_modules_path = null;
bin_linker.target_node_modules_path = &node_modules_path;
bin_linker.target_package_name = package_name;
if (PackageManager.verbose_install) {
Output.prettyErrorln("<d>[Bin Linker]<r> {s} -> {s} retrying without native bin link", .{
package_name.slice(),
target_package_name.slice(),
});
}
bin_linker.link(false);
}
| pkg_resolutions_buffer: []const PackageID, | ||
| pkg_metas: []const Package.Meta, | ||
| pkg_id: PackageID, | ||
| target_node_modules_path: *?bun.AbsPath(.{}), |
There was a problem hiding this comment.
| target_node_modules_path: *?bun.AbsPath(.{}), |
it doesn't make sense to have this and the return value optional. in my opinion this function should return the ?Entry.Id and if non-null callers would use it to assign target_node_modules_path and create the package name string
| ) orelse strings.StringOrTinyString.init(dep_name); | ||
| var can_retry_without_native_binlink_optimization = target_node_modules_path != null; | ||
|
|
||
| while (true) { |
There was a problem hiding this comment.
same thing here: while(true) should be deleted
There was a problem hiding this comment.
and can_retry_without_native_binlink_optimization
| const default_native_binlinks_name_hashes = &[_]PackageNameHash{ | ||
| bun.Semver.String.Builder.stringHash("esbuild"), | ||
| }; | ||
|
|
||
| const default_ignore_name_hashes = &[_]PackageNameHash{ | ||
| bun.Semver.String.Builder.stringHash("sharp"), | ||
| }; |
There was a problem hiding this comment.
maybe we should add a bun pm command to list these like we do for the default trusted list
### What does this PR do? Fixes a regression introduced in Bun v1.3.2 with #24283. We are not able to skip `sharp` lifecycle scripts before v0.33.0 because previous versions did not use optional dependencies with prebuilds. Fixes #24550 Fixes ENG-21519 ### How did you verify your code works? Manually --------- Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com>
Summary
This PR introduces a new postinstall optimization system that significantly reduces the need to run lifecycle scripts for certain packages by intelligently handling their requirements at install time.
Key Features
1. Native Binlink Optimization
When packages like
esbuildship platform-specific binaries as optional dependencies, we now:esbuild)@esbuild/darwin-arm64)Result: No postinstall scripts needed for esbuild and similar packages.
2. Lifecycle Script Skipping
For packages like
sharpthat run heavy postinstall scripts:sharp)Configuration
Both features can be configured via
package.json:{ "nativeDependencies": ["esbuild", "my-custom-package"], "ignoreScripts": ["sharp", "another-package"] }Set to empty arrays to disable defaults:
{ "nativeDependencies": [], "ignoreScripts": [] }Environment variable overrides:
BUN_FEATURE_FLAG_DISABLE_NATIVE_DEPENDENCY_LINKER=1- disable native binlinkBUN_FEATURE_FLAG_DISABLE_IGNORE_SCRIPTS=1- disable script ignoringImplementation Details
Core Components
postinstall_optimizer.zig: New file containing the optimizer logicPostinstallOptimizerenum withnative_binlinkandignorevariantsListtype to track optimization strategies per package hashesbuild(native binlink) andsharp(ignore)Bin.Linkerchanges: Extended to support separate target pathstarget_node_modules_path: Where to find the actual binarytarget_package_name: Name of the package containing the binaryModified Components
PackageInstaller.zig: Checks optimizer before:
isolated_install/Installer.zig: Similar checks for isolated linker mode
maybeReplaceNodeModulesPath()resolves platform-specific packagesLockfile: Added
postinstall_optimizerfield to persist configurationChanges Included
esbuildfrom 0.21.5 to 0.25.11 (testing with latest)env_var.zigTest Plan
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Tests
Documentation