fix(install): hoist highest version in isolated installs #26141
Conversation
Previously, the hoisted version in `.bun/node_modules/` was determined by alphabetical workspace ordering - whichever workspace came first alphabetically would have its version of a shared dependency hoisted. This caused issues in monorepos where different workspaces depend on different versions of the same package (e.g., @types/react@18 vs @types/react@19). Now, when multiple versions of a package are encountered, the highest version (by semver) is hoisted instead of the first-seen version. This ensures consistent behavior regardless of workspace naming. Fixes #26140 Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
|
Updated 1:19 PM PT - Jan 16th, 2026
❌ @autofix-ci[bot], your commit 94691cb has 2 failures in
🧪 To try this PR locally: bunx bun-pr 26141That installs a local version of the PR into your bun-26141 --bun |
WalkthroughImplements version-aware hoisting by introducing HoistInfo and changing hoist storage to track entry and package IDs; updates hoisting logic to prefer higher npm semantic versions when deciding which entry to hoist. Adds regression tests verifying highest-version hoisting in workspace monorepos. Minor import reorder in Request.zig. Changes
Suggested reviewers
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@test/regression/issue/26140.test.ts`:
- Around line 122-126: Remove the redundant stderr content assertions and only
assert the process exit code: delete the two lines with
expect(stderr).not.toContain("panic:") and
expect(stderr).not.toContain("error:") in the test that awaits
Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); keep the
await of proc outputs if needed and retain the expect(exitCode).toBe(0)
assertion.
- Around line 57-61: Remove the unused stdout capture and the fragile stderr
string assertions: instead of awaiting Promise.all([proc.stdout.text(),
proc.stderr.text(), proc.exited]) and asserting on stdout/stderr contents, await
only proc.exited to get exitCode (e.g., const exitCode = await proc.exited) and
keep only expect(exitCode).toBe(0); remove the variables stdout and stderr and
the lines expect(stderr).not.toContain("panic:") and
expect(stderr).not.toContain("error:").
📜 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.
📒 Files selected for processing (2)
src/install/isolated_install.zigtest/regression/issue/26140.test.ts
🧰 Additional context used
📓 Path-based instructions (3)
test/**/*.test.{ts,js,jsx,tsx,mjs,cjs}
📄 CodeRabbit inference engine (test/CLAUDE.md)
test/**/*.test.{ts,js,jsx,tsx,mjs,cjs}: Usebun bd test <...test file>to run tests with compiled code changes. Do not usebun testas it will not include your changes.
Usebun:testfor files ending in*.test.{ts,js,jsx,tsx,mjs,cjs}. For test files without .test extension in test/js/node/test/{parallel,sequential}/*.js, usebun bd <file>instead ofbun bd test <file>since they expect exit code 0.
Do not set a timeout on tests. Bun already has timeouts built-in.
Files:
test/regression/issue/26140.test.ts
test/**/*.test.ts
📄 CodeRabbit inference engine (CLAUDE.md)
test/**/*.test.ts: Use Bun's Jest-compatible test runner with proper test fixtures and imports fromharness
Always useport: 0when binding to ports in tests - never hardcode ports or use custom random port functions
UsenormalizeBunSnapshotto normalize snapshot output in tests
Never write tests that check for 'panic', 'uncaught exception', or similar strings in test output as these will never fail in CI
UsetempDirfromharnessto create temporary directories in tests - do not usetmpdirSyncorfs.mkdtempSync
In spawned process tests, use expect(stdout).toBe(...) BEFORE expect(exitCode).toBe(0) for more useful error messages
Do not usesetTimeoutin tests - await the condition to be met instead, as you are testing the CONDITION not TIME PASSING
Files:
test/regression/issue/26140.test.ts
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@importstatements 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/install/isolated_install.zig
🧠 Learnings (13)
📚 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/regression/issue/26140.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/regression/issue/26140.test.ts
📚 Learning: 2026-01-15T03:22:50.711Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-15T03:22:50.711Z
Learning: For GitHub Issue-specific tests, place the test file in `test/regression/issue/${issueNumber}.test.ts` with a REAL issue number
Applied to files:
test/regression/issue/26140.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/regression/issue/26140.test.ts
📚 Learning: 2026-01-15T03:22:50.711Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-15T03:22:50.711Z
Learning: Applies to test/**/*.test.ts : Use Bun's Jest-compatible test runner with proper test fixtures and imports from `harness`
Applied to files:
test/regression/issue/26140.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/regression/issue/26140.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/regression/issue/26140.test.ts
📚 Learning: 2026-01-15T03:22:50.711Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-15T03:22:50.711Z
Learning: Applies to test/**/*.test.ts : Use `normalizeBunSnapshot` to normalize snapshot output in tests
Applied to files:
test/regression/issue/26140.test.ts
📚 Learning: 2025-11-20T19:51:32.288Z
Learnt from: markovejnovic
Repo: oven-sh/bun PR: 24880
File: packages/bun-vscode/package.json:382-385
Timestamp: 2025-11-20T19:51:32.288Z
Learning: In the Bun repository, dependencies may be explicitly added to package.json files (even when not directly imported in code) to force version upgrades on transitive dependencies, particularly as part of Aikido security scanner remediation to ensure vulnerable transitive dependencies resolve to patched versions.
Applied to files:
test/regression/issue/26140.test.ts
📚 Learning: 2026-01-14T21:08:10.438Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: test/js/node/test/parallel/CLAUDE.md:0-0
Timestamp: 2026-01-14T21:08:10.438Z
Learning: These are Node.js compatibility tests not written by Bun and cannot be modified
Applied to files:
test/regression/issue/26140.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/regression/issue/26140.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: When spawning Bun processes in tests, use `bunExe` and `bunEnv` from `harness` to ensure the same build of Bun is used and debug logging is silenced.
Applied to files:
test/regression/issue/26140.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/install/isolated_install.zig
🧬 Code graph analysis (1)
test/regression/issue/26140.test.ts (1)
test/harness.ts (1)
tempDir(277-284)
🔇 Additional comments (4)
test/regression/issue/26140.test.ts (1)
15-77: Well-structured regression test with good coverage.The test correctly validates the fix by:
- Setting up a monorepo with workspaces having different
is-numberversions- Verifying the higher version (7.0.0) is hoisted regardless of alphabetical order
- Confirming both versions are installed in their respective locations
The use of
usingfor automatic temp directory cleanup and the clear test documentation are appreciated. Based on learnings, this test is correctly placed intest/regression/issue/${issueNumber}.test.ts.src/install/isolated_install.zig (3)
423-428: Good introduction ofHoistInfostruct for tracking hoisted entries.The struct appropriately captures both the entry ID and package ID needed for version comparison and potential un-hoisting. The change from
voidtoHoistInfoin the hash map is a clean way to track the necessary metadata.
541-576: Correct implementation of highest-version hoisting logic.The logic properly:
- Hoists first-seen packages immediately
- Compares npm versions when a package name is encountered again
- Un-hoists the previous entry and hoists the new one when the new version is higher
- Preserves first-seen behavior for non-npm resolution types (git, tarball, etc.)
The use of
new_version.order(existing_version, string_buf, string_buf) == .gtcorrectly determines if the new version is greater.
523-524: Good placement ofnew_entry_idcalculation.Moving the
new_entry_idcalculation before the hoisting logic ensures it's available for storing inHoistInfowhen the entry is hoisted. This is necessary since the store hasn't been appended to yet at this point.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
Remove unused stdout capture and fragile stderr string assertions. Only check exit code for install success. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
|
This is the cause of the issue. Fix this behavior difference |
|
@claude Concise Summary pnpm: Processes workspace packages in filesystem path order (packages/aaa before packages/bbb). First workspace to declare a dependency name wins the hoisted slot. Bun bug: Sorts by package.json name instead of filesystem path. Fix: Sort workspace packages by their importer/filesystem path to match pnpm. |
Summary
@types/*packages in.bun/node_modules/being resolved based on alphabetical workspace package name orderingTest plan
test/regression/issue/26140.test.tsbun bd test test/regression/issue/26140.test.ts)Fixes #26140
🤖 Generated with Claude Code