Skip to content

fix: prevent segfault in auto-install due to log allocator mismatch#26031

Open
brkalow wants to merge 5 commits into
oven-sh:mainfrom
brkalow:fix/auto-install-allocator-crash
Open

fix: prevent segfault in auto-install due to log allocator mismatch#26031
brkalow wants to merge 5 commits into
oven-sh:mainfrom
brkalow:fix/auto-install-allocator-crash

Conversation

@brkalow

@brkalow brkalow commented Jan 13, 2026

Copy link
Copy Markdown
Contributor

I'm not entirely sure if this fix is correct, but it did address the observed segfault. No hard feelings if this is the wrong approach. 🙏

What does this PR do?

Fixes a segfault that occurs when auto-install runs during transpilation (e.g., running a script with no
node_modules directory).

The crash happened because the PackageManager's log was swapped to a temporary log with an allocator that could
become invalid. When runTasks attempted to log HTTP errors via manager.log.addErrorFmt(), the invalid allocator
caused a segfault.

Fix: Don't swap the PM's log during transpilation. PM errors are properly propagated via callbacks
(onPackageManifestError, onDependencyError) rather than through the log, so this has no user-facing impact.

How did you verify your code works?

  • Added regression tests in test/cli/run/run-autoinstall.test.ts
  • Tests verify no crash signals (SIGABRT/SIGSEGV) when auto-install encounters missing packages
Stack trace Segmentation fault at address 0x119C311C
  • 1 unknown/js code
  • Allocator.zig:141: array_list.AlignedManaged
  • array_list.zig:444: logger.Log.addErrorFmt
  • runTasks.zig:247: install.PackageManager.runTasks.runTasks
  • PackageManagerEnqueue.zig:342: resolver.resolver.Resolver.enqueueDependencyToResolve
  • resolver.zig:2007: resolver.resolver.Resolver.loadNodeModules
  • resolver.zig:2527: resolver.resolver.Resolver.resolveWithoutRemapping
  • resolver.zig:1483: resolver.resolver.Resolver.checkPackagePath
  • resolver.zig:1297: resolver.resolver.Resolver.resolveWithoutSymlinks
  • resolver.zig:856: resolver.resolver.Resolver.resolveAndAutoInstall
  • VirtualMachine.zig:1660: bun.js.VirtualMachine.resolveMaybeNeedsTrailingSlash
  • BunObject.zig:825: bun.js.api.BunObject.doResolveWithArgs
  • host_fn.zig:96: Bun__resolveSync
  • ImportMetaObject.cpp:410: functionImportMeta__resolveSyncPrivate
  • 1 unknown/js code
  • jsc_llint_commonCallOp__llintOpWithMetadata__llintOpWithReturn__llintOp__commonOp__fn__fn__makeReturn__fn__fn__fn__666_callHelper__dispatch_LowLevelInterpreter64_asm_2543
  • 1 unknown/js code
  • jsc_llint_commonCallOp__llintOpWithMetadata__llintOpWithReturn__llintOp__commonOp__fn__fn__makeReturn__fn__fn__fn__666_callHelper__dispatch_LowLevelInterpreter64_asm_2543
  • jsc_llint_commonCallOp__llintOpWithMetadata__llintOpWithReturn__llintOp__commonOp__fn__fn__makeReturn__fn__fn__fn__666_callHelper__dispatch_LowLevelInterpreter64_asm_2543

Features: tsconfig, Bun.stderr, Bun.stdout, bunfig, http_server, jsc, dev_server

When the PackageManager was initialized via `initWithRuntime`, it received
the resolver's log object. This log's internal allocator could become
invalid when the log was temporarily swapped during transpilation
(in ModuleLoader.zig and AsyncModule.zig).

During auto-install, when `runTasks` attempted to log HTTP errors via
`manager.log.addErrorFmt()`, the log's `msgs.allocator` was used to grow
the message array. If this allocator had been freed or was from a
temporary arena, it caused a segmentation fault at address 0x119C311C.

The fix:
1. Create a dedicated log for the PackageManager in `initWithRuntimeOnce`
   that uses `bun.default_allocator` - a stable, long-lived allocator.
2. Remove the log swapping for the PackageManager in ModuleLoader.zig
   and AsyncModule.zig, since the PM now has its own dedicated log.

This ensures the PackageManager always has a valid allocator for error
logging, regardless of what temporary log swaps occur during transpilation.

Fixes: oven-sh#20084
Related: oven-sh#25544

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
@coderabbitai

coderabbitai Bot commented Jan 13, 2026

Copy link
Copy Markdown
Contributor

Walkthrough

Removed unsafe swaps of the package manager log during transpilation in AsyncModule and ModuleLoader, replacing them with explanatory comments and propagating PM errors via callbacks. Added two regression tests ensuring auto-install resolution errors do not cause segmentation faults.

Changes

Cohort / File(s) Summary
PackageManager log swap removal
src/bun.js/AsyncModule.zig, src/bun.js/ModuleLoader.zig
Removed code that temporarily assigned packageManager().log during transpilation and the corresponding defer restorations; replaced with comments explaining allocator-lifetime unsafety and that PM errors are surfaced via callbacks.
Auto-install regression tests
test/cli/run/run-autoinstall.test.ts
Added two tests verifying auto-install does not segfault on resolution errors: one JS case importing a missing package, and one TS case that triggers transpilation and resolution of a missing package.

Possibly related PRs

Suggested reviewers

  • nektro
  • pfgithub
🚥 Pre-merge checks | ✅ 2
✅ Passed checks (2 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and specifically describes the main change: preventing a segfault in auto-install caused by a log allocator mismatch, which aligns with the core fix across multiple files.
Description check ✅ Passed The PR description fully addresses both required template sections with clear explanations of the fix, root cause analysis, and verification approach through regression tests.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.


Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Fix all issues with AI agents
In @test/cli/run/run-autoinstall.test.ts:
- Around line 127-129: Remove the forbidden assertion that checks for the
literal "panic": locate the expect(stderrText).not.toContain("panic") assertion
in run-autoinstall.test.ts (the block using stderrText and
expect(...).not.toContain for crash strings) and delete that line in both
occurrences (the two test cases around the existing checks for "Segmentation
fault" and "Bun has crashed"); leave the other assertions and existing exit-code
checks intact.
📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Disabled knowledge base sources:

  • Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between 3196178 and 1df0010.

📒 Files selected for processing (3)
  • src/bun.js/AsyncModule.zig
  • src/bun.js/ModuleLoader.zig
  • test/cli/run/run-autoinstall.test.ts
🧰 Additional context used
📓 Path-based instructions (5)
**/*.zig

📄 CodeRabbit inference engine (CLAUDE.md)

In Zig code, be careful with allocators and use defer for cleanup

Files:

  • src/bun.js/AsyncModule.zig
  • src/bun.js/ModuleLoader.zig
src/**/*.zig

📄 CodeRabbit inference engine (src/CLAUDE.md)

src/**/*.zig: Use the # prefix for private fields in Zig structs, e.g., struct { #foo: u32 };
Use Decl literals in Zig, e.g., const decl: Decl = .{ .binding = 0, .value = 0 };
Place @import statements at the bottom of the file in Zig (auto formatter will handle positioning)
Never use @import() inline inside functions in Zig; always place imports at the bottom of the file or containing struct

Files:

  • src/bun.js/AsyncModule.zig
  • src/bun.js/ModuleLoader.zig
**/*.test.ts?(x)

📄 CodeRabbit inference engine (CLAUDE.md)

**/*.test.ts?(x): Never use bun test directly - always use bun bd test to run tests with debug build changes
For single-file tests, prefer -e flag over tempDir
For multi-file tests, prefer tempDir and Bun.spawn over single-file tests
Use normalizeBunSnapshot to normalize snapshot output of tests
Never write tests that check for 'panic', 'uncaught exception', or similar strings in test output
Use tempDir from harness to create temporary directories - do not use tmpdirSync or fs.mkdtempSync
When spawning processes in tests, expect stdout before expecting exit code for more useful error messages on test failure
Do not write flaky tests - do not use setTimeout in tests; instead await the condition to be met
Verify tests fail with USE_SYSTEM_BUN=1 bun test <file> and pass with bun bd test <file> - tests are invalid if they pass with USE_SYSTEM_BUN=1
Test files must end with .test.ts or .test.tsx
Avoid shell commands like find or grep in tests - use Bun's Glob and built-in tools instead

Files:

  • test/cli/run/run-autoinstall.test.ts
test/**/*.test.ts?(x)

📄 CodeRabbit inference engine (CLAUDE.md)

Always use port: 0 in tests - do not hardcode ports or use custom random port number functions

Files:

  • test/cli/run/run-autoinstall.test.ts
test/**/*.test.{ts,js,jsx,tsx,mjs,cjs}

📄 CodeRabbit inference engine (test/CLAUDE.md)

test/**/*.test.{ts,js,jsx,tsx,mjs,cjs}: Use bun bd test <...test file> to run tests with compiled code changes. Do not use bun test as it will not include your changes.
Use bun:test for files ending in *.test.{ts,js,jsx,tsx,mjs,cjs}. For test files without .test extension in test/js/node/test/{parallel,sequential}/*.js, use bun bd <file> instead of bun bd test <file> since they expect exit code 0.
Do not set a timeout on tests. Bun already has timeouts built-in.

Files:

  • test/cli/run/run-autoinstall.test.ts
🧠 Learnings (15)
📓 Common learnings
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: src/js/CLAUDE.md:0-0
Timestamp: 2025-11-24T18:37:11.466Z
Learning: Applies to src/js/{builtins,node,bun,thirdparty,internal}/**/*.{ts,js} : Use `$debug()` for debug logging and `$assert()` for assertions; both are stripped in release builds
📚 Learning: 2025-11-24T18:37:11.466Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: src/js/CLAUDE.md:0-0
Timestamp: 2025-11-24T18:37:11.466Z
Learning: Applies to src/js/{builtins,node,bun,thirdparty,internal}/**/*.{ts,js} : Use `$debug()` for debug logging and `$assert()` for assertions; both are stripped in release builds

Applied to files:

  • src/bun.js/AsyncModule.zig
  • src/bun.js/ModuleLoader.zig
  • test/cli/run/run-autoinstall.test.ts
📚 Learning: 2026-01-05T16:32:07.551Z
Learnt from: alii
Repo: oven-sh/bun PR: 25474
File: src/bun.js/event_loop/Sigusr1Handler.zig:0-0
Timestamp: 2026-01-05T16:32:07.551Z
Learning: In Zig codebases (e.g., Bun), treat std.posix.sigaction as returning void and do not perform runtime error handling for its failure. The Zig standard library views sigaction failures as programmer errors (unreachable) because they only occur with invalid signals like SIGKILL/SIGSTOP. Apply this pattern across Zig files that call sigaction (e.g., crash_handler.zig, main.zig, filter_run.zig, process.zig) and ensure failures are not handled as recoverable errors; prefer reaching an explicit unreachable/compile-time assumption when such failures are detected.

Applied to files:

  • src/bun.js/AsyncModule.zig
  • src/bun.js/ModuleLoader.zig
📚 Learning: 2025-12-16T00:21:32.179Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-16T00:21:32.179Z
Learning: Applies to test/regression/issue/*.test.ts : Place regression tests for specific GitHub issues in `test/regression/issue/${issueNumber}.test.ts` with real issue numbers only

Applied to files:

  • test/cli/run/run-autoinstall.test.ts
📚 Learning: 2025-12-16T00:21:32.179Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-16T00:21:32.179Z
Learning: Applies to **/*.test.ts?(x) : Never write tests that check for 'panic', 'uncaught exception', or similar strings in test output

Applied to files:

  • test/cli/run/run-autoinstall.test.ts
📚 Learning: 2025-12-16T00:21:32.179Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-16T00:21:32.179Z
Learning: Applies to **/*.test.ts?(x) : Never use `bun test` directly - always use `bun bd test` to run tests with debug build changes

Applied to files:

  • test/cli/run/run-autoinstall.test.ts
📚 Learning: 2025-12-16T00:21:32.179Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-16T00:21:32.179Z
Learning: Applies to **/*.test.ts?(x) : When spawning processes in tests, expect stdout before expecting exit code for more useful error messages on test failure

Applied to files:

  • test/cli/run/run-autoinstall.test.ts
📚 Learning: 2025-12-16T00:21:32.179Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-16T00:21:32.179Z
Learning: Applies to **/*.test.ts?(x) : Do not write flaky tests - do not use `setTimeout` in tests; instead await the condition to be met

Applied to files:

  • test/cli/run/run-autoinstall.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/cli/run/run-autoinstall.test.ts
📚 Learning: 2026-01-05T23:04:01.518Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: test/CLAUDE.md:0-0
Timestamp: 2026-01-05T23:04:01.518Z
Learning: Applies to test/**/*.test.{ts,js,jsx,tsx,mjs,cjs} : Use `bun bd test <...test file>` to run tests with compiled code changes. Do not use `bun test` as it will not include your changes.

Applied to files:

  • test/cli/run/run-autoinstall.test.ts
📚 Learning: 2026-01-05T23:04:01.518Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: test/CLAUDE.md:0-0
Timestamp: 2026-01-05T23:04:01.518Z
Learning: Organize regression tests for specific issues in `/test/regression/issue/${issueNumber}.test.ts`. Do not place regression tests in the regression directory if there is no associated issue number.

Applied to files:

  • test/cli/run/run-autoinstall.test.ts
📚 Learning: 2025-11-24T18:36:59.706Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: src/bun.js/bindings/v8/CLAUDE.md:0-0
Timestamp: 2025-11-24T18:36:59.706Z
Learning: Applies to src/bun.js/bindings/v8/test/v8/v8.test.ts : Add corresponding test cases to test/v8/v8.test.ts using checkSameOutput() function to compare Node.js and Bun output

Applied to files:

  • test/cli/run/run-autoinstall.test.ts
📚 Learning: 2025-12-16T00:21:32.179Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-16T00:21:32.179Z
Learning: Applies to **/*.test.ts?(x) : Verify tests fail with `USE_SYSTEM_BUN=1 bun test <file>` and pass with `bun bd test <file>` - tests are invalid if they pass with USE_SYSTEM_BUN=1

Applied to files:

  • test/cli/run/run-autoinstall.test.ts
📚 Learning: 2025-12-16T00:21:32.179Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-16T00:21:32.179Z
Learning: Applies to **/*.test.ts?(x) : For multi-file tests, prefer `tempDir` and `Bun.spawn` over single-file tests

Applied to files:

  • test/cli/run/run-autoinstall.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/cli/run/run-autoinstall.test.ts
🧬 Code graph analysis (1)
test/cli/run/run-autoinstall.test.ts (1)
test/harness.ts (1)
  • tmpdirSync (1221-1223)
🔇 Additional comments (4)
src/bun.js/ModuleLoader.zig (1)

212-218: LGTM! Clear documentation of the allocator safety design decision.

The comment effectively explains both the problem (allocator lifetime during transpilation) and the solution (PM errors propagate via callbacks like onPackageManifestError, onDependencyError). This will help future maintainers understand why the PM log is intentionally not swapped here.

src/bun.js/AsyncModule.zig (1)

665-669: LGTM! Consistent documentation with ModuleLoader.zig.

The comment maintains consistency with the parallel change in ModuleLoader.zig, ensuring both code paths that handle transpilation document the same allocator safety concern.

test/cli/run/run-autoinstall.test.ts (2)

91-133: Good regression test coverage.

The test correctly exercises the auto-install code path by:

  1. Creating a project without node_modules to trigger auto-install
  2. Importing a non-existent package to trigger the error path
  3. Verifying no crash signals and proper error messaging

The exit code checks for SIGABRT (134) and SIGSEGV (139) are appropriate for detecting the segfault that was fixed.


135-188: Good TypeScript transpilation regression coverage.

This test correctly exercises the transpilation + resolution path that was specifically mentioned in the PR as the crash trigger scenario. Including tsconfig.json ensures the TypeScript handling is properly engaged.

Comment thread test/cli/run/run-autoinstall.test.ts Outdated
Comment thread test/cli/run/run-autoinstall.test.ts Outdated

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🤖 Fix all issues with AI agents
In @test/cli/run/run-autoinstall.test.ts:
- Around line 181-183: Remove the check that asserts stderrText does not contain
"panic" in the test run-autoinstall.test.ts; specifically delete or comment out
the line referencing expect(stderrText).not.toContain("panic") (which uses the
stderrText variable alongside the other checks for "Segmentation fault" and "Bun
has crashed") so the test only verifies the remaining crash strings and exit
code as per guidelines.
- Around line 92-132: Move the assertions that inspect stderrText before the
assertions that check exitCode so test failures show the captured error output
first; specifically, in the "should handle missing package gracefully without
segfault" test reorder the expectations so the checks using stderrText
(expect(stderrText).not.toContain(...), expect(stderrText).toContain(...)) run
before the exitCode checks (expect(exitCode).not.toBe(134),
expect(exitCode).not.toBe(139)), keeping the same assertions and variable names
(stderrText, exitCode) and preserving the final semantic that the test ensures
no segfault and that the missing-package message is present.
📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Disabled knowledge base sources:

  • Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between 1df0010 and 5b05fdf.

📒 Files selected for processing (1)
  • test/cli/run/run-autoinstall.test.ts
🧰 Additional context used
📓 Path-based instructions (3)
**/*.test.ts?(x)

📄 CodeRabbit inference engine (CLAUDE.md)

**/*.test.ts?(x): Never use bun test directly - always use bun bd test to run tests with debug build changes
For single-file tests, prefer -e flag over tempDir
For multi-file tests, prefer tempDir and Bun.spawn over single-file tests
Use normalizeBunSnapshot to normalize snapshot output of tests
Never write tests that check for 'panic', 'uncaught exception', or similar strings in test output
Use tempDir from harness to create temporary directories - do not use tmpdirSync or fs.mkdtempSync
When spawning processes in tests, expect stdout before expecting exit code for more useful error messages on test failure
Do not write flaky tests - do not use setTimeout in tests; instead await the condition to be met
Verify tests fail with USE_SYSTEM_BUN=1 bun test <file> and pass with bun bd test <file> - tests are invalid if they pass with USE_SYSTEM_BUN=1
Test files must end with .test.ts or .test.tsx
Avoid shell commands like find or grep in tests - use Bun's Glob and built-in tools instead

Files:

  • test/cli/run/run-autoinstall.test.ts
test/**/*.test.ts?(x)

📄 CodeRabbit inference engine (CLAUDE.md)

Always use port: 0 in tests - do not hardcode ports or use custom random port number functions

Files:

  • test/cli/run/run-autoinstall.test.ts
test/**/*.test.{ts,js,jsx,tsx,mjs,cjs}

📄 CodeRabbit inference engine (test/CLAUDE.md)

test/**/*.test.{ts,js,jsx,tsx,mjs,cjs}: Use bun bd test <...test file> to run tests with compiled code changes. Do not use bun test as it will not include your changes.
Use bun:test for files ending in *.test.{ts,js,jsx,tsx,mjs,cjs}. For test files without .test extension in test/js/node/test/{parallel,sequential}/*.js, use bun bd <file> instead of bun bd test <file> since they expect exit code 0.
Do not set a timeout on tests. Bun already has timeouts built-in.

Files:

  • test/cli/run/run-autoinstall.test.ts
🧠 Learnings (20)
📓 Common learnings
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: src/js/CLAUDE.md:0-0
Timestamp: 2025-11-24T18:37:11.466Z
Learning: Applies to src/js/{builtins,node,bun,thirdparty,internal}/**/*.{ts,js} : Use `$debug()` for debug logging and `$assert()` for assertions; both are stripped in release builds
📚 Learning: 2025-12-16T00:21:32.179Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-16T00:21:32.179Z
Learning: Applies to test/regression/issue/*.test.ts : Place regression tests for specific GitHub issues in `test/regression/issue/${issueNumber}.test.ts` with real issue numbers only

Applied to files:

  • test/cli/run/run-autoinstall.test.ts
📚 Learning: 2025-12-16T00:21:32.179Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-16T00:21:32.179Z
Learning: Applies to **/*.test.ts?(x) : Never write tests that check for 'panic', 'uncaught exception', or similar strings in test output

Applied to files:

  • test/cli/run/run-autoinstall.test.ts
📚 Learning: 2025-12-16T00:21:32.179Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-16T00:21:32.179Z
Learning: Applies to **/*.test.ts?(x) : Never use `bun test` directly - always use `bun bd test` to run tests with debug build changes

Applied to files:

  • test/cli/run/run-autoinstall.test.ts
📚 Learning: 2025-12-16T00:21:32.179Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-16T00:21:32.179Z
Learning: Applies to **/*.test.ts?(x) : Verify tests fail with `USE_SYSTEM_BUN=1 bun test <file>` and pass with `bun bd test <file>` - tests are invalid if they pass with USE_SYSTEM_BUN=1

Applied to files:

  • test/cli/run/run-autoinstall.test.ts
📚 Learning: 2025-12-16T00:21:32.179Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-16T00:21:32.179Z
Learning: Applies to **/*.test.ts?(x) : When spawning processes in tests, expect stdout before expecting exit code for more useful error messages on test failure

Applied to files:

  • test/cli/run/run-autoinstall.test.ts
📚 Learning: 2025-12-16T00:21:32.179Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-16T00:21:32.179Z
Learning: Applies to **/*.test.ts?(x) : Do not write flaky tests - do not use `setTimeout` in tests; instead await the condition to be met

Applied to files:

  • test/cli/run/run-autoinstall.test.ts
📚 Learning: 2026-01-05T23:04:01.518Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: test/CLAUDE.md:0-0
Timestamp: 2026-01-05T23:04:01.518Z
Learning: Organize regression tests for specific issues in `/test/regression/issue/${issueNumber}.test.ts`. Do not place regression tests in the regression directory if there is no associated issue number.

Applied to files:

  • test/cli/run/run-autoinstall.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/cli/run/run-autoinstall.test.ts
📚 Learning: 2025-12-16T00:21:32.179Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-16T00:21:32.179Z
Learning: Applies to test/**/*.test.ts?(x) : Always use `port: 0` in tests - do not hardcode ports or use custom random port number functions

Applied to files:

  • test/cli/run/run-autoinstall.test.ts
📚 Learning: 2025-12-16T00:21:32.179Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-16T00:21:32.179Z
Learning: Applies to **/*.test.ts?(x) : For multi-file tests, prefer `tempDir` and `Bun.spawn` over single-file tests

Applied to files:

  • test/cli/run/run-autoinstall.test.ts
📚 Learning: 2025-11-24T18:36:59.706Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: src/bun.js/bindings/v8/CLAUDE.md:0-0
Timestamp: 2025-11-24T18:36:59.706Z
Learning: Applies to src/bun.js/bindings/v8/test/v8/v8.test.ts : Add corresponding test cases to test/v8/v8.test.ts using checkSameOutput() function to compare Node.js and Bun output

Applied to files:

  • test/cli/run/run-autoinstall.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/run/run-autoinstall.test.ts
📚 Learning: 2025-09-20T00:58:38.042Z
Learnt from: markovejnovic
Repo: oven-sh/bun PR: 22568
File: test/js/valkey/valkey.test.ts:561-564
Timestamp: 2025-09-20T00:58:38.042Z
Learning: For test/js/valkey/valkey.test.ts, do not comment on synchronous throw assertions for async Redis methods (like ctx.redis.set(), ctx.redis.unsubscribe(), etc.) - Bun's Redis client implementation differs from Node.js and can throw synchronously even for async methods. The maintainer has explicitly requested to stop looking at this error pattern.

Applied to files:

  • test/cli/run/run-autoinstall.test.ts
📚 Learning: 2026-01-05T23:04:01.518Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: test/CLAUDE.md:0-0
Timestamp: 2026-01-05T23:04:01.518Z
Learning: Applies to test/**/*.test.{ts,js,jsx,tsx,mjs,cjs} : Use `bun:test` for files ending in `*.test.{ts,js,jsx,tsx,mjs,cjs}`. For test files without .test extension in test/js/node/test/{parallel,sequential}/*.js, use `bun bd <file>` instead of `bun bd test <file>` since they expect exit code 0.

Applied to files:

  • test/cli/run/run-autoinstall.test.ts
📚 Learning: 2025-12-16T00:21:32.179Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-16T00:21:32.179Z
Learning: Applies to **/*.test.ts?(x) : Avoid shell commands like `find` or `grep` in tests - use Bun's Glob and built-in tools instead

Applied to files:

  • test/cli/run/run-autoinstall.test.ts
📚 Learning: 2025-11-08T04:06:33.198Z
Learnt from: Jarred-Sumner
Repo: oven-sh/bun PR: 24491
File: test/js/bun/transpiler/declare-global.test.ts:17-17
Timestamp: 2025-11-08T04:06:33.198Z
Learning: In Bun test files, `await using` with Bun.spawn() is the preferred pattern for spawned processes regardless of whether they are short-lived or long-running. Do not suggest replacing `await using proc = Bun.spawn(...)` with `const proc = Bun.spawn(...); await proc.exited;`.

Applied to files:

  • test/cli/run/run-autoinstall.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/cli/run/run-autoinstall.test.ts
📚 Learning: 2025-11-24T18:37:11.466Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: src/js/CLAUDE.md:0-0
Timestamp: 2025-11-24T18:37:11.466Z
Learning: Applies to src/js/{builtins,node,bun,thirdparty,internal}/**/*.{ts,js} : Use `$debug()` for debug logging and `$assert()` for assertions; both are stripped in release builds

Applied to files:

  • test/cli/run/run-autoinstall.test.ts
📚 Learning: 2025-10-18T23:43:42.502Z
Learnt from: Jarred-Sumner
Repo: oven-sh/bun PR: 23817
File: src/js/node/test.ts:282-282
Timestamp: 2025-10-18T23:43:42.502Z
Learning: In the Bun repository, the error code generation script (generate-node-errors.ts) always runs during the build process. When reviewing code that uses error code intrinsics like $ERR_TEST_FAILURE, $ERR_INVALID_ARG_TYPE, etc., do not ask to verify whether the generation script has been run or will run, as it is automatically executed.

Applied to files:

  • test/cli/run/run-autoinstall.test.ts
🧬 Code graph analysis (1)
test/cli/run/run-autoinstall.test.ts (2)
test/harness.ts (1)
  • tmpdirSync (1221-1223)
test/js/node/test_runner/fixtures/02-hooks.js (1)
  • JSON (7-7)
🔇 Additional comments (2)
test/cli/run/run-autoinstall.test.ts (2)

88-91: LGTM!

Good documentation explaining the regression scenario. The describe block name and comments clearly indicate the purpose of these tests.


134-187: Good coverage of the transpilation code path.

This test correctly exercises the scenario where the segfault occurred during transpilation. Same suggestion applies here about reordering stderr assertions before exit code checks for better debugging on failure.

Comment on lines +92 to +132
test("should handle missing package gracefully without segfault", async () => {
const dir = tmpdirSync();
mkdirSync(dir, { recursive: true });

// Create a project that imports a non-existent package
await Promise.all([
Bun.write(
join(dir, "index.js"),
"import nonExistentPackage from 'this-package-does-not-exist-12345'; console.log(nonExistentPackage);",
),
Bun.write(
join(dir, "package.json"),
JSON.stringify({
name: "test-autoinstall-crash",
type: "module",
}),
),
]);

// Run without node_modules to trigger auto-install code path
const { exitCode, stderr } = Bun.spawnSync({
cmd: [bunExe(), join(dir, "index.js")],
cwd: dir,
env: bunEnv,
stdout: "pipe",
stderr: "pipe",
});

const stderrText = stderr?.toString("utf8") ?? "";

// Should NOT crash with segfault (exit codes 128+N indicate death by signal N)
expect(exitCode).not.toBe(134); // SIGABRT (128 + 6)
expect(exitCode).not.toBe(139); // SIGSEGV (128 + 11)

// Should NOT contain crash indicators
expect(stderrText).not.toContain("Segmentation fault");
expect(stderrText).not.toContain("Bun has crashed");

// Should contain a normal error message about the missing package
expect(stderrText).toContain("this-package-does-not-exist-12345");
});

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick | 🔵 Trivial

LGTM! Consider reordering assertions for better debugging.

The test correctly validates the segfault regression. Per coding guidelines, checking stderr content before exit code provides more useful error messages on test failure. Consider moving stderr assertions before exit code assertions.

♻️ Suggested reordering
     const stderrText = stderr?.toString("utf8") ?? "";

-    // Should NOT crash with segfault (exit codes 128+N indicate death by signal N)
-    expect(exitCode).not.toBe(134); // SIGABRT (128 + 6)
-    expect(exitCode).not.toBe(139); // SIGSEGV (128 + 11)
-
     // Should NOT contain crash indicators
     expect(stderrText).not.toContain("Segmentation fault");
     expect(stderrText).not.toContain("Bun has crashed");

     // Should contain a normal error message about the missing package
     expect(stderrText).toContain("this-package-does-not-exist-12345");
+
+    // Should NOT crash with segfault (exit codes 128+N indicate death by signal N)
+    expect(exitCode).not.toBe(134); // SIGABRT (128 + 6)
+    expect(exitCode).not.toBe(139); // SIGSEGV (128 + 11)
🤖 Prompt for AI Agents
In @test/cli/run/run-autoinstall.test.ts around lines 92 - 132, Move the
assertions that inspect stderrText before the assertions that check exitCode so
test failures show the captured error output first; specifically, in the "should
handle missing package gracefully without segfault" test reorder the
expectations so the checks using stderrText
(expect(stderrText).not.toContain(...), expect(stderrText).toContain(...)) run
before the exitCode checks (expect(exitCode).not.toBe(134),
expect(exitCode).not.toBe(139)), keeping the same assertions and variable names
(stderrText, exitCode) and preserving the final semantic that the test ensures
no segfault and that the missing-package message is present.

Comment thread test/cli/run/run-autoinstall.test.ts Outdated
Comment thread test/cli/run/run-autoinstall.test.ts Outdated
Comment thread test/cli/run/run-autoinstall.test.ts Outdated
Comment thread test/cli/run/run-autoinstall.test.ts Outdated

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🤖 Fix all issues with AI agents
In @test/cli/run/run-autoinstall.test.ts:
- Around line 120-131: Reorder the assertions so the stderr content checks run
before the exit code checks: move the lines that build stderrText and the
expect(...).not.toContain / expect(...).toContain assertions (the checks using
stderrText for "Segmentation fault", "Bun has crashed", and
"this-package-does-not-exist-12345") to appear prior to the
expect(exitCode).not.toBe(...) assertions (the SIGABRT/SIGSEGV checks). Keep the
same variables (stderrText, exitCode) and messages unchanged so failures show
stderr diagnostics first.
- Around line 175-185: Reorder the assertions so the stderr content checks run
before the exit-code checks: first assert stderrText contains
"another-nonexistent-pkg-67890" and does not contain "Segmentation fault" or
"Bun has crashed" (using the stderrText variable), then assert exitCode is not
134 or 139 (using exitCode); update the block around stderrText and exitCode in
run-autoinstall.test.ts to reflect this new order for better failure
diagnostics.
📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Disabled knowledge base sources:

  • Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between 5b05fdf and 24299ae.

📒 Files selected for processing (1)
  • test/cli/run/run-autoinstall.test.ts
🧰 Additional context used
📓 Path-based instructions (3)
**/*.test.ts?(x)

📄 CodeRabbit inference engine (CLAUDE.md)

**/*.test.ts?(x): Never use bun test directly - always use bun bd test to run tests with debug build changes
For single-file tests, prefer -e flag over tempDir
For multi-file tests, prefer tempDir and Bun.spawn over single-file tests
Use normalizeBunSnapshot to normalize snapshot output of tests
Never write tests that check for 'panic', 'uncaught exception', or similar strings in test output
Use tempDir from harness to create temporary directories - do not use tmpdirSync or fs.mkdtempSync
When spawning processes in tests, expect stdout before expecting exit code for more useful error messages on test failure
Do not write flaky tests - do not use setTimeout in tests; instead await the condition to be met
Verify tests fail with USE_SYSTEM_BUN=1 bun test <file> and pass with bun bd test <file> - tests are invalid if they pass with USE_SYSTEM_BUN=1
Test files must end with .test.ts or .test.tsx
Avoid shell commands like find or grep in tests - use Bun's Glob and built-in tools instead

Files:

  • test/cli/run/run-autoinstall.test.ts
test/**/*.test.ts?(x)

📄 CodeRabbit inference engine (CLAUDE.md)

Always use port: 0 in tests - do not hardcode ports or use custom random port number functions

Files:

  • test/cli/run/run-autoinstall.test.ts
test/**/*.test.{ts,js,jsx,tsx,mjs,cjs}

📄 CodeRabbit inference engine (test/CLAUDE.md)

test/**/*.test.{ts,js,jsx,tsx,mjs,cjs}: Use bun bd test <...test file> to run tests with compiled code changes. Do not use bun test as it will not include your changes.
Use bun:test for files ending in *.test.{ts,js,jsx,tsx,mjs,cjs}. For test files without .test extension in test/js/node/test/{parallel,sequential}/*.js, use bun bd <file> instead of bun bd test <file> since they expect exit code 0.
Do not set a timeout on tests. Bun already has timeouts built-in.

Files:

  • test/cli/run/run-autoinstall.test.ts
🧠 Learnings (23)
📚 Learning: 2025-12-16T00:21:32.179Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-16T00:21:32.179Z
Learning: Applies to test/regression/issue/*.test.ts : Place regression tests for specific GitHub issues in `test/regression/issue/${issueNumber}.test.ts` with real issue numbers only

Applied to files:

  • test/cli/run/run-autoinstall.test.ts
📚 Learning: 2025-12-16T00:21:32.179Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-16T00:21:32.179Z
Learning: Applies to **/*.test.ts?(x) : Never write tests that check for 'panic', 'uncaught exception', or similar strings in test output

Applied to files:

  • test/cli/run/run-autoinstall.test.ts
📚 Learning: 2025-12-16T00:21:32.179Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-16T00:21:32.179Z
Learning: Applies to **/*.test.ts?(x) : When spawning processes in tests, expect stdout before expecting exit code for more useful error messages on test failure

Applied to files:

  • test/cli/run/run-autoinstall.test.ts
📚 Learning: 2026-01-05T23:04:01.518Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: test/CLAUDE.md:0-0
Timestamp: 2026-01-05T23:04:01.518Z
Learning: Organize regression tests for specific issues in `/test/regression/issue/${issueNumber}.test.ts`. Do not place regression tests in the regression directory if there is no associated issue number.

Applied to files:

  • test/cli/run/run-autoinstall.test.ts
📚 Learning: 2025-12-16T00:21:32.179Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-16T00:21:32.179Z
Learning: Applies to **/*.test.ts?(x) : Do not write flaky tests - do not use `setTimeout` in tests; instead await the condition to be met

Applied to files:

  • test/cli/run/run-autoinstall.test.ts
📚 Learning: 2025-12-16T00:21:32.179Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-16T00:21:32.179Z
Learning: Applies to **/*.test.ts?(x) : Never use `bun test` directly - always use `bun bd test` to run tests with debug build changes

Applied to files:

  • test/cli/run/run-autoinstall.test.ts
📚 Learning: 2025-10-18T23:43:42.502Z
Learnt from: Jarred-Sumner
Repo: oven-sh/bun PR: 23817
File: src/js/node/test.ts:282-282
Timestamp: 2025-10-18T23:43:42.502Z
Learning: In the Bun repository, the error code generation script (generate-node-errors.ts) always runs during the build process. When reviewing code that uses error code intrinsics like $ERR_TEST_FAILURE, $ERR_INVALID_ARG_TYPE, etc., do not ask to verify whether the generation script has been run or will run, as it is automatically executed.

Applied to files:

  • test/cli/run/run-autoinstall.test.ts
📚 Learning: 2025-12-16T00:21:32.179Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-16T00:21:32.179Z
Learning: Applies to **/*.test.ts?(x) : Verify tests fail with `USE_SYSTEM_BUN=1 bun test <file>` and pass with `bun bd test <file>` - tests are invalid if they pass with USE_SYSTEM_BUN=1

Applied to files:

  • test/cli/run/run-autoinstall.test.ts
📚 Learning: 2025-12-16T00:21:32.179Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-16T00:21:32.179Z
Learning: Applies to test/**/*.test.ts?(x) : Always use `port: 0` in tests - do not hardcode ports or use custom random port number functions

Applied to files:

  • test/cli/run/run-autoinstall.test.ts
📚 Learning: 2025-11-24T18:36:59.706Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: src/bun.js/bindings/v8/CLAUDE.md:0-0
Timestamp: 2025-11-24T18:36:59.706Z
Learning: Applies to src/bun.js/bindings/v8/test/v8/v8.test.ts : Add corresponding test cases to test/v8/v8.test.ts using checkSameOutput() function to compare Node.js and Bun output

Applied to files:

  • test/cli/run/run-autoinstall.test.ts
📚 Learning: 2026-01-05T23:04:01.518Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: test/CLAUDE.md:0-0
Timestamp: 2026-01-05T23:04:01.518Z
Learning: Applies to test/**/*.test.{ts,js,jsx,tsx,mjs,cjs} : Use `bun:test` for files ending in `*.test.{ts,js,jsx,tsx,mjs,cjs}`. For test files without .test extension in test/js/node/test/{parallel,sequential}/*.js, use `bun bd <file>` instead of `bun bd test <file>` since they expect exit code 0.

Applied to files:

  • test/cli/run/run-autoinstall.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/run/run-autoinstall.test.ts
📚 Learning: 2025-09-20T00:58:38.042Z
Learnt from: markovejnovic
Repo: oven-sh/bun PR: 22568
File: test/js/valkey/valkey.test.ts:561-564
Timestamp: 2025-09-20T00:58:38.042Z
Learning: For test/js/valkey/valkey.test.ts, do not comment on synchronous throw assertions for async Redis methods (like ctx.redis.set(), ctx.redis.unsubscribe(), etc.) - Bun's Redis client implementation differs from Node.js and can throw synchronously even for async methods. The maintainer has explicitly requested to stop looking at this error pattern.

Applied to files:

  • test/cli/run/run-autoinstall.test.ts
📚 Learning: 2025-11-08T04:06:33.198Z
Learnt from: Jarred-Sumner
Repo: oven-sh/bun PR: 24491
File: test/js/bun/transpiler/declare-global.test.ts:17-17
Timestamp: 2025-11-08T04:06:33.198Z
Learning: In Bun test files, `await using` with Bun.spawn() is the preferred pattern for spawned processes regardless of whether they are short-lived or long-running. Do not suggest replacing `await using proc = Bun.spawn(...)` with `const proc = Bun.spawn(...); await proc.exited;`.

Applied to files:

  • test/cli/run/run-autoinstall.test.ts
📚 Learning: 2025-10-18T05:23:24.403Z
Learnt from: theshadow27
Repo: oven-sh/bun PR: 23798
File: test/js/bun/telemetry-server.test.ts:91-100
Timestamp: 2025-10-18T05:23:24.403Z
Learning: In the Bun codebase, telemetry tests (test/js/bun/telemetry-*.test.ts) should focus on telemetry API behavior: configure/disable/isEnabled, callback signatures and invocation, request ID correlation, and error handling. HTTP protocol behaviors like status code normalization (e.g., 200 with empty body → 204) should be tested in HTTP server tests (test/js/bun/http/), not in telemetry tests. Keep separation of concerns: telemetry tests verify the telemetry API contract; HTTP tests verify HTTP semantics.

Applied to files:

  • test/cli/run/run-autoinstall.test.ts
📚 Learning: 2025-09-20T00:57:56.685Z
Learnt from: markovejnovic
Repo: oven-sh/bun PR: 22568
File: test/js/valkey/valkey.test.ts:268-276
Timestamp: 2025-09-20T00:57:56.685Z
Learning: For test/js/valkey/valkey.test.ts, do not comment on synchronous throw assertions for async Redis methods like ctx.redis.set() - the maintainer has explicitly requested to stop looking at this error pattern.

Applied to files:

  • test/cli/run/run-autoinstall.test.ts
📚 Learning: 2026-01-05T23:04:01.518Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: test/CLAUDE.md:0-0
Timestamp: 2026-01-05T23:04:01.518Z
Learning: Always check exit codes and test error scenarios when spawning processes in tests.

Applied to files:

  • test/cli/run/run-autoinstall.test.ts
📚 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:

  • test/cli/run/run-autoinstall.test.ts
📚 Learning: 2025-09-07T08:20:47.215Z
Learnt from: RiskyMH
Repo: oven-sh/bun PR: 22258
File: src/cli/test_command.zig:1258-1281
Timestamp: 2025-09-07T08:20:47.215Z
Learning: For Bun's test line filtering feature, the parseFileLineArg function should only handle the specific cases of "file:line" and "file:line:col" formats. It should not try to be overly tolerant of other patterns, as components like ":col" or other non-numeric segments could legitimately be part of filenames. The current conservative approach that checks for numeric segments in expected positions is appropriate.

Applied to files:

  • test/cli/run/run-autoinstall.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/cli/run/run-autoinstall.test.ts
📚 Learning: 2025-12-16T00:21:32.179Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-16T00:21:32.179Z
Learning: Applies to **/*.test.ts?(x) : For multi-file tests, prefer `tempDir` and `Bun.spawn` over single-file tests

Applied to files:

  • test/cli/run/run-autoinstall.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/cli/run/run-autoinstall.test.ts
📚 Learning: 2025-11-24T18:37:11.466Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: src/js/CLAUDE.md:0-0
Timestamp: 2025-11-24T18:37:11.466Z
Learning: Applies to src/js/{builtins,node,bun,thirdparty,internal}/**/*.{ts,js} : Use `$debug()` for debug logging and `$assert()` for assertions; both are stripped in release builds

Applied to files:

  • test/cli/run/run-autoinstall.test.ts
🧬 Code graph analysis (1)
test/cli/run/run-autoinstall.test.ts (1)
test/harness.ts (1)
  • tmpdirSync (1221-1223)
🔇 Additional comments (1)
test/cli/run/run-autoinstall.test.ts (1)

91-132: Regression tests appropriately cover both code paths.

The two tests effectively target:

  1. JavaScript auto-install path (missing package in .js file)
  2. TypeScript transpilation + auto-install path (missing package in .ts file)

This aligns well with the PR's fix, which addresses the PackageManager log allocator issue during transpilation. The tests verify crash prevention by checking exit codes (134 for SIGABRT, 139 for SIGSEGV) and confirm normal error handling by validating package name appears in stderr.

Comment on lines +120 to +131
const stderrText = stderr?.toString("utf8") ?? "";

// Should NOT crash with segfault (exit codes 128+N indicate death by signal N)
expect(exitCode).not.toBe(134); // SIGABRT (128 + 6)
expect(exitCode).not.toBe(139); // SIGSEGV (128 + 11)

// Should NOT contain crash indicators
expect(stderrText).not.toContain("Segmentation fault");
expect(stderrText).not.toContain("Bun has crashed");

// Should contain a normal error message about the missing package
expect(stderrText).toContain("this-package-does-not-exist-12345");

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick | 🔵 Trivial

Consider reordering assertions: check stderr before exit code for better diagnostics.

Per coding guidelines, when spawning processes in tests, expect output (stdout/stderr) before expecting exit code for more useful error messages on test failure. If these assertions fail, seeing the stderr content first provides more context.

♻️ Suggested reordering
     const stderrText = stderr?.toString("utf8") ?? "";
 
+    // Should contain a normal error message about the missing package
+    expect(stderrText).toContain("this-package-does-not-exist-12345");
+
+    // Should NOT contain crash indicators
+    expect(stderrText).not.toContain("Segmentation fault");
+    expect(stderrText).not.toContain("Bun has crashed");
+
     // Should NOT crash with segfault (exit codes 128+N indicate death by signal N)
     expect(exitCode).not.toBe(134); // SIGABRT (128 + 6)
     expect(exitCode).not.toBe(139); // SIGSEGV (128 + 11)
-
-    // Should NOT contain crash indicators
-    expect(stderrText).not.toContain("Segmentation fault");
-    expect(stderrText).not.toContain("Bun has crashed");
-
-    // Should contain a normal error message about the missing package
-    expect(stderrText).toContain("this-package-does-not-exist-12345");
📝 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.

Suggested change
const stderrText = stderr?.toString("utf8") ?? "";
// Should NOT crash with segfault (exit codes 128+N indicate death by signal N)
expect(exitCode).not.toBe(134); // SIGABRT (128 + 6)
expect(exitCode).not.toBe(139); // SIGSEGV (128 + 11)
// Should NOT contain crash indicators
expect(stderrText).not.toContain("Segmentation fault");
expect(stderrText).not.toContain("Bun has crashed");
// Should contain a normal error message about the missing package
expect(stderrText).toContain("this-package-does-not-exist-12345");
const stderrText = stderr?.toString("utf8") ?? "";
// Should contain a normal error message about the missing package
expect(stderrText).toContain("this-package-does-not-exist-12345");
// Should NOT contain crash indicators
expect(stderrText).not.toContain("Segmentation fault");
expect(stderrText).not.toContain("Bun has crashed");
// Should NOT crash with segfault (exit codes 128+N indicate death by signal N)
expect(exitCode).not.toBe(134); // SIGABRT (128 + 6)
expect(exitCode).not.toBe(139); // SIGSEGV (128 + 11)
🤖 Prompt for AI Agents
In @test/cli/run/run-autoinstall.test.ts around lines 120 - 131, Reorder the
assertions so the stderr content checks run before the exit code checks: move
the lines that build stderrText and the expect(...).not.toContain /
expect(...).toContain assertions (the checks using stderrText for "Segmentation
fault", "Bun has crashed", and "this-package-does-not-exist-12345") to appear
prior to the expect(exitCode).not.toBe(...) assertions (the SIGABRT/SIGSEGV
checks). Keep the same variables (stderrText, exitCode) and messages unchanged
so failures show stderr diagnostics first.

Comment on lines +175 to +185
const stderrText = stderr?.toString("utf8") ?? "";

// Should NOT crash with segfault (exit codes 128+N indicate death by signal N)
expect(exitCode).not.toBe(134); // SIGABRT (128 + 6)
expect(exitCode).not.toBe(139); // SIGSEGV (128 + 11)

expect(stderrText).not.toContain("Segmentation fault");
expect(stderrText).not.toContain("Bun has crashed");

// Should contain a normal error message
expect(stderrText).toContain("another-nonexistent-pkg-67890");

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick | 🔵 Trivial

Same suggestion: reorder assertions for better diagnostics on failure.

Consistent with the first test, checking stderr content before exit codes provides more useful context if the test fails.

♻️ Suggested reordering
     const stderrText = stderr?.toString("utf8") ?? "";
 
+    // Should contain a normal error message
+    expect(stderrText).toContain("another-nonexistent-pkg-67890");
+
+    expect(stderrText).not.toContain("Segmentation fault");
+    expect(stderrText).not.toContain("Bun has crashed");
+
     // Should NOT crash with segfault (exit codes 128+N indicate death by signal N)
     expect(exitCode).not.toBe(134); // SIGABRT (128 + 6)
     expect(exitCode).not.toBe(139); // SIGSEGV (128 + 11)
-
-    expect(stderrText).not.toContain("Segmentation fault");
-    expect(stderrText).not.toContain("Bun has crashed");
-
-    // Should contain a normal error message
-    expect(stderrText).toContain("another-nonexistent-pkg-67890");
🤖 Prompt for AI Agents
In @test/cli/run/run-autoinstall.test.ts around lines 175 - 185, Reorder the
assertions so the stderr content checks run before the exit-code checks: first
assert stderrText contains "another-nonexistent-pkg-67890" and does not contain
"Segmentation fault" or "Bun has crashed" (using the stderrText variable), then
assert exitCode is not 134 or 139 (using exitCode); update the block around
stderrText and exitCode in run-autoinstall.test.ts to reflect this new order for
better failure diagnostics.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Fix all issues with AI agents
In @test/cli/run/run-autoinstall.test.ts:
- Around line 112-128: Reorder the assertions so the test asserts on stderrText
before checking exitCode to surface useful failure output first: move the
expect(stderrText).toContain("this-package-does-not-exist-12345") to precede the
exitCode assertions that check for SIGABRT/SIGSEGV (the variables exitCode and
stderrText and the Bun.spawnSync call are the anchors to change); keep the
stderrText computation and the exitCode assertions intact but placed after the
stderr assertion so test failures show stderr output first for diagnostics.
📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Disabled knowledge base sources:

  • Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between 24299ae and 81dfac2.

📒 Files selected for processing (1)
  • test/cli/run/run-autoinstall.test.ts
🧰 Additional context used
📓 Path-based instructions (3)
**/*.test.ts?(x)

📄 CodeRabbit inference engine (CLAUDE.md)

**/*.test.ts?(x): Never use bun test directly - always use bun bd test to run tests with debug build changes
For single-file tests, prefer -e flag over tempDir
For multi-file tests, prefer tempDir and Bun.spawn over single-file tests
Use normalizeBunSnapshot to normalize snapshot output of tests
Never write tests that check for 'panic', 'uncaught exception', or similar strings in test output
Use tempDir from harness to create temporary directories - do not use tmpdirSync or fs.mkdtempSync
When spawning processes in tests, expect stdout before expecting exit code for more useful error messages on test failure
Do not write flaky tests - do not use setTimeout in tests; instead await the condition to be met
Verify tests fail with USE_SYSTEM_BUN=1 bun test <file> and pass with bun bd test <file> - tests are invalid if they pass with USE_SYSTEM_BUN=1
Test files must end with .test.ts or .test.tsx
Avoid shell commands like find or grep in tests - use Bun's Glob and built-in tools instead

Files:

  • test/cli/run/run-autoinstall.test.ts
test/**/*.test.ts?(x)

📄 CodeRabbit inference engine (CLAUDE.md)

Always use port: 0 in tests - do not hardcode ports or use custom random port number functions

Files:

  • test/cli/run/run-autoinstall.test.ts
test/**/*.test.{ts,js,jsx,tsx,mjs,cjs}

📄 CodeRabbit inference engine (test/CLAUDE.md)

test/**/*.test.{ts,js,jsx,tsx,mjs,cjs}: Use bun bd test <...test file> to run tests with compiled code changes. Do not use bun test as it will not include your changes.
Use bun:test for files ending in *.test.{ts,js,jsx,tsx,mjs,cjs}. For test files without .test extension in test/js/node/test/{parallel,sequential}/*.js, use bun bd <file> instead of bun bd test <file> since they expect exit code 0.
Do not set a timeout on tests. Bun already has timeouts built-in.

Files:

  • test/cli/run/run-autoinstall.test.ts
🧠 Learnings (25)
📚 Learning: 2025-12-16T00:21:32.179Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-16T00:21:32.179Z
Learning: Applies to test/regression/issue/*.test.ts : Place regression tests for specific GitHub issues in `test/regression/issue/${issueNumber}.test.ts` with real issue numbers only

Applied to files:

  • test/cli/run/run-autoinstall.test.ts
📚 Learning: 2025-12-16T00:21:32.179Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-16T00:21:32.179Z
Learning: Applies to **/*.test.ts?(x) : Never write tests that check for 'panic', 'uncaught exception', or similar strings in test output

Applied to files:

  • test/cli/run/run-autoinstall.test.ts
📚 Learning: 2025-12-16T00:21:32.179Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-16T00:21:32.179Z
Learning: Applies to **/*.test.ts?(x) : Never use `bun test` directly - always use `bun bd test` to run tests with debug build changes

Applied to files:

  • test/cli/run/run-autoinstall.test.ts
📚 Learning: 2025-12-16T00:21:32.179Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-16T00:21:32.179Z
Learning: Applies to **/*.test.ts?(x) : Verify tests fail with `USE_SYSTEM_BUN=1 bun test <file>` and pass with `bun bd test <file>` - tests are invalid if they pass with USE_SYSTEM_BUN=1

Applied to files:

  • test/cli/run/run-autoinstall.test.ts
📚 Learning: 2025-12-16T00:21:32.179Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-16T00:21:32.179Z
Learning: Applies to **/*.test.ts?(x) : When spawning processes in tests, expect stdout before expecting exit code for more useful error messages on test failure

Applied to files:

  • test/cli/run/run-autoinstall.test.ts
📚 Learning: 2025-12-16T00:21:32.179Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-16T00:21:32.179Z
Learning: Applies to **/*.test.ts?(x) : Do not write flaky tests - do not use `setTimeout` in tests; instead await the condition to be met

Applied to files:

  • test/cli/run/run-autoinstall.test.ts
📚 Learning: 2026-01-05T23:04:01.518Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: test/CLAUDE.md:0-0
Timestamp: 2026-01-05T23:04:01.518Z
Learning: Organize regression tests for specific issues in `/test/regression/issue/${issueNumber}.test.ts`. Do not place regression tests in the regression directory if there is no associated issue number.

Applied to files:

  • test/cli/run/run-autoinstall.test.ts
📚 Learning: 2025-11-24T18:36:59.706Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: src/bun.js/bindings/v8/CLAUDE.md:0-0
Timestamp: 2025-11-24T18:36:59.706Z
Learning: Applies to src/bun.js/bindings/v8/test/v8/v8.test.ts : Add corresponding test cases to test/v8/v8.test.ts using checkSameOutput() function to compare Node.js and Bun output

Applied to files:

  • test/cli/run/run-autoinstall.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/cli/run/run-autoinstall.test.ts
📚 Learning: 2025-12-16T00:21:32.179Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-16T00:21:32.179Z
Learning: Applies to test/**/*.test.ts?(x) : Always use `port: 0` in tests - do not hardcode ports or use custom random port number functions

Applied to files:

  • test/cli/run/run-autoinstall.test.ts
📚 Learning: 2026-01-05T23:04:01.518Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: test/CLAUDE.md:0-0
Timestamp: 2026-01-05T23:04:01.518Z
Learning: Applies to test/**/*.test.{ts,js,jsx,tsx,mjs,cjs} : Use `bun:test` for files ending in `*.test.{ts,js,jsx,tsx,mjs,cjs}`. For test files without .test extension in test/js/node/test/{parallel,sequential}/*.js, use `bun bd <file>` instead of `bun bd test <file>` since they expect exit code 0.

Applied to files:

  • test/cli/run/run-autoinstall.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/run/run-autoinstall.test.ts
📚 Learning: 2025-09-20T00:58:38.042Z
Learnt from: markovejnovic
Repo: oven-sh/bun PR: 22568
File: test/js/valkey/valkey.test.ts:561-564
Timestamp: 2025-09-20T00:58:38.042Z
Learning: For test/js/valkey/valkey.test.ts, do not comment on synchronous throw assertions for async Redis methods (like ctx.redis.set(), ctx.redis.unsubscribe(), etc.) - Bun's Redis client implementation differs from Node.js and can throw synchronously even for async methods. The maintainer has explicitly requested to stop looking at this error pattern.

Applied to files:

  • test/cli/run/run-autoinstall.test.ts
📚 Learning: 2025-11-08T04:06:33.198Z
Learnt from: Jarred-Sumner
Repo: oven-sh/bun PR: 24491
File: test/js/bun/transpiler/declare-global.test.ts:17-17
Timestamp: 2025-11-08T04:06:33.198Z
Learning: In Bun test files, `await using` with Bun.spawn() is the preferred pattern for spawned processes regardless of whether they are short-lived or long-running. Do not suggest replacing `await using proc = Bun.spawn(...)` with `const proc = Bun.spawn(...); await proc.exited;`.

Applied to files:

  • test/cli/run/run-autoinstall.test.ts
📚 Learning: 2025-10-18T05:23:24.403Z
Learnt from: theshadow27
Repo: oven-sh/bun PR: 23798
File: test/js/bun/telemetry-server.test.ts:91-100
Timestamp: 2025-10-18T05:23:24.403Z
Learning: In the Bun codebase, telemetry tests (test/js/bun/telemetry-*.test.ts) should focus on telemetry API behavior: configure/disable/isEnabled, callback signatures and invocation, request ID correlation, and error handling. HTTP protocol behaviors like status code normalization (e.g., 200 with empty body → 204) should be tested in HTTP server tests (test/js/bun/http/), not in telemetry tests. Keep separation of concerns: telemetry tests verify the telemetry API contract; HTTP tests verify HTTP semantics.

Applied to files:

  • test/cli/run/run-autoinstall.test.ts
📚 Learning: 2025-09-20T00:57:56.685Z
Learnt from: markovejnovic
Repo: oven-sh/bun PR: 22568
File: test/js/valkey/valkey.test.ts:268-276
Timestamp: 2025-09-20T00:57:56.685Z
Learning: For test/js/valkey/valkey.test.ts, do not comment on synchronous throw assertions for async Redis methods like ctx.redis.set() - the maintainer has explicitly requested to stop looking at this error pattern.

Applied to files:

  • test/cli/run/run-autoinstall.test.ts
📚 Learning: 2026-01-05T23:04:01.518Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: test/CLAUDE.md:0-0
Timestamp: 2026-01-05T23:04:01.518Z
Learning: Always check exit codes and test error scenarios when spawning processes in tests.

Applied to files:

  • test/cli/run/run-autoinstall.test.ts
📚 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:

  • test/cli/run/run-autoinstall.test.ts
📚 Learning: 2025-09-07T08:20:47.215Z
Learnt from: RiskyMH
Repo: oven-sh/bun PR: 22258
File: src/cli/test_command.zig:1258-1281
Timestamp: 2025-09-07T08:20:47.215Z
Learning: For Bun's test line filtering feature, the parseFileLineArg function should only handle the specific cases of "file:line" and "file:line:col" formats. It should not try to be overly tolerant of other patterns, as components like ":col" or other non-numeric segments could legitimately be part of filenames. The current conservative approach that checks for numeric segments in expected positions is appropriate.

Applied to files:

  • test/cli/run/run-autoinstall.test.ts
📚 Learning: 2025-11-06T00:58:23.965Z
Learnt from: markovejnovic
Repo: oven-sh/bun PR: 24417
File: test/js/bun/spawn/spawn.test.ts:903-918
Timestamp: 2025-11-06T00:58:23.965Z
Learning: In Bun test files, `await using` with spawn() is appropriate for long-running processes that need guaranteed cleanup on scope exit or when explicitly testing disposal behavior. For short-lived processes that exit naturally (e.g., console.log scripts), the pattern `const proc = spawn(...); await proc.exited;` is standard and more common, as evidenced by 24 instances vs 4 `await using` instances in test/js/bun/spawn/spawn.test.ts.

Applied to files:

  • test/cli/run/run-autoinstall.test.ts
📚 Learning: 2026-01-05T23:04:01.518Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: test/CLAUDE.md:0-0
Timestamp: 2026-01-05T23:04:01.518Z
Learning: Applies to test/**/*-fixture.ts : Test files that spawn Bun processes should end in `*-fixture.ts` to identify them as test fixtures rather than tests themselves.

Applied to files:

  • test/cli/run/run-autoinstall.test.ts
📚 Learning: 2025-12-16T00:21:32.179Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-16T00:21:32.179Z
Learning: Applies to **/*.test.ts?(x) : For multi-file tests, prefer `tempDir` and `Bun.spawn` over single-file tests

Applied to files:

  • test/cli/run/run-autoinstall.test.ts
📚 Learning: 2025-11-24T18:37:11.466Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: src/js/CLAUDE.md:0-0
Timestamp: 2025-11-24T18:37:11.466Z
Learning: Applies to src/js/{builtins,node,bun,thirdparty,internal}/**/*.{ts,js} : Use `$debug()` for debug logging and `$assert()` for assertions; both are stripped in release builds

Applied to files:

  • test/cli/run/run-autoinstall.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/cli/run/run-autoinstall.test.ts
📚 Learning: 2025-10-18T23:43:42.502Z
Learnt from: Jarred-Sumner
Repo: oven-sh/bun PR: 23817
File: src/js/node/test.ts:282-282
Timestamp: 2025-10-18T23:43:42.502Z
Learning: In the Bun repository, the error code generation script (generate-node-errors.ts) always runs during the build process. When reviewing code that uses error code intrinsics like $ERR_TEST_FAILURE, $ERR_INVALID_ARG_TYPE, etc., do not ask to verify whether the generation script has been run or will run, as it is automatically executed.

Applied to files:

  • test/cli/run/run-autoinstall.test.ts
🧬 Code graph analysis (1)
test/cli/run/run-autoinstall.test.ts (1)
test/harness.ts (1)
  • tmpdirSync (1221-1223)
🔇 Additional comments (2)
test/cli/run/run-autoinstall.test.ts (2)

87-91: LGTM! Clear regression test documentation.

The comment provides good context about the crash cause (allocator invalidation), which will help future maintainers understand the purpose of these tests.


130-179: LGTM! Good coverage for the transpilation code path.

This test correctly targets the TypeScript transpilation scenario mentioned in the PR objectives, where the allocator mismatch was occurring. The same suggestion about asserting stderr before exit code applies here for consistency.

Comment on lines +112 to +128
const { exitCode, stderr } = Bun.spawnSync({
cmd: [bunExe(), join(dir, "index.js")],
cwd: dir,
env: bunEnv,
stdout: "pipe",
stderr: "pipe",
});

const stderrText = stderr?.toString("utf8") ?? "";

// Should NOT crash with segfault (exit codes 128+N indicate death by signal N)
expect(exitCode).not.toBe(134); // SIGABRT (128 + 6)
expect(exitCode).not.toBe(139); // SIGSEGV (128 + 11)

// Should contain a normal error message about the missing package
expect(stderrText).toContain("this-package-does-not-exist-12345");
});

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick | 🔵 Trivial

Consider checking stderr before exit code for better failure diagnostics.

Based on coding guidelines, expecting output before exit code provides more useful error messages on test failure. If the process crashes, seeing what stderr contained helps diagnose the issue.

💡 Suggested reorder
     const { exitCode, stderr } = Bun.spawnSync({
       cmd: [bunExe(), join(dir, "index.js")],
       cwd: dir,
       env: bunEnv,
       stdout: "pipe",
       stderr: "pipe",
     });

     const stderrText = stderr?.toString("utf8") ?? "";

-    // Should NOT crash with segfault (exit codes 128+N indicate death by signal N)
-    expect(exitCode).not.toBe(134); // SIGABRT (128 + 6)
-    expect(exitCode).not.toBe(139); // SIGSEGV (128 + 11)
-
     // Should contain a normal error message about the missing package
     expect(stderrText).toContain("this-package-does-not-exist-12345");
+
+    // Should NOT crash with segfault (exit codes 128+N indicate death by signal N)
+    expect(exitCode).not.toBe(134); // SIGABRT (128 + 6)
+    expect(exitCode).not.toBe(139); // SIGSEGV (128 + 11)
🤖 Prompt for AI Agents
In @test/cli/run/run-autoinstall.test.ts around lines 112 - 128, Reorder the
assertions so the test asserts on stderrText before checking exitCode to surface
useful failure output first: move the
expect(stderrText).toContain("this-package-does-not-exist-12345") to precede the
exitCode assertions that check for SIGABRT/SIGSEGV (the variables exitCode and
stderrText and the Bun.spawnSync call are the anchors to change); keep the
stderrText computation and the exitCode assertions intact but placed after the
stderr assertion so test failures show stderr output first for diagnostics.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant